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

SHA-3 support (+ minor fixes & perf) #7708

Merged
merged 44 commits into from
Jun 7, 2023
Merged

SHA-3 support (+ minor fixes & perf) #7708

merged 44 commits into from
Jun 7, 2023

Conversation

daverodgman
Copy link
Contributor

Description

Minor fixes on top of #5820, and perf improvement (around 1.8x for SHA3-256 on M1 Pro).

Agreed with @d3zd3z that since we've both reviewed #5820, we are OK for David to single-review these updates.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog provided
  • backport not required - new feature
  • tests provided

polhenarejos and others added 30 commits May 9, 2022 01:04
@polhenarejos
Add SHA-3 module.
0cd1f1c 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Add test vectors (from NIST) for SHA-3.
f645705 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Aligning spaces
90f360e 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
SHA-3 does not use SHA3_ALT anymore.
084649d 
Next releases will not use alt files.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
olen = 0 is not allowed for SHA-3.
85eeda0 
Sanity checks are moved to mbedtls_sha3_xxx() functions.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Fix when no SHA3 family is found.
116411e 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Remove sha3_alt.h
e6b8c83 
Next releases will not rely on alt files.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
olen parameter shall contain the length of the buffer.
1f3ae16 
For SHA-3 families, it must be at least 28, 32, 48 or 64, depending on the family.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Added SHA3 to MD.
4712d4c 
This enables HMAC with SHA3.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Added SHA3 to benchmark.
ebb3640 
Taken from Mbed-TLS#1549, as it is closed.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Fix when reusing the same context for another operation.
938b5ab 
Occurs in hmac, where multiple hashes are performed with the same context) and thus, it requires to reinitialize the internal states to 0.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Adding tests for MD SHA3 (taken from Mbed-TLS#1549).
8aadc61 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Fix travis build.
13acb1b 
Enum values should not be conditioned.

Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Add self tests (taken from Mbed-TLS#1549).
7dbd5d1 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Add sha3 streaming and reuse tests.
90f803c 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Fix dependency of HMAC-SHA384 tests.
e884fd7 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Merge branch 'Mbed-TLS:development' into sha3
c9754c3
@polhenarejos @gilles-peskine-arm
Update library/sha3.c
39fb1d5 
Co-authored-by: Gilles Peskine <[email protected]>
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Merge branch 'development' into sha3
4e74733 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Bad merge. These tests are not used.
a391a9e 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Style.
a677928 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
SHA3 cannot be tested alone, as ENTROPY_C needs also SHA256 enabled.
aa426e0 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Correct style.
b3b220c 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Merge branch 'development' into sha3
f61d6c0
@polhenarejos
Fix md test with sha3.
0004a86 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Merge branch 'development' into sha3
d06c6fc 
Signed-off-by: Pol Henarejos <[email protected]>
@polhenarejos
Fix coding style.
2d80769 
Signed-off-by: Pol Henarejos <[email protected]>
@dave-rodgman @daverodgman
Consume input in 8-byte chunks
bcfd79c 
Signed-off-by: Dave Rodgman <[email protected]>
@dave-rodgman @daverodgman
remove not-needed fields from SHA-3 context
1789d84 
Signed-off-by: Dave Rodgman <[email protected]>
@dave-rodgman @daverodgman
Use faster type for state index
e627bef 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman daverodgman changed the base branch from development to 2.28-sphinx-versioned-documentation June 7, 2023 17:07
@daverodgman daverodgman changed the base branch from 2.28-sphinx-versioned-documentation to development June 7, 2023 17:07
@daverodgman
Correct minor merge mistakes
b61cd10 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Fix test dependency
a8af4f4 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman daverodgman changed the base branch from development to 2.28-sphinx-versioned-documentation June 7, 2023 17:30
@daverodgman daverodgman changed the base branch from 2.28-sphinx-versioned-documentation to development June 7, 2023 17:30
@daverodgman daverodgman requested a review from d3zd3z June 7, 2023 17:38
@daverodgman daverodgman added enhancement needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) historical-reviewed Reviewed & agreed to keep legacy PR/issue single-reviewer This PR qualifies for having only one reviewer labels Jun 7, 2023
d3zd3z
d3zd3z approved these changes Jun 7, 2023
View reviewed changes
Copy link
Contributor

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

The additional changes look good. Thanks, @daverodgman.

@daverodgman daverodgman merged commit ccfb3fe into Mbed-TLS:development Jun 7, 2023
@daverodgman daverodgman deleted the sha3-updated branch June 7, 2023 21:09
@daverodgman daverodgman mentioned this pull request Jun 7, 2023
Merged
4 tasks
mpg
mpg reviewed Jun 8, 2023
View reviewed changes
include/mbedtls/md.h
} mbedtls_md_type_t;

/* Note: this should always be >= PSA_HASH_MAX_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this entire note? I think it is still valid (except for the last sentence) and useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I made a mistake in the merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, happens!

@mpg
Copy link
Contributor

mpg commented Jun 8, 2023

Agreed with @d3zd3z that since we've both reviewed #5820, we are OK for David to single-review these updates.

I don't think that's in agreement with our process. Some of your updates are IMO not trivial enough to qualify for being single-reviewer. I think there should have been a 2nd reviewer for the commits you added - obviously no need for that person to re-review the commits from 5820 that already had two approvals.

(Not a big deal, but still wanted to point it out.)

mpg
mpg reviewed Jun 8, 2023
View reviewed changes
include/mbedtls/md.h
@@ -112,6 +112,9 @@
#define MBEDTLS_MD_CAN_SHA512
#define MBEDTLS_MD_SOME_LEGACY
#endif
#if defined(MBEDTLS_SHA3_C)
#define MBEDTLS_MD_CAN_SHA3
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that's quite right. PSA has distinct algorithm identifiers for the various SHA-3 variants, hence distinct PSA_WANT macros. Since the MD_CAN macros are here to provide an abstraction above MBEDTLS_xxx_C and PSA_WANT macros, they need to be as fine-grained as the most fine-grained of the two, so PSA here.

Note: since this is a public macro, this needs fixing before the next release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #7713 and added it to the next release EPIC.

This was referenced Jun 8, 2023
Closed
Closed
Closed
@daverodgman daverodgman mentioned this pull request Jun 8, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg left review comments

@d3zd3z d3zd3z d3zd3z approved these changes

Assignees
No one assigned
Labels
component-crypto Crypto primitives and low-level interfaces enhancement historical-reviewed Reviewed & agreed to keep legacy PR/issue needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, single-reviewer This PR qualifies for having only one reviewer size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@daverodgman @mpg @d3zd3z @polhenarejos @dave-rodgman