Updated webrev -
https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
Added one command line -importcert in TrustCert.java.
Added createCacerts() in test/lib SecurityTools.java.
Thanks,
Hai-May
> On Jun 4, 2020, at 5:57 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
>
>
>> 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
