feat: Azure Key Vault native Certificate-object storage mode

[rocogamer]

Summary

Adds a configurable storage_mode to the Azure Key Vault storage backend so it can persist certificates as native Certificate objects (PKCS12, issuer_name=\"Unknown\") in addition to — or instead of — the existing per-PEM Secrets layout. Native Certificate objects are consumed directly by App Service, Application Gateway, Front Door, API Management and AKS Ingress without manual PFX export.

• storage_mode: secrets (default) — current per-PEM Secrets layout, fully backwards-compatible.
• storage_mode: certificate — single Certificate object, metadata stored as tags.
• storage_mode: both — write to both surfaces; reads prefer Secrets (cheaper).

A new admin endpoint POST /api/storage/azure-keyvault/backfill-certificates imports Certificate objects for domains already stored as Secrets (skipping ones already imported); restricted to storage_mode='both' so the legacy Secrets are still listed during the walk. Exposed as a "Backfill Certificate objects" button in Settings → Storage when the backend mode allows it.

Implementation notes

• Composition: AzureKeyVaultBackend keeps auth/naming/retry; the new private _AzureKeyVaultCertificateImporter encapsulates Certificate-API specifics. Both Azure SDK clients are lazy so an install without azure-keyvault-certificates only fails when a Certificate-mode call is actually attempted.
• Tags ↔ metadata round-trip lives in a single class-level helper with a strict allow-list, so vault-level tags from Azure Policy (Environment, CostCenter, …) do not pollute the rehydrated metadata. Truncated SAN lists drop the (incomplete-by-construction) trailing fragment to avoid exposing a malformed FQDN.
• Surface independence: in both mode store_certificate and delete_certificate run each surface under its own try/except. A Secrets-API outage no longer aborts the Certificate-object write (and vice versa); the method returns False on partial failure so callers can react.
• Pre-existing bug fixed in passing: _list_secret_domains filtered endswith('-metadata'), which never matched anything in production because _sanitize_secret_name always appends an 8-char CRC32 suffix. The filter is now anchored as ^cert-.+-metadata-[0-9a-f]{8}$. Without this, list_certificates() and the new backfill endpoint would have walked an empty list.
• Settings migration: backfills storage_mode='secrets' for upgraded installs at load time so existing settings.json files keep working without manual edits.
• Service Principal permissions: documented in docs/architecture.md. secrets mode is unchanged; certificate/both modes additionally require Certificates Get/List/Import/Delete and keep Secrets Get/List (Key Vault exposes the imported PFX, including the private key, only via the Secret with the same name as the Certificate object).
• Sanitised error messages: both StorageBackendTest and the new backfill endpoint return only the exception type to the client; full Azure SDK detail (tenant ids, request ids, WWW-Authenticate headers) stays in logger.error.

Files changed

• modules/core/storage_backends.py — new helper class, three modes, surface-independent store/delete, CRC-aware list filter.
• modules/core/settings.py — nested-default migration for azure_keyvault.storage_mode.
• modules/api/{models,resources}.py — storage_mode field on the model, new backfill endpoint, Certificate-API probe in StorageBackendTest.
• templates/partials/settings_storage.html + static/js/settings.js — storage-mode selector and Backfill button (visible only when active mode is both).
• requirements-azure-storage.txt, requirements-storage-all.txt — azure-keyvault-certificates>=4.7.0.
• docs/architecture.md — three-mode table + Service Principal permissions matrix.
• RELEASE_NOTES.md — Unreleased entry.
• tests/test_azure_keyvault_certificate_storage.py — 38 new unit tests with mocked SDK clients.

Test plan

• pytest tests/test_azure_keyvault_certificate_storage.py -v — 38/38 pass (2 skip cleanly when azure-keyvault-certificates is not installed).
• pytest tests/ -m unit -v — full unit suite, 0 regressions.
• AST parse on every modified Python file.
• Backwards-compat: load a settings.json without storage_mode, confirm it migrates to secrets silently.
• Smoke test against a real Azure Key Vault (requires a Service Principal with Certificates + Secrets permissions):
  1. Configure SP in UI with storage_mode=both; press Test connection — both Secrets and Certificate API access verified.
  2. Issue a new cert from the dashboard — confirm in Azure Portal that Secrets cert-…-{cert,chain,fullchain,privkey}-pem-{crc} AND a cert-{domain}-{crc} Certificate object (with domain/dns_provider/staging tags) both appear.
  3. Renew the cert — confirm a new version of the same Certificate object (not a new object).
  4. Switch to storage_mode=certificate; issue a new cert — only the Certificate object is created, no Secrets.
  5. GET /api/certificates/{domain} — returns the four PEMs reconstructed from the PFX export.
  6. DELETE /api/certificates/{domain} — both surfaces removed in both mode.
  7. Switch back to both with legacy Secrets-only domains, press Backfill Certificate objects — every legacy domain gets an imported Certificate object; already-imported ones are reported as skipped.
  8. Bind the Certificate object from an Azure App Service / Application Gateway with managed identity — TLS works without manual PFX export.

Notes for reviewers

• Default storage_mode='secrets' means existing installs see no behaviour change after upgrade.
• The azure-keyvault-certificates SDK is in the optional storage extras (requirements-azure-storage.txt); CI does not install it by default, so the two SDK-touching tests skip cleanly under pytest.importorskip.
• The _with_retry decorator is currently shadowed by inner try/except blocks across all Azure KV methods (and other backends). This is pre-existing in main and out of scope here, but worth a follow-up issue.

🤖 Generated with Claude Code

[fabriziosalmi]

This is a substantive 1700-line feature with a careful test surface (38 unit tests, mocked SDK clients, backwards-compat migration, three modes, CRC-aware list filter fix in passing). I want to give it a proper review rather than a rushed one — the architecture decisions (composition pattern, surface-independent store/delete, the metadata allow-list, sanitised error messages) deserve careful reading.

Realistic timeline: I'll target this as a v2.5.0 candidate (after the smaller in-flight items: #119 fix #113, #122 dns alias rebase, #120 *_FILE secrets, #126 download ?file= param all land). The patch versions in this 2.4.x line are issue-triage only; this PR is feature-shaped and deserves a minor bump.

Quick first-pass comments (not blockers — putting them here so they're not lost):

1. `storage_mode='both'` semantics: writes go to both surfaces independently (good — Secrets outage doesn't block Certificate write), reads prefer Secrets ("cheaper" — agreed). What happens on `get_certificate(domain)` when Secrets has version N and Certificate-object has version N-1 (e.g. a Certificate-mode write succeeded but the Secrets-mode write later failed in a previous renewal)? Worth confirming the read returns the freshest version that exists, not unconditionally Secrets.
2. The CRC-aware list_secret_domains regex fix is its own bug worth calling out in the release notes — without it, list_certificates() walked an empty list silently. Pre-existing, but invisible to anyone who hadn't noticed certs disappearing from the list view.
3. Backfill endpoint restricted to storage_mode='both': clear and correct.

Will do the full pass — checked the test file already exists in the diff and has good coverage of the helper class. Ping me if you want me to start the review now and just deferred merging, vs deferred everything.

[rocogamer]

Thanks for the thoughtful first-pass review! I've addressed all three points you raised:

1. Stale read detection in both mode

You flagged the real risk: get_certificate returning stale Secrets data when the Certificate-object surface is newer. Fixed by making _retrieve_from_secrets return the updated_on timestamp alongside the PEMs/metadata, and adding a get_certificate_update_time() helper on the importer. retrieve_certificate now compares timestamps of both surfaces and returns whichever is freshest:

• Secrets present, Cert absent → return Secrets (no change, cheaper path)
• Secrets absent, Cert present → return Cert (fallthrough, was already there)
• Both present, Cert newer → Cert wins
• Both present, Secrets newer → Secrets wins (faster path + fresh)

Two new tests cover the skew scenarios in TestRetrieveBothMode (40 total, all green).

2. CRC-aware list_secret_domains regex fix

Agreed this is its own bug. Added a standalone ### Bug Fixes entry in RELEASE_NOTES.md calling out that endswith('-metadata') silently matched zero secrets in production because _sanitize_secret_name always appends an 8-char CRC32 suffix. The fix anchors the regex to ^cert-.+-metadata-[0-9a-f]{8}$.

3. Branch sync

Rebased on top of current main (now includes #119, #122, #120, #126, v2.4.7 trixie bump). Merge is clean — MERGEABLE again.

Diff summary

• modules/core/storage_backends.py — stale read detection logic, new helper
• tests/test_azure_keyvault_certificate_storage.py — 2 new tests + fixed 3 existing mocks with updated_on datetimes
• RELEASE_NOTES.md — stale read entry + CRC fix entry

Ready for your full review at your convenience — no rush, understood this is a v2.5.0 candidate.

[rocogamer]

Apologies — I just noticed that the git merge main I ran to resolve the merge conflicts pulled in way more than intended. The PR diff is now cluttered with commits that have nothing to do with the Azure Key Vault Certificate-object feature.

Here's what leaked in and what each thing is:

Leaked features (not part of this PR):

Feature	What it does	Why it's here
Configurable certificate key type/size	Lets operators choose RSA 2048/3072/4096 or ECDSA P-256/P-384 per cert or as a global default. Touches certificates.py, ca_manager.py, settings.py, utils.py, UI templates, and a 641-line test file (test_key_options.py).	Merged from our fork's feat/configurable-key-type-size branch.
APScheduler app_context fix	Fixes a RuntimeError: Working outside of application context in cron-triggered background jobs by pushing app.app_context() explicitly. Touches factory.py.	Small fix we developed today while testing the merged code.
Build/deploy tweaks	EXTRA_REQUIREMENTS build-arg, certbot-dns-azure==2.5.0 pinning, etc.	Carried over from our fork's main during the sync.

What belongs to this PR:

• modules/core/storage_backends.py — original Azure KV three-mode backend + stale read detection
• tests/test_azure_keyvault_certificate_storage.py — 40 unit tests
• The Azure KV entries in RELEASE_NOTES.md, modules/api/models.py, modules/api/resources.py, modules/core/settings.py, docs/architecture.md, templates, and static/js/settings.js

Plan: I'll clean this up — either by doing a proper rebase of just the Azure KV commits on top of upstream/main, or by opening a fresh PR with only the relevant changes. Let me know which you prefer and I'll get it sorted quickly. Really sorry for the noise in the diff.

[fabriziosalmi]

Full Review: Azure Key Vault Certificate-object Storage

Thanks for the thorough work on this — the architecture is genuinely impressive. The composition pattern, surface-independent writes, stale-read detection (addressing my earlier comment about version skew in both mode), metadata allow-list, and sanitised error messages are all well thought out.

🔴 Blocker: PR is contaminated

As you noted in your latest comment, the git merge main pulled in ~1200 lines of unrelated code:

Leaked feature	Impact
Configurable key type/size	641-line test file + 322 lines in certificates.py + ca_manager.py + utils.py
APScheduler app_context fix	79 lines in factory.py
Build/deploy tweaks	Dockerfile, docker-compose.yml, requirements.txt

Please open a fresh PR with only the Azure KV commits (cherry-pick or interactive rebase). The 3413-line diff is not reviewable in its current state — I need to see only the ~2200 lines that belong to this feature.

✅ Architecture positives (confirmed on code review)

• Composition: _AzureKeyVaultCertificateImporter encapsulates Certificate-API specifics — clean separation
• Surface independence: store_certificate() and delete_certificate() wrap each surface in independent try/except — a Secrets outage doesn't block the Certificate write
• Stale-read detection: retrieve_certificate() in both mode compares updated_on timestamps and returns the freshest — exactly what I flagged in my first comment ✅
• issuer_name="Unknown": prevents Key Vault from attempting auto-renewal (CertMate stays source of truth)
• CRC fix: the list_secret_domains regex anchoring is a real bug fix — endswith('-metadata') never matched anything in production
• Test coverage: 974 lines, 40 unit tests, comprehensive mock coverage

🟡 Non-blocking suggestions for the clean PR

1. Document that Secret API exposes the private key — when importing a Certificate object, Azure creates a companion Secret containing the full PFX (including private key). Anyone with Secrets/Get can extract it. This is by-design in Azure but operators should be aware.
2. Backfill rate limiting — the backfill endpoint iterates all domains synchronously. For vaults with hundreds of certs, this may trigger Azure throttling (HTTP 429). Consider a batch_size parameter or background job.
3. Pin cryptography>=41.0.0 — load_pem_x509_certificates (plural) was added in v41. Older versions will fail with a confusing ImportError.

Timeline

This is a v2.5.0 candidate — once the PR is rebased clean, I'll do a final pass and merge. No rush, take your time getting the rebase right.

[rocogamer]

@fabriziosalmi Opened the clean rebase as #139 — same 12 Azure-KV commits cherry-picked onto current upstream/main, none of the leaked feature/build commits. Diff is 1878/126 across 11 files (only the files that belong to this feature).

I also addressed all three non-blocker suggestions from your review in #139:

1. Secrets/Get private-key exposure — pulled out of the permissions-table cell into a prominent Security note in docs/architecture.md under the storage-modes section.
2. Backfill rate limiting — added a ?limit=N query parameter on the endpoint; response now includes a remaining count so operators with large vaults can paginate. Pagination is safe because each call is idempotent (already-imported domains are reported as skipped).
3. cryptography>=41.0.0 floor — added explicitly to requirements-azure-storage.txt and requirements-storage-all.txt. Main requirements.txt already pins cryptography==46.0.7, but the extras were silently relying on the transitive constraint; making the floor explicit means a layered install fails loudly with a pip resolver error rather than an opaque runtime ImportError.

Tests: 40/40 pass.

Closing this one in favour of #139.