RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options
sha.jiang at oracle.com
sha.jiang at oracle.com
Sat Jun 13 01:22:01 UTC 2020
Hi Hai-May,
On 2020/6/13 06:34, Hai-May Chao wrote:
> Hi John,
>
> Updated Webrev -
> https://cr.openjdk.java.net/~hchao/8244148/webrev.03/
Thanks for this updated webrev!
I have no more comment.
Best regards,
John Jiang
>
>> On Jun 11, 2020, at 1:45 AM, sha.jiang at oracle.com
>> <mailto:sha.jiang at 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/
>> -- 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.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/20200613/b38c1a43/attachment.htm>
More information about the security-dev
mailing list