Page MenuHomePhabricator

Ldap auth extension vs. ldap vs. username Case
Closed, ResolvedPublic

Description

NOTE: As of 2019-04-15, Wikitech's LdapAuthenication extension is configured use cn:caseExactMatch: when looking up a developer account in LDAP. If you are getting an Incorrect username or password entered. Please try again. error message when you attempt to login, double check that the Username you are providing matches the case of your User:... page exactly.

We've just discovered that I can log in to wikitech with any case variant of my username:

Andrew Bogott
Andrew bogott
Andrew BOGOTT
etc.

Each time I log in with a novel case variant, the login succeeds but I get a NEW account in the wikitech db with the new case variation. This means that different case combinations can have different 2fa creds, edit histories, etc.

There are currently 35 wikitech users with multiple case-differing accounts in the labswiki db. We need to fix the ldap extension and then clean up/merge all of those accounts.

Event Timeline

Andrew created this task.May 19 2017, 4:21 PM
Restricted Application added a project: Cloud-Services. · View Herald TranscriptMay 19 2017, 4:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

The wgLdapLoserCaseUsername may be meant to address this issue but that setting is off on wikitech.

Presuming that the ldap auth check is case insensitive (which I think it is), we have effectively decided that usernames on ldap-auth wikis have case insensitive usernames. The step that actually creates the wikimedia user account out of the ldap creds needs to make a case-insensitive search for existing accounts before creation and then either tell the user to log in with dIffeRenT cAse or else just log them in as the existing user.

Reedy added a comment.May 19 2017, 4:40 PM
mysql:wikiadmin@silver [labswiki]> select user_name, count(user_name) as cnt from user group by lcase( user_name ) HAVING cnt > 1;
+--------------------------+-----+
| user_name                | cnt |
+--------------------------+-----+
| Andrew BOGOTT            |   3 |
| ArielGlenn               |   2 |
| CodeDev                  |   2 |
| DamianZaremba            |   2 |
| EBernhardson             |   2 |
| FDans                    |   2 |
| GWicke                   |   2 |
| IAlex                    |   2 |
| Jack Phoenix             |   2 |
| JameerBabu               |   2 |
| JAufrecht                |   2 |
| JayBox                   |   2 |
| Jeroen De Dauw           |   2 |
| JForrester               |   2 |
| JGirault                 |   2 |
| JSalsman                 |   2 |
| Khaled El Mansoury       |   2 |
| LA2                      |   2 |
| MarkAHershberger         |   2 |
| MArostegui               |   2 |
| Mike Morearty            |   2 |
| MikeMel                  |   2 |
| NeilK                    |   2 |
| Ryan Lane                |   2 |
| Sean Chen                |   2 |
| Shweta Chandrakant Pawar |   2 |
| SMcCandlish              |   2 |
| SNiedzielski             |   2 |
| SPage                    |   2 |
| Tim Starling             |   2 |
| TParis                   |   2 |
| Vedmaka Wakalaka         |   2 |
| Victor Vasiliev          |   2 |
| WebIntegrity             |   2 |
| YasBot                   |   2 |
+--------------------------+-----+
35 rows in set (0.05 sec)

The step that actually creates the wikimedia user account out of the ldap creds needs to [snip]

Not exactly. When LdapPrimaryAuthenticationProvider::beginPrimaryAuthentication() is going to return a successful AuthenticationRequest (here), it needs to use the right version of the account name instead of just using the passed-in $username.

It might do a lookup as you propose, or it might do $username = $ldap->getCanonicalName( $username ); which as far as I can tell is probably how the pre-AuthManager code decided what casing of the name to use. I don't know which might be preferable.

It might do a lookup as you propose, or it might do $username = $ldap->getCanonicalName( $username ); which as far as I can tell is probably how the pre-AuthManager code decided what casing of the name to use. I don't know which might be preferable.

Actually, since AuthManager doesn't give providers any way to control the version of the name used when an account is being created manually, perhaps it would be best to do the lookup rather than using $ldap->getCanonicalName().

Andrew added a comment.Jun 1 2017, 3:22 PM

Bump! @Reedy or @Anomie can one of you propose a patch for this or do I need to dig into this extension?

Anomie added a comment.Jun 1 2017, 4:13 PM

I think I'll pass. Some notes:

  • There's a config option ($wgLDAPLowerCaseUsername) to control whether LDAP lowercases or not. It looks like the default is to lowercase. I don't know what mangling LDAP might do if the flag is changed to not-lowercase, but that'll need to be taken into account. The code makes it seem like there is potential for some mangling in that case.
  • The code to look up all variations of the username may need some thought to be efficient.
  • Make sure that whatever this does actually matches whatever LDAP does. If you consider some names different that LDAP considers the same, you'll still have this bug. If you consider two names the same that LDAP considers different, you might make it possible to be logged into the wrong account.
bd808 added a subscriber: bd808.Mar 22 2018, 4:41 PM

Getting worse as time goes on:

(wikiadmin@m5-master.eqiad.wmnet) [labswiki]> select user_name, count(user_name) as cnt from user group by lcase( user_name ) HAVING cnt > 1;
+--------------------------+-----+
| user_name                | cnt |
+--------------------------+-----+
| AndreG-P                 |   2 |
| Andrew BOGOTT            |   3 |
| ArielGlenn               |   2 |
| CodeDev                  |   2 |
| DamianZaremba            |   2 |
| DBrant                   |   2 |
| EBernhardson             |   2 |
| FDans                    |   2 |
| FreedomFighterSparrow    |   2 |
| GWicke                   |   2 |
| IAlex                    |   2 |
| Jack Phoenix             |   2 |
| JameerBabu               |   2 |
| JAufrecht                |   2 |
| JayBox                   |   2 |
| Jeroen De Dauw           |   2 |
| JForrester               |   2 |
| JGirault                 |   2 |
| JSalsman                 |   2 |
| Khaled El Mansoury       |   2 |
| LA2                      |   2 |
| LMixter                  |   2 |
| MarkAHershberger         |   2 |
| MArostegui               |   2 |
| Mike Morearty            |   2 |
| MikeMel                  |   2 |
| MNeisler                 |   2 |
| NeilK                    |   2 |
| PHedenskog               |   2 |
| QoreQyaS                 |   2 |
| RIsler                   |   2 |
| RoySmith                 |   2 |
| RV1971                   |   2 |
| Ryan Lane                |   2 |
| SAM0410                  |   2 |
| Sean Chen                |   2 |
| Shweta Chandrakant Pawar |   2 |
| SMcCandlish              |   2 |
| SNiedzielski             |   2 |
| SPage                    |   2 |
| Tim Starling             |   2 |
| TParis                   |   2 |
| UltrasonicNXT            |   2 |
| VCloudernBeer            |   2 |
| Vedmaka Wakalaka         |   2 |
| Victor Vasiliev          |   2 |
| WebIntegrity             |   2 |
| WMDE-Fisch               |   2 |
| YasBot                   |   2 |
+--------------------------+-----+
49 rows in set (0.11 sec)
bd808 added a comment.Mar 22 2018, 5:54 PM

To filter out accounts that have been "fixed" by having 0 edit variations blocked (currently just one account):

SELECT user_name, COUNT(user_name) AS cnt
FROM user
WHERE user_id not in (SELECT ipb_user FROM ipblocks)
GROUP BY LCASE( user_name )
HAVING cnt > 1;
bd808 added a comment.Apr 6 2018, 12:09 AM

This might be horrible in practice, but it looks like in theory we could change the lookup attribute to cn:caseExactMatch: instead of cn and the LDAP server would enforce a case-sensitive search:

$ ldap cn=bryandavis cn
dn: uid=bd808,ou=people,dc=wikimedia,dc=org
cn: BryanDavis
$ ldap cn=BryanDavis cn
dn: uid=bd808,ou=people,dc=wikimedia,dc=org
cn: BryanDavis
$ ldap cn:caseExactMatch:=bryandavis cn
$ ldap cn:caseExactMatch:=BryanDavis cn
dn: uid=bd808,ou=people,dc=wikimedia,dc=org
cn: BryanDavis
jbond added a subscriber: jbond.Mar 18 2019, 10:13 AM

Change 497423 had a related patch set uploaded (by BryanDavis; owner: Bryan Davis):
[operations/mediawiki-config@master] wikitech: Use cn:caseExactMatch: as account search filter

https://gerrit.wikimedia.org/r/497423

Tgr added a comment.Mar 18 2019, 11:51 PM

We need a method that returns all case variants of a name that exist in LDAP; the rest of the patch does not require familiarity with LDAP. If that list is empty, allow user creation; if the list is exactly one element, only allow login/creation if it matches the casing provided by the user (since providers cannot change the casing of the MediaWiki user); if it's larger, probably just log and die. Also return the list in providerNormalizeUsername() for account creation UI checks.

It's worse than just ldap, gerrit also creates a ton of dupes with differing case.

chasemp added a subscriber: chasemp.EditedApr 1 2019, 5:06 PM

Can we tell ldap to enforce non-case sensitivity?

Edit: Is there potentially a way to game the workflow in T219277: Wikitech password reset flow here? Could someone potentially register a case difference for say Foo -> FoO and get a password reset that causes weird interaction between wikitech and ldap?

bd808 added a comment.Apr 1 2019, 7:47 PM

Can we tell ldap to enforce non-case sensitivity?

Yes, with the :caseExactMatch: matching rule. See T165795#4110716. I'm not sure if it is possible to make the cn/sn matching case exact by default.

As I noted in the WIP gerrit change to do this on wikitech there are currently 251 records in the LDAP directory that would be prevented from authn to Wikitech if we applied this without any changes to the directory content. It should be "easy" to fix the conflicting cn/sn data to match MediaWiki's Title casing rules for usernames.

Thanks @bd808. I'm going to propose we do this in the next working group meeting on monday.

@ArielGlenn points out that we should make sure that modules/icinga/files/cgi.cfg works as expected after any changes. Apparently there have been authn problems in icninga in the past that may have been related to case sensitivity.

Can we tell ldap to enforce non-case sensitivity?

Yes, with the :caseExactMatch: matching rule. See T165795#4110716.

This works, but there seems to be a pretty big caveat with this. Queries are way slower in a simple test. I am guessing that when passing matching rules in the queries, the indexes are not used. Look for example at the output below.

akosiaris@mwmaint1002:~$ time ldapsearch -LLLx 'cn:caseExactMatch:=Alexandros Kosiaris'  dn
dn: uid=akosiaris,ou=people,dc=wikimedia,dc=org
real	0m0.604s
user	0m0.004s
sys	0m0.004s

akosiaris@mwmaint1002:~$ time ldapsearch -LLLx 'cn=Alexandros Kosiaris'  dn
dn: uid=akosiaris,ou=people,dc=wikimedia,dc=org
real	0m0.013s
user	0m0.004s
sys	0m0.004s

600ms for a run of the mill query will probably kill performance for everything if it becomes default. If we limit it to a query that happens fairly rarely (like on account registration) it should be fine, but defaulting to it is probably a no go.

I'm not sure if it is possible to make the cn/sn matching case exact by default.

No, it is not I am afraid (unless I am horribly mistaken). commonName case is hardcoded in the software[1], as it is required by multiple things internal to the software. We would have to rebuilt our own version of the software, changing a ton of thing in the process and probably breaking a ton of assumptions internal to the software. We would also be deviating from one of the most basic LDAP schemas in the history of the protocol meaning a ton of compatibility problems on various other tools as well

[1] https://www.openldap.org/devel/gitweb.cgi?p=openldap.git;a=blob;f=servers/slapd/schema_prep.c#l916 Note that it is SUP of name which is caseIgnoreMatch.

chasemp added a comment.EditedApr 8 2019, 5:00 PM

I'm not close to an ldap expert so links below are for myself :D

Updated: https://phabricator.wikimedia.org/T165795#5094719

Here's what I feel like I've learned after reading the above and searching around for documentation.

  • LDAP expects DN/CN to be case-insensitive when searching and comparing as part of the default schema
  • Because this behavior is codified in various RFC's it would be reasonable for switching CN to case sensitive with LDAP itself to break various consumers
  • Because case-insensitivity is the default in the schema, it is how values are indexed which reinforces what Alex wrote above. Case sensitive searching without schema changes, reindexing, and hackery seems improbably from a performance standpoint.
  • Our real point of concern here is not necessarily case insensitive matching for authentication, but case insensitive creation for account duping due to case insensitive matching for authentication.
  • It seems both Wikitech and Striker (if separate for this logic) should do a case insensitive search during the username creation process and throw an error if the CN already exists in any case combination...which it doesn't do now I guess. It's weird because it's defaulting to case insensitive logic except when it comes to account creation.

https://tools.ietf.org/html/rfc4517
https://tools.ietf.org/html/rfc2256
https://ldapwiki.com/wiki/Distinguished%20Name%20Case%20Sensitivity
https://ldapwiki.com/wiki/CaseExactMatch
https://ldapwiki.com/wiki/LDAP%20Server%20Standards%20and%20Specifications
http://shibboleth.net/pipermail/users/2011-August/000046.html
https://www.openldap.org/lists/openldap-software/200310/msg00752.html

chasemp added a parent task: Restricted Task.Apr 8 2019, 5:12 PM

@bd808 explained the problem to be better, and the above outline is a misunderstanding on my part. Seems like the short term fix here is to have a case-insensitive check at the wikitech layer that prevents the case account duping which will/could have weird consequences with the case insensitivity on LDAP. @bd808 agreed to poke at it this week.

bd808 added a comment.Apr 13 2019, 3:31 PM
  • Our real point of concern here is not necessarily case insensitive matching for authentication, but case insensitive creation for account duping due to case insensitive matching for authentication.
  • It seems both Wikitech and Striker (if separate for this logic) should do a case insensitive search during the username creation process and throw an error if the CN already exists in any case combination...which it doesn't do now I guess. It's weird because it's defaulting to case insensitive logic except when it comes to account creation.

The issue is entirely on the Wikitech & LdapAuthentication side of things. There is no corruption or duplication in the LDAP directory. There is however a bug where Wikitech can create multiple local MediaWiki accounts which are associated with a single LDAP account. Even more confusing, this association is implicit rather than explicit. By that I mean that there is no data in the MediaWiki users table that tells you what the associated LDAP record is. Instead the association is made a the point of login by asking the LDAP directory to authenticate the username and password supplied in the login form. This is where the case-insensitive matching on the LDAP side is causing problems. Right now "BryanDavis" and "Bryandavis" would both pass the MediaWiki Title normalization unchanged, and both would be mapped to uid=bd808,ou=people,dc=wikimedia,dc=org by the LDAP auth-bind lookup.

I see and understand the concerns from @akosiaris in T165795#5092381. I personally believe this un-indexed lookup is an acceptable overhead in the short term because it will only be used in the Wikitech account login flow. This is not a high rate of change event in our current system. It would be disastrous to try to put an un-indexed lookup on an event that happens thousands of times a day like unix shell auth in Cloud VPS instances. I don't think we will even notice it on Wikitech logins though.

I see and understand the concerns from @akosiaris in T165795#5092381. I personally believe this un-indexed lookup is an acceptable overhead in the short term because it will only be used in the Wikitech account login flow. This is not a high rate of change event in our current system.

Instinctively, I agree. It would be nice to know the rate of logins however. Do you know if we have it somewhere? My grafana foo has failed me up to now.

Tgr added a comment.Apr 15 2019, 6:34 PM

statsd typically does not allow you to slice the data per wiki. Logstash says there were 60 logins in the last 7 days, which is trivial (unless you are worried about the potential DoS angle).

bd808 added a comment.Apr 15 2019, 7:57 PM

statsd typically does not allow you to slice the data per wiki. Logstash says there were 60 logins in the last 7 days, which is trivial (unless you are worried about the potential DoS angle).

Expanding out to the full data currently in ELK for this we get about 30 days worth of data (wikitech log shipping was broken for a while) and see 235 events. Completely trivial.

Change 497423 had a related patch set uploaded (by BryanDavis; owner: Bryan Davis):
[operations/mediawiki-config@master] wikitech: Use cn:caseExactMatch: as account search filter

https://gerrit.wikimedia.org/r/497423

Change 497423 merged by jenkins-bot:
[operations/mediawiki-config@master] wikitech: Use cn:caseExactMatch: as account search filter

https://gerrit.wikimedia.org/r/497423

Mentioned in SAL (#wikimedia-operations) [2019-04-15T23:17:40Z] <bd808> scap: SWAT: [[gerrit:497423|wikitech: Use cn:caseExactMatch: as account search filter]] (T165795)

bd808 updated the task description. (Show Details)

Message override added to wikitech to point folks back to this ticket when their logins fail:

bd808 claimed this task.Apr 15 2019, 11:44 PM
bd808 triaged this task as High priority.

Email announce of change sent to cloud-announce and wikitech-l.

Unblocked users with duplicate accounts that we need to cleanup now that we have blocked case-insensitive logins:

MariaDB [labswiki]> SELECT user_name, COUNT(user_name) AS cnt
    -> FROM user
    -> WHERE user_id not in (SELECT ipb_user FROM ipblocks)
    -> GROUP BY LCASE( user_name )
    -> HAVING cnt > 1;
+--------------------------+-----+
| user_name                | cnt |
+--------------------------+-----+
| AKlapper                 |   2 |
| AndreG-P                 |   2 |
| Andrew BOGOTT            |   3 |
| ArielGlenn               |   2 |
| Ben Brand                |   2 |
| CodeDev                  |   2 |
| CParle                   |   2 |
| CStone                   |   2 |
| DamianZaremba            |   2 |
| DBrant                   |   2 |
| DEXi                     |   2 |
| DGideas                  |   2 |
| FDans                    |   2 |
| FreedomFighterSparrow    |   2 |
| GWicke                   |   2 |
| IAlex                    |   2 |
| Jack Phoenix             |   2 |
| JameerBabu               |   2 |
| JarBot                   |   2 |
| JAufrecht                |   2 |
| JayBox                   |   2 |
| Jeroen De Dauw           |   2 |
| JForrester               |   2 |
| JGirault                 |   2 |
| Jnanaranjan Sahu         |   2 |
| JSalsman                 |   2 |
| Khaled El Mansoury       |   2 |
| LA2                      |   2 |
| LMixter                  |   2 |
| MarkAHershberger         |   2 |
| MArostegui               |   2 |
| MEpps                    |   2 |
| Mike Morearty            |   2 |
| MikeMel                  |   2 |
| MNeisler                 |   2 |
| Moritz.Finke             |   2 |
| NadyaD                   |   2 |
| NeilK                    |   2 |
| PHedenskog               |   2 |
| QoreQyaS                 |   2 |
| QuangThong81             |   2 |
| RIsler                   |   2 |
| RoySmith                 |   2 |
| RUYABA                   |   2 |
| RV1971                   |   2 |
| Ryan Lane                |   2 |
| SAM0410                  |   2 |
| Sean Chen                |   2 |
| ShikhaJadoun1997         |   2 |
| ShreyasMinocha           |   2 |
| Shweta Chandrakant Pawar |   2 |
| SMcCandlish              |   2 |
| SmithnWesson09           |   2 |
| SNiedzielski             |   2 |
| SPage                    |   2 |
| SRodlund                 |   2 |
| Tim Starling             |   2 |
| TParis                   |   2 |
| UltrasonicNXT            |   2 |
| VCloudernBeer            |   2 |
| Vedmaka Wakalaka         |   2 |
| Victor Vasiliev          |   2 |
| WebIntegrity             |   2 |
| YasBot                   |   2 |
| YOUR1                    |   2 |
+--------------------------+-----+
65 rows in set (0.14 sec)

Thank for working on this! I've been on edge about this for ages but never felt equipped to properly tackle it.

Change 504434 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[operations/mediawiki-config@master] wikitech: Enable the UserMerge extension for clean-up

https://gerrit.wikimedia.org/r/504434

Change 504434 merged by jenkins-bot:
[operations/mediawiki-config@master] wikitech: Enable the UserMerge extension for clean-up

https://gerrit.wikimedia.org/r/504434

Mentioned in SAL (#wikimedia-operations) [2019-04-16T22:09:45Z] <jforrester@deploy1001> Synchronized wmf-config/InitialiseSettings.php: T165795 Enable the UserMerge extension for clean-up on wikitech (duration: 01m 00s)

Change 504447 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[operations/mediawiki-config@master] wikitech: Give bureaucrats the usermerge right

https://gerrit.wikimedia.org/r/504447

Change 504447 merged by jenkins-bot:
[operations/mediawiki-config@master] wikitech: Give bureaucrats the usermerge right

https://gerrit.wikimedia.org/r/504447

Mentioned in SAL (#wikimedia-operations) [2019-04-16T22:24:05Z] <jforrester@deploy1001> Synchronized wmf-config/InitialiseSettings.php: T165795 Give bureaucrats the usermerge right (duration: 00m 59s)

bd808 closed this task as Resolved.

Duplicate accounts have been cleaned up and config changes should prevent creating new duplicates.