chore: id token verification performs client id check

teamkeel · Nov 13, 2023 · 66b34ad · 66b34ad
1 parent 83af251
commit 66b34ad
Show file tree

Hide file tree

Showing 8 changed files with 186 additions and 39 deletions.
diff --git a/config/auth.go b/config/auth.go
@@ -50,21 +50,26 @@ func (c *AuthConfig) GetOidcProviders() []Provider {
 	return oidcProviders
 }
 
-func (c *AuthConfig) HasOidcIssuer(issuer string) (bool, error) {
+// GetProvidersOidcIssuer gets all providers by issuer url.
+// It's possible that multiple providers from the same issuer as configured.
+func (c *AuthConfig) GetProvidersOidcIssuer(issuer string) ([]Provider, error) {
+	providers := []Provider{}
+
 	for _, p := range c.Providers {
 		if p.Type == OAuthProvider {
 			continue
 		}
 
 		issuerUrl, err := p.GetIssuer()
 		if err != nil {
-			return false, err
+			return nil, err
 		}
 		if issuerUrl == issuer {
-			return true, nil
+			providers = append(providers, p)
 		}
 	}
-	return false, nil
+
+	return providers, nil
 }
 
 func (c *Provider) GetIssuer() (string, error) {

diff --git a/config/config.go b/config/config.go
@@ -239,14 +239,14 @@ func Validate(config *ProjectConfig) *ConfigErrors {
 		}
 	}
 
-	if config.Auth.Tokens != nil && config.Auth.Tokens.AccessTokenExpiry <= 0 {
+	if config.Auth.Tokens != nil && config.Auth.Tokens.AccessTokenExpiry < 0 {
 		errors = append(errors, &ConfigError{
 			Type:    "invalid",
 			Message: fmt.Sprintf(ConfigAuthTokenExpiryMustBePositive, "access", "accessTokenExpiry"),
 		})
 	}
 
-	if config.Auth.Tokens != nil && config.Auth.Tokens.RefreshTokenExpiry <= 0 {
+	if config.Auth.Tokens != nil && config.Auth.Tokens.RefreshTokenExpiry < 0 {
 		errors = append(errors, &ConfigError{
 			Type:    "invalid",
 			Message: fmt.Sprintf(ConfigAuthTokenExpiryMustBePositive, "refresh", "refreshTokenExpiry"),

diff --git a/config/config_test.go b/config/config_test.go
@@ -201,19 +201,28 @@ func TestMissingOrInvalidTokenEndpoint(t *testing.T) {
 	assert.Contains(t, err.Error(), "auth provider 'missing-endpoint' has missing or invalid https url for field: tokenUrl\n")
 }
 
-func TestHasIssuer(t *testing.T) {
+func TestGetOidcIssuer(t *testing.T) {
 	config, err := Load("fixtures/test_auth.yaml")
 	assert.NoError(t, err)
 
-	hasGoogleIssuer, err := config.Auth.HasOidcIssuer("https://accounts.google.com/")
+	googleIssuer, err := config.Auth.GetProvidersOidcIssuer("https://accounts.google.com/")
 	assert.NoError(t, err)
-	assert.True(t, hasGoogleIssuer)
+	assert.Len(t, googleIssuer, 2)
 
-	hasCustomIssuer, err := config.Auth.HasOidcIssuer("https://dev-skhlutl45lbqkvhv.us.auth0.com")
+	auth0Issuer, err := config.Auth.GetProvidersOidcIssuer("https://dev-skhlutl45lbqkvhv.us.auth0.com")
 	assert.NoError(t, err)
-	assert.True(t, hasCustomIssuer)
+	assert.Len(t, auth0Issuer, 1)
 
-	hasUnknownIssuer, err := config.Auth.HasOidcIssuer("https://nope.com")
+	nopeIssuer, err := config.Auth.GetProvidersOidcIssuer("https://nope.com")
 	assert.NoError(t, err)
-	assert.False(t, hasUnknownIssuer)
+	assert.Len(t, nopeIssuer, 0)
+}
+
+func TestGetOidcSameIssuers(t *testing.T) {
+	config, err := Load("fixtures/test_auth_same_issuers.yaml")
+	assert.NoError(t, err)
+
+	googleIssuer, err := config.Auth.GetProvidersOidcIssuer("https://accounts.google.com/")
+	assert.NoError(t, err)
+	assert.Len(t, googleIssuer, 3)
 }
diff --git a/config/fixtures/test_auth_same_issuers.yaml b/config/fixtures/test_auth_same_issuers.yaml
@@ -0,0 +1,18 @@
+auth:
+  tokens:
+    accessTokenExpiry: 3600
+    refreshTokenExpiry: 604800
+
+  providers:
+    - type: google
+      name: google_1
+      clientId: 1234
+
+    - type: google
+      name: google_2
+      clientId: 1234
+
+    - type: oidc
+      name: google_3
+      issuerUrl: https://accounts.google.com/
+      clientId: 1234
diff --git a/runtime/apis/authapi/token_endpoint_test.go b/runtime/apis/authapi/token_endpoint_test.go
@@ -39,6 +39,7 @@ func TestTokenExchange_ValidNewIdentity(t *testing.T) {
 			{
 				Type:      config.OpenIdConnectProvider,
 				Name:      "my-oidc",
+				ClientId:  "oidc-client-id",
 				IssuerUrl: server.Issuer,
 			},
 		},
@@ -49,7 +50,7 @@ func TestTokenExchange_ValidNewIdentity(t *testing.T) {
 	})
 
 	// Get ID token from server
-	idToken, err := server.FetchIdToken("id|285620", []string{})
+	idToken, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"})
 	require.NoError(t, err)
 
 	// Make a token exchange grant request
@@ -105,6 +106,7 @@ func TestTokenExchange_ValidNewIdentityAllUserInfo(t *testing.T) {
 			{
 				Type:      config.OpenIdConnectProvider,
 				Name:      "my-oidc",
+				ClientId:  "oidc-client-id",
 				IssuerUrl: server.Issuer,
 			},
 		},
@@ -130,7 +132,7 @@ func TestTokenExchange_ValidNewIdentityAllUserInfo(t *testing.T) {
 	})
 
 	// Get ID token from server
-	idToken, err := server.FetchIdToken("id|285620", []string{})
+	idToken, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"})
 	require.NoError(t, err)
 
 	// Make a token exchange grant request
@@ -186,6 +188,7 @@ func TestTokenExchange_ValidUpdatedIdentity(t *testing.T) {
 			{
 				Type:      config.OpenIdConnectProvider,
 				Name:      "my-oidc",
+				ClientId:  "oidc-client-id",
 				IssuerUrl: server.Issuer,
 			},
 		},
@@ -200,7 +203,7 @@ func TestTokenExchange_ValidUpdatedIdentity(t *testing.T) {
 	})
 
 	// Get ID token from server
-	idToken, err := server.FetchIdToken("id|285620", []string{})
+	idToken, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"})
 	require.NoError(t, err)
 
 	// Make a token exchange grant request
@@ -455,6 +458,7 @@ func TestRefreshToken_Valid(t *testing.T) {
 			{
 				Type:      config.OpenIdConnectProvider,
 				Name:      "my-oidc",
+				ClientId:  "oidc-client-id",
 				IssuerUrl: server.Issuer,
 			},
 		},
@@ -465,7 +469,7 @@ func TestRefreshToken_Valid(t *testing.T) {
 	})
 
 	// Get ID token from server
-	idToken, err := server.FetchIdToken("id|285620", []string{})
+	idToken, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"})
 	require.NoError(t, err)
 
 	// Make a token exchange grant request

diff --git a/runtime/oauth/id_token.go b/runtime/oauth/id_token.go
@@ -66,12 +66,12 @@ func VerifyIdToken(ctx context.Context, idTokenRaw string) (*oidc.IDToken, error
 		return nil, err
 	}
 
-	hasIssuer, err := authConfig.HasOidcIssuer(issuer)
+	providers, err := authConfig.GetProvidersOidcIssuer(issuer)
 	if err != nil {
 		return nil, err
 	}
 
-	if !hasIssuer {
+	if len(providers) == 0 {
 		return nil, fmt.Errorf("issuer %s not registered to authenticate on this server", issuer)
 	}
 
@@ -82,17 +82,25 @@ func VerifyIdToken(ctx context.Context, idTokenRaw string) (*oidc.IDToken, error
 	}
 	span.AddEvent("Provider's ODIC config fetched")
 
-	// TODO: Enable this check once we have the client ID as configurable
-	verifier := provider.Verifier(&oidc.Config{
-		SkipClientIDCheck: true,
-	})
-
-	// Verify that the ID token legitimately was signed by the provider and that it has not expired
-	idToken, err := verifier.Verify(ctx, idTokenRaw)
-	if err != nil {
-		return nil, err
+	var verificationErrs error
+
+	// Verify against each configuired client ID for this issuer
+	for _, p := range providers {
+		// Checking the clientId during verification ensures that the ID token was intended for this client,
+		// because it could have been stolen from any other application with an ID token from this same issuer.
+		oidcConfig := &oidc.Config{
+			ClientID: p.ClientId,
+		}
+
+		// Verify that the ID token legitimately was signed by the provider and that it has not expired
+		idToken, err := provider.Verifier(oidcConfig).Verify(ctx, idTokenRaw)
+		if err != nil {
+			verificationErrs = errors.Join(err, verificationErrs)
+		} else {
+			span.AddEvent("ID Token verified")
+			return idToken, nil
+		}
 	}
-	span.AddEvent("ID Token verified")
 
-	return idToken, nil
+	return nil, verificationErrs
 }