Add certificate and token auth and authz on the POST API endpoint #177

mprahl · 2024-02-12T21:57:10Z

If a certificate is provided, the HTTP server will run over HTTPS which enables certificate authentication.

The authorization requires a user to have patch access to the policy status on the managed cluster namespace of the managed cluster that the compliance event is for.

Relates:
https://issues.redhat.com/browse/ACM-6866

yiraeChristineKim · 2024-02-13T14:22:36Z

Makefile

+	@echo "Copying the compliance API certificates locally"
+	kubectl -n $(KIND_NAMESPACE) get secret compliance-api-cert -o json | jq -r '.data["tls.crt"]' | base64 -d > dev-tls.crt
+	kubectl -n $(KIND_NAMESPACE) get secret compliance-api-cert -o json | jq -r '.data["ca.crt"]' | base64 -d >> dev-ca.crt
+	kubectl -n $(KIND_NAMESPACE) get secret compliance-api-cert -o json | jq -r '.data["tls.key"]' | base64 -d > dev-tls.key


Where this secret is created?

It's defined in build/kind/postgres.yaml using CertManager.

yiraeChristineKim · 2024-02-13T14:35:29Z

controllers/complianceeventsapi/auth.go

+	if !allowed {
+		var username string
+
+		userInfo, err := getTokenUserInfo(req.Context(), authenticatedClient, token)


This is just for log.. do we really need?

I changed it to try parsing the token locally as a best effort. It's important to log unauthorized access attempts for auditing.

yiraeChristineKim · 2024-02-13T14:42:33Z

controllers/complianceeventsapi/server.go

+	allowed, err := canRecordComplianceEvent(cfg, authenticatedClient, authenticator, reqEvent.Cluster.Name, r)
+	if err != nil {
+		if errors.Is(err, ErrNotAuthorized) {
+			writeErrMsgJSON(w, "Unauthorized", http.StatusUnauthorized)


This error is propagator SA doesn't have "CREATE" for SubjectAccessReviews. I think it is StatusInternalServerError.

There's a problem with 401 Unauthorized, the HTTP status code for authentication errors. And that’s just it: it’s for authentication, not authorization. Receiving a 401 response is the server telling you, “you aren’t authenticated–either not authenticated at all or authenticated incorrectly–but please reauthenticate and try again.” To help you out, it will always include a WWW-Authenticate header that describes how to authenticate.

This is what I found. Here isn't about authentication.

I think it's right. If the user provided credential (token or cert) does not allow us to get to the subject access review call, then it's unauthorized (bad authentication provided). If the subject access review says the user is not allowed then it should be a forbidden status code.

subjectaccess use propagator's SA, subjectaccess throw err ErrNotAuthorized mean propagator SA don't have GET access

Also you already wrote //+kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create This won't fall into here

gparvin · 2024-02-13T17:13:37Z

controllers/complianceeventsapi/auth.go

+	}
+
+	// req.TLS.PeerCertificates will be empty if certificate authentication is not enabled (e.g. endpoint is not HTTPS)
+	if req.TLS != nil && len(req.TLS.PeerCertificates) > 0 {


If not HTTPS will an error be returned?

If not HTTPS then req.TLS will be nil so then only token based authentication is supported.

gparvin · 2024-02-13T17:17:45Z

controllers/complianceeventsapi/auth.go

+		return false, ErrNotAuthorized
+	}
+
+	userClient, err := getClientFromToken(cfg, token)


Is the previous HTTPS check for certificate auth and this is handling token auth -- but both are HTTPS connections?

Right... certificate auth is only checked if the connection is HTTPS and a certificate was provided for authentication.

If the connection is HTTP or is HTTPS + no certificate authentication was provided, then we fall back to token auth.

Does anything need the HTTP connection? Can we limit it to HTTPS only?

I think it's good to have an HTTP option in case you want a proxy to handle the TLS. In ACM, we never will though.

I agree to both http and https. We have CSV endpoint too. I expect individual users would use CSV endpoint using Curl

If a certificate is provided, the HTTP server will run over HTTPS which enables certificate authentication. The authorization requires a user to have patch access to the policy status on the managed cluster namespace of the managed cluster that the compliance event is for. Relates: https://issues.redhat.com/browse/ACM-6866 Signed-off-by: mprahl <[email protected]>

gparvin · 2024-02-13T17:43:35Z

main.go

+	pflag.StringVar(
+		&complianceAPICert, "compliance-history-api-cert", "",
+		"The path to the certificate the compliance history API will use for HTTPS (CA cert, if any, concatenated "+
+			"after server cert). If not set, HTTP will be used.",
+	)
+	pflag.StringVar(
+		&complianceAPIKey, "compliance-history-api-key", "",
+		"The path to the private key the compliance history API will use for HTTPS. If not set, HTTP will be used.",


Can we make these required? Basically requiring HTTPS

yiraeChristineKim · 2024-02-13T19:01:32Z

controllers/complianceeventsapi/auth.go

+	return strings.TrimSpace(strings.TrimPrefix(req.Header.Get("Authorization"), "Bearer"))
+}
+
+// getClientFromToken will generate a Kubernetes client using the input config and token. No authentication data from


Should we merge this PR first then merge mine? I want to reuse this funcs

yiraeChristineKim · 2024-02-13T19:09:01Z

controllers/complianceeventsapi/auth.go

+}
+
+// getTokenUsername will parse the token and return the username. If the token is invalid, an empty string is returned.
+func getTokenUsername(token string) string {


Oh?! This was changed?! it was token obj create?. am I dreaming?

openshift-ci · 2024-02-13T19:56:38Z

[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

mprahl requested review from gparvin, JustinKuli and yiraeChristineKim February 12, 2024 21:57

openshift-ci bot added dco-signoff: yes approved labels Feb 12, 2024

yiraeChristineKim reviewed Feb 13, 2024

View reviewed changes

mprahl requested a review from yiraeChristineKim February 13, 2024 15:34

gparvin reviewed Feb 13, 2024

View reviewed changes

yiraeChristineKim reviewed Feb 13, 2024

View reviewed changes

mprahl requested review from yiraeChristineKim and gparvin February 13, 2024 19:55

yiraeChristineKim approved these changes Feb 13, 2024

View reviewed changes

openshift-ci bot assigned yiraeChristineKim Feb 13, 2024

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

openshift-merge-bot bot merged commit 857a468 into open-cluster-management-io:main Feb 13, 2024

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

🤖 Sync from open-cluster-management-io/governance-policy-propagator: #177 stolostron/governance-policy-propagator#563

Merged

dhaiducek mentioned this pull request Feb 14, 2024

Sync common Makefile stolostron/governance-policy-framework#721

Merged

Add certificate and token auth and authz on the POST API endpoint #177

Add certificate and token auth and authz on the POST API endpoint #177

Uh oh!

Conversation

mprahl commented Feb 12, 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!

yiraeChristineKim Feb 13, 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!

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!

mprahl Feb 13, 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!

Choose a reason for hiding this comment

Uh oh!

yiraeChristineKim Feb 13, 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!

openshift-ci bot commented Feb 13, 2024

Uh oh!

Uh oh!

yiraeChristineKim Feb 13, 2024 •

edited

Loading

mprahl Feb 13, 2024 •

edited

Loading

yiraeChristineKim Feb 13, 2024 •

edited

Loading