diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5a50278..f6eaea3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -16,7 +16,7 @@ Thanks for considering a contribution. This repo is the worker node for [Greffon By signing off your commits you certify that you wrote the code (or have the right to submit it) and license it to the project under [AGPL v3](./LICENSE). Full text at [developercertificate.org](https://developercertificate.org/). ```bash -git commit -s -m "feat: add CRL refresh backoff" +git commit -s -m "feat: add monitor status backoff" ``` PRs without DCO sign-off cannot be merged. @@ -34,7 +34,7 @@ GREFFER_WORKERS_ENABLED=true GREFFER_PROTOCOL=http \ poetry run uvicorn --factory app.main:create_app --host 0.0.0.0 --port 8001 ``` -`source env.env` is required — `Settings` has no auto-loaded env file and `apply_ops_migrations` exits early without `GREFFER_ID`. `GREFFER_WORKERS_ENABLED=true` turns on the register/monitor/CRL tasks. `GREFFER_PROTOCOL=http` overrides env.env's `https` default: bare uvicorn has no TLS, so without the override the greffer registers an `https` callback URL that the manager can't reach. (Run the full compose stack if you want production-like nginx TLS.) The greffer also needs a reachable manager and a Docker daemon. See [README.md](./README.md) for the env var set. +`source env.env` is required — `Settings` has no auto-loaded env file and `apply_ops_migrations` exits early without `GREFFER_ID`. `GREFFER_WORKERS_ENABLED=true` turns on the register/monitor tasks. `GREFFER_PROTOCOL=http` overrides env.env's `https` default: bare uvicorn has no TLS, so without the override the greffer registers an `https` callback URL that the manager can't reach. (Run the full compose stack if you want production-like nginx TLS.) The greffer also needs a reachable manager and a Docker daemon. See [README.md](./README.md) for the env var set. ## Code style @@ -44,7 +44,7 @@ GREFFER_WORKERS_ENABLED=true GREFFER_PROTOCOL=http \ - Use `transaction`-style atomic patterns for multi-step on-disk state changes - Never `verify=False` on `requests`/`httpx` calls - Secrets from environment variables only — never hardcoded -- Respect the `--workers 1` invariant: background tasks (register / monitor / CRL sync) assume a single process +- Respect the `--workers 1` invariant: background tasks (register / monitor) assume a single process ## Tests diff --git a/Dockerfile b/Dockerfile index 14a9fb1..92efb7f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -25,7 +25,7 @@ COPY . /app # than the pre-cutover `;` which started anyway. # # --workers 1 is a hard requirement: multi-worker uvicorn would spawn N -# copies of each background task (register / monitor / CRL sync), each +# copies of each background task (register / monitor), each # minting its own token and fighting the manager over cert state. See # HLD #3 § Single-worker uvicorn constraint. CMD ["sh", "-c", "poetry run python -m app.cli apply_ops_migrations && exec poetry run uvicorn --factory app.main:create_app --host 0.0.0.0 --port 8000 --workers 1"] diff --git a/README.md b/README.md index ecbddc9..b2ea814 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ Docker Engine (docker-compose) ──► greffon instances Nginx (per-instance TLS reverse proxy) ``` -The greffer runs as a single uvicorn worker (`--workers 1`) on purpose: three background tasks (register / monitor / CRL sync) live in one process, and multi-worker would spawn duplicate copies that fight over cert state. +The greffer runs as a single uvicorn worker (`--workers 1`) on purpose: two background tasks (register / monitor) live in one process, and multi-worker would spawn duplicate copies that fight over cert state. ## Local development @@ -46,7 +46,7 @@ set -a && source env.env && set +a # Run on-disk state migrations before the server binds (they must not race request handlers) poetry run python -m app.cli apply_ops_migrations -# GREFFER_WORKERS_ENABLED=true turns on the register / monitor / CRL background tasks — +# GREFFER_WORKERS_ENABLED=true turns on the register / monitor background tasks — # without it the greffer starts but never registers with the manager or sends callbacks. # GREFFER_PROTOCOL=http: bare uvicorn serves plain HTTP. env.env defaults to https because # the full compose stack puts an nginx TLS proxy in front; running uvicorn directly has no diff --git a/SECURITY.md b/SECURITY.md index a906200..e6e6b0e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -27,7 +27,7 @@ Include as much as you can: ## Scope — especially relevant for the greffer -- mTLS certificate / private key handling and the CRL sync path +- TLS certificate / private key handling (cert install path) - Jinja2 rendering of catalog compose templates (template injection, arbitrary host paths) - Docker SDK usage (container escape, host mount exposure) - The Nginx per-instance proxy config generation diff --git a/app/lifespan.py b/app/lifespan.py index 308ddec..9c7b7ff 100644 --- a/app/lifespan.py +++ b/app/lifespan.py @@ -23,7 +23,7 @@ @asynccontextmanager async def lifespan(app: FastAPI) -> AsyncIterator[None]: - """App lifespan — start the three background workers if enabled. + """App lifespan — start the background workers if enabled. ``greffer_workers_enabled`` defaults to False so unit tests don't accidentally spawn real workers. Production sets diff --git a/app/settings.py b/app/settings.py index b305517..086835b 100644 --- a/app/settings.py +++ b/app/settings.py @@ -89,12 +89,11 @@ def _empty_string_is_none(cls, v): docker_nginx_name: str = "greffer-nginx-1" - crl_sync_interval: int = 300 monitor_interval: int = 5 skip_ops_migrations: bool = False - # Workers (register / monitor_status / CRL sync). Disabled by default + # Workers (register / monitor_status). Disabled by default # so unit tests don't accidentally start real workers. Production # enables via ``GREFFER_WORKERS_ENABLED=true`` in compose. # diff --git a/app/workers/__init__.py b/app/workers/__init__.py index d918c06..db5dadc 100644 --- a/app/workers/__init__.py +++ b/app/workers/__init__.py @@ -1,17 +1,22 @@ """Background workers — greffer lifecycle tasks running under FastAPI lifespan. -Three workers port the Django daemon threads at ``apps/controller/views.py:14-22``: +Two workers port the Django daemon threads at ``apps/controller/views.py:14-22``: * ``register_worker`` — one-shot; POSTs registration + polls for cert. * ``monitor_worker`` — forever loop; reports greffon instance status changes. -* ``crl_sync_worker`` — forever loop; fetches updated CRL periodically. + +(A third legacy worker, ``crl_sync_worker``, copied the manager's CRL +into the nginx container. Removed: no nginx config ever loaded the file +(``ssl_crl`` requires client-cert verification, which this nginx does +not do), so the sync had no effect. Revocation enforcement is deferred +to the platform's planned step-ca migration.) Startup is gated by ``Settings.greffer_workers_enabled`` (env var ``GREFFER_WORKERS_ENABLED``, default False) so unit tests don't accidentally spawn real workers. Production sets it to ``true`` in ``docker-compose.yml``. -Single-worker uvicorn only. Multi-worker would spawn N × 3 pollers per +Single-worker uvicorn only. Multi-worker would spawn N × 2 pollers per container, each with a distinct token, fighting over manager state. """ from __future__ import annotations @@ -21,7 +26,6 @@ from fastapi import FastAPI -from app.workers.crl import crl_sync_worker from app.workers.monitor import monitor_worker from app.workers.register import register_worker @@ -32,7 +36,6 @@ def start_workers(app: FastAPI) -> list[asyncio.Task]: return [ asyncio.create_task(register_worker(app), name="greffer-register"), asyncio.create_task(monitor_worker(app), name="greffer-monitor"), - asyncio.create_task(crl_sync_worker(app), name="greffer-crl-sync"), ] diff --git a/app/workers/crl.py b/app/workers/crl.py deleted file mode 100644 index 8ef9688..0000000 --- a/app/workers/crl.py +++ /dev/null @@ -1,39 +0,0 @@ -"""CRL sync worker — periodically fetch the CRL from the manager. - -Ports ``apps/utils/greffon/base_server.py:sync_crl()``. Legacy order is -**sleep-then-fetch** (so the first tick happens after ``CRL_SYNC_INTERVAL`` -seconds, not immediately). That's preserved here: the initial CRL is -fetched synchronously at the end of ``register_worker``, and this forever -loop handles every subsequent tick. -""" -from __future__ import annotations - -import asyncio -import logging - -import anyio -from fastapi import FastAPI - -from app.settings import Settings -from app.workers.register import _fetch_and_store_crl - -logger = logging.getLogger("greffer") - - -async def crl_sync_worker(app: FastAPI) -> None: - settings: Settings = app.state.settings - try: - while True: - # Sleep first to match legacy order — register_worker has - # already fetched the initial CRL. - await asyncio.sleep(settings.crl_sync_interval) - # _fetch_and_store_crl owns its own exception handling (logs - # and continues); no outer try/except needed here. - # abandon_on_cancel=True keeps shutdown snappy even if the - # fetch is mid-flight. - await anyio.to_thread.run_sync( - _fetch_and_store_crl, settings, abandon_on_cancel=True - ) - except asyncio.CancelledError: - logger.info("crl_sync cancelled") - raise diff --git a/app/workers/register.py b/app/workers/register.py index 296ada5..2f8fdc0 100644 --- a/app/workers/register.py +++ b/app/workers/register.py @@ -1,4 +1,4 @@ -"""Register worker — POST to manager, poll for cert, install, fetch initial CRL. +"""Register worker — POST to manager, poll for cert, install. Ports ``apps/utils/greffon/base_server.py:register()`` to asyncio. The outer loops are rewritten as async so they cancel cleanly on lifespan shutdown; @@ -102,9 +102,6 @@ async def register_worker(app: FastAPI) -> None: data, abandon_on_cancel=True, ) - await anyio.to_thread.run_sync( - _fetch_and_store_crl, settings, abandon_on_cancel=True - ) logger.info("register complete") return await asyncio.sleep(_CERT_POLL_SECONDS) @@ -221,27 +218,3 @@ def _maybe_install_initial_tunnel_config( return if wrote: logger.info("initial_tunnel_config_installed path=%s", target) - - -def _fetch_and_store_crl(settings: Settings) -> None: - """Fetch CRL from manager, copy into nginx container. Shared with - ``crl_sync_worker`` (see ``app/workers/crl.py``).""" - from apps.utils.docker.base import copy_file_into_container - - try: - res = requests.get( - f"{settings.greffon_base_server}/api/greffer/ca/crl/", - verify=settings.greffer_ssl_verify, - timeout=10, - ) - if res.status_code == 200: - copy_file_into_container( - settings.docker_nginx_name, "/root", "revoked.crl", res.text - ) - logger.info("CRL updated successfully") - else: - logger.warning("Failed to fetch CRL: HTTP %s", res.status_code) - except Exception as e: - # Preserve legacy behavior: log and continue. The caller's async - # loop handles retry timing. - logger.warning("Failed to fetch CRL: %s", e) diff --git a/docker-compose.yml b/docker-compose.yml index 6b2140d..8e5fd9d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -24,7 +24,7 @@ services: environment: # Feature #3 shipped dormant; flip it on here at the cutover moment. # Without this, the greffer would boot with the HTTP surface alive - # but no register / monitor / CRL workers — the manager would never + # but no register / monitor workers — the manager would never # see a cert handshake or instance-status callbacks. - GREFFER_WORKERS_ENABLED=true extra_hosts: diff --git a/tests/test_lifespan.py b/tests/test_lifespan.py index a40f2f1..ca2fab4 100644 --- a/tests/test_lifespan.py +++ b/tests/test_lifespan.py @@ -38,10 +38,10 @@ async def test_lifespan_no_tasks_when_workers_disabled( @pytest.mark.asyncio -async def test_lifespan_starts_three_workers_when_enabled( +async def test_lifespan_starts_workers_when_enabled( settings: Settings, ) -> None: - """``greffer_workers_enabled=True`` → three tasks started with expected + """``greffer_workers_enabled=True`` → both tasks started with expected names, cancelled on shutdown.""" settings.greffer_workers_enabled = True # type: ignore[misc] app = create_app(token="t", settings=settings) @@ -54,20 +54,18 @@ async def _noop_worker(_app): # reaches them before `start_workers` looks them up. with patch("app.workers.register_worker", new=_noop_worker), patch( "app.workers.monitor_worker", new=_noop_worker - ), patch("app.workers.crl_sync_worker", new=_noop_worker): + ): async with lifespan(app): current_names = { t.get_name() for t in asyncio.all_tasks() if not t.done() } assert "greffer-register" in current_names assert "greffer-monitor" in current_names - assert "greffer-crl-sync" in current_names # After lifespan shutdown the worker tasks must be gone. leftover = {t.get_name() for t in asyncio.all_tasks() if not t.done()} assert "greffer-register" not in leftover assert "greffer-monitor" not in leftover - assert "greffer-crl-sync" not in leftover def test_greffer_workers_enabled_env_var_binds( diff --git a/tests/test_settings.py b/tests/test_settings.py index e172bb8..2d94c4d 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -32,7 +32,6 @@ def test_defaults_apply_when_env_unset(monkeypatch: pytest.MonkeyPatch) -> None: assert s.greffer_public_scheme == "https" assert s.greffon_path == Path("/data") assert s.docker_nginx_name == "greffer-nginx-1" - assert s.crl_sync_interval == 300 assert s.monitor_interval == 5 assert s.skip_ops_migrations is False assert s.logger_name == "greffer" @@ -41,11 +40,11 @@ def test_defaults_apply_when_env_unset(monkeypatch: pytest.MonkeyPatch) -> None: def test_env_overrides_defaults(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("GREFFER_ID", "x") monkeypatch.setenv("GREFFON_PATH", "/tmp/alt") - monkeypatch.setenv("CRL_SYNC_INTERVAL", "60") + monkeypatch.setenv("MONITOR_INTERVAL", "60") monkeypatch.setenv("GREFFER_SSL_VERIFY", "false") s = Settings() assert s.greffon_path == Path("/tmp/alt") - assert s.crl_sync_interval == 60 + assert s.monitor_interval == 60 assert s.greffer_ssl_verify is False diff --git a/tests/test_workers_crl.py b/tests/test_workers_crl.py deleted file mode 100644 index 11d8f04..0000000 --- a/tests/test_workers_crl.py +++ /dev/null @@ -1,86 +0,0 @@ -"""Tests for crl_sync_worker. - -Key behavior: sleep-then-fetch ordering (not fetch-then-sleep), matching -the legacy ``sync_crl`` function in ``apps/utils/greffon/base_server.py``. -""" -from __future__ import annotations - -import asyncio -from unittest.mock import patch - -import pytest - -from app.main import create_app -from app.settings import Settings -from app.workers.crl import crl_sync_worker - - -@pytest.mark.asyncio -async def test_crl_worker_sleeps_first_then_fetches( - settings: Settings, monkeypatch: pytest.MonkeyPatch -) -> None: - app = create_app(token="t", settings=settings) - - events: list[str] = [] - - async def _record_sleep(s: float) -> None: - events.append(f"sleep({s})") - if len([e for e in events if e.startswith("sleep")]) >= 2: - # Cancel after the second sleep so the test ends. - raise asyncio.CancelledError - - def _record_fetch(_settings): - events.append("fetch") - - monkeypatch.setattr("app.workers.crl.asyncio.sleep", _record_sleep) - # Patch the binding used by crl_sync_worker (imported at module load - # via `from app.workers.register import _fetch_and_store_crl`). - # Patching `app.workers.register._fetch_and_store_crl` would miss - # this already-bound reference. - monkeypatch.setattr("app.workers.crl._fetch_and_store_crl", _record_fetch) - - with pytest.raises(asyncio.CancelledError): - await crl_sync_worker(app) - - # First event must be a sleep, not a fetch. - assert events[0].startswith("sleep") - assert "fetch" in events - - -@pytest.mark.asyncio -async def test_crl_worker_uses_crl_sync_interval( - settings: Settings, monkeypatch: pytest.MonkeyPatch -) -> None: - settings.crl_sync_interval = 42 # type: ignore[misc] - app = create_app(token="t", settings=settings) - - sleeps: list[float] = [] - - async def _record_sleep(s: float) -> None: - sleeps.append(s) - raise asyncio.CancelledError # cancel after first sleep - - monkeypatch.setattr("app.workers.crl.asyncio.sleep", _record_sleep) - - with pytest.raises(asyncio.CancelledError): - await crl_sync_worker(app) - - assert sleeps == [42] - - -@pytest.mark.asyncio -async def test_crl_worker_cancellable_during_sleep( - settings: Settings, monkeypatch: pytest.MonkeyPatch -) -> None: - app = create_app(token="t", settings=settings) - - def _noop_fetch(_s): - return - - monkeypatch.setattr("app.workers.crl._fetch_and_store_crl", _noop_fetch) - - task = asyncio.create_task(crl_sync_worker(app)) - await asyncio.sleep(0.01) - task.cancel() - with pytest.raises(asyncio.CancelledError): - await task diff --git a/tests/test_workers_register.py b/tests/test_workers_register.py index 56a9009..ab89f60 100644 --- a/tests/test_workers_register.py +++ b/tests/test_workers_register.py @@ -13,7 +13,6 @@ from app.main import create_app from app.settings import Settings from app.workers.register import ( - _fetch_and_store_crl, _fetch_cert, _install_cert, _maybe_install_initial_tunnel_config, @@ -148,39 +147,6 @@ def test_install_cert_optional_ca(settings: Settings) -> None: ) -def test_fetch_and_store_crl_happy_path(settings: Settings) -> None: - with patch("app.workers.register.requests") as mock_requests, patch( - "apps.utils.docker.base.copy_file_into_container" - ) as mock_copy: - mock_response = MagicMock() - mock_response.status_code = 200 - mock_response.text = "CRL-DATA" - mock_requests.get.return_value = mock_response - _fetch_and_store_crl(settings) - mock_copy.assert_called_once_with( - settings.docker_nginx_name, "/root", "revoked.crl", "CRL-DATA" - ) - - -def test_fetch_and_store_crl_swallows_exception(settings: Settings) -> None: - """Any exception is caught and logged — parity with legacy sync_crl.""" - with patch("app.workers.register.requests") as mock_requests: - mock_requests.get.side_effect = requests.ConnectionError("boom") - # Must not raise. - _fetch_and_store_crl(settings) - - -def test_fetch_and_store_crl_skips_on_non_200(settings: Settings) -> None: - with patch("app.workers.register.requests") as mock_requests, patch( - "apps.utils.docker.base.copy_file_into_container" - ) as mock_copy: - mock_response = MagicMock() - mock_response.status_code = 500 - mock_requests.get.return_value = mock_response - _fetch_and_store_crl(settings) - mock_copy.assert_not_called() - - def test_resolve_hostname_returns_ip() -> None: with patch("app.workers.register.socket") as mock_socket: mock_socket.gethostname.return_value = "host" @@ -195,8 +161,8 @@ def test_resolve_hostname_returns_ip() -> None: @pytest.mark.asyncio async def test_register_worker_happy_path(settings: Settings) -> None: - """Happy path: POST succeeds, GET returns 200, cert installs, CRL - fetched, worker returns.""" + """Happy path: POST succeeds, GET returns 200, cert installs, + worker returns.""" app = create_app(token="tok", settings=settings) with patch("app.workers.register.requests") as mock_requests, patch( "apps.utils.docker.base.copy_file_into_container" @@ -211,17 +177,13 @@ async def test_register_worker_happy_path(settings: Settings) -> None: "certificate": "CERT", "private_key": "KEY", } - crl_response = MagicMock() - crl_response.status_code = 200 - crl_response.text = "CRL" - # First GET is cert, second GET is CRL. - mock_requests.get.side_effect = [cert_response, crl_response] + mock_requests.get.side_effect = [cert_response] await register_worker(app) mock_requests.post.assert_called_once() - # cert install -> pem.crt + cert.key ; CRL install -> revoked.crl = 3 copies - assert mock_copy.call_count == 3 + # cert install -> pem.crt + cert.key = 2 copies + assert mock_copy.call_count == 2 @pytest.mark.asyncio @@ -255,10 +217,7 @@ async def _record_sleep(s: float) -> None: cert_response = MagicMock() cert_response.status_code = 200 cert_response.json.return_value = {"certificate": "C", "private_key": "K"} - crl_response = MagicMock() - crl_response.status_code = 200 - crl_response.text = "CRL" - mock_requests.get.side_effect = [cert_response, crl_response] + mock_requests.get.side_effect = [cert_response] await register_worker(app) @@ -293,10 +252,7 @@ async def _noop_sleep(_s: float) -> None: cert_response = MagicMock() cert_response.status_code = 200 cert_response.json.return_value = {"certificate": "C", "private_key": "K"} - crl = MagicMock() - crl.status_code = 200 - crl.text = "CRL" - mock_requests.get.side_effect = [cert_response, crl] + mock_requests.get.side_effect = [cert_response] await register_worker(app) @@ -352,22 +308,18 @@ async def _noop_sleep(_s: float) -> None: cert_response = MagicMock() cert_response.status_code = 200 cert_response.json.return_value = {"certificate": "C", "private_key": "K"} - crl = MagicMock() - crl.status_code = 200 - crl.text = "CRL" - # Sequence: connection error, timeout, success, CRL. + # Sequence: connection error, timeout, success. mock_requests.get.side_effect = [ requests.ConnectionError("blip"), requests.Timeout("slow"), cert_response, - crl, ] # Must complete without propagating the transient errors. await register_worker(app) - # Two failed polls + one successful cert fetch + one CRL fetch = 4 GETs. - assert mock_requests.get.call_count == 4 + # Two failed polls + one successful cert fetch = 3 GETs. + assert mock_requests.get.call_count == 3 @pytest.mark.asyncio @@ -391,15 +343,12 @@ async def _noop_sleep(_s: float) -> None: success = MagicMock() success.status_code = 200 success.json.return_value = {"certificate": "C", "private_key": "K"} - crl = MagicMock() - crl.status_code = 200 - crl.text = "CRL" - mock_requests.get.side_effect = [fail, fail, success, crl] + mock_requests.get.side_effect = [fail, fail, success] await register_worker(app) - # 2 failed cert polls + 1 successful + 1 CRL fetch = 4 - assert mock_requests.get.call_count == 4 + # 2 failed cert polls + 1 successful = 3 + assert mock_requests.get.call_count == 3 @pytest.mark.asyncio @@ -422,10 +371,7 @@ async def _noop_sleep(_s: float) -> None: cert_response = MagicMock() cert_response.status_code = 200 cert_response.json.return_value = {"certificate": "C", "private_key": "K"} - crl = MagicMock() - crl.status_code = 200 - crl.text = "CRL" - mock_requests.get.side_effect = [cert_response, crl] + mock_requests.get.side_effect = [cert_response] await register_worker(app) @@ -455,10 +401,7 @@ async def _noop_sleep(_s: float) -> None: cert_response = MagicMock() cert_response.status_code = 200 cert_response.json.return_value = {"certificate": "C", "private_key": "K"} - crl = MagicMock() - crl.status_code = 200 - crl.text = "CRL" - mock_requests.get.side_effect = [cert_response, crl] + mock_requests.get.side_effect = [cert_response] await register_worker(app)