RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

Hai-May Chao hai-may.chao at oracle.com
Fri Jun 12 23:09:51 UTC 2020 


> On Jun 12, 2020, at 5:37 AM, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> I re-read the CSR.
> 
> The precise meaning of the 2 options for -printcert is: "If the cert is a trusted certificate in either keystore or cacerts, we will not warn if it uses a weak signature algorithm". This is so subtle and I wonder it's worth describing it. Or we just say "This command does not check for the weakness of a certificate's signature algorithm if it's a trusted certificate in the user keystore (specified by -keystore) or the `cacerts` keystore (if -trustcacerts is specified)”.

Updated CSR with the second mentioned.

> 
> For -printcrl, the "issuer CA" is a little confusing. I would say "This commands attempts to verify the CRL using a certificate from the user keystore (specified by -keystore) or the `cacerts` keystore (if -trustcacerts is specified), and print out a warning if it cannot be verified”.

CRL issuer is usually the CA, and the implementation of -printcrl tries to get its issuer from the user or cacerts keystore. Updated CSR as suggested as if there may be confusing.

Thanks,
Hai-May

> 
> Thanks,
> Max
> 
> p.s. A new kind of check comes to my mind. Suppose you are printing a certificate whose own key is 2048-bit RSA, signature is using SHA256withRSA, but the signer key is only 512 bits (we can notice this from the size of the signature). Shall we print out a warning? 

Your thought?

> 
>> On Jun 11, 2020, at 11:21 AM, Weijun Wang <weijun.wang at oracle.com> wrote:
>> 
>> "to a self-signed certificate in the keystore". This is not correct. What we look for is a TrustedCertificateEntry.
>> 
>> "It emits warning when a certificate is not trusted and uses weak algorithms". Precisely, it's "uses a weak signature algorithm".
>> 
>> --Max
>> 
>> 
>>> On Jun 10, 2020, at 5:31 PM, Hai-May Chao <hai-may.chao at oracle.com> wrote:
>>> 
>>> 
>>> 
>>>> On Jun 9, 2020, at 5:58 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>> 
>>>> Good to see both -keystore and -trustcacerts mentioned. Some comments:
>>>> 
>>>> 1. I think there is no need to say "from -file cert_file". Or, do you mean the new function does not apply to those from -sslserver and -jarfile? If so, that might be a problem.
>>> 
>>> You’re right. Removed it.
>>> 
>>>> 
>>>> 2. While you said "attempts to construct a chain of trust", do you also want to describe what happens if it succeeds or fails?
>>> 
>>> Updated manpage.
>>> 
>>>> 
>>>> 3. It will be nice if you can include the exact diff of the man page files either inside the CSR itself or as a comment.
>>>> 
>>> 
>>> Included the diff of the manpage in the CSR.
>>> 
>>> Thanks,
>>> Hai-May
>>> 
>>> 
>>>> Thanks,
>>>> Max
>>>> 
>>>>> On Jun 9, 2020, at 10:51 PM, Hai-May Chao <hai-may.chao at oracle.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On Jun 7, 2020, at 6:08 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>>>> 
>>>>>> Looks fine to me.
>>>>>> 
>>>>>> For CSR, since there is already a "Note" there for these 2 options, you can add a few words about what -keystore and -trustcacerts can do.
>>>>> 
>>>>> Updated CSR as suggested.
>>>>> 
>>>>> Thanks,
>>>>> Hai-May
>>>>> 
>>>>> 
>>>>>> 
>>>>>> Thanks,
>>>>>> Max
>>>>>> 
>>>>>>> On Jun 8, 2020, at 4:01 AM, Hai-May Chao <hai-may.chao at oracle.com> wrote:
>>>>>>> 
>>>>>>> Updated webrev -
>>>>>>> 
>>>>>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Hai-May
>>>>>>> 
>>>>>>> 
>>>>>>>> On Jun 5, 2020, at 11:04 PM, Weijun Wang <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> 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
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>

More information about the security-dev mailing list