feat(mtls): present client cert on outbound calls (FastAPI port)#18
feat(mtls): present client cert on outbound calls (FastAPI port)#18omarsy wants to merge 1 commit into
Conversation
FastAPI port of greffer-side mTLS: once registration has installed cert+key locally, every outbound call to the manager (register retry, cert poll, CRL fetch, status callback) presents the greffer's client cert and verifies the manager against the CA it issued us. During the bootstrap window (pre-registration) we fall back to verify=True against system CAs so the initial POST + cert poll can still complete. This is the FastAPI replacement for the abandoned Django-era PR #13 — none of those files exist post-cutover, so this is a greenfield port, not a rebase. Semantics are identical. - Settings: drop `greffer_ssl_verify` (boolean was the problem); add `greffer_cert_dir: Path = /etc/greffer/certs` and `greffer_allow_insecure_bootstrap: bool = False`. - register.py: new `_client_auth(settings)` helper returns requests kwargs (verify + optional cert); used by `_post_register`, `_fetch_cert`, `_fetch_and_store_crl`. New `_write_local_cert` atomic-writes under greffer_cert_dir (tmp+rename, mode 0o600 for the key). `_install_cert` writes key before cert so `_client_auth`'s "cert exists" precondition implies "key is durable". New `_check_secure_bootstrap(settings)` runs at worker start and refuses to proceed unless GREFFON_BASE_SERVER is https or GREFFER_ALLOW_INSECURE_BOOTSTRAP=1 is set. New `_validate_cert_response` rejects responses missing required fields instead of crashing the worker with KeyError. - monitor.py: `_report_status_change` uses `_client_auth`. Worker blocks on `app.state.registered` (new `asyncio.Event` set by register_worker at the end of cert install) before the first tick — so restarts with leftover instance dirs don't fire status callbacks in the bootstrap posture and get 403'd by the manager's mTLS gate. - main.py: create `app.state.registered` in `create_app`. - env.env: drop `GREFFER_SSL_VERIFY=false` (the security regression); add `GREFFER_ALLOW_INSECURE_BOOTSTRAP=1` since dev talks to manager over http://host.docker.internal:8000. Tests: 194/194 pass. New coverage for every helper (client_auth matrix, secure_bootstrap opt-in, atomic-write mode + no-stray-tmp invariant, malformed-response retry, registered-event gating of monitor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 140a069181
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "issuing_ca" in data: | ||
| _write_local_cert(settings, _CA_FILE, data["issuing_ca"]) |
There was a problem hiding this comment.
This branch only updates ca.pem when issuing_ca is present, so an old CA file is left behind on subsequent registrations that omit it (which your own comments treat as valid). Because _client_auth switches to verify=<ca.pem> whenever that file exists, outbound manager calls can continue using a stale CA instead of falling back to system trust, which can break register/poll/CRL/status traffic after CA rotation or when the manager stops returning issuing_ca.
Useful? React with 👍 / 👎.
| raise RuntimeError( | ||
| f"GREFFON_BASE_SERVER={settings.greffon_base_server!r} is not https. " | ||
| "The bootstrap register/cert-poll calls carry the greffer token and " | ||
| "receive the greffer private key in the response body. Set " | ||
| "GREFFER_ALLOW_INSECURE_BOOTSTRAP=1 to permit (dev only)." |
There was a problem hiding this comment.
Raising here does not actually make the service "refuse to start" because register_worker runs as a background task created in start_workers and startup does not await it; the app can come up while registration has already failed. In that state monitor_worker waits indefinitely on app.state.registered, so a critical bootstrap misconfiguration becomes a silent partial-outage instead of a fail-fast startup error.
Useful? React with 👍 / 👎.
|
Closing — we're dropping TLS-layer mTLS in favor of application-level token auth with gated rotation on the manager side. Cloudflare proxies the manager origin, which would terminate the TLS handshake at the edge and defeat nginx's ssl_verify_client. Instead, we're hardening the existing X-GREFFON-TOKEN flow: adding auth to the two currently-unauthed endpoints, and gating cert re-issuance on an explicit admin action so a leaked token can't silently exfiltrate a fresh private key. Manager-side PR landing soon as greffon/manager#TBD; no greffer-side code change is needed. |
1 participant
Summary
FastAPI port of the greffer-side mTLS work that was originally in #13 (closed — targeted the Django codebase, obsoleted by the cutover in #14–#17).
Once registration has installed cert+key under
settings.greffer_cert_dir, every outbound call to the manager (register retry, cert poll, CRL fetch, status callback) presents the greffer's client cert and verifies the manager against the CA it issued us. During the bootstrap window (pre-registration) the calls fall back toverify=Trueagainst system CAs so the initial POST + cert poll can still complete.Changes
greffer_ssl_verify(the shape was the problem); addgreffer_cert_dir: Path = /etc/greffer/certsandgreffer_allow_insecure_bootstrap: bool = False.app/workers/register.py:_client_auth(settings)helper — requests kwargs (verify + optional cert), used by_post_register,_fetch_cert,_fetch_and_store_crl._write_local_cert(settings, file_name, content, mode)— atomic tmp+rename,0o600for the key,0o700directory mode._install_certwrites key before cert so_client_auth's "cert exists" precondition implies "key is durable" — no half-registered window where a monitor tick sees only half the pair._check_secure_bootstrap(settings)at worker start: refuseshttp://unlessGREFFER_ALLOW_INSECURE_BOOTSTRAP=1is set. The bootstrap POST carries the token and the cert-poll response carries the private key; an operator copying dev env.env to prod must make a conscious choice._validate_cert_response— a malformed 200 no longer crashes the worker withKeyError.app/workers/monitor.py:_report_status_changeuses_client_auth. Worker blocks onapp.state.registered(newasyncio.Eventset byregister_workerat the end of cert install) before the first tick. Without this, a restart with leftover instance dirs fires status callbacks in the bootstrap posture and gets 403'd by the manager's mTLS location gate.app/main.py: createapp.state.registeredincreate_app.env.env: dropGREFFER_SSL_VERIFY=false(the security regression that slipped back in during the cutover); addGREFFER_ALLOW_INSECURE_BOOTSTRAP=1since dev talks to manager overhttp://host.docker.internal:8000.Dependency + merge order
Depends on (blocks) greffon/manager#12 (manager-side mTLS enforcement). Land this PR first — it's backward-compatible (manager ignores the presented cert until its own PR lands). Merging greffon/manager#12 without this one breaks the control plane (status callbacks get 403'd).
Test plan
pytest→ 194/194 pass with a clean env (run viadocker runto bypass the composeenv_filethat otherwise injects dev values)._client_authmatrix (bootstrap / all-present / cert-present-no-ca),_check_secure_bootstrap(https / opt-in / refuse),_write_local_cert(atomicity + no stray.tmp+ mode),_validate_cert_responserejection cases,register_workersetsapp.state.registered,monitor_workerwaits for registered before first tick,_report_status_changepresents client cert when cert dir is populated.rg "GREFFER_SSL_VERIFY|greffer_ssl_verify" .→ zero hits.Follow-ups / out of scope
e2e/behave) currently runs the manager as bare Django; with manager-side mTLS enforcement live, the e2e needs nginx in front to match prod. Separate chore in the root repo.get_greffer_certendpoint on the manager side is still unauthenticated — pre-existing issue, now higher blast radius, worth a dedicated security PR.🤖 Generated with Claude Code