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

Move cipher ctx 'original iv' parameter into the provider #10026

Closed
wants to merge 4 commits into from

Conversation

slontis
Copy link
Contributor

@slontis slontis commented Sep 26, 2019

Checklist
  • documentation is added or updated
  • tests are added or updated

@slontis slontis added the branch: master Merge to master branch label Sep 26, 2019
@slontis slontis added this to In progress in 3.0 New Core + FIPS via automation Sep 26, 2019
3.0 New Core + FIPS automation moved this from In progress to Needs review Sep 26, 2019
crypto/evp/evp_lib.c Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Sep 30, 2019

Naming wise, I wonder if the parameter isn't a bit misnamed. What does "original" mean? What origin does it refer to? So I wonder if it wouldn't be better to call it "initial IV", to express that it's the IV that was specified when initializing the cipher, i.e. when calling EVP_CipherInit & friends.

(remember that EVP_CipherInit can be called more than once on the same context, so "original" would make me wonder if it's the IV from the first EVP_CipherInit on the same context, or the last, if I didn't know the code as well as I do)

doc/man7/provider-cipher.pod Outdated Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Sep 30, 2019

Speaking of IV, I'm starting to wonder why we even share the running IV (which, strictly speaking, isn't an IV any more) from providers. With legacy code, I fully understand, because all back-ends needed to access the IV that was centrally stored in the EVP_CIPHER_CTX instead of their own private context. With provided implementations that hold the IV in the own private context, there should be about zero need to share that one, just the actual initialization vector.

In other words, there should really just be a OSSL_CIPHER_PARAM_IV, and it should only access what we currently call "the original IV".

Perhaps this is a matter for another PR. I'll be happy to oblige.

@beldmit
Copy link
Member

beldmit commented Sep 30, 2019

I need to look at it more thoroughly. GOST TLS requires an original IV as a source to derive the per-packet IV. See https://tools.ietf.org/html/draft-smyshlyaev-tls12-gost-suites-05#section-4.1.1 for details.

@slontis
Copy link
Contributor Author

slontis commented Oct 3, 2019

there should really just be a OSSL_CIPHER_PARAM_IV, and it should only access what we currently call "the original IV".

Good point - If there is a need for the running IV (which I dont think there is now) we would add it later.
I have removed the references to the original iv param - and it seems to work fine.

@slontis
Copy link
Contributor Author

slontis commented Oct 3, 2019

need to look at it more thoroughly. GOST TLS requires an original IV as a source to derive the per-packet IV. See https://tools.ietf.org/html/draft-smyshlyaev-tls12-gost-suites-05#section-4.1.1 for details.

TLS related ciphers normally use a seperate mechanism via some ctrls to set the fixed_iv and aad so this should not be an issue.

@slontis slontis changed the title WIP: Move cipher ctx 'original iv' parameter into the provider Move cipher ctx 'original iv' parameter into the provider Oct 3, 2019
providers/common/ciphers/cipher_common.c Outdated Show resolved Hide resolved
providers/common/ciphers/cipher_gcm.c Show resolved Hide resolved
@slontis
Copy link
Contributor Author

slontis commented Oct 3, 2019

Updated

3.0 New Core + FIPS automation moved this from Needs review to Reviewer approved Oct 3, 2019
levitte
levitte approved these changes Oct 3, 2019
@slontis
Copy link
Contributor Author

slontis commented Oct 3, 2019

rebased due to simple merge conflict in header ciphercommon.h. Approval should still apply.

@levitte
Copy link
Member

levitte commented Oct 3, 2019

Approval should still apply.

Not formally. A re-review will confirm that you didn't goof up the merge fixups

levitte
levitte approved these changes Oct 3, 2019
levitte pushed a commit that referenced this pull request Oct 7, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #10026)
@slontis
Copy link
Contributor Author

slontis commented Oct 7, 2019

Thanks.. Merged to master.

@slontis slontis closed this Oct 7, 2019
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Oct 7, 2019
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants