[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Pki-devel] [PATCH] pki-cfu-0070-Ticket-1309-Recovering-of-a-revoked-cert-erroneously.patch



Hi John,
Thanks for the review.
Please see this updated patch (pki-cfu-0072).
thanks,
Christina

On 05/22/2015 07:03 PM, John Magne wrote:
Looks good, some comments:

1. public void tdbAddCertificatesForCUID(String cuid, ArrayList<TPSCertRecord> certs, ExternalRegAttrs erAttrs)
+            throws TPSException {
+        String method = "TPSTokendb.tdbAddCertificatesForCUID (with erAttrs): ";
+        CMS.debug(method + "begins");
+        if (cuid == null || certs== null || certs.isEmpty() || erAttrs == null) {
+            CMS.debug(method + "params cuid, certs and erAttrs cannot be null or empty");
+        }

Here we do a bunch of sanity checking but fail to throw the exception.


2. There are a few TPSException(s) thrown but no code is specified, taking whatever the default is.

3. The little inner enum class should probably be positioned at the bottom on the class.



----- Original Message -----
From: "Christina Fu" <cfu redhat com>
To: pki-devel redhat com
Sent: Friday, May 22, 2015 5:34:30 PM
Subject: [Pki-devel] [PATCH]	pki-cfu-0070-Ticket-1309-Recovering-of-a-revoked-cert-erroneously.patch

This patch addresses issues reported in:
https://fedorahosted.org/pki/ticket/1309Recovering of a revoked cert
erroneously reflects "active" in the token db cert entry

A new config param has been introduced to allow/disallow of recovering a
revoked certificate for externalReg:
externalReg.allowRecoverInvalidCert.enable=true
by default it is true

Please review.
thanks,
Christina

_______________________________________________
Pki-devel mailing list
Pki-devel redhat com
https://www.redhat.com/mailman/listinfo/pki-devel

>From ddae8adcbf21781afe870874d900926a46e793ba Mon Sep 17 00:00:00 2001
From: Christina Fu <cfu redhat com>
Date: Tue, 19 May 2015 17:34:51 -0700
Subject: [PATCH] Ticket 1309 Recovering of a revoked cert erroneously reflects
 "active" in the token db cert entry

---
 .../dogtagpki/server/connector/IRemoteRequest.java |  1 +
 base/tps/shared/conf/CS.cfg.in                     | 33 ++++++++++--------
 .../src/org/dogtagpki/server/tps/TPSTokendb.java   | 40 ++++++++++++++++++++++
 .../server/tps/cms/CARemoteRequestHandler.java     | 10 ++++++
 .../server/tps/cms/CARetrieveCertResponse.java     | 28 +++++++++++++++
 .../server/tps/main/ExternalRegAttrs.java          | 36 +++++++++++++++++++
 .../server/tps/main/ExternalRegCertToRecover.java  | 30 ++++++++++++++++
 .../server/tps/processor/TPSEnrollProcessor.java   | 34 ++++++++++++++++--
 .../server/tps/processor/TPSProcessor.java         | 21 ++++++++++++
 9 files changed, 215 insertions(+), 18 deletions(-)

diff --git a/base/server/cms/src/org/dogtagpki/server/connector/IRemoteRequest.java b/base/server/cms/src/org/dogtagpki/server/connector/IRemoteRequest.java
index 8025813e644feb0b2e5dc55b3fc9a3d781fec1a6..23312496888eb26a518755af5f9e1dce4ca4e8fc 100644
--- a/base/server/cms/src/org/dogtagpki/server/connector/IRemoteRequest.java
+++ b/base/server/cms/src/org/dogtagpki/server/connector/IRemoteRequest.java
@@ -96,6 +96,7 @@ public interface IRemoteRequest {
     public static final String CA_RESPONSE_Certificate_SubjectDN = "SubjectDN";
     public static final String CA_RESPONSE_Certificate_serial = "serialno";
     public static final String CA_RESPONSE_Certificate_chain_b64 = "certChainBase64";
+    public static final String CA_RESPONSE_Certificate_RevocationReason = "revocationReason";
 
     // KRA request params
     public static final String KRA_UserId = "userid";
diff --git a/base/tps/shared/conf/CS.cfg.in b/base/tps/shared/conf/CS.cfg.in
index 2f64b33e480db3a9f6009b3d0ddeb2140dd09b7c..fb2f9d4f96f21b50fdb58fa96af66af9a22093c6 100644
--- a/base/tps/shared/conf/CS.cfg.in
+++ b/base/tps/shared/conf/CS.cfg.in
@@ -133,22 +133,25 @@ externalReg._000=#########################################
 externalReg._001=#External Registration
 externalReg._002=#    Design: http://pki.fedoraproject.org/wiki/TPS_-_New_Recovery_Option:_External_Registration_DS
 externalReg._003=#
-externalReg._004=# enable - is user external registration DB enabled?
-externalReg._005=# authId - auth id of the user external registration DB
-externalReg._006=# delegation.enable - is delegation enabled?
-externalReg._007=#
-externalReg._008=# default.tokenType - when set, defaults to it if not specified in user
-externalReg._009=#         record
-externalReg._010=#
-externalReg._011=# format.loginRequest.enable - login required for format?
-externalReg._012=#         1. requires no login to format
-externalReg._013=#            or
-externalReg._014=#         2. user record does not contain tokenType
-externalReg._015=#
-externalReg._016=# mappingResolver - when exists, tells whcih mappingResolver to use
-externalReg._017=#         to map to the right keySet
-externalReg._018=#########################################
+externalReg._004=# allowRecoverInvalidCert.enable - defalut is allowed
+externalReg._005=#         to recover invalid (revoked, expired, not-yet-valid certs)
+externalReg._006=# enable - is user external registration DB enabled?
+externalReg._007=# authId - auth id of the user external registration DB
+externalReg._008=# delegation.enable - is delegation enabled?
+externalReg._009=#
+externalReg._010=# default.tokenType - when set, defaults to it if not specified in user
+externalReg._011=#         record
+externalReg._012=#
+externalReg._013=# format.loginRequest.enable - login required for format?
+externalReg._014=#         1. requires no login to format
+externalReg._015=#            or
+externalReg._016=#         2. user record does not contain tokenType
+externalReg._017=#
+externalReg._018=# mappingResolver - when exists, tells whcih mappingResolver to use
+externalReg._019=#         to map to the right keySet
+externalReg._020=#########################################
 externalReg.authId=ldap1
+externalReg.allowRecoverInvalidCert.enable=true
 externalReg.default.tokenType=externalRegAddToToken
 externalReg.delegation.enable=false
 externalReg.enable=false
diff --git a/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java b/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java
index c3fd70df93a717fa1d9478288d5baa3f75fe9239..c155e2bf79693b48a1e22a19b03b369ae4192706 100644
--- a/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java
+++ b/base/tps/src/org/dogtagpki/server/tps/TPSTokendb.java
@@ -33,7 +33,10 @@ import org.dogtagpki.server.tps.cms.CARevokeCertResponse;
 import org.dogtagpki.server.tps.dbs.ActivityDatabase;
 import org.dogtagpki.server.tps.dbs.TPSCertRecord;
 import org.dogtagpki.server.tps.dbs.TokenRecord;
+import org.dogtagpki.server.tps.main.ExternalRegAttrs;
+import org.dogtagpki.server.tps.main.ExternalRegCertToRecover;
 import org.dogtagpki.tps.main.TPSException;
+import org.dogtagpki.tps.msg.EndOpMsg.TPSStatus;
 
 import com.netscape.certsrv.apps.CMS;
 import com.netscape.certsrv.base.EBaseException;
@@ -223,11 +226,48 @@ public class TPSTokendb {
 
     /*
      * tdbAddCertificatesForCUID adds certificates issued for the token CUID
+     * - this instance pre-process the cert records to update the cert statuses
      * @param cuid the cuid of the token
      * @param certs an ArrayList of TPSCertRecord
+     * @param erAttrs the ExternalRegAttrs collection
      */
+    public void tdbAddCertificatesForCUID(String cuid, ArrayList<TPSCertRecord> certs, ExternalRegAttrs erAttrs)
+            throws TPSException {
+        String method = "TPSTokendb.tdbAddCertificatesForCUID (with erAttrs): ";
+        String auditMsg = "";
+        CMS.debug(method + "begins");
+        if (cuid == null || certs== null || certs.isEmpty() || erAttrs == null) {
+            auditMsg =  "params cuid, certs and erAttrs cannot be null or empty";
+            CMS.debug(method + auditMsg);
+            throw new TPSException(method + auditMsg, TPSStatus.STATUS_ERROR_MISCONFIGURATION);
+        }
+        CMS.debug("TPSTokendb.tdbAddCertificatesForCUID: number of certs to update:"+ certs.size());
+
+        // update cert status first
+        for (TPSCertRecord cert : certs) {
+            ExternalRegCertToRecover.CertStatus status = ExternalRegCertToRecover.CertStatus.UNINITIALIZED;
+            status = erAttrs.getCertStatus(cert.getSerialNumber());
+            if (status == ExternalRegCertToRecover.CertStatus.UNINITIALIZED) {
+                //cert not found in ExternalReg; don't reset status; don't report
+                continue;
+            }
+
+            //cert is one of the ExternalReg recovered certs, update the status
+            CMS.debug(method + "found and set status for:" + cert.getSerialNumber());
+            cert.setStatus(status.toString());
+
+        }
+
+        tdbAddCertificatesForCUID(cuid, certs);
+        CMS.debug(method + "ends");
+
+
+    }
+
     public void tdbAddCertificatesForCUID(String cuid, ArrayList<TPSCertRecord> certs)
             throws TPSException {
+        String method = "TPSTokendb.tdbAddCertificatesForCUID: ";
+        CMS.debug(method + "begins");
         boolean tokenExist = isTokenPresent(cuid);
         if (!tokenExist){
             CMS.debug("TPSTokendb.tdbAddCertificatesForCUID: token not found: "+ cuid);
diff --git a/base/tps/src/org/dogtagpki/server/tps/cms/CARemoteRequestHandler.java b/base/tps/src/org/dogtagpki/server/tps/cms/CARemoteRequestHandler.java
index 5e2bfc724bcb469173e6451bfd9839c051ff33b5..d70bf5d797ee34ebfd84f707149dc6212fa56cba 100644
--- a/base/tps/src/org/dogtagpki/server/tps/cms/CARemoteRequestHandler.java
+++ b/base/tps/src/org/dogtagpki/server/tps/cms/CARemoteRequestHandler.java
@@ -349,6 +349,16 @@ public class CARemoteRequestHandler extends RemoteRequestHandler
                 }
             }
 
+            value = xmlResponse.getValue(IRemoteRequest.CA_RESPONSE_Certificate_RevocationReason);
+            if (value == null) {
+                CMS.debug("CARemoteRequestHandler:: retrieveCertificate(): response missing name-value pair for: " +
+                        IRemoteRequest.CA_RESPONSE_Certificate_RevocationReason);
+            } else {
+                CMS.debug("CARemoteRequestHandler:: retrieveCertificate(): got IRemoteRequest.CA_RESPONSE_Certificate_RevocationReason = "
+                        + value);
+                response.put(IRemoteRequest.CA_RESPONSE_Certificate_RevocationReason, value);
+            }
+
             CMS.debug("CARemoteRequestHandler: retrieveCertificate(): ends.");
             return new CARetrieveCertResponse(response);
         } else {
diff --git a/base/tps/src/org/dogtagpki/server/tps/cms/CARetrieveCertResponse.java b/base/tps/src/org/dogtagpki/server/tps/cms/CARetrieveCertResponse.java
index a356907facc6f6d23f6d4172e2319dbae6567755..bf19d72b137a801752dc737cac218d629a734c4d 100644
--- a/base/tps/src/org/dogtagpki/server/tps/cms/CARetrieveCertResponse.java
+++ b/base/tps/src/org/dogtagpki/server/tps/cms/CARetrieveCertResponse.java
@@ -18,6 +18,8 @@
 
 package org.dogtagpki.server.tps.cms;
 
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
 import java.util.Hashtable;
 
 import netscape.security.x509.X509CertImpl;
@@ -42,4 +44,30 @@ public class CARetrieveCertResponse extends RemoteResponse
     public X509CertImpl getCert() {
         return (X509CertImpl) nameValTable.get(IRemoteRequest.CA_RESPONSE_Certificate_x509);
     }
+
+    public String getRevocationReason() {
+        return (String) nameValTable.get(IRemoteRequest.CA_RESPONSE_Certificate_RevocationReason);
+    }
+
+    public boolean isCertRevoked() {
+        String retRevocationReason = getRevocationReason();
+        if (retRevocationReason != null) {
+            return true;
+        }
+        // revocationReason not found means cert not revoked
+        return false;
+    }
+
+    /*
+     * This is checking the validity;  Revocation check should be done by calling isCertRevoked()
+     */
+    public boolean isCertValid() {
+        X509CertImpl cert = getCert();
+        try {
+            cert.checkValidity();
+            return true;
+        } catch (CertificateExpiredException | CertificateNotYetValidException e) {
+            return false;
+        }
+    }
 }
diff --git a/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegAttrs.java b/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegAttrs.java
index af8f5211716960f178ede891b909a1cd42f2ae2b..b3b745cecfd4cb445d7c44c34dfe0f7201114932 100644
--- a/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegAttrs.java
+++ b/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegAttrs.java
@@ -1,8 +1,11 @@
 package org.dogtagpki.server.tps.main;
 
+import java.math.BigInteger;
 import java.util.ArrayList;
 
 import org.dogtagpki.server.tps.engine.TPSEngine;
+import org.dogtagpki.tps.main.TPSException;
+import org.dogtagpki.tps.msg.EndOpMsg.TPSStatus;
 
 import com.netscape.certsrv.apps.CMS;
 import com.netscape.certsrv.base.EBaseException;
@@ -106,4 +109,37 @@ public class ExternalRegAttrs {
     public boolean getIsDelegation() {
         return isDelegation;
     }
+
+    /*
+     *
+     * @param serialString serial number in hex
+     */
+    public ExternalRegCertToRecover.CertStatus getCertStatus(String serialString) throws TPSException {
+        String method = "ExternalRegAttrs.getCertStatus:";
+        String auditMsg = "";
+        CMS.debug(method + "begins. getCertsToRecoverCount=" + getCertsToRecoverCount());
+        if (serialString == null) {
+            auditMsg = "parameter serialString cannnot be null";
+            CMS.debug(method + auditMsg);
+            throw new TPSException(method + auditMsg, TPSStatus.STATUS_ERROR_MISCONFIGURATION);
+        } else
+            CMS.debug(method + "searching for serialString =" + serialString);
+        if (serialString.startsWith("0x")) {
+            serialString = serialString.substring(2);
+        }
+        BigInteger serial = new BigInteger(serialString, 16);
+        CMS.debug(method + "searching for serial=" + serial);
+        for (ExternalRegCertToRecover cert: certsToRecover) {
+            CMS.debug(method + "cert.getSerial()=" + cert.getSerial());
+            if (serial.compareTo(cert.getSerial()) == 0) {
+                CMS.debug(method + " cert found... returning status: " + cert.getCertStatus().toString());
+                return cert.getCertStatus();
+            }
+        }
+        auditMsg = "cert not found in ExternalReg, status not reset";
+        CMS.debug(method + auditMsg);
+        // no match means cert was not one of the ExternalReg recovered certs; so don't reset
+        // use UNINITIALIZED to mean not found, as all certs in externalReg must have been set by now
+        return ExternalRegCertToRecover.CertStatus.UNINITIALIZED;
+    }
 }
diff --git a/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegCertToRecover.java b/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegCertToRecover.java
index dfc54d2212dadaf75ea7f9efb362e28aa9627443..a445012ebbd6d790893adbca452280e20aa6e5a5 100644
--- a/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegCertToRecover.java
+++ b/base/tps/src/org/dogtagpki/server/tps/main/ExternalRegCertToRecover.java
@@ -3,11 +3,13 @@ package org.dogtagpki.server.tps.main;
 import java.math.BigInteger;
 
 public class ExternalRegCertToRecover {
+
     BigInteger keyid;
     BigInteger serial;
     String caConn;
     String kraConn;
     boolean isRetainable;
+    CertStatus certStatus = CertStatus.UNINITIALIZED;
 
     public ExternalRegCertToRecover() {
         isRetainable = false;
@@ -52,4 +54,32 @@ public class ExternalRegCertToRecover {
     public boolean getIsRetainable() {
         return isRetainable;
     }
+
+    public void setCertStatus(CertStatus status) {
+        this.certStatus = status;
+    }
+
+    public CertStatus getCertStatus() {
+        return certStatus;
+    }
+
+    public enum CertStatus {
+        UNINITIALIZED("uninitialized"),
+        ACTIVE("active"),
+        REVOKED("revoked"),
+        EXPIRED("expired")
+        ;
+
+        private final String certStatusString;
+        private CertStatus(final String status) {
+            this.certStatusString = status;
+        }
+
+        @Override
+        public String toString() {
+            return certStatusString;
+        }
+    }
 }
+
+
diff --git a/base/tps/src/org/dogtagpki/server/tps/processor/TPSEnrollProcessor.java b/base/tps/src/org/dogtagpki/server/tps/processor/TPSEnrollProcessor.java
index 8c7535626dfb516c65b8760831864310a2938547..185430f195990ed2f01e8d334d16e00437a950ca 100644
--- a/base/tps/src/org/dogtagpki/server/tps/processor/TPSEnrollProcessor.java
+++ b/base/tps/src/org/dogtagpki/server/tps/processor/TPSEnrollProcessor.java
@@ -38,6 +38,7 @@ import org.dogtagpki.server.tps.engine.TPSEngine;
 import org.dogtagpki.server.tps.engine.TPSEngine.ENROLL_MODES;
 import org.dogtagpki.server.tps.main.ExternalRegAttrs;
 import org.dogtagpki.server.tps.main.ExternalRegCertToRecover;
+import org.dogtagpki.server.tps.main.ExternalRegCertToRecover.CertStatus;
 import org.dogtagpki.server.tps.main.ObjectSpec;
 import org.dogtagpki.server.tps.main.PKCS11Obj;
 import org.dogtagpki.server.tps.mapping.BaseMappingResolver;
@@ -122,6 +123,7 @@ public class TPSEnrollProcessor extends TPSProcessor {
         String cuid = appletInfo.getCUIDhexStringPlain();
         session.setTokenRecord(tokenRecord);
         String tokenType = null;
+        ExternalRegAttrs erAttrs = null;
 
         if (isExternalReg) {
             CMS.debug("In TPSEnrollProcessor.enroll isExternalReg: ON");
@@ -163,7 +165,6 @@ public class TPSEnrollProcessor extends TPSProcessor {
                         TPSStatus.STATUS_ERROR_LOGIN);
             }
 
-            ExternalRegAttrs erAttrs;
             try {
                 erAttrs = processExternalRegAttrs(authId);
             } catch (Exception ee) {
@@ -566,7 +567,10 @@ public class TPSEnrollProcessor extends TPSProcessor {
         }
         CMS.debug(method + " adding certs to token with tdbAddCertificatesForCUID...");
         ArrayList<TPSCertRecord> certRecords = certsInfo.toTPSCertRecords(tokenRecord.getId(), tokenRecord.getUserID());
-        tps.tdb.tdbAddCertificatesForCUID(tokenRecord.getId(), certRecords);
+        if (isExternalReg)
+            tps.tdb.tdbAddCertificatesForCUID(tokenRecord.getId(), certRecords, erAttrs);
+        else
+            tps.tdb.tdbAddCertificatesForCUID(tokenRecord.getId(), certRecords);
         CMS.debug(method + " tokendb updated with certs to the cuid so that it reflects what's on the token");
 
         auditMsg = "appletVersion=" + lastObjVer + "; tokenType =" + selectedTokenType + "; userid =" + userid;
@@ -965,9 +969,10 @@ public class TPSEnrollProcessor extends TPSProcessor {
             }
 
             String retCertB64 = certResp.getCertB64();
+            byte[] cert_bytes;
             if (retCertB64 != null) {
                 CMS.debug(method + "recovered:  retCertB64: " + retCertB64);
-                byte[] cert_bytes = Utils.base64decode(retCertB64);
+                cert_bytes = Utils.base64decode(retCertB64);
 
                 TPSBuffer cert_bytes_buf = new TPSBuffer(cert_bytes);
                 CMS.debug(method + "recovered: retCertB64: "
@@ -978,6 +983,27 @@ public class TPSEnrollProcessor extends TPSProcessor {
                 return TPSStatus.STATUS_ERROR_RECOVERY_FAILED;
             }
 
+            if (certResp.isCertRevoked()) {
+                CMS.debug(method + " cert revoked");
+                if (!allowRecoverInvalidCert()) {
+                    auditMsg = "revoked cert not allowed on token per policy;";
+                    CMS.debug(method + auditMsg);
+                    return TPSStatus.STATUS_ERROR_RECOVERY_FAILED;
+                }
+                erCert.setCertStatus(CertStatus.REVOKED);
+                CMS.debug(method + " erCert status =" + erCert.getCertStatus());
+            } else {
+                CMS.debug(method + " cert not revoked ");
+                erCert.setCertStatus(CertStatus.ACTIVE);
+
+                // check if expired or not yet valid
+                if (certResp.isCertValid()) {
+                    auditMsg = "cert expired or not yet valid";
+                    CMS.debug(auditMsg);
+                    erCert.setCertStatus(CertStatus.EXPIRED); // it could be not yet valid
+                }
+            }
+
             // recover keys
             KRARecoverKeyResponse keyResp = null;
             if (kraConn != null) {
@@ -1014,6 +1040,8 @@ public class TPSEnrollProcessor extends TPSProcessor {
                 }
             }
 
+
+
             CertEnrollInfo cEnrollInfo = new CertEnrollInfo();
 
             cEnrollInfo.setTokenToBeRecovered(tokenRecord);
diff --git a/base/tps/src/org/dogtagpki/server/tps/processor/TPSProcessor.java b/base/tps/src/org/dogtagpki/server/tps/processor/TPSProcessor.java
index 82c0734acbed141837e8db419dac01fa56cb0cb2..c917d0c1009e9dd94cdc7d097f8b60454db48bd2 100644
--- a/base/tps/src/org/dogtagpki/server/tps/processor/TPSProcessor.java
+++ b/base/tps/src/org/dogtagpki/server/tps/processor/TPSProcessor.java
@@ -1558,6 +1558,27 @@ public class TPSProcessor {
     }
 
     /*
+     * allow global policy  for externalReg to set in config whether invalid certs are allowed
+     * to be recovered on token
+     * Invalid certs are:
+     *  - revoked certs
+     *  - expired certs
+     *  - certs not yet valid
+     */
+    public boolean allowRecoverInvalidCert() throws TPSException {
+        String method = "TPSProcessor.allowRecoverInvalidCert:";
+        boolean ret = true;
+        IConfigStore configStore = CMS.getConfigStore();
+        String configName = "externalReg.allowRecoverInvalidCert.enable";
+        try {
+            ret = configStore.getBoolean(configName, true);
+        } catch (EBaseException e) {
+            throw new TPSException(method + e.getMessage() , TPSStatus.STATUS_ERROR_MISCONFIGURATION);
+        }
+        return ret;
+    }
+
+    /*
      * processExternalRegAttrs :
      * - retrieve from authToken relevant attributes for externalReg
      * - parse the multi-valued attributes
-- 
1.8.4.2


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]