feat: remove identity headers and simplify subscription info#645
feat: remove identity headers and simplify subscription info#645jrhyness 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.
|
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}
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 |
2 participants
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):