Merge pull request #5935 from NginxProxyManager/fix/5919-access-list-render

Fix access list rules and SSL missing from re-rendered host configs

backend/internal/host.js

@@ -79,15 +79,18 @@ const internalHost = {
 			redirection_hosts: [],
 		};
 
-		const proxyRes = await proxyHostModel.query().where("is_deleted", 0);
+		const proxyRes = await proxyHostModel
+			.query()
+			.where("is_deleted", 0)
+			.withGraphFetched("[certificate, access_list.[clients,items]]");
 		responseObject.proxy_hosts = internalHost._getHostsWithDomains(proxyRes, domainNames);
 		responseObject.total_count += responseObject.proxy_hosts.length;
 
-		const redirRes = await redirectionHostModel.query().where("is_deleted", 0);
+		const redirRes = await redirectionHostModel.query().where("is_deleted", 0).withGraphFetched("[certificate]");
 		responseObject.redirection_hosts = internalHost._getHostsWithDomains(redirRes, domainNames);
 		responseObject.total_count += responseObject.redirection_hosts.length;
 
-		const deadRes = await deadHostModel.query().where("is_deleted", 0);
+		const deadRes = await deadHostModel.query().where("is_deleted", 0).withGraphFetched("[certificate]");
 		responseObject.dead_hosts = internalHost._getHostsWithDomains(deadRes, domainNames);
 		responseObject.total_count += responseObject.dead_hosts.length;
 

backend/internal/nginx.js

@@ -7,6 +7,7 @@ import errs from "../lib/error.js";
 import utils from "../lib/utils.js";
 import { debug, nginx as logger } from "../logger.js";
 import accessListModel from "../models/access_list.js";
+import certificateModel from "../models/certificate.js";
 
 const __filename = fileURLToPath(import.meta.url);
 const __dirname = dirname(__filename);
@@ -209,11 +210,13 @@ const internalNginx = {
 	 * @param   {Object}  host
 	 * @returns {Promise}
 	 */
-	generateConfig: (host_type, host_row) => {
+	generateConfig: async (host_type, host_row) => {
 		// Prevent modifying the original object:
 		const host = JSON.parse(JSON.stringify(host_row));
 		const nice_host_type = internalNginx.getFileFriendlyHostType(host_type);
 
+		await internalNginx.loadRenderRelations(nice_host_type, host);
+
 		debug(logger, `Generating ${nice_host_type} Config:`, JSON.stringify(host, null, 2));
 
 		const renderEngine = utils.getRenderEngine();
@@ -299,6 +302,53 @@ const internalNginx = {
 		});
 	},
 
+	/**
+	 * The templates rely on the certificate and access list relations being loaded.
+	 * Not every caller fetches them (or fetches them fully), and when they're missing
+	 * the config silently renders without SSL or without any access rules (#5919).
+	 * Load anything that is referenced but not loaded onto the given host object.
+	 *
+	 * @param   {String}  host_type
+	 * @param   {Object}  host
+	 * @returns {Promise}
+	 */
+	loadRenderRelations: async (host_type, host) => {
+		if (host.certificate_id > 0 && host.certificate?.id !== host.certificate_id) {
+			const certificate = await certificateModel
+				.query()
+				.where("is_deleted", 0)
+				.andWhere("id", host.certificate_id)
+				.first();
+			host.certificate = certificate ? JSON.parse(JSON.stringify(certificate)) : null;
+		}
+
+		if (host_type === "proxy_host" && host.access_list_id > 0) {
+			if (
+				host.access_list?.id !== host.access_list_id ||
+				!Array.isArray(host.access_list.clients) ||
+				!Array.isArray(host.access_list.items)
+			) {
+				const accessList = await accessListModel
+					.query()
+					.where("is_deleted", 0)
+					.andWhere("id", host.access_list_id)
+					.withGraphFetched("[clients,items]")
+					.first();
+				host.access_list = accessList ? JSON.parse(JSON.stringify(accessList)) : null;
+			}
+
+			if (!host.access_list) {
+				logger.warn(
+					`Proxy Host #${host.id} references Access List #${host.access_list_id} which does not exist`,
+				);
+			} else if (!host.access_list.clients.length && !host.access_list.items.length) {
+				logger.warn(
+					`Proxy Host #${host.id} uses Access List #${host.access_list_id} which has no clients or authorizations, no access restrictions will apply`,
+				);
+			}
+		}
+	},
+
 	/**
 	 * This generates a temporary nginx config listening on port 80 for the domain names listed
 	 * in the certificate setup. It allows the letsencrypt acme challenge to be requested by letsencrypt

backend/internal/proxy-host.js

@@ -323,7 +323,7 @@ const internalProxyHost = {
 			.then(() => {
 				return internalProxyHost.get(access, {
 					id: data.id,
-					expand: ["certificate", "owner", "access_list"],
+					expand: ["certificate", "owner", "access_list.[clients,items]"],
 				});
 			})
 		.then(async (row) => {

docker/docker-compose.ci.yml

@@ -37,6 +37,7 @@ services:
           - website1.example.com
           - website2.example.com
           - website3.example.com
+          - website4.example.com
 
   examplesite:
     image: "${IMAGE}-examplesite:ci-${BUILD_NUMBER}"

docker/docker-compose.dev.yml

@@ -16,6 +16,7 @@ services:
           - website1.example.com
           - website2.example.com
           - website3.example.com
+          - website4.example.com
     environment:
       TZ: "${TZ:-Australia/Brisbane}"
       PUID: 1000

test/cypress/e2e/api/AccessListHostToggle.cy.js

@@ -0,0 +1,210 @@
+/// <reference types="cypress" />
+
+// Regression tests for #5919, #4286 and #5710: access list rules must survive
+// every path that re-renders a proxy host config, not just the initial save.
+describe('Access Lists across Proxy Host disable/enable', () => {
+	const domain = 'website4.example.com';
+
+	const creds = {
+		username: 'toggle-user',
+		password: 'toggle-pass',
+	};
+
+	let token;
+	let authListId;
+	let clientListId;
+	let hostId;
+
+	/**
+	 * Requests a path on the proxy host directly (bypassing squid)
+	 * and yields the HTTP status code
+	 *
+	 * @param {object}  [auth]
+	 * @param {string}  auth.username
+	 * @param {string}  auth.password
+	 */
+	const request = (auth) => {
+		const authArg = auth ? `-u '${auth.username}:${auth.password}'` : '';
+		return cy.exec(`curl --noproxy '*' -s -o /dev/null -w '%{http_code}' ${authArg} http://${domain}/`)
+			.then((result) => {
+				expect(result.exitCode).to.eq(0);
+				return result.stdout.trim();
+			});
+	};
+
+	// nginx reloads are signalled and return immediately, so the old
+	// config can still be served for a moment after an API change.
+	// Retry until the expected status is seen.
+	const expectStatus = (auth, status) => {
+		let last = null;
+		cy.waitUntil(() => request(auth).then((res) => {
+			last = res;
+			return res === status;
+		}), {
+			timeout:  15000,
+			interval: 500,
+			errorMsg: () => `${domain} did not return ${status}, last status: ${last}`,
+		});
+	};
+
+	const createAccessList = (data) => {
+		return cy.task('backendApiPost', {
+			token: token,
+			path:  '/api/nginx/access-lists',
+			data:  {
+				satisfy_any: false,
+				pass_auth:   false,
+				items:       [],
+				clients:     [],
+				...data,
+			}
+		}).then((res) => {
+			cy.validateSwaggerSchema('post', 201, '/nginx/access-lists', res);
+			expect(res).to.have.property('id');
+			expect(res.id).to.be.greaterThan(0);
+			return cy.wrap(res.id);
+		});
+	};
+
+	const setHostAccessList = (accessListId) => {
+		cy.task('backendApiPut', {
+			token: token,
+			path:  `/api/nginx/proxy-hosts/${hostId}`,
+			data:  {
+				access_list_id: accessListId,
+			}
+		}).then((data) => {
+			// No swagger validation here: with a host access list set, the expanded
+			// access_list in the response has no proxy_host_count, which the schema requires
+			expect(data).to.have.property('access_list_id', accessListId);
+		});
+	};
+
+	const toggleHost = () => {
+		cy.task('backendApiPost', {
+			token: token,
+			path:  `/api/nginx/proxy-hosts/${hostId}/disable`,
+		}).then((data) => {
+			cy.validateSwaggerSchema('post', 200, '/nginx/proxy-hosts/{hostID}/disable', data);
+			expect(data).to.be.equal(true);
+		});
+
+		cy.task('backendApiPost', {
+			token: token,
+			path:  `/api/nginx/proxy-hosts/${hostId}/enable`,
+		}).then((data) => {
+			cy.validateSwaggerSchema('post', 200, '/nginx/proxy-hosts/{hostID}/enable', data);
+			expect(data).to.be.equal(true);
+		});
+	};
+
+	before(() => {
+		cy.resetUsers();
+		cy.getToken().then((tok) => {
+			token = tok;
+		});
+	});
+
+	it('Should be able to create an auth access list and a client access list', () => {
+		createAccessList({
+			name:  'Toggle Auth',
+			items: [creds],
+		}).then((id) => {
+			authListId = id;
+		});
+
+		// The test runner is never in this range, so it must always be denied
+		createAccessList({
+			name:    'Toggle Clients',
+			clients: [
+				{
+					directive: 'allow',
+					address:   '203.0.113.0/24',
+				}
+			],
+		}).then((id) => {
+			clientListId = id;
+		});
+	});
+
+	it('Should be able to create a proxy host without an access list', () => {
+		cy.task('backendApiPost', {
+			token: token,
+			path:  '/api/nginx/proxy-hosts',
+			data:  {
+				domain_names:   [domain],
+				forward_scheme: 'http',
+				forward_host:   'examplesite',
+				forward_port:   80,
+				access_list_id: 0,
+				certificate_id: 0,
+				meta:           {
+					dns_challenge: false
+				},
+				advanced_config:         '',
+				locations:               [],
+				block_exploits:          false,
+				caching_enabled:         false,
+				allow_websocket_upgrade: false,
+				http2_support:           false,
+				hsts_enabled:            false,
+				hsts_subdomains:         false,
+				ssl_forced:              false
+			}
+		}).then((data) => {
+			cy.validateSwaggerSchema('post', 201, '/nginx/proxy-hosts', data);
+			expect(data).to.have.property('id');
+			expect(data.id).to.be.greaterThan(0);
+			hostId = data.id;
+		});
+
+		expectStatus(null, '200');
+	});
+
+	it('Should enforce an auth access list applied to an existing host', () => {
+		setHostAccessList(authListId);
+		expectStatus(null, '401');
+		expectStatus(creds, '200');
+	});
+
+	it('Should still enforce the auth access list after the host is disabled and enabled', () => {
+		toggleHost();
+		expectStatus(null, '401');
+		expectStatus(creds, '200');
+	});
+
+	it('Should enforce a client access list applied to an existing host', () => {
+		setHostAccessList(clientListId);
+		expectStatus(null, '403');
+	});
+
+	it('Should still enforce the client access list after the host is disabled and enabled', () => {
+		toggleHost();
+		expectStatus(null, '403');
+	});
+
+	it('Should be able to delete the proxy host and access lists', () => {
+		cy.task('backendApiDelete', {
+			token: token,
+			path:  `/api/nginx/proxy-hosts/${hostId}`,
+		}).then((data) => {
+			cy.validateSwaggerSchema('delete', 200, '/nginx/proxy-hosts/{hostID}', data);
+			expect(data).to.be.equal(true);
+		});
+
+		// Each delete signals an nginx reload and returns immediately. A request
+		// made while the workers are being swapped can be reset by the nginx
+		// serving the API (502 from squid), so let each reload settle first.
+		[authListId, clientListId].forEach((id) => {
+			cy.wait(1500);
+			cy.task('backendApiDelete', {
+				token: token,
+				path:  `/api/nginx/access-lists/${id}`,
+			}).then((data) => {
+				cy.validateSwaggerSchema('delete', 200, '/nginx/access-lists/{listID}', data);
+				expect(data).to.be.equal(true);
+			});
+		});
+	});
+
+});

test/cypress/plugins/backendApi/client.mjs

@@ -1,3 +1,4 @@
+import http from "node:http";
 import axios from "axios";
 import logger from "./logger.mjs";
 
@@ -8,6 +9,10 @@ const BackendApi = function (config, token) {
 	this.axios = axios.create({
 		baseURL: config.baseUrl,
 		timeout: 90000,
+		// Don't reuse sockets: a pooled keep-alive connection that squid or the
+		// backend has since closed fails with a 502 (ERR_READ_ERROR 104), which
+		// made tests flaky.
+		httpAgent: new http.Agent({ keepAlive: false }),
 	});
 };
 

test/docker/nginx/conf.d/website123.conf

@@ -1,6 +1,6 @@
 server {
     listen       80;
-    server_name  website1.example.com website2.example.com website3.example.com;
+    server_name  website1.example.com website2.example.com website3.example.com website4.example.com;
     root         /www;
     index        index.html;