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

Support ldap authentication in vault agent #21641

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

sebinjohn
Copy link
Contributor

@sebinjohn sebinjohn commented Jul 6, 2023

This PR adds support for "ldap" authentication in Vault agent.
This feature comes in handy in situations where entities in ldap are used as a service accounts.

@sebinjohn sebinjohn marked this pull request as ready for review July 6, 2023 22:47
@@ -0,0 +1,3 @@
```release-note:feature
agent: support ldap auth
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change the component from agent to auto-auth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed agent to "auto-auth"

return nil, errors.New("could not convert 'username' config value to string")
}

passwordRaw, ok := conf.Config["password"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach is not as secure as we would like. A better approach would be to have the configuration specify the filename of a file that contains the password. This offers a lot more flexibility to users of this automatic authentication method. Off the top of my head, I can think of two benefits:

  1. the Vault Agent can continue to run even if the LDAP password has changed so long as the file is read each time the (*ldapMethod) Authenticate method is executed and that the file's content is kept up-to-date
  2. running the Vault Agent in Kubernetes would make it easier to directly mount a Kubernetes Secret containing the LDAP password as a file within the Vault Agent's Pod
    The JWT automatic authentication method (command/agentproxyshared/auth/jwt/jwt.go) is a good example of the above approach. In addition, that method includes an extra parameter remove_jwt_after_reading, which causes the Vault Agent to delete the file content after it has read it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed will make the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made this change.

@sebinjohn
Copy link
Contributor Author

@marcboudreau Not sure what the PR review policies are so pinging here.

@VioletHynes
Copy link
Contributor

VioletHynes commented Aug 8, 2023

Hey @sebinjohn ! Apologies for the slow response, but I want to make sure we don't lose track of this one as this is an awesome contribution.

This looks great! The only things I'm looking for before we merge this:

  • The two TODO comments resolved (the comments and related lines removed)
  • A docs page added for this feature, which should involve adding a page (e.g. you can copy /vault/website/content/docs/agent-and-proxy/autoauth/methods/jwt.mdx) and then updating website/data/docs-nav-data.json to add an entry to the auto-auth section.

Thanks for bearing with us, and thanks so much for this contribution! Looking forward to merging this.

I should be fully available to help push this one over the line, so feel free to give me an @VioletHynes once you've made your changes or if I'm slow to review.

@sebinjohn sebinjohn requested a review from a team August 11, 2023 04:28
@sebinjohn sebinjohn requested review from a team as code owners August 11, 2023 04:28
@sebinjohn sebinjohn requested a review from a team August 11, 2023 04:28
@sebinjohn
Copy link
Contributor Author

@VioletHynes I have made the suggested changes. Sorry about the TODOs.

@VioletHynes
Copy link
Contributor

VioletHynes commented Aug 11, 2023

@sebinjohn It looks like your test is failing in CI?

=== Failed
=== FAIL: command/agentproxyshared/auth/ldap TestIngressPass (0.00s)
    ldap_test.go:103: logs did no contain expected error [WARN]  empty pass file read 2023-08-11T04:40:41.068Z [WARN]  empty password file read

See the bottom of https://github.com/hashicorp/vault/actions/runs/5828943440/job/15807606080?pr=21641

There are a couple of small text updates to the docs (mostly Ldap -> LDAP) that I'll make myself now to avoid any additional back and forth.

The only thing I'm looking for to get this merged is the text fix, and I'll merge this :)

Thanks for bearing with us on this, we really appreciate this contribution, so I'm really excited to get it over the line!

@sebinjohn
Copy link
Contributor Author

I will work on the test failures. Thanks for the pointers.

@sebinjohn
Copy link
Contributor Author

@VioletHynes I have fixed the test failures. I believe the other CI failures are unrelated to this PR.

Copy link
Contributor

@VioletHynes VioletHynes left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks a tonne for the submission :D

@VioletHynes VioletHynes dismissed marcboudreau’s stale review August 14, 2023 13:18

Dismissing, as changes have been implemented and I've been looking over this PR

@VioletHynes VioletHynes merged commit ebd4002 into hashicorp:main Aug 14, 2023
@@ -0,0 +1,3 @@
```release-note:feature
auto-auth: support ldap auth
Copy link
Collaborator

Choose a reason for hiding this comment

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

@VioletHynes or @marcboudreau would one of you be able to update this file to use the right format for a new feature (see the changelog process page) or change the section to an improvement?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll get this fixed shortly :)

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.

4 participants