[oidc] encode and validate state params

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.

Author: AlexTugarev

components/public-api-server/go.mod

@@ -42,6 +42,7 @@ require (
 	github.com/felixge/httpsnoop v1.0.1 // indirect
 	github.com/go-jose/go-jose/v3 v3.0.0 // indirect
 	github.com/go-sql-driver/mysql v1.6.0 // indirect
+	github.com/golang-jwt/jwt/v4 v4.4.3 // indirect
 	github.com/golang/protobuf v1.5.2 // indirect
 	github.com/gorilla/websocket v1.5.0 // indirect
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect

components/public-api-server/go.sum

@@ -66,7 +66,7 @@ func (s *Service) OAuth2Middleware(next http.Handler) http.Handler {
 			return
 		}
 
-		state, err := decodeStateParam(stateParam)
+		state, err := s.decodeStateParam(stateParam)
 		if err != nil {
 			http.Error(rw, "bad state param", http.StatusBadRequest)
 			return

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

@@ -22,7 +22,7 @@ func TestRoute_start(t *testing.T) {
 	idpUrl := newFakeIdP(t)
 
 	// setup test server with client routes
-	baseUrl, _, configId := newTestServer(t, testServerParams{
+	baseUrl, _, configId, _ := newTestServer(t, testServerParams{
 		issuer:      idpUrl,
 		returnToURL: "",
 	})
@@ -50,11 +50,11 @@ func TestRoute_callback(t *testing.T) {
 	idpUrl := newFakeIdP(t)
 
 	// setup test server with client routes
-	baseUrl, stateParam, _ := newTestServer(t, testServerParams{
+	baseUrl, stateParam, _, service := newTestServer(t, testServerParams{
 		issuer:      idpUrl,
 		returnToURL: "/relative/url/to/some/page",
 	})
-	state, err := encodeStateParam(*stateParam)
+	state, err := service.encodeStateParam(*stateParam)
 	require.NoError(t, err)
 
 	// hit the /callback endpoint
@@ -92,7 +92,7 @@ type testServerParams struct {
 	clientID    string
 }
 
-func newTestServer(t *testing.T, params testServerParams) (url string, state *StateParam, configId string) {
+func newTestServer(t *testing.T, params testServerParams) (url string, state *StateParam, configId string, oidcService *Service) {
 	router := chi.NewRouter()
 	oidcService, dbConn := setupOIDCServiceForTests(t)
 	router.Mount("/oidc", Router(oidcService))
@@ -124,5 +124,5 @@ func newTestServer(t *testing.T, params testServerParams) (url string, state *St
 		ReturnToURL:    params.returnToURL,
 	}
 
-	return url, stateParam, configId
+	return url, stateParam, configId, oidcService
 }

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

@@ -14,7 +14,6 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
-	"strings"
 
 	"github.com/coreos/go-oidc/v3/oidc"
 	goidc "github.com/coreos/go-oidc/v3/oidc"
@@ -28,8 +27,9 @@ import (
 )
 
 type Service struct {
-	dbConn *gorm.DB
-	cipher db.Cipher
+	dbConn   *gorm.DB
+	cipher   db.Cipher
+	stateJWT *StateJWT
 
 	verifierByIssuer      map[string]*goidc.IDTokenVerifier
 	sessionServiceAddress string
@@ -57,18 +57,20 @@ type AuthFlowResult struct {
 	Claims  map[string]interface{} `json:"claims"`
 }
 
-func NewService(sessionServiceAddress string, dbConn *gorm.DB, cipher db.Cipher) *Service {
+func NewService(sessionServiceAddress string, dbConn *gorm.DB, cipher db.Cipher, stateJWT *StateJWT) *Service {
 	return &Service{
 		verifierByIssuer:      map[string]*goidc.IDTokenVerifier{},
 		sessionServiceAddress: sessionServiceAddress,
 
 		dbConn: dbConn,
 		cipher: cipher,
+
+		stateJWT: stateJWT,
 	}
 }
 
-func newTestService(sessionServiceAddress string, dbConn *gorm.DB, cipher db.Cipher) *Service {
-	service := NewService(sessionServiceAddress, dbConn, cipher)
+func newTestService(sessionServiceAddress string, dbConn *gorm.DB, cipher db.Cipher, stateJWT *StateJWT) *Service {
+	service := NewService(sessionServiceAddress, dbConn, cipher, stateJWT)
 	service.skipVerifyIdToken = true
 	return service
 }
@@ -82,7 +84,7 @@ func (s *Service) GetStartParams(config *ClientConfig, redirectURL string) (*Sta
 		// TODO(at) read a relative URL from `returnTo` query param of the start request
 		ReturnToURL: "/",
 	}
-	state, err := encodeStateParam(stateParam)
+	state, err := s.encodeStateParam(stateParam)
 	if err != nil {
 		return nil, fmt.Errorf("failed to encode state")
 	}
@@ -104,21 +106,23 @@ func (s *Service) GetStartParams(config *ClientConfig, redirectURL string) (*Sta
 	}, nil
 }
 
-// TODO(at) state should be a JWT encoding a redirect location
-// For now, just use base64
-func encodeStateParam(state StateParam) (string, error) {
-	b, err := json.Marshal(state)
-	if err != nil {
-		return "", fmt.Errorf("failed to marshal state to json: %w", err)
-	}
-
-	return base64.StdEncoding.EncodeToString(b), nil
+func (s *Service) encodeStateParam(state StateParam) (string, error) {
+	encodedState, err := s.stateJWT.Encode(StateClaims{
+		ClientConfigID: state.ClientConfigID,
+		ReturnToURL:    state.ReturnToURL,
+	})
+	return encodedState, err
 }
 
-func decodeStateParam(encoded string) (StateParam, error) {
-	var result StateParam
-	err := json.NewDecoder(base64.NewDecoder(base64.StdEncoding, strings.NewReader(encoded))).Decode(&result)
-	return result, err
+func (s *Service) decodeStateParam(encodedToken string) (StateParam, error) {
+	claims, err := s.stateJWT.Decode(encodedToken)
+	if err != nil {
+		return StateParam{}, err
+	}
+	return StateParam{
+		ClientConfigID: claims.ClientConfigID,
+		ReturnToURL:    claims.ReturnToURL,
+	}, nil
 }
 
 func randString(size int) (string, error) {
@@ -152,7 +156,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")
 	}

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

@@ -120,13 +120,13 @@ func TestGetClientConfigFromCallbackRequest(t *testing.T) {
 		OAuth2Config:   &oauth2.Config{},
 	})
 
-	state, err := encodeStateParam(StateParam{
+	state, err := service.encodeStateParam(StateParam{
 		ClientConfigID: configID,
 		ReturnToURL:    "",
 	})
 	require.NoError(t, err, "failed encode state param")
 
-	state_unknown, err := encodeStateParam(StateParam{
+	state_unknown, err := service.encodeStateParam(StateParam{
 		ClientConfigID: "UNKNOWN",
 		ReturnToURL:    "",
 	})
@@ -209,10 +209,11 @@ func setupOIDCServiceForTests(t *testing.T) (*Service, *gorm.DB) {
 
 	dbConn := dbtest.ConnectForTests(t)
 	cipher := dbtest.CipherSet(t)
+	stateJWT := newTestStateJWT([]byte("ANY KEY"), 5*time.Minute)
 
 	sessionServerAddress := newFakeSessionServer(t)
 
-	service := newTestService(sessionServerAddress, dbConn, cipher)
+	service := newTestService(sessionServerAddress, dbConn, cipher, stateJWT)
 	return service, dbConn
 }
 

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

@@ -0,0 +1,61 @@
+// Copyright (c) 2023 Gitpod GmbH. All rights reserved.
+// Licensed under the GNU Affero General Public License (AGPL).
+// See License.AGPL.txt in the project root for license information.
+
+package oidc
+
+import (
+	"time"
+
+	"github.com/golang-jwt/jwt/v4"
+)
+
+type StateJWT struct {
+	key       []byte
+	expiresIn time.Duration
+}
+
+func NewStateJWT(key []byte) *StateJWT {
+	return &StateJWT{
+		key:       key,
+		expiresIn: 5 * time.Minute,
+	}
+}
+
+func newTestStateJWT(key []byte, expiresIn time.Duration) *StateJWT {
+	thing := NewStateJWT(key)
+	thing.expiresIn = expiresIn
+	return thing
+}
+
+type StateClaims struct {
+	// Internal client ID
+	ClientConfigID string `json:"clientId"`
+	ReturnToURL    string `json:"returnTo"`
+
+	jwt.RegisteredClaims
+}
+
+func (s *StateJWT) Encode(claims StateClaims) (string, error) {
+
+	expirationTime := time.Now().Add(s.expiresIn)
+	claims.ExpiresAt = jwt.NewNumericDate(expirationTime)
+
+	token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)
+	encodedToken, err := token.SignedString(s.key)
+
+	return encodedToken, err
+}
+
+func (s *StateJWT) Decode(tokenString string) (*StateClaims, error) {
+	claims := &StateClaims{}
+	_, err := jwt.ParseWithClaims(
+		tokenString,
+		claims,
+		func(token *jwt.Token) (interface{}, error) {
+			return []byte(s.key), nil
+		},
+	)
+
+	return claims, err
+}

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

@@ -0,0 +1,80 @@
+// Copyright (c) 2023 Gitpod GmbH. All rights reserved.
+// Licensed under the GNU Affero General Public License (AGPL).
+// See License.AGPL.txt in the project root for license information.
+
+package oidc
+
+import (
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/golang-jwt/jwt/v4"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_Encode(t *testing.T) {
+	stateJWT := NewStateJWT([]byte("ANY KEY"))
+	encodedState, err := stateJWT.Encode(StateClaims{
+		ClientConfigID: "test-id",
+		ReturnToURL:    "test-url",
+	})
+	require.NoError(t, err)
+	// check for header: { "alg": "HS256", "typ": "JWT" }
+	require.Contains(t, encodedState, "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.", "")
+}
+
+func Test_Decode(t *testing.T) {
+
+	testCases := []struct {
+		Label         string
+		Key4Encode    string
+		expiresIn     time.Duration
+		Key4Decode    string
+		ExpectedError string
+	}{
+		{
+			Label:         "happy path",
+			Key4Encode:    "ANY KEY",
+			expiresIn:     5 * time.Minute,
+			Key4Decode:    "ANY KEY",
+			ExpectedError: "",
+		},
+		{
+			Label:         "expired state token",
+			Key4Encode:    "ANY KEY",
+			expiresIn:     0 * time.Second,
+			Key4Decode:    "ANY KEY",
+			ExpectedError: "token is expired",
+		},
+		{
+			Label:         "signature is invalid",
+			Key4Encode:    "OTHER KEY",
+			expiresIn:     5 * time.Minute,
+			Key4Decode:    "ANY KEY",
+			ExpectedError: jwt.ErrSignatureInvalid.Error(),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Label, func(t *testing.T) {
+			encoder := newTestStateJWT([]byte(tc.Key4Encode), tc.expiresIn)
+			decoder := NewStateJWT([]byte(tc.Key4Decode))
+			encodedState, err := encoder.Encode(StateClaims{
+				ClientConfigID: "test-id",
+				ReturnToURL:    "test-url",
+			})
+			if err != nil && tc.ExpectedError == "" {
+				require.FailNowf(t, "Unexpected error on `Encode`.", "Error: %", err)
+			}
+			_, err = decoder.Decode(encodedState)
+			if err != nil && tc.ExpectedError == "" {
+				require.FailNowf(t, "Unexpected error on `Decode`.", "Error: %", err)
+			}
+			if err != nil && !strings.Contains(err.Error(), tc.ExpectedError) {
+				require.FailNowf(t, "Unmatched error.", "Got error: %", err.Error())
+			}
+		})
+	}
+
+}

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

@@ -75,11 +75,13 @@ func Start(logger *logrus.Entry, version string, cfg *config.Configuration) erro
 		}
 	}
 
+	var stateJWT *oidc.StateJWT
 	if cfg.OIDCClientJWTSigningSecretPath != "" {
-		_, err := readSecretFromFile(cfg.OIDCClientJWTSigningSecretPath)
+		oidcClientJWTSigningSecret, err := readSecretFromFile(cfg.OIDCClientJWTSigningSecretPath)
 		if err != nil {
 			return fmt.Errorf("failed to read JWT signing secret for OIDC flows: %w", err)
 		}
+		stateJWT = oidc.NewStateJWT([]byte(oidcClientJWTSigningSecret))
 	} else {
 		log.Info("No JWT signing secret for OIDC flows is configured.")
 	}
@@ -109,7 +111,7 @@ func Start(logger *logrus.Entry, version string, cfg *config.Configuration) erro
 
 	srv.HTTPMux().Handle("/stripe/invoices/webhook", handlers.ContentTypeHandler(stripeWebhookHandler, "application/json"))
 
-	oidcService := oidc.NewService(cfg.SessionServiceAddress, dbConn, cipherSet)
+	oidcService := oidc.NewService(cfg.SessionServiceAddress, dbConn, cipherSet, stateJWT)
 
 	if registerErr := register(srv, &registerDependencies{
 		connPool:    connPool,