-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
gh-136306: Add support for SSL groups #136307
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: main
Are you sure you want to change the base?
Changes from all commits
662d66e
9acb2c3
03072c4
187ff2e
452bdec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -960,6 +960,15 @@ def test_get_ciphers(self): | |
len(intersection), 2, f"\ngot: {sorted(names)}\nexpected: {sorted(expected)}" | ||
) | ||
|
||
def test_groups(self): | ||
ctx = ssl.create_default_context() | ||
self.assertIsNone(ctx.set_groups('P-256')) | ||
self.assertIsNone(ctx.set_groups('P-256:X25519')) | ||
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. Is there a way to properly test that the groups were changed even on versions before 3.5? like changing some other parameters nd see that it doesn't fail? 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. The test where the client and server have no overlapping groups is an example of this. Even when the specific group can't be queried, the fact that the handshake succeeds when there are overlapping group values and doesn't succeed when there aren't should run on all versions of OpenSSL. It's only the additional check on the specific group chosen which would only run on OpenSSL 3.2 and later. I see this as just an added confirmation, though. |
||
|
||
if ssl.OPENSSL_VERSION_INFO >= (3, 5): | ||
self.assertNotIn('P-256', ctx.get_groups()) | ||
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. Can we create multiple contexts to check this? Like: 1 context for which we set P-256 and then check def test(self):
ctx = ...
ctx.set_groups('P-256')
# checks
ctx = ...
ctx.set_groups('P-256:X25519')
# checks 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. The values returned by ctx.get_groups() are affected only be ctx.minimum_version and ctx.maximum_version, and not by calls to ctx.set_groups(). There is a separate set of tests added in ThreadedTests.test_groups() which set the client and server contexts to different values and see what the resulting selected value is by querying SSLObject.group() after the handshake completes, including a test where the connection fails due to no overlapping groups. This is following the pattern of the test_ecdh_curve() test just above that. |
||
self.assertIn('P-256', ctx.get_groups(include_aliases=True)) | ||
|
||
def test_options(self): | ||
# Test default SSLContext options | ||
ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT) | ||
|
@@ -2701,6 +2710,8 @@ def server_params_test(client_context, server_context, indata=b"FOO\n", | |
'session_reused': s.session_reused, | ||
'session': s.session, | ||
}) | ||
if ssl.OPENSSL_VERSION_INFO >= (3, 2): | ||
stats.update({'group': s.group()}) | ||
s.close() | ||
stats['server_alpn_protocols'] = server.selected_alpn_protocols | ||
stats['server_shared_ciphers'] = server.shared_ciphers | ||
|
@@ -4126,6 +4137,38 @@ def test_ecdh_curve(self): | |
chatty=True, connectionchatty=True, | ||
sni_name=hostname) | ||
|
||
def test_groups(self): | ||
# server secp384r1, client auto | ||
client_context, server_context, hostname = testing_context() | ||
|
||
server_context.set_groups("secp384r1") | ||
server_context.minimum_version = ssl.TLSVersion.TLSv1_3 | ||
stats = server_params_test(client_context, server_context, | ||
chatty=True, connectionchatty=True, | ||
sni_name=hostname) | ||
if ssl.OPENSSL_VERSION_INFO >= (3, 2): | ||
self.assertEqual(stats['group'], "secp384r1") | ||
|
||
# server auto, client secp384r1 | ||
client_context, server_context, hostname = testing_context() | ||
client_context.set_groups("secp384r1") | ||
server_context.minimum_version = ssl.TLSVersion.TLSv1_3 | ||
stats = server_params_test(client_context, server_context, | ||
chatty=True, connectionchatty=True, | ||
sni_name=hostname) | ||
if ssl.OPENSSL_VERSION_INFO >= (3, 2): | ||
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. Let's add a constant (in the file) say 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. In this case, I think two constants would be required. The SSLObject.group() call requires 3.2 or later, but the SSLContext.get_groups() call requires 3.5 or later. |
||
self.assertEqual(stats['group'], "secp384r1") | ||
|
||
# server / client curve mismatch | ||
client_context, server_context, hostname = testing_context() | ||
client_context.set_groups("prime256v1") | ||
server_context.set_groups("secp384r1") | ||
server_context.minimum_version = ssl.TLSVersion.TLSv1_3 | ||
with self.assertRaises(ssl.SSLError): | ||
server_params_test(client_context, server_context, | ||
chatty=True, connectionchatty=True, | ||
sni_name=hostname) | ||
|
||
def test_selected_alpn_protocol(self): | ||
# selected_alpn_protocol() is None unless ALPN is used. | ||
client_context, server_context, hostname = testing_context() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
:mod:`ssl` can now get and set groups used for key agreement. | ||
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. We need a what's new entry 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. I haven't done a what's new entry before - where would I add this? Is it part of the NEWS entry or something separate? 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. It's something in Doc/whatsnew/3.15.rst. See the other entries and add an entry for the ssl module under features section (if the ssl section doesn't exist, create one). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2142,6 +2142,35 @@ _ssl__SSLSocket_cipher_impl(PySSLSocket *self) | |
return cipher_to_tuple(current); | ||
} | ||
|
||
/*[clinic input] | ||
@critical_section | ||
_ssl._SSLSocket.group | ||
[clinic start generated code]*/ | ||
|
||
static PyObject * | ||
_ssl__SSLSocket_group_impl(PySSLSocket *self) | ||
/*[clinic end generated code: output=9c168ee877017b95 input=5f187d8bf0d433b7]*/ | ||
{ | ||
#if OPENSSL_VERSION_NUMBER >= 0x30200000L | ||
const char *group_name; | ||
|
||
if (self->ssl == NULL) { | ||
Py_RETURN_NONE; | ||
} | ||
|
||
group_name = SSL_get0_group_name(self->ssl); | ||
if (group_name == NULL) { | ||
Py_RETURN_NONE; | ||
} | ||
|
||
return PyUnicode_DecodeFSDefault(group_name); | ||
#else | ||
PyErr_SetString(PyExc_NotImplementedError, | ||
"Getting selected group requires OpenSSL 3.2 or later."); | ||
return NULL; | ||
#endif | ||
} | ||
|
||
/*[clinic input] | ||
@critical_section | ||
_ssl._SSLSocket.version | ||
|
@@ -3023,6 +3052,7 @@ static PyMethodDef PySSLMethods[] = { | |
_SSL__SSLSOCKET_GETPEERCERT_METHODDEF | ||
_SSL__SSLSOCKET_GET_CHANNEL_BINDING_METHODDEF | ||
_SSL__SSLSOCKET_CIPHER_METHODDEF | ||
_SSL__SSLSOCKET_GROUP_METHODDEF | ||
_SSL__SSLSOCKET_SHARED_CIPHERS_METHODDEF | ||
_SSL__SSLSOCKET_VERSION_METHODDEF | ||
_SSL__SSLSOCKET_SELECTED_ALPN_PROTOCOL_METHODDEF | ||
|
@@ -3402,6 +3432,73 @@ _ssl__SSLContext_get_ciphers_impl(PySSLContext *self) | |
|
||
} | ||
|
||
/*[clinic input] | ||
@critical_section | ||
_ssl._SSLContext.set_groups | ||
grouplist: str | ||
/ | ||
[clinic start generated code]*/ | ||
|
||
static PyObject * | ||
_ssl__SSLContext_set_groups_impl(PySSLContext *self, const char *grouplist) | ||
/*[clinic end generated code: output=0b5d05dfd371ffd0 input=2cc64cef21930741]*/ | ||
{ | ||
if (!SSL_CTX_set1_groups_list(self->ctx, grouplist)) { | ||
_setSSLError(get_state_ctx(self), "unrecognized group", 0, __FILE__, __LINE__); | ||
return NULL; | ||
} | ||
Py_RETURN_NONE; | ||
} | ||
|
||
/*[clinic input] | ||
@critical_section | ||
_ssl._SSLContext.get_groups | ||
* | ||
include_aliases: bool = False | ||
[clinic start generated code]*/ | ||
|
||
static PyObject * | ||
_ssl__SSLContext_get_groups_impl(PySSLContext *self, int include_aliases) | ||
/*[clinic end generated code: output=6d6209dd1051529b input=3e8ee5deb277dcc5]*/ | ||
{ | ||
#if OPENSSL_VERSION_NUMBER >= 0x30500000L | ||
STACK_OF(OPENSSL_CSTRING) *groups; | ||
const char *group; | ||
size_t i, num; | ||
PyObject *result = NULL; | ||
|
||
if ((groups = sk_OPENSSL_CSTRING_new_null()) == NULL) { | ||
_setSSLError(get_state_ctx(self), "Can't allocate stack", 0, __FILE__, __LINE__); | ||
return NULL; | ||
} | ||
|
||
if (!SSL_CTX_get0_implemented_groups(self->ctx, include_aliases, groups)) { | ||
_setSSLError(get_state_ctx(self), "Can't get groups", 0, __FILE__, __LINE__); | ||
sk_OPENSSL_CSTRING_free(groups); | ||
return NULL; | ||
} | ||
|
||
num = sk_OPENSSL_CSTRING_num(groups); | ||
result = PyList_New(num); | ||
if (result == NULL) { | ||
_setSSLError(get_state_ctx(self), "Can't allocate list", 0, __FILE__, __LINE__); | ||
sk_OPENSSL_CSTRING_free(groups); | ||
return NULL; | ||
} | ||
|
||
for (i = 0; i < num; ++i) { | ||
group = sk_OPENSSL_CSTRING_value(groups, i); | ||
PyList_SET_ITEM(result, i, PyUnicode_DecodeFSDefault(group)); | ||
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. This call can fail (decode call) so you need to handle the failure 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. The values returned here are constant strings coming from OpenSSL, and they should always be plain ASCII. So, the decode should always succeed in this case. I can put in a check here, but it would effectively be dead code unless there was some kind of bug in OpenSSL. The function _ssl__SSLSocket_compression_impl contains another example of this. 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. We need it if Python runs out of memory unfortunately. It comverts a const char* to a PyObject* so we need such guards. |
||
} | ||
|
||
sk_OPENSSL_CSTRING_free(groups); | ||
return result; | ||
#else | ||
PyErr_SetString(PyExc_NotImplementedError, | ||
"Getting implemented groups requires OpenSSL 3.5 or later."); | ||
return NULL; | ||
#endif | ||
} | ||
|
||
static int | ||
do_protocol_selection(int alpn, unsigned char **out, unsigned char *outlen, | ||
|
@@ -5249,6 +5346,7 @@ static struct PyMethodDef context_methods[] = { | |
_SSL__SSLCONTEXT__WRAP_SOCKET_METHODDEF | ||
_SSL__SSLCONTEXT__WRAP_BIO_METHODDEF | ||
_SSL__SSLCONTEXT_SET_CIPHERS_METHODDEF | ||
_SSL__SSLCONTEXT_SET_GROUPS_METHODDEF | ||
_SSL__SSLCONTEXT__SET_ALPN_PROTOCOLS_METHODDEF | ||
_SSL__SSLCONTEXT_LOAD_CERT_CHAIN_METHODDEF | ||
_SSL__SSLCONTEXT_LOAD_DH_PARAMS_METHODDEF | ||
|
@@ -5259,6 +5357,7 @@ static struct PyMethodDef context_methods[] = { | |
_SSL__SSLCONTEXT_CERT_STORE_STATS_METHODDEF | ||
_SSL__SSLCONTEXT_GET_CA_CERTS_METHODDEF | ||
_SSL__SSLCONTEXT_GET_CIPHERS_METHODDEF | ||
_SSL__SSLCONTEXT_GET_GROUPS_METHODDEF | ||
_SSL__SSLCONTEXT_SET_PSK_CLIENT_CALLBACK_METHODDEF | ||
_SSL__SSLCONTEXT_SET_PSK_SERVER_CALLBACK_METHODDEF | ||
{NULL, NULL} /* sentinel */ | ||
|
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.