ROX-23709: Refactor authentication handler #1789

kovayur · 2024-05-06T16:44:17Z

Description

This change splits the authentication handler into 3 parts, each dedicated to a different API: private, public, admin.
The goal is to limit the number of trusted issuers for a particular API.

Every part has its own issuerURIs which allows to limit the issuers for every API:

private handler represented by the new DataplaneOIDCIssuers configuration parameter;
admin handler is configured with the InternalSSORealm issuer uri.
default handler serves the rest (excl. InternalSSORealm)

JwksURL and JwksFile were moved to from ServerConfig to IAMConfig to avoid circular dependency between packages.

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-06T16:44:21Z

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

dhaus67 · 2024-05-07T06:09:41Z

config/dataplane-oidc-issuers.yaml

@@ -0,0 +1,4 @@
+---
+# A list of DataPlane OpenID Connect issuers that should be verified for issued tokens.
+# Endpoints must be given in the format https://<endpoint>/auth/realms/<your-realm> without trailing slash.


That format is explicit for keycloak, but not for other issuers. Just mention that you should provide the issuer as specificed in the tokens you are attempting to verify would be better IMO.

You are right. Fixed.

dhaus67 · 2024-05-07T06:11:57Z

pkg/client/iam/config.go

 }

-// AdditionalSSOIssuers ...
-type AdditionalSSOIssuers struct {
+// OIDCIssuers ...


Let's give this a proper comment.

dhaus67 · 2024-05-07T06:12:53Z

pkg/client/iam/config.go

@@ -190,7 +215,58 @@ func (a *AdditionalSSOIssuers) resolveURIs() error {
 	return nil
 }

-func getOpenIDConfiguration(c http.Client, baseURL string) (*openIDConfiguration, error) {
+func autoSenseHTTPClient(url string) (*http.Client, error) {


Let's just call it createHTTPClient? Seems like an easier read.

dhaus67 · 2024-05-07T06:13:51Z

pkg/client/iam/config.go

+	}
+	// default client
+	return &http.Client{
+		Timeout: time.Minute,


Do we have any proxy settings we have to respect for fleet manager?

I'm not aware of any proxy settings.

dhaus67 · 2024-05-07T06:14:42Z

pkg/client/iam/config.go

-func getOpenIDConfiguration(c http.Client, baseURL string) (*openIDConfiguration, error) {
+func autoSenseHTTPClient(url string) (*http.Client, error) {
+	// Fleet Manager runs on the Data Plane cluster
+	if url == kubernetesIssuer {


Is this a special case in local deployment/testing? Would be great to mention this explicitly, same applies for the isLocalCluster case.

Right, added clarifying comments.

dhaus67 · 2024-05-07T06:15:42Z

pkg/client/iam/config.go

+		if err != nil {
+			return err
+		}


What will happen if resolveURIs / ReadFiles will err out - will we just log this or the process will exit? Just want to avoid being in a state where we only have a partial read of issuers.

It should return error and the process should exit

dhaus67 · 2024-05-07T06:17:28Z

pkg/client/iam/config.go

+func isLocalHost(host string) bool {
+	if ip := net.ParseIP(host); ip != nil {
+		return ip.IsLoopback()
+	}
+	return strings.ToLower(host) == "localhost"
+}


nit: we can use the stackrox netutil lib for that.

Fixed. TBH I was "inspired" by it 😜

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

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-14T22:24:09Z

dev/config/dataplane-oidc-issuers.yaml

@@ -0,0 +1 @@
+- https://127.0.0.1:6443


Maybe as a sane default for dev clusters adding https://kubernetes.default.svc would make sense as well - at the very least that's the issuer in "my" dev k8s cluster.

The intention was to run Fleet Manager locally (not in container)

dhaus67 · 2024-05-14T22:26:44Z

internal/dinosaur/pkg/handlers/authentication.go

+
+	privateAPIHandlerBuilder := authentication.NewHandler().
+		Logger(authnLogger).
+		KeysURL(IAMConfig.RedhatSSORealm.JwksEndpointURI).


Eventually we'd remove this one, as we don't expect sso tokens to be valid anymore for the private API? Any plans on that? Would make sense to leave a marker here just in case.

Yes, I added an item to the checklist in the ticket.

dhaus67 · 2024-05-15T12:30:05Z

internal/dinosaur/pkg/handlers/authentication.go

+)
+
+func (h *compositeAuthenticationHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	if strings.HasPrefix(r.URL.Path, adminAPIPrefix) {


Shouldn't this be routes.AdminAPIPrefix? Same applies below for the private API one.

No, it should be the full path. adminAPIPrefix contains routes.AdminAPIPrefix

dhaus67 · 2024-05-15T13:00:11Z

internal/dinosaur/pkg/handlers/authentication.go

+		KeysURL(IAMConfig.JwksURL).                          // ocm JWK JSON web token signing certificates URL
+		KeysFile(IAMConfig.JwksFile).                        // ocm JWK backup JSON web token signing certificates


You haven't really added this, but: what's the issuer for those? It should be SSO tokens simply, so not sure where it's pointing to?

I see the following reasons for having IAMConfig.JwksURL and IAMConfig.JwksFile

Use the mock authentication server in integration tests;

Add an JWKS for the static token auth.

dhaus67 · 2024-05-15T13:04:05Z

pkg/client/iam/config.go

+		JwksURL:    "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/certs",
+		JwksFile:   "config/jwks-file.json",


Those should just be the RedHatSSORealm's config, right? So why do we set this now here statically, when RedHatSSORealm will resolve to this dynamically.

See the comment above.

openshift-ci · 2024-05-22T01:36:25Z

[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 6, 2024 16:44

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

kovayur temporarily deployed to development May 6, 2024 16:44 — with GitHub Actions Inactive

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

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 6be6984 to 47dc705 Compare May 6, 2024 16:51

kovayur temporarily deployed to development May 6, 2024 16:51 — with GitHub Actions Inactive

dhaus67 reviewed May 7, 2024

View reviewed changes

kovayur temporarily deployed to development May 7, 2024 10:48 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 7, 2024 11:04 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 7, 2024 11:23 — with GitHub Actions Inactive

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 4451c39 to 14e2b6c Compare May 7, 2024 12:20

kovayur temporarily deployed to development May 7, 2024 12:20 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 7, 2024 15:28 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 7, 2024 18:38 — with GitHub Actions Inactive

kovayur marked this pull request as ready for review May 7, 2024 18:56

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

kovayur temporarily deployed to development May 7, 2024 18:56 — with GitHub Actions Inactive

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from b838e27 to 9af3abd Compare May 8, 2024 14:50

kovayur temporarily deployed to development May 8, 2024 14:50 — with GitHub Actions Inactive

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 9af3abd to 62d0537 Compare May 8, 2024 14:51

kovayur temporarily deployed to development May 8, 2024 14:51 — with GitHub Actions Inactive

kovayur temporarily deployed to development May 8, 2024 16:41 — with GitHub Actions Inactive

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 7c3d605 to cdec282 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: Trust Data Plane OAuth issuers in fleetshard authorization middleware #1801

Merged

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

Merged

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

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

kovayur temporarily deployed to development May 15, 2024 11:39 — 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 temporarily deployed to development May 15, 2024 11:57 — with GitHub Actions Inactive

dhaus67 reviewed May 15, 2024

View reviewed changes

kovayur force-pushed the yury/ROX-23709-fleetshard-auth branch from 9353319 to 26b84f6 Compare May 16, 2024 12:47

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

kovayur added 3 commits May 21, 2024 11:54

ROX-23709: Refactor authentication handler

476f6b7

Fix url prefixes

ce9590d

Fix tests

d0507ef

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

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

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

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

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

kovayur deleted the yury/ROX-23709-fleetshard-auth branch May 22, 2024 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-23709: Refactor authentication handler #1789

ROX-23709: Refactor authentication handler #1789

kovayur commented May 6, 2024 •

edited

Loading

openshift-ci bot commented May 6, 2024

dhaus67 May 7, 2024

kovayur May 7, 2024

dhaus67 May 7, 2024

kovayur May 7, 2024

dhaus67 May 7, 2024

kovayur May 7, 2024

dhaus67 May 7, 2024

kovayur May 7, 2024

dhaus67 May 7, 2024

kovayur May 7, 2024

dhaus67 May 7, 2024

kovayur May 7, 2024

dhaus67 May 7, 2024

kovayur May 7, 2024

kovayur commented May 10, 2024 •

edited

Loading

dhaus67 May 14, 2024

kovayur May 21, 2024

dhaus67 May 14, 2024

kovayur May 21, 2024

dhaus67 May 15, 2024

kovayur May 21, 2024

dhaus67 May 15, 2024

kovayur May 21, 2024

dhaus67 May 15, 2024

kovayur May 21, 2024

openshift-ci bot commented May 22, 2024

		KeysURL(IAMConfig.JwksURL). // ocm JWK JSON web token signing certificates URL
		KeysFile(IAMConfig.JwksFile). // ocm JWK backup JSON web token signing certificates

		JwksURL: "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/certs",
		JwksFile: "config/jwks-file.json",

ROX-23709: Refactor authentication handler #1789

ROX-23709: Refactor authentication handler #1789

Conversation

kovayur commented May 6, 2024 • edited Loading

Description

Checklist (Definition of Done)

Test manual

openshift-ci bot commented May 6, 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

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 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented May 22, 2024

kovayur commented May 6, 2024 •

edited

Loading

kovayur commented May 10, 2024 •

edited

Loading