Hi Max,
> On Jun 3, 2020, at 12:59 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
> The source change looks fine to me.
>
> In TrustedCert.java:
>
> - You can use FileOutputStream and Files.copy(Path,OutputStream) in cat().
This cat() is taken from WealAlg.java.
>
> - There is no need to recreate root.jks and root.pem.
The sequences of the commands used in this test scenario allows me to test
-printcert for the -trustcacerts and -keytsore options. We had discussion
offline about it. The test uses trusted certificates and checks no warnings on
the weak algorithms to address the requirement described in the bug. I believe
it does serve that purpose, and looks legitimate to me. There could be
different ways of testing a functionality, and please let me know if there is a
problem with the current approach.
Please also elaborate your comment about no need to recreate root.jks and
root.pem.
>
> - Why not use -trustcacerts below?
>
> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
Because here is to import the server (end-entity) cert, and it will not make a
difference for the test result whether to use the -trustcacerts or not. It’s
the ca (intermediate) cert needs to have it in this test scenario. I intended
to leave it out in #160 to distinguish between server and ca certs.
>
> - It's probably better to add a " " between cmd and options in patchcmd().
> Same in TrustedCRL.java.
Ok, will change it.
>
> In TrustedCRL.java:
>
> - No need to recreate ks and ca.crl. Just call "-printcrl" with different
> options.
Same reply as above.
>
> - Why create using MD5withRSA? Do you meant to warn about the weak algorithm?
Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA used in
root CA where no warning will be emitted. There is another -gencrl in #119
without using MD5withRSA so I’d have two test cases.
>
> Also I would suggest you create a dedicate method (maybe in
> SecurityTools.java) to create your own cacerts. There is no need to copy over
> the system cacerts, just make sure the file is created with the JKS
> storetype. We are thinking of upgrading the storetype of cacerts and it's
> nice to do this at a single place so we can modify it easily later.
I created a method in SecurityTools.java to create the own cacerts. With this
keystore, the subsequent importing a certificate reply would not work. It turns
out that its caks.size() is zero detected at establishCertChain() in
keytool/Main.java after root cert has been imported to that cacerts. At this
point I’d like to suggest a separate bug be filed to cover the cacerts
enhancement that you suggested.
Thanks,
Hai-May
> Thanks,
> Max
>
>
>> On Jun 2, 2020, at 2:37 AM, Hai-May Chao <hai-may.c...@oracle.com> wrote:
>>
>> Hi,
>>
>> I’d like to request a review for:
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244148
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8246269
>> Webrev: http://cr.openjdk.java.net/~hchao/8244148/webrev.00/
>>
>> The change is to add the support of -trustcacerts and -keystore options to
>> -printcert and -princrl command for keytool. This enables keytool to use the
>> trusted certificates when verifying untrusted artifacts that are signed by
>> CAs. It also incorporates Max’s change that consolidates the code to get the
>> default location of cacerts keystore.
>>
>> Thanks,
>> Hai-May
>>
>
