Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
On Thu, 1 Apr 2021 16:53:31 GMT, Weijun Wang 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
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
On Thu, 1 Apr 2021 16:34:43 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: > > 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
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
On Thu, 1 Apr 2021 16:25:49 GMT, Hai-May Chao 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
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
On Thu, 1 Apr 2021 16:34:43 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: > > 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
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
On Thu, 1 Apr 2021 16:26:39 GMT, Hai-May Chao 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
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
On Thu, 1 Apr 2021 16:25:13 GMT, Hai-May Chao 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
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
> 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
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v2]
On Wed, 31 Mar 2021 13:36:39 GMT, Weijun Wang 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,