On Thu, 19 Jan 2012 23:10:44 -0500, Austin Clements <amdra...@mit.edu> wrote: > 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 > > compatibility > > while preserving compatibility with gmime 2.4 too. > > > > This is mostly based on id:"8762i8hrb9....@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 > > is > > currently broken (signature verification with signer key unavailable), most > > likely because of a bug in gmime which will hopefully be fixed in a future > > 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(-) > > > > 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; > > > > /* Context provided by the caller. */ > > +#ifdef GMIME_ATLEAST_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_ATLEAST_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_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 > > > > 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, > > GMimeObject *part) > > 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, node->ctx->cryptoctx, &decrypt_result, &err); > > +#else > > node->decrypted_child = g_mime_multipart_encrypted_decrypt > > (encrypteddata, node->ctx->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); > > + g_object_ref (node->sig_list); > > 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. > > > + 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")); > > @@ -182,6 +212,15 @@ _mime_node_create (const 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), node->ctx->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 > > @@ -195,17 +234,30 @@ _mime_node_create (const mime_node_t *parent, > > GMimeObject *part) > > node->verify_attempted = TRUE; > > node->sig_validity = sig_validity; > > if (sig_validity) { > > - GMimeSignatureValidity **proxy = > > - talloc (node, GMimeSignatureValidity *); > > + GMimeSignatureValidity **proxy = talloc (node, > > GMimeSignatureValidity *); > > (See below) > > > *proxy = sig_validity; > > talloc_set_destructor (proxy, _signature_validity_free); > > } > > +#endif > > } > > } > > > > +#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 *); > > 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.) > > > + *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); > > 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 @@ > > > > #include <gmime/gmime.h> > > > > +/* 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" > > > > /* 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; > > > > @@ -290,11 +306,17 @@ typedef struct mime_node { > > > > /* 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 > > > > /* 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); > > > > /* 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 = notmuch_reply_format_default; > > > > if (decrypt) { > > +#ifdef GMIME_ATLEAST_26 > > + /* TODO: GMimePasswordRequestFunc */ > > + params.cryptoctx = g_mime_gpg_context_new (NULL, "gpg"); > > +#else > > GMimeSession* session = g_object_new (g_mime_session_get_type(), NULL); > > params.cryptoctx = g_mime_gpg_context_new (session, "gpg"); > > +#endif > > if (params.cryptoctx) { > > g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) > > params.cryptoctx, FALSE); > > params.decrypt = TRUE; > > } else { > > fprintf (stderr, "Failed to construct gpg context.\n"); > > } > > +#ifndef GMIME_ATLEAST_26 > > g_object_unref (session); > > +#endif > > } > > > > config = 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); > > > > static void > > +#ifdef GMIME_ATLEAST_26 > > +format_part_sigstatus_json (GMimeSignatureList* siglist); > > +#else > > format_part_sigstatus_json (const GMimeSignatureValidity* validity); > > +#endif > > > > static void > > format_part_content_json (GMimeObject *part); > > @@ -486,6 +490,21 @@ show_text_part_content (GMimeObject *part, GMimeStream > > *stream_out) > > g_object_unref(stream_filter); > > } > > > > +#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 > > > > static void > > format_part_start_text (GMimeObject *part, int *part_count) > > @@ -592,6 +612,73 @@ format_part_encstatus_json (int status) > > printf ("}]"); > > } > > > > +#ifdef GMIME_ATLEAST_26 > > +static void > > +format_part_sigstatus_json (GMimeSignatureList *siglist) > > +{ > > + printf (", \"sigstatus\": ["); > > + > > + if (!siglist) { > > + printf ("]"); > > + return; > > + } > > + > > + void *ctx_quote = talloc_new (NULL); > > + int i; > > + for (i = 0; i < g_mime_signature_list_length (siglist); i++) { > > + GMimeSignature *signature = g_mime_signature_list_get_signature > > (siglist, i); > > + > > + if (i > 0) > > + printf (", "); > > + > > + printf ("{"); > > + > > + /* status */ > > + GMimeSignatureStatus status = g_mime_signature_get_status (signature); > > + printf ("\"status\": %s", > > + json_quote_str (ctx_quote, > > + signature_status_to_string (status))); > > + > > + GMimeCertificate *certificate = g_mime_signature_get_certificate > > (signature); > > + if (status == GMIME_SIGNATURE_STATUS_GOOD) { > > + if (certificate) > > + printf (", \"fingerprint\": %s", json_quote_str (ctx_quote, > > g_mime_certificate_get_fingerprint (certificate))); > > + /* these dates are seconds since the epoch; should we > > + * provide a more human-readable format string? */ > > + time_t created = g_mime_signature_get_created (signature); > > + if (created != -1) > > + printf (", \"created\": %d", (int) created); > > + time_t expires = 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 = g_mime_certificate_get_name (certificate); > > + GMimeCertificateTrust trust = g_mime_certificate_get_trust > > (certificate); > > + if (name && (trust == GMIME_CERTIFICATE_TRUST_FULLY || trust == > > GMIME_CERTIFICATE_TRUST_ULTIMATE)) > > + printf (", \"userid\": %s", json_quote_str (ctx_quote, > > name)); > > + } > > + } else if (certificate) { > > + const char *key_id = g_mime_certificate_get_key_id (certificate); > > + if (key_id) > > + printf (", \"keyid\": %s", json_quote_str (ctx_quote, key_id)); > > + } > > + > > + GMimeSignatureError errors = g_mime_signature_get_errors (signature); > > + if (errors != 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 > > GMimeSignatureValidity* validity) > > > > talloc_free (ctx_quote); > > } > > +#endif > > > > static void > > format_part_content_json (GMimeObject *part) > > @@ -990,13 +1078,20 @@ notmuch_show_command (void *ctx, unused (int argc), > > unused (char *argv[])) > > } else if ((STRNCMP_LITERAL (argv[i], "--verify") == 0) || > > (STRNCMP_LITERAL (argv[i], "--decrypt") == 0)) { > > if (params.cryptoctx == NULL) { > > +#ifdef GMIME_ATLEAST_26 > > + /* TODO: GMimePasswordRequestFunc */ > > + if (NULL == (params.cryptoctx = g_mime_gpg_context_new(NULL, > > "gpg"))) > > +#else > > GMimeSession* session = g_object_new(g_mime_session_get_type(), > > NULL); > > if (NULL == (params.cryptoctx = 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.cryptoctx, > > FALSE); > > +#ifndef GMIME_ATLEAST_26 > > g_object_unref (session); > > session = NULL; > > +#endif > > } > > if (STRNCMP_LITERAL (argv[i], "--decrypt") == 0) > > params.decrypt = 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); > > > > 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 > > > > format->part_content (part); > > > > 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" > > > > 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 > > 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=2.6.x gmime-2.6 || test_subtest_known_broken 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 update to 2.6.x." #endif #endif > > > # move the gnupghome temporarily out of the way > > mv "${GNUPGHOME}"{,.bak} > > output=$(notmuch show --format=json --verify subject:"test signed message > > 001" \ -- Thomas/Schnouki
pgpX11TcJKYPB.pgp
Description: PGP signature
_______________________________________________ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch