Improve NSS token handling

The updated NSS crypto-policy enables all tokens which broke
requesting certificates due to the way that tokens were managed.

0003-Include-the-token-name-when-a-PIN-is-provided-but-is.patch

@@ -0,0 +1,49 @@
+From c029b32c04a9a5993b9c8715fb82421fee613137 Mon Sep 17 00:00:00 2001
+From: Rob Crittenden <rcritten@redhat.com>
+Date: Fri, 31 Aug 2018 10:37:12 -0400
+Subject: [PATCH 2/7] Include the token name when a PIN is provided but is
+ unused
+
+This improves the output so the user will know which token
+the PIN is missing for. Theoretically it should be the token
+they asked for but this will show certmogner's view of it.
+---
+ src/certread-n.c | 6 +++---
+ src/keygen-n.c   | 4 ++--
+ 2 files changed, 5 insertions(+), 5 deletions(-)
+
+diff --git a/src/certread-n.c b/src/certread-n.c
+index f2e78c07..57a38dcf 100644
+--- a/src/certread-n.c
++++ b/src/certread-n.c
+@@ -259,9 +259,9 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 			if ((pin != NULL) &&
+ 			    (strlen(pin) > 0) &&
+ 			    (cb_data.n_attempts == 0)) {
+-				cm_log(1, "PIN was not needed to auth to cert "
+-				       "db, though one was provided. "
+-				       "Treating this as an error.\n");
++				cm_log(1, "PIN was not needed to auth to token "
++				       "%s, though one was provided. "
++				       "Treating this as an error.\n", token);
+ 				goto next_slot;
+ 			}
+ 		}
+diff --git a/src/keygen-n.c b/src/keygen-n.c
+index 8078a520..84b0bbd3 100644
+--- a/src/keygen-n.c
++++ b/src/keygen-n.c
+@@ -400,8 +400,8 @@ next_slot:
+ 	    (strlen(pin) > 0) &&
+ 	    (cb_data.n_attempts == 0)) {
+ 		cm_log(1, "PIN was not needed to auth to key "
+-		       "store, though one was provided. "
+-		       "Treating this as an error.\n");
++		       "store token %s, though one was provided. "
++		       "Treating this as an error.\n", token);
+ 		PK11_FreeSlotList(slotlist);
+ 		error = NSS_ShutdownContext(ctx);
+ 		if (error != SECSuccess) {
+-- 
+2.14.4
+

0004-Add-utility-function-to-get-the-internal-token-name.patch

@@ -0,0 +1,134 @@
+From f396b19b2c222fa0a50e9bb9704059af4578e678 Mon Sep 17 00:00:00 2001
+From: Rob Crittenden <rcritten@redhat.com>
+Date: Fri, 31 Aug 2018 12:08:35 -0400
+Subject: [PATCH 3/7] Add utility function to get the internal token name
+
+The NSS internal token is the default if no token is specified for
+the cert or the key.
+---
+ src/certread-n.c | 6 +++++-
+ src/certsave-n.c | 3 +++
+ src/keygen-n.c   | 3 +++
+ src/keyiread-n.c | 3 +++
+ src/submit-n.c   | 5 ++++-
+ src/util-n.c     | 6 ++++++
+ src/util-n.h     | 1 +
+ 7 files changed, 25 insertions(+), 2 deletions(-)
+
+diff --git a/src/certread-n.c b/src/certread-n.c
+index 57a38dcf..1d9217c6 100644
+--- a/src/certread-n.c
++++ b/src/certread-n.c
+@@ -190,6 +190,9 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 		cm_log(1, "Error reading PIN for cert db.\n");
+ 		_exit(CM_SUB_STATUS_ERROR_AUTH);
+ 	}
++	if (entry->cm_cert_token == NULL) {
++		entry->cm_cert_token = util_internal_token_name();
++	}
+ 	PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb);
+ 	for (sle = slotlist->head;
+ 	     ((sle != NULL) && (sle->slot != NULL));
+@@ -253,7 +256,8 @@ cm_certread_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 			}
+ 			error = PK11_Authenticate(sle->slot, PR_TRUE, &cb_data);
+ 			if (error != SECSuccess) {
+-				cm_log(1, "Error authenticating to cert db.\n");
++				cm_log(1, "certread-n: Error authenticating to cert db "
++					   "slot %s.\n", PK11_GetTokenName(sle->slot));
+ 				goto next_slot;
+ 			}
+ 			if ((pin != NULL) &&
+diff --git a/src/certsave-n.c b/src/certsave-n.c
+index af176ce5..193309c5 100644
+--- a/src/certsave-n.c
++++ b/src/certsave-n.c
+@@ -214,6 +214,9 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 			_exit(CM_SUB_STATUS_ERROR_AUTH);
+ 		}
+ 		PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb);
++		if (entry->cm_cert_token == NULL) {
++			entry->cm_cert_token = util_internal_token_name();
++		}
+ 		for (sle = slotlist->head;
+ 		     ((sle != NULL) && (sle->slot != NULL));
+ 		     sle = sle->next)
+diff --git a/src/keygen-n.c b/src/keygen-n.c
+index 84b0bbd3..f7fdf6c0 100644
+--- a/src/keygen-n.c
++++ b/src/keygen-n.c
+@@ -272,6 +272,9 @@ cm_keygen_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 		cm_log(1, "Error locating token for key generation.\n");
+ 		_exit(CM_SUB_STATUS_ERROR_NO_TOKEN);
+ 	}
++	if (entry->cm_cert_token == NULL) {
++		entry->cm_cert_token = util_internal_token_name();
++	}
+ 	/* Walk the list looking for the requested slot, or the first one if
+ 	 * none was requested. */
+ 	slot = NULL;
+diff --git a/src/keyiread-n.c b/src/keyiread-n.c
+index 89913aa2..b8408bf1 100644
+--- a/src/keyiread-n.c
++++ b/src/keyiread-n.c
+@@ -152,6 +152,9 @@ cm_keyiread_n_get_keys(struct cm_store_entry *entry, int readwrite)
+ 		_exit(CM_SUB_STATUS_ERROR_AUTH);
+ 	}
+ 	PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb);
++	if (entry->cm_key_token == NULL) {
++		entry->cm_key_token = util_internal_token_name();
++	}
+ 	n_tokens = 0;
+ 	pubkey = NULL;
+ 	/* In practice, the internal slot is either a non-storage slot (in
+diff --git a/src/submit-n.c b/src/submit-n.c
+index 872153ea..da07d253 100644
+--- a/src/submit-n.c
++++ b/src/submit-n.c
+@@ -346,6 +346,9 @@ cm_submit_n_decrypt_envelope(const unsigned char *envelope,
+ 		cm_log(1, "Error reading PIN for key storage.\n");
+ 		goto done;
+ 	}
++	if (args->entry->cm_key_token == NULL) {
++		args->entry->cm_key_token = util_internal_token_name();
++	}
+ 	PK11_SetPasswordFunc(&cm_pin_read_for_cert_nss_cb);
+ 	n_tokens = 0;
+ 	/* In practice, the internal slot is either a non-storage slot (in
+@@ -402,7 +405,7 @@ cm_submit_n_decrypt_envelope(const unsigned char *envelope,
+ 		}
+ 		error = PK11_Authenticate(slot, PR_TRUE, &cb_data);
+ 		if (error != SECSuccess) {
+-			cm_log(1, "Error authenticating to token "
++			cm_log(1, "submit-n: Error authenticating to token "
+ 			       "\"%s\".\n", token);
+ 			goto done;
+ 		}
+diff --git a/src/util-n.c b/src/util-n.c
+index 7805e58e..293e2583 100644
+--- a/src/util-n.c
++++ b/src/util-n.c
+@@ -287,3 +287,9 @@ util_set_db_entry_cert_owner(const char *dbdir, struct cm_store_entry *entry)
+ 	util_set_db_owner_perms(dbdir, secmoddb, entry->cm_cert_owner,
+ 				entry->cm_cert_perms);
+ }
++
++char *
++util_internal_token_name()
++{
++	return strdup(PK11_GetTokenName(PK11_GetInternalKeySlot()));
++}
+diff --git a/src/util-n.h b/src/util-n.h
+index 8a918d5c..637fd4b1 100644
+--- a/src/util-n.h
++++ b/src/util-n.h
+@@ -29,5 +29,6 @@ void util_set_db_entry_key_owner(const char *dbdir,
+ 				 struct cm_store_entry *entry);
+ void util_set_db_entry_cert_owner(const char *dbdir,
+ 				  struct cm_store_entry *entry);
++char * util_internal_token_name();
+ 
+ #endif
+-- 
+2.14.4
+

0005-Only-de-duplicate-certificates-within-the-same-token.patch

@@ -0,0 +1,41 @@
+From 6ebe5695a626c6cd254b249bbebf9846bcb936c0 Mon Sep 17 00:00:00 2001
+From: Rob Crittenden <rcritten@redhat.com>
+Date: Tue, 4 Sep 2018 11:06:13 -0400
+Subject: [PATCH 4/7] Only de-duplicate certificates within the same token
+
+certmonger may not have read/write access to tokens other than
+the one it is examining so don't try to de-duplicate certificates
+on other tokens.
+---
+ src/certsave-n.c | 8 +++++---
+ 1 file changed, 5 insertions(+), 3 deletions(-)
+
+diff --git a/src/certsave-n.c b/src/certsave-n.c
+index 193309c5..d0152cad 100644
+--- a/src/certsave-n.c
++++ b/src/certsave-n.c
+@@ -391,8 +391,9 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 						     !CERT_LIST_EMPTY(certlist) &&
+ 						     !CERT_LIST_END(node, certlist);
+ 						     node = CERT_LIST_NEXT(node)) {
+-							if (!SECITEM_ItemsAreEqual(&subject,
+-										   &node->cert->derSubject)) {
++							if ((!SECITEM_ItemsAreEqual(&subject,
++									   &node->cert->derSubject)) &&
++										(sle->slot == node->cert->slot)) {
+ 								cm_log(3, "Found a "
+ 								       "certificate "
+ 								       "with the same "
+@@ -441,7 +442,8 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 					     node = CERT_LIST_NEXT(node)) {
+ 						if ((node->cert->nickname != NULL) &&
+ 						    (strcmp(entry->cm_cert_nickname,
+-							    node->cert->nickname) != 0))
++							    node->cert->nickname) != 0) &&
++								(sle->slot == node->cert->slot))
+ 						{
+ 							i++;
+ 							cm_log(3, "Found a "
+-- 
+2.14.4
+

0006-Ensure-that-an-OpenSSL-random-seed-file-exists-when-.patch

@@ -0,0 +1,30 @@
+From 697dd085e7b2ce15eefc454509987270131d7f1e Mon Sep 17 00:00:00 2001
+From: Rob Crittenden <rcritten@redhat.com>
+Date: Tue, 4 Sep 2018 16:59:28 -0400
+Subject: [PATCH 5/7] Ensure that an OpenSSL random seed file exists when
+ testing
+
+Otherwise some openssl command-line invocations will fail and
+because of the way the tests are done the error message is not
+shown.
+---
+ tests/Makefile.am | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/tests/Makefile.am b/tests/Makefile.am
+index 4e407434..fe368dc0 100644
+--- a/tests/Makefile.am
++++ b/tests/Makefile.am
+@@ -433,6 +433,9 @@ subdirs += \
+ endif
+ 
+ check: all
++	if [ ! -e $$HOME/.rnd ] ; then \
++		openssl rand -writerand $$HOME/.rnd; \
++	fi
+ 	for required in certutil cmsutil pk12util openssl diff cmp mktemp \
+ 			dos2unix unix2dos dbus-launch ; do \
+ 		which $$required || exit 1; \
+-- 
+2.14.4
+

0007-Log-test-failures-of-bad-pin.patch

@@ -0,0 +1,29 @@
+From e93ecadec7c868f4227e084ffb65c70a6efd7314 Mon Sep 17 00:00:00 2001
+From: Rob Crittenden <rcritten@redhat.com>
+Date: Tue, 4 Sep 2018 18:12:18 -0400
+Subject: [PATCH 6/7] Log test failures of bad pin
+
+Previously this would show a "don't know why" failure.
+---
+ tests/tools/certsave.c | 5 +++++
+ 1 file changed, 5 insertions(+)
+
+diff --git a/tests/tools/certsave.c b/tests/tools/certsave.c
+index ac0f73ec..fd86a4c1 100644
+--- a/tests/tools/certsave.c
++++ b/tests/tools/certsave.c
+@@ -106,6 +106,11 @@ main(int argc, char **argv)
+ 				printf("Failed to save (%s:%s), "
+ 				       "filesystem permissions error.\n",
+ 				       ctype, entry->cm_cert_storage_location);
++			} else
++			if (cm_certsave_pin_error(state) == 0) {
++				printf("Failed to save (%s:%s), "
++				       "pin error.\n",
++				       ctype, entry->cm_cert_storage_location);
+ 			} else {
+ 				printf("Failed to save (%s:%s), "
+ 				       "don't know why.\n",
+-- 
+2.14.4
+

0008-Use-only-PK11_ImportCert-to-import-certs-not-CERT_Im.patch

@@ -0,0 +1,95 @@
+From 15d406ee3afbb52832d5c61a1afb735724d109a2 Mon Sep 17 00:00:00 2001
+From: Rob Crittenden <rcritten@redhat.com>
+Date: Tue, 18 Sep 2018 10:21:28 -0400
+Subject: [PATCH 7/7] Use only PK11_ImportCert to import certs, not
+ CERT_ImportCerts
+
+CERT_ImportCerts always imports a given certificate into the
+certificate database, whether a token is requested or not.
+
+Using PK11_ImportCert will import the cert, associate the key
+properly and will only add the certificate to the appropriate
+token.
+---
+ src/certsave-n.c | 37 +++++++++++--------------------------
+ 1 file changed, 11 insertions(+), 26 deletions(-)
+
+diff --git a/src/certsave-n.c b/src/certsave-n.c
+index d0152cad..fcb43148 100644
+--- a/src/certsave-n.c
++++ b/src/certsave-n.c
+@@ -100,7 +100,7 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 	NSSInitContext *ctx;
+ 	CERTCertDBHandle *certdb;
+ 	CERTCertList *certlist;
+-	CERTCertificate **returned, *oldcert, cert;
++	CERTCertificate *oldcert, *newcert, cert;
+ 	CERTCertTrust trust;
+ 	CERTSignedData csdata;
+ 	CERTCertListNode *node;
+@@ -497,33 +497,18 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 					}
+ 				}
+ 				/* Import the certificate. */
+-				returned = NULL;
+-				error = CERT_ImportCerts(certdb,
+-							 certUsageUserCertImport,
+-							 1, &item, &returned,
+-							 PR_TRUE,
+-							 PR_FALSE,
+-							 entry->cm_cert_nickname);
+-				ec = PORT_GetError();
+-				if (error == SECSuccess) {
+-					/* If NSS uses SQL DB storage, CERT_ImportCerts creates
+-					 * an incomplete internal state (the cert isn't
+-					 * associated with the private key, and calling
+-					 * PK11_FindKeyByAnyCert returns no result).
+-					 * As a workaround, we import the cert again using
+-					 * PK11_ImportCert, which magically fixes the issue.
+-					 * See rhbz#1532188 */
++				newcert = CERT_DecodeCertFromPackage((char *)item->data, item->len);
++				if (newcert != NULL) {
+ 					error = PK11_ImportCert(sle->slot,
+-						returned[0],
++						newcert,
+ 						CK_INVALID_HANDLE,
+-						returned[0]->nickname,
++						entry->cm_cert_nickname,
+ 						PR_FALSE);
+ 				}
+ 				if (error == SECSuccess) {
+-					cm_log(1, "Imported certificate \"%s\", got "
++					cm_log(1, "Imported certificate with "
+ 					       "nickname \"%s\".\n",
+-					       entry->cm_cert_nickname,
+-					       returned[0]->nickname);
++					       entry->cm_cert_nickname);
+ 					status = 0;
+ 					/* Set the trust on the new certificate,
+ 					 * perhaps matching the trust on an
+@@ -536,7 +521,7 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 						trust.objectSigningFlags = CERTDB_USER;
+ 					}
+ 					error = CERT_ChangeCertTrust(certdb,
+-								     returned[0],
++								     newcert,
+ 								     &trust);
+ 					ec = PORT_GetError();
+ 					if (error != SECSuccess) {
+@@ -621,10 +606,10 @@ cm_certsave_n_main(int fd, struct cm_store_ca *ca, struct cm_store_entry *entry,
+ 				}
+ 				/* If we managed to import the certificate, mark its
+ 				 * key for having its nickname removed. */
+-				if ((returned != NULL) && (returned[0] != NULL)) {
+-					privkey = PK11_FindKeyByAnyCert(returned[0], NULL);
++				if (newcert != NULL) {
++					privkey = PK11_FindKeyByAnyCert(newcert, NULL);
+ 					privkeys = add_privkey_to_list(privkeys, privkey);
+-					CERT_DestroyCertArray(returned, 1);
++					CERT_DestroyCertificate(newcert);
+ 				}
+ 				/* In case we're rekeying, but failed, mark the
+ 				 * candidate key for name-clearing or removal, too. */
+-- 
+2.14.4
+

certmonger.spec

@@ -26,7 +26,7 @@
 
 Name:		certmonger
 Version:	0.79.6
-Release:	2%{?dist}
+Release:	3%{?dist}
 Summary:	Certificate status monitor and PKI enrollment client
 
 Group:		System Environment/Daemons
@@ -111,7 +111,14 @@ Requires(preun):	/sbin/chkconfig, /sbin/service, dbus, sed
 Conflicts: libtevent < 0.9.13
 %endif
 
-Patch6:	0006-NSS-crypto-policy-sets-minimum-RSA-and-DSA-key-size-.patch
+Patch1:	0001-NSS-crypto-policy-sets-minimum-RSA-and-DSA-key-size-.patch
+Patch2:	0002-Use-the-correct-slot-when-saving-certificates-in-NSS.patch
+Patch3:	0003-Include-the-token-name-when-a-PIN-is-provided-but-is.patch
+Patch4:	0004-Add-utility-function-to-get-the-internal-token-name.patch
+Patch5:	0005-Only-de-duplicate-certificates-within-the-same-token.patch
+Patch6:	0006-Ensure-that-an-OpenSSL-random-seed-file-exists-when-.patch
+Patch7:	0007-Log-test-failures-of-bad-pin.patch
+Patch8:	0008-Use-only-PK11_ImportCert-to-import-certs-not-CERT_Im.patch
 
 %description
 Certmonger is a service which is primarily concerned with getting your
@@ -119,7 +126,14 @@ system enrolled with a certificate authority (CA) and keeping it enrolled.
 
 %prep
 %setup -q
+%patch1 -p1
+%patch2 -p1
+%patch3 -p1
+%patch4 -p1
+%patch5 -p1
 %patch6 -p1
+%patch7 -p1
+%patch8 -p1
 
 %if 0%{?rhel} > 0
 # Enabled by default for RHEL for bug #765600, still disabled by default for
@@ -247,6 +261,11 @@ exit 0
 %endif
 
 %changelog
+* Mon Oct  1 2018 Rob Crittenden <rcritten@redhat.com> - 0.79.6-3
+- Improve NSS token handling. The updated NSS crypto-policy enables all
+  tokens which broke requesting certificates due to the way that tokens
+  were managed.
+
 * Thu Jul 12 2018 Fedora Release Engineering <releng@fedoraproject.org> - 0.79.6-2
 - Rebuilt for https://fedoraproject.org/wiki/Fedora_29_Mass_Rebuild