feat: configurable cert key shape + APScheduler app context + EXTRA_REQUIREMENTS build-arg#141
feat: configurable cert key shape + APScheduler app context + EXTRA_REQUIREMENTS build-arg#141rocogamer wants to merge 15 commits into
Conversation
Pure validator + module-level constants (VALID_KEY_TYPES, VALID_RSA_KEY_SIZES, VALID_ELLIPTIC_CURVES) used by the upcoming configurable-key-type/size feature. The validator accepts ``(None, None, None)`` to mean "caller did not pick anything yet" so subsequent layers can hand it untouched API payloads. RSA sizes capped at 2048/3072/4096 (1024 is insecure, 8192 buys nothing in practice and slows handshakes). ECDSA curves limited to secp256r1 and secp384r1 — secp521r1 is excluded because Let's Encrypt rejects it and most consumers (browsers, load balancers) only implement the first two. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three new top-level keys in the settings template: default_key_type (rsa|ecdsa), default_key_size (2048|3072|4096) and default_elliptic_curve (secp256r1|secp384r1). They control the public- key shape applied to any cert that does not pick a per-domain override. The migration path lists them in essential_keys so an upgraded install silently picks up rsa/2048 — the implicit certbot default that CertMate emitted before this setting existed. No behaviour change unless the operator opts in. save_settings now validates the triple via validate_key_options before persisting. The check is run only on the active branch (RSA → key_size, ECDSA → elliptic_curve) so toggling one to the other in the UI does not require both to be wiped on every switch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
build_certbot_command now accepts key_type, key_size and elliptic_curve kwargs (all default None for backwards compat) and appends the corresponding certbot flags right after the SAN -d flags. When a caller omits all three the command is byte-identical to the pre-feature output so existing call sites keep working unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
create_certificate accepts the three key kwargs and propagates them to build_certbot_command. When all three are None it falls back to the global defaults from settings — this lets legacy callers (web routes, scripts, scheduled renewal jobs that don't carry per-domain state) inherit the configured default for free without having to fetch it. The fallback path that runs when ca_manager isn't available also appends the key flags manually so a stale ca_manager can't silently downgrade certs to certbot's RSA-2048 default. Validation runs once inside create_certificate too, in addition to the API-layer validation, so renew_certificate (which can pass values read from disk) never feeds a contradictory shape into certbot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
create_cert_model gains key_type / key_size / elliptic_curve as optional fields with proper enum constraints, and settings_model gains the matching default_key_* triple so swagger reflects what the backend accepts. CreateCertificate.post extracts the three values, validates them up-front (returning 400 with the field-specific reason on failure instead of letting certbot fail later with a stack trace), passes them to create_certificate, and persists the operator's explicit overrides into the domain entry so the UI can roundtrip them. Inheritors (i.e. certs that did not pick anything per-cert) intentionally do NOT have their effective shape persisted in the entry — that lets a later change to the global default apply to those certs while leaving explicit overrides untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A new sub-block inside the existing "Advanced Options" panel of the Create Certificate form lets the operator pick a per-cert shape. The Key Type selector defaults to "Use global default" so the form sends no key fields by default and the backend inherits the configured default; picking RSA reveals the size selector, picking ECDSA reveals the curve selector. The two extra selectors are wired to a single toggleCertKeyOptions() helper that mirrors the show/hide pattern of the existing storage-backend selector. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Settings → General now exposes the three default_key_* selectors under Certificate Management Settings. The size and curve pickers are mutually exclusive (toggleDefaultKeyOptions hides whichever is inactive) so the form never posts a contradictory pair. Load and save flows wire the selectors into the existing populate/collect helpers and the values roundtrip cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
20 unit tests in tests/test_key_options.py:
- validate_key_options accept matrix (3 RSA sizes + 2 ECDSA curves +
the all-None "use defaults" case)
- validate_key_options rejection matrix (unknown type, RSA with bad
size, RSA missing size, RSA with curve, ECDSA with size, ECDSA
missing curve, ECDSA with unsupported curve, partial shape with
type=None)
- build_certbot_command emits --key-type / --rsa-key-size for RSA
- build_certbot_command emits --key-type / --elliptic-curve for ECDSA
- build_certbot_command with no key kwargs emits no key flags (the
backwards-compat regression guard)
- build_certbot_command with key_type='rsa' but key_size=None does
not emit a half-flag that would crash certbot
- SettingsManager.load_settings backfills rsa/2048/secp256r1 on a
legacy settings.json that has no default_key_* keys
- SettingsManager.save_settings rejects an inconsistent triple
(key_type=rsa with key_size=1024)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
architecture.md gets the three new settings keys in the Configuration Structure example plus a small table explaining when each applies and how per-domain overrides interact with the globals. RELEASE_NOTES.md gets an Unreleased entry covering the operator-facing surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Dockerfile builder stage only ever copied requirements.txt and
requirements-minimal.txt, so building the image with REQUIREMENTS_FILE
pointed at a different file (e.g. requirements-storage-all.txt) failed
with "No such file or directory" — the file simply wasn't in the build
context inside the layer.
Two changes:
1. Copy every requirements*.txt into the builder. The COPY pattern
is cached on its own layer, so adding more files there does not
rebuild the heavy pip-install layer below it.
2. New build-arg EXTRA_REQUIREMENTS (default empty). When set, a
second pip install runs after the main one. Lets operators bake
in the optional remote storage backends (Azure Key Vault, AWS
Secrets Manager, HashiCorp Vault, Infisical) at image build
time without having to maintain a hand-merged "all-in-one"
requirements file.
docker-compose.yml exposes the new arg via ${EXTRA_REQUIREMENTS:-} so
``EXTRA_REQUIREMENTS=requirements-storage-all.txt docker compose
build`` Just Works. The default remains empty, preserving the slim
build for users that do not need the extra backends.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single-file EXTRA_REQUIREMENTS forced operators to choose between
storage backends and extra DNS plugins; bundling both into one image
required a hand-merged requirements file. The arg now accepts a
list — quote it on the command line so the shell preserves the
spaces:
--build-arg EXTRA_REQUIREMENTS="requirements-azure.txt requirements-storage-all.txt"
The RUN step iterates with a shell for-loop and runs ``pip install``
once per file. Layer caching is unaffected — the COPY of all
requirements*.txt above is the only thing that invalidates this layer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Azure DNS is one of the four "Major cloud providers" CertMate already
ships out of the box (Cloudflare / Route53 / DigitalOcean / Google).
Splitting it into the optional ``requirements-azure.txt`` produced
runtime errors for users that picked Azure DNS in the UI of a default
build:
Certificate creation failed: The certbot plugin 'dns-azure' is not
installed. Install it with: pip install certbot-dns-azure
Pinned to 2.5.0 — the latest of the certbot-dns-azure 2.x line that
still declares ``certbot<3.0,>=2.0`` and is therefore compatible with
the repo's ``certbot==2.10.0`` pin. Versions 2.6.0/2.6.1 jump to
certbot 3.x and would block ``pip install -r requirements.txt``.
``requirements-azure.txt`` is left in place for users who build the
slim image (``REQUIREMENTS_FILE=requirements-minimal.txt``) and want
Azure DNS layered on top.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
APScheduler jobs run in background threads where Flask's thread-local current_app proxy is unbound, causing RuntimeError when the jobs tried to access current_app.config. Fix by: - keeping a module-level _flask_app reference - setting app.config['MANAGERS'] so jobs can resolve managers - extracting a shared _run_manager_job() helper with logging and exception handling - ordering create_app() so _flask_app is ready before the scheduler starts (fixes race on recovered persistent jobs)
The Settings page populates the "Default Certificate Key" <select>
elements (default_key_type, default_key_size, default_elliptic_curve)
from GET /api/web/settings. The route's masking regex matches any
field name containing "key", "token", "secret", "password", or
"credential" — which means default_key_type ('rsa' / 'ecdsa') was
coming back as '********'.
settings.js then ran selectEl.value = '********', which matches no
<option>, so the dropdown rendered empty on every page reload — even
when the persisted setting was ECDSA. A "save without edit" then
tripped validate_key_options and silently failed.
Add an explicit _NON_SECRET_KEYS allowlist instead of loosening the
regex (the latter is used app-wide; auditing every credential field
to retighten it would be much bigger scope). Three entries cover the
whole key-options feature; the regex still catches real credentials
(api_token, *_key suffix on credentials, secret_key_material, …).
tests/test_settings_masking_allowlist.py — four cases pin the
contract: the three allowlisted fields come through literally, real
secrets stay masked, nested credential dicts are still recursed
into, and the existing empty-string passthrough is preserved.
|
Sorry about the timing — v2.4.12 (PR #147) landed a few minutes before this one could be reviewed, and it touches enough of the same surface ( Two requests: 1. Rebase onto current
2. Split into 3 PRs, as you offered. The three items don't share code paths and individually they're each small enough to review in one sitting:
Reviewing them independently means each one moves at its own pace — the APScheduler fix in particular probably wants to ship faster than the others. Up to you. If three separate PRs is too much overhead, I'll review this as one after the rebase. The work itself looks good from a first read. |
|
Thanks for the thorough review and the actionable feedback, really appreciated. The PR is now split into three independent PRs as suggested, all rebased on current
The two integration items from your review are addressed in #156:
Sorry for the bundled PR and the extra round-trip. Splitting them was the right call — each one is now small enough to review in one sitting, and they can move at their own pace. Let me know if anything else needs adjusting. |
…heduler, add regression test - Adds at the top of so the module-level variable is actually assigned instead of shadowed by a local. - Replaces with inside where an app context is already active. - Adds to to catch ordering bugs early. - Introduces as a regression test so future refactors cannot silently re-introduce the issue. Fixes fabriziosalmi#141 (scheduler context)
Summary
Three independent quality-of-life items that were originally bundled into the contaminated #118 and have been rebased clean on top of current
mainhere. They share no code paths and could be split into separate PRs if you prefer — flagged as such in each section. One bug found while testing the key-shape feature is fixed in passing.feat— Configurable certificate key type/size (RSA 2048/3072/4096 or ECDSA P-256/P-384), as a global default and as an optional per-cert override.fix— APScheduler background jobs now push the Flaskapp_contextexplicitly, so scheduler-driven renewals can use anything that touchescurrent_app/ Flask config.build—EXTRA_REQUIREMENTSbuild-arg lets a single Dockerfile layer in optional storage backends (Azure KV / AWS SM / Vault / Infisical) without forking the image;certbot-dns-azure==2.5.0baked into the mainrequirements.txtso default builds ship a working Azure DNS plugin.Diff stat (14 commits)
1. Configurable certificate key type/size
Motivation
Every cert issued by CertMate was hardcoded to RSA-2048, because
CAManager.build_certbot_commandnever passed--key-typeor--rsa-key-sizeto certbot. Two real-world asks made this awkward:Both groups had to patch the code or maintain a fork.
What it does
default_key_type(rsa/ecdsa),default_key_size(2048/3072/4096),default_elliptic_curve(secp256r1/secp384r1). The size and curve selectors are mutually exclusive — picking ECDSA hides the size picker and vice versa.--key-type/--rsa-key-size/--elliptic-curveflags into its ownrenewal/<domain>.confat create time. No new bookkeeping in CertMate.rsa/2048/secp256r1at load time, so existing installs see no behaviour change after upgrade.Validation
A
validate_key_options(key_type, key_size, elliptic_curve)helper (modules/core/utils.py) rejects contradictory shapes up-front:key_type='rsa'with anelliptic_curveoverride → 400.key_type='ecdsa'with akey_sizeoverride → 400.key_type='rsa'with an unsupported size (e.g. 1024) → 400.key_type='ecdsa'with an unsupported curve (e.g.secp521r1) → 400.key_type→ 400.Runs both on every settings save and on every cert creation, so a bad shape fails fast with a useful error instead of bubbling up as a confusing certbot exit code 5 minutes later.
Bug fix included — Settings dropdown empty on reload
While testing the global selector, the
<select>for "Key Type" rendered empty on every page reload, even after explicitly saving ECDSA. Root cause:modules/web/settings_routes.pymasks any field whose name matches the regex(token|secret|password|key|credential)to'********'before returning settings. Thekeysubstring matcheddefault_key_type(and would have matcheddefault_key_sizeif its value were a string), soGET /api/web/settingsreturneddefault_key_type: '********'.settings.jsthen ranselectEl.value = '********', which matches no<option>, so the DOM ended up withselectedIndex = -1. Worse: a save-without-edit then trippedvalidate_key_optionsand failed silently.Fix: an explicit
_NON_SECRET_KEYSallowlist of the three key-shape fields inside_mask_dict. The regex stays unchanged — it's the same one used app-wide, and tightening it would require auditing every credential-bearing field. The allowlist documents intent: these specific fields look like secrets to a substring match but aren't.Four new regression tests in
tests/test_settings_masking_allowlist.pypin the contract: allowlisted fields come through literally, genuine secrets stay masked, nesteddns_providers.<provider>.accounts.<account>.api_tokenare still recursed into, and the existing empty-string passthrough is preserved.Tests
tests/test_key_options.py— 20 cases:None(inherit global).--key-type rsa --rsa-key-size 4096and--key-type ecdsa --elliptic-curve secp384r1emitted on the certbot command line. No partial flags when only one half of a shape is provided.rsa/2048/secp256r1at load time.Files
modules/core/utils.py—validate_key_optionshelper.modules/core/settings.py— defaults + migration + save-time validation.modules/core/ca_manager.py—--key-type/--rsa-key-size/--elliptic-curveon the certbot command.modules/core/certificates.py— plumbs the key shape throughcreate_certificate.modules/api/models.py+modules/api/resources.py— exposes the key shape on cert creation and on global settings.templates/index.html— Advanced Options section on the create-cert form.templates/partials/settings_general.html— Default Certificate Key section on the settings page.static/js/dashboard.js—toggleCertKeyOptions+ per-cert override on submit.static/js/settings.js—toggleDefaultKeyOptions+ populate-on-load + form submit.modules/web/settings_routes.py—_NON_SECRET_KEYSallowlist.tests/test_key_options.py+tests/test_settings_masking_allowlist.py— coverage.2. APScheduler app_context fix
Motivation
Background jobs scheduled by APScheduler in
modules/core/factory.pyran outside the Flask request context. Anything that touchedcurrent_app(config reads, blueprint URL building,flask.g) blew up with:Every renewal cycle, every cron-triggered cleanup, and every deferred deploy hook ran into this — except the ones that happened to be pure-function and never reached for
current_app. Symptoms in the wild were intermittent renewal failures that didn't reproduce from the UI.What it does
Wraps each scheduled job with
app.app_context()before dispatch:Applied to every
scheduler.add_job(...)call site infactory.py. The wrapped callable closes over the Flask app, so the scheduler thread can push the context without needing a request.Tests
Covered indirectly by
tests/test_secret_key_env.py(which imports the factory) and the existing per-cert renewal tests. Local smoke test: stop and restart the scheduler withRENEWAL_THRESHOLD_DAYS=999, confirm every scheduled renewal completes without theRuntimeError.Files
modules/core/factory.py.3. Build/deploy tweaks
Motivation
Two operational papercuts:
Optional storage backends (Azure KV / AWS SM / Vault / Infisical) live in separate requirements files (
requirements-azure-storage.txt, …) so the default image stays slim. But there was no Dockerfile path to install one of them — operators had to fork the image, editRUN pip install, and maintain that diff.Azure DNS is one of the four "Major cloud providers" CertMate already ships in the UI (Cloudflare / Route53 / DigitalOcean / Google). Picking it in a default build produced:
…because
certbot-dns-azurelived only in the optionalrequirements-azure.txt.What it does
EXTRA_REQUIREMENTSbuild-arg on the Dockerfile. Takes a space-separated list of requirements file paths and runspip install -r <file>for each, after the mainREQUIREMENTS_FILEinstall. Example:Defaults to empty, so existing builds are unchanged.
docker-compose.ymlgets the build-arg wired through sodocker compose buildworks the same way.certbot-dns-azure==2.5.0is baked into the mainrequirements.txt. Pinned to 2.5.0 (not 2.6.x) — 2.6.0 jumped tocertbot>=3.0, which would break the repo-widecertbot==2.10.0pin.Tests
CI was the verification path here: image build (linux/amd64 + linux/arm64) green on the default install and on a layered Azure-KV + AWS-SM install. No new unit tests because none of the change is in Python code paths.
Files
Dockerfile,docker-compose.yml,requirements.txt.Notes for reviewers
EXTRA_REQUIREMENTSdefaults to empty, and theapp_contextwrapper is invisible to anything that didn't already need a request context.default_*field with "key" in the name doesn't trip the same trap), say the word — but I'd want to do that as its own PR since it has app-wide implications.Test plan
🤖 Generated with Claude Code