[PATCH v2 02/10] crypto: make shared crypto code behave library-like

Jani Nikula jani at nikula.org
Sat Sep 23 08:36:18 PDT 2017


On Fri, 15 Sep 2017, Daniel Kahn Gillmor <dkg at fifthhorseman.net> wrote:
> If we're going to reuse the crypto code across both the library and
> the client, then it needs to report error states properly and not
> write to stderr.
> ---
>  lib/database.cc |  6 ++++
>  lib/notmuch.h   | 17 +++++++++++
>  mime-node.c     |  7 ++++-
>  util/crypto.c   | 89 ++++++++++++++++++++++++++++-----------------------------
>  util/crypto.h   |  6 ++--
>  5 files changed, 76 insertions(+), 49 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 79eb3d69..82a3d463 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -413,6 +413,12 @@ notmuch_status_to_string (notmuch_status_t status)
>  	return "Operation requires a database upgrade";
>      case NOTMUCH_STATUS_PATH_ERROR:
>  	return "Path supplied is illegal for this function";
> +    case NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL:
> +	return "Crypto protocol missing, malformed, or unintelligible";
> +    case NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION:
> +	return "Crypto engine initialization failure";
> +    case NOTMUCH_STATUS_UNKNOWN_CRYPTO_PROTOCOL:
> +	return "Unknown crypto protocol";
>      default:
>      case NOTMUCH_STATUS_LAST_STATUS:
>  	return "Unknown error status value";
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index f26565f3..6c76fb40 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -191,6 +191,23 @@ typedef enum _notmuch_status {
>       * function, in a way not covered by a more specific argument.
>       */
>      NOTMUCH_STATUS_ILLEGAL_ARGUMENT,
> +    /**
> +     * A MIME object claimed to have cryptographic protection which
> +     * notmuch tried to handle, but the protocol was not specified in
> +     * an intelligible way.
> +     */
> +    NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL,
> +    /**
> +     * Notmuch attempted to do crypto processing, but could not
> +     * initialize the engine needed to do so.
> +     */
> +    NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION,
> +    /**
> +     * A MIME object claimed to have cryptographic protection, and
> +     * notmuch attempted to process it, but the specific protocol was
> +     * something that notmuch doesn't know how to handle.
> +     */
> +    NOTMUCH_STATUS_UNKNOWN_CRYPTO_PROTOCOL,
>      /**
>       * Not an actual status value. Just a way to find out how many
>       * valid status values there are.
> diff --git a/mime-node.c b/mime-node.c
> index d9ff7de1..6cd7d2de 100644
> --- a/mime-node.c
> +++ b/mime-node.c
> @@ -265,7 +265,12 @@ _mime_node_create (mime_node_t *parent, GMimeObject *part)
>  	|| (GMIME_IS_MULTIPART_SIGNED (part) && node->ctx->crypto->verify)) {
>  	GMimeContentType *content_type = g_mime_object_get_content_type (part);
>  	const char *protocol = g_mime_content_type_get_parameter (content_type, "protocol");
> -	cryptoctx = _notmuch_crypto_get_gmime_context (node->ctx->crypto, protocol);
> +	notmuch_status_t status;
> +	status = _notmuch_crypto_get_gmime_ctx_for_protocol (node->ctx->crypto,
> +							     protocol, &cryptoctx);
> +	if (status) /* this is a warning, not an error */
> +	    fprintf (stderr, "Warning: %s (%s).\n", notmuch_status_to_string (status),
> +		     protocol ? protocol : "(NULL)");

For NULL protocol this will print "((NULL))".

>  	if (!cryptoctx)
>  	    return NULL;

I guess this will work because we initialize cryptoctx to NULL, but if
we return the status, I think we should trust status == success means
cryptoctx is fine, and otherwise we shouldn't touch or look at
cryptoctx.

Other than that, LGTM.


BR,
Jani.

>      }
> diff --git a/util/crypto.c b/util/crypto.c
> index 97e8c8f4..e7908197 100644
> --- a/util/crypto.c
> +++ b/util/crypto.c
> @@ -27,86 +27,86 @@
>  #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
>  
>  #if (GMIME_MAJOR_VERSION < 3)
> -/* Create a GPG context (GMime 2.6) */
> -static GMimeCryptoContext*
> -create_gpg_context (_notmuch_crypto_t *crypto)
> +/* Create or pass on a GPG context (GMime 2.6) */
> +static notmuch_status_t
> +get_gpg_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)
>  {
> -    GMimeCryptoContext *gpgctx;
> +    if (ctx == NULL || crypto == NULL)
> +	return NOTMUCH_STATUS_NULL_POINTER;
>  
>      if (crypto->gpgctx) {
> -	return crypto->gpgctx;
> +	*ctx = crypto->gpgctx;
> +	return NOTMUCH_STATUS_SUCCESS;
>      }
>  
>      /* TODO: GMimePasswordRequestFunc */
> -    gpgctx = g_mime_gpg_context_new (NULL, crypto->gpgpath ? crypto->gpgpath : "gpg");
> -    if (! gpgctx) {
> -	fprintf (stderr, "Failed to construct gpg context.\n");
> -	return NULL;
> +    crypto->gpgctx = g_mime_gpg_context_new (NULL, crypto->gpgpath ? crypto->gpgpath : "gpg");
> +    if (! crypto->gpgctx) {
> +	return NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION;
>      }
> -    crypto->gpgctx = gpgctx;
>  
> -    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) gpgctx, TRUE);
> -    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) gpgctx, FALSE);
> +    g_mime_gpg_context_set_use_agent ((GMimeGpgContext *) crypto->gpgctx, TRUE);
> +    g_mime_gpg_context_set_always_trust ((GMimeGpgContext *) crypto->gpgctx, FALSE);
>  
> -    return crypto->gpgctx;
> +    *ctx = crypto->gpgctx;
> +    return NOTMUCH_STATUS_SUCCESS;
>  }
>  
> -/* Create a PKCS7 context (GMime 2.6) */
> -static GMimeCryptoContext*
> -create_pkcs7_context (_notmuch_crypto_t *crypto)
> +/* Create or pass on a PKCS7 context (GMime 2.6) */
> +static notmuch_status_t 
> +get_pkcs7_context (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx)
>  {
> -    GMimeCryptoContext *pkcs7ctx;
> +    if (ctx == NULL || crypto == NULL)
> +	return NOTMUCH_STATUS_NULL_POINTER;
>  
> -    if (crypto->pkcs7ctx)
> -	return crypto->pkcs7ctx;
> +    if (crypto->pkcs7ctx) {
> +	*ctx = crypto->pkcs7ctx;
> +	return NOTMUCH_STATUS_SUCCESS;
> +    }
>  
>      /* TODO: GMimePasswordRequestFunc */
> -    pkcs7ctx = g_mime_pkcs7_context_new (NULL);
> -    if (! pkcs7ctx) {
> -	fprintf (stderr, "Failed to construct pkcs7 context.\n");
> -	return NULL;
> +    crypto->pkcs7ctx = g_mime_pkcs7_context_new (NULL);
> +    if (! crypto->pkcs7ctx) {
> +	return NOTMUCH_STATUS_FAILED_CRYPTO_CONTEXT_CREATION;
>      }
> -    crypto->pkcs7ctx = pkcs7ctx;
>  
> -    g_mime_pkcs7_context_set_always_trust ((GMimePkcs7Context *) pkcs7ctx,
> +    g_mime_pkcs7_context_set_always_trust ((GMimePkcs7Context *) crypto->pkcs7ctx,
>  					   FALSE);
>  
> -    return crypto->pkcs7ctx;
> +    *ctx = crypto->pkcs7ctx;
> +    return NOTMUCH_STATUS_SUCCESS;
>  }
>  static const struct {
>      const char *protocol;
> -    GMimeCryptoContext *(*get_context) (_notmuch_crypto_t *crypto);
> +    notmuch_status_t (*get_context) (_notmuch_crypto_t *crypto, GMimeCryptoContext **ctx);
>  } protocols[] = {
>      {
>  	.protocol = "application/pgp-signature",
> -	.get_context = create_gpg_context,
> +	.get_context = get_gpg_context,
>      },
>      {
>  	.protocol = "application/pgp-encrypted",
> -	.get_context = create_gpg_context,
> +	.get_context = get_gpg_context,
>      },
>      {
>  	.protocol = "application/pkcs7-signature",
> -	.get_context = create_pkcs7_context,
> +	.get_context = get_pkcs7_context,
>      },
>      {
>  	.protocol = "application/x-pkcs7-signature",
> -	.get_context = create_pkcs7_context,
> +	.get_context = get_pkcs7_context,
>      },
>  };
>  
>  /* for the specified protocol return the context pointer (initializing
>   * if needed) */
> -GMimeCryptoContext *
> -_notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protocol)
> +notmuch_status_t
> +_notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
> +					    const char *protocol,
> +					    GMimeCryptoContext **ctx)
>  {
> -    GMimeCryptoContext *cryptoctx = NULL;
> -    size_t i;
> -
> -    if (! protocol) {
> -	fprintf (stderr, "Cryptographic protocol is empty.\n");
> -	return cryptoctx;
> -    }
> +    if (! protocol)
> +	return NOTMUCH_STATUS_MALFORMED_CRYPTO_PROTOCOL;
>  
>      /* As per RFC 1847 section 2.1: "the [protocol] value token is
>       * comprised of the type and sub-type tokens of the Content-Type".
> @@ -114,15 +114,12 @@ _notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protoc
>       * parameter names as defined in this document are
>       * case-insensitive."  Thus, we use strcasecmp for the protocol.
>       */
> -    for (i = 0; i < ARRAY_SIZE (protocols); i++) {
> +    for (size_t i = 0; i < ARRAY_SIZE (protocols); i++) {
>  	if (strcasecmp (protocol, protocols[i].protocol) == 0)
> -	    return protocols[i].get_context (crypto);
> +	    return protocols[i].get_context (crypto, ctx);
>      }
>  
> -    fprintf (stderr, "Unknown or unsupported cryptographic protocol %s.\n",
> -	     protocol);
> -
> -    return NULL;
> +    return NOTMUCH_STATUS_UNKNOWN_CRYPTO_PROTOCOL;
>  }
>  
>  void
> diff --git a/util/crypto.h b/util/crypto.h
> index 6d15a6ae..d653ffb4 100644
> --- a/util/crypto.h
> +++ b/util/crypto.h
> @@ -21,8 +21,10 @@ typedef struct _notmuch_crypto {
>  
>  
>  #if (GMIME_MAJOR_VERSION < 3)
> -GMimeCryptoContext *
> -_notmuch_crypto_get_gmime_context (_notmuch_crypto_t *crypto, const char *protocol);
> +notmuch_status_t
> +_notmuch_crypto_get_gmime_ctx_for_protocol (_notmuch_crypto_t *crypto,
> +					    const char *protocol,
> +					    GMimeCryptoContext **ctx);
>  #endif
>  
>  void
> -- 
> 2.14.1
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch


More information about the notmuch mailing list