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

OCSP responder to serve status check for itself using latest CRL #4504

Merged
merged 11 commits into from
Aug 8, 2023
Merged

OCSP responder to serve status check for itself using latest CRL #4504

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b9a9eb0
Modify init order for OCSP subsystem
fmarco76 Jul 19, 2023
57df77e
Add callback for CRL validation at application level
fmarco76 Jul 21, 2023
e6a1e0c
Make crl check for connection optional
fmarco76 Jul 28, 2023
7ad255e
Add crl check for OCSP acting as server
fmarco76 Jul 28, 2023
d54adc9
Move callback reference from CMS to CMSEngine
fmarco76 Jul 31, 2023
1615339
OCSP default CRL check and CA cert validation
fmarco76 Aug 3, 2023
2b31525
Rename checkSubsystemConnection to validateConnCertWithCRL
fmarco76 Aug 4, 2023
4366f72
Use AKI/SKI to match peer certificate with CA CRL
fmarco76 Aug 4, 2023
584cc11
Add comment for the option ocsp.store.ldapStore.validateConnCertWithCRL
fmarco76 Aug 4, 2023
3513512
Modify local variable names
fmarco76 Aug 4, 2023
47f268c
Update log message for revoked certificate
fmarco76 Aug 8, 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
95 changes: 95 additions & 0 deletions base/ocsp/src/main/java/com/netscape/cms/ocsp/CRLLdapValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// --- BEGIN COPYRIGHT BLOCK ---
// This program is free software; you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation; version 2 of the License.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License along
// with this program; if not, write to the Free Software Foundation, Inc.,
// 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
//
// (C) 2023 Red Hat, Inc.
// All rights reserved.
// --- END COPYRIGHT BLOCK ---
package com.netscape.cms.ocsp;

import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.Security;
import java.security.SignatureException;
import java.security.cert.CertificateException;
import java.security.cert.X509CRLEntry;
import java.util.Enumeration;

import org.mozilla.jss.crypto.X509Certificate;
import org.mozilla.jss.netscape.security.x509.X509CRLImpl;
import org.mozilla.jss.netscape.security.x509.X509CertImpl;
import org.mozilla.jss.ssl.SSLCertificateApprovalCallback;

import com.netscape.certsrv.base.EBaseException;
import com.netscape.certsrv.dbs.crldb.ICRLIssuingPointRecord;

public class CRLLdapValidator implements SSLCertificateApprovalCallback {

public static org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(CRLLdapValidator.class);

private LDAPStore crlStore;



public CRLLdapValidator(LDAPStore crlStore) {
super();
this.crlStore = crlStore;
}


@Override
public boolean approve(X509Certificate certificate, ValidityStatus currentStatus) {
logger.info("CRLLdapValidator: validate of peer's certificate for the connection " + certificate.getSubjectDN());
ICRLIssuingPointRecord pt = null;
try {
Enumeration<ICRLIssuingPointRecord> eCRL = crlStore.searchAllCRLIssuingPointRecord(-1);
while (eCRL.hasMoreElements() && pt == null) {
ICRLIssuingPointRecord tPt = eCRL.nextElement();
logger.debug("CRLLdapValidator: CRL check issuer " + tPt.getId());
if(tPt.getId().equals(certificate.getIssuerDN().toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain about issuerDN string comparison here. If you look at the processRequest(...) method in ocsp/src/main/java/com/netscape/cms/ocsp/LDAPStore.java, it uses the CA's public key hash as key to locate the right CRL, which is more assuring, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have implemented this using the CheckCertServlet as reference where the ICRLIssuingPointRecord is identified using the common name. Nevertheless, using the key hash is definitely a stronger mechanism. I will try to use the processRequest method or extract some code in a separate method.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ladycfu I did not find an easy way to get the issuer key hash from the certificate but to be more confident I have verified that the CA cert is the one signing the certificate. The test is done only if the DNs match to avoid the overhead of encoding/decoding certificates. I have performed the operation considering Mozilla-JSS provider is available. If this is not always the case some extra step are needed. Is this approach viable for the job?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @fmarco76 , I think in general, it's a better practice to avoid string comparison when it comes to DN names, due to encoding differences. Looking closely, the processRequest was for processing an ocsp request which contains the CertID, so it's not exactly what we have here. We have the peer cert, X509Certificate certificate, and we have the ca cert, X509CertImpl ca_certImpl = new X509CertImpl(tPt.getCACert()); and we can get the ca's public key PublicKey ca_pubkey = ca_certImpl.getPublicKey(); where PublicKey is java.security.PublicKey. Meanwhile, the peer cert X509Certificate certificate can have it's public key gotten with PublicKey peerCert_pubkey = certificate.getPublicKey(); So now you can compare. I'm not sure where you said you did signature verification. I could be wrong, but that sounds more expensive than the PublicKey comparison. Anyway, I think this patch is as good as it gets for the time we have, so you could leave this as an improvement for later if you want if you donn't have time for it now. Thanks.

Copy link
Member Author

@fmarco76 fmarco76 Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @fmarco76 , I think in general, it's a better practice to avoid string comparison when it comes to DN names, due to encoding differences.

IIRC String in java have the same encoding independently of how they are stored in the certificate. The encoding is converted when they are read/write. The format could be different but this is related to the used classes so it is known (although it could change moving to different versions of Java).

Looking closely, the processRequest was for processing an ocsp request which contains the CertID, so it's not exactly what we have here. We have the peer cert, X509Certificate certificate, and we have the ca cert, X509CertImpl ca_certImpl = new X509CertImpl(tPt.getCACert()); and we can get the ca's public key PublicKey ca_pubkey = ca_certImpl.getPublicKey(); where PublicKey is java.security.PublicKey. Meanwhile, the peer cert X509Certificate certificate can have it's public key gotten with PublicKey peerCert_pubkey = certificate.getPublicKey(); So now you can compare. I'm not sure where you said you did signature verification. I could be wrong, but that sounds more expensive than the PublicKey comparison.

The problem here is that you get the PublicKey of the CA and the PublicKey of the peer certificate. They are different so there is no way to compare. The OCSP request contain the hash of the issuer PublicKey so it is possible to get the hash of the CA PublicKey and compare.
From the peer certificate I found no way to get the issuer PublicKey to compare with the CA.
The approach I implemented is in line 61-63 of CRLLdapValidator.

It is definitely more expensive of the PublicKey comparison and this is the reason I do only if the DN match. However, considering a low number of CA for an OCSP we could skip the DN match and do always the signature check. Is this assumption correct?

Anyway, I think this patch is as good as it gets for the time we have, so you could leave this as an improvement for later if you want if you don't have time for it now. Thanks.

I can work on this until the end of the sprint so we still have several days before merge.

try {
X509CertImpl caCert = new X509CertImpl(tPt.getCACert());
X509CertImpl certToVerify = new X509CertImpl(certificate.getEncoded());
certToVerify.verify(caCert.getPublicKey(), Security.getProvider("Mozilla-JSS"));
pt = tPt;
} catch (CertificateException | InvalidKeyException | NoSuchAlgorithmException
| SignatureException e) {
logger.error("CRLLdapValidator: issuer certificate cannot verify the certificate signature." );
}
}
}
} catch (EBaseException e) {
logger.error("CRLLdapValidator: problem find CRL issuing point. " + e.getMessage(), e);
return false;
}
if (pt == null) {
logger.error("CRLLdapValidator: CRL issuing point not found for " + certificate.getIssuerDN());
return false;
}
try {
X509CRLImpl crl = new X509CRLImpl(pt.getCRL());
X509CRLEntry crlentry = crl.getRevokedCertificate(certificate.getSerialNumber());

if (crlentry == null) {
if (crlStore.isNotFoundGood()) {
return true;
}
}
} catch (Exception e) {
logger.error("CRLLdapValidator: crl check error. " + e.getMessage(), e);
}
logger.info("CRLLdapValidator: peer certificate not valid");
return false;
}

}
13 changes: 12 additions & 1 deletion base/ocsp/src/main/java/com/netscape/cms/ocsp/LDAPStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
// --- END COPYRIGHT BLOCK ---
package com.netscape.cms.ocsp;

import java.lang.Integer;
import java.math.BigInteger;
import java.security.MessageDigest;
import java.security.cert.X509CRL;
Expand Down Expand Up @@ -82,6 +81,7 @@ public class LDAPStore implements IDefStore, IExtendedPluginInfo {
private static final String DEF_CA_CERT_ATTR = "cACertificate;binary";
private static final String PROP_HOST = "host";
private static final String PROP_PORT = "port";
private static final String PROP_VALIDATE_CONNECTION_WITH_CRL = "validateConnCertWithCRL";
Copy link
Member Author

Choose a reason for hiding this comment

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

The option ocsp.store.ldapStore.validateConnCertWithCRL enables the revocation verification of peer certificates using the CRL stored in the LDAP shared with the CA.

When it is set to true (default value), the peer certificate of all the outcome connections from the OCSP subsystem are verified with the CRL.

If the option auths.revocationChecking.enabled is also set to true the peer certificate ot all the income connections to the OCSP subsystem are verified with the CRL.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about put the info right here in the code where PROP_VALIDATE_CONNECTION_WITH_CRL = "validateConnCertWithCRL" is assigned? Comments in code are longer lasting and provide info others later. I will also be using it for the doc changes.


private final static String PROP_NOT_FOUND_GOOD = "notFoundAsGood";
private final static String PROP_INCLUDE_NEXT_UPDATE =
Expand All @@ -94,6 +94,8 @@ public class LDAPStore implements IDefStore, IExtendedPluginInfo {
private String mCACertAttr = null;
protected Hashtable<String, Long> mReqCounts = new Hashtable<>();
private Hashtable<X509CertImpl, X509CRLImpl> mCRLs = new Hashtable<>();
private boolean mValidateConnection = true;


/**
* Constructs the default store.
Expand Down Expand Up @@ -137,6 +139,7 @@ public void init(IConfigStore config, DBSubsystem dbSubsystem) throws EBaseExcep
DEF_CA_CERT_ATTR);
mByName = mConfig.getBoolean(PROP_BY_NAME, true);

mValidateConnection = mConfig.getBoolean(PROP_VALIDATE_CONNECTION_WITH_CRL, true);
}

/**
Expand Down Expand Up @@ -238,6 +241,9 @@ public void startup() throws EBaseException {

updater.start();
}
if(mValidateConnection) {
CMS.getCMSEngine().setApprovalCallback(new CRLLdapValidator(this));
}
}

@Override
Expand Down Expand Up @@ -490,6 +496,11 @@ public void setConfigParameters(NameValuePairs pairs)
mConfig.put(key, pairs.get(key));
}
}

public boolean isCRLCheckAvailable() {
return mValidateConnection;
}

}

class CRLUpdater extends Thread {
Expand Down
134 changes: 134 additions & 0 deletions base/ocsp/src/main/java/org/dogtagpki/server/ocsp/OCSPEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,27 @@

package org.dogtagpki.server.ocsp;

import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.Security;
import java.security.SignatureException;
import java.security.cert.CertificateException;
import java.security.cert.X509CRLEntry;
import java.security.cert.X509Certificate;
import java.util.Enumeration;

import javax.security.auth.x500.X500Principal;
import javax.servlet.annotation.WebListener;

import org.mozilla.jss.netscape.security.x509.X509CRLImpl;
import org.mozilla.jss.netscape.security.x509.X509CertImpl;
import org.mozilla.jss.ssl.SSLCertificateApprovalCallback.ValidityStatus;

import com.netscape.certsrv.base.EBaseException;
import com.netscape.certsrv.base.IConfigStore;
import com.netscape.certsrv.base.ISubsystem;
import com.netscape.certsrv.dbs.crldb.ICRLIssuingPointRecord;
import com.netscape.cms.ocsp.LDAPStore;
import com.netscape.cmscore.apps.CMS;
import com.netscape.cmscore.apps.CMSEngine;
import com.netscape.cmscore.apps.EngineConfig;
Expand Down Expand Up @@ -63,5 +80,122 @@ public void initSubsystem(ISubsystem subsystem, IConfigStore subsystemConfig) th
}

super.initSubsystem(subsystem, subsystemConfig);
if (subsystem instanceof OCSPAuthority) {
subsystem.startup();
}
}


protected void startupSubsystems() throws Exception {

for (ISubsystem subsystem : subsystems.values()) {
logger.info("CMSEngine: Starting " + subsystem.getId() + " subsystem");
if (!(subsystem instanceof OCSPAuthority))
subsystem.startup();
}

// global admin servlet. (anywhere else more fit for this ?)
}
@Override
protected void initSequence() throws Exception {

initDebug();
init();
initPasswordStore();
initSubsystemListeners();
initSecurityProvider();
initPluginRegistry();
initLogSubsystem();
initDatabase();
initJssSubsystem();
initDBSubsystem();
initUGSubsystem();
initOIDLoaderSubsystem();
initX500NameSubsystem();
// skip TP subsystem;
// problem in needing dbsubsystem in constructor. and it's not used.
initRequestSubsystem();


startupSubsystems();

initAuthSubsystem();
initAuthzSubsystem();
initJobsScheduler();

configureAutoShutdown();
configureServerCertNickname();
configureExcludedLdapAttrs();

initSecurityDomain();
}

@Override
public boolean isRevoked(X509Certificate[] certificates) {
LDAPStore crlStore = null;
for (ISubsystem subsystem : subsystems.values()) {
if (subsystem instanceof OCSPAuthority) {
OCSPAuthority ocsp = (OCSPAuthority) subsystem;
if (ocsp.getDefaultStore() instanceof LDAPStore) {
crlStore = (LDAPStore) ocsp.getDefaultStore();
}
break;
}
}

if (crlStore == null || !crlStore.isCRLCheckAvailable()) {
return super.isRevoked(certificates);
}

for (X509Certificate cert: certificates) {
if(!crlCertValid(crlStore, cert, null)) {
return true;
}
}
return false;

}


private boolean crlCertValid(LDAPStore crlStore, X509Certificate certificate, ValidityStatus currentStatus) {
logger.info("OCSPEngine: validate of peer's certificate for the connection " + certificate.getSubjectX500Principal());
ICRLIssuingPointRecord pt = null;
try {
Enumeration<ICRLIssuingPointRecord> eCRL = crlStore.searchAllCRLIssuingPointRecord(-1);
while (eCRL.hasMoreElements() && pt == null) {
ICRLIssuingPointRecord tPt = eCRL.nextElement();
logger.debug("OCSPEngine: CRL check issuer " + tPt.getId());
if(certificate.getIssuerX500Principal().equals(new X500Principal(tPt.getId()))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above regarding using CA's public key hash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same approach as above.

try {
X509CertImpl caCert = new X509CertImpl(tPt.getCACert());
certificate.verify(caCert.getPublicKey(), Security.getProvider("Mozilla-JSS"));
pt = tPt;
} catch (CertificateException | InvalidKeyException | NoSuchAlgorithmException
| SignatureException e) {
logger.error("OCSPEngine: issuer certificate cannot verify the certificate signature." );
}
}
}
} catch (EBaseException e) {
logger.error("OCSPEngine: problem find CRL issuing point for " + certificate.getIssuerX500Principal().toString());
return false;
}
if (pt == null) {
logger.error("OCSPEngine: CRL issuing point not found for " + certificate.getIssuerX500Principal().toString());
return false;
}
try {
X509CRLImpl crl = new X509CRLImpl(pt.getCRL());
X509CRLEntry crlentry = crl.getRevokedCertificate(certificate.getSerialNumber());

if (crlentry == null && crlStore.isNotFoundGood()) {
return true;
}
} catch (Exception e) {
logger.error("OCSPEngine: crl check error. " + e.getMessage());
}
logger.info("OCSPEngine: peer certificate not valid");
return false;
}

}
51 changes: 33 additions & 18 deletions base/server/src/main/java/com/netscape/cmscore/apps/CMSEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import org.mozilla.jss.crypto.SignatureAlgorithm;
import org.mozilla.jss.netscape.security.util.Cert;
import org.mozilla.jss.netscape.security.x509.X509CertImpl;
import org.mozilla.jss.ssl.SSLCertificateApprovalCallback;

import com.netscape.certsrv.authentication.ISharedToken;
import com.netscape.certsrv.base.EBaseException;
Expand Down Expand Up @@ -151,6 +152,8 @@ public class CMSEngine implements ServletContextListener {
protected LogSubsystem logSubsystem = LogSubsystem.getInstance();
protected JssSubsystem jssSubsystem = JssSubsystem.getInstance();
protected DBSubsystem dbSubsystem = new DBSubsystem();
protected SSLCertificateApprovalCallback approvalCallback;



protected RequestRepository requestRepository;
Expand Down Expand Up @@ -301,6 +304,14 @@ public void registerPendingListener(String name, IRequestListener listener) {
pendingNotifier.registerListener(name, listener);
}

public SSLCertificateApprovalCallback getApprovalCallback() {
return approvalCallback;
}

public void setApprovalCallback(SSLCertificateApprovalCallback approvalCallback) {
this.approvalCallback = approvalCallback;
}

public void loadConfig(String path) throws Exception {
ConfigStorage storage = new FileConfigStore(path);
config = createConfig(storage);
Expand Down Expand Up @@ -1102,6 +1113,28 @@ public void start() throws Exception {

CMS.setCMSEngine(this);

initSequence();

// Register realm for this subsystem
ProxyRealm.registerRealm(id, new PKIRealm());

ready = true;
isStarted = true;

mStartupTime = System.currentTimeMillis();

logger.info(name + " engine started");
// Register TomcatJSS socket listener
TomcatJSS tomcatJss = TomcatJSS.getInstance();
if(serverSocketListener == null) {
serverSocketListener = new PKIServerSocketListener();
}
tomcatJss.addSocketListener(serverSocketListener);

notifySubsystemStarted();
}

protected void initSequence() throws Exception {
initDebug();
initPasswordStore();
initSubsystemListeners();
Expand Down Expand Up @@ -1131,24 +1164,6 @@ public void start() throws Exception {
configureExcludedLdapAttrs();

initSecurityDomain();

// Register realm for this subsystem
ProxyRealm.registerRealm(id, new PKIRealm());

ready = true;
isStarted = true;

mStartupTime = System.currentTimeMillis();

logger.info(name + " engine started");
// Register TomcatJSS socket listener
TomcatJSS tomcatJss = TomcatJSS.getInstance();
if(serverSocketListener == null) {
serverSocketListener = new PKIServerSocketListener();
}
tomcatJss.addSocketListener(serverSocketListener);

notifySubsystemStarted();
}

public boolean isInRunningState() {
Expand Down
Loading