mirror of
https://github.com/lldap/lldap.git
synced 2026-04-25 16:25:55 +03:00
[GH-ISSUE #254] ldapsearch result returns duplicate dn attribute #92
Labels
No labels
backend
blocked
bug
cleanup
dependencies
docker
documentation
duplicate
enhancement
enhancement
frontend
github_actions
good first issue
help wanted
help wanted
integration
invalid
ldap
pull-request
question
rust
rust
tests
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/lldap-lldap#92
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @ikaruswill on GitHub (Jul 14, 2022).
Original GitHub issue: https://github.com/lldap/lldap/issues/254
Context
I have a home-assistant installation that relies on an
ldap-authscript that runsldapsearchagainst an LDAP server by directly binding as logging in user, and running a search onmemberof. If the result does not return exactly 1dn, the login fails. I was just debugging this yesterday and I came across this issue.Problem
ldapsearchresults return duplicatednattribute, not seen in queryingopenldapwith the exact same query.Against LLDAP:
Against OpenLDAP:
Relevant logs:
@nitnelave commented on GitHub (Jul 14, 2022):
Oh, I see, dn is returned as part of the search result and as an attribute. That's easy to fix :)
@ikaruswill commented on GitHub (Jul 14, 2022):
For reference:
https://github.com/bob1de/ldap-auth-sh/blob/master/ldap-auth.sh
Myself and I believe quite a number of other users use this script to integrate LDAP authentication into home-assistant.
@ikaruswill commented on GitHub (Oct 10, 2022):
Hi there @nitnelave any chance you'll be pushing a new tagged release that includes this change soon?
@nitnelave commented on GitHub (Oct 10, 2022):
Sure, let me prepare a minor release.
@ikaruswill commented on GitHub (Oct 10, 2022):
On this issue, I just bumped my LLDAP version to
0.4.1and I'm afraid there was another problem.Issue
It appears that dexidp/dex project requires some predefined attribute to be present on the
Personobject in order to match aGroup's list ofmemberattributes' values to determine group membership. Removing thednattribute from thePersonobject essentially disables this matching process here as there is nodnattribute to access ingetAttrs(), so the auth process silently fails.Context
For context, I'm using dex as an auth connector for argoproj/argo-cd, and dex itself is a widely used auth connector with increasing adoption.
Why did it work on OpenLDAP
As for why it used to work in OpenLDAP, it can probably be explained by the
RFC2307bisschema used in OpenLDAP in the past where thememberUIDattribute on thePersonwas used as amemberattribute value on theGroup, hence matching was no issue. But since LLDAP does not follow theRFC2307bisschema, it indirectly gave rise to this issue.Options
home-assistantcommunity to rethink authentication via the ldap-auth.sh script if they were to come across the same issue.uidof thePersonobject as value ofmemberattributes inGroupobjects, which is a breaking change.What are your thoughts? @nitnelave
@nitnelave commented on GitHub (Oct 10, 2022):
A small starting note: I made the initial change not just because it fixes the automation script, but also because it makes sense on its own for the project. I don't think reverting is the way to go.
On RFC2307 vs 2307bis: as I see it, the difference is not between
memberandmemberUID, but betweenmember(attribute of groups) andmemberOf(attribute of user). LLDAP implements both :)A non-breaking change going forward can be to add the
memberUIDattribute to the groups, that identifies users based on their UID instead of DN.It seems you can also configure Dex to use
CNorUIDinstead ofDN, in the configuration:Regarding the group search, they say:
# Following list contains field pairs that are used to match a user to a group. It adds an additional
# requirement to the filter that an attribute in the group must match the user's
# attribute value.
userMatchers:
- userAttr: uid
groupAttr: member
It seems that setting the
userAttrtodnshould work? And if theirgetAttrfunction doesn't return anything for thedn, I'd say that it's a bug on their part. If they don't want to fix it, we might look into returning it as an attribute IFF it's requested as an attribute, but I'm a bit skeptical that it's the right way forward.@ikaruswill commented on GitHub (Oct 11, 2022):
Hmm interesting, thanks for taking a deep look into this.
I had thought that the change in #281 was to remove the
dnattribute onPersonreturned when executing a search. Based on what you're saying and after taking a closer look at the PR, it seems thednattribute is still returned as a default behaviour, just that an extradnreturn was removed. My bad.Fully agree with your response overall. Let me dive a little deeper into their code to see what's going on with the
getAttrsfunction. If all else fails, I'll file another issue as a feature request for thememberUIDattribute onGroupobject.Really appreciate your time!
@ikaruswill commented on GitHub (Oct 11, 2022):
Update:
After digging deeper, I found a bug in
dex'sgetAttrsfunction.In the underlying dependency package
go-ldap, thednattribute is treated in a special way and is not considered an "attribute" of theldap.Entryobject so it cannot be found inldap.Entry.Attributes.go-ldap'sldap.Entrytype (Ref):And here is where the
dnis parsed and its value inserted into theldap.Entry.DNattribute.go-ldap'sldap.Entry.Unmarshalmethod (Ref):This creates an edge case, so
getAttrshas to handle for whendnis specified as theuserAttrfor matching, where it must retrieve thednvalue fromldap.Entry.DNitself rather than from the list inldap.Entry.Attributes.Back to
dex, in thegetAttrsfunction, we can see how they handle the edge case.But this
if name == "DN"block uses direct string matching and does not account for when the configuration specifies the lower-cased value "dn". Thus after switching theuserAttrto "DN", it worked flawlessly.I initially specified
dnas theuserAttrand it just didn't work withlldap:0.4.1since the duplicatednattribute was removed. My guess as to why it worked prior to the latest changes is that only the firstdnwas processed separately, and the duplicatednwas accepted as an attribute ofldap.Entry, tbh I never really got down to figuring out why.Nonetheless, thanks for pointing me in the right direction. I'll go ahead and submit a PR to dex to do a case-insensitive match on the value "DN".
@nitnelave commented on GitHub (Oct 11, 2022):
I'll just mention that their documentation has a pretty explicit paragraph about "DN" being case-sensitive :) But yeah, I think it's better to have a case-insensitive "DN" (actuallly every attribute should be case insensitive according to the LDAP RFCs).
Good job with the investigation!
@ikaruswill commented on GitHub (Oct 11, 2022):
Hah! I can't believe I missed that! 🤦♂️ Thanks for the heads up.