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(-)
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
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?
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.
On Tue, Oct 31, 2017 at 10:27:24AM +0200, Jani Nikula wrote:
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
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!
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.
Yep, sounds good.
Although... perhaps starting to sign tags should not force signing commits?
Why would it be desirable to *not* sign tags?
Sean
On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul seanpaul@chromium.org wrote:
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
On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter daniel@ffwll.ch wrote:
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
2017-10-31 Sean Paul seanpaul@chromium.org:
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:
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
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 Paul seanpaul@chromium.org writes:
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:
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:
I'm afraid my conclusion is the same. But we can revisit the idea once we have everyone using signed tags!
BR, Jani.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Deucher, Alexander -
Eric Anholt -
Gustavo Padovan -
Jani Nikula -
Sean Paul