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

[Gustavo Padovan]

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 and we also need
a signing party to sign each others keys.

Gustavo