LDAP should not check username on "sn" field

Bug #997700 reported by Doug Johnston
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Fix Released
Medium
Unassigned
Essex
Fix Released
Medium
Unassigned
keystone (Ubuntu)
Fix Released
Undecided
Unassigned
Precise
Fix Released
Undecided
Unassigned

Bug Description

The ldap backend is hardcoded to only check the entered username against the "sn" attribute type. In general, this is a misuse of the "sn" attribute, which refers to SurName, but the fact that this is hardcoded is more troublesome, as the username field may take on different attribute types in various LDAP implementations. Most widely used would be "cn", or CommonName, which generally maps to a username.

Most LDAP implementations allow the name of the field on which the query is done to be specified in a config file. Indeed, there are many options in the keystone config relating to LDAP fields, but this is not one of them.

See: https://github.com/openstack/keystone/blob/master/keystone/identity/backends/ldap/core.py#L251

The quick fix is to make this "cn" instead of "sn", the better fix would be to make this an option in the config.

I imaging this would make ldap auth fail for the majority of people - everyone who doesn't have their username the same as their lastname.

Adam Young (ayoung)
Changed in keystone:
assignee: nobody → Adam Young (ayoung)
Revision history for this message
Adam Young (ayoung) wrote :

So the trivial first fix is to change sn to cn for the name field in the identity LDAP backend.

The problem is that the cn is used as the user ID field already, and this cannot be the Name. I suspect that the right field to use would be the uid. But for inetOrgPerson, that is listed as MAY not MUST. That goes all the way up the hierarchy to 'Person' where the only two MUST fields are cn and sn.

Revision history for this message
Tim Spriggs (tims-t) wrote :

It would be nice to have a generic mapping table that can be specified in the config. Since LDAP is fairly malleable and users can create their own schema there is a good chance that people want to use non-standard attributes for various fields with keystone. Also, having a generic mapping table would make it easier for people who are interested in using other ldap servers (such as Active Directory) as a backend.

From my (perhaps limited) experience, the defaults you want for a Unix style environment are: uid, cn, posixAccount

posixAccount requires cn and uid:

https://www.google.com/search?q=posixaccount+schema

However, it's possible that people want to use other attributes (again, AD as a prime example)

Revision history for this message
Doug Johnston (dvjohnston) wrote :

Why do you say this cannot be the Name? Why is the check being done on name in the first place? Using the sample session below, the very first search is on "sn" = <the value the user types into the username field> (in this case, "me"). Seems cn is the most sensible. I would also assume that inetOrgPerson is just another default setting - I dont think it's mandatory, but does appear to be a default for a person in OpenLDAP, which is what I'm using in the session below.

The fact that uid isn't mandatory is a problem with LDAP in general - that different organizations use different fields to represent different things, hence the real need to make this a setting. I'm not a ldap expert by any means though, so others might have a more informed opinion on this. To take inspiration from another LDAP config (this is Jetbrains TeamCity):

2012-05-08 20:26:10 DEBUG [keystone.common.wsgi] arg_dict: {}
2012-05-08 20:26:10 DEBUG [keystone.common.ldap.core] LDAP init: url=ldap://ldap.blah.com
2012-05-08 20:26:10 DEBUG [keystone.common.ldap.core] LDAP bind: dn=cn=manager,dc=blah,dc=com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP search: dn=ou=People,dc=blah,dc=com, scope=1, query=(&(sn=me)(objectClass=inetOrgPerson))
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP init: url=ldap://ldap.blah.com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP bind: dn=cn=manager,dc=blah,dc=com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP search: dn=cn=me,ou=People,dc=blah,dc=com, scope=0, query=(objectClass=inetOrgPerson)
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP init: url=ldap://ldap.blah.com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP bind: dn=cn=me,ou=People,dc=blah,dc=com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP init: url=ldap://ldap.blah.com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP bind: dn=cn=manager,dc=blah,dc=com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP search: dn=cn=blah,cn=com,ou=Groups, scope=1, query=(&(member=cn=me,ou=People,dc=blah,dc=com)(objectClass=groupOfNames))
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP init: url=ldap://ldap.blah.com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP bind: dn=cn=manager,dc=blah,dc=com
2012-05-08 20:26:11 DEBUG [keystone.common.ldap.core] LDAP search: dn=cn=None,cn=blah,cn=com,ou=Groups, scope=0, query=(objectClass=groupOfNames)

Revision history for this message
Tim Spriggs (tims-t) wrote :

Doug: uid is mandatory for the posixAccount objectClass. It's not a problem with LDAP, it just makes it slightly more complex to support in software.

If someone wants to use the same ldap auth source that their unix system uses then you will likely see posixAccount (or even subclasses of posixAccount) which guarantees everything that a unix account needs.

Another potential glitch is groupOfUniqueNames vs posixGroup. While the former contains a list of verified DNs (uniqueMember), the latter is a list of usernames (memberUID). There is nothing stopping someone from having an object with both classes. Maintaing both sets of classes is another story...

Many pieces of software will adopt their own schema when their settings don't map 1:1 and simply enforce usage of their own schema to make their own lives easier. The expense is that ldap administrators lives get slightly harder as they then need to support the different schema. In no way am I recommending this, just mentioning it.

Revision history for this message
dvjohnston (doug-johnston) wrote : Re: [Bug 997700] Re: LDAP should not check username on "sn" field

Quite right. I wasn't trying to suggest it was a problem per se, just
that it's flexibility often makes proper configuration more
complicated. In this case, just one more parameter.

On Thu, May 10, 2012 at 3:11 PM, Tim Spriggs <email address hidden> wrote:
> Doug: uid is mandatory for the posixAccount objectClass. It's not a
> problem with LDAP, it just makes it slightly more complex to support in
> software.
>
> If someone wants to use the same ldap auth source that their unix system
> uses then you will likely see posixAccount (or even subclasses of
> posixAccount) which guarantees everything that a unix account needs.
>
> Another potential glitch is groupOfUniqueNames vs posixGroup. While the
> former contains a list of verified DNs (uniqueMember), the latter is a
> list of usernames (memberUID). There is nothing stopping someone from
> having an object with both classes. Maintaing both sets of classes is
> another story...
>
> Many pieces of software will adopt their own schema when their settings
> don't map 1:1 and simply enforce usage of their own schema to make their
> own lives easier. The expense is that ldap administrators lives get
> slightly harder as they then need to support the different schema. In no
> way am I recommending this, just mentioning it.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/997700
>
> Title:
>  LDAP should not check username on "sn" field
>
> Status in OpenStack Identity (Keystone):
>  New
>
> Bug description:
>  The ldap backend is hardcoded to only check the entered username
>  against the "sn" attribute type. In general, this is a misuse of the
>  "sn" attribute, which refers to SurName, but the fact that this is
>  hardcoded is more troublesome, as the username field may take on
>  different attribute types in various LDAP implementations. Most widely
>  used would be "cn", or CommonName, which generally maps to a username.
>
>  Most LDAP implementations allow the name of the field on which the
>  query is done to be specified in a config file. Indeed, there are many
>  options in the keystone config relating to LDAP fields, but this is
>  not one of them.
>
>  See:
>  https://github.com/openstack/keystone/blob/master/keystone/identity/backends/ldap/core.py#L251
>
>  The quick fix is to make this "cn" instead of "sn", the better fix
>  would be to make this an option in the config.
>
>  I imaging this would make ldap auth fail for the majority of people -
>  everyone who doesn't have their username the same as their lastname.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/keystone/+bug/997700/+subscriptions

Revision history for this message
Tim Spriggs (tims-t) wrote :

I looked a little more carefully at the code [1] and also noticed that you have a query where you then return the first result:

    return users[0]

    I am still not fully aware of the rest of the code but this breaks when a non-unique field is chosen (such as sn.) Also, watching the queries on the backend LDAP server [2] I notice that you run 3 queries in order to authenticate a user:

    1. search for user
    2. get user entry
    3. bind as user

    Also, each query is done through a separate (and authenticated) connection to the ldap server.

    Why not just bind if you have a base dn already? In the case below it would end up looking like:

    BIND REQ conn=3098764 op=0 msgID=1 type=SIMPLE dn="sn=Spriggs,ou=People,dc=base_dn"

    From there you can immediately run (on the now authenticated connection) a query for the user object that you know exists since you just bound to it correctly. This is where specifying uid instead of sn in the config helps other sites too.

    Finally, in the quest to find groups [3] it looks like there is a bug when none are found, thus the search for "cn=None,ou=Groups,...". I'd like to also suggest changing the filter to eventually fill out as:

(|
  (&(memberUid=tims)(objectClass=posixGroup))
  (&(uniqueMember=uid=tims,ou=People,dc=base_dn)(objectClass=groupOfUniqueNames))
)

Feel free to contact me offlist if you have questions. I'd be happy to jump in and test with you in the next two weeks.

Cheers,
-Tim

--

    [1] https://github.com/openstack/keystone/blob/master/keystone/identity/backends/ldap/core.py#L272

    [2] Find/auth user
    SEARCH REQ conn=3098762 op=1 msgID=2 base="ou=People,dc=base_dn" scope=singleLevel filter="(&(sn=Spriggs)(objectClass=inetOrgPerson))" attrs="ALL"
    SEARCH REQ conn=3098763 op=1 msgID=2 base="uid=tims,ou=People,dc=base_dn" scope=baseObject filter="(objectClass=inetOrgPerson)" attrs="ALL"
    BIND REQ conn=3098764 op=0 msgID=1 type=SIMPLE dn="uid=tims,ou=People,dc=base_dn"

    [3] Find groups
    SEARCH REQ conn=3098765 op=1 msgID=2 base="ou=Groups,dc=base_dn" scope=singleLevel filter="(&(memberUid=uid=tims,ou=People,dc=base_dn)(objectClass=posixGroup))" attrs="ALL"
    SEARCH REQ conn=3098766 op=1 msgID=2 base="cn=None,ou=Groups,dc=base_dn" scope=baseObject filter="(objectClass=posixGroup)" attrs="ALL"

Revision history for this message
Adam Young (ayoung) wrote :

I've attached code. While I have run it through the unit tests, I have not tested it against a live LDAP server. I will do so shortly, but need to set it up.

IN the [LDAP] section of the config, you should be able to specify

user_id_attribute = uid
user_name_attribute = cn

Or some other variation. Please let me know if this meets your needs.

Joseph Heck (heckj)
Changed in keystone:
status: New → Triaged
importance: Undecided → Medium
Revision history for this message
Tim Spriggs (tims-t) wrote :

I went ahead and applied this to my Debian system and I still get queries for cn=username instead of uid=username. The code does not seem to use the user_id_attribute setting at all. However, using the following in my config it will end up using uid instead of cn:

user_name_attribute = uid

Do you want me to post separate bugs for the other issues I have run into?

Revision history for this message
Adam Young (ayoung) wrote :

No lets keep it in this bug, as the fix will end up covering both.

I think that you did a query that uses the user name and not user id, but without seeing exactly what you did, it is just conjecture.

THe patch I posted is what allows user_name_attribute = uid to work.

Did you get any logging information? W hat exactly did you test?

Revision history for this message
Tim Spriggs (tims-t) wrote :

My LDAP entry looks something like this:

---
 4 0 uid=tims,ou=People,dc=...
 5 uid: tims
 6 objectClass: shadowaccount
 7 objectClass: hinetaccount
 8 objectClass: pirlaccount
 9 objectClass: ptysaccount
10 objectClass: person
11 objectClass: organizationalPerson
12 objectClass: inetOrgPerson
13 objectClass: lplaccount
14 objectClass: top
15 givenName: Tim
16 hinetHomeDirectory: /home/hinet/tims
17 cn: Tim Spriggs
18 hinetLoginShell: /bin/tcsh
19 sn: Spriggs
20 gecos: Tim Spriggs
21 pirlloginShell: /bin/tcsh
22 uidNumber: 1925
23 mail: tims@...
24 pirlhomeDirectory: /home/pirl_xfs/tims
25 gidNumber: 10212
26 lplloginShell: /bin/tcsh
27 lplhomeDirectory: /home/tims
---

I tested authentication with a short script:
---
#!/bin/bash

echo -n user:
read OS_USERNAME
echo -n password:
stty -echo
read OS_PASSWORD
stty echo
echo

export OS_USERNAME OS_PASSWORD
keystone --auth_url http://ks0...:5000/v2.0 token-get
---

OS_USERNAME was set to "tims"

This results in the following queries:
...
SEARCH REQ conn=3782342 op=1 msgID=2 base="ou=People,dc=..." scope=singleLevel filter="(&(uid=tims)(objectClass=inetOrgPerson))" attrs="ALL"
SEARCH RES conn=3782342 op=1 msgID=2 result=0 nentries=1 etime=1
...
SEARCH REQ conn=3782343 op=1 msgID=2 base="uid=tims,ou=People,dc=..." scope=baseObject filter="(objectClass=inetOrgPerson)" attrs="ALL"
SEARCH RES conn=3782343 op=1 msgID=2 result=0 nentries=1 etime=1
...
BIND REQ conn=3782344 op=0 msgID=1 type=SIMPLE dn="uid=tims,ou=People,dc=..."
BIND RES conn=3782344 op=0 msgID=1 result=0 authDN="uid=tims,ou=People,dc=..." etime=1
...
SEARCH REQ conn=3782345 op=1 msgID=2 base="ou=Groups,dc=..." scope=singleLevel filter="(&(memberUid=uid=tims,ou=People,dc=...)(objectClass=posixGroup))" attrs="ALL"
SEARCH RES conn=3782345 op=1 msgID=2 result=0 nentries=0 etime=5
...
SEARCH REQ conn=3782346 op=1 msgID=2 base="cn=None,ou=Groups,dc=..." scope=baseObject filter="(objectClass=posixGroup)" attrs="ALL"
SEARCH RES conn=3782346 op=1 msgID=2 result=32 message="The search base entry 'cn=None,ou=Groups,dc=...' does not exist" nentries=0 etime=1

When I run the script I get this for output:

./test_keystone_auth
user:tims
password:
No handlers could be found for logger "keystoneclient.v2_0.client"
+----------+----------------------------------+
| Property | Value |
+----------+----------------------------------+
| expires | 2012-06-14T21:42:01Z |
| id | 318f8XXXXXXXXXXXXXXXXXXXXXXXXXXX |
| user_id | tims |
+----------+----------------------------------+

(I'm not sure what "id" is exactly so I replaced the end with X)

Revision history for this message
Adam Young (ayoung) wrote :

Id is a unique identifier, and if you do the defaul;t with Keystone, which is to let it set them, it will generate an UUID for the user. By default, the ID field of the user can be seen under the User object in keystone/identity/backends/ldap/core.py

class UserApi(common_ldap.BaseLdap, ApiShimMixin):
    DEFAULT_OU = 'ou=Users'
    DEFAULT_STRUCTURAL_CLASSES = ['person']
    DEFAULT_ID_ATTRIBUTE = 'cn'
    DEFAULT_OBJECTCLASS = 'inetOrgPerson'
    options_name = 'user'
    DEFAULT_ID_ATTRIBUTE = 'cn'

I am not sure where yours came from.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (stable/essex)

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/10370

Revision history for this message
Alan Pevec (apevec) wrote :

> Fix proposed to branch: stable/essex

What about master?

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (master)

Fix proposed to branch: master
Review: https://review.openstack.org/10544

Changed in keystone:
status: Triaged → In Progress
Changed in keystone:
assignee: Adam Young (ayoung) → Anne Gentle (annegentle)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (master)

Reviewed: https://review.openstack.org/10544
Committed: http://github.com/openstack/keystone/commit/4f3dcb6c9b23867e6049f24c851b12904aee3b76
Submitter: Jenkins
Branch: master

commit 4f3dcb6c9b23867e6049f24c851b12904aee3b76
Author: Adam Young <email address hidden>
Date: Thu Jul 26 15:30:39 2012 -0400

    Allow overloading of username and tenant name in the config files.

    Includes documentation and sample config file values.

    Bug 997700

    Patchset adds DocImpact flag for notifying doc team about these new
    config file values.

    Change-Id: Ibd3fade3f233a3b89a1c2feaa0a6b5a9569ad86c

Changed in keystone:
status: In Progress → Fix Committed
Anne Gentle (annegentle)
Changed in keystone:
assignee: Anne Gentle (annegentle) → nobody
Thierry Carrez (ttx)
Changed in keystone:
milestone: none → folsom-3
status: Fix Committed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (stable/essex)

Fix proposed to branch: stable/essex
Review: https://review.openstack.org/11737

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (stable/essex)

Reviewed: https://review.openstack.org/11737
Committed: http://github.com/openstack/keystone/commit/a130848c71f1bc65dcf98c085dee0c4796748faa
Submitter: Jenkins
Branch: stable/essex

commit a130848c71f1bc65dcf98c085dee0c4796748faa
Author: Adam Young <email address hidden>
Date: Thu Jul 26 15:30:39 2012 -0400

    Allow overloading of username and tenant name in the config files.

    Includes documentation and sample config file values.

    Bug 997700

    Patchset adds DocImpact flag for notifying doc team about these new
    config file values.

    (cherry picked from commit 4f3dcb6c9b23867e6049f24c851b12904aee3b76)

    Conflicts:

     etc/keystone.conf.sample
     keystone/config.py

    Change-Id: I94a162be07c224c705333804a53910833df96b8e

tags: added: in-stable-essex
Dave Walker (davewalker)
Changed in keystone (Ubuntu):
status: New → Fix Released
Changed in keystone (Ubuntu Precise):
status: New → Confirmed
Revision history for this message
Adam Gandelman (gandelman-a) wrote : Verification report.

Please find the attached test log from the Ubuntu Server Team's CI infrastructure. As part of the verification process for this bug, Keystone has been deployed and configured across multiple nodes using precise-proposed as an installation source. After successful bring-up and configuration of the cluster, a number of exercises and smoke tests have be invoked to ensure the updated package did not introduce any regressions. A number of test iterations were carried out to catch any possible transient errors.

Please Note the list of installed packages at the top and bottom of the report.

For records of upstream test coverage of this update, please see the Jenkins links in the comments of the relevant upstream code-review(s):

Trunk review: https://review.openstack.org/10544
Stable review: https://review.openstack.org/11737

As per the provisional Micro Release Exception granted to this package by the Technical Board, we hope this contributes toward verification of this update.

Revision history for this message
Adam Gandelman (gandelman-a) wrote :

Test coverage log.

tags: added: verification-done
Revision history for this message
Clint Byrum (clint-fewbar) wrote : Update Released

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package keystone - 2012.1+stable~20120824-a16a0ab9-0ubuntu2

---------------
keystone (2012.1+stable~20120824-a16a0ab9-0ubuntu2) precise-proposed; urgency=low

  * New upstream release (LP: #1041120):
    - debian/patches/0013-Flush-tenant-membership-deletion-before-user.patch:
      Dropped.
  * Resynchronize with stable/essex:
    - authenticate in ldap backend doesn't return a list of roles
      (LP: #1035428)
    - LDAP should not check username on "sn" field (LP: #997700)
    - Admin API doesn't valid token. (LP: #1006815, #1006822)
    - Memcache token backend eventually stops working. (LP: #1012381)
    - EC2 credentials not migrated from legacy (diablo) database. (LP: #1016056)
    - Deleting tenants or users does not cleanup metadata. (LP: #973243)
    - Deleting tenants does not cleanup its user associations. (LP: #974199)
    - TokenNotFound not raised in testsuite beacuse of timezone issues. (LP: #983800)
    - Token authentication for a user in a disabled tenant does not raise
      Unauthorized error. (LP: #988920)
    - export_legacy_catalog doesn't convert url names correctly. (LP: #994936)
    - Following a password compromise and subsequent password change,
      tokens remain valid. (LP: #996595)
    - Tokens remain valid after a user account is disabled. (LP: #997194)
 -- Adam Gandelman <email address hidden> Fri, 24 Aug 2012 03:34:59 -0400

Changed in keystone (Ubuntu Precise):
status: Confirmed → Fix Released
Thierry Carrez (ttx)
Changed in keystone:
milestone: folsom-3 → 2012.2
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to keystone (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/278791

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.