RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair

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

RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair

Hai-May Chao-2
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

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

Commit messages:
 - 8260693: Provide the support for specifying a signer in keytool -genkeypair

Changes: https://git.openjdk.java.net/jdk/pull/3281/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3281&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260693
  Stats: 521 lines in 6 files changed: 499 ins; 0 del; 22 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair

Weijun Wang-2
On Wed, 31 Mar 2021 06:30:01 GMT, Hai-May Chao <[hidden email]> 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

Some comments on the CSR:
1. In the "Solution" section, we might need to point out that since there is no standard way to create a certificate request in this case (because the request must be signed by the owner's private key) we will not be able to enhance `-certreq` and `-gencert` to achieve the goal.
2. In the "Specification" section, "It is a required option...". I would prefer "This is especially useful when...". Also, we should not mention "JKS" since there can be other keystores using different key passwords. Just say "It can be specified if the private key of the signer entry is protected by a different password from the store password". Or you can look at how `-keypass` is described in other commands.
3. I don't think it's necessary to list `X448` and `X25519` in the "Default key size" section since that's the only key size for the algorithms. We only need to say `255 (when using -genkeypair and -keyalg is "EdDSA" or "XDH")`.

src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 114:

> 112:     }
> 113:
> 114:     /**

The original constructor can be modified to call `this(keyType,sigAlg,providerName,null,null)`. This is also a good time to modify `public CertAndKeyGen (String keyType, String sigAlg)` to call `this(keyType,sigAlg,null)`. Also please simplify the method javadoc.

src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 234:

> 232:             if (sigAlg == null) {
> 233:                 throw new IllegalArgumentException(
> 234:                         "Cannot derive signature algorithm from "

The text of the exception should be updated because a different key could be used.

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

> 1928:         }
> 1929:
> 1930:         if (reqSigner && signerAlias == null) {

What if we do not use a `reqSigner` flag? Will the error message be misleading. I guess it will something like "Cannot derive a signature algorithm from the key algorithm". My opinion is that it's quite enough.

If we keep it, we will have to maintain it.

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

> 1939:             signerFlag = true;
> 1940:
> 1941:             if (keyStore.containsAlias(signerAlias) == false) {

It's probably more precise to make sure the entry is a `PrivateKeyEntry` because we have other entries like `TrustedCertificateEntry` and `SecretKeyEntry`. Or you can double check this below to ensure both `signerPrivateKey` and `signerCert` are non null.

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

> 1949:                     (PrivateKey)recoverKey(signerAlias, storePass, signerKeyPass).fst;
> 1950:             Certificate signerCert = keyStore.getCertificate(signerAlias);
> 1951:             byte[] encoded = signerCert.getEncoded();

Most likely `signerCert` is already a `X509CertImpl`.

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

> 2011:         }
> 2012:
> 2013:         X509Certificate[] chain = new X509Certificate[1];

Since the chain might contain one, I'd suggest we just declare a `newCert` here. When signer flag is not on, we can simply get the chain with `new Certificate[] {newCert}`.

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

> 2023:                     ("Generating.keysize.bit.keyAlgName.key.pair.and.self.signed.certificate.sigAlgName.with.a.validity.of.validality.days.for"));
> 2024:             if (groupName != null) {
> 2025:                 keysize = KeyUtil.getKeySize(privKey);

Don't understand why `keysize` only needs to be calculated when there is no signer flag. In fact, we can just always use the `getKeySize` output below.

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

> 2041:         if (signerFlag) {
> 2042:             Certificate[] signerChain = keyStore.getCertificateChain(signerAlias);
> 2043:             Certificate[] tmpChain = new X509Certificate[signerChain.length + 1];

This is not a temp chain, it's the final chain.

src/java.base/share/classes/sun/security/tools/keytool/Resources.java line 312:

> 310:                 "Generating {0} bit {1} key pair and self-signed certificate ({2}) with a validity of {3} days\n\tfor: {4}"},
> 311:         {"Generating.keysize.bit.keyAlgName.key.pair.and.a.certificate.sigAlgName.issued.by.an.entry.specified.by.the.signer.option.with.a.validity.of.validality.days.for",
> 312:                 "Generating {0} bit {1} key pair and a certificate ({2}) issued by an entry specified by the -signer option with a validity of {3} days\n\tfor: {4}"},

How about printing out signer's alias?

src/java.base/share/classes/sun/security/util/KeyUtil.java line 102:

> 100:             size = pubk.getParams().getP().bitLength();
> 101:         } else if (key instanceof XECKey) {
> 102:             String name = ((NamedParameterSpec)((XECKey) key).getParams()).getName();

I hope the params is always `NamedParameterSpec` but would rather be safe to check instanceof first.

test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 230:

> 228:     }
> 229:
> 230:     static void testSignerJKS() throws Exception {

Please add some comments on why testing with JKS is necessary.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Hai-May Chao-2
In reply to this post by Hai-May Chao-2
> 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:

  Updated with review comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3281/files
  - new: https://git.openjdk.java.net/jdk/pull/3281/files/f33cf92a..5d4d11cc

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

  Stats: 129 lines in 6 files changed: 38 ins; 42 del; 49 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Hai-May Chao-2
In reply to this post by Weijun Wang-2
On Wed, 31 Mar 2021 13:36:39 GMT, Weijun Wang <[hidden email]> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Updated with review comments
>
> Some comments on the CSR:
> 1. In the "Solution" section, we might need to point out that since there is no standard way to create a certificate request in this case (because the request must be signed by the owner's private key) we will not be able to enhance `-certreq` and `-gencert` to achieve the goal.
> 2. In the "Specification" section, "It is a required option...". I would prefer "This is especially useful when...". Also, we should not mention "JKS" since there can be other keystores using different key passwords. Just say "It can be specified if the private key of the signer entry is protected by a different password from the store password". Or you can look at how `-keypass` is described in other commands.
> 3. I don't think it's necessary to list `X448` and `X25519` in the "Default key size" section since that's the only key size for the algorithms. We only need to say `255 (when using -genkeypair and -keyalg is "EdDSA" or "XDH")`.

@wangweij Thanks for the review. Updated the CSR with your comments.

> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 114:
>
>> 112:     }
>> 113:
>> 114:     /**
>
> The original constructor can be modified to call `this(keyType,sigAlg,providerName,null,null)`. This is also a good time to modify `public CertAndKeyGen (String keyType, String sigAlg)` to call `this(keyType,sigAlg,null)`. Also please simplify the method javadoc.

Updated the original constructor to call `this(keyType,sigAlg,providerName,null,null)` and its comment block. But leave another original constructor `public CertAndKeyGen (String keyType, String sigAlg)` unchanged, because it does not have `providerName` and `NoSuchProviderException` should not be declared like other constructors.

> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 234:
>
>> 232:             if (sigAlg == null) {
>> 233:                 throw new IllegalArgumentException(
>> 234:                         "Cannot derive signature algorithm from "
>
> The text of the exception should be updated because a different key could be used.

Done.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1930:
>
>> 1928:         }
>> 1929:
>> 1930:         if (reqSigner && signerAlias == null) {
>
> What if we do not use a `reqSigner` flag? Will the error message be misleading. I guess it will something like "Cannot derive a signature algorithm from the key algorithm". My opinion is that it's quite enough.
>
> If we keep it, we will have to maintain it.

Removed this flag to let the "Cannot derive signature algorithm from keyAlg" be emitted instead.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1941:
>
>> 1939:             signerFlag = true;
>> 1940:
>> 1941:             if (keyStore.containsAlias(signerAlias) == false) {
>
> It's probably more precise to make sure the entry is a `PrivateKeyEntry` because we have other entries like `TrustedCertificateEntry` and `SecretKeyEntry`. Or you can double check this below to ensure both `signerPrivateKey` and `signerCert` are non null.

As `RecoveryKey()` will make sure if the entry exists in the keystore and is a `PrivateKeyEntry`, removed this checking and updated to check for if `signerCert` is null.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1951:
>
>> 1949:                     (PrivateKey)recoverKey(signerAlias, storePass, signerKeyPass).fst;
>> 1950:             Certificate signerCert = keyStore.getCertificate(signerAlias);
>> 1951:             byte[] encoded = signerCert.getEncoded();
>
> Most likely `signerCert` is already a `X509CertImpl`.

Updated to only do this line of code when it is not a `X509CertImpl`.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2013:
>
>> 2011:         }
>> 2012:
>> 2013:         X509Certificate[] chain = new X509Certificate[1];
>
> Since the chain might contain one, I'd suggest we just declare a `newCert` here. When signer flag is not on, we can simply get the chain with `new Certificate[] {newCert}`.

Not sure the reason why a change is needed for the existing logic.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2025:
>
>> 2023:                     ("Generating.keysize.bit.keyAlgName.key.pair.and.self.signed.certificate.sigAlgName.with.a.validity.of.validality.days.for"));
>> 2024:             if (groupName != null) {
>> 2025:                 keysize = KeyUtil.getKeySize(privKey);
>
> Don't understand why `keysize` only needs to be calculated when there is no signer flag. In fact, we can just always use the `getKeySize` output below.

For the no -signer case, calculating the `keysize` based on the `groupName` is the original logic, and `groupName` and `keysize` can not coexist. Updated the -signer case to do the same.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2043:
>
>> 2041:         if (signerFlag) {
>> 2042:             Certificate[] signerChain = keyStore.getCertificateChain(signerAlias);
>> 2043:             Certificate[] tmpChain = new X509Certificate[signerChain.length + 1];
>
> This is not a temp chain, it's the final chain.

Renamed this variable name to `finalChain`.

> src/java.base/share/classes/sun/security/tools/keytool/Resources.java line 312:
>
>> 310:                 "Generating {0} bit {1} key pair and self-signed certificate ({2}) with a validity of {3} days\n\tfor: {4}"},
>> 311:         {"Generating.keysize.bit.keyAlgName.key.pair.and.a.certificate.sigAlgName.issued.by.an.entry.specified.by.the.signer.option.with.a.validity.of.validality.days.for",
>> 312:                 "Generating {0} bit {1} key pair and a certificate ({2}) issued by an entry specified by the -signer option with a validity of {3} days\n\tfor: {4}"},
>
> How about printing out signer's alias?

Added signer's alias to the message.

> src/java.base/share/classes/sun/security/util/KeyUtil.java line 102:
>
>> 100:             size = pubk.getParams().getP().bitLength();
>> 101:         } else if (key instanceof XECKey) {
>> 102:             String name = ((NamedParameterSpec)((XECKey) key).getParams()).getName();
>
> I hope the params is always `NamedParameterSpec` but would rather be safe to check instanceof first.

Added the checking.

> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 230:
>
>> 228:     }
>> 229:
>> 230:     static void testSignerJKS() throws Exception {
>
> Please add some comments on why testing with JKS is necessary.

Added the comments about JKS keystore.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Weijun Wang-2
On Thu, 1 Apr 2021 16:25:13 GMT, Hai-May Chao <[hidden email]> wrote:

>> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 114:
>>
>>> 112:     }
>>> 113:
>>> 114:     /**
>>
>> The original constructor can be modified to call `this(keyType,sigAlg,providerName,null,null)`. This is also a good time to modify `public CertAndKeyGen (String keyType, String sigAlg)` to call `this(keyType,sigAlg,null)`. Also please simplify the method javadoc.
>
> Updated the original constructor to call `this(keyType,sigAlg,providerName,null,null)` and its comment block. But leave another original constructor `public CertAndKeyGen (String keyType, String sigAlg)` unchanged, because it does not have `providerName` and `NoSuchProviderException` should not be declared like other constructors.

You're right.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Weijun Wang-2
In reply to this post by Hai-May Chao-2
On Thu, 1 Apr 2021 16:26:39 GMT, Hai-May Chao <[hidden email]> wrote:

>> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2013:
>>
>>> 2011:         }
>>> 2012:
>>> 2013:         X509Certificate[] chain = new X509Certificate[1];
>>
>> Since the chain might contain one, I'd suggest we just declare a `newCert` here. When signer flag is not on, we can simply get the chain with `new Certificate[] {newCert}`.
>
> Not sure the reason why a change is needed for the existing logic.

With a signer, it makes no sense to create a single-cert array at the beginning. I am suggesting:
X509Certificate newCert  = keypair.getSelfCertificate(...);
Certificate[] finalChain;
if (signerFlag) {
    finalChain = new ...
    finalChain[0] = newCert;
} else {
   finalChain = new Certificate[] { newCert };
}
keyStore.setEntry(..., finalChain);

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Weijun Wang-2
In reply to this post by Hai-May Chao-2
On Thu, 1 Apr 2021 16:34:43 GMT, Hai-May Chao <[hidden email]> 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:
>
>   Updated with review comments

src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 88:

> 86:      * constructor CertAndKeyGen(String keyType, String sigAlg,
> 87:      * String providerName, PrivateKey signerPrivateKey,
> 88:      * X500Name signerSubjectName)

You can simply add a `@see #CertAndKeyGen(String, String, String, PrivateKey, X500Name)`.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Weijun Wang-2
In reply to this post by Hai-May Chao-2
On Thu, 1 Apr 2021 16:34:43 GMT, Hai-May Chao <[hidden email]> 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:
>
>   Updated with review comments

src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 132:

> 130:         if (signerPrivateKey != null) {
> 131:             this.signerFlag = true;
> 132:         }

If you make this line `this.sigerFlag = signerPrivateKey != null`, then we can make most field final.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Weijun Wang-2
In reply to this post by Hai-May Chao-2
On Thu, 1 Apr 2021 16:25:49 GMT, Hai-May Chao <[hidden email]> wrote:

>> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1941:
>>
>>> 1939:             signerFlag = true;
>>> 1940:
>>> 1941:             if (keyStore.containsAlias(signerAlias) == false) {
>>
>> It's probably more precise to make sure the entry is a `PrivateKeyEntry` because we have other entries like `TrustedCertificateEntry` and `SecretKeyEntry`. Or you can double check this below to ensure both `signerPrivateKey` and `signerCert` are non null.
>
> As `RecoveryKey()` will make sure if the entry exists in the keystore and is a `PrivateKeyEntry`, removed this checking and updated to check for if `signerCert` is null.

Yes, it must be a private key entry. On the other hand, I think it's unnecessary to check about the `signerCert`. I don't think it'll be ever null.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v3]

Hai-May Chao-2
In reply to this post by Hai-May Chao-2
> 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:

  Updated with review comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3281/files
  - new: https://git.openjdk.java.net/jdk/pull/3281/files/5d4d11cc..d63818ba

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

  Stats: 26 lines in 2 files changed: 3 ins; 15 del; 8 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]

Hai-May Chao-2
In reply to this post by Weijun Wang-2
On Thu, 1 Apr 2021 16:53:31 GMT, Weijun Wang <[hidden email]> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Updated with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 88:
>
>> 86:      * constructor CertAndKeyGen(String keyType, String sigAlg,
>> 87:      * String providerName, PrivateKey signerPrivateKey,
>> 88:      * X500Name signerSubjectName)
>
> You can simply add a `@see #CertAndKeyGen(String, String, String, PrivateKey, X500Name)`.

Done.

> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 132:
>
>> 130:         if (signerPrivateKey != null) {
>> 131:             this.signerFlag = true;
>> 132:         }
>
> If you make this line `this.sigerFlag = signerPrivateKey != null`, then we can make most field final.

Done, and no change to final.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v3]

Hai-May Chao-2
In reply to this post by Weijun Wang-2
On Thu, 1 Apr 2021 16:49:19 GMT, Weijun Wang <[hidden email]> wrote:

>> Not sure the reason why a change is needed for the existing logic.
>
> With a signer, it makes no sense to create a single-cert array at the beginning. I am suggesting:
> X509Certificate newCert  = keypair.getSelfCertificate(...);
> Certificate[] finalChain;
> if (signerFlag) {
>     finalChain = new ...
>     finalChain[0] = newCert;
> } else {
>    finalChain = new Certificate[] { newCert };
> }
> keyStore.setEntry(..., finalChain);

Done.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v3]

Hai-May Chao-2
In reply to this post by Weijun Wang-2
On Thu, 1 Apr 2021 17:04:33 GMT, Weijun Wang <[hidden email]> wrote:

>> As `RecoveryKey()` will make sure if the entry exists in the keystore and is a `PrivateKeyEntry`, removed this checking and updated to check for if `signerCert` is null.
>
> Yes, it must be a private key entry. On the other hand, I think it's unnecessary to check about the `signerCert`. I don't think it'll be ever null.

Ok, removed this checking.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v3]

Weijun Wang-2
In reply to this post by Hai-May Chao-2
On Thu, 1 Apr 2021 20:37:47 GMT, Hai-May Chao <[hidden email]> 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:
>
>   Updated with review comments

src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 104:

> 102:      *          key will be chosen after the first keypair is generated.
> 103:      * @param providerName name of the provider
> 104:      * @param signerPrivateKey signer's private key

Add an "(optional)" or "can be null" to the last 2 `@param`s.

src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 296:

> 294:     // Create a self-signed certificate, or a certificate that is signed by
> 295:     // a signer when the signer's private key is provided.
> 296:     public X509Certificate getSelfCertificate (X500Name myname, Date firstDate,

It'a pity the method name still contains "self", but I'm OK to keep it as is.

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

> 1161:             }
> 1162:             doGenKeyPair(alias, dname, keyAlgName, keysize, groupName, sigAlgName,
> 1163:                     signerAlias);

Maybe we can just keep using the old argument list. `signerAlias` is a global variable and it's visible everywhere.

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

> 1925:         CertAndKeyGen keypair;
> 1926:         if (signerAlias != null) {
> 1927:             signerFlag = true;

Is the `signerFlag` really useful? We can just check `signerAlias != null`.

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

> 1976:             } else {
> 1977:                 certImpl = new X509CertImpl(signerCert.getEncoded());
> 1978:             }

The exact same 7 lines above also appears in the code change above. Is it possible to combine them?

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.specified.by.the.signer.option.with.a.validity.of.validality.days.for",
> 310:                 "Generating {0} bit {1} key pair and a certificate ({2}) issued by an entry <{3}> specified by the -signer option with a validity of {4} days\n\tfor: {5}"},

A little too long? Can we remove the "specified by the -signer option" words or even the whole "issued by an entry <{3}> specified by the -signer option"?

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v3]

Hai-May Chao-2
On Thu, 1 Apr 2021 21:27:52 GMT, Weijun Wang <[hidden email]> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Updated with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line 104:
>
>> 102:      *          key will be chosen after the first keypair is generated.
>> 103:      * @param providerName name of the provider
>> 104:      * @param signerPrivateKey signer's private key
>
> Add an "(optional)" or "can be null" to the last 2 `@param`s.

Done.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1163:
>
>> 1161:             }
>> 1162:             doGenKeyPair(alias, dname, keyAlgName, keysize, groupName, sigAlgName,
>> 1163:                     signerAlias);
>
> Maybe we can just keep using the old argument list. `signerAlias` is a global variable and it's visible everywhere.

I'd prefer to have it. It is the same as other global arguments being passed to this method.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1927:
>
>> 1925:         CertAndKeyGen keypair;
>> 1926:         if (signerAlias != null) {
>> 1927:             signerFlag = true;
>
> Is the `signerFlag` really useful? We can just check `signerAlias != null`.

Sure, use `signerAlias` instead.

> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1978:
>
>> 1976:             } else {
>> 1977:                 certImpl = new X509CertImpl(signerCert.getEncoded());
>> 1978:             }
>
> The exact same 7 lines above also appears in the code change above. Is it possible to combine them?

Here is mainly to locate the signer’s SKID, and moved it up to the first occurrence of these lines of code to combine them.

> 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.specified.by.the.signer.option.with.a.validity.of.validality.days.for",
>> 310:                 "Generating {0} bit {1} key pair and a certificate ({2}) issued by an entry <{3}> specified by the -signer option with a validity of {4} days\n\tfor: {5}"},
>
> A little too long? Can we remove the "specified by the -signer option" words or even the whole "issued by an entry <{3}> specified by the -signer option"?

Removed the "specified by the -signer option”, and keep the "issued by an entry <{3}>” that I agreed it’d be good to print out the signer’s alias in response to your previous review comment.

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

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

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]

Hai-May Chao-2
In reply to this post by Hai-May Chao-2
> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]

Weijun Wang-2
On Thu, 1 Apr 2021 23:36:04 GMT, Hai-May Chao <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]

Weijun Wang-2
On Fri, 2 Apr 2021 01:44:03 GMT, Weijun Wang <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]

Weijun Wang-2
On Fri, 2 Apr 2021 01:56:15 GMT, Weijun Wang <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]

Xue-Lei Andrew Fan
In reply to this post by Weijun Wang-2
On Fri, 2 Apr 2021 01:56:15 GMT, Weijun Wang <[hidden email]> 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
12