diff --git a/config/auth.go b/config/auth.go index 368cf72d5..124188dd9 100644 --- a/config/auth.go +++ b/config/auth.go @@ -50,7 +50,11 @@ 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 @@ -58,13 +62,14 @@ func (c *AuthConfig) HasOidcIssuer(issuer string) (bool, error) { 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 index 537e2f64f..57169b5dd 100644 --- 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 index 5055e9ef9..c23ed9012 100644 --- 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 new file mode 100644 index 000000000..5d35df055 --- /dev/null +++ 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 \ No newline at end of file diff --git a/runtime/apis/authapi/token_endpoint_test.go b/runtime/apis/authapi/token_endpoint_test.go index a696aa770..8a1a95fca 100644 --- 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 index 307dcad89..1650c91ba 100644 --- 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 } diff --git a/runtime/oauth/id_token_test.go b/runtime/oauth/id_token_test.go index 8db68fafb..a861f4bdc 100644 --- a/runtime/oauth/id_token_test.go +++ b/runtime/oauth/id_token_test.go @@ -26,6 +26,7 @@ func TestIdTokenAuth_Valid(t *testing.T) { { Type: config.OpenIdConnectProvider, Name: "my-oidc", + ClientId: "oidc-client-id", IssuerUrl: server.Issuer, }, }, @@ -36,7 +37,52 @@ func TestIdTokenAuth_Valid(t *testing.T) { }) // Get ID token from server - idTokenRaw, err := server.FetchIdToken("id|285620", []string{}) + idTokenRaw, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"}) + require.NoError(t, err) + + idToken, err := oauth.VerifyIdToken(ctx, idTokenRaw) + + require.NoError(t, err) + require.NotNil(t, idToken) +} + +func TestIdTokenAuthMultipleIssuers_Valid(t *testing.T) { + ctx := context.Background() + + // OIDC test server + server, err := oauthtest.NewOIDCServer() + require.NoError(t, err) + + // Set up auth config + ctx = runtimectx.WithOAuthConfig(ctx, &config.AuthConfig{ + Providers: []config.Provider{ + { + Type: config.OpenIdConnectProvider, + Name: "my-oidc", + ClientId: "oidc-client-id-1", + IssuerUrl: server.Issuer, + }, + { + Type: config.OpenIdConnectProvider, + Name: "my-oidc", + ClientId: "oidc-client-id-2", + IssuerUrl: server.Issuer, + }, + { + Type: config.OpenIdConnectProvider, + Name: "my-oidc", + ClientId: "oidc-client-id-3", + IssuerUrl: server.Issuer, + }, + }, + }) + + server.SetUser("id|285620", &oauth.UserClaims{ + Email: "keelson@keel.so", + }) + + // Get ID token from server + idTokenRaw, err := server.FetchIdToken("id|285620", []string{"oidc-client-id-3"}) require.NoError(t, err) idToken, err := oauth.VerifyIdToken(ctx, idTokenRaw) @@ -58,6 +104,7 @@ func TestIdTokenAuth_IncorrectlySigned(t *testing.T) { { Type: config.OpenIdConnectProvider, Name: "my-oidc", + ClientId: "oidc-client-id", IssuerUrl: server.Issuer, }, }, @@ -68,7 +115,7 @@ func TestIdTokenAuth_IncorrectlySigned(t *testing.T) { }) // Get ID token from server - idTokenRaw, err := server.FetchIdToken("id|285620", []string{}) + idTokenRaw, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"}) require.NoError(t, err) // Renew the public set, thus making the ID token invalid @@ -95,6 +142,7 @@ func TestIdTokenAuth_IssuerMismatch(t *testing.T) { { Type: config.OpenIdConnectProvider, Name: "my-oidc", + ClientId: "oidc-client-id", IssuerUrl: server.Issuer, }, }, @@ -107,7 +155,7 @@ func TestIdTokenAuth_IssuerMismatch(t *testing.T) { }) // Get ID token from server - idTokenRaw, err := server.FetchIdToken("id|285620", []string{}) + idTokenRaw, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"}) require.NoError(t, err) server.Config["issuer"] = "http://accounts.google.com" @@ -119,6 +167,61 @@ func TestIdTokenAuth_IssuerMismatch(t *testing.T) { require.Nil(t, idToken) } +func TestIdTokenAuth_ClientIdMismatch(t *testing.T) { + ctx := context.Background() + + // OIDC test server + server, err := oauthtest.NewOIDCServer() + require.NoError(t, err) + + // Set up auth config + ctx = runtimectx.WithOAuthConfig(ctx, &config.AuthConfig{ + Providers: []config.Provider{ + { + Type: config.OpenIdConnectProvider, + Name: "my-oidc-1", + ClientId: "oidc-client-id-1", + IssuerUrl: server.Issuer, + }, + { + Type: config.OpenIdConnectProvider, + Name: "my-oidc-2", + ClientId: "oidc-client-id-2", + IssuerUrl: server.Issuer, + }, + { + Type: config.OpenIdConnectProvider, + Name: "my-oidc-3", + ClientId: "oidc-client-id-3", + IssuerUrl: server.Issuer, + }, + { + Type: config.OpenIdConnectProvider, + Name: "my-oidc-4", + ClientId: "oidc-client-id-4", + IssuerUrl: "https://someother.com", + }, + }, + }) + + server.SetUser("id|285620", &oauth.UserClaims{ + Email: "keelson@keel.so", + }) + + // Get ID token from server + idTokenRaw, err := server.FetchIdToken("id|285620", []string{"different-client-id"}) + require.NoError(t, err) + + idToken, err := oauth.VerifyIdToken(ctx, idTokenRaw) + + require.Error(t, err) + require.Contains(t, err.Error(), "oidc: expected audience \"oidc-client-id-1\" got [\"different-client-id\"]") + require.Contains(t, err.Error(), "oidc: expected audience \"oidc-client-id-2\" got [\"different-client-id\"]") + require.Contains(t, err.Error(), "oidc: expected audience \"oidc-client-id-3\" got [\"different-client-id\"]") + require.NotContains(t, err.Error(), "oidc: expected audience \"oidc-client-id-4\" got [\"different-client-id\"]") + require.Nil(t, idToken) +} + func TestIdTokenAuth_IssuerNotConfigured(t *testing.T) { ctx := context.Background() @@ -134,7 +237,7 @@ func TestIdTokenAuth_IssuerNotConfigured(t *testing.T) { }) // Get ID token from server - idTokenRaw, err := server.FetchIdToken("id|285620", []string{}) + idTokenRaw, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"}) require.NoError(t, err) idToken, err := oauth.VerifyIdToken(ctx, idTokenRaw) @@ -157,6 +260,7 @@ func TestIdTokenAuth_ExpiredIdToken(t *testing.T) { { Type: config.OpenIdConnectProvider, Name: "my-oidc", + ClientId: "oidc-client-id", IssuerUrl: server.Issuer, }, }, @@ -169,7 +273,7 @@ func TestIdTokenAuth_ExpiredIdToken(t *testing.T) { }) // Get ID token from server - idTokenRaw, err := server.FetchIdToken("id|285620", []string{}) + idTokenRaw, err := server.FetchIdToken("id|285620", []string{"oidc-client-id"}) require.NoError(t, err) idToken, err := oauth.VerifyIdToken(ctx, idTokenRaw) diff --git a/runtime/oauth/oauthtest/oidc_test_server.go b/runtime/oauth/oauthtest/oidc_test_server.go index d76ab75fc..295f93cc7 100644 --- a/runtime/oauth/oauthtest/oidc_test_server.go +++ b/runtime/oauth/oauthtest/oidc_test_server.go @@ -21,10 +21,9 @@ type OidcServer struct { Issuer string Config map[string]any IdTokenLifespan time.Duration - - server *httptest.Server - PrivateKey *rsa.PrivateKey - users map[string]*oauth.UserClaims + server *httptest.Server + PrivateKey *rsa.PrivateKey + users map[string]*oauth.UserClaims } func (o *OidcServer) SetUser(sub string, claims *oauth.UserClaims) {