Date: Tue, 17 Jan 2012 15:38:36 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Thomas Jost <schnouki@schnouki.net>
Subject: Re: [PATCH] Add pseudo-compatibility with gmime 2.6
Message-ID: <20120117203836.GR16740@mit.edu>
References: <8762gbtd6p.fsf@schnouki.net>
	<1326758199-18058-1-git-send-email-schnouki@schnouki.net>
	<20120117034714.GG16740@mit.edu> <8739bea9lc.fsf@thor.loria.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8739bea9lc.fsf@thor.loria.fr>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Thomas Jost on Jan 17 at 11:48 am:
> On Mon, 16 Jan 2012 22:47:14 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > Quoth Thomas Jost on Jan 17 at 12:56 am:
> > > This is mostly based on id:"8762i8hrb9.fsf@bookbinder.fernseed.info".
> > > 
> > > 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
> > > fails (signature verification with signer key unavailable) but this will be hard
> > > to fix since the new API does not report the reason why a signature verification
> > > fails (other than the human-readable error message).
> > 
> > What is the result of this failing test?
> 
> The test looks like that:
> 
>         FAIL   signature verification with signer key unavailable
>            --- crypto.4.expected   2012-01-16 23:05:06.765651183 +0000
>            +++ crypto.4.output     2012-01-16 23:05:06.765651183 +0000
>            @@ -12,9 +12,7 @@
>              "Bcc": "",
>              "Date": "01 Jan 2000 12:00:00 -0000"},
>              "body": [{"id": 1,
>            - "sigstatus": [{"status": "error",
>            - "keyid": "6D92612D94E46381",
>            - "errors": 2}],
>            + "sigstatus": [],
>              "content-type": "multipart/signed",
>              "content": [{"id": 2,
>              "content-type": "text/plain",
>        Failed to verify signed part: gpg: keyblock resource `/home/schnouki/dev/notmuch/test/tmp.crypto/gnupg/pubring.gpg': file open error
>        gpg: armor header: Version: GnuPG v1.4.11 (GNU/Linux)
>        gpg: Signature made Mon Jan 16 23:05:06 2012 UTC using RSA key ID 94E46381
>        gpg: Can't check signature: public key not found
> 
> So basically if a signer public key is missing, we can't get the status
> signature: empty "sigstatus" field in the JSON output, "Unknown
> signature status" in Emacs.
> 
> IMHO this is a bug in gmime 2.6, and I think I know what causes it. I'll
> file a bug in gmime and hopefully they'll find a cleaner way to fix it
> than the one I came up with :)

Oh, okay.  That does seem like a bug in GMime.  Do you think it would
be possible to mark this test as "broken" if notmuch is using GMime
2.6?  Off the top of my head, I can't think of an easy way to plumb
that information through to the test suite.  I don't think we should
push any patches that knowingly break a test, even if it's in just one
configuration.

> > 
> > > ---
> > >  mime-node.c      |   50 ++++++++++++++++++++++++++-
> > >  notmuch-client.h |   27 ++++++++++++++-
> > >  notmuch-reply.c  |    7 ++++
> > >  notmuch-show.c   |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  show-message.c   |    4 ++
> > >  5 files changed, 181 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/mime-node.c b/mime-node.c
> > > index d26bb44..ae2473d 100644
> > > --- a/mime-node.c
> > > +++ b/mime-node.c
> > > @@ -33,7 +33,11 @@ typedef struct mime_node_context {
> > >      GMimeMessage *mime_message;
> > >  
> > >      /* Context provided by the caller. */
> > > +#ifdef GMIME_26
> > > +    GMimeCryptoContext *cryptoctx;
> > > +#else
> > >      GMimeCipherContext *cryptoctx;
> > > +#endif
> > >      notmuch_bool_t decrypt;
> > >  } mime_node_context_t;
> > >  
> > > @@ -57,8 +61,12 @@ _mime_node_context_free (mime_node_context_t *res)
> > >  
> > >  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_26
> > > +		GMimeCryptoContext *cryptoctx,
> > > +#else
> > > +		GMimeCipherContext *cryptoctx,
> > > +#endif
> > > +		notmuch_bool_t decrypt, mime_node_t **root_out)
> > >  {
> > >      const char *filename = notmuch_message_get_filename (message);
> > >      mime_node_context_t *mctx;
> > > @@ -112,12 +120,21 @@ DONE:
> > >      return status;
> > >  }
> > >  
> > > +#ifdef GMIME_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
> > >  
> > >  static mime_node_t *
> > >  _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > > @@ -165,11 +182,23 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > >  	    GMimeMultipartEncrypted *encrypteddata =
> > >  		GMIME_MULTIPART_ENCRYPTED (part);
> > >  	    node->decrypt_attempted = TRUE;
> > > +#ifdef GMIME_26
> > > +	    GMimeDecryptResult *decrypt_result = g_mime_decrypt_result_new ();
> > 
> > I think g_mime_multipart_encrypted_decrypt allocates the
> > GMimeDecryptResult for you, so this will just leak memory.
> 
> You're right, fixed. Will it be freed along with node->decrypted_child,
> or do we need to handle this ourself?

That would be nice to know.  My guess would be that we have to free it
ourselves (or dereference it, at any rate), but the documentation
doesn't say.

> > >  	    } else {
> > >  		fprintf (stderr, "Failed to decrypt part: %s\n",
> > >  			 (err ? err->message : "no error explanation given"));
> > > @@ -182,6 +211,18 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > >  		     "(must be exactly 2)\n",
> > >  		     node->nchildren);
> > >  	} else {
> > > +#ifdef GMIME_26
> > > +	    GMimeSignatureList *sig_list = g_mime_multipart_signed_verify
> > > +		(GMIME_MULTIPART_SIGNED (part), node->ctx->cryptoctx, &err);
> > > +	    node->verify_attempted = TRUE;
> > > +	    node->sig_list = sig_list;
> > > +	    if (sig_list) {
> > > +		GMimeSignatureList **proxy =
> > > +		    talloc (node, GMimeSignatureList *);
> > > +		*proxy = sig_list;
> > > +		talloc_set_destructor (proxy, _signature_list_free);
> > > +	    }
> > > +#else
> > >  	    /* For some reason the GMimeSignatureValidity returned
> > >  	     * here is not a const (inconsistent with that
> > >  	     * returned by
> > > @@ -200,10 +241,15 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > >  		*proxy = sig_validity;
> > >  		talloc_set_destructor (proxy, _signature_validity_free);
> > >  	    }
> > > +#endif
> > >  	}
> > >      }
> > >  
> > > +#ifdef GMIME_26
> > > +    if (node->verify_attempted && !node->sig_list)
> > 
> > Hmm.  This is correct for signed parts, but will incorrectly trigger
> > for an encrypted part with no signatures.  For 2.6, I think this error
> > checking may have to move into the branches of the if encrypted/signed
> > since for encrypted parts you have to check if
> > g_mime_multipart_encrypted_decrypt returned NULL.
> 
> That sound right. The weird part is that it did not cause anything to
> fail in the test suite...

It would be worth adding a test with an encrypted but unsigned part.
I don't know enough GPG myself to do that.