Bug2229930-crlCertValid-leaf-only #4554
Conversation
This patch addresses the issue where OCSPEngine:crlCertValid attempts to verify up the chain and failed because when using CRL to validate certs, the CAs up the chain are issued by different CAs. OCSPEngine:crlCertValid should be limited to leaf certs validation only. fixes https://bugzilla.redhat.com/show_bug.cgi?id=2229930
There was a problem hiding this comment.
I'm not too familiar with this area so I have some questions below.
Will this change the responses described in this page?
https://github.com/dogtagpki/pki/wiki/OCSP-status-responses
If it has been sufficiently tested feel free to merge.
| if (ext == null) | ||
| return false; | ||
| else { | ||
| Boolean bool = (Boolean) ext.get(BasicConstraintsExtension.IS_CA); |
There was a problem hiding this comment.
good question. Boolean by default is null, however, BasicConstraintsExtension has the following defined: // Private data members
private boolean ca = false;
| if (CertUtils.isCACert(cert)) | ||
| continue; |
There was a problem hiding this comment.
Will there ever be a case where the leaf cert is also a CA cert? If that's the case, the leaf cert will not be checked.
There was a problem hiding this comment.
Is it also guaranteed that all non-leaf certs are CA certs? If that's not guaranteed, the loop might pick the wrong cert to check.
There was a problem hiding this comment.
If we just want to check the leaf cert without touching the non-leaf certs at all, we probably can do something like this:
try {
CertificateChain certChain = new CertificateChain(certificates);
certChain.sort();
certificates = certChain.getChain();
X509Certificate leafCert = certificates[certificates.length - 1];
return !crlCertValid(crlStore, leafCert, null);
} catch (Exception e) {
throw new RuntimeException("Unable to check cert revocation: " + e.getMessage(), e);
}
There was a problem hiding this comment.
btw, I didn't realize there were comments before I clicked merge, so I'm replying after the fact. sorry about it. So, to answer your questions. The crl cert validation is only for validating peer certs. 1. in the pki setup, we never use a ca or self-signed cert for SSL connections, either a client cert, or a server cert. 2. yes all non-leaf certs are supposed to be ca certs, hence forming a chain. 3. Yes your proposal of using CertificateChain to sort is a very good idea. I actually copied a part of this code from JssSubsystem isCACert(). How about we create a separate ticket to combine the two isCACert() and make the improvement at the same time? I'll leave it as it for now, since Pritam has already tested it. Thanks!
No. This fix should not affect the outcome. My understanding is that this area is for using CRL to verify ocsp's peer certs, not for responding to ocsp requests. @fmarco76 please confirm. Thanks! |
|
Yes, I have not tested this code but it is not related to the OCSP output, only to the internal verification so the status table is still valid. |
This patch addresses the issue where OCSPEngine:crlCertValid attempts to verify up the chain and failed because when using CRL to validate certs, the CAs up the chain are issued by different CAs.
OCSPEngine:crlCertValid should be limited to leaf certs validation only.
fixes https://bugzilla.redhat.com/show_bug.cgi?id=2229930