[Pending #11074] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys, take 2 by levitte · Pull Request #11025 · openssl/openssl

levitte · 2020-02-06T09:03:03Z

This is an adaptation of the following functions for provided 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.

This replaces #10807 and is built on top of #11006

levitte · 2020-02-06T09:04:47Z

The commits from #11006 are here as well, but will disappear as soon as that one is merged and this gets rebased. This PR will remain a draft until that happens.

Feel free to roam around in the last 6 commits and comment on those here.

slontis · 2020-02-07T03:54:08Z

I dont think I am going to review this until it is rebased.
Looking through > 50 files (36 commits) takes quite a while :)

levitte · 2020-02-07T06:48:51Z

It is possible to look at one commit at a time... But yeah, I get it, the combo is massive

crypto/evp/p_lib.c

levitte · 2020-02-07T08:53:29Z

Rebase done, now that #11006 is in

levitte · 2020-02-07T08:59:17Z

Please make initial reviews. I'm still keeping it WIP for a little bit more while investigating what happens in a merge with #10797, but I suspect I'm pretty much done...

levitte · 2020-02-07T08:59:55Z

(at the very least, I like this take much more than #10807)

mattcaswell · 2020-02-07T09:26:38Z

crypto/evp/keymgmt_lib.c

I have a tendency to think we should have a different return code for "unknown"

I originally had that idea for #11006, actually. so was basically thinking that we could have these possible return values for all of EVP_KEYMGMT:

# define EVP_KEYMGMT_FAILURE 0 # define EVP_KEYMGMT_FALSE 0 # define EVP_KEYMGMT_SUCCESS 1 # define EVP_KEYMGMT_TRUE 1 # define EVP_KEYMGMT_UNSUPPORTED -1

That would leave it to the caller to decide how to handle unknown / unsupported cases.

I'm not sure if such a change belongs in this PR, though...

mattcaswell · 2020-02-07T09:43:32Z

crypto/evp/keymgmt_lib.c

If the finder doesn't return true, is there any point in going around the inner loop?

Yes. The idea is that it may be sufficient if either of the EVP_PKEYs have a keymgmt for which finder returns true.
The next step will be prepare_binary_op, which does a cross-wise export and should end up finding the most suitable keymgmt to use.

Probably I'm misunderstanding something here. If finder doesn't return true here, then when we get to the inner loop one of the following is true:

tmp_keymgmt1 and tmpkeydata1 are both NULL - which means the "bigbreak" condition will never be hit in that case
or

tmp_keymgmt1 and tmpkeydata1 have the same values as the last time we went around the outer loop. So if we didn't find a match in the inner loop last time - why go around it again?

I believe this question needs to wait for #11074, as conditions are changing with that one. Basically, when that one lands, any EVP_PKEY with pkeys[0].keymgmt == NULL is to be considered unassigned as far as this file goes (legacy export is taken care of in evp_pkey_make_provided()).

crypto/evp/keymgmt_lib.c

crypto/evp/p_lib.c

crypto/evp/keymgmt_lib.c

levitte · 2020-02-12T13:44:49Z

This is now pending on #11074

levitte · 2020-02-13T12:16:32Z

This is not WIP any more, but still pending on #11074. Early reviews are welcome

crypto/evp/keymgmt_lib.c

This adds evp_keymgmt_util_match() and affects EVP_PKEY_cmp() and EVP_PKEY_cmp_parameters(). The word 'match' was used for the new routines because many associate 'cmp' with comparison functions that allows sorting, i.e. return -1, 0 or 1 depending on the order in which the two compared elements should be sorted. EVP_PKEY_cmp() and EVP_PKEY_cmp_parameters() don't quite do that.

This adds evp_keymgmt_util_copy() and affects EVP_PKEY_copy_parameters()

…entations

mattcaswell · 2020-02-18T17:30:08Z

crypto/evp/keymgmt_lib.c

Probably I'm misunderstanding something here. If finder doesn't return true here, then when we get to the inner loop one of the following is true:

tmp_keymgmt1 and tmpkeydata1 are both NULL - which means the "bigbreak" condition will never be hit in that case
or

tmp_keymgmt1 and tmpkeydata1 have the same values as the last time we went around the outer loop. So if we didn't find a match in the inner loop last time - why go around it again?

slontis

please add a ec_match

slontis · 2020-02-20T00:35:53Z

crypto/evp/p_lib.c

+     * do this check, after having exported either of them that isn't
+     * provided.
+     */
+    if (!(evp_pkey_is_pure_provided(to) && evp_pkey_is_pure_provided(from))) {


This logic doesnt match the comment does it? Dont you want !X && !Y
You are currently testing is either key a legacy key?

You're right. I'll have another look in the morning...

Cheers, I have just about finished reviewing.. I dont want you being held up. Is there any other PR's you need to be reviewed?

I need #11074 in place before tackling this one

slontis · 2020-02-20T00:39:47Z

crypto/evp/p_lib.c


-    if (from->ameth && from->ameth->param_copy)
+    /*
+     * Only use the routine for provided keys if either key is purely provided.


This comment also doesn't match the logic below (which is both keys)
Looks like you mean to use ||

I need #11074 in place before tackling this one

crypto/evp/keymgmt_lib.c

slontis · 2020-02-20T00:58:57Z

crypto/evp/keymgmt_lib.c

+static int implements_copy_and_export(EVP_KEYMGMT *keymgmt)
+{
+    return keymgmt->copy != NULL
+        && (keymgmt->import == NULL || keymgmt->export != NULL);


I don't understand. A provider can do import-only or export-only or both. Is that concept not supported by this change?

Hmmm, good question. I'll have a look again.

levitte · 2020-02-24T18:23:31Z

This one is closed in favor of #11158

levitte added the branch: master Applies to master branch label Feb 6, 2020

levitte mentioned this pull request Feb 6, 2020

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

Closed

slontis reviewed Feb 7, 2020

View reviewed changes

crypto/evp/p_lib.c Outdated Show resolved Hide resolved

levitte force-pushed the provider-key-check+cmp+dup-take2 branch from fa3c304 to e5db8ce Compare February 7, 2020 08:53

levitte marked this pull request as ready for review February 7, 2020 08:57

mattcaswell requested changes Feb 7, 2020

View reviewed changes

levitte commented Feb 7, 2020

View reviewed changes

crypto/evp/keymgmt_lib.c Outdated Show resolved Hide resolved

This was referenced Feb 7, 2020

Implement domparam and key generation #10289

Merged

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

Closed

levitte changed the title ~~[WIP] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys, take 2~~ [WIP, Pending #11074] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys, take 2 Feb 12, 2020

levitte force-pushed the provider-key-check+cmp+dup-take2 branch 2 times, most recently from 328ec0f to a5cd9b8 Compare February 13, 2020 08:30

levitte changed the title ~~[WIP, Pending #11074] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys, take 2~~ [Pending #11074] EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys, take 2 Feb 13, 2020

slontis reviewed Feb 13, 2020

View reviewed changes

crypto/evp/keymgmt_lib.c Outdated Show resolved Hide resolved

levitte force-pushed the provider-key-check+cmp+dup-take2 branch 2 times, most recently from 3b6593b to e79072e Compare February 17, 2020 15:32

levitte added 6 commits February 18, 2020 13:08

EVP: Adapt EVP_PKEY_missing_parameters() for provider keys

66e05a9

KEYMGMT: Add a keydata matching function

273e134

KEYMGMT: Add a keydata copy function

a8d1f5c

EVP: Add support for copying provided EVP_PKEYs

bc4c4a5

This adds evp_keymgmt_util_copy() and affects EVP_PKEY_copy_parameters()

PROV: Add a OP_keymgmt_match() function to our DH, DSA and RSA implem…

a58b1b8

…entations

fixup! EVP: Add support for copying provided EVP_PKEYs

bce9191

levitte force-pushed the provider-key-check+cmp+dup-take2 branch from e79072e to bce9191 Compare February 18, 2020 12:08

mattcaswell requested changes Feb 18, 2020

View reviewed changes

slontis mentioned this pull request Feb 19, 2020

PROV: add RSA signature implementation #10557

Merged

slontis reviewed Feb 20, 2020

View reviewed changes

crypto/evp/keymgmt_lib.c Show resolved Hide resolved

slontis reviewed Feb 20, 2020

View reviewed changes

levitte mentioned this pull request Feb 20, 2020

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

Merged

2 tasks

fixup! EVP: Add support for comparing provided EVP_PKEYs

09e416a

levitte mentioned this pull request Feb 24, 2020

EVP: Adapt EVP_PKEY checking, comparing and copying for provider keys, take 3 #11158

Merged

levitte closed this Feb 24, 2020

levitte deleted the provider-key-check+cmp+dup-take2 branch February 24, 2020 18:23

Uh oh!

Conversation

levitte commented Feb 6, 2020

Uh oh!

levitte commented Feb 6, 2020

Uh oh!

slontis commented Feb 7, 2020

Uh oh!

levitte commented Feb 7, 2020

Uh oh!

Uh oh!

levitte commented Feb 7, 2020

Uh oh!

levitte commented Feb 7, 2020

Uh oh!

levitte commented Feb 7, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

levitte commented Feb 13, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slontis left a comment

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 Feb 20, 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!

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!

levitte commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

levitte commented Feb 12, 2020 •

edited

Loading

slontis Feb 20, 2020 •

edited

Loading