fix: address CodeRabbit security and validation findings

Three improvements based on CodeRabbit review:

1. UUID Validation (Defense-in-Depth):
   - Add strict UUID validation in ValidateAPIKey before using metadata.ID
   - Fail-closed: return error instead of proceeding with malformed IDs
   - Prevents malformed IDs from being used in cache keys or authz decisions
   - Updated tests to use realistic UUIDs instead of human-readable IDs

2. Negative TTL Test Coverage:
   - Extract ValidateCacheTTLs() helper function from SetupWithManager
   - Update TestMaaSAuthPolicyReconciler_NegativeTTLRejection to actually
     test the validation logic used by SetupWithManager
   - Test now verifies expectSetupFail cases properly reject negative TTLs

3. X-MaaS-Group JSON Construction (Injection Prevention):
   - Replace string concatenation ('["' + ... + '"]') with CEL string() conversion
   - Properly escapes special characters in group names
   - Prevents JSON injection if group names contain quotes or brackets
   - Empty arrays now serialize as [] instead of potentially malformed strings

All tests pass after changes.

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

Author: jrhyness

maas-api/internal/api_keys/service.go

@@ -239,6 +239,14 @@ 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,

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"}
@@ -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"
 
@@ -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"
 

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

@@ -459,10 +459,11 @@ allow {
 						"metrics":  false,
 						"priority": int64(0),
 					},
-					// Groups - construct JSON array string from API key validation or K8s identity
+					// 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": `'["' + ((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups).join('","') + '"]'`,
+							"expression": `string(((has(auth.metadata) && has(auth.metadata.apiKeyValidation)) ? auth.metadata.apiKeyValidation.groups : auth.identity.user.groups))`,
 						},
 						"metrics":  false,
 						"priority": int64(0),
@@ -758,17 +759,26 @@ func (r *MaaSAuthPolicyReconciler) updateStatus(ctx context.Context, policy *maa
 	}
 }
 
-func (r *MaaSAuthPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	// Validate cache TTL configuration
-	log := ctrl.Log.WithName("maas-authpolicy-controller")
-
-	// Reject negative TTL values
+// 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. "+

maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go

@@ -585,18 +585,24 @@ func TestMaaSAuthPolicyReconciler_NegativeTTLRejection(t *testing.T) {
 				AuthzCacheTTL:    tc.authzTTL,
 			}
 
-			// SetupWithManager requires a manager, but we can't easily create one in unit tests.
-			// Instead, directly test the validation logic by checking the authzCacheTTL() defensive clamp
-			// and noting that SetupWithManager would reject negative values.
-
-			// The defensive clamp in authzCacheTTL() should never return negative values
-			effectiveTTL := r.authzCacheTTL()
-			if effectiveTTL < 0 {
-				t.Errorf("authzCacheTTL() returned negative value %d, should be clamped to 0", effectiveTTL)
-			}
+			// 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)
+				}
 
-			// Note: Full SetupWithManager validation is tested via integration tests
-			// as it requires a controller-runtime manager.
+				// 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)
+				}
+			}
 		})
 	}
 }