[PATCH v2 0/3] cli: clean up gmime 2.4 vs. 2.6 conditional build

[PATCH v2 0/3] cli: clean up gmime 2.4 vs. 2.6 conditional build

[Jani Nikula]

This is v2 of [1]. Added comments per David's request, and while at it,
added a third patch to conform the existing conditional build in notmuch
show to the same style. The whole series should have no functional
changes, and thus v2 should have no functional changes since v1. ;)

I have not tested this on gmime 2.4.

BR,
Jani.


[1] id:a9f12ba474fc51df71e2fd2b7a20a8d101729c6e.1362319765.git.jani@nikula.org

Jani Nikula (3):
  cli: crypto: abstract gpg context creation for clarity
  cli: mime node: abstract decryption and signature verification
  cli: conform to same conditional build style as elsewhere in
    notmuch-show

 crypto.c       |   66 ++++++++++++-----
 mime-node.c    |  218 ++++++++++++++++++++++++++++++++++++--------------------
 notmuch-show.c |   46 ++++++------
 3 files changed, 215 insertions(+), 115 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/3] cli: crypto: abstract gpg context creation for clarity

[Jani Nikula]

The code filled with #ifdef GMIME_ATLEAST_26 is difficult to
read. Abstract gpg context creation into a function, with separate
implementations for GMime 2.4 and 2.6, to clarify the code.

There should be no functional changes.
---
 crypto.c |   66 ++++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 19 deletions(-)

diff --git a/crypto.c b/crypto.c
index cb361e1..9736517 100644
--- a/crypto.c
+++ b/crypto.c
@@ -20,6 +20,48 @@
 
 #include "notmuch-client.h"
 
+#ifdef GMIME_ATLEAST_26
+
+/* Create a GPG context (GMime 2.6) */
+static notmuch_crypto_context_t *
+create_gpg_context (void)
+{
+    notmuch_crypto_context_t *gpgctx;
+
+    /* TODO: GMimePasswordRequestFunc */
+    gpgctx = g_mime_gpg_context_new (NULL, "gpg");
+    if (! gpgctx)
+	return NULL;
+
+    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) gpgctx, TRUE);
+    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
+
+    return gpgctx;
+}
+
+#else /* GMIME_ATLEAST_26 */
+
+/* Create a GPG context (GMime 2.4) */
+static notmuch_crypto_context_t *
+create_gpg_context (void)
+{
+    GMimeSession *session;
+    notmuch_crypto_context_t *gpgctx;
+
+    session = g_object_new (g_mime_session_get_type (), NULL);
+    gpgctx = g_mime_gpg_context_new (session, "gpg");
+    g_object_unref (session);
+
+    if (! gpgctx)
+	return NULL;
+
+    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
+
+    return gpgctx;
+}
+
+#endif /* GMIME_ATLEAST_26 */
+
 /* for the specified protocol return the context pointer (initializing
  * if needed) */
 notmuch_crypto_context_t *
@@ -33,28 +75,14 @@ notmuch_crypto_get_context (notmuch_crypto_t *crypto, const char *protocol)
      * parameter names as defined in this document are
      * case-insensitive."  Thus, we use strcasecmp for the protocol.
      */
-    if ((strcasecmp (protocol, "application/pgp-signature") == 0)
-	|| (strcasecmp (protocol, "application/pgp-encrypted") == 0)) {
-	if (!crypto->gpgctx) {
-#ifdef GMIME_ATLEAST_26
-	    /* TODO: GMimePasswordRequestFunc */
-	    crypto->gpgctx = g_mime_gpg_context_new (NULL, "gpg");
-#else
-	    GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL);
-	    crypto->gpgctx = g_mime_gpg_context_new (session, "gpg");
-	    g_object_unref (session);
-#endif
-	    if (crypto->gpgctx) {
-#ifdef GMIME_ATLEAST_26
-		g_mime_gpg_context_set_use_agent ((GMimeGpgContext*) crypto->gpgctx, TRUE);
-#endif
-		g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) crypto->gpgctx, FALSE);
-	    } else {
+    if (strcasecmp (protocol, "application/pgp-signature") == 0 ||
+	strcasecmp (protocol, "application/pgp-encrypted") == 0) {
+	if (! crypto->gpgctx) {
+	    crypto->gpgctx = create_gpg_context ();
+	    if (! crypto->gpgctx)
 		fprintf (stderr, "Failed to construct gpg context.\n");
-	    }
 	}
 	cryptoctx = crypto->gpgctx;
-
     } else {
 	fprintf (stderr, "Unknown or unsupported cryptographic protocol.\n");
     }
-- 
1.7.10.4

[PATCH v2 2/3] cli: mime node: abstract decryption and signature verification

[Jani Nikula]

The code filled with #ifdef GMIME_ATLEAST_26 is difficult to
read. Abstract the decryption and signature verification into
functions, with separate implementations for GMime 2.4 and 2.6, to
clarify the code.

There should be no functional changes.
---
 mime-node.c |  218 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 142 insertions(+), 76 deletions(-)

diff --git a/mime-node.c b/mime-node.c
index 839737a..851f963 100644
--- a/mime-node.c
+++ b/mime-node.c
@@ -130,26 +130,163 @@ DONE:
 }
 
 #ifdef GMIME_ATLEAST_26
+
+/* Signature list destructor (GMime 2.6) */
 static int
 _signature_list_free (GMimeSignatureList **proxy)
 {
     g_object_unref (*proxy);
     return 0;
 }
-#else
+
+/* Set up signature list destructor (GMime 2.6) */
+static void
+set_signature_list_destructor (mime_node_t *node)
+{
+    GMimeSignatureList **proxy = talloc (node, GMimeSignatureList *);
+    if (proxy) {
+	*proxy = node->sig_list;
+	talloc_set_destructor (proxy, _signature_list_free);
+    }
+}
+
+/* Verify a signed mime node (GMime 2.6) */
+static void
+node_verify (mime_node_t *node, GMimeObject *part,
+	     notmuch_crypto_context_t *cryptoctx)
+{
+    GError *err = NULL;
+
+    node->verify_attempted = TRUE;
+    node->sig_list = g_mime_multipart_signed_verify
+	(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
+
+    if (node->sig_list)
+	set_signature_list_destructor (node);
+    else
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 err ? err->message : "no error explanation given");
+
+    if (err)
+	g_error_free (err);
+}
+
+/* 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)
+{
+    GError *err = NULL;
+    GMimeDecryptResult *decrypt_result = NULL;
+    GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
+
+    node->decrypt_attempted = TRUE;
+    node->decrypted_child = g_mime_multipart_encrypted_decrypt
+	(encrypteddata, cryptoctx, &decrypt_result, &err);
+    if (! node->decrypted_child) {
+	fprintf (stderr, "Failed to decrypt part: %s\n",
+		 err ? err->message : "no error explanation given");
+	goto DONE;
+    }
+
+    node->decrypt_success = TRUE;
+    node->verify_attempted = TRUE;
+
+    /* This may be NULL if the part is not signed. */
+    node->sig_list = g_mime_decrypt_result_get_signatures (decrypt_result);
+    if (node->sig_list) {
+	g_object_ref (node->sig_list);
+	set_signature_list_destructor (node);
+    }
+    g_object_unref (decrypt_result);
+
+ DONE:
+    if (err)
+	g_error_free (err);
+}
+
+#else /* GMIME_ATLEAST_26 */
+
+/* Signature validity destructor (GMime 2.4) */
 static int
 _signature_validity_free (GMimeSignatureValidity **proxy)
 {
     g_mime_signature_validity_free (*proxy);
     return 0;
 }
-#endif
+
+/* Set up signature validity destructor (GMime 2.4) */
+static void
+set_signature_validity_destructor (mime_node_t *node)
+{
+    GMimeSignatureValidity **proxy = talloc (node, GMimeSignatureValidity *);
+    if (proxy) {
+	*proxy = node->sig_validity;
+	talloc_set_destructor (proxy, _signature_validity_free);
+    }
+}
+
+/* Verify a signed mime node (GMime 2.4) */
+static void
+node_verify (mime_node_t *node, GMimeObject *part,
+	     notmuch_crypto_context_t *cryptoctx)
+{
+    GError *err = NULL;
+
+    node->verify_attempted = TRUE;
+    node->sig_validity = g_mime_multipart_signed_verify
+	(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
+    if (node->sig_validity) {
+	set_signature_validity_destructor (node);
+    } else {
+	fprintf (stderr, "Failed to verify signed part: %s\n",
+		 err ? err->message : "no error explanation given");
+    }
+
+    if (err)
+	g_error_free (err);
+}
+
+/* Decrypt and optionally verify an encrypted mime node (GMime 2.4) */
+static void
+node_decrypt_and_verify (mime_node_t *node, GMimeObject *part,
+			 notmuch_crypto_context_t *cryptoctx)
+{
+    GError *err = NULL;
+    GMimeMultipartEncrypted *encrypteddata = GMIME_MULTIPART_ENCRYPTED (part);
+
+    node->decrypt_attempted = TRUE;
+    node->decrypted_child = g_mime_multipart_encrypted_decrypt
+	(encrypteddata, cryptoctx, &err);
+    if (! node->decrypted_child) {
+	fprintf (stderr, "Failed to decrypt part: %s\n",
+		 err ? err->message : "no error explanation given");
+	goto DONE;
+    }
+
+    node->decrypt_success = TRUE;
+    node->verify_attempted = TRUE;
+
+    /* The GMimeSignatureValidity returned here is a const, unlike the
+     * one returned by g_mime_multipart_signed_verify() in
+     * node_verify() above, so the destructor is not needed.
+     */
+    node->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
+    if (! node->sig_validity)
+	fprintf (stderr, "Failed to verify encrypted signed part: %s\n",
+		 err ? err->message : "no error explanation given");
+
+ DONE:
+    if (err)
+	g_error_free (err);
+}
+
+#endif  /* GMIME_ATLEAST_26 */
 
 static mime_node_t *
 _mime_node_create (mime_node_t *parent, GMimeObject *part)
 {
     mime_node_t *node = talloc_zero (parent, mime_node_t);
-    GError *err = NULL;
     notmuch_crypto_context_t *cryptoctx = NULL;
 
     /* Set basic node properties */
@@ -198,32 +335,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 		     "message (must be exactly 2)\n",
 		     node->nchildren);
 	} else {
-	    GMimeMultipartEncrypted *encrypteddata =
-		GMIME_MULTIPART_ENCRYPTED (part);
-	    node->decrypt_attempted = TRUE;
-#ifdef GMIME_ATLEAST_26
-	    GMimeDecryptResult *decrypt_result = NULL;
-	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
-		(encrypteddata, cryptoctx, &decrypt_result, &err);
-#else
-	    node->decrypted_child = g_mime_multipart_encrypted_decrypt
-		(encrypteddata, cryptoctx, &err);
-#endif
-	    if (node->decrypted_child) {
-		node->decrypt_success = node->verify_attempted = TRUE;
-#ifdef GMIME_ATLEAST_26
-		/* This may be NULL if the part is not signed. */
-		node->sig_list = g_mime_decrypt_result_get_signatures (decrypt_result);
-		if (node->sig_list)
-		    g_object_ref (node->sig_list);
-		g_object_unref (decrypt_result);
-#else
-		node->sig_validity = g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
-#endif
-	    } else {
-		fprintf (stderr, "Failed to decrypt part: %s\n",
-			 (err ? err->message : "no error explanation given"));
-	    }
+	    node_decrypt_and_verify (node, part, cryptoctx);
 	}
     } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->verify && cryptoctx) {
 	if (node->nchildren != 2) {
@@ -232,56 +344,10 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
 		     "(must be exactly 2)\n",
 		     node->nchildren);
 	} else {
-#ifdef GMIME_ATLEAST_26
-	    node->sig_list = g_mime_multipart_signed_verify
-		(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
-	    node->verify_attempted = TRUE;
-
-	    if (!node->sig_list)
-		fprintf (stderr, "Failed to verify signed part: %s\n",
-			 (err ? err->message : "no error explanation given"));
-#else
-	    /* For some reason the GMimeSignatureValidity returned
-	     * here is not a const (inconsistent with that
-	     * returned by
-	     * g_mime_multipart_encrypted_get_signature_validity,
-	     * and therefore needs to be properly disposed of.
-	     *
-	     * In GMime 2.6, they're both non-const, so we'll be able
-	     * to clean up this asymmetry. */
-	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
-		(GMIME_MULTIPART_SIGNED (part), cryptoctx, &err);
-	    node->verify_attempted = TRUE;
-	    node->sig_validity = sig_validity;
-	    if (sig_validity) {
-		GMimeSignatureValidity **proxy =
-		    talloc (node, GMimeSignatureValidity *);
-		*proxy = sig_validity;
-		talloc_set_destructor (proxy, _signature_validity_free);
-	    }
-#endif
+	    node_verify (node, part, cryptoctx);
 	}
     }
 
-#ifdef GMIME_ATLEAST_26
-    /* sig_list may be created in both above cases, so we need to
-     * cleanly handle it here. */
-    if (node->sig_list) {
-	GMimeSignatureList **proxy = talloc (node, GMimeSignatureList *);
-	*proxy = node->sig_list;
-	talloc_set_destructor (proxy, _signature_list_free);
-    }
-#endif
-
-#ifndef GMIME_ATLEAST_26
-    if (node->verify_attempted && !node->sig_validity)
-	fprintf (stderr, "Failed to verify signed part: %s\n",
-		 (err ? err->message : "no error explanation given"));
-#endif
-
-    if (err)
-	g_error_free (err);
-
     return node;
 }
 
-- 
1.7.10.4

[PATCH v2 3/3] cli: conform to same conditional build style as elsewhere in notmuch-show

[Jani Nikula]

Conform to the same style for #ifdef GMIME_ATLEAST_26 conditional
builds as elsewhere.

There are no functional changes.
---
 notmuch-show.c |   46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index c2ec122..62178f7 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -335,6 +335,8 @@ show_text_part_content (GMimeObject *part, GMimeStream *stream_out,
 }
 
 #ifdef GMIME_ATLEAST_26
+
+/* Get signature status string (GMime 2.6) */
 static const char*
 signature_status_to_string (GMimeSignatureStatus x)
 {
@@ -348,25 +350,8 @@ signature_status_to_string (GMimeSignatureStatus x)
     }
     return "unknown";
 }
-#else
-static const char*
-signer_status_to_string (GMimeSignerStatus x)
-{
-    switch (x) {
-    case GMIME_SIGNER_STATUS_NONE:
-	return "none";
-    case GMIME_SIGNER_STATUS_GOOD:
-	return "good";
-    case GMIME_SIGNER_STATUS_BAD:
-	return "bad";
-    case GMIME_SIGNER_STATUS_ERROR:
-	return "error";
-    }
-    return "unknown";
-}
-#endif
 
-#ifdef GMIME_ATLEAST_26
+/* Signature status sprinter (GMime 2.6) */
 static void
 format_part_sigstatus_sprinter (sprinter_t *sp, mime_node_t *node)
 {
@@ -441,7 +426,27 @@ format_part_sigstatus_sprinter (sprinter_t *sp, mime_node_t *node)
 
     sp->end (sp);
 }
-#else
+
+#else /* GMIME_ATLEAST_26 */
+
+/* Get signature status string (GMime 2.4) */
+static const char*
+signer_status_to_string (GMimeSignerStatus x)
+{
+    switch (x) {
+    case GMIME_SIGNER_STATUS_NONE:
+	return "none";
+    case GMIME_SIGNER_STATUS_GOOD:
+	return "good";
+    case GMIME_SIGNER_STATUS_BAD:
+	return "bad";
+    case GMIME_SIGNER_STATUS_ERROR:
+	return "error";
+    }
+    return "unknown";
+}
+
+/* Signature status sprinter (GMime 2.4) */
 static void
 format_part_sigstatus_sprinter (sprinter_t *sp, mime_node_t *node)
 {
@@ -504,7 +509,8 @@ format_part_sigstatus_sprinter (sprinter_t *sp, mime_node_t *node)
 
     sp->end (sp);
 }
-#endif
+
+#endif /* GMIME_ATLEAST_26 */
 
 static notmuch_status_t
 format_part_text (const void *ctx, sprinter_t *sp, mime_node_t *node,
-- 
1.7.10.4

Re: [PATCH v2 0/3] cli: clean up gmime 2.4 vs. 2.6 conditional build

[Tomi Ollila]

On Sat, Mar 30 2013, Jani Nikula <jani@nikula.org> wrote:

> This is v2 of [1]. Added comments per David's request, and while at it,
> added a third patch to conform the existing conditional build in notmuch
> show to the same style. The whole series should have no functional
> changes, and thus v2 should have no functional changes since v1. ;)
>
> I have not tested this on gmime 2.4.

I have gmime 2.4(.24) on this system. The code compiles and
exactly the same set of tests pass/fail as without this patch
(unfortunately this system cannot cope with just some of the
crypto patches, so I cannot give absolute guarantee:

crypto: Testing PGP/MIME signature verification and decryption
 PASS   emacs delivery of signed message
 PASS   signature verification
 PASS   signature verification with full owner trust
 PASS   signature verification with signer key unavailable
 PASS   emacs delivery of encrypted message with attachment
 FAIL   decryption, --format=text
 FAIL   decryption, --format=json
 FAIL   decryption, --format=json, --part=4
 FAIL   decrypt attachment (--part=5 --format=raw)
 FAIL   decryption failure with missing key
 PASS   emacs delivery of encrypted + signed message
 FAIL   decryption + signature verification
 FAIL   reply to encrypted message
 FAIL   signature verification with revoked key

anyway, as said, exactly the same set fails without these pathes

The reason for failure is most probably in my system and not something
w/ notmuch crypto support -- If I run `emacs foo.org.gpg` this just
hangs waiting me to give passphrase in (invisible?) curses request)

So, bottom line: LGTM.

> BR,
> Jani.

Tomi


> [1] id:a9f12ba474fc51df71e2fd2b7a20a8d101729c6e.1362319765.git.jani@nikula.org
>
> Jani Nikula (3):
>   cli: crypto: abstract gpg context creation for clarity
>   cli: mime node: abstract decryption and signature verification
>   cli: conform to same conditional build style as elsewhere in
>     notmuch-show
>
>  crypto.c       |   66 ++++++++++++-----
>  mime-node.c    |  218 ++++++++++++++++++++++++++++++++++++--------------------
>  notmuch-show.c |   46 ++++++------
>  3 files changed, 215 insertions(+), 115 deletions(-)
>
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 0/3] cli: clean up gmime 2.4 vs. 2.6 conditional build

[Tomi Ollila]

On Sun, Mar 31 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:

> On Sat, Mar 30 2013, Jani Nikula <jani@nikula.org> wrote:
>
>> This is v2 of [1]. Added comments per David's request, and while at it,
>> added a third patch to conform the existing conditional build in notmuch
>> show to the same style. The whole series should have no functional
>> changes, and thus v2 should have no functional changes since v1. ;)
>>
>> I have not tested this on gmime 2.4.
>
> I have gmime 2.4(.24) on this system. The code compiles and
> exactly the same set of tests pass/fail as without this patch
> (unfortunately this system cannot cope with just some of the
> crypto patches, so I cannot give absolute guarantee:

Ok, the failure cases below are just those which are changed in
this patch. So I had to dig further...

... actually this
  'PASS   emacs delivery of encrypted message with attachment'
fails to deliver the message.

After the following change:

--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -442,7 +442,8 @@ emacs_deliver_message ()
 
     test_emacs \
        "(let ((message-send-mail-function 'message-smtpmail-send-it)
-              (smtpmail-smtp-server \"localhost\")
+              (smtpmail-smtp-server 'local)
+              ;;(smtpmail-smtp-server \"localhost\")
               (smtpmail-smtp-service \"25025\"))
           (notmuch-hello)
           (notmuch-mua-mail)

The failured cases below PASSes.

(I've stumbled the same problem a few cases before but haven't 
figured out a good patch so far...)

>
> crypto: Testing PGP/MIME signature verification and decryption
>  PASS   emacs delivery of signed message
>  PASS   signature verification
>  PASS   signature verification with full owner trust
>  PASS   signature verification with signer key unavailable
>  PASS   emacs delivery of encrypted message with attachment
>  FAIL   decryption, --format=text
>  FAIL   decryption, --format=json
>  FAIL   decryption, --format=json, --part=4
>  FAIL   decrypt attachment (--part=5 --format=raw)
>  FAIL   decryption failure with missing key
>  PASS   emacs delivery of encrypted + signed message
>  FAIL   decryption + signature verification
>  FAIL   reply to encrypted message
>  FAIL   signature verification with revoked key
>
> anyway, as said, exactly the same set fails without these pathes
>
> The reason for failure is most probably in my system and not something
> w/ notmuch crypto support -- If I run `emacs foo.org.gpg` this just
> hangs waiting me to give passphrase in (invisible?) curses request)
>
> So, bottom line: LGTM.
>
>> BR,
>> Jani.
>
> Tomi
>
>
>> [1] id:a9f12ba474fc51df71e2fd2b7a20a8d101729c6e.1362319765.git.jani@nikula.org
>>
>> Jani Nikula (3):
>>   cli: crypto: abstract gpg context creation for clarity
>>   cli: mime node: abstract decryption and signature verification
>>   cli: conform to same conditional build style as elsewhere in
>>     notmuch-show
>>
>>  crypto.c       |   66 ++++++++++++-----
>>  mime-node.c    |  218 ++++++++++++++++++++++++++++++++++++--------------------
>>  notmuch-show.c |   46 ++++++------
>>  3 files changed, 215 insertions(+), 115 deletions(-)
>>
>> -- 
>> 1.7.10.4

Re: [PATCH v2 0/3] cli: clean up gmime 2.4 vs. 2.6 conditional build

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> This is v2 of [1]. Added comments per David's request, and while at it,
> added a third patch to conform the existing conditional build in notmuch
> show to the same style. The whole series should have no functional
> changes, and thus v2 should have no functional changes since v1. ;)
>

Pushed.

d