Handle PKCS#7 S/MIME messages

Handle PKCS#7 S/MIME messages

[Daniel Kahn Gillmor]

This series applies after the "Add tests for S/MIME PKCS#7 messages"
series, which was introduced in
id:20200428185723.660184-1-dkg@fifthhorseman.net

With this series applied, notmuch handles standard PKCS#7 S/MIME
messages (using GnuPG's gpgsm as a backend, as mediated by GMime's use
of GPGME) as well as it handles PGP/MIME messages.

In addition to showing and replying to these messages, the series
covers indexing the cleartext of encrypted messages, and understanding
protected headers.

          --dkg

[PATCH 1/9] lib: index PKCS7 SignedData parts

[Daniel Kahn Gillmor]

When we are indexing, we should treat SignedData parts the same way
that we treat a multipart object, indexing the wrapped part as a
distinct MIME object.

Unfortunately, this means doing some sort of cryptographic
verification whose results we throw away, because GMime doesn't offer
us any way to unwrap without doing signature verification.

I've opened https://github.com/jstedfast/gmime/issues/67 to request
the capability from GMime but for now, we'll just accept the
additional performance hit.

As we do this indexing, we also apply the "signed" tag, by analogy
with how we handle multipart/signed messages.  These days, that kind
of change should probably be done with a property instead, but that's
a different set of changes.  This one is just for consistency.

Note that we are currently *only* handling signedData parts, which are
basically clearsigned messages.  PKCS#7 parts can also be
envelopedData and authEnvelopedData (which are effectively encryption
layers), and compressedData (which afaict isn't implemented anywhere,
i've never encountered it).  We're laying the groundwork for indexing
these other S/MIME types here, but we're only dealing with signedData
for now.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 lib/index.cc       | 57 ++++++++++++++++++++++++++++++++++++++++++++++
 test/T355-smime.sh |  2 --
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index 158ba5cf..bbf13dc5 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -372,6 +372,12 @@ _index_encrypted_mime_part (notmuch_message_t *message, notmuch_indexopts_t *ind
 			    GMimeMultipartEncrypted *part,
 			    _notmuch_message_crypto_t *msg_crypto);
 
+static void
+_index_pkcs7_part (notmuch_message_t *message,
+		   notmuch_indexopts_t *indexopts,
+		   GMimeObject *part,
+		   _notmuch_message_crypto_t *msg_crypto);
+
 /* Callback to generate terms for each mime part of a message. */
 static void
 _index_mime_part (notmuch_message_t *message,
@@ -466,6 +472,11 @@ _index_mime_part (notmuch_message_t *message,
 	goto DONE;
     }
 
+    if (GMIME_IS_APPLICATION_PKCS7_MIME (part)) {
+	_index_pkcs7_part (message, indexopts, part, msg_crypto);
+	goto DONE;
+    }
+
     if (! (GMIME_IS_PART (part))) {
 	_notmuch_database_log (notmuch_message_get_database (message),
 			       "Warning: Not indexing unknown mime part: %s.\n",
@@ -608,6 +619,52 @@ _index_encrypted_mime_part (notmuch_message_t *message,
 
 }
 
+static void
+_index_pkcs7_part (notmuch_message_t *message,
+		   notmuch_indexopts_t *indexopts,
+		   GMimeObject *part,
+		   _notmuch_message_crypto_t *msg_crypto)
+{
+    GMimeApplicationPkcs7Mime *pkcs7;
+    GMimeSecureMimeType p7type;
+    GMimeObject *mimeobj = NULL;
+    GMimeSignatureList *sigs = NULL;
+    GError *err = NULL;
+    notmuch_database_t *notmuch = NULL;
+
+    pkcs7 = GMIME_APPLICATION_PKCS7_MIME (part);
+    p7type = g_mime_application_pkcs7_mime_get_smime_type (pkcs7);
+    notmuch = notmuch_message_get_database (message);
+    _index_content_type (message, part);
+
+    if (p7type == GMIME_SECURE_MIME_TYPE_SIGNED_DATA) {
+	sigs = g_mime_application_pkcs7_mime_verify (pkcs7, GMIME_VERIFY_NONE, &mimeobj, &err);
+	if (sigs == NULL) {
+	    _notmuch_database_log (notmuch, "Failed to verify PKCS#7 SignedData during indexing. (%d:%d) [%s]\n",
+				   err->domain, err->code, err->message);
+	    g_error_free (err);
+	    goto DONE;
+	}
+	_notmuch_message_add_term (message, "tag", "signed");
+	GMimeObject *toindex = mimeobj;
+	if (_notmuch_message_crypto_potential_payload (msg_crypto, mimeobj, part, 0) &&
+	    msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
+	    toindex = _notmuch_repair_crypto_payload_skip_legacy_display (mimeobj);
+	    if (toindex != mimeobj)
+		notmuch_message_add_property (message, "index.repaired", "skip-protected-headers-legacy-display");
+	}
+	_index_mime_part (message, indexopts, toindex, msg_crypto);
+    } else {
+	_notmuch_database_log (notmuch, "Cannot currently handle PKCS#7 smime-type '%s'\n",
+			       g_mime_object_get_content_type_parameter (part, "smime-type"));
+    }
+ DONE:
+    if (mimeobj)
+	g_object_unref (mimeobj);
+    if (sigs)
+	g_object_unref (sigs);
+}
+
 static notmuch_status_t
 _notmuch_message_index_user_headers (notmuch_message_t *message, GMimeMessage *mime_message)
 {
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 117fa2b9..01e53e33 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -132,13 +132,11 @@ expected=''
 test_expect_equal "$expected" "$output"
 
 test_begin_subtest "know the MIME type of the embedded part in PKCS#7 SignedData"
-test_subtest_known_broken
 output=$(notmuch search --output=messages 'mimetype:text/plain')
 expected=id:smime-onepart-signed@protected-headers.example
 test_expect_equal "$expected" "$output"
 
 test_begin_subtest "PKCS#7 SignedData message is tagged 'signed'"
-test_subtest_known_broken
 output=$(notmuch dump id:smime-onepart-signed@protected-headers.example)
 expected='#notmuch-dump batch-tag:3 config,properties,tags
 +inbox +signed +unread -- id:smime-onepart-signed@protected-headers.example'
-- 
2.26.2

[PATCH 2/9] smime: Identify encrypted S/MIME parts during indexing

[Daniel Kahn Gillmor]

We don't handle them correctly yet, but we can at least mark them as
being encrypted.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 lib/index.cc       | 4 ++++
 test/T355-smime.sh | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/index.cc b/lib/index.cc
index bbf13dc5..f029b334 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -654,6 +654,10 @@ _index_pkcs7_part (notmuch_message_t *message,
 		notmuch_message_add_property (message, "index.repaired", "skip-protected-headers-legacy-display");
 	}
 	_index_mime_part (message, indexopts, toindex, msg_crypto);
+    } else if (p7type == GMIME_SECURE_MIME_TYPE_ENVELOPED_DATA) {
+	_notmuch_message_add_term (message, "tag", "encrypted");
+	if (notmuch_indexopts_get_decrypt_policy (indexopts) != NOTMUCH_DECRYPT_FALSE)
+	    _notmuch_database_log (notmuch, "Cannot decrypt PKCS#7 envelopedData (S/MIME encrypted messages)\n");
     } else {
 	_notmuch_database_log (notmuch, "Cannot currently handle PKCS#7 smime-type '%s'\n",
 			       g_mime_object_get_content_type_parameter (part, "smime-type"));
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 01e53e33..0d78f262 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -98,7 +98,6 @@ test_json_nodes <<<"$output" \
                 'crypto_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Notmuch Test Suite"'
 
 test_begin_subtest "encrypted+signed message is known to be encrypted, but signature is unknown"
-test_subtest_known_broken
 output=$(notmuch search subject:"test encrypted message 001")
 test_expect_equal "$output" "thread:0000000000000002   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message 001 (encrypted inbox)"
 
-- 
2.26.2

[PATCH 3/9] cli: include wrapped part of PKCS#7 SignedData in the MIME tree

[Daniel Kahn Gillmor]

Unwrap a PKCS#7 SignedData part unconditionally when the cli is
traversing the MIME tree, and return it as a "child" of what would
otherwise be a leaf in the tree.

Unfortunately, this also breaks the JSON output.  We will fix that
next.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 mime-node.c        | 23 +++++++++++++++++++++--
 test/T355-smime.sh |  2 +-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index ff6805bf..b6431e3b 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -220,8 +220,17 @@ node_verify (mime_node_t *node, GMimeObject *part)
     notmuch_status_t status;
 
     node->verify_attempted = true;
-    node->sig_list = g_mime_multipart_signed_verify (
-	GMIME_MULTIPART_SIGNED (part), GMIME_VERIFY_NONE, &err);
+    if (GMIME_IS_APPLICATION_PKCS7_MIME (part))
+	node->sig_list = g_mime_application_pkcs7_mime_verify (
+	    GMIME_APPLICATION_PKCS7_MIME (part), GMIME_VERIFY_NONE, &node->unwrapped_child, &err);
+    else
+	node->sig_list = g_mime_multipart_signed_verify (
+	    GMIME_MULTIPART_SIGNED (part), GMIME_VERIFY_NONE, &err);
+
+    if (node->unwrapped_child) {
+	node->nchildren = 1;
+	set_unwrapped_child_destructor (node);
+    }
 
     if (node->sig_list)
 	set_signature_list_destructor (node);
@@ -376,6 +385,12 @@ _mime_node_set_up_part (mime_node_t *node, GMimeObject *part, int numchild)
 	} else {
 	    node_verify (node, part);
 	}
+    } else if (GMIME_IS_APPLICATION_PKCS7_MIME (part) &&
+	       GMIME_SECURE_MIME_TYPE_SIGNED_DATA == g_mime_application_pkcs7_mime_get_smime_type (GMIME_APPLICATION_PKCS7_MIME (part))) {
+	/* If node->ctx->crypto->verify is false, it would be better
+	 * to just unwrap (instead of verifying), but
+	 * https://github.com/jstedfast/gmime/issues/67 */
+	node_verify (node, part);
     } else {
 	if (_notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, node->parent ? node->parent->part : NULL, numchild) &&
 	    node->ctx->msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
@@ -409,6 +424,10 @@ mime_node_child (mime_node_t *parent, int child)
 		GMIME_MULTIPART (parent->part), child);
     } else if (GMIME_IS_MESSAGE (parent->part)) {
 	sub = g_mime_message_get_mime_part (GMIME_MESSAGE (parent->part));
+    } else if (GMIME_IS_APPLICATION_PKCS7_MIME (parent->part) &&
+	       parent->unwrapped_child &&
+	       child == 0) {
+	sub = parent->unwrapped_child;
     } else {
 	/* This should have been caught by _mime_node_set_up_part */
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 0d78f262..710e51ec 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -142,7 +142,6 @@ expected='#notmuch-dump batch-tag:3 config,properties,tags
 test_expect_equal "$expected" "$output"
 
 test_begin_subtest "show contents of PKCS#7 SignedData message"
-test_subtest_known_broken
 output=$(notmuch show --format=raw --part=2 id:smime-onepart-signed@protected-headers.example)
 whitespace=' '
 expected="Bob, we need to cancel this contract.
@@ -178,6 +177,7 @@ On Tue, 26 Nov 2019 20:11:29 -0400, Alice Lovelace <alice@smime.example> wrote:
 test_expect_equal "$expected" "$output"
 
 test_begin_subtest "show PKCS#7 SignedData outputs valid JSON"
+test_subtest_known_broken
 output=$(notmuch show --format=json id:smime-onepart-signed@protected-headers.example)
 test_valid_json "$output"
 
-- 
2.26.2

[PATCH 4/9] cli/show: If a leaf part has children, show them instead of omitting

[Daniel Kahn Gillmor]

Until we did PKCS#7 unwrapping, no leaf MIME part could have a child.

Now, we treat the unwrapped MIME part as the child of the PKCS#7
SignedData object.  So in that case, we want to show it instead of
deliberately omitting the content.

This fixes the test of the protected subject in
id:smime-onepart-signed@protected-headers.example.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 notmuch-show.c                 | 11 ++++++++++-
 test/T355-smime.sh             |  2 --
 test/T356-protected-headers.sh |  1 -
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index ab1cd144..36265043 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -759,7 +759,16 @@ format_part_sprinter (const void *ctx, sprinter_t *sp, mime_node_t *node,
 	    sp->string_len (sp, (char *) part_content->data, part_content->len);
 	    g_object_unref (stream_memory);
 	} else {
-	    format_omitted_part_meta_sprinter (sp, meta, GMIME_PART (node->part));
+	    /* if we have a child part despite being a standard
+	     * (non-multipart) MIME part, that means there is
+	     * something to unwrap, which we will present in
+	     * content: */
+	    if (node->nchildren) {
+		sp->map_key (sp, "content");
+		sp->begin_list (sp);
+		nclose = 1;
+	    } else
+		format_omitted_part_meta_sprinter (sp, meta, GMIME_PART (node->part));
 	}
     } else if (GMIME_IS_MULTIPART (node->part)) {
 	sp->map_key (sp, "content");
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 710e51ec..099a3df7 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -177,12 +177,10 @@ On Tue, 26 Nov 2019 20:11:29 -0400, Alice Lovelace <alice@smime.example> wrote:
 test_expect_equal "$expected" "$output"
 
 test_begin_subtest "show PKCS#7 SignedData outputs valid JSON"
-test_subtest_known_broken
 output=$(notmuch show --format=json id:smime-onepart-signed@protected-headers.example)
 test_valid_json "$output"
 
 test_begin_subtest "Verify signature on PKCS#7 SignedData message"
-test_subtest_known_broken
 output=$(notmuch show --format=json id:smime-onepart-signed@protected-headers.example)
 test_json_nodes <<<"$output" \
                 'crypto:[0][0][0]["crypto"]["signed"]["status"][0]={
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index 520cb71c..bca50e29 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -157,7 +157,6 @@ test_expect_equal "$output" id:protected-with-legacy-display@crypto.notmuchmail.
 
 for variant in multipart-signed onepart-signed; do
     test_begin_subtest "verify signed PKCS#7 subject ($variant)"
-    [ "$variant" = multipart-signed ] || test_subtest_known_broken
     output=$(notmuch show --verify --format=json "id:smime-${variant}@protected-headers.example")
     test_json_nodes <<<"$output" \
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
-- 
2.26.2

[PATCH 5/9] cli/reply: Ignore PKCS#7 wrapper parts when replying

[Daniel Kahn Gillmor]

When composing a reply, no one wants to see this line in the proposed
message:

    Non-text part: application/pkcs7-mime

So we hide it, the same way we hide PGP/MIME cruft.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 notmuch-reply.c    | 5 +++--
 test/T355-smime.sh | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 2c30f6f9..ceb4f39b 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -65,8 +65,9 @@ format_part_reply (GMimeStream *stream, mime_node_t *node)
 	GMimeContentDisposition *disposition = g_mime_object_get_content_disposition (node->part);
 
 	if (g_mime_content_type_is_type (content_type, "application", "pgp-encrypted") ||
-	    g_mime_content_type_is_type (content_type, "application", "pgp-signature")) {
-	    /* Ignore PGP/MIME cruft parts */
+	    g_mime_content_type_is_type (content_type, "application", "pgp-signature") ||
+	    g_mime_content_type_is_type (content_type, "application", "pkcs7-mime")) {
+	    /* Ignore PGP/MIME and S/MIME cruft parts */
 	} else if (g_mime_content_type_is_type (content_type, "text", "*") &&
 		   ! g_mime_content_type_is_type (content_type, "text", "html")) {
 	    show_text_part_content (node->part, stream, NOTMUCH_SHOW_TEXT_PART_REPLY);
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 099a3df7..4b67a559 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -156,7 +156,6 @@ OpenPGP Example Corp"
 test_expect_equal "$expected" "$output"
 
 test_begin_subtest "reply to PKCS#7 SignedData message with proper quoting and attribution"
-test_subtest_known_broken
 output=$(notmuch reply id:smime-onepart-signed@protected-headers.example)
 expected="From: Notmuch Test Suite <test_suite@notmuchmail.org>
 Subject: Re: The FooCorp contract
-- 
2.26.2

[PATCH 6/9] crypto: Make _notmuch_crypto_decrypt take a GMimeObject

[Daniel Kahn Gillmor]

As we prepare to handle S/MIME-encrypted PKCS#7 EnvelopedData (which
is not multipart), we don't want to be limited to passing only
GMimeMultipartEncrypted MIME parts to _notmuch_crypto_decrypt.

There is no functional change here, just a matter of adjusting how we
pass arguments internally.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 lib/index.cc  | 8 ++++----
 mime-node.c   | 3 +--
 util/crypto.c | 6 +++---
 util/crypto.h | 2 +-
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index f029b334..da9a3abe 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -369,7 +369,7 @@ _index_content_type (notmuch_message_t *message, GMimeObject *part)
 
 static void
 _index_encrypted_mime_part (notmuch_message_t *message, notmuch_indexopts_t *indexopts,
-			    GMimeMultipartEncrypted *part,
+			    GMimeObject *part,
 			    _notmuch_message_crypto_t *msg_crypto);
 
 static void
@@ -439,7 +439,7 @@ _index_mime_part (notmuch_message_t *message,
 				     g_mime_multipart_get_part (multipart, i));
 		if (i == GMIME_MULTIPART_ENCRYPTED_CONTENT) {
 		    _index_encrypted_mime_part (message, indexopts,
-						GMIME_MULTIPART_ENCRYPTED (part),
+						part,
 						msg_crypto);
 		} else {
 		    if (i != GMIME_MULTIPART_ENCRYPTED_VERSION) {
@@ -551,7 +551,7 @@ _index_mime_part (notmuch_message_t *message,
 static void
 _index_encrypted_mime_part (notmuch_message_t *message,
 			    notmuch_indexopts_t *indexopts,
-			    GMimeMultipartEncrypted *encrypted_data,
+			    GMimeObject *encrypted_data,
 			    _notmuch_message_crypto_t *msg_crypto)
 {
     notmuch_status_t status;
@@ -603,7 +603,7 @@ _index_encrypted_mime_part (notmuch_message_t *message,
 	g_object_unref (decrypt_result);
     }
     GMimeObject *toindex = clear;
-    if (_notmuch_message_crypto_potential_payload (msg_crypto, clear, GMIME_OBJECT (encrypted_data), GMIME_MULTIPART_ENCRYPTED_CONTENT) &&
+    if (_notmuch_message_crypto_potential_payload (msg_crypto, clear, encrypted_data, GMIME_MULTIPART_ENCRYPTED_CONTENT) &&
 	msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
 	toindex = _notmuch_repair_crypto_payload_skip_legacy_display (clear);
 	if (toindex != clear)
diff --git a/mime-node.c b/mime-node.c
index b6431e3b..c2ee858d 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -253,7 +253,6 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
     GError *err = NULL;
     GMimeDecryptResult *decrypt_result = NULL;
     notmuch_status_t status;
-    GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
     notmuch_message_t *message = NULL;
 
     if (! node->unwrapped_child) {
@@ -266,7 +265,7 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
 	node->unwrapped_child = _notmuch_crypto_decrypt (&node->decrypt_attempted,
 							 node->ctx->crypto->decrypt,
 							 message,
-							 encrypteddata, &decrypt_result, &err);
+							 part, &decrypt_result, &err);
 	if (node->unwrapped_child)
 	    set_unwrapped_child_destructor (node);
     }
diff --git a/util/crypto.c b/util/crypto.c
index 0bb6f526..fbd5f011 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -34,7 +34,7 @@ GMimeObject *
 _notmuch_crypto_decrypt (bool *attempted,
 			 notmuch_decryption_policy_t decrypt,
 			 notmuch_message_t *message,
-			 GMimeMultipartEncrypted *part,
+			 GMimeObject *part,
 			 GMimeDecryptResult **decrypt_result,
 			 GError **err)
 {
@@ -55,7 +55,7 @@ _notmuch_crypto_decrypt (bool *attempted,
 	    }
 	    if (attempted)
 		*attempted = true;
-	    ret = g_mime_multipart_encrypted_decrypt (part,
+	    ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part),
 						      GMIME_DECRYPT_NONE,
 						      notmuch_message_properties_value (list),
 						      decrypt_result, err);
@@ -81,7 +81,7 @@ _notmuch_crypto_decrypt (bool *attempted,
     GMimeDecryptFlags flags = GMIME_DECRYPT_NONE;
     if (decrypt == NOTMUCH_DECRYPT_TRUE && decrypt_result)
 	flags |= GMIME_DECRYPT_EXPORT_SESSION_KEY;
-    ret = g_mime_multipart_encrypted_decrypt (part, flags, NULL,
+    ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part), flags, NULL,
 					      decrypt_result, err);
     return ret;
 }
diff --git a/util/crypto.h b/util/crypto.h
index f8bda0d1..4fa5599c 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -18,7 +18,7 @@ GMimeObject *
 _notmuch_crypto_decrypt (bool *attempted,
 			 notmuch_decryption_policy_t decrypt,
 			 notmuch_message_t *message,
-			 GMimeMultipartEncrypted *part,
+			 GMimeObject *part,
 			 GMimeDecryptResult **decrypt_result,
 			 GError **err);
 
-- 
2.26.2

[PATCH 7/9] crypto: handle PKCS#7 envelopedData in _notmuch_crypto_decrypt

[Daniel Kahn Gillmor]

In the two places where _notmuch_crypto_decrypt handles
multipart/encrypted messages (PGP/MIME), we should also handle PKCS#7
envelopedData (S/MIME).

This is insufficient for fully handling S/MIME encrypted data because
_notmuch_crypto_decrypt isn't yet actually invoked for envelopedData
parts, but that will happen in the following changes.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 util/crypto.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/util/crypto.c b/util/crypto.c
index fbd5f011..c09f467b 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -55,10 +55,21 @@ _notmuch_crypto_decrypt (bool *attempted,
 	    }
 	    if (attempted)
 		*attempted = true;
-	    ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part),
-						      GMIME_DECRYPT_NONE,
-						      notmuch_message_properties_value (list),
-						      decrypt_result, err);
+	    if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
+		ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part),
+							  GMIME_DECRYPT_NONE,
+							  notmuch_message_properties_value (list),
+							  decrypt_result, err);
+	    } else if (GMIME_IS_APPLICATION_PKCS7_MIME (part)) {
+	        GMimeApplicationPkcs7Mime *pkcs7 = GMIME_APPLICATION_PKCS7_MIME (part);
+		GMimeSecureMimeType type = g_mime_application_pkcs7_mime_get_smime_type (pkcs7);
+		if (type == GMIME_SECURE_MIME_TYPE_ENVELOPED_DATA) {
+		    ret = g_mime_application_pkcs7_mime_decrypt (pkcs7,
+								 GMIME_DECRYPT_NONE,
+								 notmuch_message_properties_value (list),
+								 decrypt_result, err);
+		}
+	    }
 	    if (ret)
 		break;
 	}
@@ -81,8 +92,17 @@ _notmuch_crypto_decrypt (bool *attempted,
     GMimeDecryptFlags flags = GMIME_DECRYPT_NONE;
     if (decrypt == NOTMUCH_DECRYPT_TRUE && decrypt_result)
 	flags |= GMIME_DECRYPT_EXPORT_SESSION_KEY;
-    ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part), flags, NULL,
-					      decrypt_result, err);
+    if (GMIME_IS_MULTIPART_ENCRYPTED (part)) {
+	ret = g_mime_multipart_encrypted_decrypt (GMIME_MULTIPART_ENCRYPTED (part), flags, NULL,
+						  decrypt_result, err);
+    } else if (GMIME_IS_APPLICATION_PKCS7_MIME (part)) {
+	GMimeApplicationPkcs7Mime *pkcs7 = GMIME_APPLICATION_PKCS7_MIME (part);
+	GMimeSecureMimeType p7type = g_mime_application_pkcs7_mime_get_smime_type (pkcs7);
+	if (p7type == GMIME_SECURE_MIME_TYPE_ENVELOPED_DATA) {
+	    ret = g_mime_application_pkcs7_mime_decrypt (pkcs7, flags, NULL,
+							 decrypt_result, err);
+	}
+    }
     return ret;
 }
 
-- 
2.26.2

[PATCH 8/9] smime: Pass PKCS#7 envelopedData to node_decrypt_and_verify

[Daniel Kahn Gillmor]

This change means we can support "notmuch show --decrypt=true" for
S/MIME encrypted messages, resolving several outstanding broken tests,
including all the remaining S/MIME protected header examples.

We do not yet handle indexing the cleartext of S/MIME encrypted
messages, though.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 mime-node.c                    | 6 ++++++
 test/T355-smime.sh             | 2 --
 test/T356-protected-headers.sh | 2 --
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index c2ee858d..f552e03a 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -390,6 +390,12 @@ _mime_node_set_up_part (mime_node_t *node, GMimeObject *part, int numchild)
 	 * to just unwrap (instead of verifying), but
 	 * https://github.com/jstedfast/gmime/issues/67 */
 	node_verify (node, part);
+    } else if (GMIME_IS_APPLICATION_PKCS7_MIME (part) &&
+	       GMIME_SECURE_MIME_TYPE_ENVELOPED_DATA == g_mime_application_pkcs7_mime_get_smime_type (GMIME_APPLICATION_PKCS7_MIME (part)) &&
+	       (node->ctx->crypto->decrypt != NOTMUCH_DECRYPT_FALSE)) {
+	node_decrypt_and_verify (node, part);
+	if (node->unwrapped_child && node->nchildren == 0)
+	    node->nchildren = 1;
     } else {
 	if (_notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, node->parent ? node->parent->part : NULL, numchild) &&
 	    node->ctx->msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL) {
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 4b67a559..0d32a7d5 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -80,7 +80,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "Decryption (notmuch CLI)"
-test_subtest_known_broken
 notmuch show --decrypt=true subject:"test encrypted message 001" |\
     grep "^This is a" > OUTPUT
 cat <<EOF > EXPECTED
@@ -89,7 +88,6 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest "Cryptographic message status (encrypted+signed)"
-test_subtest_known_broken
 output=$(notmuch show --format=json --decrypt=true subject:"test encrypted message 001")
 test_json_nodes <<<"$output" \
                 'crypto_encrypted:[0][0][0]["crypto"]["decrypted"]["status"]="full"' \
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index bca50e29..547b0c9a 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -168,7 +168,6 @@ done
 
 for variant in sign+enc sign+enc+legacy-disp; do
     test_begin_subtest "confirm signed and encrypted PKCS#7 subject ($variant)"
-    test_subtest_known_broken
     output=$(notmuch show --decrypt=true --format=json "id:smime-${variant}@protected-headers.example")
     test_json_nodes <<<"$output" \
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
@@ -179,7 +178,6 @@ for variant in sign+enc sign+enc+legacy-disp; do
 done
 
 test_begin_subtest "confirm encryption-protected PKCS#7 subject (enc+legacy-disp)"
-test_subtest_known_broken
 output=$(notmuch show --decrypt=true --format=json "id:smime-enc+legacy-disp@protected-headers.example")
 test_json_nodes <<<"$output" \
                 'encrypted:[0][0][0]["crypto"]["decrypted"]={"status":"full","header-mask":{"Subject":"..."}}' \
-- 
2.26.2

[PATCH 9/9] smime: Index cleartext of envelopedData when requested

[Daniel Kahn Gillmor]

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 lib/index.cc       | 5 +++--
 test/T355-smime.sh | 2 --
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index da9a3abe..826aa341 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -656,8 +656,9 @@ _index_pkcs7_part (notmuch_message_t *message,
 	_index_mime_part (message, indexopts, toindex, msg_crypto);
     } else if (p7type == GMIME_SECURE_MIME_TYPE_ENVELOPED_DATA) {
 	_notmuch_message_add_term (message, "tag", "encrypted");
-	if (notmuch_indexopts_get_decrypt_policy (indexopts) != NOTMUCH_DECRYPT_FALSE)
-	    _notmuch_database_log (notmuch, "Cannot decrypt PKCS#7 envelopedData (S/MIME encrypted messages)\n");
+	_index_encrypted_mime_part (message, indexopts,
+				    part,
+				    msg_crypto);
     } else {
 	_notmuch_database_log (notmuch, "Cannot currently handle PKCS#7 smime-type '%s'\n",
 			       g_mime_object_get_content_type_parameter (part, "smime-type"));
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 0d32a7d5..3573b5ee 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -107,12 +107,10 @@ test_begin_subtest "Reindex cleartext"
 test_expect_success "notmuch reindex --decrypt=true subject:'test encrypted message 001'"
 
 test_begin_subtest "signature is now known"
-test_subtest_known_broken
 output=$(notmuch search subject:"test encrypted message 001")
 test_expect_equal "$output" "thread:0000000000000002   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message 001 (encrypted inbox signed)"
 
 test_begin_subtest "Encrypted body is indexed"
-test_subtest_known_broken
 output=$(notmuch search 'this is a test encrypted message')
 test_expect_equal "$output" "thread:0000000000000002   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message 001 (encrypted inbox signed)"
 
-- 
2.26.2

Re: Handle PKCS#7 S/MIME messages

[Tomi Ollila]

On Thu, Apr 30 2020, Daniel Kahn Gillmor wrote:

> This series applies after the "Add tests for S/MIME PKCS#7 messages"
> series, which was introduced in
> id:20200428185723.660184-1-dkg@fifthhorseman.net
>
> With this series applied, notmuch handles standard PKCS#7 S/MIME
> messages (using GnuPG's gpgsm as a backend, as mediated by GMime's use
> of GPGME) as well as it handles PGP/MIME messages.
>
> In addition to showing and replying to these messages, the series
> covers indexing the cleartext of encrypted messages, and understanding
> protected headers.

I did not see anything suspicious in code, but

I got these test failures:

in ubuntu 19.10 native environment, and

in debian 10 (podman) container running in fedora 31 system


T355-smime: Testing S/MIME signature verification and decryption
 FAIL   Verify signature on PKCS#7 SignedData message
 crypto: value not equal: data[0][0][0]["crypto"]["signed"]["status"][0] =
 {'status': 'good', 
  'fingerprint': '702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB', 
  'created': 1574813489,
  'expires': 2611032858} != 
 {'created': 1574813489, 
  'expires': 2611032858,
  'fingerprint': '702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB', 
  'userid': 'CN=Alice Lovelace', 
  'status': 'good'}

T356-protected-headers: Testing Message decryption with protected headers
 FAIL   verify signed PKCS#7 subject (multipart-signed)
 sig_uid: object not found:  data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
 FAIL   verify signed PKCS#7 subject (onepart-signed)
 sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
 FAIL   confirm signed and encrypted PKCS#7 subject (sign+enc)
 sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
 FAIL   confirm signed and encrypted PKCS#7 subject (sign+enc+legacy-disp)
 sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]

>
>           --dkg

Tomi

Re: Handle PKCS#7 S/MIME messages

[Daniel Kahn Gillmor]

[-- Attachment #1.1: Type: text/plain, Size: 2688 bytes --]

Hi Tomi--

On Sat 2020-05-02 00:15:57 +0300, Tomi Ollila wrote:
> I did not see anything suspicious in code, but
>
> I got these test failures:
>
> in ubuntu 19.10 native environment, and
>
> in debian 10 (podman) container running in fedora 31 system
>
>
> T355-smime: Testing S/MIME signature verification and decryption
>  FAIL   Verify signature on PKCS#7 SignedData message
>  crypto: value not equal: data[0][0][0]["crypto"]["signed"]["status"][0] =
>  {'status': 'good', 
>   'fingerprint': '702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB', 
>   'created': 1574813489,
>   'expires': 2611032858} != 
>  {'created': 1574813489, 
>   'expires': 2611032858,
>   'fingerprint': '702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB', 
>   'userid': 'CN=Alice Lovelace', 
>   'status': 'good'}
>
> T356-protected-headers: Testing Message decryption with protected headers
>  FAIL   verify signed PKCS#7 subject (multipart-signed)
>  sig_uid: object not found:  data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
>  FAIL   verify signed PKCS#7 subject (onepart-signed)
>  sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
>  FAIL   confirm signed and encrypted PKCS#7 subject (sign+enc)
>  sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
>  FAIL   confirm signed and encrypted PKCS#7 subject (sign+enc+legacy-disp)
>  sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]

Thanks for identifying these.  These are problems related to a bug in
the released version of GMime on those platforms.  Unfixed versions of
gmime cannot report *any* certificate validity for X.509 certificates:

   https://github.com/jstedfast/gmime/pull/90

The fix for gmime is pretty simple, but it's not something we can
address directly in notmuch.

The fix was first released in GMime version 3.2.7, but it was first in
debian in gmime 3.2.6-2, and should be relatively easy to backport for
any distro that wants it (i suppose i could probably get it into the
next point release for debian 10 as well, since it is a bugfix for an
already-exposed API).

So, how should we deal with this in notmuch?  It seems a bit silly to
bump our required version of gmime to the (relatively new) version
3.2.7, for a fix for a cornercase of a novel use case.

Maybe the test suite should change based on version of GMime?  That
would cause problems for distros that backport the GMime fix, though.

I guess i could write a reproducer for the gmime issue and we could
include it in ./configure, and modify the test suite on that basis.

Any other suggestions?

    --dkg

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Handle PKCS#7 S/MIME messages

[Tomi Ollila]

On Mon, May 04 2020, Daniel Kahn Gillmor wrote:

> Hi Tomi--
>
> On Sat 2020-05-02 00:15:57 +0300, Tomi Ollila wrote:
>> I did not see anything suspicious in code, but
>>
>> I got these test failures:
>>
>> in ubuntu 19.10 native environment, and
>>
>> in debian 10 (podman) container running in fedora 31 system
>>
>>
>> T355-smime: Testing S/MIME signature verification and decryption
>>  FAIL   Verify signature on PKCS#7 SignedData message
>>  crypto: value not equal: data[0][0][0]["crypto"]["signed"]["status"][0] =
>>  {'status': 'good', 
>>   'fingerprint': '702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB', 
>>   'created': 1574813489,
>>   'expires': 2611032858} != 
>>  {'created': 1574813489, 
>>   'expires': 2611032858,
>>   'fingerprint': '702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB', 
>>   'userid': 'CN=Alice Lovelace', 
>>   'status': 'good'}
>>
>> T356-protected-headers: Testing Message decryption with protected headers
>>  FAIL   verify signed PKCS#7 subject (multipart-signed)
>>  sig_uid: object not found:  data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
>>  FAIL   verify signed PKCS#7 subject (onepart-signed)
>>  sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
>>  FAIL   confirm signed and encrypted PKCS#7 subject (sign+enc)
>>  sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
>>  FAIL   confirm signed and encrypted PKCS#7 subject (sign+enc+legacy-disp)
>>  sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]
>
> Thanks for identifying these.  These are problems related to a bug in
> the released version of GMime on those platforms.  Unfixed versions of
> gmime cannot report *any* certificate validity for X.509 certificates:
>
>    https://github.com/jstedfast/gmime/pull/90
>
> The fix for gmime is pretty simple, but it's not something we can
> address directly in notmuch.
>
> The fix was first released in GMime version 3.2.7, but it was first in
> debian in gmime 3.2.6-2, and should be relatively easy to backport for
> any distro that wants it (i suppose i could probably get it into the
> next point release for debian 10 as well, since it is a bugfix for an
> already-exposed API).
>
> So, how should we deal with this in notmuch?  It seems a bit silly to
> bump our required version of gmime to the (relatively new) version
> 3.2.7, for a fix for a cornercase of a novel use case.
>
> Maybe the test suite should change based on version of GMime?  That
> would cause problems for distros that backport the GMime fix, though.
>
> I guess i could write a reproducer for the gmime issue and we could
> include it in ./configure, and modify the test suite on that basis.

Reproducer in case gmime version is less than 3.2.7 -- with newer
gmimes that has to work so if that ever broke in newer gmimes we'd
notice (reproducer could hide that).

>
> Any other suggestions?
>
>     --dkg

Tomi

Re: Handle PKCS#7 S/MIME messages

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> Reproducer in case gmime version is less than 3.2.7 -- with newer
> gmimes that has to work so if that ever broke in newer gmimes we'd
> notice (reproducer could hide that).
>
>>
>> Any other suggestions?
>>

I guess it depends how many distros you think will patch this. It's not
so hard for distros to patch and / or skip a test. I think Daniel is in
the best position to judge this. If it's just Debian, then we can handle
it. If it's everyone, then that is probably a less good tradeoff.

d

[PATCH 1/2] test-lib: mark function variables as local

[Daniel Kahn Gillmor]

Several functions in test/test-lib.sh used variable names that are
also used outside of those functions (e.g. $output and $expected are
used in many of the test scripts), but they are not expected to
communicate via those variables.

We mark those variables "local" within test-lib.sh so that they do not
get clobbered when used outside test-lib.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/test-lib.sh | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 5c8eab7c..e8feab3b 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -109,7 +109,6 @@ unset ALTERNATE_EDITOR
 
 add_gnupg_home ()
 {
-    local output
     [ -e "${GNUPGHOME}/gpg.conf" ] && return
     _gnupg_exit () { gpgconf --kill all 2>/dev/null || true; }
     at_exit_function _gnupg_exit
@@ -427,7 +426,7 @@ emacs_fcc_message ()
 # number of messages.
 add_email_corpus ()
 {
-    corpus=${1:-default}
+    local corpus=${1:-default}
 
     rm -rf ${MAIL_DIR}
     cp -a $NOTMUCH_SRCDIR/test/corpora/$corpus ${MAIL_DIR}
@@ -465,14 +464,14 @@ test_expect_equal ()
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 parameters to test_expect_equal"
 
-	output="$1"
-	expected="$2"
+	local output="$1"
+	local expected="$2"
 	if ! test_skip "$test_subtest_name"
 	then
 		if [ "$output" = "$expected" ]; then
 			test_ok_
 		else
-			testname=$this_test.$test_count
+			local testname=$this_test.$test_count
 			echo "$expected" > $testname.expected
 			echo "$output" > $testname.output
 			test_failure_ "$(diff -u $testname.expected $testname.output)"
@@ -491,16 +490,16 @@ test_expect_equal_file ()
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 parameters to test_expect_equal_file"
 
-	file1="$1"
-	file2="$2"
+	local file1="$1"
+	local file2="$2"
 	if ! test_skip "$test_subtest_name"
 	then
 		if diff -q "$file1" "$file2" >/dev/null ; then
 			test_ok_
 		else
-			testname=$this_test.$test_count
-			basename1=`basename "$file1"`
-			basename2=`basename "$file2"`
+			local testname=$this_test.$test_count
+			local basename1=`basename "$file1"`
+			local basename2=`basename "$file2"`
 			cp "$file1" "$testname.$basename1"
 			cp "$file2" "$testname.$basename2"
 			test_failure_ "$(diff -u "$testname.$basename1" "$testname.$basename2")"
@@ -516,9 +515,9 @@ test_expect_equal_json () {
     # decode stdin as ASCII.  We need to read JSON in UTF-8, so
     # override Python's stdio encoding defaults.
     local script='import json, sys; json.dump(json.load(sys.stdin), sys.stdout, sort_keys=True, indent=4)'
-    output=$(echo "$1" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
+    local output=$(echo "$1" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
         || echo "$1")
-    expected=$(echo "$2" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
+    local expected=$(echo "$2" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
         || echo "$2")
     shift 2
     test_expect_equal "$output" "$expected" "$@"
@@ -540,6 +539,7 @@ test_sort_json () {
 # read the source of test/json_check_nodes.py (or the output when
 # invoking it without arguments) for an explanation of the syntax.
 test_json_nodes () {
+        local output
         exec 1>&6 2>&7		# Restore stdout and stderr
 	if [ -z "$inside_subtest" ]; then
 		error "bug in the test script: test_json_eval without test_begin_subtest"
@@ -577,7 +577,7 @@ test_emacs_expect_t () {
 		inside_subtest=
 
 		# Report success/failure.
-		result=$(cat OUTPUT)
+		local result=$(cat OUTPUT)
 		if [ "$result" = t ]
 		then
 			test_ok_
@@ -717,7 +717,7 @@ declare -A test_subtest_missing_external_prereq_
 
 # declare prerequisite for the given external binary
 test_declare_external_prereq () {
-	binary="$1"
+	local binary="$1"
 	test "$#" = 2 && name=$2 || name="$binary(1)"
 
 	if ! hash $binary 2>/dev/null; then
@@ -734,7 +734,7 @@ $binary () {
 # called indirectly (e.g. from emacs).
 # Returns success if dependency is available, failure otherwise.
 test_require_external_prereq () {
-	binary="$1"
+	local binary="$1"
 	if [[ ${test_missing_external_prereq_["${binary}"]} == t ]]; then
 		# dependency is missing, call the replacement function to note it
 		eval "$binary"
@@ -1075,8 +1075,8 @@ test_ruby() {
 }
 
 test_C () {
-    exec_file="test${test_count}"
-    test_file="${exec_file}.c"
+    local exec_file="test${test_count}"
+    local test_file="${exec_file}.c"
     cat > ${test_file}
     ${TEST_CC} ${TEST_CFLAGS} -I${NOTMUCH_SRCDIR}/test -I${NOTMUCH_SRCDIR}/lib -o ${exec_file} ${test_file} -L${NOTMUCH_BUILDDIR}/lib/ -lnotmuch -ltalloc
     echo "== stdout ==" > OUTPUT.stdout
@@ -1086,17 +1086,17 @@ test_C () {
 }
 
 make_shim () {
-    base_name="$1"
-    test_file="${base_name}.c"
-    shim_file="${base_name}.so"
+    local base_name="$1"
+    local test_file="${base_name}.c"
+    local shim_file="${base_name}.so"
     cat > ${test_file}
     ${TEST_CC} ${TEST_CFLAGS} ${TEST_SHIM_CFLAGS} -I${NOTMUCH_SRCDIR}/test -I${NOTMUCH_SRCDIR}/lib -o ${shim_file} ${test_file} ${TEST_SHIM_LDFLAGS}
 }
 
 notmuch_with_shim () {
-    base_name="$1"
+    local base_name="$1"
     shift
-    shim_file="${base_name}.so"
+    local shim_file="${base_name}.so"
     LD_PRELOAD=./${shim_file}${LD_PRELOAD:+:$LD_PRELOAD} notmuch-shared "$@"
 }
 
-- 
2.26.2

[PATCH 2/2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

[Daniel Kahn Gillmor]

When checking cryptographic signatures, Notmuch relies on GMime to
tell it whether the certificate that signs a message has a valid User
ID or not.

If the User ID is not valid, then notmuch does not report the signer's
User ID to the user.  This means that the consumer of notmuch's
cryptographic summary of a message (or of its protected headers) can
be confident in relaying the reported identity to the user.

However, some versions of GMime before 3.2.7 cannot report Certificate
validity for X.509 certificates.  This is resolved upstream in GMime
at https://github.com/jstedfast/gmime/pull/90.

We adapt to this by marking tests of reported User IDs for
S/MIME-signed messages as known-broken if GMime is older than 3.2.7
and has not been patched.

If GMime >= 3.2.7 and certificate validity still doesn't work for
X.509 certs, then there has likely been a regression in GMime and we
should fail early, during ./configure.

To break out these specific User ID checks from other checks, i had to
split some tests into two parts, and reuse $output across the two
subtests.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 configure                      | 79 ++++++++++++++++++++++++++++++++++
 test/T355-smime.sh             | 19 +++++---
 test/T356-protected-headers.sh | 15 ++++++-
 3 files changed, 104 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 0cfdaa6f..92e5bd1b 100755
--- a/configure
+++ b/configure
@@ -536,6 +536,82 @@ EOF
     if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
         rm -rf "$TEMP_GPG"
     fi
+
+    # see https://github.com/jstedfast/gmime/pull/90
+    # should be fixed in GMime in 3.2.7, but some distros might patch
+    printf "Checking for GMime X.509 certificate validity... "
+
+    cat > _check_x509_validity.c <<EOF
+#include <stdio.h>
+#include <gmime/gmime.h>
+
+int main () {
+    GError *error = NULL;
+    GMimeParser *parser = NULL;
+    GMimeApplicationPkcs7Mime *body = NULL;
+    GMimeSignatureList *sig_list = NULL;
+    GMimeSignature *sig = NULL;
+    GMimeCertificate *cert = NULL;
+    GMimeObject *output = NULL;
+    GMimeValidity validity = GMIME_VALIDITY_UNKNOWN;
+    int len;
+
+    g_mime_init ();
+    parser = g_mime_parser_new ();
+    g_mime_parser_init_with_stream (parser, g_mime_stream_file_open("test/corpora/pkcs7/smime-onepart-signed.eml", "r", &error));
+    if (error) return !! fprintf (stderr, "failed to instantiate parser with test/corpora/pkcs7/smime-onepart-signed.eml\n");
+
+    body = GMIME_APPLICATION_PKCS7_MIME(g_mime_message_get_mime_part (g_mime_parser_construct_message (parser, NULL)));
+    if (body == NULL) return !!	fprintf (stderr, "did not find a application/pkcs7 message\n");
+
+    sig_list = g_mime_application_pkcs7_mime_verify (body, GMIME_VERIFY_NONE, &output, &error);
+    if (error || output == NULL) return !! fprintf (stderr, "verify failed\n");
+
+    if (sig_list == NULL) return !! fprintf (stderr, "no GMimeSignatureList found\n");
+    len = g_mime_signature_list_length (sig_list);
+    if (len != 1) return !! fprintf (stderr, "expected 1 signature, got %d\n", len);
+    sig = g_mime_signature_list_get_signature (sig_list, 0);
+    if (sig == NULL) return !! fprintf (stderr, "no GMimeSignature found at position 0\n");
+    cert = g_mime_signature_get_certificate (sig);
+    if (cert == NULL) return !! fprintf (stderr, "no GMimeCertificate found\n");
+    validity = g_mime_certificate_get_id_validity (cert);
+    if (validity != GMIME_VALIDITY_FULL) return !! fprintf (stderr, "Got validity %d, expected %d\n", validity, GMIME_VALIDITY_FULL);
+
+    return 0;
+}
+EOF
+    if ! TEMP_GPG=$(mktemp -d "${TMPDIR:-/tmp}/notmuch.XXXXXX"); then
+        printf 'No.\nCould not make tempdir for testing X.509 certificate validity support.\n'
+        errors=$((errors + 1))
+    elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c ${gmime_ldflags} -o _check_x509_validity \
+            && echo disable-crl-checks > "$TEMP_GPG/gpgsm.conf" \
+            && echo "4D:E0:FF:63:C0:E9:EC:01:29:11:C8:7A:EE:DA:3A:9A:7F:6E:C1:0D S" >> "$TEMP_GPG/trustlist.txt" \
+            && GNUPGHOME=${TEMP_GPG} gpgsm --batch --quiet --import < "$srcdir"/test/smime/ca.crt
+    then
+        if GNUPGHOME=${TEMP_GPG} ./_check_x509_validity; then
+            gmime_x509_cert_validity=1
+            printf "Yes.\n"
+        else
+            gmime_x509_cert_validity=0
+            printf "No.\n"
+            if pkg-config --exists "gmime-3.0 >= 3.2.7"; then
+                cat <<EOF
+*** Error: GMime fails to calculate X.509 certificate validity, and
+is later than 3.2.7, which should have fixed this issue.
+
+Please follow up on https://github.com/jstedfast/gmime/pull/90 with
+more details.
+EOF
+                errors=$((errors + 1))
+            fi
+        fi
+    else
+        printf 'No.\nFailed to set up gpgsm for testing X.509 certificate validity support.\n'
+        errors=$((errors + 1))
+    fi
+    if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
+        rm -rf "$TEMP_GPG"
+    fi
 else
     have_gmime=0
     printf "No.\n"
@@ -1334,6 +1410,9 @@ NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${WITH_RETRY_LOCK}
 # Which backend will Xapian use by default?
 NOTMUCH_DEFAULT_XAPIAN_BACKEND=${default_xapian_backend}
 
+# Whether GMime can verify X.509 certificate validity
+NOTMUCH_GMIME_X509_CERT_VALIDITY=${gmime_x509_cert_validity}
+
 # do we have man pages?
 NOTMUCH_HAVE_MAN=$((have_sphinx))
 
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 3573b5ee..170f8649 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -177,13 +177,18 @@ test_valid_json "$output"
 
 test_begin_subtest "Verify signature on PKCS#7 SignedData message"
 output=$(notmuch show --format=json id:smime-onepart-signed@protected-headers.example)
+
+test_json_nodes <<<"$output" \
+                'created:[0][0][0]["crypto"]["signed"]["status"][0]["created"]=1574813489' \
+                'expires:[0][0][0]["crypto"]["signed"]["status"][0]["expires"]=2611032858' \
+                'fingerprint:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
+                'status:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"'
+
+test_begin_subtest "Verify signature on PKCS#7 SignedData message signer User ID"
+if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ]; then
+    test_subtest_known_broken
+fi
 test_json_nodes <<<"$output" \
-                'crypto:[0][0][0]["crypto"]["signed"]["status"][0]={
-                        "created" : 1574813489,
-                        "expires" : 2611032858,
-                        "fingerprint" : "702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB",
-                        "userid" : "CN=Alice Lovelace",
-                        "status" : "good"
-                     }'
+                'userid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
 
 test_done
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index 547b0c9a..074a2345 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -162,8 +162,13 @@ for variant in multipart-signed onepart-signed; do
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
                     'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
                     'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
-                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
                     'not_encrypted:[0][0][0]["crypto"]!"decrypted"'
+    test_begin_subtest "verify signed PKCS#7 subject ($variant) signer User ID"
+    if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ]; then
+        test_subtest_known_broken
+    fi
+    test_json_nodes <<<"$output" \
+                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
 done
 
 for variant in sign+enc sign+enc+legacy-disp; do
@@ -173,8 +178,14 @@ for variant in sign+enc sign+enc+legacy-disp; do
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
                     'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
                     'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
-                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
                     'encrypted:[0][0][0]["crypto"]["decrypted"]={"status":"full","header-mask":{"Subject":"..."}}'
+    test_begin_subtest "confirm signed and encrypted PKCS#7 subject ($variant) signer User ID"
+    if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ]; then
+        test_subtest_known_broken
+    fi
+    test_json_nodes <<<"$output" \
+                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
+
 done
 
 test_begin_subtest "confirm encryption-protected PKCS#7 subject (enc+legacy-disp)"
-- 
2.26.2

Re: [PATCH 1/2] test-lib: mark function variables as local

[Tomi Ollila]

On Wed, May 06 2020, Daniel Kahn Gillmor wrote:

> Several functions in test/test-lib.sh used variable names that are
> also used outside of those functions (e.g. $output and $expected are
> used in many of the test scripts), but they are not expected to
> communicate via those variables.
>
> We mark those variables "local" within test-lib.sh so that they do not
> get clobbered when used outside test-lib.


Good stuff

robustness comment IMO:

There is slight difference when writing

    local foo=`false`

and

    local foo; foo=`false`


former does not "fail"; latter does,


Although there is (currently!) no difference in our test code
(we don't have `set -e` there, IMO the former serves as a bad
example for anyone looking the code. 

(same applies to export foo=`bar`, readonly foo=`bar` and so
on, for anyone curious...)

IMO better declare all local variables in one line separately,
e.g.

    local output expected

and then either

    output=$1
    expected=$2
or
    output=$1 expected=$2

( FYI: exection latter in shell differs in a way one could do
  output=$expected expected=$output ) (IIRC, did not test >;)

(add double quotes around $1 and $2 if you desire =D)


well, when doing change just add the `local` line, smaller diff :)


Tomi

>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  test/test-lib.sh | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 5c8eab7c..e8feab3b 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -109,7 +109,6 @@ unset ALTERNATE_EDITOR
>  
>  add_gnupg_home ()
>  {
> -    local output
>      [ -e "${GNUPGHOME}/gpg.conf" ] && return
>      _gnupg_exit () { gpgconf --kill all 2>/dev/null || true; }
>      at_exit_function _gnupg_exit
> @@ -427,7 +426,7 @@ emacs_fcc_message ()
>  # number of messages.
>  add_email_corpus ()
>  {
> -    corpus=${1:-default}
> +    local corpus=${1:-default}
>  

Re: [PATCH 2/2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

[Tomi Ollila]

On Wed, May 06 2020, Daniel Kahn Gillmor wrote:

> When checking cryptographic signatures, Notmuch relies on GMime to
> tell it whether the certificate that signs a message has a valid User
> ID or not.
>
> If the User ID is not valid, then notmuch does not report the signer's
> User ID to the user.  This means that the consumer of notmuch's
> cryptographic summary of a message (or of its protected headers) can
> be confident in relaying the reported identity to the user.
>
> However, some versions of GMime before 3.2.7 cannot report Certificate
> validity for X.509 certificates.  This is resolved upstream in GMime
> at https://github.com/jstedfast/gmime/pull/90.
>
> We adapt to this by marking tests of reported User IDs for
> S/MIME-signed messages as known-broken if GMime is older than 3.2.7
> and has not been patched.
>
> If GMime >= 3.2.7 and certificate validity still doesn't work for
> X.509 certs, then there has likely been a regression in GMime and we
> should fail early, during ./configure.
>
> To break out these specific User ID checks from other checks, i had to
> split some tests into two parts, and reuse $output across the two
> subtests.

This works, on top of the previous series -- I skipped git am:ing 1/2,
so thos works without it -- it is good stuff just IMO requires some changes)

Tomi

>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  configure                      | 79 ++++++++++++++++++++++++++++++++++
>  test/T355-smime.sh             | 19 +++++---
>  test/T356-protected-headers.sh | 15 ++++++-
>  3 files changed, 104 insertions(+), 9 deletions(-)
>
> diff --git a/configure b/configure
> index 0cfdaa6f..92e5bd1b 100755
> --- a/configure
> +++ b/configure
> @@ -536,6 +536,82 @@ EOF
>      if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
>          rm -rf "$TEMP_GPG"
>      fi
> +
> +    # see https://github.com/jstedfast/gmime/pull/90
> +    # should be fixed in GMime in 3.2.7, but some distros might patch
> +    printf "Checking for GMime X.509 certificate validity... "
> +
> +    cat > _check_x509_validity.c <<EOF
> +#include <stdio.h>
> +#include <gmime/gmime.h>
> +
> +int main () {
> +    GError *error = NULL;
> +    GMimeParser *parser = NULL;
> +    GMimeApplicationPkcs7Mime *body = NULL;
> +    GMimeSignatureList *sig_list = NULL;
> +    GMimeSignature *sig = NULL;
> +    GMimeCertificate *cert = NULL;
> +    GMimeObject *output = NULL;
> +    GMimeValidity validity = GMIME_VALIDITY_UNKNOWN;
> +    int len;
> +
> +    g_mime_init ();
> +    parser = g_mime_parser_new ();
> +    g_mime_parser_init_with_stream (parser, g_mime_stream_file_open("test/corpora/pkcs7/smime-onepart-signed.eml", "r", &error));
> +    if (error) return !! fprintf (stderr, "failed to instantiate parser with test/corpora/pkcs7/smime-onepart-signed.eml\n");
> +
> +    body = GMIME_APPLICATION_PKCS7_MIME(g_mime_message_get_mime_part (g_mime_parser_construct_message (parser, NULL)));
> +    if (body == NULL) return !!	fprintf (stderr, "did not find a application/pkcs7 message\n");
> +
> +    sig_list = g_mime_application_pkcs7_mime_verify (body, GMIME_VERIFY_NONE, &output, &error);
> +    if (error || output == NULL) return !! fprintf (stderr, "verify failed\n");
> +
> +    if (sig_list == NULL) return !! fprintf (stderr, "no GMimeSignatureList found\n");
> +    len = g_mime_signature_list_length (sig_list);
> +    if (len != 1) return !! fprintf (stderr, "expected 1 signature, got %d\n", len);
> +    sig = g_mime_signature_list_get_signature (sig_list, 0);
> +    if (sig == NULL) return !! fprintf (stderr, "no GMimeSignature found at position 0\n");
> +    cert = g_mime_signature_get_certificate (sig);
> +    if (cert == NULL) return !! fprintf (stderr, "no GMimeCertificate found\n");
> +    validity = g_mime_certificate_get_id_validity (cert);
> +    if (validity != GMIME_VALIDITY_FULL) return !! fprintf (stderr, "Got validity %d, expected %d\n", validity, GMIME_VALIDITY_FULL);
> +
> +    return 0;
> +}
> +EOF
> +    if ! TEMP_GPG=$(mktemp -d "${TMPDIR:-/tmp}/notmuch.XXXXXX"); then
> +        printf 'No.\nCould not make tempdir for testing X.509 certificate validity support.\n'
> +        errors=$((errors + 1))
> +    elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c ${gmime_ldflags} -o _check_x509_validity \
> +            && echo disable-crl-checks > "$TEMP_GPG/gpgsm.conf" \
> +            && echo "4D:E0:FF:63:C0:E9:EC:01:29:11:C8:7A:EE:DA:3A:9A:7F:6E:C1:0D S" >> "$TEMP_GPG/trustlist.txt" \
> +            && GNUPGHOME=${TEMP_GPG} gpgsm --batch --quiet --import < "$srcdir"/test/smime/ca.crt
> +    then
> +        if GNUPGHOME=${TEMP_GPG} ./_check_x509_validity; then
> +            gmime_x509_cert_validity=1
> +            printf "Yes.\n"
> +        else
> +            gmime_x509_cert_validity=0
> +            printf "No.\n"
> +            if pkg-config --exists "gmime-3.0 >= 3.2.7"; then
> +                cat <<EOF
> +*** Error: GMime fails to calculate X.509 certificate validity, and
> +is later than 3.2.7, which should have fixed this issue.
> +
> +Please follow up on https://github.com/jstedfast/gmime/pull/90 with
> +more details.
> +EOF
> +                errors=$((errors + 1))
> +            fi
> +        fi
> +    else
> +        printf 'No.\nFailed to set up gpgsm for testing X.509 certificate validity support.\n'
> +        errors=$((errors + 1))
> +    fi
> +    if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
> +        rm -rf "$TEMP_GPG"
> +    fi
>  else
>      have_gmime=0
>      printf "No.\n"
> @@ -1334,6 +1410,9 @@ NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${WITH_RETRY_LOCK}
>  # Which backend will Xapian use by default?
>  NOTMUCH_DEFAULT_XAPIAN_BACKEND=${default_xapian_backend}
>  
> +# Whether GMime can verify X.509 certificate validity
> +NOTMUCH_GMIME_X509_CERT_VALIDITY=${gmime_x509_cert_validity}
> +
>  # do we have man pages?
>  NOTMUCH_HAVE_MAN=$((have_sphinx))
>  
> diff --git a/test/T355-smime.sh b/test/T355-smime.sh
> index 3573b5ee..170f8649 100755
> --- a/test/T355-smime.sh
> +++ b/test/T355-smime.sh
> @@ -177,13 +177,18 @@ test_valid_json "$output"
>  
>  test_begin_subtest "Verify signature on PKCS#7 SignedData message"
>  output=$(notmuch show --format=json id:smime-onepart-signed@protected-headers.example)
> +
> +test_json_nodes <<<"$output" \
> +                'created:[0][0][0]["crypto"]["signed"]["status"][0]["created"]=1574813489' \
> +                'expires:[0][0][0]["crypto"]["signed"]["status"][0]["expires"]=2611032858' \
> +                'fingerprint:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
> +                'status:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"'
> +
> +test_begin_subtest "Verify signature on PKCS#7 SignedData message signer User ID"
> +if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ]; then
> +    test_subtest_known_broken
> +fi
>  test_json_nodes <<<"$output" \
> -                'crypto:[0][0][0]["crypto"]["signed"]["status"][0]={
> -                        "created" : 1574813489,
> -                        "expires" : 2611032858,
> -                        "fingerprint" : "702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB",
> -                        "userid" : "CN=Alice Lovelace",
> -                        "status" : "good"
> -                     }'
> +                'userid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
>  
>  test_done
> diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
> index 547b0c9a..074a2345 100755
> --- a/test/T356-protected-headers.sh
> +++ b/test/T356-protected-headers.sh
> @@ -162,8 +162,13 @@ for variant in multipart-signed onepart-signed; do
>                      'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
>                      'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
>                      'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
> -                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
>                      'not_encrypted:[0][0][0]["crypto"]!"decrypted"'
> +    test_begin_subtest "verify signed PKCS#7 subject ($variant) signer User ID"
> +    if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ]; then
> +        test_subtest_known_broken
> +    fi
> +    test_json_nodes <<<"$output" \
> +                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
>  done
>  
>  for variant in sign+enc sign+enc+legacy-disp; do
> @@ -173,8 +178,14 @@ for variant in sign+enc sign+enc+legacy-disp; do
>                      'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
>                      'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
>                      'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
> -                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
>                      'encrypted:[0][0][0]["crypto"]["decrypted"]={"status":"full","header-mask":{"Subject":"..."}}'
> +    test_begin_subtest "confirm signed and encrypted PKCS#7 subject ($variant) signer User ID"
> +    if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ]; then
> +        test_subtest_known_broken
> +    fi
> +    test_json_nodes <<<"$output" \
> +                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
> +
>  done
>  
>  test_begin_subtest "confirm encryption-protected PKCS#7 subject (enc+legacy-disp)"
> -- 
> 2.26.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/2] test-lib: mark function variables as local

[Daniel Kahn Gillmor]

[-- Attachment #1.1: Type: text/plain, Size: 576 bytes --]

On Thu 2020-05-07 10:31:38 +0300, Tomi Ollila wrote:
> Good stuff
>
> robustness comment IMO:
>
> There is slight difference when writing
>
>     local foo=`false`
>
> and
>
>     local foo; foo=`false`
>
>
> former does not "fail"; latter does,

thanks for pointing this out.  On IRC, jindraj pointed me to
http://tldp.org/LDP/abs/html/localvar.html   this is an unusual and
confusing set of behaviors i had not understood about how local
interacts with return codes, etc.

I'll send around a revised set of patches that uses "local foo" instead
of "local foo=".

     --dkg

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 1/2 v2] test-lib: mark function variables as local

[Daniel Kahn Gillmor]

Several functions in test/test-lib.sh used variable names that are
also used outside of those functions (e.g. $output and $expected are
used in many of the test scripts), but they are not expected to
communicate via those variables.

We mark those variables "local" within test-lib.sh so that they do not
get clobbered when used outside test-lib.

We also move the local variable declarations to beginning of each
function, to avoid weird gotchas with local variable declarations as
described in https://tldp.org/LDP/abs/html/localvar.html.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/test-lib.sh | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 5c8eab7c..58972339 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -109,7 +109,6 @@ unset ALTERNATE_EDITOR
 
 add_gnupg_home ()
 {
-    local output
     [ -e "${GNUPGHOME}/gpg.conf" ] && return
     _gnupg_exit () { gpgconf --kill all 2>/dev/null || true; }
     at_exit_function _gnupg_exit
@@ -345,13 +344,14 @@ trap 'trap_signal' HUP INT TERM
 # to the message and encrypting/signing.
 emacs_deliver_message ()
 {
-    local subject="$1"
-    local body="$2"
+    local subject body smtp_dummy_pid smtp_dummy_port
+    subject="$1"
+    body="$2"
     shift 2
     # before we can send a message, we have to prepare the FCC maildir
     mkdir -p "$MAIL_DIR"/sent/{cur,new,tmp}
     # eval'ing smtp-dummy --background will set smtp_dummy_pid and -_port
-    local smtp_dummy_pid= smtp_dummy_port=
+    smtp_dummy_pid= smtp_dummy_port=
     eval `$TEST_DIRECTORY/smtp-dummy --background sent_message`
     test -n "$smtp_dummy_pid" || return 1
     test -n "$smtp_dummy_port" || return 1
@@ -391,13 +391,14 @@ emacs_deliver_message ()
 # new" after message delivery
 emacs_fcc_message ()
 {
-    local nmn_args=''
+    local nmn_args subject body
+    nmn_args=''
     while [[ "$1" =~ ^-- ]]; do
         nmn_args="$nmn_args $1"
         shift
     done
-    local subject="$1"
-    local body="$2"
+    subject="$1"
+    body="$2"
     shift 2
     # before we can send a message, we have to prepare the FCC maildir
     mkdir -p "$MAIL_DIR"/sent/{cur,new,tmp}
@@ -427,6 +428,7 @@ emacs_fcc_message ()
 # number of messages.
 add_email_corpus ()
 {
+    local corpus
     corpus=${1:-default}
 
     rm -rf ${MAIL_DIR}
@@ -457,6 +459,7 @@ test_begin_subtest ()
 # name.
 test_expect_equal ()
 {
+	local output expected testname
 	exec 1>&6 2>&7		# Restore stdout and stderr
 	if [ -z "$inside_subtest" ]; then
 		error "bug in the test script: test_expect_equal without test_begin_subtest"
@@ -483,6 +486,7 @@ test_expect_equal ()
 # Like test_expect_equal, but takes two filenames.
 test_expect_equal_file ()
 {
+	local file1 file2 testname basename1 basename2
 	exec 1>&6 2>&7		# Restore stdout and stderr
 	if [ -z "$inside_subtest" ]; then
 		error "bug in the test script: test_expect_equal_file without test_begin_subtest"
@@ -512,10 +516,11 @@ test_expect_equal_file ()
 # canonicalized before diff'ing.  If an argument cannot be parsed, it
 # is used unchanged so that there's something to diff against.
 test_expect_equal_json () {
+    local script output expected
     # The test suite forces LC_ALL=C, but this causes Python 3 to
     # decode stdin as ASCII.  We need to read JSON in UTF-8, so
     # override Python's stdio encoding defaults.
-    local script='import json, sys; json.dump(json.load(sys.stdin), sys.stdout, sort_keys=True, indent=4)'
+    script='import json, sys; json.dump(json.load(sys.stdin), sys.stdout, sort_keys=True, indent=4)'
     output=$(echo "$1" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
         || echo "$1")
     expected=$(echo "$2" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
@@ -540,6 +545,7 @@ test_sort_json () {
 # read the source of test/json_check_nodes.py (or the output when
 # invoking it without arguments) for an explanation of the syntax.
 test_json_nodes () {
+        local output
         exec 1>&6 2>&7		# Restore stdout and stderr
 	if [ -z "$inside_subtest" ]; then
 		error "bug in the test script: test_json_eval without test_begin_subtest"
@@ -561,6 +567,7 @@ test_json_nodes () {
 }
 
 test_emacs_expect_t () {
+	local result
 	test "$#" = 1 ||
 	error "bug in the test script: not 1 parameter to test_emacs_expect_t"
 	if [ -z "$inside_subtest" ]; then
@@ -653,7 +660,8 @@ notmuch_json_show_sanitize ()
 
 notmuch_emacs_error_sanitize ()
 {
-    local command=$1
+    local command
+    command=$1
     shift
     for file in "$@"; do
 	echo "=== $file ==="
@@ -717,6 +725,7 @@ declare -A test_subtest_missing_external_prereq_
 
 # declare prerequisite for the given external binary
 test_declare_external_prereq () {
+	local binary
 	binary="$1"
 	test "$#" = 2 && name=$2 || name="$binary(1)"
 
@@ -734,6 +743,7 @@ $binary () {
 # called indirectly (e.g. from emacs).
 # Returns success if dependency is available, failure otherwise.
 test_require_external_prereq () {
+	local binary
 	binary="$1"
 	if [[ ${test_missing_external_prereq_["${binary}"]} == t ]]; then
 		# dependency is missing, call the replacement function to note it
@@ -1075,6 +1085,7 @@ test_ruby() {
 }
 
 test_C () {
+    local exec_file test_file
     exec_file="test${test_count}"
     test_file="${exec_file}.c"
     cat > ${test_file}
@@ -1086,6 +1097,7 @@ test_C () {
 }
 
 make_shim () {
+    local base_name test_file shim_file
     base_name="$1"
     test_file="${base_name}.c"
     shim_file="${base_name}.so"
@@ -1094,6 +1106,7 @@ make_shim () {
 }
 
 notmuch_with_shim () {
+    local base_name shim_file
     base_name="$1"
     shift
     shim_file="${base_name}.so"
-- 
2.26.2

Re: [PATCH 1/2 v2] test-lib: mark function variables as local

[Tomi Ollila]

On Fri, May 08 2020, Daniel Kahn Gillmor wrote:

> Several functions in test/test-lib.sh used variable names that are
> also used outside of those functions (e.g. $output and $expected are
> used in many of the test scripts), but they are not expected to
> communicate via those variables.
>
> We mark those variables "local" within test-lib.sh so that they do not
> get clobbered when used outside test-lib.
>
> We also move the local variable declarations to beginning of each
> function, to avoid weird gotchas with local variable declarations as
> described in https://tldp.org/LDP/abs/html/localvar.html.

LGTM. I have to tolerate 'we' (passive) as I seem to use those myself, too
(although communicating a bit different thing...) =D

Tomi

> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  test/test-lib.sh | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 5c8eab7c..58972339 100644
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -109,7 +109,6 @@ unset ALTERNATE_EDITOR
>  
>  add_gnupg_home ()
>  {
> -    local output
>      [ -e "${GNUPGHOME}/gpg.conf" ] && return
>      _gnupg_exit () { gpgconf --kill all 2>/dev/null || true; }
>      at_exit_function _gnupg_exit
> @@ -345,13 +344,14 @@ trap 'trap_signal' HUP INT TERM
>  # to the message and encrypting/signing.
>  emacs_deliver_message ()
>  {
> -    local subject="$1"
> -    local body="$2"
> +    local subject body smtp_dummy_pid smtp_dummy_port
> +    subject="$1"
> +    body="$2"
>      shift 2
>      # before we can send a message, we have to prepare the FCC maildir
>      mkdir -p "$MAIL_DIR"/sent/{cur,new,tmp}
>      # eval'ing smtp-dummy --background will set smtp_dummy_pid and -_port
> -    local smtp_dummy_pid= smtp_dummy_port=
> +    smtp_dummy_pid= smtp_dummy_port=
>      eval `$TEST_DIRECTORY/smtp-dummy --background sent_message`
>      test -n "$smtp_dummy_pid" || return 1
>      test -n "$smtp_dummy_port" || return 1
> @@ -391,13 +391,14 @@ emacs_deliver_message ()
>  # new" after message delivery
>  emacs_fcc_message ()
>  {
> -    local nmn_args=''
> +    local nmn_args subject body
> +    nmn_args=''
>      while [[ "$1" =~ ^-- ]]; do
>          nmn_args="$nmn_args $1"
>          shift
>      done
> -    local subject="$1"
> -    local body="$2"
> +    subject="$1"
> +    body="$2"
>      shift 2
>      # before we can send a message, we have to prepare the FCC maildir
>      mkdir -p "$MAIL_DIR"/sent/{cur,new,tmp}
> @@ -427,6 +428,7 @@ emacs_fcc_message ()
>  # number of messages.
>  add_email_corpus ()
>  {
> +    local corpus
>      corpus=${1:-default}
>  
>      rm -rf ${MAIL_DIR}
> @@ -457,6 +459,7 @@ test_begin_subtest ()
>  # name.
>  test_expect_equal ()
>  {
> +	local output expected testname
>  	exec 1>&6 2>&7		# Restore stdout and stderr
>  	if [ -z "$inside_subtest" ]; then
>  		error "bug in the test script: test_expect_equal without test_begin_subtest"
> @@ -483,6 +486,7 @@ test_expect_equal ()
>  # Like test_expect_equal, but takes two filenames.
>  test_expect_equal_file ()
>  {
> +	local file1 file2 testname basename1 basename2
>  	exec 1>&6 2>&7		# Restore stdout and stderr
>  	if [ -z "$inside_subtest" ]; then
>  		error "bug in the test script: test_expect_equal_file without test_begin_subtest"
> @@ -512,10 +516,11 @@ test_expect_equal_file ()
>  # canonicalized before diff'ing.  If an argument cannot be parsed, it
>  # is used unchanged so that there's something to diff against.
>  test_expect_equal_json () {
> +    local script output expected
>      # The test suite forces LC_ALL=C, but this causes Python 3 to
>      # decode stdin as ASCII.  We need to read JSON in UTF-8, so
>      # override Python's stdio encoding defaults.
> -    local script='import json, sys; json.dump(json.load(sys.stdin), sys.stdout, sort_keys=True, indent=4)'
> +    script='import json, sys; json.dump(json.load(sys.stdin), sys.stdout, sort_keys=True, indent=4)'
>      output=$(echo "$1" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
>          || echo "$1")
>      expected=$(echo "$2" | PYTHONIOENCODING=utf-8 $NOTMUCH_PYTHON -c "$script" \
> @@ -540,6 +545,7 @@ test_sort_json () {
>  # read the source of test/json_check_nodes.py (or the output when
>  # invoking it without arguments) for an explanation of the syntax.
>  test_json_nodes () {
> +        local output
>          exec 1>&6 2>&7		# Restore stdout and stderr
>  	if [ -z "$inside_subtest" ]; then
>  		error "bug in the test script: test_json_eval without test_begin_subtest"
> @@ -561,6 +567,7 @@ test_json_nodes () {
>  }
>  
>  test_emacs_expect_t () {
> +	local result
>  	test "$#" = 1 ||
>  	error "bug in the test script: not 1 parameter to test_emacs_expect_t"
>  	if [ -z "$inside_subtest" ]; then
> @@ -653,7 +660,8 @@ notmuch_json_show_sanitize ()
>  
>  notmuch_emacs_error_sanitize ()
>  {
> -    local command=$1
> +    local command
> +    command=$1
>      shift
>      for file in "$@"; do
>  	echo "=== $file ==="
> @@ -717,6 +725,7 @@ declare -A test_subtest_missing_external_prereq_
>  
>  # declare prerequisite for the given external binary
>  test_declare_external_prereq () {
> +	local binary
>  	binary="$1"
>  	test "$#" = 2 && name=$2 || name="$binary(1)"
>  
> @@ -734,6 +743,7 @@ $binary () {
>  # called indirectly (e.g. from emacs).
>  # Returns success if dependency is available, failure otherwise.
>  test_require_external_prereq () {
> +	local binary
>  	binary="$1"
>  	if [[ ${test_missing_external_prereq_["${binary}"]} == t ]]; then
>  		# dependency is missing, call the replacement function to note it
> @@ -1075,6 +1085,7 @@ test_ruby() {
>  }
>  
>  test_C () {
> +    local exec_file test_file
>      exec_file="test${test_count}"
>      test_file="${exec_file}.c"
>      cat > ${test_file}
> @@ -1086,6 +1097,7 @@ test_C () {
>  }
>  
>  make_shim () {
> +    local base_name test_file shim_file
>      base_name="$1"
>      test_file="${base_name}.c"
>      shim_file="${base_name}.so"
> @@ -1094,6 +1106,7 @@ make_shim () {
>  }
>  
>  notmuch_with_shim () {
> +    local base_name shim_file
>      base_name="$1"
>      shift
>      shim_file="${base_name}.so"
> -- 
> 2.26.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/2 v2] test-lib: mark function variables as local

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> Several functions in test/test-lib.sh used variable names that are
> also used outside of those functions (e.g. $output and $expected are
> used in many of the test scripts), but they are not expected to
> communicate via those variables.
>
> We mark those variables "local" within test-lib.sh so that they do not
> get clobbered when used outside test-lib.
>
> We also move the local variable declarations to beginning of each
> function, to avoid weird gotchas with local variable declarations as
> described in https://tldp.org/LDP/abs/html/localvar.html.

Pushed this one to master.

I'm confused about where to apply 2/2. If I apply it on top of (updated)
master, it causes test failures. If I apply after the rest of the
patches in this thread then presumably there is some interval where the
build is broken (if only for certain GMime versions).

d

Re: [PATCH 1/2 v2] test-lib: mark function variables as local

[Daniel Kahn Gillmor]

[-- Attachment #1.1: Type: text/plain, Size: 611 bytes --]

On Sat 2020-05-09 08:47:10 -0300, David Bremner wrote:
> I'm confused about where to apply 2/2. If I apply it on top of (updated)
> master, it causes test failures. If I apply after the rest of the
> patches in this thread then presumably there is some interval where the
> build is broken (if only for certain GMime versions).

yes, that's right.  For certain GMime versions, there's an intermediate
breakage.  I think you're saying you want me to rerun the series, with
these things split out so that even on systems with older GMime, tests
pass at every commit.  I'll try to get that done today.

     --dkg

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2 v2] test-lib: mark function variables as local

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

>
> yes, that's right.  For certain GMime versions, there's an intermediate
> breakage.  I think you're saying you want me to rerun the series, with
> these things split out so that even on systems with older GMime, tests
> pass at every commit.  I'll try to get that done today.

Sounds perfect.

d

Re: [PATCH 1/2 v2] test-lib: mark function variables as local

[Daniel Kahn Gillmor]

[-- Attachment #1.1: Type: text/plain, Size: 881 bytes --]

On Sat 2020-05-09 08:47:10 -0300, David Bremner wrote:
> I'm confused about where to apply 2/2. If I apply it on top of (updated)
> master, it causes test failures. If I apply after the rest of the
> patches in this thread then presumably there is some interval where the
> build is broken (if only for certain GMime versions).

fwiw, i believe that the current master
( 627460d7bbbb6b95a07084c2b6fc7f647a5547e1 ) *is* broken on systems with
older gmime.

In particular, on debian buster, i see:

T356-protected-headers: Testing Message decryption with protected headers
 FAIL   verify signed PKCS#7 subject (multipart-signed)
	sig_uid: object not found: data[0][0][0]["crypto"]["signed"]["status"][0]["userid"]

My [PATCH v2/2 v2] in this thread (sent shortly) should resolve this
situation.  Then, i'll send a rebased version of the full S/MIME series
in a new thread.

   --dkg

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/2 v2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

[Daniel Kahn Gillmor]

When checking cryptographic signatures, Notmuch relies on GMime to
tell it whether the certificate that signs a message has a valid User
ID or not.

If the User ID is not valid, then notmuch does not report the signer's
User ID to the user.  This means that the consumer of notmuch's
cryptographic summary of a message (or of its protected headers) can
be confident in relaying the reported identity to the user.

However, some versions of GMime before 3.2.7 cannot report Certificate
validity for X.509 certificates.  This is resolved upstream in GMime
at https://github.com/jstedfast/gmime/pull/90.

We adapt to this by marking tests of reported User IDs for
S/MIME-signed messages as known-broken if GMime is older than 3.2.7
and has not been patched.

If GMime >= 3.2.7 and certificate validity still doesn't work for
X.509 certs, then there has likely been a regression in GMime and we
should fail early, during ./configure.

To break out these specific User ID checks from other checks, i had to
split some tests into two parts, and reuse $output across the two
subtests.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 configure                      | 79 ++++++++++++++++++++++++++++++++++
 test/T355-smime.sh             | 17 +++++---
 test/T356-protected-headers.sh | 13 +++++-
 3 files changed, 100 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 0cfdaa6f..92e5bd1b 100755
--- a/configure
+++ b/configure
@@ -536,6 +536,82 @@ EOF
     if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
         rm -rf "$TEMP_GPG"
     fi
+
+    # see https://github.com/jstedfast/gmime/pull/90
+    # should be fixed in GMime in 3.2.7, but some distros might patch
+    printf "Checking for GMime X.509 certificate validity... "
+
+    cat > _check_x509_validity.c <<EOF
+#include <stdio.h>
+#include <gmime/gmime.h>
+
+int main () {
+    GError *error = NULL;
+    GMimeParser *parser = NULL;
+    GMimeApplicationPkcs7Mime *body = NULL;
+    GMimeSignatureList *sig_list = NULL;
+    GMimeSignature *sig = NULL;
+    GMimeCertificate *cert = NULL;
+    GMimeObject *output = NULL;
+    GMimeValidity validity = GMIME_VALIDITY_UNKNOWN;
+    int len;
+
+    g_mime_init ();
+    parser = g_mime_parser_new ();
+    g_mime_parser_init_with_stream (parser, g_mime_stream_file_open("test/corpora/pkcs7/smime-onepart-signed.eml", "r", &error));
+    if (error) return !! fprintf (stderr, "failed to instantiate parser with test/corpora/pkcs7/smime-onepart-signed.eml\n");
+
+    body = GMIME_APPLICATION_PKCS7_MIME(g_mime_message_get_mime_part (g_mime_parser_construct_message (parser, NULL)));
+    if (body == NULL) return !!	fprintf (stderr, "did not find a application/pkcs7 message\n");
+
+    sig_list = g_mime_application_pkcs7_mime_verify (body, GMIME_VERIFY_NONE, &output, &error);
+    if (error || output == NULL) return !! fprintf (stderr, "verify failed\n");
+
+    if (sig_list == NULL) return !! fprintf (stderr, "no GMimeSignatureList found\n");
+    len = g_mime_signature_list_length (sig_list);
+    if (len != 1) return !! fprintf (stderr, "expected 1 signature, got %d\n", len);
+    sig = g_mime_signature_list_get_signature (sig_list, 0);
+    if (sig == NULL) return !! fprintf (stderr, "no GMimeSignature found at position 0\n");
+    cert = g_mime_signature_get_certificate (sig);
+    if (cert == NULL) return !! fprintf (stderr, "no GMimeCertificate found\n");
+    validity = g_mime_certificate_get_id_validity (cert);
+    if (validity != GMIME_VALIDITY_FULL) return !! fprintf (stderr, "Got validity %d, expected %d\n", validity, GMIME_VALIDITY_FULL);
+
+    return 0;
+}
+EOF
+    if ! TEMP_GPG=$(mktemp -d "${TMPDIR:-/tmp}/notmuch.XXXXXX"); then
+        printf 'No.\nCould not make tempdir for testing X.509 certificate validity support.\n'
+        errors=$((errors + 1))
+    elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c ${gmime_ldflags} -o _check_x509_validity \
+            && echo disable-crl-checks > "$TEMP_GPG/gpgsm.conf" \
+            && echo "4D:E0:FF:63:C0:E9:EC:01:29:11:C8:7A:EE:DA:3A:9A:7F:6E:C1:0D S" >> "$TEMP_GPG/trustlist.txt" \
+            && GNUPGHOME=${TEMP_GPG} gpgsm --batch --quiet --import < "$srcdir"/test/smime/ca.crt
+    then
+        if GNUPGHOME=${TEMP_GPG} ./_check_x509_validity; then
+            gmime_x509_cert_validity=1
+            printf "Yes.\n"
+        else
+            gmime_x509_cert_validity=0
+            printf "No.\n"
+            if pkg-config --exists "gmime-3.0 >= 3.2.7"; then
+                cat <<EOF
+*** Error: GMime fails to calculate X.509 certificate validity, and
+is later than 3.2.7, which should have fixed this issue.
+
+Please follow up on https://github.com/jstedfast/gmime/pull/90 with
+more details.
+EOF
+                errors=$((errors + 1))
+            fi
+        fi
+    else
+        printf 'No.\nFailed to set up gpgsm for testing X.509 certificate validity support.\n'
+        errors=$((errors + 1))
+    fi
+    if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
+        rm -rf "$TEMP_GPG"
+    fi
 else
     have_gmime=0
     printf "No.\n"
@@ -1334,6 +1410,9 @@ NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${WITH_RETRY_LOCK}
 # Which backend will Xapian use by default?
 NOTMUCH_DEFAULT_XAPIAN_BACKEND=${default_xapian_backend}
 
+# Whether GMime can verify X.509 certificate validity
+NOTMUCH_GMIME_X509_CERT_VALIDITY=${gmime_x509_cert_validity}
+
 # do we have man pages?
 NOTMUCH_HAVE_MAN=$((have_sphinx))
 
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 117fa2b9..f8e8e396 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -187,13 +187,16 @@ test_valid_json "$output"
 test_begin_subtest "Verify signature on PKCS#7 SignedData message"
 test_subtest_known_broken
 output=$(notmuch show --format=json id:smime-onepart-signed@protected-headers.example)
+
+test_json_nodes <<<"$output" \
+                'created:[0][0][0]["crypto"]["signed"]["status"][0]["created"]=1574813489' \
+                'expires:[0][0][0]["crypto"]["signed"]["status"][0]["expires"]=2611032858' \
+                'fingerprint:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
+                'status:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"'
+
+test_begin_subtest "Verify signature on PKCS#7 SignedData message signer User ID"
+test_subtest_known_broken
 test_json_nodes <<<"$output" \
-                'crypto:[0][0][0]["crypto"]["signed"]["status"][0]={
-                        "created" : 1574813489,
-                        "expires" : 2611032858,
-                        "fingerprint" : "702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB",
-                        "userid" : "CN=Alice Lovelace",
-                        "status" : "good"
-                     }'
+                'userid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
 
 test_done
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index 520cb71c..5fd27434 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -163,8 +163,13 @@ for variant in multipart-signed onepart-signed; do
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
                     'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
                     'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
-                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
                     'not_encrypted:[0][0][0]["crypto"]!"decrypted"'
+    test_begin_subtest "verify signed PKCS#7 subject ($variant) signer User ID"
+    if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ] || [ "$variant" != multipart-signed ]; then
+        test_subtest_known_broken
+    fi
+    test_json_nodes <<<"$output" \
+                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
 done
 
 for variant in sign+enc sign+enc+legacy-disp; do
@@ -175,8 +180,12 @@ for variant in sign+enc sign+enc+legacy-disp; do
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
                     'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
                     'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
-                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
                     'encrypted:[0][0][0]["crypto"]["decrypted"]={"status":"full","header-mask":{"Subject":"..."}}'
+    test_begin_subtest "confirm signed and encrypted PKCS#7 subject ($variant) signer User ID"
+    test_subtest_known_broken
+    test_json_nodes <<<"$output" \
+                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
+
 done
 
 test_begin_subtest "confirm encryption-protected PKCS#7 subject (enc+legacy-disp)"
-- 
2.26.2

Re: [PATCH 2/2 v2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> When checking cryptographic signatures, Notmuch relies on GMime to
> tell it whether the certificate that signs a message has a valid User
> ID or not.
>
> If the User ID is not valid, then notmuch does not report the signer's
> User ID to the user.  This means that the consumer of notmuch's
> cryptographic summary of a message (or of its protected headers) can
> be confident in relaying the reported identity to the user.
>
> However, some versions of GMime before 3.2.7 cannot report Certificate
> validity for X.509 certificates.  This is resolved upstream in GMime
> at https://github.com/jstedfast/gmime/pull/90.
>
> We adapt to this by marking tests of reported User IDs for
> S/MIME-signed messages as known-broken if GMime is older than 3.2.7
> and has not been patched.
>
> If GMime >= 3.2.7 and certificate validity still doesn't work for
> X.509 certs, then there has likely been a regression in GMime and we
> should fail early, during ./configure.
>
> To break out these specific User ID checks from other checks, i had to
> split some tests into two parts, and reuse $output across the two
> subtests.
>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
> ---
>  configure                      | 79 ++++++++++++++++++++++++++++++++++
>  test/T355-smime.sh             | 17 +++++---
>  test/T356-protected-headers.sh | 13 +++++-
>  3 files changed, 100 insertions(+), 9 deletions(-)
>
> diff --git a/configure b/configure
> index 0cfdaa6f..92e5bd1b 100755
> --- a/configure
> +++ b/configure
> @@ -536,6 +536,82 @@ EOF
>      if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
>          rm -rf "$TEMP_GPG"
>      fi
> +
> +    # see https://github.com/jstedfast/gmime/pull/90
> +    # should be fixed in GMime in 3.2.7, but some distros might patch
> +    printf "Checking for GMime X.509 certificate validity... "
> +
> +    cat > _check_x509_validity.c <<EOF
> +#include <stdio.h>
> +#include <gmime/gmime.h>
> +
> +int main () {
> +    GError *error = NULL;
> +    GMimeParser *parser = NULL;
> +    GMimeApplicationPkcs7Mime *body = NULL;
> +    GMimeSignatureList *sig_list = NULL;
> +    GMimeSignature *sig = NULL;
> +    GMimeCertificate *cert = NULL;
> +    GMimeObject *output = NULL;
> +    GMimeValidity validity = GMIME_VALIDITY_UNKNOWN;
> +    int len;
> +
> +    g_mime_init ();
> +    parser = g_mime_parser_new ();
> +    g_mime_parser_init_with_stream (parser, g_mime_stream_file_open("test/corpora/pkcs7/smime-onepart-signed.eml", "r", &error));
> +    if (error) return !! fprintf (stderr, "failed to instantiate parser with test/corpora/pkcs7/smime-onepart-signed.eml\n");
> +
> +    body = GMIME_APPLICATION_PKCS7_MIME(g_mime_message_get_mime_part (g_mime_parser_construct_message (parser, NULL)));
> +    if (body == NULL) return !!	fprintf (stderr, "did not find a application/pkcs7 message\n");

I find these long lines with !! in the middle pretty surprising. Is
there some reason for this style? It doesn't seem to fit with the usual
conventions.

This line in particular has a tab in the middle.


> +    elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c ${gmime_ldflags} -o _check_x509_validity \

The other test files are cleaned up in configure (source and binary)
once we are done with them.

As far as I could follow, the changes to the tests themselves look
reasonable.

Re: [PATCH 2/2 v2] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

[Daniel Kahn Gillmor]

[-- Attachment #1.1: Type: text/plain, Size: 1864 bytes --]

Thanks for the review, and for the poke about out-of-tree builds on IRC,
Bremner.  Another revision is coming in a minute.  Notes below…

On Thu 2020-05-21 20:29:05 -0300, David Bremner wrote:
> I find these long lines with !! in the middle pretty surprising. Is
> there some reason for this style? It doesn't seem to fit with the usual
> conventions.

Hm, do we even have conventions for inlined C in ./configure?

If you'd rather i expand these to something more verbose, i can do so,
but i was under the impression that we wanted to keep these C
interstitials fairly compact so that ./configure is still (somewhat)
readable as a shell script.

The "return !! fprintf…" idiom is a compact way to get a non-zero
process error code and an error message to stderr without introducing a
code block.  The only way for fprintf to return 0 (which would result in
the process returning 0) is if 0 bytes are written and no error
occurred, which isn't possible with any of the format strings supplied
here.

Another approach would be to pull the C entirely out of ./configure, but
that could have a problem when dealing with out-of-tree builds.  (i just
noticed a problem for this test with out-of-tree builds, which i'll
revise in a minute)

> This line in particular has a tab in the middle.

i dunno how that got there, i'll have it fixed in the upcoming revision.

>> +    elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c ${gmime_ldflags} -o _check_x509_validity \
>
> The other test files are cleaned up in configure (source and binary)
> once we are done with them.

good point, i'll ensure that they get cleaned up alongside
_check_session_keys* in the upcoming revision.

> As far as I could follow, the changes to the tests themselves look
> reasonable.

thanks for the thoughtful review!

       --dkg

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/2 v3] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

[Daniel Kahn Gillmor]

When checking cryptographic signatures, Notmuch relies on GMime to
tell it whether the certificate that signs a message has a valid User
ID or not.

If the User ID is not valid, then notmuch does not report the signer's
User ID to the user.  This means that the consumer of notmuch's
cryptographic summary of a message (or of its protected headers) can
be confident in relaying the reported identity to the user.

However, some versions of GMime before 3.2.7 cannot report Certificate
validity for X.509 certificates.  This is resolved upstream in GMime
at https://github.com/jstedfast/gmime/pull/90.

We adapt to this by marking tests of reported User IDs for
S/MIME-signed messages as known-broken if GMime is older than 3.2.7
and has not been patched.

If GMime >= 3.2.7 and certificate validity still doesn't work for
X.509 certs, then there has likely been a regression in GMime and we
should fail early, during ./configure.

To break out these specific User ID checks from other checks, i had to
split some tests into two parts, and reuse $output across the two
subtests.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 configure                      | 83 +++++++++++++++++++++++++++++++++-
 test/T355-smime.sh             | 17 ++++---
 test/T356-protected-headers.sh | 13 +++++-
 3 files changed, 102 insertions(+), 11 deletions(-)

diff --git a/configure b/configure
index 0cfdaa6f..37368bda 100755
--- a/configure
+++ b/configure
@@ -494,7 +494,7 @@ int main () {
     if (error) return !! fprintf (stderr, "failed to instantiate parser with test/corpora/crypto/basic-encrypted.eml\n");
 
     body = GMIME_MULTIPART_ENCRYPTED(g_mime_message_get_mime_part (g_mime_parser_construct_message (parser, NULL)));
-    if (body == NULL) return !!	fprintf (stderr, "did not find a multipart encrypted message\n");
+    if (body == NULL) return !! fprintf (stderr, "did not find a multipart encrypted message\n");
 
     output = g_mime_multipart_encrypted_decrypt (body, GMIME_DECRYPT_EXPORT_SESSION_KEY, NULL, &decrypt_result, &error);
     if (error || output == NULL) return !! fprintf (stderr, "decryption failed\n");
@@ -536,6 +536,82 @@ EOF
     if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
         rm -rf "$TEMP_GPG"
     fi
+
+    # see https://github.com/jstedfast/gmime/pull/90
+    # should be fixed in GMime in 3.2.7, but some distros might patch
+    printf "Checking for GMime X.509 certificate validity... "
+
+    cat > _check_x509_validity.c <<EOF
+#include <stdio.h>
+#include <gmime/gmime.h>
+
+int main () {
+    GError *error = NULL;
+    GMimeParser *parser = NULL;
+    GMimeApplicationPkcs7Mime *body = NULL;
+    GMimeSignatureList *sig_list = NULL;
+    GMimeSignature *sig = NULL;
+    GMimeCertificate *cert = NULL;
+    GMimeObject *output = NULL;
+    GMimeValidity validity = GMIME_VALIDITY_UNKNOWN;
+    int len;
+
+    g_mime_init ();
+    parser = g_mime_parser_new ();
+    g_mime_parser_init_with_stream (parser, g_mime_stream_file_open("$srcdir/test/corpora/pkcs7/smime-onepart-signed.eml", "r", &error));
+    if (error) return !! fprintf (stderr, "failed to instantiate parser with test/corpora/pkcs7/smime-onepart-signed.eml\n");
+
+    body = GMIME_APPLICATION_PKCS7_MIME(g_mime_message_get_mime_part (g_mime_parser_construct_message (parser, NULL)));
+    if (body == NULL) return !!	fprintf (stderr, "did not find a application/pkcs7 message\n");
+
+    sig_list = g_mime_application_pkcs7_mime_verify (body, GMIME_VERIFY_NONE, &output, &error);
+    if (error || output == NULL) return !! fprintf (stderr, "verify failed\n");
+
+    if (sig_list == NULL) return !! fprintf (stderr, "no GMimeSignatureList found\n");
+    len = g_mime_signature_list_length (sig_list);
+    if (len != 1) return !! fprintf (stderr, "expected 1 signature, got %d\n", len);
+    sig = g_mime_signature_list_get_signature (sig_list, 0);
+    if (sig == NULL) return !! fprintf (stderr, "no GMimeSignature found at position 0\n");
+    cert = g_mime_signature_get_certificate (sig);
+    if (cert == NULL) return !! fprintf (stderr, "no GMimeCertificate found\n");
+    validity = g_mime_certificate_get_id_validity (cert);
+    if (validity != GMIME_VALIDITY_FULL) return !! fprintf (stderr, "Got validity %d, expected %d\n", validity, GMIME_VALIDITY_FULL);
+
+    return 0;
+}
+EOF
+    if ! TEMP_GPG=$(mktemp -d "${TMPDIR:-/tmp}/notmuch.XXXXXX"); then
+        printf 'No.\nCould not make tempdir for testing X.509 certificate validity support.\n'
+        errors=$((errors + 1))
+    elif ${CC} ${CFLAGS} ${gmime_cflags} _check_x509_validity.c ${gmime_ldflags} -o _check_x509_validity \
+            && echo disable-crl-checks > "$TEMP_GPG/gpgsm.conf" \
+            && echo "4D:E0:FF:63:C0:E9:EC:01:29:11:C8:7A:EE:DA:3A:9A:7F:6E:C1:0D S" >> "$TEMP_GPG/trustlist.txt" \
+            && GNUPGHOME=${TEMP_GPG} gpgsm --batch --quiet --import < "$srcdir"/test/smime/ca.crt
+    then
+        if GNUPGHOME=${TEMP_GPG} ./_check_x509_validity; then
+            gmime_x509_cert_validity=1
+            printf "Yes.\n"
+        else
+            gmime_x509_cert_validity=0
+            printf "No.\n"
+            if pkg-config --exists "gmime-3.0 >= 3.2.7"; then
+                cat <<EOF
+*** Error: GMime fails to calculate X.509 certificate validity, and
+is later than 3.2.7, which should have fixed this issue.
+
+Please follow up on https://github.com/jstedfast/gmime/pull/90 with
+more details.
+EOF
+                errors=$((errors + 1))
+            fi
+        fi
+    else
+        printf 'No.\nFailed to set up gpgsm for testing X.509 certificate validity support.\n'
+        errors=$((errors + 1))
+    fi
+    if [ -n "$TEMP_GPG" -a -d "$TEMP_GPG" ]; then
+        rm -rf "$TEMP_GPG"
+    fi
 else
     have_gmime=0
     printf "No.\n"
@@ -1043,7 +1119,7 @@ for flag in -Wmissing-declarations; do
 done
 printf "\n\t%s\n" "${WARN_CFLAGS}"
 
-rm -f minimal minimal.c _libversion.c _libversion _libversion.sh _check_session_keys.c _check_session_keys
+rm -f minimal minimal.c _libversion.c _libversion _libversion.sh _check_session_keys.c _check_session_keys _check_x509_validity.c _check_x509_validity
 
 # construct the Makefile.config
 cat > Makefile.config <<EOF
@@ -1334,6 +1410,9 @@ NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${WITH_RETRY_LOCK}
 # Which backend will Xapian use by default?
 NOTMUCH_DEFAULT_XAPIAN_BACKEND=${default_xapian_backend}
 
+# Whether GMime can verify X.509 certificate validity
+NOTMUCH_GMIME_X509_CERT_VALIDITY=${gmime_x509_cert_validity}
+
 # do we have man pages?
 NOTMUCH_HAVE_MAN=$((have_sphinx))
 
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index 117fa2b9..f8e8e396 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -187,13 +187,16 @@ test_valid_json "$output"
 test_begin_subtest "Verify signature on PKCS#7 SignedData message"
 test_subtest_known_broken
 output=$(notmuch show --format=json id:smime-onepart-signed@protected-headers.example)
+
+test_json_nodes <<<"$output" \
+                'created:[0][0][0]["crypto"]["signed"]["status"][0]["created"]=1574813489' \
+                'expires:[0][0][0]["crypto"]["signed"]["status"][0]["expires"]=2611032858' \
+                'fingerprint:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
+                'status:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"'
+
+test_begin_subtest "Verify signature on PKCS#7 SignedData message signer User ID"
+test_subtest_known_broken
 test_json_nodes <<<"$output" \
-                'crypto:[0][0][0]["crypto"]["signed"]["status"][0]={
-                        "created" : 1574813489,
-                        "expires" : 2611032858,
-                        "fingerprint" : "702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB",
-                        "userid" : "CN=Alice Lovelace",
-                        "status" : "good"
-                     }'
+                'userid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
 
 test_done
diff --git a/test/T356-protected-headers.sh b/test/T356-protected-headers.sh
index 520cb71c..5fd27434 100755
--- a/test/T356-protected-headers.sh
+++ b/test/T356-protected-headers.sh
@@ -163,8 +163,13 @@ for variant in multipart-signed onepart-signed; do
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
                     'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
                     'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
-                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
                     'not_encrypted:[0][0][0]["crypto"]!"decrypted"'
+    test_begin_subtest "verify signed PKCS#7 subject ($variant) signer User ID"
+    if [ $NOTMUCH_GMIME_X509_CERT_VALIDITY -ne 1 ] || [ "$variant" != multipart-signed ]; then
+        test_subtest_known_broken
+    fi
+    test_json_nodes <<<"$output" \
+                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
 done
 
 for variant in sign+enc sign+enc+legacy-disp; do
@@ -175,8 +180,12 @@ for variant in sign+enc sign+enc+legacy-disp; do
                     'signed_subject:[0][0][0]["crypto"]["signed"]["headers"]=["Subject"]' \
                     'sig_good:[0][0][0]["crypto"]["signed"]["status"][0]["status"]="good"' \
                     'sig_fpr:[0][0][0]["crypto"]["signed"]["status"][0]["fingerprint"]="702BA4B157F1E2B7D16B0C6A5FFC8A7DE2057DEB"' \
-                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"' \
                     'encrypted:[0][0][0]["crypto"]["decrypted"]={"status":"full","header-mask":{"Subject":"..."}}'
+    test_begin_subtest "confirm signed and encrypted PKCS#7 subject ($variant) signer User ID"
+    test_subtest_known_broken
+    test_json_nodes <<<"$output" \
+                    'sig_uid:[0][0][0]["crypto"]["signed"]["status"][0]["userid"]="CN=Alice Lovelace"'
+
 done
 
 test_begin_subtest "confirm encryption-protected PKCS#7 subject (enc+legacy-disp)"
-- 
2.26.2

Re: [PATCH 2/2 v3] smime: tests of X.509 certificate validity are known-broken on GMime < 3.2.7

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

>
> We adapt to this by marking tests of reported User IDs for
> S/MIME-signed messages as known-broken if GMime is older than 3.2.7
> and has not been patched.

pushed

d