Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 11:52:16 GMT, Weijun Wang wrote: >>> Maybe we don't need to resolve it in this code change. If we look carefully >>> at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 >>> is using the signer's SKID in 10.1 as its own SKID and it has no AKID. >>> Currently, keytool will generate a new SKID and use signer's SKID as AKID. >>> If we really want to generate a certificate that's identical to the one in >>> the RFC, we'll need a way to tell keytool to omit the AKID (something like >>> "-ext akid=none"). >> >> Better to use AKID for certification path building. > > I don't mean we will remove it by default. Just think there needs a way to > remove either AKID or SKID, because we always generate them automatically. The support for omitting AKID will be tracked by different PR if needed. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 01:40:16 GMT, Weijun Wang wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1978:
>
>> 1976: keypair.getPublicKeyAnyway(),
>> 1977: signerSubjectKeyId);
>> 1978: } else {
>
> No need for an if-else block? `signerSubjectKeyId` is null in this case.
Removed.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2013:
>
>> 2011: x500Name};
>> 2012: System.err.println(form.format(source));
>> 2013: }
>
> Either put the declaration of `source` outside the if-else block and move the
> println line outside; or, move the declaration of `form` inside. It looks
> confusing that `form` is declared outside and `source` inside.
Done.
> src/java.base/share/classes/sun/security/tools/keytool/Resources.java line
> 310:
>
>> 308: "Generating {0} bit {1} key pair and self-signed
>> certificate ({2}) with a validity of {3} days\n\tfor: {4}"},
>> 309:
>> {"Generating.keysize.bit.keyAlgName.key.pair.and.a.certificate.sigAlgName.issued.by.an.entry.signerAlias.with.a.validity.of.validality.days.for",
>> 310: "Generating {0} bit {1} key pair and a certificate
>> ({2}) issued by an entry <{3}> with a validity of {4} days\n\tfor: {5}"},
>
> I feel "issued by <{3}>" sounds more normal. No need to say "an entry". You
> decide it.
Removed "an entry".
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 04:03:50 GMT, Xue-Lei Andrew Fan wrote: >> Maybe we don't need to resolve it in this code change. If we look carefully >> at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 >> is using the signer's SKID in 10.1 as its own SKID and it has no AKID. >> Currently, keytool will generate a new SKID and use signer's SKID as AKID. >> If we really want to generate a certificate that's identical to the one in >> the RFC, we'll need a way to tell keytool to omit the AKID (something like >> "-ext akid=none"). > >> Maybe we don't need to resolve it in this code change. If we look carefully >> at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 >> is using the signer's SKID in 10.1 as its own SKID and it has no AKID. >> Currently, keytool will generate a new SKID and use signer's SKID as AKID. >> If we really want to generate a certificate that's identical to the one in >> the RFC, we'll need a way to tell keytool to omit the AKID (something like >> "-ext akid=none"). > > Better to use AKID for certification path building. I don't mean we will remove it by default. Just think there needs a way to remove either AKID or SKID, because we always generate them automatically. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 01:56:15 GMT, Weijun Wang wrote: > Maybe we don't need to resolve it in this code change. If we look carefully > at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 > is using the signer's SKID in 10.1 as its own SKID and it has no AKID. > Currently, keytool will generate a new SKID and use signer's SKID as AKID. If > we really want to generate a certificate that's identical to the one in the > RFC, we'll need a way to tell keytool to omit the AKID (something like "-ext > akid=none"). Better to use AKID for certification path building. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 01:56:15 GMT, Weijun Wang wrote: >> Only a few minor comments. Approved. > > Maybe we don't need to resolve it in this code change. If we look carefully > at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 > is using the signer's SKID in 10.1 as its own SKID and it has no AKID. > Currently, keytool will generate a new SKID and use signer's SKID as AKID. If > we really want to generate a certificate that's identical to the one in the > RFC, we'll need a way to tell keytool to omit the AKID (something like "-ext > akid=none"). A simple fix you can do this time although unrelated to the issue. `Main::createV3Extensions` shows a `@param akey` in spec but the actual argument name is `pkey`. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 01:44:03 GMT, Weijun Wang wrote: >> Hai-May Chao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> update with review comments > > Only a few minor comments. Approved. Maybe we don't need to resolve it in this code change. If we look carefully at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 is using the signer's SKID in 10.1 as its own SKID and it has no AKID. Currently, keytool will generate a new SKID and use signer's SKID as AKID. If we really want to generate a certificate that's identical to the one in the RFC, we'll need a way to tell keytool to omit the AKID (something like "-ext akid=none"). - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Thu, 1 Apr 2021 23:36:04 GMT, Hai-May Chao wrote:
>> Please review the changes that adds the -signer option to keytool
>> -genkeypair command. As key agreement algorithms do not have a signing
>> algorithm, the specified signer's private key will be used to sign and
>> generate a key agreement certificate.
>> CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update with review comments
Only a few minor comments. Approved.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1978:
> 1976: keypair.getPublicKeyAnyway(),
> 1977: signerSubjectKeyId);
> 1978: } else {
No need for an if-else block? `signerSubjectKeyId` is null in this case.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2013:
> 2011: x500Name};
> 2012: System.err.println(form.format(source));
> 2013: }
Either put the declaration of `source` outside the if-else block and move the
println line outside; or, move the declaration of `form` inside. It looks
confusing that `form` is declared outside and `source` inside.
src/java.base/share/classes/sun/security/tools/keytool/Resources.java line 310:
> 308: "Generating {0} bit {1} key pair and self-signed
> certificate ({2}) with a validity of {3} days\n\tfor: {4}"},
> 309:
> {"Generating.keysize.bit.keyAlgName.key.pair.and.a.certificate.sigAlgName.issued.by.an.entry.signerAlias.with.a.validity.of.validality.days.for",
> 310: "Generating {0} bit {1} key pair and a certificate ({2})
> issued by an entry <{3}> with a validity of {4} days\n\tfor: {5}"},
I feel "issued by <{3}>" sounds more normal. No need to say "an entry". You
decide it.
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
> Please review the changes that adds the -signer option to keytool -genkeypair > command. As key agreement algorithms do not have a signing algorithm, the > specified signer's private key will be used to sign and generate a key > agreement certificate. > CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325 Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: update with review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/3281/files - new: https://git.openjdk.java.net/jdk/pull/3281/files/d63818ba..157601f2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3281&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3281&range=02-03 Stats: 39 lines in 4 files changed: 7 ins; 19 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/3281.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3281/head:pull/3281 PR: https://git.openjdk.java.net/jdk/pull/3281
