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
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.
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.
Thanks,
Andrew
I'm still looking over the tests but these are some comments so far.
--Sean
Xuelei Fan wrote:
Hi,
bug description:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6852744
webrev: http://cr.openjdk.java.net/~xuelei/6852744/webrev/
Evaluation of the bug:
1. There is a loop of forward builder for self-issused intermediate
certificates.
The ForwardBuilder looks for the next certificate based on
IssuerDN/SubjectDN. However, a self-issued certificate has the same
IssuerDN and SubjectDN, the looking will loop on the self-issued
certificate untill the loop detected.
2. Circular dependences
In the PIT tests, the valid of the intermediate CA certificate
(oldCA) depends on the CRL; the valid of CRL depends on its issuer,
the self-issued intermediate CA certificate (newWithOldCA); the valid
of newWithOldCA depends on its issuer, the oldCA, here comes a dead
loop.
Thanks,
Xuelei
