Refactor evp_pkey_make_provided() to do legacy to provider export

[levitte]

Previously, evp-keymgmt_util_export_to_provider() took care of all
kinds of exports of EVP_PKEYs to provider side keys, be it from its
legacy key or from another provider side key. This works most of the
times, but there may be cases where the caller wants to be a bit more
in control of what sort of export happens when.

Also, when it's time to remove all legacy stuff, that job will be much
easier if we have a better separation between legacy support and
support of provided stuff, as far as we can take it.

This changes moves the support of legacy key to provider side key
export from evp-keymgmt_util_export_to_provider() to
evp_pkey_make_provided(), and makes sure the latter is called from all
EVP_PKEY functions that handle legacy stuff.

Checklist

• documentation is added or updated
• tests are added or updated

[levitte]

The only Travis failure was an early timeout

[levitte]

Ping!

[levitte]

Travis failure not relevant

[levitte]

There are two other PRs waiting for this one, so please give it focus

[slontis]

I mainly get this code now that I went back to look at the code before the PR.
My brain is still however having a hard time understanding why we need an array of keymgmt/keydata for a single key. Is this to import into multiple providers which all have differents keymanagers (which is a case we dont currently have for say a DH key - i.e there is only one DH keymanager ??).

[levitte]

My brain is still however having a hard time understanding why we need an array of keymgmt/keydata for a single key. Is this to import into multiple providers which all have differents keymanagers (which is a case we dont currently have for say a DH key - i.e there is only one DH keymanager ??).

The design is that the keymgmt we use is tightly coupled with the operations that use keys (i.e. they must exists in the same provider), so they can safely share key data. That does mean that any new provider that implements (to take your example) DH will have to write their own keymgmt for DH keys, because they will probably have their own structure to organize the DH data in (for example, if they've implemented it on top of gmp).
This means that to flexibly use EVP_PKEYs with diverse providers that might support different operations, we need to be able to pass the key data between their respective keymgmt, and we hold a cache of the result to avoid having to do that more than once if we can help it.

[richsalz]

I do not think "flexibly sharing keys" across providers is a good thing because it could lead to security issues. For example, caching keys is a CVE waiting to happen. I prefer to see a much simpler model of explicit export/import (or perhaps those two combined into one atomic thing).

Doing a lot of work to support flexibility, which is not going to be a common path I believe, is the wrong trade-off to maek.

[levitte]

The cache stores pointers to provider side keys. The EVP_PKEY itself doesn't store any key and never has. The security bits stay with the providers, and it's their choice if they want to export anything or not.

The answer back to you, @richsalz, is that if we make transfers of key material between providers explicit, a scenario with multiple providers will become quite cumbersome for the applications, and will most probably hinder trying to use several providers together.

[richsalz]

The answer back to you, @richsalz, is that if we make transfers of key material between providers explicit, a scenario with multiple providers will become quite cumbersome for the applications, and will most probably hinder trying to use several providers together.

And for this release, which is not an LTS, that is the right thing to do. Once you get experience about applications working with multiple providers and trying to use several providers at the same time, then figure out how to support it. As of now, there isn't even a use-case for this.

[levitte]

I guess we will see.

[richsalz]

I guess we will see.

No, we won't. Because once this gets merged it's over. And the possible mistakes, complication, security issues, and poor fit for real needs will live forever.

OpenSSL is really bad at predicting the future and this is another instance.

I know I'm going to lose this argument, as I usually do. Shrug.

[slontis]

No, we won't. Because once this gets merged it's over.

Err while I agree with part of what you are saying in principle, this does seem like an overreaction. As far as I can see this code is all internal, so it can be changed under the hood without affecting external users.
Multiple providers for 3.0 are needed, caching could be deferred.
Either way it can be fixed if (when) there are issues..

[richsalz]

Multiple providers are needed, I agree.

Do we need to implicitly/automatically share key material across those providers?

Either way it can be fixed

In years maybe. But most likely it will work good enough for most people and there's new features to be added, and it will never happen. I base this on decades of observation and years of participation.

I'm not a project member, shrug, do what you're gonna do.

[levitte]

Let's start with a very simple fact: no keymgmt is obligated to export. That means that if pkeys[0].keymgmt points at a keymgmt that doesn't supply an export function, operations with a different provider are simply not supported. In other words, sharing key data is a voluntary function, the affected keymgmt must explicitly agree to it.

On to current existing practices, in very simple terms (this is very condensed code from apps/pkeyutl.c):

    /* pkey is an EVP_PKEY passed from...  somewhere */
    EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL);
    
    if (ctx == NULL
        || !EVP_PKEY_sign_init(ctx)
        || !EVP_PKEY_sign(ctx, out, &outlen, in, inlen))
        goto error;

(even in a legacy scenario, there's a lot that goes on implicitly, EVP_PKEY_CTX_new() might find an engine that has an EVP_PKEY_METHOD for the that key)

We want that code to continue to work in a provided scenario, right?

What happens under the hood in a provided scenario is that EVP_PKEY_sign() tries an implicit fetch of an EVP_SIGNATURE that matches the key type of the pkey that was passed to EVP_PKEY_CTX_new(), and if there is one, there must also be a matching keymgmt.

If the pkey contains a legacy key, i.e. was assigned one with functions/macros like EVP_PKEY_assign_RSA(), or came from a PEM read, or a d2i_PrivateKey(), then it seems pretty straight forward, we export the legacy key data to the keymgmt (assuming it has an import function).
This is already a situation where the key data is shared between two backends

If the pkey doesn't contain a legacy key, i.e. it was created with something like EVP_PKEY_fromdata(), but also possibly with a PEM read that creates an EVP_PKEY with provided keydata rather than a legacy key (this is looming in a future where legacy support is gone). We have a very similar situation to what I describe in the previous paragraph, and may end up in a situation where the provider for the EVP_SIGNATURE is from a different provider than the one of the keymgmt used to create the pkey, which leads to the same need to export key data as above.

Two situations that are really exactly the same, the only thing that differs is where the pkey originated.

So what I hear is that such exports should be explicit rather than implicit. Technically, that should be no problem whatsoever. The question is, should that be done only when the pkey origin is a provided key, or also when the origin is a legacy key?

So let's imagine a situation with explicit export... as far as I'm concerned, that is technically not very hard:

    EVP_PKEY *tmp_pkey = EVP_PKEY_export_to(keymgmt, pkey);
    EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(tmp_pkey, NULL);

    if (ctx == NULL
        || !EVP_PKEY_sign_init(ctx)
        || !EVP_PKEY_sign(ctx, out, &outlen, in, inlen))
        goto error;

The situations where I see this happening are these:

1. We do not support implicit export at all, not even for a pkey that contains a legacy key.
   NOTE: this also means that we do not support using provided implementations with legacy keys
2. We do support implicit export for a pkey with legacy origins, but not for a pkey with provided origins (i.e. doesn't contain a legacy key), and this part of the application code doesn't know what the origin is.

Here are a few situations where the application might not be able to know what kind of keydata the pkey contains:

1. It might be created with EVP_PKEY_assign_RSA(), but just as well with EVP_PKEY_fromdata()
2. It might be loaded from some source, be it file, LDAP store, PKCS#11, whatever, that results in a pkey with provided keydata rather than legacy. (even a provided OSSL_STORE file: loader would do that, meaning that certain PEM functions would end up producing a pkey with provided keydata)

Therefore, I suspect that all application would (sooner rather than later) simply add that extra EVP_PKEY_export_to() line everywhere an EVP_PKEY_CTX is created, or risk spurious errors.

We can do this, technically.

Now, here's something that we have repeated over and over; well-behaved application should just need a recompile and work as they are with OpenSSL 3.0 and the provider architecture (and any provider that plays along). This is an objective of the 3.0 effort.
The code snippet without the explicit export is well-behaved code, so requiring an EVP_PKEY_export_to() in most if not all situations means breaking that objective.

For you to consider...

[levitte]

Meanwhile, letting this PR stall means stalling #11025 (the first take tried to work without this refactoring, and the result was a horrible mess), an indirectly #10289 (because of tests that fail without #11025). I would rather not see that happen.

If we do decide to go the explicit export route, I suggest letting that happen in the form of another refactoring PR.

[t-j-h]

@richsalz asked Do we need to implicitly/automatically share key material across those providers?

Yes.

And one of the many reasons is that we want to move things around relatively freely between providers without the application code having to get explicitly involved in that happening.

As @levitte mentioned - a specific provider can elect to opt of out this - and place the burden on all applications that want to use keys independent of origin.

There is a much bigger mess potential here if we force all applications to change code any time we move an algorithm between providers - and that would be highly undesirable outcome.

We can debate about how to design and implement something - but the objective that applications don't have to get involved in explicit changes simply because something moves between providers is pretty fundamental.

A lot of thought and discussion went into the desired future state (i.e. beyond 3.0) and @levitte is keeping that in mind as he works on the current 3.0 code because we don't want to repeat mistakes of the past where we adopted something using a very short-term viewpoint of only solving the absolute bare minimum right-in-front-of-us context.

Of course it is a balance between too far in one direction, or two far in the other direction and different people have different views. But an alarmist approach in the discussion thread on this and other PRs does not help in the communication.

Your points have merit - but expressing them in the manner that you have to indicate that if things aren't done the way you think then the sky will fall in - that simply undermines your intent of asking for confirmation of a direction.

That you feel that you don't need the capability in 3.0 for your needs is well understood - but you also need to appreciate that there are many other viewpoints and usage models that have to be considered when making these decisions.

I don't think you fundamentally disagree about the ultimate end-state and that this is actually just a disagreement about the timing of the steps to get there - but that is just my assumption from what you have communicated. You haven't actually said that explicitly.

[paulidale]

I'd like @slontis to review this in addition to me.

[richsalz]

@t-j-h my responses are generally reasonable and reasoned; see #11074 (comment) #11074 (comment) Let me restate myself.

I think automatically converting keys among providers is not needed for this release. We don't have any information for how applications would want to do that, and our only obligations are to support the default and FIPS providers. I think making applications explicitly import/export keys is safer -- no need to worry about a cache getting mis-used -- and not spending time on this concept is a better use of project resources. The schedule has already slipped once.

[mattcaswell]

not spending time on this concept is a better use of project resources. The schedule has already slipped once.

This PR is a refactor of code that is already present. It already does the caching. Ripping it out and dealing with all the subsequent problems that would ensue would be a bigger problem for the project in terms of schedule.

[richsalz]

Then leave the code as-is and don't work on this PR until, say, the end of alpha?

[t8m]

Other PRs depend on this.

[richsalz]

Other PR's depend on this

I am not positive that they can't be changed, but I don't know enough of the code.

It's hard when one person is busy writing code and another gives quick approvals, to take time to consider. Oh well, that's the nature of the software business.

[mattcaswell]

LGTM

[levitte]

Other PRs depend on this.

Most of all #11025. It's predecessor, #10807, was such a mess that even I had a hard time understanding what I was doing. This PR is meant to make a clearer separation between EVP_PKEYs with legacy keys and EVP_PKEYs with just provided keys, and makes it possible for the cleaner #11025.

[mattcaswell]

My approval still counts after the above fixup

[slontis]

Good work Richard.

[levitte]

I'll merge later tonight, or early tomorrow

[levitte]

Merged.

However... the whole discussion about the key cache had me think that what it does is mightily unclear. As a matter of fact, it has a dual role. More on that in an upcoming PR, which also leads to a new and much simpler take on EVP_PKEY check, copy and compare, to replace #11025.

[levitte]

See #11148