| Summary: | git update hook to validate gpg signatures on commits | ||
|---|---|---|---|
| Product: | Gentoo Infrastructure | Reporter: | Robin Johnson <robbat2> |
| Component: | Git | Assignee: | Gentoo Infrastructure <infra-bugs> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | admwiggin, adrian.ratiu, alex_y_xu, bangert, ferringb, ford_prefect, hasufell, kensington, mgorny, nikoli, pacho, peter, prometheanfire, rauchwolke, viklevin2, wking |
| Priority: | High | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | |||
| Bug Blocks: | 333531, 502062 | ||
| Attachments: |
My update hook
update hook, v2 |
||
|
Description
Robin Johnson
2014-02-22 00:05:27 UTC
See bug 502062 as well, there it was found that the gitolite update hook already does almost everything we need. Relatedly, on the Git front, I see it got easier to do signing in Git, and these will definitely help in the gentoo front, as if you always do pull/rebase/am with --gpg-sign, then you'll be good to push with less resigning. http://thread.gmane.org/gmane.comp.version-control.git/242412 * bc/gpg-sign-everywhere (2014-02-11) 9 commits (merged to 'next' on 2014-02-13 at 390376c) + pull: add the --gpg-sign option. + rebase: add the --gpg-sign option + rebase: parse options in stuck-long mode + rebase: don't try to match -M option + rebase: remove useless arguments check + am: add the --gpg-sign option + am: parse options in stuck-long mode + git-sh-setup.sh: add variable to use the stuck-long mode + cherry-pick, revert: add the --gpg-sign option Teach "--gpg-sign" option to many commands that create commits. Changes to some scripted Porcelains use unsafe variable substitutions and still need to be tightened. Will cook in 'next'. * nv/commit-gpgsign-config (2013-12-17) 3 commits (merged to 'next' on 2014-01-03 at 9780cbb) + test the commit.gpgsign config option + commit-tree: add and document --no-gpg-sign + Add the commit.gpgsign option to sign all commits Introduce commit.gpgsign configuration variable to force every commit to be GPG signed. The variable cannot be overriden from the command line of some of the commands that create commits except for "git commit" and "git commit-tree", but I am not convinced that it is a good idea to sprinkle support for --no-gpg-sign everywhere. Will cook in 'next'. I swear I posted this somewhere. http://mikegerwitz.com/papers/git-horror-story has a use case for, explanation of, and implementation of required git commit signing. It was "Last updated 2013-11-10 00:16:01 EST". I have not verified whether the information is still correct, but it should be reasonably so. (In reply to Alex Xu (Hello71) from comment #3) > I swear I posted this somewhere. > > http://mikegerwitz.com/papers/git-horror-story has a use case for, > explanation of, and implementation of required git commit signing. > > It was "Last updated 2013-11-10 00:16:01 EST". I have not verified whether > the information is still correct, but it should be reasonably so. What was the point of posting that link? We KNOW it's required. We just need to write the update.secondary hook and deploy it for the repo, per: http://stackoverflow.com/questions/13870607/using-gitolite-vrefs-for-update-hook http://gitolite.com/gitolite/dev-notes.html#update-hook Here's somebody else's regular update hook that checks GPG signatures and enforces some other policies (eg merge-only master): https://github.com/isislovecruft/scripts/blob/master/check-commit-signature (In reply to Robin Johnson from comment #4) > (In reply to Alex Xu (Hello71) from comment #3) > > I swear I posted this somewhere. > > > > http://mikegerwitz.com/papers/git-horror-story has a use case for, > > explanation of, and implementation of required git commit signing. > > > > It was "Last updated 2013-11-10 00:16:01 EST". I have not verified whether > > the information is still correct, but it should be reasonably so. > What was the point of posting that link? We KNOW it's required. > > We just need to write the update.secondary hook and deploy it for the repo, > per: > http://stackoverflow.com/questions/13870607/using-gitolite-vrefs-for-update- > hook > http://gitolite.com/gitolite/dev-notes.html#update-hook > > Here's somebody else's regular update hook that checks GPG signatures and > enforces some other policies (eg merge-only master): > https://github.com/isislovecruft/scripts/blob/master/check-commit-signature I was pointing out a resource where an example script might be found. In hindsight, such resources are likely readily accessible on the internet. @Robin: I've been operating under the assumption option #2 was the intent; resigning the commit being the thought. In hindsight, that may be academically pure, but less friendly to actual workflows. I don't really like the notion of you pushing some commit I shared into the tree on my behalf, and having the history look like I did it- my view, the pusher is the one truly making the change, thus the signing has to be theirs (they own the fallout). Comments/thoughts? (In reply to Brian Harring from comment #6) > @Robin: I've been operating under the assumption option #2 was the intent; > resigning the commit being the thought. > > In hindsight, that may be academically pure, but less friendly to actual > workflows. I don't really like the notion of you pushing some commit I > shared into the tree on my behalf, and having the history look like I did > it- my view, the pusher is the one truly making the change, thus the signing > has to be theirs (they own the fallout). > > Comments/thoughts? I think we should use option 1 along with logging the pusher in git notes (bug 502062). (In reply to Alex Xu (Hello71) from comment #7) > (In reply to Brian Harring from comment #6) > > @Robin: I've been operating under the assumption option #2 was the intent; > > resigning the commit being the thought. Both of you are delving into workflows, which is a separate issue than actual validation, and I'd like to keep separate from this bug. I'm going to start a thread on -dev to discuss it, because I've been wondering about merge-only master, plus branches for each dev & team (or even multiple branches if a dev wanted them). > > In hindsight, that may be academically pure, but less friendly to actual > > workflows. I don't really like the notion of you pushing some commit I > > shared into the tree on my behalf, and having the history look like I did > > it- my view, the pusher is the one truly making the change, thus the signing > > has to be theirs (they own the fallout). > > Comments/thoughts? The case of pusher != committer will be very rare. It comes down to who is creating the merge commit? Here is a scenario (host, repo, branch, commits): gentoo:gentoo-x86:origin/master:A localhost:gentoo-x86:dev/robbat2:A-B-Z github:gentoo-x86:dev/ferringb:A-B-C I find that commit C is useful, and does fix correctly. It is also already signed by your developer key, but NOT yet pushed to gentoo. I pull it to my tree, explicitly signing the merge commit D. My tree now looks like: localhost:gentoo-x86:dev/robbat2:A-B-Z-C-D If I want to push that as is, the GPG validation must accept signatures from all devs. If I must re-sign C, then it the tree is actually A-B-Z-C'-D, and what happens when ferringb tries to pull that sequence into his repo? > I think we should use option 1 along with logging the pusher in git notes > (bug 502062). I'm actually more in favour of #3 in his option, but again, as I say, this bug is about the actual signing, not the policies. (In reply to Robin Johnson from comment #8) > (In reply to Alex Xu (Hello71) from comment #7) > > (In reply to Brian Harring from comment #6) > > > @Robin: I've been operating under the assumption option #2 was the intent; > > > resigning the commit being the thought. > Both of you are delving into workflows, which is a separate issue than > actual validation, and I'd like to keep separate from this bug. I'm going to > start a thread on -dev to discuss it, because I've been wondering about > merge-only master, plus branches for each dev & team (or even multiple > branches if a dev wanted them). it's possible I'm just dense, but I don't think you ever started that thread. how does funtoo do it? > > > In hindsight, that may be academically pure, but less friendly to actual > > > workflows. I don't really like the notion of you pushing some commit I > > > shared into the tree on my behalf, and having the history look like I did > > > it- my view, the pusher is the one truly making the change, thus the signing > > > has to be theirs (they own the fallout). > > > Comments/thoughts? > The case of pusher != committer will be very rare. It comes down to who is > creating the merge commit? > > Here is a scenario (host, repo, branch, commits): > gentoo:gentoo-x86:origin/master:A > localhost:gentoo-x86:dev/robbat2:A-B-Z > github:gentoo-x86:dev/ferringb:A-B-C > > I find that commit C is useful, and does fix correctly. It is also already > signed by your developer key, but NOT yet pushed to gentoo. > > I pull it to my tree, explicitly signing the merge commit D. My tree now > looks like: > localhost:gentoo-x86:dev/robbat2:A-B-Z-C-D > > If I want to push that as is, the GPG validation must accept signatures from > all devs. yes, that's how it must be to keep hashes equal. > If I must re-sign C, then it the tree is actually A-B-Z-C'-D, and what > happens when ferringb tries to pull that sequence into his repo? don't re-sign. > > I think we should use option 1 along with logging the pusher in git notes > > (bug 502062). > I'm actually more in favour of #3 in his option, but again, as I say, this > bug is about the actual signing, not the policies. i'm pretty sure we're going around in circles confusing the Options 1 and 2 in description and my link. Created attachment 384710 [details]
My update hook
This is how I see it. I've tested it a bit, and seems to refuse correctly what needs to be refused.
Specifically:
1. disallows creating tags in the repo,
2. in 'master', disallows:
a. forced updates,
b. mistakenly committed ChangeLogs,
c. first-line commits with no, invalid or untrusted signatures,
3. for other (developer/project) branches, everything is allowed -- we don't give any support for them.
This assumes that GPG verification infra is in place, i.e. something else fetches, trusts and revokes keys as necessary.
Created attachment 384726 [details]
update hook, v2
Small fix: I meant git rev-list "${new_rev}" "^${old_rev}" for the commits-to-check-for-signature.
(In reply to Michał Górny from comment #10) > Created attachment 384710 [details] ... > This assumes that GPG verification infra is in place, i.e. something else > fetches, trusts and revokes keys as necessary. I don't think this catches some of the things that the other validation script does: https://github.com/isislovecruft/scripts/blob/master/check-commit-signature Also, we can have multiple scripts, and I'm wondering if it would be best to seperate the checks in them: 1. If master or other trust-required branch: Validate signature (probably using a modified check-commit-signature, because we need to check against a list of allowed keys) 1.1. Since we are allowing unsigned branches, we must validate entire branches on merge, ALL commits therein need to be signed (otherwise we are open to the same substitution attack). 2. Disallow forced-update 3. Changelog/QA stuff 4. Existing checks for commits, like metadata.xml (In reply to Robin Johnson from comment #12) > (In reply to Michał Górny from comment #10) > > Created attachment 384710 [details] > ... > > This assumes that GPG verification infra is in place, i.e. something else > > fetches, trusts and revokes keys as necessary. > I don't think this catches some of the things that the other validation > script does: > https://github.com/isislovecruft/scripts/blob/master/check-commit-signature > The only differences in signature validation I see are: * collaborators fingerprints are hardcoded, which I don't think makes sense for our use case. We are not randomly pulling gpg keys from servers, are we? * uses fragile grep/cut methods to extract the key id (--pretty=format:'%GK' prints the long key id here, so what's the problem?) * calls gpg manually instead of using --pretty=format:'%G?', because the fingerprints are hardcoded. So that's a lot of additional code that's unnecessary if we don't hardcode fingerprints and rely on git to verify the signatures. (In reply to Robin Johnson from comment #12) > 1.1. Since we are allowing unsigned branches, we must validate entire > branches on merge, ALL commits therein need to be signed (otherwise we are > open to the same substitution attack). I could be misunderstanding you, but I don't see a reason to sign or cryptographyically valiate branch 'B' of a merge. The only important point is the merge commit which indicates that branch 'B' has been reviewed by the developer. The posted script skips those branches and will only check the signatures of branch 'A' which also holds the merge commit. How would you be able to exploit this fact, unless you find a way to * inject code into branch 'B' without changing any of the SHA1 ids * somehow force push that to origin (which is not possible) (In reply to Robin Johnson from comment #12) > (In reply to Michał Górny from comment #10) > > Created attachment 384710 [details] > ... > > This assumes that GPG verification infra is in place, i.e. something else > > fetches, trusts and revokes keys as necessary. > I don't think this catches some of the things that the other validation > script does: > https://github.com/isislovecruft/scripts/blob/master/check-commit-signature Not really. The difference is that mine reuses GPG features, while the other reinvents the wheel, poorly. > Also, we can have multiple scripts, and I'm wondering if it would be best > to seperate the checks in them: > 1. If master or other trust-required branch: Validate signature (probably > using a modified check-commit-signature, because we need to check against a > list of allowed keys) > 1.1. Since we are allowing unsigned branches, we must validate entire > branches on merge, ALL commits therein need to be signed (otherwise we are > open to the same substitution attack). > 2. Disallow forced-update > 3. Changelog/QA stuff > 4. Existing checks for commits, like metadata.xml It may be feasible to do so. However, I would start with putting everything into one hook first and seeing how splittable it ends up. (In reply to Julian Ospald (hasufell) from comment #14) > (In reply to Robin Johnson from comment #12) > > 1.1. Since we are allowing unsigned branches, we must validate entire > > branches on merge, ALL commits therein need to be signed (otherwise we are > > open to the same substitution attack). > > I could be misunderstanding you, but I don't see a reason to sign or > cryptographyically valiate branch 'B' of a merge. The only important point > is the merge commit which indicates that branch 'B' has been reviewed by the > developer. > > The posted script skips those branches and will only check the signatures of > branch 'A' which also holds the merge commit. > > How would you be able to exploit this fact, unless you find a way to > * inject code into branch 'B' without changing any of the SHA1 ids I'm concerned about the existing collision and and chosen-prefix attacks against SHA1 being used here, just like the Flame malware successfully used a chosen-prefix against MD5 in a code-signing cert. Here's a chosen-prefix attack against our repo: 1. Attacker constructs a init.d script, regular part at the start, malicious part at the end 1.1. This would be fairly simple, just construct two start() functions, one of which is mundane, the other is malicious. 1.2. Both variants of the script have the same SHA1... 2. Attacker gets the the script committed, unmodified, by a developer to that dev's branch, with copy A that is entirely innocuous. 2.1. Let this script be stored as blob B, referenced by H(B) = ...sha1-id... 2.2. The developer is lazy and doesn't sign this commit, because it's on their own branch. 3. Dev merges to the master branch, with only the merge commit signed; not the prior commit. 4. Attacker breaks into read-only git mirror, and replaces the mundane variant of blob in the git pack with a malicious variant. 5. All new clones/pulls that fetch that blob will contain the exploit. 6. Profit. Counter-measures: a. Require commits to ALL branches to be signed by a dev. b. Require ALL commits on the master branch to be signed by a dev. Well, I'm not a crypto specialist but if the blob containing the file has the same hash, the commit doesn't change. And if the commit doesn't change, the signature is still valid. (In reply to Michał Górny from comment #17) > Well, I'm not a crypto specialist but if the blob containing the file has > the same hash, the commit doesn't change. And if the commit doesn't change, > the signature is still valid. The Git commit-signing design explicitly signs the entire commit, including blob contents, to avoid this security problem. The catch is that merge commits only have references and a changelog, not any file content. (In reply to Robin Johnson from comment #18) > The Git commit-signing design explicitly signs the entire commit, including > blob contents, to avoid this security problem. The Git gommit-signing design explicitly signs the commit, which contains the tree SHA1. It does not re-hash all of the subtrees and blobs underneath that tree [1]. However, constructing both a useful blob *and* a malicious blob that share the same SHA1 is a lot harder than just coming up with some gibberish that shares a SHA1 with a good blob [2]. [1]: http://mid.gmane.org/robbat2-20120603T073705-606889647Z@orbis-terrarum.net [2]: http://thread.gmane.org/gmane.comp.version-control.git/210320 We have signed commits now and sigs are required on most repos. |