ROX-23709: Trust Data Plane OAuth issuers in fleetshard authorization middleware #1801

kovayur · 2024-05-08T18:21:06Z

Description

This change adds Data Plane OAuth issuers validation in fleetshard authorization middleware.
The old authorization remains for backward compatibility

Checklist (Definition of Done)

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup
make verify lint binary test test/integration

openshift-ci · 2024-05-08T18:21:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

kovayur · 2024-05-10T19:17:39Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kovayur and the rest of your teammates on Graphite

dhaus67 · 2024-05-13T11:32:12Z

pkg/auth/fleetshard_authz.go

@@ -40,6 +42,8 @@ func (c *FleetShardAuthZConfig) AddFlags(fs *pflag.FlagSet) {
 		"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")


nit: maybe to make it more clear to folks reading through things:

Suggested change

"Fleetshard authZ middleware allowed subject")

"Fleetshard authZ middleware allowed subject claim of the token")

Obsolete, moved to the config file

dhaus67 · 2024-05-13T11:33:30Z

pkg/auth/fleetshard_authz.go

@@ -24,13 +24,15 @@ type FleetShardAuthZConfig struct {
 	Enabled           bool
 	AllowedOrgIDs     AllowedOrgIDs
 	AllowedOrgIDsFile string
+	AllowedSubject    string


Seeing as the effort will be relatively low, might make sense to allow setting multiple subjects as allowed ones, similar to the previous org IDs? While currently not eminent, it's not a lot of work to add support for that and will make our life easier in the future.

dhaus67 · 2024-05-13T11:36:09Z

pkg/auth/fleetshard_authz_middleware.go

+				return
+			}
+
+			glog.Infof("fleetshard subject is not allowed (expected=%s, actual=%s)", expected, s)


In case the subject is empty or malformed:

Suggested change

glog.Infof("fleetshard subject is not allowed (expected=%s, actual=%s)", expected, s)

glog.Infof("fleetshard subject is not allowed (expected=%s, actual=%q)", expected, s)

Obsolete, added the generic claims check

dhaus67 · 2024-05-13T11:37:19Z

pkg/auth/fleetshard_authz_middleware.go

@@ -47,3 +69,29 @@ func checkAllowedOrgIDs(allowedOrgIDs AllowedOrgIDs) mux.MiddlewareFunc {
 		})
 	}
 }
+
+func checkSubject(expected string) mux.MiddlewareFunc {


nit:

Suggested change

func checkSubject(expected string) mux.MiddlewareFunc {

func checkSubject(allowedSubject string) mux.MiddlewareFunc {

Obsolete, added the generic claims check

dhaus67 · 2024-05-13T11:48:18Z

One thing in addition that would work well besides verifying the subject would also be verifying the audience. We can set the audience to a custom one in the projected volume mount (and also should, so that the token cannot be used to access the API server) and additionally verify that here.

kovayur · 2024-05-14T09:08:03Z

One thing in addition that would work well besides verifying the subject would also be verifying the audience. We can set the audience to a custom one in the projected volume mount (and also should, so that the token cannot be used to access the API server) and additionally verify that here.

Done

dhaus67 · 2024-05-21T01:01:38Z

pkg/auth/acs_claims.go

@@ -16,6 +16,11 @@ func (c *ACSClaims) VerifyIssuer(cmp string, req bool) bool {
 	return jwt.MapClaims(*c).VerifyIssuer(cmp, req)
 }

+// VerifyAudience wraps jwt.VerifyAudience


super nit: while we're at it, might make sense to give proper comments to all the functions here..

dhaus67 · 2024-05-21T01:02:45Z

pkg/auth/acs_claims.go

@@ -16,6 +16,11 @@ func (c *ACSClaims) VerifyIssuer(cmp string, req bool) bool {
 	return jwt.MapClaims(*c).VerifyIssuer(cmp, req)
 }

+// VerifyAudience wraps jwt.VerifyAudience
+func (c *ACSClaims) VerifyAudience(cmp string, req bool) bool {


Looking at this, does it even make sense to give clients the option to set required to false? I don't think we should IMO, but since this is more or less pre-existing, it's fine to leave as is and address in a follow-up, but I feel like we should introduce some sane defaults here to avoid people unsetting the required by mistake.

Agree, removed the required option

dhaus67 · 2024-05-21T01:04:50Z

pkg/auth/fleetshard_authz_middleware.go

+					audienceAccepted = true
+					break


instead of breaking you could just call next.ServeHTTP here. Then you don't have the additional indentation at the bottom with !audienceAccepted.

Done, thanks

dhaus67 · 2024-05-21T01:06:32Z

pkg/auth/acs_claims.go

@@ -79,6 +84,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() (interface{}, error) {


Is it going to be a string or string array? Either way, I think it would be nice to have a concrete type here instead of the interface.

It can be either string or array

I added logic to cast it to []string and a few unit tests

dhaus67 · 2024-05-21T01:07:11Z

pkg/auth/fleetshard_authz_middleware.go

+
+			if !audienceAccepted {
+				audience, _ := claims.GetAudience()
+				glog.Infof("none of the audiences (%q) in the access token is not in the list of allowed values [%s]",


Since audience is of type interface, what will %q actually print here? Better to return a concrete type in GetAudience().

Changed to strings.join since claims.getAudience now returns []string. See the comment above.

dhaus67 · 2024-05-22T01:41:21Z

pkg/auth/acs_claims.go

-// IsOrgAdmin ...
+// GetAudience returns the audience claim of the token. It identifies the token consumer.
+func (c *ACSClaims) GetAudience() ([]string, error) {
+	aud := make([]string, 0)


super nit: probably preferences, but might as well do var aud []string 🤷

This was done on purpose in order to get consistent results in the following test cases:

should return empty slice if the claim is empty array

should return empty slice if the claim is empty interface

If I change this line to var aud []string, GetAudience will return an empty and nil slice respectively.

dhaus67 · 2024-05-22T01:42:10Z

pkg/auth/acs_claims.go

+		for _, a := range v {
+			vs, ok := a.(string)
+			if !ok {
+				return nil, fmt.Errorf("can't parse part of the audience claim: %q", a)


I'd rather fail open and create a log message that parts of the audience couldn't be parsed but not fail closed like we do here.

… middleware

openshift-ci · 2024-05-22T10:14:40Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2024-05-22T10:14:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaus67, kovayur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dhaus67,kovayur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kovayur requested a review from dhaus67 May 8, 2024 18:21

openshift-ci bot added the do-not-merge/work-in-progress label May 8, 2024

kovayur temporarily deployed to development May 8, 2024 18:21 — with GitHub Actions Inactive

openshift-ci bot added the approved label May 8, 2024

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 7c3d605 to cdec282 Compare May 10, 2024 19:17

kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from 0064307 to fd7a041 Compare May 10, 2024 19:17

kovayur temporarily deployed to development May 10, 2024 19:17 — with GitHub Actions Inactive

This was referenced May 10, 2024

ROX-23709: Refactor authentication handler #1789

Merged

ROX-23709: Load token from file in fleetshard-sync #1802

Merged

dhaus67 reviewed May 13, 2024

View reviewed changes

kovayur temporarily deployed to development May 13, 2024 18:44 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 14, 2024 08:53 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 14, 2024 08:59 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 14, 2024 09:05 — with GitHub Actions Inactive

kovayur marked this pull request as ready for review May 14, 2024 09:08

openshift-ci bot removed the do-not-merge/work-in-progress label May 14, 2024

kovayur temporarily deployed to development May 14, 2024 09:08 — with GitHub Actions Inactive

kovayur requested a review from dhaus67 May 14, 2024 09:08

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from cdec282 to 4834ca2 Compare May 15, 2024 11:39

kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from d6f9367 to 2752423 Compare May 15, 2024 11:40

kovayur temporarily deployed to development May 15, 2024 11:40 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 15, 2024 11:54 — with GitHub Actions Inactive

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 4834ca2 to 9353319 Compare May 15, 2024 11:57

kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from a7a9c4c to 44ede5a Compare May 15, 2024 11:59

kovayur temporarily deployed to development May 15, 2024 11:59 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 16, 2024 12:48 — with GitHub Actions Inactive

dhaus67 reviewed May 21, 2024

View reviewed changes

kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from bbf4f42 to a40721e Compare May 21, 2024 12:48

openshift-merge-robot added the needs-rebase label May 21, 2024

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 26b84f6 to d0507ef Compare May 21, 2024 12:48

openshift-merge-robot removed the needs-rebase label May 21, 2024

kovayur temporarily deployed to development May 21, 2024 15:58 — with GitHub Actions Inactive

kovayur requested a review from dhaus67 May 21, 2024 16:02

dhaus67 approved these changes May 22, 2024

View reviewed changes

openshift-ci bot assigned dhaus67 May 22, 2024

openshift-ci bot added the lgtm label May 22, 2024

Base automatically changed from yury/ROX-23709-fleetshard-auth to main May 22, 2024 08:15

openshift-merge-robot added the needs-rebase label May 22, 2024

kovayur added 5 commits May 22, 2024 11:50

ROX-23709: Trust Data Plane OAuth issuers in fleetshard authorization…

e321c92

… middleware

Support the audience as list and add more test cases

2b426da

Add the type casting for audience

9ca11d1

Address review feedback

d13e393

Fail open on parsing the audience claim

c5a1af0

kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from 14eb2cd to c5a1af0 Compare May 22, 2024 10:14

openshift-ci bot removed the lgtm label May 22, 2024

kovayur had a problem deploying to development May 22, 2024 10:14 — with GitHub Actions Failure

openshift-merge-robot removed the needs-rebase label May 22, 2024

kovayur temporarily deployed to development May 22, 2024 11:00 — with GitHub Actions Inactive

Fix service template

f48b741

kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from 6417b2b to f48b741 Compare May 22, 2024 15:17

kovayur temporarily deployed to development May 22, 2024 15:17 — with GitHub Actions Inactive

kovayur merged commit 280129a into main May 22, 2024
7 checks passed

kovayur deleted the yury/ROX-23709-fleetshard-authz branch May 22, 2024 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-23709: Trust Data Plane OAuth issuers in fleetshard authorization middleware #1801

ROX-23709: Trust Data Plane OAuth issuers in fleetshard authorization middleware #1801

kovayur commented May 8, 2024

openshift-ci bot commented May 8, 2024

kovayur commented May 10, 2024 •

edited

Loading

dhaus67 May 13, 2024

kovayur May 14, 2024

dhaus67 May 13, 2024

kovayur May 14, 2024

dhaus67 May 13, 2024

kovayur May 14, 2024

dhaus67 May 13, 2024

kovayur May 14, 2024

dhaus67 commented May 13, 2024

kovayur commented May 14, 2024

dhaus67 May 21, 2024

kovayur May 21, 2024

dhaus67 May 21, 2024

kovayur May 21, 2024

dhaus67 May 21, 2024

kovayur May 21, 2024

dhaus67 May 21, 2024

kovayur May 21, 2024

kovayur May 21, 2024 •

edited

Loading

dhaus67 May 21, 2024

kovayur May 21, 2024

dhaus67 May 22, 2024

kovayur May 22, 2024

dhaus67 May 22, 2024

kovayur May 22, 2024

openshift-ci bot commented May 22, 2024

openshift-ci bot commented May 22, 2024

	"Fleetshard authZ middleware allowed subject")
	"Fleetshard authZ middleware allowed subject claim of the token")

	glog.Infof("fleetshard subject is not allowed (expected=%s, actual=%s)", expected, s)
	glog.Infof("fleetshard subject is not allowed (expected=%s, actual=%q)", expected, s)

	func checkSubject(expected string) mux.MiddlewareFunc {
	func checkSubject(allowedSubject string) mux.MiddlewareFunc {

ROX-23709: Trust Data Plane OAuth issuers in fleetshard authorization middleware #1801

ROX-23709: Trust Data Plane OAuth issuers in fleetshard authorization middleware #1801

Conversation

kovayur commented May 8, 2024

Description

Checklist (Definition of Done)

Test manual

openshift-ci bot commented May 8, 2024

kovayur commented May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaus67 commented May 13, 2024

kovayur commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kovayur May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented May 22, 2024

openshift-ci bot commented May 22, 2024

kovayur commented May 10, 2024 •

edited

Loading

kovayur May 21, 2024 •

edited

Loading