On Fri, 15 Sep 2017, Daniel Kahn Gillmor <dkg@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@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch _______________________________________________ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch