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

2021-02-16 Thread Hai-May Chao 
On Tue, 16 Feb 2021 18:33:52 GMT, Sean Mullan  wrote:

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

Original logic using this equality test. Fixed as suggested.

-

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

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

2021-02-16 Thread Sean Mullan 
On Fri, 12 Feb 2021 20:42:03 GMT, Hai-May Chao  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

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

2021-02-12 Thread Hai-May Chao 
On Fri, 12 Feb 2021 21:01:48 GMT, Weijun Wang  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

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

2021-02-12 Thread Weijun Wang 
On Fri, 12 Feb 2021 20:42:03 GMT, Hai-May Chao  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

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

2021-02-12 Thread Hai-May Chao 
> 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=2343=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2343=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