Re: [PATCH v2 4/5] cli: new crypto verify flag to handle verification

[Jani Nikula <jani@nikula.org>]

On Fri, 18 May 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> Use this flag rather than depend on the existence of an initialized
> gpgctx, to determine whether we should verify a multipart/signed.  We
> will be moving to create the ctx lazily, so we don't want to depend on
> it being previously initialized if it's not needed.
> ---
>  mime-node.c      |    5 ++---
>  notmuch-client.h |    8 ++++----
>  notmuch-reply.c  |    1 +
>  notmuch-show.c   |   14 +++++++++++---
>  4 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/mime-node.c b/mime-node.c
> index 3dda900..3adbe5a 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -183,8 +183,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>      }
>  
>      /* Handle PGP/MIME parts */
> -    if (GMIME_IS_MULTIPART_ENCRYPTED (part)
> -	&& node->ctx->crypto->gpgctx && node->ctx->crypto->decrypt) {
> +    if (GMIME_IS_MULTIPART_ENCRYPTED (part) && node->ctx->crypto->decrypt) {
>  	if (node->nchildren != 2) {
>  	    /* this violates RFC 3156 section 4, so we won't bother with it. */
>  	    fprintf (stderr, "Error: %d part(s) for a multipart/encrypted "
> @@ -218,7 +217,7 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  			 (err ? err->message : "no error explanation given"));
>  	    }
>  	}
> -    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->gpgctx) {
> +    } else if (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->verify) {
>  	if (node->nchildren != 2) {
>  	    /* this violates RFC 3156 section 5, so we won't bother with it. */
>  	    fprintf (stderr, "Error: %d part(s) for a multipart/signed message "
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 9892968..c671c13 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -80,6 +80,7 @@ typedef struct notmuch_crypto {
>  #else
>      GMimeCipherContext* gpgctx;
>  #endif
> +    notmuch_bool_t verify;
>      notmuch_bool_t decrypt;
>  } notmuch_crypto_t;
>  
> @@ -345,10 +346,9 @@ struct mime_node {
>  };
>  
>  /* Construct a new MIME node pointing to the root message part of
> - * message.  If crypto->gpgctx is non-NULL, it will be used to verify
> - * signatures on any child parts.  If crypto->decrypt is true, then
> - * crypto.gpgctx will additionally be used to decrypt any encrypted
> - * child parts.
> + * message. If crypto->verify is true, signed child parts will be
> + * verified. If crypto->decrypt is true, encrypted child parts will be
> + * decrypted.
>   *
>   * Return value:
>   *
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 34a906e..345be76 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -674,6 +674,7 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>      int opt_index, ret = 0;
>      int (*reply_format_func)(void *ctx, notmuch_config_t *config, notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
>      notmuch_crypto_t crypto = {
> +	.verify = FALSE,
>  	.decrypt = FALSE
>      };
>      int format = FORMAT_DEFAULT;
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 66c74e2..f4ee038 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -987,11 +987,11 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>  	.part = -1,
>  	.omit_excluded = TRUE,
>  	.crypto = {
> +	    .verify = FALSE,
>  	    .decrypt = FALSE
>  	}
>      };
>      int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
> -    notmuch_bool_t verify = FALSE;
>      int exclude = EXCLUDE_TRUE;
>  
>      notmuch_opt_desc_t options[] = {
> @@ -1008,7 +1008,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>  	{ NOTMUCH_OPT_INT, &params.part, "part", 'p', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &params.entire_thread, "entire-thread", 't', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.decrypt, "decrypt", 'd', 0 },
> -	{ NOTMUCH_OPT_BOOLEAN, &verify, "verify", 'v', 0 },
> +	{ NOTMUCH_OPT_BOOLEAN, &params.crypto.verify, "verify", 'v', 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
>  
> @@ -1018,6 +1018,10 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>  	return 1;
>      }
>  
> +    /* decryption implies verification */
> +    if (params.crypto.decrypt)
> +	params.crypto.verify = TRUE;

This does not change existing behaviour, only makes it more obvious
(which is good), but this seems to be missing from the man page. I
presume technically decryption doesn't have to imply verification, but
it's probably a good thing. It should be documented, but does not have
to be a part of this series.

Thanks for working on this. The series looks good to me (apart from the
comments already made by Austin), and the compromises after our debate
reasonable.


BR,
Jani.


> +
>      if (format_sel == NOTMUCH_FORMAT_NOT_SPECIFIED) {
>  	/* if part was requested and format was not specified, use format=raw */
>  	if (params.part >= 0)
> @@ -1052,7 +1056,7 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>  	break;
>      }
>  
> -    if (params.crypto.decrypt || verify) {
> +    if (params.crypto.decrypt || params.crypto.verify) {
>  #ifdef GMIME_ATLEAST_26
>  	/* TODO: GMimePasswordRequestFunc */
>  	params.crypto.gpgctx = g_mime_gpg_context_new (NULL, "gpg");
> @@ -1063,6 +1067,10 @@ notmuch_show_command (void *ctx, unused (int argc), unused (char *argv[]))
>  	if (params.crypto.gpgctx) {
>  	    g_mime_gpg_context_set_always_trust ((GMimeGpgContext*) params.crypto.gpgctx, FALSE);
>  	} else {
> +	    /* If we fail to create the gpgctx set the verify and
> +	     * decrypt flags to FALSE so we don't try to do any
> +	     * further verification or decryption */
> +	    params.crypto.verify = FALSE;
>  	    params.crypto.decrypt = FALSE;
>  	    fprintf (stderr, "Failed to construct gpg context.\n");
>  	}
> -- 
> 1.7.10
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch