On Thu, 1 Apr 2021 20:37:47 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 > > 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