Allow indexing cleartext of encrypted messages (v3)

Allow indexing cleartext of encrypted messages (v3)

[Daniel Kahn Gillmor]

This is the third draft of the series initially announced in
id:1449718786-28000-1-git-send-email-dkg@fifthhorseman.net (second
draft was in
id:1453258369-7366-1-git-send-email-dkg@fifthhorseman.net).  It
differs from v2 in that it incorporates the recent improvements in
detecting and processing S/MIME signatures.

From the v2 description:

> Notmuch currently doesn't index the cleartext of encrypted mail.  This
> is the right choice by default, because the index is basically
> cleartext-equivalent, and we wouldn't want every indexed mailstore to
> leak the contents of its encrypted mails.
> 
> However, if a notmuch user has their index in a protected location,
> they may prefer the convenience of being able to search the contents
> of (at least some of) their encrypted mail.
> 
> This series of patches enables notmuch to index the cleartext of
> specific encrypted messages when they're being added via "notmuch new"
> or "notmuch insert", via a new --try-decrypt flag.
> 
> If --try-decrypt is used, and decryption is successful for part of a
> message, the message gets an additional "index-decrypted" tag.  If
> decryption of part of a message fails, the message gets an additional
> "index-decryption-failed" tag.

v2 addresses the concerns raised from the helpful feedback on the
previous series, and adds a notmuch_indexopts_t object that can be
used to declare options for indexing messages, including a
"try_decrypt" boolean.

Additionally, this series adds a new function to libnotmuch:

  notmuch_message_reindex (notmuch_message_t *message,
                           notmuch_indexopts_t *indexopts)

Which allows user of the library to adjust the indexing options of a
given message.

The CLI is additionally augmented with a new notmuch subcommand,
"notmuch reindex", which also has a --try-decrypt flag.

So a user who has their message index stored securely and wants to
index the cleartext of all encrypted messages they've received can do
something like:

  notmuch reindex --try-decrypt tag:encrypted and not tag:index-decrypted

Or can clear all indexed cleartext from their database with:

  notmuch reindex tag:encrypted and tag:index-decrypted

[PATCH v3 01/16] add util/search-path.{c, h} to test for executables in $PATH

[Daniel Kahn Gillmor]

This is a utility function we can use to see whether an executable is
available.
---
 util/Makefile.local |  2 +-
 util/search-path.c  | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 util/search-path.h  | 24 +++++++++++++++++++++++
 3 files changed, 80 insertions(+), 1 deletion(-)
 create mode 100644 util/search-path.c
 create mode 100644 util/search-path.h

diff --git a/util/Makefile.local b/util/Makefile.local
index 905f237..8b2b91b 100644
--- a/util/Makefile.local
+++ b/util/Makefile.local
@@ -5,7 +5,7 @@ extra_cflags += -I$(srcdir)/$(dir)
 
 libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
 		  $(dir)/string-util.c $(dir)/talloc-extra.c $(dir)/zlib-extra.c \
-		$(dir)/util.c
+		$(dir)/util.c $(dir)/search-path.c
 
 libutil_modules := $(libutil_c_srcs:.c=.o)
 
diff --git a/util/search-path.c b/util/search-path.c
new file mode 100644
index 0000000..5eac367
--- /dev/null
+++ b/util/search-path.c
@@ -0,0 +1,55 @@
+#include "search-path.h"
+#include <stdlib.h>
+#include <talloc.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+
+notmuch_bool_t
+test_for_executable(const char* exename)
+{
+    char *c = NULL, *save = NULL, *tok;
+    size_t n;
+    int dfd = -1;
+    notmuch_bool_t ret = FALSE;
+
+    if (strchr(exename, '/')) {
+	if (0 == access(exename, X_OK))
+	    return TRUE;
+	else
+	    return FALSE;
+    }
+    
+    c = getenv("PATH");
+    if (c)
+	c = talloc_strdup(NULL, c);
+    else {
+	n = confstr(_CS_PATH, NULL, 0);
+	c = (char*)talloc_size(NULL, n);
+	if (!c)
+	    return FALSE;
+	confstr(_CS_PATH, c, n);
+    }
+
+    tok = strtok_r(c, ":", &save);
+    while (tok) {
+	dfd = open(tok, O_DIRECTORY | O_RDONLY);
+	if (dfd != -1) {
+	    if (!faccessat(dfd, exename, X_OK, 0)) {
+		ret = TRUE;
+		goto done;
+	    }
+	    close(dfd);
+	}
+	tok = strtok_r(NULL, ":", &save);
+    }
+done:
+    if (dfd != -1)
+	close(dfd);
+    if (c)
+	talloc_free(c);
+    return ret;
+}
diff --git a/util/search-path.h b/util/search-path.h
new file mode 100644
index 0000000..727d0b3
--- /dev/null
+++ b/util/search-path.h
@@ -0,0 +1,24 @@
+#ifndef _SEARCH_PATH_H
+#define _SEARCH_PATH_H
+
+#include "notmuch.h"
+
+/* can an executable be found with the given name?
+ * 
+ * Return TRUE only if we can find something to execute with the
+ * associated name.
+ *
+ * if the name has a '/' in it, we look for it directly with
+ * access(exename, X_OK).
+ * 
+ * otherwise, we look for it in $PATH (or in confstr(_CS_PATH), if
+ * $PATH is unset).
+ *
+ * This should match the logic for execvp (as well as matching user
+ * expectations, hopefully).
+ */
+
+notmuch_bool_t
+test_for_executable(const char *exename);
+
+#endif
-- 
2.7.0.rc3

[PATCH v3 02/16] Move crypto.c into libutil

[Daniel Kahn Gillmor]

This prepares us for using the crypto object in both the library and
the client.

i've prefixed notmuch_crypto with _ to indicate that while this can be
built into the library when needed, it's not something to be exported
or used externally.
---
 Makefile.local      |   1 -
 crypto.c            | 134 --------------------------------------------------
 mime-node.c         |  12 ++---
 notmuch-client.h    |  23 ++-------
 notmuch-reply.c     |   2 +-
 notmuch-show.c      |   2 +-
 util/Makefile.local |   2 +-
 util/crypto.c       | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 util/crypto.h       |  25 ++++++++++
 9 files changed, 175 insertions(+), 164 deletions(-)
 delete mode 100644 crypto.c
 create mode 100644 util/crypto.c
 create mode 100644 util/crypto.h

diff --git a/Makefile.local b/Makefile.local
index 066ecf2..6206771 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -293,7 +293,6 @@ notmuch_client_srcs =		\
 	sprinter-text.c		\
 	query-string.c		\
 	mime-node.c		\
-	crypto.c		\
 	tag-util.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
diff --git a/crypto.c b/crypto.c
deleted file mode 100644
index 3dabc97..0000000
--- a/crypto.c
+++ /dev/null
@@ -1,134 +0,0 @@
-/* notmuch - Not much of an email program, (just index and search)
- *
- * Copyright © 2012 Jameson Rollins
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see http://www.gnu.org/licenses/ .
- *
- * Authors: Jameson Rollins <jrollins@finestructure.net>
- */
-
-#include "notmuch-client.h"
-
-/* Create a GPG context (GMime 2.6) */
-static notmuch_crypto_context_t *
-create_gpg_context (notmuch_crypto_t *crypto)
-{
-    notmuch_crypto_context_t *gpgctx;
-
-    if (crypto->gpgctx)
-	return crypto->gpgctx;
-
-    /* TODO: GMimePasswordRequestFunc */
-    gpgctx = g_mime_gpg_context_new (NULL, crypto->gpgpath ? crypto->gpgpath : "gpg");
-    if (! gpgctx) {
-	fprintf (stderr, "Failed to construct gpg context.\n");
-	return NULL;
-    }
-    crypto->gpgctx = gpgctx;
-
-    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) gpgctx, TRUE);
-    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
-
-    return gpgctx;
-}
-
-/* Create a PKCS7 context (GMime 2.6) */
-static notmuch_crypto_context_t *
-create_pkcs7_context (notmuch_crypto_t *crypto)
-{
-    notmuch_crypto_context_t *pkcs7ctx;
-
-    if (crypto->pkcs7ctx)
-	return crypto->pkcs7ctx;
-
-    /* TODO: GMimePasswordRequestFunc */
-    pkcs7ctx = g_mime_pkcs7_context_new (NULL);
-    if (! pkcs7ctx) {
-	fprintf (stderr, "Failed to construct pkcs7 context.\n");
-	return NULL;
-    }
-    crypto->pkcs7ctx = pkcs7ctx;
-
-    g_mime_pkcs7_context_set_always_trust ((GMimePkcs7Context *) pkcs7ctx,
-					   FALSE);
-
-    return pkcs7ctx;
-}
-static const struct {
-    const char *protocol;
-    notmuch_crypto_context_t *(*get_context) (notmuch_crypto_t *crypto);
-} protocols[] = {
-    {
-	.protocol = "application/pgp-signature",
-	.get_context = create_gpg_context,
-    },
-    {
-	.protocol = "application/pgp-encrypted",
-	.get_context = create_gpg_context,
-    },
-    {
-	.protocol = "application/pkcs7-signature",
-	.get_context = create_pkcs7_context,
-    },
-    {
-	.protocol = "application/x-pkcs7-signature",
-	.get_context = create_pkcs7_context,
-    },
-};
-
-/* for the specified protocol return the context pointer (initializing
- * if needed) */
-notmuch_crypto_context_t *
-notmuch_crypto_get_context (notmuch_crypto_t *crypto, const char *protocol)
-{
-    notmuch_crypto_context_t *cryptoctx = NULL;
-    size_t i;
-
-    if (! protocol) {
-	fprintf (stderr, "Cryptographic protocol is empty.\n");
-	return cryptoctx;
-    }
-
-    /* As per RFC 1847 section 2.1: "the [protocol] value token is
-     * comprised of the type and sub-type tokens of the Content-Type".
-     * As per RFC 1521 section 2: "Content-Type values, subtypes, and
-     * parameter names as defined in this document are
-     * case-insensitive."  Thus, we use strcasecmp for the protocol.
-     */
-    for (i = 0; i < ARRAY_SIZE (protocols); i++) {
-	if (strcasecmp (protocol, protocols[i].protocol) == 0)
-	    return protocols[i].get_context (crypto);
-    }
-
-    fprintf (stderr, "Unknown or unsupported cryptographic protocol %s.\n",
-	     protocol);
-
-    return NULL;
-}
-
-int
-notmuch_crypto_cleanup (notmuch_crypto_t *crypto)
-{
-    if (crypto->gpgctx) {
-	g_object_unref (crypto->gpgctx);
-	crypto->gpgctx = NULL;
-    }
-
-    if (crypto->pkcs7ctx) {
-	g_object_unref (crypto->pkcs7ctx);
-	crypto->pkcs7ctx = NULL;
-    }
-
-    return 0;
-}
diff --git a/mime-node.c b/mime-node.c
index e96e663..a8f5670 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -33,7 +33,7 @@ typedef struct mime_node_context {
     GMimeMessage *mime_message;
 
     /* Context provided by the caller. */
-    notmuch_crypto_t *crypto;
+    _notmuch_crypto_t *crypto;
 } mime_node_context_t;
 
 static int
@@ -56,7 +56,7 @@ _mime_node_context_free (mime_node_context_t *res)
 
 notmuch_status_t
 mime_node_open (const void *ctx, notmuch_message_t *message,
-		notmuch_crypto_t *crypto, mime_node_t **root_out)
+		_notmuch_crypto_t *crypto, mime_node_t **root_out)
 {
     const char *filename = notmuch_message_get_filename (message);
     mime_node_context_t *mctx;
@@ -151,7 +151,7 @@ set_signature_list_destructor (mime_node_t *node)
 /* Verify a signed mime node (GMime 2.6) */
 static void
 node_verify (mime_node_t *node, GMimeObject *part,
-	     notmuch_crypto_context_t *cryptoctx)
+	     GMimeCryptoContext *cryptoctx)
 {
     GError *err = NULL;
 
@@ -172,7 +172,7 @@ node_verify (mime_node_t *node, GMimeObject *part,
 /* Decrypt and optionally verify an encrypted mime node (GMime 2.6) */
 static void
 node_decrypt_and_verify (mime_node_t *node, GMimeObject *part,
-			 notmuch_crypto_context_t *cryptoctx)
+			 GMimeCryptoContext *cryptoctx)
 {
     GError *err = NULL;
     GMimeDecryptResult *decrypt_result = NULL;
@@ -207,7 +207,7 @@ static mime_node_t *
 _mime_node_create (mime_node_t *parent, GMimeObject *part)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
-    notmuch_crypto_context_t *cryptoctx = NULL;
+    GMimeCryptoContext *cryptoctx = NULL;
 
     /* Set basic node properties */
     node->part = part;
@@ -244,7 +244,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	|| (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->verify)) {
 	GMimeContentType *content_type = g_mime_object_get_content_type (part);
 	const char *protocol = g_mime_content_type_get_parameter (content_type, "protocol");
-	cryptoctx = notmuch_crypto_get_context (node->ctx->crypto, protocol);
+	cryptoctx = _notmuch_crypto_get_gmime_context (node->ctx->crypto, protocol);
     }
 
     /* Handle PGP/MIME parts */
diff --git a/notmuch-client.h b/notmuch-client.h
index 18e6c60..a41e90a 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -30,10 +30,6 @@
 
 #include <gmime/gmime.h>
 
-typedef GMimeCryptoContext notmuch_crypto_context_t;
-/* This is automatically included only since gmime 2.6.10 */
-#include <gmime/gmime-pkcs7-context.h>
-
 #include "notmuch.h"
 
 /* This is separate from notmuch-private.h because we're trying to
@@ -53,6 +49,7 @@ typedef GMimeCryptoContext notmuch_crypto_context_t;
 #include <ctype.h>
 
 #include "talloc-extra.h"
+#include "crypto.h"
 
 #define unused(x) x __attribute__ ((unused))
 
@@ -70,21 +67,13 @@ typedef struct notmuch_show_format {
 			      const struct notmuch_show_params *params);
 } notmuch_show_format_t;
 
-typedef struct notmuch_crypto {
-    notmuch_crypto_context_t* gpgctx;
-    notmuch_crypto_context_t* pkcs7ctx;
-    notmuch_bool_t verify;
-    notmuch_bool_t decrypt;
-    const char *gpgpath;
-} notmuch_crypto_t;
-
 typedef struct notmuch_show_params {
     notmuch_bool_t entire_thread;
     notmuch_bool_t omit_excluded;
     notmuch_bool_t output_body;
     notmuch_bool_t raw;
     int part;
-    notmuch_crypto_t crypto;
+    _notmuch_crypto_t crypto;
     notmuch_bool_t include_html;
 } notmuch_show_params_t;
 
@@ -167,12 +156,6 @@ typedef struct _notmuch_config notmuch_config_t;
 void
 notmuch_exit_if_unsupported_format (void);
 
-notmuch_crypto_context_t *
-notmuch_crypto_get_context (notmuch_crypto_t *crypto, const char *protocol);
-
-int
-notmuch_crypto_cleanup (notmuch_crypto_t *crypto);
-
 int
 notmuch_count_command (notmuch_config_t *config, int argc, char *argv[]);
 
@@ -423,7 +406,7 @@ struct mime_node {
  */
 notmuch_status_t
 mime_node_open (const void *ctx, notmuch_message_t *message,
-		notmuch_crypto_t *crypto, mime_node_t **node_out);
+		_notmuch_crypto_t *crypto, mime_node_t **node_out);
 
 /* Return a new MIME node for the requested child part of parent.
  * parent will be used as the talloc context for the returned child
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 6df54fc..eccfb32 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -862,7 +862,7 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
     if (reply_format_func (config, config, query, &params, reply_all, sp) != 0)
 	return EXIT_FAILURE;
 
-    notmuch_crypto_cleanup (&params.crypto);
+    _notmuch_crypto_cleanup (&params.crypto);
     notmuch_query_destroy (query);
     notmuch_database_destroy (notmuch);
 
diff --git a/notmuch-show.c b/notmuch-show.c
index 87e52bb..3c91ece 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1171,7 +1171,7 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	ret = do_show (config, query, format, sprinter, &params);
     }
 
-    notmuch_crypto_cleanup (&params.crypto);
+    _notmuch_crypto_cleanup (&params.crypto);
     notmuch_query_destroy (query);
     notmuch_database_destroy (notmuch);
 
diff --git a/util/Makefile.local b/util/Makefile.local
index 8b2b91b..7590618 100644
--- a/util/Makefile.local
+++ b/util/Makefile.local
@@ -5,7 +5,7 @@ extra_cflags += -I$(srcdir)/$(dir)
 
 libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
 		  $(dir)/string-util.c $(dir)/talloc-extra.c $(dir)/zlib-extra.c \
-		$(dir)/util.c $(dir)/search-path.c
+		$(dir)/util.c $(dir)/search-path.c $(dir)/crypto.c
 
 libutil_modules := $(libutil_c_srcs:.c=.o)
 
diff --git a/util/crypto.c b/util/crypto.c
new file mode 100644
index 0000000..eab12a3
--- /dev/null
+++ b/util/crypto.c
@@ -0,0 +1,138 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2012 Jameson Rollins
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Authors: Jameson Rollins <jrollins@finestructure.net>
+ *          Daniel Kahn Gillmor <dkg@fifthhorseman.net>
+ */
+
+#include "notmuch.h"
+#include "crypto.h"
+#include <string.h>
+
+#define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
+
+/* Create a GPG context (GMime 2.6) */
+static GMimeCryptoContext*
+create_gpg_context (_notmuch_crypto_t *crypto)
+{
+    GMimeCryptoContext *gpgctx;
+
+    if (crypto->gpgctx) {
+	return crypto->gpgctx;
+    }
+
+    /* TODO: GMimePasswordRequestFunc */
+    gpgctx = g_mime_gpg_context_new (NULL, crypto->gpgpath ? crypto->gpgpath : "gpg");
+    if (! gpgctx) {
+	fprintf (stderr, "Failed to construct gpg context.\n");
+	return NULL;
+    }
+    crypto->gpgctx = gpgctx;
+
+    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) gpgctx, TRUE);
+    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
+
+    return crypto->gpgctx;
+}
+
+/* Create a PKCS7 context (GMime 2.6) */
+static notmuch_crypto_context_t *
+create_pkcs7_context (notmuch_crypto_t *crypto)
+{
+    notmuch_crypto_context_t *pkcs7ctx;
+
+    if (crypto->pkcs7ctx)
+	return crypto->pkcs7ctx;
+
+    /* TODO: GMimePasswordRequestFunc */
+    pkcs7ctx = g_mime_pkcs7_context_new (NULL);
+    if (! pkcs7ctx) {
+	fprintf (stderr, "Failed to construct pkcs7 context.\n");
+	return NULL;
+    }
+    crypto->pkcs7ctx = pkcs7ctx;
+
+    g_mime_pkcs7_context_set_always_trust ((GMimePkcs7Context *) pkcs7ctx,
+					   FALSE);
+
+    return crypto->pkcs7ctx;
+}
+static const struct {
+    const char *protocol;
+    GMimeCryptoContext *(*get_context) (_notmuch_crypto_t *crypto);
+} protocols[] = {
+    {
+	.protocol = "application/pgp-signature",
+	.get_context = create_gpg_context,
+    },
+    {
+	.protocol = "application/pgp-encrypted",
+	.get_context = create_gpg_context,
+    },
+    {
+	.protocol = "application/pkcs7-signature",
+	.get_context = create_pkcs7_context,
+    },
+    {
+	.protocol = "application/x-pkcs7-signature",
+	.get_context = create_pkcs7_context,
+    },
+};
+
+/* for the specified protocol return the context pointer (initializing
+ * if needed) */
+GMimeCryptoContext *
+_notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protocol)
+{
+    GMimeCryptoContext *cryptoctx = NULL;
+    size_t i;
+
+    if (! protocol) {
+	fprintf (stderr, "Cryptographic protocol is empty.\n");
+	return cryptoctx;
+    }
+
+    /* As per RFC 1847 section 2.1: "the [protocol] value token is
+     * comprised of the type and sub-type tokens of the Content-Type".
+     * As per RFC 1521 section 2: "Content-Type values, subtypes, and
+     * parameter names as defined in this document are
+     * case-insensitive."  Thus, we use strcasecmp for the protocol.
+     */
+    for (i = 0; i < ARRAY_SIZE (protocols); i++) {
+	if (strcasecmp (protocol, protocols[i].protocol) == 0)
+	    return protocols[i].get_context (crypto);
+    }
+
+    fprintf (stderr, "Unknown or unsupported cryptographic protocol %s.\n",
+	     protocol);
+
+    return NULL;
+}
+
+void
+_notmuch_crypto_cleanup (_notmuch_crypto_t *crypto)
+{
+    if (crypto->gpgctx) {
+	g_object_unref (crypto->gpgctx);
+	crypto->gpgctx = NULL;
+    }
+
+    if (crypto->pkcs7ctx) {
+	g_object_unref (crypto->pkcs7ctx);
+	crypto->pkcs7ctx = NULL;
+    }
+}
diff --git a/util/crypto.h b/util/crypto.h
new file mode 100644
index 0000000..d4a51e8
--- /dev/null
+++ b/util/crypto.h
@@ -0,0 +1,25 @@
+#ifndef _CRYPTO_H
+#define _CRYPTO_H
+
+#include "notmuch.h"
+#include <gmime/gmime.h>
+/* This is automatically included only since gmime 2.6.10 */
+#include <gmime/gmime-pkcs7-context.h>
+
+typedef struct _notmuch_crypto {
+    GMimeCryptoContext* gpgctx;
+    GMimeCryptoContext* pkcs7ctx;
+    notmuch_bool_t verify;
+    notmuch_bool_t decrypt;
+    const char *gpgpath;
+} _notmuch_crypto_t;
+
+
+GMimeCryptoContext *
+_notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protocol);
+
+void
+_notmuch_crypto_cleanup (_notmuch_crypto_t *crypto);
+
+
+#endif
-- 
2.7.0.rc3

[PATCH v3 03/16] make shared crypto code behave library-like

[Daniel Kahn Gillmor]

If we're going to reuse the crypto code across both the library and
the client, then it needs to report error states properly and not
write to stderr.
---
 lib/database.cc |  6 +++++
 lib/notmuch.h   | 17 ++++++++++++
 mime-node.c     |  7 ++++-
 util/crypto.c   | 81 +++++++++++++++++++++++++++------------------------------
 util/crypto.h   |  6 +++--
 5 files changed, 72 insertions(+), 45 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 3b342f1..0d4dc9b 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -349,6 +349,12 @@ notmuch_status_to_string (notmuch_status_t status)
 	return "Operation requires a database upgrade";
     case NOTMUCH_STATUS_PATH_ERROR:
 	return "Path supplied is illegal for this function";
+    case NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL:
+	return "Crypto protocol missing, malformed, or unintelligible";
+    case NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION:
+	return "Crypto engine initialization failure";
+    case NOTMUCH_STATUS_UNKNOWN_CRYPTO_PROTOCOL:
+	return "Unknown crypto protocol";
     default:
     case NOTMUCH_STATUS_LAST_STATUS:
 	return "Unknown error status value";
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 310a8b8..00002f1 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -171,6 +171,23 @@ typedef enum _notmuch_status {
      */
     NOTMUCH_STATUS_PATH_ERROR,
     /**
+     * A MIME object claimed to have cryptographic protection which
+     * notmuch tried to handle, but the protocol was not specified in
+     * an intelligible way.
+     */
+    NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL,
+    /**
+     * Notmuch attempted to do crypto processing, but could not
+     * initialize the engine needed to do so.
+     */
+    NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION,
+    /**
+     * A MIME object claimed to have cryptographic protection, and
+     * notmuch attempted to process it, but the specific protocol was
+     * something that notmuch doesn't know how to handle.
+     */
+    NOTMUCH_STATUS_UNKNOWN_CRYPTO_PROTOCOL,
+    /**
      * Not an actual status value. Just a way to find out how many
      * valid status values there are.
      */
diff --git a/mime-node.c b/mime-node.c
index a8f5670..59c0da2 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -244,7 +244,12 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 	|| (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->verify)) {
 	GMimeContentType *content_type = g_mime_object_get_content_type (part);
 	const char *protocol = g_mime_content_type_get_parameter (content_type, "protocol");
-	cryptoctx = _notmuch_crypto_get_gmime_context (node->ctx->crypto, protocol);
+	notmuch_status_t status;
+	status = _notmuch_crypto_get_gmime_ctx_for_protocol (node->ctx->crypto,
+							     protocol, &cryptoctx);
+	if (status) /* this is a warning, not an error */
+	    fprintf (stderr, "Warning: %s (%s).\n", notmuch_status_to_string (status),
+		     protocol ? protocol : "(NULL)");
     }
 
     /* Handle PGP/MIME parts */
diff --git a/util/crypto.c b/util/crypto.c
index eab12a3..151a352 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -26,62 +26,65 @@
 #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
 
 /* Create a GPG context (GMime 2.6) */
-static GMimeCryptoContext*
-create_gpg_context (_notmuch_crypto_t *crypto)
+static notmuch_status_t
+get_gpg_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)
 {
-    GMimeCryptoContext *gpgctx;
+    if (ctx == NULL || crypto == NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
 
     if (crypto->gpgctx) {
-	return crypto->gpgctx;
+	*ctx = crypto->gpgctx;
+	return NOTMUCH_STATUS_SUCCESS;
     }
 
     /* TODO: GMimePasswordRequestFunc */
-    gpgctx = g_mime_gpg_context_new (NULL, crypto->gpgpath ? crypto->gpgpath : "gpg");
-    if (! gpgctx) {
-	fprintf (stderr, "Failed to construct gpg context.\n");
-	return NULL;
+    crypto->gpgctx = g_mime_gpg_context_new (NULL, crypto->gpgpath ? crypto->gpgpath : "gpg");
+    if (! crypto->gpgctx) {
+	return NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION;
     }
-    crypto->gpgctx = gpgctx;
 
-    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) gpgctx, TRUE);
-    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
+    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) crypto->gpgctx, TRUE);
+    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) crypto->gpgctx, FALSE);
 
-    return crypto->gpgctx;
+    *ctx = crypto->gpgctx;
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 /* Create a PKCS7 context (GMime 2.6) */
-static notmuch_crypto_context_t *
-create_pkcs7_context (notmuch_crypto_t *crypto)
+static notmuch_status_t 
+create_pkcs7_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)
 {
-    notmuch_crypto_context_t *pkcs7ctx;
+    if (ctx == NULL || crypto == NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
 
-    if (crypto->pkcs7ctx)
-	return crypto->pkcs7ctx;
+    if (crypto->pkcs7ctx) {
+	*ctx = crypto->pkcs7ctx;
+	return NOTMUCH_STATUS_SUCCESS;
+    }
 
     /* TODO: GMimePasswordRequestFunc */
-    pkcs7ctx = g_mime_pkcs7_context_new (NULL);
-    if (! pkcs7ctx) {
-	fprintf (stderr, "Failed to construct pkcs7 context.\n");
-	return NULL;
+    crypto->pkcs7ctx = g_mime_pkcs7_context_new (NULL);
+    if (! crypto->pkcs7ctx) {
+	return NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION;
     }
-    crypto->pkcs7ctx = pkcs7ctx;
 
-    g_mime_pkcs7_context_set_always_trust ((GMimePkcs7Context *) pkcs7ctx,
+    g_mime_pkcs7_context_set_always_trust ((GMimePkcs7Context *) crypto->pkcs7ctx,
 					   FALSE);
 
-    return crypto->pkcs7ctx;
+    *ctx = crypto->pkcs7ctx;
+    return NOTMUCH_STATUS_SUCCESS;
 }
 static const struct {
     const char *protocol;
-    GMimeCryptoContext *(*get_context) (_notmuch_crypto_t *crypto);
+    notmuch_status_t (*get_context) (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx);
 } protocols[] = {
     {
 	.protocol = "application/pgp-signature",
-	.get_context = create_gpg_context,
+	.get_context = get_gpg_context,
     },
     {
 	.protocol = "application/pgp-encrypted",
-	.get_context = create_gpg_context,
+	.get_context = get_gpg_context,
     },
     {
 	.protocol = "application/pkcs7-signature",
@@ -95,16 +98,13 @@ static const struct {
 
 /* for the specified protocol return the context pointer (initializing
  * if needed) */
-GMimeCryptoContext *
-_notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protocol)
+notmuch_status_t
+_notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
+					    const char *protocol,
+					    GMimeCryptoContext **ctx)
 {
-    GMimeCryptoContext *cryptoctx = NULL;
-    size_t i;
-
-    if (! protocol) {
-	fprintf (stderr, "Cryptographic protocol is empty.\n");
-	return cryptoctx;
-    }
+    if (! protocol)
+	return NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL;
 
     /* As per RFC 1847 section 2.1: "the [protocol] value token is
      * comprised of the type and sub-type tokens of the Content-Type".
@@ -112,15 +112,12 @@ _notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protoc
      * parameter names as defined in this document are
      * case-insensitive."  Thus, we use strcasecmp for the protocol.
      */
-    for (i = 0; i < ARRAY_SIZE (protocols); i++) {
+    for (size_t i = 0; i < ARRAY_SIZE (protocols); i++) {
 	if (strcasecmp (protocol, protocols[i].protocol) == 0)
-	    return protocols[i].get_context (crypto);
+	    return protocols[i].get_context (crypto, ctx);
     }
 
-    fprintf (stderr, "Unknown or unsupported cryptographic protocol %s.\n",
-	     protocol);
-
-    return NULL;
+    return NOTMUCH_STATUS_UNKNOWN_CRYPTO_PROTOCOL;
 }
 
 void
diff --git a/util/crypto.h b/util/crypto.h
index d4a51e8..7cb0a39 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -15,8 +15,10 @@ typedef struct _notmuch_crypto {
 } _notmuch_crypto_t;
 
 
-GMimeCryptoContext *
-_notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protocol);
+notmuch_status_t
+_notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
+					    const char *protocol,
+					    GMimeCryptoContext **ctx);
 
 void
 _notmuch_crypto_cleanup (_notmuch_crypto_t *crypto);
-- 
2.7.0.rc3

[PATCH v3 04/16] Provide _notmuch_crypto_{set,get}_gpg_path

[Daniel Kahn Gillmor]

Use functions to access the gpg_path for a _notmuch_crypto_t object.
This lets us return sensible defaults based on the state of the user's
machine.
---
 notmuch-reply.c | 13 ++++++++++---
 notmuch-show.c  | 12 ++++++++++--
 util/crypto.c   | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 util/crypto.h   |  8 +++++++-
 4 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index eccfb32..793e6f9 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -790,13 +790,15 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 	.crypto = {
 	    .verify = FALSE,
 	    .decrypt = FALSE,
-	    .gpgpath = NULL
+	    .gpg_path = NULL
 	}
     };
     int format = FORMAT_DEFAULT;
     int reply_all = TRUE;
     struct sprinter *sp = NULL;
-
+    notmuch_status_t status;
+    const char *gpg_path = NULL;
+    
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format, "format", 'f',
 	  (notmuch_keyword_t []){ { "default", FORMAT_DEFAULT },
@@ -845,7 +847,12 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
-    params.crypto.gpgpath = notmuch_config_get_crypto_gpg_path (config);
+    gpg_path = notmuch_config_get_crypto_gpg_path (config);
+    status = _notmuch_crypto_set_gpg_path (&(params.crypto), gpg_path);
+    if (status != NOTMUCH_STATUS_SUCCESS) {
+	fprintf (stderr, "Error: could not set gpg_path to '%s'.\n", gpg_path);
+	return EXIT_FAILURE;
+    }
 
     if (notmuch_database_open (notmuch_config_get_database_path (config),
 			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
diff --git a/notmuch-show.c b/notmuch-show.c
index 3c91ece..096fd49 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1006,13 +1006,15 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	.crypto = {
 	    .verify = FALSE,
 	    .decrypt = FALSE,
-	    .gpgpath = NULL
+	    .gpg_path = NULL
 	},
 	.include_html = FALSE
     };
     int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
     int exclude = EXCLUDE_TRUE;
     int entire_thread = ENTIRE_THREAD_DEFAULT;
+    notmuch_status_t status;
+    const char *gpg_path = NULL;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_KEYWORD, &format_sel, "format", 'f',
@@ -1130,7 +1132,13 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
-    params.crypto.gpgpath = notmuch_config_get_crypto_gpg_path (config);
+
+    gpg_path = notmuch_config_get_crypto_gpg_path (config);
+    status = _notmuch_crypto_set_gpg_path (&(params.crypto), gpg_path);
+    if (status != NOTMUCH_STATUS_SUCCESS) {
+	fprintf (stderr, "Error: could not set gpg_path to '%s'.\n", gpg_path);
+	return EXIT_FAILURE;
+    }
 
     if (notmuch_database_open (notmuch_config_get_database_path (config),
 			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
diff --git a/util/crypto.c b/util/crypto.c
index 151a352..c89ce8a 100644
--- a/util/crypto.c
+++ b/util/crypto.c
@@ -21,7 +21,9 @@
 
 #include "notmuch.h"
 #include "crypto.h"
+#include "search-path.h"
 #include <string.h>
+#include <talloc.h>
 
 #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
 
@@ -38,7 +40,7 @@ get_gpg_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)
     }
 
     /* TODO: GMimePasswordRequestFunc */
-    crypto->gpgctx = g_mime_gpg_context_new (NULL, crypto->gpgpath ? crypto->gpgpath : "gpg");
+    crypto->gpgctx = g_mime_gpg_context_new (NULL, _notmuch_crypto_get_gpg_path(crypto));
     if (! crypto->gpgctx) {
 	return NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION;
     }
@@ -120,6 +122,47 @@ _notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
     return NOTMUCH_STATUS_UNKNOWN_CRYPTO_PROTOCOL;
 }
 
+const char*
+_notmuch_crypto_get_gpg_path (const _notmuch_crypto_t *crypto)
+{
+    if (crypto->gpg_path)
+	return crypto->gpg_path;
+
+#define try_gpg_path(z) if (test_for_executable(z)) return z
+    try_gpg_path("gpg2");
+    try_gpg_path("gpg");
+#undef try_gpg_path
+    return NULL;
+}
+
+notmuch_status_t
+_notmuch_crypto_set_gpg_path (_notmuch_crypto_t *crypto, const char* gpg_path)
+{
+    /* return success if this matches what is already configured */
+    if ((!gpg_path && !crypto->gpg_path) ||
+	(gpg_path && crypto->gpg_path && 0 == strcmp(gpg_path, crypto->gpg_path)))
+	return NOTMUCH_STATUS_SUCCESS;
+
+    if (!gpg_path && !test_for_executable(gpg_path))
+	return NOTMUCH_STATUS_FILE_ERROR;
+
+    /* clear any existing gpgctx, since things are changing */
+    if (crypto->gpgctx) {
+	g_object_unref (crypto->gpgctx);
+	crypto->gpgctx = NULL;
+    }
+
+    if (crypto->gpg_path) {
+	talloc_free(crypto->gpg_path);
+	crypto->gpg_path = NULL;
+    }
+
+    if (gpg_path)
+	crypto->gpg_path = talloc_strdup (NULL, gpg_path);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
 void
 _notmuch_crypto_cleanup (_notmuch_crypto_t *crypto)
 {
@@ -132,4 +175,7 @@ _notmuch_crypto_cleanup (_notmuch_crypto_t *crypto)
 	g_object_unref (crypto->pkcs7ctx);
 	crypto->pkcs7ctx = NULL;
     }
+
+    talloc_free (crypto->gpg_path);
+    crypto->gpg_path = NULL;
 }
diff --git a/util/crypto.h b/util/crypto.h
index 7cb0a39..70fc8ef 100644
--- a/util/crypto.h
+++ b/util/crypto.h
@@ -11,7 +11,7 @@ typedef struct _notmuch_crypto {
     GMimeCryptoContext* pkcs7ctx;
     notmuch_bool_t verify;
     notmuch_bool_t decrypt;
-    const char *gpgpath;
+    char *gpg_path;
 } _notmuch_crypto_t;
 
 
@@ -20,6 +20,12 @@ _notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
 					    const char *protocol,
 					    GMimeCryptoContext **ctx);
 
+notmuch_status_t
+_notmuch_crypto_set_gpg_path (_notmuch_crypto_t *crypto, const char *gpg_path);
+
+const char *
+_notmuch_crypto_get_gpg_path (const _notmuch_crypto_t *crypto);
+
 void
 _notmuch_crypto_cleanup (_notmuch_crypto_t *crypto);
 
-- 
2.7.0.rc3

[PATCH v3 05/16] Use a blank _notmuch_crypto to choose the default gpg_path

[Daniel Kahn Gillmor]

This way we're only choosing a default in one place.
---
 notmuch-config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index d252bb2..7cd19a7 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -418,7 +418,8 @@ notmuch_config_open (void *ctx,
     }
 
     if (notmuch_config_get_crypto_gpg_path (config) == NULL) {
-	notmuch_config_set_crypto_gpg_path (config, "gpg");
+	_notmuch_crypto_t crypto = { .gpg_path = NULL };
+	notmuch_config_set_crypto_gpg_path (config, _notmuch_crypto_get_gpg_path (&crypto));
     }
     
     /* Whenever we know of configuration sections that don't appear in
-- 
2.7.0.rc3

[PATCH v3 06/16] Prefer gpg2 in the test suite if available

[Daniel Kahn Gillmor]

Now that the notmuch client prefers gpg2 if available, having the test
suite use the same preference makes it more likely to validate as
expected.

Be warned that the final test in T350-crypto.sh fails with an infinite
loop in gpg if you're using an unpatched GnuPG 2.1.10, due to an
upstream GnuPG bug: https://bugs.gnupg.org/gnupg/issue2187.  In
debian, this is resolved in 2.1.10-3
---
 test/README         |  2 +-
 test/T030-config.sh |  2 +-
 test/T040-setup.sh  |  2 +-
 test/T350-crypto.sh | 16 ++++++++--------
 test/test-lib.sh    | 10 +++++++++-
 5 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/test/README b/test/README
index e54e36b..9a7e539 100644
--- a/test/README
+++ b/test/README
@@ -16,7 +16,7 @@ that you know if you break anything.
   - emacs(1)
   - emacsclient(1)
   - gdb(1)
-  - gpg(1)
+  - gpg(1) or gpg2(1)
   - python(1)
 
 Running Tests
diff --git a/test/T030-config.sh b/test/T030-config.sh
index f404908..daa7b44 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -54,7 +54,7 @@ new.tags=unread;inbox;
 new.ignore=
 search.exclude_tags=
 maildir.synchronize_flags=true
-crypto.gpg_path=gpg
+crypto.gpg_path=$GPG
 foo.string=this is another string value
 foo.list=this;is another;list value;"
 
diff --git a/test/T040-setup.sh b/test/T040-setup.sh
index cf0c00b..9313aec 100755
--- a/test/T040-setup.sh
+++ b/test/T040-setup.sh
@@ -29,6 +29,6 @@ new.tags=foo;bar;
 new.ignore=
 search.exclude_tags=baz;
 maildir.synchronize_flags=true
-crypto.gpg_path=gpg"
+crypto.gpg_path=$GPG"
 
 test_done
diff --git a/test/T350-crypto.sh b/test/T350-crypto.sh
index 3656cce..4bc15bc 100755
--- a/test/T350-crypto.sh
+++ b/test/T350-crypto.sh
@@ -12,11 +12,11 @@ add_gnupg_home ()
     local output
     [ -d ${GNUPGHOME} ] && return
     mkdir -m 0700 "$GNUPGHOME"
-    gpg --no-tty --import <$TEST_DIRECTORY/gnupg-secret-key.asc >"$GNUPGHOME"/import.log 2>&1
+    $GPG --no-tty --import <$TEST_DIRECTORY/gnupg-secret-key.asc >"$GNUPGHOME"/import.log 2>&1
     test_debug "cat $GNUPGHOME/import.log"
-    if (gpg --quick-random --version >/dev/null 2>&1) ; then
+    if ($GPG --quick-random --version >/dev/null 2>&1) ; then
 	echo quick-random >> "$GNUPGHOME"/gpg.conf
-    elif (gpg --debug-quick-random --version >/dev/null 2>&1) ; then
+    elif ($GPG --debug-quick-random --version >/dev/null 2>&1) ; then
 	echo debug-quick-random >> "$GNUPGHOME"/gpg.conf
     fi
     echo no-emit-version >> "$GNUPGHOME"/gpg.conf
@@ -26,7 +26,7 @@ add_gnupg_home ()
 
 add_gnupg_home
 # get key fingerprint
-FINGERPRINT=$(gpg --no-tty --list-secret-keys --with-colons --fingerprint | grep '^fpr:' | cut -d: -f10)
+FINGERPRINT=$($GPG --no-tty --list-secret-keys --with-colons --fingerprint | grep '^fpr:' | cut -d: -f10)
 
 test_expect_success 'emacs delivery of signed message' \
 'emacs_fcc_message \
@@ -67,8 +67,8 @@ test_expect_equal_json \
 
 test_begin_subtest "signature verification with full owner trust"
 # give the key full owner trust
-echo "${FINGERPRINT}:6:" | gpg --no-tty --import-ownertrust >>"$GNUPGHOME"/trust.log 2>&1
-gpg --no-tty --check-trustdb >>"$GNUPGHOME"/trust.log 2>&1
+echo "${FINGERPRINT}:6:" | $GPG --no-tty --import-ownertrust >>"$GNUPGHOME"/trust.log 2>&1
+$GPG --no-tty --check-trustdb >>"$GNUPGHOME"/trust.log 2>&1
 output=$(notmuch show --format=json --verify subject:"test signed message 001" \
     | notmuch_json_show_sanitize \
     | sed -e 's|"created": [1234567890]*|"created": 946728000|')
@@ -325,8 +325,8 @@ Notmuch Test Suite key revocation (automated) $(date '+%F_%T%z')
 y
 
 " \
-    | gpg --no-tty --quiet --command-fd 0 --armor --gen-revoke "0x${FINGERPRINT}!" 2>/dev/null \
-    | gpg --no-tty --quiet --import
+    | $GPG --no-tty --quiet --command-fd 0 --armor --gen-revoke "0x${FINGERPRINT}!" 2>/dev/null \
+    | $GPG --no-tty --quiet --import
 output=$(notmuch show --format=json --verify subject:"test signed message 001" \
     | notmuch_json_show_sanitize \
     | sed -e 's|"created": [1234567890]*|"created": 946728000|')
diff --git a/test/test-lib.sh b/test/test-lib.sh
index cc08a98..8c1f3c6 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -85,6 +85,13 @@ unset GREP_OPTIONS
 # For emacsclient
 unset ALTERNATE_EDITOR
 
+# choose the preferred GnuPG binary:
+if command -v gpg2 > /dev/null; then
+    GPG=gpg2
+else
+    GPG=gpg
+fi
+
 # Convenience
 #
 # A regexp to match 5 and 40 hexdigits
@@ -1144,6 +1151,7 @@ test_emacs () {
 				$load_emacs_tests \
 				--eval '(setq server-name \"$server_name\")' \
 				--eval '(server-start)' \
+				--eval '(setq epg-gpg-program \"$GPG\")' \
 				--eval '(orphan-watchdog $$)'" || return
 		EMACS_SERVER="$server_name"
 		# wait until the emacs server is up
@@ -1332,7 +1340,7 @@ test_declare_external_prereq dtach
 test_declare_external_prereq emacs
 test_declare_external_prereq ${TEST_EMACSCLIENT}
 test_declare_external_prereq gdb
-test_declare_external_prereq gpg
+test_declare_external_prereq gpg2 || test_declare_external_prereq gpg
 test_declare_external_prereq openssl
 test_declare_external_prereq gpgsm
 test_declare_external_prereq ${NOTMUCH_PYTHON}
-- 
2.7.0.rc3

[PATCH v3 07/16] create a notmuch_indexopts_t index options object

[Daniel Kahn Gillmor]

This is currently mostly a wrapper around _notmuch_crypto_t that keeps
its internals private and doesn't expose any of the GMime API.
However, non-crypto indexing options might also be added later to
indexopts (e.g. filters or other transformations).
---
 lib/Makefile.local    |  1 +
 lib/indexopts.c       | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/notmuch-private.h |  7 +++++
 lib/notmuch.h         | 63 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 143 insertions(+)
 create mode 100644 lib/indexopts.c

diff --git a/lib/Makefile.local b/lib/Makefile.local
index 3a07090..1652b1b 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -39,6 +39,7 @@ libnotmuch_c_srcs =		\
 	$(dir)/message-file.c	\
 	$(dir)/messages.c	\
 	$(dir)/sha1.c		\
+	$(dir)/indexopts.c	\
 	$(dir)/tags.c
 
 libnotmuch_cxx_srcs =		\
diff --git a/lib/indexopts.c b/lib/indexopts.c
new file mode 100644
index 0000000..da36e2b
--- /dev/null
+++ b/lib/indexopts.c
@@ -0,0 +1,72 @@
+/* indexopts.c - options for indexing messages
+ *
+ * Copyright © 2015 Daniel Kahn Gillmor
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
+ */
+
+#include "notmuch-private.h"
+
+notmuch_indexopts_t *
+notmuch_indexopts_create ()
+{
+    notmuch_indexopts_t *ret;
+    
+    ret = talloc_zero (NULL, notmuch_indexopts_t);
+
+    return ret;
+}
+
+notmuch_status_t
+notmuch_indexopts_set_try_decrypt (notmuch_indexopts_t *indexopts,
+				   notmuch_bool_t try_decrypt)
+{
+    if (!indexopts)
+	return NOTMUCH_STATUS_NULL_POINTER;
+    indexopts->crypto.decrypt = try_decrypt;
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+notmuch_bool_t
+notmuch_indexopts_get_try_decrypt (const notmuch_indexopts_t *indexopts)
+{
+    if (!indexopts)
+	return FALSE;
+    return indexopts->crypto.decrypt;
+}
+
+notmuch_status_t
+notmuch_indexopts_set_gpg_path (notmuch_indexopts_t *indexopts,
+				const char *gpg_path)
+{
+    if (!indexopts)
+	return NOTMUCH_STATUS_NULL_POINTER;
+    return _notmuch_crypto_set_gpg_path (&(indexopts->crypto), gpg_path);
+}
+
+const char*
+notmuch_indexopts_get_gpg_path (const notmuch_indexopts_t *indexopts)
+{
+    if (!indexopts)
+	return NULL;
+    return _notmuch_crypto_get_gpg_path (&(indexopts->crypto));
+}
+
+void
+notmuch_indexopts_destroy (notmuch_indexopts_t *indexopts)
+{
+    talloc_free (indexopts);
+}
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 5dd4770..e9c1e8a 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -51,6 +51,7 @@ NOTMUCH_BEGIN_DECLS
 #include "xutil.h"
 #include "error_util.h"
 #include "string-util.h"
+#include "crypto.h"
 
 #pragma GCC visibility push(hidden)
 
@@ -544,6 +545,12 @@ _notmuch_thread_create (void *ctx,
 			notmuch_exclude_t omit_exclude,
 			notmuch_sort_t sort);
 
+/* indexopts.c */
+
+typedef struct _notmuch_indexopts {
+    _notmuch_crypto_t crypto;
+} notmuch_indexopts_t;
+
 NOTMUCH_END_DECLS
 
 #ifdef __cplusplus
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 00002f1..3679c54 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -214,6 +214,7 @@ typedef struct _notmuch_message notmuch_message_t;
 typedef struct _notmuch_tags notmuch_tags_t;
 typedef struct _notmuch_directory notmuch_directory_t;
 typedef struct _notmuch_filenames notmuch_filenames_t;
+typedef struct _notmuch_indexopts notmuch_indexopts_t;
 #endif /* __DOXYGEN__ */
 
 /**
@@ -1846,6 +1847,68 @@ notmuch_filenames_move_to_next (notmuch_filenames_t *filenames);
 void
 notmuch_filenames_destroy (notmuch_filenames_t *filenames);
 
+/**
+ * Create a notmuch_indexopts_t object.
+ *
+ * This object describes options on how indexing can happen when a
+ * message is added to the index.
+ */
+notmuch_indexopts_t *
+notmuch_indexopts_create ();
+
+/**
+ * Specify whether to decrypt encrypted parts while indexing.
+ * 
+ * Be aware that the index is likely sufficient to reconstruct the
+ * cleartext of the message itself, so please ensure that the notmuch
+ * message index is adequately protected. DO NOT SET THIS FLAG TO TRUE
+ * without considering the security of your index.
+ *
+ */
+notmuch_status_t
+notmuch_indexopts_set_try_decrypt (notmuch_indexopts_t *indexopts,
+				   notmuch_bool_t try_decrypt);
+
+/**
+ * Return whether to decrypt encrypted parts while indexing.
+ * see notmuch_indexopts_set_try_decrypt.
+ */
+notmuch_bool_t
+notmuch_indexopts_get_try_decrypt (const notmuch_indexopts_t *indexopts);
+
+/**
+ * Specify the name (or name and path) of the gpg executable, in case
+ * GnuPG needs to be used during indexing.  The default should usually
+ * be fine.
+ * 
+ * Passing NULL to this will reset it to the default.
+ * 
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: the path was accepted and will be used.
+ * 
+ * NOTMUCH_STATUS_FILE_ERROR: the path given either wasn't found or
+ *      wasn't executable.
+ */
+notmuch_status_t
+notmuch_indexopts_set_gpg_path (notmuch_indexopts_t *indexopts,
+				const char *gpg_path);
+
+/**
+ * Return the name (possibly including path) of the gpg executable to
+ * be used in case GnuPG needs to be used during indexing.
+ * 
+ * see notmuch_indexopts_set_gpg_path
+ */
+const char*
+notmuch_indexopts_get_gpg_path (const notmuch_indexopts_t *indexopts);
+
+/**
+ * Destroy a notmuch_indexopts_t object.
+ */
+void
+notmuch_indexopts_destroy (notmuch_indexopts_t *options);
+
 /* @} */
 
 NOTMUCH_END_DECLS
-- 
2.7.0.rc3

[PATCH v3 08/16] reorganize indexing of multipart/signed and multipart/encrypted

[Daniel Kahn Gillmor]

This prepares the codebase for a cleaner changeset for dealing with
indexing some encrypted messages in the clear.
---
 lib/index.cc | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/lib/index.cc b/lib/index.cc
index f166aef..ab0fd78 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -333,27 +333,26 @@ _index_mime_part (notmuch_message_t *message,
 	GMimeMultipart *multipart = GMIME_MULTIPART (part);
 	int i;
 
-	if (GMIME_IS_MULTIPART_SIGNED (multipart))
-	  _notmuch_message_add_term (message, "tag", "signed");
-
-	if (GMIME_IS_MULTIPART_ENCRYPTED (multipart))
-	  _notmuch_message_add_term (message, "tag", "encrypted");
-
-	for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
-	    if (GMIME_IS_MULTIPART_SIGNED (multipart)) {
-		/* Don't index the signature. */
-		if (i == 1)
-		    continue;
-		if (i > 1)
-		    _notmuch_database_log (_notmuch_message_database (message),
-					  "Warning: Unexpected extra parts of multipart/signed. Indexing anyway.\n");
-	    }
-	    if (GMIME_IS_MULTIPART_ENCRYPTED (multipart)) {
-		/* Don't index encrypted parts. */
-		continue;
-	    }
+	if (GMIME_IS_MULTIPART_SIGNED (multipart)) {
+	    _notmuch_message_add_term (message, "tag", "signed");
+	    /* FIXME: should we try to validate the signature? */
+	    
+	    /* FIXME: is it always just the first part that is signed in
+	     all multipart/signed messages?*/
 	    _index_mime_part (message,
-			      g_mime_multipart_get_part (multipart, i));
+			      g_mime_multipart_get_part (multipart, 0));
+	    
+	    if (g_mime_multipart_get_count (multipart) > 2)
+		_notmuch_database_log (_notmuch_message_database (message),
+				       "Warning: Unexpected extra parts of multipart/signed. Indexing anyway.\n");
+	} else if (GMIME_IS_MULTIPART_ENCRYPTED (multipart)) {
+	    /* Don't index encrypted parts */
+	    _notmuch_message_add_term (message, "tag", "encrypted");
+	} else {
+	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
+		_index_mime_part (message,
+				  g_mime_multipart_get_part (multipart, i));
+	    }
 	}
 	return;
     }
-- 
2.7.0.rc3

[PATCH v3 09/16] index encrypted parts when asked.

[Daniel Kahn Gillmor]

If we see index options that ask us to decrypt when indexing a
message, and we encounter an encrypted part, we'll try to descend into
it.

If we can decrypt, we tag the message with index-decrypted.

If we can't decrypt (or recognize the encrypted type of mail), we tag
with decryption-failed.

Note that a single message may be tagged with "encrypted" and
"index-decrypted" and "decryption-failed".  For example, consider a
message that includes multiple layers of encryption.  It is
automatically tagged with "encrypted".  If we decrypt the outer layer
("index-decrypted"), but fail on the inner layer
("decryption-failed").
---
 lib/database.cc       |  3 ++-
 lib/index.cc          | 64 ++++++++++++++++++++++++++++++++++++++++++++++++---
 lib/notmuch-private.h |  1 +
 3 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 0d4dc9b..7d88f69 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -2402,6 +2402,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS, ret2;
     notmuch_private_status_t private_status;
     notmuch_bool_t is_ghost = false;
+    notmuch_indexopts_t *indexopts = NULL;
 
     const char *date, *header;
     const char *from, *to, *subject;
@@ -2514,7 +2515,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 	    date = _notmuch_message_file_get_header (message_file, "date");
 	    _notmuch_message_set_header_values (message, date, from, subject);
 
-	    ret = _notmuch_message_index_file (message, message_file);
+	    ret = _notmuch_message_index_file (message, indexopts, message_file);
 	    if (ret)
 		goto DONE;
 	} else {
diff --git a/lib/index.cc b/lib/index.cc
index ab0fd78..eb406d2 100644
--- a/lib/index.cc
+++ b/lib/index.cc
@@ -300,9 +300,14 @@ _index_address_list (notmuch_message_t *message,
     }
 }
 
+static void
+_index_encrypted_mime_part (notmuch_message_t *message, notmuch_indexopts_t *indexopts,
+			    GMimeContentType *content_type, GMimeMultipartEncrypted *part);
+
 /* Callback to generate terms for each mime part of a message. */
 static void
 _index_mime_part (notmuch_message_t *message,
+		  notmuch_indexopts_t *indexopts,
 		  GMimeObject *part)
 {
     GMimeStream *stream, *filter;
@@ -340,17 +345,19 @@ _index_mime_part (notmuch_message_t *message,
 	    /* FIXME: is it always just the first part that is signed in
 	     all multipart/signed messages?*/
 	    _index_mime_part (message,
+			      indexopts,
 			      g_mime_multipart_get_part (multipart, 0));
 	    
 	    if (g_mime_multipart_get_count (multipart) > 2)
 		_notmuch_database_log (_notmuch_message_database (message),
 				       "Warning: Unexpected extra parts of multipart/signed. Indexing anyway.\n");
 	} else if (GMIME_IS_MULTIPART_ENCRYPTED (multipart)) {
-	    /* Don't index encrypted parts */
 	    _notmuch_message_add_term (message, "tag", "encrypted");
+	    _index_encrypted_mime_part(message, indexopts, content_type, GMIME_MULTIPART_ENCRYPTED (part));
 	} else {
 	    for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
 		_index_mime_part (message,
+				  indexopts,
 				  g_mime_multipart_get_part (multipart, i));
 	    }
 	}
@@ -362,7 +369,7 @@ _index_mime_part (notmuch_message_t *message,
 
 	mime_message = g_mime_message_part_get_message (GMIME_MESSAGE_PART (part));
 
-	_index_mime_part (message, g_mime_message_get_mime_part (mime_message));
+	_index_mime_part (message, indexopts, g_mime_message_get_mime_part (mime_message));
 
 	return;
     }
@@ -432,8 +439,59 @@ _index_mime_part (notmuch_message_t *message,
     }
 }
 
+/* descend (if desired) into the cleartext part of an encrypted MIME
+ * part while indexing. */
+static void
+_index_encrypted_mime_part (notmuch_message_t *message,
+			    notmuch_indexopts_t *indexopts,
+			    GMimeContentType *content_type,
+			    GMimeMultipartEncrypted *encrypted_data)
+{
+    notmuch_status_t status;
+    GMimeCryptoContext* crypto_ctx = NULL;
+    const char *protocol = NULL;
+    GError *err = NULL;
+    notmuch_database_t * notmuch = NULL;
+    GMimeObject *clear = NULL;
+
+    if (!indexopts || !notmuch_indexopts_get_try_decrypt (indexopts))
+	return;
+
+    protocol = g_mime_content_type_get_parameter (content_type, "protocol");
+    notmuch = _notmuch_message_database (message);
+    
+    status = _notmuch_crypto_get_gmime_ctx_for_protocol (&(indexopts->crypto),
+							 protocol, &crypto_ctx);
+    if (status) {
+	_notmuch_database_log (notmuch, "Warning: setup failed for decrypting "
+			       "during indexing. (%d)\n", status);
+	_notmuch_message_add_term (message, "tag", "index-decryption-failed");
+	return;
+    }
+
+    /* we don't need the GMimeDecryptResult, because we're not looking
+     * at validating signatures, and we don't care about indexing who
+     * the message was ostensibly encrypted to.
+     */
+    clear = g_mime_multipart_encrypted_decrypt(encrypted_data, crypto_ctx,
+					       NULL, &err);
+    if (err) {
+	_notmuch_database_log (notmuch, "Failed to decrypt during indexing. (%d:%d) [%s]\n",
+			       err->domain, err->code, err->message);
+	g_error_free(err);
+	/* Indicate that we failed to decrypt during indexing */
+	_notmuch_message_add_term (message, "tag", "index-decryption-failed");
+	return;
+    }
+    _index_mime_part (message, indexopts, clear);
+    g_object_unref (clear);
+    
+    _notmuch_message_add_term (message, "tag", "index-decrypted");
+}
+
 notmuch_status_t
 _notmuch_message_index_file (notmuch_message_t *message,
+			     notmuch_indexopts_t *indexopts,
 			     notmuch_message_file_t *message_file)
 {
     GMimeMessage *mime_message;
@@ -463,7 +521,7 @@ _notmuch_message_index_file (notmuch_message_t *message,
     subject = g_mime_message_get_subject (mime_message);
     _notmuch_message_gen_terms (message, "subject", subject);
 
-    _index_mime_part (message, g_mime_message_get_mime_part (mime_message));
+    _index_mime_part (message, indexopts, g_mime_message_get_mime_part (mime_message));
 
     return NOTMUCH_STATUS_SUCCESS;
 }
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index e9c1e8a..9bd4f33 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -425,6 +425,7 @@ _notmuch_message_file_get_header (notmuch_message_file_t *message,
 
 notmuch_status_t
 _notmuch_message_index_file (notmuch_message_t *message,
+			     notmuch_indexopts_t *indexopts,
 			     notmuch_message_file_t *message_file);
 
 /* messages.c */
-- 
2.7.0.rc3

[PATCH v3 10/16] Add n_d_add_message_with_indexopts (extension of n_d_add_message)

[Daniel Kahn Gillmor]

Expose the ability to ask for index options via the library interface.
This _add_message_with_indexopts function is now a generalized form of
the older _add_message.  It lets you specify parameters and
configurations that can affect the indexing, like indexing encrypted
messages in the clear should the user choose to do so.

We also adjust the tests so that we test the extended function
returning bad values (since the non-extended function just calls the
extended one).
---
 lib/database.cc     | 20 ++++++++++++++++----
 lib/notmuch.h       | 14 ++++++++++++++
 test/T070-insert.sh |  4 ++--
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 7d88f69..990b0b1 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -2393,16 +2393,16 @@ _notmuch_database_link_message (notmuch_database_t *notmuch,
 }
 
 notmuch_status_t
-notmuch_database_add_message (notmuch_database_t *notmuch,
-			      const char *filename,
-			      notmuch_message_t **message_ret)
+notmuch_database_add_message_with_indexopts (notmuch_database_t *notmuch,
+					     const char *filename,
+					     notmuch_indexopts_t *indexopts,
+					     notmuch_message_t **message_ret)
 {
     notmuch_message_file_t *message_file;
     notmuch_message_t *message = NULL;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS, ret2;
     notmuch_private_status_t private_status;
     notmuch_bool_t is_ghost = false;
-    notmuch_indexopts_t *indexopts = NULL;
 
     const char *date, *header;
     const char *from, *to, *subject;
@@ -2552,6 +2552,18 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
     return ret;
 }
 
+
+notmuch_status_t
+notmuch_database_add_message (notmuch_database_t *notmuch,
+			      const char *filename,
+			      notmuch_message_t **message_ret)
+{
+    return notmuch_database_add_message_with_indexopts (notmuch, filename,
+							NULL,
+							message_ret);
+    
+}
+
 notmuch_status_t
 notmuch_database_remove_message (notmuch_database_t *notmuch,
 				 const char *filename)
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3679c54..854a451 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -588,6 +588,20 @@ notmuch_status_t
 notmuch_database_add_message (notmuch_database_t *database,
 			      const char *filename,
 			      notmuch_message_t **message);
+/**
+ * Add a new message to the given notmuch database or associate an
+ * additional filename with an existing message using specified
+ * options.
+ * 
+ * This does the same thing as notmuch_database_add_message except
+ * that it passes a pre-configured set of indexing options to indicate
+ * how the specific message should be indexed.
+ */
+notmuch_status_t
+notmuch_database_add_message_with_indexopts (notmuch_database_t *database,
+					     const char *filename,
+					     notmuch_indexopts_t *indexopts,
+					     notmuch_message_t **message);
 
 /**
  * Remove a message filename from the given notmuch database. If the
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index e7ec6a6..557f9d5 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -192,14 +192,14 @@ for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
 gen_insert_msg
 cat <<EOF > index-file-$code.gdb
 set breakpoint pending on
-break notmuch_database_add_message
+break notmuch_database_add_message_with_indexopts
 commands
 return NOTMUCH_STATUS_$code
 continue
 end
 run
 EOF
-test_begin_subtest "error exit when add_message returns $code"
+test_begin_subtest "error exit when add_message_with_indexopts returns $code"
 gdb --batch-silent --return-child-result -x index-file-$code.gdb \
     --args notmuch insert  < $gen_msg_filename
 test_expect_equal $? 1
-- 
2.7.0.rc3

[PATCH v3 11/16] add --try-decrypt to notmuch insert

[Daniel Kahn Gillmor]

allow an incoming message to be delivered while indexing the
cleartext.

This requires the secret keys for the message to be available.  For
the moment, the most functional approach is to ensure that gpg-agent
is running and knows about any secret keys that might be useful to
decrypt incoming mail.

Any additional recommendations for how to phrase the caveat for this
option are welcome.

If ~/.notmuch-config contains crypto.gpg_path, and gpg is needed for
indexing, the configuration option will be used to find gpg.
---
 completion/notmuch-completion.bash |  2 +-
 doc/man1/notmuch-insert.rst        | 11 +++++++++++
 notmuch-insert.c                   | 32 +++++++++++++++++++++++++++++---
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash
index cc58392..4bc9040 100644
--- a/completion/notmuch-completion.bash
+++ b/completion/notmuch-completion.bash
@@ -224,7 +224,7 @@ _notmuch_insert()
     ! $split &&
     case "${cur}" in
 	--*)
-	    local options="--create-folder --folder= --keep --no-hooks ${_notmuch_shared_options}"
+	    local options="--create-folder --folder= --keep --no-hooks --try-decrypt ${_notmuch_shared_options}"
 	    compopt -o nospace
 	    COMPREPLY=( $(compgen -W "$options" -- ${cur}) )
 	    return
diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
index 2c9c0d0..9c76b30 100644
--- a/doc/man1/notmuch-insert.rst
+++ b/doc/man1/notmuch-insert.rst
@@ -50,6 +50,17 @@ Supported options for **insert** include
     ``--no-hooks``
         Prevent hooks from being run.
 
+    ``--try-decrypt``
+
+        If the message is encrypted, try to decrypt the message while
+        indexing.  If decryption is successful, index the cleartext
+        itself.  The message is stored to disk in its original form
+        (ciphertext).  Be aware that the index is likely sufficient to
+        reconstruct the cleartext of the message itself, so please
+        ensure that the notmuch message index is adequately
+        protected. DO NOT USE THIS FLAG without considering the
+        security of your index.
+
 EXIT STATUS
 ===========
 
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 5205c17..eae1ec5 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -378,12 +378,13 @@ FAIL:
  */
 static notmuch_status_t
 add_file (notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,
-	  notmuch_bool_t synchronize_flags, notmuch_bool_t keep)
+	  notmuch_bool_t synchronize_flags, notmuch_bool_t keep,
+	  notmuch_indexopts_t *indexopts)
 {
     notmuch_message_t *message;
     notmuch_status_t status;
 
-    status = notmuch_database_add_message (notmuch, path, &message);
+    status = notmuch_database_add_message_with_indexopts (notmuch, path, indexopts, &message);
     if (status == NOTMUCH_STATUS_SUCCESS) {
 	status = tag_op_list_apply (message, tag_ops, 0);
 	if (status) {
@@ -455,17 +456,20 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_bool_t create_folder = FALSE;
     notmuch_bool_t keep = FALSE;
     notmuch_bool_t no_hooks = FALSE;
+    notmuch_bool_t try_decrypt = FALSE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
     char *newpath;
     int opt_index;
     unsigned int i;
+    notmuch_indexopts_t *indexopts;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &keep, "keep", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &try_decrypt, "try-decrypt", 0, 0 },
 	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
@@ -545,8 +549,29 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
+    indexopts = notmuch_indexopts_create ();
+    if (!indexopts) {
+	fprintf (stderr, "Error: could not create index options.\n");
+	return EXIT_FAILURE;
+    }
+    status = notmuch_indexopts_set_try_decrypt (indexopts, try_decrypt);
+    if (status != NOTMUCH_STATUS_SUCCESS) {
+	fprintf (stderr, "Error: Failed to set try_decrypt to %s. (%s)\n",
+		 try_decrypt ? "True" : "False", notmuch_status_to_string (status));
+	notmuch_indexopts_destroy (indexopts);
+	return EXIT_FAILURE;
+    }
+    if (try_decrypt) {
+	const char* gpg_path = notmuch_config_get_crypto_gpg_path (config);
+	status = notmuch_indexopts_set_gpg_path (indexopts, gpg_path);
+	if (status)
+	    fprintf (stderr, "Warning: failed to set database gpg_path to '%s' (%s)\n",
+		     gpg_path ? gpg_path : "(NULL)",
+		     notmuch_status_to_string (status));
+    }
+
     /* Index the message. */
-    status = add_file (notmuch, newpath, tag_ops, synchronize_flags, keep);
+    status = add_file (notmuch, newpath, tag_ops, synchronize_flags, keep, indexopts);
 
     /* Commit changes. */
     close_status = notmuch_database_destroy (notmuch);
@@ -577,5 +602,6 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	notmuch_run_hook (db_path, "post-insert");
     }
 
+    notmuch_indexopts_destroy (indexopts);
     return status ? EXIT_FAILURE : EXIT_SUCCESS;
 }
-- 
2.7.0.rc3

[PATCH v3 12/16] add --try-decrypt to notmuch new

[Daniel Kahn Gillmor]

Try to decrypt any encrypted parts of newly-discovered messages while
indexing them.  The cleartext of any successfully-decrypted messages
will be indexed, with tags applied in the same form as from notmuch
insert --try-decrypt.

If ~/.notmuch-config contains crypto.gpg_path, and gpg is needed for
indexing, the configuration option will be used to find gpg.
---
 completion/notmuch-completion.bash |  2 +-
 doc/man1/notmuch-new.rst           | 10 ++++++++++
 notmuch-new.c                      | 30 +++++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash
index 4bc9040..214f776 100644
--- a/completion/notmuch-completion.bash
+++ b/completion/notmuch-completion.bash
@@ -247,7 +247,7 @@ _notmuch_new()
 
     case "${cur}" in
 	-*)
-	    local options="--no-hooks --quiet ${_notmuch_shared_options}"
+	    local options="--no-hooks --try-decrypt --quiet ${_notmuch_shared_options}"
 	    compopt -o nospace
 	    COMPREPLY=( $(compgen -W "${options}" -- ${cur}) )
 	    ;;
diff --git a/doc/man1/notmuch-new.rst b/doc/man1/notmuch-new.rst
index 787ed78..cf08021 100644
--- a/doc/man1/notmuch-new.rst
+++ b/doc/man1/notmuch-new.rst
@@ -43,6 +43,16 @@ Supported options for **new** include
     ``--quiet``
         Do not print progress or results.
 
+    ``--try-decrypt``
+
+        For each message, if it is encrypted, try to decrypt it while
+        indexing.  If decryption is successful, index the cleartext
+        itself.  Be aware that the index is likely sufficient to
+        reconstruct the cleartext of the message itself, so please
+        ensure that the notmuch message index is adequately
+        protected. DO NOT USE THIS FLAG without considering the
+        security of your index.
+
 SEE ALSO
 ========
 
diff --git a/notmuch-new.c b/notmuch-new.c
index e503776..3d5efd5 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -49,6 +49,7 @@ typedef struct {
     size_t new_tags_length;
     const char **new_ignore;
     size_t new_ignore_length;
+    notmuch_indexopts_t *indexopts;
 
     int total_files;
     int processed_files;
@@ -260,7 +261,8 @@ add_file (notmuch_database_t *notmuch, const char *filename,
     if (status)
 	goto DONE;
 
-    status = notmuch_database_add_message (notmuch, filename, &message);
+    status = notmuch_database_add_message_with_indexopts (notmuch, filename,
+							  state->indexopts, &message);
     switch (status) {
     /* Success. */
     case NOTMUCH_STATUS_SUCCESS:
@@ -929,6 +931,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     add_files_state_t add_files_state = {
 	.verbosity = VERBOSITY_NORMAL,
 	.debug = FALSE,
+	.indexopts = NULL,
 	.output_is_a_tty = isatty (fileno (stdout)),
     };
     struct timeval tv_start;
@@ -942,6 +945,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     unsigned int i;
     notmuch_bool_t timer_is_active = FALSE;
     notmuch_bool_t no_hooks = FALSE;
+    notmuch_bool_t try_decrypt = FALSE;
     notmuch_bool_t quiet = FALSE, verbose = FALSE;
     notmuch_status_t status;
 
@@ -950,6 +954,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN,  &verbose, "verbose", 'v', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &add_files_state.debug, "debug", 'd', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
+	{ NOTMUCH_OPT_BOOLEAN,  &try_decrypt, "try-decrypt", 0, 0 },
 	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
 	{ 0, 0, 0, 0, 0 }
     };
@@ -1067,6 +1072,28 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (notmuch == NULL)
 	return EXIT_FAILURE;
 
+    add_files_state.indexopts = notmuch_indexopts_create ();
+    if (!add_files_state.indexopts) {
+	fprintf (stderr, "Error: could not create index options.\n");
+	return EXIT_FAILURE;
+    }
+    status = notmuch_indexopts_set_try_decrypt (add_files_state.indexopts, try_decrypt);
+    if (status != NOTMUCH_STATUS_SUCCESS) {
+	fprintf (stderr, "Error: Failed to set try_decrypt to %s. (%s)\n",
+		 try_decrypt ? "True" : "False", notmuch_status_to_string (status));
+	notmuch_indexopts_destroy (add_files_state.indexopts);
+	return EXIT_FAILURE;
+    }
+    if (try_decrypt) {
+	const char* gpg_path = notmuch_config_get_crypto_gpg_path (config);
+	status = notmuch_indexopts_set_gpg_path (add_files_state.indexopts, gpg_path);
+	if (status)
+	    fprintf (stderr, "Warning: failed to set database gpg_path to '%s' (%s)\n",
+		     gpg_path ? gpg_path : "(NULL)",
+		     notmuch_status_to_string (status));
+    }
+
+    
     /* Set up our handler for SIGINT. We do this after having
      * potentially done a database upgrade we this interrupt handler
      * won't support. */
@@ -1150,5 +1177,6 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
     if (!no_hooks && !ret && !interrupted)
 	ret = notmuch_run_hook (db_path, "post-new");
 
+    notmuch_indexopts_destroy (add_files_state.indexopts);
     return ret || interrupted ? EXIT_FAILURE : EXIT_SUCCESS;
 }
-- 
2.7.0.rc3

[PATCH v3 13/16] add indexopts to notmuch python bindings.

[Daniel Kahn Gillmor]

Make notmuch indexing options are not available in python as
the notmuch.Indexopts class.  Users can do something like:

 import notmuch
 d = notmuch.Database()
 indexopts = notmuch.Indexopts(try_decrypt=true)
 d.add_message(fname, indexopts=indexopts)
---
 bindings/python/notmuch/__init__.py  |  1 +
 bindings/python/notmuch/database.py  | 21 +++++---
 bindings/python/notmuch/globals.py   |  5 ++
 bindings/python/notmuch/indexopts.py | 97 ++++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 7 deletions(-)
 create mode 100644 bindings/python/notmuch/indexopts.py

diff --git a/bindings/python/notmuch/__init__.py b/bindings/python/notmuch/__init__.py
index 29416a5..fe2d886 100644
--- a/bindings/python/notmuch/__init__.py
+++ b/bindings/python/notmuch/__init__.py
@@ -54,6 +54,7 @@ Copyright 2010-2011 Sebastian Spaeth <Sebastian@SSpaeth.de>
 from .database import Database
 from .directory import Directory
 from .filenames import Filenames
+from .indexopts import Indexopts
 from .message import Message
 from .messages import Messages
 from .query import Query
diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index 93e7b7a..1b0ee1a 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -29,6 +29,7 @@ from .globals import (
     NotmuchDirectoryP,
     NotmuchMessageP,
     NotmuchTagsP,
+    NotmuchIndexoptsP,
 )
 from .errors import (
     STATUS,
@@ -383,12 +384,13 @@ class Database(object):
         # return the Directory, init it with the absolute path
         return Directory(abs_dirpath, dir_p, self)
 
-    _add_message = nmlib.notmuch_database_add_message
-    _add_message.argtypes = [NotmuchDatabaseP, c_char_p,
-                             POINTER(NotmuchMessageP)]
-    _add_message.restype = c_uint
-
-    def add_message(self, filename, sync_maildir_flags=False):
+    _add_message_with_indexopts = nmlib.notmuch_database_add_message_with_indexopts
+    _add_message_with_indexopts.argtypes = [NotmuchDatabaseP, c_char_p,
+                                            NotmuchIndexoptsP,
+                                            POINTER(NotmuchMessageP)]
+    _add_message_with_indexopts.restype = c_uint
+        
+    def add_message(self, filename, sync_maildir_flags=False, indexopts=None):
         """Adds a new message to the database
 
         :param filename: should be a path relative to the path of the
@@ -409,6 +411,9 @@ class Database(object):
             API. You might want to look into the underlying method
             :meth:`Message.maildir_flags_to_tags`.
 
+        :param indexopts: a nomtuch.Indexopts object indicating custom
+            options desired for indexing.
+
         :returns: On success, we return
 
            1) a :class:`Message` object that can be used for things
@@ -436,10 +441,12 @@ class Database(object):
               :attr:`STATUS`.READ_ONLY_DATABASE
                       Database was opened in read-only mode so no message can
                       be added.
+
         """
         self._assert_db_is_initialized()
         msg_p = NotmuchMessageP()
-        status = self._add_message(self._db, _str(filename), byref(msg_p))
+
+        status = self._add_message_with_indexopts(self._db, _str(filename), indexopts._indexopts, byref(msg_p))
 
         if not status in [STATUS.SUCCESS, STATUS.DUPLICATE_MESSAGE_ID]:
             raise NotmuchError(status)
diff --git a/bindings/python/notmuch/globals.py b/bindings/python/notmuch/globals.py
index 6872a29..c4b9832 100644
--- a/bindings/python/notmuch/globals.py
+++ b/bindings/python/notmuch/globals.py
@@ -88,3 +88,8 @@ NotmuchDirectoryP = POINTER(NotmuchDirectoryS)
 class NotmuchFilenamesS(Structure):
     pass
 NotmuchFilenamesP = POINTER(NotmuchFilenamesS)
+
+
+class NotmuchIndexoptsS(Structure):
+    pass
+NotmuchIndexoptsP = POINTER(NotmuchIndexoptsS)
diff --git a/bindings/python/notmuch/indexopts.py b/bindings/python/notmuch/indexopts.py
new file mode 100644
index 0000000..b0d4603
--- /dev/null
+++ b/bindings/python/notmuch/indexopts.py
@@ -0,0 +1,97 @@
+"""
+This file is part of notmuch.
+
+Notmuch is free software: you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by the
+Free Software Foundation, either version 3 of the License, or (at your
+option) any later version.
+
+Notmuch is distributed in the hope that it will be useful, but WITHOUT
+ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with notmuch.  If not, see <http://www.gnu.org/licenses/>.
+
+Copyright 2015 Daniel Kahn Gillmor <dkg@fifthhorseman.net>
+"""
+from ctypes import c_char_p, c_bool, c_int
+from .globals import (
+    nmlib,
+    NotmuchIndexoptsP,
+)
+from .errors import (
+    STATUS,
+    NullPointerError,
+    NotInitializedError,
+)
+
+
+class Indexopts(object):
+    """Represents available options for notmuch indexing.
+    """
+
+    # create
+    _create = nmlib.notmuch_indexopts_create
+    _create.argtypes = []
+    _create.restype = NotmuchIndexoptsP
+
+    def __init__(self, try_decrypt=False, gpg_path=None):
+        """
+        :param try_decrypt: True if notmuch should try to decrypt messages
+             while indexing, and index the cleartext.
+
+        :param gpg_path: the name or path to the preferred GnuPG binary.
+        """
+        self._indexopts = Indexopts._create()
+        self.gpg_path = gpg_path
+        self.try_decrypt = try_decrypt
+
+    # try_decrypt
+    _get_try_decrypt = nmlib.notmuch_indexopts_get_try_decrypt
+    _get_try_decrypt.argtypes = [NotmuchIndexoptsP]
+    _get_try_decrypt.restype = bool
+
+    _set_try_decrypt = nmlib.notmuch_indexopts_set_try_decrypt
+    _set_try_decrypt.argtypes = [NotmuchIndexoptsP, c_bool]
+    _set_try_decrypt.restype = c_int
+
+    @property
+    def try_decrypt(self):
+        return Indexopts._get_try_decrypt(self._indexopts)
+
+    @try_decrypt.setter
+    def try_decrypt(self, try_decrypt):
+        status = Indexopts._set_try_decrypt(self._indexopts, try_decrypt)
+        if status != STATUS.SUCCESS:
+            raise NotmuchError(status)
+
+    # gpg_path
+    _get_gpg_path = nmlib.notmuch_indexopts_get_gpg_path
+    _get_gpg_path.argtypes = [NotmuchIndexoptsP]
+    _get_gpg_path.restype = c_char_p
+
+    _set_gpg_path = nmlib.notmuch_indexopts_set_gpg_path
+    _set_gpg_path.argtypes = [NotmuchIndexoptsP, c_char_p]
+    _set_gpg_path.restype = c_int
+
+    @property
+    def gpg_path(self):
+        return Indexopts._get_gpg_path(self._indexopts)
+
+    @gpg_path.setter
+    def gpg_path(self, gpg_path):
+        status = Indexopts._set_gpg_path(self._indexopts, gpg_path)
+        if status != STATUS.SUCCESS:
+            raise NotmuchError(status)
+    
+
+    _destroy = nmlib.notmuch_indexopts_destroy
+    _destroy.argtypes = [NotmuchIndexoptsP]
+    _destroy.restype = None
+
+    def __del__(self):
+        """Close and free the indexopts"""
+        if self._indexopts:
+            self._destroy(self._indexopts)
-- 
2.7.0.rc3

[PATCH v3 14/16] test indexing cleartext version of delivered messages.

[Daniel Kahn Gillmor]

This requires a bit of reorganization:

 * add_gnupg_home gets moved to test-lib.sh, and

 * we allow passing --long-arguments to "notmuch new" via
   emacs_fcc_message
---
 test/T350-crypto.sh           | 15 ---------------
 test/T357-index-decryption.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 test/test-lib.sh              | 26 +++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 16 deletions(-)
 create mode 100755 test/T357-index-decryption.sh

diff --git a/test/T350-crypto.sh b/test/T350-crypto.sh
index 4bc15bc..50cc526 100755
--- a/test/T350-crypto.sh
+++ b/test/T350-crypto.sh
@@ -7,21 +7,6 @@
 test_description='PGP/MIME signature verification and decryption'
 . ./test-lib.sh || exit 1
 
-add_gnupg_home ()
-{
-    local output
-    [ -d ${GNUPGHOME} ] && return
-    mkdir -m 0700 "$GNUPGHOME"
-    $GPG --no-tty --import <$TEST_DIRECTORY/gnupg-secret-key.asc >"$GNUPGHOME"/import.log 2>&1
-    test_debug "cat $GNUPGHOME/import.log"
-    if ($GPG --quick-random --version >/dev/null 2>&1) ; then
-	echo quick-random >> "$GNUPGHOME"/gpg.conf
-    elif ($GPG --debug-quick-random --version >/dev/null 2>&1) ; then
-	echo debug-quick-random >> "$GNUPGHOME"/gpg.conf
-    fi
-    echo no-emit-version >> "$GNUPGHOME"/gpg.conf
-}
-
 ##################################################
 
 add_gnupg_home
diff --git a/test/T357-index-decryption.sh b/test/T357-index-decryption.sh
new file mode 100755
index 0000000..03e49cc
--- /dev/null
+++ b/test/T357-index-decryption.sh
@@ -0,0 +1,42 @@
+#!/usr/bin/env bash
+
+# TODO: test index-decryption-failed
+
+test_description='indexing decrypted mail'
+. ./test-lib.sh || exit 1
+
+##################################################
+
+add_gnupg_home
+# get key fingerprint
+FINGERPRINT=$($GPG --no-tty --list-secret-keys --with-colons --fingerprint | grep '^fpr:' | cut -d: -f10)
+
+# create a test encrypted message
+test_expect_success 'emacs delivery of encrypted message' \
+'emacs_fcc_message \
+    "test encrypted message for cleartext index 001" \
+    "This is a test encrypted message with a wumpus.\n" \
+    "(mml-secure-message-encrypt)"'
+
+test_begin_subtest "search for unindexed cleartext"
+output=$(notmuch search wumpus)
+expected=''
+test_expect_equal \
+    "$output" \
+    "$expected"
+
+# create a test encrypted message that is indexed in the clear
+test_expect_success 'emacs delivery of encrypted message' \
+'emacs_fcc_message --try-decrypt \
+    "test encrypted message for cleartext index 002" \
+    "This is a test encrypted message with a wumpus.\n" \
+    "(mml-secure-message-encrypt)"'
+
+test_begin_subtest "emacs delivery of encrypted message, indexed cleartext"
+output=$(notmuch search wumpus)
+expected='thread:0000000000000002   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 002 (encrypted inbox index-decrypted)'
+test_expect_equal \
+    "$output" \
+    "$expected"
+
+test_done
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 8c1f3c6..1fea6c7 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -92,6 +92,21 @@ else
     GPG=gpg
 fi
 
+add_gnupg_home ()
+{
+    local output
+    [ -d ${GNUPGHOME} ] && return
+    mkdir -m 0700 "$GNUPGHOME"
+    $GPG --no-tty --import <$TEST_DIRECTORY/gnupg-secret-key.asc >"$GNUPGHOME"/import.log 2>&1
+    test_debug "cat $GNUPGHOME/import.log"
+    if ($GPG --quick-random --version >/dev/null 2>&1) ; then
+	echo quick-random >> "$GNUPGHOME"/gpg.conf
+    elif ($GPG --debug-quick-random --version >/dev/null 2>&1) ; then
+	echo debug-quick-random >> "$GNUPGHOME"/gpg.conf
+    fi
+    echo no-emit-version >> "$GNUPGHOME"/gpg.conf
+}
+
 # Convenience
 #
 # A regexp to match 5 and 40 hexdigits
@@ -514,8 +529,17 @@ emacs_deliver_message ()
 # Accepts arbitrary extra emacs/elisp functions to modify the message
 # before sending, which is useful to doing things like attaching files
 # to the message and encrypting/signing.
+#
+# If any GNU-style long-arguments (like --quiet or --try-decrypt) are
+# at the head of the argument list, they are sent directly to "notmuch
+# new" after message delivery
 emacs_fcc_message ()
 {
+    local nmn_args=''
+    while [[ "$1" =~ ^-- ]]; do
+        nmn_args="$nmn_args $1"
+        shift
+    done
     local subject="$1"
     local body="$2"
     shift 2
@@ -534,7 +558,7 @@ emacs_fcc_message ()
 	   (insert \"${body}\")
 	   $@
 	   (notmuch-mua-send-and-exit))" || return 1
-    notmuch new >/dev/null
+    notmuch new $nmn_args >/dev/null
 }
 
 # Generate a corpus of email and add it to the database.
-- 
2.7.0.rc3

[PATCH v3 15/16] added notmuch_message_reindex

[Daniel Kahn Gillmor]

This new function asks the database to reindex a given message, using
the supplied indexopts.

This can be used, for example, to index the cleartext of an encrypted
message.
---
 lib/message.cc | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 lib/notmuch.h  | 14 +++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/lib/message.cc b/lib/message.cc
index 8d72ea2..3b35418 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -529,7 +529,9 @@ static void
 _notmuch_message_remove_terms (notmuch_message_t *message, const char *prefix)
 {
     Xapian::TermIterator i;
-    size_t prefix_len = strlen (prefix);
+    size_t prefix_len = 0;
+
+    prefix_len = strlen (prefix);
 
     while (1) {
 	i = message->doc.termlist_begin ();
@@ -1667,3 +1669,90 @@ _notmuch_message_database (notmuch_message_t *message)
 {
     return message->notmuch;
 }
+
+notmuch_status_t
+notmuch_message_reindex (notmuch_message_t *message,
+			 notmuch_indexopts_t *indexopts)
+{
+    notmuch_database_t *notmuch = NULL;
+    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS, status;
+    notmuch_tags_t *tags = NULL;
+    notmuch_filenames_t *filenames, *orig_filenames = NULL;
+    const char *filename = NULL, *tag = NULL;
+    notmuch_message_t *newmsg = NULL;
+    notmuch_bool_t readded = FALSE, skip;
+    const char *autotags[] = {
+		    "attachment",
+		    "encrypted",
+		    "signed",
+		    "index-decrypted",
+		    "index-decryption-failed" };
+
+    if (message == NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
+    
+    notmuch = _notmuch_message_database (message);
+
+    /* cache tags and filenames */
+    tags = notmuch_message_get_tags(message);
+    filenames = notmuch_message_get_filenames(message);
+    orig_filenames = notmuch_message_get_filenames(message);
+    
+    /* walk through filenames, removing them until the message is gone */
+    for ( ; notmuch_filenames_valid (filenames);
+	  notmuch_filenames_move_to_next (filenames)) {
+	filename = notmuch_filenames_get (filenames);
+
+	ret = notmuch_database_remove_message (notmuch, filename);
+	if (ret != NOTMUCH_STATUS_SUCCESS &&
+	    ret != NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+	    return ret;
+    }
+    if (ret != NOTMUCH_STATUS_SUCCESS)
+	return ret;
+    
+    /* re-add the filenames with the associated indexopts */
+    for (; notmuch_filenames_valid (orig_filenames);
+	 notmuch_filenames_move_to_next (orig_filenames)) {
+	filename = notmuch_filenames_get (orig_filenames);
+
+	status = notmuch_database_add_message_with_indexopts(notmuch,
+							     filename,
+							     indexopts,
+							     readded ? NULL : &newmsg);
+	if (status == NOTMUCH_STATUS_SUCCESS ||
+	    status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
+	    if (!readded) {
+		/* re-add tags */
+		for (; notmuch_tags_valid (tags);
+		     notmuch_tags_move_to_next (tags)) {
+		    tag = notmuch_tags_get (tags);
+		    skip = FALSE;
+		    
+		    for (size_t i = 0; i < ARRAY_SIZE(autotags); i++)
+			if (strcmp (tag, autotags[i]) == 0)
+			    skip = TRUE;
+		    
+		    if (!skip) {
+			status = notmuch_message_add_tag (newmsg, tag);
+			if (status != NOTMUCH_STATUS_SUCCESS)
+			    ret = status;
+		    }
+		}
+		readded = TRUE;
+	    }
+	} else {
+	    /* if we failed to add this filename, go ahead and try the
+	     * next one as though it were first, but report the
+	     * error... */
+	    ret = status;
+	}
+    }
+    if (newmsg)
+	notmuch_message_destroy (newmsg);
+	    		
+    /* should we also destroy the incoming message object?  at the
+     * moment, we leave that to the caller */
+    return ret;
+}
+
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 854a451..e6287cd 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1377,6 +1377,20 @@ notmuch_filenames_t *
 notmuch_message_get_filenames (notmuch_message_t *message);
 
 /**
+ * Re-index the e-mail corresponding to 'message' using the supplied index options
+ *
+ * Returns the status of the re-index operation.  (see the return
+ * codes documented in notmuch_database_add_message)
+ *
+ * After reindexing, the user should discard the message object passed
+ * in here by calling notmuch_message_destroy, since it refers to the
+ * original message, not to the reindexed message.
+ */
+notmuch_status_t
+notmuch_message_reindex (notmuch_message_t *message,
+			 notmuch_indexopts_t *indexopts);
+
+/**
  * Message flags.
  */
 typedef enum _notmuch_message_flag {
-- 
2.7.0.rc3

[PATCH v3 16/16] add "notmuch reindex" subcommand

[Daniel Kahn Gillmor]

This new subcommand takes a set of search terms, and re-indexes the
list of matching messages using the supplied options.

This can be used to index the cleartext of encrypted messages with
something like:

 notmuch reindex --try-decrypt \
    tag:encrypted and not tag:index-decrypted
---
 Makefile.local                    |   1 +
 doc/conf.py                       |   7 ++
 doc/man1/notmuch-reindex.rst      |  41 ++++++++++
 doc/man1/notmuch.rst              |   1 +
 doc/man7/notmuch-search-terms.rst |   7 +-
 notmuch-client.h                  |   3 +
 notmuch-reindex.c                 | 152 ++++++++++++++++++++++++++++++++++++++
 notmuch.c                         |   2 +
 test/T357-index-decryption.sh     |  53 +++++++++++++
 9 files changed, 265 insertions(+), 2 deletions(-)
 create mode 100644 doc/man1/notmuch-reindex.rst
 create mode 100644 notmuch-reindex.c

diff --git a/Makefile.local b/Makefile.local
index 6206771..e03a83d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -281,6 +281,7 @@ notmuch_client_srcs =		\
 	notmuch-dump.c		\
 	notmuch-insert.c	\
 	notmuch-new.c		\
+	notmuch-reindex.c       \
 	notmuch-reply.c		\
 	notmuch-restore.c	\
 	notmuch-search.c	\
diff --git a/doc/conf.py b/doc/conf.py
index 65adafe..f98d67a 100644
--- a/doc/conf.py
+++ b/doc/conf.py
@@ -94,6 +94,10 @@ man_pages = [
         u'incorporate new mail into the notmuch database',
         [u'Carl Worth and many others'], 1),
 
+('man1/notmuch-reindex','notmuch-reindex',
+        u're-index matching messages',
+        [u'Carl Worth and many others'], 1),
+
 ('man1/notmuch-reply','notmuch-reply',
         u'constructs a reply template for a set of messages',
         [u'Carl Worth and many others'], 1),
@@ -162,6 +166,9 @@ texinfo_documents = [
 ('man1/notmuch-new','notmuch-new',u'notmuch Documentation',
       u'Carl Worth and many others', 'notmuch-new',
       'incorporate new mail into the notmuch database','Miscellaneous'),
+('man1/notmuch-reindex','notmuch-reindex',u'notmuch Documentation',
+      u'Carl Worth and many others', 'notmuch-reindex',
+      're-index matching messages','Miscellaneous'),
 ('man1/notmuch-reply','notmuch-reply',u'notmuch Documentation',
       u'Carl Worth and many others', 'notmuch-reply',
       'constructs a reply template for a set of messages','Miscellaneous'),
diff --git a/doc/man1/notmuch-reindex.rst b/doc/man1/notmuch-reindex.rst
new file mode 100644
index 0000000..7ccc947
--- /dev/null
+++ b/doc/man1/notmuch-reindex.rst
@@ -0,0 +1,41 @@
+===========
+notmuch-reindex
+===========
+
+SYNOPSIS
+========
+
+**notmuch** **reindex** [*option* ...] <*search-term*> ...
+
+DESCRIPTION
+===========
+
+Re-index all messages matching the search terms.
+
+See **notmuch-search-terms(7)** for details of the supported syntax for
+<*search-term*\ >.
+
+The **reindex** command searches for all messages matching the
+supplied search terms, and re-creates the full-text index on these
+messages using the supplied options.
+
+Supported options for **reindex** include
+
+    ``--try-decrypt``
+
+        For each message, if it is encrypted, try to decrypt it while
+        indexing.  If decryption is successful, index the cleartext
+        itself.  Be aware that the index is likely sufficient to
+        reconstruct the cleartext of the message itself, so please
+        ensure that the notmuch message index is adequately
+        protected. DO NOT USE THIS FLAG without considering the
+        security of your index.
+
+SEE ALSO
+========
+
+**notmuch(1)**, **notmuch-config(1)**, **notmuch-count(1)**,
+**notmuch-dump(1)**, **notmuch-hooks(5)**, **notmuch-insert(1)**,
+**notmuch-new(1)**,
+**notmuch-reply(1)**, **notmuch-restore(1)**, **notmuch-search(1)**,
+**notmuch-search-terms(7)**, **notmuch-show(1)**, **notmuch-tag(1)**
diff --git a/doc/man1/notmuch.rst b/doc/man1/notmuch.rst
index 3acfbdb..d9ba146 100644
--- a/doc/man1/notmuch.rst
+++ b/doc/man1/notmuch.rst
@@ -140,6 +140,7 @@ SEE ALSO
 
 **notmuch-config(1)**, **notmuch-count(1)**, **notmuch-dump(1)**,
 **notmuch-hooks(5)**, **notmuch-insert(1)**, **notmuch-new(1)**,
+**notmuch-reindex(1)**,
 **notmuch-reply(1)**, **notmuch-restore(1)**, **notmuch-search(1)**,
 **notmuch-search-terms(7)**, **notmuch-show(1)**, **notmuch-tag(1)**,
 **notmuch-address(1)**
diff --git a/doc/man7/notmuch-search-terms.rst b/doc/man7/notmuch-search-terms.rst
index 2fbc16d..f9c4676 100644
--- a/doc/man7/notmuch-search-terms.rst
+++ b/doc/man7/notmuch-search-terms.rst
@@ -9,6 +9,8 @@ SYNOPSIS
 
 **notmuch** **dump** [--format=(batch-tag|sup)] [--] [--output=<*file*>] [--] [<*search-term*> ...]
 
+**notmuch** **reindex** [option ...] <*search-term*> ...
+
 **notmuch** **search** [option ...] <*search-term*> ...
 
 **notmuch** **show** [option ...] <*search-term*> ...
@@ -375,5 +377,6 @@ SEE ALSO
 
 **notmuch(1)**, **notmuch-config(1)**, **notmuch-count(1)**,
 **notmuch-dump(1)**, **notmuch-hooks(5)**, **notmuch-insert(1)**,
-**notmuch-new(1)**, **notmuch-reply(1)**, **notmuch-restore(1)**,
-**notmuch-search(1)**, **notmuch-show(1)**, **notmuch-tag(1)**
+**notmuch-new(1)**, **notmuch-reindex(1)**, **notmuch-reply(1)**,
+**notmuch-restore(1)**, **notmuch-search(1)**, **notmuch-show(1)**,
+**notmuch-tag(1)**
diff --git a/notmuch-client.h b/notmuch-client.h
index a41e90a..89b1180 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -169,6 +169,9 @@ int
 notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[]);
 
 int
+notmuch_reindex_command (notmuch_config_t *config, int argc, char *argv[]);
+
+int
 notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[]);
 
 int
diff --git a/notmuch-reindex.c b/notmuch-reindex.c
new file mode 100644
index 0000000..6fc88c5
--- /dev/null
+++ b/notmuch-reindex.c
@@ -0,0 +1,152 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2016 Daniel Kahn Gillmor
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
+ */
+
+#include "notmuch-client.h"
+#include "string-util.h"
+
+static volatile sig_atomic_t interrupted;
+
+static void
+handle_sigint (unused (int sig))
+{
+    static char msg[] = "Stopping...         \n";
+
+    /* This write is "opportunistic", so it's okay to ignore the
+     * result.  It is not required for correctness, and if it does
+     * fail or produce a short write, we want to get out of the signal
+     * handler as quickly as possible, not retry it. */
+    IGNORE_RESULT (write (2, msg, sizeof (msg) - 1));
+    interrupted = 1;
+}
+
+/* reindex all messages matching 'query_string' using the passed-in indexopts
+ */
+static int
+reindex_query (notmuch_database_t *notmuch, const char *query_string,
+	       notmuch_indexopts_t *indexopts)
+{
+    notmuch_query_t *query;
+    notmuch_messages_t *messages;
+    notmuch_message_t *message;
+    notmuch_status_t status;
+
+    int ret = NOTMUCH_STATUS_SUCCESS;
+
+    query = notmuch_query_create (notmuch, query_string);
+    if (query == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	return 1;
+    }
+
+    /* reindexing is not interested in any special sort order */
+    notmuch_query_set_sort (query, NOTMUCH_SORT_UNSORTED);
+
+    status = notmuch_query_search_messages_st (query, &messages);
+    if (print_status_query ("notmuch reindex", query, status))
+	return status;
+
+    for (;
+	 notmuch_messages_valid (messages) && ! interrupted;
+	 notmuch_messages_move_to_next (messages)) {
+	message = notmuch_messages_get (messages);
+
+	notmuch_message_reindex(message, indexopts);
+	notmuch_message_destroy (message);
+	if (ret != NOTMUCH_STATUS_SUCCESS)
+	    break;
+    }
+
+    notmuch_query_destroy (query);
+
+    return ret || interrupted;
+}
+
+int
+notmuch_reindex_command (notmuch_config_t *config, int argc, char *argv[])
+{
+    char *query_string = NULL;
+    notmuch_database_t *notmuch;
+    struct sigaction action;
+    notmuch_bool_t try_decrypt = FALSE;
+    int opt_index;
+    int ret;
+    notmuch_status_t status;
+    notmuch_indexopts_t *indexopts = NULL;
+
+    /* Set up our handler for SIGINT */
+    memset (&action, 0, sizeof (struct sigaction));
+    action.sa_handler = handle_sigint;
+    sigemptyset (&action.sa_mask);
+    action.sa_flags = SA_RESTART;
+    sigaction (SIGINT, &action, NULL);
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_BOOLEAN, &try_decrypt, "try-decrypt", 0, 0 },
+	{ NOTMUCH_OPT_INHERIT, (void *) &notmuch_shared_options, NULL, 0, 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
+	return EXIT_FAILURE;
+
+    notmuch_process_shared_options (argv[0]);
+
+    if (notmuch_database_open (notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+	return EXIT_FAILURE;
+
+    notmuch_exit_if_unmatched_db_uuid (notmuch);
+
+    indexopts = notmuch_indexopts_create();
+    if (!indexopts)
+	return EXIT_FAILURE;
+
+    status = notmuch_indexopts_set_try_decrypt (indexopts, try_decrypt);
+    if (status)
+	fprintf (stderr, "Warning: failed to set --try-decrypt to %d (%s)\n",
+		 try_decrypt, notmuch_status_to_string (status));
+
+    if (try_decrypt) {
+	const char* gpg_path = notmuch_config_get_crypto_gpg_path (config);
+	status = notmuch_indexopts_set_gpg_path (indexopts, gpg_path);
+	if (status)
+	    fprintf (stderr, "Warning: failed to set gpg_path for reindexing to '%s' (%s)\n",
+		     gpg_path ? gpg_path : "(NULL)",
+		     notmuch_status_to_string (status));
+    }
+
+    query_string = query_string_from_args (config, argc-opt_index, argv+opt_index);
+    if (query_string == NULL) {
+	fprintf (stderr, "Out of memory\n");
+	return EXIT_FAILURE;
+    }
+
+    if (*query_string == '\0') {
+	fprintf (stderr, "Error: notmuch reindex requires at least one search term.\n");
+	return EXIT_FAILURE;
+    }
+    
+    ret = reindex_query (notmuch, query_string, indexopts);
+
+    notmuch_database_destroy (notmuch);
+
+    return ret || interrupted ? EXIT_FAILURE : EXIT_SUCCESS;
+}
diff --git a/notmuch.c b/notmuch.c
index ce6c575..df9cf4e 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -123,6 +123,8 @@ static command_t commands[] = {
       "Restore the tags from the given dump file (see 'dump')." },
     { "compact", notmuch_compact_command, FALSE,
       "Compact the notmuch database." },
+    { "reindex", notmuch_reindex_command, FALSE,
+      "Re-index all messages matching the search terms." },
     { "config", notmuch_config_command, FALSE,
       "Get or set settings in the notmuch configuration file." },
     { "help", notmuch_help_command, TRUE, /* create but don't save config */
diff --git a/test/T357-index-decryption.sh b/test/T357-index-decryption.sh
index 03e49cc..c60e152 100755
--- a/test/T357-index-decryption.sh
+++ b/test/T357-index-decryption.sh
@@ -39,4 +39,57 @@ test_expect_equal \
     "$output" \
     "$expected"
 
+# add a tag to all messages to ensure that it stays after reindexing
+test_expect_success 'tagging all messages' \
+                    'notmuch tag +blarney "encrypted message"'
+test_begin_subtest "verify that tags are all present"
+output=$(notmuch search tag:blarney)
+expected='thread:0000000000000001   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 001 (blarney encrypted inbox)
+thread:0000000000000002   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 002 (blarney encrypted inbox index-decrypted)'
+test_expect_equal \
+    "$output" \
+    "$expected"
+
+# see if first message shows up after reindexing with --try-decrypt
+test_expect_success 'reindex old messages' \
+                    'notmuch reindex --try-decrypt tag:encrypted and not tag:index-decrypted'
+test_begin_subtest "reindexed encrypted message, including cleartext"
+output=$(notmuch search wumpus)
+expected='thread:0000000000000002   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 002 (blarney encrypted inbox index-decrypted)
+thread:0000000000000003   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 001 (blarney encrypted inbox index-decrypted)'
+test_expect_equal \
+    "$output" \
+    "$expected"
+
+
+# try to remove cleartext indexing
+test_expect_success 'reindex without cleartext' \
+                    'notmuch reindex tag:encrypted and tag:index-decrypted'
+test_begin_subtest "reindexed encrypted messages, without cleartext"
+output=$(notmuch search wumpus)
+expected=''
+test_expect_equal \
+    "$output" \
+    "$expected"
+
+# ensure that the tags remain even when we are dropping the cleartext.
+test_begin_subtest "verify that tags remain without cleartext"
+output=$(notmuch search tag:blarney)
+expected='thread:0000000000000004   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 002 (blarney encrypted inbox)
+thread:0000000000000005   2000-01-01 [1/1] Notmuch Test Suite; test encrypted message for cleartext index 001 (blarney encrypted inbox)'
+test_expect_equal \
+    "$output" \
+    "$expected"
+
+
+# TODO: test removal of a message from the message store between
+# indexing and reindexing.
+
+# TODO: insert the same message into the message store twice, index,
+# remove one of them from the message store, and then reindex.
+# reindexing should return a failure but the message should still be
+# present? -- or what should the semantics be if you ask to reindex a
+# message whose underlying files have been renamed or moved or
+# removed?
+
 test_done
-- 
2.7.0.rc3

Re: Allow indexing cleartext of encrypted messages (v3)

[Tomi Ollila]

On Sun, Jan 31 2016, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:

> This is the third draft of the series initially announced in
> id:1449718786-28000-1-git-send-email-dkg@fifthhorseman.net (second
> draft was in
> id:1453258369-7366-1-git-send-email-dkg@fifthhorseman.net).  It
> differs from v2 in that it incorporates the recent improvements in
> detecting and processing S/MIME signatures.

Looks pretty good. Nothing to bikeshed. Did not run tests yet.

Tomi


>
> From the v2 description:
>
>> Notmuch currently doesn't index the cleartext of encrypted mail.  This
>> is the right choice by default, because the index is basically
>> cleartext-equivalent, and we wouldn't want every indexed mailstore to
>> leak the contents of its encrypted mails.
>> 
>> However, if a notmuch user has their index in a protected location,
>> they may prefer the convenience of being able to search the contents
>> of (at least some of) their encrypted mail.
>> 
>> This series of patches enables notmuch to index the cleartext of
>> specific encrypted messages when they're being added via "notmuch new"
>> or "notmuch insert", via a new --try-decrypt flag.
>> 
>> If --try-decrypt is used, and decryption is successful for part of a
>> message, the message gets an additional "index-decrypted" tag.  If
>> decryption of part of a message fails, the message gets an additional
>> "index-decryption-failed" tag.
>
> v2 addresses the concerns raised from the helpful feedback on the
> previous series, and adds a notmuch_indexopts_t object that can be
> used to declare options for indexing messages, including a
> "try_decrypt" boolean.
>
> Additionally, this series adds a new function to libnotmuch:
>
>   notmuch_message_reindex (notmuch_message_t *message,
>                            notmuch_indexopts_t *indexopts)
>
> Which allows user of the library to adjust the indexing options of a
> given message.
>
> The CLI is additionally augmented with a new notmuch subcommand,
> "notmuch reindex", which also has a --try-decrypt flag.
>
> So a user who has their message index stored securely and wants to
> index the cleartext of all encrypted messages they've received can do
> something like:
>
>   notmuch reindex --try-decrypt tag:encrypted and not tag:index-decrypted
>
> Or can clear all indexed cleartext from their database with:
>
>   notmuch reindex tag:encrypted and tag:index-decrypted
>
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: Allow indexing cleartext of encrypted messages (v3)

[Jameson Graef Rollins]

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

I've done only a cursory read through of the code, but structurally it
looks good to me.  I like the reworking of crypto.c as a utility
library.  It applies cleaning to trunk, and all tests pass, including
the 11 new ones.

jamie.

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

Re: [PATCH v3 01/16] add util/search-path.{c, h} to test for executables in $PATH

[David Bremner]

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

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

> +notmuch_bool_t
> +test_for_executable(const char* exename)
> +{
> +    char *c = NULL, *save = NULL, *tok;
> +    size_t n;
> +    int dfd = -1;

c, dfd, and n could be more meaningful as variable names.

% uncrustify --config devel/uncrustify.cfg --replace util/search-path.c util/search-path.h

yields quite a few whitespace changes (diff attached)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wschanges.diff --]
[-- Type: text/x-diff, Size: 2191 bytes --]

diff --git a/util/search-path.c b/util/search-path.c
index 5eac367..39601e4 100644
--- a/util/search-path.c
+++ b/util/search-path.c
@@ -9,47 +9,47 @@
 
 
 notmuch_bool_t
-test_for_executable(const char* exename)
+test_for_executable (const char *exename)
 {
     char *c = NULL, *save = NULL, *tok;
     size_t n;
     int dfd = -1;
     notmuch_bool_t ret = FALSE;
 
-    if (strchr(exename, '/')) {
-	if (0 == access(exename, X_OK))
+    if (strchr (exename, '/')) {
+	if (0 == access (exename, X_OK))
 	    return TRUE;
 	else
 	    return FALSE;
     }
-    
-    c = getenv("PATH");
+
+    c = getenv ("PATH");
     if (c)
-	c = talloc_strdup(NULL, c);
+	c = talloc_strdup (NULL, c);
     else {
-	n = confstr(_CS_PATH, NULL, 0);
-	c = (char*)talloc_size(NULL, n);
-	if (!c)
+	n = confstr (_CS_PATH, NULL, 0);
+	c = (char *) talloc_size (NULL, n);
+	if (! c)
 	    return FALSE;
-	confstr(_CS_PATH, c, n);
+	confstr (_CS_PATH, c, n);
     }
 
-    tok = strtok_r(c, ":", &save);
+    tok = strtok_r (c, ":", &save);
     while (tok) {
-	dfd = open(tok, O_DIRECTORY | O_RDONLY);
+	dfd = open (tok, O_DIRECTORY | O_RDONLY);
 	if (dfd != -1) {
-	    if (!faccessat(dfd, exename, X_OK, 0)) {
+	    if (! faccessat (dfd, exename, X_OK, 0)) {
 		ret = TRUE;
 		goto done;
 	    }
-	    close(dfd);
+	    close (dfd);
 	}
-	tok = strtok_r(NULL, ":", &save);
+	tok = strtok_r (NULL, ":", &save);
     }
-done:
+  done:
     if (dfd != -1)
-	close(dfd);
+	close (dfd);
     if (c)
-	talloc_free(c);
+	talloc_free (c);
     return ret;
 }
diff --git a/util/search-path.h b/util/search-path.h
index 727d0b3..14c4d14 100644
--- a/util/search-path.h
+++ b/util/search-path.h
@@ -4,13 +4,13 @@
 #include "notmuch.h"
 
 /* can an executable be found with the given name?
- * 
+ *
  * Return TRUE only if we can find something to execute with the
  * associated name.
  *
  * if the name has a '/' in it, we look for it directly with
  * access(exename, X_OK).
- * 
+ *
  * otherwise, we look for it in $PATH (or in confstr(_CS_PATH), if
  * $PATH is unset).
  *
@@ -19,6 +19,6 @@
  */
 
 notmuch_bool_t
-test_for_executable(const char *exename);
+test_for_executable (const char *exename);
 
 #endif

[-- Attachment #3: Type: text/plain, Size: 922 bytes --]


> +    notmuch_bool_t ret = FALSE;
> +
> +    if (strchr(exename, '/')) {
> +	if (0 == access(exename, X_OK))
> +	    return TRUE;
> +	else
> +	    return FALSE;
> +    }
> +    
> +    c = getenv("PATH");
> +    if (c)
> +	c = talloc_strdup(NULL, c);

Is there some advantage to using the talloc_ functions here?

> +    else {

Is n needed outside this block? if not, it could be declared here (and
then I don't care about the single letter name).

> +	n = confstr(_CS_PATH, NULL, 0);

according to a glance at the man page, this might return 0 if there is
no value for _CS_PATH set?


> +
> +    tok = strtok_r(c, ":", &save);
> +    while (tok) {
same comment about block local declaration of dfd

> +	dfd = open(tok, O_DIRECTORY | O_RDONLY);

> +	tok = strtok_r(NULL, ":", &save);

not sure if it helps, but there is also strtok_len in libutil

> +done:

as a real nitpick, we have always (?) used DONE for a label.

[PATCH v4] add util/search-path.{c, h} to test for executables in $PATH

[Daniel Kahn Gillmor]

This is a utility function we can use to see whether an executable is
available.
---
 util/Makefile.local |  2 +-
 util/search-path.c  | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 util/search-path.h  | 24 ++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 util/search-path.c
 create mode 100644 util/search-path.h

diff --git a/util/Makefile.local b/util/Makefile.local
index 905f237..8b2b91b 100644
--- a/util/Makefile.local
+++ b/util/Makefile.local
@@ -5,7 +5,7 @@ extra_cflags += -I$(srcdir)/$(dir)
 
 libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
 		  $(dir)/string-util.c $(dir)/talloc-extra.c $(dir)/zlib-extra.c \
-		$(dir)/util.c
+		$(dir)/util.c $(dir)/search-path.c
 
 libutil_modules := $(libutil_c_srcs:.c=.o)
 
diff --git a/util/search-path.c b/util/search-path.c
new file mode 100644
index 0000000..9da21cb
--- /dev/null
+++ b/util/search-path.c
@@ -0,0 +1,50 @@
+#include "search-path.h"
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+
+notmuch_bool_t
+test_for_executable (const char *exename)
+{
+    char *path = NULL, *save = NULL, *tok;
+    notmuch_bool_t ret = FALSE;
+
+    if (strchr (exename, '/')) {
+	if (0 == access (exename, X_OK))
+	    return TRUE;
+	else
+	    return FALSE;
+    }
+
+    path = getenv ("PATH");
+    if (path)
+	path = strdup (path);
+    else {
+	size_t n = confstr (_CS_PATH, NULL, 0);
+	path = (char *) malloc (n);
+	if (! path)
+	    return FALSE;
+	confstr (_CS_PATH, path, n);
+    }
+
+    tok = strtok_r (path, ":", &save);
+    while (tok) {
+	int dir_fd = open (tok, O_DIRECTORY | O_RDONLY);
+	if (dir_fd != -1) {
+	    int access = faccessat (dir_fd, exename, X_OK, 0);
+	    close (dir_fd);
+	    if (access == 0) {
+		ret = TRUE;
+		break;
+	    }
+	}
+	tok = strtok_r (NULL, ":", &save);
+    }
+    if (path)
+	free (path);
+    return ret;
+}
diff --git a/util/search-path.h b/util/search-path.h
new file mode 100644
index 0000000..14c4d14
--- /dev/null
+++ b/util/search-path.h
@@ -0,0 +1,24 @@
+#ifndef _SEARCH_PATH_H
+#define _SEARCH_PATH_H
+
+#include "notmuch.h"
+
+/* can an executable be found with the given name?
+ *
+ * Return TRUE only if we can find something to execute with the
+ * associated name.
+ *
+ * if the name has a '/' in it, we look for it directly with
+ * access(exename, X_OK).
+ *
+ * otherwise, we look for it in $PATH (or in confstr(_CS_PATH), if
+ * $PATH is unset).
+ *
+ * This should match the logic for execvp (as well as matching user
+ * expectations, hopefully).
+ */
+
+notmuch_bool_t
+test_for_executable (const char *exename);
+
+#endif
-- 
2.7.0

Re: [PATCH v3 15/16] added notmuch_message_reindex

[Jameson Graef Rollins]

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

On Sun, Jan 31 2016, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
> This new function asks the database to reindex a given message, using
> the supplied indexopts.
>
> This can be used, for example, to index the cleartext of an encrypted
> message.

I just wanted to mention that I think there's a problem with the reindex
functionality introduced in this patch (or in 16/16).  It looks like
this function irrevocably busts apart threads.  dkg and I are
investigating.

jamie.

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

Re: [PATCH v3 15/16] added notmuch_message_reindex

[Daniel Kahn Gillmor]

On Tue 2016-02-09 19:41:01 -0500, Jameson Graef Rollins wrote:
> On Sun, Jan 31 2016, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:
>> This new function asks the database to reindex a given message, using
>> the supplied indexopts.
>>
>> This can be used, for example, to index the cleartext of an encrypted
>> message.
>
> I just wanted to mention that I think there's a problem with the reindex
> functionality introduced in this patch (or in 16/16).  It looks like
> this function irrevocably busts apart threads.  dkg and I are
> investigating.

it doesn't appear to be irrevocable to me, but it is definitely doing
something weird with threading.

we have a three message thread tagged "weird-thread":

  ( a ← b ← c )

if i do:

   notmuch reindex id:a

then we see two threads:

  ( a ) , ( b ← c )

if i then do:

   notmuch reindex id:b

then i see two threads:


   ( a ← b ) ,  ( c )


and if i then do:

   notmuch reindex id:c

then i see one thread again:

   ( a ← b ← c )

but interestingly, reindexing all the messages together by search tearm

  notmuch reindex tag:weird-thread

doesn't affect the threading.


i think this is happening without regard to --try-decrypt, fwiw.

  --dkg

Re: [PATCH v3 02/16] Move crypto.c into libutil

[David Bremner]

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

> This prepares us for using the crypto object in both the library and
> the client.
>
> i've prefixed notmuch_crypto with _ to indicate that while this can be
> built into the library when needed, it's not something to be exported
> or used externally.

[...] snip

> diff --git a/mime-node.c b/mime-node.c
> index e96e663..a8f5670 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -33,7 +33,7 @@ typedef struct mime_node_context {
>      GMimeMessage *mime_message;
>  
>      /* Context provided by the caller. */
> -    notmuch_crypto_t *crypto;
> +    _notmuch_crypto_t *crypto;
>  } mime_node_context_t;

is this maybe search and replace gone wild? or did you mean
to rename the type as well as the functions?

>  static void
>  node_verify (mime_node_t *node, GMimeObject *part,
> -	     notmuch_crypto_context_t *cryptoctx)
> +	     GMimeCryptoContext *cryptoctx)
>  {

This change of parameter type seems significant. Does it deserve a
comment in the commit message?

Re: [PATCH v3 03/16] make shared crypto code behave library-like

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
> -static GMimeCryptoContext*
> -create_gpg_context (_notmuch_crypto_t *crypto)
> +static notmuch_status_t
> +get_gpg_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)

I was a littled puzzled by this renaming. I guess it's fine, it just
creates a bit more diff noise.
>  /* Create a PKCS7 context (GMime 2.6) */
> -static notmuch_crypto_context_t *
> -create_pkcs7_context (notmuch_crypto_t *crypto)
> +static notmuch_status_t 
> +create_pkcs7_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)

Especially since you only renamed the gpg version. Or did I miss
something?
> +notmuch_status_t
> +_notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
> +					    const char *protocol,
> +					    GMimeCryptoContext **ctx)
>  {
> -    GMimeCryptoContext *cryptoctx = NULL;
> -    size_t i;
> -
> -    if (! protocol) {
> -	fprintf (stderr, "Cryptographic protocol is empty.\n");
> -	return cryptoctx;
> -    }
> +    if (! protocol)
> +	return NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL;
>  
>      /* As per RFC 1847 section 2.1: "the [protocol] value token is
>       * comprised of the type and sub-type tokens of the Content-Type".
> @@ -112,15 +112,12 @@ _notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protoc
>       * parameter names as defined in this document are
>       * case-insensitive."  Thus, we use strcasecmp for the protocol.
>       */
> -    for (i = 0; i < ARRAY_SIZE (protocols); i++) {
> +    for (size_t i = 0; i < ARRAY_SIZE (protocols); i++) {

I forget whether we are using C99 for loop variable declarations.  I
like them myself. If someone figures out the answer, maybe they can
update devel/STYLE.

d

Re: [PATCH v3 04/16] Provide _notmuch_crypto_{set,get}_gpg_path

[David Bremner]

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

> +#define try_gpg_path(z) if (test_for_executable(z)) return z
> +    try_gpg_path("gpg2");
> +    try_gpg_path("gpg");
> +#undef try_gpg_path
> +    return NULL;
> +}

I think I'd prefer just inlining these two ifs.

> +notmuch_status_t
> +_notmuch_crypto_set_gpg_path (_notmuch_crypto_t *crypto, const char* gpg_path)
> +{
> +    /* return success if this matches what is already configured */
> +    if ((!gpg_path && !crypto->gpg_path) ||
> +	(gpg_path && crypto->gpg_path && 0 == strcmp(gpg_path, crypto->gpg_path)))
> +	return NOTMUCH_STATUS_SUCCESS;

these changes probably need a pass of uncrustify or equivalent.

Re: [PATCH v3 05/16] Use a blank _notmuch_crypto to choose the default gpg_path

[David Bremner]

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

> This way we're only choosing a default in one place.
> ---
>  notmuch-config.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index d252bb2..7cd19a7 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -418,7 +418,8 @@ notmuch_config_open (void *ctx,
>      }
>  
>      if (notmuch_config_get_crypto_gpg_path (config) == NULL) {
> -	notmuch_config_set_crypto_gpg_path (config, "gpg");
> +	_notmuch_crypto_t crypto = { .gpg_path = NULL };
> +	notmuch_config_set_crypto_gpg_path (config, _notmuch_crypto_get_gpg_path (&crypto));
>      }

I'm not sure if it's worth it, but this particular usage would be
cleaner if _notmuch_crypto_get_path allowed a NULL argument.

d

Re: [PATCH v3 06/16] Prefer gpg2 in the test suite if available

[David Bremner]

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

>  
> +# choose the preferred GnuPG binary:
> +if command -v gpg2 > /dev/null; then
> +    GPG=gpg2
> +else
> +    GPG=gpg
> +fi

It feels like this should maybe be in configure, to centralize the choice
of GPG default. At least, that would be consistent with python / python2

d

Re: [PATCH v3 02/16] Move crypto.c into libutil

[Daniel Kahn Gillmor]

On Tue 2016-02-09 21:21:01 -0500, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> This prepares us for using the crypto object in both the library and
>> the client.
>>
>> i've prefixed notmuch_crypto with _ to indicate that while this can be
>> built into the library when needed, it's not something to be exported
>> or used externally.
>
> [...] snip
>
>> diff --git a/mime-node.c b/mime-node.c
>> index e96e663..a8f5670 100644
>> --- a/mime-node.c
>> +++ b/mime-node.c
>> @@ -33,7 +33,7 @@ typedef struct mime_node_context {
>>      GMimeMessage *mime_message;
>>  
>>      /* Context provided by the caller. */
>> -    notmuch_crypto_t *crypto;
>> +    _notmuch_crypto_t *crypto;
>>  } mime_node_context_t;
>
> is this maybe search and replace gone wild? or did you mean
> to rename the type as well as the functions?

this is a type in the util library (which is shared between libnotmuch
and the cli, but not exposed by libnotmuch).  As such, i deliberately
renamed the type to appear more explicitly "private".  I do not think we
want to expose it to the library API, in part because it "contaminates"
libnotmuch with the gmime API (because it explicitly contemplates the
use of GMimeCryptoContext, see below).

>>  static void
>>  node_verify (mime_node_t *node, GMimeObject *part,
>> -	     notmuch_crypto_context_t *cryptoctx)
>> +	     GMimeCryptoContext *cryptoctx)
>>  {
>
> This change of parameter type seems significant. Does it deserve a
> comment in the commit message?

I don't think this is a big change.  notmuch_crypto_context was
originally a typedef to GMimeCryptoContext, and it wasn't being exposed
by the library API.  Rather than pretend that it's something distinct, i
think it makes more explicit that this internal part of util deals with
libgmime.

        --dkg

Re: [PATCH v3 03/16] make shared crypto code behave library-like

[Daniel Kahn Gillmor]

On Tue 2016-02-09 21:37:04 -0500, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>> -static GMimeCryptoContext*
>> -create_gpg_context (_notmuch_crypto_t *crypto)
>> +static notmuch_status_t
>> +get_gpg_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)
>
> I was a littled puzzled by this renaming. I guess it's fine, it just
> creates a bit more diff noise.

the renaming from create_ to get_ is meaningful -- this function won't
actually create a new gpg context if one already exists.

>>  /* Create a PKCS7 context (GMime 2.6) */
>> -static notmuch_crypto_context_t *
>> -create_pkcs7_context (notmuch_crypto_t *crypto)
>> +static notmuch_status_t 
>> +create_pkcs7_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)
>
> Especially since you only renamed the gpg version. Or did I miss
> something?

ah, you're right here, this should be fixed.

>> +notmuch_status_t
>> +_notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
>> +					    const char *protocol,
>> +					    GMimeCryptoContext **ctx)
>>  {
>> -    GMimeCryptoContext *cryptoctx = NULL;
>> -    size_t i;
>> -
>> -    if (! protocol) {
>> -	fprintf (stderr, "Cryptographic protocol is empty.\n");
>> -	return cryptoctx;
>> -    }
>> +    if (! protocol)
>> +	return NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL;
>>  
>>      /* As per RFC 1847 section 2.1: "the [protocol] value token is
>>       * comprised of the type and sub-type tokens of the Content-Type".
>> @@ -112,15 +112,12 @@ _notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protoc
>>       * parameter names as defined in this document are
>>       * case-insensitive."  Thus, we use strcasecmp for the protocol.
>>       */
>> -    for (i = 0; i < ARRAY_SIZE (protocols); i++) {
>> +    for (size_t i = 0; i < ARRAY_SIZE (protocols); i++) {
>
> I forget whether we are using C99 for loop variable declarations.  I
> like them myself. If someone figures out the answer, maybe they can
> update devel/STYLE.

is what i did above a C99 loop variable declaration?  If so, i also like
them.  I've left it in for now :)

       --dkg

Re: [PATCH v3 04/16] Provide _notmuch_crypto_{set,get}_gpg_path

[Daniel Kahn Gillmor]

On Wed 2016-02-10 06:45:47 -0500, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> +#define try_gpg_path(z) if (test_for_executable(z)) return z
>> +    try_gpg_path("gpg2");
>> +    try_gpg_path("gpg");
>> +#undef try_gpg_path
>> +    return NULL;
>> +}
>
> I think I'd prefer just inlining these two ifs.
>
>> +notmuch_status_t
>> +_notmuch_crypto_set_gpg_path (_notmuch_crypto_t *crypto, const char* gpg_path)
>> +{
>> +    /* return success if this matches what is already configured */
>> +    if ((!gpg_path && !crypto->gpg_path) ||
>> +	(gpg_path && crypto->gpg_path && 0 == strcmp(gpg_path, crypto->gpg_path)))
>> +	return NOTMUCH_STATUS_SUCCESS;
>
> these changes probably need a pass of uncrustify or equivalent.

I've made these changes in my working copy; i'll publish them as part of
v4 soon.

   --dkg

Re: [PATCH v3 05/16] Use a blank _notmuch_crypto to choose the default gpg_path

[Daniel Kahn Gillmor]

On Wed 2016-02-10 06:49:54 -0500, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> This way we're only choosing a default in one place.
>> ---
>>  notmuch-config.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/notmuch-config.c b/notmuch-config.c
>> index d252bb2..7cd19a7 100644
>> --- a/notmuch-config.c
>> +++ b/notmuch-config.c
>> @@ -418,7 +418,8 @@ notmuch_config_open (void *ctx,
>>      }
>>  
>>      if (notmuch_config_get_crypto_gpg_path (config) == NULL) {
>> -	notmuch_config_set_crypto_gpg_path (config, "gpg");
>> +	_notmuch_crypto_t crypto = { .gpg_path = NULL };
>> +	notmuch_config_set_crypto_gpg_path (config, _notmuch_crypto_get_gpg_path (&crypto));
>>      }
>
> I'm not sure if it's worth it, but this particular usage would be
> cleaner if _notmuch_crypto_get_path allowed a NULL argument.

Agreed; this change will be in my v4 series.

        --dkg

Re: [PATCH v3 06/16] Prefer gpg2 in the test suite if available

[Daniel Kahn Gillmor]

On Wed 2016-02-10 06:54:12 -0500, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>>  
>> +# choose the preferred GnuPG binary:
>> +if command -v gpg2 > /dev/null; then
>> +    GPG=gpg2
>> +else
>> +    GPG=gpg
>> +fi
>
> It feels like this should maybe be in configure, to centralize the choice
> of GPG default. At least, that would be consistent with python / python2

I'm not sure what to do here.  Ultimately, i don't want there to be a
choice because i plan on having /usr/bin/gpg provided by gpg2, so this
will all be moot.

So i'm not inclined to spend much more time/engineering effort on it,
but if someone wants to propose moving these choices into configure, i'd
be willing to review.

   --dkg

Re: [PATCH v3 15/16] added notmuch_message_reindex

[Daniel Kahn Gillmor]

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

On Tue 2016-02-09 20:01:43 -0500, Daniel Kahn Gillmor wrote:
>> I just wanted to mention that I think there's a problem with the reindex
>> functionality introduced in this patch (or in 16/16).  It looks like
>> this function irrevocably busts apart threads.  dkg and I are
>> investigating.
>
> it doesn't appear to be irrevocable to me, but it is definitely doing
> something weird with threading.

OK, this is definitely tickling some problems with threading, but those
are problems that are present already in existing versions of notmuch,
unrelated to this series.

When removing a message from the database, its earlier presence doesn't
become a ghost message, and as a result anything that points to it
doesn't get assembled into the prior thread properly.

The attached tarball has a python test showing this behavior with a
simple thread of two messages:

0 dkg@frigg:~/src/notmuch/threading-test$ ./run-test 
Found 2 total files (that's not much mail).
Processed 2 total files in almost no time.
Added 2 new messages to the database.
Threads: 1
removing and re-adding a@example.com
Threads: 2
removing and re-adding b@example.com
Threads: 1
0 dkg@frigg:~/src/notmuch/threading-test$ 

the relevant python function is:


def remove_and_readd(db, mid):
    print('removing and re-adding', mid)
    m = db.find_message(mid)
    f = m.get_filename()
    db.remove_message(f)
    db.add_message(f)



I think when a message is removed from the database, we need to know
whether anything else (in its same thread?) refers to it.  If so, we
should keep it around as a ghost message instead of fully removing it.

does this sound like the right approach?

     --dkg


[-- Attachment #1.2: threading-test.tgz --]
[-- Type: application/x-gtar-compressed, Size: 838 bytes --]

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

Re: [PATCH v3 06/16] Prefer gpg2 in the test suite if available

[David Bremner]

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

> On Wed 2016-02-10 06:54:12 -0500, David Bremner wrote:
>> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>>
>>>  
>>> +# choose the preferred GnuPG binary:
>>> +if command -v gpg2 > /dev/null; then
>>> +    GPG=gpg2
>>> +else
>>> +    GPG=gpg
>>> +fi
>>
>> It feels like this should maybe be in configure, to centralize the choice
>> of GPG default. At least, that would be consistent with python / python2
>
> I'm not sure what to do here.  Ultimately, i don't want there to be a
> choice because i plan on having /usr/bin/gpg provided by gpg2, so this
> will all be moot.
>

Well, not everyone is running Debian, so your plans may not be that
relevant ;).

d

Re: [PATCH v3 06/16] Prefer gpg2 in the test suite if available

[Tomi Ollila]

On Wed, Feb 10 2016, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:

> On Wed 2016-02-10 06:54:12 -0500, David Bremner wrote:
>> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>>
>>>  
>>> +# choose the preferred GnuPG binary:
>>> +if command -v gpg2 > /dev/null; then
>>> +    GPG=gpg2
>>> +else
>>> +    GPG=gpg
>>> +fi
>>
>> It feels like this should maybe be in configure, to centralize the choice
>> of GPG default. At least, that would be consistent with python / python2
>
> I'm not sure what to do here.  Ultimately, i don't want there to be a
> choice because i plan on having /usr/bin/gpg provided by gpg2, so this
> will all be moot.

Code searches through PATH, so IMO this test could do the same (with
reasonable similarity (like above)). This could add check whether GPG
is already defined that definition is used...


>
> So i'm not inclined to spend much more time/engineering effort on it,
> but if someone wants to propose moving these choices into configure, i'd
> be willing to review.
>
>    --dkg

Re: [PATCH v3 15/16] added notmuch_message_reindex

[David Bremner]

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

>
> I think when a message is removed from the database, we need to know
> whether anything else (in its same thread?) refers to it.  If so, we
> should keep it around as a ghost message instead of fully removing it.
>
> does this sound like the right approach?
>

This seems right to me, but maybe Austin, in copy, can comment.

d

Re: [PATCH v3 07/16] create a notmuch_indexopts_t index options object

[David Bremner]

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

> +
> +notmuch_indexopts_t *
> +notmuch_indexopts_create ()
> +{
> +    notmuch_indexopts_t *ret;
> +    
> +    ret = talloc_zero (NULL, notmuch_indexopts_t);
> +
> +    return ret;
> +}

I have the same question about using talloc with a NULL context. It
_looks_ like there is no actual benefit here?

d

Re: [PATCH v3 08/16] reorganize indexing of multipart/signed and multipart/encrypted

[David Bremner]

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

> This prepares the codebase for a cleaner changeset for dealing with
> indexing some encrypted messages in the clear.
> ---
>  lib/index.cc | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/lib/index.cc b/lib/index.cc
> index f166aef..ab0fd78 100644
> --- a/lib/index.cc
> +++ b/lib/index.cc
> @@ -333,27 +333,26 @@ _index_mime_part (notmuch_message_t *message,
>  	GMimeMultipart *multipart = GMIME_MULTIPART (part);
>  	int i;
>  
> -	if (GMIME_IS_MULTIPART_SIGNED (multipart))
> -	  _notmuch_message_add_term (message, "tag", "signed");
> -
> -	if (GMIME_IS_MULTIPART_ENCRYPTED (multipart))
> -	  _notmuch_message_add_term (message, "tag", "encrypted");
> -
> -	for (i = 0; i < g_mime_multipart_get_count (multipart); i++) {
> -	    if (GMIME_IS_MULTIPART_SIGNED (multipart)) {
> -		/* Don't index the signature. */
> -		if (i == 1)
> -		    continue;
> -		if (i > 1)
> -		    _notmuch_database_log (_notmuch_message_database (message),
> -					  "Warning: Unexpected extra parts of multipart/signed. Indexing anyway.\n");
> -	    }
> -	    if (GMIME_IS_MULTIPART_ENCRYPTED (multipart)) {
> -		/* Don't index encrypted parts. */
> -		continue;
> -	    }
> +	if (GMIME_IS_MULTIPART_SIGNED (multipart)) {
> +	    _notmuch_message_add_term (message, "tag", "signed");
> +	    /* FIXME: should we try to validate the signature? */
> +	    
> +	    /* FIXME: is it always just the first part that is signed in
> +	     all multipart/signed messages?*/
>  	    _index_mime_part (message,
> -			      g_mime_multipart_get_part (multipart, i));
> +			      g_mime_multipart_get_part (multipart, 0));

It took me a long moment to understand the change from "index part if not 1"
to "index part 0". If that deserved explanation (which I'm not so certain
about), it would be in the commit message.

Re: [PATCH v3 09/16] index encrypted parts when asked.

[David Bremner]

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

> If we see index options that ask us to decrypt when indexing a
> message, and we encounter an encrypted part, we'll try to descend into
> it.
>
> If we can decrypt, we tag the message with index-decrypted.
>
> If we can't decrypt (or recognize the encrypted type of mail), we tag
> with decryption-failed.

I have some mild reservations about hard-coding this tagging into the
library layer, but I guess it's consistent with what is there already.
I guess we can use indexopts to control automatic tagging later.

One observation is that both index-decrypted and decryption-failed are
properties of the database state and not the message, syncing them
between machines will be a bit tricky.

> +/* descend (if desired) into the cleartext part of an encrypted MIME
> + * part while indexing. */
> +static void
> +_index_encrypted_mime_part (notmuch_message_t *message,
> +			    notmuch_indexopts_t *indexopts,
> +			    GMimeContentType *content_type,
> +			    GMimeMultipartEncrypted *encrypted_data)

Is there a good reason not to propagate crypto errors back to the
caller? It seems like it should be up to the top level caller to the
library (e.g. the CLI) to decide to ignore such errors. I'm not very
happy with tags as the only error reporting mechanism. I understand that
_index_mime_part would need to return a status value, but that change
doesn't look to disruptive?

d

Re: [PATCH v3 11/16] add --try-decrypt to notmuch insert

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
> +    if (try_decrypt) {
> +	const char* gpg_path = notmuch_config_get_crypto_gpg_path (config);
> +	status = notmuch_indexopts_set_gpg_path (indexopts, gpg_path);
> +	if (status)
> +	    fprintf (stderr, "Warning: failed to set database gpg_path to '%s' (%s)\n",
> +		     gpg_path ? gpg_path : "(NULL)",
> +		     notmuch_status_to_string (status));
> +    }

Should this really be non-fatal event? I agree that the indexing can
carry on, but are there reasons for this to fail that don't indicate
something has gone really wrong?

d

Re: [PATCH v3 13/16] add indexopts to notmuch python bindings.

[David Bremner]

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

> Make notmuch indexing options are not available in python as
> the notmuch.Indexopts class.  Users can do something like:

I'd like to have feedback from Justus before I merge this. That need not
block the rest of the series I guess?

d

Re: [PATCH v3 15/16] added notmuch_message_reindex

[David Bremner]

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

> +    const char *autotags[] = {
> +		    "attachment",
> +		    "encrypted",
> +		    "signed",
> +		    "index-decrypted",
> +		    "index-decryption-failed" };

Hmm. Is this really the only place these values are needed? That's a bit
surprising to me.

> +    /* cache tags and filenames */
> +    tags = notmuch_message_get_tags(message);
> +    filenames = notmuch_message_get_filenames(message);
> +    orig_filenames = notmuch_message_get_filenames(message);
> +    
> +    /* walk through filenames, removing them until the message is gone */
> +    for ( ; notmuch_filenames_valid (filenames);
> +	  notmuch_filenames_move_to_next (filenames)) {
> +	filename = notmuch_filenames_get (filenames);

What's the expected lifetime of the tags, filenames, and orig_filenames
lists? I guess they live until message dies. Are we expecting message to
die fairly quickly in the usual case?

> +    
> +    /* re-add the filenames with the associated indexopts */
> +    for (; notmuch_filenames_valid (orig_filenames);
> +	 notmuch_filenames_move_to_next (orig_filenames)) {
> +	filename = notmuch_filenames_get (orig_filenames);
> +
> +	status = notmuch_database_add_message_with_indexopts(notmuch,
> +							     filename,
> +							     indexopts,
> +							     readded ? NULL : &newmsg);

I guess you thought about this already, but I take it you made an
intentional choice to (attempt to) reindex all the files rather than use
_notmuch_message_add_file_name?

Re: [PATCH v3 16/16] add "notmuch reindex" subcommand

[David Bremner]

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

> This new subcommand takes a set of search terms, and re-indexes the
> list of matching messages using the supplied options.
>
> This can be used to index the cleartext of encrypted messages with
> something like:
>
>  notmuch reindex --try-decrypt \
>     tag:encrypted and not tag:index-decrypted

I haven't reviewed this patch yet.  Before I do I'd like to discuss the
issue of the apparent existing bug in thread handling that it exposes[1].

Assuming that analysis is correct (and I have no reason not to believe
so), the bug is in existing notmuch code and not related to this
change. On the other hand, currently users have to work a bit to expose
this bug, while this command would be inherently buggy (through no fault
of it's own) from introduction.  With my release manager hat on, I'm not
very happy with the hypothetical announcement "We have this new command,
but it will break your threads". So what I'd like to understand is to
what extent the reindex command, with the current notmuch codebase
(i.e. no new ghost message code), is "suitable for release".

One option would be to merge a version of most of the proposed changes
(with some minor updates), and leave the re-index (and maybe python
bindings changes) for a second series.


[1]: id:871t8ko50r.fsf@alice.fifthhorseman.net