Quoth Thomas Jost on Jan 17 at 12:56 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.

Awesome.  Comments inline below.

> This is mostly based on id:"8762i8hrb9.fsf at 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?

> ---
>  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.

> +         node->decrypted_child = g_mime_multipart_encrypted_decrypt
> +             (encrypteddata, node->ctx->cryptoctx, &decrypt_result, &err);
> +         if (node->decrypted_child) {
> +             node->decrypt_success = node->verify_attempted = TRUE;
> +             node->sig_list = g_mime_decrypt_result_get_signatures 
> (decrypt_result);
> +             if (!node->sig_list)
> +                 fprintf (stderr, "Failed to get signatures: %s\n",
> +                          (err ? err->message : "no error explanation 
> given"));

My understanding is that g_mime_decrypt_result_get_signatures returns
NULL if there are no signatures and that this isn't an error.  This
differs from 2.4, which would return an empty but non-NULL list.

Also, I believe you have to free the sig_list in both branches now,
which means the talloc_set_destructor can be moved to common logic
outside of the if decrypted/signed.

> +#else
>           node->decrypted_child = g_mime_multipart_encrypted_decrypt
>               (encrypteddata, node->ctx->cryptoctx, &err);
>           if (node->decrypted_child) {
>               node->decrypt_success = node->verify_attempted = TRUE;
>               node->sig_validity = 
> g_mime_multipart_encrypted_get_signature_validity (encrypteddata);
> +#endif

It's confusing to have the open braces in the #ifdef'd region with a
matching close brace outside of it (and I imagine this confuses
editors and uncrustify, too).  You could either copy the else part in
both branches of the #ifdef or avoid duplicated code with something
like

#ifdef GMIME_26
  .. node->decrypted_child = ..
#else
  .. node->decrypted_child = ..
#endif
  if (node->decrypted_child) {
    node->decrypt_success = node->verify_attempted = TRUE;
#ifdef GMIME_26
    node->sig_list = ..
#else
    node->sig_validity = ..
#endif
  } else {
    fprintf (stderr, ..);
  }

>           } 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.

> +#else
>      if (node->verify_attempted && !node->sig_validity)
> +#endif
>       fprintf (stderr, "Failed to verify signed part: %s\n",
>                (err ? err->message : "no error explanation given"));
>  
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 517c010..e85f882 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -30,6 +30,12 @@
>  
>  #include <gmime/gmime.h>
>  
> +/* GMIME_CHECK_VERSION is only available in gmime >= 2.5. But so are
> + * GMIME_MAJOR_VERSION and friends. */

Hah.

> +#ifdef GMIME_MAJOR_VERSION
> +#define GMIME_26
> +#endif
> +
>  #include "notmuch.h"
>  
>  /* This is separate from notmuch-private.h because we're trying to
> @@ -69,7 +75,11 @@ typedef struct notmuch_show_format {
>      void (*part_start) (GMimeObject *part,
>                       int *part_count);
>      void (*part_encstatus) (int status);
> +#ifdef GMIME_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 +93,11 @@ typedef struct notmuch_show_params {
>      int entire_thread;
>      int raw;
>      int part;
> +#ifdef GMIME_26
> +    GMimeCryptoContext* cryptoctx;
> +#else
>      GMimeCipherContext* cryptoctx;
> +#endif
>      int decrypt;
>  } notmuch_show_params_t;
>  
> @@ -286,7 +300,12 @@ typedef struct mime_node {
>       * signature.  May be NULL if signature verification failed.  If
>       * there are simply no signatures, this will be non-NULL with an
>       * empty signers list. */
> +#ifdef GMIME_26
> +    /* TODO: update the above comment... */

Since this behaves very differently in 2.6, I think documenting it is
important (and being very careful about the differences).  Maybe

/* The list of signatures for signed or encrypted containers.  If
 * there are no signatures, this will be NULL. */

> +    GMimeSignatureList* sig_list;
> +#else
>      const GMimeSignatureValidity *sig_validity;
> +#endif
>  
>      /* Internal: Context inherited from the root iterator. */
>      struct mime_node_context *ctx;
> @@ -311,8 +330,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_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 da3acce..dc37c51 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_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_26
>       g_object_unref (session);
> +#endif
>      }
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
> diff --git a/notmuch-show.c b/notmuch-show.c
> index d14dac9..263ab72 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_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_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,75 @@ format_part_encstatus_json (int status)
>      printf ("}]");
>  }
>  
> +#ifdef GMIME_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) {

Style nit: notmuch uses 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)
> +     {

Style nit: break after brace.

(Presumably this is copied from the existing
format_part_sigstatus_json, but since it's technically new code
there's no reason not to fix these up.)

> +         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);

Is it intentional that the two above checks are different?  I would
think the second should be expires != -1.

> +         /* 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)
> +         {

Break after brace.

> +             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\": %x", errors);

This should be %d (I would say 0x%x, but JSON doesn't support hex
literals).  I see this bug came from the original
format_part_sigstatus_json.  Maybe there should be a quick patch
before this one that fixes the source of the bug?

> +     }
> +
> +     printf ("}");
> +     }
> +
> +    printf ("]");
> +
> +    talloc_free (ctx_quote);
> +}
> +#else
>  static void
>  format_part_sigstatus_json (const GMimeSignatureValidity* validity)
>  {
> @@ -652,6 +741,7 @@ format_part_sigstatus_json (const GMimeSignatureValidity* 
> validity)
>  
>      talloc_free (ctx_quote);
>  }
> +#endif
>  
>  static void
>  format_part_content_json (GMimeObject *part)
> @@ -990,13 +1080,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_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_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..65269fd 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_26
> +     format->part_sigstatus (node->sig_list);
> +#else
>       format->part_sigstatus (node->sig_validity);
> +#endif
>  
>      format->part_content (part);
>  

Reply via email to