On Wed 2019-05-22 09:18:53 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> 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