-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[3.1] Honor OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT as set and default to UNCOMPRESSED #19681
Conversation
For the upcoming vote, I think it is worth paying particular attention to the proposed CHANGES entry:
|
Excluding the new CHANGES entry, in terms of code/test/documentation this PR does not differ from #16624 other than being based on top of the |
Is there any reason for this not going into master? Since #16624 is stalled specifically for 3.0, it would now make sense to make that one 3.0 only and leave it to this PR to implement this in all current development branches, not just 3.1 |
Side note: I played around with the code when we were discussing this (when I hadn't looked at #16624 yet), and came to practically the exact same changes. I feel ready to approve this more or less immediately (pending anything that should be dealt with in an OTC meeting, of course). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this briefly for now, I'd also be inclined to approve it.
I am preparing this PR specifically for the upcoming OTC vote I have been tasked to prepare. The idea is that the vote is going to be specific to 3.1, and its outcome will inform also how we want to proceed when it comes to the other current development branches, and if it should be backported to 3.0 as well as a bugfix (which is handled by #16624). The intent is also to keep this item, in terms of a resolution for #16595, separate from the issue being explored in the related #18320 and #19365, which check if 3.0 introduced unintentional regressions compared to 1.1.1, when it comes to generating/(de)serializing |
@romen, all I'm saying is that the "branch: master" label would be appropriate, to indicate a favorable cherry-pick up to master. |
I guess we can always do that once OTC decides regarding 3.1. |
Ok, fair enough |
The failure might be relevant. |
We will need a pyca update to fix the issue. |
@t8m this patch fixes it diff --git a/tests/hazmat/primitives/test_ec.py b/tests/hazmat/primitives/test_ec.py
index 707d23360..a594a039d 100644
--- a/tests/hazmat/primitives/test_ec.py
+++ b/tests/hazmat/primitives/test_ec.py
@@ -479,7 +479,7 @@ class TestECDSAVectors:
# BoringSSL rejects infinity points before it ever gets to us, so it
# uses a more generic error message.
match = (
- "infinity" if not backend._lib.CRYPTOGRAPHY_IS_BORINGSSL else None
+ r'infinity|invalid form' if not backend._lib.CRYPTOGRAPHY_IS_BORINGSSL else None
)
with pytest.raises(ValueError, match=match):
serialization.load_pem_public_key( |
@romen will you submit this as PR against pyca? |
What do you expect is the order of events here? |
As they can include the preemptive fix without breaking anything, I'd ask them for that so this PR can update the pyca at once. |
…anges 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. [0]: openssl/openssl#19681
Here is the tentative PR against their |
I don't think I can push pyca/cryptography#7829 further: they'd prefer to change the test input rather than expecting a different error. Also, the errors I see on their master branch are triggered by this change set, but debugging them goes beyond my python skills. The seemingly unrelated errors on master might be cause to keep the vote running for a longer time until someone can root cause why this PR leaves some errors on the stack even when testing rsa, dsa, and ecx keys that seem completely unrelated. |
…anges 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. [0]: openssl/openssl#19681
Here is the log of running the These errors are not triggered when checking out The errors are due to a $ 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 |
…anges 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. [0]: openssl/openssl#19681
I just closed pyca/cryptography#7829 in favor of pyca/cryptography#7833, as they requested to target |
24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually. |
This pull request is ready to merge |
…o UNCOMPRESSED Originally the code to im/export the EC pubkey was meant to be consumed only by the im/export functions when crossing the provider boundary. Having our providers exporting to a COMPRESSED format octet string made sense to avoid memory waste, as it wasn't exposed outside the provider API, and providers had all tools available to convert across the three formats. Later on, with #13139 deprecating the `EC_KEY_*` functions, more state was added among the params imported/exported on an EC provider-native key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`). Finally, in #14800, `EVP_PKEY_todata()` was introduced and prominently exposed directly to users outside the provider API, and the choice of COMPRESSED over UNCOMPRESSED as the default became less sensible in light of usability, given the latter is more often needed by applications and protocols. This commit fixes it, by using `EC_KEY_get_conv_form()` to get the point format from the internal state (an `EC_KEY` under the hood) of the provider-side object, and using it on `EVP_PKEY_export()`/`EVP_PKEY_todata()` to format `OSSL_PKEY_PARAM_PUB_KEY`. The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via `EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the more specialized methods. For symmetry, this commit also alters `ec_pkey_export_to()` in `crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC keys: it exclusively used COMPRESSED format, and now it honors the conversion format specified in the EC_KEY object being exported to a provider when this function is called. Expand documentation about `OSSL_PKEY_PARAM_PUB_KEY` and mention the 3.1 change in behavior for our providers. Fixes #16595 Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19681)
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19681)
…o UNCOMPRESSED Originally the code to im/export the EC pubkey was meant to be consumed only by the im/export functions when crossing the provider boundary. Having our providers exporting to a COMPRESSED format octet string made sense to avoid memory waste, as it wasn't exposed outside the provider API, and providers had all tools available to convert across the three formats. Later on, with #13139 deprecating the `EC_KEY_*` functions, more state was added among the params imported/exported on an EC provider-native key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`). Finally, in #14800, `EVP_PKEY_todata()` was introduced and prominently exposed directly to users outside the provider API, and the choice of COMPRESSED over UNCOMPRESSED as the default became less sensible in light of usability, given the latter is more often needed by applications and protocols. This commit fixes it, by using `EC_KEY_get_conv_form()` to get the point format from the internal state (an `EC_KEY` under the hood) of the provider-side object, and using it on `EVP_PKEY_export()`/`EVP_PKEY_todata()` to format `OSSL_PKEY_PARAM_PUB_KEY`. The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via `EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the more specialized methods. For symmetry, this commit also alters `ec_pkey_export_to()` in `crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC keys: it exclusively used COMPRESSED format, and now it honors the conversion format specified in the EC_KEY object being exported to a provider when this function is called. Expand documentation about `OSSL_PKEY_PARAM_PUB_KEY` and mention the 3.1 change in behavior for our providers. Fixes #16595 Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #19681) (cherry picked from commit 926db47)
Merged to openssl-3.1 and master branches. Closing. |
…o UNCOMPRESSED Originally the code to im/export the EC pubkey was meant to be consumed only by the im/export functions when crossing the provider boundary. Having our providers exporting to a COMPRESSED format octet string made sense to avoid memory waste, as it wasn't exposed outside the provider API, and providers had all tools available to convert across the three formats. Later on, with openssl#13139 deprecating the `EC_KEY_*` functions, more state was added among the params imported/exported on an EC provider-native key (including `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`, although it did not affect the format used to export `OSSL_PKEY_PARAM_PUB_KEY`). Finally, in openssl#14800, `EVP_PKEY_todata()` was introduced and prominently exposed directly to users outside the provider API, and the choice of COMPRESSED over UNCOMPRESSED as the default became less sensible in light of usability, given the latter is more often needed by applications and protocols. This commit fixes it, by using `EC_KEY_get_conv_form()` to get the point format from the internal state (an `EC_KEY` under the hood) of the provider-side object, and using it on `EVP_PKEY_export()`/`EVP_PKEY_todata()` to format `OSSL_PKEY_PARAM_PUB_KEY`. The default for an `EC_KEY` was already UNCOMPRESSED, and it is altered if the user sets `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT` via `EVP_PKEY_fromdata()`, `EVP_PKEY_set_params()`, or one of the more specialized methods. For symmetry, this commit also alters `ec_pkey_export_to()` in `crypto/ec/ec_ameth.c`, part of the `EVP_PKEY_ASN1_METHOD` for legacy EC keys: it exclusively used COMPRESSED format, and now it honors the conversion format specified in the EC_KEY object being exported to a provider when this function is called. Expand documentation about `OSSL_PKEY_PARAM_PUB_KEY` and mention the 3.1 change in behavior for our providers. Fixes openssl#16595 Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19681) (cherry picked from commit 926db47)
Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#19681) (cherry picked from commit d656efb)
Originally the code to im/export the EC pubkey was meant to be consumed only by the im/export functions when crossing the provider boundary.
Having our providers exporting to a COMPRESSED format octet string made sense to avoid memory waste, as it wasn't exposed outside the provider API, and providers had all tools available to convert across the three formats.
Later on, with #13139 deprecating the
EC_KEY_*
functions, more state was added among the params imported/exported on an EC provider-native key (includingOSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT
, although it did not affect the format used to exportOSSL_PKEY_PARAM_PUB_KEY
).Finally, in #14800,
EVP_PKEY_todata()
was introduced and prominently exposed directly to users outside the provider API, and the choice of COMPRESSED over UNCOMPRESSED as the default became less sensible in light of usability, given the latter is more often needed by applications and protocols.This commit fixes it, by using
EC_KEY_get_conv_form()
to get the point format from the internal state (anEC_KEY
under the hood) of the provider-side object, and using it onEVP_PKEY_export()
/EVP_PKEY_todata()
to formatOSSL_PKEY_PARAM_PUB_KEY
.The default for an
EC_KEY
was already UNCOMPRESSED, and it is altered if the user setsOSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT
viaEVP_PKEY_fromdata()
,EVP_PKEY_set_params()
, or one of the more specialized methods.For symmetry, this commit also alters
ec_pkey_export_to()
incrypto/ec/ec_ameth.c
, part of theEVP_PKEY_ASN1_METHOD
for legacy EC keys: it exclusively used COMPRESSED format, and now it honors the conversion format specified in theEC_KEY
object being exported to a provider when this function is called.Fixes #16595 ("EVP_PKEY_todata exporting compressed EC public key")
Checklist