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

[Hai-May Chao]

On Wed, 31 Mar 2021 13:36:39 GMT, Weijun Wang <weijun at openjdk.org> 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