> On Jun 7, 2020, at 6:08 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
> Looks fine to me.
>
> For CSR, since there is already a "Note" there for these 2 options, you can
> add a few words about what -keystore and -trustcacerts can do.
Updated CSR as suggested.
Thanks,
Hai-May
>
> Thanks,
> Max
>
>> On Jun 8, 2020, at 4:01 AM, Hai-May Chao <hai-may.c...@oracle.com> wrote:
>>
>> Updated webrev -
>>
>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
>>
>> Thanks,
>> Hai-May
>>
>>
>>> On Jun 5, 2020, at 11:04 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
>>>
>>> I still think duplicated commands in TrustedCert.java are useless. Line 104
>>> and line 133 are exactly the same, line 109 and line 138 are exactly the
>>> same, and you haven't made any change to these 2 files in between.
>>>
>>> Same for line 80 and line 96 of TrustedCRL.java.
>>>
>>> Everything else is fine.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>> On Jun 6, 2020, at 2:25 AM, Hai-May Chao <hai-may.c...@oracle.com> wrote:
>>>>
>>>> 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
>>>>
>>>
>>
>
