<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Hai-May,<br>
</p>
<div class="moz-cite-prefix">On 2020/6/8 04:01, Hai-May Chao wrote:<br>
</div>
<blockquote type="cite"
cite="mid:8786A3F4-A140-4FCC-A6AB-6EAB5788859D@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Updated webrev -
<div class=""><br class="">
</div>
<div class=""><a
href="https://cr.openjdk.java.net/~hchao/8244148/webrev.02/"
class="" moz-do-not-send="true">https://cr.openjdk.java.net/~hchao/8244148/webrev.02/</a></div>
</blockquote>
-- src/java.base/share/classes/sun/security/util/FilePaths.java<br>
Would it be better to use String.join() or even java.nio.file.Path
to<br>
build the file path?<br>
<br>
--
src/java.base/share/classes/sun/security/util/AnchorCertificates.java<br>
55 File f = new File(FilePaths.cacerts());<br>
...<br>
59 try (FileInputStream fis = new
FileInputStream(f)) {<br>
f is used only once, so it may be unnecessary.<br>
<br>
56 KeyStore cacerts;<br>
Though it's not in your change, would you mind to declare this
variable<br>
in the try block? I just want to narrow the variable scope.<br>
<br>
-- test/lib/jdk/test/lib/SecurityTools.java<br>
Could move method createCacerts() to
test/lib/jdk/test/lib/security/KeyStoreUtils.java?<br>
This class contains a set of methods for creating trust/key stores.<br>
And getCertFromFile() in
test/lib/jdk/test/lib/security/CertUtils.java<br>
can load certificate from a file.<br>
<br>
289 int pos = 0;<br>
...<br>
294 for (String crt : crts) {<br>
If not using for-each style, pos can be declared in the for
statement.<br>
<br>
--
test/jdk/sun/security/tools/keytool/fakecacerts/java.base/sun/security/util/FilePaths.java<br>
This patch can be used by other tests, so could you please move it
to a<br>
common path? Like
test/jdk/sun/security/util/module_patch/java.base/sun/security/util/FilePaths.java<br>
<br>
32 public static String cacerts() {<br>
33 return "mycacerts";<br>
34 }<br>
Could it be flexible for returning an alternative path?<br>
For example, accept a system property, say test.cacerts, for
specifying<br>
an alternative path. mycacerts is the default value of this
property.<br>
<br>
Best regards,<br>
John Jiang<br>
<br>
<blockquote type="cite"
cite="mid:8786A3F4-A140-4FCC-A6AB-6EAB5788859D@oracle.com">
<div class=""><br class="">
</div>
<div class="">Thanks,</div>
<div class="">Hai-May</div>
<div class=""><br class="">
</div>
<div class="">
<div><br class="">
<blockquote type="cite" class="">
<div class="">On Jun 5, 2020, at 11:04 PM, Weijun Wang <<a
href="mailto:weijun.wang@oracle.com" class=""
moz-do-not-send="true">weijun.wang@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">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.<br class="">
<br class="">
Same for line 80 and line 96 of TrustedCRL.java.<br
class="">
<br class="">
Everything else is fine.<br class="">
<br class="">
Thanks,<br class="">
Max<br class="">
<br class="">
<br class="">
<blockquote type="cite" class="">On Jun 6, 2020, at 2:25
AM, Hai-May Chao <<a
href="mailto:hai-may.chao@oracle.com" class=""
moz-do-not-send="true">hai-may.chao@oracle.com</a>>
wrote:<br class="">
<br class="">
Updated webrev - <br class="">
<br class="">
<a
href="https://cr.openjdk.java.net/~hchao/8244148/webrev.01/"
class="" moz-do-not-send="true">https://cr.openjdk.java.net/~hchao/8244148/webrev.01/</a><br
class="">
<br class="">
Added one command line -importcert in TrustCert.java.<br
class="">
Added createCacerts() in test/lib SecurityTools.java.<br
class="">
<br class="">
Thanks,<br class="">
Hai-May<br class="">
<br class="">
<br class="">
<br class="">
<blockquote type="cite" class="">On Jun 4, 2020, at
5:57 AM, Weijun Wang <a class="moz-txt-link-rfc2396E" href="mailto:weijun.wang@oracle.com"><weijun.wang@oracle.com></a>
wrote:<br class="">
<br class="">
<br class="">
<br class="">
<blockquote type="cite" class="">On Jun 4, 2020, at
7:29 PM, Hai-May Chao
<a class="moz-txt-link-rfc2396E" href="mailto:hai-may.chao@oracle.com"><hai-may.chao@oracle.com></a> wrote:<br class="">
<br class="">
Hi Max,<br class="">
<br class="">
<blockquote type="cite" class="">On Jun 3, 2020,
at 12:59 AM, Weijun Wang
<a class="moz-txt-link-rfc2396E" href="mailto:weijun.wang@oracle.com"><weijun.wang@oracle.com></a> wrote:<br
class="">
<br class="">
The source change looks fine to me.<br class="">
<br class="">
In TrustedCert.java:<br class="">
<br class="">
- You can use FileOutputStream and
Files.copy(Path,OutputStream) in cat().<br
class="">
</blockquote>
<br class="">
This cat() is taken from WealAlg.java.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
- There is no need to recreate root.jks and
root.pem.<br class="">
</blockquote>
<br class="">
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.<br class="">
</blockquote>
<br class="">
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.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
Please also elaborate your comment about no need
to recreate root.jks and root.pem.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
- Why not use -trustcacerts below?<br class="">
<br class="">
160 kt("-importcert -file server.pem
-noprompt", "server.jks”);<br class="">
</blockquote>
<br class="">
<br class="">
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.<br class="">
</blockquote>
<br class="">
OK.<br class="">
<br class="">
Then how about we add a new command before line 155?<br
class="">
<br class="">
kt("-importcert -file ca.pem",
"ca.jks").shouldNotHaveExitValue(0);<br class="">
<br class="">
This would prove the "-trustcacerts" on line 155 is
really useful.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
<blockquote type="cite" class=""><br class="">
- It's probably better to add a " " between cmd
and options in patchcmd(). Same in
TrustedCRL.java.<br class="">
</blockquote>
<br class="">
Ok, will change it.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
In TrustedCRL.java:<br class="">
<br class="">
- No need to recreate ks and ca.crl. Just call
"-printcrl" with different options.<br class="">
</blockquote>
<br class="">
Same reply as above.<br class="">
</blockquote>
<br class="">
Same question as above.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
<blockquote type="cite" class=""><br class="">
- Why create using MD5withRSA? Do you meant to
warn about the weak algorithm?<br class="">
</blockquote>
<br class="">
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.<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
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.<br class="">
</blockquote>
<br class="">
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.<br
class="">
</blockquote>
<br class="">
I meant creating the cacerts in one method,
something like<br class="">
<br class="">
void createCacerts(String ks, String... crt);<br
class="">
<br class="">
and you can call createCacerts("mycacerts",
"root.crt") to create it. The method can call
KeyStore APIs and not keytool commands.<br class="">
<br class="">
BTW, what does caks.size() == 0 matter here?<br
class="">
<br class="">
Thanks,<br class="">
Max<br class="">
<br class="">
<br class="">
<blockquote type="cite" class=""><br class="">
Thanks,<br class="">
Hai-May<br class="">
<br class="">
<br class="">
<blockquote type="cite" class="">Thanks,<br
class="">
Max<br class="">
<br class="">
<br class="">
<blockquote type="cite" class="">On Jun 2, 2020,
at 2:37 AM, Hai-May Chao
<a class="moz-txt-link-rfc2396E" href="mailto:hai-may.chao@oracle.com"><hai-may.chao@oracle.com></a> wrote:<br
class="">
<br class="">
Hi,<br class="">
<br class="">
I’d like to request a review for:<br class="">
<br class="">
JBS:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8244148">https://bugs.openjdk.java.net/browse/JDK-8244148</a><br
class="">
CSR:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8246269">https://bugs.openjdk.java.net/browse/JDK-8246269</a><br
class="">
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~hchao/8244148/webrev.00/">http://cr.openjdk.java.net/~hchao/8244148/webrev.00/</a><br
class="">
<br class="">
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.<br class="">
<br class="">
Thanks,<br class="">
Hai-May<br class="">
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br class="">
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
</body>
</html>