-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update README.md regarding ACCP-FIPS and randomness #400
base: main
Are you sure you want to change the base?
Conversation
Clarify that since 2.4.0 there is no difference between ACCP and ACCP-FIPS regarding registration of SecureRandom.
README.md
Outdated
* Due to the fact that an older branch of AWS-LC is used in FIPS-mode, there will be performance differences between ACCP and ACCP-FIPS. We highly recommend performing detailed performance testing of your application if you choose to experiment with ACCP-FIPS. | ||
* Before version 2.4.0, ACCP-FIPS did not register SecureRandom by default due to the performance of AWS-LC’s entropy source in FIPS-mode, with older versions of AWS-LC. Since version 2.4.0, ACCP-FIPS behaves as ACCP: it registers SecureRandom from AWS-LC by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should link to https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/FIPS.md, which provides details about the entropy sources used in the versions of AWS-LC. I would highlight that ACCP uses AWS-LC-FIPS v2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few paragraphs above, it says "ACCP-FIPS is a variation of ACCP which uses AWS-LC-FIPS 2.x as its cryptographic module.".
Should we add the following sentence:
For details about the FIPS module of AWS-LC-FIPS, including the entropy sources used, see the AWS-LC FIPS.md documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ I think that sentence makes sense to add.
Also, not sure how pedantic we need to be, but the "don't register SecureRandom in FIPS" behavior was introduced in 2.1.0, so we could be a little more precise here:
Between versions 2.1.0 and 2.3.3 (inclusive), ACCP-FIPS did not register SecureRandom by default ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit with those changes.
It looks like, since this PR was made, wording about the fact that SecureRandom was not registered before version 2.4.0 was already added to the README. However, the wording did not include @darylmartin100 and @WillChilds-Klein's comments. |
Issue #, if available: N/A
Description of changes:
Clarify that since 2.4.0 there is no difference between ACCP and ACCP-FIPS regarding registration of SecureRandom.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.