feat: remove identity headers and simplify subscription info#645
Merged
jrhyness merged 7 commits intoopendatahub-io:mainfrom Mar 31, 2026
Merged
feat: remove identity headers and simplify subscription info#645jrhyness merged 7 commits intoopendatahub-io:mainfrom
jrhyness merged 7 commits intoopendatahub-io:mainfrom
Conversation
Remove X-MaaS-Username, X-MaaS-Group, X-MaaS-Key-Id, and X-MaaS-Subscription headers from requests forwarded to upstream model workloads (defense-in-depth). All identity information remains available to TokenRateLimitPolicy and gateway telemetry through AuthPolicy's filters.identity mechanism, but is not exposed to model runtime pods to prevent accidental disclosure in logs or dumps. Changes: - Remove response.success.headers from controller-generated AuthPolicies - Add test verifying headers not forwarded and identity data available - Update docs to explain security model and scope (model routes only) Out of scope: maas-api static AuthPolicy unchanged (still uses headers for ExtractUserInfo middleware on maas-api-route). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Pass the complete subscription-info object instead of extracting individual fields, simplifying the AuthPolicy and enabling consumers to access any field without controller changes. Changes: - Add subscription_info field with full auth.metadata["subscription-info"] object - Remove redundant top-level fields: - organizationId (now: subscription_info.organizationId) - costCenter (now: subscription_info.costCenter) - subscription_labels (now: subscription_info.labels) - Update TelemetryPolicy to access nested fields - Add test coverage verifying subscription_info returns full object Benefits: - Cleaner policy: single object vs multiple extracted fields - Extensibility: new subscription fields automatically available - Consistency: field name matches source (subscription-info → subscription_info) Consumers should update expressions: Before: auth.identity.organizationId After: auth.identity.subscription_info.organizationId Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fix CEL syntax error in AuthPolicy subscription_info field that was causing AuthConfig validation failures and blocking model access. The has() macro in Authorino/Kuadrant CEL does not support map subscript notation has(auth.metadata["subscription-info"]). Changed to check for a specific field within the object: has(auth.metadata["subscription-info"].name). Error message before fix: ERROR: <input>:1:18: invalid argument to has() macro | has(auth.metadata["subscription-info"]) ? ... | .................^ This was causing AuthPolicies to remain in Enforced=False state, preventing smoke tests from passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Keep X-MaaS-Subscription header injection for Istio Telemetry's per-subscription latency tracking. Other identity headers (Username, Group, Key-Id) remain removed for defense-in-depth. Istio Telemetry runs in Envoy and cannot access auth.identity context - it can only read request headers. The X-MaaS-Subscription header is server-controlled (injected by Authorino from validated subscription), not client-provided. Updates test to verify X-MaaS-Subscription is present while other identity headers remain absent.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoved forwarding of identity headers ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Security & Actionable Issues
CVE references: none directly applicable to these configuration/documentation/test changes. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/configuration-and-management/maas-controller-overview.md`:
- Line 238: Docs claim X-MaaS-Subscription is not injected but code in
maasauthpolicy_controller.go (around the injection logic at lines handling Istio
telemetry, e.g., the block at maasauthpolicy_controller.go:462-468) and the unit
test maasauthpolicy_controller_test.go (assertions at ~1120-1123) show
X-MaaS-Subscription is injected; update the documentation text to reflect that
X-MaaS-Subscription is injected for Istio telemetry/per-subscription latency
tracking, and add the proposed clarifying sentence after the bullet list to
state this exception and its purpose.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d1e42bf8-5820-4e26-9ef5-9bc6eb42f154
📒 Files selected for processing (5)
deployment/base/observability/gateway-telemetry-policy.yamldocs/content/architecture.mddocs/content/configuration-and-management/maas-controller-overview.mdmaas-controller/pkg/controller/maas/maasauthpolicy_controller.gomaas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go
docs/content/configuration-and-management/maas-controller-overview.md
Outdated
Show resolved
Hide resolved
Update documentation to reflect that X-MaaS-Subscription header IS injected (exception to the identity header removal), while other identity headers (Username, Group, Key-Id) remain removed. Explains why Istio requires the header (cannot access auth.identity context) and clarifies the value is server-controlled, not client-provided.
Use apiKeyValidation.subscription (bound) instead of subscription-info.name (selected) for the X-MaaS-Subscription header. For K8s tokens, the expression returns empty string so no header is injected. This aligns with actual usage: API keys for inference (needs header for Istio Telemetry), K8s tokens for /v1/models catalog browsing (no subscription tracking needed). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
jrhyness
added
3.4
ready-for-review
Merges upstream changes from main while preserving the simplified AuthPolicy headers approach from this PR. Conflict resolution: - Kept Authorization header stripping from PR opendatahub-io#643 - Kept X-MaaS-Subscription header (for Istio Telemetry) - Removed X-MaaS-Username, X-MaaS-Group, X-MaaS-Key-Id headers (as intended by this PR's simplification) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@jrhyness: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jland-redhat, jrhyness The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
chaitanya1731
pushed a commit
that referenced
this pull request
Apr 6, 2026
## Description
Remove identity headers from model inference requests and simplify
subscription-info handling in controller-generated AuthPolicies
(defense-in-depth security improvements).
Changes:
- Remove X-MaaS-Username, X-MaaS-Group, and X-MaaS-Key-Id headers from
requests forwarded to model workloads
- Simplify AuthPolicy by passing complete subscription-info object
instead of extracting individual fields
- Fix CEL syntax error in subscription_info has() check that was
blocking AuthPolicy enforcement
- Identity data remains available to TokenRateLimitPolicy and telemetry
via filters.identity mechanism
**Security Motivation:**
Model workloads (vLLM, Llama.cpp, external APIs) do not require identity
headers. Removing them prevents accidental disclosure in:
- Model runtime logs
- Upstream debug dumps
- Misconfigured proxies or sidecars
- External provider logs (for ExternalModel backends)
**Scope:**
Only affects controller-generated AuthPolicies for model inference
routes. The static maas-api AuthPolicy is unchanged (maas-api's
ExtractUserInfo middleware still requires headers).
**Subscription Info Changes:**
Before (multiple extracted fields):
```yaml
filters:
identity:
- organizationId: auth.metadata["subscription-info"].organizationId
- costCenter: auth.metadata["subscription-info"].costCenter
- subscription_labels: auth.metadata["subscription-info"].labels
After (single object):
filters:
identity:
- subscription_info: auth.metadata["subscription-info"]
Consumers update expressions from auth.identity.organizationId to
auth.identity.subscription_info.organizationId.
How Has This Been Tested?
Unit Tests:
1. TestMaaSAuthPolicyReconciler_IdentityHeadersNotForwarded
- Verifies headers (X-MaaS-Username, X-MaaS-Group, X-MaaS-Key-Id) are
NOT in response.success.headers
- Confirms identity data IS available via filters.identity for
TRLP/telemetry
- Ensures defense-in-depth without breaking rate limiting or metrics
2. TestMaaSAuthPolicyReconciler_SubscriptionInfoObject
- Verifies subscription_info field returns complete object (not
extracted fields)
- Confirms organizationId, costCenter, and labels are accessible via
nested paths
- Tests has() check uses correct CEL syntax (subscription-info.name)
Integration Testing:
- ✅ All existing controller tests pass (no regressions)
- ✅ CEL syntax fix unblocks AuthPolicy enforcement (Enforced=True)
- ✅ TelemetryPolicy updated to use nested subscription_info fields
Manual Testing:
Smoke tests were blocked by CEL syntax error; fixed in this PR. CI
pipeline will validate full e2e flows.
Merge criteria:
- The commits are squashed in a cohesive manner and have meaningful
messages.
- Testing instructions have been added in the PR body (for PRs involving
changes that are not immediately obvious).
- The developer has manually tested the changes and verified that the
changes work
For:
https://redhat.atlassian.net/browse/RHOAIENG-55482
https://redhat.atlassian.net/browse/RHOAIENG-55480
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Identity headers are no longer forwarded to upstream model workloads;
gateway-only consumption enforced.
* Telemetry label sources corrected to consistently use subscription
info fields for metrics.
* **Documentation**
* Clarified identity propagation and defense-in-depth: what is exposed
to gateway telemetry and rate limiting, and what is not forwarded to
upstream workloads.
* **Tests**
* Added reconciliation test verifying no identity headers upstream and
required subscription telemetry.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-merge-bot bot
pushed a commit
that referenced
this pull request
Apr 6, 2026
Automated promotion of **41 commit(s)** from `stable` to `rhoai`. ``` 7df0b05 feat(deployment): support configurable cluster audience for AuthPolicy (#688) 1527654 docs: add testing guide for new contributors (#681) 45e8533 feat: add payload-processing as MaaS sub-component (#662) 53d7b27 chore(docs): document pre-requisites for observability stack (#677) 91b3a51 chore(deps): bump github.com/go-jose/go-jose/v4 from 4.1.3 to 4.1.4 in /maas-controller (#676) a05deb5 chore(deps): bump github.com/go-jose/go-jose/v4 from 4.1.1 to 4.1.4 in /maas-api (#675) a2cd0de docs: fix CRD reference mismatches and README default operator (#667) dc82561 refactor: allow custom db connection in `deploy.sh` (#649) 2a424d5 fix: handle Forbidden error on namespace GET operation during startup (#668) 2878473 test: add additional GO test coverage in `maas-api/internal/api_keys` (#623) 841f96e fix: generate RBAC ClusterRole from kubebuilder markers (#665) 8cd9505 refactor(observability): migrate metric labels from tier to subscription (#508) 2962519 ci: remove multi-arch build platforms from odh tekton pipelineruns (#664) 50ec2a8 docs: add CVE fix guidance and automation workflow (#603) 7ba7216 fix: add when clauses to AuthPolicy fallbacks and remove redundant patch (#663) 7b73da4 test: improve maas-controller test coverage from 43.6% to 66.5% (#651) 2af4155 feat: add a usage dashboard (#624) 85d906f refactor: use Kustomize components for shared overlay configuration (#551) d7f4495 fix: clean up Kuadrant/RHCL OLM resources before namespace deletion (#653) d3b8012 docs: add missing ODH webhook wait command in platform-setup docs (#641) 416c93d feat: improve deploy and test script robustness (#650) 3d08353 fix: avoid maas-controller env value + valueFrom kustomize merge (#657) 1b86d12 ci: add maas-controller CI workflow and remove obsolete image build from maas-api (#656) 409b0a7 chore: automation for docs pre-release (#563) d0dc95b docs: document TLS_SELF_SIGNED and TLS_MIN_VERSION env vars (#640) 1221310 feat: remove identity headers and simplify subscription info (#645) 152e531 chore: upgrade/validate Kuadrant 1.4.2 (#643) d4a15d8 feat: require tokenRateLimits and remove tokenRateLimitRef for 3.4 (#644) 6b2d7e4 ci: extend Konflux docs-only skip to match Prow skip rules (#642) 291d215 docs: fix access probe response codes in model-listing-flow (#559) ``` --------- Signed-off-by: Dmytro Zaharnytskyi <zdmytro@redhat.com> Signed-off-by: Mynhardt Burger <mynhardt@gmail.com> Signed-off-by: Brent Salisbury <bsalisbu@redhat.com> Signed-off-by: Wen Liang <liangwen12year@gmail.com> Signed-off-by: Chaitanya Kulkarni <ckulkarn@redhat.com> Signed-off-by: Chaitanya Kulkarni <chkulkar@redhat.com> Signed-off-by: Arik Hadas <ahadas@redhat.com> Signed-off-by: Tal Gitelman <tgitelma@redhat.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Dmytro Zaharnytskyi <zdmytro@redhat.com> Co-authored-by: Egor Lunin <banky.webmaster@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Mynhardt Burger <mynhardt@gmail.com> Co-authored-by: Brent Salisbury <bsalisbu@redhat.com> Co-authored-by: liangwen12year <36004580+liangwen12year@users.noreply.github.com> Co-authored-by: Jim Rhyness <jrhyness@redhat.com> Co-authored-by: somya-bhatnagar <sbhatnag@redhat.com> Co-authored-by: Jamie Land <38305141+jland-redhat@users.noreply.github.com> Co-authored-by: Chaitanya Kulkarni <chkulkar@redhat.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Arik Hadas <ahadas@redhat.com> Co-authored-by: tgitelman <tgitelma@redhat.com> Co-authored-by: Andrew Ballantyne <8126518+andrewballantyne@users.noreply.github.com> Co-authored-by: angaduom <12499805+angaduom@users.noreply.github.com> Co-authored-by: angaduom <angaduom@users.noreply.github.com> Co-authored-by: Yuriy Teodorovych <71162952+yu-teo@users.noreply.github.com> Co-authored-by: Yuriy Teodorovych <Yuriy@ibm.com> Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.
Description
Remove identity headers from model inference requests and simplify subscription-info handling in controller-generated AuthPolicies (defense-in-depth security improvements).
Changes:
Security Motivation:
Model workloads (vLLM, Llama.cpp, external APIs) do not require identity headers. Removing them prevents accidental disclosure in:
Scope:
Only affects controller-generated AuthPolicies for model inference routes. The static maas-api AuthPolicy is unchanged (maas-api's ExtractUserInfo middleware still requires headers).
Subscription Info Changes:
Before (multiple extracted fields):