Re: [PATCH 03/18] crypto: use stashed session-key properties for decryption, if available

[David Bremner <david@tethera.net>]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:


> Note that this only works for GMime 2.6.21 and later (the session key
> interface wasn't available before then).  I don't think we're ready
> for this to be a minimum version requirement yet, so instead if you
> build against a prior version of GMime, it simply won't try to use
> stashed session keys.

Since you wrote this, I've deprecated GMime 2.6. I'm not sure that
changes anything here, but seems worth mentioning

> ---
>  doc/man7/notmuch-properties.rst | 31 +++++++++++++++++++++++++++++++
>  lib/index.cc                    |  2 +-
>  mime-node.c                     | 13 ++++++++++---
>  util/crypto.c                   | 31 ++++++++++++++++++++++++++++++-
>  util/crypto.h                   |  3 ++-
>  5 files changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/doc/man7/notmuch-properties.rst b/doc/man7/notmuch-properties.rst
> index 68121359..31d7a104 100644
> --- a/doc/man7/notmuch-properties.rst
> +++ b/doc/man7/notmuch-properties.rst
> @@ -74,6 +74,35 @@ of its normal activity.
>      **notmuch-config(1)**), then this property will not be set on that
>      message.
>  
> +**session-key**
> +
> +    When **notmuch-show(1)** or **nomtuch-reply** encounters a message
> +    with an encrypted part and ``--decrypt`` is set, if notmuch finds a
> +    ``session-key=`` property associated with the message, it will try
> +    that stashed session key for decryption.
> +

Its a nitpick, but I don't really understand/like including = with the
property name.  That will break, e.g. for anyone attempting to use it
from the API.

> -    clear = _notmuch_crypto_decrypt (crypto_ctx, encrypted_data, NULL, &err);
> +    clear = _notmuch_crypto_decrypt (message, crypto_ctx, encrypted_data, NULL, &err);

The way the diff works out, I was pretty confused by seeing several
"wrong" calls to _notmuch_crypto_decrypt before the actual change. It
would be nice to telegraph that somehow, perhaps in the commit message.

>  {
>      GMimeObject *ret = NULL;
>  
> +    /* the versions of notmuch that can support session key decryption */
> +#if (GMIME_MAJOR_VERSION >= 3 || (GMIME_MAJOR_VERSION == 2 && GMIME_MINOR_VERSION == 6 && GMIME_MICRO_VERSION >= 21))

Personally I would be fine with (and probably happier) only supporting
new features when using gmime-3.0.  Debugging crypto related stuff is
always hard (see recent discussion about _mime_node_create, where we
still don't know what's wrong, and are just papering over the problem),
and it seems worth striving for simplicity as much as possible.  I also
don't know how motivated gmime upstream is to fix bugs in 2.6; I could
certainly understand if the answer was "not very".

There is, by the way, a function notmuch_built_with that can be used to
introspect the library as to what optional features it is built
with. It's used in notmuch_config to report back to the user about the
presence of optional features.