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

Adds an option to enable sAMAccountname logins when upndomain is set #146

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

kwagga
Copy link
Contributor

@kwagga kwagga commented Dec 6, 2024

Active Directory allows LDAP binds as userprincipalname@upndomain as well as samaccountname@updomain.

With the current LDAP filter samaccountname logins fail when the upndomain configuration parameter is set, since the filter only only checks for userprincipalname=username@updomain.

This PR provides an enable_samaccountname_login option that can be set in the LDAP Authentication method. This will cause the LDAP user search filter to match either userprincipalname or samaccountname attributes instead of just the userprincipalname when the upndomain configuration parameter is set.

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps some unit tests?

@kwagga
Copy link
Contributor Author

kwagga commented Dec 11, 2024

A unit test is a great idea! I've attempted adding a test based on success-with-anon-bind-upn-domain by adding the EnableSamaccountnameLogin: true, client option. The test is successful, but since the eve user does not have a sAMAccountname attribute and value the ldap filter isn't tested. Would such a test be sufficient?

@kwagga
Copy link
Contributor Author

kwagga commented Dec 13, 2024

I've added a few unit tests that sets EnableSamaccountnameLogin: true.

Ideally I would have wanted to test the ldap filter by passing [email protected]. That would require changes to gldap.testdirectory. Adding a sAMAccountname attribute is possible but the gldap.testdirectory builds a Distinguished Name [email protected],ou=people,dc=example,dc=org object which is rather strange. I expected a DN of cn=eve,ou=people,dc=example,dc=org with a UserPrincipalName attribute instead.

@kwagga kwagga requested a review from jimlambrt December 17, 2024 10:43
@jimlambrt
Copy link
Collaborator

I did some digging and I agree that gldap needs some changes. I'm willing to accept this as is for now and then write a proper unit test in a future PR.

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment: we need better unit tests in a future PR

@jimlambrt jimlambrt merged commit 43d3999 into hashicorp:main Dec 17, 2024
5 checks passed
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

Successfully merging this pull request may close these issues.

2 participants