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, () => {}); + } + } }, /** 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(", ")}`); } }