[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15856423#comment-15856423 ] ASF subversion and git services commented on KNOX-861: -- Commit 2bdc703945a3c0645f39dbb35f7165308a8cff80 in knox's branch refs/heads/master from [~moresandeep] [ https://git-wip-us.apache.org/repos/asf?p=knox.git;h=2bdc703 ] KNOX-861 - Support for pluggable validator for Header pre authentication provider (Mohammad Kamrul Islam via Sandeep More) > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.3.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15854811#comment-15854811 ] Sandeep More commented on KNOX-861: --- [~kamrul] Do you have instructions on how to test custom validators ? > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.3.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15854265#comment-15854265 ] Sandeep More commented on KNOX-861: --- [~kamrul] Thanks for the patch ! I will try to check it in today. Thanks for the two bugs. About Documentation you can find information here https://cwiki.apache.org/confluence/display/KNOX/Site+Maintenance If you run into issues let me know we can either talk it out on the JIRA or mailing list. Thanks again ! > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.3.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15853628#comment-15853628 ] Mohammad Kamrul Islam commented on KNOX-861: [~moresandeep] : uploaded a new patch. Two proposals: Both proposals are very relevant ant important. Created JIRA and assigned it to me -- working on it soon. 1. Support multiple validators in KNOX-869 2. Documentation will be added in KNOX-870. Question: How to add documentation in Knox? Some help will be really appreciated. > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.3.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15851955#comment-15851955 ] Sandeep More commented on KNOX-861: --- Looking at the code it looks like only one validator can be used as of now, it might be be a good enhancement if we can use multiple validators. for e.g. PreAuthService.getValidator() could return List then AbstractPreAuthFederationFilter.isValid() method can iterate over all validators and amke sure they are valid. Just a thought ! > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15851796#comment-15851796 ] Sandeep More commented on KNOX-861: --- Also, it would be awesome if you could provide some steps on how to go about testing this feature and some documentation about this new feature ! > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15851790#comment-15851790 ] Sandeep More commented on KNOX-861: --- Thanks for the quick turnaround [~kamrul] really appreciate it ! There are few minor comments: 1) DefaultValidator 1.1) Nitpick - add @sine tag 1.2) Nitpick - Typo in comments, perfromed -> performed, get rid of the '+' symbols in the comments 2) HeaderPreAuthFederationFilterTest 2.1) Change 'MyValidator' class name to something like DummyValidator or TestValidator - basically avoid using words like 'My' 3) IPValidatorTest 3.1) The test testNotInitialized() passes because httpRequest.getRemoteAddr() is null (because of Mockito) in real world there will always be a value for httpRequest.getRemoteAddr(). This is because the test hits a case which it should not in IpAddressValidator.java #76 > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15850779#comment-15850779 ] Mohammad Kamrul Islam commented on KNOX-861: Thanks [~moresandeep] for your constructive and organized comments! I accepted most of your comments. One thing: > 3.3) getValidatorMap() should be removed or be private, the validatorMap is > exposed via a public static method where it could accidentally be modified, Actually I wanted to open it for unit test purpose only. However, I now made it unmodifiable. > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.2.patch, KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15850360#comment-15850360 ] Sandeep More commented on KNOX-861: --- Hello [~kamrul] Thanks for the path again, it looks good, following are some of my initial thoughts on the patch, I did not check the UnitTests yet. 1) pom.xml (gateway-provider-security-preauth) - All version should be defined in the main project POM 1.1) Remove version for mockito-core 1.2) Same as above, remove gateway-server version 2) AbstractPreauthFederationFilter 2.1) filterConfig variable is protected should be private 2.2) we should be importing * i.e. java.util.*, insted import only the required classes 3) PreAuthUtils 3.1) I would recommend calling this class something like 'PreAuthService' since it deals with loading of various Impls from classpath. 3.2) it would be good to have validatorMap as a ConcurrentHashMap rather than just a HashMap 3.3) getValidatorMap() should be removed or be private, the validatorMap is exposed via a public static method where it could accidentally be modified, 3.4) initializeValidators() should be private 3.5) A nitpick - add @Since 0.12 tag 4) PreAuthValidator 4.1) Is there a reason why we change to Abstract class from Interface because making it abstract will limit the extensibility. 5) PreAuthFederationFilter 5.1) Remove unused imports and variables (I think 5 imports and 3 variables. 5.2) public PreAuthValidator getValidator() - exposes the internal private validator via public method. Again, thanks for picking this up ! > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[ https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15847854#comment-15847854 ] Larry McCay commented on KNOX-861: -- Hi [~kamrul] - this looks pretty good at first glance. I will need to test it in the next day or so and review in greater detail. This will be a really nice improvement for establishing trust in various ways before accepting the asserted identity from the header. > Support pluggable validator for Header pre authentication provider > --- > > Key: KNOX-861 > URL: https://issues.apache.org/jira/browse/KNOX-861 > Project: Apache Knox > Issue Type: Improvement > Components: Server >Reporter: Mohammad Kamrul Islam >Assignee: Mohammad Kamrul Islam > Fix For: 0.12.0 > > Attachments: KNOX-861.patch > > > Currently there are two validators : IPValidator, DefaultValidator, > IPValidator validates the source IP. DefaultValidator is basically a > no-auth. > The proposal is to provide a pluggable validator to support any custom > validation logic. Another requirement is to maintain the backward > compatibility of any existing features. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (KNOX-861) Support pluggable validator for Header pre authentication provider
[
https://issues.apache.org/jira/browse/KNOX-861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15833549#comment-15833549
]
Larry McCay commented on KNOX-861:
--
[~kamrul] - this sounds like a great improvement.
A couple tips for making this a purely custom mechanism for circumstances where
the validator cannot be contributed to the community - which I think is
probably important here.
1. Jars containing the validator would be added to the {GATEWAY_HOME}/ext
directory which will put them on the classpath
2. The PreAuthValidator interface could be used to discover validator impls
from the classpath using the java ServiceLoader mechanism
3. The configured name within the topology would then be used to disambiguate
the implementation to use - since there will be more than one
4. The existing IpValidator and DefaultValidator should be added to the same
ServiceLoader mechanism as well - just add a file to the
resources/META_INF/services directory for that interface that has those two
classnames in it - just as exists for the contributor interface there already
> Support pluggable validator for Header pre authentication provider
> ---
>
> Key: KNOX-861
> URL: https://issues.apache.org/jira/browse/KNOX-861
> Project: Apache Knox
> Issue Type: Improvement
> Components: Server
>Reporter: Mohammad Kamrul Islam
>Assignee: Mohammad Kamrul Islam
> Fix For: 0.12.0
>
>
> Currently there are two validators : IPValidator, DefaultValidator,
> IPValidator validates the source IP. DefaultValidator is basically a
> no-auth.
> The proposal is to provide a pluggable validator to support any custom
> validation logic. Another requirement is to maintain the backward
> compatibility of any existing features.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
