Update invalid EC key test for compatibility with upcoming OpenSSL changes

[romen]

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.

[alex]

To make sure I understand: OpenSSL is still rejecting a point at infinity, it's just doing it with a different error message now?

[romen]

This is my first contribution: I did read your documentation about contributing, but can use your guidance on conventions regarding the commit message and development practices.

Also note I am directly targeting the 38.0.x branch, as current master seems to trigger other errors when running our 95-test_external_pyca.t hook, but my python understanding is too limited to properly debug them.

[romen]

To make sure I understand: OpenSSL is still rejecting a point at infinity, it's just doing it with a different error message now?

It's not even detected as a point at infinity, because before it gets parsed as a point the encoding of the key used in that tests is detected as invalid.

@levitte provided an analysis here: openssl/openssl#19681 (comment)

[alex]

Ok, so the issue is that we have an encoded point, and instead of correctly marking the point as compressed, we have that byte set to 0, and previously it defaulted to compressed, but now it's just being rejected.

In that case, teh correct fix is to change the input to be a properly encoded infinity point, and leave the assertion as is.

[romen]

It seems the errors I get on master are triggered by applying this patch, but in ways I cannot fully debug on my own and that to my exploration appear to be in unrelated locations.

I am attaching the log with the errors in case someone can help: log.txt

[alex]

So these errors mean that a test function exited with errors left on the stack -- which means we failed to handle an error returned by OpenSSL somewhere. Unfortunately, there's not really a better way to handle it then to take one of the identified failing tests (e.g. test_round_trip_private_serialization) and review every OpenSSL API it invokes and make sure we handle any error codes returned. (Obviously if you know what function on the OpenSSL side is returning the errors you can try a bottom-up approach instead.)

…

On Mon, Nov 21, 2022 at 5:23 PM Nicola Tuveri ***@***.***> wrote: It seems the errors I get on master are triggered by applying this patch, but in ways I cannot fully debug on my own and that to my exploration appear to be in unrelated locations. I am attaching the log with the errors in case someone can help: log.txt — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[romen]

I am thinking at this point it's probably better to close this PR: you prefer changing the test inputs rather than updating the expected error message, and debugging the errors on master is beyond my python debugging skills.

Do you think these could be fixed before we get to merge openssl/openssl#19681 in 3.1 and master? Or should we disable pyca tests until this can be addressed properly on your side?

[alex]

I can update test_load_invalid_ec_key_from_pem to properly encode the test cases pretty easily.

[alex]

Ok, I checked in with some folks who are smarter than myself, and they say that actually this is encoded correctly. So if OpenSSL is going to reject these on "format" grounds (since its a 0 instead of a 2/3/4), that's fine with us.

So if you can fix the flake8 issue, this should be mergable.

my apologies for flip-flopping on this!

[romen]

My fixup! commit did not appease flake: can you suggest what is the least invasive idiomatic way of fixing this? I hardly find relevant sources on how to split an if-expression (rather than a if-statement) on multiple lines and it also seems doing it might still hit parsing limits within flake.

[alex]

Suggested change

	r'infinity|invalid form' \
	if not backend._lib.CRYPTOGRAPHY_IS_BORINGSSL else None
	r"infinity|invalid form"
	if not backend._lib.CRYPTOGRAPHY_IS_BORINGSSL
	else None

[romen]

Thanks!

Now flakes seems to be fine with my changes, but fails on some other files I did not change:

• src/cryptography/hazmat/primitives/serialization/ssh.py
• tests/test_interfaces.py

[reaperhulk]

That failure is due to a mypy version issue that was fixed on main. You are targeting the 38.0.x branch. You'll want to retarget this PR to main (it may be easiest to just close this and open a new one)

[alex]

Also, this PR is currently pointed at the 38.0.x branch. Please point it at main, that's where all changes are landed.

[romen]

Closing in favor of #7833 which targets main instead of 38.0.x.