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

Counter KDF: NIST SP 800-108r1-upd1 #399

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Conversation

amirhosv
Copy link
Contributor

@amirhosv amirhosv commented Aug 27, 2024

Description of changes:

The following algorithm for SecretKeyFactory are added:

  • CounterKdfWithHmacSHA256 (not available in FIPS builds)
  • CounterKdfWithHmacSHA384 (not available in FIPS builds)
  • CounterKdfWithHmacSHA512 (not available in FIPS builds)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@amirhosv amirhosv force-pushed the cntrkdf branch 5 times, most recently from 39dc35c to 65892ec Compare August 27, 2024 19:44
@amirhosv amirhosv marked this pull request as ready for review August 27, 2024 20:46
@amirhosv amirhosv requested a review from a team as a code owner August 27, 2024 20:46
src/com/amazon/corretto/crypto/provider/CounterKdfSpi.java Outdated Show resolved Hide resolved
public CounterKdfSpec(
final byte[] secret, final byte[] info, final int outputLen, final String algorithName) {
this.secret = Objects.requireNonNull(secret);
if (this.secret.length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

From a cryptographic perspective, should we really be letting people have 1-byte KDK's? This seems like an unnecessary footgun. FWIW, I did look at the AWS-LC source and they also only check that this is non-zero. So if we follow the principal of not adding any new semantics in ACCP this should stay (but I feel gross about it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the library perspective, I think the right approach is to implement the standard faithfully.

@@ -0,0 +1,802 @@
[TEST]

HASH=SHA1
Copy link
Contributor

Choose a reason for hiding this comment

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

You've imported KATs for SHA1 KDFs but we do not support this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file has been copy-pasted from AWS-LC's repo. In case we have to add SHA1 in future, then there do not have to change this file.

README.md Show resolved Hide resolved
csrc/counter_kdf.cpp Show resolved Hide resolved
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein left a comment

Choose a reason for hiding this comment

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

perhaps in a follow-up, should we add benchmarks for our KDFs?


// The rest of the tests are only available in non-FIPS mode.
@Test
public void concatenationKdfExpectsConcatenationKdfSpecAsKeySpec() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be counterKdfExpectsCounterKdfSpecAsKeySpec? we're requesting a Counter KDF factory and asserting that it doesn't accept a PBEKeySpec

@amirhosv amirhosv merged commit 0650b55 into corretto:main Sep 4, 2024
10 checks passed
@amirhosv amirhosv deleted the cntrkdf branch September 4, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants