docs(security): add a security architecture reference for external review - #110
Merged
Conversation
…view A one-page audit report (security-audit.md) answers "what's been checked and fixed." It doesn't answer the question a security engineer asks before either: what actually talks to what, over which protocol, and how each side proves who it is. That's this document — every connection in the system (browser, control plane, Postgres, worker, SQL Server, and the optional Entra/SIEM/notification/install paths), each with its protocol, port, direction, and authentication method, plus a component diagram and an authentication-method-by-actor summary. Every claim was checked against the actual config schema, workflow files, and source rather than written from memory — worker auth modes, dashboard auth modes, port numbers, the mTLS CA, the credential-encryption flow, and the npm trusted-publishing setup were all grepped and read directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AyYg2j8FVkLjiaVcj5HCkj
#115) ## What and why Implements automatic certificate renewal for mTLS-authenticated workers. Previously, the 90-day client certificates issued at enrolment were the only issuance path, making certificate expiry an outage timer: every worker would lose connectivity at day 90 and require manual re-enrolment with a fresh token. This change adds: 1. **Worker-side renewal** (`CertificateRenewer`): Monitors the client certificate's validity, generates a new CSR at half-life (45 days), and sends it to the control plane over the authenticated session. On success, installs the new certificate and reconnects to exercise it while the old one still works. 2. **Server-side renewal** (`renewWorkerCertificate`): Issues a fresh certificate over the CSR, stores it with label "Renewed", keeps the current credential valid (so the session carrying the response doesn't break), and revokes older spare credentials to prevent accumulation. 3. **Protocol additions**: New `CertificateRenewalRequest` and `CertificateRenewalResponse` message types in the worker↔hub stream. 4. **Auth posture review**: New startup check that warns operators if the deployment still has workers on `token` mode, which is a bearer secret and weaker than the now-automatic `mtls`. 5. **Installer updates**: Both `bootstrap.sh` and `install.ps1` now accept `--auth-mode` / `-AuthMode` (defaulting to `mtls`), and `enrol` fails with a clear error if the configured mode does not match the mode the enrolment token was minted for. The renewal basis is possession of a working credential (proven by the TLS handshake that opened the session), matching EST `simplereenroll` (RFC 7030) and kubelet client-certificate rotation. This means a stolen key can renew itself indefinitely, but that is addressed by the per-connection revocation check and audit trail, not by an expiry the legitimate worker would hit first. ## Blast radius - [x] Touches **authentication or authorisation** - [x] Changes the **wire contract** (`.proto`) - [ ] Changes the **database schema** — **no schema change and no migration.** Renewal reuses the existing `worker_credentials` table, which already supports multiple rows per worker with independent `revoked_at`. That is what makes the old and new certificates able to overlap without a migration. **What could go wrong:** - A worker configured for `mtls` but enrolled with a `token`-mode token would receive no certificate and fail to authenticate. **Mitigation:** `enrol` now validates the mode match and fails with a clear error instead of writing an empty certificate file; the dashboard's install command carries the token's own mode. - A renewal that succeeds on the server but fails to install on the worker leaves the old certificate valid and retries hourly. **Mitigation:** renewal happens at half-life, leaving ~45 days of runway. - Key and certificate are two files and cannot be replaced in one atomic step. A mismatched pair survives restarts and locks a worker out exactly as an expiry would. **Mitigation:** the pair is verified against each other before either is renamed into place, leaving only the gap between two renames, which contains no I/O. - Stolen worker keys can renew indefinitely. **Mitigation:** per-connection revocation check and an audit row on every renewal. The hub also refuses renewal from any worker that did not authenticate with mTLS, so a token- or Entra-mode worker cannot obtain a certificate its enrolment never established it holds a key for. ## How it was tested - [x] `pnpm lint`, `pnpm typecheck`, `pnpm test:unit` (670 tests), `pnpm proto:check`, `pnpm audit --audit-level high` - [x] `pnpm build:sea`, and the bundle started from an empty directory (`node rsagent-worker.mjs --rsagent-selftest`) - [x] Integration tests — **not run locally** (no Docker for the SQL Server container); green in CI on this branch New test suites: - `packages/worker/test/cert-renewal.test.ts` (12): state machine with faked timers — renewal at half-life, immediate renewal when already past it, jitter bounds, delays exceeding `setTimeout`'s 24.9-day ceiling, refusal and timeout retries, mismatched-key rejection, and abandonment on disconnect. - `packages/server/test/worker-cert-renewal.test.ts` (14): against a real Postgres — the certificate in use stays valid across its own renewal, older spares are revoked, other workers are untouched, the CSR subject is discarded in favour of the enrolled worker id, and the hub refuses renewal for token/entra workers. - `packages/server/test/auth-posture.test.ts` (12): the startup review, including that it fails *on* rather than off. Both installers' config templating was rendered for all three auth modes with and without `--ca-cert` and parsed back through the worker's own zod schema. That caught a `set -e` abort in the bash templating: a failing command substitution inside a variable assignment takes the assignment's exit status, which would have silently killed the installer for `token`/`entra` without `--ca-cert`. Safe in the heredoc the pattern came from; not in an assignment. ## Checklist - [x] No SQL built by string concatenation or interpolation - [x] New behaviour has a test - [x] Error messages tell the reader what to do next - [x] Generated protobuf regenerated and committed (`pnpm proto:gen`) - [x] Docs updated — `authentication.md` (worker auth section rewritten around which mode to choose), `security-architecture.md`, `security.md`, `deployment.md`, `README.md` - [x] `docs/migration.md` updated — closes the "worker certificate auto-rotation" known gap and reverses the "worker authentication defaults to an API key" decision ## Known gaps this leaves behind Both are recorded in `docs/migration.md` and neither is in scope here: - **Nothing alerts on a worker that has stopped renewing.** Renewal makes expiry unlikely, not impossible. `certExpiresAt` reaches the dashboard without being displayed; until it is, the signal is a `worker.certificate.renewed` audit row per worker per half-lifetime. - **`RSAGENT_GRPC_TLS_CLIENT_CA` (bring-your-own CA) is half-wired.** TLS accepts the operator's certificate and then authentication rejects it, because identity is a fingerprint row that only enrolment and renewal create. Cheap to finish given identity is the fingerprint rather than the chain, but until then it should fail at startup rather than per connection. ## Note on the base branch This PR targets `docs/security-architecture`, not `main` — it is stacked on the commit it was branched from. Worth confirming that is intended before merging. https://claude.ai/code/session_0178yxB15VZFq9XU5x1ViCEY --------- Co-authored-by: Claude <noreply@anthropic.com>
PSScriptAnalyzer's PSAvoidUsingEmptyCatchBlock (scoped in on main since this branch diverged, per PSScriptAnalyzerSettings.psd1) flagged the managed- identity detection added for install.ps1 -AuthMode: a comment is not a statement, so a catch body containing only one was still empty to the rule. The rule exists to catch a real failure being swallowed silently, which this genuinely isn't -- no IMDS present, or it didn't answer within a second, is the expected case on non-Azure hosts. Write-Verbose keeps it silent by default while giving -Verbose somewhere to look, and satisfies the rule without changing the advisory-only behaviour: the probe still never selects an auth mode, only suggests one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0178yxB15VZFq9XU5x1ViCEY
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Requested alongside the security audit report: a document a security engineer can review to sign off on the deployment, distinct from the audit history.
docs/security-audit.md(#109) answers "what's been checked and fixed"; this answers "what talks to what, over which protocol, and how does each side prove who it is."Covers every connection in the system with a table of protocol/port/direction/authentication:
Plus a Mermaid component diagram, a walkthrough of the SQL-credential client-side-encryption flow, an authentication-methods-by-actor summary table, and a reviewer checklist of deployment-specific things worth confirming.
Every claim was checked against the actual config schema (
config.tsin bothserverandworker), workflow files, and source — not written from memory.Test plan
pnpm lint && pnpm typecheck && pnpm test:unit— all green (docs-only change, no code touched)🤖 Generated with Claude Code
https://claude.ai/code/session_01AyYg2j8FVkLjiaVcj5HCkj