Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot log into datahub-frontend if the username contains a space #4915

Closed
tullis opened this issue May 13, 2022 · 4 comments
Closed

Cannot log into datahub-frontend if the username contains a space #4915

tullis opened this issue May 13, 2022 · 4 comments
Assignees
Labels
bug Bug report devops PR or Issue related to DataHub backend & deployment stale

Comments

@tullis
Copy link

tullis commented May 13, 2022

Bug Description
When using an LDAP authentication source with JAAS - the usernames that are returned from the directory sometimes contains spaces.

For these users, although the authentication succeeds, the play framework then throws a 500 error and the login attempt fails.

The reason seems to be that the frontend is trying to insert the username into a cookie, but the space character is being rejected.

Our JAAS configuration is as follows:

WHZ-Authentication {
  com.sun.security.auth.module.LdapLoginModule required
  userProvider="ldaps://ldap-ro.eqiad.wikimedia.org:636/ou=people,dc=wikimedia,dc=org ldaps://ldap-ro.codfw.wikimedia.org:636/ou=people,dc=wikimedia,dc=org"
  userFilter="(&(objectClass=inetOrgPerson)(cn={USERNAME})(|(memberof=cn=nda,ou=groups,dc=wikimedia,dc=org)(memberof=cn=wmf,ou=groups,dc=wikimedia,dc=org)))"
  authzIdentity="{uid}"
  debug="true";
};

This is the search-first mode which searches for the user's LDAP entry with their common name (which may contain spaces).
Their distinguished name matches the search, returning one object that is then used for an authentication attempt.

I added the authzIdentity="{uid}" statement to the JAAS configuration file to try to ensure that the uid attribute was used in the construction of the user's urn. However it disn't work because it only adds this as an //additional// UserPrincipal on the Subject. The username with the space is still there. Therefore the authzIdentity could be removed from the configuration and the bug would still apply.

When a user whose cn contains a space authenticates, the datahub-frontend log contains the following:

May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] search-first mode; SSL enabled
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] user provider: ldaps://ldap-ro.eqiad.wikimedia.org:636/ou=people,dc=wikimedia,dc=org
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] searching for entry belonging to user: Emil Chetty
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] found entry: uid=echetty,ou=people,dc=wikimedia,dc=org
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] attempting to authenticate user: Emil Chetty
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] authentication succeeded
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] added LdapPrincipal "uid=echetty,ou=people,dc=wikimedia,dc=org" to Subject
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] added UserPrincipal "Emil Chetty" to Subject
May 12 10:32:36 stat1008 playBinary[23376]:                 [LdapLoginModule] added UserPrincipal "echetty" to Subject
May 12 10:32:36 stat1008 playBinary[23376]: 10:32:36 [application-akka.actor.default-dispatcher-142] ERROR akka.actor.ActorSystemImpl - Internal server error, sending 500 response
May 12 10:32:36 stat1008 playBinary[23376]: java.lang.IllegalArgumentException: Cookie value contains an invalid char:

Expected behavior
The user should be logged in.

Desktop (please complete the following information):
This is a server-side issue, so the browser context is not really relevant.

Additional context
There is an upstream bug report in the play framework, which is relevant: t2v/play2-auth#180

The answers here indicate that the spaces should be encoded before being added to the cookie. Perhaps this technique would work for datahub-frontend too.

@tullis tullis added the bug Bug report label May 13, 2022
@RyanHolstien
Copy link
Collaborator

@tullis In your WHZ configuration, you're using the user CN as the link for the username: cn={USERNAME}. Is there a reason you need to use this key as the user login? Typically the CN is a "display-style" name and not a unique identifier. The authZ addition to the config would not impact the cookie issue as the username cookie is directly linked to what you use to login to DataHub with.

To have the uid be what gets stored in the cookie, you would need to set it up something like:

WHZ-Authentication {
  com.sun.security.auth.module.LdapLoginModule required
  userProvider="ldaps://ldap-ro.eqiad.wikimedia.org:636/ou=people,dc=wikimedia,dc=org ldaps://ldap-ro.codfw.wikimedia.org:636/ou=people,dc=wikimedia,dc=org"
  userFilter="(&(objectClass=inetOrgPerson)(uid={USERNAME})(|(memberof=cn=nda,ou=groups,dc=wikimedia,dc=org)(memberof=cn=wmf,ou=groups,dc=wikimedia,dc=org)))"
  debug="true";
};

where instead of filtering on CN you are filtering on uid and then have users login with their uid. Typically email or some other unique identifier is used as the LDAP login to avoid the "John Doe" problem rather than the common name which only enforces uniqueness on the same OU.

See:
https://ldapwiki.com/wiki/CommonName
https://ldapwiki.com/wiki/Best%20Practices%20For%20Unique%20Identifiers

There are a couple of possible solutions here, but we would generally prefer to take the approach of using a more valid identifier as the login ID if at all possible.

@tullis
Copy link
Author

tullis commented May 23, 2022

Thanks @RyanHolstien - Yes we're currently using the uid value in the way you have suggested.

Is there a reason you need to use this key as the user login?

The use of the cn attribute has been requested in our case simply because that's the username that people use for other systems that also use the same authentication source. In this case it's a Wikimedia Developer Account and unfortunately the guidelines state that spaces may be used.

I'll see if it's possible for us to proceed based only on the uid attribute as suggested, but use of the cn is still something that I think people would prefer if it were possible.

@maggiehays maggiehays added the devops PR or Issue related to DataHub backend & deployment label Jul 8, 2022
@github-actions
Copy link

This issue is stale because it has been open for 30 days with no activity. If you believe this is still an issue on the latest DataHub release please leave a comment with the version that you tested it with. If this is a question/discussion please head to https://slack.datahubproject.io. For feature requests please use https://feature-requests.datahubproject.io

@github-actions github-actions bot added the stale label Sep 15, 2022
@github-actions
Copy link

This issue was closed because it has been inactive for 30 days since being marked as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report devops PR or Issue related to DataHub backend & deployment stale
Projects
None yet
Development

No branches or pull requests

3 participants