RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options
sha.jiang at oracle.com
sha.jiang at oracle.com
Thu Jun 11 08:45:28 UTC 2020
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.wang at oracle.com
>> <mailto:weijun.wang at 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.chao at oracle.com
>>> <mailto:hai-may.chao at 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.wang at oracle.com> wrote:
>>>>
>>>>
>>>>
>>>>> On Jun 4, 2020, at 7:29 PM, Hai-May Chao <hai-may.chao at oracle.com>
>>>>> wrote:
>>>>>
>>>>> Hi Max,
>>>>>
>>>>>> On Jun 3, 2020, at 12:59 AM, Weijun Wang <weijun.wang at 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.chao at 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
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200611/caee7670/attachment.htm>
More information about the security-dev
mailing list