gh-136306: Add support for SSL groups #136307
Conversation
This is an initial implementation of the feature proposed in issue python#136306.
Looks like the indentation is required. Got a doc build error without it.
|
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? |
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) |
There was a problem hiding this comment.
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.
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. |
|
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) |
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:
|
|
Following up on my last comment: In the SSL module docs, there's a "TLS 1.3" section which actually mentions that |
|
Helping fixing those issues or modernizing API calls is appreciated so choose what you want! |
|
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? |
There was a problem hiding this comment.
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!)
|
Ah, you need to regenerate the files as there was a conflict created by something. |
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... |
Scratch that - I just saw your other message about the conflict. I'll look into it. |
|
@picnixz, sorry to bother you, but any update on this? |
|
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. |
|
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. |
|
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) |
|
Thank you for this feature! I've merged it so that you aren't blocked anymore. We'll fix further issues when they appear. |
|
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). |
|
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.c
index 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: |
|
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? |
|
The issue seems to be in some string handling. Maybe the FSDecode call? |
|
|
|
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.h
index 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.h
index 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.h
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/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h
index 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.h
index 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.c
index 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, ...) \ |
|
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. |
|
Mmh, the issue is flaky. I'll see if this still happens on main. |
|
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.
This is an initial implementation of the feature proposed in issue #136306.
📚 Documentation preview 📚: https://cpython-previews--136307.org.readthedocs.build/