I still need to review RevocationChecker but a few more comments ...
* OCSPResponse
- the responderKeyId field is never set. I believe line 299 should be:
responderKeyId = seq.getData().getOctetString();
- lines 372-385. I think it would be better to not do any matching yet,
and store all of the certs in a list. The matching should be done later
in verify if we still need to find a trusted responder cert.
- lines 443-4, it seems expensive to iterate over all the possible
responders, verifying the signature each time to determine who has
signed the response if it is signed by the issuing CA. What about the
following logic instead:
1. check the responderName or responderId against the list of trusted
responder certs that are passed in. I think it would be better to store
the responderCerts in a map instead of a list, using the keyId or name
as key (you could create a new ResponderKey class that compares either
the name or the keyId, whichever is set).
2. If step 1 is successful (finds a matching cert), this cert is
already trusted so you can proceed to verifying the signature. There is
no need to look at the certs in the OCSPResponse.
3. If step 1 fails, then you can look at the certs in the
OCSPResponse, find the cert that matches the keyId or name, verify the
signature, then check if it is authorized.
- line 611, it seems the caller could set this if verify returns true.
Then you could make this method static.
--Sean
On 10/22/2013 12:06 PM, Sean Mullan wrote:
I am still reviewing, but here are some comments so far:
* X509CertImpl
I would prefer if getSubjectKeyIdentifier returned a KeyIdentifier so
that it is consistent with the getAuthKeyId method. Also, in
OCSPResponse, you can then just call KeyIdentifier.equals instead of
comparing the bytes yourself with Arrays.equals.
* RevocationChecker
RevocationChecker can be re-used for subsequent revocation checks by
calling the init method. So, you need to clear the contents of the
responderCerts list each time init is called. You can add this after
line 323 in the init method
responderCerts.clear();
--Sean
On 10/21/2013 05:36 PM, Vincent Ryan wrote:
Please review this fix to support key-rollover certs
(same name, different keys):
Bug: https://bugs.openjdk.java.net/browse/JDK-8012636
Webrev: http://cr.openjdk.java.net/~vinnie/8012636/webrev.00/
This issue arises when an OCSP responder replaces its public key
but retains its subject name. The OCSP client must be able to
validate responses signed by both keys.
Thanks.
