-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 TLSContextWrapper to not override tls_verify #10098
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
LGTM, although I wonder why tls_verify
was originally overridden to true
whenever tls_*
were options configured.
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.
Couple of thoughts:
- This is a breaking behavioral change, a few integrations rely on this. It's not clear to me how much impact this change can have for integrations and users who are relying on this behavior. Maybe worse case scenario is that certs won't be verified anymore ?
- It still makes sense to force
tls_verify
to True iftls_ca_cert
is set. Indeed, passingtls_ca_cert
is only required for verifying a given cert based on some list of Certificat Authority. Passingtls_ca_cert
is useless if we don't plan to verify the cert.
I did that, initially because it doesn't make sense to set |
Thanks Florian, that makes sense. I've addressed this and added back the override for |
@pytest.mark.parametrize('param', ('tls_ca_cert', 'tls_cert', 'tls_private_key', 'tls_private_key_password')) | ||
def test_config_overwrite(self, param): | ||
config = {'tls_verify': False, param: 'foo'} | ||
config = {param: param} |
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.
You should keep tls_verify: False
here because the default is True.
And remove all but tls_ca_cert
from the parametrize decorator
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.
Good if tests are passing
What does this PR do?
Previously, the
TLSContextWrapper
was overriding the configuredtls_verify
value and forcing it toTrue
if any of these configs were present,tls_ca_cert
,tls_cert
,tls_private_key
,tls_private_key_password
even iftls_verify: false
is configured. This PR updates the override to only forcetls_verify: true
iftls_ca_cert
is configured.Motivation
Customer support case
Additional Notes
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attached