From d3f50c6a77b164cc876192ab95639ad913f33deb Mon Sep 17 00:00:00 2001
From: Christina Fu <cfu@redhat.com>
Date: Thu, 20 Jul 2017 17:50:38 -0700
Subject: [PATCH] Ticket #1665 (code realignment) Certificate Revocation
Reasons not being updated in some cases This patch makes sure that when a
token is temporarily lost (certs on_hold), its certs are properly revoked
when moving to other revocation reasons when marked damaged or permanently
lost. In addition, on the CA side, this patch to some degree mimics the
original request for transitions from SUPERSEDED to KEY_COMPROMISED, although
in the current TPS that is prohibited. Also, the original requested code
skipped over informing CRLIssuingPoints, while in this patch, that is not
skipped as the revocation reason has changed it should be updated; Time
stamp in the cert record is also updated, which is different from the
original requested code. Development tests were conducted on currently
allowed TPS token state transitions only.
Change-Id: I675ce13892a7c48eba42870a87954398d7dc8168
(cherry picked from commit 36213c8b614775feadfebef54db034e1155d33c7)
(cherry picked from commit 34aabcc5fb21f35d96f76fc5b822959f26aacf42)
---
base/ca/src/com/netscape/ca/CAService.java | 58 +++++++++++++++++--
.../netscape/certsrv/dbs/certdb/ICertRecord.java | 9 +++
.../com/netscape/cms/servlet/cert/DoRevokeTPS.java | 51 +++++++++++++----
base/server/cmsbundle/src/LogMessages.properties | 2 +
base/server/cmsbundle/src/UserMessages.properties | 1 +
.../src/com/netscape/cmscore/dbs/CertRecord.java | 65 +++++++++++++++-------
.../cmscore/dbs/CertificateRepository.java | 32 ++++++-----
7 files changed, 166 insertions(+), 52 deletions(-)
diff --git a/base/ca/src/com/netscape/ca/CAService.java b/base/ca/src/com/netscape/ca/CAService.java
index c9eacfe..7cc6a31 100644
--- a/base/ca/src/com/netscape/ca/CAService.java
+++ b/base/ca/src/com/netscape/ca/CAService.java
@@ -980,8 +980,28 @@ public class CAService implements ICAService, IService {
BigInteger serialno = crlentry.getSerialNumber();
Date revdate = crlentry.getRevocationDate();
CRLExtensions crlentryexts = crlentry.getExtensions();
+ String msg = "";
CMS.debug("CAService.revokeCert: revokeCert begins");
+
+ // Get the revocation reason
+ Enumeration enum1 = crlentryexts.getElements();
+ RevocationReason revReason = null;
+ while (enum1.hasMoreElements()) {
+ Extension ext = (Extension) enum1.nextElement();
+ if (ext instanceof CRLReasonExtension) {
+ revReason = ((CRLReasonExtension) ext).getReason();
+ break;
+ }
+ }
+ if (revReason == null) {
+ CMS.debug("CAService.revokeCert: missing revocation reason");
+ mCA.log(ILogger.LL_FAILURE, CMS.getLogMessage("CMSCORE_CA_MISSING_REV_REASON=", serialno.toString(16)));
+ throw new ECAException(
+ CMS.getUserMessage("CMS_CA_MISSING_REV_REASON",
+ "0x" + serialno.toString(16)));
+ }
+
CertRecord certRec = (CertRecord) mCA.getCertificateRepository().readCertificateRecord(serialno);
if (certRec == null) {
@@ -995,24 +1015,52 @@ public class CAService implements ICAService, IService {
// allow revoking certs that are on hold.
String certStatus = certRec.getStatus();
- if ((certStatus.equals(ICertRecord.STATUS_REVOKED) &&
- !certRec.isCertOnHold()) ||
+ RevocationReason recRevReason = null;
+ if (certStatus.equals(ICertRecord.STATUS_REVOKED)) {
+ try {
+ recRevReason = certRec.getRevReason();
+ } catch (Exception e) {
+ throw new EBaseException(e);
+ }
+ if (recRevReason == null) {
+ msg = "existing revoked cert missing revocation reason";
+ CMS.debug("CAService.revokeCert: " + msg);
+ throw new EBaseException(msg);
+ }
+ }
+
+ // for cert already revoked, also check whether revocation reason is changed from SUPERSEDED to KEY_COMPROMISE
+ if (((certStatus.equals(ICertRecord.STATUS_REVOKED) &&
+ !certRec.isCertOnHold()) &&
+ ((recRevReason != RevocationReason.SUPERSEDED) ||
+ revReason != RevocationReason.KEY_COMPROMISE))
+ ||
certStatus.equals(ICertRecord.STATUS_REVOKED_EXPIRED)) {
CMS.debug("CAService.revokeCert: cert already revoked:" +
serialno.toString());
throw new ECAException(CMS.getUserMessage("CMS_CA_CERT_ALREADY_REVOKED",
"0x" + Long.toHexString(serialno.longValue())));
}
+
try {
+ // if cert has already revoked, update the revocation info only
CMS.debug("CAService.revokeCert: about to call markAsRevoked");
- if (certRec.isCertOnHold()) {
+ if (certStatus.equals(ICertRecord.STATUS_REVOKED) && certRec.isCertOnHold()) {
mCA.getCertificateRepository().markAsRevoked(serialno,
- new RevocationInfo(revdate, crlentryexts), true /*isAlreadyOnHold*/);
+ new RevocationInfo(revdate, crlentryexts),
+ true /*isAlreadyRevoked*/);
+ CMS.debug("CAService.revokeCert: on_hold cert marked revoked");
+ mCA.log(ILogger.LL_INFO,
+ CMS.getLogMessage("CMSCORE_CA_CERT_REVO_INFO_UPDATE",
+ recRevReason.toString(),
+ revReason.toString(),
+ serialno.toString(16)));
} else {
mCA.getCertificateRepository().markAsRevoked(serialno,
new RevocationInfo(revdate, crlentryexts));
}
- CMS.debug("CAService.revokeCert: cert revoked");
+
+ CMS.debug("CAService.revokeCert: cert now revoked");
mCA.log(ILogger.LL_INFO, CMS.getLogMessage("CMSCORE_CA_CERT_REVOKED",
serialno.toString(16)));
// inform all CRLIssuingPoints about revoked certificate
diff --git a/base/common/src/com/netscape/certsrv/dbs/certdb/ICertRecord.java b/base/common/src/com/netscape/certsrv/dbs/certdb/ICertRecord.java
index 3a0c955..65db57e 100644
--- a/base/common/src/com/netscape/certsrv/dbs/certdb/ICertRecord.java
+++ b/base/common/src/com/netscape/certsrv/dbs/certdb/ICertRecord.java
@@ -20,6 +20,9 @@ package com.netscape.certsrv.dbs.certdb;
import java.math.BigInteger;
import java.util.Date;
+import com.netscape.certsrv.base.EBaseException;
+import netscape.security.x509.RevocationReason;
+import netscape.security.x509.X509ExtensionException;
import netscape.security.x509.X509CertImpl;
import com.netscape.certsrv.base.MetaInfo;
@@ -181,4 +184,10 @@ public interface ICertRecord extends IDBObj {
* is this cert on hold?
*/
public boolean isCertOnHold();
+
+ /**
+ * returns the revocation reason
+ */
+ public RevocationReason getRevReason()
+ throws EBaseException, X509ExtensionException;
}
diff --git a/base/server/cms/src/com/netscape/cms/servlet/cert/DoRevokeTPS.java b/base/server/cms/src/com/netscape/cms/servlet/cert/DoRevokeTPS.java
index a9a6238..47062f2 100644
--- a/base/server/cms/src/com/netscape/cms/servlet/cert/DoRevokeTPS.java
+++ b/base/server/cms/src/com/netscape/cms/servlet/cert/DoRevokeTPS.java
@@ -1,4 +1,4 @@
-// --- 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.
@@ -330,8 +330,10 @@ public class DoRevokeTPS extends CMSServlet {
String auditRequestType = auditRequestType(reason);
RequestStatus auditApprovalStatus = null;
String auditReasonNum = String.valueOf(reason);
- String method = "DoRevokeTPS.process";
+ String method = "DoRevokeTPS.process:";
+ String msg = "";
+ CMS.debug(method + "begins");
if (revokeAll != null) {
CMS.debug("DoRevokeTPS.process revokeAll" + revokeAll);
@@ -357,6 +359,8 @@ public class DoRevokeTPS extends CMSServlet {
Vector<RevokedCertImpl> revCertImplsV = new Vector<RevokedCertImpl>();
// Construct a CRL reason code extension.
+
+ CMS.debug(method + "reason code = " + reason);
RevocationReason revReason = RevocationReason.fromInt(reason);
CRLReasonExtension crlReasonExtn = new CRLReasonExtension(revReason);
@@ -401,22 +405,47 @@ public class DoRevokeTPS extends CMSServlet {
}
if (xcert != null) {
+ RevocationReason recRevReason = null;
+ if (rec.getStatus().equals(ICertRecord.STATUS_REVOKED)) {
+ try {
+ recRevReason = rec.getRevReason();
+ } catch (Exception ex) {
+ CMS.debug(method + ex.toString());
+ throw new EBaseException(ex);
+ }
+ if (recRevReason == null) {
+ msg = "existing revoked cert missing revocation reason";
+ CMS.debug(method + msg);
+ throw new EBaseException(msg);
+ }
+ }
+
rarg.addStringValue("serialNumber",
xcert.getSerialNumber().toString(16));
- if (rec.getStatus().equals(ICertRecord.STATUS_REVOKED)
- && !rec.isCertOnHold()) {
- alreadyRevokedCertFound = true;
- CMS.debug(method + "Certificate 0x" + xcert.getSerialNumber().toString(16) + " has already been revoked.");
- } else {
+ boolean updateRevocation = true;
+ if ((rec.getStatus().equals(ICertRecord.STATUS_REVOKED) &&
+ revReason == RevocationReason.KEY_COMPROMISE)) {
+ updateRevocation = false;
+ if ((recRevReason == RevocationReason.SUPERSEDED) ||
+ (rec.isCertOnHold())) {
+ updateRevocation = true;
+ CMS.debug(method + "Certificate 0x" + xcert.getSerialNumber().toString(16)
+ + " has been revoked, but reason is changed");
+ } else {
+ alreadyRevokedCertFound = true;
+ CMS.debug("Certificate 0x" + xcert.getSerialNumber().toString(16) + " has been revoked.");
+ }
+ }
+ if (updateRevocation) {
oldCertsV.addElement(xcert);
- RevokedCertImpl revCertImpl =
- new RevokedCertImpl(xcert.getSerialNumber(),
- CMS.getCurrentDate(), entryExtn);
+ RevokedCertImpl revCertImpl = new RevokedCertImpl(xcert.getSerialNumber(),
+ CMS.getCurrentDate(), entryExtn);
revCertImplsV.addElement(revCertImpl);
- CMS.debug(method + "Certificate 0x" + xcert.getSerialNumber().toString(16) + " is going to be revoked.");
+ CMS.debug(method + "Certificate 0x" + xcert.getSerialNumber().toString(16)
+ + " is going to be revoked.");
count++;
}
} else {
diff --git a/base/server/cmsbundle/src/LogMessages.properties b/base/server/cmsbundle/src/LogMessages.properties
index 5e51440..ff432b6 100644
--- a/base/server/cmsbundle/src/LogMessages.properties
+++ b/base/server/cmsbundle/src/LogMessages.properties
@@ -119,7 +119,9 @@ CMSCORE_CA_STORE_SERIAL=CA stored signed certificate serial number 0x{0}
CMSCORE_CA_MARK_SERIAL=CA marked certificate serial number 0x{0} as renewed with serial number 0x{1}
CMSCORE_CA_NO_STORE_SERIAL=Could not store certificate serial number 0x{0}
CMSCORE_CA_CERT_NOT_FOUND=Cannot find certificate serial number 0x{0}
+CMSCORE_CA_MISSING_REV_REASON=Missing revocation reason for revocation request on serial number 0x{0}
CMSCORE_CA_CERT_REVOKED=Revoked certificate serial number 0x{0}
+CMSCORE_CA_CERT_REVO_INFO_UPDATE=Revocation reason changed from {0} to {1} Certificate serial number 0x{2}
CMSCORE_CA_ERROR_REVOCATION=Error revoking certificate {0}. Error {1}
CMSCORE_CA_CERT_ON_HOLD=Certificate {0} has to be on-hold.
CMSCORE_CA_CERT_UNREVOKED=Unrevoked certificate serial number 0x{0}
diff --git a/base/server/cmsbundle/src/UserMessages.properties b/base/server/cmsbundle/src/UserMessages.properties
index ed2a620..4d1b755 100644
--- a/base/server/cmsbundle/src/UserMessages.properties
+++ b/base/server/cmsbundle/src/UserMessages.properties
@@ -397,6 +397,7 @@ CMS_CA_CERT4CRL_FAILED=One or more revoked certificates could not be recorded by
CMS_CA_UNCERT4CRL_FAILED=One or more revoked certificates could not be removed by the CLA
CMS_CA_RENEW_FAILED=One or more certificates could not be renewed
CMS_CA_CANT_FIND_CERT_SERIAL=Cannot find certificate with serial number {0}
+CMS_CA_MISSING_REV_REASON=Missing revocation reason for revocatoin request on serial number {0}
CMS_CA_TOKEN_NOT_FOUND=Token {0} not found
CMS_CA_CERT_OBJECT_NOT_FOUND=Certificate object not found
CMS_CA_TOKEN_ERROR=Token Error
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertRecord.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertRecord.java
index a79f7a3..d4f3c03 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertRecord.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertRecord.java
@@ -23,12 +23,6 @@ import java.util.Date;
import java.util.Enumeration;
import java.util.Vector;
-import netscape.security.x509.CRLExtensions;
-import netscape.security.x509.CRLReasonExtension;
-import netscape.security.x509.RevocationReason;
-import netscape.security.x509.X509CertImpl;
-import netscape.security.x509.X509ExtensionException;
-
import com.netscape.certsrv.apps.CMS;
import com.netscape.certsrv.base.EBaseException;
import com.netscape.certsrv.base.MetaInfo;
@@ -37,6 +31,12 @@ import com.netscape.certsrv.dbs.IDBObj;
import com.netscape.certsrv.dbs.certdb.ICertRecord;
import com.netscape.certsrv.dbs.certdb.IRevocationInfo;
+import netscape.security.x509.CRLExtensions;
+import netscape.security.x509.CRLReasonExtension;
+import netscape.security.x509.RevocationReason;
+import netscape.security.x509.X509CertImpl;
+import netscape.security.x509.X509ExtensionException;
+
/**
* A class represents a serializable certificate record.
* <P>
@@ -274,27 +274,50 @@ public class CertRecord implements IDBObj, ICertRecord {
return mModifyTime;
}
+ /*
+ * getRevReason -
+ * @returns RevocationReason if cert is revoked; null if not
+ * it throws exceptions if anything failed
+ */
+ public RevocationReason getRevReason()
+ throws EBaseException, X509ExtensionException {
+ String method = "CertRecord.getRevReason:";
+ String msg = "";
+ //CMS.debug(method + " checking for cert serial: "
+ // + getSerialNumber().toString());
+ IRevocationInfo revInfo = getRevocationInfo();
+ if (revInfo == null) {
+ msg = "revInfo null for" + getSerialNumber().toString();
+ CMS.debug(method + msg);
+ return null;
+ }
+
+ CRLExtensions crlExts = revInfo.getCRLEntryExtensions();
+ if (crlExts == null)
+ throw new X509ExtensionException("crlExts null");
+
+ CRLReasonExtension reasonExt = null;
+ reasonExt = (CRLReasonExtension) crlExts.get(CRLReasonExtension.NAME);
+ if (reasonExt == null)
+ throw new EBaseException("reasonExt null");
+
+ return reasonExt.getReason();
+ }
+
public boolean isCertOnHold() {
String method = "CertRecord.isCertOnHold:";
CMS.debug(method + " checking for cert serial: "
- + getSerialNumber().toString());
- IRevocationInfo revInfo = getRevocationInfo();
- if (revInfo != null) {
- CRLExtensions crlExts = revInfo.getCRLEntryExtensions();
- if (crlExts == null) return false;
- CRLReasonExtension reasonExt = null;
- try {
- reasonExt = (CRLReasonExtension) crlExts.get(CRLReasonExtension.NAME);
- } catch (X509ExtensionException e) {
- CMS.debug(method + " returning false:" + e.toString());
- return false;
- }
- if (reasonExt.getReason() == RevocationReason.CERTIFICATE_HOLD) {
- CMS.debug(method + " returning true");
+ + getSerialNumber().toString());
+ try {
+ RevocationReason revReason = getRevReason();
+ if (revReason == RevocationReason.CERTIFICATE_HOLD) {
+ CMS.debug(method + "for " + getSerialNumber().toString() + " returning true");
return true;
}
+ } catch (Exception e) {
+ CMS.debug(method + e);
}
- CMS.debug(method + " returning false");
+ CMS.debug(method + "for " + getSerialNumber().toString() + " returning false");
return false;
}
diff --git a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
index 9a333fe..367917f 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
@@ -1110,19 +1110,21 @@ public class CertificateRepository extends Repository
/**
* Marks certificate as revoked.
- * isAlreadyOnHold - boolean to indicate that the cert was revoked onHold
- * When a cert was originally onHold, some of the ldap attributes
- * already exist, so "MOD_REPLACE" is needed instead of "MOD_ADD"
+ * isAlreadyRevoked - boolean to indicate that the cert was revoked
+ * ( possibly onHold )
+ * When a cert was originally revoked (possibly onHold),
+ * some of the ldap attributes already exist,
+ * so "MOD_REPLACE" is needed instead of "MOD_ADD"
*/
public void markAsRevoked(BigInteger id, IRevocationInfo info)
throws EBaseException {
markAsRevoked(id, info, false);
}
- public void markAsRevoked(BigInteger id, IRevocationInfo info, boolean isAlreadyOnHold)
+
+ public void markAsRevoked(BigInteger id, IRevocationInfo info, boolean isAlreadyRevoked)
throws EBaseException {
- String method = "CertificateRepository.markAsRevoked:";
ModificationSet mods = new ModificationSet();
- if (isAlreadyOnHold) {
+ if (isAlreadyRevoked) {
mods.add(CertRecord.ATTR_REVO_INFO, Modification.MOD_REPLACE, info);
} else {
mods.add(CertRecord.ATTR_REVO_INFO, Modification.MOD_ADD, info);
@@ -1134,30 +1136,30 @@ public class CertificateRepository extends Repository
* When already revoked onHold, the fields already existing in record
* can only be replaced instead of added
*/
- if (isAlreadyOnHold) {
+ if (isAlreadyRevoked) {
if (uid == null) {
mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_REPLACE,
- "system");
+ "system");
} else {
mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_REPLACE,
- uid);
+ uid);
}
mods.add(CertRecord.ATTR_REVOKED_ON, Modification.MOD_REPLACE,
- CMS.getCurrentDate());
+ CMS.getCurrentDate());
} else {
if (uid == null) {
mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_ADD,
- "system");
+ "system");
} else {
mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_ADD,
- uid);
+ uid);
}
mods.add(CertRecord.ATTR_REVOKED_ON, Modification.MOD_ADD,
- CMS.getCurrentDate());
+ CMS.getCurrentDate());
+ mods.add(CertRecord.ATTR_CERT_STATUS, Modification.MOD_REPLACE,
+ CertRecord.STATUS_REVOKED);
}
- mods.add(CertRecord.ATTR_CERT_STATUS, Modification.MOD_REPLACE,
- CertRecord.STATUS_REVOKED);
modifyCertificateRecord(id, mods);
}
--
1.8.3.1