| Message ID | 20171030210625.36897-1-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reminder, we have this new list dim-tools@lists.freedesktop.org for maintainer tools patches. Cc'd. On Mon, 30 Oct 2017, Sean Paul <seanpaul@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? > Signed-off-by: Sean Paul <seanpaul@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? Although... perhaps starting to sign tags should not force signing commits? 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
On Tue, Oct 31, 2017 at 10:27:24AM +0200, Jani Nikula wrote: > > Reminder, we have this new list dim-tools@lists.freedesktop.org for > maintainer tools patches. Cc'd. > > On Mon, 30 Oct 2017, Sean Paul <seanpaul@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? Same here. I get why signing tags makes sense, because email is an entirely unsecured protocol. Signing commits otoh seems mildly silly. What's the threat/attack model that prevents? And if we go with signed commits, shouldn't we encourage contributors to sign their mails and have some means to add the verification of the same to the commit? This is what happens when you merge a signed patch at least ... I'm confused about this. -Daniel > > > Signed-off-by: Sean Paul <seanpaul@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? > > Although... perhaps starting to sign tags should not force signing > commits? > > 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > Reminder, we have this new list dim-tools@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@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@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? 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
On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula > <jani.nikula@linux.intel.com> wrote: >> >> Reminder, we have this new list dim-tools@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@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@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. -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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dim-tools
On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> wrote: >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula >> <jani.nikula@linux.intel.com> wrote: >>> >>> Reminder, we have this new list dim-tools@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@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@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. 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dim-tools > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
2017-10-31 Sean Paul <seanpaul@chromium.org>: > On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> wrote: > >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula > >> <jani.nikula@linux.intel.com> wrote: > >>> > >>> Reminder, we have this new list dim-tools@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@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@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
On Wed, Nov 1, 2017 at 7:12 AM, Gustavo Padovan <gustavo@padovan.org> wrote: > 2017-10-31 Sean Paul <seanpaul@chromium.org>: > >> On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> wrote: >> >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula >> >> <jani.nikula@linux.intel.com> wrote: >> >>> >> >>> Reminder, we have this new list dim-tools@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@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@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? Sean [1]- https://lists.gnupg.org/pipermail/gnupg-users/2015-October/054608.html > > Gustavo >
> -----Original Message----- > From: dim-tools [mailto:dim-tools-bounces@lists.freedesktop.org] On Behalf > Of Sean Paul > Sent: Wednesday, November 01, 2017 8:52 AM > To: Gustavo Padovan > Cc: Daniel Vetter; Intel Graphics Development; dim- > tools@lists.freedesktop.org; dri-devel; Daniel Vetter > Subject: Re: [Intel-gfx] [maintainer-tools PATCH] dim: Sign commits in > addition to tags > > On Wed, Nov 1, 2017 at 7:12 AM, Gustavo Padovan <gustavo@padovan.org> > wrote: > > 2017-10-31 Sean Paul <seanpaul@chromium.org>: > > > >> On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > >> > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> > wrote: > >> >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula > >> >> <jani.nikula@linux.intel.com> wrote: > >> >>> > >> >>> Reminder, we have this new list dim-tools@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@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@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? I feel like this raises the bar pretty high if we want to move to more committers. Most of the work will fall on the few people willing to jump through the hoops or patches will just get dropped as people lose interest after fighting with gpg. I can't say I've ever had a particularly straight forward interaction with gpg for anything. Alex > > Sean > > [1]- https://lists.gnupg.org/pipermail/gnupg-users/2015- > October/054608.html > > > > > Gustavo > > > _______________________________________________ > dim-tools mailing list > dim-tools@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dim-tools
Sean Paul <seanpaul@chromium.org> writes: > On Wed, Nov 1, 2017 at 7:12 AM, Gustavo Padovan <gustavo@padovan.org> wrote: >> 2017-10-31 Sean Paul <seanpaul@chromium.org>: >> >>> On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> wrote: >>> >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula >>> >> <jani.nikula@linux.intel.com> wrote: >>> >>> >>> >>> Reminder, we have this new list dim-tools@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@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@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.
On Wed, Nov 1, 2017 at 1:00 PM, Eric Anholt <eric@anholt.net> wrote: > Sean Paul <seanpaul@chromium.org> writes: > >> On Wed, Nov 1, 2017 at 7:12 AM, Gustavo Padovan <gustavo@padovan.org> wrote: >>> 2017-10-31 Sean Paul <seanpaul@chromium.org>: >>> >>>> On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>> >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula >>>> >> <jani.nikula@linux.intel.com> wrote: >>>> >>> >>>> >>> Reminder, we have this new list dim-tools@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@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@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
On Wed, 01 Nov 2017, Sean Paul <seanpaul@chromium.org> wrote: > On Wed, Nov 1, 2017 at 1:00 PM, Eric Anholt <eric@anholt.net> wrote: >> Sean Paul <seanpaul@chromium.org> writes: >> >>> On Wed, Nov 1, 2017 at 7:12 AM, Gustavo Padovan <gustavo@padovan.org> wrote: >>>> 2017-10-31 Sean Paul <seanpaul@chromium.org>: >>>> >>>>> On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>>> > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul <seanpaul@chromium.org> wrote: >>>>> >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula >>>>> >> <jani.nikula@linux.intel.com> wrote: >>>>> >>> >>>>> >>> Reminder, we have this new list dim-tools@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@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@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. I'm afraid my conclusion is the same. But we can revisit the idea once we have everyone using signed tags! BR, Jani.
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 +} + 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
Expanding on Jani's work to sign tags, this patch adds signing for git commit/am. Signed-off-by: Sean Paul <seanpaul@chromium.org> --- This has been lightly tested with dim apply-branch/dim push-branch. Sean dim | 78 +++++++++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 27 deletions(-)