Remove DNS provider credentials from disk after certbot runs - #5814
Open
shawnhank wants to merge 2 commits into
Open
Remove DNS provider credentials from disk after certbot runs#5814shawnhank wants to merge 2 commits into
shawnhank wants to merge 2 commits into
Conversation
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().
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
I run NPM at home with 14 proxy hosts, all on Let's Encrypt via the Cloudflare DNS-01
challenge. None of those services are publicly reachable, so HTTP-01 isn't an option —
DNS-01 is the only way to get certificates for them.
I hit this while opening a single internal service to the internet through a Cloudflare
Tunnel. Putting one thing on the public side made me go through what an attacker would
actually get if that host were compromised, rather than what I assumed they'd get. Working
down that list I got to the DNS-01 credential and went looking for where it lives:
Fourteen files, each holding a live Cloudflare API token in plaintext, the oldest from
January. Permissions are right —
0600, root-owned — but they are permanent.In
requestLetsEncryptSslWithDnsChallenge, cleanup only runs when certbot fails:The success path returns without unlinking, so the file survives for the life of the
certificate. The only other cleanup is in the revoke path, which runs when a certificate
is deleted.
This matters more than "a secret is on disk." Anything running as root — a compromised
process, a script, malware — can read that 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.
An exfiltration path built on them looks like ordinary HTTPS. With a Cloudflare
Zone:DNS:Edittoken the same holder can also repoint every hostname in the zone.Why the one-line fix doesn't work
Moving that
unlinkinto afinallybreaks renewals. certbot records the credentialspath in its renewal config at issuance:
and
renewLetsEncryptSslWithDnsChallengeshells out tocertbot renew, which reads thatpath back. Delete the file and every DNS-01 renewal fails — silently, until certificates
start expiring.
What this changes
The renew path now writes the credentials file immediately before invoking certbot, and
both paths remove it in a
finally. The file exists for the duration of a certbot runinstead of permanently.
There was a third writer.
setupCertbotPlugins()inbackend/setup.jswrote a credentialsfile for every DNS-01 certificate on every backend start (
flag: "wx", so it only filledin missing ones). That existed precisely because the renew path did not write the file itself
— something had to put it back before
certbot renewwent looking. Now that renew writes itsown, that boot-time write is the only thing restoring credentials to disk, and it would undo
the cleanup on the next container restart. Removing it leaves one writer, immediately before
use. The
fsimport andpromisesarray become unused; the "Added Certbot plugins" log lineis kept, now gated on
plugins.lengthrather than on a promise array that only ever heldcredential writes.
One wrinkle worth explaining: the credentials aren't on the certificate object the renew
function receives.
renew()sources it frominternalCertificate.get(), which pipes therow through
utils.omitRow(omissions())— andomissions()listsmeta.dns_provider_credentialsprecisely so it can't travel out over the API. The renewpath therefore reads the row from the model directly.
What it does not fix
The token still lives in the
certificatestable. It has to — otherwise there'd benothing to write the file from. Anyone with a backup of NPM's data volume still has it.
This narrows one of the two copies, not both.
Behaviour changes worth flagging
certbot renewby hand inside the container will no longer work for DNS-01certificates, since the credentials file won't be sitting there between runs. Renewals
need to go through NPM, which is already how the scheduler drives them.
overwrites it with the stored value. If there is no stored value, it logs a warning and
leaves whatever is on disk alone, so an older certificate can't be broken by this.
Testing
Validated on a throwaway NPM 2.15.1 container against Let's Encrypt staging, using the
dns-cloudflareplugin with a real CloudflareZone:DNS:Edittoken. The 2.15.1 image shipsbackend/internal/certificate.jsbyte-identical todevelop, so the patched file was mountedstraight over
/app/internal/certificate.js— no version skew between what was tested and whatthis PR changes.
Baseline, stock 2.15.1:
credentials-2left on disk holding the live tokenpath only ever read it
Patched:
certificate.jspatchedsetup.jswriterThe restart case is worth calling out: with only the
certificate.jschange, issuance left 0files but a restart brought them straight back — 0 after issuance, 1 after
docker restart.With both changes it stays at 0 across restart, and a renewal afterwards still succeeds. The
Added Certbot pluginslog line still fires once per start.The file's whole lifecycle, captured by polling once a second across a renewal:
certbot reported
Congratulations, all renewals succeededon every run, and both testcertificates ended up valid with fresh expiry dates.
Not exercised:
route53, whose credentials path is passed throughAWS_CONFIG_FILEratherthan a
--credentialsargument — same file lifecycle, but a different code path — and DNSproviders other than Cloudflare.