Repository navigation
feat(redis)!: refuse external access without TLS at admission - #4639
Conversation
github-actions
Bot
added
size/L
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughAdds a capability-gated Redis admission policy for external access and TLS changes. The policy is tested, and Redis parameter descriptions and security documentation now describe its rules. ChangesRedis external TLS policy
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Caller
participant AdmissionAPI
participant RedisPolicy
Caller->>AdmissionAPI: Submit Redis create or update
AdmissionAPI->>RedisPolicy: Evaluate external access and TLS rules
RedisPolicy-->>AdmissionAPI: Allow or deny request
AdmissionAPI-->>Caller: Return admission result
Merge Risk: ⚪ Minimal · up to The Redis admission rules appear ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change restricts plaintext exposure without expanding tenant permissions. Its protection is conditional: legacy endpoints remain editable, privileged repair paths retain exceptions, and policy availability and concurrent updates limit the guarantee. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
16a502a to
868df53
Compare
A Redis release with external: true and TLS off publishes a plaintext endpoint outside the cluster, and nothing stopped a tenant from asking for one. The chart never infers TLS from external, because enabling it moves Redis to a TLS-only listener, so external: true without an explicit tls.enabled: true is exactly that case. A ValidatingAdmissionPolicy on the Redis kind now refuses it with a message naming the release and a remedy that works. TLS is fixed when a Redis instance is created: the operator's RedisFailover schema refuses a change, but the Redis API accepts it. A spec's tls.enabled is therefore a request, not the instance's state, and cannot prove on UPDATE that an instance has TLS; trusting it would let a tenant flip it, expose the instance, and remove it again. So external access can be enabled only at creation, where tls.enabled does decide the instance, and any UPDATE turning it on is refused. The RedisFailover schema does not exist before the first reconcile, so a release admitted as external with TLS could have TLS dropped before the instance is created. tls.enabled is therefore frozen while external access stays on. Withdrawing external access in the same edit lifts the freeze, which leaves a tenant a way to unblock an instance whose TLS was flipped earlier, and the callers trusted by the tenant host policy are exempt so an administrator can repair one in place. Other edits to an instance that is already external pass, so releases exposed in plaintext before the policy keep working. Postgres is left out on purpose. CNPG pins ssl=on, and its default pg_hba accepts plaintext from any client that asks, whatever the chart's tls.enabled says; that field only decides whether the external hostname goes into the certificate. A policy keyed on it would close nothing. The decisions are tested by compiling the shipped template with the apiserver's own CEL compiler, since a structural chart test cannot show what the policy admits. BREAKING CHANGE: creating a Redis with external: true and without tls.enabled: true, which applied before, is now refused, including a GitOps re-apply of such a manifest into a fresh cluster. Turning external on for an existing Redis is refused as well, so exposing an instance means creating a new one with TLS, and tls.enabled can no longer be changed on an instance with external access. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
868df53 to
800bb03
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
The guard sits at the point where redis exposure is actually decided. The external LoadBalancer in packages/apps/redis/templates/service.yaml renders only from .Values.external, and that value reaches the chart solely through the redises CR the policy gates. The policy runs inside cozystack-api the same way the already-shipped cozystack-tenant-host-policy does. One MINOR comment-bloat note below.
Findings
- [MINOR]
packages/system/cozystack-basics/templates/database-external-tls-policy.yaml:4, 25-line#prose essay ships verbatim into the applied VAP manifest
Caveats
- Live end-to-end enforcement (a real
cozystack-apidenying a tenant apply) is out of scope for this hermetic review; it rests on three static signals, not an assumption:pkg/cmd/server/start.go:103-120disables only MutatingAdmissionPolicy and keeps ValidatingAdmissionPolicy;packages/system/cozystack-api/templates/rbac.yaml:21-23grants list/watch onvalidatingadmissionpolicies/validatingadmissionpolicybindings(not the mutating ones) so the plugin sources cluster policies; andcozystack-tenant-host-policyis an identical shipped VAP on the same aggregated group. Acozystack-pr-testrun can confirm it live.
Recommended follow-ups (non-blocking)
- The policy, its file, and its inline comment are named for
database, but the binding matches onlyredises. The same plaintext endpoint outside the cluster is reachable by other first-party kinds a tenant can create (clusterroles.yaml:260-273):valkeyrenders the same redis-operator LoadBalancer on 6379 and has no TLS option anywhere in the chart (#4622),postgresaccepts plaintext sessions regardless of the field (#4619), and atcp-balancercan forward to a redis Service (called out as by-design in the PR body). The inline comment states the inclusion criterion as "engines whosetls.enableddecides whether the server takes plaintext"; kafka, qdrant and nats also fit that wording (an explicittls.enabled: falsekeeps a plaintext listener up whileexternalis on) yet are out of scope. Scoping to redis is deliberate (feat(redis)!, decision record in community#91), so this is a wording note rather than a defect: when the 25-line comment moves to docs (above), narrow its criterion to what the policy actually binds, otherwise the generic name reads as a platform-wide guarantee the policy does not make. - A non-boolean
external("true",1) is admitted by the CEL ("true" == trueevaluates to false, sonewExternalis false) and the aggregated apiserver does not type-check the spec against the kind's openAPISchema (it is used only for defaulting,rest.go:112-137), so the value is refused one layer down by the redis chart's own values schema rather than at admission. Fail-closed, but the tenant sees a failed HelmRelease instead of a clear denial. Optional hardening if you want the admission layer to own that message.
| {{- /* Render only where the ValidatingAdmissionPolicy API is served. Same guard as gateway-hostname-policy.yaml. */}} | ||
| {{- if .Capabilities.APIVersions.Has "admissionregistration.k8s.io/v1/ValidatingAdmissionPolicy" }} | ||
| --- | ||
| # Refuses a database exposed outside the cluster with TLS off. The kinds are |
There was a problem hiding this comment.
[MINOR] 25-line # prose essay ships verbatim into the applied VAP manifest
The block on lines 4-28 (# Refuses a database exposed...) is a # YAML comment, not a Go-template comment, so render does not strip it: helm template --show-only templates/database-external-tls-policy.yaml --api-versions admissionregistration.k8s.io/v1/ValidatingAdmissionPolicy | grep -c '^#' prints 27, so the essay lands in the object applied to every cluster and drifts stale once the CEL changes. Convert it to a {{- /* */}} Go-template comment (stripped at render, the accepted fix here) or move it to the docs site with a one-line pointer. The capability-guard comment on line 1 is already the correct {{- /* */}} form.
2 participants
What this PR does
An admission policy now refuses a Redis release that would expose a plaintext endpoint outside the cluster. Redis never turns TLS on by itself, so
external: truewithouttls.enabled: trueis exactly that case. The error names the release and a remedy that works.The rule has three parts, because TLS is fixed when a Redis instance is created: the RedisFailover schema refuses a change, while the Redis API accepts one and the HelmRelease then fails. A spec's
tls.enabledis therefore a request, not proof of what the instance runs.external: trueneedstls.enabled: true.externalon is refused whatevertls.enabledsays.externalstays on,tls.enabledcannot change. Before the first reconcile nothing else stops TLS being dropped from a release admitted as external with TLS. Withdrawingexternalin the same edit lifts the freeze, and the callers trusted bycozystack-tenant-host-policyare exempt, so an instance whose TLS was flipped earlier can still be unblocked.Edits to a release that was already external pass, so releases exposed in plaintext before this policy keep working. The guarantee covers the Redis app. A generic publisher such as
tcp-balancercan still forward to a plaintext Redis, by design.This diverges from the Security section of the
external-database-exposureproposal, which refuses only an explicittls.enabled: falseand covers every engine. That rule was written before redis had TLS and matches only postgres. Postgres is left out here on purpose: CloudNativePG keepsssl=onand its default pg_hba accepts plaintext from any client that asks, whatever the chart'stls.enabledsays, so an admission rule on that field closes nothing. That gap is #4619. The proposal update and a decision record are cozystack/community#91.The policy follows
cozystack-tenant-host-policy: a ValidatingAdmissionPolicy incozystack-basics, enforced insidecozystack-apifor the aggregated kind. Its decisions are tested in Go by compiling the shipped template with the apiserver's own CEL compiler and running Redis objects through it, including the bypass sequences found in review. A structural chart test can't show what the policy admits. The redis README, theexternalfield description and the threat model describe the new rule.Related: #4622 (valkey
externalis always plaintext), #4624 (the redistls.enableddescription implies a live switch).Screenshots
Downstream repositories
I walked the trigger map against the diff and nothing in it fires, but I left the boxes empty because cozystack/community is reached anyway: the
external-database-exposureproposal still states the old admission rule. Its update and a decision record are cozystack/community#91.Release note
Summary by CodeRabbit