From ae60dee6854c57344b80ca4688b920e519e639c6 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Sat, 9 Nov 2024 10:40:55 -0800 Subject: [PATCH 1/6] fix: Fix the auth proxy trust by ensuring the proxy is in the trust - Validate that the closest peer to the express server is trusted proxy - Add a new trustedAuthProxies config to eventually be used for this - Add a allowedLoginMethod config to enable fully disabling header auth --- src/app-account.js | 5 +++++ src/config-types.ts | 6 +++++- src/load-config.js | 20 ++++++++++++++++++-- src/util/validate-user.js | 20 +++++++++----------- 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/app-account.js b/src/app-account.js index cc18ea32e..7ee5a613f 100644 --- a/src/app-account.js +++ b/src/app-account.js @@ -11,6 +11,7 @@ import { needsBootstrap, getLoginMethod, } from './account-db.js'; +import config from './load-config.js'; let app = express(); app.use(errorMiddleware); @@ -45,6 +46,10 @@ app.post('/login', (req, res) => { let loginMethod = getLoginMethod(req); console.log('Logging in via ' + loginMethod); let tokenRes = null; + if (!config.allowedLoginMethods.includes(loginMethod)) { + res.send({ status: 'error', reason: 'login-method-unsupported' }); + return; + } switch (loginMethod) { case 'header': { let headerVal = req.get('x-actual-password') || ''; diff --git a/src/config-types.ts b/src/config-types.ts index 8be7ba49d..a9b678269 100644 --- a/src/config-types.ts +++ b/src/config-types.ts @@ -1,9 +1,13 @@ import { ServerOptions } from 'https'; +type LoginMethod = 'password' | 'header'; + export interface Config { mode: 'test' | 'development'; - loginMethod: 'password' | 'header'; + loginMethod: LoginMethod; + allowedLoginMethods: LoginMethod[]; trustedProxies: string[]; + trustedAuthProxies?: string[]; dataDir: string; projectRoot: string; port: number; diff --git a/src/load-config.js b/src/load-config.js index 19696d59d..e6371f1e0 100644 --- a/src/load-config.js +++ b/src/load-config.js @@ -54,7 +54,8 @@ if (process.env.ACTUAL_CONFIG_PATH) { /** @type {Omit} */ let defaultConfig = { loginMethod: 'password', - // assume local networks are trusted for header authentication + allowedLoginMethods: ['password', 'header'], + // assume local networks are trusted trustedProxies: [ '10.0.0.0/8', '172.16.0.0/12', @@ -62,6 +63,9 @@ let defaultConfig = { 'fc00::/7', '::1/128', ], + // fallback to trustedProxies, but in the future trustedProxies will only be used for express trust + // and trustedAuthProxies will just be for header auth + trustedAuthProxies: null, port: 5006, hostname: '::', webRoot: path.join( @@ -105,9 +109,21 @@ const finalConfig = { loginMethod: process.env.ACTUAL_LOGIN_METHOD ? process.env.ACTUAL_LOGIN_METHOD.toLowerCase() : config.loginMethod, + allowedLoginMethods: process.env.ACTUAL_ALLOWED_LOGIN_METHODS + ? process.env.ACTUAL_ALLOWED_LOGIN_METHODS.split(',') + .map((q) => q.trim().toLowerCase()) + .filter(Boolean) + : config.allowedLoginMethods, trustedProxies: process.env.ACTUAL_TRUSTED_PROXIES - ? process.env.ACTUAL_TRUSTED_PROXIES.split(',').map((q) => q.trim()) + ? process.env.ACTUAL_TRUSTED_PROXIES.split(',') + .map((q) => q.trim()) + .filter(Boolean) : config.trustedProxies, + trustedAuthProxies: process.env.ACTUAL_TRUSTED_AUTH_PROXIES + ? process.env.ACTUAL_TRUSTED_AUTH_PROXIES.split(',') + .map((q) => q.trim()) + .filter(Boolean) + : config.trustedAuthProxies, port: +process.env.ACTUAL_PORT || +process.env.PORT || config.port, hostname: process.env.ACTUAL_HOSTNAME || config.hostname, serverFiles: process.env.ACTUAL_SERVER_FILES || config.serverFiles, diff --git a/src/util/validate-user.js b/src/util/validate-user.js index 117fb779b..86331344a 100644 --- a/src/util/validate-user.js +++ b/src/util/validate-user.js @@ -1,6 +1,5 @@ import { getSession } from '../account-db.js'; import config from '../load-config.js'; -import proxyaddr from 'proxy-addr'; import ipaddr from 'ipaddr.js'; /** @@ -30,24 +29,23 @@ export default function validateUser(req, res) { } export function validateAuthHeader(req) { - if (config.trustedProxies.length == 0) { - return true; - } - - let sender = proxyaddr(req, 'uniquelocal'); - let sender_ip = ipaddr.process(sender); + // fallback to trustedProxies when trustedAuthProxies not set + const trustedAuthProxies = config.trustedAuthProxies ?? config.trustedProxies; + // ensure the first hop from our server is trusted + let peer = req.socket.remoteAddress; + let peerIp = ipaddr.process(peer); const rangeList = { - allowed_ips: config.trustedProxies.map((q) => ipaddr.parseCIDR(q)), + allowed_ips: trustedAuthProxies.map((q) => ipaddr.parseCIDR(q)), }; /* eslint-disable @typescript-eslint/ban-ts-comment */ // @ts-ignore : there is an error in the ts definition for the function, but this is valid - var matched = ipaddr.subnetMatch(sender_ip, rangeList, 'fail'); + var matched = ipaddr.subnetMatch(peerIp, rangeList, 'fail'); /* eslint-enable @typescript-eslint/ban-ts-comment */ if (matched == 'allowed_ips') { - console.info(`Header Auth Login permitted from ${sender}`); + console.info(`Header Auth Login permitted from ${peer}`); return true; } else { - console.warn(`Header Auth Login attempted from ${sender}`); + console.warn(`Header Auth Login attempted from ${peer}`); return false; } } From 2bff727caab8e94c356f8507df760aae87c5fed3 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Sat, 9 Nov 2024 10:50:20 -0800 Subject: [PATCH 2/6] chore: Add release note --- upcoming-release-notes/499.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 upcoming-release-notes/499.md diff --git a/upcoming-release-notes/499.md b/upcoming-release-notes/499.md new file mode 100644 index 000000000..6c41db698 --- /dev/null +++ b/upcoming-release-notes/499.md @@ -0,0 +1,6 @@ +--- +category: Bugfix +authors: [twk3] +--- + +Fix the auth proxy trust by ensuring the proxy is in the trust From 6a19dad9334c681ba0441194cd2ddb2acbcc3b76 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Wed, 13 Nov 2024 07:37:57 -0800 Subject: [PATCH 3/6] Use express trust to fix rate limit validation errors --- src/app.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app.js b/src/app.js index 6dbf3506d..a2df0c97d 100644 --- a/src/app.js +++ b/src/app.js @@ -20,6 +20,7 @@ process.on('unhandledRejection', (reason) => { app.disable('x-powered-by'); app.use(cors()); +app.set('trust proxy', config.trustedProxies); app.use( rateLimit({ windowMs: 60 * 1000, From 1188aa71ce4054f9045f9090103bcfaa42acecd5 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Fri, 13 Dec 2024 07:45:01 -0800 Subject: [PATCH 4/6] Add openid to the list of allowed login methods --- src/account-db.js | 7 +++++-- src/load-config.js | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/account-db.js b/src/account-db.js index c8c30bf0b..5ecbccb04 100644 --- a/src/account-db.js +++ b/src/account-db.js @@ -53,9 +53,12 @@ export function getLoginMethod(req) { return req.body.loginMethod; } - const activeMethod = getActiveLoginMethod(); + if (config.loginMethod) { + return config.loginMethod; + } - return config.loginMethod || activeMethod || 'password'; + const activeMethod = getActiveLoginMethod(); + return activeMethod || 'password'; } export async function bootstrap(loginSettings) { diff --git a/src/load-config.js b/src/load-config.js index 9dabd7138..67fba0dde 100644 --- a/src/load-config.js +++ b/src/load-config.js @@ -54,7 +54,7 @@ if (process.env.ACTUAL_CONFIG_PATH) { /** @type {Omit} */ let defaultConfig = { loginMethod: 'password', - allowedLoginMethods: ['password', 'header'], + allowedLoginMethods: ['password', 'header', 'openid'], // assume local networks are trusted trustedProxies: [ '10.0.0.0/8', From d8d24706c15eaa29a8b2ee69605c2816c8680ba8 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Sat, 14 Dec 2024 10:27:44 -0800 Subject: [PATCH 5/6] chore: apply review feedback --- src/account-db.js | 3 ++- src/app-account.js | 5 ----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/account-db.js b/src/account-db.js index 5ecbccb04..446aaefa3 100644 --- a/src/account-db.js +++ b/src/account-db.js @@ -48,7 +48,8 @@ export function getActiveLoginMethod() { export function getLoginMethod(req) { if ( typeof req !== 'undefined' && - (req.body || { loginMethod: null }).loginMethod + (req.body || { loginMethod: null }).loginMethod && + config.allowedLoginMethods.includes(req.body.loginMethod) ) { return req.body.loginMethod; } diff --git a/src/app-account.js b/src/app-account.js index 49bee03f6..d1867d2a4 100644 --- a/src/app-account.js +++ b/src/app-account.js @@ -14,7 +14,6 @@ import { } from './account-db.js'; import { changePassword, loginWithPassword } from './accounts/password.js'; import { isValidRedirectUrl, loginWithOpenIdSetup } from './accounts/openid.js'; -import config from './load-config.js'; let app = express(); app.use(express.json()); @@ -60,10 +59,6 @@ app.post('/login', async (req, res) => { let loginMethod = getLoginMethod(req); console.log('Logging in via ' + loginMethod); let tokenRes = null; - if (!config.allowedLoginMethods.includes(loginMethod)) { - res.send({ status: 'error', reason: 'login-method-unsupported' }); - return; - } switch (loginMethod) { case 'header': { let headerVal = req.get('x-actual-password') || ''; From 66038c203cf6856bb79b48ad7b432352a8c64e02 Mon Sep 17 00:00:00 2001 From: DJ Mountney Date: Sat, 14 Dec 2024 10:30:41 -0800 Subject: [PATCH 6/6] chore: Update debug statements --- src/load-config.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/load-config.js b/src/load-config.js index 67fba0dde..f87e2b6cc 100644 --- a/src/load-config.js +++ b/src/load-config.js @@ -224,6 +224,11 @@ debug(`using user files directory ${finalConfig.userFiles}`); debug(`using web root directory ${finalConfig.webRoot}`); debug(`using login method ${finalConfig.loginMethod}`); debug(`using trusted proxies ${finalConfig.trustedProxies.join(', ')}`); +debug( + `using trusted auth proxies ${ + finalConfig.trustedAuthProxies?.join(', ') ?? 'same as trusted proxies' + }`, +); if (finalConfig.https) { debug(`using https key: ${'*'.repeat(finalConfig.https.key.length)}`);