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

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

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

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

Conflicts