Closed dkg closed 7 years ago
Just did a quick review and it seemed ok, just trying to decide what I think about adding it to GMime itself.
Code-wise, the PR looks good (although I like to declare all my variables at the top of functions instead of inline in my C code). And yay unit tests :)
I'll try to think about this more tomorrow.
I normally prefer more widely accepted standards so I don't have to maintain a feature that goes extinct, but there's always the brownie points for being one of the "first" :)
That said, I'm a bit concerned about the whole Alice-sends-you-a-message-and-defines-Bob's-public-key thing... that seems really sketchy for 2 reasons:
I suppose DKIM might solve the modification problem assuming the Autocrypt: header is signed.
Thanks for the review, @jstedfast !
fwiw, Autocrypt is being built into a handful of active implementations right now (K-9, enigmail, deltachat, mailpile, bitmask), and having this available in GMime would make it a much easier lift for a handful of others that already depend on GMime.
As for me landing this feature into notmuch, i could just add this work to the notmuch codebase. But i would prefer to make it more widely available (i want to encourage balsa and geary to pick this up too), and this part of the Autocrypt spec (parsing e-mail headers) is a more natural fit for GMime anyway.
If it would help convince you to label this feature as a "technology preview" or something in its initial introduction in GMime, i don't mind such a label. I've tried to keep the main interface exposed (g_mime_message_get_autocrypt_headers()
) relatively simple and minimal so we can expand it as necessary when we see how it gets used.
As for your more substantive concerns about Alice-sends-you-a-message-and-defines-Bob's-public-key thing, i think you're talking about Autocrypt-Gossip
headers. I agree that the scenario has a lot of sketchy corner cases, but (a) i think that the autocrypt spec does a good job of ensuring that "gossiped" keys like that are never preferred over direct-knowledge keys (which is why it's an explicitly distinct set of keys in the proposed API in this PR), and (b) there's no requirement for an autocrypt client to implement gossip if they're sketched out by it (though it really does make encrypted group conversations more likely to succeed without annoying users).
If you'd prefer to take a subset of the patches, leaving out the gossip work for now, that'd also be fine with me (though given the underlying structure i worked toward, making gossip functionality available isn't a particularly big reach).
what if said mail gets intercepted and modified in transit?
This is a real potential problem, but it's explicitly out of scope for Autocrypt level 1. See the first bullet point of the features of Autocrypt. We will address active attackers in the long run, but we can gain widespread protection against passive attackers in the short term if we get this working, which offers a better shot at protection against (or at least detection of) active attackers than the last 20 years of trying to defend against active attackers first and not even protecting against passive monitors :/
I'll work on a revised branch that addresses your concerns about inline variable declaration. Do you really want all variables declared at the top of a function, or is moving them to the top of necessary scope (enclosing curly braces) acceptable?
I don't see any reason to accept the patch for Autocrypt: parsing but not Autocrypt-Gossip: to be honest.
Thanks for the explanation of Gossip. I missed that it was treated differently last night.
Thinking about this more, I guess including public keys in headers isn't actually any worse than assuming that a key fingerprint that you find posted on a website is accurate (even if said website is an OpenPGP key server). Ultimately you will need to verify that fingerprint with the person you think it belongs to in whatever way you deem suitably trustworthy for your purposes (whether that be as stringent as meeting said person in meatspace and verifying said person's photo ID card or just IM'ing them and asking for verification).
yep, agreed that actual key verification is out of scope for Autocrypt (well, at least for level 1) -- it's just a key distribution mechanism, associated with some guidelines for how to make sensible best-effort guesses at what keys to use if you haven't done any additional verification.
Did you want me to resubmit the series with the shifted variable declarations? do you care about top-of-function vs. top-of-scope?
I'll just merge.
Autocrypt is "e-mail encryption for humans".
Some of my best friends are humans! And it would be nice to make it easier for the MUAs that those humans use to adopt the standards developed under the Autocrypt umbrella.
This series makes it possible to easily extract the relevant
Autocrypt:
andAutocrypt-Gossip:
in-band key discovery headers from aGMimeMessage
. My aim is to use this to store and record Autocrypt peer state within Notmuch, but i'd just as soon have this functionality available to other GMime-using MUAs.This series on its own isn't sufficient for a fully-capable Autocrypt "level 1" MUA, (it doesn't actually deal with storing or updating peer state; it doesn't deal with generating or maintaining secret keys) but i think it presents is a reasonably solid foundation that an MUA can use to build out this capability.
If i find that there are other Autocrypt-friendly features that fit under the GMime umbrella, i'll probably propose them in a separate pull request.