We shouldn't remove this without understanding which OpenSSL versions it was referring to and whether they are still in use by CPython builds (1.1.x-ish API'd AWS-LC at a minimum, otherwise OpenSSL 3.0+).

The clearing of the error queue is still happening here. I just took advantage of an existing helper function setSSLError to take care of this:

static PyObject *
_setSSLError (_sslmodulestate *state, const char *errstr, int errcode, const char *filename, int lineno)
{
    if (errstr == NULL)
        errcode = ERR_peek_last_error();
    else
        errcode = 0;
    fill_and_set_sslerror(state, NULL, state->PySSLErrorObject, errcode, errstr, lineno, errcode);
    ERR_clear_error();
    return NULL;
}

Let's keep it that way and change it in a follow-up PR instead. Actually, the errcode parameter for _setSSLError is redundant as it's always overwritten.

What do you think about changing the text here to something like:

* Added new method :meth:`ssl.SSLContext.set_ciphersuites` for setting TLS 1.3
  ciphers. For TLS 1.2 or earlier, :meth:`ssl.SSLContext.set_ciphers` should
  continue to be used. Both calls can be made on the same context and the
  selected cipher suite will depend on the TLS version negotiated when a
  connection is made.

I wanted to somehow capture that these calls aren't mutually exclusive if you don't know in advance what version of TLS will end up being used. The existing call only affects cipher suites chosen when TLS 1.2 or earlier is negotiated, and the new call only affects cipher suites chosen when TLS 1.3 is negotiated.

@picnixz, are you ok with this new wording?

I've checked in the new wording I proposed. Let me know if you want any changes here.

I think this change may be ready to go if there are no other review comments.

Sorry, I forgot about this PR and I missed the notification I think. Let me have a look again.

You can also update the other note. I never saw that we had a missing capital letter.

Fixed both instances.

You don't need to update the entry just above. We'll clean it up in a PR for #133879.

Just for the future, but how about adding max_version as well? and put min_version and max_version on their own lines? even if we don't use it, I think it's fine to add it just in case.

The current version of test_wrap_socket() only lets you specify min_version for now. It would be easy to add max, but it's called from 35 places. Would you change all of these to set both min & max?

The defaults for min/max version are special symbols MININUM_SUPPORTED and MAXIMUM_SUPPORTED, so there's no need to set the maximum unless you want to forcibly restrict it (in the future) to not using a TLS version greater than 1.3. I'm not sure I see a good reason to do that here, at least not yet. It would really come down to whether TLS 1.4 did something to yet again change the method used to set cipher suites, which I'm hoping won't be the case.

Actually, it was more to ease checks in internal code paths.

Sorry, I'm not sure what you mean.

My suggestion was:

def test_wrap_socket(sock, *,
                     cert_reqs=ssl.CERT_NONE, ca_certs=None,
                     ciphers=None, ciphersuites=None,
                     min_version=None, max_version=None,
                     ...):

    ...
    if min_version is not None:
        context.minimum_version = min_version
    if max_version is not None:
        context.maximum_version = max_version
    ...

That way, we can set maximum_version in tests if needed. For now, we don't but in the future we could. For instance, a test where we call set_ciphersuites but with context.maximum_version = ssl.TLSVersion.TLSv1_2. Or is the maximum version actually ignored?

This could raise so we should be careful (for weird build options, I think it's possible to disable TLS_AES_256_GCM_SHA384). I would suggest only calling set_ciphersuites in tests that need it (that is those decorated by requires_tls_version. Otherwise the server_context would unconditoinally have this ciphersuite even if a test doesn't need it.

I must admit I didn't love that change, but the problem here is in testing the mismatched ciphers case. I we leave the server accepting all ciphers, then there's no way to trigger a mismatched cipher, and I think the server is set up only once and then re-used for multiple test methods.

This mismatch test could be broken into its own test class so that we only set client and server cipher suites for that one specific test, but it would mean adding more boiler plate to set up the server in two different classes.

I felt pretty comfortable letting it be shared as this change doesn't impact the set of TLS 1.2 cipher suites used by any of these tests (either client or server) and any existing code would not be attempting to set any TLS 1.3 ciphers as that functionality didn't exist. Some of those tests might use TLS 1.3 today based on the defaults set but changing the server side to a specific cipher will still allow TLS 1.3 to be used, since the client-side would not be restricting TLS 1.3 ciphers.

That said, if you'd prefer I replicate the server setup to restrict this, I'm happy to do so.

As for the specific cipher I chose, I tried to pick the most "vanilla" one I could. For a test like this to exist, we're going to have to pick SOMETHING on the client and something else on the server to create the mismatch. I could try to do something like a get_ciphers(), filtering on TLS 1.3, and then pick two different values from that list, but it would complicate the test. For the unit testing, do you expect any of the test environments to disable these cipher suites? I see other test code hardcoding ciphers like these.

and I think the server is set up only once and then re-used for multiple test methods.

The server context is new for every test though? setUp() is called before every test method. If you want to ease your life, just have different test methods, each of them will have their own server_context that is independent.

I was thinking of setUpClass() instead of setUp(), which only runs once for all tests in a class. However, even with setUp() running before each test, I'd still need to somehow pass into setUp() whether to restrict the ciphers on a test-by-test basis, and I'm not sure how to do that.

Since there's not a lot of setup code, I'm going to see what it looks like if I add a new class, with its own separate setUp() function, used only for this mismatch test. That'll minimize changes made to the existing test code.

Ah yes, I see the issue. It's because we create the server before and changing its context afterwards wouldn't help right?

Yes, that's right.

Ok - I've checked in a new version of the test cases which uses its own class, reverting the changes to the previously modified class. This version also attempts to dynamically select ciphers from the list of available ciphers on the system. It assumes at least one cipher is available when the TLS 1.3 enabled check passes, but it will skip the mismatched cipher test if there's only one available TLS 1.3 cipher.

This needs a .. versionadded:: next (sorry I forgot about it).

Note to myself: I would have added a linebreak after this sentence to separate from "If no cipher..." but I think the entire page needs some edits, so let's not spend too much time on wording for now.

In this paragraph, I would say "The XXX method" instead of "The method XXX".

For posterity: I really hate that OpenSSL named it SSL_CTX_set_ciphersuites for TLS 1.3 and later, and SSL_CTX_set_cipher_list for TLS 1.2 and below. I hope that this won't be annoying to users.

I was surprised what they did here at first and I'm not a big fan about the naming chosen, but I can see why they didn't want to try and extend the existing command. It allows all kinds of partial matches on the five individual elements that make up a cipher suite in TLS 1.2 and earlier, plus other special keywords like LOW/MEDIUM/HIGH. All of that is gone with TLS 1.3, and the new function is much simpler and only supports an exact match against the very small number of cipher suites defined in TLS 1.3.

I don't really know why they didn't include "_list" in the name of the OpenSSL function, given that this seems to be the convention for other such calls that take a string argument.

The good news here is that things like post-quantum crypto are going to drive migration to TLS 1.3, and at some point setting TLS 1.2 cipher suites will no longer matter. At that point, the previous set_ciphers() can be deprecated and eventually removed, with only set_ciphersuites() remaining.

My suggestion was:

def test_wrap_socket(sock, *,
                     cert_reqs=ssl.CERT_NONE, ca_certs=None,
                     ciphers=None, ciphersuites=None,
                     min_version=None, max_version=None,
                     ...):

    ...
    if min_version is not None:
        context.minimum_version = min_version
    if max_version is not None:
        context.maximum_version = max_version
    ...

That way, we can set maximum_version in tests if needed. For now, we don't but in the future we could. For instance, a test where we call set_ciphersuites but with context.maximum_version = ssl.TLSVersion.TLSv1_2. Or is the maximum version actually ignored?

I think we should make a separate test method for this one, otherwise the entire test is marked as skipped (but the two first checks must unconditionally pass). You can also swap the if condition:

def test_mismatched_ciphersuites(self):
    if self.matching_cipher == self.mismatched_cipher:
        self.skipTest("only one TLS 1.3 cipher is available")
    ...

matching_client should be matching_cipher right?

Let's keep it that way and change it in a follow-up PR instead. Actually, the errcode parameter for _setSSLError is redundant as it's always overwritten.