It was always the intention that string controls would be directly, simply and fairly uniformly translated to OSSL_PARAM. Let's remember that libcrypto is becoming a framework, and can therefore not assume too much knowledge of what the provided implementations want.

travis failures need addressing

I had to fix a couple of things, so a re-approval is needed, regardless of if @romen's concern is already answered or not.

Ping?

@vt-alt, could you please remind the test case?

@beldmit openssl dgst -sha256 -mac hmac -macopt key:123456901234567890123456789012 </dev/null ?

Sure! Many thanks!

Hmmm, that test fails... but does that have anything to do with this PR?
I'm all for correcting that error, but would rather do that in a separate PR.

On my local build I get
MAC parameter error "key:123456901234567890123456789012"..
So not directly caused by this PR.

Hmmm, that test fails... but does that have anything to do with this PR?
I'm all for correcting that error, but would rather do that in a separate PR.

My misunderstanding, sorry.

So not directly caused by this PR.

Thanks. I'll investigate

I thought this is related to issue #10814 where @levitte said "I found out why this happens... It's the case of raw keys that's not properly intercepted in pkey_mac_str_ctrl(). We really shouldn't need to intercept that, but since it has local implications, we must.".

I'll comment on it there

Rebase needed

That was a trivial conflict, easily resolved. This PR isn't changed by it, so I'll take that reconfirmation and merge.

Just to be safe, I'll wait a bit for the CIs, though

Merged

972fa31 Decentralize legacy_ctrl_str_to_param()
00bc1ad Don't pass a digest-size to signature implementations
31a796d PROV: Implement padding mode words in the RSA ASYM_CIPHER implementation
8bee651 PROV: Fix the DSA SIGNATURE implementation for better digests handling
e3b1cca EVP_MD_CTX_ctrl(): Remove unnecessary control

I know my issue comes late, but I couldn't find cycles to spare until now.

I see now another problem with this: by going directly to the provider interface with EVP_PKEY_CTX_set_params() from inside legacy_ctrl_str_to_param() we are bypassing the dedicated EVP_PKEY_CTX_set_foo().
I get this is exactly the intent of this PR, but the problem I see with this now is that we are pushing beyond the provider boundary all the tweaks we might need to retain backward compatibility and I think this is wrong.

Yes, the code inside libcrypto looks nicer and saner this way, but this way we lose the ability to return specific return values for different error conditions or we force every external provider to implement whatever workarounds we devise inside the providers we distribute to retain the legacy behavior.

Examples from #10631 "ecdh_kdf_md" and "ecdh_cofactor_mode"

Recall that EVP_PKEY_CTX_ctrl_str used to call ctx->pmeth->ctrl_str() so different EVP_PKEY_method diverged in what values they returned and under which conditions.

The EVP_PKEY_method for EC implemented ctrl_str to do some parsing, possibly raise some specific error codes if something failed during parsing, and then called in the dedicated EVP_PKEY_CTX_set_foo() function, hence the return values for EVP_PKEY_CTX_ctrl_str for EC must adhere to whatever was the behavior of EVP_PKEY_CTX_set_foo() if we want to retain backward compatibility:

When EVP_PKEY_CTX_set_ecdh_{kdf_md,cofactor_mode} is called, for backward compatibility it should return:

• -2 if this EVP_PKEY_CTX is not for a DERIVE operation
  • does not concern the provider implementation
  • libcrypto should return -2 to the caller if this condition is detected
• -1 if this EVP_PKEY_CTX does not contain an EVP_PKEY_EC key
  • does not concern the provider implementation
  • libcrypto should return -1 to the caller if this condition is detected
• -2 if this EVP_PKEY_CTX does not support setting the kdf_md or the cofactor_mode
  • for the former case we are also supposed to raise EC_R_INVALID_DIGEST if the string value cannot be resolved to a valid EVP_MD *
  • the latter is particularly relevant because we historically allowed for underdefined curves that do not specify the cofactor, as it is considered optional in the serialization.
    For such curves, the cofactor used to be set to 0 and with b783bee we are now attempting to compute it, but there might be curves for which the computation fails to deliver the cofactor and so ecdh_cofactor_mode cannot be turned on!
  • libcrypto should return -2 if the provider does not signal these as settable parameters
  • the provider should return a failure if it understands these parameters but something goes wrong when trying to set them, e.g., unknown and uncomputable cofactor, but it is arguably libcrypto responsibility to know that this failure should be mapped to a -2 return value for legacy compatibility

Unfortunately we have very limited unit testing about being consistent w.r.t. legacy about specific return values in case of errors or about checking that the same errors are added to the stack, so there is a chance that we are breaking backward compatibility with this PR and not detecting it also for the RSA and DH that are currently included here.

edit: added more details

@levitte could you have a look at this?
At the moment I am rebasing #10631 and I am tempted to keep the structure of pre-10947 for the EC stuff in legacy_ctr_str_to_param() so that it just parses the input value and calls the dedicated legacy EVP_PKEY_CTX_set_foo() that knows what are the different return values to return in different conditions!

I see now another problem with this: by going directly to the provider interface with EVP_PKEY_CTX_set_params() from inside legacy_ctrl_str_to_param() we are bypassing the dedicated EVP_PKEY_CTX_set_foo().

Er, yes?

So let's see if I can clarify something that seems to be a point of confusion, and I'm going to do see by using the term "backend", as a broad term for "implementations under the hood". In pre-3.0 terms, any EVP_CIPHER, EVP_MD, EVP_PKEY_METHOD, EVP_PKEY_ASN1_METHOD implements a backend, whether it happens to be built in (our ameth and pmeth files) or coming from en engine, or coming from a provider in the case of EVP_CIPHER and EVP_MD. The EVP API per se doesn't really know, and shouldn't really know what the backends are up to, and this is very visible when you look at how the pre-3.0 EVP_PKEY_CTX_ctrl_str() is implemented (this is from 1.1.1):

So what had happened previous to this PR was that code that belonged in the backends was moved to central EVP functions when it should have moved from the old backend (whatever ameth or pmeth file) to the new backends (providers). That was wrong, on principle.

we force every external provider to implement whatever workarounds we devise inside the providers we distribute to retain the legacy behavior.

Do we? Would you claim that any ENGINE implementor that wanted to implement their version of, say, RSA, was forced to implement the exact behaviour the our backends (crypto/rsa/rsa_ameth.c or crypto/rsa/rsa_pmeth.c)? To what extent? 'cause believe you me, that wasn't necessarily the case...

The very simple truth is central EVP code such as EVP_PKEY_CTX_ctrl_str() cannot possibly know what a backend will or will not accept, the design is to simply pass on what it gets, in both directions (except for, weirdly enough, "digest", in the pre-3.0 code... that one confuses me a bit).

All this being said, you're quite right that there are things that aren't the same between the old (ameth and pmeth code) and the new (providers) backends, and that can be summed up in three parts:

1. The control names aren't necessarily the same as the parameter names (we could have made them the same, but quite frankly, the old control names were unnecessarily wordy, like why the hell do we need names like "rsa_keygen_bits" when it should already be known that we're dealing with RSA, and that we're doing key generation? "bits" can and should be enough!), so legacy_ctrl_str_to_param translates the names it currently know, and I fully expect to see that list getting longer.
2. Provider code isn't expected to have to parse the parameter value, such as all the atoi() calls we see. The only place where the provider code still needs to fend for itself is with control names such av "rsa_pss_saltlen", where our implementation expects certain key words or a number, but other than that, OSSL_PARAM_allocate_from_text() does everything it can to parse the value into something that the provider can handle, according to what it declares.
3. Return values. Now... here's something that I did gloss over a bit in this PR (and so did everyone that reviewed), and I agree that OSSL_PARAM_allocate_from_text() should be able to indicate to the caller that a parameter wasn't recognised, letting the caller act appropriately on that, such as having legacy_ctrl_str_to_params() return -2.

Now... to your analysis of return codes:

• -2 if this EVP_PKEY_CTX is not for a DERIVE operation
  • does not concern the provider implementation
  • libcrypto should return -2 to the caller if this condition is detected

I don't understand why EVP_PKEY_CTX_ctrl_str() or legacy_ctrl_str_to_params() should check what operation is currently handled, apart from what EVP_PKEY_CTX_ctrl_str() already does to differentiate calls that should go to providers from calls that should go to our legacy backends. The method pointed at by ctx->pmeth (in the legacy case) or others (in the provider case, EVP_PKEY_CTX_set_params() takes care of ensuring that the correct one is used) should be trusted. This was true in pre-3.0 EVP_PKEY_CTX_ctrLstr(), it should contrinue to be true going forward.

• -1 if this EVP_PKEY_CTX does not contain an EVP_PKEY_EC key
  • does not concern the provider implementation
  • libcrypto should return -1 to the caller if this condition is detected

Er.... If the backend code to handle an EVP_PKEY_EC key (or corresponding keytype in the provider case) is being called by EVP_PKEY_CTX_ctrl_str() with a key that's not an EC key, then something else is deeply wrong, and this isn't within EVP_PKEY_CTX_ctrl_str()'s control.

• -2 if this EVP_PKEY_CTX does not support setting the kdf_md or the cofactor_mode

Now, here you do have a point, and that gets back to what I said in 3. above, that legacy_ctrl_str_to_params() needs to be able to return -2 when it can detect that a specific parameter name isn't supported by the provider code. I agree with this concern. Fortunately, OSSL_PARAM_allocate_from_text() is a new function, we can change what it returns for this sort of case.

• for the former case we are also supposed to raise EC_R_INVALID_DIGEST if the string value cannot be resolved to a valid EVP_MD *

If you look at other code that we have moved from diverse built in methods to our providers, we haven't concerned ourselves with preserving exact error codes. They are all over the place regardless.

• the latter is particularly relevant because we historically allowed for underdefined curves that do not specify the cofactor, as it is considered optional in the serialization.

This is again about -2, right? I hope I have answered that clearly enough.

For such curves, the cofactor used to be set to 0 and with b783bee we are now attempting to compute it, but there might be curves for which the computation fails to deliver the cofactor and so ecdh_cofactor_mode cannot be turned on!

Er... how is this a concern specifically for control values (legacy) or parameters (provider) being passed to the backend via EVP_PKEY_CTX_ctrl_str()? This sounds much more like a key creation thing.

I see now another problem with this: by going directly to the provider interface with EVP_PKEY_CTX_set_params() from inside legacy_ctrl_str_to_param() we are bypassing the dedicated EVP_PKEY_CTX_set_foo().

Er, yes?

It's not that I don't like the idea of doing it like this, it's just that I am afraid we are breaking backward compatibility: before EVP_PKEY_CTX_ctrl_str() was basically a string based front end for some of the EVP_PKEY_CTX_ctrl() options, now we are skipping altogether that layer, and there is a risk of pushing legacy hacks onto the providers if they want to preserve backward compatibility.

The EVP API per se doesn't really know, and shouldn't really know what the backends are up to, and this is very visible when you look at how the pre-3.0 EVP_PKEY_CTX_ctrl_str() is implemented (this is from 1.1.1):

openssl/crypto/evp/pmeth_lib.c

Lines 407 to 418 in ff6d0a6

	int EVP_PKEY_CTX_ctrl_str(EVP_PKEY_CTX *ctx,
	const char *name, const char *value)
	{
	if (!ctx || !ctx->pmeth || !ctx->pmeth->ctrl_str) {
	EVPerr(EVP_F_EVP_PKEY_CTX_CTRL_STR, EVP_R_COMMAND_NOT_SUPPORTED);
	return -2;
	}
	if (strcmp(name, "digest") == 0)
	return EVP_PKEY_CTX_md(ctx, EVP_PKEY_OP_TYPE_SIG, EVP_PKEY_CTRL_MD,
	value);
	return ctx->pmeth->ctrl_str(ctx, name, value);
	}

This is not the whole picture though, as the code I linked before (also from 1.1.1) shows

openssl/crypto/ec/ec_pmeth.c

Lines 354 to 392 in 894da2f

	static int pkey_ec_ctrl_str(EVP_PKEY_CTX *ctx,
	const char *type, const char *value)
	{
	if (strcmp(type, "ec_paramgen_curve") == 0) {
	int nid;
	nid = EC_curve_nist2nid(value);
	if (nid == NID_undef)
	nid = OBJ_sn2nid(value);
	if (nid == NID_undef)
	nid = OBJ_ln2nid(value);
	if (nid == NID_undef) {
	ECerr(EC_F_PKEY_EC_CTRL_STR, EC_R_INVALID_CURVE);
	return 0;
	}
	return EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx, nid);
	} else if (strcmp(type, "ec_param_enc") == 0) {
	int param_enc;
	if (strcmp(value, "explicit") == 0)
	param_enc = 0;
	else if (strcmp(value, "named_curve") == 0)
	param_enc = OPENSSL_EC_NAMED_CURVE;
	else
	return -2;
	return EVP_PKEY_CTX_set_ec_param_enc(ctx, param_enc);
	} else if (strcmp(type, "ecdh_kdf_md") == 0) {
	const EVP_MD *md;
	if ((md = EVP_get_digestbyname(value)) == NULL) {
	ECerr(EC_F_PKEY_EC_CTRL_STR, EC_R_INVALID_DIGEST);
	return 0;
	}
	return EVP_PKEY_CTX_set_ecdh_kdf_md(ctx, md);
	} else if (strcmp(type, "ecdh_cofactor_mode") == 0) {
	int co_mode;
	co_mode = atoi(value);
	return EVP_PKEY_CTX_set_ecdh_cofactor_mode(ctx, co_mode);
	}
	return -2;
	}

EVP_PKEY_CTX_ctrl_str() calls into the backed, which does parsing and minimal checks before calling EVP_PKEY_CTX_ctrl(), which does a lot of checks before eventually calling back to pmeth->ctrl implementation for that backend.
In turn the pmeth->ctrl implementation also had to adhere to the conventions set by the EVP API about behaviour of EVP_PKEY_CTX_ctrl() so that it was indeed possible to use this kind of controls more or less uniformly across different backends.

we force every external provider to implement whatever workarounds we devise inside the providers we distribute to retain the legacy behavior.

Do we? Would you claim that any ENGINE implementor that wanted to implement their version of, say, RSA, was forced to implement the exact behaviour the our backends (crypto/rsa/rsa_ameth.c or crypto/rsa/rsa_pmeth.c)? To what extent? 'cause believe you me, that wasn't necessarily the case...

ENGINEs imposed their own restraints on external implementers, and returning the same return values and error codes for at least a subset of these controls was indeed one of the restraints, but you are definitely more knowledgeable than me about ENGINEs so I probably am missing your point.

Here what I see as a problem is that if to behave like legacy a specific EVP_PKEY_CTX_ctrl_str() needs to do something shady like returning -1 for some errors, 0 for others and -2 for some more, by calling directly into EVP_PKEY_CTX_set_params() instead of routing through the dedicated EVP_PKEY_CTX_set_foo() we are pushing the task of being compatible with legacy onto the provider-side handling of OSSL_PARAMS, which I don't think is what we want!

The very simple truth is central EVP code such as EVP_PKEY_CTX_ctrl_str() cannot possibly know what a backend will or will not accept, the design is to simply pass on what it gets, in both directions (except for, weirdly enough, "digest", in the pre-3.0 code... that one confuses me a bit).

This is fine, as long the complexity of being backward compatible with legacy libcrypto behavior is not pushed onto the provider implementation.
Any "we used to do this in this case, so we have to add checks and try to reproduce as closely as possible what we did before" logic pertains inside the libcrypto boundary and not in the provider.
It can't be on provider side, otherwise we are pushing these workarounds onto every external provider implementation, and it becomes a hard requirement if the external implementer wants to avoid breaking legacy applications when the user loads their provider.

It is fine to resolve that being backward compatible with return values or error codes is just nit picking and deliberate that we will break our promise in this niche aspect, but it should still be discussed (and probably OMC voted).

All this being said, you're quite right that there are things that aren't the same between the old (ameth and pmeth code) and the new (providers) backends, and that can be summed up in three parts:

1. The control names aren't necessarily the same as the parameter names [...]

Agreed!

2. Provider code isn't expected to have to parse the parameter value, such as all the atoi() calls we see. [...]

Agreed!

3. Return values. [...]

Agreed!

Now... to your analysis of return codes:

• -2 -1 if this EVP_PKEY_CTX is not for a DERIVE operation

  • does not concern the provider implementation
  • libcrypto should return -2 -1 to the caller if this condition is detected

I don't understand why EVP_PKEY_CTX_ctrl_str() or legacy_ctrl_str_to_params() should check what operation is currently handled, apart from what EVP_PKEY_CTX_ctrl_str() already does to differentiate calls that should go to providers from calls that should go to our legacy backends. The method pointed at by ctx->pmeth (in the legacy case) or others (in the provider case, EVP_PKEY_CTX_set_params() takes care of ensuring that the correct one is used) should be trusted. This was true in pre-3.0 EVP_PKEY_CTX_ctrLstr(), it should contrinue to be true going forward.

Let's dive into the handling of "ecdh_cofactor_mode" from the point of view of a 1.1.1 application calling EVP_PKEY_CTX_ctrl_str() and evaluate which layer checks what and what values are returned and error codes raised for the cases where something fails.

(shows 3 different conditions under which EVP_PKEY_CTX_ctrl_str() could return -1, each with specific error codes, and other 2 possible cases for returning -2)

(return -2 if p1 out of range, or if the group is missing)

So

should answer to the "why legacy_ctrl_str_to_param() or EVP_PKEY_CTX_ctrl_str() should check what operation is currently handled": because pre-3.0 was doing so and signaling with specific error codes and return value.

• -1 if this EVP_PKEY_CTX does not contain an EVP_PKEY_EC key

  • does not concern the provider implementation
  • libcrypto should return -1 to the caller if this condition is detected

Er.... If the backend code to handle an EVP_PKEY_EC key (or corresponding keytype in the provider case) is being called by EVP_PKEY_CTX_ctrl_str() with a key that's not an EC key, then something else is deeply wrong, and this isn't within EVP_PKEY_CTX_ctrl_str()'s control.

Agreed that if this happens there are probably bigger problems: before 3.0 we used to handle these cases in EVP_PKEY_CTX_ctrl_str() and to signal with specific return values and error codes.
Now we don't anymore, and it is a potentially breaking change, which at least requires discussion, probably a vote, and an entry in the document that we will probably write for alpha1 to invite external developers to test their applications, engines, providers against 3.0 and pay attention to some details that we deliberately modified in the transition to 3.0.

• -2 if this EVP_PKEY_CTX does not support setting the kdf_md or the cofactor_mode

Now, here you do have a point, and that gets back to what I said in 3. above, that legacy_ctrl_str_to_params() needs to be able to return -2 when it can detect that a specific parameter name isn't supported by the provider code. I agree with this concern. Fortunately, OSSL_PARAM_allocate_from_text() is a new function, we can change what it returns for this sort of case.

I agree this should be manageable without too much fuss!

• for the former case we are also supposed to raise EC_R_INVALID_DIGEST if the string value cannot be resolved to a valid EVP_MD *

If you look at other code that we have moved from diverse built in methods to our providers, we haven't concerned ourselves with preserving exact error codes. They are all over the place regardless.

I noticed this, as well as the fact that in the DH provider implementation we have TODOs to raise errors.
What I wasn't aware of is the fact that we deliberately plan to break compatibility about error codes (we don't guarantee we check for all the corner cases we were checking before, we might change the reason name, same reason might have a different numeric value).
Was there a discussion about this that I missed?

• the latter is particularly relevant because we historically allowed for underdefined curves that do not specify the cofactor, as it is considered optional in the serialization.

This is again about -2, right? I hope I have answered that clearly enough.

Yes, the -2 on not supported by provider is clear enough.

For such curves, the cofactor used to be [...]

Er... how is this a concern specifically for control values (legacy) or parameters (provider) being passed to the backend via EVP_PKEY_CTX_ctrl_str()? This sounds much more like a key creation thing.

This was just meant to be an example of a case where it was and is reasonable that applications checked for that -2 and to the error stack to handle the case of "we wished to use cofactor ecdh, it was not supported, but maybe it is one of those special alien curves that need special care to turn cofactor mode on".

So

openssl/crypto/evp/pmeth_lib.c

Lines 387 to 390 in 894da2f

	if ((optype != -1) && !(ctx->operation & optype)) {
	EVPerr(EVP_F_EVP_PKEY_CTX_CTRL, EVP_R_INVALID_OPERATION);
	return -1;
	}

should answer to the "why legacy_ctrl_str_to_param() or EVP_PKEY_CTX_ctrl_str() should check what operation is currently handled": because pre-3.0 was doing so and signaling with specific error codes and return value.

It does not. There's a huge structural difference that you've ignored. In legacy code EVP_PKEY_METHOD is one big method that handles all sorts of operations using asymmetric keys, while we have split them up in provider code, so we have EVP_SIGNATURE, EVP_ASYM_CIPHER, EVP_KEYEXCH et al (will there be others? I've lost track), so the check already happens at a higher level before provider code is even reached. Therefore, the provider code does not need to make that kind of check.

What I would expect, the day that legacy code goes away, is that this:

gets replace with this:

     ERR_raise(ERR_LIB_EVP, EVP_R_INVALID_OPERATION); 
     return -1;

(or quite frankly, I hope that everything called _ctrl is gone by then, because it's considered legacy)

but since we currently have a mixture of providers and legacy backends, we simply let legacy take care of whatever there's no provider code.

(return -2 if p1 out of range, or if the group is missing)

I actually think that returning -2 in such cases is wrong. 0 should be returned, as it is an error.

The manual for EVP_PKEY_CTX_ctrl() sheds a bit of clarity when -2 can be expected (italics mine, for clarification):

The function EVP_PKEY_CTX_ctrl() sends a control operation to the
context ctx. The key type used must match keytype if it is not -1.
...
RETURN VALUES
EVP_PKEY_CTX_ctrl() and its macros return a positive value for success
and 0 or a negative value for failure. In particular a return value of
-2 indicates the operation is not supported by the public key
algorithm.

"the operation", here, refers to the cmd, i.e. things like EVP_PKEY_CTRL_EC_ECDH_COFACTOR. Why someone thought it was a good idea to use the same return value because the value sent in with that cmd is beyond me.

As for the diverse EVP_PKEY_CTX_set_whatever() that you see in the legacy backends, I assume you understand that they aren't expected to be reproduced in provider code.

Regarding the first half of #10947 (comment)

I am not ignoring the fact that the architectural reorganization makes the check likely superfluous, I am just concerned that by skipping it (because it is superfluous and hopefully triggered before calling any ctrl function) we might be altering the return value and error codes raised in these corner cases.

I am likely to be overthinking this, and probably you are completely right in assuming that these checks even if present would never trigger the special return value and raised error, but without extensive testing we have no way of guaranteeing this is the case!

I actually think that returning -2 in such cases is wrong. 0 should be returned, as it is an error.

I tend to agree with you, and can't wait for the moment we will drop the ctrl functions, but even if it is an error, it is an ossified behavior from at least 1.0.2, that should require a discussion before being changed!

@romen, you should check #11046, I hope that it answers some of your concerns

I tend to agree with you, and can't wait for the moment we will drop the ctrl functions, but even if it is an error, it is an ossified behavior from at least 1.0.2, that should require a discussion before being changed!

If we really want to keep (wrongly) returning -2 for incorrect parameter values, then I can only see one answer, that the providers will have to return -2 in those cases. Trying to do some magic in the EVP code will only create a total mess, of which we already saw the effect before this PR.

If we really want to keep (wrongly) returning -2 for incorrect parameter values, then I can only see one answer, that the providers will have to return -2 in those cases.

If we (or OMC) doesn't vote to approve breaking legacy compatibility we should figure out where all that ugliness would need to go.

I would argue that, with libcrypto becoming essentially a framework, any temporary workaround to maintain our promise of backward compatibility must be handled inside the framework code, no matter how ugly, and can't really be forced on external providers!

Maybe some other reviewer may chime in: does this really constitute breaking behavior? Do we need an OMC vote to get rid of the ugliness?

I will look at #11046 as soon as possible!