Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:42:16 GMT, Hai-May Chao wrote:
>> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
>>
>>> 94:
>>> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
>>> 96: if (certChain.length != 2) {
>>
>> Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
>> throw an exception but call `System.exit(1)`? We usually do not call this
>> method in a test because the test framework must take great care so that
>> itself does not get terminated.
>
> Changed to throw the exception for errors. Meanwhile, the test is pretty
> straightforward/simple, and using if comparison should serve its testing need
> and it does not make the code complicated.
You can choose your style, but `Asserts.assertEquals(certChain.length, 2,
"Generated cert chain is in error")` is definitely simpler and will give you
more info when it fails.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:48:20 GMT, Hai-May Chao wrote: >> In testSignerJKS() has made sure that the new entry created with new key >> password can *only* be accessed when a correct key password is provided in >> order to retrieve the corresponding signer’s private key. > > The new entry protected by the new key password is an existing function, and > its testing should have been covered. OK. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:42:18 GMT, Hai-May Chao wrote: >> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 299: >> >>> 297: System.exit(1); >>> 298: } >>> 299: >> >> Since you are here, you can check if the new entry is indeed protected by >> the new key password. > > In testSignerJKS() has made sure that the new entry created with new key > password can *only* be accessed when a correct key password is provided in > order to retrieve the corresponding signer’s private key. The new entry protected by the new key password is an existing function, and its testing should have been covered. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 00:01:57 GMT, Weijun Wang wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java
> line 88:
>
>> 86: */
>> 87: public CertAndKeyGen (String keyType, String sigAlg, String
>> providerName)
>> 88: throws NoSuchAlgorithmException, NoSuchProviderException
>
> Indent the line 8 spaces further.
Done.
> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
>
>> 94:
>> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
>> 96: if (certChain.length != 2) {
>
> Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
> throw an exception but call `System.exit(1)`? We usually do not call this
> method in a test because the test framework must take great care so that
> itself does not get terminated.
Changed to throw the exception for errors. Meanwhile, the test is pretty
straightforward/simple, and using if comparison should serve its testing need
and it does not make the code complicated.
> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 299:
>
>> 297: System.exit(1);
>> 298: }
>> 299:
>
> Since you are here, you can check if the new entry is indeed protected by the
> new key password.
In testSignerJKS() has made sure that the new entry created with new key
password can *only* be accessed when a correct key password is provided in
order to retrieve the corresponding signer’s private key.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Wed, 7 Apr 2021 23:17:53 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:
>
> update with review comments
No comment on src side.
src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line
88:
> 86: */
> 87: public CertAndKeyGen (String keyType, String sigAlg, String
> providerName)
> 88: throws NoSuchAlgorithmException, NoSuchProviderException
Indent the line 8 spaces further.
test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
> 94:
> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
> 96: if (certChain.length != 2) {
Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
throw an exception but call `System.exit(1)`? We usually do not call this
method in a test because the test framework must take great care so that itself
does not get terminated.
test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 299:
> 297: System.exit(1);
> 298: }
> 299:
Since you are here, you can check if the new entry is indeed protected by the
new key password.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
> 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: update with review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/3281/files - new: https://git.openjdk.java.net/jdk/pull/3281/files/157601f2..3be5bc98 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3281&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3281&range=03-04 Stats: 28 lines in 3 files changed: 0 ins; 12 del; 16 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
