Skip to content

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

Merged
jimlambrt merged 3 commits intohashicorp:mainfrom
kwagga:ldap-add-samaccountname-login-option
Dec 17, 2024
Merged

Adds an option to enable sAMAccountname logins when upndomain is set#146
jimlambrt merged 3 commits intohashicorp:mainfrom
kwagga:ldap-add-samaccountname-login-option

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.

@kwagga kwagga requested a review from jimlambrt as a code owner December 6, 2024 13:24
@kwagga kwagga mentioned this pull request Dec 6, 2024
Merged
6 tasks
jimlambrt
jimlambrt requested changes Dec 6, 2024
View reviewed changes
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 •
edited
Loading

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

jimlambrt commented Dec 17, 2024

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.

jimlambrt
jimlambrt approved these changes Dec 17, 2024
View reviewed changes
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
@himran92 himran92 mentioned this pull request Dec 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimlambrt jimlambrt jimlambrt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kwagga @jimlambrt