Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is an initial implementation of the feature proposed in issuepython#136306.
picnixz left a comment
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.
A first round of comments
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Looks like the indentation is required. Got a doc build error without it.
picnixz left a comment
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.
Some other comments (sorry I'm on mobile so it's hard)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ronf commentedJul 5, 2025
I'm not sure why the latest round of tests failed - The only change in this last commit was a doc file change, so I'm guessing it's a transient CI failure. Is there a way to trigger it to retry? |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
picnixz commentedJul 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
You can make an empty commit or ask a triager to rerun the CI (but there is none apart from me that is watching the PR I think) |
picnixz left a comment
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 looks great. I was planning to add some PQ support to Python now that ML-KEM/DEM were standardized but this is a good start as well. I'll have a look at the tests when I'm on my laptop again (Friday) but you can consider this PR to be approved unless I find something by then.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ronf commentedJul 7, 2025
Thanks very much! There seems to be less urgency in providing support for ML-DSA and SLH-DSA for authentication than there was for ML-KEM for key agreement mainly because of the "capture now, decrypt later" concerns, but I'd be happy to contribute to an effort around supporting those as well. I'd actually like to learn more about what OpenSSL APIs actually need to change here. |
picnixz commentedJul 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I am currently modernizing the use of OpenSSL HMAC in hashlib and I actually wondered whether we used APIs that were deprecated in 3.0. If you want, you can help me here, at least for the SSL module (I'd prefer that we work on separate modules to avoid merge conflicts), namely look at the calls we make to OpenSSL and check if there are deprecated calls in 3.x. If there are, open an issue and a PR that modernize such calls (for HMAC, we moved from using the old HMAC_* interface to the more generic EVP_MAC interface) |
ronf commentedJul 7, 2025
I don't know if I'll have the cycles to actually take on fixing all the issues that we find, but I gave this a quick look to get a sense for the scope of the problem. There aren't as many changes as I was expecting to find, but it's still fairly complicated if we want to continue to support all the way back to OpenSSL 1.1.1 while avoiding functions deprecated in OpenSSL 3.x. In many cases, the old API calls still exist in 3.x but the new API will only work on 3.x. I'm thinking leaving the old calls in place may be our best bet for now, to avoid conditional compilation. An example is replacing all references to BIO_new() with BIO_new_ex() where we pass in an explicit NULL argument for the library context. It doesn't seem worth a #if everywhere that appears as long as 3.x continues to provide a wrapper for the old BIO_new() call. A similar issue shows up around one use of BIO_new_mem_buf(). Here are some of the items I've found:
|
ronf commentedJul 8, 2025
Following up on my last comment: In the SSL module docs, there's a "TLS 1.3" section which actually mentions that |
picnixz commentedJul 8, 2025
Helping fixing those issues or modernizing API calls is appreciated so choose what you want! |
ronf commentedJul 16, 2025
I just noticed that I left off the "gh-136306" off of my last commit. Should I amend and force push to fix that, or is it ok to proceed without that? Is there anything else you'd like me to change? |
picnixz left a comment
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 all my comments got properly addressed. Since we're currently doing sprints at EuroPython, I hope that@gpshead can take a quick look at it. Otherwise, I'll merge it before leaving on Tuesday (I actually tried to find the PR again but I couldn't so it took me a while, sorry!)
picnixz commentedJul 19, 2025
Ah, you need to regenerate the files as there was a conflict created by something. |
picnixz commentedJul 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In general, we don't force push commits because we squash everything at the end (and usually I rewrite the commit message as well to remove some commits messages that are not necessary). It's also better not to force-push as it makes incremental review harder. I've left a comment somewhere saying when it's fine to force-push but I never find it again when I need to... |
ronf commentedJul 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Scratch that - I just saw your other message about the conflict. I'll look into it. |
ronf commentedJul 24, 2025
@picnixz, sorry to bother you, but any update on this? |
picnixz commentedJul 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I wanted@gpshead to have a look. On my side I accept it. It will anyway be merged into 3.15 because 3.14 is feature-locked since May. |
ronf commentedJul 25, 2025
Ok, thanks. I understand about the target being 3.15. I just had some other Python SSL changes I wanted to get started on (TLS 1.3 support for cipher suites and signature algs), and I was hoping to do them consecutively to avoid conflicts. |
picnixz commentedJul 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ah I see. Ok I'll take the responsibility for this one. Ping me on Sunday if nothing has been done until now (I think Greg's actually EuroPython) |
377b787 intopython:mainUh oh!
There was an error while loading.Please reload this page.
picnixz commentedJul 28, 2025
Thank you for this feature! I've merged it so that you aren't blocked anymore. We'll fix further issues when they appear. |
ronf commentedJul 29, 2025
Thanks very much! When the changes are ready, I'll create a new issue and PR for TLS 1.3 cipher suite support (SSL_CTX_set_ciphersuites). |
devdanzin commentedJul 29, 2025
This seems to have broken a special JIT build that was working before. Sharing here in case the issue is obvious to you. Here's the diff I apply before building: index 8b7f12bf03d..329b1f615e3 100644--- a/Include/internal/pycore_optimizer.h+++ b/Include/internal/pycore_optimizer.h@@ -116,12 +116,12 @@ PyAPI_FUNC(void) _Py_Executors_InvalidateCold(PyInterpreterState *interp); // Used as the threshold to trigger executor invalidation when // trace_run_counter is greater than this value.-#define JIT_CLEANUP_THRESHOLD 100000+#define JIT_CLEANUP_THRESHOLD 10000 // This is the length of the trace we project initially. #define UOP_MAX_TRACE_LENGTH 800-#define TRACE_STACK_SIZE 5+#define TRACE_STACK_SIZE 10 int _Py_uop_analyze_and_optimize(_PyInterpreterFrame *frame, _PyUOpInstruction *trace, int trace_len, int curr_stackentries,@@ -152,7 +152,7 @@ static inline uint16_t uop_get_error_target(const _PyUOpInstruction *inst) } // Holds locals, stack, locals, stack ... co_consts (in that order)-#define MAX_ABSTRACT_INTERP_SIZE 4096+#define MAX_ABSTRACT_INTERP_SIZE 8192 #define TY_ARENA_SIZE (UOP_MAX_TRACE_LENGTH * 5)@@ -163,7 +163,7 @@ static inline uint16_t uop_get_error_target(const _PyUOpInstruction *inst) // progress (and inserting a new ENTER_EXECUTOR instruction). In practice, this // is the "maximum amount of polymorphism" that an isolated trace tree can // handle before rejoining the rest of the program.-#define MAX_CHAIN_DEPTH 4+#define MAX_CHAIN_DEPTH 16 /* Symbols */ /* See explanation in optimizer_symbols.c */diff --git a/Python/optimizer.c b/Python/optimizer.cindex 8d01d605ef4..33c00169a60 100644--- a/Python/optimizer.c+++ b/Python/optimizer.c@@ -456,7 +456,7 @@ BRANCH_TO_GUARD[4][2] = { #define CONFIDENCE_RANGE 1000-#define CONFIDENCE_CUTOFF 333+#define CONFIDENCE_CUTOFF 100 #ifdef Py_DEBUG #define DPRINTF(level, ...) \ And here's the segfault it gets on start: |
picnixz commentedJul 29, 2025
Could you try to remove all three functions and check which one could be the culprit please? also which version of OpenSSL are you using? |
picnixz commentedJul 29, 2025
The issue seems to be in some string handling. Maybe the FSDecode call? |
devdanzin commentedJul 29, 2025
|
devdanzin commentedJul 29, 2025
It seems removing all functions doesn't stop the segfault. AFAICT the issue is related to index 454c8dde031..9e21c41421a 100644--- a/Include/internal/pycore_backoff.h+++ b/Include/internal/pycore_backoff.h@@ -99,8 +99,8 @@ backoff_counter_triggers(_Py_BackoffCounter counter) // Must be larger than ADAPTIVE_COOLDOWN_VALUE, otherwise when JIT code is // invalidated we may construct a new trace before the bytecode has properly // re-specialized:-#define JUMP_BACKWARD_INITIAL_VALUE 4095-#define JUMP_BACKWARD_INITIAL_BACKOFF 12+#define JUMP_BACKWARD_INITIAL_VALUE 63+#define JUMP_BACKWARD_INITIAL_BACKOFF 6 static inline _Py_BackoffCounter initial_jump_backoff_counter(void) {@@ -112,8 +112,8 @@ initial_jump_backoff_counter(void) * Must be larger than ADAPTIVE_COOLDOWN_VALUE, * otherwise when a side exit warms up we may construct * a new trace before the Tier 1 code has properly re-specialized. */-#define SIDE_EXIT_INITIAL_VALUE 4095-#define SIDE_EXIT_INITIAL_BACKOFF 12+#define SIDE_EXIT_INITIAL_VALUE 63+#define SIDE_EXIT_INITIAL_BACKOFF 6 static inline _Py_BackoffCounter initial_temperature_backoff_counter(void)diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.hindex 493377b4c25..5e7dda3a371 100644--- a/Include/internal/pycore_global_objects_fini_generated.h+++ b/Include/internal/pycore_global_objects_fini_generated.h@@ -1005,6 +1005,7 @@ _PyStaticObjects_CheckRefcnt(PyInterpreterState *interp) { _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(imag)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(importlib)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(in_fd));+ _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(include_aliases)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(incoming)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(index)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(indexgroup));diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.hindex 5dfea2f479d..6908cbf78f3 100644--- a/Include/internal/pycore_global_strings.h+++ b/Include/internal/pycore_global_strings.h@@ -496,6 +496,7 @@ struct _Py_global_strings { STRUCT_FOR_ID(imag) STRUCT_FOR_ID(importlib) STRUCT_FOR_ID(in_fd)+ STRUCT_FOR_ID(include_aliases) STRUCT_FOR_ID(incoming) STRUCT_FOR_ID(index) STRUCT_FOR_ID(indexgroup)diff --git a/Include/internal/pycore_optimizer.h b/Include/internal/pycore_optimizer.hindex 8b7f12bf03d..329b1f615e3 100644--- a/Include/internal/pycore_optimizer.h+++ b/Include/internal/pycore_optimizer.h@@ -116,12 +116,12 @@ PyAPI_FUNC(void) _Py_Executors_InvalidateCold(PyInterpreterState *interp); // Used as the threshold to trigger executor invalidation when // trace_run_counter is greater than this value.-#define JIT_CLEANUP_THRESHOLD 100000+#define JIT_CLEANUP_THRESHOLD 10000 // This is the length of the trace we project initially. #define UOP_MAX_TRACE_LENGTH 800-#define TRACE_STACK_SIZE 5+#define TRACE_STACK_SIZE 10 int _Py_uop_analyze_and_optimize(_PyInterpreterFrame *frame, _PyUOpInstruction *trace, int trace_len, int curr_stackentries,@@ -152,7 +152,7 @@ static inline uint16_t uop_get_error_target(const _PyUOpInstruction *inst) } // Holds locals, stack, locals, stack ... co_consts (in that order)-#define MAX_ABSTRACT_INTERP_SIZE 4096+#define MAX_ABSTRACT_INTERP_SIZE 8192 #define TY_ARENA_SIZE (UOP_MAX_TRACE_LENGTH * 5)@@ -163,7 +163,7 @@ static inline uint16_t uop_get_error_target(const _PyUOpInstruction *inst) // progress (and inserting a new ENTER_EXECUTOR instruction). In practice, this // is the "maximum amount of polymorphism" that an isolated trace tree can // handle before rejoining the rest of the program.-#define MAX_CHAIN_DEPTH 4+#define MAX_CHAIN_DEPTH 16 /* Symbols */ /* See explanation in optimizer_symbols.c */diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.hindex 85ced09d29d..da2ed7422c9 100644--- a/Include/internal/pycore_runtime_init_generated.h+++ b/Include/internal/pycore_runtime_init_generated.h@@ -1003,6 +1003,7 @@ extern "C" { INIT_ID(imag), \ INIT_ID(importlib), \ INIT_ID(in_fd), \+ INIT_ID(include_aliases), \ INIT_ID(incoming), \ INIT_ID(index), \ INIT_ID(indexgroup), \diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.hindex 6018d98d156..b1f411945e7 100644--- a/Include/internal/pycore_unicodeobject_generated.h+++ b/Include/internal/pycore_unicodeobject_generated.h@@ -1772,6 +1772,10 @@ _PyUnicode_InitStaticStrings(PyInterpreterState *interp) { _PyUnicode_InternStatic(interp, &string); assert(_PyUnicode_CheckConsistency(string, 1)); assert(PyUnicode_GET_LENGTH(string) != 1);+ string = &_Py_ID(include_aliases);+ _PyUnicode_InternStatic(interp, &string);+ assert(_PyUnicode_CheckConsistency(string, 1));+ assert(PyUnicode_GET_LENGTH(string) != 1); string = &_Py_ID(incoming); _PyUnicode_InternStatic(interp, &string); assert(_PyUnicode_CheckConsistency(string, 1));diff --git a/Python/optimizer.c b/Python/optimizer.cindex 8d01d605ef4..33c00169a60 100644--- a/Python/optimizer.c+++ b/Python/optimizer.c@@ -456,7 +456,7 @@ BRANCH_TO_GUARD[4][2] = { #define CONFIDENCE_RANGE 1000-#define CONFIDENCE_CUTOFF 333+#define CONFIDENCE_CUTOFF 100 #ifdef Py_DEBUG #define DPRINTF(level, ...) \ |
picnixz commentedAug 4, 2025
We're having a failure on CentOS. This is a bit annoying as it should eb a stable buildbot but I think it's because of openssl. |
picnixz commentedAug 4, 2025
Mmh, the issue is flaky. I'll see if this still happens on main. |
picnixz commentedAug 4, 2025
Yeah I can actually confirm that the CentOS builtbot is actually down since this PR:https://buildbot.python.org/#/builders/828. |
…on#136307)Add support for getting and setting groups used for key agreement.* `ssl.SSLSocket.group()` returns the name of the group used for the key agreement of the current session establishment. This feature requires Python to be built with OpenSSL 3.2 or later.* `ssl.SSLContext.get_groups()` returns the list of names of groups that are compatible with the TLS version of the current context. This feature requires Python to be built with OpenSSL 3.5 or later.* `ssl.SSLContext.set_groups()` sets the groups allowed for key agreement for sockets created with this context. This feature is always supported.
Uh oh!
There was an error while loading.Please reload this page.
This is an initial implementation of the feature proposed in issue#136306.
📚 Documentation preview 📚:https://cpython-previews--136307.org.readthedocs.build/