RFR 8215776: Keytool importkeystore may mix up certificate chain entries when DNs conflict

[Xuelei Fan]

On 1/21/2019 7:06 PM, Weijun Wang wrote:
>
>> On Jan 22, 2019, at 10:33 AM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>
>> On 1/21/2019 4:38 PM, Weijun Wang wrote:
>>> So what do you think of my original webrev? It only compares KID and subject/issuer, not caring about other extensions (like BC).
>> The original webrev looks right to me except that I'm  not sure if a new AuthorityKeyIdentifierExtension was needed.  Is it sufficient to use the octet string of the DER value?
> The struct of AuthorityKeyIdentifier and SubjectKeyIdentifier is a little different. By using the AuthorityKeyIdentifierExtension class, I don't need to extract the field myself.
>
>     AuthorityKeyIdentifier ::= SEQUENCE {
>        keyIdentifier             [0] KeyIdentifier           OPTIONAL,
>        authorityCertIssuer       [1] GeneralNames            OPTIONAL,
>        authorityCertSerialNumber [2] CertificateSerialNumber OPTIONAL  }
>
>    SubjectKeyIdentifier ::= KeyIdentifier
>
> and since getExtensionValue() returns the extension value encoded as an OCTET STRING, I will also need to extract the content inside.
>
> I also cannot call the X509CertImpl methods directly because it's only X509Certificate here.

I see.  Thanks!

>
>> It may need to selectors to use the X509CertSelector, for issuers w/o AKID. I will leave it to you for the final decision.
> I'll either need to go thru all certs twice or remember the fallback one like what I did in the current loop. It doesn't make much difference.

I meant to use the certs one time.

for (X509Certificate cert : allCerts) {

    if (cert has SKID) {

      // use the selector with SKID

   } else {

     // use the selector without SKID

  }

}

The benefit is limited as you have to construct the AKID.

I'm fine and have no more comments if you want to go with webrev.00.

Xuelei


> Thanks,
> Max
>
>> Xuelei
>>
>>> Thanks,
>>> Max
>>>
>>>
>>>> On Jan 22, 2019, at 1:39 AM, Xuelei Fan <xuelei.fan at oracle.com>
>>>>   wrote:
>>>>
>>>>
>>>>> but it seems it cannot deal with the case where a cert has the correct subject but no SKID extension. Or do you think this should never happen?
>>>>>
>>>> It could happen, especially for self-signed cert.  See also, the sun.security.provider.certpath.ForwardBuilder#PKIXCertComparator.
>>>> Xuelei
>>>> On 1/21/2019 2:05 AM, Weijun Wang wrote:
>>>>
>>>>> ;
>>>>>
>>>>> but it seems it cannot deal with the case where a cert has the correct subject but no SKID extension. Or do you think this should never happen?
>>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>>
>>>>>> On Jan 17, 2019, at 11:41 AM, Weijun Wang <weijun.wang at oracle.com>
>>>>>>   wrote:
>>>>>>
>>>>>> I'll take a look. I thought java.security.cert.X509CertSelector is used by CertPath validators and builders internally and never thought it can be called directly.
>>>>>>
>>>>>> Thanks,
>>>>>> Max
>>>>>>
>>>>>>
>>>>>>> On Jan 17, 2019, at 1:49 AM, Xuelei Fan <xuelei.fan at oracle.com>
>>>>>>>   wrote:
>>>>>>>
>>>>>>> Hi Max,
>>>>>>>
>>>>>>> I did not look into the detailed implementation of findIssuer() yet. Have you considered to use java.security.cert.X509CertSelector?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Xuelei
>>>>>>>
>>>>>>> On 1/9/2019 6:59 AM, Weijun Wang wrote:
>>>>>>>
>>>>>>>> Please take a review at
>>>>>>>>   
>>>>>>>> https://cr.openjdk.java.net/~weijun/8215776/webrev.00/
>>>>>>>>
>>>>>>>> PKCS12KeyStore now can find certificate issuers more precisely using SubjectKeyIdentifier and AuthorityKeyIdentifier. I thought about using CertPath builder or checking signatures but those changes are too much.
>>>>>>>> Thanks,
>>>>>>>> Max
>>>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190121/b5a344d6/attachment.htm>