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

fix validate_certs parameter beeing ignored when used with the retries parameter #462

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

argetlam-coder
Copy link

SUMMARY

I run a local vault instance with a self-signed certificate. For this reason I have set the validate_certs parameter to False. However, when I then used the vault_kv2_get module or lookup plugin in combination with the retries parameter, the query still failed because the certificate validation was performed.
This was because the validate property of the retry request session was not set correctly. I fixed this error with this pull request.

Fixes #461

ISSUE TYPE
  • Bugfix Pull Request

changelogs/fragments/462-validate-certs-fix.yml Outdated Show resolved Hide resolved
@@ -108,7 +108,7 @@ def _filter(k, v):

retry_action = hvopts.pop('retry_action')
if 'retries' in hvopts:
hvopts['session'] = self._get_custom_requests_session(new_callback=self._retry_callback_generator(retry_action), **hvopts.pop('retries'))
hvopts['session'] = self._get_custom_requests_session(new_callback=self._retry_callback_generator(retry_action), conopt_verify=self._conopt_verify, **hvopts.pop('retries'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm debating whether we should bother passing this as parameter. Instead, we can make the original call here, and then set hvopts['session'].verify = self._conopt_verify. It feels like that may be the correct place to do this. Let me know if you object.

Copy link
Author

Choose a reason for hiding this comment

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

We can do it the way you suggested. I actually had it that way in my first draft, but then I thought it wouldn't match the rest of the style of the code and changed it.
Should I change it?

@briantist briantist added the bug Something isn't working label Dec 14, 2024
@briantist briantist added this to the v6.2.1 milestone Dec 14, 2024
@briantist briantist self-assigned this Dec 14, 2024
@briantist
Copy link
Collaborator

if possible, it would be great to introduce a test that would have caught this as well

@argetlam-coder
Copy link
Author

if possible, it would be great to introduce a test that would have caught this as well

I can try to write a test for it. I just need to have a quick look at how it works, as I normally don't program in Python.

…retries and modified the corresponding unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot disable certificate validation when using vault_kv2_get lookup
2 participants