EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys by levitte · Pull Request #10807 · openssl/openssl

levitte · 2020-01-10T20:32:12Z

This affects the following function, which can now deal with provider
side domain parameters and keys:

EVP_PKEY_copy_parameters()
EVP_PKEY_missing_parameters()
EVP_PKEY_cmp_parameters()
EVP_PKEY_cmp()

This also adds the necessary EVP_KEYMGMT interfaces to support those functions, and the implementations for RSA, DSA and DH.

levitte · 2020-01-10T20:35:26Z

Together with #10557, #10803, #10805 and #10806, this got evp_test to pass in the #10797 torture test

levitte · 2020-01-13T08:37:19Z

There was a small fixup needed, but other than that, at least my personal tests go through. Final review?

mattcaswell · 2020-01-13T10:25:00Z

crypto/evp/keymgmt_lib.c

What is the purpose of the assignment to i here?

Must be old cruft. Removing.

Doesn't seem to have been removed.

Nope, but if you read the comment a couple of lines up, I think the reason will become clear 😉

Wouldnt if (!ossl_assert())
return 0;
be better?

crypto/evp/keymgmt_lib.c

mattcaswell · 2020-01-13T10:52:37Z

Travis failures also look relevant.

levitte · 2020-01-14T01:38:13Z

After all the excellent remarks from @mattcaswell, I've had the time to rethink this PR, and have refactored it quite a bit.

~~Also, this is now pending #10837.~~ (not any more, that one has been merged)

levitte · 2020-01-20T09:45:22Z

I believe this PR got the last little nudge it needed to be complete enough.

mattcaswell · 2020-01-21T11:29:15Z

crypto/evp/keymgmt_lib.c

Doesn't seem to have been removed.

mattcaswell · 2020-01-21T11:40:20Z

crypto/evp/keymgmt_lib.c

This doesn's seem to quite make sense...why do we check provdata2 == NULL? I would have expected:

if (provdata1 != NULL && provdata2 != NULL)

Good point.

mattcaswell · 2020-01-21T11:45:55Z

crypto/evp/keymgmt_lib.c

Wouldn't this up ref be better inside the if block below? That way you don't have to then down-ref it in the else block. Or alternatively place it immediately before the final assignment to->pkeys[0].keymgmt = keymgmt_to

I'll have another look. Thanks for the heads up

mattcaswell · 2020-01-21T11:48:39Z

crypto/evp/keymgmt_meth.c

So iskey, cmpkey and dupkey are mandatory?

mattcaswell · 2020-01-21T11:55:27Z

crypto/evp/p_lib.c

The NULL here for the libctx parameter seems wrong

I'm quite confused by what you are trying to do here...we just seem to be trying to export the key data to "whatever", regardless of whether the key already exists in a provider?

Not "whatever". To a keymgmt that corresponds to the type of the from key, and only if it's not already exported to the keymgmt of that key.

What libctx would you pass to evp_pkey_make_provided?

I'm not sure, but this is problematic. If you're trying to copy parameters from one key to another for a key type that does not exist in the default provider (but does in some other libctx) then its not going to work.

mattcaswell · 2020-01-21T12:13:24Z

crypto/evp/p_lib.c

So a copy parameters operation frees any pre-existing key data? Is that the correct semantics?

frees any pre-existing key data

Just the legacy bits, and only if from doesn't have any legacy bits... but I need to have another look at the evp_pkey_make_provided call...

mattcaswell · 2020-01-21T12:20:50Z

crypto/evp/p_lib.c

Again, I'm concerned about the use of the default libctx here. What if the default libctx doesn't have a provider for this type of key loaded?

mattcaswell · 2020-01-21T12:24:22Z

doc/man7/provider-keymgmt.pod

"is or contains"...I'm not sure I understand this. Please explain?

mattcaswell · 2020-01-21T12:30:29Z

providers/implementations/keymgmt/rsa_kmgmt.c.orig

What is this file?

Oops, stray

mattcaswell · 2020-01-21T12:30:42Z

providers/implementations/keymgmt/rsa_kmgmt.c.rej

What is this file?

Oops, stray

slontis · 2020-01-22T01:48:54Z

crypto/evp/keymgmt_lib.c

Do all entries in pk->pkeys[i] have the same domainparams setting?
If they do then maybe it should not be stored in the array.
If they dont then this logic is incorrect.

When I designed this cache, I did intend for it to be able to hold a mixture, but that turned out difficult / flawed. That's why you see these indicators on a per cache item basis, and you're right, it can be taken out from there. I think that's a topic for a separate PR, though...

slontis · 2020-01-22T01:55:13Z

crypto/evp/keymgmt_lib.c

I really dislike this being called _cmp()
cmp() to me means return 0 if equal.

The public name will not change, obviously, but feel free to suggest a better name for this function.

slontis · 2020-01-22T02:06:48Z

crypto/evp/p_lib.c

The old code checked for ameth == NULL (Maybe put that back in just in case)

slontis · 2020-01-22T02:10:07Z

doc/man7/provider-keymgmt.pod

maybe remove is or

slontis · 2020-01-22T02:10:46Z

doc/man7/provider-keymgmt.pod

slontis · 2020-01-22T02:15:53Z

providers/implementations/keymgmt/dh_kmgmt.c

Should this be ||

OR
return == 0
&& == 0

slontis · 2020-01-22T02:24:36Z

providers/implementations/keymgmt/dh_kmgmt.c

Not sure why you need these null checks : BN_cmp returns 0 if both NULL or if they are equal otherwise it returns -1 or 1 dependent on a < b.. So the cmp()==0 should be sufficient.

I hadn't looked at BN_cmp's behavior. Thanks for the heads up

slontis · 2020-01-22T02:26:43Z

providers/implementations/keymgmt/dh_kmgmt.c

If we can set keys up via setparams would it be possible to only have the private key bit?

The general assumption with openssl is that a valid key pair always has the public half. A lot will fall apart if that doesn't hold true.

slontis · 2020-01-22T02:28:10Z

providers/implementations/keymgmt/dh_kmgmt.c

hmm this is an interesting check... Are we only interested in the public key comparison here?

I basically copied the code from dh_ameth.c. Interestingly enough, the legacy function pointer is called pub_cmp, so it's a bit more explicit. I was thinking that it would be up to the provider to decide exactly how detailed the comparison should be. In this implementation, I chose to do like we did before. For some implementations where the private half is hidden (typically HSMs), this would be the only possible comparison.

With the assumption that the private half actually corresponds to the public part, this comparison is enough. I suppose that key validation will ensure this validity?

slontis · 2020-01-22T02:29:29Z

providers/implementations/keymgmt/dsa_kmgmt.c

OR...
return BN_cmp() == 0
&& BN_cmp() == 0...

This applies both to domain parameters and actual keys

This affects the following function, which can now deal with provider side domain parameters and keys: - EVP_PKEY_copy_parameters() - EVP_PKEY_missing_parameters() - EVP_PKEY_cmp_parameters() - EVP_PKEY_cmp() -

…er keys

With ex_data support in DSA keys gone for the FIPS module, there's no need to create a key with a library context as input, i.e. we can go back to normal DSA_new(). Also, we avoid using DSAparams_dup(), as that does a lot of ASN.1 magic that we don't actually need. Just duplicate P, G and Q.

…er keys

- DSA_get0_p() - DSA_get0_q() - DSA_get0_g() - DSA_get0_pub_key() - DSA_get0_priv_key()

…er keys

…er keys This helps in some cases where it's not terribly important if the provider data is a set of domain parameters or a keypair. -1 is used by EVP_PKEY_copy_parameters(), which allows copying from provider data that's a keypair just as well as domain parameters. This means that providers can't really have a strict separation between keypairs and domain parameters, as the former is increasingly assumed to contain the latter.

levitte · 2020-01-30T05:13:45Z

I've realized that this needs reworking on a deeper level than I thought. Most of all, functions like EVP_PKEY_copy_parameters can't be implemented as a duplicating function, and it needs KEYMGMT support to import domain parameters into an already existing key. The consequence is that the KEYMGMT libcrypto <-> provider interface needs to be somewhat redesigned.

Thank you all for the feedback here, it will absolutely work as valuable input for the redesign.

PR coming up!

levitte · 2020-01-31T09:38:22Z

PR took a little longer than I expected. However, I've raised an issue, see #10979

levitte · 2020-02-03T16:42:32Z

#11006 is the new KEYMGMT design. I'll work that one into this one.

levitte · 2020-02-06T09:05:25Z

Killing this in favor of #11025

levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Jan 10, 2020

This was referenced Jan 10, 2020

Implement domparam and key generation #10289

Merged

EXPERIMENTAL: remove all the legacy bits from a EVP_PKEY #10797

Closed

levitte force-pushed the provider-key-check+cmp+dup branch from adbe04e to bda8501 Compare January 13, 2020 08:36

mattcaswell requested changes Jan 13, 2020

View reviewed changes

levitte changed the title ~~EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys~~ [WIP, Pending #10837] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys Jan 14, 2020

levitte mentioned this pull request Jan 15, 2020

EVP: Add evp_pkey_make_provided() and refactor around it #10850

Closed

levitte changed the title ~~[WIP, Pending #10837] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys~~ [WIP, Pending #10837, #10850] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys Jan 15, 2020

levitte force-pushed the provider-key-check+cmp+dup branch 2 times, most recently from 3870a24 to f069026 Compare January 15, 2020 12:25

levitte changed the title ~~[WIP, Pending #10837, #10850] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys~~ [WIP, Pending #10850] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys Jan 15, 2020

levitte force-pushed the provider-key-check+cmp+dup branch 2 times, most recently from f7604a9 to b980b8c Compare January 18, 2020 04:30

levitte changed the title ~~[WIP, Pending #10850] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys~~ [WIP] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys Jan 18, 2020

levitte force-pushed the provider-key-check+cmp+dup branch from 64341d5 to 890ea03 Compare January 20, 2020 09:42

levitte changed the title ~~[WIP] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys~~ EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys Jan 20, 2020

mattcaswell requested changes Jan 21, 2020

View reviewed changes

levitte force-pushed the provider-key-check+cmp+dup branch from 890ea03 to 4e77c05 Compare January 21, 2020 20:18

levitte mentioned this pull request Jan 21, 2020

Make the RSA ASYM_CIPHER implementation available inside the FIPS module #10881

Closed

slontis reviewed Jan 22, 2020

View reviewed changes

crypto/evp/p_lib.c Outdated

Copy link

Member

slontis Jan 22, 2020

Choose a reason for hiding this comment

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

The old code checked for ameth == NULL (Maybe put that back in just in case)

slontis reviewed Jan 22, 2020

View reviewed changes

doc/man7/provider-keymgmt.pod Outdated

Copy link

Member

slontis Jan 22, 2020

Choose a reason for hiding this comment

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

maybe remove is or

slontis reviewed Jan 22, 2020

View reviewed changes

doc/man7/provider-keymgmt.pod Outdated

Copy link

Member

slontis Jan 22, 2020

Choose a reason for hiding this comment

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

if it does

slontis reviewed Jan 22, 2020

View reviewed changes

levitte force-pushed the provider-key-check+cmp+dup branch from 4e77c05 to 3e3cd5b Compare January 26, 2020 16:26

levitte added 12 commits January 28, 2020 17:08

EVP KEYMGMT: Add support for checking, comparing and duplicating

a17f883

This applies both to domain parameters and actual keys

EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys

541a84e

This affects the following function, which can now deal with provider side domain parameters and keys: - EVP_PKEY_copy_parameters() - EVP_PKEY_missing_parameters() - EVP_PKEY_cmp_parameters() - EVP_PKEY_cmp() -

PROV: Add check, compare and duplicate functionality to the RSA keymgmt

c192b19

PROV: Add check, compare and duplicate functionality to the DSA keymgmt

924f692

PROV: Add check, compare and duplicate functionality to the DH keymgmt

31ff069

fixup! EVP: Adapt EVP_PKEY checking, comparing and copying for provid…

551bc97

…er keys

fixup! EVP: Adapt EVP_PKEY checking, comparing and copying for provid…

3ed2dd8

…er keys

DSA: Move a few functions around to make them accessible for FIPS

f30a258

- DSA_get0_p() - DSA_get0_q() - DSA_get0_g() - DSA_get0_pub_key() - DSA_get0_priv_key()

fixup! EVP: Adapt EVP_PKEY checking, comparing and copying for provid…

5f0c70a

…er keys

fixup! EVP: Adapt EVP_PKEY checking, comparing and copying for provid…

cd260d4

…er keys

levitte force-pushed the provider-key-check+cmp+dup branch from 3e3cd5b to b2eb8e3 Compare January 28, 2020 16:08

levitte mentioned this pull request Feb 6, 2020

[Pending #11074] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys, take 2 #11025

Closed

levitte closed this Feb 6, 2020

levitte deleted the provider-key-check+cmp+dup branch February 6, 2020 09:05

levitte mentioned this pull request Feb 20, 2020

Refactor evp_pkey_make_provided() to do legacy to provider export #11074

Merged

2 tasks

Uh oh!

Conversation

levitte commented Jan 10, 2020

Uh oh!

levitte commented Jan 10, 2020

Uh oh!

levitte commented Jan 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattcaswell commented Jan 13, 2020

Uh oh!

levitte commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Jan 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slontis Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

levitte commented Jan 14, 2020 •

edited

Loading

slontis Jan 22, 2020 •

edited

Loading

slontis Jan 22, 2020 •

edited

Loading