[OIDC] Check issuer URL for reachability #16331

AlexTugarev · 2023-02-10T06:44:12Z

Description

As part of #15960, this should help to identify issues with entered OIDC client config and/or the service an Org admin wants to connect with. Non reachable services are a common source of errors when connecting to 3rd party services. An early check helps to identify problems early on.

Related Issue(s)

Part of #15960

How to test

Try to create an OIDC client with unreachable issuer,
see there is an error logged on JS console and network tab, e.g.

{"code":"invalid_argument","message":"Get \"https://accounts.foobaar.com\": dial tcp: lookup accounts.foobaar.com on 10.43.0.10:53: no such host"}

Verify, creating of OIDC clients with reachable issuer (e.g. https://accounts.google.com) works.

Release Notes

NONE

Documentation

Build Options:

/werft with-github-actions
Experimental feature to run the build with GitHub Actions (and not in Werft).
leeway-no-cache
leeway-target=components:all
/werft no-test
Run Leeway with --dont-test

Publish Options

/werft publish-to-npm
/werft publish-to-jb-marketplace

Installer Options

with-ee-license
with-slow-database
with-dedicated-emulation
with-ws-manager-mk2
workspace-feature-flags
Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

werft-gitpod-dev-com · 2023-02-10T06:44:21Z

started the job as gitpod-build-at-check-issuer.1 because the annotations in the pull request description changed
(with .werft/ from main)

easyCZ

Looking good, left a couple of comments which can do in a follow up. It would be great to also test the issuerReachability in a test case.

easyCZ · 2023-02-10T07:45:13Z

components/public-api-server/pkg/apiv1/oidc.go

+		},
+	}
+
+	resp, err := client.Get(host)


You could use the HEAD request here. It better communicates that you only want to know if it's reachable, rather than actually needing the payload.

Let's change to Head, I've no preferences.

easyCZ · 2023-02-10T07:46:23Z

components/public-api-server/pkg/apiv1/oidc.go

@@ -318,3 +326,28 @@ func toDbOIDCSpec(oauth2Config *v1.OAuth2Config, oidcConfig *v1.OIDCConfig) db.O
 		Scopes:       append([]string{goidc.ScopeOpenID, "profile", "email"}, oauth2Config.GetScopes()...),
 	}
 }
+
+func assertIssuerIsReachable(host string) error {


This should accept the context and construct the request with context (http.NewRequest().WithContext()). This ensures that if the caller cancels, we also cancel.

In general, anything crossing a network boundary should accept context, and propagate it.

easyCZ · 2023-02-10T07:46:54Z

components/public-api-server/pkg/apiv1/oidc.go

+
+func assertIssuerIsReachable(host string) error {
+	tr := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},


Why are we disabling TLS? If necessary, a comment would help.

easyCZ · 2023-02-10T07:49:46Z

components/public-api-server/pkg/apiv1/oidc.go

+func assertIssuerIsReachable(host string) error {
+	tr := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+		Proxy:           http.ProxyFromEnvironment,


Do we need this? The environment this runs is in controlled by us, and we don't have a system level proxy configured.

Took this from a k8s reachability probe.

we don't have a system level proxy configured.

Since #12726 we have an option to set it on the installation level.

The environment this runs is in controlled by us

I'm not sure this is the question. If the Org only provides a proxy to reach internal Git, which was several times the case with self-hosted, it might still be relevant.

easyCZ · 2023-02-10T07:50:05Z

components/public-api-server/pkg/apiv1/oidc.go

@@ -50,6 +53,11 @@ func (s *OIDCService) CreateClientConfig(ctx context.Context, req *connect.Reque
 		return nil, err
 	}

+	err = assertIssuerIsReachable(req.Msg.GetConfig().GetOidcConfig().GetIssuer())


Does it make sense to check the URL actually parses as a URL, before we make this request?

How do we make sure this doesn't get used as a way to DDOS third party systems? If I supply a https://my-victim.org, we'd hit those endpoints on behalf of the attacker.

One way to guard would be to rate-limit the create itself.

+1 for rate-limiting.

easyCZ · 2023-02-10T07:53:21Z

components/public-api-server/pkg/apiv1/oidc_test.go

@@ -96,13 +99,13 @@ func TestOIDCService_CreateClientConfig_FeatureFlagEnabled(t *testing.T) {
 	})

 	t.Run("creates oidc client config", func(t *testing.T) {
-		serverMock, client, dbConn := setupOIDCService(t, withOIDCFeatureEnabled)
+		serverMock, client, dbConn, issuer := setupOIDCService(t, withOIDCFeatureEnabled)


It took me quite a while to wrap my head around the fact that we're using our server, which has our API handlers, as the issuer, for the validation check during create.

Perhaps it would make sense to start a second server, and return it as part of setupOIDCService which is the issuer server. That would also give us the ability to test for the case when the check fails, which we're currently not covering AFAIK.

🙈 indeed, this was a shortcut to have a known to be reachable host.

💯 will touch on this in follow-up to extract it. As mentioned a while ago, the mocked OIDC services should become proper mocks to get full coverage of the handlers of ours.

easyCZ · 2023-02-10T07:54:05Z

components/public-api-server/pkg/apiv1/oidc.go

+	}
+	resp.Body.Close()
+	if resp.StatusCode > 499 {
+		return fmt.Errorf("returned status %d", resp.StatusCode)


Suggested change

return fmt.Errorf("returned status %d", resp.StatusCode)

return fmt.Errorf("OIDC reachability check returned status %d", resp.StatusCode)

This ends up being the public facing error message, so we should communicate as much detail as possible.

[oidc] assertIssuerIsReachable

2bb3e24

roboquat added release-note-none do-not-merge/work-in-progress labels Feb 10, 2023

roboquat added the size/L label Feb 10, 2023

AlexTugarev marked this pull request as ready for review February 10, 2023 07:39

AlexTugarev requested a review from a team February 10, 2023 07:39

roboquat removed the do-not-merge/work-in-progress label Feb 10, 2023

github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 10, 2023

easyCZ approved these changes Feb 10, 2023

View reviewed changes

roboquat merged commit f4889ad into main Feb 10, 2023

roboquat deleted the at/check-issuer branch February 10, 2023 07:58

AlexTugarev mentioned this pull request Feb 10, 2023

[oidc] encode and validate state params #16317

Merged

15 tasks

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OIDC] Check issuer URL for reachability #16331

[OIDC] Check issuer URL for reachability #16331

AlexTugarev commented Feb 10, 2023 •

edited

Loading

werft-gitpod-dev-com bot commented Feb 10, 2023

easyCZ left a comment

easyCZ Feb 10, 2023

AlexTugarev Feb 10, 2023

easyCZ Feb 10, 2023

easyCZ Feb 10, 2023

easyCZ Feb 10, 2023

easyCZ Feb 10, 2023

AlexTugarev Feb 10, 2023

easyCZ Feb 10, 2023

AlexTugarev Feb 10, 2023

easyCZ Feb 10, 2023

AlexTugarev Feb 10, 2023

easyCZ Feb 10, 2023

easyCZ Feb 10, 2023

	return fmt.Errorf("returned status %d", resp.StatusCode)
	return fmt.Errorf("OIDC reachability check returned status %d", resp.StatusCode)

[OIDC] Check issuer URL for reachability #16331

[OIDC] Check issuer URL for reachability #16331

Conversation

AlexTugarev commented Feb 10, 2023 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Build Options:

Preview Environment Options:

werft-gitpod-dev-com bot commented Feb 10, 2023

easyCZ left a 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

Choose a reason for hiding this comment

AlexTugarev commented Feb 10, 2023 •

edited

Loading