diff --git a/README.md b/README.md index 5e37b5bb7..c175bc5c4 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,8 @@ For detailed instructions, see the [Deployment Guide](docs/content/quickstart.md |----------|-------------|---------| | `MAAS_API_IMAGE` | Custom MaaS API container image (works in both operator and kustomize modes) | `quay.io/user/maas-api:pr-123` | | `MAAS_CONTROLLER_IMAGE` | Custom MaaS controller container image | `quay.io/user/maas-controller:pr-123` | +| `METADATA_CACHE_TTL` | TTL in seconds for Authorino metadata HTTP caching | `60` (default), `300` | +| `AUTHZ_CACHE_TTL` | TTL in seconds for Authorino OPA authorization caching | `60` (default), `30` | | `OPERATOR_CATALOG` | Custom operator catalog | `quay.io/opendatahub/catalog:pr-456` | | `OPERATOR_IMAGE` | Custom operator image | `quay.io/opendatahub/operator:pr-456` | | `OPERATOR_TYPE` | Operator type (rhoai/odh) | `odh` | @@ -127,6 +129,7 @@ MAAS_API_IMAGE=quay.io/myuser/maas-api:pr-123 \ - [Deployment Guide](docs/content/quickstart.md) - Complete deployment instructions - [MaaS API Documentation](maas-api/README.md) - Go API for key management +- [Authorino Caching Configuration](docs/content/configuration-and-management/authorino-caching.md) - Cache tuning for metadata and authorization Online Documentation: [https://opendatahub-io.github.io/models-as-a-service/](https://opendatahub-io.github.io/models-as-a-service/) diff --git a/deployment/base/maas-api/policies/auth-policy.yaml b/deployment/base/maas-api/policies/auth-policy.yaml index 2e2bb04b5..6853c6918 100644 --- a/deployment/base/maas-api/policies/auth-policy.yaml +++ b/deployment/base/maas-api/policies/auth-policy.yaml @@ -72,7 +72,7 @@ spec: when: - predicate: request.headers.authorization.startsWith("Bearer sk-oai-") plain: - expression: '''["'' + auth.metadata.apiKeyValidation.groups.join(''","'') + ''"]''' + selector: auth.metadata.apiKeyValidation.groups.@tostr priority: 0 # Groups: from OpenShift identity as JSON array (when OC token used) X-MaaS-Group-OC: diff --git a/deployment/base/maas-controller/manager/manager.yaml b/deployment/base/maas-controller/manager/manager.yaml index e850de80a..cb576396b 100644 --- a/deployment/base/maas-controller/manager/manager.yaml +++ b/deployment/base/maas-controller/manager/manager.yaml @@ -33,6 +33,8 @@ spec: - --maas-api-namespace=$(MAAS_API_NAMESPACE) - --maas-subscription-namespace=$(MAAS_SUBSCRIPTION_NAMESPACE) - --cluster-audience=$(CLUSTER_AUDIENCE) + - --metadata-cache-ttl=$(METADATA_CACHE_TTL) + - --authz-cache-ttl=$(AUTHZ_CACHE_TTL) env: - name: GATEWAY_NAME value: "maas-default-gateway" @@ -46,6 +48,10 @@ spec: value: "models-as-a-service" - name: CLUSTER_AUDIENCE value: "https://kubernetes.default.svc" + - name: METADATA_CACHE_TTL + value: "60" + - name: AUTHZ_CACHE_TTL + value: "60" image: maas-controller name: manager imagePullPolicy: Always diff --git a/deployment/overlays/odh/kustomization.yaml b/deployment/overlays/odh/kustomization.yaml index 50cc3ca34..c464a03fb 100644 --- a/deployment/overlays/odh/kustomization.yaml +++ b/deployment/overlays/odh/kustomization.yaml @@ -51,6 +51,32 @@ patches: target: kind: Deployment name: maas-api +- patch: |- + apiVersion: apps/v1 + kind: Deployment + metadata: + name: maas-controller + spec: + template: + spec: + containers: + - name: manager + env: + - name: METADATA_CACHE_TTL + valueFrom: + configMapKeyRef: + key: metadata-cache-ttl + name: maas-parameters + optional: true + - name: AUTHZ_CACHE_TTL + valueFrom: + configMapKeyRef: + key: authz-cache-ttl + name: maas-parameters + optional: true + target: + kind: Deployment + name: maas-controller replacements: # Gateway policies must be in openshift-ingress to target maas-default-gateway diff --git a/deployment/overlays/odh/params.env b/deployment/overlays/odh/params.env index 7cb2e5d16..6d662eb0f 100644 --- a/deployment/overlays/odh/params.env +++ b/deployment/overlays/odh/params.env @@ -4,3 +4,5 @@ gateway-namespace=openshift-ingress gateway-name=maas-default-gateway app-namespace=opendatahub api-key-max-expiration-days=30 +metadata-cache-ttl=60 +authz-cache-ttl=60 diff --git a/docs/content/configuration-and-management/authorino-caching.md b/docs/content/configuration-and-management/authorino-caching.md new file mode 100644 index 000000000..f109db996 --- /dev/null +++ b/docs/content/configuration-and-management/authorino-caching.md @@ -0,0 +1,226 @@ +# Authorino Caching Configuration + +This document describes the Authorino/Kuadrant caching configuration in MaaS, including how to tune cache TTLs for metadata and authorization evaluators. + +--- + +## Overview + +MaaS-generated AuthPolicy resources enable Authorino-style caching on: + +- **Metadata evaluators** (HTTP calls to maas-api): + - `apiKeyValidation` - validates API keys and returns user identity + groups + - `subscription-info` - selects the appropriate subscription for the request + +- **Authorization evaluators** (OPA policy evaluation): + - `auth-valid` - validates authentication (API key OR K8s token) + - `subscription-valid` - ensures a valid subscription was selected + - `require-group-membership` - checks user/group membership against allowed lists + +Caching reduces load on maas-api and CPU spent on Rego re-evaluation by reusing results when the cache key repeats within the TTL window. + +--- + +## Configuration + +### Environment Variables + +The maas-controller deployment supports the following environment variables to configure cache TTLs: + +| Variable | Description | Default | Unit | Constraints | +|----------|-------------|---------|------|-------------| +| `METADATA_CACHE_TTL` | TTL for metadata HTTP caching (apiKeyValidation, subscription-info) | `60` | seconds | Must be ≥ 0 | +| `AUTHZ_CACHE_TTL` | TTL for OPA authorization caching (auth-valid, subscription-valid, require-group-membership) | `60` | seconds | Must be ≥ 0 | + +**Note:** The controller will fail to start if either TTL is set to a negative value. + +### Deployment Configuration + +#### Via params.env (ODH Overlay) + +Edit `deployment/overlays/odh/params.env`: + +```env +metadata-cache-ttl=300 # 5 minutes +authz-cache-ttl=30 # 30 seconds +``` + +These values are injected into the maas-controller deployment via ConfigMap. + +#### Via manager.yaml (Base Deployment) + +Edit `deployment/base/maas-controller/manager/manager.yaml`: + +```yaml +env: + - name: METADATA_CACHE_TTL + value: "300" # 5 minutes + - name: AUTHZ_CACHE_TTL + value: "30" # 30 seconds +``` + +### Important: Authorization Cache TTL Capping + +**Authorization caches are automatically capped at the metadata cache TTL** to prevent stale authorization decisions. + +Authorization evaluators (auth-valid, subscription-valid, require-group-membership) depend on metadata evaluators (apiKeyValidation, subscription-info). If authorization caches outlive metadata caches, stale metadata can lead to incorrect authorization decisions. + +**Example:** +```yaml +METADATA_CACHE_TTL=60 # 1 minute +AUTHZ_CACHE_TTL=300 # 5 minutes (will be capped at 60 seconds) +``` + +In this scenario: +- Metadata caches use 60-second TTL ✅ +- Authorization caches use **60-second TTL** (capped, not 300) ✅ +- A warning is logged at startup: "Authorization cache TTL exceeds metadata cache TTL" + +**Recommendation:** Set `AUTHZ_CACHE_TTL ≤ METADATA_CACHE_TTL` to avoid confusion. + +--- + +## Cache Key Design + +Cache keys are carefully designed to prevent data leakage between principals, subscriptions, and models. + +### Collision Resistance + +Cache keys use single-character delimiters (`|` and `,`) to separate components: + +- **Field delimiter**: `|` separates major components (user ID, groups, subscription, model) +- **Group delimiter**: `,` joins multiple group names + +**For API Keys - Collision Resistant:** +Cache keys use database-assigned UUIDs instead of usernames: +- User ID: Database primary key (UUID format in `api_keys.id` column) +- Immutable and unique per API key +- Not user-controllable (assigned by database on creation) +- Example key: `550e8400-e29b-41d4-a716-446655440000|team,admin|sub1|ns/model` +- No collision risk even if groups contain delimiters (UUID prefix ensures uniqueness) + +**For Kubernetes Tokens - Already Safe:** +Kubernetes usernames follow validated format enforced by the K8s API: +- Pattern: `system:serviceaccount:namespace:sa-name` +- Kubernetes validates namespace/SA names (DNS-1123: alphanumeric + hyphens only) +- No special characters like `|` or `,` allowed in usernames +- Creating service accounts requires cluster permissions (not user self-service) + +**Implementation:** +The `apiKeyValidation` metadata evaluator returns a `userId` field: +- API keys: Set to `api_keys.id` (database UUID) +- Cache keys reference `auth.metadata.apiKeyValidation.userId` in CEL expressions +- This eliminates username-based collision attacks + +### Metadata Caches + +**apiKeyValidation:** +- **Only runs for API key requests** (Authorization header matches `Bearer sk-oai-*`) +- Key: `` +- Ensures each unique API key has its own cache entry +- Does not run for Kubernetes token requests (prevents cache pollution) +- Returns `userId` field set to database UUID (`api_keys.id`) + +**subscription-info:** +- Key: `|||/` +- For API keys: `userId` is database UUID from `apiKeyValidation` response +- For K8s tokens: `userId` is validated K8s username (`system:serviceaccount:...`) +- Groups joined with `,` delimiter +- Ensures cache isolation per user, group membership, requested subscription, and model + +### Authorization Caches + +**auth-valid:** +- Key: `||/` +- For API keys: `api-key||model` +- For K8s tokens: `k8s-token||model` + +**subscription-valid:** +- Key: Same as subscription-info metadata (ensures cache coherence) +- Format: `|||` +- For API keys: `userId` is database UUID. For K8s tokens: validated username. + +**require-group-membership:** +- Key: `||/` +- For API keys: `userId` is database UUID. For K8s tokens: validated username. +- Groups joined with `,` delimiter +- Ensures cache isolation per user identity and model + +--- + +## Operational Tuning + +### When to Increase Metadata Cache TTL + +- **High API key validation load**: If maas-api is experiencing high load from repeated `/internal/v1/api-keys/validate` calls +- **Stable API keys**: API key metadata (username, groups) doesn't change frequently +- **Example**: Set `METADATA_CACHE_TTL=300` (5 minutes) to reduce maas-api load by 5x + +### When to Decrease Authorization Cache TTL + +- **Group membership changes**: If users are frequently added/removed from groups +- **Security compliance**: Shorter TTL ensures access changes propagate faster +- **Example**: Set `AUTHZ_CACHE_TTL=30` (30 seconds) for faster group membership updates + +### Monitoring + +After changing TTL values, monitor: +- **maas-api load**: Reduced `/internal/v1/api-keys/validate` and `/internal/v1/subscriptions/select` call rates +- **Authorino CPU**: Reduced OPA evaluation CPU usage +- **Request latency**: Cache hits should have lower P99 latency + +--- + +## Security Notes + +### Cache Key Correctness + +All cache keys include sufficient dimensions to prevent cross-principal or cross-subscription cache sharing: + +- **Never share cache entries between different users** +- **Never share cache entries between different API keys** +- **Never share cache entries between different models** (model namespace/name in key) +- **Never share cache entries between different group memberships** (groups in key) + +### Cache Key Collision Risk + +**API Keys - No Collision Risk:** +Cache keys use database-assigned UUIDs instead of usernames: +- User IDs are unique 128-bit UUIDs (format: `550e8400-e29b-41d4-a716-446655440000`) +- Immutable and assigned by PostgreSQL at API key creation +- Not user-controllable (no self-service user ID selection) +- Even if groups contain delimiters (`,` or `|`), the UUID prefix prevents collision +- Example: Two users with groups `["team,admin"]` and `["team", "admin"]` have different UUIDs, so no collision + +**Kubernetes Tokens - No Collision Risk:** +Kubernetes usernames are validated by the K8s API server: +- Format: `system:serviceaccount:namespace:sa-name` +- Kubernetes enforces DNS-1123 naming: `[a-z0-9]([-a-z0-9]*[a-z0-9])?` +- No special characters like `|` or `,` allowed +- Creating service accounts requires cluster RBAC permissions (not user self-service) + +**Remaining Edge Case - Group Ordering:** +Group array ordering affects cache keys: +- `["admin", "user"]` produces different key than `["user", "admin"]` +- CEL has no array sort() function +- Impact: Suboptimal cache hit rate if group order varies between OIDC token refreshes +- Mitigation: OIDC providers and K8s TokenReview typically return groups in consistent order + +### Stale Data Window + +Cache TTL represents the maximum staleness window: + +- **Metadata caches**: API key revocation or group membership changes may take up to `METADATA_CACHE_TTL` seconds to propagate +- **Authorization caches**: Authorization policy changes may take up to `AUTHZ_CACHE_TTL` seconds to propagate + +For immediate policy enforcement after changes: +1. Delete the affected AuthPolicy to clear Authorino's cache +2. Or wait for the TTL to expire + +--- + +## References + +- [Authorino Caching User Guide](https://docs.kuadrant.io/latest/authorino/docs/features/#caching) +- [AuthPolicy Reference](https://docs.kuadrant.io/latest/kuadrant-operator/doc/reference/authpolicy/) +- [MaaS Controller Overview](./maas-controller-overview.md) diff --git a/maas-api/internal/api_keys/service.go b/maas-api/internal/api_keys/service.go index 9888aba25..cd98df434 100644 --- a/maas-api/internal/api_keys/service.go +++ b/maas-api/internal/api_keys/service.go @@ -251,10 +251,18 @@ func (s *Service) ValidateAPIKey(ctx context.Context, key string) (*ValidationRe }, nil } + // Validate metadata.ID is a syntactically valid UUID (fail-closed defense-in-depth) + // Database should always return valid UUIDs, but verify to prevent malformed IDs + // from being used in cache keys or authorization decisions + if _, err := uuid.Parse(metadata.ID); err != nil { + s.logger.Error("API key has invalid UUID format", "key_id", metadata.ID, "error", err) + return nil, fmt.Errorf("database integrity error: invalid key ID format: %w", err) + } + // Success - return user identity and groups for Authorino return &ValidationResult{ Valid: true, - UserID: metadata.Username, + UserID: metadata.ID, // Database-assigned UUID (immutable, collision-resistant) Username: metadata.Username, KeyID: metadata.ID, Groups: groups, // Original user groups for subscription-based authorization diff --git a/maas-api/internal/api_keys/service_test.go b/maas-api/internal/api_keys/service_test.go index ab09a8d24..52dcd2ce2 100644 --- a/maas-api/internal/api_keys/service_test.go +++ b/maas-api/internal/api_keys/service_test.go @@ -43,8 +43,8 @@ func TestValidateAPIKey_ValidKey(t *testing.T) { ctx := context.Background() svc, store := createTestService(t) - // Create a valid API key - keyID := "test-key-id" + // Create a valid API key with UUID (matches production database behavior) + keyID := "550e8400-e29b-41d4-a716-446655440000" plainKey, hash := createTestAPIKey(t) username := "alice" groups := []string{"tier-premium", "system:authenticated"} @@ -58,7 +58,7 @@ func TestValidateAPIKey_ValidKey(t *testing.T) { require.NotNil(t, result) assert.True(t, result.Valid) - assert.Equal(t, username, result.UserID) + assert.Equal(t, keyID, result.UserID) // UserID is the database-assigned key ID (UUID) assert.Equal(t, username, result.Username) assert.Equal(t, keyID, result.KeyID) assert.Equal(t, groups, result.Groups) @@ -108,7 +108,7 @@ func TestValidateAPIKey_RevokedKey(t *testing.T) { svc, store := createTestService(t) // Create and immediately revoke a key - keyID := "revoked-key-id" + keyID := "550e8400-e29b-41d4-a716-446655440001" plainKey, hash := createTestAPIKey(t) username := "bob" groups := []string{"tier-free"} @@ -134,7 +134,7 @@ func TestValidateAPIKey_ExpiredKey(t *testing.T) { svc, store := createTestService(t) // Create a key that's already expired - keyID := "expired-key-id" + keyID := "550e8400-e29b-41d4-a716-446655440002" plainKey, hash := createTestAPIKey(t) username := "charlie" groups := []string{"tier-basic"} @@ -157,7 +157,7 @@ func TestValidateAPIKey_EmptyGroups(t *testing.T) { svc, store := createTestService(t) // Create a key with no groups (nil) - keyID := "no-groups-key" + keyID := "550e8400-e29b-41d4-a716-446655440003" plainKey, hash := createTestAPIKey(t) username := "dave" @@ -170,7 +170,7 @@ func TestValidateAPIKey_EmptyGroups(t *testing.T) { require.NotNil(t, result) assert.True(t, result.Valid) - assert.Equal(t, username, result.UserID) + assert.Equal(t, keyID, result.UserID) // UserID is the database-assigned key ID (UUID) assert.NotNil(t, result.Groups, "Groups should be empty array, not nil") assert.Empty(t, result.Groups, "Groups should be empty") } @@ -180,7 +180,7 @@ func TestValidateAPIKey_UpdatesLastUsed(t *testing.T) { svc, store := createTestService(t) // Create a valid API key - keyID := "last-used-key" + keyID := "550e8400-e29b-41d4-a716-446655440004" plainKey, hash := createTestAPIKey(t) username := "eve" groups := []string{"tier-enterprise"} @@ -216,7 +216,7 @@ func TestGetAPIKey(t *testing.T) { svc, store := createTestService(t) // Create a key - keyID := "get-test-key" + keyID := "550e8400-e29b-41d4-a716-446655440005" _, hash := createTestAPIKey(t) username := "alice" keyName := "Alice's Key" @@ -249,7 +249,7 @@ func TestRevokeAPIKey(t *testing.T) { svc, store := createTestService(t) // Create a key - keyID := "revoke-test-key" + keyID := "550e8400-e29b-41d4-a716-446655440006" _, hash := createTestAPIKey(t) username := "bob" diff --git a/maas-controller/cmd/manager/main.go b/maas-controller/cmd/manager/main.go index 3ebf8022c..b982b5256 100644 --- a/maas-controller/cmd/manager/main.go +++ b/maas-controller/cmd/manager/main.go @@ -149,6 +149,8 @@ func main() { var maasAPINamespace string var maasSubscriptionNamespace string var clusterAudience string + var metadataCacheTTL int64 + var authzCacheTTL int64 flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metrics endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") @@ -159,6 +161,8 @@ func main() { flag.StringVar(&maasAPINamespace, "maas-api-namespace", "opendatahub", "The namespace where maas-api service is deployed.") flag.StringVar(&maasSubscriptionNamespace, "maas-subscription-namespace", "models-as-a-service", "The namespace to watch for MaaS CRs.") flag.StringVar(&clusterAudience, "cluster-audience", "https://kubernetes.default.svc", "The OIDC audience of the cluster for TokenReview. HyperShift/ROSA clusters use a custom OIDC provider URL.") + flag.Int64Var(&metadataCacheTTL, "metadata-cache-ttl", 60, "TTL in seconds for Authorino metadata HTTP caching (apiKeyValidation, subscription-info).") + flag.Int64Var(&authzCacheTTL, "authz-cache-ttl", 60, "TTL in seconds for Authorino OPA authorization caching (auth-valid, subscription-valid, require-group-membership).") opts := zap.Options{Development: false} opts.BindFlags(flag.CommandLine) @@ -214,11 +218,13 @@ func main() { os.Exit(1) } if err := (&maas.MaaSAuthPolicyReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - MaaSAPINamespace: maasAPINamespace, - GatewayName: gatewayName, - ClusterAudience: clusterAudience, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + MaaSAPINamespace: maasAPINamespace, + GatewayName: gatewayName, + ClusterAudience: clusterAudience, + MetadataCacheTTL: metadataCacheTTL, + AuthzCacheTTL: authzCacheTTL, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MaaSAuthPolicy") os.Exit(1) diff --git a/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go b/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go index d3dc4163e..53bd4dc6c 100644 --- a/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go +++ b/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go @@ -26,6 +26,7 @@ import ( "strings" "github.com/go-logr/logr" + maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" @@ -42,8 +43,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - - maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1" ) // MaaSAuthPolicyReconciler reconciles a MaaSAuthPolicy object @@ -60,6 +59,14 @@ type MaaSAuthPolicyReconciler struct { // ClusterAudience is the OIDC audience of the cluster (configurable via flags). // Standard clusters use "https://kubernetes.default.svc"; HyperShift/ROSA use a custom OIDC provider URL. ClusterAudience string + + // MetadataCacheTTL is the TTL in seconds for Authorino metadata HTTP caching. + // Applies to apiKeyValidation and subscription-info metadata evaluators. + MetadataCacheTTL int64 + + // AuthzCacheTTL is the TTL in seconds for Authorino OPA authorization caching. + // Applies to auth-valid, subscription-valid, and require-group-membership authorization evaluators. + AuthzCacheTTL int64 } func (r *MaaSAuthPolicyReconciler) clusterAudience() string { @@ -69,6 +76,28 @@ func (r *MaaSAuthPolicyReconciler) clusterAudience() string { return defaultClusterAudience } +// authzCacheTTL returns the safe TTL for authorization caches that depend on metadata. +// Authorization cache entries must not outlive their dependent metadata cache entries, +// otherwise stale metadata can lead to incorrect authorization decisions. +// Returns the minimum of AuthzCacheTTL and MetadataCacheTTL, clamped to non-negative values. +func (r *MaaSAuthPolicyReconciler) authzCacheTTL() int64 { + metadata := r.MetadataCacheTTL + authz := r.AuthzCacheTTL + + // Defensive: clamp negative values to 0 (should be caught at startup, but defensive) + if metadata < 0 { + metadata = 0 + } + if authz < 0 { + authz = 0 + } + + if authz < metadata { + return authz + } + return metadata +} + //+kubebuilder:rbac:groups=maas.opendatahub.io,resources=maasauthpolicies,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups=maas.opendatahub.io,resources=maasauthpolicies/status,verbs=get;update;patch //+kubebuilder:rbac:groups=maas.opendatahub.io,resources=maasauthpolicies/finalizers,verbs=update @@ -194,18 +223,35 @@ func (r *MaaSAuthPolicyReconciler) reconcileModelAuthPolicies(ctx context.Contex apiKeyValidationURL := fmt.Sprintf("https://maas-api.%s.svc.cluster.local:8443/internal/v1/api-keys/validate", r.MaaSAPINamespace) subscriptionSelectorURL := fmt.Sprintf("https://maas-api.%s.svc.cluster.local:8443/internal/v1/subscriptions/select", r.MaaSAPINamespace) - rule := map[string]any{ - "metadata": map[string]any{ + rule := map[string]interface{}{ + "metadata": map[string]interface{}{ // API Key Validation - validates the API key and returns user identity + groups - "apiKeyValidation": map[string]any{ - "http": map[string]any{ + // Only runs for API key requests (sk-oai-* prefix), not K8s tokens + "apiKeyValidation": map[string]interface{}{ + "when": []interface{}{ + map[string]interface{}{ + "selector": "request.headers.authorization", + "operator": "matches", + "value": "^Bearer sk-oai-.*", + }, + }, + "http": map[string]interface{}{ "url": apiKeyValidationURL, "contentType": "application/json", "method": "POST", - "body": map[string]any{ + "body": map[string]interface{}{ "expression": `{"key": request.headers.authorization.replace("Bearer ", "")}`, }, }, + // Cache API key validation results keyed by the API key itself. + // Key format: "api-key-value" + // This prevents repeated validation calls for the same API key within the TTL window. + "cache": map[string]interface{}{ + "key": map[string]interface{}{ + "selector": `request.headers.authorization.replace("Bearer ", "")`, + }, + "ttl": r.MetadataCacheTTL, + }, "metrics": false, "priority": int64(0), }, @@ -213,44 +259,45 @@ func (r *MaaSAuthPolicyReconciler) reconcileModelAuthPolicies(ctx context.Contex // For API keys: uses subscription bound to the key at mint time // For K8s tokens: uses X-MaaS-Subscription header if provided, otherwise finds all accessible // Priority 1 ensures this runs after apiKeyValidation (priority 0). - "subscription-info": map[string]any{ - "http": map[string]any{ + "subscription-info": map[string]interface{}{ + "http": map[string]interface{}{ "url": subscriptionSelectorURL, "contentType": "application/json", "method": "POST", - "body": map[string]any{ + "body": map[string]interface{}{ "expression": fmt.Sprintf(`{ - "groups": auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups, - "username": auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.username : auth.identity.user.username, - "requestedSubscription": auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.subscription : ("x-maas-subscription" in request.headers ? request.headers["x-maas-subscription"] : ""), + "groups": (has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups, + "username": (has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.username : auth.identity.user.username, + "requestedSubscription": (has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.subscription : ("x-maas-subscription" in request.headers ? request.headers["x-maas-subscription"] : ""), "requestedModel": "%s/%s" }`, ref.Namespace, ref.Name), }, }, - // Cache subscription selection results keyed by username, groups, requested subscription, and model. + // Cache subscription selection results keyed by user ID, groups, requested subscription, and model. // Each model has its own cache entry since subscription validation is model-specific. - // Key format: "username|groups-hash|requested-subscription|model-namespace/model-name" + // Key format: "userId|groups|requested-subscription|model-namespace/model-name" + // For API keys: userId is database-assigned UUID (collision-resistant) + // For K8s tokens: userId is validated username (system:serviceaccount:namespace:sa-name) // Groups are joined with commas to create a stable string representation. - "cache": map[string]any{ - "key": map[string]any{ - //nolint:lll // CEL expression must be on single line - "selector": fmt.Sprintf(`(auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.username : auth.identity.user.username) + "|" + (auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups).join(",") + "|" + (auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.subscription : ("x-maas-subscription" in request.headers ? request.headers["x-maas-subscription"] : "")) + "|%s/%s"`, ref.Namespace, ref.Name), + "cache": map[string]interface{}{ + "key": map[string]interface{}{ + "selector": fmt.Sprintf(`((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.userId : auth.identity.user.username) + "|" + ((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups).join(",") + "|" + ((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.subscription : ("x-maas-subscription" in request.headers ? request.headers["x-maas-subscription"] : "")) + "|%s/%s"`, ref.Namespace, ref.Name), }, - "ttl": int64(60), + "ttl": r.MetadataCacheTTL, }, "metrics": false, "priority": int64(1), }, }, - "authentication": map[string]any{ + "authentication": map[string]interface{}{ // API Keys - plain authentication, actual validation in metadata layer // Only processes tokens with sk-oai- prefix (OpenAI-compatible API keys) - "api-keys": map[string]any{ - "plain": map[string]any{ + "api-keys": map[string]interface{}{ + "plain": map[string]interface{}{ "selector": "request.headers.authorization", }, - "when": []any{ - map[string]any{ + "when": []interface{}{ + map[string]interface{}{ "selector": "request.headers.authorization", "operator": "matches", "value": "^Bearer sk-oai-.*", @@ -264,17 +311,17 @@ func (r *MaaSAuthPolicyReconciler) reconcileModelAuthPolicies(ctx context.Contex // Inferencing endpoints require API keys for billing/tracking // The api-keys authentication (priority 0) runs first and will consume API key requests, // so we don't need to explicitly exclude them here - "kubernetes-tokens": map[string]any{ - "kubernetesTokenReview": map[string]any{ - "audiences": []any{r.clusterAudience()}, + "kubernetes-tokens": map[string]interface{}{ + "kubernetesTokenReview": map[string]interface{}{ + "audiences": []interface{}{r.clusterAudience()}, }, - "when": []any{ - map[string]any{ + "when": []interface{}{ + map[string]interface{}{ "selector": "request.url_path", "operator": "matches", "value": ".*/v1/models$", }, - map[string]any{ + map[string]interface{}{ "selector": "request.headers.authorization", "operator": "neq", "value": "", @@ -287,15 +334,15 @@ func (r *MaaSAuthPolicyReconciler) reconcileModelAuthPolicies(ctx context.Contex } // Build authorization rules - authRules := make(map[string]any) + authRules := make(map[string]interface{}) // Validate authentication: API key must be valid, OR K8s token must be authenticated // For API keys: check apiKeyValidation.valid == true (boolean) // For K8s tokens: check that identity.username exists (TokenReview succeeded) - authRules["auth-valid"] = map[string]any{ + authRules["auth-valid"] = map[string]interface{}{ "metrics": false, "priority": int64(0), - "opa": map[string]any{ + "opa": map[string]interface{}{ "rego": `# API key authentication: validate the key allow { object.get(input.auth.metadata, "apiKeyValidation", {}) @@ -307,32 +354,48 @@ allow { object.get(input.auth.identity, "user", {}).username != "" }`, }, + // Cache authorization result keyed by authentication source and identity. + // For API keys: uses the API key value + // For K8s tokens: uses the username + // Key format: "auth-type|identity|model" + // TTL cannot exceed metadata TTL (auth-valid depends on apiKeyValidation metadata) + "cache": map[string]interface{}{ + "key": map[string]interface{}{ + "selector": fmt.Sprintf(`(has(auth.metadata.apiKeyValidation) ? "api-key|" + request.headers.authorization.replace("Bearer ", "") : "k8s-token|" + auth.identity.user.username) + "|%s/%s"`, ref.Namespace, ref.Name), + }, + "ttl": r.authzCacheTTL(), + }, } // Fail-close: require successful subscription selection (name must be present) - authRules["subscription-valid"] = map[string]any{ + authRules["subscription-valid"] = map[string]interface{}{ "metrics": false, "priority": int64(0), - "opa": map[string]any{ + "opa": map[string]interface{}{ "rego": `allow { object.get(input.auth.metadata["subscription-info"], "name", "") != "" }`, }, + // Cache authorization result keyed by subscription selection inputs. + // Uses same key dimensions as subscription-info metadata to ensure cache coherence. + // Key format: "userId|groups|requested-subscription|model" + // For API keys: userId is database UUID. For K8s tokens: validated username. + // TTL cannot exceed metadata TTL (subscription-valid depends on subscription-info metadata) + "cache": map[string]interface{}{ + "key": map[string]interface{}{ + "selector": fmt.Sprintf(`((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.userId : auth.identity.user.username) + "|" + ((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups).join(",") + "|" + ((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.subscription : ("x-maas-subscription" in request.headers ? request.headers["x-maas-subscription"] : "")) + "|%s/%s"`, ref.Namespace, ref.Name), + }, + "ttl": r.authzCacheTTL(), + }, } // Build aggregated authorization rule from ALL auth policies' subjects // Uses OPA to check membership for both API keys and K8s tokens if len(allowedGroups) > 0 || len(allowedUsers) > 0 { - groupsJSON, err := json.Marshal(allowedGroups) - if err != nil { - return nil, fmt.Errorf("failed to marshal allowed groups: %w", err) - } - usersJSON, err := json.Marshal(allowedUsers) - if err != nil { - return nil, fmt.Errorf("failed to marshal allowed users: %w", err) - } - authRules["require-group-membership"] = map[string]any{ + groupsJSON, _ := json.Marshal(allowedGroups) + usersJSON, _ := json.Marshal(allowedUsers) + authRules["require-group-membership"] = map[string]interface{}{ "metrics": false, "priority": int64(0), - "opa": map[string]any{ + "opa": map[string]interface{}{ "rego": fmt.Sprintf(` # Allowed groups and users from all MaaSAuthPolicies allowed_groups := %s @@ -363,6 +426,17 @@ allow { } `, string(groupsJSON), string(usersJSON)), }, + // Cache authorization result keyed by user ID, groups, and model. + // The allowed groups/users are baked into the OPA rego, so the cache is per-model-policy. + // Key format: "userId|groups|model" + // For API keys: userId is database UUID. For K8s tokens: validated username. + // TTL cannot exceed metadata TTL (require-group-membership depends on apiKeyValidation metadata for groups) + "cache": map[string]interface{}{ + "key": map[string]interface{}{ + "selector": fmt.Sprintf(`((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.userId : auth.identity.user.username) + "|" + ((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups).join(",") + "|%s/%s"`, ref.Namespace, ref.Name), + }, + "ttl": r.authzCacheTTL(), + }, } } @@ -375,81 +449,81 @@ allow { // Also inject subscription metadata from subscription-info for Limitador metrics. // For API keys: username/groups come from apiKeyValidation metadata // For K8s tokens: username/groups come from auth.identity - rule["response"] = map[string]any{ - "success": map[string]any{ - "headers": map[string]any{ + rule["response"] = map[string]interface{}{ + "success": map[string]interface{}{ + "headers": map[string]interface{}{ // Username from API key validation or K8s token identity - "X-MaaS-Username": map[string]any{ - "plain": map[string]any{ - "expression": `auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.username : auth.identity.user.username`, + "X-MaaS-Username": map[string]interface{}{ + "plain": map[string]interface{}{ + "expression": `(has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.username : auth.identity.user.username`, }, "metrics": false, "priority": int64(0), }, - // Groups - construct JSON array string from API key validation or K8s identity - "X-MaaS-Group": map[string]any{ - "plain": map[string]any{ - "expression": `'["' + (auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups).join('","') + '"]'`, + // Groups - serialize to JSON array string from API key validation or K8s identity + // Using string() conversion of JSON-serialized groups for proper escaping + "X-MaaS-Group": map[string]interface{}{ + "plain": map[string]interface{}{ + "expression": `string(((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups))`, }, "metrics": false, "priority": int64(0), }, // Key ID for tracking (only for API keys) - "X-MaaS-Key-Id": map[string]any{ - "plain": map[string]any{ - "expression": `auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.keyId : ""`, + "X-MaaS-Key-Id": map[string]interface{}{ + "plain": map[string]interface{}{ + "expression": `(has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.keyId : ""`, }, "metrics": false, "priority": int64(0), }, // Subscription bound to API key (only for API keys) // For K8s tokens, this header is not injected (empty string) - "X-MaaS-Subscription": map[string]any{ - "plain": map[string]any{ - "expression": `auth.metadata.apiKeyValidation.valid == true ? auth.metadata.apiKeyValidation.subscription : ""`, + "X-MaaS-Subscription": map[string]interface{}{ + "plain": map[string]interface{}{ + "expression": `(has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.subscription : ""`, }, "metrics": false, "priority": int64(0), }, }, - "filters": map[string]any{ - "identity": map[string]any{ - "json": map[string]any{ - "properties": map[string]any{ - "groups": map[string]any{"expression": "auth.metadata.apiKeyValidation.groups"}, - "groups_str": map[string]any{"expression": `auth.metadata.apiKeyValidation.groups.join(",")`}, - "userid": map[string]any{ + "filters": map[string]interface{}{ + "identity": map[string]interface{}{ + "json": map[string]interface{}{ + "properties": map[string]interface{}{ + "groups": map[string]interface{}{"expression": "auth.metadata.apiKeyValidation.groups"}, + "groups_str": map[string]interface{}{"expression": `auth.metadata.apiKeyValidation.groups.join(",")`}, + "userid": map[string]interface{}{ "selector": "auth.metadata.apiKeyValidation.username", }, - "keyId": map[string]any{ + "keyId": map[string]interface{}{ "selector": "auth.metadata.apiKeyValidation.keyId", }, // Subscription metadata from /internal/v1/subscriptions/select endpoint - "selected_subscription": map[string]any{ + "selected_subscription": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].name) ? auth.metadata["subscription-info"].name : ""`, }, // Model-scoped subscription key for TRLP isolation: namespace/name@modelNamespace/modelName - "selected_subscription_key": map[string]any{ - //nolint:lll // CEL expression must be on single line + "selected_subscription_key": map[string]interface{}{ "expression": fmt.Sprintf( `has(auth.metadata["subscription-info"].namespace) && has(auth.metadata["subscription-info"].name) ? auth.metadata["subscription-info"].namespace + "/" + auth.metadata["subscription-info"].name + "@%s/%s" : ""`, ref.Namespace, ref.Name, ), }, - "organizationId": map[string]any{ + "organizationId": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].organizationId) ? auth.metadata["subscription-info"].organizationId : ""`, }, - "costCenter": map[string]any{ + "costCenter": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].costCenter) ? auth.metadata["subscription-info"].costCenter : ""`, }, - "subscription_labels": map[string]any{ + "subscription_labels": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].labels) ? auth.metadata["subscription-info"].labels : {}`, }, // Error information (for debugging - only populated when selection fails) - "subscription_error": map[string]any{ + "subscription_error": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].error) ? auth.metadata["subscription-info"].error : ""`, }, - "subscription_error_message": map[string]any{ + "subscription_error_message": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].message) ? auth.metadata["subscription-info"].message : ""`, }, }, @@ -459,22 +533,22 @@ allow { }, }, // Custom denial responses that include subscription error details - "unauthenticated": map[string]any{ + "unauthenticated": map[string]interface{}{ "code": int64(401), - "message": map[string]any{ + "message": map[string]interface{}{ "value": "Authentication required", }, }, - "unauthorized": map[string]any{ + "unauthorized": map[string]interface{}{ "code": int64(403), - "body": map[string]any{ + "body": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].message) ? auth.metadata["subscription-info"].message : "Access denied"`, }, - "headers": map[string]any{ - "x-ext-auth-reason": map[string]any{ + "headers": map[string]interface{}{ + "x-ext-auth-reason": map[string]interface{}{ "expression": `has(auth.metadata["subscription-info"].error) ? auth.metadata["subscription-info"].error : "unauthorized"`, }, - "content-type": map[string]any{ + "content-type": map[string]interface{}{ "value": "text/plain", }, }, @@ -499,8 +573,8 @@ allow { refs = append(refs, authPolicyRef{Name: authPolicyName, Namespace: httpRouteNS, Model: ref.Name, ModelNamespace: ref.Namespace}) - spec := map[string]any{ - "targetRef": map[string]any{ + spec := map[string]interface{}{ + "targetRef": map[string]interface{}{ "group": "gateway.networking.k8s.io", "kind": "HTTPRoute", "name": httpRouteName, @@ -694,7 +768,7 @@ func getAuthPolicyConditionState(ap *unstructured.Unstructured) (accepted, enfor return accepted, enforced } for _, c := range conditions { - cond, ok := c.(map[string]any) + cond, ok := c.(map[string]interface{}) if !ok { continue } @@ -738,7 +812,35 @@ func (r *MaaSAuthPolicyReconciler) updateStatus(ctx context.Context, policy *maa } } +// ValidateCacheTTLs validates that cache TTL configuration is valid. +// Returns an error if either TTL is negative (fail-closed validation). +func (r *MaaSAuthPolicyReconciler) ValidateCacheTTLs() error { + if r.MetadataCacheTTL < 0 { + return fmt.Errorf("metadata cache TTL must be non-negative, got %d", r.MetadataCacheTTL) + } + if r.AuthzCacheTTL < 0 { + return fmt.Errorf("authorization cache TTL must be non-negative, got %d", r.AuthzCacheTTL) + } + return nil +} + func (r *MaaSAuthPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { + // Validate cache TTL configuration + log := ctrl.Log.WithName("maas-authpolicy-controller") + + // Reject negative TTL values + if err := r.ValidateCacheTTLs(); err != nil { + return err + } + + if r.AuthzCacheTTL > r.MetadataCacheTTL { + log.Info("WARNING: Authorization cache TTL exceeds metadata cache TTL. "+ + "Authorization caches will be capped at metadata TTL to prevent stale authorization decisions.", + "authzCacheTTL", r.AuthzCacheTTL, + "metadataCacheTTL", r.MetadataCacheTTL, + "effectiveAuthzTTL", r.authzCacheTTL()) + } + // Watch generated AuthPolicies so we re-reconcile when someone manually edits them. generatedAuthPolicy := &unstructured.Unstructured{} generatedAuthPolicy.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"}) @@ -849,7 +951,6 @@ func (r *MaaSAuthPolicyReconciler) mapHTTPRouteToMaaSAuthPolicies(ctx context.Co } return requests } - // deduplicateAndSort removes duplicates from a string slice and sorts it. // This ensures stable output across reconciles, preventing spurious updates // caused by non-deterministic Kubernetes List order. diff --git a/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go b/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go index 882425a79..75b1e347b 100644 --- a/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go +++ b/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go @@ -20,6 +20,7 @@ import ( "context" "testing" + maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -28,8 +29,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" - - maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1" ) // newPreexistingAuthPolicy builds a Kuadrant AuthPolicy as an unstructured object @@ -271,6 +270,7 @@ func TestMaaSAuthPolicyReconciler_DeleteAnnotation(t *testing.T) { }) } } + // TestMaaSAuthPolicyReconciler_RemoveModelRef verifies that removing a modelRef from // a MaaSAuthPolicy deletes the aggregated AuthPolicy for the removed model while // keeping the AuthPolicy for the remaining model intact. @@ -557,3 +557,521 @@ func TestMaaSAuthPolicyReconciler_MultiplePoliciesDeletion(t *testing.T) { t.Errorf("AuthPolicy should be deleted after deleting last parent policy, but got error: %v", err) } } + +// TestMaaSAuthPolicyReconciler_CachingConfiguration verifies that the controller +// generates cache blocks on metadata and authorization evaluators with configurable TTLs. +func TestMaaSAuthPolicyReconciler_CachingConfiguration(t *testing.T) { + const ( + modelName = "llm" + namespace = "default" + httpRouteName = "maas-model-" + modelName + authPolicyName = "maas-auth-" + modelName + maasPolicyName = "policy-a" + ) + + tests := []struct { + name string + metadataTTL int64 + authzTTL int64 + wantMetadataTTL int64 + wantAuthzTTL int64 + }{ + { + name: "default TTLs (60s)", + metadataTTL: 60, + authzTTL: 60, + wantMetadataTTL: 60, + wantAuthzTTL: 60, + }, + { + name: "custom metadata TTL (300s)", + metadataTTL: 300, + authzTTL: 60, + wantMetadataTTL: 300, + wantAuthzTTL: 60, + }, + { + name: "custom authz TTL (30s)", + metadataTTL: 60, + authzTTL: 30, + wantMetadataTTL: 60, + wantAuthzTTL: 30, + }, + { + name: "both custom (5min metadata, 1min authz)", + metadataTTL: 300, + authzTTL: 60, + wantMetadataTTL: 300, + wantAuthzTTL: 60, + }, + { + name: "authz TTL capped at metadata TTL (authz=300, metadata=60)", + metadataTTL: 60, + authzTTL: 300, + wantMetadataTTL: 60, + wantAuthzTTL: 60, // Should be capped at metadata TTL + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) + route := newHTTPRoute(httpRouteName, namespace) + maasPolicy := newMaaSAuthPolicy(maasPolicyName, namespace, "team-a", maasv1alpha1.ModelRef{Name: modelName, Namespace: namespace}) + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithRESTMapper(testRESTMapper()). + WithObjects(model, route, maasPolicy). + WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). + Build() + + r := &MaaSAuthPolicyReconciler{ + Client: c, + Scheme: scheme, + MaaSAPINamespace: "maas-system", + MetadataCacheTTL: tc.metadataTTL, + AuthzCacheTTL: tc.authzTTL, + } + + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} + if _, err := r.Reconcile(context.Background(), req); err != nil { + t.Fatalf("Reconcile: unexpected error: %v", err) + } + + got := &unstructured.Unstructured{} + got.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"}) + if err := c.Get(context.Background(), types.NamespacedName{Name: authPolicyName, Namespace: namespace}, got); err != nil { + t.Fatalf("Get AuthPolicy %q: %v", authPolicyName, err) + } + + // Verify apiKeyValidation metadata has cache with correct TTL + apiKeyValTTL, found, err := unstructured.NestedInt64(got.Object, "spec", "rules", "metadata", "apiKeyValidation", "cache", "ttl") + if err != nil || !found { + t.Errorf("apiKeyValidation cache.ttl missing or invalid: found=%v err=%v", found, err) + } else if apiKeyValTTL != tc.wantMetadataTTL { + t.Errorf("apiKeyValidation cache.ttl = %d, want %d", apiKeyValTTL, tc.wantMetadataTTL) + } + + // Verify apiKeyValidation has cache key + apiKeyValCacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "metadata", "apiKeyValidation", "cache", "key", "selector") + if err != nil || !found { + t.Errorf("apiKeyValidation cache.key.selector missing: found=%v err=%v", found, err) + } else if apiKeyValCacheKey == "" { + t.Errorf("apiKeyValidation cache.key.selector is empty") + } + + // Verify subscription-info metadata has cache with correct TTL + subInfoTTL, found, err := unstructured.NestedInt64(got.Object, "spec", "rules", "metadata", "subscription-info", "cache", "ttl") + if err != nil || !found { + t.Errorf("subscription-info cache.ttl missing or invalid: found=%v err=%v", found, err) + } else if subInfoTTL != tc.wantMetadataTTL { + t.Errorf("subscription-info cache.ttl = %d, want %d", subInfoTTL, tc.wantMetadataTTL) + } + + // Verify auth-valid authorization has cache with correct TTL + authValidTTL, found, err := unstructured.NestedInt64(got.Object, "spec", "rules", "authorization", "auth-valid", "cache", "ttl") + if err != nil || !found { + t.Errorf("auth-valid cache.ttl missing or invalid: found=%v err=%v", found, err) + } else if authValidTTL != tc.wantAuthzTTL { + t.Errorf("auth-valid cache.ttl = %d, want %d", authValidTTL, tc.wantAuthzTTL) + } + + // Verify subscription-valid authorization has cache with correct TTL + subValidTTL, found, err := unstructured.NestedInt64(got.Object, "spec", "rules", "authorization", "subscription-valid", "cache", "ttl") + if err != nil || !found { + t.Errorf("subscription-valid cache.ttl missing or invalid: found=%v err=%v", found, err) + } else if subValidTTL != tc.wantAuthzTTL { + t.Errorf("subscription-valid cache.ttl = %d, want %d", subValidTTL, tc.wantAuthzTTL) + } + + // Verify require-group-membership authorization has cache with correct TTL + groupMembershipTTL, found, err := unstructured.NestedInt64(got.Object, "spec", "rules", "authorization", "require-group-membership", "cache", "ttl") + if err != nil || !found { + t.Errorf("require-group-membership cache.ttl missing or invalid: found=%v err=%v", found, err) + } else if groupMembershipTTL != tc.wantAuthzTTL { + t.Errorf("require-group-membership cache.ttl = %d, want %d", groupMembershipTTL, tc.wantAuthzTTL) + } + + // Verify cache keys are present and non-empty + authValidCacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "authorization", "auth-valid", "cache", "key", "selector") + if err != nil || !found { + t.Errorf("auth-valid cache.key.selector missing: found=%v err=%v", found, err) + } else if authValidCacheKey == "" { + t.Errorf("auth-valid cache.key.selector is empty") + } + + subValidCacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "authorization", "subscription-valid", "cache", "key", "selector") + if err != nil || !found { + t.Errorf("subscription-valid cache.key.selector missing: found=%v err=%v", found, err) + } else if subValidCacheKey == "" { + t.Errorf("subscription-valid cache.key.selector is empty") + } + + groupMembershipCacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "authorization", "require-group-membership", "cache", "key", "selector") + if err != nil || !found { + t.Errorf("require-group-membership cache.key.selector missing: found=%v err=%v", found, err) + } else if groupMembershipCacheKey == "" { + t.Errorf("require-group-membership cache.key.selector is empty") + } + }) + } +} + +// TestMaaSAuthPolicyReconciler_NegativeTTLRejection verifies that the controller +// rejects negative cache TTL values at setup time. +func TestMaaSAuthPolicyReconciler_NegativeTTLRejection(t *testing.T) { + tests := []struct { + name string + metadataTTL int64 + authzTTL int64 + expectSetupFail bool + }{ + { + name: "positive TTLs allowed", + metadataTTL: 60, + authzTTL: 30, + expectSetupFail: false, + }, + { + name: "zero TTLs allowed", + metadataTTL: 0, + authzTTL: 0, + expectSetupFail: false, + }, + { + name: "negative metadata TTL rejected", + metadataTTL: -10, + authzTTL: 60, + expectSetupFail: true, + }, + { + name: "negative authz TTL rejected", + metadataTTL: 60, + authzTTL: -10, + expectSetupFail: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r := &MaaSAuthPolicyReconciler{ + MetadataCacheTTL: tc.metadataTTL, + AuthzCacheTTL: tc.authzTTL, + } + + // Test the validation logic used by SetupWithManager + err := r.ValidateCacheTTLs() + + if tc.expectSetupFail { + if err == nil { + t.Errorf("Expected validation to fail for negative TTLs, but got no error") + } + } else { + if err != nil { + t.Errorf("Expected validation to succeed, but got error: %v", err) + } + + // Also verify authzCacheTTL() defensive clamp never returns negative + effectiveTTL := r.authzCacheTTL() + if effectiveTTL < 0 { + t.Errorf("authzCacheTTL() returned negative value %d, should be clamped to 0", effectiveTTL) + } + } + }) + } +} + +// TestMaaSAuthPolicyReconciler_CacheKeyIsolation verifies that cache keys include +// the necessary dimensions to prevent cross-principal or cross-model cache sharing. +// +// This is a security-critical test: incorrect cache keys could leak authentication +// or authorization results between different users, API keys, or models. +func TestMaaSAuthPolicyReconciler_CacheKeyIsolation(t *testing.T) { + const ( + modelName = "llm" + namespace = "default" + httpRouteName = "maas-model-" + modelName + authPolicyName = "maas-auth-" + modelName + maasPolicyName = "policy-a" + ) + + model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) + route := newHTTPRoute(httpRouteName, namespace) + maasPolicy := newMaaSAuthPolicy(maasPolicyName, namespace, "team-a", maasv1alpha1.ModelRef{Name: modelName, Namespace: namespace}) + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithRESTMapper(testRESTMapper()). + WithObjects(model, route, maasPolicy). + WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). + Build() + + r := &MaaSAuthPolicyReconciler{ + Client: c, + Scheme: scheme, + MaaSAPINamespace: "maas-system", + MetadataCacheTTL: 60, + AuthzCacheTTL: 60, + } + + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} + if _, err := r.Reconcile(context.Background(), req); err != nil { + t.Fatalf("Reconcile: unexpected error: %v", err) + } + + got := &unstructured.Unstructured{} + got.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"}) + if err := c.Get(context.Background(), types.NamespacedName{Name: authPolicyName, Namespace: namespace}, got); err != nil { + t.Fatalf("Get AuthPolicy %q: %v", authPolicyName, err) + } + + // Test 1: apiKeyValidation cache key must include API key material + t.Run("apiKeyValidation includes API key", func(t *testing.T) { + cacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "metadata", "apiKeyValidation", "cache", "key", "selector") + if err != nil || !found { + t.Fatalf("apiKeyValidation cache.key.selector missing: found=%v err=%v", found, err) + } + + // Must include the API key from the request header + if !contains(cacheKey, "request.headers.authorization") { + t.Errorf("apiKeyValidation cache key must include API key from request headers, got: %s", cacheKey) + } + + // Should extract the Bearer token + if !contains(cacheKey, `replace("Bearer ", "")`) { + t.Errorf("apiKeyValidation cache key should extract Bearer token, got: %s", cacheKey) + } + }) + + // Test 2: subscription-info cache key must include userId (for API keys) or username (for K8s tokens), groups, and model + t.Run("subscription-info includes username, groups, model", func(t *testing.T) { + cacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "metadata", "subscription-info", "cache", "key", "selector") + if err != nil || !found { + t.Fatalf("subscription-info cache.key.selector missing: found=%v err=%v", found, err) + } + + // Must include userId for API keys (database UUID for collision resistance) + if !contains(cacheKey, "userId") { + t.Errorf("subscription-info cache key must include userId for API keys, got: %s", cacheKey) + } + + // Must include username as fallback for K8s tokens + if !contains(cacheKey, "username") { + t.Errorf("subscription-info cache key must include username for K8s tokens, got: %s", cacheKey) + } + + // Must include groups (from either API key or K8s token) + if !contains(cacheKey, "groups") { + t.Errorf("subscription-info cache key must include groups, got: %s", cacheKey) + } + + // Must include model namespace and name to prevent cross-model sharing + if !contains(cacheKey, namespace) { + t.Errorf("subscription-info cache key must include model namespace %q, got: %s", namespace, cacheKey) + } + if !contains(cacheKey, modelName) { + t.Errorf("subscription-info cache key must include model name %q, got: %s", modelName, cacheKey) + } + + // Groups should be joined to create stable string representation + if !contains(cacheKey, "join") { + t.Errorf("subscription-info cache key should join groups for stability, got: %s", cacheKey) + } + }) + + // Test 3: auth-valid cache key must distinguish API keys from K8s tokens + t.Run("auth-valid distinguishes API keys and K8s tokens", func(t *testing.T) { + cacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "authorization", "auth-valid", "cache", "key", "selector") + if err != nil || !found { + t.Fatalf("auth-valid cache.key.selector missing: found=%v err=%v", found, err) + } + + // Must distinguish between API key and K8s token authentication + if !contains(cacheKey, "api-key") && !contains(cacheKey, "k8s-token") { + t.Errorf("auth-valid cache key must distinguish auth types (api-key vs k8s-token), got: %s", cacheKey) + } + + // Must include model to prevent cross-model sharing + if !contains(cacheKey, namespace) || !contains(cacheKey, modelName) { + t.Errorf("auth-valid cache key must include model namespace/name, got: %s", cacheKey) + } + + // Must include identity (API key or username) + if !contains(cacheKey, "username") && !contains(cacheKey, "authorization") { + t.Errorf("auth-valid cache key must include identity, got: %s", cacheKey) + } + }) + + // Test 4: subscription-valid cache key must match subscription-info for coherence + t.Run("subscription-valid matches subscription-info key", func(t *testing.T) { + subValidKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "authorization", "subscription-valid", "cache", "key", "selector") + if err != nil || !found { + t.Fatalf("subscription-valid cache.key.selector missing: found=%v err=%v", found, err) + } + + subInfoKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "metadata", "subscription-info", "cache", "key", "selector") + if err != nil || !found { + t.Fatalf("subscription-info cache.key.selector missing: found=%v err=%v", found, err) + } + + // subscription-valid should use the same key as subscription-info for cache coherence + if subValidKey != subInfoKey { + t.Errorf("subscription-valid cache key should match subscription-info for cache coherence\nsubscription-valid: %s\nsubscription-info: %s", subValidKey, subInfoKey) + } + + // Verify it includes all necessary dimensions + if !contains(subValidKey, "userId") { + t.Errorf("subscription-valid cache key must include userId for API keys, got: %s", subValidKey) + } + if !contains(subValidKey, "username") { + t.Errorf("subscription-valid cache key must include username for K8s tokens, got: %s", subValidKey) + } + if !contains(subValidKey, "groups") { + t.Errorf("subscription-valid cache key must include groups, got: %s", subValidKey) + } + + if !contains(subValidKey, namespace) || !contains(subValidKey, modelName) { + t.Errorf("subscription-valid cache key must include model namespace/name, got: %s", subValidKey) + } + }) + + // Test 5: require-group-membership cache key must include username, groups, and model + t.Run("require-group-membership includes username, groups, model", func(t *testing.T) { + cacheKey, found, err := unstructured.NestedString(got.Object, "spec", "rules", "authorization", "require-group-membership", "cache", "key", "selector") + if err != nil || !found { + t.Fatalf("require-group-membership cache.key.selector missing: found=%v err=%v", found, err) + } + + // Must include username (authorization depends on user identity) + if !contains(cacheKey, "username") { + t.Errorf("require-group-membership cache key must include username, got: %s", cacheKey) + } + + // Must include groups (authorization checks group membership) + if !contains(cacheKey, "groups") { + t.Errorf("require-group-membership cache key must include groups, got: %s", cacheKey) + } + + // Must include model to prevent cross-model sharing + if !contains(cacheKey, namespace) || !contains(cacheKey, modelName) { + t.Errorf("require-group-membership cache key must include model namespace/name, got: %s", cacheKey) + } + + // Groups should be joined for stable representation + if !contains(cacheKey, "join") { + t.Errorf("require-group-membership cache key should join groups, got: %s", cacheKey) + } + }) +} + +// TestMaaSAuthPolicyReconciler_CacheKeyModelIsolation verifies that different models +// generate different cache keys to prevent cross-model cache sharing. +func TestMaaSAuthPolicyReconciler_CacheKeyModelIsolation(t *testing.T) { + const namespace = "default" + + // Create two different models + model1Name := "llm-1" + model2Name := "llm-2" + + model1 := newMaaSModelRef(model1Name, namespace, "ExternalModel", model1Name) + route1 := newHTTPRoute("maas-model-"+model1Name, namespace) + policy1 := newMaaSAuthPolicy("policy-1", namespace, "team-a", maasv1alpha1.ModelRef{Name: model1Name, Namespace: namespace}) + + model2 := newMaaSModelRef(model2Name, namespace, "ExternalModel", model2Name) + route2 := newHTTPRoute("maas-model-"+model2Name, namespace) + policy2 := newMaaSAuthPolicy("policy-2", namespace, "team-a", maasv1alpha1.ModelRef{Name: model2Name, Namespace: namespace}) + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithRESTMapper(testRESTMapper()). + WithObjects(model1, route1, policy1, model2, route2, policy2). + WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). + Build() + + r := &MaaSAuthPolicyReconciler{ + Client: c, + Scheme: scheme, + MaaSAPINamespace: "maas-system", + MetadataCacheTTL: 60, + AuthzCacheTTL: 60, + } + + // Reconcile both policies + req1 := ctrl.Request{NamespacedName: types.NamespacedName{Name: "policy-1", Namespace: namespace}} + if _, err := r.Reconcile(context.Background(), req1); err != nil { + t.Fatalf("Reconcile policy-1: %v", err) + } + + req2 := ctrl.Request{NamespacedName: types.NamespacedName{Name: "policy-2", Namespace: namespace}} + if _, err := r.Reconcile(context.Background(), req2); err != nil { + t.Fatalf("Reconcile policy-2: %v", err) + } + + // Get both AuthPolicies + ap1 := &unstructured.Unstructured{} + ap1.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"}) + if err := c.Get(context.Background(), types.NamespacedName{Name: "maas-auth-" + model1Name, Namespace: namespace}, ap1); err != nil { + t.Fatalf("Get AuthPolicy for model-1: %v", err) + } + + ap2 := &unstructured.Unstructured{} + ap2.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"}) + if err := c.Get(context.Background(), types.NamespacedName{Name: "maas-auth-" + model2Name, Namespace: namespace}, ap2); err != nil { + t.Fatalf("Get AuthPolicy for model-2: %v", err) + } + + // Test that cache keys differ between models for each evaluator + evaluators := []struct { + name string + path []string + }{ + {"subscription-info", []string{"spec", "rules", "metadata", "subscription-info", "cache", "key", "selector"}}, + {"auth-valid", []string{"spec", "rules", "authorization", "auth-valid", "cache", "key", "selector"}}, + {"subscription-valid", []string{"spec", "rules", "authorization", "subscription-valid", "cache", "key", "selector"}}, + {"require-group-membership", []string{"spec", "rules", "authorization", "require-group-membership", "cache", "key", "selector"}}, + } + + for _, eval := range evaluators { + t.Run(eval.name+" isolates models", func(t *testing.T) { + key1, found, err := unstructured.NestedString(ap1.Object, eval.path...) + if err != nil || !found { + t.Fatalf("model-1 %s cache key missing: found=%v err=%v", eval.name, found, err) + } + + key2, found, err := unstructured.NestedString(ap2.Object, eval.path...) + if err != nil || !found { + t.Fatalf("model-2 %s cache key missing: found=%v err=%v", eval.name, found, err) + } + + // Keys must differ between models (they include model namespace/name) + if key1 == key2 { + t.Errorf("%s cache keys must differ between models, but both are: %s", eval.name, key1) + } + + // Verify model-1 key contains model-1 name + if !contains(key1, model1Name) { + t.Errorf("model-1 %s cache key must contain model name %q, got: %s", eval.name, model1Name, key1) + } + + // Verify model-2 key contains model-2 name + if !contains(key2, model2Name) { + t.Errorf("model-2 %s cache key must contain model name %q, got: %s", eval.name, model2Name, key2) + } + }) + } +} + +// contains is a helper to check if a string contains a substring (case-sensitive). +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(substr) == 0 || + func() bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false + }()) +} +