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") }) }