From: Thomas Jost <schnouki@schnouki.net>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH v3 2/2] Add compatibility with gmime 2.6
In-Reply-To: <20120120041044.GU16740@mit.edu>
References: <87ty3r2rqt.fsf@schnouki.net>
	<1327017987-3361-1-git-send-email-schnouki@schnouki.net>
	<1327017987-3361-3-git-send-email-schnouki@schnouki.net>
	<20120120041044.GU16740@mit.edu>
User-Agent: Notmuch/0.11+95~gde2fd30 (http://notmuchmail.org) Emacs/24.0.92.1
	(x86_64-unknown-linux-gnu)
Date: Fri, 20 Jan 2012 10:37:31 +0100
Message-ID: <87fwfaemuc.fsf@thor.loria.fr>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Cc: notmuch@notmuchmail.org
Precedence: list

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On Thu, 19 Jan 2012 23:10:44 -0500, Austin Clements <amdragon@MIT.EDU> wrot=
e:
> Nearly there.  A few more small comments.

Thanks again :) Will post new version soon, including a new patch to
update NEWS and INSTALL.

> Quoth Thomas Jost on Jan 20 at  1:06 am:
> > There are lots of API changes in gmime 2.6 crypto handling. By adding
> > preprocessor directives, it is however possible to add gmime 2.6 compat=
ibility
> > while preserving compatibility with gmime 2.4 too.
> >=20
> > This is mostly based on id:"8762i8hrb9.fsf@bookbinder.fernseed.info".
> >=20
> > This was tested against both gmime 2.6.4 and 2.4.31. With gmime 2.4.31,=
 the
> > crypto tests all work fine (as expected). With gmime 2.6.4, one crypto =
test is
> > currently broken (signature verification with signer key unavailable), =
most
> > likely because of a bug in gmime which will hopefully be fixed in a fut=
ure
> > version.
> > ---
> >  mime-node.c      |   60 ++++++++++++++++++++++++++++++++--
> >  notmuch-client.h |   30 ++++++++++++++++-
> >  notmuch-reply.c  |    7 ++++
> >  notmuch-show.c   |   95 ++++++++++++++++++++++++++++++++++++++++++++++=
++++++++
> >  show-message.c   |    4 ++
> >  test/crypto      |    2 +
> >  6 files changed, 192 insertions(+), 6 deletions(-)
> >=20
> > diff --git a/mime-node.c b/mime-node.c
> > index d26bb44..ad19f5e 100644
> > --- a/mime-node.c
> > +++ b/mime-node.c
> > @@ -33,7 +33,11 @@ typedef struct mime_node_context {
> >      GMimeMessage *mime_message;
> >=20=20
> >      /* Context provided by the caller. */
> > +#ifdef GMIME_ATLEAST_26
> > +    GMimeCryptoContext *cryptoctx;
> > +#else
> >      GMimeCipherContext *cryptoctx;
> > +#endif
> >      notmuch_bool_t decrypt;
> >  } mime_node_context_t;
> >=20=20
> > @@ -57,8 +61,12 @@ _mime_node_context_free (mime_node_context_t *res)
> >=20=20
> >  notmuch_status_t
> >  mime_node_open (const void *ctx, notmuch_message_t *message,
> > -		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
> > -		mime_node_t **root_out)
> > +#ifdef GMIME_ATLEAST_26
> > +		GMimeCryptoContext *cryptoctx,
> > +#else
> > +		GMimeCipherContext *cryptoctx,
> > +#endif
> > +		notmuch_bool_t decrypt, mime_node_t **root_out)
> >  {
> >      const char *filename =3D notmuch_message_get_filename (message);
> >      mime_node_context_t *mctx;
> > @@ -112,12 +120,21 @@ DONE:
> >      return status;
> >  }
> >=20=20
> > +#ifdef GMIME_ATLEAST_26
> > +static int
> > +_signature_list_free (GMimeSignatureList **proxy)
> > +{
> > +    g_object_unref (*proxy);
> > +    return 0;
> > +}
> > +#else
> >  static int
> >  _signature_validity_free (GMimeSignatureValidity **proxy)
> >  {
> >      g_mime_signature_validity_free (*proxy);
> >      return 0;
> >  }
> > +#endif
> >=20=20
> >  static mime_node_t *
> >  _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > @@ -165,11 +182,24 @@ _mime_node_create (const mime_node_t *parent, GMi=
meObject *part)
> >  	    GMimeMultipartEncrypted *encrypteddata =3D
> >  		GMIME_MULTIPART_ENCRYPTED (part);
> >  	    node->decrypt_attempted =3D TRUE;
> > +#ifdef GMIME_ATLEAST_26
> > +	    GMimeDecryptResult *decrypt_result =3D NULL;
> > +	    node->decrypted_child =3D g_mime_multipart_encrypted_decrypt
> > +		(encrypteddata, node->ctx->cryptoctx, &decrypt_result, &err);
> > +#else
> >  	    node->decrypted_child =3D g_mime_multipart_encrypted_decrypt
> >  		(encrypteddata, node->ctx->cryptoctx, &err);
> > +#endif
> >  	    if (node->decrypted_child) {
> >  		node->decrypt_success =3D node->verify_attempted =3D TRUE;
> > +#ifdef GMIME_ATLEAST_26
> > +		/* This may be NULL if the part is not signed. */
> > +		node->sig_list =3D g_mime_decrypt_result_get_signatures (decrypt_res=
ult);
> > +		g_object_ref (node->sig_list);
>=20
> Apparently you can't g_object_ref NULL, so there should be a
> conditional around this.

Right, added. Thanks.

> (Does g_mime_decrypt_result_get_signatures *really* return NULL for an
> empty list? I feel like various tests should have failed in various
> versions of this patch if it did.)

Yes it does. I added some fprintf() in gmime (in gpg_decrypt() and
g_mime_decrypt_result_get_signatures()). Decryption tests (after "emacs
delivery of encrypted message with attachment") clearly show that
g_mime_decrypt_result_get_signatures returns NULL when there's no
signatures in an encrypted part.

I guess the reason why tests did not fail is that
format_part_sigstatus_json just displays an empty JSON array if sig_list
is NULL. And that's also what's expected when an encrypted part has no
signature.

I had a quick look at the GObject code; apparently if you pass NULL to
g_object_ref (or anything that is not a GObject) it will just return
NULL. So that's why this one did not fail either.

>=20
> > +		g_object_unref (decrypt_result);
> > +#else
> >  		node->sig_validity =3D g_mime_multipart_encrypted_get_signature_vali=
dity (encrypteddata);
> > +#endif
> >  	    } else {
> >  		fprintf (stderr, "Failed to decrypt part: %s\n",
> >  			 (err ? err->message : "no error explanation given"));
> > @@ -182,6 +212,15 @@ _mime_node_create (const mime_node_t *parent, GMim=
eObject *part)
> >  		     "(must be exactly 2)\n",
> >  		     node->nchildren);
> >  	} else {
> > +#ifdef GMIME_ATLEAST_26
> > +	    node->sig_list =3D g_mime_multipart_signed_verify
> > +		(GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err);
> > +	    node->verify_attempted =3D 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
> > @@ -195,17 +234,30 @@ _mime_node_create (const mime_node_t *parent, GMi=
meObject *part)
> >  	    node->verify_attempted =3D TRUE;
> >  	    node->sig_validity =3D sig_validity;
> >  	    if (sig_validity) {
> > -		GMimeSignatureValidity **proxy =3D
> > -		    talloc (node, GMimeSignatureValidity *);
> > +		GMimeSignatureValidity **proxy =3D talloc (node, GMimeSignatureValid=
ity *);
>=20
> (See below)
>=20
> >  		*proxy =3D sig_validity;
> >  		talloc_set_destructor (proxy, _signature_validity_free);
> >  	    }
> > +#endif
> >  	}
> >      }
> >=20=20
> > +#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 =3D
> > +	    talloc (node, GMimeSignatureList *);
>=20
> Oops.  I think you un-split the wrong line.  The one up above is now
> too long and this one is still unnecessarily split.

Argh. Sorry about that. (I guess hacking while watching DS9 is not a
good idea: too distracting. Won't do it again until next time.)

>=20
> > +	*proxy =3D 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
> >=20=20
> >      if (err)
> >  	g_error_free (err);
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index 62ede28..9c1d383 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -30,6 +30,14 @@
> >=20=20
> >  #include <gmime/gmime.h>
> >=20=20
> > +/* GMIME_CHECK_VERSION in gmime 2.4 is not usable from the
> > + * preprocessor (it calls a runtime function). But since
> > + * GMIME_MAJOR_VERSION and friends were added in gmime 2.6, we can use
> > + * these to check the version number. */
> > +#ifdef GMIME_MAJOR_VERSION
> > +#define GMIME_ATLEAST_26
> > +#endif
> > +
> >  #include "notmuch.h"
> >=20=20
> >  /* This is separate from notmuch-private.h because we're trying to
> > @@ -69,7 +77,11 @@ typedef struct notmuch_show_format {
> >      void (*part_start) (GMimeObject *part,
> >  			int *part_count);
> >      void (*part_encstatus) (int status);
> > +#ifdef GMIME_ATLEAST_26
> > +    void (*part_sigstatus) (GMimeSignatureList* siglist);
> > +#else
> >      void (*part_sigstatus) (const GMimeSignatureValidity* validity);
> > +#endif
> >      void (*part_content) (GMimeObject *part);
> >      void (*part_end) (GMimeObject *part);
> >      const char *part_sep;
> > @@ -83,7 +95,11 @@ typedef struct notmuch_show_params {
> >      int entire_thread;
> >      int raw;
> >      int part;
> > +#ifdef GMIME_ATLEAST_26
> > +    GMimeCryptoContext* cryptoctx;
> > +#else
> >      GMimeCipherContext* cryptoctx;
> > +#endif
> >      int decrypt;
> >  } notmuch_show_params_t;
> >=20=20
> > @@ -290,11 +306,17 @@ typedef struct mime_node {
> >=20=20
> >      /* True if signature verification on this part was attempted. */
> >      notmuch_bool_t verify_attempted;
> > +#ifdef GMIME_ATLEAST_26
> > +    /* The list of signatures for signed or encrypted containers. If
> > +     * there are no signatures, this will be NULL. */
> > +    GMimeSignatureList* sig_list;
> > +#else
> >      /* For signed or encrypted containers, the validity of the
> >       * signature.  May be NULL if signature verification failed.  If
> >       * there are simply no signatures, this will be non-NULL with an
> >       * empty signers list. */
> >      const GMimeSignatureValidity *sig_validity;
> > +#endif
> >=20=20
> >      /* Internal: Context inherited from the root iterator. */
> >      struct mime_node_context *ctx;
> > @@ -319,8 +341,12 @@ typedef struct mime_node {
> >   */
> >  notmuch_status_t
> >  mime_node_open (const void *ctx, notmuch_message_t *message,
> > -		GMimeCipherContext *cryptoctx, notmuch_bool_t decrypt,
> > -		mime_node_t **node_out);
> > +#ifdef GMIME_ATLEAST_26
> > +		GMimeCryptoContext *cryptoctx,
> > +#else
> > +		GMimeCipherContext *cryptoctx,
> > +#endif
> > +		notmuch_bool_t decrypt, mime_node_t **node_out);
> >=20=20
> >  /* 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 0f682db..bf67960 100644
> > --- a/notmuch-reply.c
> > +++ b/notmuch-reply.c
> > @@ -688,15 +688,22 @@ notmuch_reply_command (void *ctx, int argc, char =
*argv[])
> >  	reply_format_func =3D notmuch_reply_format_default;
> >=20=20
> >      if (decrypt) {
> > +#ifdef GMIME_ATLEAST_26
> > +	/* TODO: GMimePasswordRequestFunc */
> > +	params.cryptoctx =3D g_mime_gpg_context_new (NULL, "gpg");
> > +#else
> >  	GMimeSession* session =3D g_object_new (g_mime_session_get_type(), NU=
LL);
> >  	params.cryptoctx =3D g_mime_gpg_context_new (session, "gpg");
> > +#endif
> >  	if (params.cryptoctx) {
> >  	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.cr=
yptoctx, FALSE);
> >  	    params.decrypt =3D TRUE;
> >  	} else {
> >  	    fprintf (stderr, "Failed to construct gpg context.\n");
> >  	}
> > +#ifndef GMIME_ATLEAST_26
> >  	g_object_unref (session);
> > +#endif
> >      }
> >=20=20
> >      config =3D notmuch_config_open (ctx, NULL, NULL);
> > diff --git a/notmuch-show.c b/notmuch-show.c
> > index 91f566c..190210c 100644
> > --- a/notmuch-show.c
> > +++ b/notmuch-show.c
> > @@ -76,7 +76,11 @@ static void
> >  format_part_encstatus_json (int status);
> >=20=20
> >  static void
> > +#ifdef GMIME_ATLEAST_26
> > +format_part_sigstatus_json (GMimeSignatureList* siglist);
> > +#else
> >  format_part_sigstatus_json (const GMimeSignatureValidity* validity);
> > +#endif
> >=20=20
> >  static void
> >  format_part_content_json (GMimeObject *part);
> > @@ -486,6 +490,21 @@ show_text_part_content (GMimeObject *part, GMimeSt=
ream *stream_out)
> >  	g_object_unref(stream_filter);
> >  }
> >=20=20
> > +#ifdef GMIME_ATLEAST_26
> > +static const char*
> > +signature_status_to_string (GMimeSignatureStatus x)
> > +{
> > +    switch (x) {
> > +    case GMIME_SIGNATURE_STATUS_GOOD:
> > +	return "good";
> > +    case GMIME_SIGNATURE_STATUS_BAD:
> > +	return "bad";
> > +    case GMIME_SIGNATURE_STATUS_ERROR:
> > +	return "error";
> > +    }
> > +    return "unknown";
> > +}
> > +#else
> >  static const char*
> >  signer_status_to_string (GMimeSignerStatus x)
> >  {
> > @@ -501,6 +520,7 @@ signer_status_to_string (GMimeSignerStatus x)
> >      }
> >      return "unknown";
> >  }
> > +#endif
> >=20=20
> >  static void
> >  format_part_start_text (GMimeObject *part, int *part_count)
> > @@ -592,6 +612,73 @@ format_part_encstatus_json (int status)
> >      printf ("}]");
> >  }
> >=20=20
> > +#ifdef GMIME_ATLEAST_26
> > +static void
> > +format_part_sigstatus_json (GMimeSignatureList *siglist)
> > +{
> > +    printf (", \"sigstatus\": [");
> > +
> > +    if (!siglist) {
> > +	printf ("]");
> > +	return;
> > +    }
> > +
> > +    void *ctx_quote =3D talloc_new (NULL);
> > +    int i;
> > +    for (i =3D 0; i < g_mime_signature_list_length (siglist); i++) {
> > +	GMimeSignature *signature =3D g_mime_signature_list_get_signature (si=
glist, i);
> > +
> > +	if (i > 0)
> > +	    printf (", ");
> > +
> > +	printf ("{");
> > +
> > +	/* status */
> > +	GMimeSignatureStatus status =3D g_mime_signature_get_status (signatur=
e);
> > +	printf ("\"status\": %s",
> > +		json_quote_str (ctx_quote,
> > +				signature_status_to_string (status)));
> > +
> > +	GMimeCertificate *certificate =3D g_mime_signature_get_certificate (s=
ignature);
> > +	if (status =3D=3D GMIME_SIGNATURE_STATUS_GOOD) {
> > +	    if (certificate)
> > +		printf (", \"fingerprint\": %s", json_quote_str (ctx_quote, g_mime_c=
ertificate_get_fingerprint (certificate)));
> > +	    /* these dates are seconds since the epoch; should we
> > +	     * provide a more human-readable format string? */
> > +	    time_t created =3D g_mime_signature_get_created (signature);
> > +	    if (created !=3D -1)
> > +		printf (", \"created\": %d", (int) created);
> > +	    time_t expires =3D g_mime_signature_get_expires (signature);
> > +	    if (expires > 0)
> > +		printf (", \"expires\": %d", (int) expires);
> > +	    /* output user id only if validity is FULL or ULTIMATE. */
> > +	    /* note that gmime is using the term "trust" here, which
> > +	     * is WRONG.  It's actually user id "validity". */
> > +	    if (certificate) {
> > +		const char *name =3D g_mime_certificate_get_name (certificate);
> > +		GMimeCertificateTrust trust =3D g_mime_certificate_get_trust (certif=
icate);
> > +		if (name && (trust =3D=3D GMIME_CERTIFICATE_TRUST_FULLY || trust =3D=
=3D GMIME_CERTIFICATE_TRUST_ULTIMATE))
> > +		    printf (", \"userid\": %s", json_quote_str (ctx_quote, name));
> > +	    }
> > +	} else if (certificate) {
> > +	    const char *key_id =3D g_mime_certificate_get_key_id (certificate=
);
> > +	    if (key_id)
> > +		printf (", \"keyid\": %s", json_quote_str (ctx_quote, key_id));
> > +	}
> > +
> > +	GMimeSignatureError errors =3D g_mime_signature_get_errors (signature=
);
> > +	if (errors !=3D GMIME_SIGNATURE_ERROR_NONE) {
> > +	    printf (", \"errors\": %d", errors);
> > +	}
> > +
> > +	printf ("}");
> > +     }
> > +
> > +    printf ("]");
> > +
> > +    talloc_free (ctx_quote);
> > +}
> > +#else
> >  static void
> >  format_part_sigstatus_json (const GMimeSignatureValidity* validity)
> >  {
> > @@ -652,6 +739,7 @@ format_part_sigstatus_json (const GMimeSignatureVal=
idity* validity)
> >=20=20
> >      talloc_free (ctx_quote);
> >  }
> > +#endif
> >=20=20
> >  static void
> >  format_part_content_json (GMimeObject *part)
> > @@ -990,13 +1078,20 @@ notmuch_show_command (void *ctx, unused (int arg=
c), unused (char *argv[]))
> >  	} else if ((STRNCMP_LITERAL (argv[i], "--verify") =3D=3D 0) ||
> >  		   (STRNCMP_LITERAL (argv[i], "--decrypt") =3D=3D 0)) {
> >  	    if (params.cryptoctx =3D=3D NULL) {
> > +#ifdef GMIME_ATLEAST_26
> > +		/* TODO: GMimePasswordRequestFunc */
> > +		if (NULL =3D=3D (params.cryptoctx =3D g_mime_gpg_context_new(NULL, "=
gpg")))
> > +#else
> >  		GMimeSession* session =3D g_object_new(g_mime_session_get_type(), NU=
LL);
> >  		if (NULL =3D=3D (params.cryptoctx =3D g_mime_gpg_context_new(session=
, "gpg")))
> > +#endif
> >  		    fprintf (stderr, "Failed to construct gpg context.\n");
> >  		else
> >  		    g_mime_gpg_context_set_always_trust((GMimeGpgContext*)params.cry=
ptoctx, FALSE);
> > +#ifndef GMIME_ATLEAST_26
> >  		g_object_unref (session);
> >  		session =3D NULL;
> > +#endif
> >  	    }
> >  	    if (STRNCMP_LITERAL (argv[i], "--decrypt") =3D=3D 0)
> >  		params.decrypt =3D 1;
> > diff --git a/show-message.c b/show-message.c
> > index 8768889..83ecf81 100644
> > --- a/show-message.c
> > +++ b/show-message.c
> > @@ -48,7 +48,11 @@ show_message_part (mime_node_t *node,
> >  	format->part_encstatus (node->decrypt_success);
> >=20=20
> >      if (node->verify_attempted && format->part_sigstatus)
> > +#ifdef GMIME_ATLEAST_26
> > +	format->part_sigstatus (node->sig_list);
> > +#else
> >  	format->part_sigstatus (node->sig_validity);
> > +#endif
> >=20=20
> >      format->part_content (part);
> >=20=20
> > diff --git a/test/crypto b/test/crypto
> > index 0af4aa8..3779abc 100755
> > --- a/test/crypto
> > +++ b/test/crypto
> > @@ -104,6 +104,8 @@ test_expect_equal \
> >      "$expected"
> >=20=20
> >  test_begin_subtest "signature verification with signer key unavailable"
> > +# this is broken with current versions of gmime-2.6
> > +(ldd $(which notmuch) | grep -q gmime-2.6) && test_subtest_known_broken
>=20
> Just to be nitpicky, you should either escape the . in the regexp or
> pass -F to grep.  Otherwise I think this hack is fine (though it might
> have to get a little fancier once GMime fixes this bug).

Added -F :)

I guess we could also use pkg-config to test if gmime 2.6 is present. It
would also be simpler to test the version number once gmime fixes this
bug:

  pkg-config --atleast-version=3D2.6.x gmime-2.6 || test_subtest_known_brok=
en

But this would mean assuming that notmuch is built against a system-wide
gmime. Or it would require setting PKG_CONFIG_PATH before tests...
Complicated.

So IMHO once gmime fixes this bug we should just remove the
test_subtest_known_broken and maybe add something like this in
notmuch-client.h:

  #ifdef GMIME_MAJOR_VERSION
  #define GMIME_ATLEAST_26
  #if !GMIME_CHECK_VERSION(2, 6, x)
  #warning "Building against an old and buggy version of gmime. Please upda=
te to 2.6.x."
  #endif
  #endif

>=20
> >  # move the gnupghome temporarily out of the way
> >  mv "${GNUPGHOME}"{,.bak}
> >  output=3D$(notmuch show --format=3Djson --verify subject:"test signed =
message 001" \

=2D-=20
Thomas/Schnouki

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQEcBAEBAgAGBQJPGTXbAAoJEMPdciX+bh5I3YoH/1UyDErgI8P14uoXlpNvSI/R
SW6nf/wJtiEYQBKYl/7tvFdYc45NendE4QMegYoJvRHy0NyVKHv94vQS9N+kxsnX
RN4GEoUPSseFneZVR9mcP7nCWUcMzSqFYGejoqH09BmDEPZPFDW6llKmXRQpjTqQ
G6PMd9rS/bt6Z4Vl9nOLIv6amTaT6m/MHMgKCHSJAT9x+/ZNwfOK4uFEKt1ODtvD
enTp16RAqfOnHyo+jtLjGpjWbIyiO8eZiL+mUvd6Hn3XaEqcbWgsNuKwKNHvJq1L
pbw7+nXOfTFAYahovy8btxp3faIJ5MafyZk4vbBjJtcbWZn3Fes5G+BTCcjCABY=
=Mr4d
-----END PGP SIGNATURE-----
--=-=-=--