fix: address CodeRabbit PR review findings

Security fixes:
- Add fail-closed validation for API keys without bound subscription
- Reject API key requests missing x-maas-subscription header (403)
- Fix tests to use bare subscription names matching production behavior

Model listing improvements:
- Include ownedBy in deduplication key to keep different MaaSModelRef
  resources separate even if they serve the same model
- Update documentation to reflect new deduplication behavior
- Update tests to expect separate entries for different MaaSModelRefs

Controller fixes:
- Use GetAPIReader instead of GetClient for pre-start cluster detection
  (cache not started yet, would return stale/missing data)
- Add RBAC permission for config.openshift.io/authentications (get)
- Deduplicate and sort aggregated policy lists to prevent spurious
  reconciles from non-deterministic Kubernetes List order
- Fix malformed Rego syntax in auth-valid rule (use separate allow blocks)

Test cleanup:
- Remove unnecessary f-string prefixes from log.info calls (Ruff F541)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Author: jrhyness

docs/content/configuration-and-management/model-listing-flow.md

@@ -140,16 +140,22 @@ All models in the response include a `subscriptions` array with metadata for eac
 
 ### Deduplication Behavior
 
-When `X-MaaS-Return-All-Models: true` is used, models are deduplicated by `(id, url)` key:
+Models are deduplicated by `(id, url, ownedBy)` key:
 
-- **Same id + same URL**: Single entry with subscriptions aggregated into the `subscriptions` array
-- **Same id + different URLs**: Separate entries (different model endpoints)
+- **Same id + same URL + same MaaSModelRef (ownedBy)**: Single entry with subscriptions aggregated into the `subscriptions` array
+- **Different id, URL, or MaaSModelRef**: Separate entries
 
-**Example:**
-- Model `gpt-3.5` at URL `https://example.com/gpt-3.5` is accessible via subscriptions A and B
+**User token authentication** (multiple subscriptions):
+- Model `gpt-3.5` from MaaSModelRef `namespace-a/model-a` at URL `https://example.com/gpt-3.5` is accessible via subscriptions A and B
   - Result: One entry with `subscriptions: [{name: "A"}, {name: "B"}]`
-- Model `gpt-3.5` at URL `https://example.com/gpt-3.5-premium` is only in subscription B
-  - Result: Separate entry with `subscriptions: [{name: "B"}]`
+- Model `gpt-3.5` from MaaSModelRef `namespace-b/model-b` at the same URL is only in subscription B
+  - Result: Separate entry with `subscriptions: [{name: "B"}]` (different MaaSModelRef)
+- Model `gpt-3.5` at URL `https://example.com/gpt-3.5-premium` from `namespace-a/model-a` is only in subscription B
+  - Result: Separate entry with `subscriptions: [{name: "B"}]` (different URL)
+
+**API key authentication** (single subscription):
+- Deduplication handles edge cases where multiple MaaSModelRef resources point to the same model endpoint
+- Each unique MaaSModelRef resource appears as a separate entry
 
 !!! tip "Subscription metadata fields"
     The `displayName` and `description` fields are read from the MaaSSubscription CRD's `spec.displayName` and `spec.description` fields. If these fields are not set in the CRD, they will be empty strings in the response.

maas-api/internal/api_keys/service.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/google/uuid"
@@ -227,6 +228,17 @@ func (s *Service) ValidateAPIKey(ctx context.Context, key string) (*ValidationRe
 		groups = []string{} // Return empty array if no groups stored
 	}
 
+	// Fail closed: reject keys with no bound subscription (CWE-284)
+	// This prevents legacy keys, bad migrations, or manual writes with empty subscription
+	// from bypassing the "subscription bound at mint" access control invariant
+	if strings.TrimSpace(metadata.Subscription) == "" {
+		s.logger.Warn("API key missing bound subscription", "key_id", metadata.ID)
+		return &ValidationResult{
+			Valid:  false,
+			Reason: "key has no subscription bound",
+		}, nil
+	}
+
 	// Success - return user identity and groups for Authorino
 	return &ValidationResult{
 		Valid:        true,

maas-api/internal/handlers/models.go

@@ -188,11 +188,23 @@ func (h *ModelsHandler) ListLLMs(c *gin.Context) {
 	// For API keys: Authorino injects this from auth.metadata.apiKeyValidation.subscription
 	// For user tokens: This header is not present (Authorino doesn't inject it)
 	requestedSubscription := strings.TrimSpace(c.GetHeader("x-maas-subscription"))
+	isAPIKeyRequest := strings.HasPrefix(authHeader, "Bearer sk-oai-")
+
+	// Fail closed: API keys without a bound subscription must be rejected
+	if isAPIKeyRequest && requestedSubscription == "" {
+		h.logger.Debug("API key request missing bound subscription header")
+		c.JSON(http.StatusForbidden, gin.H{
+			"error": gin.H{
+				"message": "API key has no subscription bound",
+				"type":    "permission_error",
+			}})
+		return
+	}
 
 	// Determine behavior based on auth method:
 	// - API key with subscription → filter by that subscription (requestedSubscription != "")
 	// - User token → return all accessible models (requestedSubscription == "")
-	returnAllModels := requestedSubscription == ""
+	returnAllModels := !isAPIKeyRequest && requestedSubscription == ""
 
 	// Get user context for subscription selection
 	var userContext *token.UserContext
@@ -264,11 +276,13 @@ func (h *ModelsHandler) ListLLMs(c *gin.Context) {
 			}
 		} else {
 			// Filter models by subscription(s) and aggregate subscriptions
-			// Deduplication key is (model ID, URL) - models with the same ID and URL are
-			// the same instance and should have their subscriptions aggregated into an array.
+			// Deduplication key is (model ID, URL, OwnedBy) - models with the same ID, URL, and
+			// MaaSModelRef (namespace/name) are the same instance and should have their
+			// subscriptions aggregated into an array.
 			type modelKey struct {
-				id  string
-				url string
+				id      string
+				url     string
+				ownedBy string
 			}
 			modelsByKey := make(map[modelKey]*models.Model)
 
@@ -300,12 +314,12 @@ func (h *ModelsHandler) ListLLMs(c *gin.Context) {
 						Description: sub.Description,
 					}
 
-					// Create key from model ID and URL
+					// Create key from model ID, URL, and OwnedBy (namespace/name of MaaSModelRef)
 					urlStr := ""
 					if model.URL != nil {
 						urlStr = model.URL.String()
 					}
-					key := modelKey{id: model.ID, url: urlStr}
+					key := modelKey{id: model.ID, url: urlStr, ownedBy: model.OwnedBy}
 
 					if existingModel, exists := modelsByKey[key]; exists {
 						// Model already exists - append subscription if not already present
@@ -327,7 +341,10 @@ func (h *ModelsHandler) ListLLMs(c *gin.Context) {
 				if keys[i].id != keys[j].id {
 					return keys[i].id < keys[j].id
 				}
-				return keys[i].url < keys[j].url
+				if keys[i].url != keys[j].url {
+					return keys[i].url < keys[j].url
+				}
+				return keys[i].ownedBy < keys[j].ownedBy
 			})
 			for _, k := range keys {
 				modelList = append(modelList, *modelsByKey[k])

maas-api/internal/handlers/models_test.go

@@ -402,9 +402,9 @@ func TestListingModelsWithSubscriptionHeader(t *testing.T) {
 	testLogger := logger.Development()
 
 	// Create mock servers that require specific subscription headers
-	// Use qualified names (namespace/name) to match the format sent by the handler
-	premiumModelServer := createMockModelServerWithSubscriptionCheck(t, "premium-model", "test-namespace/premium")
-	freeModelServer := createMockModelServerWithSubscriptionCheck(t, "free-model", "test-namespace/free")
+	// Use bare subscription names to match what Authorino injects from API key validation
+	premiumModelServer := createMockModelServerWithSubscriptionCheck(t, "premium-model", "premium")
+	freeModelServer := createMockModelServerWithSubscriptionCheck(t, "free-model", "free")
 
 	// Build MaaSModelRef unstructured list
 	maasModelRefItems := []*unstructured.Unstructured{
@@ -581,10 +581,10 @@ func TestListModels_ReturnAllModels(t *testing.T) {
 	testLogger := logger.Development()
 
 	// Create mock servers for models
-	// Use qualified names (namespace/name) to match the format sent by the handler
-	model1Server := createMockModelServerWithSubscriptionCheck(t, "model-1", "test-namespace/sub-a")
-	model2Server := createMockModelServerWithSubscriptionCheck(t, "model-2", "test-namespace/sub-b")
-	model3Server := createMockModelServerWithSubscriptionCheck(t, "model-3", "test-namespace/sub-a")
+	// Use bare subscription names to match what Authorino injects from API key validation
+	model1Server := createMockModelServerWithSubscriptionCheck(t, "model-1", "sub-a")
+	model2Server := createMockModelServerWithSubscriptionCheck(t, "model-2", "sub-b")
+	model3Server := createMockModelServerWithSubscriptionCheck(t, "model-3", "sub-a")
 
 	// Setup MaaSModelRef lister with three models
 	lister := fakeMaaSModelRefLister{
@@ -1049,7 +1049,7 @@ func TestListModels_DifferentModelRefsWithSameURLAndModelID(t *testing.T) {
 	v1 := router.Group("/v1")
 	v1.GET("/models", tokenHandler.ExtractUserInfo(), modelsHandler.ListLLMs)
 
-	t.Run("different MaaSModelRefs with same URL and model ID are deduplicated", func(t *testing.T) {
+	t.Run("different MaaSModelRefs with same URL and model ID remain separate entries", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "/v1/models", nil)
 		require.NoError(t, err)
@@ -1066,14 +1066,19 @@ func TestListModels_DifferentModelRefsWithSameURLAndModelID(t *testing.T) {
 		err = json.Unmarshal(w.Body.Bytes(), &response)
 		require.NoError(t, err)
 
-		// Should have 1 entry because same URL and model ID (deduplicated)
-		assert.Len(t, response.Data, 1, "Same URL and model ID should be deduplicated into single entry")
+		// Should have 2 entries because different MaaSModelRef resources (different ownedBy)
+		// even though they have the same URL and model ID
+		assert.Len(t, response.Data, 2, "Different MaaSModelRef resources should remain separate entries")
 
-		model := response.Data[0]
-		assert.Equal(t, "gpt-4", model.ID)
-		assert.Equal(t, sharedModelServer.URL, model.URL.String())
-		require.Len(t, model.Subscriptions, 1, "Model should have 1 subscription")
-		assert.Equal(t, "sub-a", model.Subscriptions[0].Name)
+		// Both should have model ID "gpt-4" and same URL but different ownedBy
+		for _, model := range response.Data {
+			assert.Equal(t, "gpt-4", model.ID)
+			assert.Equal(t, sharedModelServer.URL, model.URL.String())
+			require.Len(t, model.Subscriptions, 1, "Each model should have 1 subscription")
+			assert.Equal(t, "sub-a", model.Subscriptions[0].Name)
+			// OwnedBy should be either namespace-a/gpt-4-ref or namespace-b/gpt-4-another-ref
+			assert.Contains(t, []string{"namespace-a/gpt-4-ref", "namespace-b/gpt-4-another-ref"}, model.OwnedBy)
+		}
 	})
 }
 
@@ -1082,9 +1087,9 @@ func TestListModels_DifferentModelRefsWithSameModelIDAndDifferentSubscriptions(t
 
 	// Create two mock servers that both return the same model ID "gpt-4"
 	// One accessible via sub-a, one via sub-b
-	// Use qualified names (namespace/name) to match the format sent by the handler
-	modelServerA := createMockModelServerWithSubscriptionCheck(t, "gpt-4", "test-namespace/sub-a")
-	modelServerB := createMockModelServerWithSubscriptionCheck(t, "gpt-4", "test-namespace/sub-b")
+	// Use bare subscription names to match what Authorino injects from API key validation
+	modelServerA := createMockModelServerWithSubscriptionCheck(t, "gpt-4", "sub-a")
+	modelServerB := createMockModelServerWithSubscriptionCheck(t, "gpt-4", "sub-b")
 
 	// Setup MaaSModelRef lister with two different MaaSModelRefs in different namespaces
 	lister := fakeMaaSModelRefLister{

maas-controller/cmd/manager/main.go

@@ -54,7 +54,8 @@ func init() {
 
 // getClusterServiceAccountIssuer fetches the cluster's service account issuer from OpenShift/ROSA configuration.
 // Returns empty string if not found or not running on OpenShift/ROSA.
-func getClusterServiceAccountIssuer(c client.Client) (string, error) {
+// Uses client.Reader (not client.Client) so it can be called before the manager cache starts.
+func getClusterServiceAccountIssuer(c client.Reader) (string, error) {
 	// Try to fetch the OpenShift Authentication config resource
 	// This works on OpenShift/ROSA but not on vanilla Kubernetes
 	authConfig := &unstructured.Unstructured{}
@@ -128,8 +129,9 @@ func main() {
 	}
 
 	// Auto-detect cluster audience from OpenShift/ROSA if using default value
+	// Use GetAPIReader() instead of GetClient() because the cache hasn't started yet
 	if clusterAudience == "https://kubernetes.default.svc" {
-		if detectedAudience, err := getClusterServiceAccountIssuer(mgr.GetClient()); err == nil && detectedAudience != "" {
+		if detectedAudience, err := getClusterServiceAccountIssuer(mgr.GetAPIReader()); err == nil && detectedAudience != "" {
 			setupLog.Info("auto-detected cluster service account issuer", "audience", detectedAudience)
 			clusterAudience = detectedAudience
 		} else if err != nil {

maas-controller/pkg/controller/maas/maasauthpolicy_controller.go

@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -59,13 +60,6 @@ type MaaSAuthPolicyReconciler struct {
 	ClusterAudience string
 }
 
-func (r *MaaSAuthPolicyReconciler) gatewayName() string {
-	if r.GatewayName != "" {
-		return r.GatewayName
-	}
-	return defaultGatewayName
-}
-
 func (r *MaaSAuthPolicyReconciler) clusterAudience() string {
 	if r.ClusterAudience != "" {
 		return r.ClusterAudience
@@ -79,6 +73,7 @@ func (r *MaaSAuthPolicyReconciler) clusterAudience() string {
 //+kubebuilder:rbac:groups=maas.opendatahub.io,resources=maasmodelrefs,verbs=get;list;watch
 //+kubebuilder:rbac:groups=kuadrant.io,resources=authpolicies,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=httproutes,verbs=get;list;watch
+//+kubebuilder:rbac:groups=config.openshift.io,resources=authentications,verbs=get
 
 // Reconcile is part of the main kubernetes reconciliation loop
 const maasAuthPolicyFinalizer = "maas.opendatahub.io/authpolicy-cleanup"
@@ -182,6 +177,12 @@ func (r *MaaSAuthPolicyReconciler) reconcileModelAuthPolicies(ctx context.Contex
 			}
 		}
 
+		// Deduplicate and sort to ensure stable output across reconciles
+		// (Kubernetes List order is not guaranteed to be deterministic)
+		policyNames = deduplicateAndSort(policyNames)
+		allowedGroups = deduplicateAndSort(allowedGroups)
+		allowedUsers = deduplicateAndSort(allowedUsers)
+
 		// Construct API URLs using configured namespace
 		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)
@@ -287,12 +288,14 @@ func (r *MaaSAuthPolicyReconciler) reconcileModelAuthPolicies(ctx context.Contex
 			"metrics":  false,
 			"priority": int64(0),
 			"opa": map[string]interface{}{
-				"rego": `allow {
-  # API key authentication: validate the key
+				"rego": `# API key authentication: validate the key
+allow {
   object.get(input.auth.metadata, "apiKeyValidation", {})
   input.auth.metadata.apiKeyValidation.valid == true
-} {
-  # Kubernetes token authentication: check identity exists
+}
+
+# Kubernetes token authentication: check identity exists
+allow {
   object.get(input.auth.identity, "user", {}).username != ""
 }`,
 			},
@@ -779,3 +782,24 @@ 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.
+func deduplicateAndSort(items []string) []string {
+	if len(items) == 0 {
+		return items
+	}
+	// Use a map to deduplicate
+	seen := make(map[string]bool, len(items))
+	for _, item := range items {
+		seen[item] = true
+	}
+	// Build deduplicated slice
+	result := make([]string, 0, len(seen))
+	for item := range seen {
+		result = append(result, item)
+	}
+	// Sort for deterministic output
+	sort.Strings(result)
+	return result
+}

test/e2e/tests/test_models_endpoint.py

@@ -864,7 +864,7 @@ def test_different_modelrefs_same_model_id(self):
             log.info(f"📊 API Response: {len(models)} total model(s), {len(unique_ids)} unique ID(s)")
             log.info(f"   Model IDs: {model_ids}")
             log.info(f"   Unique IDs: {unique_ids}")
-            log.info(f"   Subscription had: 2 different modelRefs both serving 'facebook/opt-125m'")
+            log.info("   Subscription had: 2 different modelRefs both serving 'facebook/opt-125m'")
 
             # Both modelRefs serve the same model ID
             assert len(unique_ids) == 1, \
@@ -1511,7 +1511,7 @@ def test_api_key_with_deleted_subscription_403(self):
             _wait_reconcile()
 
             # Query with API key (gateway injects deleted subscription name)
-            log.info(f"Querying /v1/models with API key bound to deleted subscription")
+            log.info("Querying /v1/models with API key bound to deleted subscription")
             r = requests.get(
                 f"{_maas_api_url()}/v1/models",
                 headers={
@@ -1572,7 +1572,7 @@ def test_api_key_with_inaccessible_subscription_403(self):
             # User tries to query with their token but specifying the other user's subscription
             # This simulates what would happen if an API key was bound to a subscription
             # the user doesn't have access to
-            log.info(f"Querying /v1/models with user token and inaccessible subscription")
+            log.info("Querying /v1/models with user token and inaccessible subscription")
             r = requests.get(
                 f"{_maas_api_url()}/v1/models",
                 headers={