Update invalid EC key test for compatibility with upcoming OpenSSL changes

[romen]

This is a rebase of #7829 to target main rather than 38.0.x, as requested.

The original description:

One of the tests checking behavior with invalid EC keys hardcoded the error reason.

This commit replaces the string matching with a regex to match both the current string and a new reason, introduced by upcoming OpenSSL changes 0, which would otherwise trigger a false positive failure.

---

Asking for help

Ping @alex, who has been helping me since #7829 (thanks!)

Ping @levitte and @mspncp who also might lend me a hand (or four) in figuring out how to root cause the remaining errors. This PR is rebased on latest main@pyca and includes formatting amendments to my original patch to appease flake, but it does still trigger the same errors that i mentioned in openssl/openssl#19681 (comment). I also include again a discussion of the same log.txt below.

---

Surprising errors specific to main@pyca/cryptography

• 📎 log.txt

The log above is obtained running test_external_pyca from the tip of openssl/openssl#19681, after ensuring the submodule pyca-cryptography points to an older (but equivalent) version of this PR.

These errors are not triggered when checking out openssl-3.1 and the same commit under pyca-cryptography/, so it seems they are an indirect consequence of the openssl/openssl#19681 changes.

The errors are due to a param of incompatible type left on the error stack during teardown, but are surprising and hard to debug because they happen during the teardown of seemingly unrelated tests, e.g. :

$ cat log.txt |grep "ERROR at" |cut -c -110
_ ERROR at teardown of TestDSASerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-s] _
_ ERROR at teardown of TestDSASerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-longerpasswor
_ ERROR at teardown of TestDSASerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-!*$&(@#$*&($T
_ ERROR at teardown of TestDSASerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-\x01\x01\x01\
_ ERROR at teardown of TestEd25519Signing.test_round_trip_private_serialization[Encoding.PEM-PrivateFormat.PKC
_ ERROR at teardown of TestEd448Signing.test_round_trip_private_serialization[Encoding.PEM-PrivateFormat.PKCS8
_ ERROR at teardown of TestECSerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-s] _
_ ERROR at teardown of TestECSerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-longerpassword
_ ERROR at teardown of TestECSerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-!*$&(@#$*&($T@
_ ERROR at teardown of TestECSerialization.test_private_bytes_encrypted_pem[PrivateFormat.PKCS8-\x01\x01\x01\x
_ ERROR at teardown of TestX448Exchange.test_round_trip_private_serialization[Encoding.PEM-PrivateFormat.PKCS8
_ ERROR at teardown of TestX25519Exchange.test_round_trip_private_serialization[Encoding.PEM-PrivateFormat.PKC

Notice that these errors are specific to main@pyca/cryptography, and they are not triggered when running against the 38.0.x branch (as exemplified by the original #7829.

[reaperhulk]

@romen our test suite checks for errors on the stack between every single test so any teardown error means that an unexpected error was added to the stack during that test. These may be bugs in OpenSSL 3.1.x or they may be new errors that should be present on the stack and we need to handle them.

We actually test against master in this project now and do not see these errors; how divergent is the 3.1 branch from master?

[romen]

@romen our test suite checks for errors on the stack between every single test so any teardown error means that an unexpected error was added to the stack during that test. These may be bugs in OpenSSL 3.1.x or they may be new errors that should be present on the stack and we need to handle them.

We actually test against master in this project now and do not see these errors; how divergent is the 3.1 branch from master?

@reaperhulk the new surprising errors are specifically triggered by the changes in openssl/openssl#19681 which will likely land in openssl-3.1 and master soon.
I was asked to proactively fix the pyca-cryptography tests before we land that change, and that's how this PR (and the old #7829) were born.

• 3d9e574 (the current tip of this PR) addresses the expected problem in the pyca testsuite, that was checking the reason of the error raised by openssl when parsing a malformed EC key
• the surprising teardown errors mentioned in the description are the concerning ones, that I'd like to clear before merging this in pyca.

[romen]

@reaperhulk 689b687 is not meant to be merged, but is just a way to experiment if I can get the CI here to trigger the same errors by testing against openssl/openssl#19681

[romen]

@reaperhulk 689b687 is not meant to be merged, but is just a way to experiment if I can get the CI here to trigger the same errors by testing against openssl/openssl#19681

@reaperhulk @alex @levitte @mspncp here are the errors also in the CI run here: https://github.com/pyca/cryptography/actions/runs/3527647029/jobs/5916940092

[romen]

@levitte did dig a bit on the OpenSSL side of things:

This happens when a param is passed with a data type that's not what the other end expected has been passed. So it may be that somewhere, there's a call of a param setter or getter that goes unchecked.

In the same comment he suggested a way to try and highlight the places where this is happening.

Here is the new log which highlights various python functions that seems to be doing this: 📎 newlog.txt

@reaperhulk @alex : Does this help figuring out what is going on in main that doesn't happen in 38.0.x? Ultimately I believe there are places where likely you hardcoded the EC point conversion format as "compressed", rather than decoding according to SECG, but cannot find such a place on my own, and how it is exercised by functions that seem completely unrelated to me.

[alex]

I'm not quite sure what's going on, but that log file is basically unreadable. Output appears to be interleaved between multiple threads or something.

That said, looking at the crashing stacks, the top python frame is _load_key which is calling PEM_read_bio_PrivateKey. When the result of that function is NULL, it consumes the error stack. So if there's something being left on the error stack, it's because PEM_read_bio_PrivateKey is returning success and putting something on the error stack there.

[romen]

I'm not quite sure what's going on, but that log file is basically unreadable. Output appears to be interleaved between multiple threads or something.

That said, looking at the crashing stacks, the top python frame is _load_key which is calling PEM_read_bio_PrivateKey. When the result of that function is NULL, it consumes the error stack. So if there's something being left on the error stack, it's because PEM_read_bio_PrivateKey is returning success and putting something on the error stack there.

It's unclear under which conditions this might be happening: in our internal tests we don't seem to have that problem, and it's suspicious that this is happening in DSA and ECX tests, when the upcoming change only affects EC keys.

[reaperhulk]

I've run these tests locally and it's adding errors to the stack in PEM_write_bio_PKCS8PrivateKey. Here's a minimal C reproducer:

int main(void) {
    char *p8 = "-----BEGIN PRIVATE KEY-----\nMIIBTAIBADCCASwGByqGSM44BAEwggEfAoGBAKoJMMwUWCUiHK/6KKwolBlqJ4M9\n5ewhJweRaJQgd3Si57I4sNNvGySZosJYUIPrAUMpJEGNhn+qIS3RBx1NzrJ4J5St\nOTzAik1K2n9o1ug5pfzTS05ALYLLioy0D+wxkRv5vTYLA0yqy0xelHmSVzyekAmc\nGw8FlAyr5dLeSaFnAhUArcDoabNvCsATpoH99NSJnWmCBFECgYEAjGtFia+lOk0Q\nSL/DRtHzhsp1UhzPct2qJRKGiA7hMgH/SIkLv8M9ebrK7HHnp3hQe9XxpmQi45QV\nvgPnEUG6Mk9bkxMZKRgsiKn6QGKDYGbOvnS1xmkMfRARBsJAq369VOTjMB/Qhs5q\n2ski+ycTorCIfLoTubxozlz/8kHNMkYEFwIVAKU1qOHQ2Rvq/IvuHZsqOo3jMRID\n-----END PRIVATE KEY-----";
    BIO *b = BIO_new_mem_buf(p8, strlen(p8));
    assert(b != NULL);
    EVP_PKEY *pkey = PEM_read_bio_PrivateKey(b, NULL, NULL, NULL);
    assert(pkey != NULL);
    const BIO_METHOD *m = BIO_s_mem();
    assert(m != NULL);
    BIO *write_bio = BIO_new(m);
    assert(write_bio != NULL);
    const EVP_CIPHER *cipher = EVP_get_cipherbyname("aes-256-cbc");
    assert(cipher != NULL);
    int res = PEM_write_bio_PKCS8PrivateKey(write_bio, pkey, cipher, "s", 1, NULL, NULL);
    assert(res == 1);
    assert(ERR_get_error() == 0);
}

[mspncp]

Thank you for your help @reaperhulk!

[romen]

Thanks @reaperhulk!

Using your reproducer I managed to figure out that this was fixed recently in openssl-3.1 via a48081ac606c7bbce5e3adad7ad2d6dfc1b4f215.

I pushed another commit tweaking the CI to run against a48081, a48081~1, and the upcoming OpenSSL change rebased on latest openssl-3.1. This should demonstrate when this was fixed, and show that nothing else needs to be changed in this PR.

[alex]

So I think with the CI changes reverted this can be merged.

[romen]

So I think with the CI changes reverted this can be merged.

Rebased on latest main, and dropped the commits tweaking the CI configuration.

[romen]

Assuming this will be merged to main, is there a process for backports to 38.0.x?

[alex]

We generally only backport security or critical fixes. Since this isn't even a functional change for users, it wouldn't qualify.

Notwithstanding that, if we do a 38.0.4 for some other reason we can include this. Otherwise it'll be in 39.0.

[alex]

We ended up doing a 38.0.4 today which includes this commit.

[romen]

We ended up doing a 38.0.4 today which includes this commit.

@alex @reaperhulk Thanks for the update, and for all the help!