Page MenuHomePhabricator

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

Assigned To
Authored By
Andrew
May 19 2017, 4:21 PM
Referenced Files
F28669822: Screen Shot 2019-04-15 at 17.41.26.png
Apr 15 2019, 11:42 PM
Tokens
"Like" token, awarded by Rustyboss1."Like" token, awarded by MutazAwad."Love" token, awarded by Jdforrester-WMF."Party Time" token, awarded by Andrew.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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.

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?

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.

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.

  • 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.

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).

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)

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

Screen Shot 2019-04-15 at 17.41.26.png (105×307 px, 17 KB)

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)

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

Though it is resolved, this task is still linked from [[Special:Userlogin]] when you get an "incorrect password" error message, which I'm getting a lot (see T229227#5585969).

JJMC89 changed the task status from Duplicate to Resolved.Nov 15 2021, 7:02 AM

Hello @Andrew . Can you tell me how can I log in to WikiTech. I have tried several times but I couldn't!

@Abdullah_Al_Noman: Hi, that is off-topic here. Please ask in a support forum instead - thanks!

Crtn32002 removed a subscriber: Crtn32002.
Crtn32002 added a subscriber: Crtn32002.
This comment was removed by Aklapper.
This comment was removed by Aklapper.