tests: add a test-security target and run it in CI #18434

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:run_security_check_ci changing 8 files +27 −4

fanquake commented at 3:52 AM on March 26, 2020: member

Wladimir asked about running the test-security-check.py script in our CI. This PR adds a target for that: make test-security and adds it to a few CI jobs.
fanquake added the label Build system on Mar 26, 2020
fanquake added the label Tests on Mar 26, 2020
fanquake force-pushed on Mar 26, 2020
fanquake force-pushed on Mar 26, 2020
DrahtBot commented at 7:35 AM on March 26, 2020: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #19068 (build: Use a zip instead of dmg for macOS releases by fanquake)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in contrib/devtools/test-security-check.py:24 in 210bbfdd78 outdated

  20 | @@ -21,7 +21,7 @@ def write_testcode(filename):
  21 |  
  22 |  def call_security_check(cc, source, executable, options):
  23 |      subprocess.check_call([cc,source,'-o',executable] + options)
  24 | -    p = subprocess.Popen(['./security-check.py',executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True)
  25 | +    p = subprocess.Popen(['../contrib/devtools/security-check.py',executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True)

MarcoFalke commented at 1:28 PM on March 26, 2020:

This will break when the distdir is more than one folder deep. I'd prefer if the script was linked like everything else (test_runner.py, etc)

practicalswift commented at 5:05 PM on March 26, 2020: contributor

Concept ACK: enforcement via automation is good
MarcoFalke commented at 5:41 PM on March 26, 2020: member

The same is already run via gitian, right?
DrahtBot commented at 7:31 PM on April 17, 2020: member



🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot added the label Needs rebase on Apr 17, 2020
fanquake referenced this in commit c90a9e6fff on Apr 22, 2020
laanwj commented at 6:12 PM on May 14, 2020: member

Concept ACK

in ci/test/00_setup_env_native_tsan.sh:13 in 210bbfdd78 outdated

   9 | @@ -10,6 +10,7 @@ export CONTAINER_NAME=ci_native_tsan
  10 |  export DOCKER_NAME_TAG=ubuntu:16.04
  11 |  export PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libqrencode-dev"
  12 |  export NO_DEPENDS=1
  13 | +export RUN_SECURITY_TESTS=false

MarcoFalke commented at 10:28 PM on May 14, 2020:

Instead of setting it to false in every config file, why not set it to false by default and only explicitly set it to true in the one config that needs it?

If every config has to specify the value, it sort of defeats the purpose of the default value.

fanquake commented at 3:41 AM on May 15, 2020:

Have fixed this up.

fanquake force-pushed on May 15, 2020
fanquake removed the label Needs rebase on May 15, 2020
fanquake force-pushed on May 15, 2020
fanquake commented at 3:43 AM on May 15, 2020: member

Have rebased and should have fixed up all the issues here.

The same is already run via gitian, right?

We run the symbol and security checks in gitian, but not the security-check test.
MarcoFalke commented at 10:17 AM on May 15, 2020: member

Concept ACK (have not reviewed the code beside a cursory glance on the ci config files)
DrahtBot commented at 11:09 AM on May 22, 2020: member



🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot added the label Needs rebase on May 22, 2020
fanquake force-pushed on May 22, 2020
fanquake removed the label Needs rebase on May 22, 2020
fanquake force-pushed on May 22, 2020
fanquake requested review from laanwj on May 23, 2020
fjahr commented at 11:47 AM on June 12, 2020: member

Concept ACK

Also looked at the code and it looks good to me, but I don't feel qualified enough in this area to give a full ACK at the moment.
practicalswift commented at 1:08 PM on June 12, 2020: contributor

ACK cb3be57a83aa2065061e6cb77cec16167f133d3c -- patch looks correct and Travis is happy
tests: run test-security-check.py in CI 968aaae940
test: use subprocess.run() in test-security-check.py 9fe71a57a6

in Makefile.am:346 in cb3be57a83 outdated

 341 | @@ -342,3 +342,14 @@ clean-local: clean-docs
 342 |  	rm -rf coverage_percent.txt test_bitcoin.coverage/ total.coverage/ fuzz.coverage/ test/tmp/ cache/ $(OSX_APP)
 343 |  	rm -rf test/functional/__pycache__ test/functional/test_framework/__pycache__ test/cache share/rpcauth/__pycache__
 344 |  	rm -rf osx_volname dist/ dpi36.background.tiff dpi72.background.tiff
 345 | +
 346 | +test-security:

laanwj commented at 5:32 PM on June 15, 2020:

Maybe test-security-check target instead of test-security? I mean, it's a test for the binary security check. It doesn't check the security of the actual binaries.

fanquake commented at 11:58 AM on June 16, 2020:

Sure. I've updated the target to test-security-check.

fanquake force-pushed on Jun 16, 2020
laanwj commented at 4:20 PM on June 16, 2020: member

ACK 9fe71a57a6780569e618cf9a8d4f1acf6321017f
laanwj merged this on Jun 16, 2020
laanwj closed this on Jun 16, 2020
fanquake deleted the branch on Jun 17, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18415 scripts: add MACHO tests to test-security-check.py #18713 scripts: Add MACHO stack canary check to security-check.py

tests: add a test-security target and run it in CI #18434

Conflicts