-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[3007.x] Fixing vault client unwrap function to respect server.verify option. #66215
Conversation
Hey @voyvodov, thanks for the submission. Would you mind writing a test case for this? |
Ah, we fixed this in the saltext, but forgot to port it here. The correct fix would be to just pass the verify option since unwrap requests do not need to be authenticated. salt-extensions/saltext-vault#32 |
But why making this different? If we don't need auth, why not just removing the headers? This why, if there is need to change something in the way requests are made, a single place will be touched, not "search-and-replace". |
On a second thought, you might be right. I don't remember the specifics of why I implemented unwrapping in this way tbh, but it was intentional at the time. Suspected it was about limiting token usage or some chicken-and-egg problem, but that doesn't hold up when reflecting on the code. This patch (necessarily) duplicates some light calls regarding header rendering ( |
Added a small test which verifies that unwrap is respecting |
This should be targeted at the |
Looks like you have some pre-commit failures |
c73fa49
to
16fa228
Compare
Fixed. I'm not sure why it wasn't catched during the last push |
@voyvodov You might want to check and see if you need to implement the same changes to the salt-extension for Vault, which has better support for Vault, see https://github.com/salt-extensions/saltext-vault. Eventually this will be the preferred solution in Salt 3008 |
It's already there. |
What does this PR do?
Currently
VaultClient.unwrap
is doing own request call without respecting verify option. Any other function is reusing self.request or self.raw_request function which are respecting correctly verify opt. This will change unwrap function to also utilize self.post() which is reusing self.request.What issues does this PR fix or reference?
Fixes: #66213
Previous Behavior
Vault module not working with self-signed certificate on disk or
verify: False
New Behavior
Vault modules works as expected.
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes