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

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

 



On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
>> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula
>> <jani.nikula@xxxxxxxxxxxxxxx> wrote:
>>>
>>> Reminder, we have this new list dim-tools@xxxxxxxxxxxxxxxxxxxxx for
>>> maintainer tools patches. Cc'd.
>>>
>>
>> Ahh, cool. I didn't realize dim grew up!
>>
>>> On Mon, 30 Oct 2017, Sean Paul <seanpaul@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> ---
>>>>
>>>> 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.

Sean

> -Daniel
>
>>
>> Sean
>>
>>
>>> BR,
>>> Jani.
>>>
>>>
>>>> +
>>>>  function read_integration_config
>>>>  {
>>>>       # clear everything first to allow configuration reload
>>>> @@ -473,12 +484,14 @@ EOF
>>>>  # append all arguments as tags at the end of the commit message of HEAD
>>>>  function dim_commit_add_tag
>>>>  {
>>>> +     local gpg_keyid
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>>       for arg; do
>>>>               # the first sed deletes all trailing blank lines at the end
>>>>               git log -1 --pretty=%B | \
>>>>                       sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' | \
>>>>                       sed "\$a${arg}" | \
>>>> -                     git commit --amend -F-
>>>> +                     git commit $gpg_keyid --amend -F-
>>>>       done
>>>>  }
>>>>
>>>> @@ -604,7 +617,7 @@ function update_rerere_cache
>>>>
>>>>  function commit_rerere_cache
>>>>  {
>>>> -     local remote file commit_message
>>>> +     local remote file commit_message gpg_keyid
>>>>
>>>>       echo -n "Updating rerere cache... "
>>>>
>>>> @@ -640,7 +653,8 @@ function commit_rerere_cache
>>>>               $(git --version)
>>>>               EOF
>>>>
>>>> -     if git commit -F $commit_message >& /dev/null; then
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>> +     if git commit $gpg_keyid -F $commit_message >& /dev/null; then
>>>>               echo -n "New commit. "
>>>>       else
>>>>               echo -n "Nothing changed. "
>>>> @@ -653,13 +667,14 @@ function commit_rerere_cache
>>>>
>>>>  function dim_rebuild_tip
>>>>  {
>>>> -     local integration_branch specfile first rerere repo remote
>>>> +     local integration_branch specfile first rerere repo remote gpg_keyid
>>>>
>>>>       integration_branch=drm-tip
>>>>       specfile=$(mktemp)
>>>>       first=1
>>>>
>>>>       rerere=$DIM_PREFIX/drm-rerere
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>>
>>>>       cd $rerere
>>>>       if git status --porcelain | grep -q -v "^[ ?][ ?]"; then
>>>> @@ -731,7 +746,7 @@ function dim_rebuild_tip
>>>>
>>>>                       # because we filter out fast-forward merges there will
>>>>                       # always be something to commit
>>>> -                     git commit --no-edit --quiet
>>>> +                     git commit $gpg_keyid --no-edit --quiet
>>>>                       echo "Done."
>>>>               fi
>>>>
>>>> @@ -743,7 +758,7 @@ function dim_rebuild_tip
>>>>       echo -n "Adding integration manifest $integration_branch: $dim_timestamp... "
>>>>       mv $specfile integration-manifest
>>>>       git add integration-manifest
>>>> -     git commit --quiet -m "$integration_branch: $dim_timestamp integration manifest"
>>>> +     git commit $gpg_keyid --quiet -m "$integration_branch: $dim_timestamp integration manifest"
>>>>       echo "Done."
>>>>
>>>>       remote=$(repo_to_remote drm-tip)
>>>> @@ -848,7 +863,7 @@ function dim_push
>>>>
>>>>  function apply_patch #patch_file
>>>>  {
>>>> -     local patch message_id committer_email patch_from sob rv
>>>> +     local patch message_id committer_email patch_from sob rv gpg_keyid
>>>>
>>>>       patch="$1"
>>>>       shift
>>>> @@ -860,7 +875,8 @@ function apply_patch #patch_file
>>>>               sob=-s
>>>>       fi
>>>>
>>>> -     git am --scissors -3 $sob "$@" $patch
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>> +     git am --scissors -3 $sob $gpg_keyid "$@" $patch
>>>>
>>>>       if [ -n "$message_id" ]; then
>>>>               dim_commit_add_tag "Link: https://patchwork.freedesktop.org/patch/msgid/$message_id";
>>>> @@ -911,7 +927,7 @@ function dim_apply_branch
>>>>
>>>>  function dim_apply_pull
>>>>  {
>>>> -     local branch file message_id pull_branch rv
>>>> +     local branch file message_id pull_branch rv gpg_keyid
>>>>
>>>>       branch=${1:?$usage}
>>>>       file=$(mktemp)
>>>> @@ -929,7 +945,8 @@ function dim_apply_pull
>>>>
>>>>       message_id=$(message_get_id $file)
>>>>
>>>> -     git commit --amend -s --no-edit
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>> +     git commit $gpg_keyid --amend -s --no-edit
>>>>       if [ -n "$message_id" ]; then
>>>>               dim_commit_add_tag "Link: https://patchwork.freedesktop.org/patch/msgid/$message_id";
>>>>       else
>>>> @@ -945,7 +962,7 @@ function dim_apply_pull
>>>>
>>>>  function dim_backmerge
>>>>  {
>>>> -     local branch upstream patch_file
>>>> +     local branch upstream patch_file gpg_keyid
>>>>
>>>>       branch=${1:?$usage}
>>>>       upstream=${2:?$usage}
>>>> @@ -990,8 +1007,9 @@ function dim_backmerge
>>>>               echoerr "   git commit -a"
>>>>       fi
>>>>
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>>       git add -u
>>>> -     git commit -s
>>>> +     git commit $gpg_keyid -s
>>>>  }
>>>>
>>>>  function dim_add_link
>>>> @@ -1227,7 +1245,7 @@ function dim_magic_patch
>>>>
>>>>  function dim_create_branch
>>>>  {
>>>> -     local branch repo remote
>>>> +     local branch repo remote gpg_keyid
>>>>
>>>>       branch=${1:?$usage}
>>>>       start=${2:-HEAD}
>>>> @@ -1250,13 +1268,14 @@ function dim_create_branch
>>>>       cd $DIM_PREFIX/drm-rerere
>>>>       $DRY sed -i "s/^\() # DO NOT CHANGE THIS LINE\)$/\t\"$repo\t\t${branch//\//\\\/}\"\n\1/" $integration_config
>>>>
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>>       $DRY git add $integration_config
>>>> -     $DRY git commit --quiet -m "Add $repo $branch to $integration_config"
>>>> +     $DRY git commit $gpg_keyid --quiet -m "Add $repo $branch to $integration_config"
>>>>  }
>>>>
>>>>  function dim_remove_branch
>>>>  {
>>>> -     local branch repo remote
>>>> +     local branch repo remote gpg_keyid
>>>>
>>>>       branch=${1:?$usage}
>>>>
>>>> @@ -1288,8 +1307,9 @@ function dim_remove_branch
>>>>       cd $DIM_PREFIX/drm-rerere
>>>>       $DRY sed -i "/^[[:space:]]*\"${repo}[[:space:]]\+${branch//\//\\\/}.*$/d" $integration_config
>>>>
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>>       $DRY git add $integration_config
>>>> -     $DRY git commit --quiet -m "Remove $repo $branch from $integration_config"
>>>> +     $DRY git commit $gpg_keyid --quiet -m "Remove $repo $branch from $integration_config"
>>>>
>>>>       dim_rebuild_tip
>>>>  }
>>>> @@ -1579,7 +1599,7 @@ function dim_for_each_workdir
>>>>
>>>>  function dim_update_next
>>>>  {
>>>> -     local remote
>>>> +     local remote gpg_keyid
>>>>
>>>>       assert_branch drm-intel-next-queued
>>>>
>>>> @@ -1597,12 +1617,13 @@ function dim_update_next
>>>>               exit 2
>>>>       fi
>>>>
>>>> +     gpg_keyid=$(gpg_keyid_for_commit)
>>>>       driver_date=$(date +%Y%m%d)
>>>>       driver_timestamp=$(date +%s)
>>>>       $DRY sed -i -e "s/^#define DRIVER_DATE.*\"[0-9]*\"$/#define DRIVER_DATE\t\t\"$driver_date\"/; s/^#define DRIVER_TIMESTAMP.*/#define DRIVER_TIMESTAMP\t$driver_timestamp/" \
>>>>            drivers/gpu/drm/i915/i915_drv.h
>>>>       $DRY git add drivers/gpu/drm/i915/i915_drv.h
>>>> -     git commit $DRY_RUN -sm "drm/i915: Update DRIVER_DATE to $driver_date"
>>>> +     git commit $DRY_RUN $gpg_keyid -sm "drm/i915: Update DRIVER_DATE to $driver_date"
>>>>
>>>>       gitk drm-intel-next-queued ^$(repo_to_remote drm-upstream)/drm-next &
>>>>
>>>> @@ -1614,7 +1635,7 @@ function dim_update_next
>>>>
>>>>  function dim_update_next_continue
>>>>  {
>>>> -     local remote intel_remote req_file suffix tag tag_testing
>>>> +     local remote intel_remote req_file suffix tag tag_testing gpg_keyid
>>>>
>>>>       assert_branch drm-intel-next-queued
>>>>
>>>> @@ -1630,7 +1651,8 @@ function dim_update_next_continue
>>>>               tag_testing="drm-intel-testing-$dim_today-$((++suffix))"
>>>>       done
>>>>
>>>> -     $DRY git tag -a $DIM_GPG_KEYID $tag $intel_remote/drm-intel-next
>>>> +     gpg_keyid=$(gpg_keyid_for_tag)
>>>> +     $DRY git tag -a $gpg_keyid $tag $intel_remote/drm-intel-next
>>>>       git push $DRY_RUN $intel_remote $tag
>>>>
>>>>       echo "Updating drm-intel-testing to latest drm-tip"
>>>> @@ -1655,7 +1677,7 @@ function dim_update_next_continue
>>>>
>>>>  function dim_tag_next
>>>>  {
>>>> -     local intel_remote tag suffix
>>>> +     local intel_remote tag suffix gpg_keyid
>>>>
>>>>       cd $DIM_PREFIX/$DIM_REPO
>>>>
>>>> @@ -1670,7 +1692,8 @@ function dim_tag_next
>>>>                       tag="drm-intel-next-$dim_today-$((++suffix))"
>>>>               done
>>>>
>>>> -             $DRY git tag -a $DIM_GPG_KEYID $tag $intel_remote/drm-intel-next
>>>> +             gpg_keyid=$(gpg_keyid_for_tag)
>>>> +             $DRY git tag -a $gpg_keyid $tag $intel_remote/drm-intel-next
>>>>               git push $DRY_RUN $intel_remote $tag
>>>>       else
>>>>               echo "drm-intel-next not up-to-date, aborting"
>>>> @@ -1700,7 +1723,7 @@ function prep_pull_tag_summary
>>>>  # dim_pull_request branch upstream
>>>>  function dim_pull_request
>>>>  {
>>>> -     local branch upstream remote repo req_file url_list git_url suffix tag
>>>> +     local branch upstream remote repo req_file url_list git_url suffix tag gpg_keyid
>>>>
>>>>       branch=${1:?$usage}
>>>>       upstream=${2:?$usage}
>>>> @@ -1731,7 +1754,8 @@ function dim_pull_request
>>>>               done
>>>>               gitk "$branch@{upstream}" ^$upstream &
>>>>               prep_pull_tag_summary | $DRY git tag -F- $tag "$branch@{upstream}"
>>>> -             $DRY git tag -a $DIM_GPG_KEYID -f $tag
>>>> +             gpg_keyid=$(gpg_keyid_for_tag)
>>>> +             $DRY git tag -a $gpg_keyid -f $tag
>>>>               $DRY git push $remote $tag
>>>>               prep_pull_mail $req_file $tag
>>>
>>> --
>>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> dim-tools mailing list
>> dim-tools@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dim-tools
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux