> On Jun 9, 2020, at 5:58 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
> Good to see both -keystore and -trustcacerts mentioned. Some comments:
>
> 1. I think there is no need to say "from -file cert_file". Or, do you mean
> the new function does not apply to those from -sslserver and -jarfile? If so,
> that might be a problem.
You’re right. Removed it.
>
> 2. While you said "attempts to construct a chain of trust", do you also want
> to describe what happens if it succeeds or fails?
Updated manpage.
>
> 3. It will be nice if you can include the exact diff of the man page files
> either inside the CSR itself or as a comment.
>
Included the diff of the manpage in the CSR.
Thanks,
Hai-May
> Thanks,
> Max
>
>> On Jun 9, 2020, at 10:51 PM, Hai-May Chao <hai-may.c...@oracle.com> wrote:
>>
>>
>>
>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
