revision 2: easing access to the cryptographic envelope

revision 2: easing access to the cryptographic envelope

[Daniel Kahn Gillmor]

This is the second revision of the series originally posted at
id:20190424183113.29242-1-dkg@fifthhorseman.net

(that series no longer applies directly to master due to all the
cleanup that has been merged recently)

This series is an important baseline for my work on protected headers,
which i aim to start posting for review on this list soon, but can be
seen meanwhile on the protected-headers branch at
https://gitlab.com/dkg/notmuch.

------

E-mail structures are potentially arbitrarily complicated.
Cryptographic protection standards like S/MIME and OpenPGP or PGP/MIME
are often applicable to some elements of some messages.

Last year's "E-Fail" attacks made it clear that trying to provide
normal users with cryptographic protections on piecemeal parts of an
e-mail message is a recipe for disaster, both from an implementation
perspective and a user experience perspective.

I've argued in more detail at [0] about the need to treat
cryptographic protections at the message level, rather than at the
subpart level.

[0] https://dkg.fifthhorseman.net/blog/e-mail-cryptography.html

This series makes "notmuch show" track and emit message-wide
cryptographic state, providing an interface that simple clients that
use "notmuch show" can rely on for their UI and UX.

It doesn't yet apply this layer to the emacs interface, because at the
moment many users of the emacs interface are nerds who are as likely
to understand the intricacies of MIME structure as anyone, and for the
moment, just augmenting the notmuch show schemata in a sensible way is
enough of a chunk to bite off.

 (though i'd be happy to review and support the use of this
 per-message cryptographic state in notmuch-emacs if/when this lands!)

I'd appreciate any review and feedback!

Regards,

        --dkg

[PATCH v2 1/4] util/crypto: _notmuch_message_crypto: tracks message-wide crypto state

[Daniel Kahn Gillmor]

E-mail encryption and signatures reported by notmuch are at the MIME
part level.  This makes sense in the dirty details, but for users we
need to have a per-message conception of the cryptographic state of
the e-mail.  (see
https://dkg.fifthhorseman.net/blog/e-mail-cryptography.html for more
discussion of why this is important).

The object created in this patch is a useful for tracking the
cryptographic state of the underlying message as a whole, based on a
depth-first search of the message's MIME structure.

This object stores a signature list of the message, but we don't
handle it yet.  Further patches in this series will make use of the
signature list.
---
 util/crypto.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++
 util/crypto.h | 65 +++++++++++++++++++++++++++++++++++++
 2 files changed, 154 insertions(+)

diff --git a/util/crypto.c b/util/crypto.c
index 99104e78..55ff10e6 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -82,3 +82,92 @@ _notmuch_crypto_decrypt (bool *attempted,
 					     decrypt_result, err);
     return ret;
 }
+
+
+static int
+_notmuch_message_crypto_cleanup (_notmuch_message_crypto_t *msg_crypto)
+{
+    if (!msg_crypto)
+	return 0;
+    if (msg_crypto->sig_list)
+	g_object_unref (msg_crypto->sig_list);
+    return 0;
+}
+
+_notmuch_message_crypto_t *
+_notmuch_message_crypto_new (void *ctx)
+{
+    _notmuch_message_crypto_t *ret = talloc_zero (ctx, _notmuch_message_crypto_t);
+    talloc_set_destructor (ret, _notmuch_message_crypto_cleanup);
+    return ret;
+}
+
+
+notmuch_status_t
+_notmuch_message_crypto_potential_sig_list (_notmuch_message_crypto_t *msg_crypto, GMimeSignatureList *sigs)
+{
+    if (!msg_crypto)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    /* Signatures that arrive after a payload part during DFS are not
+     * part of the cryptographic envelope: */
+    if (msg_crypto->payload_encountered)
+	return NOTMUCH_STATUS_SUCCESS;
+
+    if (msg_crypto->sig_list)
+	g_object_unref (msg_crypto->sig_list);
+
+    msg_crypto->sig_list = sigs;
+    if (sigs)
+	g_object_ref (sigs);
+
+    if (msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL)
+	msg_crypto->signature_encrypted = true;
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+
+notmuch_status_t
+_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum)
+{
+    if (!msg_crypto || !payload)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    /* only fire on the first payload part encountered */
+    if (msg_crypto->payload_encountered)
+	return NOTMUCH_STATUS_SUCCESS;
+
+    /* the first child of multipart/encrypted that matches the
+     * encryption protocol should be "control information" metadata,
+     * not payload.  So we skip it. (see
+     * https://tools.ietf.org/html/rfc1847#page-8) */
+    if (parent && GMIME_IS_MULTIPART_ENCRYPTED (parent) && childnum == GMIME_MULTIPART_ENCRYPTED_VERSION) {
+	const char *enc_type = g_mime_object_get_content_type_parameter (parent, "protocol");
+	GMimeContentType *ct = g_mime_object_get_content_type (payload);
+	if (ct && enc_type) {
+	    const char *part_type = g_mime_content_type_get_mime_type (ct);
+	    if (part_type && strcmp (part_type, enc_type) == 0)
+		return NOTMUCH_STATUS_SUCCESS;
+	}
+    }
+
+    msg_crypto->payload_encountered = true;
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+
+notmuch_status_t
+_notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_crypto)
+{
+    if (!msg_crypto)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    if (!msg_crypto->payload_encountered)
+	msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_FULL;
+    else if (msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_NONE)
+	msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_PARTIAL;
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
diff --git a/util/crypto.h b/util/crypto.h
index af3998e8..c6fa7f4b 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -25,6 +25,71 @@ _notmuch_crypto_decrypt (bool *attempted,
 void
 _notmuch_crypto_cleanup (_notmuch_crypto_t *crypto);
 
+/* The user probably wants to know if the entire message was in the
+ * clear.  When replying, the MUA probably wants to know whether there
+ * was any part decrypted in the message.  And when displaying to the
+ * user, we probably only want to display "encrypted message" if the
+ * entire message was covered by encryption. */
+typedef enum {
+    NOTMUCH_MESSAGE_DECRYPTED_NONE = 0,
+    NOTMUCH_MESSAGE_DECRYPTED_PARTIAL,
+    NOTMUCH_MESSAGE_DECRYPTED_FULL,
+} _notmuch_message_decryption_status_t;
+
+/* description of the cryptographic state of a given message overall;
+ * for use by simple user agents.
+ */
+typedef struct _notmuch_message_crypto {
+    /* encryption status: partial, full, none */
+    _notmuch_message_decryption_status_t decryption_status;
+    /* FIXME: can we show what key(s) a fully-encrypted message was
+     * encrypted to? This data is not necessarily cryptographically
+     * reliable; even when we decrypt, we might not know which public
+     * key was used (e.g. if we're using a session key). */
+
+    /* signature status of the whole message (either the whole message
+     * is signed, or it is not) -- this means that partially-signed
+     * messages will get no signature status. */
+    GMimeSignatureList * sig_list;
+    /* if part of the message was signed, and the MUA is clever, it
+     * can determine on its own exactly which part and try to make
+     * more sense of it. */
+
+    /* mark this flag once we encounter a payload (i.e. something that
+     * is not part of the cryptographic envelope) */
+    bool payload_encountered;
+
+    /* if both signed and encrypted, was the signature encrypted? */
+    bool signature_encrypted;
+} _notmuch_message_crypto_t;
+
+
+/* _notmuch_message_crypto_t objects should be released with
+ * talloc_free (), or they will be released along with their parent
+ * context.
+ */
+_notmuch_message_crypto_t *
+_notmuch_message_crypto_new (void *ctx);
+
+/* call potential_sig_list during a depth-first-search on a message to
+ * consider a particular signature as relevant for the message.
+ */
+notmuch_status_t
+_notmuch_message_crypto_potential_sig_list (_notmuch_message_crypto_t *msg_crypto, GMimeSignatureList *sigs);
+
+/* call successful_decryption during a depth-first-search on a message
+ * to indicate that a part was successfully decrypted.
+ */
+notmuch_status_t
+_notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_crypto);
+
+/* call potential_payload during a depth-first-search on a message
+ * when encountering a message part that is not part of the envelope.
+ */
+notmuch_status_t
+_notmuch_message_crypto_potential_payload (_notmuch_message_crypto_t *msg_crypto, GMimeObject *payload, GMimeObject *parent, int childnum);
+
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.20.1

[PATCH v2 2/4] cli: expose message-wide crypto status from mime-node

[Daniel Kahn Gillmor]

The mime node context (a per-message context) gains a cryptographic
status object, and the mime_node_t object itself can return a view on
that status to an interested party.

The status is not yet populated, and for now we can keep that view
read-only, so that it can only be populated/modified during MIME tree
traversal.
---
 mime-node.c      | 7 +++++++
 notmuch-client.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/mime-node.c b/mime-node.c
index e33336bb..66ff7446 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -34,6 +34,7 @@ typedef struct mime_node_context {
     GMimeStream *stream;
     GMimeParser *parser;
     GMimeMessage *mime_message;
+    _notmuch_message_crypto_t *msg_crypto;
 
     /* Context provided by the caller. */
     _notmuch_crypto_t *crypto;
@@ -54,6 +55,12 @@ _mime_node_context_free (mime_node_context_t *res)
     return 0;
 }
 
+const _notmuch_message_crypto_t*
+mime_node_get_message_crypto_status (mime_node_t *node)
+{
+    return node->ctx->msg_crypto;
+}
+
 notmuch_status_t
 mime_node_open (const void *ctx, notmuch_message_t *message,
 		_notmuch_crypto_t *crypto, mime_node_t **root_out)
diff --git a/notmuch-client.h b/notmuch-client.h
index d762d3cc..a82cb431 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -439,6 +439,9 @@ mime_node_child (mime_node_t *parent, int child);
 mime_node_t *
 mime_node_seek_dfs (mime_node_t *node, int n);
 
+const _notmuch_message_crypto_t*
+mime_node_get_message_crypto_status (mime_node_t *node);
+
 typedef enum dump_formats {
     DUMP_FORMAT_AUTO,
     DUMP_FORMAT_BATCH_TAG,
-- 
2.20.1

[PATCH v2 3/4] mime-node: track whole-message crypto state while walking the tree

[Daniel Kahn Gillmor]

Deliberately populate the message's cryptographic status while walking
the MIME tree from the CLI.

Note that the additional numchild argument added to _mime_node_create
is a passthrough needed to be able to adequately populate the crypto
state object.
---
 mime-node.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 66ff7446..48b45247 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -135,6 +135,8 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
 	goto DONE;
     }
 
+    mctx->msg_crypto = _notmuch_message_crypto_new (mctx);
+
     mctx->crypto = crypto;
 
     /* Create the root node */
@@ -180,6 +182,7 @@ static void
 node_verify (mime_node_t *node, GMimeObject *part)
 {
     GError *err = NULL;
+    notmuch_status_t status;
 
     node->verify_attempted = true;
     node->sig_list = g_mime_multipart_signed_verify
@@ -193,6 +196,10 @@ node_verify (mime_node_t *node, GMimeObject *part)
 
     if (err)
 	g_error_free (err);
+
+    status = _notmuch_message_crypto_potential_sig_list(node->ctx->msg_crypto, node->sig_list);
+    if (status) /* this is a warning, not an error */
+	fprintf (stderr, "Warning: failed to note signature status: %s.\n", notmuch_status_to_string (status));
 }
 
 /* Decrypt and optionally verify an encrypted mime node */
@@ -201,6 +208,7 @@ 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;
 
@@ -223,6 +231,9 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
     }
 
     node->decrypt_success = true;
+    status = _notmuch_message_crypto_successful_decryption (node->ctx->msg_crypto);
+    if (status) /* this is a warning, not an error */
+	fprintf (stderr, "Warning: failed to note decryption status: %s.\n", notmuch_status_to_string (status));
 
     if (decrypt_result) {
 	/* This may be NULL if the part is not signed. */
@@ -231,6 +242,9 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
 	    node->verify_attempted = true;
 	    g_object_ref (node->sig_list);
 	    set_signature_list_destructor (node);
+	    status = _notmuch_message_crypto_potential_sig_list(node->ctx->msg_crypto, node->sig_list);
+	    if (status) /* this is a warning, not an error */
+		fprintf (stderr, "Warning: failed to note signature status: %s.\n", notmuch_status_to_string (status));
 	}
 
 	if (node->ctx->crypto->decrypt == NOTMUCH_DECRYPT_TRUE && message) {
@@ -251,9 +265,10 @@ node_decrypt_and_verify (mime_node_t *node, GMimeObject *part)
 }
 
 static mime_node_t *
-_mime_node_create (mime_node_t *parent, GMimeObject *part)
+_mime_node_create (mime_node_t *parent, GMimeObject *part, int numchild)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
+    notmuch_status_t status;
 
     /* Set basic node properties */
     node->part = part;
@@ -305,6 +320,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	} else {
 	    node_verify (node, part);
 	}
+    } else {
+	status = _notmuch_message_crypto_potential_payload (node->ctx->msg_crypto, part, parent ? parent->part : NULL, numchild);
+	if (status)
+	    fprintf (stderr, "Warning: failed to record potential crypto payload (%s).\n", notmuch_status_to_string (status));
     }
 
     return node;
@@ -332,7 +351,7 @@ mime_node_child (mime_node_t *parent, int child)
 	INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
 			g_type_name (G_OBJECT_TYPE (parent->part)));
     }
-    node = _mime_node_create (parent, sub);
+    node = _mime_node_create (parent, sub, child);
 
     if (child == parent->next_child && parent->next_part_num != -1) {
 	/* We're traversing in depth-first order.  Record the child's
-- 
2.20.1

[PATCH v2 4/4] cli/show: emit new whole-message crypto status output

[Daniel Kahn Gillmor]

This allows MUAs that don't want to think about per-mime-part
cryptographic status to have a simple high-level overview of the
message's cryptographic state.

Sensibly structured encrypted and/or signed messages will work fine
with this.  The only requirement for the simplest encryption + signing
is that the message have all of its encryption and signing protection
(the "cryptographic envelope") in a contiguous set of MIME layers at
the very outside of the message itself.

This is because messages with some subparts signed or encrypted, but
with other subparts with no cryptographic protection is very difficult
to reason about, and even harder for the user to make sense of or work
with.

For further characterization of the Cryptographic Envelope and some of
the usability tradeoffs, see here:

   https://dkg.fifthhorseman.net/blog/e-mail-cryptography.html#cryptographic-envelope
---
 devel/schemata      | 18 ++++++++++++++++++
 notmuch-show.c      | 29 +++++++++++++++++++++++++++++
 test/T350-crypto.sh | 17 +++++++++++++----
 test/T355-smime.sh  |  5 +++--
 4 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/devel/schemata b/devel/schemata
index 42b1bcf3..33633ab3 100644
--- a/devel/schemata
+++ b/devel/schemata
@@ -33,6 +33,8 @@ v3
 v4
 - replace signature error integer bitmask with a set of flags for
   individual errors.
+- (notmuch 0.29) added message.crypto to identify overall message
+  cryptographic state
 
 Common non-terminals
 --------------------
@@ -73,9 +75,25 @@ message = {
     tags:           [string*],
 
     headers:        headers,
+    crypto?:        crypto,   # omitted if crypto disabled, or if no part was signed or encrypted.
     body?:          [part]    # omitted if --body=false
 }
 
+# when showing the message, was any or all of it decrypted?
+msgdecstatus: "full"|"partial"
+
+# The overall cryptographic state of the message as a whole:
+crypto = {
+    signed?:    {
+                  status:      sigstatus,
+                  # was the set of signatures described under encrypted cover?
+                  encrypted:   bool,
+                },
+    decrypted?: {
+                  status: msgdecstatus,
+                }
+}
+
 # A MIME part (format_part_sprinter)
 part = {
     id:             int|string, # part id (currently DFS part number)
diff --git a/notmuch-show.c b/notmuch-show.c
index b95fc389..c5a814ad 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -628,6 +628,35 @@ format_part_sprinter (const void *ctx, sprinter_t *sp, mime_node_t *node,
 	    format_part_sprinter (ctx, sp, mime_node_child (node, 0), true, include_html);
 	    sp->end (sp);
 	}
+
+	if (notmuch_format_version >= 4) {
+	    const _notmuch_message_crypto_t *msg_crypto = mime_node_get_message_crypto_status (node);
+	    if (msg_crypto->sig_list ||
+		msg_crypto->decryption_status != NOTMUCH_MESSAGE_DECRYPTED_NONE) {
+		sp->map_key (sp, "crypto");
+		sp->begin_map (sp);
+		if (msg_crypto->sig_list) {
+		    sp->map_key (sp, "signed");
+		    sp->begin_map (sp);
+		    sp->map_key (sp, "status");
+		    format_part_sigstatus_sprinter (sp, msg_crypto->sig_list);
+		    if (msg_crypto->signature_encrypted) {
+			sp->map_key (sp, "encrypted");
+			sp->boolean (sp, msg_crypto->signature_encrypted);
+		    }
+		    sp->end (sp);
+		}
+		if (msg_crypto->decryption_status != NOTMUCH_MESSAGE_DECRYPTED_NONE) {
+		    sp->map_key (sp, "decrypted");
+		    sp->begin_map (sp);
+		    sp->map_key (sp, "status");
+		    sp->string (sp, msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_FULL ? "full" : "partial");
+		    sp->end (sp);
+		}
+		sp->end (sp);
+	    }
+	}
+
 	sp->end (sp);
 	return;
     }
diff --git a/test/T350-crypto.sh b/test/T350-crypto.sh
index 3539bafe..c3f8138e 100755
--- a/test/T350-crypto.sh
+++ b/test/T350-crypto.sh
@@ -25,7 +25,7 @@ test_expect_equal "$output" "thread:XXX   2000-01-01 [1/1] Notmuch Test Suite; t
 test_begin_subtest "signature verification"
 output=$(notmuch show --format=json --verify subject:"test signed message 001" \
     | notmuch_json_show_sanitize \
-    | sed -e 's|"created": [1234567890]*|"created": 946728000|')
+    | sed -e 's|"created": [1234567890]*|"created": 946728000|g')
 expected='[[[{"id": "XXXXX",
  "match": true,
  "excluded": false,
@@ -33,6 +33,7 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["inbox","signed"],
+ "crypto": {"signed": {"status": [{ "status": "good", "created": 946728000, "fingerprint": "'$FINGERPRINT'", "userid": "'"$SELF_USERID"'"}]}},
  "headers": {"Subject": "test signed message 001",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
@@ -74,6 +75,7 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["inbox","signed"],
+ "crypto": {"signed": {"status": [{ "status": "bad", "keyid": "'$(echo $FINGERPRINT | cut -c 25-)'"}]}},
  "headers": {"Subject": "bad signed message 001",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
@@ -143,7 +145,7 @@ gpg --quiet --batch --no-tty --export-ownertrust > "$GNUPGHOME/ownertrust.bak"
 echo "${FINGERPRINT}:3:" | gpg --quiet --batch --no-tty --import-ownertrust
 output=$(notmuch show --format=json --verify subject:"test signed message 001" \
     | notmuch_json_show_sanitize \
-    | sed -e 's|"created": [1234567890]*|"created": 946728000|')
+    | sed -e 's|"created": [1234567890]*|"created": 946728000|g')
 expected='[[[{"id": "XXXXX",
  "match": true,
  "excluded": false,
@@ -151,6 +153,7 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["inbox","signed"],
+ "crypto": {"signed": {"status": [{ "status": "good", "created": 946728000, "fingerprint": "'$FINGERPRINT'"}]}},
  "headers": {"Subject": "test signed message 001",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
@@ -177,7 +180,7 @@ test_begin_subtest "signature verification with signer key unavailable"
 mv "${GNUPGHOME}"{,.bak}
 output=$(notmuch show --format=json --verify subject:"test signed message 001" \
     | notmuch_json_show_sanitize \
-    | sed -e 's|"created": [1234567890]*|"created": 946728000|')
+    | sed -e 's|"created": [1234567890]*|"created": 946728000|g')
 expected='[[[{"id": "XXXXX",
  "match": true,
  "excluded": false,
@@ -185,6 +188,7 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["inbox","signed"],
+ "crypto": {"signed": {"status": [{"errors": {"key-missing": true}, "keyid": "'$(echo $FINGERPRINT | cut -c 25-)'", "status": "error"}]}},
  "headers": {"Subject": "test signed message 001",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
@@ -264,6 +268,7 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["encrypted","inbox"],
+ "crypto": {"decrypted": {"status": "full"}},
  "headers": {"Subject": "test encrypted message 001",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
@@ -350,7 +355,7 @@ test_expect_success \
 test_begin_subtest "decryption + signature verification"
 output=$(notmuch show --format=json --decrypt=true subject:"test encrypted message 002" \
     | notmuch_json_show_sanitize \
-    | sed -e 's|"created": [1234567890]*|"created": 946728000|')
+    | sed -e 's|"created": [1234567890]*|"created": 946728000|g')
 expected='[[[{"id": "XXXXX",
  "match": true,
  "excluded": false,
@@ -358,6 +363,9 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["encrypted","inbox"],
+ "crypto": {"signed": {"status": [{ "status": "good", "created": 946728000, "fingerprint": "'$FINGERPRINT'", "userid": "'"$SELF_USERID"'"}],
+                       "encrypted": true },
+            "decrypted": {"status": "full"}},
  "headers": {"Subject": "test encrypted message 002",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
@@ -433,6 +441,7 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["inbox","signed"],
+ "crypto": {"signed": {"status": [{"errors": {"key-revoked": true}, "keyid": "'$(echo $FINGERPRINT | cut -c 25-)'", "status": "error"}]}},
  "headers": {"Subject": "test signed message 001",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
diff --git a/test/T355-smime.sh b/test/T355-smime.sh
index e410286b..336da917 100755
--- a/test/T355-smime.sh
+++ b/test/T355-smime.sh
@@ -50,8 +50,8 @@ test_expect_equal_file EXPECTED OUTPUT
 test_begin_subtest "signature verification (notmuch CLI)"
 output=$(notmuch show --format=json --verify subject:"test signed message 001" \
     | notmuch_json_show_sanitize \
-    | sed -e 's|"created": [-1234567890]*|"created": 946728000|' \
-	  -e 's|"expires": [-1234567890]*|"expires": 424242424|' )
+    | sed -e 's|"created": [-1234567890]*|"created": 946728000|g' \
+	  -e 's|"expires": [-1234567890]*|"expires": 424242424|g' )
 expected='[[[{"id": "XXXXX",
  "match": true,
  "excluded": false,
@@ -59,6 +59,7 @@ expected='[[[{"id": "XXXXX",
  "timestamp": 946728000,
  "date_relative": "2000-01-01",
  "tags": ["inbox","signed"],
+ "crypto": {"signed": {"status": [{"fingerprint": "'$FINGERPRINT'", "status": "good","userid": "CN=Notmuch Test Suite","expires": 424242424, "created": 946728000}]}},
  "headers": {"Subject": "test signed message 001",
  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
  "To": "test_suite@notmuchmail.org",
-- 
2.20.1

Re: [PATCH v2 1/4] util/crypto: _notmuch_message_crypto: tracks message-wide crypto state

[David Bremner]

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

> +static int
> +_notmuch_message_crypto_cleanup (_notmuch_message_crypto_t *msg_crypto)
> +{
> +    if (!msg_crypto)
> +	return 0;
> +    if (msg_crypto->sig_list)
> +	g_object_unref (msg_crypto->sig_list);
> +    return 0;
> +}

we currently call destructors

   - *_destroy
   - *_destructor (the most popular option)
   - *_free

Is there a good reason to introduce a fourth option?

> +notmuch_status_t
> +_notmuch_message_crypto_potential_sig_list (_notmuch_message_crypto_t *msg_crypto, GMimeSignatureList *sigs)
> +{
> +    if (!msg_crypto)
> +	return NOTMUCH_STATUS_NULL_POINTER;
> +
> +    /* Signatures that arrive after a payload part during DFS are not
> +     * part of the cryptographic envelope: */
> +    if (msg_crypto->payload_encountered)
> +	return NOTMUCH_STATUS_SUCCESS;
> +
> +    if (msg_crypto->sig_list)
> +	g_object_unref (msg_crypto->sig_list);
> +
> +    msg_crypto->sig_list = sigs;
> +    if (sigs)
> +	g_object_ref (sigs);

It might be worth a comment here to explain the interaction between
talloc and glib, i.e. why this is needed.

> +
> +
> +notmuch_status_t
> +_notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_crypto)
> +{
> +    if (!msg_crypto)
> +	return NOTMUCH_STATUS_NULL_POINTER;
> +
> +    if (!msg_crypto->payload_encountered)
> +	msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_FULL;
> +    else if (msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_NONE)
> +	msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_PARTIAL;
> +

The logic / purpose of this is not very clear without referring to the header.

> +/* The user probably wants to know if the entire message was in the
> + * clear.  When replying, the MUA probably wants to know whether there
> + * was any part decrypted in the message.  And when displaying to the
> + * user, we probably only want to display "encrypted message" if the
> + * entire message was covered by encryption. */

I know you've discussed this elsewhere, but do you have some sense that
most clients are generating encrypted messages that are "well formed" in
the sense of the entire message being covered by encryption? It might be
good to have some messages from enigmail etc... in the test suite when
we merge this change.

Re: [PATCH v2 4/4] cli/show: emit new whole-message crypto status output

[David Bremner]

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

>      headers:        headers,
> +    crypto?:        crypto,   # omitted if crypto disabled, or if no part was signed or encrypted.
>      body?:          [part]    # omitted if --body=false
>  }

I'm wondering about the "upward compatible" aspect of this. If the
crypto key is ommitted, a client doesn't know whether to interpret that
as no part was signed or encrypted, or just an older version of notmuch.

d

Re: [PATCH v2 4/4] cli/show: emit new whole-message crypto status output

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Thu 2019-05-23 07:50:43 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>>      headers:        headers,
>> +    crypto?:        crypto,   # omitted if crypto disabled, or if no part was signed or encrypted.
>>      body?:          [part]    # omitted if --body=false
>>  }
>
> I'm wondering about the "upward compatible" aspect of this. If the
> crypto key is ommitted, a client doesn't know whether to interpret that
> as no part was signed or encrypted, or just an older version of notmuch.

I understand your concern here.  Would making
notmuch_built_with("message_crypto_summary") return true solve the
problem?

        --dkg

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

Re: [PATCH v2 1/4] util/crypto: _notmuch_message_crypto: tracks message-wide crypto state

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 3880 bytes --]

On Wed 2019-05-22 09:18:53 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> +static int
>> +_notmuch_message_crypto_cleanup (_notmuch_message_crypto_t *msg_crypto)
>> +{
>> +    if (!msg_crypto)
>> +	return 0;
>> +    if (msg_crypto->sig_list)
>> +	g_object_unref (msg_crypto->sig_list);
>> +    return 0;
>> +}
>
> we currently call destructors
>
>    - *_destroy
>    - *_destructor (the most popular option)
>    - *_free
>
> Is there a good reason to introduce a fourth option?

nope.  I'm happy to stick with _destructor if you prefer it.

>> +notmuch_status_t
>> +_notmuch_message_crypto_potential_sig_list (_notmuch_message_crypto_t *msg_crypto, GMimeSignatureList *sigs)
>> +{
>> +    if (!msg_crypto)
>> +	return NOTMUCH_STATUS_NULL_POINTER;
>> +
>> +    /* Signatures that arrive after a payload part during DFS are not
>> +     * part of the cryptographic envelope: */
>> +    if (msg_crypto->payload_encountered)
>> +	return NOTMUCH_STATUS_SUCCESS;
>> +
>> +    if (msg_crypto->sig_list)
>> +	g_object_unref (msg_crypto->sig_list);
>> +
>> +    msg_crypto->sig_list = sigs;
>> +    if (sigs)
>> +	g_object_ref (sigs);
>
> It might be worth a comment here to explain the interaction between
> talloc and glib, i.e. why this is needed.

OK, it'll be something like:

/ * This signature list needs to persist as long as the _n_m_crypto
  * object survives. Increasing its reference counter prevents
  * garbage-collection until after _n_m_crypto_destructor is called. */


>> +
>> +
>> +notmuch_status_t
>> +_notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t *msg_crypto)
>> +{
>> +    if (!msg_crypto)
>> +	return NOTMUCH_STATUS_NULL_POINTER;
>> +
>> +    if (!msg_crypto->payload_encountered)
>> +	msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_FULL;
>> +    else if (msg_crypto->decryption_status == NOTMUCH_MESSAGE_DECRYPTED_NONE)
>> +	msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_PARTIAL;
>> +
>
> The logic / purpose of this is not very clear without referring to the header.
>
>> +/* The user probably wants to know if the entire message was in the
>> + * clear.  When replying, the MUA probably wants to know whether there
>> + * was any part decrypted in the message.  And when displaying to the
>> + * user, we probably only want to display "encrypted message" if the
>> + * entire message was covered by encryption. */

do you think i should move this explanation into the .c file instead of
the header?  I think it's more important that it be visible to someone
saying "what are these statuses?"  I could copy the text into the .c
file as well, but then i worry about keeping it in sync.  Maybe just a
reference is sufficient?

> I know you've discussed this elsewhere, but do you have some sense that
> most clients are generating encrypted messages that are "well formed" in
> the sense of the entire message being covered by encryption? It might be
> good to have some messages from enigmail etc... in the test suite when
> we merge this change.

Yes, PGP/MIME encrypted messages and signed from enigmail well-formed in
this sense.  However, some mail transport agents (including mailman!)
mangle them in ways that may change the well-formedness.  (see
https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling
for more discussion on the topic of mangled messages; i plan to submit
some patches to notmuch related to that work soon)

My approach thus far around building the corpus of
cryptographically-protected e-mail has been to keep the examples
deliberately minimalist, so that they can be understood by someone
taking them apart and inspecting by hand.

If someone wants a trove of real-world messy e-mails i certainly
wouldn't object to that (indeed, i'd be happy to help!), but i don't
think it should be a blocker to land this series.

        --dkg

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

Re: [PATCH v2 4/4] cli/show: emit new whole-message crypto status output

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Fri 2019-05-24 16:09:38 -0400, Daniel Kahn Gillmor wrote:
> On Thu 2019-05-23 07:50:43 -0300, David Bremner wrote:
>> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>>
>>>      headers:        headers,
>>> +    crypto?:        crypto,   # omitted if crypto disabled, or if no part was signed or encrypted.
>>>      body?:          [part]    # omitted if --body=false
>>>  }
>>
>> I'm wondering about the "upward compatible" aspect of this. If the
>> crypto key is ommitted, a client doesn't know whether to interpret that
>> as no part was signed or encrypted, or just an older version of notmuch.
>
> I understand your concern here.  Would making
> notmuch_built_with("message_crypto_summary") return true solve the
> problem?

or, should we just make the crypto member always present?

Another alternative is to condition the presence of the crypto member on
the arguments (like if --verify is set), but the decision for --decrypt
is a bit awkward because of our default of --decrypt=auto.

          --dkg

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

Re: [PATCH v2 1/4] util/crypto: _notmuch_message_crypto: tracks message-wide crypto state

[David Bremner]

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

> On Wed 2019-05-22 09:18:53 -0300, David Bremner wrote:
>> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>>
>>> +static int
>>> +_notmuch_message_crypto_cleanup (_notmuch_message_crypto_t *msg_crypto)
>>> +{
>>> +    if (!msg_crypto)
>>> +	return 0;
>>> +    if (msg_crypto->sig_list)
>>> +	g_object_unref (msg_crypto->sig_list);
>>> +    return 0;
>>> +}
>>
>> we currently call destructors
>>
>>    - *_destroy
>>    - *_destructor (the most popular option)
>>    - *_free
>>
>> Is there a good reason to introduce a fourth option?
>
> nope.  I'm happy to stick with _destructor if you prefer it.

Sounds good.

>>
>> It might be worth a comment here to explain the interaction between
>> talloc and glib, i.e. why this is needed.
>
> OK, it'll be something like:
>
> / * This signature list needs to persist as long as the _n_m_crypto
>   * object survives. Increasing its reference counter prevents
>   * garbage-collection until after _n_m_crypto_destructor is called. */

Yep, sounds good. I only suggest it because I had to learn this stuff
the hard way.

> do you think i should move this explanation into the .c file instead of
> the header?  I think it's more important that it be visible to someone
> saying "what are these statuses?"  I could copy the text into the .c
> file as well, but then i worry about keeping it in sync.  Maybe just a
> reference is sufficient?

A reference is fine.
>
> Yes, PGP/MIME encrypted messages and signed from enigmail well-formed in
> this sense.  However, some mail transport agents (including mailman!)
> mangle them in ways that may change the well-formedness.  (see
> https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling
> for more discussion on the topic of mangled messages; i plan to submit
> some patches to notmuch related to that work soon)
>
> My approach thus far around building the corpus of
> cryptographically-protected e-mail has been to keep the examples
> deliberately minimalist, so that they can be understood by someone
> taking them apart and inspecting by hand.
>
> If someone wants a trove of real-world messy e-mails i certainly
> wouldn't object to that (indeed, i'd be happy to help!), but i don't
> think it should be a blocker to land this series.

OK.

Re: [PATCH v2 4/4] cli/show: emit new whole-message crypto status output

[David Bremner]

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

>
> or, should we just make the crypto member always present?
>

That was what I was thinking, sorry to be less explicit than I could
have been.

> Another alternative is to condition the presence of the crypto member on
> the arguments (like if --verify is set), but the decision for --decrypt
> is a bit awkward because of our default of --decrypt=auto.
>