Ticket 1665 - Cert Revocation Reasons not being updated when on-hold

This patch fixes the following areas:
* In the CA, when revokeCert is called, make it possible to move from on_hold
to revoke.
* In the servlet that handles TPS revoke (DoRevokeTPS), make sure it allows
the on_hold cert to be put in the bucket to be revoked.
* there are a few minor fixes such as typos and one have to do with the
populate method in SubjectDNInput.java needs better handling of subject in
case it's null.
Note: This patch does not make attempt to allow agents to revoke certs that
are on_hold from agent interface.  The search filter needs to be modified to
allow that.

9 files changed, 99 insertions, 16 deletions

diff --git a/base/ca/src/com/netscape/ca/CAService.java b/base/ca/src/com/netscape/ca/CAService.java
index 2b5d5f732..485acc691 100644
--- a/base/ca/src/com/netscape/ca/CAService.java
+++ b/base/ca/src/com/netscape/ca/CAService.java
@@ -1001,9 +1001,11 @@ public class CAService implements ICAService, IService {
         Date revdate = crlentry.getRevocationDate();
         CRLExtensions crlentryexts = crlentry.getExtensions();
 
+        CMS.debug("CAService.revokeCert: revokeCert begins");
         CertRecord certRec = (CertRecord) mCA.getCertificateRepository().readCertificateRecord(serialno);
 
         if (certRec == null) {
+            CMS.debug("CAService.revokeCert: cert record not found");
             mCA.log(ILogger.LL_FAILURE, CMS.getLogMessage("CMSCORE_CA_CERT_NOT_FOUND", serialno.toString(16)));
             throw new ECAException(
                     CMS.getUserMessage("CMS_CA_CANT_FIND_CERT_SERIAL",
@@ -1013,14 +1015,24 @@ public class CAService implements ICAService, IService {
         // allow revoking certs that are on hold.
         String certStatus = certRec.getStatus();
 
-        if (certStatus.equals(ICertRecord.STATUS_REVOKED) ||
+        if ((certStatus.equals(ICertRecord.STATUS_REVOKED) &&
+            !certRec.isCertOnHold()) ||
                 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 {
-            mCA.getCertificateRepository().markAsRevoked(serialno,
+            CMS.debug("CAService.revokeCert: about to call markAsRevoked");
+            if (certRec.isCertOnHold()) {
+                mCA.getCertificateRepository().markAsRevoked(serialno,
+                    new RevocationInfo(revdate, crlentryexts), true /*isAlreadyOnHold*/);
+            } else {
+                mCA.getCertificateRepository().markAsRevoked(serialno,
                     new RevocationInfo(revdate, crlentryexts));
+            }
+            CMS.debug("CAService.revokeCert: cert revoked");
             mCA.log(ILogger.LL_INFO, CMS.getLogMessage("CMSCORE_CA_CERT_REVOKED",
                     serialno.toString(16)));
             // inform all CRLIssuingPoints about revoked certificate
@@ -1052,6 +1064,7 @@ public class CAService implements ICAService, IService {
                 }
             }
         } catch (EBaseException e) {
+            CMS.debug("CAService.revokeCert: " + e.toString());
             mCA.log(ILogger.LL_FAILURE,
                     CMS.getLogMessage("CMSCORE_CA_ERROR_REVOCATION", serialno.toString(), e.toString()));
             //e.printStackTrace();
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 23f4e07d4..3a0c9559e 100644
--- a/base/common/src/com/netscape/certsrv/dbs/certdb/ICertRecord.java
+++ b/base/common/src/com/netscape/certsrv/dbs/certdb/ICertRecord.java
@@ -176,4 +176,9 @@ public interface ICertRecord extends IDBObj {
      * @return revocation info
      */
     public IRevocationInfo getRevocationInfo();
+
+    /**
+     * is this cert on hold?
+     */
+    public boolean isCertOnHold();
 }
diff --git a/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java b/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java
index 066043b13..f113ea03e 100644
--- a/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java
+++ b/base/common/src/com/netscape/certsrv/dbs/certdb/ICertificateRepository.java
@@ -135,10 +135,13 @@ public interface ICertificateRepository extends IRepository {
      *
      * @param id serial number
      * @param info revocation information
+     * @param isAlreadyOnHold boolean to indicate if the cert was revoked onHold
      * @exception EBaseException failed to mark
      */
     public void markAsRevoked(BigInteger id, IRevocationInfo info)
             throws EBaseException;
+    public void markAsRevoked(BigInteger id, IRevocationInfo info, boolean isAlreadyOnHold)
+            throws EBaseException;
 
     /**
      * Updates certificate status.
diff --git a/base/server/cms/src/com/netscape/cms/profile/common/EnrollProfile.java b/base/server/cms/src/com/netscape/cms/profile/common/EnrollProfile.java
index 1f0e47069..7d7ce0661 100644
--- a/base/server/cms/src/com/netscape/cms/profile/common/EnrollProfile.java
+++ b/base/server/cms/src/com/netscape/cms/profile/common/EnrollProfile.java
@@ -1170,7 +1170,7 @@ public abstract class EnrollProfile extends BasicProfile
             // keeping "aoluid" to be backward compatible
             req.setExtData("aoluid", sn);
             req.setExtData("uid", sn);
-            CMS.debug("EnrollPrifile: fillNSNKEY(): uid=" + sn);
+            CMS.debug("EnrollProfile: fillNSNKEY(): uid=" + sn);
 
         } catch (Exception e) {
             CMS.debug("EnrollProfile: Unable to fill NSNKEY: " + e);
@@ -1195,7 +1195,7 @@ public abstract class EnrollProfile extends BasicProfile
             //                              X500Name("CN="+sn)));
             req.setExtData("tokencuid", tcuid);
 
-            CMS.debug("EnrollPrifile: fillNSNKEY(): tokencuid=" + tcuid);
+            CMS.debug("EnrollProfile: fillNSNKEY(): tokencuid=" + tcuid);
 
         } catch (Exception e) {
             CMS.debug("EnrollProfile: Unable to fill NSHKEY: " + e);
diff --git a/base/server/cms/src/com/netscape/cms/profile/input/SubjectDNInput.java b/base/server/cms/src/com/netscape/cms/profile/input/SubjectDNInput.java
index a12351f8a..0f1341728 100644
--- a/base/server/cms/src/com/netscape/cms/profile/input/SubjectDNInput.java
+++ b/base/server/cms/src/com/netscape/cms/profile/input/SubjectDNInput.java
@@ -94,7 +94,7 @@ public class SubjectDNInput extends EnrollInput implements IProfileInput {
         String subjectName = "";
 
         subjectName = ctx.get(VAL_SUBJECT);
-        if (subjectName.equals("")) {
+        if (subjectName == null || subjectName.equals("")) {
             throw new EProfileException(
                     CMS.getUserMessage(getLocale(request),
                             "CMS_PROFILE_SUBJECT_NAME_NOT_FOUND"));
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 a08f02be1..30bd2cde0 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
@@ -51,6 +51,7 @@ import com.netscape.certsrv.ca.ICertificateAuthority;
 import com.netscape.certsrv.common.ICMSRequest;
 import com.netscape.certsrv.dbs.certdb.ICertRecord;
 import com.netscape.certsrv.dbs.certdb.ICertificateRepository;
+import com.netscape.certsrv.dbs.certdb.IRevocationInfo;
 import com.netscape.certsrv.logging.AuditFormat;
 import com.netscape.certsrv.logging.ILogger;
 import com.netscape.certsrv.publish.IPublisherProcessor;
@@ -332,6 +333,7 @@ public class DoRevokeTPS extends CMSServlet {
         String auditRequestType = auditRequestType(reason);
         RequestStatus auditApprovalStatus = null;
         String auditReasonNum = String.valueOf(reason);
+        String method = "DoRevokeTPS.process";
 
         if (revokeAll != null) {
             CMS.debug("DoRevokeTPS.process revokeAll" + revokeAll);
@@ -405,9 +407,10 @@ public class DoRevokeTPS extends CMSServlet {
                     rarg.addStringValue("serialNumber",
                             xcert.getSerialNumber().toString(16));
 
-                    if (rec.getStatus().equals(ICertRecord.STATUS_REVOKED)) {
+                    if (rec.getStatus().equals(ICertRecord.STATUS_REVOKED)
+                          && !rec.isCertOnHold()) {
                         alreadyRevokedCertFound = true;
-                        CMS.debug("Certificate 0x" + xcert.getSerialNumber().toString(16) + " has been revoked.");
+                        CMS.debug(method + "Certificate 0x" + xcert.getSerialNumber().toString(16) + " has already been revoked.");
                     } else {
                         oldCertsV.addElement(xcert);
 
@@ -416,7 +419,7 @@ public class DoRevokeTPS extends CMSServlet {
                                         CMS.getCurrentDate(), entryExtn);
 
                         revCertImplsV.addElement(revCertImpl);
-                        CMS.debug("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 {
@@ -428,7 +431,7 @@ public class DoRevokeTPS extends CMSServlet {
                 // Situation where no certs were reoked here, but some certs
                 // requested happened to be already revoked. Don't return error.
                 if (alreadyRevokedCertFound == true && badCertsRequested == false) {
-                    CMS.debug("Only have previously revoked certs in the list.");
+                    CMS.debug(method + "Only have previously revoked certs in the list.");
                     // store a message in the signed audit log file
                     auditMessage = CMS.getLogMessage(
                             LOGGING_SIGNED_AUDIT_CERT_STATUS_CHANGE_REQUEST,
@@ -843,6 +846,7 @@ public class DoRevokeTPS extends CMSServlet {
         return;
     }
 
+
     /**
      * Signed Audit Log Requester ID
      *
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 422d506d5..a79f7a300 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertRecord.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertRecord.java
@@ -23,7 +23,11 @@ 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;
@@ -270,6 +274,30 @@ public class CertRecord implements IDBObj, ICertRecord {
         return mModifyTime;
     }
 
+    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");
+                return true;
+            }
+        }
+        CMS.debug(method + " returning false");
+        return false;
+    }
+
     /**
      * String representation
      */
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 f22beeb67..d0a604ec2 100644
--- a/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
+++ b/base/server/cmscore/src/com/netscape/cmscore/dbs/CertificateRepository.java
@@ -1110,24 +1110,52 @@ 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"
      */
     public void markAsRevoked(BigInteger id, IRevocationInfo info)
             throws EBaseException {
+        markAsRevoked(id, info, false);
+    }
+    public void markAsRevoked(BigInteger id, IRevocationInfo info, boolean isAlreadyOnHold)
+            throws EBaseException {
+        String method = "CertificateRepository.markAsRevoked:";
         ModificationSet mods = new ModificationSet();
-
-        mods.add(CertRecord.ATTR_REVO_INFO, Modification.MOD_ADD, info);
+        if (isAlreadyOnHold) {
+            mods.add(CertRecord.ATTR_REVO_INFO, Modification.MOD_REPLACE, info);
+        } else { 
+            mods.add(CertRecord.ATTR_REVO_INFO, Modification.MOD_ADD, info);
+        }
         SessionContext ctx = SessionContext.getContext();
         String uid = (String) ctx.get(SessionContext.USER_ID);
 
-        if (uid == null) {
-            mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_ADD,
+        /*
+         * When already revoked onHold, the fields already existing in record
+         * can only be replaced instead of added
+         */
+        if (isAlreadyOnHold) {
+            if (uid == null) {
+                mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_REPLACE,
                     "system");
+            } else {
+                mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_REPLACE,
+                    uid);
+            }
+            mods.add(CertRecord.ATTR_REVOKED_ON, Modification.MOD_REPLACE,
+                CMS.getCurrentDate());
         } else {
-            mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_ADD,
+            if (uid == null) {
+                mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_ADD,
+                    "system");
+            } else {
+                mods.add(CertRecord.ATTR_REVOKED_BY, Modification.MOD_ADD,
                     uid);
-        }
-        mods.add(CertRecord.ATTR_REVOKED_ON, Modification.MOD_ADD,
+            }
+            mods.add(CertRecord.ATTR_REVOKED_ON, Modification.MOD_ADD,
                 CMS.getCurrentDate());
+        }
+
         mods.add(CertRecord.ATTR_CERT_STATUS, Modification.MOD_REPLACE,
                 CertRecord.STATUS_REVOKED);
         modifyCertificateRecord(id, mods);
diff --git a/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java b/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java
index 2e46b037c..7997cc579 100644
--- a/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java
+++ b/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java
@@ -393,6 +393,7 @@ public class TPSTokendb {
 
             // get conn ID
             String config = "op.enroll." + cert.getType() + ".keyGen." + cert.getKeyType() + ".ca.conn";
+            CMS.debug(method + ": " + " getting config: " + config);
             String connID = configStore.getString(config);
 
             RevocationReason revokeReason = RevocationReason.UNSPECIFIED;
@@ -408,6 +409,7 @@ public class TPSTokendb {
             // get revoke reason
             config = "op.enroll." + cert.getType() + ".keyGen." + cert.getKeyType() +
                     ".recovery." + tokenReason + ".revokeCert.reason";
+            CMS.debug(method + ": " + " getting config: " + config);
             int reasonInt = configStore.getInteger(config, 0);
             revokeReason = RevocationReason.fromInt(reasonInt);