Avoid downgrading keys in libssl #11898
Conversation
mattcaswell
added
branch: master
|
Regarding EVP_PKEY_id(), I have a branch that refactors how EVP_PKEYs are treated with regards to EVP identities |
So should I be backing out the EVP_PKEY_id() changes in this PR then? |
Dunno. Are you in a rush to get this in, or can you wait 'til I'm done with mine? (mine is a fix of #11823, which complains about exactly that issue, that |
This PR is a dependency on an upcoming PR of mine that I'm currently working on - so I'd like to get it in. I don't actually need the EVP_PKEY_id() changes in this PR, except to say that I had one test failure without them because the test itself was getting surprising results from EVP_PKEY_id(). I could just fix the test without attempting to fix EVP_PKEY_id() itself and that would be good enough for now. |
That works for me... and I can make sure to modify that test accordingly in my branch. |
|
@mattcaswell, the work I was talking about here is now available, see #11913 |
mattcaswell
force-pushed
the
avoid-libssl-downgrade
branch
from
ccefd0a to
f4dcb49
Compare
mattcaswell
changed the title
|
All comments addressed. I've moved this to WIP because the tests will now fail until #11913 is merged. |
That was a surprising turn of events. Does that mean I was fast enough? 😉 |
It meant there were complications, so I decided just to wait for your PR. |
Okie |
EVP_PKEY_[get1|set1]_tls_encodedpoint() only worked if an ameth was present which isn't the case for provided keys. Support has been added to dh, ec and ecx keys.
Even if there is no data to import we should still create an empty key.
An ECX key doesn't have any parameters associated with it. Therefore it always has all the parameters it needs, and the "has" function should return 1 if asked about parameters. Without this EVP_PKEY_missing_parameters() fails for ECX keys.
We were downgrading to legacy keys at various points in libssl in order to get or set an encoded point. Now that the encoded point functions work with provided keys this is no longer necessary.
mattcaswell
force-pushed
the
avoid-libssl-downgrade
branch
from
14e79a2 to
83ecb62
Compare
mattcaswell
changed the title
|
#11913 has now been merged, so this has been rebased and taken out of WIP. Please take a look. |
|
Travis got an error: |
doc/man7/EVP_PKEY-EC.pod
Outdated
| @@ -39,6 +39,11 @@ The public key value in EC point format. | |||
|
|
|||
| The private key value. | |||
|
|
|||
| =item "tls-encoded-pt" (B<OSSL_PKEY_PARAM_TLS_ENCODED_PT>) <octet string> | |||
|
|
|||
| Used for getting and setting the encoding of the EC public key as in key | |||
There was a problem hiding this comment.
same here..
Also extra space before key
I'm fairly sure this is just a glitch. |
|
Feedback addressed. Please take another look. |
doc/man7/EVP_PKEY-EC.pod
Outdated
| @@ -41,7 +41,7 @@ The private key value. | |||
|
|
|||
| =item "tls-encoded-pt" (B<OSSL_PKEY_PARAM_TLS_ENCODED_PT>) <octet string> | |||
|
|
|||
| Used for getting and setting the encoding of the EC public key as in key | |||
slontis
added
approval: done
|
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. |
Its not related to this PR. It happens when the test dumps PEM files to stdout and they happen to have a line that starts with the text "ok". It confuses the test harness. I raised an issue for it: |
mattcaswell
added
approval: ready to merge
EVP_PKEY_[get1|set1]_tls_encodedpoint() only worked if an ameth was present which isn't the case for provided keys. Support has been added to dh, ec and ecx keys. Reviewed-by: Shane Lontis <[email protected]> (Merged from #11898)
Even if there is no data to import we should still create an empty key. Reviewed-by: Shane Lontis <[email protected]> (Merged from #11898)
An ECX key doesn't have any parameters associated with it. Therefore it always has all the parameters it needs, and the "has" function should return 1 if asked about parameters. Without this EVP_PKEY_missing_parameters() fails for ECX keys. Reviewed-by: Shane Lontis <[email protected]> (Merged from #11898)
We were downgrading to legacy keys at various points in libssl in order to get or set an encoded point. Now that the encoded point functions work with provided keys this is no longer necessary. Reviewed-by: Shane Lontis <[email protected]> (Merged from #11898)
|
Nit fixed. Pushed to master. Thanks! |
We were downgrading to legacy keys at various points in libssl in order to get or set an encoded point. This PR adds provider support for getting or setting the encoded point, and therefore the deliberate downgrading can be removed. This also fixes a few additional bugs that came to light with using provider only keys.
Finally I have to add a downgrade in EVP_PKEY_id() because provider only keys don't otherwise set the pkey->type field. One of the tests was checking this and hence failing. Applications that do the same might also fail so we should automatically downgrade where possible. To balance that I've removed all usage of EVP_PKEY_id() from libssl so that we don't inadvertently downgrade keys.
There are a few spots left in libssl where keys will be downgraded indirectly - but this PR gets most instances.