Updated webrev - https://cr.openjdk.java.net/~hchao/8244148/webrev.02/ <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 >> >
