Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make EVP_MD_CTX_[gettable|settable]_params() take an EVP_MD_CTX #9998

Closed
wants to merge 1 commit into from

Conversation

@mattcaswell
Copy link
Member

mattcaswell commented Sep 24, 2019

EVP_MD_CTX_gettable_params() and EVP_MD_CTX_settable_params() were
confusingly named because they did not take an EVP_MD_CTX parameter. In
fact they really should take a ctx since the parameters may be influenced
by how that ctx has been set up.

@mattcaswell mattcaswell added this to In progress in 3.0 New Core + FIPS via automation Sep 24, 2019
@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

A similar change will also be needed for MACs and CIPHERs which have a similar problem.

This PR is needed for an upcoming PR for moving EVP_Digest[Sign|Verify]*() to the providers. There the gettable/settable params are influenced by how things have been initialised.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

Could you tell me of an actual case where the parameters would actually change depending on context? How does one even list that?????

I do agree that the API naming isn't the brightest, but I do believe this PR takes it in the wrong direction, unless you have a truly compelling case.

My counter proposal would be this rename:

EVP_{type}_CTX_gettable_params() => EVP_{type}_gettable_ctx_params()
EVP_{type}_CTX_settable_params() => EVP_{type}_settable_ctx_params()

That does better reflect the intention with those functions.

@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

I hit this problem when converting EVP_DigestSign*/EVP_DigestVerify*() (still a work in progress - not yet made it as far as a PR).

EVP_DigestSignInit() looks like this:

 int EVP_DigestSignInit(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
                        const EVP_MD *type, ENGINE *e, EVP_PKEY *pkey);

My WIP code introduces a "provider aware" version will be like this:

 int EVP_DigestSignInit_ex(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
                           const char *mdname, const char *props,
                           EVP_PKEY *pkey, EVP_SIGNATURE *signature);

The idea is that the provider operation for DigestSign is based on EVP_SIGNATURE. The actual digest bit is up to the provider to resolve. We just pass in the algorithm name here and it is up to the provider to figure out how to do that. Our own providers will do a "fetch" based on the digest name. The digest name could be something entirely internal to that provider. Some externally provided DigestSign operations may not even have a separately fetchable digest associated with it. Actually that's even the case for some of ours today. For example implementations of "Pure" EdDSA do the hashing as part of the signature algorithm - its not a separate thing at all and there is no "EVP_MD" object for this.
Nonetheless, in the case of DigestSign, there is still a separate EVP_MD_CTX and we need to be able to pass digest related parameters to the operation. So how do you call EVP_MD_CTX_gettable_params() which takes an EVP_MD as a parameter, if there is no separate EVP_MD object?

@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

As a further illustration of this see this comment on the Ed25519 page:

When calling EVP_DigestSignInit() or EVP_DigestVerifyInit(), the
digest B<type> parameter B<MUST> be set to B<NULL>.

So if you're using Ed25519 - you have an EVP_MD_CTX but no EVP_MD.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

If the EVP_MD_CTX doesn't have an associated EVP_MD, then there are no settable/gettable parameters to be had, 'cause there are no supporting funtions to handle parameters.

This sounds like EVP_SIGNATURE needs to be able to handle parameters. Now, I understand that some of those parameters would simply be passed on to the associated digest implementation if there is one. I think it would be perfectly admissible to actually have that happen, that an EVP_SIGNATURE implementation could pass along params. The consequence of this is that for anyone to find out all the gettable/settable parameters for a specific asym and digest implementation combo, they would have to do something like this:

    const EVP_MD *md = EVP_MD_CTX_md(mdctx);
    const OSSL_PARAM *mdparams =
        md == NULL ? NULL : EVP_MD_gettable_ctx_params(md);
    const OSSL_PARAM *sigparams = EVP_SIGNATURE_gettable_ctx_params(sig);
@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

This sounds like EVP_SIGNATURE needs to be able to handle parameters. Now, I understand that some of those parameters would simply be passed on to the associated digest implementation if there is one. I think it would be perfectly admissible to actually have that happen, that an EVP_SIGNATURE implementation could pass along params. The consequence of this is that for anyone to find out all the gettable/settable parameters for a specific asym and digest implementation combo, they would have to do something like this:

I think this is very very wrong. It is counter intuitive for users of the API. They should be able to do the same thing no matter what kind of EVP_SIGNATURE they have got. For our own providers it would mean that params targetted at the MD would go to the EVP_SIGNATURE instead. We then have to separate out the parameters destined for our own EVP_SIGNATURE and process those and then pass along any other parameters to the digest implementation. This incidentally implies that that EVP_SIGNATURE and EVP_MD paramater name namespace is one and the same to avoid clashes.
I did consider this scheme during my EVP_DIgestSign* implementation work but rejected it as unworkable and unnecessarily complex.

Instead I see EVP_SIGNATURE implementations being able to respond to two different types of parameter:

OSSL_CORE_MAKE_FUNC(const OSSL_PARAM *, OP_signature_gettable_ctx_params,
                    (void))
OSSL_CORE_MAKE_FUNC(int, OP_signature_set_ctx_params,
                    (void *ctx, const OSSL_PARAM params[]))
OSSL_CORE_MAKE_FUNC(const OSSL_PARAM *, OP_signature_settable_ctx_params,
                    (void))
OSSL_CORE_MAKE_FUNC(int, OP_signature_get_ctx_md_params,
                    (void *ctx, OSSL_PARAM params[]))
OSSL_CORE_MAKE_FUNC(const OSSL_PARAM *, OP_signature_gettable_ctx_md_params,
                    (void))
OSSL_CORE_MAKE_FUNC(int, OP_signature_set_ctx_md_params,
                    (void *ctx, const OSSL_PARAM params[]))
OSSL_CORE_MAKE_FUNC(const OSSL_PARAM *, OP_signature_settable_ctx_md_params,
                    (void))

Then we just get EVP_MD_CTX_set_params() call the set_ctx_md_params function, whereas EVP_PKEY_CTX_set_params() calls set_ctx_params.

@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

If the EVP_MD_CTX doesn't have an associated EVP_MD, then there are no settable/gettable parameters to be had, 'cause there are no supporting funtions to handle parameters.

This is not the case if the provider has fetched the digest internally. In that case libcrypto does not have the EVP_MD that is being used - but there is no reason why the EVP_MD_CTX shouldn't be able to accept md related params.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

We kinda have the shared namespace thing going on already, if you look at KDFs. They have a different take, that they simply take a few parameters that they will then pass the value of down to the underlying algorithm. That's kinda sorta what you proposed just now, except that you're splitting signature parameters and underlying digest parameters apart, while KDFs have them as their own.

That too could be food for thought; have it all being signature params, with the knowledge that some of the values will be passed down in whatever way the provided implementation finds suitable.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

That being said, having the signature impl provide underlying digest parameters through a separate function is acceptable as well.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

As for EVP_MD_CTX being able to take certain parameters, yes of course that's possible... for parameters that are purely in the structure itself, i.e. libcrypto side. These aren't parameters that will be passed to a provided implementation, right? Or how do you figure that would work? A cache, in the hope that some implementation will eventually accept them?

@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

These aren't parameters that will be passed to a provided implementation, right? Or how do you figure that would work? A cache, in the hope that some implementation will eventually accept them?

These are parameters that will be passed to a provided implementation. It works in exactly the same way as EVP_MD_CTX_ctrl works, i.e. you're not supposed to call it until its been init'd. If you attempt to call it before then you get an error. As soon as its been init'd you've got an implementation and you can pass down the parameters.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

Side thing, btw... in KDFs and so on, we pass the underlying MD name through OSSL_PARAM. Is there a reason to not use a similar way with signatures? For signature algos that don't need it (pure EdDSA), simply don't pass such a parameter... I don't mind terribly much passing it directly, but was thinking that for API consistency, that could be a way to do it.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

Another thought. API wise, you don't have to follow old patterns if they turn out to be bulky, as long as old APIs can be implemented in terms of the new.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

(done... I'll shut up and wait now 😉)

EVP_MD_CTX_gettable_params() and EVP_MD_CTX_settable_params() were
confusingly named because they did not take an EVP_MD_CTX parameter.

In addition we add the functions EVP_MD_gettable_ctx_params() and
EVP_MD_settable_ctx_params() which do the same thing but are passed
an EVP_MD object instead.
@mattcaswell mattcaswell force-pushed the mattcaswell:evp-md-ctx-gettable branch from e7930b9 to 7864461 Sep 24, 2019
@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

Updated commit force pushed which hopefully will be acceptable to both of us. It implements a compromise position with both EVP_MD_CTX_[gettable|settable]_params() which take an EVP_MD_CTX, and EVP_MD_[gettable|settable]_ctx_params() which take an EVP_MD.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

Meanwhile, I wrote something above for your consideration

Copy link
Member

levitte left a comment

For consistency, we should probably rename all other EVP_{type}_CTX_{set,get}table_params to EVP_{type}_{set,get}table_ctx_params... I have no problem if that happens in another PR, but neither am I opposed if that happens in this PR.

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Sep 24, 2019
@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

Side thing, btw... in KDFs and so on, we pass the underlying MD name through OSSL_PARAM. Is there a reason to not use a similar way with signatures? For signature algos that don't need it (pure EdDSA), simply don't pass such a parameter... I don't mind terribly much passing it directly, but was thinking that for API consistency, that could be a way to do it.

I thought about doing it this way but decided against it. Most algorithms will need this (at least for the foreseeable future) and I kind of feel that if its a parameter that exists in the parameter list of EVP_DigestSign, then it deserves to be passed directly.

@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 24, 2019

For consistency, we should probably rename all other EVP_{type}CTX{set,get}table_params to EVP_{type}_{set,get}table_ctx_params... I have no problem if that happens in another PR, but neither am I opposed if that happens in this PR.

Yes. I'd rather do it in another PR because my EVP_DigestSign work depends on this change, but not on the other similar changes - so I'd like to get this in soon.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 24, 2019

I'd rather do it in another PR

Totally cool. And btw, it doesn't have to be you (or me) 😉

@mattcaswell

This comment has been minimized.

Copy link
Member Author

mattcaswell commented Sep 25, 2019

Pushed. Thanks.

3.0 New Core + FIPS automation moved this from Reviewer approved to Done Sep 25, 2019
levitte pushed a commit that referenced this pull request Sep 25, 2019
EVP_MD_CTX_gettable_params() and EVP_MD_CTX_settable_params() were
confusingly named because they did not take an EVP_MD_CTX parameter.

In addition we add the functions EVP_MD_gettable_ctx_params() and
EVP_MD_settable_ctx_params() which do the same thing but are passed
an EVP_MD object instead.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9998)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.