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

Rework EVP_CIPHER support, enhance existing functions #9328

Closed
wants to merge 14 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Jul 9, 2019

This adds missing support for providers in diverse EVP_CIPHER and EVP_CIPHER_CTX functions. A few helper macros found in crypto/evp/evp_locl.h minimize the code duplication.

One bit may be controversial: I've change the proliferation of small functions return small bits of information, such as key length, iv length, that sort of things, into parameters fetchable and settable with get_params and set_params type functions. My main reasoning is that it makes for a more consistent transfer of information data. Others may disagree and I'm open for other points of view.

The cipher context IV was a bit interesting. EVP_CIPHER_CTX_iv() returns a pointer to the live IV, while EVP_CIPHER_CTX_ctrl() with the type EVP_CTRL_GET_IV gets a copy of the live IV. To support both, we support getting it with both the OSSL_PARAM_OCTET_STRING and OSSL_PARAM_OCTET_PTR datatypes.
This is a first, and a bit unexpected... I hadn't foreseen this kind of use for OSSL_PARAM_OCTET_PTR, and this type of use may not always be very well supported, by providers who do not want to share their internals like this (I expect the FIPS provider to refuse this, since that constitutes an intermediate value that probably shouldn't get out).


NOTE: this PR includes the test change from #9324.
NOTE: this PR overlaps #9301 a bit. I left all the AEAD, GCM and CCM stuff alone 'cause I knew it was there already. But @slontis, we may have to discuss coding strategies, they differ a bit 😉

A lot of the different numbers associated with ciphers are really
algorithm parameters.  Key length, block size, IV length, that sort of
thing.
The cipher context IV was a bit interesting.  EVP_CIPHER_CTX_iv()
returns a pointer to the live IV, while EVP_CIPHER_CTX_ctrl() with the
type EVP_CTRL_GET_IV gets a copy of the live IV.  To support both, we
support getting it with both the OSSL_PARAM_OCTET_STRING and
OSSL_PARAM_OCTET_PTR datatypes.
@levitte levitte added the branch: master Merge to master branch label Jul 9, 2019
crypto/evp/evp_locl.h Outdated Show resolved Hide resolved
crypto/evp/evp_locl.h Outdated Show resolved Hide resolved
crypto/evp/evp_locl.h Outdated Show resolved Hide resolved
crypto/evp/evp_lib.c Outdated Show resolved Hide resolved
crypto/evp/evp_enc.c Outdated Show resolved Hide resolved
crypto/evp/evp_locl.h Outdated Show resolved Hide resolved
crypto/evp/evp_locl.h Outdated Show resolved Hide resolved
@levitte
Copy link
Member Author

levitte commented Jul 10, 2019

Ok, I've reworked the ugly macros with a function and a set of callbacks. Thanks for pushing me

@paulidale
Copy link
Contributor

git add ? :)

@levitte
Copy link
Member Author

levitte commented Jul 10, 2019

D'oh. You're entirely correct

@levitte
Copy link
Member Author

levitte commented Jul 10, 2019

CIs are now happy

@petrovr
Copy link

petrovr commented Jul 10, 2019

Just finish tests - proposed correction resolves issue #8895.

Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, nice job IMO.

crypto/evp/evp_enc.c Outdated Show resolved Hide resolved
crypto/evp/evp_enc.c Show resolved Hide resolved
crypto/evp/evp_enc.c Show resolved Hide resolved
crypto/evp/evp_lib.c Show resolved Hide resolved
crypto/evp/evp_lib.c Show resolved Hide resolved
crypto/evp/evp_locl.h Show resolved Hide resolved
crypto/evp/evp_locl.h Outdated Show resolved Hide resolved
providers/common/ciphers/aes.c Show resolved Hide resolved
providers/common/ciphers/aes.c Outdated Show resolved Hide resolved
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better.

crypto/evp/evp_utils.c Outdated Show resolved Hide resolved
@slontis
Copy link
Member

slontis commented Jul 11, 2019

LGTM

@levitte
Copy link
Member Author

levitte commented Jul 11, 2019

Merged.

06c8331 Adapt the provider AES for more use of OSSL_PARAM
1327323 Adapt diverse EVP_CIPHER functions to use get_params and set_params interfaces
8094237 Make more use of OSSL_PARAM for ciphers
48ebde2 test/evp_test.c: [ciphers] Test that we get back the same IV we gave

@levitte levitte closed this Jul 11, 2019
levitte added a commit that referenced this pull request Jul 11, 2019
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #9328)
levitte added a commit that referenced this pull request Jul 11, 2019
A lot of the different numbers associated with ciphers are really
algorithm parameters.  Key length, block size, IV length, that sort of
thing.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #9328)
levitte added a commit that referenced this pull request Jul 11, 2019
…nterfaces

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #9328)
levitte added a commit that referenced this pull request Jul 11, 2019
The cipher context IV was a bit interesting.  EVP_CIPHER_CTX_iv()
returns a pointer to the live IV, while EVP_CIPHER_CTX_ctrl() with the
type EVP_CTRL_GET_IV gets a copy of the live IV.  To support both, we
support getting it with both the OSSL_PARAM_OCTET_STRING and
OSSL_PARAM_OCTET_PTR datatypes.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #9328)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants