New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PROPOSAL: require mandatory GPG sign for every commits (or collaborators) #15457

Closed
phra opened this Issue Sep 18, 2017 · 30 comments

Comments

Projects
None yet
9 participants
@phra

phra commented Sep 18, 2017

Hi,

I propose to increase the security of this repository requiring a GPG sign verification for every commits, à la Linux kernel.

Of course, it can be a bit intimidating for new committers, but NodeJS is a very popular JavaScript runtime and we should try to implement the maximum level of security available to prevent any tampering with it.

As a fallback, I suggest at least to require the GPG sign for every stable collaborator.

What do you think about it guys?

@tniessen

This comment has been minimized.

Member

tniessen commented Sep 18, 2017

No strong opinion on this, just some thoughts:

  • Collaborators use 2FA on GitHub, so the only way for a third party to push to this repository (that I know of) is by somehow retrieving one of the personal access tokens.
  • Signing every commit does not enhance security further than signing the last commit of each change set from a single author.
  • Having contributors sign commits does not really add anything either as those are being reviewed manually (so we don't really care who pushed the changes, we just care about the contents) and the signatures cannot be copied to the master branch due to rebasing.
@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Sep 18, 2017

I'm not really certain this would do any good, and it would probably be lost through our branch management anyways.

I'd be ok with having this be for releasers doing releases though, including myself.

@phra

This comment has been minimized.

phra commented Sep 19, 2017

The main benefit is to prevent that someone can push a commit with an arbitrary user.name and user.email in their .gitconfig without having a proof that proves the identity is correct, that is the non-repudation property.

As you can see on this repository you have both started to contribute to my brand new repository: https://github.com/phra/git-author-commit/commits/master
Only the latest commit can be trusted because it's GPG signed.

Just my two cents.

@tniessen

This comment has been minimized.

Member

tniessen commented Sep 19, 2017

Collaborators and contributors are free to sign commits as they like at the moment. Don't get me wrong, signing commits does add to security and I approve doing so, but I don't see much value in enforcing this behavior at the moment.

As you can see on this repository you have both started to contribute to my brand new repository:

And signing my commits in this repository would not have prevented that.
The problem with GPG signatures is that you can say "yes, that's mine", but not "nope, that's not mine". You could still push unsigned commits with my email address and name, they just won't have the green badge.
(By the way, I would be careful with that, depending on your country of residence, online impersonation and/or resulting harm for those who have been impersonated can be considered a federal crime.)

Sure, it is a valid point that having all contributors sign all commits would allow us to be certain that they are indeed the author of the commit, but as long as we require all changes to be proposed via PRs which we manually review, we know who actually pushed the code. (And if you want to make some great contributions and put my name under them, please, go ahead.)

tniessen referenced this issue in phra/git-author-commit Sep 19, 2017

tniessen referenced this issue in phra/git-author-commit Sep 19, 2017

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 19, 2017

Furthermore, we accept (and have collaborators that submit) anonymous and pseudonymous contributions. Mandating GPG signing doesn't full-out prohibit that but it makes the barrier to entry much higher.

@phra

This comment has been minimized.

phra commented Sep 19, 2017

Ok, I'm already positive towards the fact that you can see the security benefits of GPG signed commits besides the existing protections, that was my goal actually. 🙂

Just keep in mind that the same attack that I've done on my own repository impersonating you can be replicated by anyone that has write permission on the Node.js repository itself and GPG signs can effectively prevent that. 🙂

You are free to close the ticket.

@tniessen

This comment has been minimized.

Member

tniessen commented Sep 19, 2017

Personally, I would be okay with requiring people with write access to this repository to sign commits which they push to public branches (not including PRs), many collaborators already do so.

@phra

This comment has been minimized.

phra commented Sep 19, 2017

@tniessen that would be already a big improvement! 💯

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Sep 19, 2017

As you can see on this repository you have both started to contribute to my brand new repository: https://github.com/phra/git-author-commit/commits/master

Right - at least I've known about this for a long time already.
Our existing crosslinking policies already mitigate this though as all (except some release) commits have references to discussions in the issue tracker. That makes it much easier to find if a commit wasn't what was agreed upon.

@phra

This comment has been minimized.

phra commented Sep 19, 2017

I've sent a report to GitHub via HackerOne to ask for a visual feedback when the authors of commit/push don't match (like the Verified badge) and for an opt-in option to globally forbid pushing commits that pretend to be authored by us but they are not GPG signed. I can keep you updated about their response.

@tniessen

This comment has been minimized.

Member

tniessen commented Sep 19, 2017

globally forbid pushing commits that pretend to be authored by us but they are not GPG signed

I might miss your point here, but I don't think this is going to work. If I push a signed commit and someone else cherry-picks it onto another branch, he won't be able to push it?

@phra

This comment has been minimized.

phra commented Sep 19, 2017

@tniessen if the commit is GPG signed it can freely be used by anyone and you can keep the authorship. Other people should also be able to push GPG signed commits from other authors. (if they were not edited after the sign of course)

I can copy a piece of the report to describe what I mean:

There are three different levels of trustiness about a commit as I can see:

- GPG Signed: a GPG sign is attached to the commit effectively proving the authorship regardless of the user that is pushing it
- Authenticated: the user that is pushing the commit is the same that has authored the commit itself.
- Not Authenticated/Anonymous: the commit is not GPG signed and the user that is pushing the commit differs from the user that pretends to have authored the commit itself.

### Proposed solutions

- At the moment is only possible to visually distinguish between 1st and 2nd cases with the "Verified" badge, but it would be ideal also have a visual feedback to distinguish between 2nd and 3rd scenarios.
- Implement an opt-in option in personal settings to forbid any push of not-signed commits that pretends to be authored by us.

so the opt-in option that i'm thinking about will only affect not signed commits if enabled.

@tniessen

This comment has been minimized.

Member

tniessen commented Sep 19, 2017

if the commit is GPG signed it can freely be used by anyone and you can keep the authorship.

If I am not mistaken, the SHA of the tree object is part of the signature, therefore rebasing / cherry-picking / merging would invalidate and thus remove the signature, right? I think it is not the author but the committer who signs a commit.

@phra

This comment has been minimized.

phra commented Sep 19, 2017

@tniessen uhm.. actually you are right I think, I was not remembering that a commit points to the previous one! thanks for noticing it!
Yep, keeping authorship (i.e. the original GPG sign) when altering the previous history of a GPG signed commit can be a problem.. maybe an acceptable trade-off can be to keep authorship + sign when working with commits without altering the previous history, e.g. forks, but discarding the authorship if someone apply that commit to a different git history. (it's effectively a different commit)

@phra

This comment has been minimized.

phra commented Sep 19, 2017

GitHub response:

Hi,

Thanks for the submission!
This is working as designed, and we are aware there are ways tracking commit attribution can be improved.
We've considered some of your proposed solutions in the past, and we may make commit attribution more refined in the future, but don't have anything to announce now.
As a result, this is not eligible for reward under the Bug Bounty program.

Best regards and happy hacking!
Brent

so yeah, at the moment the most acceptable tradeoff is @tniessen's proposal:

Personally, I would be okay with requiring people with write access to this repository to sign commits which they push to public branches (not including PRs), many collaborators already do so.

EDIT: btw it would be nice to have a personal, not global opt-in option to prevent not GPG signed commits to be pushed pretending that are authored by you on a particular repository.

@benjamingr

This comment has been minimized.

Member

benjamingr commented Sep 20, 2017

Because releasers manually take commits from masters to releases and review what they're releasing, and those already have PGP keys and are signed - I don't see the point.

This would only potentially help people building directly from master and not a release which I don't think justifies the overhead.

-1 from me on it.

@phra

This comment has been minimized.

phra commented Sep 20, 2017

I don't see the point.

Did you see this?

As I said in my first comment to this issue:

The main benefit is to prevent that someone can push a commit with an arbitrary user.name and user.email in their .gitconfig without having a proof that proves the identity is correct, that is the non-repudation property.

It's not about opinions on overhead, but non-repudiation property, that a cumulative signature cannot guarantee.

By the way, maybe I've found the right requirement:

  • Every commits that appears on master branch MUST be GPG-signed.

I think that this can be the optimal trade-off because master branch should be immutable, so no problem with git rebase et alia.

Releasers can simply cherry-pick GPG-signed commits and then GPG-sign the release as well, with all the security benefits of it.

What do you think about this?

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 21, 2017

The value simply isn't that big because of how the project is structured and it might scare away contributors. So yeah, I don't see it happening.

@gibfahn

This comment has been minimized.

Member

gibfahn commented Sep 24, 2017

The value simply isn't that big because of how the project is structured and it might scare away contributors.

This requirement would only affect collaborators, as all non-collaborator PRs are rebased onto master anyway.

This would only potentially help people building directly from master and not a release which I don't think justifies the overhead.

There isn't really much overhead, you just set up your gpg keys, do git config --global commit.gpgsign true, and that's it. You do have to enter your gpg key password once per session, but that's not exactly a huge effort.

We do (I think) limit access to release branches (see nodejs/Release#199), and pushes to those branches would be much more obvious.

I don't see the downside to requiring this

Furthermore, we accept (and have collaborators that submit) anonymous and pseudonymous contributions. Mandating GPG signing doesn't full-out prohibit that but it makes the barrier to entry much higher.

I don't think it makes it higher, you just use the same name/email in your gpg key that you use in your Github account (and in your commits). Same as an ssh key. Github just checks that the key used matches the one you save in your account.

Collaborators use 2FA on GitHub, so the only way for a third party to push to this repository (that I know of) is by somehow retrieving one of the personal access tokens.

Or by retrieving the private ssh key of a collaborator, which doesn't seem impossible, if you don't password protect your ssh key, then it's just stored in plaintext in ~/.ssh.

Signing every commit does not enhance security further than signing the last commit of each change set from a single author.

Sure, but it's easier for both collaborators and people checking to just sign all commits automatically.

I think the first step would be to add a note to the COLLABORATOR_GUIDE recommending that people enable gpg signing, and then to ask people who currently don't sign their commits whether they would be willing to. If no-one has a strong objection to requiring gpg signatures for commits then we can consider making it mandatory (although I'm not sure how we'd enforce it without protecting the branch, which would prevent force pushing).

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 25, 2017

I don't see the downside to requiring this

The downside is that there is no real upside, IMO, just more hassle. It doesn't significantly increase security like e.g. 2FA does.

Or by retrieving the private ssh key of a collaborator, which doesn't seem impossible

Maybe not (although I hope and assume everyone encrypts their private key) but there is not much damage an attacker could cause. Any vandalism would be restored within minutes.

@gibfahn

This comment has been minimized.

Member

gibfahn commented Sep 25, 2017

Maybe not (although I hope and assume everyone encrypts their private key) but there is not much damage an attacker could cause. Any vandalism would be restored within minutes.

If someone were to land a commit that was slightly modified to master, would anyone notice? I'd say probably not. It's quite possible that someone could make changes to code that were not detected before being included in a release. I don't think the attack vector is "someone deleting stuff", it'd be someone adding backdoors to the code.

We get close to a hundred commits a week, and AFAIK no-one checks the majority of them.


FWIW you can check how many people don't have GPG enabled with:

git log --since="one month" --pretty=format:"%h %cn" | 
  while read i; do test "$(git verify-commit ${i%% *} 2>&1)" || echo ${i#* }; done | 
  sort | uniq
@gibfahn

This comment has been minimized.

Member

gibfahn commented Sep 25, 2017

although I hope and assume everyone encrypts their private key

Also in my experience this is far from being the case.

@phra

This comment has been minimized.

phra commented Nov 30, 2017

@gibfahn thanks for one liner, i will put it as an alias in my shell!

although I hope and assume everyone encrypts their private key

actually i hope that people also encrypt HDDs and SSDs, at least the ones with a privileged access to nodejs repository because in theory if you don't encrypt your HDD i can physically remove it, backdoor it, put it back again and just wait for you to write the password on the keyboard, after having copied the encrypted key, of course!

FYI GitHub enabled GPG signatures for actions taken on the website (like a PR merge) that involves GPG signed commits, so basically every commit that is signed locally will be re-signed by GitHub if required.

screenshot from 2017-11-30 17-57-04

in their reply, they said:

We've considered some of your proposed solutions in the past, and we may make commit attribution more refined in the future, but don't have anything to announce now.

so probably they were thinking about this feature that was not ready at that time yet.

this means that if people will enable GPG locally, it's possibile to have the entire git history GPG signed for additional protection against tampering.

just my 2 cents 💸

@gibfahn

This comment has been minimized.

Member

gibfahn commented Nov 30, 2017

this means that if people will enable GPG locally, it's possibile to have the entire git history GPG signed for additional protection against tampering.

We don't use the Github merging features at all, so that wouldn't be a problem anyway.

@mhdawson

This comment has been minimized.

Member

mhdawson commented Nov 30, 2017

I end up using a number of different machines including some shared ones. I worry that gpg signing will either limit where I can commit from (making me less efficient) or I'm going to have more work moving keys around. Is this unjustified ?

@gibfahn

This comment has been minimized.

Member

gibfahn commented Nov 30, 2017

@mhdawson signing is done at commit time, and as all commits are rebased on master, you'd only have to have gpg keys on the machine you use to land commits.

Moving gpg keys is about as painful as moving ssh keys in my experience, so not hugely painful. But this is the question, is it's going to make people's workflows harder then that's a reason not to do it.

If we have an commit landing queue, then that avoids the question entirely.

@phra

This comment has been minimized.

phra commented Dec 1, 2017

@mhdawson @gibfahn i personally use this strategy for backing up my keys:

BACKUP

  1. generate a pair of keys
    gpg2 --full-generate-key
  2. send public key to keyservers
    gpg2 --send-keys B52FAC34A07EB53F340E985491FF93D1B85D76B5
  3. encrypt and sign the private key
    gpg2 --export-secret-keys --armor | gpg2 --encrypt --sign --armor -r greensoncio@gmail.com > .phra.asc
  4. upload the encrypted version of the key as a gist on github
    MANUAL STEP

RESTORE

  1. retrieve my public key
    gpg2 --recv-keys B52FAC34A07EB53F340E985491FF93D1B85D76B5
  2. set trust level of the key in GPG
    gpg2 --edit-key greensoncio@gmail.com trust
  3. retrieve an encrypted and signed copy of my private key from a gist
    wget -O ~/.phra.asc <gist_url>
  4. decrypt the key
    gpg2 ~/.phra.asc
  5. import the key
    gpg2 --import ~/.phra
  6. remove decrypted copy from the filesystem
    shred -zuvn 3 ~/.phra

restore function in my .bash_aliases:

upgrade_gpg_keys() {
     gpg2 --recv-keys B85D76B5 &&
     gpg2 --edit-key greensoncio@gmail.com trust &&
     wget -O ~/.phra.asc <gist_url> &&
     gpg2 ~/.phra.asc &&
     gpg2 --import ~/.phra &&
     shred -zuvn 3 ~/.phra
}

GIT CONFIGURATION
then to configure git you have to update its configuration:

  1. set signing key
    git config --global user.signingkey B52FAC34A07EB53F340E985491FF93D1B85D76B5
  2. enable signed commits globally
    git config --global commit.gpgsign true

GPG CONFIGURATION optional
if you want to be able to check GitHub's signatures you have to import its key:

  1. import the key
    gpg2 --recv-keys 4AEE18F83AFDEB23
  2. set trust level
    gpg2 --edit-key noreply@github.com trust
@gibfahn

This comment has been minimized.

Member

gibfahn commented Dec 1, 2017

git config --global user.signingkey B52FAC34A07EB53F340E985491FF93D1B85D76B5

FWIW you don't need to do this if you only have one key which has the same email as git config --global user.email.

@mhdawson

This comment has been minimized.

Member

mhdawson commented Dec 1, 2017

@gibfahn yes, I have at least 3-4 different machines that I might use for landing changes.

@Trott Trott removed the discuss label Mar 11, 2018

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Apr 22, 2018

I do not see this happening, so I am closing the issue. If we consider else wise, we can still reopen this.

Thanks a lot for bringing this subject up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment