From 3b4b44143b0d0742139b988d589ce7cac1365ff6 Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 08:18:24 -0800 Subject: [PATCH 01/10] Don't kill all processes when testing os.kill --- app/tests/api/logging/test_audit.py | 36 +++++++++++++++++------------ 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/app/tests/api/logging/test_audit.py b/app/tests/api/logging/test_audit.py index 1dac8463..52691ab1 100644 --- a/app/tests/api/logging/test_audit.py +++ b/app/tests/api/logging/test_audit.py @@ -40,18 +40,6 @@ def init_audit_hook(): ], id="open", ), - pytest.param( - os.kill, - (-1, signal.SIGTERM), # Using PID=-1 since it should not ever be a valid PID - [ - { - "msg": "os.kill", - "audit.args.pid": -1, - "audit.args.sig": signal.SIGTERM, - } - ], - id="os.kill", - ), pytest.param( os.rename, ("/tmp/oldname", "/tmp/newname"), @@ -154,10 +142,28 @@ def test_audit_hook( assert_record_match(record, expected_record) +def test_os_kill(init_audit_hook, caplog: pytest.LogCaptureFixture): + # Start a process to kill + process = subprocess.Popen("ls") + os.kill(process.pid, signal.SIGTERM) + + expected_records = [ + {"msg": "subprocess.Popen"}, + { + "msg": "os.kill", + "audit.args.pid": process.pid, + "audit.args.sig": signal.SIGTERM, + }, + ] + + assert len(caplog.records) == len(expected_records) + for record, expected_record in zip(caplog.records, expected_records): + assert record.levelname == "AUDIT" + assert_record_match(record, expected_record) + + def test_do_not_log_popen_env( - init_audit_hook, - caplog: pytest.LogCaptureFixture, - monkeypatch: pytest.MonkeyPatch, + init_audit_hook, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch ): monkeypatch.setenv("FOO", "SENSITIVE-DATA") subprocess.Popen(["ls"], env=os.environ) From ae1899840fd69e51fe97af01d23d9c47b4ad2fa1 Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 08:26:01 -0800 Subject: [PATCH 02/10] Use cat instead of ls to avoid race condition --- app/tests/api/logging/test_audit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/tests/api/logging/test_audit.py b/app/tests/api/logging/test_audit.py index 52691ab1..a9dc7654 100644 --- a/app/tests/api/logging/test_audit.py +++ b/app/tests/api/logging/test_audit.py @@ -144,7 +144,7 @@ def test_audit_hook( def test_os_kill(init_audit_hook, caplog: pytest.LogCaptureFixture): # Start a process to kill - process = subprocess.Popen("ls") + process = subprocess.Popen("cat") os.kill(process.pid, signal.SIGTERM) expected_records = [ From 3eec7e28b2b20969122c264425c46ea6a24ba943 Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 08:49:18 -0800 Subject: [PATCH 03/10] Isolate audit tests from rest of tests --- .github/workflows/ci-app.yml | 5 ++++- app/Makefile | 9 ++++++--- app/pyproject.toml | 3 +++ app/tests/api/logging/test_audit.py | 8 ++++++-- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci-app.yml b/.github/workflows/ci-app.yml index 4ba72a0f..a5d99be5 100644 --- a/.github/workflows/ci-app.yml +++ b/.github/workflows/ci-app.yml @@ -36,4 +36,7 @@ jobs: run: make lint-security - name: Start tests - run: make test-coverage + run: | + make test-audit + make test-coverage + diff --git a/app/Makefile b/app/Makefile index aa671907..0e7ec538 100644 --- a/app/Makefile +++ b/app/Makefile @@ -121,14 +121,17 @@ db-migrate-heads: ## Show migrations marked as a head # Testing ################################################## -test: - $(PY_RUN_CMD) pytest $(args) +test: ## Run all tests except for audit logging tests + $(PY_RUN_CMD) pytest -m "not audit" $(args) + +test-audit: ## Run audit logging tests + $(PY_RUN_CMD) pytest -m "audit" $(args) test-watch: $(PY_RUN_CMD) pytest-watch --clear $(args) test-coverage: - $(PY_RUN_CMD) coverage run --branch --source=api -m pytest $(args) + $(PY_RUN_CMD) coverage run --branch --source=api -m pytest -m "not audit" $(args) $(PY_RUN_CMD) coverage report test-coverage-report: ## Open HTML test coverage report diff --git a/app/pyproject.toml b/app/pyproject.toml index a57cf841..08aa68e8 100644 --- a/app/pyproject.toml +++ b/app/pyproject.toml @@ -100,5 +100,8 @@ filterwarnings = [ "ignore::DeprecationWarning:apispec.*", "ignore::DeprecationWarning:certifi.*"] # pytest-watch errors if the closing bracket is on it's own line +markers = [ + "audit: mark a test as a security audit log test, to be run isolated from other tests"] + [tool.coverage.run] omit = ["api/db/migrations/*.py"] diff --git a/app/tests/api/logging/test_audit.py b/app/tests/api/logging/test_audit.py index a9dc7654..874c9a9b 100644 --- a/app/tests/api/logging/test_audit.py +++ b/app/tests/api/logging/test_audit.py @@ -18,6 +18,12 @@ import api.logging.audit as audit +# Do not run these tests alongside the rest of the test suite since +# this tests adds an audit hook that interfere with other tests, +# and at the time of writing there isn't a known way to remove +# audit hooks. +pytestmark = pytest.mark.audit + @pytest.fixture(scope="session") def init_audit_hook(): @@ -35,7 +41,6 @@ def init_audit_hook(): "msg": "open", "audit.args.path": "/dev/null", "audit.args.mode": "w", - "audit.args.flags": 524865, } ], id="open", @@ -72,7 +77,6 @@ def init_audit_hook(): "msg": "open", "audit.args.path": "/dev/null", "audit.args.mode": None, - "audit.args.flags": 524354, } ], id="os.open", From da712bccd70d21d7895e54e4f25e35589c0c822b Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 09:38:07 -0800 Subject: [PATCH 04/10] Set db environment variables --- app/tests/api/adapters/db/test_db.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/app/tests/api/adapters/db/test_db.py b/app/tests/api/adapters/db/test_db.py index 1c24d861..3726b965 100644 --- a/app/tests/api/adapters/db/test_db.py +++ b/app/tests/api/adapters/db/test_db.py @@ -75,14 +75,16 @@ def test_make_connection_uri(username_password_port, expected): ) -def test_get_connection_parameters_require_environment(monkeypatch: pytest.MonkeyPatch): +# Include db_client fixture to set environment variables needed by get_db_config +def test_get_connection_parameters_require_environment(db_client, monkeypatch: pytest.MonkeyPatch): monkeypatch.delenv("ENVIRONMENT") db_config = get_db_config() with pytest.raises(Exception, match="ENVIRONMENT is not set"): get_connection_parameters(db_config) -def test_get_connection_parameters(monkeypatch: pytest.MonkeyPatch): +# Include db_client fixture to set environment variables needed by get_db_config +def test_get_connection_parameters(db_client, monkeypatch: pytest.MonkeyPatch): monkeypatch.setenv("ENVIRONMENT", "production") db_config = get_db_config() conn_params = get_connection_parameters(db_config) @@ -99,19 +101,22 @@ def test_get_connection_parameters(monkeypatch: pytest.MonkeyPatch): ) -def test_db_connection(): +# Include db_client fixture to set environment variables needed by get_db_config +def test_db_connection(db_client): db_client = db.init(check_db_connection=False) with db_client.get_connection() as conn: assert conn.scalar(text("SELECT 1")) == 1 -def test_check_db_connection(caplog): +# Include db_client fixture to set environment variables needed by get_db_config +def test_check_db_connection(db_client, caplog): db.init(check_db_connection=True) assert "database connection is not using SSL" in caplog.messages -def test_get_session(): - db_client = db.init(check_db_connection=False) - with db_client.get_session() as session: +# Include db_client fixture to set environment variables needed by get_db_config +def test_get_session(db_client): + client = db.init(check_db_connection=False) + with client.get_session() as session: with session.begin(): assert session.scalar(text("SELECT 1")) == 1 From dd085c6464c7a613e9806467c27a53e7607daa79 Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 12:05:17 -0800 Subject: [PATCH 05/10] Add autouse fixture to load local env vars instead --- app/tests/api/adapters/db/test_db.py | 10 +++++----- app/tests/conftest.py | 7 +++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/tests/api/adapters/db/test_db.py b/app/tests/api/adapters/db/test_db.py index 3726b965..c265609a 100644 --- a/app/tests/api/adapters/db/test_db.py +++ b/app/tests/api/adapters/db/test_db.py @@ -76,7 +76,7 @@ def test_make_connection_uri(username_password_port, expected): # Include db_client fixture to set environment variables needed by get_db_config -def test_get_connection_parameters_require_environment(db_client, monkeypatch: pytest.MonkeyPatch): +def test_get_connection_parameters_require_environment(monkeypatch: pytest.MonkeyPatch): monkeypatch.delenv("ENVIRONMENT") db_config = get_db_config() with pytest.raises(Exception, match="ENVIRONMENT is not set"): @@ -84,7 +84,7 @@ def test_get_connection_parameters_require_environment(db_client, monkeypatch: p # Include db_client fixture to set environment variables needed by get_db_config -def test_get_connection_parameters(db_client, monkeypatch: pytest.MonkeyPatch): +def test_get_connection_parameters(monkeypatch: pytest.MonkeyPatch): monkeypatch.setenv("ENVIRONMENT", "production") db_config = get_db_config() conn_params = get_connection_parameters(db_config) @@ -102,20 +102,20 @@ def test_get_connection_parameters(db_client, monkeypatch: pytest.MonkeyPatch): # Include db_client fixture to set environment variables needed by get_db_config -def test_db_connection(db_client): +def test_db_connection(): db_client = db.init(check_db_connection=False) with db_client.get_connection() as conn: assert conn.scalar(text("SELECT 1")) == 1 # Include db_client fixture to set environment variables needed by get_db_config -def test_check_db_connection(db_client, caplog): +def test_check_db_connection(caplog): db.init(check_db_connection=True) assert "database connection is not using SSL" in caplog.messages # Include db_client fixture to set environment variables needed by get_db_config -def test_get_session(db_client): +def test_get_session(): client = db.init(check_db_connection=False) with client.get_session() as session: with session.begin(): diff --git a/app/tests/conftest.py b/app/tests/conftest.py index e931992e..2d74fcb8 100644 --- a/app/tests/conftest.py +++ b/app/tests/conftest.py @@ -11,10 +11,17 @@ import api.logging import tests.api.db.models.factories as factories from api.db import models +from api.util.local import load_local_env_vars from tests.lib import db_testing logger = logging.getLogger(__name__) + +@pytest.fixture(autouse=True) +def env_vars(): + load_local_env_vars() + + #################### # Test DB session #################### From 2e3d39a6a0ca56d420275658b10a9bc006b3cf6c Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 12:06:30 -0800 Subject: [PATCH 06/10] Remove obsolete comment --- app/tests/api/adapters/db/test_db.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/tests/api/adapters/db/test_db.py b/app/tests/api/adapters/db/test_db.py index c265609a..778489d0 100644 --- a/app/tests/api/adapters/db/test_db.py +++ b/app/tests/api/adapters/db/test_db.py @@ -75,7 +75,6 @@ def test_make_connection_uri(username_password_port, expected): ) -# Include db_client fixture to set environment variables needed by get_db_config def test_get_connection_parameters_require_environment(monkeypatch: pytest.MonkeyPatch): monkeypatch.delenv("ENVIRONMENT") db_config = get_db_config() @@ -83,7 +82,6 @@ def test_get_connection_parameters_require_environment(monkeypatch: pytest.Monke get_connection_parameters(db_config) -# Include db_client fixture to set environment variables needed by get_db_config def test_get_connection_parameters(monkeypatch: pytest.MonkeyPatch): monkeypatch.setenv("ENVIRONMENT", "production") db_config = get_db_config() @@ -101,20 +99,17 @@ def test_get_connection_parameters(monkeypatch: pytest.MonkeyPatch): ) -# Include db_client fixture to set environment variables needed by get_db_config def test_db_connection(): db_client = db.init(check_db_connection=False) with db_client.get_connection() as conn: assert conn.scalar(text("SELECT 1")) == 1 -# Include db_client fixture to set environment variables needed by get_db_config def test_check_db_connection(caplog): db.init(check_db_connection=True) assert "database connection is not using SSL" in caplog.messages -# Include db_client fixture to set environment variables needed by get_db_config def test_get_session(): client = db.init(check_db_connection=False) with client.get_session() as session: From 49abc635877bf51d99246de91132cd137b276db8 Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 12:07:01 -0800 Subject: [PATCH 07/10] Revert var name --- app/tests/api/adapters/db/test_db.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/tests/api/adapters/db/test_db.py b/app/tests/api/adapters/db/test_db.py index 778489d0..1c24d861 100644 --- a/app/tests/api/adapters/db/test_db.py +++ b/app/tests/api/adapters/db/test_db.py @@ -111,7 +111,7 @@ def test_check_db_connection(caplog): def test_get_session(): - client = db.init(check_db_connection=False) - with client.get_session() as session: + db_client = db.init(check_db_connection=False) + with db_client.get_session() as session: with session.begin(): assert session.scalar(text("SELECT 1")) == 1 From 200d2ad31cb34e2d6e4fab79ecb77d25e178dfbb Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 12:54:45 -0800 Subject: [PATCH 08/10] Turn off log capture for failed tests by default --- app/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/Makefile b/app/Makefile index 7a73d8b1..dc798631 100644 --- a/app/Makefile +++ b/app/Makefile @@ -121,8 +121,10 @@ db-migrate-heads: ## Show migrations marked as a head # Testing ################################################## +# Turn off log capture for failed tests by default. +# To re-enable log capture, run `make test args="--show-capture=all"` test: ## Run all tests except for audit logging tests - $(PY_RUN_CMD) pytest -m "not audit" $(args) + $(PY_RUN_CMD) pytest -m "not audit" --show-capture=no $(args) test-audit: ## Run audit logging tests $(PY_RUN_CMD) pytest -m "audit" $(args) From 9b37c07bc909a8b3b60220f0a6e5ddcd77f25b45 Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 14:42:18 -0800 Subject: [PATCH 09/10] Update comment explaining log capture --- app/Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/Makefile b/app/Makefile index dc798631..9419537e 100644 --- a/app/Makefile +++ b/app/Makefile @@ -122,7 +122,9 @@ db-migrate-heads: ## Show migrations marked as a head ################################################## # Turn off log capture for failed tests by default. -# To re-enable log capture, run `make test args="--show-capture=all"` +# To re-enable display of captured logs when tests fail, run `make test args="--show-capture=all"` +# Or to turn off capturing of logs (and have logs print to stdout in realtime), run `make test args="--capture=no"` +# or for short `make test args="-s"` test: ## Run all tests except for audit logging tests $(PY_RUN_CMD) pytest -m "not audit" --show-capture=no $(args) From 19b4eeb693974b54219044ec289825e0e74cb48f Mon Sep 17 00:00:00 2001 From: Loren Yu Date: Fri, 3 Feb 2023 14:43:40 -0800 Subject: [PATCH 10/10] Add link to docs --- app/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/app/Makefile b/app/Makefile index 9419537e..26158dbc 100644 --- a/app/Makefile +++ b/app/Makefile @@ -125,6 +125,7 @@ db-migrate-heads: ## Show migrations marked as a head # To re-enable display of captured logs when tests fail, run `make test args="--show-capture=all"` # Or to turn off capturing of logs (and have logs print to stdout in realtime), run `make test args="--capture=no"` # or for short `make test args="-s"` +# See https://docs.pytest.org/en/latest/how-to/capture-stdout-stderr.html#how-to-capture-stdout-stderr-output test: ## Run all tests except for audit logging tests $(PY_RUN_CMD) pytest -m "not audit" --show-capture=no $(args)