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
7 changes: 5 additions & 2 deletions src/account-db.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
twk3 marked this conversation as resolved.
Show resolved Hide resolved
}

export async function bootstrap(loginSettings) {
Expand Down
5 changes: 5 additions & 0 deletions src/app-account.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ 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());
Expand Down Expand Up @@ -59,6 +60,10 @@ app.post('/login', async (req, res) => {
let loginMethod = getLoginMethod(req);
console.log('Logging in via ' + loginMethod);
let tokenRes = null;
if (!config.allowedLoginMethods.includes(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.

This is a security improvement. getLoginMethod prefers whatever's in the clientSide request. This allows us to reject that if the server admin has decided they don't want header auth enabled.

res.send({ status: 'error', reason: 'login-method-unsupported' });
return;
}
switch (loginMethod) {
case 'header': {
let headerVal = req.get('x-actual-password') || '';
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
20 changes: 18 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
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