"to a self-signed certificate in the keystore". This is not correct. What we look for is a TrustedCertificateEntry.
"It emits warning when a certificate is not trusted and uses weak algorithms". Precisely, it's "uses a weak signature algorithm". --Max > On Jun 10, 2020, at 5:31 PM, Hai-May Chao <[email protected]> wrote: > > > >> On Jun 9, 2020, at 5:58 PM, Weijun Wang <[email protected]> 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 <[email protected]> wrote: >>> >>> >>> >>>> On Jun 7, 2020, at 6:08 PM, Weijun Wang <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>>>> 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 <[email protected]> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Hi Max, >>>>>>>>> >>>>>>>>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang <[email protected]> >>>>>>>>>> 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 <[email protected]> >>>>>>>>>>> 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 >>>>>>> >>>>>> >>>>> >>>> >>> >> >
