Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix the auth proxy trust by ensuring the proxy is in the trust #499

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
10 changes: 7 additions & 3 deletions src/account-db.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,18 @@ 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;
}

const activeMethod = getActiveLoginMethod();
if (config.loginMethod) {
return config.loginMethod;
}

return config.loginMethod || activeMethod || 'password';
const activeMethod = getActiveLoginMethod();
return activeMethod || 'password';
twk3 marked this conversation as resolved.
Show resolved Hide resolved
}

export async function bootstrap(loginSettings) {
Expand Down
1 change: 1 addition & 0 deletions src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,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,
Expand Down
6 changes: 5 additions & 1 deletion src/config-types.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { ServerOptions } from 'https';

type LoginMethod = 'password' | 'header' | 'openid';

export interface Config {
mode: 'test' | 'development';
loginMethod: 'password' | 'header' | 'openid';
loginMethod: LoginMethod;
allowedLoginMethods: LoginMethod[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have method on the auth table now. But header auth is still using password, just accepting it a different way for now. Maybe we should filter the getLoginMethods using this when returning it in bootstrap as well. Haven't fully explored the recent openid changes, so I'm not quite sure.

trustedProxies: string[];
trustedAuthProxies?: string[];
dataDir: string;
projectRoot: string;
port: number;
Expand Down
25 changes: 23 additions & 2 deletions src/load-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,18 @@ if (process.env.ACTUAL_CONFIG_PATH) {
/** @type {Omit<import('./config-types.js').Config, 'mode' | 'dataDir' | 'serverFiles' | 'userFiles'>} */
let defaultConfig = {
loginMethod: 'password',
// assume local networks are trusted for header authentication
allowedLoginMethods: ['password', 'header', 'openid'],
// assume local networks are trusted
trustedProxies: [
'10.0.0.0/8',
'172.16.0.0/12',
'192.168.0.0/16',
'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(
Expand Down Expand Up @@ -116,9 +120,21 @@ const finalConfig = {
return value === 'true';
})()
: config.multiuser,
allowedLoginMethods: process.env.ACTUAL_ALLOWED_LOGIN_METHODS
? process.env.ACTUAL_ALLOWED_LOGIN_METHODS.split(',')
.map((q) => q.trim().toLowerCase())
.filter(Boolean)
: config.allowedLoginMethods,
twk3 marked this conversation as resolved.
Show resolved Hide resolved
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,
Expand Down Expand Up @@ -208,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)}`);
Expand Down
20 changes: 9 additions & 11 deletions src/util/validate-user.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import config from '../load-config.js';
import proxyaddr from 'proxy-addr';
import ipaddr from 'ipaddr.js';
import { getSession } from '../account-db.js';

Expand Down Expand Up @@ -45,24 +44,23 @@ export default function validateSession(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;
}
}
6 changes: 6 additions & 0 deletions upcoming-release-notes/499.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Bugfix
authors: [twk3]
---

Fix the auth proxy trust by ensuring the proxy is in the trust
Loading