Bugfixes to new error handling code, refactor some unreadable nested logic in handle lookup code.

The mapping between PKCS #11 objects and libhal handles isn't quite
right yet.  This is a snapshot of bugfixes accumulated along the way,
before refactoring mapping code to deal with the underlying problem.

3 files changed, 78 insertions, 66 deletions

diff --git a/GNUmakefile b/GNUmakefile
index 4a7df7c..dc41be5 100644
--- a/GNUmakefile
+++ b/GNUmakefile
@@ -140,16 +140,17 @@ ifneq "${HSMBULLY}" ""
   bully: all
 	set -x; \
 	sudo rm -f ${HSMBULLY_DATABASE} ${HSMBULLY_DATABASE}-journal ${HSMBULLY_KS_CLIENT} ${HSMBULLY_KS_SERVER}; \
-	if test -x ${HSMBULLY_SERVER_BIN}; \
+	if test -x '${HSMBULLY_SERVER_BIN}'; \
 	then \
 		sudo CRYPTECH_KEYSTORE=${HSMBULLY_KS_SERVER} ${HSMBULLY_SERVER_BIN} & \
 		pid=$$!; \
-	fi; \
-	(echo fnord; echo fnord) | sudo ./p11util --set-so-pin --set-user-pin --pin-from-stdin; \
-	sudo PKCS11_DATABASE=${HSMBULLY_DATABASE} CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ${HSMBULLY} ${HSMBULLY_OPTIONS}; \
-	if test -x ${HSMBULLY_SERVER_BIN}; \
-	then \
+		sleep 5; \
+		(echo fnord; echo fnord) | CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ./p11util --set-so-pin --set-user-pin --pin-from-stdin; \
+		PKCS11_DATABASE=${HSMBULLY_DATABASE} CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ${HSMBULLY} ${HSMBULLY_OPTIONS}; \
 		sudo kill $$pid; \
+	else \
+		(echo fnord; echo fnord) | sudo CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ./p11util --set-so-pin --set-user-pin --pin-from-stdin; \
+		sudo PKCS11_DATABASE=${HSMBULLY_DATABASE} CRYPTECH_KEYSTORE=${HSMBULLY_KS_CLIENT} ${HSMBULLY} ${HSMBULLY_OPTIONS}; \
 	fi; \
 	sudo rm -f ${HSMBULLY_DATABASE} ${HSMBULLY_DATABASE}-journal ${HSMBULLY_KS_CLIENT} ${HSMBULLY_KS_SERVER}
 
diff --git a/pkcs11.c b/pkcs11.c
index 3e47c67..4fde44f 100644
--- a/pkcs11.c
+++ b/pkcs11.c
@@ -317,11 +317,11 @@ static inline hal_error_t _hal_whine(const hal_error_t err,
   int ok = 0;
   hal_error_t code;
 
-  va_start(ap, line)
-    do {
-      code = va_arg(ap, hal_error_t);
-      ok |= (err == code);
-    } while (code != HAL_OK);
+  va_start(ap, line);
+  do {
+    code = va_arg(ap, hal_error_t);
+    ok |= (err == code);
+  } while (code != HAL_OK);
   va_end(ap);
 
   if (!ok)
@@ -368,6 +368,9 @@ static CK_RV _p11_error_from_hal(const hal_error_t err, const char * const file,
      * More here later, first see if this compiles.
      */
 
+  case HAL_OK:
+    return CKR_OK;
+
   default:
 #if DEBUG_PKCS11 || DEBUG_HAL
     fprintf(stderr, "\n%s:%u: Mapping unhandled HAL error to CKR_FUNCTION_FAILED\n", file, line);
@@ -1630,39 +1633,25 @@ static int p11_object_get_pkey_handle(const p11_session_t * const session,
       !sql_check_row(sqlite3_step(q)))
     goto fail;
 
-  switch (sqlite3_column_type(q, 0)) {
+  const int column_0_type = sqlite3_column_type(q, 0);
+  const int column_1_type = sqlite3_column_type(q, 1);
 
-  case SQLITE_INTEGER:
+  if (column_0_type == SQLITE_INTEGER)
     pkey_type = (hal_key_type_t) sqlite3_column_int64(q, 0);
-    break;
-
-  case SQLITE_NULL:
-    if (!p11_object_pkey_type(object_handle, &pkey_type))
-      goto fail;
-    break;
 
-  default:
+  else if (column_0_type != SQLITE_NULL || !p11_object_pkey_type(object_handle, &pkey_type))
     goto fail;
-  }
 
-  switch (sqlite3_column_type(q, 1)) {
-
-  case SQLITE_BLOB:
-    err = hal_whine_allow(hal_rpc_pkey_find(p11_session_hal_client(session),
-                                            p11_session_hal_session(session), pkey_handle,
-                                            pkey_type, sqlite3_column_blob(q, 1),
-                                            sqlite3_column_bytes(q, 1),
-                                            flags),
+  if (column_1_type == SQLITE_BLOB)
+    err = hal_whine_allow(hal_rpc_pkey_find(p11_session_hal_client(session), p11_session_hal_session(session), pkey_handle,
+                                            pkey_type, sqlite3_column_blob(q, 1), sqlite3_column_bytes(q, 1), flags),
                           HAL_ERROR_KEY_NOT_FOUND);
-    break;
 
-  case SQLITE_NULL:
-    err = HAL_ERROR_KEY_NOT_FOUND;
-    break;
+  else if (column_1_type == SQLITE_NULL)
+    err = hal_whine(HAL_ERROR_KEY_NOT_FOUND);
 
-  default:
+  else
     goto fail;
-  }
 
   if (err == HAL_OK)
     ok = 1;
@@ -2285,26 +2274,28 @@ static CK_RV generate_keypair_ec(p11_session_t *session,
 
 static CK_RV generate_keypair(p11_session_t *session,
                               const CK_MECHANISM_PTR pMechanism,
-                              const CK_ATTRIBUTE_PTR pPublicKeyTemplate,
-                              const CK_ULONG ulPublicKeyAttributeCount,
-                              const CK_ATTRIBUTE_PTR pPrivateKeyTemplate,
-                              const CK_ULONG ulPrivateKeyAttributeCount,
-                              CK_OBJECT_HANDLE_PTR phPublicKey,
-                              CK_OBJECT_HANDLE_PTR phPrivateKey,
                               CK_RV (*mechanism_handler)(p11_session_t *session,
+
                                                          const CK_ATTRIBUTE_PTR pPublicKeyTemplate,
                                                          const CK_ULONG ulPublicKeyAttributeCount,
                                                          const CK_OBJECT_HANDLE public_handle,
                                                          const hal_key_flags_t public_flags,
+
                                                          const CK_ATTRIBUTE_PTR pPrivateKeyTemplate,
                                                          const CK_ULONG ulPrivateKeyAttributeCount,
                                                          const CK_OBJECT_HANDLE private_handle,
                                                          const hal_key_flags_t private_flags),
+                              const CK_ATTRIBUTE_PTR pPublicKeyTemplate,
+                              const CK_ULONG ulPublicKeyAttributeCount,
                               const p11_descriptor_t * const public_descriptor,
-                              const p11_descriptor_t * const private_descriptor)
+                              CK_OBJECT_HANDLE_PTR phPublicKey,
+                              const CK_ATTRIBUTE_PTR pPrivateKeyTemplate,
+                              const CK_ULONG ulPrivateKeyAttributeCount,
+                              const p11_descriptor_t * const private_descriptor,
+                              CK_OBJECT_HANDLE_PTR phPrivateKey)
 {
-  CK_OBJECT_HANDLE private_handle = CK_INVALID_HANDLE;
   CK_OBJECT_HANDLE public_handle = CK_INVALID_HANDLE;
+  CK_OBJECT_HANDLE private_handle = CK_INVALID_HANDLE;
   handle_flavor_t public_handle_flavor = handle_flavor_session_object;
   handle_flavor_t private_handle_flavor = handle_flavor_session_object;
   hal_key_flags_t public_flags = 0;
@@ -4295,23 +4286,15 @@ CK_RV C_GenerateKeyPair(CK_SESSION_HANDLE hSession,
   switch (pMechanism->mechanism) {
 
   case CKM_RSA_PKCS_KEY_PAIR_GEN:
-    rv = generate_keypair(session, pMechanism,
-                          pPublicKeyTemplate, ulPublicKeyAttributeCount,
-                          pPrivateKeyTemplate, ulPrivateKeyAttributeCount,
-                          phPublicKey, phPrivateKey,
-                          generate_keypair_rsa_pkcs,
-                          &p11_descriptor_rsa_public_key,
-                          &p11_descriptor_rsa_private_key);
+    rv = generate_keypair(session, pMechanism, generate_keypair_rsa_pkcs,
+                          pPublicKeyTemplate,  ulPublicKeyAttributeCount,  &p11_descriptor_rsa_public_key,  phPublicKey,
+                          pPrivateKeyTemplate, ulPrivateKeyAttributeCount, &p11_descriptor_rsa_private_key, phPrivateKey);
     break;
 
   case CKM_EC_KEY_PAIR_GEN:
-    rv = generate_keypair(session, pMechanism,
-                          pPublicKeyTemplate, ulPublicKeyAttributeCount,
-                          pPrivateKeyTemplate, ulPrivateKeyAttributeCount,
-                          phPublicKey, phPrivateKey,
-                          generate_keypair_ec,
-                          &p11_descriptor_ec_public_key,
-                          &p11_descriptor_ec_private_key);
+    rv = generate_keypair(session, pMechanism, generate_keypair_ec,
+                          pPublicKeyTemplate,  ulPublicKeyAttributeCount,  &p11_descriptor_ec_public_key,  phPublicKey,
+                          pPrivateKeyTemplate, ulPrivateKeyAttributeCount, &p11_descriptor_ec_private_key, phPrivateKey);
     break;
 
   default:
diff --git a/unit_tests.py b/unit_tests.py
index f5553d4..6866a87 100644
--- a/unit_tests.py
+++ b/unit_tests.py
@@ -272,7 +272,6 @@ class TestKeys(unittest.TestCase):
         p11.C_VerifyInit(self.session, CKM_ECDSA_SHA384, public_key)
         p11.C_Verify(self.session, hamster, sig)
 
-
     def test_gen_sign_verify_ecdsa_p521_sha512(self):
         #if not args.all_tests: self.skipTest("SHA-512 not available in current build")
         public_key, private_key = p11.C_GenerateKeyPair(self.session, CKM_EC_KEY_PAIR_GEN,
@@ -286,16 +285,45 @@ class TestKeys(unittest.TestCase):
         p11.C_VerifyInit(self.session, CKM_ECDSA_SHA512, public_key)
         p11.C_Verify(self.session, hamster, sig)
 
-    def test_gen_rsa_1024(self):
-        self.assertIsKeypair(
-          p11.C_GenerateKeyPair(self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 1024,
-                                CKA_ID = "RSA-1024", CKA_SIGN = True, CKA_VERIFY = True))
+    def test_gen_sign_verify_rsa_1024(self):
+        public_key, private_key = p11.C_GenerateKeyPair(
+            self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 1024,
+            CKA_ID = "RSA-1024", CKA_SIGN = True, CKA_VERIFY = True)
+        self.assertIsKeypair(public_key, private_key)
+        hamster = "Your mother was a hamster"
+        p11.C_SignInit(self.session, CKM_SHA512_RSA_PKCS, private_key)
+        sig = p11.C_Sign(self.session, hamster)
+        self.assertIsInstance(sig, str)
+        p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, public_key)
+        p11.C_Verify(self.session, hamster, sig)
 
-    def test_gen_rsa_2048(self):
+        if False:
+            a = p11.C_GetAttributeValue(self.session, public_key,
+                                        CKA_CLASS, CKA_KEY_TYPE, CKA_VERIFY, CKA_TOKEN,
+                                        CKA_PUBLIC_EXPONENT, CKA_MODULUS)
+            a[CKA_TOKEN] = not a[CKA_TOKEN]
+            o = p11.C_CreateObject(self.session, a)
+            p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, o)
+            p11.C_Verify(self.session, hamster, sig)
+
+            self.tearDown()
+            self.setUp()
+            o = p11.C_CreateObject(self.session, a)
+            p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, o)
+            p11.C_Verify(self.session, hamster, sig)
+
+    def test_gen_sign_verify_rsa_2048(self):
         if not args.all_tests: self.skipTest("RSA key generation is still painfully slow")
-        self.assertIsKeypair(
-          p11.C_GenerateKeyPair(self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 2048,
-                                CKA_ID = "RSA-1024", CKA_SIGN = True, CKA_VERIFY = True))
+        public_key, private_key = p11.C_GenerateKeyPair(
+            self.session, CKM_RSA_PKCS_KEY_PAIR_GEN, CKA_MODULUS_BITS = 2048,
+            CKA_ID = "RSA-2048", CKA_SIGN = True, CKA_VERIFY = True)
+        self.assertIsKeypair(public_key, private_key)
+        hamster = "Your mother was a hamster"
+        p11.C_SignInit(self.session, CKM_SHA512_RSA_PKCS, private_key)
+        sig = p11.C_Sign(self.session, hamster)
+        self.assertIsInstance(sig, str)
+        p11.C_VerifyInit(self.session, CKM_SHA512_RSA_PKCS, public_key)
+        p11.C_Verify(self.session, hamster, sig)
 
     @staticmethod
     def _build_ecpoint(x, y):