> On Jun 4, 2020, at 7:29 PM, Hai-May Chao <hai-may.c...@oracle.com> wrote:
>
> 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.
I just meant that the keytool commands generating root.jks and root.pem are
exactly the same and there is no need to recreate it.
>
> 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.
OK.
Then how about we add a new command before line 155?
kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
This would prove the "-trustcacerts" on line 155 is really useful.
>
>>
>> - 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.
Same question 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.
I meant creating the cacerts in one method, something like
void createCacerts(String ks, String... crt);
and you can call createCacerts("mycacerts", "root.crt") to create it. The
method can call KeyStore APIs and not keytool commands.
BTW, what does caks.size() == 0 matter here?
Thanks,
Max
>
> 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
>>>
>>
>
