Skip to content

Comments

EVP: Downgrade keys rather than upgrade#11375

Merged
openssl-machine merged 6 commits intoopenssl:masterfrom
levitte:downgrade-key
Mar 25, 2020
Merged

EVP: Downgrade keys rather than upgrade#11375
openssl-machine merged 6 commits intoopenssl:masterfrom
levitte:downgrade-key

Conversation

@levitte
Copy link
Member

@levitte levitte commented Mar 21, 2020

Upgrading EVP_PKEYs from containing legacy keys to containing provider
side keys proved to be risky, with a number of unpleasant corner
cases, and with functions like EVP_PKEY_get0_DSA() failing
unexpectedly.

We therefore change course, and instead of upgrading legacy internal
keys to provider side internal keys, we downgrade provider side
internal keys to legacy ones. To be able to do this, we add
|import_from| and make it a callback function designed for
evp_keymgmt_export().

This means that evp_pkey_upgrade_to_provider() is replaced with
evp_pkey_downgrade().


This also clarifies the interaction between certain key fields in EVP_PKEY.

@levitte
Copy link
Member Author

levitte commented Mar 21, 2020

Missing: import_from implementations in the diverse EVP_PKEY_ASN1_METHODs

@slontis
Copy link
Member

slontis commented Mar 22, 2020

Needs a rebase..
For some reason test_fromdata_ec failed on some appveyor machines

@levitte
Copy link
Member Author

levitte commented Mar 23, 2020

Rebased

@slontis
Copy link
Member

slontis commented Mar 23, 2020

Is this still WIP?

@levitte levitte changed the title [WIP, pending on #11374] EVP: Downgrade keys rather than upgrade [pending on #11374] EVP: Downgrade keys rather than upgrade Mar 23, 2020
@levitte
Copy link
Member Author

levitte commented Mar 23, 2020

Nope, as of now

@levitte
Copy link
Member Author

levitte commented Mar 23, 2020

But caveat, I haven't tested it with the ec generation pr yet

@slontis slontis added the branch: master Applies to master branch label Mar 23, 2020
@slontis slontis requested a review from mattcaswell March 23, 2020 05:39
@levitte
Copy link
Member Author

levitte commented Mar 23, 2020

doc-nits failure seems relevant.

Fixed, I think

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me.

@levitte
Copy link
Member Author

levitte commented Mar 24, 2020

I'm paying dearly for not having proper error reporting all over the place. We really need to get a grip on that. I'll start in this PR, please do the same, @openssl

@levitte levitte force-pushed the downgrade-key branch 2 times, most recently from 223a12c to 42b60ad Compare March 24, 2020 13:04
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved assuming the nit is fixed

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Mar 24, 2020
@levitte
Copy link
Member Author

levitte commented Mar 25, 2020

Darn, there were a couple of very small fixups I hadn't pushed. Please take a look and either let your confirmation stand, or re-confirm (which will reset the grace time clock)

@mattcaswell
Copy link
Member

My approval stands.

@levitte levitte added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 25, 2020
levitte added 6 commits March 25, 2020 17:00
EVP_PKEY is rather complex, even before provider side keys entered the
stage.
You could have untyped / unassigned keys (pk->type == EVP_PKEY_NONE),
keys that had been assigned a type but no data (pk->pkey.ptr == NULL),
and fully assigned keys (pk->type != EVP_PKEY_NONE && pk->pkey.ptr != NULL).

For provider side keys, the corresponding states weren't well defined,
and the code didn't quite account for all the possibilities.

We also guard most of the legacy fields in EVP_PKEY with FIPS_MODE, so
they don't exist at all in the FIPS module.

Most of all, code needs to adapt to the case where an EVP_PKEY's
|keymgmt| is non-NULL, but its |keydata| is NULL.

Reviewed-by: Matt Caswell <[email protected]>
(Merged from openssl#11375)
This function intialises an EVP_PKEY to contain a provider side internal
key.

We take the opportunity to also document the older EVP_PKEY_set_type()
and EVP_PKEY_set_type_str().

Reviewed-by: Matt Caswell <[email protected]>
(Merged from openssl#11375)
Upgrading EVP_PKEYs from containing legacy keys to containing provider
side keys proved to be risky, with a number of unpleasant corner
cases, and with functions like EVP_PKEY_get0_DSA() failing
unexpectedly.

We therefore change course, and instead of upgrading legacy internal
keys to provider side internal keys, we downgrade provider side
internal keys to legacy ones.  To be able to do this, we add
|import_from| and make it a callback function designed for
evp_keymgmt_export().

This means that evp_pkey_upgrade_to_provider() is replaced with
evp_pkey_downgrade().

EVP_PKEY_copy_parameters() is the most deeply affected function of
this change.

Fixes openssl#11366

Reviewed-by: Matt Caswell <[email protected]>
(Merged from openssl#11375)
Downgrading EVP_PKEYs from containing provider side internal keys to
containing legacy keys demands support in the EVP_PKEY_ASN1_METHOD.

This became a bit elaborate because the code would be almost exactly
the same as the import functions int EVP_KEYMGMT.  Therefore, we end
up moving most of the code to common backend support files that can be
used both by legacy backend code and by our providers.

Reviewed-by: Matt Caswell <[email protected]>
(Merged from openssl#11375)
Provider KEYMGMT functions can handle domain parameters as well as
"other" parameters (the cofactor mode flag in ECC keys is one of
those).  The public EVP functions EVP_PKEY_copy_parameters(),
EVP_PKEY_missing_parameters(), EVP_PKEY_cmp_parameters() and
EVP_PKEY_cmp() tried to handle all parameters, but looking back at
EVP_PKEY_ASN1_METHOD code (especially crypto/ec/ec_ameth.c), it turns
out that they only need to concern themselves with domain parameters.

Reviewed-by: Matt Caswell <[email protected]>
(Merged from openssl#11375)
@openssl-machine openssl-machine merged commit 8158cf2 into openssl:master Mar 25, 2020
@levitte
Copy link
Member Author

levitte commented Mar 25, 2020

Merged!

@slontis, I think we both have a merge fest to look forward to 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants