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

Add params argument to _init calls #14383

Closed
wants to merge 35 commits into from
Closed

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Mar 1, 2021

  • documentation is added or updated
  • tests are added or updated
@paulidale paulidale modified the milestones: 3.0.0 beta1, Sprint Helium Mar 1, 2021
@paulidale paulidale marked this pull request as draft Mar 2, 2021
@paulidale paulidale changed the title Add params argument to key manager's gen_init call Add params argument to _init calls Mar 2, 2021
@paulidale paulidale changed the title Add params argument to _init calls WIP: Add params argument to _init calls Mar 2, 2021
@levitte
Copy link
Member

@levitte levitte commented Mar 2, 2021

The discussion in #14383 (comment) has me wonder, for whose benefit do we make this change? For EVP_MAC and EVP_KDF, it's quite clear, we had an intention to give the user (the callers of the EVP_MAC and EVP_KDF functions) some comfort, so they wouldn't need to call a separate set_params function. That's pretty clear cut.

This change doesn't do anything similar, as it's kept quite internal in libcrypto... I'm not sure that I can see how this benefits the author of a KEYMGMT implementation, and considering that this is only being passed NULL from the libcrypto code, it seems strangely futile.

EDIT: and @paulidale, I'm quite aware that I nudged you in this direction.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 2, 2021

The OTC wanted the params argument on all _init() calls: #11007 (comment) plus the comments after.

I'm having second thoughts -- the same ones you are. The rest is kept quite internal and it would mean a slurry of new APIs to access. Some things can't even be accessed directly currently and it seems silly to add calls just so they can be.

Hence the suggestion to add the argument to the core_dispatch APIs but to not expose it at the libcrypto level. This I could support as a future proofing exercise.

@levitte
Copy link
Member

@levitte levitte commented Mar 2, 2021

The OTC wanted the params argument on all _init() calls: #11007 (comment) plus the comments after.

Yes, and that's primarily about our public API, i.e. user callable functions like EVP_MAC_init(), ... I didn't necessarily hear that as meaning the libcrypto<->provider interface (although I do see the practicality for OSSL_FUNC_mac_init(), but that's more because of the added key + keylen, so it didn't hurt to throw in the params as well)

@t-j-h
Copy link
Member

@t-j-h t-j-h commented Mar 2, 2021

It would be a very strange choice to elect that init's in the provider interface don't have OSSL_PARAMs when we have collectively decided that init's have OSSL_PARAMs - I did not see that OTC decision as limited to only public APIs or that anyone would think that we should have a different approach in terms of capabilities in the public and provider APIs - frankly that didn't occur to me that someone would want to split hairs at that level on an OTC decision.

@levitte
Copy link
Member

@levitte levitte commented Mar 2, 2021

frankly that didn't occur to me that someone would want to split hairs at that level on an OTC decision.

Hmm, I'm pretty sure I have mentioned, more than once, that the public API and the libcrypto<->provider interface aren't necessarily the same thing. For one thing, different crowds are affected; whatever we decide for the libcrypto<->provider interface primarily concern provider authors, while the public API primarily concern application authors.

That being said, I have nothing per se against adding an OSSL_PARAM argument to the provider side init functions, not at all. However, if the only user of those functions (our libcrypto code) does nothing bu pass NULL, it seems like an exercise in futility.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 2, 2021

That being said, I have nothing per se against adding an OSSL_PARAM argument to the provider side init functions, not at all. However, if the only user of those functions (our libcrypto code) does nothing bu pass NULL, it seems like an exercise in futility.

My rationale is that we can't change the provider API so including the params now prevents problems in the future.
That we can't change the libcrypto API means we need to introduce a new function if we add the argument -- this is something we can do later in a dot release.

@levitte
Copy link
Member

@levitte levitte commented Mar 2, 2021

That we can't change the libcrypto API means we need to introduce a new function if we add the argument -- this is something we can do later in a dot release.

Okie, that translates into at least an idea of a future intention. That's what I didn't hear before. That's good enough for me.

@paulidale paulidale force-pushed the paulidale:kmgmt-args branch from 434bd0f to 9cd5668 Mar 2, 2021
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 2, 2021

I've exposed the new functionality to the public API, that was easy. The provider API is horrible now and I've broken a lot of things. Moreover, I'm even less sure of the benefits -- we don't seem to push parameters into PKEY_CTXs much, rather we just do an operation with one. Almost all digests get zero benefit from the changes. For ciphers, it seems like major overkill for little upside.

Apart from MAC and KDF which are parameter heavy, there seems to be little need for this.

It seemed like such a good idea when we discussed it with the OTC....

[/venting]

@paulidale paulidale force-pushed the paulidale:kmgmt-args branch 2 times, most recently from 03de858 to 23b3a80 Mar 3, 2021
@@ -370,6 +366,28 @@ int EVP_CipherInit_ex(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher,
return 1;
}

int EVP_CipherInit_ex2(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *cipher,

This comment has been minimized.

@paulidale

paulidale Mar 3, 2021
Author Contributor

EVP_CIPHER_init() ?

@paulidale paulidale marked this pull request as ready for review Mar 3, 2021
@paulidale paulidale changed the title WIP: Add params argument to _init calls Add params argument to _init calls Mar 3, 2021
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 3, 2021

Time for some reviewing :)

@levitte
Copy link
Member

@levitte levitte commented Mar 5, 2021

Now that we have seen the effect, I think we need to review the OTC decision.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 9, 2021

OTC decision: updating the provider side API to include the params is okay.

openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
… params

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
…th params

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
…th params

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
…the param array is null

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
openssl-machine pushed a commit that referenced this pull request Mar 11, 2021
…rams

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14383)
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Mar 11, 2021

Merged to master. Thanks for the reviews.

@paulidale paulidale closed this Mar 11, 2021
@paulidale paulidale deleted the paulidale:kmgmt-args branch Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants