mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-28 04:37:37 +00:00
🧬 fix: Harden SAML Identity Binding (#15264)
This commit is contained in:
Danny Avila
GitHub
parent
f0eda61638
commit
0d0d7d05bb
8 changed files with 473 additions and 63 deletions
|
|
@ -10,10 +10,12 @@ const {
|
|||
getAvatarFileStrategy,
|
||||
getAvatarSaveParams,
|
||||
resolveAppConfigForUser,
|
||||
resolveSamlSubject,
|
||||
TRANSIENT_SAML_NAME_ID_FORMAT,
|
||||
} = require('@librechat/api');
|
||||
const { getStrategyFunctions } = require('~/server/services/Files/strategies');
|
||||
const { resizeAvatar } = require('~/server/services/Files/images/avatar');
|
||||
const { findUser, createUser, updateUser } = require('~/models');
|
||||
const { findUser, createUser, updateUser, claimSamlIdentity } = require('~/models');
|
||||
const { getAppConfig } = require('~/server/services/Config');
|
||||
const paths = require('~/config/paths');
|
||||
|
||||
|
|
@ -118,9 +120,7 @@ const resizeIdentityProviderAvatar = async (url, userId) => {
|
|||
try {
|
||||
return await resizeAvatar({ userId, input: url });
|
||||
} catch (error) {
|
||||
logger.error(
|
||||
`[samlStrategy] resizeIdentityProviderAvatar: Error processing avatar at URL "${url}": ${error}`,
|
||||
);
|
||||
logger.error('[samlStrategy] Failed to process identity-provider avatar', error);
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
|
@ -133,9 +133,7 @@ const resizeIdentityProviderAvatar = async (url, userId) => {
|
|||
*/
|
||||
function getFullName(profile) {
|
||||
if (process.env.SAML_NAME_CLAIM) {
|
||||
logger.info(
|
||||
`[samlStrategy] Using SAML_NAME_CLAIM: ${process.env.SAML_NAME_CLAIM}, profile: ${profile[process.env.SAML_NAME_CLAIM]}`,
|
||||
);
|
||||
logger.debug(`[samlStrategy] Using SAML_NAME_CLAIM: ${process.env.SAML_NAME_CLAIM}`);
|
||||
return profile[process.env.SAML_NAME_CLAIM];
|
||||
}
|
||||
|
||||
|
|
@ -184,42 +182,41 @@ function convertToUsername(input, defaultValue = '') {
|
|||
function createSamlCallback(existingUsersOnly = false) {
|
||||
return async (profile, done) => {
|
||||
try {
|
||||
logger.info(`[samlStrategy] SAML authentication received for NameID: ${profile.nameID}`);
|
||||
logger.debug('[samlStrategy] SAML profile:', profile);
|
||||
const subject = resolveSamlSubject(profile, process.env.SAML_IDP_ISSUER);
|
||||
if (subject.error) {
|
||||
logger.warn(`[samlStrategy] Rejected SAML subject: ${subject.error}`);
|
||||
return done(null, false, { message: ErrorTypes.AUTH_FAILED });
|
||||
}
|
||||
const { nameID } = subject;
|
||||
logger.info('[samlStrategy] SAML authentication received');
|
||||
|
||||
const userEmail = getEmail(profile) || '';
|
||||
|
||||
const baseConfig = await getAppConfig({ baseOnly: true });
|
||||
if (!isEmailDomainAllowed(userEmail, baseConfig?.registration?.allowedDomains)) {
|
||||
logger.error(
|
||||
`[SAML Strategy] Authentication blocked - email domain not allowed [Email: ${userEmail}]`,
|
||||
'[samlStrategy] Authentication blocked because the email domain is not allowed',
|
||||
);
|
||||
return done(null, false, { message: 'Email domain not allowed' });
|
||||
}
|
||||
|
||||
let user = await findUser({ samlId: profile.nameID });
|
||||
logger.info(
|
||||
`[samlStrategy] User ${user ? 'found' : 'not found'} with SAML ID: ${profile.nameID}`,
|
||||
);
|
||||
let user = await findUser({ samlId: nameID });
|
||||
logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} by SAML identity`);
|
||||
|
||||
if (!user) {
|
||||
user = await findUser({ email: userEmail });
|
||||
logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} with email: ${userEmail}`);
|
||||
logger.info(`[samlStrategy] User ${user ? 'found' : 'not found'} by SAML email claim`);
|
||||
}
|
||||
|
||||
if (user && user.provider !== 'saml') {
|
||||
logger.info(
|
||||
`[samlStrategy] User ${user.email} already exists with provider ${user.provider}`,
|
||||
);
|
||||
logger.info(`[samlStrategy] SAML login conflicts with existing provider: ${user.provider}`);
|
||||
return done(null, false, {
|
||||
message: ErrorTypes.AUTH_FAILED,
|
||||
});
|
||||
}
|
||||
|
||||
if (user?.samlId && user.samlId !== profile.nameID) {
|
||||
logger.warn(
|
||||
`[samlStrategy] Refused SAML login with a different NameID for user: ${user.email}`,
|
||||
);
|
||||
if (user?.samlId && user.samlId !== nameID) {
|
||||
logger.warn('[samlStrategy] Refused SAML login with a different NameID');
|
||||
return done(null, false, {
|
||||
message: ErrorTypes.AUTH_FAILED,
|
||||
});
|
||||
|
|
@ -230,9 +227,7 @@ function createSamlCallback(existingUsersOnly = false) {
|
|||
: baseConfig;
|
||||
|
||||
if (!isEmailDomainAllowed(userEmail, appConfig?.registration?.allowedDomains)) {
|
||||
logger.error(
|
||||
`[SAML Strategy] Authentication blocked - email domain not allowed [Email: ${userEmail}]`,
|
||||
);
|
||||
logger.error('[samlStrategy] Authentication blocked by the tenant email-domain policy');
|
||||
return done(null, false, { message: 'Email domain not allowed' });
|
||||
}
|
||||
|
||||
|
|
@ -244,15 +239,13 @@ function createSamlCallback(existingUsersOnly = false) {
|
|||
|
||||
if (!user) {
|
||||
if (existingUsersOnly) {
|
||||
logger.error(
|
||||
`[samlStrategy] Admin auth blocked - user does not exist [Email: ${userEmail}]`,
|
||||
);
|
||||
logger.error('[samlStrategy] Admin auth blocked because the user does not exist');
|
||||
return done(null, false, { message: 'User does not exist' });
|
||||
}
|
||||
|
||||
user = {
|
||||
provider: 'saml',
|
||||
samlId: profile.nameID,
|
||||
samlId: nameID,
|
||||
username,
|
||||
email: userEmail,
|
||||
emailVerified: true,
|
||||
|
|
@ -261,10 +254,14 @@ function createSamlCallback(existingUsersOnly = false) {
|
|||
const balanceConfig = getBalanceConfig(appConfig);
|
||||
user = await createUser(user, balanceConfig, true, true);
|
||||
} else {
|
||||
user.provider = 'saml';
|
||||
user.samlId = profile.nameID;
|
||||
user.username = username;
|
||||
user.name = fullName;
|
||||
user = await claimSamlIdentity(user._id, nameID, {
|
||||
username,
|
||||
name: fullName,
|
||||
});
|
||||
if (!user) {
|
||||
logger.warn('[samlStrategy] Refused a concurrent SAML identity binding');
|
||||
return done(null, false, { message: ErrorTypes.AUTH_FAILED });
|
||||
}
|
||||
}
|
||||
|
||||
const picture = getPicture(profile);
|
||||
|
|
@ -274,9 +271,9 @@ function createSamlCallback(existingUsersOnly = false) {
|
|||
if (imageBuffer) {
|
||||
let fileName;
|
||||
if (crypto) {
|
||||
fileName = (await hashToken(profile.nameID)) + '.png';
|
||||
fileName = (await hashToken(nameID)) + '.png';
|
||||
} else {
|
||||
fileName = profile.nameID + '.png';
|
||||
fileName = userId + '.png';
|
||||
}
|
||||
|
||||
const fileStrategy = getAvatarFileStrategy(appConfig, process.env.CDN_PROVIDER);
|
||||
|
|
@ -290,22 +287,11 @@ function createSamlCallback(existingUsersOnly = false) {
|
|||
}),
|
||||
);
|
||||
user.avatar = imagePath ?? '';
|
||||
user = await updateUser(user._id, user);
|
||||
}
|
||||
}
|
||||
|
||||
user = await updateUser(user._id, user);
|
||||
|
||||
logger.info(
|
||||
`[samlStrategy] Login success SAML ID: ${user.samlId} | email: ${user.email} | username: ${user.username}`,
|
||||
{
|
||||
user: {
|
||||
samlId: user.samlId,
|
||||
username: user.username,
|
||||
email: user.email,
|
||||
name: user.name,
|
||||
},
|
||||
},
|
||||
);
|
||||
logger.info(`[samlStrategy] Login success for user: ${user._id}`);
|
||||
|
||||
done(null, user);
|
||||
} catch (err) {
|
||||
|
|
@ -320,12 +306,17 @@ function createSamlCallback(existingUsersOnly = false) {
|
|||
* @returns {object} The SAML configuration object.
|
||||
*/
|
||||
function getBaseSamlConfig() {
|
||||
const identifierFormat = process.env.SAML_NAME_ID_FORMAT?.trim();
|
||||
if (identifierFormat === TRANSIENT_SAML_NAME_ID_FORMAT) {
|
||||
throw new Error('SAML_NAME_ID_FORMAT must provide a stable, non-transient identifier');
|
||||
}
|
||||
return {
|
||||
entryPoint: process.env.SAML_ENTRY_POINT,
|
||||
issuer: process.env.SAML_ISSUER,
|
||||
idpCert: getCertificateContent(process.env.SAML_CERT),
|
||||
wantAssertionsSigned: process.env.SAML_USE_AUTHN_RESPONSE_SIGNED === 'true' ? false : true,
|
||||
wantAuthnResponseSigned: process.env.SAML_USE_AUTHN_RESPONSE_SIGNED === 'true' ? true : false,
|
||||
...(identifierFormat ? { identifierFormat } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ jest.mock('~/models', () => ({
|
|||
findUser: jest.fn(),
|
||||
createUser: jest.fn(),
|
||||
updateUser: jest.fn(),
|
||||
claimSamlIdentity: jest.fn(),
|
||||
}));
|
||||
jest.mock('~/server/services/Config', () => ({
|
||||
config: {
|
||||
|
|
@ -45,6 +46,8 @@ jest.mock('@librechat/api', () => ({
|
|||
: params;
|
||||
}),
|
||||
resolveAppConfigForUser: jest.fn(async (_getAppConfig, _user) => ({})),
|
||||
resolveSamlSubject: jest.fn((profile) => ({ nameID: profile.nameID })),
|
||||
TRANSIENT_SAML_NAME_ID_FORMAT: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient',
|
||||
}));
|
||||
jest.mock('~/server/services/Config/EndpointService', () => ({
|
||||
config: {},
|
||||
|
|
@ -77,15 +80,11 @@ jest.mocked(fs).existsSync = jest.fn();
|
|||
jest.mocked(fs).statSync = jest.fn();
|
||||
jest.mocked(fs).readFileSync = jest.fn();
|
||||
|
||||
// To capture the verify callback from the strategy, we grab it from the mock constructor.
|
||||
// setupSaml() registers both 'saml' (regular) and 'samlAdmin' strategies, so we capture
|
||||
// only the first callback per setupSaml() call (the regular one).
|
||||
let verifyCallback;
|
||||
const verifyCallbacks = new Map();
|
||||
SamlStrategy.mockImplementation((options, verify) => {
|
||||
if (!verifyCallback) {
|
||||
verifyCallback = verify;
|
||||
}
|
||||
return { name: 'saml', options, verify };
|
||||
const strategyName = options.callbackUrl?.includes('/api/admin/') ? 'samlAdmin' : 'saml';
|
||||
verifyCallbacks.set(strategyName, verify);
|
||||
return { name: strategyName, options, verify };
|
||||
});
|
||||
|
||||
describe('getCertificateContent', () => {
|
||||
|
|
@ -218,9 +217,9 @@ u7wlOSk+oFzDIO/UILIA
|
|||
|
||||
describe('setupSaml', () => {
|
||||
// Helper to wrap the verify callback in a promise
|
||||
const validate = (profile) =>
|
||||
const validate = (profile, strategyName = 'saml') =>
|
||||
new Promise((resolve, reject) => {
|
||||
verifyCallback(profile, (err, user, details) => {
|
||||
verifyCallbacks.get(strategyName)(profile, (err, user, details) => {
|
||||
if (err) {
|
||||
reject(err);
|
||||
} else {
|
||||
|
|
@ -242,11 +241,10 @@ describe('setupSaml', () => {
|
|||
|
||||
beforeEach(async () => {
|
||||
jest.clearAllMocks();
|
||||
// Reset so the mock captures the regular (non-admin) callback on next setupSaml() call
|
||||
verifyCallback = null;
|
||||
verifyCallbacks.clear();
|
||||
|
||||
// Configure mocks
|
||||
const { findUser, createUser, updateUser } = require('~/models');
|
||||
const { findUser, createUser, updateUser, claimSamlIdentity } = require('~/models');
|
||||
findUser.mockResolvedValue(null);
|
||||
createUser.mockImplementation(async (userData) => ({
|
||||
_id: 'mock-user-id',
|
||||
|
|
@ -256,6 +254,11 @@ describe('setupSaml', () => {
|
|||
_id: id,
|
||||
...userData,
|
||||
}));
|
||||
claimSamlIdentity.mockImplementation(async (id, samlId, userData) => {
|
||||
const result = findUser.mock.results[findUser.mock.results.length - 1];
|
||||
const existingUser = result ? await result.value : {};
|
||||
return { ...existingUser, _id: id, ...userData, samlId };
|
||||
});
|
||||
|
||||
const cert = `
|
||||
-----BEGIN CERTIFICATE-----
|
||||
|
|
@ -292,6 +295,8 @@ u7wlOSk+oFzDIO/UILIA
|
|||
delete process.env.SAML_FAMILY_NAME_CLAIM;
|
||||
delete process.env.SAML_PICTURE_CLAIM;
|
||||
delete process.env.SAML_NAME_CLAIM;
|
||||
delete process.env.SAML_NAME_ID_FORMAT;
|
||||
delete process.env.SAML_IDP_ISSUER;
|
||||
|
||||
resizeAvatar.mockResolvedValue(Buffer.from('safe avatar'));
|
||||
|
||||
|
|
@ -427,8 +432,108 @@ u7wlOSk+oFzDIO/UILIA
|
|||
expect(user.email).toBe(baseProfile.email);
|
||||
});
|
||||
|
||||
it('should preserve a matching NameID binding', async () => {
|
||||
const { findUser, claimSamlIdentity } = require('~/models');
|
||||
const existingUser = {
|
||||
_id: 'existing-user-id',
|
||||
provider: 'saml',
|
||||
email: baseProfile.email,
|
||||
samlId: baseProfile.nameID,
|
||||
};
|
||||
findUser.mockResolvedValueOnce(existingUser);
|
||||
|
||||
const { user } = await validate(baseProfile);
|
||||
|
||||
expect(user.samlId).toBe(baseProfile.nameID);
|
||||
expect(claimSamlIdentity).toHaveBeenCalledWith(
|
||||
existingUser._id,
|
||||
baseProfile.nameID,
|
||||
expect.objectContaining({ username: baseProfile.username }),
|
||||
);
|
||||
});
|
||||
|
||||
it('should atomically bind a legacy SAML account found by email', async () => {
|
||||
const { findUser, claimSamlIdentity } = require('~/models');
|
||||
const existingUser = {
|
||||
_id: 'legacy-user-id',
|
||||
provider: 'saml',
|
||||
email: baseProfile.email,
|
||||
};
|
||||
findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser);
|
||||
|
||||
const { user } = await validate(baseProfile);
|
||||
|
||||
expect(user.samlId).toBe(baseProfile.nameID);
|
||||
expect(claimSamlIdentity).toHaveBeenCalledWith(
|
||||
existingUser._id,
|
||||
baseProfile.nameID,
|
||||
expect.objectContaining({ username: baseProfile.username }),
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject a concurrent first-time binding that loses the atomic claim', async () => {
|
||||
const { findUser, updateUser, claimSamlIdentity } = require('~/models');
|
||||
const existingUser = {
|
||||
_id: 'legacy-user-id',
|
||||
provider: 'saml',
|
||||
email: baseProfile.email,
|
||||
};
|
||||
findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser);
|
||||
claimSamlIdentity.mockResolvedValueOnce(null);
|
||||
|
||||
const result = await validate(baseProfile);
|
||||
|
||||
expect(result.user).toBe(false);
|
||||
expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED);
|
||||
expect(updateUser).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([undefined, '', ' '])('should reject an invalid NameID value: %p', async (nameID) => {
|
||||
const { findUser, claimSamlIdentity } = require('~/models');
|
||||
const { resolveSamlSubject } = require('@librechat/api');
|
||||
resolveSamlSubject.mockReturnValueOnce({ error: 'missing_name_id' });
|
||||
|
||||
const result = await validate({ ...baseProfile, nameID });
|
||||
|
||||
expect(result.user).toBe(false);
|
||||
expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED);
|
||||
expect(findUser).not.toHaveBeenCalled();
|
||||
expect(claimSamlIdentity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should reject a transient NameID', async () => {
|
||||
const { findUser } = require('~/models');
|
||||
const { resolveSamlSubject } = require('@librechat/api');
|
||||
resolveSamlSubject.mockReturnValueOnce({ error: 'transient_name_id' });
|
||||
const result = await validate({
|
||||
...baseProfile,
|
||||
nameIDFormat: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient',
|
||||
});
|
||||
|
||||
expect(result.user).toBe(false);
|
||||
expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED);
|
||||
expect(findUser).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should reject an assertion from a different IdP issuer when configured', async () => {
|
||||
const { findUser } = require('~/models');
|
||||
const { resolveSamlSubject } = require('@librechat/api');
|
||||
resolveSamlSubject.mockReturnValueOnce({ error: 'issuer_mismatch' });
|
||||
process.env.SAML_IDP_ISSUER = 'https://idp.example.com';
|
||||
|
||||
const result = await validate({ ...baseProfile, issuer: 'https://other-idp.example.com' });
|
||||
|
||||
expect(result.user).toBe(false);
|
||||
expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED);
|
||||
expect(findUser).not.toHaveBeenCalled();
|
||||
expect(resolveSamlSubject).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ issuer: 'https://other-idp.example.com' }),
|
||||
'https://idp.example.com',
|
||||
);
|
||||
});
|
||||
|
||||
it('should reject an email match bound to a different NameID', async () => {
|
||||
const { findUser, updateUser } = require('~/models');
|
||||
const { findUser, updateUser, claimSamlIdentity } = require('~/models');
|
||||
const existingUser = {
|
||||
_id: 'existing-user-id',
|
||||
provider: 'saml',
|
||||
|
|
@ -442,6 +547,24 @@ u7wlOSk+oFzDIO/UILIA
|
|||
expect(result.user).toBe(false);
|
||||
expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED);
|
||||
expect(updateUser).not.toHaveBeenCalled();
|
||||
expect(claimSamlIdentity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should enforce the NameID binding for the admin SAML callback', async () => {
|
||||
const { findUser, claimSamlIdentity } = require('~/models');
|
||||
const existingUser = {
|
||||
_id: 'existing-admin-id',
|
||||
provider: 'saml',
|
||||
email: baseProfile.email,
|
||||
samlId: 'original-name-id',
|
||||
};
|
||||
findUser.mockResolvedValueOnce(null).mockResolvedValueOnce(existingUser);
|
||||
|
||||
const result = await validate(baseProfile, 'samlAdmin');
|
||||
|
||||
expect(result.user).toBe(false);
|
||||
expect(result.details.message).toBe(require('librechat-data-provider').ErrorTypes.AUTH_FAILED);
|
||||
expect(claimSamlIdentity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should block login when email exists with different provider', async () => {
|
||||
|
|
@ -504,7 +627,6 @@ u7wlOSk+oFzDIO/UILIA
|
|||
|
||||
it('uses the configured SAML picture claim for shared avatar processing', async () => {
|
||||
process.env.SAML_PICTURE_CLAIM = 'avatar_url';
|
||||
verifyCallback = null;
|
||||
await setupSaml();
|
||||
|
||||
const profile = {
|
||||
|
|
@ -522,6 +644,54 @@ u7wlOSk+oFzDIO/UILIA
|
|||
expect(fetch).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should pass the configured NameID format to both SAML strategies', async () => {
|
||||
process.env.SAML_NAME_ID_FORMAT = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent';
|
||||
process.env.SAML_IDP_ISSUER = 'https://idp.example.com';
|
||||
|
||||
await setupSaml();
|
||||
|
||||
const calls = SamlStrategy.mock.calls.slice(-2);
|
||||
for (const [options] of calls) {
|
||||
expect(options).toEqual(
|
||||
expect.objectContaining({
|
||||
identifierFormat: 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent',
|
||||
}),
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
it('should refuse to configure a transient NameID format', async () => {
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
process.env.SAML_NAME_ID_FORMAT = 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient';
|
||||
const callCount = SamlStrategy.mock.calls.length;
|
||||
|
||||
await setupSaml();
|
||||
|
||||
expect(SamlStrategy).toHaveBeenCalledTimes(callCount);
|
||||
expect(logger.error).toHaveBeenCalledWith(
|
||||
'[samlStrategy]',
|
||||
expect.objectContaining({
|
||||
message: 'SAML_NAME_ID_FORMAT must provide a stable, non-transient identifier',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should not log raw NameID or profile attributes', async () => {
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const sensitiveValue = 'sensitive-profile-attribute';
|
||||
|
||||
await validate({ ...baseProfile, sensitiveAttribute: sensitiveValue });
|
||||
|
||||
const logOutput = JSON.stringify([
|
||||
...logger.info.mock.calls,
|
||||
...logger.debug.mock.calls,
|
||||
...logger.warn.mock.calls,
|
||||
...logger.error.mock.calls,
|
||||
]);
|
||||
expect(logOutput).not.toContain(baseProfile.nameID);
|
||||
expect(logOutput).not.toContain(sensitiveValue);
|
||||
});
|
||||
|
||||
it('should save CloudFront SAML avatars under the shared avatar prefix', async () => {
|
||||
const { getStrategyFunctions } = require('~/server/services/Files/strategies');
|
||||
getAppConfig.mockResolvedValueOnce({ fileStrategies: { avatar: FileSources.cloudfront } });
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue