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.

Sign up for GitHub

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

Jump to bottom

Add GCM support for EVP_CTRL_GCM_IV_GEN/EVP_CTRL_GCM_SET_IV_INV to providers #10173

Closed
wants to merge 1 commit into from
Closed

Add GCM support for EVP_CTRL_GCM_IV_GEN/EVP_CTRL_GCM_SET_IV_INV to providers #10173

wants to merge 1 commit into from

Conversation

slontis
Copy link
Member

@slontis slontis commented Oct 14, 2019 •
edited

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

@slontis slontis added the branch: master Merge to master branch label Oct 14, 2019
@slontis slontis added this to In progress in 3.0 New Core + FIPS via automation Oct 14, 2019
@slontis slontis mentioned this pull request Oct 14, 2019
Closed
@petrovr
Copy link

petrovr commented Oct 14, 2019

Tested manually with following command:
$ ./ssh -t -o Ciphers=aes128-gcm@openssh.com localhost tty

/* PKIX-SSH *\
\* non-fips */
/dev/pts/3
Connection to localhost closed.

i.e. pass .

This modification does not provide correction for one of questions raised 9524 -
could replace EVP_CTRL_GCM_SET_IV_FIXED with EVP_CTRL_AEAD_SET_IV_FIXED in evp_enc.c ?

Does CCM support SET_IV_FIXED?
$ grep -n EVP_CTRL_'GCM|AEAD' crypto/evp/evp_enc.c include/openssl/evp.h:

crypto/evp/evp_enc.c:1099:    case EVP_CTRL_AEAD_SET_IVLEN:
crypto/evp/evp_enc.c:1104:    case EVP_CTRL_GCM_SET_IV_FIXED:
crypto/evp/evp_enc.c:1106:            OSSL_PARAM_construct_octet_string(OSSL_CIPHER_PARAM_AEAD_TLS1_IV_FIXED,
crypto/evp/evp_enc.c:1109:    case EVP_CTRL_GCM_IV_GEN:
crypto/evp/evp_enc.c:1117:    case EVP_CTRL_GCM_SET_IV_INV:
crypto/evp/evp_enc.c:1132:    case EVP_CTRL_AEAD_GET_TAG:
crypto/evp/evp_enc.c:1134:    case EVP_CTRL_AEAD_SET_TAG:
crypto/evp/evp_enc.c:1135:        params[0] = OSSL_PARAM_construct_octet_string(OSSL_CIPHER_PARAM_AEAD_TAG,
crypto/evp/evp_enc.c:1138:    case EVP_CTRL_AEAD_TLS1_AAD:
crypto/evp/evp_enc.c:1141:            OSSL_PARAM_construct_octet_string(OSSL_CIPHER_PARAM_AEAD_TLS1_AAD,
crypto/evp/evp_enc.c:1147:            OSSL_PARAM_construct_size_t(OSSL_CIPHER_PARAM_AEAD_TLS1_AAD_PAD, &sz);
include/openssl/evp.h:291:# define         EVP_CIPH_FLAG_AEAD_CIPHER       0x200000
include/openssl/evp.h:316:# define         EVP_CTRL_AEAD_SET_IVLEN         0x9
include/openssl/evp.h:317:# define         EVP_CTRL_AEAD_GET_TAG           0x10
include/openssl/evp.h:318:# define         EVP_CTRL_AEAD_SET_TAG           0x11
include/openssl/evp.h:319:# define         EVP_CTRL_AEAD_SET_IV_FIXED      0x12
include/openssl/evp.h:320:# define         EVP_CTRL_GCM_SET_IVLEN          EVP_CTRL_AEAD_SET_IVLEN
include/openssl/evp.h:321:# define         EVP_CTRL_GCM_GET_TAG            EVP_CTRL_AEAD_GET_TAG
include/openssl/evp.h:322:# define         EVP_CTRL_GCM_SET_TAG            EVP_CTRL_AEAD_SET_TAG
include/openssl/evp.h:323:# define         EVP_CTRL_GCM_SET_IV_FIXED       EVP_CTRL_AEAD_SET_IV_FIXED
include/openssl/evp.h:324:# define         EVP_CTRL_GCM_IV_GEN             0x13
include/openssl/evp.h:325:# define         EVP_CTRL_CCM_SET_IVLEN          EVP_CTRL_AEAD_SET_IVLEN
include/openssl/evp.h:326:# define         EVP_CTRL_CCM_GET_TAG            EVP_CTRL_AEAD_GET_TAG
include/openssl/evp.h:327:# define         EVP_CTRL_CCM_SET_TAG            EVP_CTRL_AEAD_SET_TAG
include/openssl/evp.h:328:# define         EVP_CTRL_CCM_SET_IV_FIXED       EVP_CTRL_AEAD_SET_IV_FIXED
include/openssl/evp.h:332: * AEAD cipher deduces payload length and returns number of bytes required to
include/openssl/evp.h:336:# define         EVP_CTRL_AEAD_TLS1_AAD          0x16
include/openssl/evp.h:337:/* Used by composite AEAD ciphers, no-op in GCM, CCM... */
include/openssl/evp.h:338:# define         EVP_CTRL_AEAD_SET_MAC_KEY       0x17
include/openssl/evp.h:340:# define         EVP_CTRL_GCM_SET_IV_INV         0x18
include/openssl/evp.h:384:# define         EVP_AEAD_TLS1_AAD_LEN           13

@slontis
Copy link
Member Author

slontis commented Oct 14, 2019 •
edited

EVP_CTRL_GCM_SET_IV_FIXED & EVP_CTRL_AEAD_SET_IV_FIXED are defined as the same thing, so it makes no difference which one is used.
SET_IV_FIXED is supported by CCM.

replaced EVP_CTRL_GCM_SET_IV_FIXED in evp_enc.c

@slontis
Copy link
Member Author

slontis commented Oct 16, 2019

ping

@slontis slontis added the approval: otc review pending This pull request needs review by an OTC member label Nov 6, 2019
@slontis
Copy link
Member Author

slontis commented Nov 11, 2019

ping: rebased

@slontis
Copy link
Member Author

slontis commented Dec 5, 2019

ping: rebased again
@levitte this will conflict with your PR where you move files around

@petrovr
Copy link

petrovr commented Jan 5, 2020

Author refuse to replace EVP_CTRL_GCM_SET_IV_FIXED with EVP_CTRL_AEAD_SET_IV_FIXED - improved readability.
Team refuse to approve restore of lost functionality.

So issue is in deadlock state.

levitte
levitte requested changes Jan 5, 2020
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

I would like to see a more generic kind of thing surrounding names...

doc/man7/provider-cipher.pod Show resolved Hide resolved
crypto/evp/evp_enc.c Show resolved Hide resolved
3.0 New Core + FIPS automation moved this from In progress to Needs review Jan 5, 2020
@levitte
Copy link
Member

levitte commented Jan 5, 2020

Rebase needed.

@levitte
Copy link
Member

levitte commented Jan 5, 2020

Team refuse to approve restore of lost functionality.

Team has quite a few things on its plate and limited resources. Please do not see delays on your favorite topics of interest as refusal.

@slontis
Copy link
Member Author

slontis commented Jan 6, 2020

Author refuse to replace EVP_CTRL_GCM_SET_IV_FIXED with EVP_CTRL_AEAD_SET_IV_FIXED - improved readability.

Err this PR has this change..

Team refuse to approve restore of lost functionality.
So issue is in deadlock state.

Hopefully this gets resolved a bit quicker now..

@slontis
Copy link
Member Author

slontis commented Jan 6, 2020

ping - rebased due to the provider folders moving.

@mattcaswell
Copy link
Member

Looks ok to me - but needs a rebase again!!

@slontis
Copy link
Member Author

slontis commented Jan 6, 2020

rebased.

@slontis
Copy link
Member Author

slontis commented Jan 8, 2020

ping

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Jan 8, 2020
@slontis slontis removed the approval: done This pull request has the required number of approvals label Jan 10, 2020
@slontis slontis added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jan 10, 2020
openssl-machine pushed a commit that referenced this pull request Jan 10, 2020
@slontis
Add GCM support for EVP_CTRL_GCM_IV_GEN and EVP_CTRL_GCM_SET_IV_INV t…
11b4435 
…o providers

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10173)
@slontis
Copy link
Member Author

slontis commented Jan 10, 2020

Thanks.. code merged to master.

@slontis slontis closed this Jan 10, 2020
3.0 New Core + FIPS automation moved this from Needs review to Done Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte requested changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
No open projects
3.0 New Core + FIPS
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@slontis @petrovr @levitte @mattcaswell