mirror of
https://github.com/better-auth/better-auth.git
synced 2026-10-18 11:32:15 -05:00
fix(sso): allow rotating SAML IdP signing certificate with linked accounts (#11687)
Co-authored-by: Gustavo Valverde <g.valverde02@gmail.com>
This commit is contained in:
co-authored by
Gustavo Valverde
parent
a878f9ab47
commit
182e92bd8a
@@ -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.
|
||||
@@ -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.
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<T>(
|
||||
@@ -213,7 +220,7 @@ function parseConfigSnapshot<T>(
|
||||
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<OIDCConfig>(
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user