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

Documentation and code issue for AD secret engine config #84

Open
peteski22 opened this issue May 15, 2022 · 0 comments
Open

Documentation and code issue for AD secret engine config #84

peteski22 opened this issue May 15, 2022 · 0 comments

Comments

@peteski22
Copy link

Issue Summary

The documentation for the AD secrets engine requires the following:

  • binddn a distinguished name for the object that will be bound when attempting to search AD
  • bindpass the password for the object identified by the binddn (above)

Additionally there are two optional configuration parameters:

  • userdn the distinguished name of the base object (OU) to be used when searching for users
  • upndomain a 'user principal name' domain (i.e. suffix of a userPrincipalName)

The documentation for upndomain contains the following:

The constructed UPN will appear as [username]@UPNDomain. Example: example.com, which will cause vault to bind as username@example.com.

The first issue is that username doesn't appear in the documentation at all, and thus is confusing for the reader/Vault operator.

The second issue is that if upndomain is supplied then binddn is ignored as a DN, and instead, the value supplied for the binddn and the upndomain are concatenated together with an @ between them. Therefore binddn as a parameter name is misleading in this context.

This causes problems as it requires Vault operators to know that by supplying upndomain (e.g. vaultproject.io) they will be required to 'repurpose' binddn as the prefix for the userPrincipalName (i.e. bob in the userPrincipalName: bob@vaultproject.io) that should be used to bind to LDAP.

From having a glance at the code, I believe this to be a bug, albeit one that's always been there, also the documentation isn't clear users.

Suggestion:

  • Don't try to repurpose the Vault core ldaputil/config fields within this plugin, consider adding additional fields to the plugin/client/config such as binddn, bindupn
  • Clarify and improve the public documentation about the priority/precedence of using the config params
  • Stretch goal: userdn is also a bit misleading as it's actually just the base DN for searches to start at, it would be awesome if this could be renamed something more intuative such as searchbasedn
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

1 participant