Shiro
  1. Shiro
  2. SHIRO-127

Improvements to Shiro's LDAP support

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Realms
    • Labels:
      None

      Description

      1. shiro-127.patch
        30 kB
        Philippe Laflamme

        Issue Links

          Activity

          Hide
          Philippe Laflamme added a comment -

          The attached path improves Shiro's LDAP support.

          The patch contains:

          • a DefaultLdapRealm class
          • modifications to move connection settings to the LdapContextFactory
          • modifications to move querying/schema settings to the DefaultLdapRealm
          • modifications to the ActiveDirectoryRealm to keep previous functionalities

          The new realm supports extracting groups from the directory in a simplistic way. The groups are returned as user roles.

          I've created an example use of the realm. It is available here: http://svn.obiba.org/sandbox/plaflamm/shiro-ldap

          To use:
          svn co http://svn.obiba.org/sandbox/plaflamm/shiro-ldap
          cd shiro-ldap
          mvn test

          Show
          Philippe Laflamme added a comment - The attached path improves Shiro's LDAP support. The patch contains: a DefaultLdapRealm class modifications to move connection settings to the LdapContextFactory modifications to move querying/schema settings to the DefaultLdapRealm modifications to the ActiveDirectoryRealm to keep previous functionalities The new realm supports extracting groups from the directory in a simplistic way. The groups are returned as user roles. I've created an example use of the realm. It is available here: http://svn.obiba.org/sandbox/plaflamm/shiro-ldap To use: svn co http://svn.obiba.org/sandbox/plaflamm/shiro-ldap cd shiro-ldap mvn test
          Hide
          Ravi Ellappan added a comment -

          Patch is not working !

          patching file core/src/main/java/org/apache/shiro/realm/ldap/AbstractLdapRealm.java
          Hunk #3 FAILED at 111.
          1 out of 3 hunks FAILED – saving rejects to file core/src/main/java/org/apache/shiro/realm/ldap/AbstractLdapRealm.java.rej

          Show
          Ravi Ellappan added a comment - Patch is not working ! patching file core/src/main/java/org/apache/shiro/realm/ldap/AbstractLdapRealm.java Hunk #3 FAILED at 111. 1 out of 3 hunks FAILED – saving rejects to file core/src/main/java/org/apache/shiro/realm/ldap/AbstractLdapRealm.java.rej
          Hide
          Emmanuel Lecharny added a comment -

          Checking the code at ActiveDirectoryRealm.java, it's very likely that you'll have a lot of troubles if you don't take care of some few points :

          • code like (line 190) :
            ...
            if (attr.getID().equals("memberOf")) {
            ...
            won't work if the attr stores the ID in uper case, or all in lower case. attr.getID() may return one of those values :
          • memberof
          • MEMBEROF
          • MemberOf
          • MeMbErOf
          • the OID (I don't know what is the memberOf's OID, but be ready to deal with things like 2.5.4.11...)
          • when creating a NamingEnumeration, always close it, otherwise you'll get some nasty errors (like very long delay if you are using a Ldap connection pool). Good luck to find the origin of those delays if you don't know that NE must be closed ...

          so
          try

          { <use a NamingEnumeration> }

          finally

          { <close the NamingEnumeration> }
          • filters like (line 171) :
            String searchFilter = "(&(objectClass=*)(userPrincipalName= {0}))";
            is strictly equivalent to
            String searchFilter = "(userPrincipalName={0}

            )";

          Of course, check that the userPrincipalName is indexed, otherwise a search using this filter will do a full scan...

          • be very careful when manipulation a DN. For the same reason than in point #1, a DN may have multiple forms. Using the DN as a String is likely to carry some strange errors too (like you don't find a match with a given DN). Comparing 2 DNs is not something simple, you won't be able to whip it in less than one week, trust me on that.

          If you want a way to identify an entry by an invariant value, don't use the DN, use the entryUUID attribute. It's unique, it's invariant, it's case non sensitive.

          Show
          Emmanuel Lecharny added a comment - Checking the code at ActiveDirectoryRealm.java, it's very likely that you'll have a lot of troubles if you don't take care of some few points : code like (line 190) : ... if (attr.getID().equals("memberOf")) { ... won't work if the attr stores the ID in uper case, or all in lower case. attr.getID() may return one of those values : memberof MEMBEROF MemberOf MeMbErOf the OID (I don't know what is the memberOf's OID, but be ready to deal with things like 2.5.4.11...) when creating a NamingEnumeration, always close it, otherwise you'll get some nasty errors (like very long delay if you are using a Ldap connection pool). Good luck to find the origin of those delays if you don't know that NE must be closed ... so try { <use a NamingEnumeration> } finally { <close the NamingEnumeration> } filters like (line 171) : String searchFilter = "(&(objectClass=*)(userPrincipalName= {0}))"; is strictly equivalent to String searchFilter = "(userPrincipalName={0} )"; Of course, check that the userPrincipalName is indexed, otherwise a search using this filter will do a full scan... be very careful when manipulation a DN. For the same reason than in point #1, a DN may have multiple forms. Using the DN as a String is likely to carry some strange errors too (like you don't find a match with a given DN). Comparing 2 DNs is not something simple, you won't be able to whip it in less than one week, trust me on that. If you want a way to identify an entry by an invariant value, don't use the DN, use the entryUUID attribute. It's unique, it's invariant, it's case non sensitive.
          Hide
          Les Hazlewood added a comment -

          As luck would have it, I'm working on a default LDAP implementation as we speak. It is not based on the patch, although I will be looking at the patch to include any missing functionality I didn't think of. Emmanuel, if you have a chance, please give it a look after I've committed it - your expertise with LDAP is much appreciated here.

          Les

          Show
          Les Hazlewood added a comment - As luck would have it, I'm working on a default LDAP implementation as we speak. It is not based on the patch, although I will be looking at the patch to include any missing functionality I didn't think of. Emmanuel, if you have a chance, please give it a look after I've committed it - your expertise with LDAP is much appreciated here. Les
          Hide
          Philippe Laflamme added a comment -

          I'm glad to see progress on this issue! LDAP support out-of-the-box would be really good for Shiro.

          I wrote the patch in January, so it probably can't be applied to trunk anymore. Didn't keep it up to date (sorry). Besides, I'm no LDAP expert, those comments from Emmanuel were quite informative. LDAP is not simple at all. For example, this whole DN comparison business, I had no idea!

          I wrote the patch by looking at the Active Directory support that was already implemented. I had tried to keep compatibility with AD, but that's also not trivial to achieve (environment is harder to setup).

          For some more inspiration on requirements, I suggest looking at Atlassian's Crowd product. Their LDAP support is obviously much more extensive than what Shiro needs to offer, but it provides some good use-cases for using LDAP for authentication (username/passwords) and authorization (providing groups/roles).

          Have fun Les!

          Show
          Philippe Laflamme added a comment - I'm glad to see progress on this issue! LDAP support out-of-the-box would be really good for Shiro. I wrote the patch in January, so it probably can't be applied to trunk anymore. Didn't keep it up to date (sorry). Besides, I'm no LDAP expert, those comments from Emmanuel were quite informative. LDAP is not simple at all. For example, this whole DN comparison business, I had no idea! I wrote the patch by looking at the Active Directory support that was already implemented. I had tried to keep compatibility with AD, but that's also not trivial to achieve (environment is harder to setup). For some more inspiration on requirements, I suggest looking at Atlassian's Crowd product. Their LDAP support is obviously much more extensive than what Shiro needs to offer, but it provides some good use-cases for using LDAP for authentication (username/passwords) and authorization (providing groups/roles). Have fun Les!
          Hide
          Les Hazlewood added a comment -

          Hi Philippe,

          Your comments and patch are very much welcome - they've helped me greatly along the way towards making a default implementation. Thanks!

          Show
          Les Hazlewood added a comment - Hi Philippe, Your comments and patch are very much welcome - they've helped me greatly along the way towards making a default implementation. Thanks!
          Hide
          Les Hazlewood added a comment -

          I just committed a significant chunk of code related to this (all new classes to ensure I don't disrupt anyone using the existing classes).

          Please see the dev list for discussion: http://shiro-developer.582600.n2.nabble.com/LDAP-rewrite-tp5385227p5385227.html

          Thanks,

          Les

          Show
          Les Hazlewood added a comment - I just committed a significant chunk of code related to this (all new classes to ensure I don't disrupt anyone using the existing classes). Please see the dev list for discussion: http://shiro-developer.582600.n2.nabble.com/LDAP-rewrite-tp5385227p5385227.html Thanks, Les
          Hide
          Alfred Reibenschuh added a comment -

          why not looking for apache's own dog-food or is tomcat 6 ldap authentication not good enough?

          https://svn.apache.org/repos/asf/tomcat/tc6.0.x/tags/TOMCAT_6_0_29/java/org/apache/catalina/realm/JNDIRealm.java

          you could copy most things verbatim.

          2cents


          fredo

          Show
          Alfred Reibenschuh added a comment - why not looking for apache's own dog-food or is tomcat 6 ldap authentication not good enough? https://svn.apache.org/repos/asf/tomcat/tc6.0.x/tags/TOMCAT_6_0_29/java/org/apache/catalina/realm/JNDIRealm.java you could copy most things verbatim. 2cents – fredo
          Hide
          Les Hazlewood added a comment -

          Hi Fredo,

          Good idea - thanks very much for the pointer. I'll see if we can merge what is there with what we already have. I can already see some things that might not make sense with our implementation, but it is a great resource to ensure ours is as robust as it can be. Thanks again!

          Les

          Show
          Les Hazlewood added a comment - Hi Fredo, Good idea - thanks very much for the pointer. I'll see if we can merge what is there with what we already have. I can already see some things that might not make sense with our implementation, but it is a great resource to ensure ours is as robust as it can be. Thanks again! Les
          Hide
          Alfred Reibenschuh added a comment -

          would be cool if shiros ldapRealm behaved identical to tomcats so that users could easily migrate in one direction or the other.

          seam like tomcat got already copied by geronimo

          http://svn.apache.org/repos/asf/geronimo/server/trunk/framework/modules/geronimo-security/src/main/java/org/apache/geronimo/security/realm/providers/LDAPLoginModule.java

          fredo

          Show
          Alfred Reibenschuh added a comment - would be cool if shiros ldapRealm behaved identical to tomcats so that users could easily migrate in one direction or the other. seam like tomcat got already copied by geronimo http://svn.apache.org/repos/asf/geronimo/server/trunk/framework/modules/geronimo-security/src/main/java/org/apache/geronimo/security/realm/providers/LDAPLoginModule.java fredo

            People

            • Assignee:
              Unassigned
              Reporter:
              Philippe Laflamme
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development