On Wed, 31 Mar 2021 13:36:39 GMT, Weijun Wang <wei...@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
