Skip to content

Add ccm (aes and aria) to providers#9280

Closed
slontis wants to merge 3 commits intoopenssl:masterfrom
slontis:prov_aes_ccm
Closed

Add ccm (aes and aria) to providers#9280
slontis wants to merge 3 commits intoopenssl:masterfrom
slontis:prov_aes_ccm

Conversation

@slontis
Copy link
Member

@slontis slontis commented Jul 1, 2019 •
edited
Loading

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

@slontis slontis requested a review from p-steuer July 1, 2019 13:25
@slontis
Copy link
Member Author

slontis commented Jul 1, 2019

@p-steuer The tests seemed to pass on a test s390 machine.

@slontis slontis added the branch: master Applies to master branch label Jul 1, 2019
p-steuer
p-steuer reviewed Jul 1, 2019
View reviewed changes
p-steuer
p-steuer reviewed Jul 1, 2019
View reviewed changes
@p-steuer
Copy link
Member

p-steuer commented Jul 1, 2019 •
edited
Loading

The tests seemed to pass on a test s390 machine.

No, ssl_old and dtls_mtu test failures are related, see my comment above.

Proposed changes:

--- a/providers/common/ciphers/aes_ccm_s390x.inc
+++ b/providers/common/ciphers/aes_ccm_s390x.inc
@@ -112,7 +112,7 @@ static int s390x_aes_ccm_setaad(PROV_AES_CCM_KEY *ctx,
 }
 
 /*-
+ * En/de-crypt plain/cipher-text. Compute tag from plaintext. Returns 1 for
  * success.
  */
 static int s390x_aes_ccm_auth_encdec(PROV_AES_CCM_KEY *ctx,
@@ -207,12 +207,27 @@ static int s390x_aes_ccm_auth_encdec(PROV_AES_CCM_KEY *ctx,
     return 1;
 }
 
+static int s390x_aes_ccm_gettag(PROV_AES_CCM_KEY *ctx,
+                                unsigned char *tag, size_t tlen)
+{
+    if (tlen > ctx->m)
+        return 0;
+    memcpy(tag, ctx->ccm.s390x.kmac.icv.b, tlen);
+    return 1;
+}
+
 static int s390x_aes_ccm_auth_encrypt(PROV_AES_CCM_KEY *ctx,
                                       const unsigned char *in,
                                       unsigned char *out, size_t len,
                                       unsigned char *tag, size_t taglen)
 {
-    return s390x_aes_ccm_auth_encdec(ctx, in, out, len, 1);
+    int rv;
+
+    rv = s390x_aes_ccm_auth_encdec(ctx, in, out, len, 1);
+    if (tag != NULL && rv == 1)
+        rv = s390x_aes_ccm_gettag(ctx, tag, taglen);
+
+    return rv;
 }
 
 static int s390x_aes_ccm_auth_decrypt(PROV_AES_CCM_KEY *ctx,
@@ -234,15 +249,6 @@ static int s390x_aes_ccm_auth_decrypt(PROV_AES_CCM_KEY *ctx,
     return rv;
 }
 
-static int s390x_aes_ccm_gettag(PROV_AES_CCM_KEY *ctx,
-                                unsigned char *tag, size_t tlen)
-{
-    if (tlen > ctx->m)
-        return 0;
-    memcpy(tag, ctx->ccm.s390x.kmac.icv.b, tlen);
-    return 1;
-}
-
 static const PROV_AES_CCM_CIPHER s390x_aes_ccm = {
     aes_ccm_cipher,
     s390x_aes_ccm_init_key,

edit: updated proposed changes to use gettag to fetch the tag value.

p-steuer
p-steuer suggested changes Jul 1, 2019
View reviewed changes
Copy link
Member

@p-steuer p-steuer left a comment

Choose a reason for hiding this comment

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

red cross looks relevant.

@slontis
Copy link
Member Author

slontis commented Jul 2, 2019

reordered the code to get rid of errors, and updated s390 with fixes from @p-steuer

@paulidale
Copy link
Contributor

paulidale commented Jul 2, 2019

Just a note that might help streamline things: as committer @p-steuer, you should be able to make commits to this branch. It might save a bit of back and forth.

There is a checkbox when creating pull requests (allow maintainer edits or somesuch) which permits this and it is checked by default.

p-steuer
p-steuer approved these changes Jul 2, 2019
View reviewed changes
Copy link
Member

@p-steuer p-steuer left a comment

Choose a reason for hiding this comment

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

@paulidale : yeah, i can push a fixup commit next time if thats preferred.
@slontis : its working now, thanks.

@slontis
Copy link
Member Author

slontis commented Jul 2, 2019 •
edited
Loading

yeah, i can push a fixup commit next time if thats preferred.

There shouldnt be too much more S390 work for the providers.
Thanks for reviewing - it has been a big help.

@slontis slontis mentioned this pull request Jul 3, 2019
Closed
2 tasks
@slontis slontis changed the title add aes_ccm to provider WIP: add aes_ccm to provider Jul 3, 2019
@slontis
Copy link
Member Author

slontis commented Jul 3, 2019

Changed to WIP until #9301 is merged

@slontis slontis force-pushed the prov_aes_ccm branch 2 times, most recently from 7181fbe to 05ac468 Compare July 23, 2019 06:07
@slontis slontis changed the title WIP: add aes_ccm to provider Add ccm (aes and aria) to providers Jul 23, 2019
@slontis
Copy link
Member Author

slontis commented Jul 23, 2019

Code has been refactored to also support ARIA.
The way that the code was broken up made this relatively easy to share most portions of the code.

@slontis
Copy link
Member Author

slontis commented Jul 23, 2019

ping

levitte
levitte requested changes Jul 23, 2019
View reviewed changes
levitte
levitte reviewed Jul 23, 2019
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.

Travis failure

@paulidale
Copy link
Contributor

paulidale commented Jul 23, 2019

Camellia CCM mode coming soon???

😏

@slontis slontis force-pushed the prov_aes_ccm branch 2 times, most recently from 63a5c98 to a8a3a83 Compare August 1, 2019 00:23
@slontis
Copy link
Member Author

slontis commented Aug 1, 2019

ping
rebased now that gcm has been merged.
Some gcm cleanups were also done here to match the ccm changes.

mattcaswell
mattcaswell reviewed Aug 1, 2019
View reviewed changes
@slontis
Copy link
Member Author

slontis commented Aug 1, 2019

Whatever solution is applied to fix the setparams for the KEYLEN in GCM code will need to be applied to this PR also.

slontis added 2 commits August 19, 2019 17:07
@slontis
add aes_ccm to provider
af01af7 
Add Cleanups for gcm - based on the changes to ccm.
@slontis
fixup! add aes_ccm to provider
c916f7a
@slontis
Copy link
Member Author

slontis commented Aug 19, 2019

ping - rebased and updated ..

levitte
levitte approved these changes Aug 19, 2019
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.

Please fix the nit

@levitte
Copy link
Member

levitte commented Aug 19, 2019

You did notice that I approved, right?

@robotiko380
Copy link

robotiko380 commented Aug 19, 2019

Nice guys...

@slontis
Copy link
Member Author

slontis commented Aug 19, 2019

You did notice that I approved, right?

Thanks.. Just left it overnight in case someone else wanted to comment.

@slontis slontis added the approval: done This pull request has the required number of approvals label Aug 19, 2019
levitte pushed a commit that referenced this pull request Aug 19, 2019
@slontis
Add aes_ccm to provider
3bfe900 
Add Cleanups for gcm - based on the changes to ccm.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Patrick Steuer <[email protected]>
(Merged from #9280)
@slontis
Copy link
Member Author

slontis commented Aug 19, 2019

Thanks for reviewing... Merged to master

@slontis slontis closed this Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell mattcaswell left review comments

@levitte levitte levitte approved these changes

+1 more reviewer

@p-steuer p-steuer p-steuer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@slontis @p-steuer @paulidale @levitte @robotiko380 @mattcaswell