[PATCH] 0027..0029 support external authorization LDAP server
These patches resolve https://fedorahosted.org/pki/ticket/1174 Regards, Fraser
Looks good in general. I notice that your patch adds the use of the Vector class. Vector is old and synchronized - which can slow things down unnecessarily. Use ArrayList or similar instead. Ade On Wed, 2015-03-11 at 19:05 +1000, Fraser Tweedale wrote:
These patches resolve https://fedorahosted.org/pki/ticket/1174
Regards, Fraser _______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
On Wed, Mar 11, 2015 at 02:04:56PM -0400, Ade Lee wrote:
Looks good in general.
I notice that your patch adds the use of the Vector class. Vector is old and synchronized - which can slow things down unnecessarily. Use ArrayList or similar instead.
Ade
Roger that; I will switch to ArrayList. For now you can all s/Vector/ArrayList/g in your heads while you review this patch :) Thanks, Fraser
On Wed, 2015-03-11 at 19:05 +1000, Fraser Tweedale wrote:
These patches resolve https://fedorahosted.org/pki/ticket/1174
Regards, Fraser _______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel
On 3/12/2015 8:12 AM, Fraser Tweedale wrote:
On Wed, Mar 11, 2015 at 02:04:56PM -0400, Ade Lee wrote:
Looks good in general.
I notice that your patch adds the use of the Vector class. Vector is old and synchronized - which can slow things down unnecessarily. Use ArrayList or similar instead.
Ade
Roger that; I will switch to ArrayList. For now you can all s/Vector/ArrayList/g in your heads while you review this patch :)
Quick comments: 1. The TOKEN_GROUPS probably should be a List<String> to simplify the creation and the usage of the list of groups. 2. I'm not quite clear the purpose of this enhancement. If it's meant to be a general-purpose directory-based authentication plugin, it would make sense to have a fully configurable parameters for retrieving the group information. However, if this is only to be used for Dogtag authentication, there's already a user-group subsystem that can provide the information. See PKIRealm.getRoles(). I may have more comments later. -- Endi S. Dewata
New patch attached (squashed some bugs and cleaned up the implementation considerably ; squashed back down into one commit). Other comments inline below. On Thu, Mar 12, 2015 at 09:40:20AM +0700, Endi Sukma Dewata wrote:
On 3/12/2015 8:12 AM, Fraser Tweedale wrote:
On Wed, Mar 11, 2015 at 02:04:56PM -0400, Ade Lee wrote:
Looks good in general.
I notice that your patch adds the use of the Vector class. Vector is old and synchronized - which can slow things down unnecessarily. Use ArrayList or similar instead.
Ade
Roger that; I will switch to ArrayList. For now you can all s/Vector/ArrayList/g in your heads while you review this patch :)
Quick comments:
1. The TOKEN_GROUPS probably should be a List<String> to simplify the creation and the usage of the list of groups.
We are constrained by the IAuthToken interface, which has only the String[] method.
2. I'm not quite clear the purpose of this enhancement. If it's meant to be a general-purpose directory-based authentication plugin, it would make sense to have a fully configurable parameters for retrieving the group information. However, if this is only to be used for Dogtag authentication, there's already a user-group subsystem that can provide the information. See PKIRealm.getRoles().
It is apparently needed for retrieving groups from external directories. https://fedorahosted.org/pki/ticket/1174 Cheers, Fraser
I may have more comments later.
-- Endi S. Dewata
On 3/12/2015 3:18 PM, Fraser Tweedale wrote:
New patch attached (squashed some bugs and cleaned up the implementation considerably ; squashed back down into one commit).
Other comments inline below.
1. The TOKEN_GROUPS probably should be a List<String> to simplify the creation and the usage of the list of groups.
We are constrained by the IAuthToken interface, which has only the String[] method.
In AuthToken the attributes are actually stored in a Hashtable<String, Object>, and there's already a generic get() that returns Object, so we probably can just add a set(String name, Object value). If the list of groups is only kept in memory I don't see a reason to concatenate them into a single string.
2. I'm not quite clear the purpose of this enhancement. If it's meant to be a general-purpose directory-based authentication plugin, it would make sense to have a fully configurable parameters for retrieving the group information. However, if this is only to be used for Dogtag authentication, there's already a user-group subsystem that can provide the information. See PKIRealm.getRoles().
It is apparently needed for retrieving groups from external directories. https://fedorahosted.org/pki/ticket/1174
OK. 3. The listGroups() is always called with "*" filter so it will pull all groups in the database, then the code will execute a search operation for each group to check if the user is a member. I think this is highly inefficient. The listGroups() should have been called with the right filter to get the right list of groups with a single search operation. 4. When constructing a DN the attribute values should be escaped with LDAPUtil.escapeRDNValue() to prevent problems with DN special characters. 5. When constructing LDAP filter the attribute values should be escaped with LDAPUtil.escapeFilter() to prevent problems with LDAP filter special characters. 6. If there's an exception the method should rethrow the exception so the client will see the problem. 7. The array of groups can be added into a list with a single operation: groups.addAll(Arrays.asList(groupsArray)); 8. The Utils.stripQuotes() right now is called multiple times for the same value. It can be simplified as follows: matched = groups.contains(Utils.stripQuotes(value)); -- Endi S. Dewata
Sorry, revising my comments based on the latest patches. On 3/12/2015 10:50 PM, Endi Sukma Dewata wrote:
1. The TOKEN_GROUPS probably should be a List<String> to simplify the creation and the usage of the list of groups.
We are constrained by the IAuthToken interface, which has only the String[] method.
In AuthToken the attributes are actually stored in a Hashtable<String, Object>, and there's already a generic get() that returns Object, so we probably can just add a set(String name, Object value). If the list of groups is only kept in memory I don't see a reason to concatenate them into a single string.
Scratch the last sentence, but I think the List<String> is still better because you won't need to convert into array of Strings.
3. The listGroups() is always called with "*" filter so it will pull all groups in the database, then the code will execute a search operation for each group to check if the user is a member. I think this is highly inefficient. The listGroups() should have been called with the right filter to get the right list of groups with a single search operation.
Never mind. The new patches already addressed this issue. The comments below are still valid.
4. When constructing a DN the attribute values should be escaped with LDAPUtil.escapeRDNValue() to prevent problems with DN special characters.
5. When constructing LDAP filter the attribute values should be escaped with LDAPUtil.escapeFilter() to prevent problems with LDAP filter special characters.
6. If there's an exception the method should rethrow the exception so the client will see the problem.
7. The array of groups can be added into a list with a single operation:
groups.addAll(Arrays.asList(groupsArray));
8. The Utils.stripQuotes() right now is called multiple times for the same value. It can be simplified as follows:
matched = groups.contains(Utils.stripQuotes(value));
One more thing: 9. I see the "gid" attribute being set in TokenAuthentication. Probably it should be changed to IAuthToken.GID so we know where it's set and used. Alternatively we can replace it with IAuthToken.GROUPS that contains just a single group, and remove IAuthToken.GID. -- Endi S. Dewata
Endi, thanks for reviewing. New patch attached; additional comments inline. On Thu, Mar 12, 2015 at 11:35:18PM +0700, Endi Sukma Dewata wrote:
Sorry, revising my comments based on the latest patches.
On 3/12/2015 10:50 PM, Endi Sukma Dewata wrote:
1. The TOKEN_GROUPS probably should be a List<String> to simplify the creation and the usage of the list of groups.
We are constrained by the IAuthToken interface, which has only the String[] method.
In AuthToken the attributes are actually stored in a Hashtable<String, Object>, and there's already a generic get() that returns Object, so we probably can just add a set(String name, Object value). If the list of groups is only kept in memory I don't see a reason to concatenate them into a single string.
Scratch the last sentence, but I think the List<String> is still better because you won't need to convert into array of Strings.
I have left it as String[] for now. If we want List<String> we can add additional methods to the interface rather than casting to/from Object all over the place.
3. The listGroups() is always called with "*" filter so it will pull all groups in the database, then the code will execute a search operation for each group to check if the user is a member. I think this is highly inefficient. The listGroups() should have been called with the right filter to get the right list of groups with a single search operation.
Never mind. The new patches already addressed this issue. The comments below are still valid.
4. When constructing a DN the attribute values should be escaped with LDAPUtil.escapeRDNValue() to prevent problems with DN special characters.
5. When constructing LDAP filter the attribute values should be escaped with LDAPUtil.escapeFilter() to prevent problems with LDAP filter special characters.
I have escaped end-user-supplied values, but not config values.
6. If there's an exception the method should rethrow the exception so the client will see the problem.
No more squashing of exceptions.
7. The array of groups can be added into a list with a single operation:
groups.addAll(Arrays.asList(groupsArray));
Superseded by item 9.
8. The Utils.stripQuotes() right now is called multiple times for the same value. It can be simplified as follows:
matched = groups.contains(Utils.stripQuotes(value));
Done.
One more thing:
9. I see the "gid" attribute being set in TokenAuthentication. Probably it should be changed to IAuthToken.GID so we know where it's set and used. Alternatively we can replace it with IAuthToken.GROUPS that contains just a single group, and remove IAuthToken.GID.
Good suggestion; implemented.
-- Endi S. Dewata
On 3/13/2015 2:00 PM, Fraser Tweedale wrote:
4. When constructing a DN the attribute values should be escaped with LDAPUtil.escapeRDNValue() to prevent problems with DN special characters.
5. When constructing LDAP filter the attribute values should be escaped with LDAPUtil.escapeFilter() to prevent problems with LDAP filter special characters.
I have escaped end-user-supplied values, but not config values.
Do you mean these lines? 1. return mGroups + "," + mGroupsBaseDN; 2. "(&(objectclass=" + mGroupObjectClass + ")(" + filter + "))", 3. filter = k + "=" + userdn; I think line #1 is fine because the values in those variables are already in the DN format, not because they are config values. Line #2 is fine too because the mGroupObjectClass is probably filter-safe due to object class naming restrictions. For line #3 I think the userdn still needs to be escaped. DNs and filters have different sets of special characters (see LDAPUtil.java) so a DN may not necessarily be filter-safe regardless how the value was obtained.
9. I see the "gid" attribute being set in TokenAuthentication. Probably it should be changed to IAuthToken.GID so we know where it's set and used. Alternatively we can replace it with IAuthToken.GROUPS that contains just a single group, and remove IAuthToken.GID.
Good suggestion; implemented.
Just one more thing, the "no gid in authToken" message probably should be replaced with "no groups in authToken". Once these are addressed, ACK. Thanks! -- Endi S. Dewata
On Fri, Mar 13, 2015 at 08:10:11PM +0700, Endi Sukma Dewata wrote:
On 3/13/2015 2:00 PM, Fraser Tweedale wrote:
4. When constructing a DN the attribute values should be escaped with LDAPUtil.escapeRDNValue() to prevent problems with DN special characters.
5. When constructing LDAP filter the attribute values should be escaped with LDAPUtil.escapeFilter() to prevent problems with LDAP filter special characters.
I have escaped end-user-supplied values, but not config values.
Do you mean these lines? 1. return mGroups + "," + mGroupsBaseDN; 2. "(&(objectclass=" + mGroupObjectClass + ")(" + filter + "))", 3. filter = k + "=" + userdn;
I think line #1 is fine because the values in those variables are already in the DN format, not because they are config values.
Line #2 is fine too because the mGroupObjectClass is probably filter-safe due to object class naming restrictions.
For line #3 I think the userdn still needs to be escaped. DNs and filters have different sets of special characters (see LDAPUtil.java) so a DN may not necessarily be filter-safe regardless how the value was obtained.
9. I see the "gid" attribute being set in TokenAuthentication. Probably it should be changed to IAuthToken.GID so we know where it's set and used. Alternatively we can replace it with IAuthToken.GROUPS that contains just a single group, and remove IAuthToken.GID.
Good suggestion; implemented.
Just one more thing, the "no gid in authToken" message probably should be replaced with "no groups in authToken".
Once these are addressed, ACK. Thanks!
Thanks Endi, amended patch pushed to master (f98e599). Cheers, Fraser
participants (3)
-
Ade Lee -
Endi Sukma Dewata -
Fraser Tweedale