From 918728fcacaa6e3a5b2e9cf06eff90c0ba662747 Mon Sep 17 00:00:00 2001 From: Shawn Hank Date: Fri, 28 Aug 2026 17:12:35 -0600 Subject: [PATCH 1/2] Remove DNS provider credentials from disk after certbot runs The credentials file written for a DNS-01 challenge was only cleaned up when certbot failed - the unlink sat in a catch block. On success the file stayed in /etc/letsencrypt/credentials for the entire life of the certificate, holding a live DNS provider API token in plaintext. The file cannot simply be deleted at issuance, because certbot records its path in the renewal config and reads it back on every `certbot renew`. So the renew path now writes the file itself immediately before invoking certbot, and both paths remove it in a finally block. Net effect: the credentials exist on disk for the duration of a certbot run rather than permanently. The value still lives in the certificates table, which is unavoidable - it has to come from somewhere to be written at all. renewLetsEncryptSslWithDnsChallenge reads the row directly from the model because renew() sources its certificate from internalCertificate.get(), which strips meta.dns_provider_credentials via omissions(). --- backend/internal/certificate.js | 69 ++++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 6 deletions(-) diff --git a/backend/internal/certificate.js b/backend/internal/certificate.js index 6498422c61..963d2bb6a4 100644 --- a/backend/internal/certificate.js +++ b/backend/internal/certificate.js @@ -881,10 +881,20 @@ const internalCertificate = { const result = await utils.execFile(certbotCommand, args, adds.opts); logger.info(result); return result; - } catch (err) { - // Don't fail if file does not exist, so no need for action in the callback + } finally { + // Remove the credentials file whether certbot succeeded or failed. + // + // This cleanup used to sit in a catch block, so it only ran when issuance FAILED. + // A certificate that issued successfully left its DNS provider API credentials in + // /etc/letsencrypt/credentials for the entire life of that certificate. Nothing + // reads the file between certbot runs, so there is no reason to keep it: + // renewLetsEncryptSslWithDnsChallenge() writes it again immediately before each + // renewal. + // + // unlink is fire-and-forget with an empty callback. If the file is already gone + // that is the end state we wanted anyway, and a missing file must never turn a + // successful issuance into a failure. fs.unlink(credentialsLocation, () => {}); - throw err; } }, @@ -981,6 +991,43 @@ const internalCertificate = { `Renewing LetsEncrypt certificates via ${dnsPlugin.name} for Cert #${certificate.id}: ${certificate.domain_names.join(", ")}`, ); + // certbot reads the DNS credentials back from the path recorded in the renewal config + // it wrote at issuance time, for example: + // + // authenticator = dns-cloudflare + // dns_cloudflare_credentials = /etc/letsencrypt/credentials/credentials-27 + // + // so the file has to be present for the duration of this run. Write it here and remove + // it again below rather than leaving it on disk between renewals. + // + // Leaving it is an avoidable exposure. Anything running as root - a compromised + // process, a script, malware - can read the token and use it to issue valid Let's + // Encrypt certificates for the domain. Those certificates are genuinely trusted, so + // traffic presented with them passes TLS inspection, IDS/IPS and DLP that would + // otherwise flag it, and an exfiltration path built on them looks like ordinary + // HTTPS. The exposure window should be one certbot run, not the life of the + // certificate. + // + // The value is not on the certificate object we were handed: renew() sources that from + // internalCertificate.get(), which pipes the row through utils.omitRow(omissions()) so + // meta.dns_provider_credentials can never travel out over the API. Read the row from + // the model directly to get at it. + const row = await certificateModel.query().where("id", certificate.id).first(); + const credentials = row?.meta?.dns_provider_credentials; + const credentialsLocation = `/etc/letsencrypt/credentials/credentials-${certificate.id}`; + + if (credentials) { + fs.mkdirSync("/etc/letsencrypt/credentials", { recursive: true }); + fs.writeFileSync(credentialsLocation, credentials, { mode: 0o600 }); + } else { + // Nothing stored to write. A certificate issued under the previous behaviour may + // still have its file on disk; leave it be and let certbot decide. Throwing here + // would break a renewal that would otherwise have succeeded. + logger.warn( + `No stored DNS credentials for Cert #${certificate.id}; relying on any existing ${credentialsLocation}`, + ); + } + const args = [ "renew", "--force-renewal", @@ -1008,9 +1055,19 @@ const internalCertificate = { logger.info(`Command: ${certbotCommand} ${args ? args.join(" ") : ""}`); - const result = await utils.execFile(certbotCommand, args, adds.opts); - logger.info(result); - return result; + try { + const result = await utils.execFile(certbotCommand, args, adds.opts); + logger.info(result); + return result; + } finally { + // Only clean up a file we put there ourselves. If `credentials` came back empty we + // wrote nothing, and an older file left on disk by the previous behaviour is the + // only thing keeping that certificate renewable - deleting it would break the next + // run for no gain. + if (credentials) { + fs.unlink(credentialsLocation, () => {}); + } + } }, /** From 210366cca0a504b997b8e739392fa62bb6dce5e0 Mon Sep 17 00:00:00 2001 From: Shawn Hank Date: Sat, 29 Aug 2026 00:11:15 -0600 Subject: [PATCH 2/2] Stop recreating DNS credentials files on every backend restart setupCertbotPlugins() wrote a credentials file for every DNS-01 certificate each time the backend started, using flag "wx" so it only filled in missing ones. That existed because the renew path did not write the file itself, so something had to put it back before `certbot renew` looked for it. With the previous commit the renew path writes the file immediately before invoking certbot, so this is now the only thing putting those credentials back on disk - and it does so for every certificate on every restart, which undoes the cleanup entirely. Removing the write leaves the `fs` import and the `promises` array unused. The "Added Certbot plugins" log line is kept but now gates on plugins.length, since it was previously gated on a promise array that only ever held credential writes. --- backend/setup.js | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/backend/setup.js b/backend/setup.js index f6b1454434..6766a18a8b 100644 --- a/backend/setup.js +++ b/backend/setup.js @@ -1,4 +1,3 @@ -import fs from "node:fs/promises"; import { installPlugins } from "./lib/certbot.js"; import utils from "./lib/utils.js"; import { setup as logger } from "./logger.js"; @@ -98,7 +97,6 @@ const setupCertbotPlugins = async () => { if (certificates?.length) { const plugins = []; - const promises = []; certificates.map((certificate) => { if (certificate.meta && certificate.meta.dns_challenge === true) { @@ -106,31 +104,23 @@ const setupCertbotPlugins = async () => { plugins.push(certificate.meta.dns_provider); } - // Make sure credentials file exists - const credentials_loc = `/etc/letsencrypt/credentials/credentials-${certificate.id}`; - if (typeof certificate.meta.dns_provider_credentials === "string") { - promises.push( - fs - .mkdir("/etc/letsencrypt/credentials", { recursive: true }) - .then(() => - fs.writeFile(credentials_loc, certificate.meta.dns_provider_credentials, { - mode: 0o600, - flag: "wx", - }), - ) - .catch((err) => { - if (err.code !== "EEXIST") throw err; - }), - ); - } + // Deliberately does NOT write the DNS credentials file here any more. + // + // It used to, so that a later `certbot renew` would find the path recorded in its + // renewal config. The effect was that every backend restart rewrote a plaintext + // DNS provider API token for every DNS-01 certificate, and left it there. + // + // internalCertificate now writes that file immediately before it runs certbot and + // removes it again afterwards, so there is exactly one writer and the credential + // is on disk only for the length of a certbot run. Recreating the files at boot + // would put every one of them straight back. } return true; }); await installPlugins(plugins); - if (promises.length) { - await Promise.all(promises); + if (plugins.length) { logger.info(`Added Certbot plugins ${plugins.join(", ")}`); } }