Re: [PATCH 3/3] emacs: Drop content-free "Unknown signature status" button

2019-05-24 Thread David Bremner
Daniel Kahn Gillmor  writes:

> On Thu 2019-05-23 22:13:59 -0300, David Bremner wrote:
>> Daniel Kahn Gillmor  writes:
>>
>>> diff --git a/emacs/notmuch-crypto.el b/emacs/notmuch-crypto.el
>>> index 353f721e..68171153 100644
>>> --- a/emacs/notmuch-crypto.el
>>> +++ b/emacs/notmuch-crypto.el
>>> @@ -93,6 +93,7 @@ mode."
>>>  (defun notmuch-crypto-insert-sigstatus-button (sigstatus from)
>>>(let* ((status (plist-get sigstatus :status))
>>>  (help-msg nil)
>>> +(show-button t)
>>>  (label "Signature not processed")
>>
>> This should probably be nil, since that particular value is never used,
>> iiuc. I can amend it if you agree.
>
[snip]
>
> If i've misunderstood the e-lisp (entirely possible!) i would be happy
> to be corrected.

No, you've just misunderstood my reply. I refer to the existing initialization
of "label", which now seems obsolete to me.

d
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH v2 4/4] cli/show: emit new whole-message crypto status output

2019-05-24 Thread David Bremner
Daniel Kahn Gillmor  writes:

>
> or, should we just make the crypto member always present?
>

That was what I was thinking, sorry to be less explicit than I could
have been.

> Another alternative is to condition the presence of the crypto member on
> the arguments (like if --verify is set), but the decision for --decrypt
> is a bit awkward because of our default of --decrypt=auto.
>

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH v2 1/4] util/crypto: _notmuch_message_crypto: tracks message-wide crypto state

2019-05-24 Thread David Bremner
Daniel Kahn Gillmor  writes:

> On Wed 2019-05-22 09:18:53 -0300, David Bremner wrote:
>> Daniel Kahn Gillmor  writes:
>>
>>> +static int
>>> +_notmuch_message_crypto_cleanup (_notmuch_message_crypto_t *msg_crypto)
>>> +{
>>> +if (!msg_crypto)
>>> +   return 0;
>>> +if (msg_crypto->sig_list)
>>> +   g_object_unref (msg_crypto->sig_list);
>>> +return 0;
>>> +}
>>
>> we currently call destructors
>>
>>- *_destroy
>>- *_destructor (the most popular option)
>>- *_free
>>
>> Is there a good reason to introduce a fourth option?
>
> nope.  I'm happy to stick with _destructor if you prefer it.

Sounds good.

>>
>> It might be worth a comment here to explain the interaction between
>> talloc and glib, i.e. why this is needed.
>
> OK, it'll be something like:
>
> / * This signature list needs to persist as long as the _n_m_crypto
>   * object survives. Increasing its reference counter prevents
>   * garbage-collection until after _n_m_crypto_destructor is called. */

Yep, sounds good. I only suggest it because I had to learn this stuff
the hard way.

> do you think i should move this explanation into the .c file instead of
> the header?  I think it's more important that it be visible to someone
> saying "what are these statuses?"  I could copy the text into the .c
> file as well, but then i worry about keeping it in sync.  Maybe just a
> reference is sufficient?

A reference is fine.
>
> Yes, PGP/MIME encrypted messages and signed from enigmail well-formed in
> this sense.  However, some mail transport agents (including mailman!)
> mangle them in ways that may change the well-formedness.  (see
> https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling
> for more discussion on the topic of mangled messages; i plan to submit
> some patches to notmuch related to that work soon)
>
> My approach thus far around building the corpus of
> cryptographically-protected e-mail has been to keep the examples
> deliberately minimalist, so that they can be understood by someone
> taking them apart and inspecting by hand.
>
> If someone wants a trove of real-world messy e-mails i certainly
> wouldn't object to that (indeed, i'd be happy to help!), but i don't
> think it should be a blocker to land this series.

OK.

___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH v2 4/4] cli/show: emit new whole-message crypto status output

2019-05-24 Thread Daniel Kahn Gillmor
On Fri 2019-05-24 16:09:38 -0400, Daniel Kahn Gillmor wrote:
> On Thu 2019-05-23 07:50:43 -0300, David Bremner wrote:
>> Daniel Kahn Gillmor  writes:
>>
>>>  headers:headers,
>>> +crypto?:crypto,   # omitted if crypto disabled, or if no part 
>>> was signed or encrypted.
>>>  body?:  [part]# omitted if --body=false
>>>  }
>>
>> I'm wondering about the "upward compatible" aspect of this. If the
>> crypto key is ommitted, a client doesn't know whether to interpret that
>> as no part was signed or encrypted, or just an older version of notmuch.
>
> I understand your concern here.  Would making
> notmuch_built_with("message_crypto_summary") return true solve the
> problem?

or, should we just make the crypto member always present?

Another alternative is to condition the presence of the crypto member on
the arguments (like if --verify is set), but the decision for --decrypt
is a bit awkward because of our default of --decrypt=auto.

  --dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH v2 1/4] util/crypto: _notmuch_message_crypto: tracks message-wide crypto state

2019-05-24 Thread Daniel Kahn Gillmor
On Wed 2019-05-22 09:18:53 -0300, David Bremner wrote:
> Daniel Kahn Gillmor  writes:
>
>> +static int
>> +_notmuch_message_crypto_cleanup (_notmuch_message_crypto_t *msg_crypto)
>> +{
>> +if (!msg_crypto)
>> +return 0;
>> +if (msg_crypto->sig_list)
>> +g_object_unref (msg_crypto->sig_list);
>> +return 0;
>> +}
>
> we currently call destructors
>
>- *_destroy
>- *_destructor (the most popular option)
>- *_free
>
> Is there a good reason to introduce a fourth option?

nope.  I'm happy to stick with _destructor if you prefer it.

>> +notmuch_status_t
>> +_notmuch_message_crypto_potential_sig_list (_notmuch_message_crypto_t 
>> *msg_crypto, GMimeSignatureList *sigs)
>> +{
>> +if (!msg_crypto)
>> +return NOTMUCH_STATUS_NULL_POINTER;
>> +
>> +/* Signatures that arrive after a payload part during DFS are not
>> + * part of the cryptographic envelope: */
>> +if (msg_crypto->payload_encountered)
>> +return NOTMUCH_STATUS_SUCCESS;
>> +
>> +if (msg_crypto->sig_list)
>> +g_object_unref (msg_crypto->sig_list);
>> +
>> +msg_crypto->sig_list = sigs;
>> +if (sigs)
>> +g_object_ref (sigs);
>
> It might be worth a comment here to explain the interaction between
> talloc and glib, i.e. why this is needed.

OK, it'll be something like:

/ * This signature list needs to persist as long as the _n_m_crypto
  * object survives. Increasing its reference counter prevents
  * garbage-collection until after _n_m_crypto_destructor is called. */


>> +
>> +
>> +notmuch_status_t
>> +_notmuch_message_crypto_successful_decryption (_notmuch_message_crypto_t 
>> *msg_crypto)
>> +{
>> +if (!msg_crypto)
>> +return NOTMUCH_STATUS_NULL_POINTER;
>> +
>> +if (!msg_crypto->payload_encountered)
>> +msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_FULL;
>> +else if (msg_crypto->decryption_status == 
>> NOTMUCH_MESSAGE_DECRYPTED_NONE)
>> +msg_crypto->decryption_status = NOTMUCH_MESSAGE_DECRYPTED_PARTIAL;
>> +
>
> The logic / purpose of this is not very clear without referring to the header.
>
>> +/* The user probably wants to know if the entire message was in the
>> + * clear.  When replying, the MUA probably wants to know whether there
>> + * was any part decrypted in the message.  And when displaying to the
>> + * user, we probably only want to display "encrypted message" if the
>> + * entire message was covered by encryption. */

do you think i should move this explanation into the .c file instead of
the header?  I think it's more important that it be visible to someone
saying "what are these statuses?"  I could copy the text into the .c
file as well, but then i worry about keeping it in sync.  Maybe just a
reference is sufficient?

> I know you've discussed this elsewhere, but do you have some sense that
> most clients are generating encrypted messages that are "well formed" in
> the sense of the entire message being covered by encryption? It might be
> good to have some messages from enigmail etc... in the test suite when
> we merge this change.

Yes, PGP/MIME encrypted messages and signed from enigmail well-formed in
this sense.  However, some mail transport agents (including mailman!)
mangle them in ways that may change the well-formedness.  (see
https://tools.ietf.org/html/draft-dkg-openpgp-pgpmime-message-mangling
for more discussion on the topic of mangled messages; i plan to submit
some patches to notmuch related to that work soon)

My approach thus far around building the corpus of
cryptographically-protected e-mail has been to keep the examples
deliberately minimalist, so that they can be understood by someone
taking them apart and inspecting by hand.

If someone wants a trove of real-world messy e-mails i certainly
wouldn't object to that (indeed, i'd be happy to help!), but i don't
think it should be a blocker to land this series.

--dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: getting all attachments, getting images

2019-05-24 Thread meOme
Exactly.
 "content-disposition" : "inline" attachments are not tagged "attachment"...
So you cannot rely on the "attachment" tag. Especially if you prefer
text/plain as default...



--
Sent from: http://notmuch.198994.n3.nabble.com/
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH v2 4/4] cli/show: emit new whole-message crypto status output

2019-05-24 Thread Daniel Kahn Gillmor
On Thu 2019-05-23 07:50:43 -0300, David Bremner wrote:
> Daniel Kahn Gillmor  writes:
>
>>  headers:headers,
>> +crypto?:crypto,   # omitted if crypto disabled, or if no part 
>> was signed or encrypted.
>>  body?:  [part]# omitted if --body=false
>>  }
>
> I'm wondering about the "upward compatible" aspect of this. If the
> crypto key is ommitted, a client doesn't know whether to interpret that
> as no part was signed or encrypted, or just an older version of notmuch.

I understand your concern here.  Would making
notmuch_built_with("message_crypto_summary") return true solve the
problem?

--dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH] test: allow disabling timeout with NOTMUCH_TEST_TIMEOUT=0

2019-05-24 Thread Daniel Kahn Gillmor
On Wed 2019-05-22 08:52:55 -0300, David Bremner wrote:
> Daniel Kahn Gillmor  writes:
>
>> Tests appear to be hanging when run under GNU timeout on debian
>> stretch.  To aid in diagnosing this or similar problems, it's handy to
>> be able to disable timeout from the command line at will.
>>
>
> 1) Do we still need / want this?

Yes, i think it's useful if you want rule out coreutils when debugging
the test suite.

> 2) care to reword the commit message?

Sure, keeping the same subject line, i'd change the body to:



To aid in diagnosing test suite tooling that interacts poorly with
coreutils' timeout, it's handy to be able to bypass it entirely.





This isn't a high priority for me, but it's not a scary change either.

thanks for the review,

   --dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch


Re: [PATCH 3/3] emacs: Drop content-free "Unknown signature status" button

2019-05-24 Thread Daniel Kahn Gillmor
On Thu 2019-05-23 22:13:59 -0300, David Bremner wrote:
> Daniel Kahn Gillmor  writes:
>
>> diff --git a/emacs/notmuch-crypto.el b/emacs/notmuch-crypto.el
>> index 353f721e..68171153 100644
>> --- a/emacs/notmuch-crypto.el
>> +++ b/emacs/notmuch-crypto.el
>> @@ -93,6 +93,7 @@ mode."
>>  (defun notmuch-crypto-insert-sigstatus-button (sigstatus from)
>>(let* ((status (plist-get sigstatus :status))
>>   (help-msg nil)
>> + (show-button t)
>>   (label "Signature not processed")
>
> This should probably be nil, since that particular value is never used,
> iiuc. I can amend it if you agree.

I don't think i understand your suggestion.  that value *is* used, by
the "(when show-button …)" construct later in the code.

The idea was to make the button shown by default, unless it was disabled
further down in the patch (in the "t" clause of the main "cond"
construct in notmuch-crypto-insert-sigstatus-button, which only triggers
if we legitimately don't have any status information (if "status" is
nil)).

So no, i think it's supposed to start off "t" and then get disabled in
the specific condition this patch tests for.

If i've misunderstood the e-lisp (entirely possible!) i would be happy
to be corrected.

   --dkg


signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch