Hi Hai-May,
On 2020/6/8 04:01, Hai-May Chao wrote:
Updated webrev -
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?
-- 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.
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.
-- 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.
289 int pos = 0;
...
294 for (String crt : crts) {
If not using for-each style, pos can be declared in the for statement.
--
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
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.
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/
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
