Fix deSEC token env var; add dnssleep; harden reload paths - #9
Open
mplabs wants to merge 1 commit into
Open
Conversation
- 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
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.
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:
The
dns_desecacme.sh plugin readsDEDYN_TOKEN(a holdover from deSEC'sdedyn.iodynamic-DNS naming); the script was exportingDESEC_TOKEN, which the plugin never looks at.Once that was fixed, issuance for a brand-new subdomain still failed intermittently with:
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-challengename 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 30after 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
DEDYN_TOKEN(notDESEC_TOKEN) for thedns_desecplugin.Validation race
--dnssleep 30to both the--issueand--renew -d ... --forcecalls, 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
/etc/pihole/tls.pemdirectly and restartspihole-FTLvia systemd, and the renewal cron needs to run as root for the reload hook'ssudocalls 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 cp/docker exec/docker restartagainst the container — it never touches/etc/piholeorpihole-FTLon 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 — invokingdockeras 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
--issueor--renewfails, instead of falling through into the install/reload steps with no valid certificate on disk.fullchain.ceritself, 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
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 thepiholeuser 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.cmpthe freshly issued cert/key against the deployed/etc/pihole/tls.pemso a mismatch fails the hook instead of reporting success.Misc script fixes
--homeand--accountemailexplicitly to the acme.sh installer, since undersudothe installer's default home-detection can pick up the invoking user's$HOMErather than root's.README corrections
curl -OURL was missing the branch path (refs/piholev6-ssl-setup.shinstead ofmain/piholev6-ssl-setup.sh) — as written it 404s.Testing
pihole.example.net), Docker-managed Pi-hole — reproduced both the token bug and the validation race pre-fix, confirmed both resolved post-fix.cmpcheck against a manually corrupted/etc/pihole/tls.pemto confirm it now fails the hook instead of reporting success.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
sudounder 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.