[security-dev 00969]: Re: code review request 6852744: PIT b61: PKI test suite fails because self signed certificates are being rejected

[Sean Mullan]

Some additional comments in the new tests describing what path building 
scenarios are being tested would be very useful.

A few comments below. Everything else looks good.

Xuelei Fan wrote:
> new webrev: http://cr.openjdk.java.net/~xuelei/6852744/webrev.01/
> 
> Sean Mullan wrote:
>> Hi Andrew,
>>
>> Here are some comments -
>>
>> ForwardBuilder:
>>
>> line 864:
>>
>> typo: s/abchor/anchor
>>
> yes, a typo.
>> In this block of code:
>>
>>  858                 if (principal != null && publicKey != null &&
>>  859                         
>> principal.equals(cert.getSubjectX500Principal())) {
>>  860                     if (publicKey.equals(cert.getPublicKey())) {
>>  861                         this.trustAnchor = anchor;
>>  862                         return true;
>>  863                     }
>>  864                     // else, it is a self-issued certificate of 
>> the abchor
>>  865                 }
>>
>> you never check if the trust anchor name is equal to the issuer of the 
>> cert before returning true. That seems to violate RFC 5280.
>>
> At line 859, when the cert's "subject" equals to the trust anchor 

Why not match it with the cert's issuer? That would then be compliant with 5280.

> principal, and the public key equals  at the same time(line 860), I 
> think the cert is the trust anchor itself.
> 
> Add a comment line in the update:
> -----------
>   858                  if (principal != null && publicKey != null &&
>   859                          
> principal.equals(cert.getSubjectX500Principal())) {
>   860                      if (publicKey.equals(cert.getPublicKey())) {
>   861                          // the cert itself is a trust anchor
>   862                          this.trustAnchor = anchor;
>   863                          return true;
>   864                      }
>   865                      // else, it is a self-issued certificate of 
> the anchor
>   866                  }
> ------------
>> lines 977-995
>>
>> RFC 5280 requires (MUST) that the KeyIdentifier be used in the 
>> AuthorityKeyIdentifier extension of the CRL, so I don't think we need 
>> to check the serial number/general names as those would be non-compliant.
> In a informational RFC, RFC4158, it says(section 3.5.12):
> --------------
> If the current certificate expresses the issuer name and serial number 
> in the AKID, certificates that match both these identifiers have highest 
> priority.  Certificates that match only the issuer name in the AKID have 
> medium priority.
> --------------
> 
> A weak implement would only compare KeyIdentifier only, and ignore 
> serial number/general names if KeyIdentifier presents. I would prefer a 
> strict one in case of a AKID indicates a serial number, but we ignore it 
> when building a path. I do believe it is possible that two certificate 
> would have the same subject KeyIdentifier, and would have to be 
> identified by serial number/general names. For example, for CA key 
> update, it is possible that a pari of certificates, OldWithNew and 
> OldWithOld, NewWithOld and NewWithNew, have the same subject 
> KeyIdentifier, because their public keys are identical.

Ok, this makes sense.

>> I would remove this block. In any case I think the intention is that 
>> there only be one form of identifier (key id OR issuer/serial) per 
>> AKID/SKID extension, so you should only have to check one of them.
>>
> Yes, I think most of the practical certificate only has one form. But as 
> a identification process, I think it would be better to be strict and 
> check both if present, otherwise, it will be easy to get criticized that 
> we don't check the other one.

This also seems ok to me.

Thanks,
Sean