Honor OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT as set and default to UNCOMPRESSED #16624
Conversation
romen
added
triaged: bug
github-actions
bot
added
the
severity: fips change
|
The PR is based on the |
|
I applied the On top of that, it is affecting the fips provider. |
paulidale
added
the
approval: done
|
Question: I had in mind that we defaulted |
I think you're mistaking the EC domain parameters encoding and the point conversion format. The domain parameters encoding should be kept by the ec/pkey apps in 3.0.0. |
No. I'm definitely thinking of point conversion format. |
@mattcaswell for the app with 3.0.0 what you expect is what happens (and I seem to remember there was a test for this): For the app as of the commit at the top of this PR, things work as expected: I will shortly update with a fixup commit for the test to show what happens when we give |
| if ((pub_key_len = EC_POINT_point2buf(ecg, pub_point, | ||
| POINT_CONVERSION_COMPRESSED, | ||
| format, |
There was a problem hiding this comment.
It's a little hard to square this change with our promise of a stable libcrypto/provider ABI.
Wouldn't it be "safer" (in that regard) to redefine OSSL_KEYMGMT_SELECT_PUBLIC_KEY to always be compressed, and define a new selector flag to get the public key in the form associated with the key (or in uncompressed form specifically)?
In particular, how would a mismatched 3.0.0/3.0.1 libcrypto/default-provider (or vice versa) handle this change?
There was a problem hiding this comment.
This is my concern as well, and the reason I put the OTC hold to discuss it tomorrow!
There was a problem hiding this comment.
On the other hand, documentation never restricted which SECG format the point would be encoded with, and AFAICT the format for import/export (used to?) be considered a provider specific detail.
|
@mattcaswell see ad692cc for an update to the test at the function level. Now it gives the point in compressed format as an input param to create the key object, does not explicitly set At least locally on my machine the new test passes. @levitte @paulidale @t8m @slontis you might want to explicitly reconfirm your approval after the new change. |
|
(last fixup commit is just to properly capitalize a comment I modified in the commit before) |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
|
OTC: We should respect the |
mattcaswell
removed
hold: need otc decision
|
where did we stuck with it? |
|
I have still to work on writing a set of tests for 1.1.1 to port to 3.0 to ensure the behavior is the same under all conditions, I am struggling with finding the time to work on it. |
|
Hopefully this helps? Functions that set conv_form: Functions the read conv_form: |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
Hey guy, any updates on this? |
|
I am sorry, haven't found the time to work on this yet. Unfortunately I have been quite busy lately, so I haven't found the time to actually design, write and tests the 1.1.1 tests, let alone port them to 3.0, and detect if the changes in this PR need amending. |
romen
removed
the
approval: review pending
|
@romen do you have any updates on this? |
|
@romen we could help with writing the tests if you could provide some guidance. |
@t8m that would be highly appreciated, as I seem to be unable to find the time required to work on this as requested by OTC. The guidance I have was mostly summed up here: #16624 (comment) The idea was to start with 1.1.1 and write tests there:
Once the tests for 1.1.1 are written to test all the combinations above, they are to be ported to 3.0 to verify nothing has changed in the existing behavior. Finally a few new tests should be added to cover the new |
Remove usage of deprecated functions.
Exceptions are:
- pkcs11 (no openssl provider support yet)
- ec (no support for uncompressed EC keys
openssl/openssl#16624)
Signed-off-by: Norbert Pocs <npocs@redhat.com>
Reviewed-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>
|
What are the circumstances for not merging it? |
@beldmit OTC wanted test runs against 1.1.1 and 3.0, before and after the changes of this PR, to ensure there is a regression, and that these changes fully address it. For details on the OTC ask, see (TL;DR: I just rehashed the same notes over and over in different comments, with different level of detail):
My original concern, upon which I put the original |
…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. Fixes openssl#16595
|
I am now going to push the squashed commits (plus some fixed typos after @paulidale's feedback in #18320 ), rebased on top of latest |
romen
force-pushed
the
issues/3.0.0/toparams_ecpoint_fmt
branch
from
6206daa
to
74bcd41
Compare
|
Closing as duplicate of #19901 |
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 statewas added among the params imported/exported on an EC provider-native
key (including
OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT, although itdid not affect the format used to export
OSSL_PKEY_PARAM_PUB_KEY).Finally, in #14800,
EVP_PKEY_todata()was introduced and prominentlyexposed 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 thepoint format from the internal state (an
EC_KEYunder the hood) of theprovider-side object, and using it on
EVP_PKEY_export()/EVP_PKEY_todata()to formatOSSL_PKEY_PARAM_PUB_KEY.The default for an
EC_KEYwas already UNCOMPRESSED, and it is alteredif the user sets
OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMATviaEVP_PKEY_fromdata(),EVP_PKEY_set_params(), or one of themore specialized methods.
For symmetry, this commit also alters
ec_pkey_export_to()incrypto/ec/ec_ameth.c, part of theEVP_PKEY_ASN1_METHODfor legacy ECkeys: 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.
Fixes #16595
Checklist