Stashed session keys

classic Classic list List threaded Threaded
47 messages Options
123
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

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

On Wed 2017-10-25 02:51:48 -0400, Daniel Kahn Gillmor wrote:

> diff --git a/util/crypto.c b/util/crypto.c
> index 087536ec..e014db5d 100644
> --- a/util/crypto.c
> +++ b/util/crypto.c
> @@ -140,13 +140,42 @@ void _notmuch_crypto_cleanup (unused(_notmuch_crypto_t *crypto))
>  #endif
>  
>  GMimeObject *
> -_notmuch_crypto_decrypt (g_mime_3_unused(GMimeCryptoContext* crypto_ctx),
> +_notmuch_crypto_decrypt (notmuch_message_t *message,
> + g_mime_3_unused(GMimeCryptoContext* crypto_ctx),
>   GMimeMultipartEncrypted *part,
>   GMimeDecryptResult **decrypt_result,
>   GError **err)
>  {
>      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))
> +    if (message) {
> + notmuch_message_properties_t *list = NULL;
> +
> + for (list = notmuch_message_get_properties (message, "session-key", TRUE);
> +     notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
> +#if (GMIME_MAJOR_VERSION < 3)
> +    ret = g_mime_multipart_encrypted_decrypt_session (part,
> +      crypto_ctx,
> +      notmuch_message_properties_value (list),
> +      decrypt_result, err);
> +#else
> +    ret = g_mime_multipart_encrypted_decrypt (part,
> +      GMIME_DECRYPT_NONE,
> +      notmuch_message_properties_value (list),
> +      decrypt_result, err);
> +#endif
> +    if (ret)
> + break;
> + }
> + if (list)
> +    notmuch_message_properties_destroy (list);
> + if (ret)
> +    return ret;
> +    }
> +#endif
> +
>  #if (GMIME_MAJOR_VERSION < 3)
>      ret = g_mime_multipart_encrypted_decrypt(part, crypto_ctx,
>       decrypt_result, err);
In the change above, i realized that we might accidentally clobber the
GError of any intermediate failed decryption attempt, which would
produce a GLib warning to stderr.

In my revised/updated series ("session-keys" on
https://gitlab.com/dkg/notmuch), i clear err (if present) before each
attempted decryption.  This effectively throws away all errors except
for the last one, but i think that's the right thing to do -- we'll try
whatever we can for decrypting, but if the final decryption fails,
that's the error we'd want reported back anyway.

           --dkg the self-reviewer :)

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: Stashed session keys

In reply to this post by Daniel Kahn Gillmor
On Wed 2017-10-25 02:51:45 -0400, Daniel Kahn Gillmor wrote:
> Now that cleartext indexing is merged, let's add the ability to stash
> session keys!

I'd really appreciate feedback on this series.  It works for me, and i'm
using it.  I don't want this to take another two years to land, if
possible.  And it blocks additional improvements that i would like to
eventually land as well.

So any review of any part of this series would be appreciated.  I want
to make notmuch a mail client that provides actually usable encrypted
mail.

Regards,

      --dkg

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Jameson Graef Rollins Jameson Graef Rollins
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 07/18] crypto: new decryption policy "auto"

In reply to this post by Daniel Kahn Gillmor
On Wed, Oct 25 2017, Daniel Kahn Gillmor <[hidden email]> wrote:

> diff --git a/util/crypto.h b/util/crypto.h
> index b23ca747..dc95b4ca 100644
> --- a/util/crypto.h
> +++ b/util/crypto.h
> @@ -16,7 +16,8 @@ typedef struct _notmuch_crypto {
>  } _notmuch_crypto_t;
>  
>  GMimeObject *
> -_notmuch_crypto_decrypt (notmuch_message_t *message,
> +_notmuch_crypto_decrypt (notmuch_decryption_policy_t decrypt,
> + notmuch_message_t *message,
>   GMimeCryptoContext* crypto_ctx,
>   GMimeMultipartEncrypted *part,
>   GMimeDecryptResult **decrypt_result,
Why does _notmuch_crypt_decrypt need to have
"notmuch_decryption_policy_t decrypt" as an input argument?  Isn't
notmuch_decryption_policy_t already an attribute of the crypto_ctx?  Is
there some situation where the policy would differ from what's specified
in the crypto_ctx?

jamie.

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Jameson Graef Rollins Jameson Graef Rollins
Reply | Threaded
Open this post in threaded view
|

Re: Stashed session keys

In reply to this post by Daniel Kahn Gillmor
On Wed, Oct 25 2017, Daniel Kahn Gillmor <[hidden email]> wrote:
> Now that cleartext indexing is merged, let's add the ability to stash
> session keys!

I have reviewed and tested this series, and it seems solidly implemented
and very well motivated.  I have been using it regularly for a couple
weeks now and have found no issues with it's usage, and have noticed the
considerable speed up when viewing encrypted threads (as much as x8 for
show on a thread of just 8 encrypted messages).  I fully support it's
integration unconditionally.

It should be emphasized that this series is actually fairly critical for
good support of message encryption.  Without this series it's actually
possible to completely lose access to encrypted mail if one were to
rotate/replace their encryption key, which one might reasonably be
expected to do.  Without access to the original encryption key or the
message session key, there is no way to access the contents of an
encrypted message.  If, however, the session key is stashed in the
index, the original encryption key can be destroyed and the message can
still be accessed.  Daniel likes to think of this in terms of being able
to "delete" encrypted messages in the wild (via deletion of the original
encryption key) whereas I like to think of it in terms of preserving
access to received encrypted messages after key rotation.  Both benefits
hold, though, obviously.

>    +------------------------+-------+------+---------+------+
>    |                        | false | auto | nostash | true |
>    +========================+=======+======+=========+======+
>    | Index cleartext using  |       |  X   |    X    |  X   |
>    | stashed session keys   |       |      |         |      |
>    +------------------------+-------+------+---------+------+
>    | Index cleartext        |       |      |    X    |  X   |
>    | using secret keys      |       |      |         |      |
>    +------------------------+-------+------+---------+------+
>    | Stash session keys     |       |      |         |  X   |
>    +------------------------+-------+------+---------+------+
>    | Delete stashed session |   X   |      |         |      |
>    | keys on reindex        |       |      |         |      |
>    +------------------------+-------+------+---------+------+
I think these policies cover all potential use cases that I can see.
However, there will need to be further work on the UX to make things
flow more smoothly.

I've been using this series with index.try_decrypt set to 'true', which
causes encrypted messages to be indexed on new.  I do this because I
don't want to be bothered to manually initiate indexing of encrypted
messages.  However, since my mail retrieval and indexing happen in the
background, this has the unfortunate side effect that I am occasionally
presented with a gpg agent prompt at random unexpected times.  Ideally,
one would leave index.try_decrypt set to 'auto', and there would be an
easy/automatic way to prompt reindexing when the user is interacting
directly with their MUA.  I haven't decided what's the best way to do
that yet, but something like the following happening automatically at
inbox view might do the trick:

  notmuch reindex --try-decrypt=true (tag:inbox AND tag:encrypted)

Finally, I think it would be worthwhile to resolve the disparity between
the usage of "decrypt" and "try-decrypt" in the CLI and config options.
I'm not sure why we're using different terms in different contexts, even
though the meanings are essentially the same.  A follow-up patch series
changing "try-decrypt" -> "decrypt" would probably be in order.

But these are next steps.  The series in question here is absolutely
ready, and needed, as is.

jamie.

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 07/18] crypto: new decryption policy "auto"

In reply to this post by Jameson Graef Rollins
On Sat 2017-11-11 15:14:03 -0800, Jameson Graef Rollins wrote:

> On Wed, Oct 25 2017, Daniel Kahn Gillmor <[hidden email]> wrote:
>> diff --git a/util/crypto.h b/util/crypto.h
>> index b23ca747..dc95b4ca 100644
>> --- a/util/crypto.h
>> +++ b/util/crypto.h
>> @@ -16,7 +16,8 @@ typedef struct _notmuch_crypto {
>>  } _notmuch_crypto_t;
>>  
>>  GMimeObject *
>> -_notmuch_crypto_decrypt (notmuch_message_t *message,
>> +_notmuch_crypto_decrypt (notmuch_decryption_policy_t decrypt,
>> + notmuch_message_t *message,
>>   GMimeCryptoContext* crypto_ctx,
>>   GMimeMultipartEncrypted *part,
>>   GMimeDecryptResult **decrypt_result,
>
> Why does _notmuch_crypt_decrypt need to have
> "notmuch_decryption_policy_t decrypt" as an input argument?  Isn't
> notmuch_decryption_policy_t already an attribute of the crypto_ctx?  Is
> there some situation where the policy would differ from what's specified
> in the crypto_ctx?
crypto_ctx here is just a GMimeCryptoContext, which doesn't know
anything about notmuch_decryption_policy_t.  Maybe i'm misunderstanding
your question?

I'd be happy to streamline the interface to this internal function, but
given that it's not an exported API, i'm not as concerned about things
like future cleanliness -- the notmuch source contains all invocations
of the function anywhere, so if we find a nicer way to streamline it in
the future, we can do that cleanup across the codebase in a single
commit.

Thanks for the review!

        --dkg

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: Stashed session keys

In reply to this post by Jameson Graef Rollins
On Sat 2017-11-11 15:31:36 -0800, Jameson Graef Rollins wrote:
> I have reviewed and tested this series, and it seems solidly
> implemented and very well motivated.  I have been using it regularly
> for a couple weeks now and have found no issues with it's usage, and
> have noticed the considerable speed up when viewing encrypted threads
> (as much as x8 for show on a thread of just 8 encrypted messages).  I
> fully support it's integration unconditionally.

thanks for the review, the testing, and the reportback.  I'm glad to
hear that it's giving you the same sort of speedups that it gives me.

> Daniel likes to think of this in terms of being able to "delete"
> encrypted messages in the wild (via deletion of the original
> encryption key) whereas I like to think of it in terms of preserving
> access to received encrypted messages after key rotation.  Both
> benefits hold, though, obviously.

yes, these are different ways of looking at the same key management
strategy.

> I think these policies cover all potential use cases that I can see.
> However, there will need to be further work on the UX to make things
> flow more smoothly.

Agreed.  The goal of this series is to provide the framework that can be
used to build smoother UX, but it doesn't get all the way to providing
the smoothest possible UX.  Such is the nature of toolkit development.

> I haven't decided what's the best way to do that yet, but something
> like the following happening automatically at inbox view might do the
> trick:
>
>   notmuch reindex --try-decrypt=true (tag:inbox AND tag:encrypted)

This seems like a reasonable way to ensure that your long-term, personal
secret keys only get accessed when you are interactively working with
your mail user agent.

You might be able to target the reindex even more narrowly by adding
something like "AND not property:index-decryption=success"

> Finally, I think it would be worthwhile to resolve the disparity between
> the usage of "decrypt" and "try-decrypt" in the CLI and config options.
> I'm not sure why we're using different terms in different contexts, even
> though the meanings are essentially the same.  A follow-up patch series
> changing "try-decrypt" -> "decrypt" would probably be in order.

If this series lands, i'd be happy to supply such an term-normalizing
series for subsequent consideration.

If people feel that this term normalization is the main blocker to
landing this series, i could try to rebase it with a different UI terms,
but rebasing the series for this change feels like busy-work to me (and
would be more effort than a simple normalization patch on top).  i'd
rather spend my limited notmuch hacking+reviewing time providing useful
features.

      --dkg

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Jameson Graef Rollins Jameson Graef Rollins
Reply | Threaded
Open this post in threaded view
|

Re: Stashed session keys

On Sun, Nov 12 2017, Daniel Kahn Gillmor <[hidden email]> wrote:

>> Finally, I think it would be worthwhile to resolve the disparity between
>> the usage of "decrypt" and "try-decrypt" in the CLI and config options.
>> I'm not sure why we're using different terms in different contexts, even
>> though the meanings are essentially the same.  A follow-up patch series
>> changing "try-decrypt" -> "decrypt" would probably be in order.
>
> If this series lands, i'd be happy to supply such an term-normalizing
> series for subsequent consideration.
>
> If people feel that this term normalization is the main blocker to
> landing this series, i could try to rebase it with a different UI terms,
> but rebasing the series for this change feels like busy-work to me (and
> would be more effort than a simple normalization patch on top).  i'd
> rather spend my limited notmuch hacking+reviewing time providing useful
> features.
I don't think it's a blocker at all, but I do think the term
normalization should happen before the next release, since I think we
should change "try-decrypt" -> "decrypt", which would affect things
already in master.  But I very much hope this series will land before
the next release, in which case I think it's fine to wait to do the
normalization after this series lands.

jamie.

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Jameson Graef Rollins Jameson Graef Rollins
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 07/18] crypto: new decryption policy "auto"

In reply to this post by Daniel Kahn Gillmor
On Sun, Nov 12 2017, Daniel Kahn Gillmor <[hidden email]> wrote:

> On Sat 2017-11-11 15:14:03 -0800, Jameson Graef Rollins wrote:
>> On Wed, Oct 25 2017, Daniel Kahn Gillmor <[hidden email]> wrote:
>>> diff --git a/util/crypto.h b/util/crypto.h
>>> index b23ca747..dc95b4ca 100644
>>> --- a/util/crypto.h
>>> +++ b/util/crypto.h
>>> @@ -16,7 +16,8 @@ typedef struct _notmuch_crypto {
>>>  } _notmuch_crypto_t;
>>>  
>>>  GMimeObject *
>>> -_notmuch_crypto_decrypt (notmuch_message_t *message,
>>> +_notmuch_crypto_decrypt (notmuch_decryption_policy_t decrypt,
>>> + notmuch_message_t *message,
>>>   GMimeCryptoContext* crypto_ctx,
>>>   GMimeMultipartEncrypted *part,
>>>   GMimeDecryptResult **decrypt_result,
>>
>> Why does _notmuch_crypt_decrypt need to have
>> "notmuch_decryption_policy_t decrypt" as an input argument?  Isn't
>> notmuch_decryption_policy_t already an attribute of the crypto_ctx?  Is
>> there some situation where the policy would differ from what's specified
>> in the crypto_ctx?
>
> crypto_ctx here is just a GMimeCryptoContext, which doesn't know
> anything about notmuch_decryption_policy_t.  Maybe i'm misunderstanding
> your question?
>
> I'd be happy to streamline the interface to this internal function, but
> given that it's not an exported API, i'm not as concerned about things
> like future cleanliness -- the notmuch source contains all invocations
> of the function anywhere, so if we find a nicer way to streamline it in
> the future, we can do that cleanup across the codebase in a single
> commit.
I guess I'm confusing how things were before, when the crypto_ctx was a
notmuch-defined thing that included the GMimeCryptoContext.

It seems like _notmuch_crypto_t could just hold the GMimeCryptoContext,
as it does for earlier versions of GMime, which would make things easier
to pass around.  But this discussion is tangent to this patch series.

jamie.

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (847 bytes) Download Attachment
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: Stashed session keys

In reply to this post by Daniel Kahn Gillmor
On Sun 2017-11-12 11:51:11 +0800, Daniel Kahn Gillmor wrote:

> On Sat 2017-11-11 15:31:36 -0800, Jameson Graef Rollins wrote:
>> I haven't decided what's the best way to do that yet, but something
>> like the following happening automatically at inbox view might do the
>> trick:
>>
>>   notmuch reindex --try-decrypt=true (tag:inbox AND tag:encrypted)
>
> This seems like a reasonable way to ensure that your long-term, personal
> secret keys only get accessed when you are interactively working with
> your mail user agent.
>
> You might be able to target the reindex even more narrowly by adding
> something like "AND not property:index-decryption=success"

Sorry, this should be "AND not property:index.decryption=success"

       --dkg
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Daniel Kahn Gillmor
Daniel Kahn Gillmor <[hidden email]> 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.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/18] crypto: Test restore of cleartext index from stashed session keys

In reply to this post by Daniel Kahn Gillmor
Daniel Kahn Gillmor <[hidden email]> writes:

> If you've got a notmuch dump that includes stashed session keys for
> every decrypted message, and you've got your message archive, you
> should be able to get back to the same index that you had before.
>

Out of curiousity, have you given any thought to what happens when
someone sends a message with the same message-id but a different
session-key? it seems like the user can potentially lose access to the
encrypted message.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 07/18] crypto: new decryption policy "auto"

In reply to this post by Daniel Kahn Gillmor
Daniel Kahn Gillmor <[hidden email]> writes:

>  GMimeObject *
> -_notmuch_crypto_decrypt (notmuch_message_t *message,
> +_notmuch_crypto_decrypt (notmuch_decryption_policy_t decrypt,
> + notmuch_message_t *message,
>   g_mime_3_unused(GMimeCryptoContext* crypto_ctx),
>   GMimeMultipartEncrypted *part,
>   GMimeDecryptResult **decrypt_result,
>   GError **err)
>  {
>      GMimeObject *ret = NULL;
> +    if (decrypt == NOTMUCH_DECRYPT_FALSE)
> + return NULL;

I'm going to assume that all is well and no return value from
_notmuch_crypto_decrypt is used without guarding for NULL.

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by David Bremner-2
Hi Bremner--

Thanks for the review!

On Tue 2017-11-14 09:02:08 -0400, David Bremner wrote:

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

well, i'm happy to hear that -- i've got no problem with deprecating
GMime 2.6, and would be fine with maintaining GMime 3.0 in
stretch-backports if that would make you feel more comfortable about the
decision.

Still, I'll be kind of bummed to have to rewrite this series to strip
out the 2.6 support: i originally wrote it only with 3.0 support, and
then went back in and added 2.6 support because at the time, you didn't
want to deprecate 2.6 :( our coding cadence isn't very well synced :/

> 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.

I don't mind changing the documentation to use ``session-key`` instead
of ``session-key=``.  *shrug*

> 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.

sure, i can add to the commit message that _notmuch_crypto_decrypt is
growing a new parameter.

> 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".

I believe the answer is "not very" -- but if there are serious bugs (i
don't think we've talked about any of this stuff as bugs in gmime) then
we should probably try to raise them with him.

> 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.

Is there any naming convention for these features?  do you want me to
add a "session-key" label with a future revision of this branch?  or are
you asking for something else?

    --dkg
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Daniel Kahn Gillmor Daniel Kahn Gillmor
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/18] crypto: Test restore of cleartext index from stashed session keys

In reply to this post by David Bremner-2
On Tue 2017-11-14 09:13:52 -0400, David Bremner wrote:

> Daniel Kahn Gillmor <[hidden email]> writes:
>
>> If you've got a notmuch dump that includes stashed session keys for
>> every decrypted message, and you've got your message archive, you
>> should be able to get back to the same index that you had before.
>
> Out of curiousity, have you given any thought to what happens when
> someone sends a message with the same message-id but a different
> session-key? it seems like the user can potentially lose access to the
> encrypted message.

yep!  I even have that case in my own mailbox due to messages i've sent
to schleuder encrypted mailing lists to which i'm also subscribed.

It works fine.  notmuch stashes both session keys against the message-id
(you can have multiple properties with the same name as long as they
have different values).  And upon decryption, it tries each session-key
in succession.  This is a little bit sloppy (maybe it would be less
sloppy to associate each message key with each version of the message
somehow?), but it's significantly simpler and basically unnoticeable
compared to the speedup gains provided by the rest of the series.

         --dkg
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 05/18] crypto: Test restore of cleartext index from stashed session keys

Daniel Kahn Gillmor <[hidden email]> writes:

> On Tue 2017-11-14 09:13:52 -0400, David Bremner wrote:
>> Daniel Kahn Gillmor <[hidden email]> writes:
>>
>>> If you've got a notmuch dump that includes stashed session keys for
>>> every decrypted message, and you've got your message archive, you
>>> should be able to get back to the same index that you had before.
>>
>> Out of curiousity, have you given any thought to what happens when
>> someone sends a message with the same message-id but a different
>> session-key? it seems like the user can potentially lose access to the
>> encrypted message.
>
> yep!  I even have that case in my own mailbox due to messages i've sent
> to schleuder encrypted mailing lists to which i'm also subscribed.
>
> It works fine.  notmuch stashes both session keys against the message-id
> (you can have multiple properties with the same name as long as they
> have different values).  And upon decryption, it tries each session-key
> in succession.  This is a little bit sloppy (maybe it would be less
> sloppy to associate each message key with each version of the message
> somehow?), but it's significantly simpler and basically unnoticeable
> compared to the speedup gains provided by the rest of the series.
>
>          --dkg

Great!

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Daniel Kahn Gillmor
Daniel Kahn Gillmor <[hidden email]> writes:

> 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".
>
> I believe the answer is "not very" -- but if there are serious bugs (i
> don't think we've talked about any of this stuff as bugs in gmime) then
> we should probably try to raise them with him.

I think Jani tried a bit to narrow it down, but didn't succeed. Part of the
problem (which I suspect is endemic to crypto issues) is that we don't
have public test cases.

>
>> 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.
>
> Is there any naming convention for these features?  do you want me to
> add a "session-key" label with a future revision of this branch?  or are
> you asking for something else?

It could be a followup, but yeah, if there is some feature that is
sometimes compiled in, and sometimes not, then it should be listed along
with the others. For whatever reason, the existing convention is
'session_key'

This discussion does make me think there should probably be a test in
configure that sets a corresponding feature macro
HAVE_GMIME_SESSION_KEYS, in a manner similar to HAVE_XAPIAN_COMPACT
(possibly just centralizing the version comparison currently used).

d
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 11/18] cli/new, insert, reindex: update documentation for --try-decrypt=auto

In reply to this post by Daniel Kahn Gillmor
>  
> -    ``--try-decrypt=(true|false)``
> +    ``--try-decrypt=(true|auto|false)``

I sympathize with Jamie's point about consistency here. I'm
not sure if this was already mentioned, but the inconsistency between
boolean and keyword arguments is a bit confusing also.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Ruben Pollan Ruben Pollan
Reply | Threaded
Open this post in threaded view
|

Re: Stashed session keys

In reply to this post by Daniel Kahn Gillmor
Quoting Daniel Kahn Gillmor (2017-10-25 08:51:45)
> Now that cleartext indexing is merged, let's add the ability to stash
> session keys!

Nice feature. I'm using it and it works fine. I notice some speed up, improving
the painfulness of reading long encrypted threads in alot. And I like to don't
be able to have around my old private keys.

I implemented some support for it in alot (using the patch I just sent adding
notmuch_message_get_property to the python binding):
https://github.com/meskio/alot/tree/session-key

Thanks for the work.

--
meskio | http://meskio.net/
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
 My contact info: http://meskio.net/crypto.txt
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Nos vamos a Croatan.

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (849 bytes) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 13/18] cli/new, insert, reindex: change index.try_decrypt to "auto" by default

In reply to this post by Daniel Kahn Gillmor
Daniel Kahn Gillmor <[hidden email]> writes:

> The new "auto" decryption policy is not only good for "notmuch show"
> and "notmuch reindex".  It's also useful for indexing messages --
> there's no good reason to not try to go ahead and index the cleartext
> of a message that we have a stashed session key for.

I'm confused here. You talk about indexing other than reindex, but the
only tests that change are reindex? Is this meant to change "notmuch
new" behaviour?

d

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/18] crypto: actually stash session keys when try-decrypt=true

In reply to this post by Daniel Kahn Gillmor
Daniel Kahn Gillmor <[hidden email]> writes:

> +        Be aware that the index is likely sufficient to reconstruct
> +        the cleartext of the message itself, so please ensure that the
> +        notmuch message index is adequately protected.  DO NOT USE
> +        ``--try-decrypt=true`` without considering the security of
> +        your index.
>  

This is probably just my ignorance, but doesn't stashing session keys
change this from likely to certain? Is it possible we decrypt thing and
don't get session keys.

> +test_begin_subtest "show the message body of the encrypted message"
> +notmuch dump wumpus
> +output=$(notmuch show wumpus | awk '/^\014part}/{ f=0 }; { if (f) { print $0 } } /^\014part{ ID: 3/{ f=1 }')
> +expected='This is a test encrypted message with a wumpus.'
> +test_expect_equal \
> +    "$output" \
> +    "$expected"

I'd be happier if we didn't further entrench the text format in the test
suite. How hard would it be to use json output (+maybe python?) here?

>   *attempted = true;
>  #if (GMIME_MAJOR_VERSION < 3)
> +#if (GMIME_MAJOR_VERSION == 2 && GMIME_MINOR_VERSION == 6 && GMIME_MICRO_VERSION >= 21)
> +    gboolean oldgetsk = g_mime_crypto_context_get_retrieve_session_key (crypto_ctx);
> +    gboolean newgetsk = (decrypt_result);
> +    if (newgetsk != oldgetsk)
> + /* This could return an error, but we can't do anything about it, so ignore it */
> + g_mime_crypto_context_set_retrieve_session_key (crypto_ctx, newgetsk, NULL);
> +#endif
>      ret = g_mime_multipart_encrypted_decrypt(part, crypto_ctx,
>       decrypt_result, err);
> +#if (GMIME_MAJOR_VERSION == 2 && GMIME_MINOR_VERSION == 6 && GMIME_MICRO_VERSION >= 21)
> +    if (newgetsk != oldgetsk)
> + g_mime_crypto_context_set_retrieve_session_key (crypto_ctx, oldgetsk, NULL);

I lost track a bit, but now there's at least 2 (maybe 3) repetitions of
this somewhat complicated test, and one more needed for
built_with.session_keys. HAVE_GMIME_SESSION_KEYS is looking better and
better.

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
123