> On Jun 12, 2020, at 5:37 AM, Weijun Wang <weijun.w...@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.w...@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.c...@oracle.com> wrote:
>>>
>>>
>>>
>>>> On Jun 9, 2020, at 5:58 PM, Weijun Wang <weijun.w...@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.c...@oracle.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On Jun 7, 2020, at 6:08 PM, Weijun Wang <weijun.w...@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.c...@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.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>
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
