fix(cozystack-basics): validate legacy Ingress hostnames against the tenant apex - #3200
Conversation
github-actions
Bot
added
the
size/XL
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request strengthens the security posture of the Cozystack platform by implementing a new ValidatingAdmissionPolicy for legacy Kubernetes Ingress resources. By enforcing hostname constraints similar to those already present for the Gateway API, it prevents tenant namespaces from claiming hostnames outside their designated apex, while still allowing legitimate external domain routing. This change ensures a consistent security boundary across both legacy and modern ingress paths. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
area/uncategorized
dosubot
Bot
added
area/networking
📝 WalkthroughWalkthroughAdds a legacy Ingress hostname admission policy, Helm render tests, e2e coverage, and gateway security-model documentation. ChangesIngress hostname policy enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the cozystack-ingress-hostname-policy ValidatingAdmissionPolicy (VAP) and its binding to enforce tenant-scoped hostname constraints on legacy Ingress resources, which serve as the default publishing path when the Gateway API is disabled. It includes comprehensive unit tests, E2E test cases, and updated documentation. The review feedback highlights a critical security vulnerability where the case-sensitive string comparisons in the CEL expression allow policy bypasses via mixed-case hostnames. To resolve this, the reviewer recommends using .lowerAscii() for case-insensitive validation, updating the unit and E2E tests to verify mixed-case handling, and making a minor wording improvement in the documentation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| {{- $rootHost := index .Values._cluster "root-host" }} | ||
| {{- if $rootHost }} | ||
| {{- $A := `namespaceObject.metadata.labels["namespace.cozystack.io/host"]` }} | ||
| {{- $celValidator := printf `(namespaceObject == null || !has(namespaceObject.metadata.labels) || !("namespace.cozystack.io/host" in namespaceObject.metadata.labels)) ? false : (!has(object.spec.defaultBackend) && (!has(object.spec.rules) || object.spec.rules.all(r, has(r.host) && r.host != "" && (r.host == %s || r.host.endsWith("." + %s) || (!r.host.startsWith("*.") && !(r.host == %q || r.host.endsWith(%q)))))) && (!has(object.spec.tls) || object.spec.tls.all(t, !has(t.hosts) || t.hosts.all(h, h != "" && (h == %s || h.endsWith("." + %s) || (!h.startsWith("*.") && !(h == %q || h.endsWith(%q))))))))` $A $A $rootHost (printf ".%s" $rootHost) $A $A $rootHost (printf ".%s" $rootHost) -}} |
There was a problem hiding this comment.
Case-Sensitivity Bypass Vulnerability
Hostnames in DNS and Ingress controllers (like ingress-nginx) are treated case-insensitively. However, the CEL expression uses case-sensitive string operators (==, endsWith, startsWith).
This allows a tenant to completely bypass the hostname policy and hijack domains under the platform root apex (such as the dashboard or other tenants' domains) simply by using uppercase or mixed-case letters in the host or tls.hosts fields of their Ingress resource (e.g., DASHBOARD.EXAMPLE.ORG instead of dashboard.example.org).
To prevent this, we should convert all hostnames to lowercase using .lowerAscii() before performing any comparisons, and also ensure the platform root host is lowercased.
{{- $rootHost := index .Values._cluster "root-host" | lower }}
{{- if $rootHost }}
{{- $A := "namespaceObject.metadata.labels[\"namespace.cozystack.io/host\"]" }}
{{- $celValidator := printf "(namespaceObject == null || !has(namespaceObject.metadata.labels) || !(\"namespace.cozystack.io/host\" in namespaceObject.metadata.labels)) ? false : (!has(object.spec.defaultBackend) && (!has(object.spec.rules) || object.spec.rules.all(r, has(r.host) && r.host != \"\" && (r.host.lowerAscii() == %s.lowerAscii() || r.host.lowerAscii().endsWith(\".\" + %s.lowerAscii()) || (!r.host.lowerAscii().startsWith(\"*.\") && !(r.host.lowerAscii() == %q || r.host.lowerAscii().endsWith(%q)))))) && (!has(object.spec.tls) || object.spec.tls.all(t, !has(t.hosts) || t.hosts.all(h, h != \"\" && (h.lowerAscii() == %s.lowerAscii() || h.lowerAscii().endsWith(\".\" + %s.lowerAscii()) || (!h.lowerAscii().startsWith(\"*.\") && !(h.lowerAscii() == %q || h.lowerAscii().endsWith(%q))))))))" $A $A $rootHost (printf ".%s" $rootHost) $A $A $rootHost (printf ".%s" $rootHost) -}}There was a problem hiding this comment.
Not changing this in this PR. A mixed-case hostname cannot bypass the policy: the apiserver validates Ingress host / tls.hosts as DNS-1123 (IsDNS1123Subdomain, uppercase rejected — kubernetes/kubernetes#71140), so this policy only ever sees already-lowercased hosts — the same reason the existing cozystack-gateway-hostname-policy and cozystack-route-hostname-policy use a case-sensitive comparison. The .lowerAscii() defense-in-depth hardening is tracked for all three policies together in #3205.
| asserts: | ||
| - documentIndex: 0 | ||
| matchRegex: | ||
| path: spec.validations[0].expression | ||
| pattern: '!\(r\.host == "example\.org" \|\| r\.host\.endsWith\("\.example\.org"\)\)' | ||
| - documentIndex: 0 | ||
| matchRegex: | ||
| path: spec.validations[0].expression | ||
| pattern: '!\(h == "example\.org" \|\| h\.endsWith\("\.example\.org"\)\)' |
There was a problem hiding this comment.
Update the test assertions to match the new case-insensitive CEL expression using .lowerAscii().
asserts:
- documentIndex: 0
matchRegex:
path: spec.validations[0].expression
pattern: '!\(r\.host\.lowerAscii\(\) == "example\.org" \|\| r\.host\.lowerAscii\(\)\.endsWith\("\.example\.org"\)\)'
- documentIndex: 0
matchRegex:
path: spec.validations[0].expression
pattern: '!\(h\.lowerAscii\(\) == "example\.org" \|\| h\.lowerAscii\(\)\.endsWith\("\.example\.org"\)\)'| asserts: | ||
| - documentIndex: 0 | ||
| matchRegex: | ||
| path: spec.validations[0].expression | ||
| pattern: '!r\.host\.startsWith\("\*\."\) && !\(r\.host == "example\.org"' | ||
| - documentIndex: 0 | ||
| matchRegex: | ||
| path: spec.validations[0].expression | ||
| pattern: '!h\.startsWith\("\*\."\) && !\(h == "example\.org"' |
There was a problem hiding this comment.
Update the wildcard test assertions to match the new case-insensitive CEL expression using .lowerAscii().
asserts:
- documentIndex: 0
matchRegex:
path: spec.validations[0].expression
pattern: '!r\.host\.lowerAscii\(\)\.startsWith\("\*\."\) && !\(r\.host\.lowerAscii\(\) == "example\.org"'
- documentIndex: 0
matchRegex:
path: spec.validations[0].expression
pattern: '!h\.lowerAscii\(\)\.startsWith\("\*\."\) && !\(h\.lowerAscii\(\) == "example\.org"'| namespace: tenant-test | ||
| spec: | ||
| rules: | ||
| - host: "dashboard.example.org" |
There was a problem hiding this comment.
Use a mixed-case hostname (e.g., "Dashboard.Example.Org") to verify that the policy is case-insensitive and correctly rejects foreign-apex hostnames regardless of case.
- host: "Dashboard.Example.Org"
| tls: | ||
| - hosts: | ||
| - "harbor.test.example.org" | ||
| secretName: noop-tls | ||
| rules: | ||
| - host: "harbor.test.example.org" |
There was a problem hiding this comment.
Use mixed-case hostnames (e.g., "Harbor.Test.Example.Org") to verify that the policy correctly allows in-apex hostnames regardless of case.
tls:
- hosts:
- "Harbor.Test.Example.Org"
secretName: noop-tls
rules:
- host: "Harbor.Test.Example.Org"
| 5. **`cozystack-namespace-host-label-policy`** — VAP on core `v1 Namespace` CREATE/UPDATE. Rejects any set or change of the `namespace.cozystack.io/host` label, except by the same trusted-caller whitelist as layer 4. This closes both first-time label writes on CREATE and first-time adds on UPDATE — only cozystack/Flux service accounts (which apply the tenant chart) can stamp the label. | ||
| 6. **Render-time `fail` in cozystack-basics.** The cozystack-basics chart fails the helm render if `_cluster.gateway-attached-namespaces` contains any `tenant-*` entry. Triggers on the helm-install path before the cluster ever sees the values — complements layer 3 which triggers at `kubectl apply` time. | ||
| 7. **`cozystack-route-hostname-policy`** — VAP on `gateway.networking.k8s.io/v1 HTTPRoute` and `v1alpha2 TLSRoute` CREATE/UPDATE. Scoped to `tenant-*` namespaces (cozy-* are cluster-admin-managed and trusted to publish under any apex). Rejects any `spec.hostnames` entry that is not equal to the namespace's `namespace.cozystack.io/host` label or a subdomain of it. Defense-in-depth against an app chart bug or supply-chain compromise that emits Gateway API resources outside the tenant's apex — tenants in Cozystack do not hold `gateway.networking.k8s.io/*` RBAC by design, so this is not a tenant-user defense. The within-apex cross-namespace case (a tenant chart claiming a hostname that is published by a `cozy-*` app) is handled by the controller at reconciliation time: when two routes from different namespaces claim the same hostname, the `cozy-*` namespace wins and the loser receives a `HostnameConflict` condition under the controller's name in `Status.Parents`. | ||
| 8. **`cozystack-ingress-hostname-policy`** — VAP on core `networking.k8s.io/v1 Ingress` CREATE/UPDATE. Gateway API is opt-in and **off by default**, so in the default configuration tenant applications publish through a legacy Ingress on the shared ingress-nginx; layers 1-7 constrain tenant hostnames on the opt-in Gateway path, and this VAP adds a hostname constraint on the default Ingress path. Scoped to `tenant-*` namespaces (cozy-* are cluster-admin-managed and trusted). A hostname on `spec.rules[].host` or `spec.tls[].hosts[]` is allowed when it is within the namespace's own `namespace.cozystack.io/host` apex, OR when it lies entirely outside the platform root apex (`_cluster.root-host`) — the second case lets a tenant route its own external custom domain (for example the `kubernetes` app's Proxied `addons.ingressNginx.hosts`, which routes a user-supplied domain to a nested cluster). A hostname that falls under the platform root apex but outside the namespace's own apex is rejected; a rule with no host (an unbounded catch-all) and `spec.defaultBackend` (a catch-all for unmatched traffic) are also rejected. Fail-closed: a `tenant-*` namespace missing its host label is denied, and the policy renders only when `_cluster.root-host` is set. This bounds which apexes a tenant Ingress may claim under the platform domain; it does not attempt to resolve every possible hostname collision. |
There was a problem hiding this comment.
The word "bounds" is used as a verb here, but "limits" or "constrains" is more idiomatic and clear in this context.
| 8. **`cozystack-ingress-hostname-policy`** — VAP on core `networking.k8s.io/v1 Ingress` CREATE/UPDATE. Gateway API is opt-in and **off by default**, so in the default configuration tenant applications publish through a legacy Ingress on the shared ingress-nginx; layers 1-7 constrain tenant hostnames on the opt-in Gateway path, and this VAP adds a hostname constraint on the default Ingress path. Scoped to `tenant-*` namespaces (cozy-* are cluster-admin-managed and trusted). A hostname on `spec.rules[].host` or `spec.tls[].hosts[]` is allowed when it is within the namespace's own `namespace.cozystack.io/host` apex, OR when it lies entirely outside the platform root apex (`_cluster.root-host`) — the second case lets a tenant route its own external custom domain (for example the `kubernetes` app's Proxied `addons.ingressNginx.hosts`, which routes a user-supplied domain to a nested cluster). A hostname that falls under the platform root apex but outside the namespace's own apex is rejected; a rule with no host (an unbounded catch-all) and `spec.defaultBackend` (a catch-all for unmatched traffic) are also rejected. Fail-closed: a `tenant-*` namespace missing its host label is denied, and the policy renders only when `_cluster.root-host` is set. This bounds which apexes a tenant Ingress may claim under the platform domain; it does not attempt to resolve every possible hostname collision. | |
| 8. **`cozystack-ingress-hostname-policy`** — VAP on core `networking.k8s.io/v1 Ingress` CREATE/UPDATE. Gateway API is opt-in and **off by default**, so in the default configuration tenant applications publish through a legacy Ingress on the shared ingress-nginx; layers 1-7 constrain tenant hostnames on the opt-in Gateway path, and this VAP adds a hostname constraint on the default Ingress path. Scoped to `tenant-*` namespaces (cozy-* are cluster-admin-managed and trusted). A hostname on `spec.rules[].host` or `spec.tls[].hosts[]` is allowed when it is within the namespace's own `namespace.cozystack.io/host` apex, OR when it lies entirely outside the platform root apex (`_cluster.root-host`) — the second case lets a tenant route its own external custom domain (for example the `kubernetes` app's Proxied `addons.ingressNginx.hosts`, which routes a user-supplied domain to a nested cluster). A hostname that falls under the platform root apex but outside the namespace's own apex is rejected; a rule with no host (an unbounded catch-all) and `spec.defaultBackend` (a catch-all for unmatched traffic) are also rejected. Fail-closed: a `tenant-*` namespace missing its host label is denied, and the policy renders only when `_cluster.root-host` is set. This limits which apexes a tenant Ingress may claim under the platform domain; it does not attempt to resolve every possible hostname collision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/extra/gateway/README.md (1)
94-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLayer 8 description omits the wildcard-vs-outside-root rejection edge case.
The bullet documents the own-apex and outside-root-apex allow paths and the under-root-outside-apex / hostless / defaultBackend deny paths, but doesn't mention that a wildcard hostname taken through the outside-root path is also denied (the CEL gates the outside-root branch on
!r.host.startsWith("*."), and this is specifically exercised by the new e2e test for*.orginhack/e2e-apps/gateway.bats). Worth a short clause noting wildcards are rejected on the outside-root path (while a wildcard under the tenant's own apex is still allowed).🤖 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 `@packages/extra/gateway/README.md` at line 94, Update the Layer 8 bullet in the gateway README to mention the wildcard edge case: in the hostname policy description, add that the “outside the platform root apex” allow path does not permit wildcard hostnames, while wildcards remain allowed only when they stay within the namespace’s own apex. Refer to the `cozystack-ingress-hostname-policy` description and keep the wording aligned with the existing own-apex/outside-root allow and deny clauses.hack/e2e-apps/gateway.bats (3)
404-699: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo e2e coverage for a
tenant-*namespace missing its own host label.The README (Layer 8 description) states the policy fails closed when a
tenant-*namespace lacks itsnamespace.cozystack.io/hostlabel, but none of the new tests exercise that path end-to-end (only the helm-unittest layer, per the stack outline, presumably covers it). Consider adding one e2e probe against atenant-*namespace without the label to pin the fail-closed contract at the live-apiserver layer too, matching the existing diagnostic test pattern at lines 62-78.🤖 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 `@hack/e2e-apps/gateway.bats` around lines 404 - 699, Add an e2e test that exercises the fail-closed path for a tenant-* namespace missing its namespace.cozystack.io/host label, since the current gateway.bats coverage only probes labeled namespaces. Use the existing diagnostic/probe style from cozystack-ingress-hostname-policy and create a namespace-scoped Ingress in a tenant-* namespace without the label, then assert the ValidatingAdmissionPolicy rejects it with the expected admission failure. Keep the new test alongside the other ingress hostname policy cases so the live-apiserver contract is pinned in the same suite.
501-533: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueWeaker assertions for hostless/empty-host/defaultBackend rejections.
These three tests only assert
grep -qi "ValidatingAdmissionPolicy"on rejection, unlike the apex-violation tests which also assert the specific"must be within"substring from the policy's sharedmessageExpression. Since the policy uses one message for every violation branch, the same substring check would also apply here and slightly strengthen these tests against false positives from an unrelated VAP/webhook rejection.Also applies to: 535-570, 572-598
🤖 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 `@hack/e2e-apps/gateway.bats` around lines 501 - 533, The rejection checks in the hostless, empty-host, and defaultBackend ingress tests are too weak because they only match ValidatingAdmissionPolicy generically. Update the affected test cases in gateway.bats, including the cozystack-ingress-hostname-policy VAP test and the related hostless/empty-host/defaultBackend assertions, to also verify the shared policy message substring "must be within" alongside the existing ValidatingAdmissionPolicy check. This should mirror the stronger apex-violation tests and reduce false positives from unrelated admission failures.
428-457: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffInline cleanup inside test bodies, per existing file convention.
Each new test performs its own pre-clean and/or post-apply
kubectl deletecleanup inline rather than relying on framework-level teardown. This mirrors the file's pre-existing pattern (used throughout the rest ofgateway.bats), so it's consistent with current style — but it repeats the same issue previously flagged forharbor.bats.Based on learnings, in E2E bats tests like
hack/e2e-apps/harbor.bats, inline cleanup should be replaced by framework-level teardown/cleanup, applying "to all bats tests under hack/e2e-apps (and similar E2E suites) to ensure consistent and reliable cleanup without embedding it in individual tests."Given the scope of an already-established pattern across dozens of existing tests in this file, a full migration here would be high effort; flagging for awareness rather than blocking this PR.
Also applies to: 466-499, 506-533, 542-570, 576-598, 604-630, 639-661, 670-698
🤖 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 `@hack/e2e-apps/gateway.bats` around lines 428 - 457, The new gateway.bats tests are doing inline pre/post test cleanup with repeated kubectl delete calls inside the test bodies, which should be moved to the suite’s shared teardown/cleanup pattern instead. Update the affected test blocks in gateway.bats (including the ingress-out-of-apex-host-probe flow and the other newly added cases) to rely on framework-level cleanup helpers rather than embedding deletion logic in each test. Keep the test bodies focused on setup, apply, and assertions, and use the existing bats teardown/cleanup conventions already used elsewhere in the suite.Source: Learnings
🤖 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 `@hack/e2e-apps/gateway.bats`:
- Around line 404-699: Add an e2e test that exercises the fail-closed path for a
tenant-* namespace missing its namespace.cozystack.io/host label, since the
current gateway.bats coverage only probes labeled namespaces. Use the existing
diagnostic/probe style from cozystack-ingress-hostname-policy and create a
namespace-scoped Ingress in a tenant-* namespace without the label, then assert
the ValidatingAdmissionPolicy rejects it with the expected admission failure.
Keep the new test alongside the other ingress hostname policy cases so the
live-apiserver contract is pinned in the same suite.
- Around line 501-533: The rejection checks in the hostless, empty-host, and
defaultBackend ingress tests are too weak because they only match
ValidatingAdmissionPolicy generically. Update the affected test cases in
gateway.bats, including the cozystack-ingress-hostname-policy VAP test and the
related hostless/empty-host/defaultBackend assertions, to also verify the shared
policy message substring "must be within" alongside the existing
ValidatingAdmissionPolicy check. This should mirror the stronger apex-violation
tests and reduce false positives from unrelated admission failures.
- Around line 428-457: The new gateway.bats tests are doing inline pre/post test
cleanup with repeated kubectl delete calls inside the test bodies, which should
be moved to the suite’s shared teardown/cleanup pattern instead. Update the
affected test blocks in gateway.bats (including the
ingress-out-of-apex-host-probe flow and the other newly added cases) to rely on
framework-level cleanup helpers rather than embedding deletion logic in each
test. Keep the test bodies focused on setup, apply, and assertions, and use the
existing bats teardown/cleanup conventions already used elsewhere in the suite.
In `@packages/extra/gateway/README.md`:
- Line 94: Update the Layer 8 bullet in the gateway README to mention the
wildcard edge case: in the hostname policy description, add that the “outside
the platform root apex” allow path does not permit wildcard hostnames, while
wildcards remain allowed only when they stay within the namespace’s own apex.
Refer to the `cozystack-ingress-hostname-policy` description and keep the
wording aligned with the existing own-apex/outside-root allow and deny clauses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12960f9b-cec0-49ef-9605-ca467d33fbd0
📒 Files selected for processing (4)
hack/e2e-apps/gateway.batspackages/extra/gateway/README.mdpackages/system/cozystack-basics/templates/ingress-hostname-policy.yamlpackages/system/cozystack-basics/tests/ingress-hostname-policy_test.yaml
1c136c3 to
d2be06b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/cozystack-basics/templates/ingress-hostname-policy.yaml (1)
62-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: hoist the repeated apex/root checks into
spec.variables.The validator repeats the namespace-apex accessor and root-apex comparison several times across the rules and TLS branches. Defining them once would make this security-critical CEL easier to audit and reduce the chance of future edits diverging between branches.
ValidatingAdmissionPolicysupportsspec.variablesinadmissionregistration.k8s.io/v1.🤖 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 `@packages/system/cozystack-basics/templates/ingress-hostname-policy.yaml` around lines 62 - 86, The CEL in the ValidatingAdmissionPolicy repeats the same namespace-apex and root-apex checks in both the rules and TLS branches, making it harder to audit and maintain. Hoist the shared expressions into spec.variables on cozystack-ingress-hostname-policy, then reference those variables from the existing $celValidator/messageExpression logic so the namespaceObject.metadata.labels and rootHost comparisons are defined once and used consistently throughout the policy.
🤖 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 `@packages/system/cozystack-basics/templates/ingress-hostname-policy.yaml`:
- Around line 62-86: The CEL in the ValidatingAdmissionPolicy repeats the same
namespace-apex and root-apex checks in both the rules and TLS branches, making
it harder to audit and maintain. Hoist the shared expressions into
spec.variables on cozystack-ingress-hostname-policy, then reference those
variables from the existing $celValidator/messageExpression logic so the
namespaceObject.metadata.labels and rootHost comparisons are defined once and
used consistently throughout the policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12d4742d-de72-49b1-8c39-dfacf79c4ca6
📒 Files selected for processing (4)
hack/e2e-apps/gateway.batspackages/extra/gateway/README.mdpackages/system/cozystack-basics/templates/ingress-hostname-policy.yamlpackages/system/cozystack-basics/tests/ingress-hostname-policy_test.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/extra/gateway/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/e2e-apps/gateway.bats
- packages/system/cozystack-basics/tests/ingress-hostname-policy_test.yaml
…tenant apex The Gateway (cozystack-gateway-hostname-policy) and Route (cozystack-route-hostname-policy) admission policies constrain tenant hostnames on the Gateway API path. Gateway API is opt-in and off by default, so in the default configuration tenant applications publish through a legacy networking.k8s.io/Ingress on the shared ingress-nginx, which had no equivalent policy. Add a fail-closed ValidatingAdmissionPolicy + Binding for Ingress in tenant-* namespaces. A hostname on spec.rules[].host or spec.tls[].hosts[] is allowed when it is within the namespace's own apex (namespace.cozystack.io/host), or when it is a concrete (non-wildcard) name entirely outside the platform root apex (_cluster.root-host); a hostname under the platform root apex but outside the namespace's own apex, and a wildcard taken through the outside-root path (which could match the platform apex itself), are rejected. The outside-root branch keeps the kubernetes app's Proxied exposeMethod working, since it routes a user-supplied external domain to a nested cluster. A rule with no host (absent or empty, an unbounded catch-all) and spec.defaultBackend are also rejected. The policy templates the platform root apex into its CEL and renders only when _cluster.root-host is set, so it is never shipped in a state that would admit every hostname. A tenant-* namespace missing its own apex label is denied. Cover the policy with helm-unittest (render, including the root-host, empty-host, and wildcard guards) and behavioral admission tests against the live apiserver (in-apex allow, external-domain allow, own-apex-wildcard allow, external-tls allow, under-root-outside-apex deny, prefix-adjacent-under-root deny, wildcard-matching-apex deny, hostless deny, empty-string-host deny, defaultBackend deny), and document it as an additional layer in the gateway security model. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
d2be06b to
d89ad70
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/system/cozystack-basics/templates/ingress-hostname-policy.yaml`:
- Around line 52-62: The fail-closed precondition in the ingress hostname policy
only checks for the presence of namespace.cozystack.io/host, so an empty string
can still bypass the intended rejection path. Update the CEL guard in the
$celValidator template to treat a blank namespace.cozystack.io/host value the
same as missing, while keeping the existing namespaceObject and labels checks
intact. Locate the logic around $rootHost, $A, and the generated expression in
the ingress-hostname-policy.yaml template and add an explicit empty-value
condition before allowing the custom-domain branch.
🪄 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: 6f1fb6f2-9da7-4c9f-b63d-bfec6d11258e
📒 Files selected for processing (4)
hack/e2e-apps/gateway.batspackages/extra/gateway/README.mdpackages/system/cozystack-basics/templates/ingress-hostname-policy.yamlpackages/system/cozystack-basics/tests/ingress-hostname-policy_test.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/extra/gateway/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/system/cozystack-basics/tests/ingress-hostname-policy_test.yaml
- hack/e2e-apps/gateway.bats
VerdictLGTM with non-blocking notes The CEL policy enforces exactly what the PR body and README claim; I verified the allow/deny logic against a full truth table and the sibling Route policy, and found no bypass of the platform apex or a sibling tenant. One upgrade-impact edge case on pre-existing non-conforming Ingresses is undocumented (MINOR). Findings[MINOR] The VAP uses Claim mismatches[OK] All PR-body claims verified. Specifically checked and confirmed:
Caveats
|
## What this PR does Adds `docs/security/threat-model.md`, a design-level threat model for Cozystack. It documents the actors, the actions that cross trust boundaries, and the security goals and non-goals, organized by trust boundary (T0–T4) following the CNCF TAG-Security Actors/Actions model. It records the usage and multitenancy model (managed-services backend vs direct dashboard/API access; combined hard multitenancy for tenant VMs and Kubernetes clusters, soft multitenancy for managed data services), the primary trust-boundary invariant (tenant RBAC grants no HelmRelease or arbitrary core writes; the aggregated API server fixes the chart reference server-side), and the two admission enforcement points (core kube-apiserver vs `cozystack-api`). Hostname integrity covers both the Gateway API path and the default legacy-Ingress path (`cozystack-ingress-hostname-policy`, #3200). Every architectural claim cites a source file so the model can be re-verified as the code evolves. This is the in-repo companion to the project's CNCF TAG-Security self-assessment, which references it. ### Screenshots N/A — documentation only. ### Release note ```release-note docs(security): add a design-level threat model (docs/security/threat-model.md) ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a security threat model for the platform. * Clarified system boundaries, trust tiers, and the main actors involved. * Documented key security goals, non-goals, and important enforcement points. * Described expected behavior for tenant actions, hostname handling, and cross-boundary access. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
What this PR does
The Gateway (
cozystack-gateway-hostname-policy) and Route (cozystack-route-hostname-policy) admission policies constrain tenant hostnames to the tenant apex on the Gateway API path. Gateway API is opt-in and off by default, so in the default configuration tenant applications publish through a legacynetworking.k8s.io/Ingresson the shared ingress-nginx, which had no equivalent policy.This adds
cozystack-ingress-hostname-policy, a fail-closedValidatingAdmissionPolicy+ binding forIngressintenant-*namespaces — the legacy-Ingress counterpart of the existing Gateway/Route hostname policies, and a new Layer 8 in the gateway security model. A hostname onspec.rules[].hostorspec.tls[].hosts[]is allowed when it is within the namespace's own apex (namespace.cozystack.io/host), or when it is a concrete (non-wildcard) name entirely outside the platform root apex (_cluster.root-host). The second case keeps thekubernetesapp's ProxiedexposeMethodworking, since it routes a user-supplied external domain to a nested cluster. A hostname under the platform root apex but outside the namespace's own apex, a wildcard taken through the outside-root path (which could otherwise match the platform apex itself), a rule with no host (absent or empty — an unbounded catch-all), andspec.defaultBackendare rejected.The policy is fail-closed: a
tenant-*namespace missing its own apex label is denied, and the policy renders only when_cluster.root-hostis set, so it is never shipped in a state that would admit every hostname.cozy-*namespaces are cluster-admin-managed and not gated. This is defense-in-depth in the same family as the Gateway/Route policies — by design tenants hold only read access toIngress.The change ships helm-unittest coverage (render, plus the root-host, empty-host, and wildcard guards) and behavioral admission tests against the live apiserver in
hack/e2e-apps/gateway.bats(in-apex allow, external-domain allow, under-root-outside-apex deny, wildcard-matching-apex deny, hostless deny, empty-string-host deny, defaultBackend deny), and documents the new layer inpackages/extra/gateway/README.md.Screenshots
N/A — no UI change.
Release note
Summary by CodeRabbit
cozystack-ingress-hostname-policy) to enforce allowed legacy Ingress rule and TLS hostnames for tenant namespaces, while blocking unsafe catch-all configurations (including missing/empty hosts andspec.defaultBackend) when the platform root host is configured.