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.

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.

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?

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.

Note to myself: change the requires_tls_version decorator so that it works.

This test is asserting that the default TLSv1_2 cipher suite is actually being used right? I'm fine with the current test but I'd like to fortify it (which I'll do) in the future where we have a ciphersuite for TLSv1.3 and a ciphersuite for TLSv1.2 and if we use min/max versions, then we're going to be using the TLSv1.2 ciphersuite we specified.