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

ldap auth is failing a regex on a list that should be a string #128

Closed
sqqqrly opened this issue Jul 11, 2019 · 4 comments
Closed

ldap auth is failing a regex on a list that should be a string #128

sqqqrly opened this issue Jul 11, 2019 · 4 comments

Comments

@sqqqrly
Copy link

sqqqrly commented Jul 11, 2019

Problem
I am deploying z2jh on a self hosted k8s cluster. When I attempted to enable LDAP auth, I hit an exception on

username = re.subn(r"([^\\]),", r"\1\,", username)[0]

The resolved_username (aka username) is a list of strings where the regex requires a string. As a result, it raises a type error.

Is this unique to my AD server?

Cause
I instrumented how resolved_username is set. The problem seems to be with def resolve_username(self, username_supplied_by_user).

This shows the username_supplied_by_user param provided to the method and its return value.

[I 2019-07-10 23:06:48.474 JupyterHub ldapauthenticator:208] START: resolve_username(foouser)
[D 2019-07-10 23:06:48.474 JupyterHub ldapauthenticator:224] Looking up user with search_base=ou=Professional Services USA,ou=ExampleCo,ou=Users,ou=Corp,dc=example,dc=com, search_filter='(sAMAccountName=foouser)', attributes=sAMAccountName
[I 2019-07-10 23:06:49.149 JupyterHub ldapauthenticator:245] RETURN lookup_dn: resolve_username(foouser) returns ['Foo User']

Here is the structure seen:

[   {   'attributes': {'cn': ['David Ohlemacher']},
        'dn': 'CN=David Ohlemacher,OU=Professional Services USA,OU=Example '
              'Global,OU=Users,OU=Corp,DC=Example,DC=com',
        'raw_attributes': {'cn': [b'David Ohlemacher']},
        'raw_dn': b'CN=David Ohlemacher,OU=Professional Services USA,OU=Exam'
                  b'ple Global,OU=Users,OU=Corp,DC=Example,DC=com',
        'type': 'searchResEntry'}
]

Possible fix
Append "[0]".

return conn.response[0]['attributes'][self.lookup_dn_user_dn_attribute][0]

Temporary hack fix

RUN \
    infinisrc="/usr/local/lib/python3.6/dist-packages/ldapauthenticator/ldapauthenticator.py" \
    && infinitemp="/root/ldapauthenticator.py" \
    && awk '/return conn.response\[0\]\['\''attributes'\''\]\[self.lookup_dn_user_dn_attribute\]/ { $0=$0"[0]" } 1' "$infinisrc" > "$infinitemp" \
    && sed -i 's?\r??g' "$infinitemp" \
    && mv "$infinitemp" "$infinisrc"

Versions

root@hub-59bd4f8567-8mp86:/srv# pip3 search ldapauthenticator
jupyterhub-ldapauthenticator (1.2.2)       - LDAP Authenticator for JupyterHub
  INSTALLED: 1.2.2 (latest)

Helm chart: jupyterhub-0.9-b609a67

sqqqrly added a commit to ohlemacher/zero-to-jupyterhub-k8s that referenced this issue Jul 11, 2019
Fix LDAP with a hack workaround described in
jupyterhub/ldapauthenticator#128.
The hub dockerfile applies a patch to the ldapathenticator py module.

This is based on upstream version 0.9-b609a67 helm chart.
This was obtained from a tarball so a bit disconnected from upstream. However, it is working so checking in.

DCO 1.1 Signed-off-by: David Ohlemacher <[email protected]>
@marcusianlevine
Copy link
Contributor

marcusianlevine commented Jul 28, 2019

@sqqqrly I believe this is a legit issue, I am developing a test suite for this repo and ran into this while setting up my test cases

Assuming I can get my PR with the test suite merged, it will also address this issue: #134

@sqqqrly
Copy link
Author

sqqqrly commented Aug 6, 2019

Great. I would be happy to give a candidate a test. Seems your UTs will likely have it covered though.

@marcusianlevine
Copy link
Contributor

Yeah my test suite got merged and I added logic that checks if the result of resolve_username is a list, and if so unpacks the first element

We're planning to get one more feature PR merged this coming Monday, and then hopefully we can cut a release candidate 🎉

@consideRatio
Copy link
Member

consideRatio commented Sep 15, 2024

I think this got fixed by #117.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants