vault-plugin-secrets-ad icon indicating copy to clipboard operation
vault-plugin-secrets-ad copied to clipboard

Documentation and code issue for AD secret engine config

Open peteski22 opened this issue 2 years ago • 0 comments

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 [email protected].

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: [email protected]) 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

peteski22 avatar May 15 '22 18:05 peteski22