Skip to content

Fix deSEC token env var; add dnssleep; harden reload paths - #9

Open
mplabs wants to merge 1 commit into
PrimePoobah:mainfrom
mplabs:fix/desec-token-and-hardening
Open

Fix deSEC token env var; add dnssleep; harden reload paths#9
mplabs wants to merge 1 commit into
PrimePoobah:mainfrom
mplabs:fix/desec-token-and-hardening

Conversation

@mplabs

@mplabs mplabs commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes deSEC certificate issuance (wrong acme.sh env var), fixes a validation race that can hit any DNS provider on a first-time hostname, adds privilege handling that's aware of which deployment mode (bare-metal vs. Docker) the script is targeting, corrects a couple of inaccuracies in the README, and adds a few reliability fixes to the install/renewal paths.

Background

Running the script against deSEC failed at DNS-01 with:

[...] You did not specify DEDYN_TOKEN yet.
[...] Please create your key and try again.
Error adding TXT record to domain: _acme-challenge.example.com

The dns_desec acme.sh plugin reads DEDYN_TOKEN (a holdover from deSEC's dedyn.io dynamic-DNS naming); the script was exporting DESEC_TOKEN, which the plugin never looks at.

Once that was fixed, issuance for a brand-new subdomain still failed intermittently with:

[...] Success for domain example.com '_acme-challenge.example.com'.
[...] All checks succeeded
[...] Verifying: example.com
[...] Invalid status. Verification error details: Incorrect TXT record

The script's own propagation check passed against a single public resolver, but Let's Encrypt validates from multiple network vantage points simultaneously. For a _acme-challenge name that has never existed before, the TXT record can pass one resolver's check while it's still replicating to some of the zone's other authoritative nameservers a second later — so validation loses the race. This isn't deSEC-specific; any DNS-01 flow validating a first-time hostname is exposed to it. A flat --dnssleep 30 after the record is added removes the race instead of relying solely on the pre-check.

While fixing these two issues I went through the rest of the script and README and found a few more places where behavior was masked or misdescribed, so I've folded those into this PR rather than opening several follow-ups.

Changes

deSEC fix

  • Export DEDYN_TOKEN (not DESEC_TOKEN) for the dns_desec plugin.

Validation race

  • Add --dnssleep 30 to both the --issue and --renew -d ... --force calls, so the script waits a fixed window after the DNS record is written instead of trusting a single-resolver propagation check.

Privilege handling made mode-aware

  • The deployment-mode prompt ("bare-metal or Docker?") now happens first, and root is required or not based on the answer, since the two modes need genuinely different privileges.
  • Bare-metal requires root: it writes /etc/pihole/tls.pem directly and restarts pihole-FTL via systemd, and the renewal cron needs to run as root for the reload hook's sudo calls to succeed later. If run as a non-root user, the script now exits with a clear explanation instead of failing deep inside the install steps.
  • Docker does not require root. The Docker reload hook only ever runs docker cp / docker exec / docker restart against the container — it never touches /etc/pihole or pihole-FTL on the host. acme.sh's install location and renewal cron now follow whichever user actually invokes the script in Docker mode, so the cron-triggered renewal later runs in the same user context as the interactive setup. This matters most for rootless Docker, where the daemon socket belongs to a specific non-root user — invoking docker as a different user (including root) reaches a different or nonexistent Docker context. If the script is run as root with Docker mode selected, it prints a warning about this rather than proceeding silently.

Fail loudly instead of silently

  • Exit non-zero immediately if --issue or --renew fails, instead of falling through into the install/reload steps with no valid certificate on disk.
  • Detect an "existing certificate" by checking for fullchain.cer itself, not just the presence of the cert directory — a previously failed issuance can leave the directory behind holding only a domain key, which the old check treated as a valid cert to renew.

Bare-metal reload hook correctness

  • The old reload chain was write && chmod && chown || true && .... Because || binds looser than the implicit precedence people expect here, a failed write still let the chain continue and report "Reload successful," leaving FTL serving the old certificate with no indication anything went wrong. The chown (which can legitimately fail, e.g. if the pihole user doesn't exist on some setups) is now the only step wrapped in || true, scoped with braces so it can't swallow failures from the steps before it.
  • After reload, cmp the freshly issued cert/key against the deployed /etc/pihole/tls.pem so a mismatch fails the hook instead of reporting success.

Misc script fixes

  • Pass --home and --accountemail explicitly to the acme.sh installer, since under sudo the installer's default home-detection can pick up the invoking user's $HOME rather than root's.

README corrections

  • The install command's curl -O URL was missing the branch path (refs/piholev6-ssl-setup.sh instead of main/piholev6-ssl-setup.sh) — as written it 404s.
  • The Usage section claimed the script "detects Docker (because it's psychic)." It never has — this has always been an explicit prompt. Reworded to say so, and moved that step to its correct position in the list (it now runs after the DNS provider prompt, matching the actual script order following the privilege-handling change above).
  • Prerequisites now notes that permission requirements differ by mode, per the privilege-handling change above.

Testing

  • Fresh issuance against deSEC for a first-time subdomain (pihole.example.net), Docker-managed Pi-hole — reproduced both the token bug and the validation race pre-fix, confirmed both resolved post-fix.
  • Re-ran the same scenario against rootless Docker specifically (my own setup) to confirm the renewal cron and reload hook run in the correct user context.
  • Verified the bare-metal reload hook's cmp check against a manually corrupted /etc/pihole/tls.pem to confirm it now fails the hook instead of reporting success.
  • Verified the corrected install URL resolves (200, not 404).
  • Did not test Cloudflare/Namecheap/GoDaddy/Route53/DigitalOcean/Linode/GCP paths directly — none of the provider-specific code changed, so I don't expect regressions there, but flagging since I can't personally confirm.
  • Did not test bare-metal Pi-hole directly (my own setups are all Docker); the bare-metal code path itself is unchanged except for the reload-hook hardening described above.

Compatibility

No new flags or prompts; the deployment-mode question is asked at the same point it always was, just before rather than after cert issuance. Bare-metal users running as root are unaffected. Docker users are unaffected unless they were invoking the script with sudo under a rootless Docker setup — in that specific case they'll now see an explicit warning rather than a renewal cron that could silently target the wrong Docker context.

- dns_desec reads DEDYN_TOKEN, not DESEC_TOKEN — the script exported the
  wrong name, so deSEC issuance always failed
- add --dnssleep 30 to --issue/--renew: Let's Encrypt's multi-vantage-point
  validation can run faster than a brand-new record replicates across all
  authoritative nameservers, even after the script's own propagation check
  passes
- require root: the renewal cron and reload hook both need it; running as
  a normal user silently installs a cron that can never deploy renewals
- key off the fullchain file, not the cert directory, to detect an existing
  cert — a failed prior run can leave a directory with only a domain key
- exit non-zero on issuance/renewal failure instead of continuing into
  install steps with no certificate
- bare-metal reload hook: guard the chown so `|| true` can't also swallow
  a failed write, and verify the deployed file actually matches the
  reissued cert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant