diff --git a/backend/internal/certificate.js b/backend/internal/certificate.js index b353c4838..56be24e47 100644 --- a/backend/internal/certificate.js +++ b/backend/internal/certificate.js @@ -46,6 +46,11 @@ const certbotNoise = [ // provider, so a generic message (e.g. "Authentication error" from another DNS plugin) isn't // reported as a Cloudflare problem. const certbotHints = [ + // Checked first: a rate limit is unambiguous and the most useful thing to report + [ + /too many (certificates|failed authorizations|new orders)|rateLimited/i, + () => "Let's Encrypt rate limit reached. Wait before retrying (see https://letsencrypt.org/docs/rate-limits/).", + ], [ /expected \S+ to be a symlink/i, () => @@ -56,11 +61,6 @@ const certbotHints = [ () => "This certificate's files in /etc/letsencrypt are damaged or missing. Restore /etc/letsencrypt from a backup, or delete this certificate and create it again.", ], - // Checked first: a rate limit is unambiguous and the most useful thing to report - [ - /too many (certificates|failed authorizations|new orders)|rateLimited/i, - () => "Let's Encrypt rate limit reached. Wait before retrying (see https://letsencrypt.org/docs/rate-limits/).", - ], [ /\b9109\b.*?location: ([0-9a-f.:]+)/i, (m) => { @@ -89,15 +89,54 @@ const certbotHints = [ ]; /** - * Turns certbot's stderr into a message the UI can show: a hint for common causes plus certbot's own reason. - * Certbot never prints DNS provider credentials. + * The values in a DNS credentials file ("key = value" lines), for redacting from certbot output. * - * @param {String} stderr - * @param {String} [dnsProvider] The certificate's DNS provider, if it uses a DNS challenge + * @param {String} [credentials] + * @returns {String[]} + */ +const credentialValues = (credentials) => + `${credentials || ""}` + .split("\n") + .map((line) => line.replace(/^[^=:]*[=:]/, "").trim()) + .filter((value) => value.length >= 4); + +/** + * Removes anything that could be a secret from certbot output before it leaves the server. + * DNS plugins can echo provider errors verbatim, including request URLs that carry API keys in + * the query string (e.g. "401 Client Error ... for url: https://...?token=..."), so this strips + * the given credential values, URL query strings and userinfo, and key=value pairs whose name + * looks secret. + * + * @param {String} text + * @param {String[]} [secrets] * @returns {String} */ -const certbotFailureMessage = (stderr, dnsProvider) => { - const reason = `${stderr || ""}` +const redactCertbotOutput = (text, secrets = []) => { + let out = `${text || ""}`; + for (const secret of [...secrets].sort((a, b) => b.length - a.length)) { + out = out.split(secret).join("[REDACTED]"); + } + return out + .replace(/\b(Bearer|Basic)\s+[A-Za-z0-9._~+/=-]+/gi, "$1 [REDACTED]") + .replace(/(\b[a-z][a-z0-9+.-]*:\/\/)[^/\s@]*@/gi, "$1[REDACTED]@") + .replace(/(\b[a-z][a-z0-9+.-]*:\/\/[^\s?#]*)\?[^\s#]*/gi, "$1?[REDACTED]") + .replace( + /\b([\w-]*(?:token|key|secret|password|passwd|pass|auth|credential|signature|sig)[\w-]*)(["']?\s*[=:]\s*["']?)[^\s"'&,;]+/gi, + "$1$2[REDACTED]", + ); +}; + +/** + * Turns certbot's stderr into a message the UI can show: a hint for common causes plus certbot's own reason. + * The output is redacted first (see redactCertbotOutput), since DNS plugins can echo credentials. + * + * @param {String} stderr + * @param {String} [dnsProvider] The certificate's DNS provider, if it uses a DNS challenge + * @param {String[]} [secrets] Values that must never appear in the message + * @returns {String} + */ +const certbotFailureMessage = (stderr, dnsProvider, secrets) => { + const reason = redactCertbotOutput(stderr, secrets) .split("\n") .map((line) => line.trim()) .filter((line) => line && !certbotNoise.some((re) => re.test(line))) @@ -122,14 +161,19 @@ const certbotFailureMessage = (stderr, dnsProvider) => { * @param {Array} args * @param {Object} [opts] * @param {String} [dnsProvider] Passed through so provider-specific hints only show for that provider + * @param {String} [credentials] The DNS credentials file contents, so its values are redacted from errors * @returns {Promise} */ -const runCertbot = async (args, opts, dnsProvider) => { +const runCertbot = async (args, opts, dnsProvider, credentials) => { try { return await utils.execFile(certbotCommand, args, opts); } catch (err) { if (err instanceof error.CommandError) { - throw new error.ValidationError(certbotFailureMessage(err.message, dnsProvider), err); + const secrets = credentialValues(credentials); + logger.error(redactCertbotOutput(err.message, secrets)); + // The raw error is not attached as `previous`: the API error handler returns + // `previous` to the client for certificate routes, and it holds unredacted output. + throw new error.ValidationError(certbotFailureMessage(err.message, dnsProvider, secrets)); } throw err; } @@ -515,12 +559,18 @@ const internalCertificate = { */ delete: async (access, data) => { await access.can("certificates:delete", data.id); - const row = await internalCertificate.get(access, { id: data.id }); + const hostRelations = ["proxy_hosts", "redirection_hosts", "dead_hosts", "streams"]; + const row = await internalCertificate.get(access, { id: data.id, expand: hostRelations }); if (!row?.id) { throw new error.ItemNotFoundError(data.id); } + const inUse = hostRelations.some((rel) => row[rel]?.length); + for (const rel of hostRelations) { + delete row[rel]; + } + await certificateModel.query().where("id", row.id).patch({ is_deleted: 1, }); @@ -537,8 +587,19 @@ const internalCertificate = { if (row.provider === "letsencrypt") { // Revoke the cert - await internalCertificate.revokeLetsEncryptSsl(row); - // Certbot leaves the files behind when it can't revoke (e.g. a broken renewal config); remove them anyway. + const revoked = await internalCertificate.revokeLetsEncryptSsl(row, true).then( + () => true, + () => false, + ); + // Certbot leaves the files behind when it can't revoke (e.g. a broken renewal config). + // Remove them anyway, unless a host still uses this certificate: its rendered config + // includes these files, and removing them would fail nginx -t on the next reload. + if (!revoked && inUse) { + logger.warn( + `Certificate #${row.id} was deleted but could not be revoked, and hosts still use it; its files in /etc/letsencrypt were kept`, + ); + return true; + } // The row is already deleted and audited, so leftover files are logged, not thrown: // an error here would report a completed delete as failed, and a retry would 404. const leftover = internalCertificate.removeLetsEncryptFiles(row.id); @@ -780,6 +841,9 @@ const internalCertificate = { if (!meta.certificate_key) { throw new error.ValidationError("Certificate Key file was not provided"); } + // The key may be the stored one (a certificate-only upload), so check the pair belongs + // together; a mismatch would make nginx -t fail for every host on the next reload. + await internalCertificate.checkKeyMatchesCertificate(meta.certificate, meta.certificate_key); const certificate = await internalCertificate.update(access, { id: data.id, @@ -794,6 +858,36 @@ const internalCertificate = { return _.pick(meta, uploaded); }, + /** + * Throws unless the private key is the one the certificate was issued for, by comparing the + * public key derived from each. + * + * @param {String} certificate PEM certificate (the leaf comes first) + * @param {String} privateKey PEM private key + * @returns {Promise} + */ + checkKeyMatchesCertificate: async (certificate, privateKey) => { + const certPath = await tempWrite(certificate); + const keyPath = await tempWrite(privateKey); + try { + const [certPub, keyPub] = await Promise.all([ + utils.execFile("openssl", ["x509", "-in", certPath, "-pubkey", "-noout"]), + utils.execFile("openssl", ["pkey", "-in", keyPath, "-pubout"]), + ]); + if (!certPub.trim() || certPub.trim() !== keyPub.trim()) { + throw new error.ValidationError("Certificate does not match the Certificate Key"); + } + } catch (err) { + if (err instanceof error.ValidationError) { + throw err; + } + throw new error.ValidationError("Could not check that the Certificate matches the Certificate Key"); + } finally { + fs.rmSync(certPath, { force: true }); + fs.rmSync(keyPath, { force: true }); + } + }, + /** * Uses the openssl command to validate the private key. * It will save the file to disk first, then run commands on it, then delete the file. @@ -1050,7 +1144,12 @@ const internalCertificate = { logger.info(`Command: ${certbotCommand} ${args ? args.join(" ") : ""}`); try { - const result = await runCertbot(args, adds.opts, certificate.meta.dns_provider); + const result = await runCertbot( + args, + adds.opts, + certificate.meta.dns_provider, + certificate.meta.dns_provider_credentials, + ); logger.info(result); return result; } finally { @@ -1251,7 +1350,7 @@ const internalCertificate = { logger.info(`Command: ${certbotCommand} ${args ? args.join(" ") : ""}`); try { - const result = await runCertbot(args, adds.opts, certificate.meta.dns_provider); + const result = await runCertbot(args, adds.opts, certificate.meta.dns_provider, credentials); logger.info(result); return result; } finally { diff --git a/backend/internal/host.js b/backend/internal/host.js index 8df7cbe53..76329b43c 100644 --- a/backend/internal/host.js +++ b/backend/internal/host.js @@ -49,7 +49,8 @@ const internalHost = { * host. Matching is by value, not path, so renaming or reordering a location that carries * admin-written config is allowed, but new or edited config is not. Each existing config can * be used once (no duplicating it onto other paths), and same-path matches are taken first. - * A location that omits advanced_config keeps the one at its path. Mutates `locations`. + * A location that omits advanced_config keeps the one at its path, unless another location + * explicitly carries it. Mutates `locations`. * * @param {Array} locations incoming locations * @param {Array} [existingLocations] the host's current locations @@ -71,12 +72,13 @@ const internalHost = { throw new errs.PermissionError("You do not have permission to modify advanced configuration"); }; - // Pass 1: locations that keep (or omit) the config at their own path + // Pass 1: locations that keep the config at their own path const unresolved = []; + const omitted = []; for (const loc of locations) { const atPath = advAtPath(loc.path); if (typeof loc.advanced_config === "undefined") { - loc.advanced_config = atPath && take(atPath) ? atPath : ""; + omitted.push(loc); } else if ((loc.advanced_config || "") === atPath && (!atPath || take(atPath))) { // unchanged } else { @@ -91,6 +93,14 @@ const internalHost = { deny(); } } + + // Pass 3: locations that omit advanced_config keep the one at their path, if no location + // that names it explicitly has claimed it. Running this last stops a new location at an + // old path taking the config away from the location it was renamed to. + for (const loc of omitted) { + const atPath = advAtPath(loc.path); + loc.advanced_config = atPath && take(atPath) ? atPath : ""; + } }, /** diff --git a/backend/internal/proxy-host.js b/backend/internal/proxy-host.js index 9a3cf7e66..5065e082e 100644 --- a/backend/internal/proxy-host.js +++ b/backend/internal/proxy-host.js @@ -3,7 +3,6 @@ import errs from "../lib/error.js"; import { castJsonIfNeed } from "../lib/helpers.js"; import utils from "../lib/utils.js"; import proxyHostModel, { scopeToOwnerOrAllowed } from "../models/proxy_host.js"; -import userPermissionModel from "../models/user_permission.js"; import internalAuditLog from "./audit-log.js"; import internalCertificate from "./certificate.js"; import internalHost from "./host.js"; @@ -455,14 +454,14 @@ const internalProxyHost = { * * @param {Number} user_id * @param {String} visibility + * @param {Object} [permissionsMeta] the user's permissions meta, as already loaded by access * @returns {Promise} */ - getCount: async (user_id, visibility) => { + getCount: async (user_id, visibility, permissionsMeta) => { const query = proxyHostModel.query().count("id as count").where("is_deleted", 0); if (visibility !== "all") { - const perm = await userPermissionModel.query().where("user_id", user_id).first(); - scopeToOwnerOrAllowed(query, user_id, perm?.meta); + scopeToOwnerOrAllowed(query, user_id, permissionsMeta); } const row = await query.first(); diff --git a/backend/internal/report.js b/backend/internal/report.js index 59f13fe69..547af3ef3 100644 --- a/backend/internal/report.js +++ b/backend/internal/report.js @@ -15,7 +15,11 @@ const internalReport = { const userId = access.token.getUserId(1); const promises = [ - internalProxyHost.getCount(userId, access_data.permission_visibility), + internalProxyHost.getCount( + userId, + access_data.permission_visibility, + access.getPermissions?.()?.meta, + ), internalRedirectionHost.getCount(userId, access_data.permission_visibility), internalStream.getCount(userId, access_data.permission_visibility), internalDeadHost.getCount(userId, access_data.permission_visibility), diff --git a/backend/lib/express/rate-limit.js b/backend/lib/express/rate-limit.js index a8754a826..512519df9 100644 --- a/backend/lib/express/rate-limit.js +++ b/backend/lib/express/rate-limit.js @@ -1,9 +1,17 @@ +import crypto from "node:crypto"; import { isCI } from "../config.js"; /** - * In-memory sliding window rate limiter for authentication endpoints + * In-memory fixed window rate limiter for authentication endpoints */ +// Keys longer than this are hashed, so a client can't make the store hold arbitrarily large +// strings (keys can include request body values such as the login identity). +const MAX_KEY_LENGTH = 128; +// Entries per limiter. A flood of unique keys sweeps expired entries, then evicts the oldest, +// instead of growing the store until the periodic cleanup runs. +const MAX_STORE_SIZE = 50000; + // One store per limiter, so separate endpoints never share buckets const stores = new Set(); @@ -21,9 +29,12 @@ const cleanupTimer = setInterval(() => { cleanupTimer.unref(); /** - * The client IP as seen by the backend. X-Forwarded-For is only honoured when the socket is - * loopback, which is the admin nginx in this container; it sets the header to its own - * $remote_addr after realip processing, so the first entry is the resolved client. + * The address to rate limit on: the TCP peer of the admin nginx, which it passes as + * X-NPM-Peer-Addr ($realip_remote_addr), only honoured when the socket is loopback (that nginx, + * in this container). The realip-resolved client address is deliberately not used: nginx trusts + * X-Real-IP from private ranges, so a client on the LAN or Docker network could pick a new + * address, and a fresh bucket, on every request. Behind another proxy this resolves to that + * proxy's address, which is why limiters also key on the target account. * * @param {Object} req * @returns {String} @@ -31,10 +42,28 @@ cleanupTimer.unref(); export const getClientIp = (req) => { const socketAddr = req.socket?.remoteAddress || "unknown"; const isTrustedLocalProxy = socketAddr === "127.0.0.1" || socketAddr === "::1" || socketAddr === "::ffff:127.0.0.1"; - const forwardedFor = isTrustedLocalProxy ? req.headers["x-forwarded-for"]?.split(",")[0].trim() : null; - return forwardedFor || socketAddr; + const peerAddr = isTrustedLocalProxy ? req.headers["x-npm-peer-addr"]?.trim() : null; + return peerAddr || socketAddr; }; +const makeRoom = (store, now) => { + for (const [k, r] of store.entries()) { + if (now - r.startTime > r.windowMs) { + store.delete(k); + } + } + // Maps iterate in insertion order, so this drops the oldest windows first + for (const k of store.keys()) { + if (store.size < MAX_STORE_SIZE) { + break; + } + store.delete(k); + } +}; + +const boundedKey = (key) => + key.length > MAX_KEY_LENGTH ? `sha256:${crypto.createHash("sha256").update(key).digest("hex")}` : key; + /** * @param {Object} [options] * @param {Number} [options.windowMs] @@ -67,29 +96,21 @@ export default function createRateLimiter(options = {}) { return next(); } - const key = keyGenerator(req, getClientIp(req)); + const key = boundedKey(String(keyGenerator(req, getClientIp(req)))); const now = Date.now(); let record = store.get(key); if (!record || now - record.startTime > windowMs) { record = { - count: 1, + count: 0, startTime: now, windowMs, }; - store.set(key, record); - - if (skipSuccessfulRequests) { - const currentRecord = record; - res.on("finish", () => { - if (res.statusCode < 400 && currentRecord.count > 0) { - currentRecord.count--; - } - }); + if (!store.has(key) && store.size >= MAX_STORE_SIZE) { + makeRoom(store, now); } - - return next(); + store.set(key, record); } record.count++; diff --git a/backend/routes/tokens.js b/backend/routes/tokens.js index 012bb95ca..7879cb9c1 100644 --- a/backend/routes/tokens.js +++ b/backend/routes/tokens.js @@ -9,7 +9,7 @@ import { getValidationSchema } from "../schema/index.js"; // Keyed on IP + target account rather than IP alone: when the admin UI sits behind another // proxy every client can resolve to the same IP, and an IP-only bucket would let anyone lock // out every user with a handful of bad logins. Brute force against one account is still -// capped per IP. +// capped per connecting address (see getClientIp). const tokenRateLimiter = createRateLimiter({ windowMs: 60 * 1000, max: 10, diff --git a/docker/rootfs/etc/nginx/conf.d/dev.conf b/docker/rootfs/etc/nginx/conf.d/dev.conf index 67efc0f8a..c54f2a46e 100644 --- a/docker/rootfs/etc/nginx/conf.d/dev.conf +++ b/docker/rootfs/etc/nginx/conf.d/dev.conf @@ -17,6 +17,9 @@ server { proxy_set_header X-Forwarded-Scheme $scheme; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Forwarded-For $remote_addr; + # The TCP peer before realip processing; the backend rate limits on this because + # X-Real-IP can be set by any client in the trusted private ranges + proxy_set_header X-NPM-Peer-Addr $realip_remote_addr; proxy_pass http://127.0.0.1:3000/; proxy_read_timeout 15m; diff --git a/docker/rootfs/etc/nginx/conf.d/production.conf.template b/docker/rootfs/etc/nginx/conf.d/production.conf.template index 452609bcb..83293ebd3 100644 --- a/docker/rootfs/etc/nginx/conf.d/production.conf.template +++ b/docker/rootfs/etc/nginx/conf.d/production.conf.template @@ -17,6 +17,9 @@ server { proxy_set_header X-Forwarded-Scheme $scheme; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Forwarded-For $remote_addr; + # The TCP peer before realip processing; the backend rate limits on this because + # X-Real-IP can be set by any client in the trusted private ranges + proxy_set_header X-NPM-Peer-Addr $realip_remote_addr; proxy_pass http://127.0.0.1:3000/; proxy_read_timeout 15m; diff --git a/frontend/src/modals/ProxyHostModal.tsx b/frontend/src/modals/ProxyHostModal.tsx index d137a459a..38cdb75be 100644 --- a/frontend/src/modals/ProxyHostModal.tsx +++ b/frontend/src/modals/ProxyHostModal.tsx @@ -53,14 +53,10 @@ const ProxyHostModal = EasyModal.create(({ id, visible, remove }: Props) => { if (!isAdmin) { delete payload.advanced_config; delete payload.advancedConfig; - const existingLocations = data?.locations || []; - payload.locations = payload.locations.map((loc: any) => { - const existingLoc = existingLocations.find((l: any) => l.path === loc.path); - return { - ...loc, - advancedConfig: existingLoc?.advancedConfig || "", - }; - }); + // Location advanced configs are sent as-is. Non-admins can't edit them (the editor is + // hidden), so each one is still the admin-written value and moves with its location + // when the path is renamed; the backend matches them by value. Looking them up by + // path here would silently drop the config from a renamed location. } setProxyHost(payload, {