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
