Directory ApacheDS
  1. Directory ApacheDS
  2. DIRSERVER-606

ou=users, ou=system - user cannot see their own entry

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.0-RC1
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:
      JDK 1.4.1
      Tried both JXplorer, and from ACEGI security

      Description

      User binds to ApacheDS as a user under ou=users, ou=system. The user cannot see their own entry to get their own attributes.

      Documentation states: Users cannot see other user entries under the 'ou=users,ou=system' entry.

      Agreed and understood. But, the user, after binding with the directory, cannot even find their own entry to get their own attributes.

      1. patch_DIRSERVER-606_2.txt
        1 kB
        Stefan Zoerner
      2. patch.txt
        0.7 kB
        Stefan Zoerner

        Activity

        Hide
        Stefan Zoerner added a comment -

        Able to reproduce. It is just like Marc describes. Starting from a default RC1, I used the admin to add an entry like this:

        dn: cn=Fiona Apple,ou=users,ou=system
        objectclass: top
        objectclass: person
        cn: Fiona Apple
        sn: Apple
        userpassword: machine

        Performing a
        $ ldapsearch -h localhost -p 10389 -D "cn=Fiona Apple,ou=users,ou=system" -w machine -s one -b "ou=users,ou=system" "(objectClass=*)" dn
        gives no results

        I assume an error in the OldAuthorizationService component. If I comment this interceptor out in the server.xml (name=oldAuthorizationService), the search op above gives Fionas entry (and all others).

        $ ldapsearch -h localhost ...
        version: 1
        dn: cn=Fiona Apple,ou=users,ou=system

        dn: cn=Kate Bush,ou=users,ou=system
        $

        Show
        Stefan Zoerner added a comment - Able to reproduce. It is just like Marc describes. Starting from a default RC1, I used the admin to add an entry like this: dn: cn=Fiona Apple,ou=users,ou=system objectclass: top objectclass: person cn: Fiona Apple sn: Apple userpassword: machine Performing a $ ldapsearch -h localhost -p 10389 -D "cn=Fiona Apple,ou=users,ou=system" -w machine -s one -b "ou=users,ou=system" "(objectClass=*)" dn gives no results I assume an error in the OldAuthorizationService component. If I comment this interceptor out in the server.xml (name=oldAuthorizationService), the search op above gives Fionas entry (and all others). $ ldapsearch -h localhost ... version: 1 dn: cn=Fiona Apple,ou=users,ou=system dn: cn=Kate Bush,ou=users,ou=system $
        Hide
        Stefan Zoerner added a comment -

        This small patch (1 line) allows users to search their own entry. It changes a condition within the isSearchable method of OldAuthorizationService.

        There is still a problem left, if the DN of the user contains uppercase letters, or if s/he uses uppercase letters in the bind DN. In these cases, the issue still exists. The reason for this is the equals method of org.apache.directory.shared.ldap.name.LdapName, which is used frequently in OldAuthorizationService, and which is case sensitive.

        Two options here:
        a) modify equals() in LdapName to ignore case
        b) perform special checks in OldAuthorizationService which ignore case

        Show
        Stefan Zoerner added a comment - This small patch (1 line) allows users to search their own entry. It changes a condition within the isSearchable method of OldAuthorizationService. There is still a problem left, if the DN of the user contains uppercase letters, or if s/he uses uppercase letters in the bind DN. In these cases, the issue still exists. The reason for this is the equals method of org.apache.directory.shared.ldap.name.LdapName, which is used frequently in OldAuthorizationService, and which is case sensitive. Two options here: a) modify equals() in LdapName to ignore case b) perform special checks in OldAuthorizationService which ignore case
        Hide
        Emmanuel Lecharny added a comment -

        I have a different behavior. I also have created the same entry (with LdapBrowser, and it's not easy, becuase if you don't create a file to store the password, then there is no way you can modify the entry after having added it.)

        Now, if I try to do :
        ldapsearch -h localhost -p 10389 -D "cn=fiona apple,ou=users,ou=system" -w machine -s sub -b "ou=users,ou=system" "(objectClass=*)" dn

        I got :
        ldap_sasl_interactive_bind_s: Insufficient access (50)
        additional info: failed on search operation

        I changed the command to :
        ldapsearch -x -h localhost -p 10389 -D "cn=fiona apple,ou=users,ou=system" -w machine -s sub -b "ou=users,ou=system" "(objectClass=*)" dn

        (the -x is for Simple authentication)

        Now, I get this error :
        ldap_bind: Invalid credentials (49)
        additional info: Bind failed

        Of course, if I use the admin DN to authenticate, it works :
        ldapsearch -x -h localhost -p 10389 -D "uid=admin,ou=system" -w secret -s sub -b "ou=users,ou=system" "(objectClass=*)" dn

        gives the following result :

        1. extended LDIF
          #
        2. LDAPv3
        3. base <ou=users,ou=system> with scope sub
        4. filter: (objectClass=*)
        5. requesting: dn
          #
        1. users, system
          dn: ou=users,ou=system
        1. Fiona Apple, users, system
          dn: cn=Fiona Apple,ou=users,ou=system
        1. search result
          search: 2
          result: 0 Success
        1. numResponses: 3
        2. numEntries: 2
        Show
        Emmanuel Lecharny added a comment - I have a different behavior. I also have created the same entry (with LdapBrowser, and it's not easy, becuase if you don't create a file to store the password, then there is no way you can modify the entry after having added it.) Now, if I try to do : ldapsearch -h localhost -p 10389 -D "cn=fiona apple,ou=users,ou=system" -w machine -s sub -b "ou=users,ou=system" "(objectClass=*)" dn I got : ldap_sasl_interactive_bind_s: Insufficient access (50) additional info: failed on search operation I changed the command to : ldapsearch -x -h localhost -p 10389 -D "cn=fiona apple,ou=users,ou=system" -w machine -s sub -b "ou=users,ou=system" "(objectClass=*)" dn (the -x is for Simple authentication) Now, I get this error : ldap_bind: Invalid credentials (49) additional info: Bind failed Of course, if I use the admin DN to authenticate, it works : ldapsearch -x -h localhost -p 10389 -D "uid=admin,ou=system" -w secret -s sub -b "ou=users,ou=system" "(objectClass=*)" dn gives the following result : extended LDIF # LDAPv3 base <ou=users,ou=system> with scope sub filter: (objectClass=*) requesting: dn # users, system dn: ou=users,ou=system Fiona Apple, users, system dn: cn=Fiona Apple,ou=users,ou=system search result search: 2 result: 0 Success numResponses: 3 numEntries: 2
        Hide
        Emmanuel Lecharny added a comment -

        Ooops...

        I have debugged the server, and I realized that the problem I had was related to the way the password was created. As you just can import it from a file, if you create this file with a standard editor, it adds a '\a' at the end of it.

        Now, I have the correct result

        ldapsearch -x -h localhost -p 10389 -D "cn=fiona apple,ou=users,ou=system" -w machine -s sub -b "ou=users,ou=system" "(objectClass=*)"

        gives :

        1. extended LDIF
          #
        2. LDAPv3
        3. base <ou=users,ou=system> with scope sub
        4. filter: (objectClass=*)
        5. requesting: ALL
          #
        1. users, system
          dn: ou=users,ou=system
          objectClass: top
          objectClass: organizationalUnit
          ou: users
        1. search result
          search: 2
          result: 0 Success
        1. numResponses: 2
        2. numEntries: 1
        Show
        Emmanuel Lecharny added a comment - Ooops... I have debugged the server, and I realized that the problem I had was related to the way the password was created. As you just can import it from a file, if you create this file with a standard editor, it adds a '\a' at the end of it. Now, I have the correct result ldapsearch -x -h localhost -p 10389 -D "cn=fiona apple,ou=users,ou=system" -w machine -s sub -b "ou=users,ou=system" "(objectClass=*)" gives : extended LDIF # LDAPv3 base <ou=users,ou=system> with scope sub filter: (objectClass=*) requesting: ALL # users, system dn: ou=users,ou=system objectClass: top objectClass: organizationalUnit ou: users search result search: 2 result: 0 Success numResponses: 2 numEntries: 1
        Hide
        Emmanuel Lecharny added a comment -

        Ok, I think that we can fix the code this way :

        ...
        if ( dn.startsWith( USER_BASE_DN ) || dn.startsWith( GROUP_BASE_DN ) )
        {
        // check that it's not the user himself.
        if ( dn.equals( dnParser.parse( principalDn.toString() ) ) )

        { return true; }

        return false;
        }
        ...

        The problem is that, if you do it, you will get the userPassword :

        ldapsearch -x -h localhost -p 10389 -D "cn=Fiona apple,ou=users,ou=system" -w machine -s one -b "ou=users,ou=system" "(objectClass=*)"

        1. extended LDIF
          ...
        2. Fiona Apple, users, system
          dn: cn=Fiona Apple,ou=users,ou=system
          ...
          userpassword:: bWFjaGluZQ==

        and it's easy to check that bWFjaGluZQ== is the base 64 encoded value for "machine"...

        Is it what we want? I don't think so.

        wdyt ?

        Show
        Emmanuel Lecharny added a comment - Ok, I think that we can fix the code this way : ... if ( dn.startsWith( USER_BASE_DN ) || dn.startsWith( GROUP_BASE_DN ) ) { // check that it's not the user himself. if ( dn.equals( dnParser.parse( principalDn.toString() ) ) ) { return true; } return false; } ... The problem is that, if you do it, you will get the userPassword : ldapsearch -x -h localhost -p 10389 -D "cn=Fiona apple,ou=users,ou=system" -w machine -s one -b "ou=users,ou=system" "(objectClass=*)" extended LDIF ... Fiona Apple, users, system dn: cn=Fiona Apple,ou=users,ou=system ... userpassword:: bWFjaGluZQ== and it's easy to check that bWFjaGluZQ== is the base 64 encoded value for "machine"... Is it what we want? I don't think so. wdyt ?
        Hide
        Stefan Zoerner added a comment -

        Emmanuel, I recommend to fix it like this (attached file patch_DIRSERVER-606_2.txt) instead. Your proposal has two disadvantages (as far as I understand the situation):

        1) Call to DnParser.parse is not synchronized (I assume it should, because other calls in the class are)

        2) At least one case is not fixed:

        $ ldapsearch -h localhost -p 10389 -D "uid=admin,ou=system" -w ***** -b "uid= admin,ou=system" "(objectClass=*)" dn

        display the admin entry, but

        $ ldapsearch -h localhost -p 10389 -D "uid=Admin,ou=system" -w ***** -b "uid= admin,ou=system" "(objectClass=*)" dn

        does not (it displays nothing), because principalDn is not normalized in isSearchable for all cases. We should consider to normalize all principalDn occurrences in this class. There might be other problems due to the many LdapName.equals() calls.

        Show
        Stefan Zoerner added a comment - Emmanuel, I recommend to fix it like this (attached file patch_ DIRSERVER-606 _2.txt) instead. Your proposal has two disadvantages (as far as I understand the situation): 1) Call to DnParser.parse is not synchronized (I assume it should, because other calls in the class are) 2) At least one case is not fixed: $ ldapsearch -h localhost -p 10389 -D "uid=admin,ou=system" -w ***** -b "uid= admin,ou=system" "(objectClass=*)" dn display the admin entry, but $ ldapsearch -h localhost -p 10389 -D "uid=Admin,ou=system" -w ***** -b "uid= admin,ou=system" "(objectClass=*)" dn does not (it displays nothing), because principalDn is not normalized in isSearchable for all cases. We should consider to normalize all principalDn occurrences in this class. There might be other problems due to the many LdapName.equals() calls.
        Hide
        Emmanuel Lecharny added a comment -

        Stefan,

        DnParser is internally synchronized, so there is no need to synchronize it outside. DnParser.parse should be totally stateless, but actually it's not, this is why it has been synchronized. However, this is not a problem.

        What I did may not be enough, as you stated, and your patch is better, but the question is : do we need to handle this bug? The problem is that if we fix it, user can see it's password, and super-user can see all passwords. Is that an expected feature? I don't think so. Alex pointed that this is OldAuthz, so it should be replaced by the newer one, with ACIs. I think that's very valid. Another solution could be to avoid sending userPassword in the response.

        For Alex it's like it is legacy interceptor. May be we should suppress it from configuration.

        Patching the server is quite easy, we can do it in five minutes, and your patch is ok (regardless to synchronization). But do we have to do it? That's the main point...

        wdyt ?

        btw, we must merge LdapName and LdapDN. We reached the conclusion with Alex that LdapName must be deleted, to avoid confusion with jdk 1.5 LdapName behavior. As we have another implementation (LdapDN) which is totally thread safe (without synchronization) and twice faster, this is the way to go. But it's not an simple patch, because LdapName is used all over the code. We may have a impact analysis before doing this move. A confluence page has been added for this task, but it's far from being complete.

        Show
        Emmanuel Lecharny added a comment - Stefan, DnParser is internally synchronized, so there is no need to synchronize it outside. DnParser.parse should be totally stateless, but actually it's not, this is why it has been synchronized. However, this is not a problem. What I did may not be enough, as you stated, and your patch is better, but the question is : do we need to handle this bug? The problem is that if we fix it, user can see it's password, and super-user can see all passwords. Is that an expected feature? I don't think so. Alex pointed that this is OldAuthz, so it should be replaced by the newer one, with ACIs. I think that's very valid. Another solution could be to avoid sending userPassword in the response. For Alex it's like it is legacy interceptor. May be we should suppress it from configuration. Patching the server is quite easy, we can do it in five minutes, and your patch is ok (regardless to synchronization). But do we have to do it? That's the main point... wdyt ? btw, we must merge LdapName and LdapDN. We reached the conclusion with Alex that LdapName must be deleted, to avoid confusion with jdk 1.5 LdapName behavior. As we have another implementation (LdapDN) which is totally thread safe (without synchronization) and twice faster, this is the way to go. But it's not an simple patch, because LdapName is used all over the code. We may have a impact analysis before doing this move. A confluence page has been added for this task, but it's far from being complete.
        Hide
        Stefan Zoerner added a comment -

        Emmanuel,

        currently (1.0 RC1), the OldAuthorizationService is enabled in the configuration by default.
        I think we have therefore two options:

        a) keep it enabled
        In this case we should fix the bugs within the service (like this one), and try to have it work as described in the documentation (or update the documentation)
        http://directory.apache.org/subprojects/apacheds/docs/users/authentication.html
        In these docs the OldAuthorizationService is interpreted as "minimal built-in rules", not legacy.
        How about renaming OldAuthorizationService to MinimalAuthorizationService (or something like that), if we decide to keep it?

        b) disable or (even better) remove the service
        In this case, we have to update the docs, and to ensure that the default ACI configuration is reasonable secure.

        Password thing: I think it should be possible to allow users to read passwords. In some occasions someone may need it (if s/he wishes to write his/her own replication, for instance). I prefer to forbid it by default via ACIs (and allow users to change this behavior).

        Show
        Stefan Zoerner added a comment - Emmanuel, currently (1.0 RC1), the OldAuthorizationService is enabled in the configuration by default. I think we have therefore two options: a) keep it enabled In this case we should fix the bugs within the service (like this one), and try to have it work as described in the documentation (or update the documentation) http://directory.apache.org/subprojects/apacheds/docs/users/authentication.html In these docs the OldAuthorizationService is interpreted as "minimal built-in rules", not legacy. How about renaming OldAuthorizationService to MinimalAuthorizationService (or something like that), if we decide to keep it? b) disable or (even better) remove the service In this case, we have to update the docs, and to ensure that the default ACI configuration is reasonable secure. Password thing: I think it should be possible to allow users to read passwords. In some occasions someone may need it (if s/he wishes to write his/her own replication, for instance). I prefer to forbid it by default via ACIs (and allow users to change this behavior).
        Hide
        Emmanuel Lecharny added a comment -

        regarding your proposal :

        a) Yeah, we can do that. This is for RC2, and its better to have something that works, even if you can read your password, than something that is badly broken.

        a-2) I agree with "minimal built-in rules". I used legacy, because it was something which will be removed soon. I don't know when, may be in 1.1 or in 1.0

        b) Yes, we have to update the doc. Feel free to ask Alex about the intricacy of ACI usage with the newer AuthorizationService. As he wrote the code, he is the one who know it the best. We sure need this doco if we want users not to be puzzled as we are...

        Password things : I'm not very confortable with it. I don't like the fact for instance that I can read my passwords in Firefox. The problem is if you can do that, then you will have to be paranoïd : each time you go to have a copy, lock your computer... I don't see any occasion where clear password need to be shown to the user, even if the files that contains the entries is not crypted (eh eh, another improvment ...). Well, this is another problem, and we can fill a JIRA for that, too

        Show
        Emmanuel Lecharny added a comment - regarding your proposal : a) Yeah, we can do that. This is for RC2, and its better to have something that works, even if you can read your password, than something that is badly broken. a-2) I agree with "minimal built-in rules". I used legacy, because it was something which will be removed soon. I don't know when, may be in 1.1 or in 1.0 b) Yes, we have to update the doc. Feel free to ask Alex about the intricacy of ACI usage with the newer AuthorizationService. As he wrote the code, he is the one who know it the best. We sure need this doco if we want users not to be puzzled as we are... Password things : I'm not very confortable with it. I don't like the fact for instance that I can read my passwords in Firefox. The problem is if you can do that, then you will have to be paranoïd : each time you go to have a copy, lock your computer... I don't see any occasion where clear password need to be shown to the user, even if the files that contains the entries is not crypted (eh eh, another improvment ...). Well, this is another problem, and we can fill a JIRA for that, too
        Hide
        Alex Karasulu added a comment -

        I like the renaming idea. I guess this OldAuthZService will always hang around as long as people don't want to grok the ACI based AuthZService. Don't know if we should bother deleting this. I guess its best to rename to Minimal/Default AuthZService as you guys suggest.

        But this problem should be solved. Users should be allowed to see their own entry. Users should be able to see and update their own entries for self service perhaps? WDYT? Sometimes I think another application with superuser access can do this and regular users can be shielded from their own attributes but this does not play well when logging in as yourself to the directory.

        Show
        Alex Karasulu added a comment - I like the renaming idea. I guess this OldAuthZService will always hang around as long as people don't want to grok the ACI based AuthZService. Don't know if we should bother deleting this. I guess its best to rename to Minimal/Default AuthZService as you guys suggest. But this problem should be solved. Users should be allowed to see their own entry. Users should be able to see and update their own entries for self service perhaps? WDYT? Sometimes I think another application with superuser access can do this and regular users can be shielded from their own attributes but this does not play well when logging in as yourself to the directory.
        Hide
        Emmanuel Lecharny added a comment -

        So let's go for the renaming. I'll do it as soon I'm back home. DefaultAuthZService seems ok.

        What we can do is to apply Stefan patch as is, and latter we can improve the password exposure (for instance, returns stars instead of real password). At least we can discuss about it later.

        wdyt ? need a vote ?

        Show
        Emmanuel Lecharny added a comment - So let's go for the renaming. I'll do it as soon I'm back home. DefaultAuthZService seems ok. What we can do is to apply Stefan patch as is, and latter we can improve the password exposure (for instance, returns stars instead of real password). At least we can discuss about it later. wdyt ? need a vote ?
        Hide
        Alex Karasulu added a comment -

        That sounds like a good plan E. No I don't think we need a vote.

        Show
        Alex Karasulu added a comment - That sounds like a good plan E. No I don't think we need a vote.
        Hide
        Emmanuel Lecharny added a comment -

        Fixed. The patch submitted by Stefan has been applied, and the OldAuthorizationService class and reference into server.xml file have been changed to DefaultAuthorizationService and defaultAuthorizationService.

        I have tested the different cases I found in all the comments, and it seems to be ok. A junit testcase would be appreciated.

        Show
        Emmanuel Lecharny added a comment - Fixed. The patch submitted by Stefan has been applied, and the OldAuthorizationService class and reference into server.xml file have been changed to DefaultAuthorizationService and defaultAuthorizationService. I have tested the different cases I found in all the comments, and it seems to be ok. A junit testcase would be appreciated.
        Hide
        Stefan Zoerner added a comment -

        Thanks Emmanuel! I have created a JUnit test in apachsds/server-unit (DefaultAuthorizationTest), which tests the situation described in this issue. Currently I have unfortunately no svn access – I will commit it to the 1.0 branch tonight if I have again.

        One question left: I have noticed that you have changed server.xml in apacheds/server-main to reflect the new service name. I assume, that it is also necessary to modify it in server-installers/src/main/installers?

        Show
        Stefan Zoerner added a comment - Thanks Emmanuel! I have created a JUnit test in apachsds/server-unit (DefaultAuthorizationTest), which tests the situation described in this issue. Currently I have unfortunately no svn access – I will commit it to the 1.0 branch tonight if I have again. One question left: I have noticed that you have changed server.xml in apacheds/server-main to reflect the new service name. I assume, that it is also necessary to modify it in server-installers/src/main/installers?
        Hide
        Emmanuel Lecharny added a comment -

        closed

        Show
        Emmanuel Lecharny added a comment - closed

          People

          • Assignee:
            Emmanuel Lecharny
            Reporter:
            Marc Batchelor
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development