-
Couldn't load subscription status.
- Fork 155
Add python 3.12 and 3.13 support #707
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
base: master
Are you sure you want to change the base?
Changes from all commits
433ab3e
f16f36d
c90654c
61c5528
06f3a1f
22fc83c
519cbdc
ec91215
a8e44f3
eea62bd
4618b1d
c3a5dbc
5a02eb7
2897e40
cc035f4
d206639
777c70d
3da6f8d
80faaf6
452693a
4b781ff
529e123
da34f31
1b46974
8352de5
042378b
65c6993
5b736ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -285,13 +285,17 @@ def open(self): | |
| six.reraise(*last_error) | ||
|
|
||
| def _create_socket(self, sock): | ||
| self.socket = ssl.wrap_socket( | ||
| context = ssl.SSLContext(self.ssl_version) | ||
| context.verify_mode = self.cert_reqs | ||
| if self.ca_certs: | ||
| context.load_verify_locations(self.ca_certs) | ||
| if self.keyfile and not self.certfile: | ||
| raise ValueError("certfile must be specified") | ||
| if self.certfile: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm testing combinations of "forgetting" to specify if keyfile and not certfile:
raise ValueError("certfile must be specified")to prevent this. Maybe this would be good to have here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| context.load_cert_chain(self.certfile, self.keyfile) | ||
| self.socket = context.wrap_socket( | ||
Check failureCode scanning / CodeQL Use of insecure SSL/TLS version
Insecure SSL/TLS protocol version TLSv1 allowed by [call to ssl.create_default_context](1).
Insecure SSL/TLS protocol version TLSv1_1 allowed by [call to ssl.create_default_context](1).
|
||
| sock, | ||
| keyfile=self.keyfile, | ||
| certfile=self.certfile, | ||
| cert_reqs=self.cert_reqs, | ||
| ssl_version=self.ssl_version, | ||
| ca_certs=self.ca_certs, | ||
| server_side=False, | ||
| do_handshake_on_connect=self.do_handshake_on_connect, | ||
| suppress_ragged_eofs=self.suppress_ragged_eofs) | ||
| self.socket.settimeout(self.timeout) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the client, we should make sure that this change does not lose any previously working configuration settings. I see for example that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
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 no longer pays attention to the configuration settings
self.cert_reqsandself.ssl_versions. That's probably okay in practice, since the defaults are good, but these settings are documented, so they should work.One option might be to not use
ssl.create_default_context()and instead usessl.SSLContextdirectly and apply these settings. LikeThis is just about as much code as before but it reproduces the previous behavior more precisely.
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.
Done