[oidc] encode and validate state params #16317

AlexTugarev · 2023-02-09T11:36:19Z

After this PR is merged, the state param of the OIDC/OAuth2 gets signed and verified. Validating of integrity is crucial, as this piece of information contains the ID of the OIDC client to continue with when Gitpod receives the callback from a 3rd party. Tests should show that expiration time is checked and signature validation is effective.

Related Issue(s)

Part of #15956

How to test

See internal post for test credentials.

Create new Org
Create new SSO client under settings
Select Login from actions menu (three dot button)
Copy the full URL (starting with https://accounts.google.com/o/oauth2/v2/auth/oauthchooseaccount?) from browser's location bar.
Now, first see a working login into your @gitpod.io account if you proceed.
Take the URL from step 4.), find the state query param, alter the last segment (they are separated by .), then paste it into a new tab. You should see client config not found. The logs will contain the bad state param as reason.

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-09T13:39:25Z

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

easyCZ

LGTM in general. Let's find a better way to avoid the skipVerification, as that pushes test code into our main implementation.

easyCZ · 2023-02-10T08:05:43Z

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

-	cipher db.Cipher
+	dbConn   *gorm.DB
+	cipher   db.Cipher
+	stateJWT *StateJWT


In some way, the fact that it's a JWT is an implementation detail. And as such, here we could go with an interface rather than the fact that it is JWT

You could express this as

type State interface { Encode(..) Decode(..) }

While this creates indirection, it does help future readers to not have to know the details of how it's encoded, just that it is.

easyCZ · 2023-02-10T08:07:51Z

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

+func newTestService(sessionServiceAddress string, dbConn *gorm.DB, cipher db.Cipher, stateJWT *StateJWT) *Service {
+	service := NewService(sessionServiceAddress, dbConn, cipher, stateJWT)
 	service.skipVerifyIdToken = true
 	return service
 }


Either move this to a test package, or make it a first class constructor which doens't mention Test. It's an anti-pattern in Go to have test code alongisde the non-test implementation.

But I think we shouldn't have the skipVerifyIdToken as a thing on this service at all. We should either abstract the verification behind an interface, such that we can mock/stub it in tests, or actually run the verification check.

Alternatively, if you want to provide this as an option for the caller, encode it into the request such that the caller can choose to have the verification skipped.

Let me know if you'd like to pair on this.

'Removed newTestService.

skipVerifyIdToken is passed to a constructor from a library. It was a recommended way to use this shortcut and remove skipVerifyIdToken in a separate PR.

With the added golang-jwt/jwt, it's now easier to extend the mocked service to actually to the checks instead of skipping verification.

Let's find a better way to avoid the skipVerification, as that pushes test code into our main implementation.

skipVerification is deemed to be removed in following PR.

Using JWT tokens for encoding/decoding/validation of state params carried throughout the OIDC/OAuth2 flow. Validating of integrity is crucial, as this piece of information contains the ID of the OIDC client to continue with when Gitpod receives the callback from a 3rd party. Tests should show that expiration time is checked and signature validation is effective.

easyCZ

/hold for final Qs

easyCZ · 2023-02-10T09:20:15Z

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

@@ -152,7 +150,7 @@ func (s *Service) GetClientConfigFromCallbackRequest(r *http.Request) (*ClientCo
 		return nil, fmt.Errorf("missing state parameter")
 	}

-	state, err := decodeStateParam(stateParam)
+	state, err := s.decodeStateParam(stateParam)
 	if err != nil {
 		return nil, fmt.Errorf("bad state param")


How do we communicate that this is a User Error, and not a system error?

same as #16331 (comment)

That needs improvements.

easyCZ · 2023-02-10T09:22:13Z

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

@@ -75,11 +75,13 @@ func Start(logger *logrus.Entry, version string, cfg *config.Configuration) erro
 		}
 	}

+	var stateJWT *oidc.StateJWT


If there's no secret, the stateJWT can now be nil. We're passing it into

oidc.NewService(cfg.SessionServiceAddress, dbConn, cipherSet, stateJWT)

Where it will panic when we try to access it on the calls. Should we hard fail if we don't have the secret? How do we handle this case?

Was about to raise this question as well.

First of, adding config for staging and prod in ops.

Should we hard fail if we don't have the secret? How do we handle this case?

Failing the whole component is really bad, but doesn't seem to be wrong as this as crucial bits from security perspective, i.e. falling back to a default is a "no go".

OTOH, a second option might be to check for stateJWT == nil in handlers and fail late. That comes at the cost of monitoring, as we'd need to cover that independently.

How was that discussed with PATs and Stripe integration?

@easyCZ, I'm opting for not failing hard for now. We can change later on. Reason: for now this is all experimental and behind the scene, so let's keep risk at minimum.

Done. Now it should not panic, but any attempt to call the endpoints while the signing key is not provided will be cancelled with an error.

IMO, it's better to hard-fail in this case, I'm happy to do a follow-up change. With Stripe, we didn't hard fail because we had the use-case that we couldn't control the stripe configuration but needed the rest of the server to work. But here, we get a better rollout signal when we rollout and miss something.

@easyCZ, let's do in follow-ups, please!

What else is required to release the breaks on this?

PR is approved, feel free to land

AlexTugarev · 2023-02-15T17:55:06Z

/hold cancel

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/L labels Feb 9, 2023

AlexTugarev force-pushed the at/state-jwt branch from 6312adb to 077843f Compare February 9, 2023 13:53

roboquat added release-note-none and removed do-not-merge/release-note-label-needed labels Feb 9, 2023

AlexTugarev marked this pull request as ready for review February 9, 2023 14:55

AlexTugarev requested a review from a team February 9, 2023 14:55

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

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

easyCZ requested changes Feb 10, 2023

View reviewed changes

AlexTugarev closed this Feb 10, 2023

AlexTugarev force-pushed the at/state-jwt branch from 077843f to 901dcd2 Compare February 10, 2023 08:48

roboquat added size/XS and removed size/L labels Feb 10, 2023

AlexTugarev reopened this Feb 10, 2023

roboquat added size/L and removed size/XS labels Feb 10, 2023

easyCZ approved these changes Feb 10, 2023

View reviewed changes

roboquat added the do-not-merge/hold label Feb 10, 2023

roboquat removed the do-not-merge/hold label Feb 15, 2023

roboquat merged commit 80dc959 into main Feb 15, 2023

roboquat deleted the at/state-jwt branch February 15, 2023 17:55

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

AlexTugarev mentioned this pull request Mar 14, 2023

[OIDC] Secure state carrying using signed/verified JWT tokens #15956

Closed

[oidc] encode and validate state params #16317

[oidc] encode and validate state params #16317

Uh oh!

Conversation

AlexTugarev commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue(s)

How to test

Release Notes

Documentation

Build Options:

Preview Environment Options:

Uh oh!

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

Uh oh!

easyCZ left a comment

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!

Choose a reason for hiding this comment

Uh oh!

easyCZ left a comment

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!

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!

AlexTugarev commented Feb 15, 2023

Uh oh!

Uh oh!

AlexTugarev commented Feb 9, 2023 •

edited

Loading