On Wed, 31 Mar 2021 06:30:01 GMT, Hai-May Chao <hc...@openjdk.org> 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
