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=3281=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3281=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
