Re: Patch attestation RFC + proof of concept

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Wed, Feb 26, 2020 at 09:50:50AM -0800, Kees Cook wrote:
> > As I envision it, the submitter workflow would look as follows:
> > 
> > - the developer runs "git-format-patch"
> > - the developer reviews the changes and makes any last-minute edits they 
> >   deem necessary before submitting their work to the list/maintainer
> > - the developer executes "git-send-email"
> > - the developer runs "attest-patches -a *.patches"
> > - the developer sends attestation.eml to signatures@xxxxxxxxxx
> >   (or the tool auto-POSTs it to the submission URL, as mentioned)
> > 
> > There can even be a fairly simple wrapper around git-send-email that 
> > would perform attestation as part of the "sending patches" stage.
> FWIW, I would find this utterly trivial to add to my workflow. (The only
> minor difference that doesn't really matter is that in addition to
> series-style patches, I also regularly send one-offs, but that would
> just be a short attestation email.)

Yes, and you can even send a single attestation email for a bunch of 
patches that have nothing to do with each-other (i.e. they don't need to 
all be part of the same series). E.g. you could have an automated script 
that hooks into your "Sent" folder and sends regular attestations for 
any new patches it finds.

> > ## The reviewer workflow
> > 
> > The reviewer does not need to concern themselves with attestation until 
> > they are ready to apply the patches to their git tree. When that moment 
> > comes:
> > 
> > - the maintainer runs get-lore-mbox -aA (-A is not implemented yet)
> > - get-lore-mbox performs attestation before generating the am-ready mbox
> > - if attestation passes, get-lore-mbox adds two trailers to each patch: 
> >   "Attestation-by:" and "Attestation-verified:". In our example case 
> >   those are:
> >   Attestation-by: Kees Cook <kees@xxxxxxxxxx> (pgp:8972F4DFDC6DC026)
> >   Attestation-verified: Konstantin Ryabitsev <konstantin@xxxxxxxxxxxxxxxxxxx>
> We'll probably need to have a stable way to deal with key aliases. Even
> in this example my email addresses are keescook@xxxxxxxxxxxx (patches
> and attestation sender), kees@xxxxxxxxxxx (comment in gpg sig), and
> kees@xxxxxxxxxx (since that's what Konstantin used to fetch my key).

The TL;DR section uses the Web Key Directory to look up key information, 
which will strip any UIDs not matching I totally disagree 
with this WKD behaviour, but I can't change it. This is why we pass the 
-F flag to attest-patches, which forces it to ignore From mismatches.

When a key is imported via something like's pgpkeys.git 
repository, it will have all of your UIDs and the "Attestation-by" line 
will use the one that matches the email address in the From: field. This 
is the default behaviour.

> And perhaps I lack imagination, but what is the overall purpose of these
> proposed tags? If it's just a hint to whether the patch has attestation,
> the '-verified:' isn't needed. Anyone wanting to check attestation would
> always need to do the full heavy lifting anyway.

It's true, the -verified tag is redundant with Signed-off-by. I'm happy 
to drop it. On the other hand, I think the "Attestation-by" trailer is 
helpful because the Author of the patch may be different from the person 
sending the patches to the maintainer. E.g. in this case, where the 
patch author is different from the sender:

So, a presence of "Attestation-by: Andrew Morton" would help build 
attestation chain. If there is no "Attestation-by: Gang He", then we'll 
know that there was no cryptographic attestation for this patch en route 
to akpm.

> > - all attestations are recorded in the public-inbox "signatures" 
> > feed that can be mirrored along all other public-inbox repositories 
> > on
> Moar blockchains! ;) But, yes, it provides a signed path from author to
> committer without interfering with existing workflows. I like it!

I tried not to use the "B" word, but sure. :) There is no 
proof-of-whatnot + global reconciliation in the case of git 
repositories, so it's not technically a blockchain in its strictest 
sense, even if we PGP-sign all commits. </pedant mode>

> > Downsides:
> > 
> > - we aren't solving the problem of delegated trust, which will continue 
> >   to be the hardest part behind any distributed development effort
> This was immediately my first question. How does a committer choose the
> correct GPG key? /me waits for the key server to appear...

The attest-patches script offers the -t flag, which forces the "TOFU" 
trust model, meaning that the fist time you come across a key that you 
don't know, it's considered good and trusted. If in the future you come 
across another key with the same email address on the UID, GnuPG will 
mark both keys as untrusted and force you to pick which one you prefer.

There is also some ongoing effort that tries to implement the 
"distributed ID" spec for git [1], which aims to standardize on a way to 
pass developer key information via the repository itself (or via a 
linked repository). It's an ambitious goal, but if it's successful, then 
it will offer a way to record key information for most prominent 
developers inside git itself (similar to pgpkeys.git).


In general terms, though, I see this mostly being relevant in ongoing 
long-term developer relationships. The goal is to prevent malicious 
tampering of patches that appear to be coming from a fellow developer 
with whom you have a long-term collaboration history and whose judgment 
you trust.

If you're receiving a patch submission from someone with whom you have 
no history, then you have no reason to trust that their code is not 
trying to do something malicious -- in which case checking their PGP 
signature will not be meaningful anyway.

> Thanks for working on this! It was fun being the alpha guinea pig. ;)

I totally forgot to thank you for kindly agreeing to guinea-pig this for 
me. :) So, thanks again!


[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux