diff --git a/CHANGES b/CHANGES index ecf335d1e..e590ba0f6 100644 --- a/CHANGES +++ b/CHANGES @@ -2729,4 +2729,5 @@ * mail: update limit plugin * ldap: fix error messages to show proper error messages in the external LDAP connector * dashboard: fix various UI elements hidden for admin user +* directoryserver: fix totp validation diff --git a/src/directoryserver.js b/src/directoryserver.js index 3f20eca8d..525c9e805 100644 --- a/src/directoryserver.js +++ b/src/directoryserver.js @@ -305,7 +305,7 @@ async function userAuth(req, res, next) { // totp check is currently requested by the client. this is the only way to auth against external cloudron dashboard, external cloudron app and external apps const TOTPTOKEN_ATTRIBUTE_NAME = 'totptoken'; // This has to be in-sync with externalldap.js const totpToken = TOTPTOKEN_ATTRIBUTE_NAME in req.dn.rdns[0].attrs ? req.dn.rdns[0].attrs[TOTPTOKEN_ATTRIBUTE_NAME].value : null; - const relaxedTotpCheck = !(TOTPTOKEN_ATTRIBUTE_NAME in req.dn.rdns[0].attrs); + const skipTotpCheck = !(TOTPTOKEN_ATTRIBUTE_NAME in req.dn.rdns[0].attrs); let verifyFunc; if (cnAttributeName === 'mail') { @@ -318,7 +318,7 @@ async function userAuth(req, res, next) { verifyFunc = users.verifyWithUsername; } - const [error, user] = await safe(verifyFunc(commonName, req.credentials || '', '', { totpToken, relaxedTotpCheck })); + const [error, user] = await safe(verifyFunc(commonName, req.credentials || '', '', { totpToken, skipTotpCheck })); if (error && error.reason === BoxError.NOT_FOUND) return next(new ldap.NoSuchObjectError(error.message)); if (error && error.reason === BoxError.INVALID_CREDENTIALS) return next(new ldap.InvalidCredentialsError(error.message)); if (error) return next(new ldap.OperationsError(error.message)); diff --git a/src/externalldap.js b/src/externalldap.js index bd9414040..b8f527230 100644 --- a/src/externalldap.js +++ b/src/externalldap.js @@ -278,23 +278,23 @@ async function maybeCreateUser(identifier) { return await users.add(user.email, { username: user.username, password: null, displayName: user.displayName, source: 'ldap' }, AuditSource.EXTERNAL_LDAP); } -async function verifyPassword(user, password, totpToken) { - assert.strictEqual(typeof user, 'object'); +async function verifyPassword(username, password, totpToken) { + assert.strictEqual(typeof username, 'string'); assert.strictEqual(typeof password, 'string'); assert(totpToken === null || typeof totpToken === 'string'); const config = await getConfig(); if (config.provider === 'noop') throw new BoxError(BoxError.BAD_STATE, 'not enabled'); - const ldapUsers = await ldapUserSearch(config, { filter: `${config.usernameField}=${user.username}` }); + const ldapUsers = await ldapUserSearch(config, { filter: `${config.usernameField}=${username}` }); if (ldapUsers.length === 0) throw new BoxError(BoxError.NOT_FOUND); if (ldapUsers.length > 1) throw new BoxError(BoxError.CONFLICT); const client = await getClient(config, { bind: false }); let userAuthDn; - if (totpToken) { - // inject totptoken into first attribute + if (totpToken !== null) { + // inject totptoken into first attribute. in ldap, '+' is the attribute separator in a RDNS const rdns = ldapUsers[0].dn.split(','); userAuthDn = `${rdns[0]}+totptoken=${totpToken},` + rdns.slice(1).join(','); } else { diff --git a/src/ldapserver.js b/src/ldapserver.js index 8f668f9e7..0454b96ae 100644 --- a/src/ldapserver.js +++ b/src/ldapserver.js @@ -68,7 +68,7 @@ async function userAuthInternal(appId, req, res, next) { verifyFunc = users.verifyWithUsername; } - const [error, user] = await safe(verifyFunc(commonName, req.credentials || '', appId || '', { relaxedTotpCheck: true, totpToken })); + const [error, user] = await safe(verifyFunc(commonName, req.credentials || '', appId || '', { skipTotpCheck: true, totpToken })); if (error && error.reason === BoxError.NOT_FOUND) return next(new ldap.NoSuchObjectError(error.message)); if (error && error.reason === BoxError.INVALID_CREDENTIALS) return next(new ldap.InvalidCredentialsError(error.message)); if (error) return next(new ldap.OperationsError(error.message)); @@ -470,13 +470,13 @@ async function verifyMailboxPassword(mailbox, password) { assert.strictEqual(typeof password, 'string'); if (mailbox.ownerType === mail.OWNERTYPE_USER) { - return await users.verify(mailbox.ownerId, password, users.AP_MAIL /* identifier */, { relaxedTotpCheck: true }); + return await users.verify(mailbox.ownerId, password, users.AP_MAIL /* identifier */, { skipTotpCheck: true }); } else if (mailbox.ownerType === mail.OWNERTYPE_GROUP) { const userIds = await groups.getMembers(mailbox.ownerId); let verifiedUser = null; for (const userId of userIds) { - const [error, result] = await safe(users.verify(userId, password, users.AP_MAIL /* identifier */, { relaxedTotpCheck: true })); + const [error, result] = await safe(users.verify(userId, password, users.AP_MAIL /* identifier */, { skipTotpCheck: true })); if (error) continue; // try the next user verifiedUser = result; break; // found a matching validated user @@ -501,7 +501,7 @@ async function authenticateSftp(req, res, next) { let [error, app] = await safe(apps.getByFqdn(parts[1])); if (error || !app) return next(new ldap.InvalidCredentialsError()); - [error] = await safe(users.verifyWithUsername(parts[0], req.credentials, app.id, { relaxedTotpCheck: true })); + [error] = await safe(users.verifyWithUsername(parts[0], req.credentials, app.id, { skipTotpCheck: true })); if (error) return next(new ldap.InvalidCredentialsError(error.message)); debug('sftp auth: success'); diff --git a/src/proxyauth.js b/src/proxyauth.js index 84061f9ae..d368e0ae8 100644 --- a/src/proxyauth.js +++ b/src/proxyauth.js @@ -68,7 +68,7 @@ async function authorizationHeader(req, res, next) { if (!app.manifest.addons.proxyAuth.basicAuth) return next(); // this is a flag because this allows auth to bypass 2FA const verifyFunc = credentials.name.indexOf('@') !== -1 ? users.verifyWithEmail : users.verifyWithUsername; - const [verifyError, user] = await safe(verifyFunc(credentials.name, credentials.pass, appId, { relaxedTotpCheck: true })); + const [verifyError, user] = await safe(verifyFunc(credentials.name, credentials.pass, appId, { skipTotpCheck: true })); if (verifyError) return next(new HttpError(403, 'Invalid username or password' )); req.user = user; diff --git a/src/routes/profile.js b/src/routes/profile.js index d13a9368a..5409675bf 100644 --- a/src/routes/profile.js +++ b/src/routes/profile.js @@ -74,7 +74,7 @@ async function update(req, res, next) { if (data.fallbackEmail) { if (!req.body.password || typeof req.body.password !== 'string') return next(new HttpError(400, 'password must be non empty string')); - const [verifyError] = await safe(users.verify(req.user.id, req.body.password, users.AP_WEBADMIN, { relaxedTotpCheck: true })); + const [verifyError] = await safe(users.verify(req.user.id, req.body.password, users.AP_WEBADMIN, { skipTotpCheck: true })); // just check password if (verifyError) return next(BoxError.toHttpError(verifyError)); req.body.password = ''; // this will prevent logs from displaying plain text password diff --git a/src/routes/users.js b/src/routes/users.js index 39cf7d624..2cb86a55e 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -145,7 +145,7 @@ async function verifyPassword(req, res, next) { if (typeof req.body.password !== 'string') return next(new HttpError(400, 'API call requires user password')); - const [error] = await safe(users.verify(req.user.id, req.body.password, users.AP_WEBADMIN, { relaxedTotpCheck: true })); + const [error] = await safe(users.verify(req.user.id, req.body.password, users.AP_WEBADMIN, { skipTotpCheck: true })); if (error) return next(BoxError.toHttpError(error)); req.body.password = ''; // this will prevent logs from displaying plain text password diff --git a/src/test/externalldap-test.js b/src/test/externalldap-test.js index 8b818fdd0..f7744c021 100644 --- a/src/test/externalldap-test.js +++ b/src/test/externalldap-test.js @@ -20,6 +20,7 @@ const async = require('async'), let gLdapServer; const LDAP_SHARED_PASSWORD = 'validpassword'; +const LDAP_SHARED_TOTP_TOKEN = '1234'; const LDAP_PORT = 4321; const LDAP_BASE_DN = 'ou=Users,dc=cloudron,dc=io'; const LDAP_GROUP_BASE_DN = 'ou=Groups,dc=cloudron,dc=io'; @@ -111,7 +112,7 @@ function startLdapServer(callback) { const obj = { dn: dn.toString(), attributes: { - objectclass: [ 'inetOrgPerson' ], + objectclass: [ 'inetorgperson' ], mail: entry.email, cn: entry.displayName } @@ -156,11 +157,20 @@ function startLdapServer(callback) { // extract the common name which might have different attribute names const attributeName = Object.keys(req.dn.rdns[0].attrs)[0]; const commonName = req.dn.rdns[0].attrs[attributeName].value; - if (!commonName) return next(new ldap.NoSuchObjectError(req.dn.toString())); + if (!commonName) return next(new ldap.NoSuchObjectError('Missing CN')); - if (!gLdapUsers.find(function (u) { return u.username === commonName; })) return next(new ldap.NoSuchObjectError(req.dn.toString())); - if (req.credentials !== LDAP_SHARED_PASSWORD) return next(new ldap.InvalidCredentialsError(req.dn.toString())); + // this code here is only for completeness. none of the apps send totptoken + const TOTPTOKEN_ATTRIBUTE_NAME = 'totptoken'; + const totpToken = TOTPTOKEN_ATTRIBUTE_NAME in req.dn.rdns[0].attrs ? req.dn.rdns[0].attrs[TOTPTOKEN_ATTRIBUTE_NAME].value : null; + const skipTotpCheck = !(TOTPTOKEN_ATTRIBUTE_NAME in req.dn.rdns[0].attrs); + const u = gLdapUsers.find(function (u) { return u.username === commonName; }); + if (!u) return next(new ldap.NoSuchObjectError('No such user')); + if (req.credentials !== LDAP_SHARED_PASSWORD) return next(new ldap.InvalidCredentialsError('Bad password')); + if (!skipTotpCheck && u.has2fa) { + if (!totpToken) return next(new ldap.InvalidCredentialsError('totpToken is required')); + if (totpToken !== LDAP_SHARED_TOTP_TOKEN) return next(new ldap.InvalidCredentialsError('Invalid totpToken')); + } res.end(); }); @@ -309,7 +319,61 @@ describe('External LDAP', function () { }); }); + describe('verifyPassword', function () { + before(async function () { + await externalLdap.setConfig(LDAP_CONFIG); + + gLdapUsers = [{ + username: 'maximus', + displayName: 'First User', + email: 'first@user.com' + }]; + }); + + it('fails for unknown user', async function () { + const [error] = await safe(externalLdap.verifyPassword('badusername', 'badpassword', null)); + expect(error.reason).to.be(BoxError.NOT_FOUND); + }); + + it('fails for bad password', async function () { + const [error] = await safe(externalLdap.verifyPassword('maximus', 'badpassword', null)); + expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); + }); + + it('succeeds for valid password', async function () { + const u = await externalLdap.verifyPassword('maximus', LDAP_SHARED_PASSWORD, null); + expect(u.username).to.be('maximus'); + }); + + it('enable totp', () => gLdapUsers[0].has2fa = true); + + it('succeeds when totp required and no totp', async function () { + const u = await externalLdap.verifyPassword('maximus', LDAP_SHARED_PASSWORD, null); + expect(u.username).to.be('maximus'); + }); + + it('fails when totp required and empty totp', async function () { + const [error] = await safe(externalLdap.verifyPassword('maximus', LDAP_SHARED_PASSWORD, '')); + expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); + }); + + it('fails when totp required and bad totp', async function () { + const [error] = await safe(externalLdap.verifyPassword('maximus', LDAP_SHARED_PASSWORD, 'badtotp')); + expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); + }); + + it('succeeds when totp required and good totp', async function () { + const u = await externalLdap.verifyPassword('maximus', LDAP_SHARED_PASSWORD, LDAP_SHARED_TOTP_TOKEN); + expect(u.username).to.be('maximus'); + }); + }); + describe('sync', function () { + before(function () { + gLdapUsers = []; + gLdapGroups = []; + }); + it('disable sync', async function () { await externalLdap.setConfig({ provider: 'noop' }); }); diff --git a/src/test/users-test.js b/src/test/users-test.js index 219d3cfb0..8dc4c332d 100644 --- a/src/test/users-test.js +++ b/src/test/users-test.js @@ -427,14 +427,13 @@ describe('User', function () { }); it('verify succeeds with relaxed 2fa', async function () { - const user = await users.verifyWithUsername(admin.username, admin.password, users.AP_WEBADMIN, { relaxedTotpCheck: true }); + const user = await users.verifyWithUsername(admin.username, admin.password, users.AP_WEBADMIN, { skipTotpCheck: true }); expect(user.id).to.be(admin.id); }); - it('verify fails with relaxed 2fa but incorrect totp', async function () { - const [error] = await safe(users.verifyWithUsername(admin.username, admin.password, users.AP_WEBADMIN, { totpToken: 'schlecht', relaxedTotpCheck: true })); - expect(error.reason).to.equal(BoxError.INVALID_CREDENTIALS); - expect(error.message).to.be('Invalid totpToken'); + it('verify succeeds with relaxed 2fa but incorrect totp (totp is ignored)', async function () { + const user = await users.verifyWithUsername(admin.username, admin.password, users.AP_WEBADMIN, { totpToken: 'schlecht', skipTotpCheck: true }); + expect(user.id).to.be(admin.id); }); it('verify succeeds with valid 2fa', async function () { diff --git a/src/users.js b/src/users.js index 36daf14e2..bf31a9187 100644 --- a/src/users.js +++ b/src/users.js @@ -360,11 +360,11 @@ async function verify(userId, password, identifier, options) { return user; } - const relaxedTotpCheck = !!options.relaxedTotpCheck; // will enforce totp only if totpToken is valid + const skipTotpCheck = !!options.skipTotpCheck; const totpToken = options.totpToken || null; if (user.source === 'ldap') { - await externalLdap.verifyPassword(user, password, totpToken); + await externalLdap.verifyPassword(user.username, password, totpToken); } else { const saltBinary = Buffer.from(user.salt, 'hex'); const [error, derivedKey] = await safe(pbkdf2Async(password, saltBinary, CRYPTO_ITERATIONS, CRYPTO_KEY_LENGTH, CRYPTO_DIGEST)); @@ -373,13 +373,10 @@ async function verify(userId, password, identifier, options) { const derivedKeyHex = Buffer.from(derivedKey, 'binary').toString('hex'); if (derivedKeyHex !== user.password) throw new BoxError(BoxError.INVALID_CREDENTIALS, 'Username and password does not match'); - if (user.twoFactorAuthenticationEnabled) { - if (totpToken) { - const verified = speakeasy.totp.verify({ secret: user.twoFactorAuthenticationSecret, encoding: 'base32', token: totpToken, window: 2 }); - if (!verified) throw new BoxError(BoxError.INVALID_CREDENTIALS, 'Invalid totpToken'); - } else if (!relaxedTotpCheck) { - throw new BoxError(BoxError.INVALID_CREDENTIALS, 'A totpToken must be provided'); - } + if (!skipTotpCheck && user.twoFactorAuthenticationEnabled) { + if (!totpToken) throw new BoxError(BoxError.INVALID_CREDENTIALS, 'A totpToken must be provided'); + const verified = speakeasy.totp.verify({ secret: user.twoFactorAuthenticationSecret, encoding: 'base32', token: totpToken, window: 2 }); + if (!verified) throw new BoxError(BoxError.INVALID_CREDENTIALS, 'Invalid totpToken'); } }