search

dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.62k stars 991 forks source link

Feature-request: PGP signed commits and verified merges from Dependabot #2145

Closed HendrikPetertje closed 1 year ago

HendrikPetertje commented 5 years ago

I work in a team using dependabot where all team members are advised to sign their commits with PGP keys. This is to make sure (to some degree) that everything done is signed and things altered by people or bots with bad intentions stick out.

Dependabot however, doesn't sign its commits, nor are the squashed commits verified Github merges. Would it be possible to make this possible?

example commits log with Dependabot on a PGP enforced team: Screenshot 2019-06-03 at 10 01 39

Thanks!

greysteil commented 5 years ago

Hmmm, Dependabot does sign its commits - where are you seeing that they're not? And are you finding that squash merges are signed for other users? I thought it was the case that squash merges created in the GitHub UI are never signed (which is an issue unrelated to Dependabot).

greysteil commented 5 years ago

@HendrikPetertje quick chase on the above.

glennawatson commented 5 years ago

Also encountering this on our repo https://github.com/reactiveui/Pharmacist/commits/master

greysteil commented 5 years ago

@glennawatson looks like you're also using squash and merge. Is the signature showing up as verified for any pull requests you merge through the GitHub UI in that way (non-Dependabot ones)?

glennawatson commented 5 years ago

Yes, good examples are in https://github.com/reactiveui/splat/commits/master -- where the last 3 commits are squash and commit and marked as verified.

greysteil commented 5 years ago

OK, thanks. I'm going to pass this on to @laserlemon who knows more about GitHub's automated bot commit signing than I do.

glennawatson commented 5 years ago

Thanks for looking into it :)

laserlemon commented 5 years ago

👋 I've reproduced the issue and found that the squash/merge commit lacks verification whenever the merging user of a pull request is different than the opening user for that pull request. Of course, this is always the case for Dependabot pull requests that are merged by a human.

I'll dig into the reasoning behind this behavior. Thank you for raising the issue!

laserlemon commented 5 years ago

Here's what I've learned… GitHub's ✨ magic ✨ commit signing takes advantage of Git's distinction between a commit's "author" and its "committer." When somebody uses the "squash and merge" button, there are a couple of possibilities for how those two bits of information are set:

Condition Author Committer
The pull request opener is also the merger The pull request opener GitHub's verification user
The pull request merger is somebody else The pull request opener The pull request merger

The problem lies in the fact that we want three bits of attribution:

…but we only have two fields in which to store that attribution. The best way forward (in my opinion) to ensure squash/merge commit signing is to choose either the pull request opener or merger to attribute as the author, leaving the committer spot always open for GitHub's verification user. However, I fear that change may be too breaking to actually implement. I'll poke around nonetheless!

laserlemon commented 5 years ago

Another possibility is to set the pull request opener as the author and the pull request merger as a co-author, leaving the committer spot available for the verification user.

greysteil commented 5 years ago

Another possibility is to set the pull request opener as the author and the pull request merger as a co-author, leaving the committer spot available for the verification user.

Ooh, that sounds like it would make a tonne of sense! Thanks for your work digging into this @laserlemon!

glennawatson commented 4 years ago

Is this work being covered elsewhere @laserlemon?

bbugh commented 4 years ago

Ran into this issue, our repository requires that all members sign commits and without that only admins can merge. Anything the community can do to help move this forward?

laserlemon commented 4 years ago

There's been some new work on squash commits and their co-authorship. The work is in this pull request and it's described in this Changelog entry.

This doesn't solve the problem… yet.

The next possible step, if we have the will to make this breaking change, would be to stop using the pull request squasher/merger as the commit's committer. Instead, we could drop that user's attribution entirely (since they didn't really write any of it necessarily), or add them as a co-author. I have a slight preference for dropping attribution for the person who pushes the merge button. It's not always indicative of who did the review work anyway.

So my proposed behavior is:

And the result would be:

laserlemon commented 4 years ago

Another option exists where we add support for a trailer like Co-authored-by, except Merged-by.

greysteil commented 4 years ago

I'm 👍 on the user who merges not being credited as the committer in a squash and merge scenario. Doesn't feel inconsistent with other merge case (because the merge commit committer is already GitHub's verification user). In fact, it feels kind of inconsistent that they are at the moment.

I could go either way on the co-author - I essentially don't have strong feelings on it. My hunch is to therefore exclude it, and add it if/when we have community feedback, either as Co-authored-by or Merged-by.

dlahn commented 4 years ago

Has there been an issue within the past couple of days where Dependabot is not signing commits? We have our repos set to require signing, and have been using Dependabot successfully like this (as all of the commits are signed.) for quite some time. However, in the past couple of days, the commits are coming in unsigned.

donmahallem commented 4 years ago

Commit merges are still signed for me but squash merges aren't signed when merged.

laserlemon commented 4 years ago

I believe they are now!

alexkuc commented 4 years ago

@laserlemon Was this confirmed? I am using "rebase and merge" strategy for dependabot and come across the following behaviour:

PR branch: Screenshot 2020-06-21 01 43 47

Rebased and merged branch: Screenshot 2020-06-21 01 44 24

simkimsia commented 2 years ago

Same issue as @alexkuc

On the PR, can see that it's verified image

At the main branch after i rebased and merged

image

I see at rebase and merge authored and signed off by dependabot but the committer ends up being me

which is not an issue because i did a regular merge on github.com and the verified is just fine

CleanShot 2021-11-09 at 16 01 28@2x

help pls?

UPDATE

I now understand the situation.

https://stackoverflow.com/questions/62950018/verified-signatures-are-gone-after-i-pressed-rebase-and-merge/62950277#62950277

The verification is done through a cryptographic signature of the contents and the commit-metadata. Hence, rebasing will break that signature.

to have verified signature, need a fastforward merge

Rebase and merge on GitHub.com will always create new SHA commits see https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/about-merge-methods-on-github#rebasing-and-merging-your-commits for reference.

So this is a limitation of GitHub.com not dependabot-core.

Ugly workaround

The ugly workaround is create the PR (say feature branch on develop branch) but then go back to local repo.

git checkout feature Take the feature branch and make sure it's rebased on origin/develop branch.

Now git checkout develop locally. Merge feature branch into develop using fast-forward merge git merge --ff-only feature

Then push local develop to origin/develop git push -u origin develop

GitHub.com will auto detect now the origin/feature and origin/develop are now on par and so will auto close the PR.

Now the commits will all have the verified badge

jeffwidman commented 1 year ago

The summary as best I can tell is the commits are now verified for fast-forward merges, but as explained in ☝️, they won't be for rebase-and-merge PR's. I do see squash-and-merge showing as verified in the commit history here in dependabot-core itself.

I'm going to close as there's nothing more we can do specifically here in Dependabot... if there's still an issue with the display of the PR's for rebase-and-merge it's a general GitHub PR's issue, not specific to Dependabot.