8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280 #2343

haimaychao · 2021-02-01T23:06:30Z

This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.

Progress

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

Issue

JDK-8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280

Reviewers

Sean Coffey (@coffeys - Reviewer) ⚠️ Review applies to 88cae5c
Sean Mullan (@seanjmullan - Reviewer)
Weijun Wang (@wangweij - Reviewer) ⚠️ Review applies to 4bd7e45

Download

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

bridgekeeper · 2021-02-01T23:06:51Z

👋 Welcome back hchao! 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 · 2021-02-01T23:08:20Z

@haimaychao The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-02-01T23:11:51Z

Webrevs

openjdk · 2021-02-05T10:16:40Z

@haimaychao 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:

8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280

Reviewed-by: coffeys, mullan, weijun

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 381 new commits pushed to the master branch:

cb84539: 8261553: Efficient mask generation using BMI2 BZHI instruction
a065879: 8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
9ba2b71: 8261657: [PPC64] Cleanup StoreCM nodes after CMS removal
f639df4: 8261401: Add sanity check for UseSHM large pages similar to the one used with hugetlb large pages
2e18b52: 8261752: Multiple GC test are missing memory requirements
c7885eb: 8261758: [TESTBUG] gc/g1/TestGCLogMessages.java fails if ergonomics detect too small InitialHeapSize
05d5955: 8261522: [PPC64] AES intrinsics write beyond the destination array
03b586b: 8261750: Remove internal class sun.net.www.MimeLauncher
8418285: 8261235: C1 compilation fails with assert(res->vreg_number() == index) failed: conversion check
a930870: 8261309: Remove remaining StoreLoad barrier with UseCondCardMark for Serial/Parallel GC
... and 371 more: https://git.openjdk.java.net/jdk/compare/2f47c39a744e0c74e6016e409b4806d3f25f2f43...master

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.

haimaychao · 2021-02-05T19:46:54Z

@coffeys Thanks for the review!

seanjmullan

I think it would be useful to add a test that checks that keytool now creates the AKID from the issuing CA's SKID. keytool -ext should be able to create a certificate with your own AKID, but you need to specify the OID and a hex-encoded string for the value. Check with @wangweij but I think you can probably enhance an existing test.

wangweij · 2021-02-05T22:33:23Z

I think it would be useful to add a test that checks that keytool now creates the AKID from the issuing CA's SKID. keytool -ext should be able to create a certificate with your own AKID, but you need to specify the OID and a hex-encoded string for the value. Check with @wangweij but I think you can probably enhance an existing test.

Unfortunately, SKID and AKID are currently added after all other extensions, therefore it will overwrite any SKID or AKID you explicitly provided. If you want to add your special SKID in the root CA cert, you'll need to move the SKID and AKID generations to the beginning the createV3Extensions method so your own SKID can override it. Just use -ext 2.5.29.14=01:02:AA:BB.

wangweij · 2021-02-10T22:41:26Z

test/jdk/sun/security/tools/keytool/CheckCertAKID.java

+                .shouldContain("AuthorityKeyIdentifier")
+                .shouldContain("0000: 00 01 02 03 04 05 06 07   08 09 10 11 12 13 14 15")
+                .shouldContain("0010: 16 17 18 19")
+                .shouldHaveExitValue(0);


Or you can directly read the certificate and look at its extensions using some API.

The current method serves the need to verify the accuracy of the AKID for this PR, and it looks straightforward to perceive I think. The API such as cert.getExtensionValue(KnownOIDs.AuthorityKeyID.value()), and new DerValue to getOctetString() could also be used.

The 3 shouldContain lines cannot prove they appear in that order.

These lines are from the output of the keytool -printcert command. If there may be a problem with the ordering, it would be worth the effort to look into -printcert. I've changed to use APIs to ease the ordering concern.

wangweij · 2021-02-11T19:48:23Z

src/java.base/share/classes/sun/security/tools/keytool/Main.java

+
+        byte[] signerSubjectKeyIdExt = ((X509Certificate)signerCert).getExtensionValue(
+                KnownOIDs.SubjectKeyID.value());
+


How about pass in the KeyIdentifier instead of PublicKey akey into the createV3Extensions method? And you can calculated with

if (signerCert instanceof X509CertImpl) { impl = (X509CertImpl) signerCert; } else { impl = new X509CertImpl(signerCert.getEncoded()); } impl.getSubjectKeyId();

Changed as suggested.

Sorry, I should have been more verbose on my suggestion. I was thinking about passing in only the KeyIdentifier and not akey. After all both of them are for the same purpose and it's clear to consolidate to only one. If the cert has an SKID then use it, otherwise calculate one using new KeyIdentifier(akey). All these are done inside the doGenCert)() method. The createV3Extensions just add an AKID if the parameter is not null.

Updated as suggested.

wangweij

LGTM.

haimaychao · 2021-02-12T21:05:21Z

Thanks for the review, Max!

seanjmullan · 2021-02-16T18:33:52Z

src/java.base/share/classes/sun/security/tools/keytool/Main.java

+        PublicKey issuerPubKey = signerCert.getPublicKey();
+
+        KeyIdentifier signerSubjectKeyId;
+        if (subjectPubKey.equals(issuerPubKey)) {


I think in most cases, this equality test will not work as there is no requirement for PublicKey to override Object.equals, so in most cases this will just check if they reference the same object. I suggest comparing the encoded bytes.

Original logic using this equality test. Fixed as suggested.

seanjmullan

Looks good. However, I think you should change the title of the bug to be more descriptive, which will help for release notes, etc. How about: "Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280."

haimaychao · 2021-02-17T16:36:54Z

@seanjmullan Good idea, and updated the bug's title as suggested. Thanks for the review.

haimaychao · 2021-02-17T16:38:03Z

/integrate

openjdk · 2021-02-17T16:39:17Z

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

cb84539: 8261553: Efficient mask generation using BMI2 BZHI instruction
a065879: 8261791: (sctp) handleSendFailed in SctpChannelImpl.c potential leaks
9ba2b71: 8261657: [PPC64] Cleanup StoreCM nodes after CMS removal
f639df4: 8261401: Add sanity check for UseSHM large pages similar to the one used with hugetlb large pages
2e18b52: 8261752: Multiple GC test are missing memory requirements
c7885eb: 8261758: [TESTBUG] gc/g1/TestGCLogMessages.java fails if ergonomics detect too small InitialHeapSize
05d5955: 8261522: [PPC64] AES intrinsics write beyond the destination array
03b586b: 8261750: Remove internal class sun.net.www.MimeLauncher
8418285: 8261235: C1 compilation fails with assert(res->vreg_number() == index) failed: conversion check
a930870: 8261309: Remove remaining StoreLoad barrier with UseCondCardMark for Serial/Parallel GC
... and 371 more: https://git.openjdk.java.net/jdk/compare/2f47c39a744e0c74e6016e409b4806d3f25f2f43...master

Your commit was automatically rebased without conflicts.

Pushed as commit 05301f5.

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

8257497: Key identifier compliance issue

88cae5c

openjdk bot added the rfr Pull request is ready for review label Feb 1, 2021

openjdk bot added the security security-dev@openjdk.org label Feb 1, 2021

coffeys approved these changes Feb 5, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 5, 2021

seanjmullan reviewed Feb 5, 2021

View reviewed changes

Test case added and not overriding -ext fix

567c200

wangweij reviewed Feb 10, 2021

View reviewed changes

API used to get AKID

2f5199c

wangweij reviewed Feb 11, 2021

View reviewed changes

haimaychao added 2 commits February 11, 2021 14:05

passing in KeyIdentifier to createV3Extensions

ddac63f

Reduced one param to createV3Extensions

4bd7e45

wangweij approved these changes Feb 12, 2021

View reviewed changes

seanjmullan reviewed Feb 16, 2021

View reviewed changes

Compare keys using encoded bytes

55bff9e

seanjmullan approved these changes Feb 17, 2021

View reviewed changes

haimaychao changed the title ~~8257497: Key identifier compliance issue~~ 8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280 Feb 17, 2021

openjdk bot closed this Feb 17, 2021

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 Feb 17, 2021


		byte[] signerSubjectKeyIdExt = ((X509Certificate)signerCert).getExtensionValue(
		KnownOIDs.SubjectKeyID.value());

8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280 #2343

8257497: Update keytool to create AKID from the SKID of the issuing certificate as specified by RFC 5280 #2343

Uh oh!

Conversation

haimaychao commented Feb 1, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Feb 1, 2021

Uh oh!

openjdk bot commented Feb 1, 2021

Uh oh!

mlbridge bot commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haimaychao commented Feb 5, 2021

Uh oh!

seanjmullan left a comment

Choose a reason for hiding this comment

Uh oh!

wangweij commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij left a comment

Choose a reason for hiding this comment

Uh oh!

haimaychao commented Feb 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanjmullan left a comment

Choose a reason for hiding this comment

Uh oh!

haimaychao commented Feb 17, 2021

Uh oh!

haimaychao commented Feb 17, 2021

Uh oh!

openjdk bot commented Feb 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

haimaychao commented Feb 1, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 1, 2021 •

edited

Loading

openjdk bot commented Feb 5, 2021 •

edited

Loading

wangweij commented Feb 5, 2021 •

edited

Loading

wangweij Feb 11, 2021 •

edited

Loading

wangweij Feb 12, 2021 •

edited

Loading