<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/13 06:34, Hai-May Chao wrote:<br>
</div>
<blockquote type="cite"
cite="mid:80FFB20F-6714-484D-9C3B-E046CCF3AD09@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Hi John,
<div class=""><br class="">
</div>
<div class="">Updated Webrev -</div>
<div class=""><a
href="https://cr.openjdk.java.net/~hchao/8244148/webrev.03/"
class="" moz-do-not-send="true">https://cr.openjdk.java.net/~hchao/8244148/webrev.03/</a></div>
</blockquote>
Thanks for this updated webrev!<br>
I have no more comment.<br>
<br>
Best regards,<br>
John Jiang<br>
<br>
<blockquote type="cite"
cite="mid:80FFB20F-6714-484D-9C3B-E046CCF3AD09@oracle.com">
<div class="">
<div><br class="">
<blockquote type="cite" class="">
<div class="">On Jun 11, 2020, at 1:45 AM, <a
href="mailto:sha.jiang@oracle.com" class=""
moz-do-not-send="true">sha.jiang@oracle.com</a> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8" class="">
<div class="">
<p class="">Hi Hai-May,<br class="">
</p>
<div class="moz-cite-prefix">On 2020/6/8 04:01, Hai-May
Chao wrote:<br class="">
</div>
<blockquote type="cite"
cite="mid:8786A3F4-A140-4FCC-A6AB-6EAB5788859D@oracle.com"
class="">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8" class="">
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
class="">
Would it be better to use String.join() or even
java.nio.file.Path to<br class="">
build the file path?<br class="">
<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>The current code is based on the original code such as in
KeyStoreUtil.java and is consistent with others.</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""> --
src/java.base/share/classes/sun/security/util/AnchorCertificates.java<br
class="">
55 File f = new
File(FilePaths.cacerts());<br class="">
...<br class="">
59 try (FileInputStream fis = new
FileInputStream(f)) {<br class="">
f is used only once, so it may be unnecessary.<br
class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
This is the existing code struct which is working fine.</div>
<div><br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""> <br class="">
56 KeyStore cacerts;<br class="">
Though it's not in your change, would you mind to
declare this variable<br class="">
in the try block? I just want to narrow the variable
scope.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
Done.</div>
<div><br class="">
</div>
<div>
<blockquote type="cite" class="">
<div class="">
<div class=""> <br class="">
-- test/lib/jdk/test/lib/SecurityTools.java<br class="">
Could move method createCacerts() to
test/lib/jdk/test/lib/security/KeyStoreUtils.java?<br
class="">
This class contains a set of methods for creating
trust/key stores.<br class="">
And getCertFromFile() in
test/lib/jdk/test/lib/security/CertUtils.java<br
class="">
can load certificate from a file.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
Moved the method createCacerts() to the suggested location
under test/.</div>
<div>Tried to use getCertFromFile() and it did not work as it
read from test.src. So we need to read the cert directly.</div>
<div><br class="">
</div>
<div>
<blockquote type="cite" class="">
<div class="">
<div class=""> <br class="">
289 int pos = 0;<br class="">
...<br class="">
294 for (String crt : crts) {<br class="">
If not using for-each style, pos can be declared in the
for statement.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
<div>Ok, changed to not using for-each style.</div>
<br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""> <br class="">
--
test/jdk/sun/security/tools/keytool/fakecacerts/java.base/sun/security/util/FilePaths.java<br
class="">
This patch can be used by other tests, so could you
please move it to a<br class="">
common path? Like
test/jdk/sun/security/util/module_patch/java.base/sun/security/util/FilePaths.java<br
class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
Moved the patch to the suggested location under test/.</div>
<div><br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""> <br class="">
32 public static String cacerts() {<br class="">
33 return "mycacerts";<br class="">
34 }<br class="">
Could it be flexible for returning an alternative path?<br
class="">
For example, accept a system property, say test.cacerts,
for specifying<br class="">
an alternative path. mycacerts is the default value of
this property.<br class="">
</div>
</div>
</blockquote>
<div><br class="">
</div>
Done.</div>
<div><br class="">
</div>
<div>Thanks,</div>
<div>Hai-May</div>
<div><br class="">
</div>
<div><br class="">
<blockquote type="cite" class="">
<div class="">
<div class=""> <br class="">
Best regards,<br class="">
John Jiang<br class="">
<br class="">
<blockquote type="cite"
cite="mid:8786A3F4-A140-4FCC-A6AB-6EAB5788859D@oracle.com"
class="">
<div class=""><br class="">
</div>
<div class="">Thanks,</div>
<div class="">Hai-May</div>
<div class=""><br class="">
</div>
<div class="">
<div class=""><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"
moz-do-not-send="true"><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"
moz-do-not-send="true"><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"
moz-do-not-send="true"><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"
moz-do-not-send="true"><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"
moz-do-not-send="true">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"
moz-do-not-send="true">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/"
moz-do-not-send="true">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>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
</body>
</html>