RFR: 8257497: Key identifier compliance issue

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8257497: Key identifier compliance issue

Hai-May Chao-2
This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.

-------------

Commit messages:
 - 8257497: Key identifier compliance issue

Changes: https://git.openjdk.java.net/jdk/pull/2343/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257497
  Stats: 32 lines in 2 files changed: 22 ins; 0 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2343.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2343/head:pull/2343

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue

Sean Coffey-2
On Mon, 1 Feb 2021 23:06:30 GMT, Hai-May Chao <[hidden email]> wrote:

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

Marked as reviewed by coffeys (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue

Hai-May Chao-2
On Fri, 5 Feb 2021 10:16:14 GMT, Sean Coffey <[hidden email]> wrote:

>> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.
>
> Marked as reviewed by coffeys (Reviewer).

@coffeys Thanks for the review!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue

Sean Mullan-2
In reply to this post by Hai-May Chao-2
On Mon, 1 Feb 2021 23:06:30 GMT, Hai-May Chao <[hidden email]> wrote:

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

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue

Weijun Wang-2
On Fri, 5 Feb 2021 21:54:19 GMT, Sean Mullan <[hidden email]> wrote:

> 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 added after all other extensions, therefore it will overwrite any SKID or AKID you explicitly provided.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v2]

Hai-May Chao-2
In reply to this post by Hai-May Chao-2
> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.

Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:

  Test case added and not overriding -ext fix

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2343/files
  - new: https://git.openjdk.java.net/jdk/pull/2343/files/88cae5cc..567c2004

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=00-01

  Stats: 111 lines in 2 files changed: 92 ins; 19 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2343.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2343/head:pull/2343

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v2]

Weijun Wang-2
On Wed, 10 Feb 2021 21:52:01 GMT, Hai-May Chao <[hidden email]> wrote:

>> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.
>
> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>
>   Test case added and not overriding -ext fix

test/jdk/sun/security/tools/keytool/CheckCertAKID.java line 69:

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

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v2]

Hai-May Chao-2
On Wed, 10 Feb 2021 22:41:26 GMT, Weijun Wang <[hidden email]> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Test case added and not overriding -ext fix
>
> test/jdk/sun/security/tools/keytool/CheckCertAKID.java line 69:
>
>> 67:                 .shouldContain("0000: 00 01 02 03 04 05 06 07   08 09 10 11 12 13 14 15")
>> 68:                 .shouldContain("0010: 16 17 18 19")
>> 69:                 .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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v2]

Weijun Wang-2
On Wed, 10 Feb 2021 23:07:51 GMT, Hai-May Chao <[hidden email]> wrote:

>> test/jdk/sun/security/tools/keytool/CheckCertAKID.java line 69:
>>
>>> 67:                 .shouldContain("0000: 00 01 02 03 04 05 06 07   08 09 10 11 12 13 14 15")
>>> 68:                 .shouldContain("0010: 16 17 18 19")
>>> 69:                 .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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v3]

Hai-May Chao-2
In reply to this post by Hai-May Chao-2
> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.

Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:

  API used to get AKID

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2343/files
  - new: https://git.openjdk.java.net/jdk/pull/2343/files/567c2004..2f5199ca

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=01-02

  Stats: 36 lines in 1 file changed: 30 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2343.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2343/head:pull/2343

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v2]

Hai-May Chao-2
In reply to this post by Weijun Wang-2
On Wed, 10 Feb 2021 23:25:45 GMT, Weijun Wang <[hidden email]> wrote:

>> 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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v3]

Weijun Wang-2
In reply to this post by Hai-May Chao-2
On Thu, 11 Feb 2021 01:01:56 GMT, Hai-May Chao <[hidden email]> wrote:

>> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.
>
> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>
>   API used to get AKID

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

> 1480:         byte[] signerSubjectKeyIdExt = ((X509Certificate)signerCert).getExtensionValue(
> 1481:                 KnownOIDs.SubjectKeyID.value());
> 1482:

How about pass in the `KeyIdentifier` instead of `PublicKey akey` into the createV3Extensions method? And you can calculated with
        X509CertImpl impl;
        if (signerCert instanceof X509CertImpl) {
            impl = (X509CertImpl) signerCert;
        } else {
            impl = new X509CertImpl(signerCert.getEncoded());
        }
        impl.getSubjectKeyId();

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v4]

Hai-May Chao-2
In reply to this post by Hai-May Chao-2
> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.

Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:

  passing in KeyIdentifier to createV3Extensions

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2343/files
  - new: https://git.openjdk.java.net/jdk/pull/2343/files/2f5199ca..ddac63f4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=02-03

  Stats: 18 lines in 2 files changed: 6 ins; 4 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2343.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2343/head:pull/2343

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v3]

Hai-May Chao-2
In reply to this post by Weijun Wang-2
On Thu, 11 Feb 2021 19:48:23 GMT, Weijun Wang <[hidden email]> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   API used to get AKID
>
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1482:
>
>> 1480:         byte[] signerSubjectKeyIdExt = ((X509Certificate)signerCert).getExtensionValue(
>> 1481:                 KnownOIDs.SubjectKeyID.value());
>> 1482:
>
> How about pass in the `KeyIdentifier` instead of `PublicKey akey` into the createV3Extensions method? And you can calculated with
>         X509CertImpl impl;
>         if (signerCert instanceof X509CertImpl) {
>             impl = (X509CertImpl) signerCert;
>         } else {
>             impl = new X509CertImpl(signerCert.getEncoded());
>         }
>         impl.getSubjectKeyId();

Changed as suggested.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v3]

Weijun Wang-2
On Thu, 11 Feb 2021 22:10:55 GMT, Hai-May Chao <[hidden email]> wrote:

>> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1482:
>>
>>> 1480:         byte[] signerSubjectKeyIdExt = ((X509Certificate)signerCert).getExtensionValue(
>>> 1481:                 KnownOIDs.SubjectKeyID.value());
>>> 1482:
>>
>> How about pass in the `KeyIdentifier` instead of `PublicKey akey` into the createV3Extensions method? And you can calculated with
>>         X509CertImpl impl;
>>         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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v5]

Hai-May Chao-2
In reply to this post by Hai-May Chao-2
> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.

Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:

  Reduced one param to createV3Extensions

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2343/files
  - new: https://git.openjdk.java.net/jdk/pull/2343/files/ddac63f4..4bd7e45c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2343&range=03-04

  Stats: 43 lines in 2 files changed: 19 ins; 14 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2343.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2343/head:pull/2343

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v3]

Hai-May Chao-2
In reply to this post by Weijun Wang-2
On Fri, 12 Feb 2021 14:49:17 GMT, Weijun Wang <[hidden email]> wrote:

>> 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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v5]

Weijun Wang-2
In reply to this post by Hai-May Chao-2
On Fri, 12 Feb 2021 20:42:03 GMT, Hai-May Chao <[hidden email]> wrote:

>> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.
>
> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>
>   Reduced one param to createV3Extensions

LGTM.

-------------

Marked as reviewed by weijun (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v5]

Hai-May Chao-2
On Fri, 12 Feb 2021 21:01:48 GMT, Weijun Wang <[hidden email]> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Reduced one param to createV3Extensions
>
> LGTM.

Thanks for the review, Max!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8257497: Key identifier compliance issue [v5]

Sean Mullan-2
In reply to this post by Hai-May Chao-2
On Fri, 12 Feb 2021 20:42:03 GMT, Hai-May Chao <[hidden email]> wrote:

>> This change is made for compliance with RFC 5280 section 4.2.1.1 for Authority Key Identifier extension.
>
> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>
>   Reduced one param to createV3Extensions

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

> 1482:
> 1483:         KeyIdentifier signerSubjectKeyId;
> 1484:         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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2343
12