I re-read the CSR. The precise meaning of the 2 options for -printcert is: "If the cert is a trusted certificate in either keystore or cacerts, we will not warn if it uses a weak signature algorithm". This is so subtle and I wonder it's worth describing it. Or we just say "This command does not check for the weakness of a certificate's signature algorithm if it's a trusted certificate in the user keystore (specified by -keystore) or the `cacerts` keystore (if -trustcacerts is specified)".
For -printcrl, the "issuer CA" is a little confusing. I would say "This commands attempts to verify the CRL using a certificate from the user keystore (specified by -keystore) or the `cacerts` keystore (if -trustcacerts is specified), and print out a warning if it cannot be verified". Thanks, Max p.s. A new kind of check comes to my mind. Suppose you are printing a certificate whose own key is 2048-bit RSA, signature is using SHA256withRSA, but the signer key is only 512 bits (we can notice this from the size of the signature). Shall we print out a warning? > On Jun 11, 2020, at 11:21 AM, Weijun Wang <weijun.w...@oracle.com> wrote: > > "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 <hai-may.c...@oracle.com> wrote: >> >> >> >>> 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 >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
