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.
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 >>> >> >
