diff --git a/.changeset/sso-saml-cert-rotation.md b/.changeset/sso-saml-cert-rotation.md new file mode 100644 index 0000000000..ad09215e09 --- /dev/null +++ b/.changeset/sso-saml-cert-rotation.md @@ -0,0 +1,5 @@ +--- +"@better-auth/sso": patch +--- + +SAML providers that already have linked accounts can rotate their IdP signing certificate again. Since 1.7.0, changing `cert` or `idpMetadata.cert` through `updateSSOProvider` returned 409 `Cannot change SSO provider identity fields while linked accounts exist`, so the only way to rotate was to delete the provider, which also removed its account links and domain verification. `guardProviderMutation` still receives certificate changes as authentication-boundary changes, so a guard that rejects those keeps blocking them. diff --git a/docs/content/docs/plugins/sso.mdx b/docs/content/docs/plugins/sso.mdx index 0d59f29a8b..431a092187 100644 --- a/docs/content/docs/plugins/sso.mdx +++ b/docs/content/docs/plugins/sso.mdx @@ -1101,6 +1101,8 @@ samlConfig: { Both `samlConfig.cert` and `samlConfig.idpMetadata.cert` accept either a single PEM string or an array. When both are set, `idpMetadata.cert` wins. +Changing the certificates keeps the accounts already linked to the provider. Whoever holds the corresponding private key for a listed certificate can sign in as any of those users, so if organization admins should not be able to change certificates on their own, reject authentication-boundary changes in `guardProviderMutation`. + If your IdP publishes a metadata XML document, pass it as `idpMetadata.metadata` instead and leave `cert` unset. The certificates come from the document and rotation happens when the IdP republishes it. If a config supplies neither `cert` nor `idpMetadata.metadata`, registration fails with `CERT_SOURCE_MISSING`, since samlify has nothing to verify SAML responses against. Applications can use `guardProviderMutation` to coordinate SSO changes with a paired directory-sync control plane. IdP entity IDs, endpoints, signing certificates, SP entity IDs, NameID formats, POST assertion-consumer URLs, and assertion-signing policy are authentication-boundary changes. If the guard blocks authentication-boundary changes while a directory is paired, unpair the directory, update the IdP trust configuration, and then re-pair it. OIDC client-secret rotation and SAML SP private-key rotation do not change the authentication boundary. diff --git a/packages/sso/src/provider-lock.test.ts b/packages/sso/src/provider-lock.test.ts index 2c04d6e213..604a7894e2 100644 --- a/packages/sso/src/provider-lock.test.ts +++ b/packages/sso/src/provider-lock.test.ts @@ -50,4 +50,40 @@ describe("SSO provider account-link locking", () => { expect(before).not.toHaveBeenCalled(); expect(after).not.toHaveBeenCalled(); }); + + /** + * @see https://github.com/better-auth/better-auth/issues/11471 + */ + it("rejects a sign-in whose SAML signing certificates changed before the lock", async () => { + const samlConfig = (cert: string) => ({ + issuer: "https://sp.example.com", + entryPoint: "https://idp.example.com/sso", + cert, + callbackUrl: "https://sp.example.com/dashboard", + idpMetadata: { entityID: "https://idp.example.com" }, + spMetadata: {}, + }); + const provider = { + id: "provider-row-1", + providerId: "workforce", + issuer: "https://idp.example.com", + userId: "owner", + }; + const update = vi.fn(async () => ({ + ...provider, + samlConfig: JSON.stringify(samlConfig("rotated-signing-certificate")), + })); + const context = { + adapter: { update } as unknown as AuthContext["adapter"], + } as AuthContext; + + await expect( + lockSSOProviderForAccountLink( + { context }, + { ...provider, samlConfig: samlConfig("original-signing-certificate") }, + ), + ).rejects.toThrow( + "SSO provider changed while account linking was in progress", + ); + }); }); diff --git a/packages/sso/src/providers.test.ts b/packages/sso/src/providers.test.ts index 79838dd36f..dba1a2a0fd 100644 --- a/packages/sso/src/providers.test.ts +++ b/packages/sso/src/providers.test.ts @@ -2435,6 +2435,42 @@ kBGIJYs= expect(updated.oidcConfig?.clientIdLastFour).toBe("****t123"); }); + /** + * @see https://github.com/better-auth/better-auth/issues/11471 + */ + it("allows SAML signing-certificate rotation when linked account rows exist", async () => { + const { auth, getAuthHeaders, registerSAMLProvider, data } = + createTestAuth(false); + const headers = await getAuthHeaders({ + email: "owner@example.com", + password: "password123", + name: "Owner", + }); + await registerSAMLProvider(headers, "my-saml-provider"); + const user = (data.user as { id: string; email: string }[]).find( + (u) => u.email === "owner@example.com", + ); + data.account.push({ + id: "linked-saml-account", + userId: user!.id, + providerId: "my-saml-provider", + accountId: "saml-account-id", + }); + + for (const samlConfig of [ + { cert: [TEST_CERT, "rotated-signing-certificate"] }, + { cert: "rotated-signing-certificate" }, + { idpMetadata: { cert: "next-signing-certificate" } }, + ]) { + const response = await auth.api.updateSSOProvider({ + body: { providerId: "my-saml-provider", samlConfig }, + headers, + asResponse: true, + }); + expect(response.status).toBe(200); + } + }); + it("allows OIDC client secret updates when linked account rows exist", async () => { const { auth, getAuthHeaders, createOIDCProviderData, data } = createTestAuth(false); diff --git a/packages/sso/src/routes/providers.ts b/packages/sso/src/routes/providers.ts index d5184252ae..da2a70a1f0 100644 --- a/packages/sso/src/routes/providers.ts +++ b/packages/sso/src/routes/providers.ts @@ -126,10 +126,16 @@ function oidcIdentityBoundaryChanged( return hasChangedField(current, updated, OIDC_IDENTITY_BOUNDARY_FIELDS); } -function samlIdentityBoundaryChanged( +/** + * Signing certificates are reported apart from the identity boundary: they + * change who can sign assertions, so the mutation guard treats them as an + * authentication-boundary change, but they do not change which IdP identities + * linked accounts belong to. + */ +function samlBoundaryChanges( current: SAMLConfig, updated: SAMLConfig, -): boolean { +): { identity: boolean; trustAnchors: boolean } { const currentIdentityProvider = createIdP(current).entityMeta; const updatedIdentityProvider = createIdP(updated).entityMeta; let currentServiceProvider: ReturnType< @@ -146,7 +152,7 @@ function samlIdentityBoundaryChanged( ? parseSAMLServiceProviderMetadata(updated.spMetadata.metadata) : null; } catch { - return true; + return { identity: true, trustAnchors: true }; } const effectiveIdentityBoundary = (config: SAMLConfig) => { const identityProvider = @@ -187,17 +193,18 @@ function samlIdentityBoundaryChanged( .map((certificate) => normalizePem(certificate) ?? certificate) .sort(); }; - return ( - hasChangedField(current, updated, SAML_IDENTITY_BOUNDARY_FIELDS) || - identityValueChanged( + return { + identity: + hasChangedField(current, updated, SAML_IDENTITY_BOUNDARY_FIELDS) || + identityValueChanged( + effectiveIdentityBoundary(current), + effectiveIdentityBoundary(updated), + ), + trustAnchors: identityValueChanged( trustAnchors(currentIdentityProvider), trustAnchors(updatedIdentityProvider), - ) || - identityValueChanged( - effectiveIdentityBoundary(current), - effectiveIdentityBoundary(updated), - ) - ); + ), + }; } function parseConfigSnapshot( @@ -213,7 +220,7 @@ function parseConfigSnapshot( return config; } -function ssoProviderIdentityBoundaryChanged( +function ssoProviderAuthBoundaryChanged( current: ProviderIdentitySnapshot, updated: ProviderIdentitySnapshot, ): boolean { @@ -229,12 +236,17 @@ function ssoProviderIdentityBoundaryChanged( updated.samlConfig, "SAML", ); - if ( - currentSamlConfig && - (!updatedSamlConfig || - samlIdentityBoundaryChanged(currentSamlConfig, updatedSamlConfig)) - ) { - return true; + if (currentSamlConfig) { + if (!updatedSamlConfig) { + return true; + } + // An unchanged config skips the metadata parses on every sign-in. + if (identityValueChanged(currentSamlConfig, updatedSamlConfig)) { + const changes = samlBoundaryChanges(currentSamlConfig, updatedSamlConfig); + if (changes.identity || changes.trustAnchors) { + return true; + } + } } const currentOidcConfig = parseConfigSnapshot( @@ -352,7 +364,7 @@ export async function lockSSOProviderForAccountLink( }); } - if (ssoProviderIdentityBoundaryChanged(provider, lockedProvider)) { + if (ssoProviderAuthBoundaryChanged(provider, lockedProvider)) { throw new APIError("CONFLICT", { code: "SSO_PROVIDER_CHANGED", message: "SSO provider changed while account linking was in progress", @@ -882,6 +894,7 @@ export const updateSSOProvider = (options: SSOOptions) => { let providerIdentityBoundaryChanged = body.issuer !== undefined && body.issuer !== existingProvider.issuer; + let trustAnchorsChanged = false; if (body.issuer !== undefined) { updateData.issuer = body.issuer; @@ -937,11 +950,14 @@ export const updateSSOProvider = (options: SSOOptions) => { validateCertSources(updatedSamlConfig); assertSAMLIdentityProviderAuthority(updatedSamlConfig); assertSAMLServiceProviderMetadataPolicy(updatedSamlConfig); - if ( - samlIdentityBoundaryChanged(currentSamlConfig, updatedSamlConfig) - ) { + const changes = samlBoundaryChanges( + currentSamlConfig, + updatedSamlConfig, + ); + if (changes.identity) { providerIdentityBoundaryChanged = true; } + trustAnchorsChanged = changes.trustAnchors; updateData.samlConfig = JSON.stringify(updatedSamlConfig); } @@ -1013,7 +1029,8 @@ export const updateSSOProvider = (options: SSOOptions) => { options, { action: "update", - isAuthenticationBoundaryChange: providerIdentityBoundaryChanged, + isAuthenticationBoundaryChange: + providerIdentityBoundaryChanged || trustAnchorsChanged, }, existingProvider, trx,