From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id BAA3A6DE00DB for ; Sat, 23 Sep 2017 08:36:25 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: 0.036 X-Spam-Level: X-Spam-Status: No, score=0.036 tagged_above=-999 required=5 tests=[AWL=0.056, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01] autolearn=disabled Received: from arlo.cworth.org ([127.0.0.1]) by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xD-TvI1eJPTf for ; Sat, 23 Sep 2017 08:36:24 -0700 (PDT) Received: from mail-lf0-f46.google.com (mail-lf0-f46.google.com [209.85.215.46]) by arlo.cworth.org (Postfix) with ESMTPS id 0B0006DE00C5 for ; Sat, 23 Sep 2017 08:36:24 -0700 (PDT) Received: by mail-lf0-f46.google.com with SMTP id m199so3410724lfe.3 for ; Sat, 23 Sep 2017 08:36:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nikula-org.20150623.gappssmtp.com; s=20150623; h=from:to:subject:in-reply-to:references:date:message-id:mime-version; bh=ROCE5VetzwimEaVwqANNvY3aVHH0O3JHyQkEwOi7moI=; b=sTNB3JrbU9Ei+LFQpyUVR7lIX3pJKS44Kv0KEhXtoCwxlLqeWkJHvA4s4Q6BXGA2a7 Pw4sTEuDc9zNKJKkliVqoCBPfkeRmwHR8SQzs6FVl2HXPF6aHr0SXYvOX9KTzUIvx5vL 8PvzTW4TF6C41W+P8JXT0Ca4kMBwEMsH2tuN+coQsaFtoH8qOsyMgsRguYuazPha40r2 mHdTSlZieYtaMZxJizAegksQz/qJML6J76BysOEF7coGKWvhVuvfVRovP094n73HPMh+ nig4bPQyTjwO55K1MXQSGNGmlrfX85wrU5C9CbzGgncP3lu+t8lM2T08GGm+3cm7qTlG 1w6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=ROCE5VetzwimEaVwqANNvY3aVHH0O3JHyQkEwOi7moI=; b=FPdk/0rtHKhloD4L8tDfZGndYxVTAw+KsFDW2DuR2DGAsoFeT0mjvIPBPvs8CeV/TW BiHHu9MVRJnvx2Z/t+kjOOTWEYKxldiBlol+RBy9xKvwJjknkliPDUW7nMpDguvPo7CT +kloOtTI/Q+Ox6XS0NB9p7z+0QnJsMMTKW43pvaqB31Ey8lNF25tukEzrjGk69HtA+Pp tXmVfp0dkcjjQKQCCF/fSxQZ07VevH1FAwhAoSHgiICqU7JKW//YoFL4pbsLIk6fryU9 cT6QbAFLYI1JGysyvndx6yEkkwV3kuQ6kXxOhKgRvTDGaN5AcYh3PxFv1FIL3qC55ptV jGpQ== X-Gm-Message-State: AHPjjUgjKwkkL9laN/vGXnH90wRbzzktYQCdZd1w52BIpFYu5SBHQBIg MQUP78GuaK+JuZuZKUjLSu6KEQIJbLE= X-Google-Smtp-Source: AOwi7QDOiJWe8s6iABghTdb5fNyZTMRrk6OuMcHh4zURZYSkywlrdpvG8YBZ4T+9xHwBnUqvqYp6Rg== X-Received: by 10.25.208.137 with SMTP id h131mr746280lfg.101.1506180982249; Sat, 23 Sep 2017 08:36:22 -0700 (PDT) Received: from localhost (mobile-access-5d6a60-234.dhcp.inet.fi. [93.106.96.234]) by smtp.gmail.com with ESMTPSA id 11sm509680lje.96.2017.09.23.08.36.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 23 Sep 2017 08:36:21 -0700 (PDT) From: Jani Nikula To: Daniel Kahn Gillmor , Notmuch Mail Subject: Re: [PATCH v2 02/10] crypto: make shared crypto code behave library-like In-Reply-To: <20170915055359.24123-3-dkg@fifthhorseman.net> References: <20170912230153.4175-10-dkg@fifthhorseman.net> <20170915055359.24123-1-dkg@fifthhorseman.net> <20170915055359.24123-3-dkg@fifthhorseman.net> Date: Sat, 23 Sep 2017 18:36:18 +0300 Message-ID: <87zi9lfmvh.fsf@nikula.org> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Sep 2017 15:36:25 -0000 On Fri, 15 Sep 2017, Daniel Kahn Gillmor 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@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch