[Intel-gfx] [maintainer-tools PATCH] dim: Sign commits in addition to tags

[Sean Paul]

On Wed, Nov 1, 2017 at 1:00 PM, Eric Anholt <eric at anholt.net> wrote:
> Sean Paul <seanpaul at chromium.org> writes:
>
>> On Wed, Nov 1, 2017 at 7:12 AM, Gustavo Padovan <gustavo at padovan.org> wrote:
>>> 2017-10-31 Sean Paul <seanpaul at chromium.org>:
>>>
>>>> On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>>>> > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul at chromium.org> wrote:
>>>> >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula
>>>> >> <jani.nikula at linux.intel.com> wrote:
>>>> >>>
>>>> >>> Reminder, we have this new list dim-tools at lists.freedesktop.org for
>>>> >>> maintainer tools patches. Cc'd.
>>>> >>>
>>>> >>
>>>> >> Ahh, cool. I didn't realize dim grew up!
>>>> >>
>>>> >>> On Mon, 30 Oct 2017, Sean Paul <seanpaul at chromium.org> wrote:
>>>> >>>> Expanding on Jani's work to sign tags, this patch adds signing for git
>>>> >>>> commit/am.
>>>> >>>
>>>> >>> I guess I'd like more rationale here. Is this something we should be
>>>> >>> doing? Is anyone else doing this?
>>>> >>>
>>>> >>
>>>> >> Sure thing. Signing commits allows Dave to use --verify-signatures
>>>> >> when pulling. If something is not signed, we'll know it was either not
>>>> >> applied with dim, or was altered on fdo (both warrant investigation).
>>>> >>
>>>> >> I suspect no one else is doing this since most trees are single
>>>> >> maintainer, and it's not possible to sign commits via git send-email.
>>>> >> Since we have the committer model, and a bunch of people with access
>>>> >> to fdo and the tree, I think it's important to add this. Especially
>>>> >> since we can do it in dim without overhead.
>>>> >>
>>>> >>>> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>>>> >>>> ---
>>>> >>>>
>>>> >>>> This has been lightly tested with dim apply-branch/dim push-branch.
>>>> >>>>
>>>> >>>> Sean
>>>> >>>>
>>>> >>>>  dim | 78 +++++++++++++++++++++++++++++++++++++++++++++------------------------
>>>> >>>>  1 file changed, 51 insertions(+), 27 deletions(-)
>>>> >>>>
>>>> >>>> diff --git a/dim b/dim
>>>> >>>> index 527989aff9ad..cd5e41f89a3a 100755
>>>> >>>> --- a/dim
>>>> >>>> +++ b/dim
>>>> >>>> @@ -67,9 +67,6 @@ DIM_TEMPLATE_SIGNATURE=${DIM_TEMPLATE_SIGNATURE:-$HOME/.dim.template.signature}
>>>> >>>>  # dim pull-request tag summary template
>>>> >>>>  DIM_TEMPLATE_TAG_SUMMARY=${DIM_TEMPLATE_TAG_SUMMARY:-$HOME/.dim.template.tagsummary}
>>>> >>>>
>>>> >>>> -# GPG key id for signing tags. If unset, don't sign.
>>>> >>>> -DIM_GPG_KEYID=${DIM_GPG_KEYID:+-u $DIM_GPG_KEYID}
>>>> >>>> -
>>>> >>>>  #
>>>> >>>>  # Internal configuration.
>>>> >>>>  #
>>>> >>>> @@ -104,6 +101,20 @@ test_request_recipients=(
>>>> >>>>  # integration configuration
>>>> >>>>  integration_config=nightly.conf
>>>> >>>>
>>>> >>>> +# GPG key id for signing tags. If unset, don't sign.
>>>> >>>> +function gpg_keyid_for_tag
>>>> >>>> +{
>>>> >>>> +     echo "${DIM_GPG_KEYID:+-u $DIM_GPG_KEYID}"
>>>> >>>> +     return 0
>>>> >>>> +}
>>>> >>>> +
>>>> >>>> +# GPG key id for committing (git commit/am). If unset, don't sign.
>>>> >>>> +function gpg_keyid_for_commit
>>>> >>>> +{
>>>> >>>> +     echo "${DIM_GPG_KEYID:+-S$DIM_GPG_KEYID}"
>>>> >>>> +     return 0
>>>> >>>> +}
>>>> >>>
>>>> >>> This seems like an overly complicated way to achieve what you want.
>>>> >>>
>>>> >>> Just put these under "Internal configuration." instead:
>>>> >>>
>>>> >>> dim_gpg_sign_tag=${DIM_GPG_KEYID:+-u $DIM_GPG_KEYID}
>>>> >>> dim_gpg_sign_commit=${DIM_GPG_KEYID:+-S$DIM_GPG_KEYID}
>>>> >>>
>>>> >>> And use directly in git tag and commit, respectively?
>>>> >>>
>>>> >>
>>>> >> Yep, sounds good.
>>>> >>
>>>> >>> Although... perhaps starting to sign tags should not force signing
>>>> >>> commits?
>>>> >>>
>>>> >>
>>>> >> Why would it be desirable to *not* sign tags?
>>>> >
>>>> > Again, what's the threat model you're trying to defend against? Atm
>>>> > anyone with commit rights to fd.o can push whatever they want to. If
>>>> > they want to be evil, they can also push whatever kind of garbage they
>>>> > want to, including commit signature and and fake Link: and review
>>>> > tags. With pull requests/tags signing them prevents a
>>>> > man-in-the-midddle attack of the unprotected pull request in the mail,
>>>> > but I still don't see what signing commits protects against.
>>>>
>>>> This is protecting against a bad actor (either through a committer's
>>>> account, or some other fdo account) gaining access to the tree on fdo
>>>> and either adding a malicious commit, or altering an existing commit.
>>>
>>> Yeah, but then we need to enforce it for all committer
>>
>> My hope is that dim makes it easy enough to get everyone on board
>> eventually. In the interim, the people with signing commits will be
>> able to attest that those commits were applied by them.
>>
>>> and we also need
>>> a signing party to sign each others keys.
>>
>> I feel like most of us see each other often enough to make this
>> possible. Even without a signing party, we still get *some* amount of
>> coverage by virtue of TOFU [1].
>>
>> Is anyone against the idea of signing commits? Is there a reason that
>> we shouldn't?
>
> We've used GPG a bunch in fdo infrastructure, and my experience is that
> it gets you basically no assurance, in exchange for a bunch of admin
> overhead (since people lose keys and need to be able to say "Yes, this
> is really me, here with a new key").
>
> I've been signing email for years, but I'm not a fan of tying
> participation in open source development to using GPG.  It's just not
> useful enough for its costs, particularly discouraging new developers.

To be fair, this change only signs commits applied by committers who
have already added their keyid to their dimrc (for signing tags). If
there is no keyid, things work as normal.

That said, it seems like the overall sentiment on signing commits is
negative, so I'll move on from the idea.

Thanks for the feedback, all!

Sean