Gitpod OIDC Identity Provider #16482

csweichel · 2023-02-20T17:43:53Z

Note This is a PoC, and not necessarily a feature that will land soon (or ever).

Description

Private Video - Watch Video

This PR implements an OIDC identity provider, and adds an IDToken function to the API surface. This function is served from public-api-server and delegates authZ/authN to server by calling a no-op GetIDToken function.

Private keys used for signing the JWT are only held in memory and are ephemeral. A Redis-backed public key cache ensures that the JSON Web Key Set contains all public keys currently in circulation. The issued ID tokens have a TTL of one hour, as do the public keys whose TTL is refreshed whenever they're used.

The IDToken API call is available through gp idp which is currently hidden, because this is not a released feature. To this end, the workspace token gained the GetIDToken function scope.

The new code has a test coverage > 70%.

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

csweichel · 2023-03-01T21:29:26Z

/werft with-preview

👎 unknown command: with-preview
Use /werft help to list the available commands

csweichel · 2023-03-01T21:29:37Z

/werft run with-preview

👍 started the job as gitpod-build-cw-idp.13
(with .werft/ from main)

easyCZ · 2023-03-03T08:26:50Z

components/public-api/gitpod/experimental/v1/idp.proto

@@ -0,0 +1,19 @@
+syntax = "proto3";


Nit, I'd name the file in full - identity-provider. Just because it avoids an extra mental lookup

easyCZ · 2023-03-03T08:27:44Z

components/public-api/gitpod/experimental/v1/idp.proto

+
+service IDPService {
+  // GetIDToken produces a new IT token
+  rpc GetIDToken(GetIDTokenRequest) returns (GetIDTokenResponse) {};


Suggested change

rpc GetIDToken(GetIDTokenRequest) returns (GetIDTokenResponse) {};

rpc GetToken(GetTokenRequest) returns (GetTokenResponse) {};

It's misleading that it says ID, when the request/response objects don't refer to an id property

The returned token is an IDToken in the OIDC sense. I added a link to the spec in the comment.

easyCZ · 2023-03-03T08:28:49Z

components/public-api/go/config/config.go

@@ -26,5 +29,11 @@ type Configuration struct {
 	// Path to directory containing database configuration files
 	DatabaseConfigPath string `json:"databaseConfigPath"`

+	// RedisAddress configures the redis connection of this component
+	RedisAddress string `json:"redisAddress"`


Let's make this a struct, which has an address as a property. I can imagine we may want to pass additional info (like creds) and having these sit under a single redis property makes it easier.

Redis struct { Address string `json:...` }

easyCZ · 2023-03-03T08:29:00Z

components/public-api/go/config/config.go

 	Server *baseserver.Configuration `json:"server,omitempty"`
 }
+
+type RedisConfiguration struct {


not anymore :)

easyCZ · 2023-03-03T08:30:51Z

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

+	}
+
+	if len(req.Msg.Audience) < 1 {
+		return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("must have at least one audience entry"))


Suggested change

return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("must have at least one audience entry"))

return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("Must have at least one audience entry"))

For consistency with other APIs here

easyCZ · 2023-03-03T08:39:20Z

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

+		})
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		err := redisClient.Ping(ctx).Err()


I haven't dug into this, but is the Redis client reconnecting? If so, we shouldn't need to do a ping here

Just creating the client doesn't check if the connection works. I'd rather the component fails to start than fails to function if Redis is unavailable.

Sure, but this only checks at connection time (server startup time), not subsequently so the value we get from this remains limited

easyCZ · 2023-03-03T08:40:28Z

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

@@ -114,13 +131,26 @@ func Start(logger *logrus.Entry, version string, cfg *config.Configuration) erro

 	oidcService := oidc.NewService(cfg.SessionServiceAddress, dbConn, cipherSet, stateJWT)

+	var idpCache idp.KeyCache
+	if redisClient == nil {
+		log.Info("No Redis cache configured - caching IDP public keys in memory.")


Do we actually want to allow this mode? Given we run multiple replicas of pub-api, this is bound to cause issues. I'd rather we say we have a dependency on Redis and we do not try to cache in memory

easyCZ · 2023-03-03T08:41:18Z

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

+	rootHandler.Mount(idpRoute, idpServiceHandler)
+
+	// IDP well-known and keys routes
+	rootHandler.Mount("/idp", deps.idpService.Router())


Out of curiosity, what stops us serving these on the idpServicehandler rather than having to have another endpoint?

Added a comment

easyCZ · 2023-03-03T08:42:12Z

components/server/src/workspace/workspace-starter.ts

@@ -1507,6 +1507,7 @@ export class WorkspaceStarter {
            "function:getTeams",
            "function:trackEvent",
            "function:getSupportedWorkspaceClasses",
+            "function:getIDToken",


That's quite opaque, can we be more specific? Like mentioning IDP?

Added a comment

easyCZ · 2023-03-03T08:43:08Z

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

+var _ v1connect.IDPServiceHandler = ((*IDPService)(nil))
+
+// GetIDToken implements v1connect.IDPServiceHandler
+func (srv *IDPService) GetIDToken(ctx context.Context, req *connect.Request[v1.GetIDTokenRequest]) (*connect.Response[v1.GetIDTokenResponse], error) {


Would be nice to have a unit test (like the other API packages) for this just to avoid setting the precedent

csweichel · 2023-03-03T11:56:56Z

/hold

csweichel · 2023-03-03T12:09:48Z

/hold cancel

(verified that the Redis readiness probe works and public-api-server starts)

aledbf · 2023-03-03T12:12:13Z

components/gitpod-cli/cmd/idp-login-aws.go

+	Use:   "aws",
+	Short: "Login to AWS",
+	RunE: func(cmd *cobra.Command, args []string) error {
+		cmd.SilenceUsage = true


Check if the aws binary exists to avoid confusing errors?

This is the behaviour if the AWS binary is not available:

How should the error look like instead?

I was thinking of requesting to install the CLI, but that is just fine.

We could link to the AWS Docs. Arguably though, if someone's using this feature they're deep enough into it that they'll have looked at our (yet to be written) docs which will need to state that the AWS CLI needs to be installed.

csweichel · 2023-03-03T12:20:44Z

/hold cancel

Redis isn't a stateful set yet, which means that when Redis gets restarted we loose the keys. That's likely to happen around deployments - and expected.

csweichel · 2023-03-03T14:49:50Z

/hold

Updating the key cache mechanism to make it more resilient against Redis failures

csweichel · 2023-03-03T15:12:45Z

/hold cancel

iQQBot · 2023-03-03T13:34:01Z

components/gitpod-cli/cmd/idp-login-vault.go

+		// vault write auth/jwt/login role=demo jwt=$TKN -format=json
+		out, err := exec.Command("vault", "write", "-format=json", "auth/jwt/login", "role="+idpLoginVaultOpts.Role, "jwt="+tkn).CombinedOutput()
+		if err != nil {
+			return fmt.Errorf("%w: %s", err, string(out))


Suggested change

return fmt.Errorf("%w: %s", err, string(out))

return xerrors.Errorf("%w: %s", err, string(out))

iQQBot · 2023-03-03T13:37:58Z

components/gitpod-cli/cmd/idp-login-aws.go

+	RunE: func(cmd *cobra.Command, args []string) error {
+		cmd.SilenceUsage = true
+		if idpLoginAwsOpts.RoleARN == "" {
+			return fmt.Errorf("missing --role-arn or IDP_AWS_ROLE_ARN env var")


Suggested change

return fmt.Errorf("missing --role-arn or IDP_AWS_ROLE_ARN env var")

return xerrors.Errorf("missing --role-arn or IDP_AWS_ROLE_ARN env var")

iQQBot · 2023-03-03T13:38:11Z

components/gitpod-cli/cmd/idp-login-aws.go

+		awsCmd := exec.Command("aws", "sts", "assume-role-with-web-identity", "--role-arn", idpLoginAwsOpts.RoleARN, "--role-session-name", fmt.Sprintf("gitpod-%d", time.Now().Unix()), "--web-identity-token", tkn)
+		out, err := awsCmd.CombinedOutput()
+		if err != nil {
+			return fmt.Errorf("%w: %s", err, string(out))


Suggested change

return fmt.Errorf("%w: %s", err, string(out))

return xerrors.Errorf("%w: %s", err, string(out))

iQQBot · 2023-03-03T16:06:33Z

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

@@ -114,13 +129,22 @@ func Start(logger *logrus.Entry, version string, cfg *config.Configuration) erro

 	oidcService := oidc.NewService(cfg.SessionServiceAddress, dbConn, cipherSet, stateJWT)

+	if redisClient == nil {


why we need check here, we have err = redisClient.Ping(ctx).Err() in above, if redisClient is nil, I think it will failed or panic.

RedisClient is never nil and without the Ping call we'll fail at runtime if redis isn't available. This way the service doesn't start rather than fail strangely when running.

iQQBot · 2023-03-03T16:12:11Z

Ah forgot to add hold, but it's small thing

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

csweichel force-pushed the cw/idp branch 4 times, most recently from 047204b to 488b79d Compare February 28, 2023 23:16

ChevronTango mentioned this pull request Mar 1, 2023

Have Gitpod-based commits GPG-signed #666

Open

csweichel force-pushed the cw/idp branch 3 times, most recently from b95fd99 to 923a5b4 Compare March 1, 2023 21:20

csweichel force-pushed the cw/idp branch 2 times, most recently from bd01a4e to 1f0c626 Compare March 3, 2023 00:23

easyCZ reviewed Mar 3, 2023

View reviewed changes

csweichel force-pushed the cw/idp branch 3 times, most recently from f6bb261 to 0f47870 Compare March 3, 2023 11:16

csweichel marked this pull request as ready for review March 3, 2023 11:28

csweichel requested review from a team March 3, 2023 11:28

roboquat removed the do-not-merge/work-in-progress label Mar 3, 2023

github-actions bot added team: webapp Issue belongs to the WebApp team team: IDE labels Mar 3, 2023

easyCZ approved these changes Mar 3, 2023

View reviewed changes

csweichel force-pushed the cw/idp branch from d40a509 to d75a334 Compare March 3, 2023 11:56

roboquat added the do-not-merge/hold label Mar 3, 2023

aledbf reviewed Mar 3, 2023

View reviewed changes

roboquat removed the do-not-merge/hold label Mar 3, 2023

csweichel added 14 commits March 3, 2023 14:23

Prototype IDP provider

b1acf4a

[gp cli] Add IDP commands

fc25531

[public-api] Remove zitadel based IDP implementation

8e414ad

[gitpod-cli] Add IDP support for Vault

c5c401e

[idp] Remove per-org IDP

2acca62

[idp] Add key cache and random key IDs

c309fe2

[idp] Defer GetIDToken authorisation to server

447ebb4

[idp] Add Redis public key cache

2896dc4

[gitpod-cli] Hide IDP commands

adad568

[idp] Add key ID to JWT

682de90

[idp] Add unit tests

50b314c

[idp] Adress review comments

f334af8

[public-api-server] Use logging middleware globally

c8ec88d

[public-api-server] Simplify service registration

4265479

csweichel force-pushed the cw/idp branch from d75a334 to 4265479 Compare March 3, 2023 14:24

roboquat added the do-not-merge/hold label Mar 3, 2023

[idp] Add Redis outage resilience

c566e6e

roboquat removed the do-not-merge/hold label Mar 3, 2023

iQQBot approved these changes Mar 3, 2023

View reviewed changes

roboquat merged commit da4cafd into main Mar 3, 2023

roboquat deleted the cw/idp branch March 3, 2023 16:11

roboquat added deployed: webapp Meta team change is running in production deployed: IDE IDE change is running in production deployed Change is completely running in production labels Mar 6, 2023

	rpc GetIDToken(GetIDTokenRequest) returns (GetIDTokenResponse) {};
	rpc GetToken(GetTokenRequest) returns (GetTokenResponse) {};

	return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("must have at least one audience entry"))
	return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("Must have at least one audience entry"))

	return fmt.Errorf("%w: %s", err, string(out))
	return xerrors.Errorf("%w: %s", err, string(out))

	return fmt.Errorf("missing --role-arn or IDP_AWS_ROLE_ARN env var")
	return xerrors.Errorf("missing --role-arn or IDP_AWS_ROLE_ARN env var")

		@@ -114,13 +129,22 @@ func Start(logger logrus.Entry, version string, cfg config.Configuration) erro

		oidcService := oidc.NewService(cfg.SessionServiceAddress, dbConn, cipherSet, stateJWT)

		if redisClient == nil {

Gitpod OIDC Identity Provider #16482

Gitpod OIDC Identity Provider #16482

Conversation

csweichel commented Feb 20, 2023 • edited Loading

Description

Release Notes

Documentation

Build Options:

Preview Environment Options:

csweichel commented Mar 1, 2023 • edited by werft-gitpod-dev-com bot Loading

csweichel commented Mar 1, 2023 • edited by werft-gitpod-dev-com bot 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

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

csweichel commented Mar 3, 2023

csweichel commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csweichel commented Mar 3, 2023

csweichel commented Mar 3, 2023

csweichel commented Mar 3, 2023

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

iQQBot commented Mar 3, 2023

csweichel commented Feb 20, 2023 •

edited

Loading

csweichel commented Mar 1, 2023 •

edited by werft-gitpod-dev-com bot

Loading

csweichel commented Mar 1, 2023 •

edited by werft-gitpod-dev-com bot

Loading

aledbf Mar 3, 2023 •

edited

Loading