From 4af3ac5526daa561bb29dcfc0b7da0a81a9ba56a Mon Sep 17 00:00:00 2001 From: Yury Kovalev Date: Mon, 13 May 2024 20:15:34 +0200 Subject: [PATCH] Address review comments --- .secrets.baseline | 4 +- config/fleetshard-authz-development.yaml | 15 +++++++ .../fleetshard-authz-org-ids-development.yaml | 10 ----- config/fleetshard-authz-org-ids-prod.yaml | 4 -- config/fleetshard-authz-prod.yaml | 9 ++++ pkg/auth/acs_claims.go | 9 ++++ pkg/auth/context_config.go | 2 + pkg/auth/fleetshard_authz.go | 36 +++++++-------- pkg/auth/fleetshard_authz_middleware.go | 45 +++++++------------ pkg/auth/fleetshard_authz_middleware_test.go | 16 +++---- pkg/auth/fleetshard_authz_test.go | 20 +++++++++ pkg/auth/testdata/fleetshard-authz.yaml | 7 +++ templates/service-template.yml | 34 +++++++++----- 13 files changed, 128 insertions(+), 83 deletions(-) create mode 100644 config/fleetshard-authz-development.yaml delete mode 100644 config/fleetshard-authz-org-ids-development.yaml delete mode 100644 config/fleetshard-authz-org-ids-prod.yaml create mode 100644 config/fleetshard-authz-prod.yaml create mode 100644 pkg/auth/fleetshard_authz_test.go create mode 100644 pkg/auth/testdata/fleetshard-authz.yaml diff --git a/.secrets.baseline b/.secrets.baseline index 56dbb7f872..4236528b2b 100644 --- a/.secrets.baseline +++ b/.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" } diff --git a/config/fleetshard-authz-development.yaml b/config/fleetshard-authz-development.yaml new file mode 100644 index 0000000000..51a34805bf --- /dev/null +++ b/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" diff --git a/config/fleetshard-authz-org-ids-development.yaml b/config/fleetshard-authz-org-ids-development.yaml deleted file mode 100644 index 4ad3f09615..0000000000 --- a/config/fleetshard-authz-org-ids-development.yaml +++ /dev/null @@ -1,10 +0,0 @@ ---- -# A list of org_id's that allow access to the internal API of the fleet manager. -# 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" diff --git a/config/fleetshard-authz-org-ids-prod.yaml b/config/fleetshard-authz-org-ids-prod.yaml deleted file mode 100644 index a55c50f987..0000000000 --- a/config/fleetshard-authz-org-ids-prod.yaml +++ /dev/null @@ -1,4 +0,0 @@ ---- -# A list of org_id's that allow access to the internal API of the fleet manager. -# ACS RH SSO Org Prod -- "16134752" diff --git a/config/fleetshard-authz-prod.yaml b/config/fleetshard-authz-prod.yaml new file mode 100644 index 0000000000..380260a726 --- /dev/null +++ b/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" diff --git a/pkg/auth/acs_claims.go b/pkg/auth/acs_claims.go index f9e38e385d..a708ab11b2 100644 --- a/pkg/auth/acs_claims.go +++ b/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) diff --git a/pkg/auth/context_config.go b/pkg/auth/context_config.go index 7d48e7e64e..ed5037a201 100644 --- a/pkg/auth/context_config.go +++ b/pkg/auth/context_config.go @@ -24,6 +24,8 @@ var ( // Sub claim in old format. alternateSubClaim = "deprecated_sub" + + audienceClaim = "aud" ) // ContextConfig ... diff --git a/pkg/auth/fleetshard_authz.go b/pkg/auth/fleetshard_authz.go index 76961a775c..050c204948 100644 --- a/pkg/auth/fleetshard_authz.go +++ b/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) } diff --git a/pkg/auth/fleetshard_authz_middleware.go b/pkg/auth/fleetshard_authz_middleware.go index 2da0448169..77e7a2b043 100644 --- a/pkg/auth/fleetshard_authz_middleware.go +++ b/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/. - 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("")) }) diff --git a/pkg/auth/fleetshard_authz_middleware_test.go b/pkg/auth/fleetshard_authz_middleware_test.go index ed8a357ce7..dbc36f2f7b 100644 --- a/pkg/auth/fleetshard_authz_middleware_test.go +++ b/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) diff --git a/pkg/auth/fleetshard_authz_test.go b/pkg/auth/fleetshard_authz_test.go new file mode 100644 index 0000000000..484ed84838 --- /dev/null +++ b/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"})) +} diff --git a/pkg/auth/testdata/fleetshard-authz.yaml b/pkg/auth/testdata/fleetshard-authz.yaml new file mode 100644 index 0000000000..60d0829cda --- /dev/null +++ b/pkg/auth/testdata/fleetshard-authz.yaml @@ -0,0 +1,7 @@ +--- +allowed_org_ids: + - "12345678" +allowed_subjects: + - "test-sub" +allowed_audiences: + - "test-aud" diff --git a/templates/service-template.yml b/templates/service-template.yml index ead953f640..8979065835 100644 --- a/templates/service-template.yml +++ b/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: