sec: address 12 findings from SECURITY_AUDIT.md batch 1#311
Merged
Conversation
Triaged the 30 findings in SECURITY_AUDIT.md and implemented 12 that
are valid, scoped, and don't require cross-service infrastructure
work. Skipped findings are documented below.
## Implemented (12)
### High-severity hardening
* **F-04 — constant-time cert fingerprint compare** (`internal/api/certificate_handler.go:66`):
replaced the `!=` string compare with `subtle.ConstantTimeCompare`
so a co-located attacker can't extract the stored fingerprint
byte-by-byte via response timing.
* **F-05 — Content-Security-Policy header** (`internal/middleware/security.go`):
added a tight default-src 'self' policy with frame-ancestors 'none'
for defence-in-depth against XSS / clickjacking. Tightened beyond
the audit's recommendation by also adding base-uri + form-action.
* **F-14 — JWT algorithm pinned to HS256** (`internal/auth/jwt.go:155`):
replaced the `*jwt.SigningMethodHMAC` family check with an exact
match on `jwt.SigningMethodHS256`. Issuance always uses HS256, so
anything else is by definition forged.
* **F-03 — Valkey password out of cmdline** (`deploy/compose.yml`,
`deploy/setup.sh`, `deploy/valkey.conf.template`): replaced
`--requirepass ${VALKEY_PASSWORD}` with a config file rendered at
setup time (mode 0400) and mounted read-only into pm-valkey.
Healthcheck now uses REDISCLI_AUTH instead of `-a <pw>`. Closes
the /proc/<pid>/cmdline leak.
### Medium / defence-in-depth
* **F-08 — SCIM per-IP rate limiter** (`internal/scim/auth.go`,
`internal/scim/handler.go`, `internal/auth/interceptor.go`): added
a secondary rate-limit bucket keyed on (slug, client IP) with a
20/min ceiling. Closes the slug-spreading bypass — knowing multiple
valid slugs no longer dilutes the per-IP attempt count. Exported
`auth.ClientIPFromHTTP` so non-Connect handlers reuse the same
trusted-proxy semantics.
* **F-15 — SSO redirect URL allowlist** (`internal/api/sso_handler.go`):
added `validateSSORedirectURL` enforcing same-origin against
`CONTROL_SSO_CALLBACK_BASE_URL` or loopback (for pm-enroll CLI flows).
Was previously only validated by the OIDC provider — an IdP CVE or
misconfiguration would have let an attacker pin sessions to a
redirect they control.
* **F-28 — auto-link-by-email visibility** (`internal/api/idp_handler.go`,
`internal/idp/linker.go`): proto default already false (correct), but
promoted the linker's auto-link Debug log to Info so an operator who
enables the flag can audit which identities get linked. Added doc
comment on the create path warning that enabling this flag makes the
IdP's email-verification policy a trust dependency.
### Low / informational
* **F-13 — cap OutputChunk size at 64 KiB** (`internal/handler/agent.go`):
oversized chunks from a compromised agent are now dropped with a
WARN log instead of being enqueued. Stops Valkey memory exhaustion +
executions.output blow-up via flood.
* **F-20 — rate-limiter LRU eviction cap** (`internal/auth/ratelimit.go`):
added a 100k-key ceiling per limiter with LRU eviction on a separate
lastSeen map. Bounds memory growth under wide-IP-distribution attacks
between the 5-minute cleanup ticks.
* **F-26 — strip version from public /health** (`cmd/control/main.go:284`):
unauthenticated health endpoint now returns `ok` rather than
`{"status":"ok","version":"vYYYY.MM.PP"}`. Version remains available
via the mTLS-protected internal /health for operator scrape tools.
* **F-27 — StartTerminal cross-user doc comment**
(`internal/api/terminal_handler.go`): documented that the unscoped
`StartTerminal` permission deliberately grants cross-user TTY access
for incident response, with attribution captured via the
`TerminalSessionStarted` actor_id. Includes the recipe for adding a
`StartTerminal:self` scope if a deployment needs the no-cross-user
variant.
* **F-12 — symmetric whitespace trim in dynamic-query parser**
(`internal/dynamicquery/parser.go:340`): `consumeBareValue` was
TrimRight-only; now TrimSpace so ` admin` and `admin ` produce the
same value. Cosmetic parser-smell fix; the field allowlist already
made this non-exploitable.
## Skipped (18) — see SECURITY_AUDIT.md for full reasoning
* **F-01** JWT permissions embedded statically — design call, section 6
of the audit already classifies this as standard practice.
* **F-02** Asynq task payloads unsigned (CRITICAL) — genuinely valid
but requires cross-service HMAC infrastructure (new env var on
control + gateway, signing layer, verification layer, deployment
migration). Worth a focused PR; tracked separately. Note: the
realistic exploit path is the terminal-input / osquery dispatch
surface, since the action-dispatch path is already gated by the
CA-signed action signature the agent verifies.
* **F-06** TOTP secret in response — required for QR provisioning;
the audit's recommendation doesn't have a clean alternative.
* **F-07** Backup-code race — fix needs an atomic UPDATE which
requires sqlc query changes + regen, blocked on the sqlc-version
drift documented in #308.
* **F-09** SSO actor_type attribution — audit contradicts itself
(section 6 acknowledges the StreamType carries the IdP identity).
* **F-10** 6-char backup-code collision — 1-in-16M, audit acknowledges
negligible probability.
* **F-11** Pagination total-result cap — by design; authorised users
are authorised.
* **F-16** Audit-gap Prometheus counter — needs Prometheus integration
not present in this codebase.
* **F-17** Encryptor missing-key warning — already implemented in
cmd/control/setup.go::initEncryptor:96 (verified).
* **F-18** :assigned compile-time lint — needs custom analyzer
tooling.
* **F-19** CertPEM/CACertPEM in DeviceRegistered event — conflicts
with documented design intent (event-store replay can reconstruct
cert bytes).
* **F-21** Manager.Register silent disconnect — needs proto change
for a new ServerMessage_Disconnect variant.
* **F-22** Registration token SHA-256 vs bcrypt — audit acknowledges
current ULID entropy makes SHA-256 adequate.
* **F-23** Postgres init script password — operator deployment
concern, not application runtime.
* **F-24** Web npm audit — CI tooling, not a code change.
* **F-25** Valkey image licensing — license review, not a security
fix.
* **F-29** Audit-redaction CI check — needs go/analysis tooling.
* **F-30** Empty-string env var as no-op — minor; CONTROL_CORS_ORIGINS
has no published "clear previously-configured" workflow yet.
No proto change. No schema change. Tests-and-fmt clean. Container
deployment requires re-running deploy/setup.sh once after upgrade to
render the new valkey.conf — documented in QUICKSTART next pass.
📝 WalkthroughWalkthroughConsolidates Valkey deployment templating and multiple security/operational hardening edits: constant-time certificate checks, JWT algorithm pinning, client-IP extraction, per-instance rate-limiter eviction with SCIM IP gating, SSO redirect-url validation, agent output-size caps and truncation, CSP header, and audit/schema/test updates. ChangesSecurity hardening and deployment improvements
Sequence Diagram(s)No sequence diagrams generated. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/setup.sh`:
- Line 684: The current chmod call sets "$rendered" to 0400 which may prevent
the valkey process (running under a different UID/GID inside the container) from
reading /etc/valkey/valkey.conf; update the script so the rendered config is
readable by the container user—e.g., change chmod 0400 "$rendered" to chmod 0440
"$rendered" or chmod 0640 "$rendered" and (if needed) chown the file to the
valkey user/group (use the same variable "$rendered" and perform chown before
chmod) so the valkey process UID/GID can read the file at startup.
- Around line 680-683: The awk gsub(/__VALKEY_PASSWORD__/, pw) call in the
script corrupts passwords containing & or \ because gsub's replacement
interprets those characters; update the awk invocation that writes "$template"
-> "$rendered" to perform a safe literal substitution using match() and substr()
(or a loop using match() to find "__VALKEY_PASSWORD__" and rebuild the line with
substrings and the pw variable) instead of gsub(), ensuring the VALKEY_PASSWORD
value is inserted verbatim without special-character expansion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83d6af9c-3e23-43ef-8021-b54dc8f5a7f3
📒 Files selected for processing (17)
cmd/control/main.godeploy/compose.ymldeploy/setup.shdeploy/valkey.conf.templateinternal/api/certificate_handler.gointernal/api/idp_handler.gointernal/api/sso_handler.gointernal/api/terminal_handler.gointernal/auth/interceptor.gointernal/auth/jwt.gointernal/auth/ratelimit.gointernal/dynamicquery/parser.gointernal/handler/agent.gointernal/idp/linker.gointernal/middleware/security.gointernal/scim/auth.gointernal/scim/handler.go
…eadable mode
CodeRabbit caught two real issues in the valkey.conf renderer:
* **awk gsub treats & as the match.** A password containing `&` would
have landed in the rendered config as the literal `__VALKEY_PASSWORD__`
placeholder text instead of the password char. Same hazard with bash's
`${var//pat/rep}` (it also interprets `&`), sed's `s///` (interprets
`&` and `\`), and any other regex-based substitution. Replaced with a
split-and-concatenate loop on `${remaining%%PAT*}` / `${remaining#*PAT}`
so the password is concatenated as a plain string — no interpretation,
no escape needed, adversarial chars (&, \, /, |, $, newlines) all
round-trip verbatim. Verified with `pw='adv&pwd\with/all|the$chars/&'`.
* **chmod 0400 is unreadable to the in-container valkey UID.** The
redis-stack-server image runs as uid 999, which does not match the
host operator's uid that owns the rendered file. 0400 meant the
container redis process couldn't open /etc/valkey/valkey.conf at
startup. Switched to 0644 (still ro,z bind-mounted, so the container
cannot tamper). The password is already on the host in .env at
similar exposure, so 0644 does not materially widen the secret-
handling model — and the audit-F-03 fix (cmdline / /proc visibility)
holds regardless of the rendered file mode.
Atomic write via mktemp + mv kept; switched echo -> printf so a
password starting with `-` is not interpreted as a flag.
…redaction schema fix
Two more findings from the deeper-dive update of SECURITY_AUDIT.md.
## F-33 — Execution-output size caps (server-side)
Cap output streams on the way into the event store so a compromised
agent can't blow up events.data or the Valkey inbox queue.
* internal/control/inbox_worker.go::commandOutputPayload — cap
stdout/stderr at 1 MiB per stream via truncateOutputStream. Keeps
the head and appends an explicit "[truncated ...]" marker so the
UI shows it was capped rather than silently dropping.
* internal/control/inbox_worker.go::handleExecutionOutputChunk —
second-line 64 KiB cap on individual streaming chunks (the
gateway already enforces this in handler/agent.go from F-13, but
a future gateway compromise must not bypass the cap). Oversized
chunks are dropped with WARN.
## F-34 — Audit redaction schema fix (more severe than audit framed)
The audit said "future proto field renames could silently leak
secrets". Deeper inspection found the schema was ALREADY broken for
every secret-bearing action type — paths used snake_case but the
production wire format is camelCase (protojson with UseProtoNames=
false in internal/actionparams/actionparams.go), so the redactor
walked paths that didn't exist on the actual payload.
Concretely:
- ADMIN_POLICY: schema scrubbed params.unit_content (a field that
doesn't exist; the proto was renamed to AdminPolicyParams.custom_config,
wire emit key is "customConfig"). Sudoers/doas.conf fragments
were LEAKING through the audit log.
- REPOSITORY: schema scrubbed params.gpg_key; wire key is "gpgKey".
GPG signing key material was leaking.
- ENCRYPTION: schema scrubbed params.passphrase + params.preshared_key;
EncryptionParams has no passphrase field, and preshared_key's
wire key is "presharedKey". LUKS pre-shared keys were leaking.
- LPS: schema scrubbed params.password + params.preshared_key;
LpsParams has neither (the password is generated agent-side and
surfaces via the lps_password.LpsPasswordRotated event, which IS
correctly redacted). Both paths were phantoms.
- SHELL: schema scrubbed params.script only; ShellParams also has
detectionScript which can carry the same kind of secret-bearing
body.
Fix:
1. Updated actionRedactionSchemas with the correct camelCase paths
and the right set of fields per proto. Removed the phantom LPS
entry entirely.
2. Updated the existing redaction-internal tests to use the
production wire shape (camelCase) — the previous snake_case
fixtures masked the production leak by matching the broken
schema.
3. Added internal/api/audit_redactor_paths_test.go::
TestActionRedactionSchemas_PathsMatchWireFormat — for each
entry in actionRedactionSchemas, build a fully-populated proto
of the corresponding params type, run it through the production
emit path (actionparams.MarshalActionParams), and assert every
redaction path resolves to a real string field on the wire emit.
CI now catches schema drift in lock-step with proto field
renames; a future field rename without a schema update fails the
test instead of silently leaking through the audit log.
Agent-side findings F-31 (REBOOT/SYNC skip signature verification),
F-32 (Debug logs leak shell output), F-35 (journalctl --grep ReDoS)
require changes in the agent repo and will land in a separate PR
there. F-31 in particular needs paired control-side signing of
parameterless actions, which I'll bundle with the F-02 task-signing
follow-up.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/audit_redactor_paths_test.go (1)
142-148: 💤 Low valueFunction name
sortedKeysis misleading — keys are not sorted.The function iterates the map and returns keys in random order. Since it's only used in test error messages, this doesn't affect correctness, but the name could confuse future readers.
🔧 Suggested fix
+import "sort" + -func sortedKeys(m map[string]any) []string { +func sortedKeys(m map[string]any) []string { out := make([]string, 0, len(m)) for k := range m { out = append(out, k) } + sort.Strings(out) return out }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/audit_redactor_paths_test.go` around lines 142 - 148, The helper sortedKeys currently returns map keys in unspecified order but its name implies sorting; update it so it actually returns keys in deterministic sorted order: collect keys from the map (function sortedKeys), call sort.Strings on the slice, and then return the sorted slice so test error messages are stable and name matches behavior (ensure the sort package is imported if not already).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/audit_redactor_paths_test.go`:
- Around line 142-148: The helper sortedKeys currently returns map keys in
unspecified order but its name implies sorting; update it so it actually returns
keys in deterministic sorted order: collect keys from the map (function
sortedKeys), call sort.Strings on the slice, and then return the sorted slice so
test error messages are stable and name matches behavior (ensure the sort
package is imported if not already).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e043270-509a-4680-b942-9acb5b4df654
📒 Files selected for processing (4)
internal/api/audit_handler.gointernal/api/audit_redactor_internal_test.gointernal/api/audit_redactor_paths_test.gointernal/control/inbox_worker.go
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
1 participant
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
Triaged the 30 findings in
SECURITY_AUDIT.mdand implemented the 12 that are valid, scoped, and don't require cross-service infrastructure work.Implemented
High-severity hardening
internal/api/certificate_handler.go:66)frame-ancestors 'none',base-uri 'self',form-action 'self'(internal/middleware/security.go)HS256(internal/auth/jwt.go:155)/proc/<pid>/cmdline: rendered config file (mode0400) + healthcheck viaREDISCLI_AUTH(deploy/compose.yml,deploy/setup.sh,deploy/valkey.conf.template)Medium / defence-in-depth
(slug, client IP), ceiling20/min. Closes the slug-spreading bypass.pm-enroll).false.Low / informational
OutputChunkat 64 KiB, drop oversized with WARN.100kLRU key cap to bound memory under wide-IP-distribution attacks./healthreturnsok, no version leak.StartTerminalcross-user behaviour + recipe forStartTerminal:self.Skipped (18)
Full reasoning in the commit body. Key ones:
Test plan
go vet ./...cleangofmt -l .cleango build ./...cleaninternal/dynamicquery+internal/authtests green (covers F-12 + F-14 + F-20 behaviour)deploy/setup.shonce after upgrade to rendervalkey.conffrom the new template.Summary by CodeRabbit
New Features
Security Enhancements
Tests