Skip to content

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

Closed
levitte wants to merge 6 commits intoopenssl:masterfrom
levitte:experiment-deassign-legacy-key
Closed

EXPERIMENTAL: remove all the legacy bits from a EVP_PKEY#10797
levitte wants to merge 6 commits intoopenssl:masterfrom
levitte:experiment-deassign-legacy-key

Conversation

@levitte
Copy link
Member

@levitte levitte commented Jan 9, 2020

This adds EVP_PKEY_deassign(), which removes all the legacy bits from an EVP_PKEY, and adapts a few tests to use this function prior to using the keys.

This is currently expected to fail in the CIs, until we have adapted all the EVP_PKEY functions to deal with provider only keys.

@levitte levitte added the branch: master Applies to master branch label Jan 9, 2020
@levitte
Copy link
Member Author

levitte commented Jan 9, 2020

Among functions where we have almost no support for provider only keys is crypto/evp/p_lib.c, most of it relies solely on EVP_PKEY_METHOD and EVP_PKEY_ASN1_METHOD structures.

@levitte
Copy link
Member Author

levitte commented Jan 9, 2020

I just added EVP_PKEY_deassign() in the most evil of places: in the load_key() and load_pubkey() functions that all apps use. Tests are expected to go KABOOM.

Incidently, I looked into some of them that took me by surprise. test_pem is failing, and that caused me to look more closely at outputs... and some of them come out differently when printed via the provider serializers. This happens to be for invalid keys, but that still indicates that something's off, either with assumptions done when exporting from legacy to provider, or when serializing to text.

@levitte
Copy link
Member Author

levitte commented Jan 10, 2020

Fixups in (will be updated)

#10557, #10778, #10803, #10805, #10806, #10808, #10824, #10851, #10920, #10947, #11006, #11025, #11037, #11038, #11056, #11074, #11078

@levitte levitte force-pushed the experiment-deassign-legacy-key branch 3 times, most recently from 28ecfcf to a55dae9 Compare January 18, 2020 04:53
@levitte levitte force-pushed the experiment-deassign-legacy-key branch from a55dae9 to 249cfc1 Compare January 21, 2020 20:33
@levitte levitte mentioned this pull request Jan 24, 2020
@levitte levitte force-pushed the experiment-deassign-legacy-key branch 2 times, most recently from 5ff196e to 75596c7 Compare January 30, 2020 14:21
@levitte levitte force-pushed the experiment-deassign-legacy-key branch from 75596c7 to 1254197 Compare February 4, 2020 19:30
@levitte levitte force-pushed the experiment-deassign-legacy-key branch 3 times, most recently from ad9448f to 2a6b1c0 Compare February 13, 2020 08:29
@levitte levitte force-pushed the experiment-deassign-legacy-key branch 3 times, most recently from a500ab3 to b4d37a7 Compare February 18, 2020 12:07
@levitte
Copy link
Member Author

levitte commented Feb 19, 2020

#11126 was a bit of a surprise... so it turns out that there's code in a few subsystems that are bypassing other central subsystems like EVP and ASN1 and call their backends directly, so weren't at all caught by this experiment (yet). CRMF seems to be one of them, I suspect CMS and a couple of others are similar. That means that we can expect to find a few "surprises" like that one...

@levitte levitte force-pushed the experiment-deassign-legacy-key branch from b4d37a7 to b5cf9ca Compare February 24, 2020 09:49
@levitte levitte force-pushed the experiment-deassign-legacy-key branch from 0fe5acf to cc78f8e Compare March 10, 2020 11:30
@levitte
Copy link
Member Author

levitte commented Mar 10, 2020

All the PRs that were identified here are now merged, so a rebase was in order.

I still expects to see failures, but not quite as many. I also have another experiment in the making that specifically targets the sub-systems PKCS7 and CM*

@levitte
Copy link
Member Author

levitte commented Mar 27, 2020

Closing this PR, as it seems to have served its purpose. Some of the failures are still potentially there, but we do not need to do explicit deassignment of the legacy part of an EVP_PKEY to trigger them any more, as the key generation code is now serving us with such EVP_PKEYs and triggering code that hasn't been adapted for such keys yet.

@levitte levitte closed this Mar 27, 2020
@levitte levitte reopened this Apr 8, 2020
@levitte
Copy link
Member Author

levitte commented Apr 8, 2020

Resurrecting this branch, to test #11422. Throw everything I have at it, basically...

@levitte levitte force-pushed the experiment-deassign-legacy-key branch from cc78f8e to 627490a Compare April 8, 2020 20:50
@levitte levitte force-pushed the experiment-deassign-legacy-key branch from 627490a to b4824c1 Compare April 8, 2020 20:51
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

This looks good -- I'll need to run through it again with a less befuddled mind though.


/* Make sure it's exported to a provider */
{
void *keydata = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be in its own block? The function isn't large and the variables can be declared at the top just as easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do I have to? Also, experimental, I really don't expect this code to end up in the main source, so really, [shrug]

@levitte levitte closed this Apr 23, 2020
@levitte levitte reopened this Apr 23, 2020
@kroeckx kroeckx added this to the 3.0.0 beta1 milestone Oct 7, 2020
@levitte
Copy link
Member Author

levitte commented Oct 7, 2020

Closing, it doesn't serve its purpose any more (or if it should, it needs to be rewritten entirely)

@levitte levitte closed this Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants