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

Fixes for verify-commits script #7713

Merged
merged 6 commits into from Jun 20, 2016

Conversation

Projects
None yet
7 participants
@petertodd
Contributor

petertodd commented Mar 18, 2016

Tried to use it for another project and ran into some issues getting it to work on Debian 8

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 18, 2016

Member

utACK 6d1016f

Member

MarcoFalke commented Mar 18, 2016

utACK 6d1016f

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 18, 2016

Member

utACK

Member

laanwj commented Mar 18, 2016

utACK

@laanwj

View changes

Show outdated Hide outdated contrib/verify-commits/verify-commits.sh
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj
Member

laanwj commented Mar 18, 2016

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Mar 21, 2016

Contributor

Which features are bash-specific (I'm pretty sure we went through this when it was merged, and there were none left)? We should just remove the bash-specific ones, not revert to forcing bash.

Contributor

TheBlueMatt commented Mar 21, 2016

Which features are bash-specific (I'm pretty sure we went through this when it was merged, and there were none left)? We should just remove the bash-specific ones, not revert to forcing bash.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Mar 21, 2016

Contributor

Also, I do prefer to not remove the commits list message...its not there to prevent someone who has malicious access to the repo, its there to encourage people to audit the commits that were merged. If you prefer, just replace with it a notice to tell people to run git log on contrib/verify-commits manually.

Contributor

TheBlueMatt commented Mar 21, 2016

Also, I do prefer to not remove the commits list message...its not there to prevent someone who has malicious access to the repo, its there to encourage people to audit the commits that were merged. If you prefer, just replace with it a notice to tell people to run git log on contrib/verify-commits manually.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Mar 23, 2016

Contributor

@TheBlueMatt Again, if you want to encourage people, do it in a README file or similar which doesn't give the wrong impression about what's the right way to do it.

Anyway, the necessity to audit the codebase in general should be something every security conscious developer should be aware of; calling out this program specifically may in itself give people the wrong impression.

Contributor

petertodd commented Mar 23, 2016

@TheBlueMatt Again, if you want to encourage people, do it in a README file or similar which doesn't give the wrong impression about what's the right way to do it.

Anyway, the necessity to audit the codebase in general should be something every security conscious developer should be aware of; calling out this program specifically may in itself give people the wrong impression.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 20, 2016

Member

utACK 6d1016f543837fe3c7b0a48573771f4b17e35ff5

Member

sipa commented May 20, 2016

utACK 6d1016f543837fe3c7b0a48573771f4b17e35ff5

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 20, 2016

Contributor

@sipa Sorry, just rebased and added one more commit.

Contributor

petertodd commented May 20, 2016

@sipa Sorry, just rebased and added one more commit.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 20, 2016

Member

ACK e2764cb23826908bde171067fc5d0b0659b18952

Member

gmaxwell commented May 20, 2016

ACK e2764cb23826908bde171067fc5d0b0659b18952

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 20, 2016

Member

utACK e2764cb

Member

MarcoFalke commented May 20, 2016

utACK e2764cb

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt May 20, 2016

Contributor

NACK. Please fix the two bash-isms instead of requiring bash, and avoid adding more OS-specificisms with GNU-isms (which aren't even in some BSDs). Feel free to move the warnings to a README file of somesort, though.

Contributor

TheBlueMatt commented May 20, 2016

NACK. Please fix the two bash-isms instead of requiring bash, and avoid adding more OS-specificisms with GNU-isms (which aren't even in some BSDs). Feel free to move the warnings to a README file of somesort, though.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 21, 2016

Contributor

@TheBlueMatt I don't know enough about bash to know what needs to be fixed or how; if you'd like to suggest some changes please do. But if it's not going to get fixed I'd rather the failure be clear - "bash is needed and isn't installed" - than unclear.

Contributor

petertodd commented May 21, 2016

@TheBlueMatt I don't know enough about bash to know what needs to be fixed or how; if you'd like to suggest some changes please do. But if it's not going to get fixed I'd rather the failure be clear - "bash is needed and isn't installed" - than unclear.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt May 21, 2016

Contributor

The three commits at https://github.com/TheBlueMatt/bitcoin/commits/2016-20-7713-fixes should replace your top three just fine.

Contributor

TheBlueMatt commented May 21, 2016

The three commits at https://github.com/TheBlueMatt/bitcoin/commits/2016-20-7713-fixes should replace your top three just fine.

TheBlueMatt and others added some commits May 21, 2016

Remove pointless warning
Any attacker who managed to make an evil commit that changed something in the
contrib/verify-commits/ directory could just as easily remove the warning
and/or modify it to not display the evil commits; telling the user to check
those commits specifically misleads them into checking just those commits
rather than the script itself.
Remove keys that are no longer used for merging
Also updated trusted git root to be right after gmaxwell's last merge.
@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 21, 2016

Contributor

Incorporated @TheBlueMatt's /bin/sh fixes, so no longer depending on bash again.

Contributor

petertodd commented May 21, 2016

Incorporated @TheBlueMatt's /bin/sh fixes, so no longer depending on bash again.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell May 21, 2016

Member

reACK 11164ec (is it me, or is it obviously slower now?)

Member

gmaxwell commented May 21, 2016

reACK 11164ec (is it me, or is it obviously slower now?)

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt May 21, 2016

Contributor

Please do leave the warning telling people to check git log in... making people see it every time until it's removed definitely has value. If not it should be added to a README somewhere.

On May 21, 2016 3:27:17 AM PDT, Gregory Maxwell notifications@github.com wrote:

reACK 11164ec


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

Contributor

TheBlueMatt commented May 21, 2016

Please do leave the warning telling people to check git log in... making people see it every time until it's removed definitely has value. If not it should be added to a README somewhere.

On May 21, 2016 3:27:17 AM PDT, Gregory Maxwell notifications@github.com wrote:

reACK 11164ec


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 22, 2016

Contributor

@TheBlueMatt Git has had vulnerabilities before (example) where even just checking out source is sufficient to pwn your system; if you can run git log on the verify-commits directory it may be too late. If we're going to be giving advice on how to use verify-commits we should do it properly; for now removing that advice is very appropriate given that it's misleading at best.

Contributor

petertodd commented May 22, 2016

@TheBlueMatt Git has had vulnerabilities before (example) where even just checking out source is sufficient to pwn your system; if you can run git log on the verify-commits directory it may be too late. If we're going to be giving advice on how to use verify-commits we should do it properly; for now removing that advice is very appropriate given that it's misleading at best.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt May 22, 2016

Contributor

I find it rather unlikely that git log would have a vulnerability that cant also be triggered by some other checkout/pull/etc flow. I dont think telling people to run git log is bad advice here.
I agree the existing auto-run-git log isnt ideal, but I dont see why telling people to do so is bad, let alone why removing it entirely is better.

Contributor

TheBlueMatt commented May 22, 2016

I find it rather unlikely that git log would have a vulnerability that cant also be triggered by some other checkout/pull/etc flow. I dont think telling people to run git log is bad advice here.
I agree the existing auto-run-git log isnt ideal, but I dont see why telling people to do so is bad, let alone why removing it entirely is better.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 30, 2016

Member

Please add the warning back in. Seems otherwise we don't get agreement here to merge this. Its removal can be discussed separately.

Member

laanwj commented May 30, 2016

Please add the warning back in. Seems otherwise we don't get agreement here to merge this. Its removal can be discussed separately.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jun 9, 2016

Contributor

Added README explaining how to use verify-commits.sh safely; this should answer @TheBlueMatt's objection to removing the warning.

Contributor

petertodd commented Jun 9, 2016

Added README explaining how to use verify-commits.sh safely; this should answer @TheBlueMatt's objection to removing the warning.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 11, 2016

Member

Tested ACK (checked out this branch in one worktree, master in another, verified master using the code from this branch).

Member

sipa commented Jun 11, 2016

Tested ACK (checked out this branch in one worktree, master in another, verified master using the code from this branch).

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 16, 2016

Member

Anything left to do here?

Member

MarcoFalke commented Jun 16, 2016

Anything left to do here?

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jun 16, 2016

Contributor

@MarcoFalke IMO it's ready for merging.

Contributor

petertodd commented Jun 16, 2016

@MarcoFalke IMO it's ready for merging.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 16, 2016

Contributor

"This is an incomplete work in progress,"

It is? I'd say it works pretty well for its goals?

On June 16, 2016 3:53:32 AM PDT, Peter Todd notifications@github.com wrote:

@MarcoFalke IMO it's ready for merging.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

Contributor

TheBlueMatt commented Jun 16, 2016

"This is an incomplete work in progress,"

It is? I'd say it works pretty well for its goals?

On June 16, 2016 3:53:32 AM PDT, Peter Todd notifications@github.com wrote:

@MarcoFalke IMO it's ready for merging.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 16, 2016

Contributor

Also you might want to note that you can run git log with paths to get a list of changes to that path in the README, just because it's useful for these scripts and not really an obvious feature that everyone is aware of.

On June 16, 2016 3:53:32 AM PDT, Peter Todd notifications@github.com wrote:

@MarcoFalke IMO it's ready for merging.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

Contributor

TheBlueMatt commented Jun 16, 2016

Also you might want to note that you can run git log with paths to get a list of changes to that path in the README, just because it's useful for these scripts and not really an obvious feature that everyone is aware of.

On June 16, 2016 3:53:32 AM PDT, Peter Todd notifications@github.com wrote:

@MarcoFalke IMO it's ready for merging.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 17, 2016

Member

Can we please finish this?
This is pretty much a trivial change, but every time there comes up a new load of nits, which of those things is critical and can't be fixed later?

Member

laanwj commented Jun 17, 2016

Can we please finish this?
This is pretty much a trivial change, but every time there comes up a new load of nits, which of those things is critical and can't be fixed later?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 17, 2016

Member

@laanwj I think we can improve the wording of the doc later.

Member

MarcoFalke commented Jun 17, 2016

@laanwj I think we can improve the wording of the doc later.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 18, 2016

Contributor

I mean I think the README is just wrong right now? We could have also taken the easy solution of splitting out the contentious change.

On June 17, 2016 4:01:23 AM PDT, MarcoFalke notifications@github.com wrote:

@laanwj I think we can improve the wording of the doc later.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

Contributor

TheBlueMatt commented Jun 18, 2016

I mean I think the README is just wrong right now? We could have also taken the easy solution of splitting out the contentious change.

On June 17, 2016 4:01:23 AM PDT, MarcoFalke notifications@github.com wrote:

@laanwj I think we can improve the wording of the doc later.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7713 (comment)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jun 18, 2016

Contributor

@TheBlueMatt Sorry, but I'm kinda mystified why you think the README is wrong; I don't see it as controversial at all to describe this functionality as incomplete, given that we definitely need a convenient way for users to verify commits safely, prior to accidentally checking out repos and possibly running malicious code. I think it's good to point that out in the hope that others continue this starting point - and it's work that many other projects need as well (I personally use verify-commits in python-bitcoinlib, as well as some internal-use-only repos).

Contributor

petertodd commented Jun 18, 2016

@TheBlueMatt Sorry, but I'm kinda mystified why you think the README is wrong; I don't see it as controversial at all to describe this functionality as incomplete, given that we definitely need a convenient way for users to verify commits safely, prior to accidentally checking out repos and possibly running malicious code. I think it's good to point that out in the hope that others continue this starting point - and it's work that many other projects need as well (I personally use verify-commits in python-bitcoinlib, as well as some internal-use-only repos).

Remove sipa's old revoked key from verify-commits
Now that the trusted root is past all commits signed by that key we don't need
it in the trusted-keys list, nor do we need to whitelist those commits in
allow-revsig-commits
@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jun 19, 2016

Contributor

@MarcoFalke Fixed your nit re: sipa's old key.

Contributor

petertodd commented Jun 19, 2016

@MarcoFalke Fixed your nit re: sipa's old key.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 19, 2016

Member

ACK 1e9aab0

Readme seems fine to me.

Member

gmaxwell commented Jun 19, 2016

ACK 1e9aab0

Readme seems fine to me.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 19, 2016

Member
Member

MarcoFalke commented Jun 19, 2016

@laanwj laanwj merged commit 1e9aab0 into bitcoin:master Jun 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 20, 2016

Merge #7713: Fixes for verify-commits script
1e9aab0 Remove sipa's old revoked key from verify-commits (Peter Todd)
966151e Add README for verify-commits (Peter Todd)
11164ec Remove keys that are no longer used for merging (Peter Todd)
22421fa Remove pointless warning (Peter Todd)
9523e8a Make verify-commits path-independent (Matt Corallo)
f7d4a25 Make verify-commits POSIX-compliant (Matt Corallo)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge bitcoin#7713: Fixes for verify-commits script
1e9aab0 Remove sipa's old revoked key from verify-commits (Peter Todd)
966151e Add README for verify-commits (Peter Todd)
11164ec Remove keys that are no longer used for merging (Peter Todd)
22421fa Remove pointless warning (Peter Todd)
9523e8a Make verify-commits path-independent (Matt Corallo)
f7d4a25 Make verify-commits POSIX-compliant (Matt Corallo)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge bitcoin#7713: Fixes for verify-commits script
1e9aab0 Remove sipa's old revoked key from verify-commits (Peter Todd)
966151e Add README for verify-commits (Peter Todd)
11164ec Remove keys that are no longer used for merging (Peter Todd)
22421fa Remove pointless warning (Peter Todd)
9523e8a Make verify-commits path-independent (Matt Corallo)
f7d4a25 Make verify-commits POSIX-compliant (Matt Corallo)

codablock added a commit to codablock/dash that referenced this pull request Dec 27, 2017

Merge bitcoin#7713: Fixes for verify-commits script
1e9aab0 Remove sipa's old revoked key from verify-commits (Peter Todd)
966151e Add README for verify-commits (Peter Todd)
11164ec Remove keys that are no longer used for merging (Peter Todd)
22421fa Remove pointless warning (Peter Todd)
9523e8a Make verify-commits path-independent (Matt Corallo)
f7d4a25 Make verify-commits POSIX-compliant (Matt Corallo)

codablock added a commit to codablock/dash that referenced this pull request Dec 28, 2017

Merge bitcoin#7713: Fixes for verify-commits script
1e9aab0 Remove sipa's old revoked key from verify-commits (Peter Todd)
966151e Add README for verify-commits (Peter Todd)
11164ec Remove keys that are no longer used for merging (Peter Todd)
22421fa Remove pointless warning (Peter Todd)
9523e8a Make verify-commits path-independent (Matt Corallo)
f7d4a25 Make verify-commits POSIX-compliant (Matt Corallo)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment