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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)) {
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[];
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'],
// assume local networks are trusted
twk3 marked this conversation as resolved.
Show resolved Hide resolved
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,
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