Fixes for verify-commits script #7713

pull petertodd wants to merge 6 commits into bitcoin:master from petertodd:2016-03-fix-verify-commits changing 6 files +39 −24
  1. petertodd commented at 10:19 AM on March 18, 2016: contributor

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

  2. jonasschnelli added the label Dev Scripts on Mar 18, 2016
  3. MarcoFalke commented at 12:13 PM on March 18, 2016: member

    utACK 6d1016f

  4. laanwj commented at 5:45 PM on March 18, 2016: member

    utACK

  5. in contrib/verify-commits/verify-commits.sh:None in 6d1016f543 outdated 
       0 | @@ -1,9 +1,7 @@
   1 | -#!/bin/sh
   2 | +#!/bin/bash
   3 |  
   4 | -DIR=$(dirname "$0")
   5 | -
   6 | -echo "Please verify all commits in the following list are not evil:"
    laanwj commented at 5:46 PM on March 18, 2016:

    Did you intend to remove this message?

    MarcoFalke commented at 5:55 PM on March 18, 2016:

    Yes, it's the third commit.

  6. laanwj commented at 5:46 PM on March 18, 2016: member

    Ping @TheBlueMatt

  7. TheBlueMatt commented at 10:34 PM on March 21, 2016: member

    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.

  8. TheBlueMatt commented at 10:40 PM on March 21, 2016: member

    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.

  10. petertodd commented at 5:08 AM on March 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.

  11. sipa commented at 7:40 AM on May 20, 2016: member

    utACK 6d1016f543837fe3c7b0a48573771f4b17e35ff5

  12. petertodd force-pushed on May 20, 2016
  13. petertodd commented at 7:59 AM on May 20, 2016: contributor

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

  14. gmaxwell commented at 8:17 AM on May 20, 2016: contributor

    ACK e2764cb23826908bde171067fc5d0b0659b18952

  15. MarcoFalke commented at 8:25 AM on May 20, 2016: member

    utACK e2764cb

  16. TheBlueMatt commented at 11:30 PM on May 20, 2016: member

    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.

  17. petertodd commented at 8:23 AM on 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.

  18. TheBlueMatt commented at 8:50 AM on May 21, 2016: member

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

  19. Make verify-commits POSIX-compliant f7d4a25fe6
  20. Make verify-commits path-independent 9523e8adaf
  21. 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.
    22421faa19
  22. Remove keys that are no longer used for merging
    Also updated trusted git root to be right after gmaxwell's last merge.
    11164ec0b4
  23. petertodd force-pushed on May 21, 2016
  24. petertodd commented at 9:30 AM on May 21, 2016: contributor

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

  25. gmaxwell commented at 10:27 AM on May 21, 2016: contributor

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

  26. TheBlueMatt commented at 8:03 PM on May 21, 2016: member

    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 11164ec0b4c1790059220b09b5827e5618a46c76

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

  27. petertodd commented at 8:40 AM on 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.

  28. TheBlueMatt commented at 11:06 PM on May 22, 2016: member

    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.

  29. laanwj commented at 10:58 AM on 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.

  30. petertodd force-pushed on Jun 9, 2016
  31. petertodd commented at 5:57 PM on June 9, 2016: contributor

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

  32. Add README for verify-commits 966151e71d
  33. petertodd force-pushed on Jun 9, 2016
  34. sipa commented at 2:47 PM on June 11, 2016: member

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

  35. MarcoFalke commented at 10:25 AM on June 16, 2016: member

    Anything left to do here?

  36. petertodd commented at 10:53 AM on June 16, 2016: contributor

    @marcofalke IMO it's ready for merging.

  37. TheBlueMatt commented at 9:54 PM on June 16, 2016: member

    "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)

  38. TheBlueMatt commented at 10:01 PM on June 16, 2016: member

    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)

  39. laanwj commented at 10:57 AM on June 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?

  40. MarcoFalke commented at 11:01 AM on June 17, 2016: member

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

  41. TheBlueMatt commented at 4:18 AM on June 18, 2016: member

    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)

  42. in contrib/verify-commits/trusted-keys:None in 966151e71d outdated 
       0 | @@ -1,8 +1,5 @@
   1 |  71A3B16735405025D447E8F274810B012346C9A6
   2 |  1F4410F6A89268CE3197A84C57896D2FF8F0B657
    MarcoFalke commented at 12:33 PM on June 18, 2016:

    Nit: I think you can remove this line and clear allow-revsig-commits.

    petertodd commented at 6:55 PM on June 18, 2016:

    Sorry, which line specifically?

    Good point re: allow-revsig

    MarcoFalke commented at 7:05 PM on June 18, 2016:

    gpg --list-key --fingerprint 1F4410F6A89268CE3197A84C57896D2FF8F0B657 returns revoked key by @sipa. Once the commits signed by this key are buried under the trusted root, you could get rid of those lines in allow-revsig-commits.

  43. petertodd commented at 6:58 PM on June 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).

  44. 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
    1e9aab0dbf
  45. petertodd commented at 12:55 AM on June 19, 2016: contributor

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

  46. gmaxwell commented at 1:15 AM on June 19, 2016: contributor

    ACK 1e9aab0dbfcb844458b5f221a9af0141bba6280f

    Readme seems fine to me.

  47. MarcoFalke commented at 9:37 AM on June 19, 2016: member

    utACK 1e9aab0.

    I think the readme could be fixed later, if desired.

  48. laanwj merged this on Jun 20, 2016
  49. laanwj closed this on Jun 20, 2016
  50. laanwj referenced this in commit f6598df765 on Jun 20, 2016
  51. codablock referenced this in commit 35ea4cd1f3 on Sep 16, 2017
  52. codablock referenced this in commit 8ce4211f1b on Sep 19, 2017
  53. codablock referenced this in commit 429b84734b on Dec 27, 2017
  54. codablock referenced this in commit 573d9314e8 on Dec 28, 2017
  55. andvgal referenced this in commit b0dadd4b7d on Jan 6, 2019
  56. DrahtBot locked this on Sep 8, 2021
Contributors
petertoddMarcoFalkelaanwjTheBlueMattsipagmaxwell
Labels
Scripts and tools
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 12:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me