-
Notifications
You must be signed in to change notification settings - Fork 524
SNOW-2176203: Support intermediates in trust stores #2520
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
Conversation
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.
Thanks!
LGTM - though it has been many years since I wrote production Python.
| try: | ||
| ctx.verify_mode = ssl.CERT_REQUIRED | ||
| except Exception: | ||
| pass |
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.
optional: According to the documentation, PROTOCOL_TLS_CLIENT enables CERT_REQUIRED and check_hostname by default.
https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLS_CLIENT
I guess I would either be explicit with both options or maybe drop this addtional code?
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.
As it is today, we do hostname verification in urllib3 rather than the SSLContext. (src/snowflake/connector/vendored/urllib3/util/ssl_.py). We might want to change that, but one thing at a time I think. This is another stdlib vs PyOpenSSL distinction. The context here just maps to the OpenSSL method used; we could probably get away with just PROTOCOL_TLS. They both map to OpenSSL.SSL.SSLv23_METHOD.
Yes, this is janky.
| from OpenSSL import crypto as _crypto | ||
|
|
||
| if hasattr(_crypto, "X509StoreFlags") and hasattr( | ||
| _crypto.X509StoreFlags, "PARTIAL_CHAIN" | ||
| ): | ||
| store.set_flags(_crypto.X509StoreFlags.PARTIAL_CHAIN) |
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.
Note/optional: There is an ssl.VERIFY_X509_PARTIAL_CHAIN which can be set on the SSLContext directly under SSLContext.verify_flags I think? (Version 3.10)
It might simplify some of this code. My apologies if I missed something.
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.
This is PyOpenSSL versus the stdlib. We're working with pretty old code here, so I try all the things :-).
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.
Left two nits
Co-authored-by: Patryk Czajka <[email protected]>
Co-authored-by: Patryk Czajka <[email protected]>
Our current python connector does not support a configuration with intermediate certificates in a trust store as roots of trust. This is allowed by RFCs; the root does not need to be self signed, and this is the behavior we have in our Go client.
These changes enable partial chain validation to normalize behavior across clients.