Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

For unknown certificates OCSP should have unknown CertStatus (part 2) #4534

Merged
merged 2 commits into from
Aug 10, 2023
Merged

For unknown certificates OCSP should have unknown CertStatus (part 2) #4534

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
941e64a
For unknown certificates OCSP should have unknown CertStatus (part 2)
ckelleyRH Aug 8, 2023
d6adf7b
Internal OCSP CA identification using request hash
fmarco76 Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 34 additions & 24 deletions base/ca/src/main/java/com/netscape/ca/CertificateAuthority.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,12 @@
import com.netscape.certsrv.ca.CAEnabledException;
import com.netscape.certsrv.ca.CAMissingCertException;
import com.netscape.certsrv.ca.CAMissingKeyException;
import com.netscape.certsrv.ca.CANotFoundException;
import com.netscape.certsrv.ca.CANotLeafException;
import com.netscape.certsrv.ca.CATypeException;
import com.netscape.certsrv.ca.ECAException;
import com.netscape.certsrv.ca.IssuerUnavailableException;
import com.netscape.certsrv.cert.CertEnrollmentRequest;
import com.netscape.certsrv.dbs.EDBRecordNotFoundException;
import com.netscape.certsrv.dbs.certdb.CertId;
import com.netscape.certsrv.logging.ILogger;
import com.netscape.certsrv.logging.event.CRLSigningInfoEvent;
Expand Down Expand Up @@ -1550,34 +1550,35 @@ public OCSPResponse validate(OCSPRequest request)
* CertID in the request.
*
* 2. If this CA is *not* the issuer, look up the issuer
* by its DN in CAEngine. If not found, fail. If
* found, dispatch to its 'validate' method. Otherwise
* continue.
* by its DN in CAEngine. If found, dispatch to its 'validate'
* method. Otherwise continue.
*
* 3. If this CA is NOT the issuing CA, we locate the
* issuing CA and dispatch to its 'validate' method.
* Otherwise, we move forward to generate and sign the
* aggregate OCSP response.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmarco76 I suggest that you request @frasertweedale for a review here since it seems have touched the lightweight CA area.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, looking closely, although it added additional name/key hash checks, the code path should be the same for the lightweight CA case. I think it should be safe for v10.13 for now. We can hold off porting to other branches till we hear back from @frasertweedale

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the path is the same. The reason to look at the right CertificationAuthority is to get the correct DN and Public Key to verify. The improvement could be to perform the test for each certificate in the request.

CertificateAuthority ocspCA = this;
if (engine.getCAs().size() > 0 && tbsReq.getRequestCount() > 0) {
for (CertificateAuthority ocspCA: engine.getCAs()) {
Request req = tbsReq.getRequestAt(0);
BigInteger serialNo = req.getCertID().getSerialNumber();

CertificateRepository certificateRepository = engine.getCertificateRepository();
X509CertImpl cert = certificateRepository.getX509Certificate(serialNo);

X500Name certIssuerDN = (X500Name) cert.getIssuerDN();
ocspCA = engine.getCA(certIssuerDN);
}

if (ocspCA == null) {
logger.error("CertificateAuthority: Could not locate issuing CA");
throw new CANotFoundException("Could not locate issuing CA");
CertID cid = req.getCertID();
byte[] nameHash = null;
String digestName = cid.getDigestName();
if (digestName != null) {
try {
MessageDigest md = MessageDigest.getInstance(digestName);
nameHash = md.digest(ocspCA.getSubjectObj().getX500Name().getEncoded());
} catch (NoSuchAlgorithmException | IOException e) {
logger.info("CertificateAuthority: OCSP request hash algorithm " + digestName + " not recognised - ");
}
}
if(Arrays.equals(nameHash, cid.getIssuerNameHash().toByteArray())) {
if(ocspCA != this) {
return ((IOCSPService) ocspCA).validate(request);
}
break;
}
}

if (ocspCA != this)
return ((IOCSPService) ocspCA).validate(request);

logger.debug("CertificateAuthority: validating OCSP request");

Expand Down Expand Up @@ -1736,11 +1737,14 @@ private BasicOCSPResponse sign(ResponseData rd) throws EBaseException {
}
}

private SingleResponse processRequest(Request req) {
public SingleResponse processRequest(Request req) {

CAEngine engine = CAEngine.getInstance();
CertificateRepository certificateRepository = engine.getCertificateRepository();

X509CertImpl caCert = mSigningUnit.getCertImpl();
X509Key key = (X509Key) caCert.getPublicKey();

CertID cid = req.getCertID();
INTEGER serialNo = cid.getSerialNumber();
logger.debug("CertificateAuthority: processing request for cert 0x" + serialNo.toString(16));
Expand All @@ -1749,15 +1753,18 @@ private SingleResponse processRequest(Request req) {
GeneralizedTime thisUpdate = new GeneralizedTime(new Date());

byte[] nameHash = null;
byte[] keyHash = null;
String digestName = cid.getDigestName();
if (digestName != null) {
try {
MessageDigest md = MessageDigest.getInstance(digestName);
nameHash = md.digest(mName.getEncoded());
keyHash = md.digest(key.getKey());
} catch (NoSuchAlgorithmException | IOException e) {
}
}
if (!Arrays.equals(cid.getIssuerNameHash().toByteArray(), nameHash)) {
if (!Arrays.equals(cid.getIssuerNameHash().toByteArray(), nameHash) ||
!Arrays.equals(cid.getIssuerKeyHash().toByteArray(), keyHash)) {
// issuer of cert is not this CA (or we couldn't work
// out whether it is or not due to unknown hash alg);
// do not return status information for this cert
Expand Down Expand Up @@ -1836,9 +1843,12 @@ private SingleResponse processRequest(Request req) {
} else {
certStatus = new UnknownInfo();
}
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure there are no other type of exceptions? This catch all exception in general is a safety net in a pinch, if not the best practice.

} catch (EDBRecordNotFoundException e) {
// not found
certStatus = new UnknownInfo(); // not issued not all
certStatus = new GoodInfo(); // not issued not all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

despite what's being said in RFC 6960 wrt "good" response, since this is inside the CA, who knows for sure that it is not a cert issued by itself, I think "unknown" is a better fit for this response (v.s. standalone ocsp who has no means of finding out). In the "note" area of the rfc in that section, it says ' the "unknown" status indicates that the status could not be determined by this responder' so making it "unknown" would be fine I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last version of the spec suggest to provide reject status in this case (or good for backward compatibility) so the client does not have to look at other responders. I leave this decision to you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmarco76 as a matter of fact, I had an about face after looking at the rfc more closely. It should return "revoked" in this case. However, not having time (minutes away from close of sprint) to find a handy way to construct a "revoked" response at the time, I opted to assign unknown to it. I think that's what the lab is expecting from the emails I received. Either way it's still better than returning "good". This is a rare case anyway, so we can leave it to the lab and patch it if needed. Thanks!

} catch (EBaseException e) {
// internal error
certStatus = new UnknownInfo();
}

return new SingleResponse(
Expand Down