8199697: FIPS 186-4 RSA Key Generation #420

valeriepeng · 2020-09-30T03:25:06Z

Could someone please help review this RFE? Update existing RSA key pair generation code following the guidelines from FIPS 186-4 and FIPS 186-5 (draft). Current proposed changes updates the prime generation code (for P, Q) based on FIPS 186-4 B.3.3 when keysize and public exponent met the requirements set in FIPS 186-4/5.

Thanks,
Valerie

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Testing

	Linux x32	Linux x64	Windows x64	macOS x64
Build	✔️ (1/1 passed)	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)		✔️ (9/9 passed)	❌ (1/9 failed)	✔️ (9/9 passed)

Failed test task

Windows x64 (hs/tier1 compiler)

Issue

JDK-8199697: FIPS 186-4 RSA Key Generation

Reviewers

Xue-Lei Andrew Fan (@XueleiFan - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/420/head:pull/420
$ git checkout pull/420

Changed RSA key pair generation code following the guidelines from FIPS 186-4.

valeriepeng · 2020-09-30T03:25:41Z

/label security

bridgekeeper · 2020-09-30T03:27:11Z

👋 Welcome back valeriep! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-09-30T03:28:16Z

@valeriepeng
The security label was successfully added.

mlbridge · 2020-09-30T03:31:07Z

Webrevs

valeriepeng · 2020-09-30T03:34:31Z

/issue 8199697

openjdk · 2020-09-30T03:34:56Z

@valeriepeng The issue 8199697 was not found in the JDK project - make sure you have entered it correctly.
As there were validation problems, no additional issues will be added to the list of solved issues.

valeriepeng · 2020-09-30T03:37:33Z

/issue 8199697

openjdk · 2020-09-30T03:38:19Z

@valeriepeng This issue is referenced in the PR title - it will now be updated.

valeriepeng · 2020-10-09T19:52:45Z

/test

openjdk · 2020-10-09T19:55:15Z

Could not create test job

valeriepeng · 2020-10-09T20:03:51Z

/test tier1

openjdk · 2020-10-09T20:04:16Z

Could not create test job

XueleiFan · 2020-10-13T05:23:01Z

test/jdk/sun/security/rsa/SpecTest.java

 * @run main SpecTest 1024
- * @run main SpecTest 1024 3
+ * @run main SpecTest 1024 65537


65537 is the default public exponent (see the main() method). So, the two test case is the same:

@run main SpecTest 1024

@run main SpecTest 1024 65537

Maybe, we can keep the test case for F0, and add a new public exponent number like 167971.

I want to stop using F0 since it's no longer deemed valid for FIPS 186-4. For backward compatibility, we don't reject F0, but perhaps we should stop using it so people will start shifting to use F4. I can replace 3 with 167971.

XueleiFan · 2020-10-20T22:21:02Z

src/java.base/share/classes/sun/security/rsa/RSAKeyPairGenerator.java

+                BigInteger n = p.multiply(q);
+                if (!useNew && n.bitLength() != keySize) {
+                    // regenerate Q if n is not the right length
+                    continue;
+                }
+                KeyPair kp = createKeyPair(type, keyParams, n, e, p, q);
+                if (kp != null) {
+                    return kp;
                }


The logic may be more clear if moving the checking of n and key generation out of the loop for q, by regenerate both p and q if needed.

Ok, will add an outer while-loop as in the existing code.

XueleiFan

Looks good to me.

openjdk · 2020-10-21T03:19:51Z

@valeriepeng This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8199697: FIPS 186-4 RSA Key Generation

Reviewed-by: xuelei

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 5 new commits pushed to the master branch:

6bd05b1: 8255074: sun.nio.fs.WindowsPath::getPathForWin32Calls synchronizes on String object
9e9f5e6: 8017179: [macosx] list1 and list2 vistble item isn't desired
2ee2b4a: 8231454: File lock in Windows on a loaded jar due to a leak in Introspector::getBeanInfo
42a6ead: 8254884: Make sure jvm does not crash with Arm SVE and Vector API
e5870cf: 8252133: The java/awt/GraphicsDevice/DisplayModes/CycleDMImage.java fails if metal pipeline is active

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

valeriepeng · 2020-10-21T22:40:52Z

/integrate

openjdk · 2020-10-21T22:45:35Z

@valeriepeng Since your change was applied there have been 27 commits pushed to the master branch:

60d3fa2: 8255022: Documentation missing for Vector API zero methods
9ade94b: 8206311: Add docs-javase, docs-reference to CI build
3445031: 8255200: ProblemList com/sun/jdi/EATests.java for ZGC
85a8949: 8254913: Increase InlineSmallCode default from 2000 to 2500 for x64
56ea490: 8233343: Deprecate -XX:+CriticalJNINatives flag which implements JavaCritical native functions
615b759: 8255070: Shenandoah: Use single thread for concurrent CLD liveness test
6020991: 8255068: [JVMCI] errors during compiler creation can be hidden
8d9e6d0: 8255041: Zero: remove old JSR 292 support leftovers
0efdde1: 8238669: Long.divideUnsigned is extremely slow for certain values (Needs to be Intrinsic)
365f19c: 8254790: SIGSEGV in string_indexof_char and stringL_indexof_char intrinsics
... and 17 more: https://git.openjdk.java.net/jdk/compare/afc967fcd0bf2d51eef7e0a9e8a7fc3d2f14575a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1191a63.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8199697: FIPS 186-4 RSA Key Generation

441c114

Changed RSA key pair generation code following the guidelines from FIPS 186-4.

openjdk bot added rfr Pull request is ready for review security security-dev@openjdk.org labels Sep 30, 2020

Merge branch 'master' into JDK-8199697

358fee0

XueleiFan reviewed Oct 20, 2020

View reviewed changes

valeriepeng added 2 commits October 21, 2020 00:58

Updated to address review comments

aebe941

Merge

61952af

XueleiFan approved these changes Oct 21, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 21, 2020

openjdk bot closed this Oct 21, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8199697: FIPS 186-4 RSA Key Generation #420

8199697: FIPS 186-4 RSA Key Generation #420

valeriepeng commented Sep 30, 2020 •

edited by openjdk bot

Loading

valeriepeng commented Sep 30, 2020

bridgekeeper bot commented Sep 30, 2020

openjdk bot commented Sep 30, 2020

mlbridge bot commented Sep 30, 2020 •

edited

Loading

valeriepeng commented Sep 30, 2020

openjdk bot commented Sep 30, 2020

valeriepeng commented Sep 30, 2020

openjdk bot commented Sep 30, 2020

valeriepeng commented Oct 9, 2020

openjdk bot commented Oct 9, 2020

valeriepeng commented Oct 9, 2020

openjdk bot commented Oct 9, 2020

XueleiFan Oct 13, 2020

valeriepeng Oct 21, 2020

XueleiFan Oct 20, 2020

valeriepeng Oct 21, 2020

XueleiFan left a comment

openjdk bot commented Oct 21, 2020 •

edited

Loading

valeriepeng commented Oct 21, 2020

openjdk bot commented Oct 21, 2020

8199697: FIPS 186-4 RSA Key Generation #420

8199697: FIPS 186-4 RSA Key Generation #420

Conversation

valeriepeng commented Sep 30, 2020 • edited by openjdk bot Loading

Progress

Testing

Issue

Reviewers

Download

valeriepeng commented Sep 30, 2020

bridgekeeper bot commented Sep 30, 2020

openjdk bot commented Sep 30, 2020

mlbridge bot commented Sep 30, 2020 • edited Loading

Webrevs

valeriepeng commented Sep 30, 2020

openjdk bot commented Sep 30, 2020

valeriepeng commented Sep 30, 2020

openjdk bot commented Sep 30, 2020

valeriepeng commented Oct 9, 2020

openjdk bot commented Oct 9, 2020

valeriepeng commented Oct 9, 2020

openjdk bot commented Oct 9, 2020

XueleiFan Oct 13, 2020

Choose a reason for hiding this comment

valeriepeng Oct 21, 2020

Choose a reason for hiding this comment

XueleiFan Oct 20, 2020

Choose a reason for hiding this comment

valeriepeng Oct 21, 2020

Choose a reason for hiding this comment

XueleiFan left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 21, 2020 • edited Loading

valeriepeng commented Oct 21, 2020

openjdk bot commented Oct 21, 2020

valeriepeng commented Sep 30, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 30, 2020 •

edited

Loading

openjdk bot commented Oct 21, 2020 •

edited

Loading