-
Notifications
You must be signed in to change notification settings - Fork 59
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
hashi_vault - add support for none
auth type
#80
hashi_vault - add support for none
auth type
#80
Conversation
Codecov Report
@@ Coverage Diff @@
## main #80 +/- ##
==========================================
+ Coverage 82.88% 82.97% +0.08%
==========================================
Files 16 16
Lines 783 787 +4
Branches 78 78
==========================================
+ Hits 649 653 +4
Misses 120 120
Partials 14 14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So first off, I want to thank you for the submission and apologize for the delay in reviewing. I've been on vacation (still am) so I may be slower to respond than usual.
As for the general idea, I have a few thoughts.
First, I want to confirm that agent auth can't already be achieved, by using "token" auth and passing anything for the token value, like
lookup('community.hashi_vault.hashi_vault', 'secret/hello:value', auth_method='token', token='fake')
I played around with agent a little in the past and I thought this worked. I ask not because that means we don't need or want something more appropriate, but because it's good to know workarounds/alternatives.
As for your addition, the method you're adding, called "agent" is not actually doing anything specifically, it's just passing nothing. So I'm wondering if this should be handled differently, like maybe having an "auth" method called none
instead? After all it is also possible to use an agent with other auth methods, so calling this "agent" could be confusing imo.
The other major issue here is that there are no tests. I'm working hard to refactor the plugin, and to improve testing, and hopefully to make it easier to add tests for future contributions, but I'm hesitant to add new features that don't include any tests.
Related to naming, if we're calling the auth method "agent" then we're sort of saying it's expected to be used with agent, and that means we should test against agent. Since we don't yet test against a Vault agent it'll be some doing to get that going in CI.
Testing against Vault itself, and against other software (like tinyproxy
which we use to test proxy operations), can be kind of pain, and I'm working on making that experience better right now.
On the other hand, if we call this auth method none
, I think we can do with much less testing of it; basically we'd probably just against Vault as normal but we'd expect an access denied in our tests.
none
will also flow better if there are other ways of running Vault server/agent in the future that make use of no authentication.
What do you think?
Hi @briantist! Thanks for the great feedback. Upfront, if you're on vacation no need at all to respond to this now! I've got a local build and distribution system in my environment where this change is needed, so it's not blocking usage. Just wanted to try and contribute back. I like the idea of renaming the method to I'm also happy to try and help with getting any CI tests in for Agent, but I'm new to this testing framework. I'll try to dig at some docs and see if I can figure out what's needed! :) Some notes on if this is needed, unfortunately just passing in a fake token errors out when the plugin tries to validate the token.
Yields:
And explicitly disabling validation
Yields:
I did a quick dig on the Vault Agent docs and didn't find an explicit answer (it was a cursory dig), but it appears that the behavior if a token is supplied it overrides Vault Agent's credentials. Without
With an invalid token:
With a different valid token:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are looking good so far, how do you feel about trying to add the tests for the none
method based on the tests for the other auth methods? I'd probably go off the token
auth method but the difference would mainly be that you wouldn't be supplying a token and that you'd expect the calls to fail, so it should be simpler to set up.
none
auth type
873a356
to
c79451f
Compare
Hi @jlrgraham23 I took the liberty of adding tests and a changelog fragment, and making a few other test updates, hope that's ok. Could you take a look and let me know if it looks good to you? And if possible to try it out with a Vault agent as you seem to have one set up already. @Akasurde since I'm now a co-author would you be willing to give this another look so there's some additional review? |
SUMMARY
Support the use of a locally running Vault Agent process to interact with Vault. This relieves the user almost entirely have need to manage or interact with tokens.
Fixes #79
ISSUE TYPE
COMPONENT NAME
community.hashi_vault, authentication methods.
ADDITIONAL INFORMATION
More details in: #79
Also related, older issue against the main Ansible repo: ansible/ansible#60728