From b88529112e501da77112063950f39eee51df8829 Mon Sep 17 00:00:00 2001 From: Dmitry Makarenko Date: Wed, 30 Sep 2026 18:06:05 +0100 Subject: [PATCH] fix: bind Microsoft native id_token issuer to its tid claim Native Verify took the expected issuer from the token's own iss claim, so the issuer check compared the token with itself. A tenant set as microsoft_tenant was never enforced, and tokens whose iss did not match tid were accepted. Verify now requires a UUID tid and checks iss against https://login.microsoftonline.com//v2.0, as the web flow does. A tenant set as a UUID is pinned (400 on mismatch, and the reason does not show the configured tenant, because this check runs before the signature check). common, organizations, consumers and domain names accept any tenant, so the production config (common) does not change. The unreachable payload-kid issuer lookup (fmt.Println, uncached http.Get on the default client) is deleted. The JWKS fetch now uses the registry HTTP client, because ClientContext is set before NewRemoteKeySet. Claude-Session: https://claude.ai/code/session_0192dFsdvVuQdqMdVxi9HDkz --- .../strategy/oidc/provider_microsoft.go | 113 ++---------- .../strategy/oidc/provider_microsoft_test.go | 165 ++++++++++-------- 2 files changed, 106 insertions(+), 172 deletions(-) diff --git a/selfservice/strategy/oidc/provider_microsoft.go b/selfservice/strategy/oidc/provider_microsoft.go index ca1dcd7fef82..3ac4ccb4a8b6 100644 --- a/selfservice/strategy/oidc/provider_microsoft.go +++ b/selfservice/strategy/oidc/provider_microsoft.go @@ -6,9 +6,6 @@ package oidc import ( "context" "encoding/json" - "fmt" - "io" - "net/http" "net/url" "strings" @@ -148,111 +145,27 @@ func (c *microsoftUnverifiedClaims) Valid() error { } func (p *ProviderMicrosoft) Verify(ctx context.Context, rawIDToken string) (*Claims, error) { - keySet := gooidc.NewRemoteKeySet(ctx, p.JWKSUrl) - ctx = gooidc.ClientContext(ctx, p.reg.HTTPClient(ctx).HTTPClient) - issuer, err := p.extractIssuerFromIDToken(rawIDToken) - if err != nil { - return nil, err - } - - claims, err := verifyToken(ctx, keySet, p.config, rawIDToken, issuer) - if err != nil { - return nil, err - } - - return p.applyObjectIDSubject(claims) -} - -func (p *ProviderMicrosoft) extractIssuerFromIDToken(rawIDToken string) (string, error) { - token, _, err := new(jwt.Parser).ParseUnverified(rawIDToken, jwt.MapClaims{}) - if err != nil { - return "", errors.WithStack(herodot.ErrInternalServerError.WithReasonf("error decoding: %s", err)) - } - - var kid string - var claimsIssuer string - - if claims, ok := token.Claims.(jwt.MapClaims); ok { - if iss, ok := claims["iss"].(string); ok { - claimsIssuer = iss - } else { - claimsIssuer = fmt.Sprintf("https://login.microsoftonline.com/%s/v2.0", p.config.Tenant) - } - if k, ok := claims["kid"].(string); ok { - kid = k - } else { - return claimsIssuer, nil - } - } - fmt.Println(kid) - issuer, err := fetchIssuerFromKid(kid) - if err != nil { - return claimsIssuer, nil - } - - return issuer, nil -} - -func fetchIssuerFromKid(targetKid string) (string, error) { - url := "https://login.microsoftonline.com/common/discovery/v2.0/keys" - jwks, err := fetchMicrosoftDiscoveryKeys(url) - if err != nil { - return "", err - } - - key, err := findKeyByKid(jwks, targetKid) - if err != nil { - return "", err + unverifiedClaims := microsoftUnverifiedClaims{} + if _, _, err := new(jwt.Parser).ParseUnverified(rawIDToken, &unverifiedClaims); err != nil { + return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Unable to decode the Microsoft ID token: %s", err)) } - return key.Iss, nil -} - -type JWK struct { - Kid string `json:"kid"` - Kty string `json:"kty"` - Use string `json:"use"` - Alg string `json:"alg,omitempty"` - X5t string `json:"x5t,omitempty"` - N string `json:"n,omitempty"` - E string `json:"e,omitempty"` - X5c []string `json:"x5c,omitempty"` - Iss string `json:"issuer,omitempty"` -} - -type JWKS struct { - Keys []JWK `json:"keys"` -} - -func fetchMicrosoftDiscoveryKeys(url string) (*JWKS, error) { - resp, err := http.Get(url) + tenantID, err := uuid.FromString(unverifiedClaims.TenantID) if err != nil { - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("faield to fetch JWKS: %s", err)) + return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("TenantID claim is not a valid UUID: %s", err)) } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("unexpected HTTP status: %s", resp.Status)) + // Only a tenant configured as a UUID can be pinned; common, organizations, consumers and domain names accept any tenant. + if configured, err := uuid.FromString(p.config.Tenant); err == nil && configured != tenantID { + return nil, errors.WithStack(herodot.ErrBadRequest.WithReasonf("The Microsoft ID token claims tenant %s, which provider %s does not accept.", tenantID, p.config.ID)) } - body, err := io.ReadAll(resp.Body) + ctx = gooidc.ClientContext(ctx, p.reg.HTTPClient(ctx).HTTPClient) + keySet := gooidc.NewRemoteKeySet(ctx, p.JWKSUrl) + claims, err := verifyToken(ctx, keySet, p.config, rawIDToken, "https://login.microsoftonline.com/"+tenantID.String()+"/v2.0") if err != nil { - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("failed to read JWKS body: %s", err)) - } - - var jwks JWKS - if err := json.Unmarshal(body, &jwks); err != nil { - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("failed to umarshal: %s", err)) + return nil, err } - return &jwks, nil -} - -func findKeyByKid(jwks *JWKS, kid string) (*JWK, error) { - for _, key := range jwks.Keys { - if key.Kid == kid { - return &key, nil - } - } - return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("key with kid %s not found", kid)) + return p.applyObjectIDSubject(claims) } diff --git a/selfservice/strategy/oidc/provider_microsoft_test.go b/selfservice/strategy/oidc/provider_microsoft_test.go index 2441fb0f62f7..207ffc27b4b0 100644 --- a/selfservice/strategy/oidc/provider_microsoft_test.go +++ b/selfservice/strategy/oidc/provider_microsoft_test.go @@ -26,6 +26,10 @@ import ( ) func TestMicrosoftVerify(t *testing.T) { + const tenantA = "a9b86385-f32c-4803-afc8-4b2312fbdf24" + const tenantB = "0b6c6f3e-5d2a-4c1b-9e8f-7a6b5c4d3e2f" + issuerFor := func(tid string) string { return "https://login.microsoftonline.com/" + tid + "/v2.0" } + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) w.Write(publicJWKS) @@ -35,120 +39,137 @@ func TestMicrosoftVerify(t *testing.T) { w.WriteHeader(200) w.Write(publicJWKS2) })) - makeClaims := func(aud string) jwt.RegisteredClaims { - return jwt.RegisteredClaims{ - Issuer: "https://login.microsoftonline.com/tenant_id/v2.0", - Subject: "acme@ory.sh", - Audience: jwt.ClaimStrings{aud}, - ExpiresAt: jwt.NewNumericDate(time.Now().Add(24 * time.Hour)), + makeClaims := func(aud, tid string) *claims { + return &claims{ + RegisteredClaims: &jwt.RegisteredClaims{ + Issuer: issuerFor(tid), + Subject: "acme@ory.sh", + Audience: jwt.ClaimStrings{aud}, + ExpiresAt: jwt.NewNumericDate(time.Now().Add(24 * time.Hour)), + }, + Email: "acme@ory.sh", + TenantID: tid, + } + } + newProvider := func(t *testing.T, config *oidc.Configuration) *oidc.ProviderMicrosoft { + _, reg := internal.NewFastRegistryWithMocks(t) + if config.ClientID == "" { + config.ClientID = "com.example.app" } + p := oidc.NewProviderMicrosoft(config, reg).(*oidc.ProviderMicrosoft) + p.JWKSUrl = ts.URL + return p + } + requireBadRequest := func(t *testing.T, err error, reason string) *herodot.DefaultError { + var herr *herodot.DefaultError + require.ErrorAs(t, err, &herr) + assert.Equal(t, http.StatusBadRequest, herr.StatusCode()) + assert.Contains(t, herr.Reason(), reason) + return herr } + t.Run("case=successful verification", func(t *testing.T) { - _, reg := internal.NewVeryFastRegistryWithoutDB(t) - apple := oidc.NewProviderMicrosoft(&oidc.Configuration{ - ClientID: "com.example.app", - Tenant: "tenant_id_fake", - }, reg).(*oidc.ProviderMicrosoft) - apple.JWKSUrl = ts.URL - token := createIdToken(t, makeClaims("com.example.app")) - - c, err := apple.Verify(context.Background(), token) + c, err := newProvider(t, &oidc.Configuration{Tenant: tenantA}).Verify(context.Background(), signIdToken(t, makeClaims("com.example.app", tenantA))) require.NoError(t, err) assert.Equal(t, "acme@ory.sh", c.Email) assert.Equal(t, "acme@ory.sh", c.Subject) - assert.Equal(t, "https://login.microsoftonline.com/tenant_id/v2.0", c.Issuer) + assert.Equal(t, issuerFor(tenantA), c.Issuer) }) t.Run("case=fails due to client_id mismatch", func(t *testing.T) { - _, reg := internal.NewFastRegistryWithMocks(t) - apple := oidc.NewProviderMicrosoft(&oidc.Configuration{ - ClientID: "com.example.app", - Tenant: "tenant_id", - }, reg).(*oidc.ProviderMicrosoft) - apple.JWKSUrl = ts.URL - token := createIdToken(t, makeClaims("com.different-example.app")) - - _, err := apple.Verify(context.Background(), token) + _, err := newProvider(t, &oidc.Configuration{Tenant: tenantA}).Verify(context.Background(), signIdToken(t, makeClaims("com.different-example.app", tenantA))) require.Error(t, err) assert.Equal(t, `token audience didn't match allowed audiences: [com.example.app] oidc: expected audience "com.example.app" got ["com.different-example.app"]`, err.Error()) }) t.Run("case=fails due to jwks mismatch", func(t *testing.T) { - _, reg := internal.NewFastRegistryWithMocks(t) - apple := oidc.NewProviderMicrosoft(&oidc.Configuration{ - ClientID: "com.example.app", - Tenant: "tenant_id", - }, reg).(*oidc.ProviderMicrosoft) - apple.JWKSUrl = tsOtherJWKS.URL - token := createIdToken(t, makeClaims("com.example.app")) - - _, err := apple.Verify(context.Background(), token) + p := newProvider(t, &oidc.Configuration{Tenant: tenantA}) + p.JWKSUrl = tsOtherJWKS.URL + + _, err := p.Verify(context.Background(), signIdToken(t, makeClaims("com.example.app", tenantA))) require.Error(t, err) assert.Equal(t, "failed to verify signature: failed to verify id token signature", err.Error()) }) t.Run("case=fails due to wrong issuer tenant", func(t *testing.T) { - _, reg := internal.NewFastRegistryWithMocks(t) - apple := oidc.NewProviderMicrosoft(&oidc.Configuration{ - ClientID: "com.example.app", - Tenant: "wrong_tenant_id", - }, reg).(*oidc.ProviderMicrosoft) - apple.JWKSUrl = tsOtherJWKS.URL - token := createIdToken(t, makeClaims("com.example.app")) - - _, err := apple.Verify(context.Background(), token) - require.Error(t, err) - assert.Equal(t, "oidc: id token issued by a different provider, expected \"https://login.microsoftonline.com/wrong_tenant_id/v2.0\" got \"https://login.microsoftonline.com/tenant_id/v2.0\"", err.Error()) + _, err := newProvider(t, &oidc.Configuration{Tenant: tenantB}).Verify(context.Background(), signIdToken(t, makeClaims("com.example.app", tenantA))) + herr := requireBadRequest(t, err, tenantA) + // The pin runs before the signature check, so the reason must not leak the configured tenant. + assert.NotContains(t, herr.Reason(), tenantB) + }) + + for _, tenant := range []string{"common", "organizations", "consumers", "contoso.onmicrosoft.com"} { + t.Run("case=accepts any tenant when tenant is "+tenant, func(t *testing.T) { + c, err := newProvider(t, &oidc.Configuration{Tenant: tenant}).Verify(context.Background(), signIdToken(t, makeClaims("com.example.app", tenantB))) + require.NoError(t, err) + assert.Equal(t, issuerFor(tenantB), c.Issuer) + }) + } + + t.Run("case=fails when issuer does not match the tid claim", func(t *testing.T) { + for _, iss := range []string{issuerFor(tenantB), "https://sts.windows.net/" + tenantA + "/"} { + cl := makeClaims("com.example.app", tenantA) + cl.Issuer = iss + _, err := newProvider(t, &oidc.Configuration{Tenant: "common"}).Verify(context.Background(), signIdToken(t, cl)) + require.Error(t, err) + assert.Contains(t, err.Error(), "oidc: id token issued by a different provider") + } + }) + + t.Run("case=fails when the tid claim is not a valid UUID", func(t *testing.T) { + for _, tid := range []string{"", "tenant_id"} { + _, err := newProvider(t, &oidc.Configuration{Tenant: "common"}).Verify(context.Background(), signIdToken(t, makeClaims("com.example.app", tid))) + requireBadRequest(t, err, "TenantID claim is not a valid UUID") + } + }) + + t.Run("case=fetches the JWKS with the registry HTTP client", func(t *testing.T) { + const jwksURL = "https://jwks.microsoft.test/keys" + _, base := internal.NewFastRegistryWithMocks(t) + reg := &mockRegistry{base, retryablehttp.NewClient()} + httpmock.ActivateNonDefault(reg.cl.HTTPClient) + t.Cleanup(httpmock.DeactivateAndReset) + httpmock.RegisterResponder("GET", jwksURL, httpmock.NewBytesResponder(200, publicJWKS)) + + p := oidc.NewProviderMicrosoft(&oidc.Configuration{ClientID: "com.example.app", Tenant: tenantA}, reg).(*oidc.ProviderMicrosoft) + p.JWKSUrl = jwksURL + _, err := p.Verify(context.Background(), signIdToken(t, makeClaims("com.example.app", tenantA))) + require.NoError(t, err) }) t.Run("case=succeedes with additional id token audience", func(t *testing.T) { - _, reg := internal.NewFastRegistryWithMocks(t) - apple := oidc.NewProviderMicrosoft(&oidc.Configuration{ + _, err := newProvider(t, &oidc.Configuration{ ClientID: "something.else.app", - Tenant: "tenant_id", + Tenant: tenantA, AdditionalIDTokenAudiences: []string{"com.example.app"}, - }, reg).(*oidc.ProviderMicrosoft) - apple.JWKSUrl = ts.URL - token := createIdToken(t, makeClaims("com.example.app")) - - _, err := apple.Verify(context.Background(), token) + }).Verify(context.Background(), signIdToken(t, makeClaims("com.example.app", tenantA))) require.NoError(t, err) }) makeClaimsWithOID := func(oid string) *claims { - cl := makeClaims("com.example.app") - return &claims{RegisteredClaims: &cl, Email: "acme@ory.sh", Object: oid} + cl := makeClaims("com.example.app", tenantA) + cl.Object = oid + return cl } - newProvider := func(t *testing.T, subjectSource string) *oidc.ProviderMicrosoft { - _, reg := internal.NewFastRegistryWithMocks(t) - p := oidc.NewProviderMicrosoft(&oidc.Configuration{ - ClientID: "com.example.app", - Tenant: "tenant_id", - SubjectSource: subjectSource, - }, reg).(*oidc.ProviderMicrosoft) - p.JWKSUrl = ts.URL - return p + newProviderWithSubjectSource := func(t *testing.T, subjectSource string) *oidc.ProviderMicrosoft { + return newProvider(t, &oidc.Configuration{Tenant: tenantA, SubjectSource: subjectSource}) } t.Run("case=uses oid as subject when subject_source is oid", func(t *testing.T) { - c, err := newProvider(t, "oid").Verify(context.Background(), signIdToken(t, makeClaimsWithOID("00000000-0000-0000-0000-00000000c0de"))) + c, err := newProviderWithSubjectSource(t, "oid").Verify(context.Background(), signIdToken(t, makeClaimsWithOID("00000000-0000-0000-0000-00000000c0de"))) require.NoError(t, err) assert.Equal(t, "00000000-0000-0000-0000-00000000c0de", c.Subject) }) t.Run("case=keeps sub as subject when subject_source is default", func(t *testing.T) { - c, err := newProvider(t, "").Verify(context.Background(), signIdToken(t, makeClaimsWithOID("00000000-0000-0000-0000-00000000c0de"))) + c, err := newProviderWithSubjectSource(t, "").Verify(context.Background(), signIdToken(t, makeClaimsWithOID("00000000-0000-0000-0000-00000000c0de"))) require.NoError(t, err) assert.Equal(t, "acme@ory.sh", c.Subject) }) t.Run("case=fails when subject_source is oid and the oid claim is missing", func(t *testing.T) { - _, err := newProvider(t, "oid").Verify(context.Background(), signIdToken(t, makeClaimsWithOID(""))) - require.Error(t, err) - var herr *herodot.DefaultError - require.ErrorAs(t, err, &herr) - assert.Equal(t, http.StatusBadRequest, herr.StatusCode()) - assert.Contains(t, herr.Reason(), "`oid` claim") + _, err := newProviderWithSubjectSource(t, "oid").Verify(context.Background(), signIdToken(t, makeClaimsWithOID(""))) + requireBadRequest(t, err, "`oid` claim") }) }