-
Notifications
You must be signed in to change notification settings - Fork 156
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 10 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,14 @@ def open(self): | |
| six.reraise(*last_error) | ||
|
|
||
| def _create_socket(self, sock): | ||
| self.socket = ssl.wrap_socket( | ||
| purpose = ssl.Purpose.SERVER_AUTH | ||
| context = ssl.create_default_context(purpose=purpose, capath=self.ca_certs) | ||
|
||
| context.check_hostname = False | ||
|
||
| 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.
Author
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
Author
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 |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -287,17 +287,20 @@ def interrupt_handler(trigger, frame): | |||
| for cipher in auth_suite_ciphers: | ||||
| self._logger.debug(cipher) | ||||
|
|
||||
| self._socket = ssl.wrap_socket( | ||||
| purpose = ssl.Purpose.CLIENT_AUTH | ||||
| capath = self.config.settings.get('ca_path') | ||||
| context = ssl.create_default_context(purpose=purpose, capath=capath) | ||||
|
||||
| certificate = auth.get_certificate_from_connection( |
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
Outdated
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 with the client, it's worth checking here about how different combinations of not specifying certificate or key behave. For example, the old module-level wrap_socket() had a check
if server_side and not certfile:
raise ValueError("certfile must be specified for server-side "
"operations")so omitting the certificate would have been a hard 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.
Fixed
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 what I had in mind was like
if certfile:
keyfile = self.config.settings.get('key_path')
context.load_cert_chain(certfile, keyfile=keyfile)
+ else:
+ raise ValueError("certfile must be specified for server-side operations")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.
Fixed
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 instead let the context raise an appropriate error. This has the benefit of letting us mock the context behavior in the tests.
Check failure
Code scanning / CodeQL
Use of insecure SSL/TLS version
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