Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL #2286
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
PYTHON-5309 Ensure AsyncMongoClient doesn't use PyOpenSSL #2286
Changes from 9 commits
efe494d
b29d1ba
d9dfb99
c847f25
ae8ecc4
03f4ba1
5349164
67100fc
88ae345
e451ceb
0312acb
4e85024
dccd96a
c86a85f
12ef993
bc76aae
a9c63c8
67c6738
3ea4de7
38ad677
c57aed2
2591169
06a710d
ef4111e
0b3c6bb
9336f58
23b7cbe
760fa97
4b8a4ed
5807ba1
683ba33
d007c5f
05c061a
350f103
5fa117f
56c9662
a7324e5
af83d81
f6b17dd
74ca8be
536f189
24354b4
4178fcc
b2324e3
17cf61d
257f8fe
74f98c5
6752a67
750a9aa
e7e36b4
8af8f09
16d3cc3
6971fed
4d12c59
a0fe2e5
7b4ae9c
f02a791
4ed055e
981a046
c20623f
bdaf87a
c2b2cc3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What's the motivation for adding this section?
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.
Oh uh, I was struggling to get it working and asked Noah a bunch of questions, so he suggested I add a section here.
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.
I'm concerned about people seeing this section and misinterpreting it to mean that they should be using tlsAllowInvalidCertificates. Can we remove it or make it very clear that this is only for local testing (using our test certificates) and not production?
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 a public api so we can't add "is_sync" here. What's the motivation for this?
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.
Oh, I added
is_sync
as a param to a few functions because inssl_support.get_ssl_context()
, we'll need to know which version of ssl should be used,, is there another way to go about this that is preferred?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, we still can not add "is_sync" here so we need to find another way.
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.
One way would be to lazily init the async SSLContext in kms_request().
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.
I think I did it? Not sure if this is what you meant, but what I have now would leave the params to AutoEncryptOpts unchanged.
I basically delayed the parse_kms_tls_options because that's where is_sync was needed. I believe i appropriately delayed the definition of kms_tls_options but let me know if I missed something. I'm not super familiar with how encryption in the driver works >.<
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.
Do these need to be separate
SSLError
andPYSSLError
types, or can we export a shared one fromssl_support
to reduce code churn?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.
Err, I tried to do something like
SSLError = (_ssl.SSLError, _pyssl.SSLError)
inssl_support
but i think it was innetwork_layer
where we do araise SSLError()
so it needed to be a specific type. Is there another way to accomplish what you're describing that would work?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.
The combined type could be here in
pool_shared.py
, as that's where it would get used.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.
We only explicitly raise
SSLError
innetwork_layer
for our socket-based Windows SSL I/O. Those will go away if we do PYTHON-5215.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.
Ahh i see, makes sense!
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.
Can we use
Union
types here to remove the need for two separate exports of everything? Something like:BLOCKING_IO_ERRORS = _ssl.BLOCKING_IO_ERRORS | _pyssl.BLOCKING_IO_ERRORS
Are there any situations where we specifically care if an exception type is from PyOpenSSL or stdlib SSL?
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.
ooo good call out. I don't think we particularly care if its pyopenssl vs stdlib ssl error.
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.
I think python3.9 didn't love the union types? so i just made them tuples. But its a similar idea.
I couldn't apply this to SSLError though because sometimes we'd raise SSLError so it needed to be one specific type.
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.
Can you expand on why the union types didn't work? We use
Union
elsewhere in our type hints.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.
hmm okay not sure what i did incorrectly last time, but it works now! sorry about that >.<
(I'm going to guess i handled
BLOCKING_IO_ERRORS
incorrectly cuz i believe that's already a tuple of types and the union of two different types of tuples was not good?)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.
Wait,, how am i supposed to do it then? I thought
Union[x, y]
was for type hints??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.
That error is from trying to use the pipe
|
operator, right? What happens when you useUnion[x, y]
instead?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.
Ah, it works! thanks!
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.
Oh, this is a runtime object, not a type hint. We should be using the same type as
ssl.BLOCKING_IO_ERRORS
(most likely a tuple or a list)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.
yeah, it didn't immediately error (as a runtime object) so i thought it was just something I didn't know about how python works HAHA but okay, changing it back to a tuple now xD
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.
Can we rename
ssl_in_use
->_ssl
that way there's less code churn.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.
I don't think so? because _ssl is referring to stdlib ssl and i don't think we'd want it to be replaced by pyopenssl just because pyopenssl is installed?
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.
It's a local variable assignment so what's the issue?
Or even just:
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.
Ah, no you're right -- I've been conditioned to think that shadowing a global var with a local var is generally bad for code readability(?) so my brain forgot that it was something we could do.
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.
Er, am i doing it incorrectly? I'm seeing the error "UnboundLocalError: local variable '_ssl' referenced before assignment"
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.
Uh, I swore I've done this before. I guess not. The only way around that is like this:
Or by renaming the global "_ssl".
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.
I actually kinda wanted to rename the global
_ssl
except i think there were some other modules that imported_ssl
fromssl_support
and then i wasn't sure if i was allowed to change it...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 know what, I think it's simpler to go back to the the local
ssl_in_use
var or just renamessl_in_use
tossl
? Apologies for the back and forth.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.
All good, went with
ssl
because its shorter.