v2.17.1 — security hardening (draconian must-fix review)

[fabriziosalmi]

v2.17.1 — security hardening (draconian must-fix review)

Five must-fix findings from a draconian security/logic review of the existing code. No new features. Each was verified against source (refute-first: only findings that survived independent skeptics) and is pinned by a regression test.

Security

• Path traversal via client-certificate Common Name (modules/core/client_certificates.py). create_client_certificate built the on-disk identifier straight from the caller-supplied Common Name (only spaces replaced) and ran mkdir(parents=True) before any other check, so a CN such as ../../../../tmp/evil created — and wrote a CA-signed key, certificate and metadata into — a directory outside the managed cert tree (single and batch issuance, operator role). The pre-existing _validate_identifier guard was only ever applied on retrieval, never to the value that constructs the identifier. Fixed by slugifying the CN at the construction site (covering every caller) plus a containment assertion before mkdir. Benign CNs slugify to themselves, so existing identifiers/paths are unchanged and the full CN is still preserved verbatim in the subject and metadata.

• Backup restore downgraded private keys to world-readable (modules/core/file_operations.py). restore_unified_backup set 0600 only for the exact name privkey.pem and 0644 for everything else, but certbot keeps the real key bytes in archive/<domain>/privkeyN.pem (the live/privkey.pem symlink points at them) and the ACME account key in accounts/.../private_key.json — both retained in the unified backup — so a restore actively rewrote served key material and the account key to 0644. Now mirrors certbot's own permissions: every private-key filename → 0600, public material (cert/chain/fullchain + metadata json) stays 0644.

• GET /api/metrics reachable unauthenticated (modules/api/resources.py). The endpoint carried no auth decorator while its sibling info endpoints (/api/cache, /api/backups) require the viewer role. Gated to match its peers; the Prometheus scrape target remains the separate, intentionally public /metrics route.

• Disabled / demoted / deleted user kept their live session (modules/core/auth.py). The role was snapshotted into the session at login and validation only checked expiry, so disabling, demoting, or deleting a user left an already-issued session valid — with the old role — for up to the session lifetime (default 8h), with no kill switch. Those transitions now invalidate the user's in-memory sessions; an unrelated edit (e.g. email) does not log the user out.

Fixes

• Webhook secrets clobbered on a generic settings save (modules/core/settings.py). v2.15.0 restored masked webhook secrets on the dedicated /api/notifications/config route, but the generic POST /api/settings path still merged the notifications subtree without restoring secrets nested in the channels.webhooks list (_deep_merge_dict replaces lists wholesale; _strip_masked_values only walks dicts), writing the mask sentinel over the real HMAC secret / bot token. Silent corruption — deliveries then signed with ********. atomic_update now restores masked list secrets generically for every deep-merge key, so both paths behave identically.

Validation

• Full unit suite: 1026 passed, 0 regressions.
• 18 new regression tests (the three behavioral ones proven to fail on pre-fix code, pass after).
• Containerized smoke (image built from this branch): 34 passed.
• Real-cert E2E against the Docker stack — Let's Encrypt staging via Cloudflare DNS-01, random certmate.org subdomains, issuer pinned to staging on the issued leaf: 13 passed (async issuance, full lifecycle, force-renew, reissue incl. path-traversal rejection).

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.85%. Comparing base (d6f48af) to head (0b87100).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   70.67%   70.85%   +0.17%     
==========================================
  Files          50       50              
  Lines       11548    11589      +41     
==========================================
+ Hits         8162     8211      +49     
+ Misses       3386     3378       -8     

Flag	Coverage Δ
unittests	70.85% <93.33%> (+0.17%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.