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

ConcatinationKDF: SSKDF from NIST.SP.800-56Cr2 Key Derivation #397

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

amirhosv
Copy link
Contributor

@amirhosv amirhosv commented Aug 8, 2024

Description of changes:

  • The following algorithms for SecretKeyFactory are added for non-FIPS builds:
    • ConcatenationKdfWithSHA256
    • ConcatenationKdfWithSHA384
    • ConcatenationKdfWithSHA512
    • ConcatenationKdfWithHmacSHA256
    • ConcatenationKdfWithHmacSHA512

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

* The following algorithms for SecretKeyFactory are added for non-FIPS
  builds:
  * ConcatenationKdfWithSHA224
  * ConcatenationKdfWithSHA256
  * ConcatenationKdfWithSHA384
  * ConcatenationKdfWithSHA512
  * ConcatenationKdfWithHmacSHA256
  * ConcatenationKdfWithHmacSHA512
@amirhosv amirhosv marked this pull request as ready for review August 8, 2024 19:04
@amirhosv amirhosv requested a review from a team as a code owner August 8, 2024 19:04
throw new IllegalArgumentException("Output size must be greater than zero.");
}
this.outputLen = outputLen;
this.algorithmName = Objects.requireNonNull(algorithmName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you validate the algorithmName here?

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 KDFs are used in different context. It's hard to come up with the exhaustive list of all acceptable algorithm names.

throw new IllegalArgumentException("Secret must be byte array with non-zero length.");
}
this.info = Objects.requireNonNull(info);
this.salt = Optional.ofNullable(salt);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the salt is provided, it should be non-zero as well.

Validate the constraint documented above "When using HMAC variants, a salt must be provided."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified the logic so that salt is never null. In case it's not provided, an empty array is used. The same goes for info.

geedo0
geedo0 previously approved these changes Aug 9, 2024
Copy link

@sgmenda-aws sgmenda-aws left a comment

Choose a reason for hiding this comment

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

lgtm with nits

Comment on lines +11 to +13
* <p>If info or salt is not provided, empty byte arrays are used.
*
* <p>The algorithmName is the name of algorithm used to create SecretKeySpec.

Choose a reason for hiding this comment

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

Suggested change
* <p>If info or salt is not provided, empty byte arrays are used.
*
* <p>The algorithmName is the name of algorithm used to create SecretKeySpec.
* <p>If info or salt is not provided, empty byte arrays are used.</p>
*
* <p>The algorithmName is the name of algorithm used to create SecretKeySpec.</p>

Choose a reason for hiding this comment

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

Turns out this is unnecessary in a java docstring, and the linter will even remove the close tags. 🤦

README.md Outdated Show resolved Hide resolved
alexw91
alexw91 previously approved these changes Aug 26, 2024
Copy link
Contributor

@alexw91 alexw91 left a comment

Choose a reason for hiding this comment

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

LGTM, only request is to remove SHA224 support unless there is a hard requirement for it.

alexw91
alexw91 previously approved these changes Aug 26, 2024
@amirhosv amirhosv merged commit 10e8edd into corretto:main Aug 27, 2024
10 checks passed
@amirhosv amirhosv deleted the sskdf branch August 27, 2024 17:33
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.

4 participants