Address review comments

.secrets.baseline

@@ -433,7 +433,7 @@
         "filename": "templates/service-template.yml",
         "hashed_secret": "4e199b4a1c40b497a95fcd1cd896351733849949",
         "is_verified": false,
-        "line_number": 700,
+        "line_number": 710,
         "is_secret": false
       }
     ],
@@ -463,5 +463,5 @@
       }
     ]
   },
-  "generated_at": "2024-05-06T16:20:14Z"
+  "generated_at": "2024-05-13T18:42:38Z"
 }

config/fleetshard-authz-development.yaml

@@ -0,0 +1,15 @@
+---
+# A list of org_id's that allow access to the internal API of the fleet manager.
+allowed_org_ids:
+  # RH ACS Organization (returned for personal tokens obtained by ocm token).
+  - "11009103"
+  # RHACS Managed Service Org for staging (Entry `RHACS Managed Service Org for AWS Billing` in Bitwarden).
+  - "16064548"
+  # ACS RH SSO Org Prod
+  - "16134752"
+  # ACS RH SSO ORG Development
+  - "16155304"
+allowed_subjects:
+  - "system:serviceaccount:rhacs:fleetshard-sync"
+allowed_audience:
+  - "acs-fleet-manager-private-api-development"

config/fleetshard-authz-prod.yaml

@@ -0,0 +1,9 @@
+---
+# A list of org_id's that allow access to the internal API of the fleet manager.
+allowed_org_ids:
+  # ACS RH SSO Org Prod
+  - "16134752"
+allowed_subjects:
+  - "system:serviceaccount:rhacs:fleetshard-sync"
+allowed_audiences:
+  - "acs-fleet-manager-private-api-production"

pkg/auth/acs_claims.go

@@ -79,6 +79,15 @@ func (c *ACSClaims) GetSubject() (string, error) {
 	return "", fmt.Errorf("can't find %q attribute in claims", tenantSubClaim)
 }
 
+// GetAudience returns the audience claim of the token. It identifies the token consumer.
+func (c *ACSClaims) GetAudience() (string, error) {
+	if sub, ok := (*c)[audienceClaim].(string); ok {
+		return sub, nil
+	}
+
+	return "", fmt.Errorf("can't find %q attribute in claims", audienceClaim)
+}
+
 // IsOrgAdmin ...
 func (c *ACSClaims) IsOrgAdmin() bool {
 	isOrgAdmin, _ := (*c)[tenantOrgAdminClaim].(bool)

pkg/auth/context_config.go

@@ -24,6 +24,8 @@ var (
 
 	// Sub claim in old format.
 	alternateSubClaim = "deprecated_sub"
+
+	audienceClaim = "aud"
 )
 
 // ContextConfig ...

pkg/auth/fleetshard_authz.go

@@ -9,59 +9,57 @@ import (
 	"gopkg.in/yaml.v2"
 )
 
-// AllowedOrgIDs ...
-type AllowedOrgIDs []string
+// ClaimValues a list of claim values that a fleetshard access token may contain
+type ClaimValues []string
 
-// IsOrgIDAllowed ...
-func (allowedOrgIDs AllowedOrgIDs) IsOrgIDAllowed(orgID string) bool {
-	return arrays.FindFirstString(allowedOrgIDs, func(allowedOrgID string) bool {
-		return orgID == allowedOrgID
+// Contains returns true if the specified value is present in the list
+func (v ClaimValues) Contains(value string) bool {
+	return arrays.FindFirstString(v, func(allowedValue string) bool {
+		return value == allowedValue
 	}) != -1
 }
 
 // FleetShardAuthZConfig ...
 type FleetShardAuthZConfig struct {
-	Enabled           bool
-	AllowedOrgIDs     AllowedOrgIDs
-	AllowedOrgIDsFile string
-	AllowedSubject    string
+	Enabled          bool        `yaml:"-"`
+	File             string      `yaml:"-"`
+	AllowedOrgIDs    ClaimValues `yaml:"allowed_org_ids"`
+	AllowedSubjects  ClaimValues `yaml:"allowed_subjects"`
+	AllowedAudiences ClaimValues `yaml:"allowed_audiences"`
 }
 
 // NewFleetShardAuthZConfig ...
 func NewFleetShardAuthZConfig() *FleetShardAuthZConfig {
 	return &FleetShardAuthZConfig{
-		Enabled:           true,
-		AllowedOrgIDsFile: "config/fleetshard-authz-org-ids-prod.yaml",
-		AllowedSubject:    "system:serviceaccount:rhacs:fleetshard-sync",
+		Enabled: true,
+		File:    "config/fleetshard-authz-prod.yaml",
 	}
 }
 
 // AddFlags ...
 func (c *FleetShardAuthZConfig) AddFlags(fs *pflag.FlagSet) {
-	fs.StringVar(&c.AllowedOrgIDsFile, "fleetshard-authz-config-file", c.AllowedOrgIDsFile,
+	fs.StringVar(&c.File, "fleetshard-authz-config-file", c.File,
 		"Fleetshard authZ middleware configuration file containing a list of allowed org IDs")
 	fs.BoolVar(&c.Enabled, "enable-fleetshard-authz", c.Enabled, "Enable fleetshard authZ "+
 		"via the list of allowed org IDs")
-	fs.StringVar(&c.AllowedSubject, "fleetshard-authz-subject", c.AllowedSubject,
-		"Fleetshard authZ middleware allowed subject")
 }
 
 // ReadFiles ...
 func (c *FleetShardAuthZConfig) ReadFiles() error {
 	if c.Enabled {
-		return readFleetShardAuthZConfigFile(c.AllowedOrgIDsFile, &c.AllowedOrgIDs)
+		return readFleetShardAuthZConfigFile(c.File, c)
 	}
 
 	return nil
 }
 
-func readFleetShardAuthZConfigFile(file string, val *AllowedOrgIDs) error {
+func readFleetShardAuthZConfigFile(file string, config *FleetShardAuthZConfig) error {
 	fileContents, err := shared.ReadFile(file)
 	if err != nil {
 		return fmt.Errorf("reading FleedShard AuthZ config: %w", err)
 	}
 
-	err = yaml.UnmarshalStrict([]byte(fileContents), val)
+	err = yaml.UnmarshalStrict([]byte(fileContents), config)
 	if err != nil {
 		return fmt.Errorf("unmarshalling FleedShard AuthZ config: %w", err)
 	}

pkg/auth/fleetshard_authz_middleware.go

@@ -34,7 +34,8 @@ func fleetShardAuthorizationMiddleware(iamConfig *iam.IAMConfig, fleetShardAuthZ
 				next = NewRequireOrgIDMiddleware().RequireOrgID(errors.ErrorNotFound)(next)
 			} else {
 				// middlewares must be applied in REVERSE order (last comes first)
-				next = checkSubject(fleetShardAuthZConfig.AllowedSubject)(next)
+				next = checkSubject(fleetShardAuthZConfig.AllowedSubjects)(next)
+				next = checkAudience(fleetShardAuthZConfig.AllowedAudiences)(next)
 				next = NewRequireIssuerMiddleware().RequireIssuer(iamConfig.DataPlaneOIDCIssuers.URIs, errors.ErrorNotFound)(next)
 			}
 
@@ -43,34 +44,21 @@ func fleetShardAuthorizationMiddleware(iamConfig *iam.IAMConfig, fleetShardAuthZ
 	}
 }
 
-func checkAllowedOrgIDs(allowedOrgIDs AllowedOrgIDs) mux.MiddlewareFunc {
-	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
-			ctx := request.Context()
-			claims, err := GetClaimsFromContext(ctx)
-			if err != nil {
-				// Deliberately return 404 here so that it will appear as the endpoint doesn't exist if requests are
-				// not authorised. Otherwise, we would leak information about existing cluster IDs, since the path
-				// of the request is /agent-clusters/<id>.
-				shared.HandleError(request, writer, errors.NotFound(""))
-				return
-			}
-
-			orgID, _ := claims.GetOrgID()
-			if allowedOrgIDs.IsOrgIDAllowed(orgID) {
-				next.ServeHTTP(writer, request)
-				return
-			}
+func checkAllowedOrgIDs(allowedOrgIDs []string) mux.MiddlewareFunc {
+	return checkClaim("org_id", ACSClaims.GetOrgID, allowedOrgIDs)
+}
 
-			glog.Infof("org_id %q is not in the list of allowed org IDs [%s]",
-				orgID, strings.Join(allowedOrgIDs, ","))
+func checkSubject(subjects []string) mux.MiddlewareFunc {
+	return checkClaim("sub", ACSClaims.GetSubject, subjects)
+}
 
-			shared.HandleError(request, writer, errors.NotFound(""))
-		})
-	}
+func checkAudience(audiences []string) mux.MiddlewareFunc {
+	return checkClaim("aud", ACSClaims.GetAudience, audiences)
 }
 
-func checkSubject(expected string) mux.MiddlewareFunc {
+type ClaimsGetter func(ACSClaims) (string, error)
+
+func checkClaim(claimName string, getter ClaimsGetter, allowedValues ClaimValues) mux.MiddlewareFunc {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
 			ctx := request.Context()
@@ -83,13 +71,14 @@ func checkSubject(expected string) mux.MiddlewareFunc {
 				return
 			}
 
-			s, _ := claims.GetSubject()
-			if expected == s {
+			claimValue, _ := getter(claims)
+			if allowedValues.Contains(claimValue) {
 				next.ServeHTTP(writer, request)
 				return
 			}
 
-			glog.Infof("fleetshard subject is not allowed (expected=%s, actual=%s)", expected, s)
+			glog.Infof("%s %q is not in the list of allowed values [%s]",
+				claimName, claimValue, strings.Join(allowedValues, ","))
 
 			shared.HandleError(request, writer, errors.NotFound(""))
 		})

pkg/auth/fleetshard_authz_middleware_test.go

@@ -19,7 +19,7 @@ func TestUseFleetShardAuthorizationMiddleware(t *testing.T) {
 	tests := map[string]struct {
 		token              *jwt.Token
 		expectedStatusCode int
-		allowedOrgIDs      AllowedOrgIDs
+		allowedOrgIDs      ClaimValues
 	}{
 		"should succeed when org_id is contained within allowed org IDs": {
 			token: &jwt.Token{
@@ -28,7 +28,7 @@ func TestUseFleetShardAuthorizationMiddleware(t *testing.T) {
 					"org_id": "123",
 				},
 			},
-			allowedOrgIDs:      AllowedOrgIDs{"123", "456"},
+			allowedOrgIDs:      ClaimValues{"123", "456"},
 			expectedStatusCode: http.StatusOK,
 		},
 		"should fail when org_id is not contained within allowed org IDs": {
@@ -38,14 +38,14 @@ func TestUseFleetShardAuthorizationMiddleware(t *testing.T) {
 					"org_id": "123",
 				},
 			},
-			allowedOrgIDs:      AllowedOrgIDs{"456"},
+			allowedOrgIDs:      ClaimValues{"456"},
 			expectedStatusCode: http.StatusNotFound,
 		},
 		"should fail when org_id is not set": {
 			token: &jwt.Token{
 				Claims: jwt.MapClaims{},
 			},
-			allowedOrgIDs:      AllowedOrgIDs{"456"},
+			allowedOrgIDs:      ClaimValues{"456"},
 			expectedStatusCode: http.StatusNotFound,
 		},
 		"should fail when issuer cannot be verified": {
@@ -55,7 +55,7 @@ func TestUseFleetShardAuthorizationMiddleware(t *testing.T) {
 					"org_id": "123",
 				},
 			},
-			allowedOrgIDs:      AllowedOrgIDs{"123", "456"},
+			allowedOrgIDs:      ClaimValues{"123", "456"},
 			expectedStatusCode: http.StatusNotFound,
 		},
 		"should fail when issuer can be verified but org_id is not set": {
@@ -64,7 +64,7 @@ func TestUseFleetShardAuthorizationMiddleware(t *testing.T) {
 					"iss": validIssuer,
 				},
 			},
-			allowedOrgIDs:      AllowedOrgIDs{"123", "456"},
+			allowedOrgIDs:      ClaimValues{"123", "456"},
 			expectedStatusCode: http.StatusNotFound,
 		},
 	}
@@ -101,7 +101,7 @@ func TestUseFleetShardAuthorizationMiddleware(t *testing.T) {
 }
 
 func TestUseFleetShardAuthorizationMiddleware_NoTokenSet(t *testing.T) {
-	var allowedOrgIds = AllowedOrgIDs{"123", "345"}
+	var allowedOrgIds = ClaimValues{"123", "345"}
 
 	// Create the router but leave out the handler setting the context token.
 	route := mux.NewRouter().PathPrefix("/agent-clusters/{id}").Subrouter()
@@ -192,7 +192,7 @@ func TestUseFleetShardAuthorizationMiddleware_DataPlaneOIDCIssuers(t *testing.T)
 					DataPlaneOIDCIssuers: &iam.OIDCIssuers{URIs: []string{validIssuer}},
 				},
 				&FleetShardAuthZConfig{
-					AllowedSubject: "fleetshard-sync",
+					AllowedSubjects: []string{"fleetshard-sync"},
 				})
 
 			req := httptest.NewRequest("GET", "http://example.com/agent-clusters/1234", nil)

pkg/auth/fleetshard_authz_test.go

@@ -0,0 +1,20 @@
+package auth
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestFleetShardAuthZConfig_ReadFiles(t *testing.T) {
+	RegisterTestingT(t)
+	c := &FleetShardAuthZConfig{
+		Enabled: true,
+		File:    "pkg/auth/testdata/fleetshard-authz.yaml",
+	}
+	err := c.ReadFiles()
+	Expect(err).ToNot(HaveOccurred())
+	Expect(c.AllowedOrgIDs).To(Equal(ClaimValues{"12345678"}))
+	Expect(c.AllowedSubjects).To(Equal(ClaimValues{"test-sub"}))
+	Expect(c.AllowedAudiences).To(Equal(ClaimValues{"test-aud"}))
+}

pkg/auth/testdata/fleetshard-authz.yaml

@@ -0,0 +1,7 @@
+---
+allowed_org_ids:
+  - "12345678"
+allowed_subjects:
+  - "test-sub"
+allowed_audiences:
+  - "test-aud"

templates/service-template.yml

@@ -612,18 +612,28 @@ objects:
       annotations:
         qontract.recycle: "true"
     data:
-      fleetshard-authz-org-ids-development.yaml: |-
-        # RH ACS Organization (returned for personal tokens obtained by ocm token).
-        - "11009103"
-        # RHACS Managed Service Org for staging (Entry `RHACS Managed Service Org for AWS Billing` in Bitwarden).
-        - "16064548"
-        # ACS RH SSO Org Prod
-        - "16134752"
-        # ACS RH SSO ORG Development
-        - "16155304"
-      fleetshard-authz-org-ids-prod.yaml: |-
-        # ACS RH SSO Org Prod
-        - "16134752"
+      fleetshard-authz-development.yaml: |-
+        allowed_org_ids:
+          # RH ACS Organization (returned for personal tokens obtained by ocm token).
+          - "11009103"
+          # RHACS Managed Service Org for staging (Entry `RHACS Managed Service Org for AWS Billing` in Bitwarden).
+          - "16064548"
+          # ACS RH SSO Org Prod
+          - "16134752"
+          # ACS RH SSO ORG Development
+          - "16155304"
+        allowed_subjects:
+          - "system:serviceaccount:rhacs:fleetshard-sync"
+        allowed_audience:
+          - "acs-fleet-manager-private-api-development"
+      fleetshard-authz-prod.yaml: |-
+        allowed_org_ids:
+          # ACS RH SSO Org Prod
+          - "16134752"
+        allowed_subjects:
+          - "system:serviceaccount:rhacs:fleetshard-sync"
+        allowed_audiences:
+          - "acs-fleet-manager-private-api-production"
   - kind: ServiceAccount
     apiVersion: v1
     metadata: