Add authorization to the compliance event API endpoints #164

yiraeChristineKim · 2024-01-23T12:24:30Z

I need to ensure that the compliance history can only be written by authorized service accounts and viewed by users with appropriate ACM access.

Definition of Done for Engineering Story Owner (Checklist)

The read API endpoint is restricted based on a user's "get" access to corresponding ManagedCluster objects. This result will filter the requests returned in the SQL queries.

Ref: https://issues.redhat.com/browse/ACM-6866
Signed-off-by: Yi Rae Kim [email protected]

yiraeChristineKim · 2024-01-23T13:08:14Z

controllers/complianceeventsapi/server.go

+		if !isAuth {
+			log.Error(err, "Do not have the Read authorization for "+ce.Cluster.Name)
+
+			continue


Continue or Break?

yiraeChristineKim · 2024-01-23T13:08:46Z

go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/stolostron/go-log-utils v0.1.2
 	github.com/stolostron/go-template-utils/v4 v4.0.1-0.20231212190701-4dc096ec1b40
 	github.com/stolostron/kubernetes-dependency-watches v0.5.2-0.20231212185913-628ab39622b8
+	gopkg.in/square/go-jose.v2 v2.6.0


controllers/complianceeventsapi/server.go

yiraeChristineKim · 2024-01-26T13:46:31Z

controllers/complianceeventsapi/auth.go

+	}
+
+	ssar := &authorizationv1.SubjectAccessReview{
+		Spec: authorizationv1.SubjectAccessReviewSpec{


Is this correct? do we want to check replicated policies in managedcluster namespace?

.github/workflows/kind.yml

controllers/complianceeventsapi/utils.go

controllers/complianceeventsapi/auth.go

JustinKuli

Some early comments I had last week - I will plan to re-review this soon anyway.

controllers/complianceeventsapi/complianceeventsapi_controller.go

JustinKuli · 2024-01-25T20:34:30Z

controllers/complianceeventsapi/server.go

+	}
+
+	if !isAuth {
+		log.Error(err, "Do not have the Read authorization for "+complianceEvent.Cluster.Name)


I don't know if we want to log every time an unauthorized request comes in - and if we do, I think it would not be at the error level since this is a user problem, not an app problem.

Maybe Matt or Patrick knows if there's a guideline we need to follow about logging unauthorized access attempts?

JustinKuli · 2024-01-25T20:35:22Z

controllers/complianceeventsapi/auth.go

+		return true, nil
+	}
+
+	log.Info("The service account does not have GET authorization for " + managedClusterName)


I don't think this log message is necessary here - if there were a lot of unauthorized requests, this could pollute the app logs.

JustinKuli

I think we need to figure out the service account stuff for the tests.

.github/workflows/kind.yml

controllers/complianceeventsapi/utils.go

yiraeChristineKim · 2024-02-01T15:36:54Z

I think we need to figure out the service account stuff for the tests.

SubjectAccessReview needs clusterrole or role so if the user doesn't have SubjectAccessReview GET role. That user will fail. I am not sure which one is better. Also, I should use SelfSubjectRulesReview(github.com/stolostron/rbac-api-utils) to get all roles at once. That needs to impersonate. Otherwise, like using SubjectAccessReview, I have to ping kubernetes API per a line. Do you have a good idea?

yiraeChristineKim · 2024-02-01T18:12:04Z

Hi all, since I opened #167 after 167 merged then I will work on this 164 PR Thanks

JustinKuli · 2024-02-02T21:49:42Z

I think we need to figure out the service account stuff for the tests.

SubjectAccessReview needs clusterrole or role so if the user doesn't have SubjectAccessReview GET role. That user will fail. I am not sure which one is better. Also, I should use SelfSubjectRulesReview(github.com/stolostron/rbac-api-utils) to get all roles at once. That needs to impersonate. Otherwise, like using SubjectAccessReview, I have to ping kubernetes API per a line. Do you have a good idea?

Limiting the number of API calls is a good reason to favor SelfSubjectRulesReview, I wasn't thinking about that.

mprahl · 2024-02-05T15:21:55Z

controllers/complianceeventsapi/auth.go

+		return nil, fmt.Errorf("failed to parse token")
+	}
+	// Token include Bearer prefix
+	config.Impersonate = rest.ImpersonationConfig{


We don't want to use impersonation because then group membership does not get taken into account.

We want to instead use the user token to make an SelfSubjectAccessReview.

For example, see how the Kubernetes client is generated using the input token:
https://github.com/stolostron/rbac-api-utils/blob/159deac7d398c3470ae730c3fbb8ffbbcca1ac5a/pkg/rbac/rbac.go#L91-L104

Then you can call GetResourceAccess with the Kubernetes client.

mprahl · 2024-02-16T17:26:09Z

test/e2e/case18_compliance_api_test.go

+
+	Describe("Test authorization", func() {
+		Describe("Test method Get", func() {
+			It("Should return StatusForbidden when it is empty Authorization", func(ctx context.Context) {


Shouldn't it be unauthorized when no authentication info is provided?

It is empty token.. maybe I should change the title

mprahl · 2024-02-16T17:27:50Z

test/e2e/case18_compliance_api_test.go

+
+	Describe("Test authorization", func() {
+		Describe("Test method Get", func() {
+			It("Should return StatusForbidden when it is empty Authorization", func(ctx context.Context) {


Could you also test the CSV endpoint?

I did line 1802

test/e2e/case18_compliance_api_test.go

mprahl · 2024-02-16T20:49:27Z

controllers/complianceeventsapi/server.go

+	return parsed, nil
+}
+
+// getAuthorizedClusterNames verifies that if a cluster filter is provided,


Suggested change

// getAuthorizedClusterNames verifies that if a cluster filter is provided,

// setAuthorizedClusters verifies that if a cluster filter is provided,

controllers/complianceeventsapi/server.go

mprahl · 2024-02-16T20:50:32Z

controllers/complianceeventsapi/server.go

+			clusterName, err := getClusterNameFromID(ctx, db, id)
+			if err != nil {
+				if errors.Is(err, sql.ErrNoRows) {
+					// This leads to send forbidden error to the user


Suggested change

// This leads to send forbidden error to the user

// Filter out invalid cluster IDs from the query

mprahl · 2024-02-16T20:52:41Z

controllers/complianceeventsapi/server.go

+			if err != nil {
+				if errors.Is(err, sql.ErrNoRows) {
+					// This leads to send forbidden error to the user
+					parsed.Filters["clusters.cluster_id"] = append(parsed.Filters["clusters.cluster_id"][:index],


This wouldn't work if there was more than one invalid cluster ID since index no longer matches. My suggestion is traverse clusterIDs in reverse so that removing any cluster ID does not impact the current index.

isn't better we reset parsed.Filters["clusters.cluster_id"] then append? more readable

mprahl · 2024-02-16T20:53:56Z

controllers/complianceeventsapi/server.go

+	// Deep copy parsed.Filters["clusters.cluster_id"]
+	copy(clusterIDs, tempClusterIDs)
+
+	if okClusterIds {


This if statement is not needed since ranging over an empty/uninitialized slice will just be skipped.

mprahl · 2024-02-16T20:54:03Z

controllers/complianceeventsapi/server.go

+	}
+
+	parsedClusterNames, okClusterNames := parsed.Filters["clusters.name"]
+	if okClusterNames {


This if statement is not needed since ranging over an empty/uninitialized slice will just be skipped.

mprahl · 2024-02-16T20:54:31Z

controllers/complianceeventsapi/server.go

+	}
+
+	// There is no cluster.id or clusterName arg from the url query
+	// In other words, the user requests all


Suggested change

// In other words, the user requests all

// In other words, the user requests all they have access to.

controllers/complianceeventsapi/server.go

Add authorization to the compliance event "GET" API endpoints Add authorization to the compliance event API endpoints Add authorization to the compliance event "GET" API endpoints after "POST" merged Ref: https://issues.redhat.com/browse/ACM-6866 Signed-off-by: Yi Rae Kim <[email protected]>

mprahl · 2024-02-19T17:36:36Z

controllers/complianceeventsapi/server.go

+			userConfig, err := getUserKubeConfig(s.cfg, r)
+			if err != nil {
+				if errors.Is(err, ErrNotAuthorized) {
+					writeErrMsgJSON(w, "The Authorization header is not set", http.StatusForbidden)


Suggested change

writeErrMsgJSON(w, "The Authorization header is not set", http.StatusForbidden)

writeErrMsgJSON(w, "The Authorization header is not set", http.StatusUnauthorized)

mprahl · 2024-02-19T17:36:43Z

controllers/complianceeventsapi/server.go

+		userConfig, err := getUserKubeConfig(s.cfg, r)
+		if err != nil {
+			if errors.Is(err, ErrNotAuthorized) {
+				writeErrMsgJSON(w, "The Authorization header is not set", http.StatusForbidden)


Suggested change

writeErrMsgJSON(w, "The Authorization header is not set", http.StatusForbidden)

writeErrMsgJSON(w, "The Authorization header is not set", http.StatusUnauthorized)

mprahl · 2024-02-19T17:37:47Z

controllers/complianceeventsapi/server.go

+		userConfig, err := getUserKubeConfig(s.cfg, r)
+		if err != nil {
+			if errors.Is(err, ErrNotAuthorized) {
+				writeErrMsgJSON(w, "The Authorization header is not set", http.StatusForbidden)


Suggested change

writeErrMsgJSON(w, "The Authorization header is not set", http.StatusForbidden)

writeErrMsgJSON(w, "The Authorization header is not set", http.StatusUnauthorized)

mprahl · 2024-02-19T17:42:48Z

controllers/complianceeventsapi/server.go

+		}
+	}
+
+	// There is no cluster.id or clusterName arg from the url query


Suggested change

// There is no cluster.id or clusterName arg from the url query

// There is no cluster.cluster_id or cluster.name arg from the url query

mprahl · 2024-02-19T17:44:01Z

controllers/complianceeventsapi/server.go

+	tempClusterIDs := parsed.Filters["clusters.cluster_id"]
+	clusterIDs := make([]string, len(tempClusterIDs))
+
+	// Deep copy parsed.Filters["clusters.cluster_id"]
+	copy(clusterIDs, tempClusterIDs)
+
+	// Reset clusters.cluster_id
+	parsed.Filters["clusters.cluster_id"] = []string{}


I think this would work and be more efficient:

Suggested change

tempClusterIDs := parsed.Filters["clusters.cluster_id"]

clusterIDs := make([]string, len(tempClusterIDs))

// Deep copy parsed.Filters["clusters.cluster_id"]

copy(clusterIDs, tempClusterIDs)

// Reset clusters.cluster_id

parsed.Filters["clusters.cluster_id"] = []string{}

clusterIDs := parsed.Filters["clusters.cluster_id"]

// Reset clusters.cluster_id

parsed.Filters["clusters.cluster_id"] = []string{}

mprahl · 2024-02-19T17:45:41Z

controllers/complianceeventsapi/server.go

+		starRules, ok := allRules["*"]
+		if ok && slices.Contains(starRules, "get") || slices.Contains(starRules, "*") {
+			return parsed, nil
+		}


The beginning of the function already checks this.

Suggested change

starRules, ok := allRules["*"]

if ok && slices.Contains(starRules, "get") || slices.Contains(starRules, "*") {

return parsed, nil

}

mprahl · 2024-02-19T17:47:31Z

controllers/complianceeventsapi/server.go

+
+	if len(unAuthorizedClusters) > 0 {
+		return parsed, fmt.Errorf("%w: the following cluster filters are not authorized: %s",
+			ErrNotAuthorized, strings.Join(unAuthorizedClusters, ", "))


I think we should have a new error called ErrForbidden to match the HTTP status codes we return. It's getting a bit confusing to use this for both cases.

mprahl

All my remaining comments are minor. I'll address them in a separate PR.

openshift-ci · 2024-02-19T17:55:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl, yiraeChristineKim

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~~ [mprahl,yiraeChristineKim]

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

Will fix in follow up PR

mprahl · 2024-02-19T18:40:09Z

controllers/complianceeventsapi/server.go

+		if err != nil {
+			if errors.Is(err, sql.ErrNoRows) {
+				// Filter out invalid cluster IDs from the query
+				continue


After thinking about this, I think this allows a user to bypass all authorization checks and have everything returned if they provide an invalid cluster ID.

I'll fix this in an upcoming PR.

openshift-ci bot requested review from dhaiducek and JustinKuli January 23, 2024 12:24

openshift-ci bot added dco-signoff: yes approved labels Jan 23, 2024

yiraeChristineKim force-pushed the ACM-6866 branch 2 times, most recently from 315fb32 to 968ee63 Compare January 23, 2024 12:35

yiraeChristineKim requested a review from mprahl January 23, 2024 13:06

yiraeChristineKim commented Jan 23, 2024

View reviewed changes

yiraeChristineKim force-pushed the ACM-6866 branch 2 times, most recently from 3e70f0f to 03309b1 Compare January 23, 2024 13:19

mprahl reviewed Jan 23, 2024

View reviewed changes

controllers/complianceeventsapi/server.go Outdated Show resolved Hide resolved

mprahl reviewed Jan 23, 2024

View reviewed changes

controllers/complianceeventsapi/server.go Outdated Show resolved Hide resolved

mprahl reviewed Jan 23, 2024

View reviewed changes

controllers/complianceeventsapi/server.go Outdated Show resolved Hide resolved

yiraeChristineKim commented Jan 26, 2024

View reviewed changes

.github/workflows/kind.yml Outdated Show resolved Hide resolved

yiraeChristineKim commented Jan 26, 2024

View reviewed changes

controllers/complianceeventsapi/utils.go Outdated Show resolved Hide resolved

yiraeChristineKim force-pushed the ACM-6866 branch from ef6b25c to 510f84d Compare January 26, 2024 14:18

yiraeChristineKim commented Jan 26, 2024

View reviewed changes

controllers/complianceeventsapi/auth.go Outdated Show resolved Hide resolved

yiraeChristineKim requested a review from mprahl January 26, 2024 15:03

JustinKuli reviewed Jan 30, 2024

View reviewed changes

JustinKuli previously requested changes Feb 1, 2024

View reviewed changes

.github/workflows/kind.yml Outdated Show resolved Hide resolved

.github/workflows/kind.yml Outdated Show resolved Hide resolved

controllers/complianceeventsapi/utils.go Outdated Show resolved Hide resolved

openshift-ci bot assigned JustinKuli Feb 1, 2024

yiraeChristineKim force-pushed the ACM-6866 branch 3 times, most recently from b55fa91 to 9e442f6 Compare February 2, 2024 15:36

mprahl reviewed Feb 5, 2024

View reviewed changes

mprahl reviewed Feb 16, 2024

View reviewed changes

test/e2e/case18_compliance_api_test.go Show resolved Hide resolved

yiraeChristineKim force-pushed the ACM-6866 branch 2 times, most recently from 450f9e6 to 2ee8a7e Compare February 16, 2024 19:31

mprahl reviewed Feb 16, 2024

View reviewed changes

controllers/complianceeventsapi/server.go Show resolved Hide resolved

mprahl reviewed Feb 16, 2024

View reviewed changes

controllers/complianceeventsapi/server.go Show resolved Hide resolved

yiraeChristineKim force-pushed the ACM-6866 branch from 2ee8a7e to 02a0023 Compare February 19, 2024 14:12

mprahl reviewed Feb 19, 2024

View reviewed changes

mprahl approved these changes Feb 19, 2024

View reviewed changes

openshift-ci bot added the lgtm label Feb 19, 2024

mprahl merged commit 2cc5ff6 into open-cluster-management-io:main Feb 19, 2024

magic-mirror-bot bot mentioned this pull request Feb 19, 2024

😿 Failed to sync the upstream PRs: #164 stolostron/governance-policy-propagator#578

Closed

mprahl reviewed Feb 19, 2024

View reviewed changes

yiraeChristineKim deleted the ACM-6866 branch February 20, 2024 21:30

	// getAuthorizedClusterNames verifies that if a cluster filter is provided,
	// setAuthorizedClusters verifies that if a cluster filter is provided,

	// This leads to send forbidden error to the user
	// Filter out invalid cluster IDs from the query

	// In other words, the user requests all
	// In other words, the user requests all they have access to.

	writeErrMsgJSON(w, "The Authorization header is not set", http.StatusForbidden)
	writeErrMsgJSON(w, "The Authorization header is not set", http.StatusUnauthorized)

	// There is no cluster.id or clusterName arg from the url query
	// There is no cluster.cluster_id or cluster.name arg from the url query

Add authorization to the compliance event API endpoints #164

Add authorization to the compliance event API endpoints #164

Uh oh!

Conversation

yiraeChristineKim commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yiraeChristineKim commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yiraeChristineKim commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JustinKuli commented Feb 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiraeChristineKim Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yiraeChristineKim commented Jan 23, 2024 •

edited

Loading

yiraeChristineKim commented Feb 1, 2024 •

edited

Loading

yiraeChristineKim commented Feb 1, 2024 •

edited

Loading

yiraeChristineKim Feb 19, 2024 •

edited

Loading