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.

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

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

Merged
merged 11 commits into from
Aug 8, 2023
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// --- 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.cert.X509CRLEntry;
import java.util.Enumeration;

import org.mozilla.jss.crypto.X509Certificate;
import org.mozilla.jss.netscape.security.x509.X509CRLImpl;
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().toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

The .toString() can be removed since it's implicitly called by string concatenation.

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

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.

pt = tPt;
}
}
} catch (EBaseException e) {
logger.error("CRLLdapValidator: problem find CRL issuing point for " + 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.

To simplify troubleshooting I'd suggest displaying the exception message and optionally attach the exception (to show the stack trace):

logger.error("...<message>...: " + e.getMessage(), e);

return false;
}
if (pt == null) {
logger.error("CRLLdapValidator: CRL issuing point not found for " + certificate.getIssuerDN().toString());
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());
}
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
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_CHECK_SUBSYSTEM_CONNECTION = "checkSubsystemConnection";

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 mCheckConnection = false;


/**
* 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);

mCheckConnection = mConfig.getBoolean(PROP_CHECK_SUBSYSTEM_CONNECTION, false);
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 debating whether this should be true for default. What's your thoughts on this @fmarco76 @edewata ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, simply put ocsp.store.ldapStore.checkSubsystemConnection=true as default in OCSP's CS.cfg. Also, based on my latest experiment, how about adding the following to the OCSP's CS.cfg: auths.revocationChecking.enabled=true . This is so that role access at the WebUI and pkiconsole can do that by default. CA already has that by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

This code is only used by the OCSP subsystem. Is there any case where we want this functionality disabled? If it's always going to be enabled maybe we don't need this param.

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 agree with @edewata, I do not see a reason why this should be disabled so I think we could make permanent this change and not triggered by an option. However, we could just makes it the default behaviour just in case the scenario where disabling the check arise. @ladycfu I am converting to true.

I have some concerns with auths.revocationChecking.enabled=true. For the OCSP subsystem this work only if the CA sends CRLs through an LDAP and not with direct publishing. Should we stopped, or at least log an error, if it is true but LDAP publishing is not configured?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason why I propose having a "switch" to go back to the previous behavior was because we want to be sure that there is no gotcha's in the new mechanism. For now, I think this switch serves its purpose, as a back up . I think it's fine to have it enabled by default. Now, wrt auths.revocationChecking.enabled=true, this "Method C" precludes the direct CA->OCSP CRL publishing because the ocsp's ssl port is not able to receive CRL from the CA due to chick-n-egg issue. The CA depends on the ocsp to start up, and the ocsp depends on the CA to start up, if replying on ca->ocsp direct publishing. The CA is to have enableOCSP=true and its internaldb's server-cert is to bear an AIA pointing to the ocsp. @fmarco76 I think you have a good suggestion there on checking whether it's ldapStore that's being set for crl update if ocsp.store.ldapStore.checkSubsystemConnection=true. btw, what is the concern you wrt auths.revocationChecking.enabled=true ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fmarco76 btw, why do you call this flag "checkSubsystemConnection"? To me it seems something like "ocsp.store.ldapStore.validateConnCertWithCRL" would be more telling. Actually, could you add an explanation in comment where this flag is defined what exactly it does? We need to add something in the doc. Thanks.

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 think you have a good suggestion there on checking whether it's ldapStore that's being set for crl update if ocsp.store.ldapStore.checkSubsystemConnection=true.

This setting is done on ldapStore of the OCSP so we do not need to verify if it is used because if it is not then this setting does not apply.

what is the concern you wrt auths.revocationChecking.enabled=true ?

In the OCSP, if this is true but the ldapStore is not used the revocation cannot be verified. I have not tested if this generate errors. Therefore I was wondering if the user should be notified that revocation will not work even it is enabled.

why do you call this flag "checkSubsystemConnection"? To me it seems something like "ocsp.store.ldapStore.validateConnCertWithCRL" would be more telling. Actually, could you add an explanation in comment where this flag is defined what exactly it does? We need to add something in the doc.

Yes, sound better, I will change the name. Where exactly should I write an explanation? A comment in github where the code is provided or a comment inside the code?

Copy link
Contributor

Choose a reason for hiding this comment

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

wrt auths.revocationChecking.enabled=true, I'm still not sure why it would require ldapStore. I will take a closer look later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the method OCSPEngine.isRevoked() is invoked when a new connection to OCSP is created. This method uses the LDAPStore to verify the certificate. If LDAPStore is not used, or the CRL verification is disabled I have not verified what happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I just dug slightly deeper and saw that you had overwritten the isRevoked in the OCSPEngine.

}

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

updater.start();
}
if(mCheckConnection) {
CMS.setApprovalCallbask(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 mCheckConnection;
}

}

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

package org.dogtagpki.server.ocsp;

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.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 +74,115 @@ 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 false;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this loop will return false as soon as it finds the first valid cert. Should we check all certs in the chain instead?

return true;

}


private boolean crlCertValid(LDAPStore crlStore, X509Certificate certificate, ValidityStatus currentStatus) {
logger.info("OCSPEngine: validate of peer's certificate for the connection " + certificate.getSubjectX500Principal().toString());
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.

pt = tPt;
}
}
} 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;
}

}
11 changes: 11 additions & 0 deletions base/server/src/main/java/com/netscape/cmscore/apps/CMS.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Locale;
import java.util.ResourceBundle;

import org.mozilla.jss.ssl.SSLCertificateApprovalCallback;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -53,6 +54,8 @@ public final class CMS {

private static CMSEngine engine;

private static SSLCertificateApprovalCallback approvalCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

In the newer branches we've removed the dependency on global variables in CMS class. I'd suggest moving this variable into CMSEngine class.


public static CMSEngine getCMSEngine() {
return engine;
}
Expand All @@ -61,6 +64,14 @@ public static void setCMSEngine(CMSEngine engine) {
CMS.engine = engine;
}

public static SSLCertificateApprovalCallback getApprovalCallback() {
return approvalCallback;
}

public static void setApprovalCallbask(SSLCertificateApprovalCallback approvalCallback) {
CMS.approvalCallback = approvalCallback;
}

/**
* Return the product name from /usr/share/pki/CS_SERVER_VERSION
* which is provided by the server theme package.
Expand Down
40 changes: 22 additions & 18 deletions base/server/src/main/java/com/netscape/cmscore/apps/CMSEngine.java
Original file line number Diff line number Diff line change
Expand Up @@ -1102,6 +1102,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 +1153,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
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public SSLSocket makeSSLSocket(String host, int port) throws UnknownHostExceptio
SSLSocket s;

if (mClientAuthCertNickname == null) {
s = new SSLSocket(host, port);
s = new SSLSocket(host, port, null, 0, CMS.getApprovalCallback(), null);

} else {
// Let's create a selection callback in the case the client auth
Expand All @@ -161,7 +161,7 @@ public SSLSocket makeSSLSocket(String host, int port) throws UnknownHostExceptio

Socket js = new Socket(InetAddress.getByName(host), port);
s = new SSLSocket(js, host,
null,
CMS.getApprovalCallback(),
new SSLClientCertificateSelectionCB(mClientAuthCertNickname));
}

Expand Down
Loading