EVP: Implement data-driven translation between known ctrl and OSSL_PARAMs by levitte · Pull Request #13913 · openssl/openssl

levitte · 2021-01-20T22:18:23Z

The idea is to make it as transparent as possible to call things like
EVP_PKEY_CTX_ctrl() with a provider backed EVP_PKEY_CTX, or things
like EVP_PKEY_get_bn_param() with a legacy EVP_PKEY.

All these sorts of calls demand that we translate between ctrl
commands and OSSL_PARAM keys, and treat the arguments appropriately.

This implementation has it being as data driven as possible, thereby
centralizing everything into one table of translation data, which
supports both directions.

Fixes #13528

levitte · 2021-01-20T22:20:23Z

Note that this isn't finished yet, there are fixup functions missing. However, the functionality and structure have crystallised enough to make the idea reviewable. Please comment, even it doesn't build, early feedback welcome!

richsalz · 2021-01-20T23:11:59Z

The fixup functions need to know the insize and outsize of all params, right? This means that third-party providers cant add new controls. Or am I just misreading the code?

levitte · 2021-01-21T07:59:01Z

@richsalz, I think you've misunderstood the purpose.

This is entirely in libcrypto and about stuff that libcrypto knows. Period. Providers are not involved (more than being recipients of OSSL_PARAM data), at all.
This is a tool that translates between ctrl functions and ctrl command numbers that libcrypto knows and OSSL_PARAM functions and names that libcrypto knows, in both directions.

Regarding insize and outsize, the OSSL_PARAM structure already delivers that. However, the limiting factor is the ctrl functions, which do not easily supply this (well, you can pass the insize as p1, and you can ask for an outsize with a separate ctrl command and passing the pointer to a size_t as p2...)

So in the end, this is meant to support calling EVP_PKEY_CTX_ctrl() with an EVP_PKEY_CTX that's set up for a provider backed operation, to support calling EVP_PKEY_CTX_set_params() with an EVP_PKEY_CTX that's set up for a legacy operation (limited to things libcrypto knows about), and to support calling EVP_PKEY_get_bn_param() with a legacy EVP_PKEY.

The life time of this source file is the same as the lifetime of legacy EVP_PKEYs, I totally expect them to be gone when ENGINE support is gone (ENGINE support being the last tangible reason why we retain legacy style EVP_PKEYs). That's also a reason to have all of this collected into one file.

(while I'm at it, I also mean to clean away the current hackery which is spread all over the source instead of being concentrated to one spot, and that includes huge amounts of code copying ... you've heard me battle that tendency before 😉)

t8m · 2021-01-21T09:00:20Z

This would be a great improvement! It would also allow to drop all the new convenience functions such as EVP_PKEY_get_group_name(). But on the other hand they could still be kept, just with very much simplified implementation.

levitte · 2021-01-21T11:59:36Z

This would be a great improvement! It would also allow to drop all the new convenience functions such as EVP_PKEY_get_group_name(). But on the other hand they could still be kept, just with very much simplified implementation.

... and [ahem] we can revert all the older convenience functions to being macros calling EVP_PKEY_CTX_ctrl() 😆

richsalz · 2021-01-21T15:56:27Z

Thank you for the detailed explanation. That should make it into the code/comments I think.

At one point we liked the wrapper functions because they provided type-safety.

t8m · 2021-01-21T16:16:11Z

Thank you for the detailed explanation. That should make it into the code/comments I think.

At one point we liked the wrapper functions because they provided type-safety.

IMO they definitely make sense for more common things. However for obscure parameters they make less sense as they contribute to API and code bloat.

levitte · 2021-01-21T16:24:10Z

At one point we liked the wrapper functions because they provided type-safety.

I agree that for ctrl wrapper, that's a nice benefit.

jon-oracle · 2021-01-27T03:47:09Z

Is there some test coverage in this area? I have some ad-hoc tests for legacy vs new EVP_PKEYs which I can clean up and supply a commit for.

levitte · 2021-01-27T05:42:17Z

Please do, thank you 😊

jon-oracle · 2021-01-27T08:10:29Z

Some tests. Will add checks for other get/set APIs in due course:

5198095

levitte · 2021-01-27T09:27:10Z

Thanks

Side note: github seemed to assume that commit belongs in openssl/openssl... it took me a moment to find out where it really belonged 😉

t8m · 2021-02-04T14:02:23Z

Linked to otc evaluated issue.

t8m · 2021-02-10T13:09:11Z

There is one thing that comes to me a little bit awkward is that for the helper functions that used to call either set/get params or ctrl based on whether the key is legacy (like for example EVP_PKEY_CTX_set_dh_kdf_type()) we now call just the ctrl. Which is an obsolete API call so perhaps we should avoid using it internally. On the other hand it is indicative that it is extremely simple to convert the helpers to the ctrl call (as they were previously just macros, it makes it kind of obvious) but it is fairly harder to use the set/get_params instead. Could this mean we are missing some helper functions or macros for working with params?

levitte · 2021-02-22T10:36:42Z

Rebasing to resolve conflicts

t8m

Approved!

paulidale

Some little nits. Nothing significant.

Good comments, they really help with comprehension.

It's a bit of a shame to have such beautiful code for legacy support.

crypto/evp/ctrl_params_translate.c

…ions

levitte · 2021-02-23T11:25:37Z

It's a bit of a shame to have such beautiful code for legacy support.

Legacy will go, but at least it will get to leave with some panache 😉

t8m · 2021-02-23T11:43:25Z

This triggers a question - should we deprecate EVP_PKEY_CTX_ctrl and all the other EVP*ctrl functions?

mattcaswell · 2021-02-23T11:44:47Z

This triggers a question - should we deprecate EVP_PKEY_CTX_ctrl and all the other EVP*ctrl functions?

Yes, we probably should.

t8m · 2021-02-23T11:51:43Z

Issue filed #14287 for the deprecations.

This does what was previously done by looking at pctx->pmeth->pkey_id, but handles both legacy and provider side contexts, and is supposed to become a replacement for the old way. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

This will help with transitioning diverse functions to be able to use the ctrl<->OSSL_PARAM translators. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

…RAMs The idea is to make it as transparent as possible to call things like EVP_PKEY_CTX_ctrl() with a provider backed EVP_PKEY_CTX, or things like EVP_PKEY_get_bn_param() with a legacy EVP_PKEY. All these sorts of calls demand that we translate between ctrl commands and OSSL_PARAM keys, and treat the arguments appropriately. This implementation has it being as data driven as possible, thereby centralizing everything into one table of translation data, which supports both directions. Fixes #13528 Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

In the interest of calling these functions on legacy EVP_PKEY contexts, only check the settable / gettable params for provider side keys, leaving to the translated EVP_PKEY_CTX_ctrl() call check the ctrl commands on its own. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

EVP_PKEY_get_group_name() now simply calls EVP_PKEY_get_utf8_string_param(). EVP_PKEY_CTX_set_group_name() now simply calls EVP_PKEY_CTX_set_params(). EVP_PKEY_get_bn_param(), EVP_PKEY_get_octet_string_param(), EVP_PKEY_get_utf8_string_param() and EVP_PKEY_get_int_param() can now handle legacy EVP_PKEYs by calling evp_pkey_get_params_to_ctrl(). EVP_PKEY_CTX_get_params() can now handle a legacy backed EVP_PKEY_CTX by calling evp_pkey_ctx_get_params_to_ctrl(). Note: EVP_PKEY_CTX_set_params() doesn't call the translator yet. Should it ever? Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

legacy_ctrl_to_param() and legacy_ctrl_str_to_param() are now replaced with calls to evp_pkey_ctx_ctrl_to_param() and evp_pkey_ctx_ctrl_str_to_param(). Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

It would check the keytype and optype before determining if it even supported the ctrl command number. This turned out to be disruptive, so we make it check that it supports the request ctrl command number first. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

levitte · 2021-02-23T12:45:52Z

Merged

6179dfc EVP: Implement EVP_PKEY_CTX_is_a()
e19246d EVP: Make evp_pkey_ctx_state() available to all of EVP
4d4928e EVP: make evp_pkey_is_assigned() usable in the FIPS module
9a1c4e4 EVP: Implement data-driven translation between known ctrl and OSSL_PARAMs
5137312 EVP: Make evp_pkey_ctx_{set,get}_params_strict() legacy aware
6fcd92d EVP: Adapt diverse OSSL_PARAM setters and getters
5524580 EVP: Adapt the EVP_PKEY_CTX ctrl functions
df4592c EVP: Adapt the DH specific EVP_PKEY_CTX setter / getter functions
13f91a7 EVP: Adapt the RSA specific EVP_PKEY_CTX setter / getter functions
bbf4dc9 EVP: Make checks in evp_pkey_ctx_store_cached_data() more restricted
f5b0083 EVP: Adapt the EC_KEY specific EVP_PKEY_CTX setter / getter functions

levitte added the branch: master Applies to master branch label Jan 20, 2021

levitte mentioned this pull request Jan 20, 2021

Calling EVP_PKEY_get_bn_param() on a legacy key does not work #13528

Closed

t8m mentioned this pull request Jan 21, 2021

Deprecate EC harder(er) #13139

Merged

2 tasks

levitte force-pushed the ctrl-params-translate branch from e1afcbd to 0cc392f Compare January 25, 2021 14:53

levitte added this to the 3.0.0 beta1 milestone Jan 26, 2021

t8m added the triaged: OTC evaluated label Feb 4, 2021

paulnelsontx self-assigned this Feb 4, 2021

levitte force-pushed the ctrl-params-translate branch 2 times, most recently from 9bb0069 to 634fc52 Compare February 5, 2021 09:17

paulnelsontx modified the milestones: 3.0.0 beta1, Sprint-Hydrogen Feb 8, 2021

levitte force-pushed the ctrl-params-translate branch from 23b0d4c to bce4661 Compare February 8, 2021 16:27

paulnelsontx modified the milestones: Sprint-Hydrogen, Sprint Hydrogen Feb 8, 2021

levitte force-pushed the ctrl-params-translate branch from 00f8f72 to df75f5a Compare February 8, 2021 22:25

levitte mentioned this pull request Feb 10, 2021

Remove TODO(3.0) tags in pkey ctrl functions. #12998

Closed

2 tasks

levitte force-pushed the ctrl-params-translate branch from 99bc21d to 2354269 Compare February 22, 2021 10:36

t8m approved these changes Feb 22, 2021

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 22, 2021

paulidale approved these changes Feb 22, 2021

View reviewed changes

fixup! EVP: Adapt the RSA specific EVP_PKEY_CTX setter / getter funct…

9a6486e

…ions

paulidale approved these changes Feb 22, 2021

View reviewed changes

t8m approved these changes Feb 23, 2021

View reviewed changes

t8m mentioned this pull request Feb 23, 2021

Deprecate EVP_PKEY_CTX_ctrl and related functions #14287

Closed

levitte added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Feb 23, 2021

openssl-machine pushed a commit that referenced this pull request Feb 23, 2021

EVP: make evp_pkey_is_assigned() usable in the FIPS module

4d4928e

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

openssl-machine pushed a commit that referenced this pull request Feb 23, 2021

EVP: Adapt the DH specific EVP_PKEY_CTX setter / getter functions

df4592c

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

openssl-machine pushed a commit that referenced this pull request Feb 23, 2021

EVP: Adapt the RSA specific EVP_PKEY_CTX setter / getter functions

13f91a7

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

openssl-machine pushed a commit that referenced this pull request Feb 23, 2021

EVP: Adapt the EC_KEY specific EVP_PKEY_CTX setter / getter functions

f5b0083

Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #13913)

levitte closed this Feb 23, 2021

levitte deleted the ctrl-params-translate branch February 23, 2021 12:45

Uh oh!

Comments

Conversation

levitte commented Jan 20, 2021

Uh oh!

levitte commented Jan 20, 2021

Uh oh!

richsalz commented Jan 20, 2021

Uh oh!

levitte commented Jan 21, 2021

Uh oh!

t8m commented Jan 21, 2021

Uh oh!

levitte commented Jan 21, 2021

Uh oh!

richsalz commented Jan 21, 2021

Uh oh!

t8m commented Jan 21, 2021

Uh oh!

levitte commented Jan 21, 2021

Uh oh!

jon-oracle commented Jan 27, 2021

Uh oh!

levitte commented Jan 27, 2021

Uh oh!

jon-oracle commented Jan 27, 2021

Uh oh!

levitte commented Jan 27, 2021

Uh oh!

t8m commented Feb 4, 2021

Uh oh!

t8m commented Feb 10, 2021

Uh oh!

levitte commented Feb 22, 2021

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

levitte commented Feb 23, 2021

Uh oh!

t8m commented Feb 23, 2021

Uh oh!

mattcaswell commented Feb 23, 2021

Uh oh!

t8m commented Feb 23, 2021

Uh oh!

levitte commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants