Hi John, Updated Webrev - https://cr.openjdk.java.net/~hchao/8244148/webrev.03/
> On Jun 11, 2020, at 1:45 AM, sha.ji...@oracle.com wrote: > > Hi Hai-May, > > On 2020/6/8 04:01, Hai-May Chao wrote: >> Updated webrev - >> >> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/ >> <https://cr.openjdk.java.net/~hchao/8244148/webrev.02/>-- >> src/java.base/share/classes/sun/security/util/FilePaths.java > Would it be better to use String.join() or even java.nio.file.Path to > build the file path? > The current code is based on the original code such as in KeyStoreUtil.java and is consistent with others. > -- src/java.base/share/classes/sun/security/util/AnchorCertificates.java > 55 File f = new File(FilePaths.cacerts()); > ... > 59 try (FileInputStream fis = new FileInputStream(f)) { > f is used only once, so it may be unnecessary. This is the existing code struct which is working fine. > > 56 KeyStore cacerts; > Though it's not in your change, would you mind to declare this variable > in the try block? I just want to narrow the variable scope. Done. > > -- test/lib/jdk/test/lib/SecurityTools.java > Could move method createCacerts() to > test/lib/jdk/test/lib/security/KeyStoreUtils.java? > This class contains a set of methods for creating trust/key stores. > And getCertFromFile() in test/lib/jdk/test/lib/security/CertUtils.java > can load certificate from a file. Moved the method createCacerts() to the suggested location under test/. Tried to use getCertFromFile() and it did not work as it read from test.src. So we need to read the cert directly. > > 289 int pos = 0; > ... > 294 for (String crt : crts) { > If not using for-each style, pos can be declared in the for statement. Ok, changed to not using for-each style. > > -- > test/jdk/sun/security/tools/keytool/fakecacerts/java.base/sun/security/util/FilePaths.java > This patch can be used by other tests, so could you please move it to a > common path? Like > test/jdk/sun/security/util/module_patch/java.base/sun/security/util/FilePaths.java Moved the patch to the suggested location under test/. > > 32 public static String cacerts() { > 33 return "mycacerts"; > 34 } > Could it be flexible for returning an alternative path? > For example, accept a system property, say test.cacerts, for specifying > an alternative path. mycacerts is the default value of this property. Done. Thanks, Hai-May > > Best regards, > John Jiang > >> >> Thanks, >> Hai-May >> >> >>> On Jun 5, 2020, at 11:04 PM, Weijun Wang <weijun.w...@oracle.com >>> <mailto: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 >>>> <mailto:hai-may.c...@oracle.com>> wrote: >>>> >>>> Updated webrev - >>>> >>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/ >>>> <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> >>>>> <mailto:weijun.w...@oracle.com> wrote: >>>>> >>>>> >>>>> >>>>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao <hai-may.c...@oracle.com> >>>>>> <mailto:hai-may.c...@oracle.com> wrote: >>>>>> >>>>>> Hi Max, >>>>>> >>>>>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang <weijun.w...@oracle.com> >>>>>>> <mailto: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> >>>>>>>> <mailto:hai-may.c...@oracle.com> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> I’d like to request a review for: >>>>>>>> >>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244148 >>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8244148> >>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8246269 >>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8246269> >>>>>>>> Webrev: http://cr.openjdk.java.net/~hchao/8244148/webrev.00/ >>>>>>>> <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 >>>> >>> >>
