>From 69fb1f3d91e054ecbb8761f0900b23d3e24de7e8 Mon Sep 17 00:00:00 2001
From: Christina Fu <cfu@dhcp-16-189.sjc.redhat.com>
Date: Wed, 5 Oct 2016 16:09:24 -0700
Subject: [PATCH] Ticket #2496 Cert/Key recovery is successful when the cert
 serial number and key id on the ldap user mismatches Problem: There are two
 ways to recover the keys with a. by cert b. by keyId When recovering by cert,
 KRA checks if cert and key matches before returning; However, in case of
 recovering by keyId, KRA has no way of checking.  TPS also has no way of
 checking because the recovered private keys are warpped. This patch adds a
 control parameter externalReg.recovery.byKeyID to determine if TPS should
 recover keys by keyIDs. By default, it is false, so certs are used to search
 for key record and recover.

Code summary for externalReg key recovery:
 config default: externalReg.recover.byKeyID=false
Recover either by keyID or by cert
 When recovering by keyid:
   - keyid in record indicates actual recovery;
   - missing of which means retention;
 When recovering by cert:
   - keyid field needs to be present
     but the value is not relevant and will be ignored (a "0" would be fine)
   - missing of keyid still means retention;

(In hindsight, recovery by keyid is probably more accident-prone and should be discouraged)
---
 base/tps/shared/conf/CS.cfg                        | 18 +++++-
 .../server/tps/cms/KRARemoteRequestHandler.java    | 46 +++++++++------
 .../server/tps/processor/TPSEnrollProcessor.java   | 68 +++++++++++++++++-----
 3 files changed, 98 insertions(+), 34 deletions(-)

diff --git a/base/tps/shared/conf/CS.cfg b/base/tps/shared/conf/CS.cfg
index a8499a2b31eec45329a238ccb8c595fcdcf9b7ce..35ca7ef1446d63a885fad6402c842c1e02029a41 100644
--- a/base/tps/shared/conf/CS.cfg
+++ b/base/tps/shared/conf/CS.cfg
@@ -150,7 +150,22 @@ 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._020=#
+externalReg._021=# recover.byKeyID - (by default, recover by cert)
+externalReg._022=#    Recover either by keyID or by cert
+externalReg._023=#     When recovering by keyid: externalReg.recover.byKeyID=false
+externalReg._024=#       - keyid in record indicates actual recovery;
+externalReg._025=#         e.g. (certstoadd: 36,ca1,5,kra1)
+externalReg._026=#       - missing of which means retention;
+externalReg._027=#         e.g. (certstoadd: 36,ca1)
+externalReg._028=#     When recovering by cert: externalReg.recover.byKeyID=true
+externalReg._029=#       - keyid field needs to be present
+externalReg._030=#         but the value is not relevant and will be ignored
+externalReg._031=#         (a "0" would be fine)
+externalReg._032=#         e.g. (certstoadd: 36,ca1,0,kra1)
+externalReg._033=#       - missing of keyid still means retention;
+externalReg._034=#         e.g. (certstoadd: 36,ca1)
+externalReg._035=#########################################
 externalReg.authId=ldap1
 externalReg.allowRecoverInvalidCert.enable=true
 externalReg.default.tokenType=externalRegAddToToken
@@ -158,6 +173,7 @@ externalReg.delegation.enable=false
 externalReg.enable=false
 externalReg.format.loginRequest.enable=true
 externalReg.mappingResolver=keySetMappingResolver
+externalReg.recover.byKeyID=false
 failover.pod.enable=false
 general.applet_ext=ijc
 general.pwlength.min=16
diff --git a/base/tps/src/org/dogtagpki/server/tps/cms/KRARemoteRequestHandler.java b/base/tps/src/org/dogtagpki/server/tps/cms/KRARemoteRequestHandler.java
index 59b5208f625a79c944115aea5806314948f5a442..80439ca145e5e38b84357e21145221ee1713dd6e 100644
--- a/base/tps/src/org/dogtagpki/server/tps/cms/KRARemoteRequestHandler.java
+++ b/base/tps/src/org/dogtagpki/server/tps/cms/KRARemoteRequestHandler.java
@@ -23,6 +23,7 @@ import java.util.Hashtable;
 
 import org.dogtagpki.server.connector.IRemoteRequest;
 import org.dogtagpki.server.tps.TPSSubsystem;
+import org.dogtagpki.tps.main.Util;
 
 import com.netscape.certsrv.apps.CMS;
 import com.netscape.certsrv.base.EBaseException;
@@ -262,26 +263,33 @@ public class KRARemoteRequestHandler extends RemoteRequestHandler
         CMS.debug("KRARemoteRequestHandler: recoverKey(): sending request to KRA");
 
         String sendMsg = null;
-        if (b64cert != null) { // recover by cert
-            sendMsg = IRemoteRequest.TOKEN_CUID + "=" +
-                    cuid +
-                    "&" + IRemoteRequest.KRA_UserId + "=" +
-                    userid +
-                    "&" + IRemoteRequest.KRA_RECOVERY_CERT + "=" +
-                    b64cert +
-                    "&" + IRemoteRequest.KRA_Trans_DesKey + "=" +
-                    sDesKey;
-        } else if (keyid != BigInteger.valueOf(0)){ // recover by keyid ... keyid != BigInteger.valueOf(0)
-            CMS.debug("KRARemoteRequestHandler: recoverKey(): keyid = " + keyid);
-            sendMsg = IRemoteRequest.TOKEN_CUID + "=" +
-                    cuid +
-                    "&" + IRemoteRequest.KRA_UserId + "=" +
-                    userid +
-                    "&" + IRemoteRequest.KRA_RECOVERY_KEYID + "=" +
-                    keyid.toString() +
-                    "&" + IRemoteRequest.KRA_Trans_DesKey + "=" +
-                    sDesKey;
+        try {
+            if (b64cert != null) { // recover by cert
+                // CMS.debug("KRARemoteRequestHandler: recoverKey(): uriEncoded cert= " + Util.uriEncode(b64cert));
+                sendMsg = IRemoteRequest.TOKEN_CUID + "=" +
+                        cuid +
+                        "&" + IRemoteRequest.KRA_UserId + "=" +
+                        userid +
+                        "&" + IRemoteRequest.KRA_RECOVERY_CERT + "=" +
+                        Util.uriEncode(b64cert) +
+                        "&" + IRemoteRequest.KRA_Trans_DesKey + "=" +
+                        Util.uriEncode(sDesKey);
+            } else if (keyid != BigInteger.valueOf(0)) { // recover by keyid ... keyid != BigInteger.valueOf(0)
+                CMS.debug("KRARemoteRequestHandler: recoverKey(): keyid = " + keyid);
+                sendMsg = IRemoteRequest.TOKEN_CUID + "=" +
+                        cuid +
+                        "&" + IRemoteRequest.KRA_UserId + "=" +
+                        userid +
+                        "&" + IRemoteRequest.KRA_RECOVERY_KEYID + "=" +
+                        keyid.toString() +
+                        "&" + IRemoteRequest.KRA_Trans_DesKey + "=" +
+                        Util.uriEncode(sDesKey);
+            }
+        } catch (Exception e) {
+            CMS.debug("KRARemoteRequestHandler: recoverKey(): uriEncode failed: " + e);
+            throw new EBaseException("KRARemoteRequestHandler: recoverKey(): uriEncode failed: " + e);
         }
+
         //CMS.debug("KRARemoteRequestHandler: recoverKey(): sendMsg =" + sendMsg);
         HttpResponse resp =
                 conn.send("TokenKeyRecovery",
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 9d42546956f9b7e57a98d167c364705d7cf66420..aa23bf0775d21518a96136e5134469dedb6642d1 100644
--- a/base/tps/src/org/dogtagpki/server/tps/processor/TPSEnrollProcessor.java
+++ b/base/tps/src/org/dogtagpki/server/tps/processor/TPSEnrollProcessor.java
@@ -14,11 +14,6 @@ import java.util.Map;
 import java.util.Random;
 import java.util.zip.DataFormatException;
 
-import netscape.security.provider.RSAPublicKey;
-//import org.mozilla.jss.pkcs11.PK11ECPublicKey;
-import netscape.security.util.BigInt;
-import netscape.security.x509.X509CertImpl;
-
 import org.dogtagpki.server.tps.TPSSession;
 import org.dogtagpki.server.tps.TPSSubsystem;
 import org.dogtagpki.server.tps.TPSTokenPolicy;
@@ -58,8 +53,6 @@ import org.mozilla.jss.pkcs11.PK11PubKey;
 import org.mozilla.jss.pkcs11.PK11RSAPublicKey;
 import org.mozilla.jss.pkix.primitive.SubjectPublicKeyInfo;
 
-import sun.security.pkcs11.wrapper.PKCS11Constants;
-
 import com.netscape.certsrv.apps.CMS;
 import com.netscape.certsrv.base.EBaseException;
 import com.netscape.certsrv.base.EPropertyNotFound;
@@ -67,6 +60,12 @@ import com.netscape.certsrv.base.IConfigStore;
 import com.netscape.certsrv.tps.token.TokenStatus;
 import com.netscape.cmsutil.util.Utils;
 
+import netscape.security.provider.RSAPublicKey;
+//import org.mozilla.jss.pkcs11.PK11ECPublicKey;
+import netscape.security.util.BigInt;
+import netscape.security.x509.X509CertImpl;
+import sun.security.pkcs11.wrapper.PKCS11Constants;
+
 public class TPSEnrollProcessor extends TPSProcessor {
 
     public TPSEnrollProcessor(TPSSession session) {
@@ -1263,18 +1262,38 @@ public class TPSEnrollProcessor extends TPSProcessor {
                 }
             }
 
+            // default: externalReg.recover.byKeyID=false
+            String b64cert = null;
+            if (getExternalRegRecoverByKeyID() == false) {
+                b64cert = certResp.getCertB64();
+                //CMS.debug("TPSEnrollProcessor.processRecovery: cert blob to recover key with: " + b64cert);
+            }
+
+            /*
+             * Recover either by keyID or by cert
+             * When recovering by keyid:
+             *   - keyid in record indicates actual recovery;
+             *   - missing of which means retention;
+             * When recovering by cert:
+             *   - keyid field needs to be present
+             *     but the value is not relevant (a "0" would be fine)
+             *   - missing of keyid still means retention;
+             */
             if (keyid == null) {
-                logMsg = " no keyid; skip key recovery; continue";
+                logMsg = " no keyid; retention; skip key recovery; continue";
                 CMS.debug(method + logMsg);
                 continue;
-            } else if (keyid.compareTo(BigInteger.valueOf(0)) == 0) {
-                logMsg = " keyid is 0; invalid; skip key recovery; continue";
+            } else {
+                logMsg = " keyid in user record: " + keyid.toString();
                 CMS.debug(method + logMsg);
-                continue;
+                if ((getExternalRegRecoverByKeyID() == false) &&
+                        keyid.compareTo(BigInteger.valueOf(0)) != 0) {
+                    logMsg = " Recovering by cert; keyid is irrelevant from user record";
+                    CMS.debug(method + logMsg);
+                }
             }
+
             // recover keys
-            logMsg = " recovering for keyid: " + keyid.toString();
-            CMS.debug(method + logMsg);
             KRARecoverKeyResponse keyResp = null;
             if (kraConn != null) {
                 logMsg = "kraConn not null:" + kraConn;
@@ -1290,7 +1309,7 @@ public class TPSEnrollProcessor extends TPSProcessor {
                 }
 
                 keyResp = kraRH.recoverKey(cuid, userid, Util.specialURLEncode(channel.getDRMWrappedDesKey()),
-                        null, keyid);
+                        getExternalRegRecoverByKeyID() ? null : b64cert, keyid);
                 if (keyResp == null) {
                     auditInfo = "recovering key not found";
                     auditRecovery(userid, appletInfo, "failure",
@@ -1602,6 +1621,27 @@ public class TPSEnrollProcessor extends TPSProcessor {
         return enabled;
     }
 
+    /**
+     * getExternalRegRecoverByKeyID returns whether externalReg
+     * recovery is recovering by keyID or not; default is by cert
+     */
+    private boolean getExternalRegRecoverByKeyID() {
+        String method = "TPSEnrollProcessor.getExternalRegRecoverByKeyID";
+        IConfigStore configStore = CMS.getConfigStore();
+        boolean recoverByKeyID = false;
+
+        try {
+            String configValue = "externalReg.recover.byKeyID";
+            recoverByKeyID = configStore.getBoolean(configValue, false);
+        } catch (EBaseException e) {
+            // should never get here anyway
+            // but if it does, just take the default "false"
+            CMS.debug(method + " exception, take default: " + e);
+        }
+        CMS.debug(method + ": returning " + recoverByKeyID);
+        return recoverByKeyID;
+    }
+
     private String getRenewConfigKeyType(int keyTypeIndex) throws TPSException {
         String method = "TPSEnrollProcessor.getRenewConfigKeyType";
         IConfigStore configStore = CMS.getConfigStore();
-- 
2.7.4

