fix(cozystack-basics): normalize tenant hostname VAP comparisons with lowerAscii - #3208
Conversation
github-actions
Bot
added
size/L
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 addresses an issue where ValidatingAdmissionPolicy (VAP) objects incorrectly rejected legitimate tenant hostnames when the associated namespace apex label contained uppercase characters. By normalizing both the resource-side host and the apex operand using .lowerAscii(), the platform now correctly handles DNS case semantics regardless of the casing provided in the namespace labels. This change ensures robust validation without impacting existing all-lowercase configurations. 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
|
dosubot
Bot
added
the
area/networking
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughHostname admission policies for Gateway, Ingress, HTTPRoute, and TLSRoute now normalize hostname comparisons with ChangesCase-insensitive hostname policy hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
dc1007c to
f83e751
Compare
![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 case-insensitive matching for tenant apex domains in ValidatingAdmissionPolicies (VAPs) across Gateway, Ingress, and Route hostname policies. By utilizing the CEL .lowerAscii() function, both the resource hostnames and the namespace-derived tenant host labels are normalized to lowercase, preventing false denials when mixed-case labels are used. The changes are accompanied by comprehensive updates to unit tests and new end-to-end BATS tests to ensure robust validation. I have no further feedback to provide as the implementation is complete and well-tested.
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.
Aleksei Sviridkin (lexfrei)
removed
the
area/uncategorized
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
The change makes every hostname comparison in all three tenant hostname VAPs case-fold symmetrically, addresses a real false-deny (and a config-dependent isolation bypass), and is a no-op in the default all-lowercase configuration.
Notes (non-blocking)
Verification performed, no defects found:
- CEL semantics verified.
lowerAscii()is provided by the Kubernetes CEL strings extension (available since 1.25) and folds ASCIIA-Zonly, leaving non-ASCII bytes unchanged. That is the correct semantics for DNS hostname case-insensitivity, which is defined over ASCII. Confirmed against the Kubernetes CEL reference, not the PR body. - Symmetry verified in rendered output.
helm template ... --set _cluster.root-host=example.orgontemplates/ingress-hostname-policy.yamlshows every operand lowered on both sides across the own-apex, subdomain, outside-root-exclusion, and wildcard-gate branches, for bothrules[].hostandtls[].hosts[]. Gateway (gateway-hostname-policy.yaml:37-38) and Route (route-hostname-policy.yaml:45, shared by HTTPRoute doc 0 and TLSRoute doc 2) are likewise symmetric. - Real bug confirmed.
packages/apps/tenant/templates/namespace.yaml:91writesnamespace.cozystack.io/hostverbatim from$computedHost(derived from user-supplied.Values.host, no lowercasing), andpackages/system/cozystack-basics/templates/tenant-root.yaml:8writes it from_cluster.root-host. A mixed-case apex therefore reaches the label unchanged, and pre-fix the case-sensitiveendsWithwrongly denied a tenant's own lowercase in-apex hostname. The uppercase-root-host outside-root-exclusion bypass described in the PR body also traces cleanly. - Tests pass.
helm unittest packages/system/cozystack-basics→ 48/48 passing. New assertions pin.lowerAscii()on each operand/branch, guarding against silent regression.
Claim mismatches
None. All PR-body claims verified [OK].
Caveats
lowerAscii()does not fold non-ASCII / IDN characters. This is not a gap:spec.rules[].host,spec.tls[].hosts[], Gateway listener hostnames, and Route hostnames are all validated as ASCII (DNS-1123 / punycode) before reaching these policies, so ASCII-only folding is complete for the inputs that can occur.- The sibling mutability VAPs
cozystack-tenant-host-policyandcozystack-namespace-host-label-policy(gateway-hostname-policy.yaml:56-160) intentionally remain case-sensitive — they detect change to a host value, where folding would be incorrect (it would letFoo.com→foo.compass as "unchanged"). This is correct and out of scope; no sibling one-sided-normalization pattern remains after this PR.
Phase 5b — Upgrade & Fresh-Install Impact
A. Existing-customer upgrade. No behavior change in the default all-lowercase configuration: for lowercase inputs lowerAscii() is the identity, so re-admission of existing tenant Ingresses/Gateways/Routes yields identical decisions. No values/schema key renamed or removed (files touched: 3 templates + 3 test files + 1 bats file only — no values.yaml / values.schema.json / Chart.yaml, so no make generate artifacts required). No default flip, no RBAC change. Clusters that previously carried a mixed-case apex become less restrictive in the correct direction (a tenant's own hostname is now admitted rather than wrongly denied) and more correct on the isolation-bypass direction (uppercase root-host no longer lets a host under the platform apex slip the exclusion). No resource that was valid pre-fix becomes rejected post-fix.
B. Fresh install. VAPs render and lint clean (helm lint 0 failures; ingress policy still gated behind _cluster.root-host being set). No new CRD/webhook install-time dependency introduced. failurePolicy: Fail and validationActions: [Deny] unchanged; matchConstraints (CREATE/UPDATE on the relevant resources) and matchConditions (tenant-* namespace gate) unchanged.
|
Thanks for the thorough fix — the three template changes and the helm-unittest coverage are solid (both operands + the templated root-host literal normalized, fail-closed guard preserved, no behavior change in the default all-lowercase config). One thing blocks it though: this branch needs a rebase onto current Suggested path:
The template + unittest changes are unaffected by the migration and look good to me as-is. While porting, worth a quick sanity-check that the mixed-case namespace CREATE (host label) is still admitted under the identity Chainsaw |
f83e751 to
e94481c
Compare
github-actions
Bot
added
the
area/uncategorized
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@hack/e2e-chainsaw/gateway/chainsaw-test.yaml`:
- Around line 209-241: Move the cleanup function and its trap registration
before the corresponding RBAC kubectl apply block, so partial Role or
RoleBinding creation is cleaned up if apply fails. Apply this ordering
consistently to the RBAC setup around vap-probe-tenant-create and the analogous
blocks at the other referenced locations, preserving the existing cleanup
commands and behavior.
🪄 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: 631964d3-5b7c-41dd-8ded-5e8329aa621f
📒 Files selected for processing (1)
hack/e2e-chainsaw/gateway/chainsaw-test.yaml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@hack/e2e-chainsaw/gateway/chainsaw-test.yaml`:
- Around line 209-241: Move the cleanup function and its trap registration
before the corresponding RBAC kubectl apply block, so partial Role or
RoleBinding creation is cleaned up if apply fails. Apply this ordering
consistently to the RBAC setup around vap-probe-tenant-create and the analogous
blocks at the other referenced locations, preserving the existing cleanup
commands and behavior.
🪄 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: 631964d3-5b7c-41dd-8ded-5e8329aa621f
📒 Files selected for processing (1)
hack/e2e-chainsaw/gateway/chainsaw-test.yaml
🛑 Comments failed to post (1)
hack/e2e-chainsaw/gateway/chainsaw-test.yaml (1)
209-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Install cleanup traps before creating temporary RBAC grants.
Each multi-resource
kubectl applyruns before itsEXITtrap is registered. Because apply is non-transactional, an error or client timeout can leave a RoleBinding or ClusterRoleBinding active—most seriously granting the default service account namespace patch or creation privileges.Move each
cleanupdefinition andtrap cleanup EXITbefore its corresponding RBAC apply.Also applies to: 275-306, 323-353
🤖 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-chainsaw/gateway/chainsaw-test.yaml` around lines 209 - 241, Move the cleanup function and its trap registration before the corresponding RBAC kubectl apply block, so partial Role or RoleBinding creation is cleaned up if apply fails. Apply this ordering consistently to the RBAC setup around vap-probe-tenant-create and the analogous blocks at the other referenced locations, preserving the existing cleanup commands and behavior.
Aleksei Sviridkin (lexfrei)
removed
the
area/uncategorized
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Re-reviewed the current HEAD hermetically. LGTM with non-blocking notes.
Verified:
helm templateof default and an uppercase-host corner;helm unittest55/55; mutation test flips 7 assertions red when the templates revert to the pre-fix shape, so the tests are non-vacuous.- The commits since the last review added test hardening (mixed-case allow/deny probes, the gateway fail-open pin, the uppercase-root-host test) rather than changing policy logic.
Non-blocking:
- PR body omits the Downstream Repositories checklist section; walking the trigger map shows no downstream repo affected, but it should be stated.
- A VAP only fires on CREATE/UPDATE, so objects already admitted under the old case-sensitive comparison are not retroactively re-validated. Inherent, not fixable here.
- Follow-up for a separate PR: the same case-sensitivity root cause exists in Go,
internal/controller/tenantgateway/reconciler.goandrenderers.godedup apex strings via case-sensitive map keys, so a case-variant duplicate defeats the dedup. A companionstrings.ToLowerwould close it.
e94481c to
bccbdb5
Compare
github-actions
Bot
added
the
area/uncategorized
bccbdb5 to
e5b1502
Compare
… lowerAscii The Ingress, Gateway, and Route hostname ValidatingAdmissionPolicies compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is already lowercase — the apiserver's hostname validation permits nothing else — but the apex operand is the namespace.cozystack.io/host label value, which Kubernetes permits to carry uppercase, and the Ingress policy additionally compares against the templated root host, a free-form platform value that nothing validates for case. A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase root host is the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy degenerates into admitting any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including the root apex itself. Only the anti-wildcard gate still holds. Wrap both operands of every comparison in .lowerAscii() so the match follows DNS case semantics — no behavior change for the all-lowercase default. Each policy reads a label it does not own: the namespace host label is settable out-of-band by a trusted caller, so none of them may assume the apex arrives normalized. The Gateway policy admits any listener hostname when the host label is absent or empty, unlike its two siblings. That is the platform's one documented fail-open exception, recorded in the threat model together with its compensating control, so changing the posture is a security-model decision rather than a side effect of normalizing case. This leaves the behavior untouched and pins its current shape in the tests. Covered by helm-unittest assertions pinning each comparison as a whole operand pair, so dropping the normalization from any single operand is caught; a rendering case that pins the uppercase-root-host escape shut; and e2e probes that set a mixed-case apex on an isolated namespace and assert both halves of the match — a lowercase in-apex HTTPRoute and Ingress are still admitted, while a foreign apex and a hostname that ends with the apex without a dot boundary are still denied. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
e5b1502 to
ba15b38
Compare
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Compiled both old and new expressions in the real apiserver CEL env (k8s.io/apiserver v0.35.0, StrictCostOpt) and ran ~35 hostname shapes through them. lowerAscii() is on both operands of all 15 comparisons, nothing one sided.
Fix is real. With _cluster.root-host: Example.ORG and tenant apex tenant-a.example.org the old expression admits victim-tenant.example.org, dashboard.example.org and bare example.org, all three denied now. With apex label Tenant-A.example.org the old form denies the tenant own app.tenant-a.example.org, now admitted. 21 mutants each dropping one .lowerAscii(), all 21 caught by helm unittest alone.
Cost is a non issue, VAP has no static gate and measured runtime cost goes 83 to 103 for one rule plus one tls host. Capability gate from #3409 intact in all three templates.
One thing before merge, please add the full-e2e label. This PR touches hack/e2e-chainsaw/gateway/** as well as basics, so final_apps is non empty and only the gateway suite runs, while a basics only diff would escalate to all 21 suites through the empty selection net in hack/select-e2e.sh:346-348. Right now the combined PR tests less than the basics change alone would.
| set -eu | ||
| root=$(kubectl get namespace tenant-root -o jsonpath='{.metadata.labels.namespace\.cozystack\.io/host}') | ||
| [ -n "$root" ] || { echo "SETUP FAILURE: tenant-root lacks the namespace.cozystack.io/host label" >&2; exit 1; } | ||
| apex="Case-Probe.${root}" |
There was a problem hiding this comment.
Case-Probe. eats 11 chars and label values cap at 63, so a platform root apex over 52 chars fails the namespace CREATE with a label length error instead of the assertion. Not reachable in CI. [ ${#apex} -le 63 ] next to the two vacuous pass guards already there would close it.
| name: ingress-case-foreign-probe | ||
| spec: | ||
| rules: | ||
| - host: "dashboard.${root}" |
There was a problem hiding this comment.
dashboard.${root} is the live platform dashboard hostname. Benign because ingress-nginx resolves host conflicts oldest wins and the probe is deleted before the step fails, but probe-does-not-exist.${root} proves the same thing with no live host overlap.
3 participants
What this PR does
The Ingress, Gateway, and Route hostname
ValidatingAdmissionPolicyobjects compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is always lowercase — the apiserver's hostname validation permits nothing else — but both apex operands can carry uppercase: thenamespace.cozystack.io/hostlabel value, which Kubernetes permits to carry uppercase and which the tenant chart writes verbatim fromtenant.spec.host, and the templated_cluster.root-host, a free-form platform value that nothing validates for case.A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase
_cluster.root-hostis the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy admits any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including a hostname belonging to another tenant and the root apex itself. Only the anti-wildcard gate still holds.This normalizes both operands of every hostname comparison with
.lowerAscii()(host, apex label, and the templated root-host literal) across all three policies, so the match follows DNS case semantics. There is no behavior change in the default all-lowercase configuration.The Gateway policy's fail-open on an absent or empty host label is deliberately left as it was: that is the platform's one documented fail-open exception in
docs/security/threat-model.md, so changing the posture is a security-model decision rather than a side effect of normalizing case. Its current shape is pinned by a test.Test coverage:
endsWith, across the rule-host and tls-host branches — so dropping the normalization from any single operand is caught. A pattern matching a lone.lowerAscii()would not: the apex appears four times in the Ingress expression, so stripping one operand still leaves three matches._cluster.root-host: Example.ORGpins the outside-root escape shut; it is red pre-fix.Normalizing the apex at the write site — the tenant namespace template and
_cluster.root-host— is tracked separately in #3328. It would not remove the need for this change: the host label is settable out-of-band by a trusted caller, so these policies must not assume the apex arrives normalized.Closes #3205.
Screenshots
N/A (no UI changes).
Release note
Summary by CodeRabbit
Bug Fixes
Tests