feature: Add external ACME CA support#258
Closed
gyptazy wants to merge 1 commit intoPegaProx:mainfrom
Closed
Conversation
Review Summary by QodoAdd external ACME CA support for custom certificate providers
WalkthroughsDescription• Add support for external ACME certificate authorities beyond Let's Encrypt • Introduce ACME provider selection with Let's Encrypt and custom URL options • Add ACME directory URL field for custom ACME-compatible certificate servers • Update validation logic to handle provider-specific requirements • Extend translations across multiple languages for new ACME provider features Diagramflowchart LR
A["User Settings"] -->|Select Provider| B["ACME Provider Selector"]
B -->|Let's Encrypt| C["Let's Encrypt Flow"]
B -->|Custom ACME| D["Custom Directory URL"]
C -->|Email Required| E["Certificate Request"]
D -->|Directory URL Required| E
E -->|request_certificate| F["ACME Client"]
F -->|get_directory_url| G["Resolve ACME Endpoint"]
G -->|HTTP-01 Challenge| H["Issue Certificate"]
File Changes1. web/src/settings_modal.js
|
Code Review by Qodo
1. Custom provider LE fallback
|
1 task
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+976
to
+982
| if 'acme_provider' in data: | ||
| provider = str(data['acme_provider'] or 'letsencrypt').strip() | ||
| settings['acme_provider'] = provider if provider in ('letsencrypt', 'custom') else 'letsencrypt' | ||
| if 'acme_directory_url' in data: | ||
| settings['acme_directory_url'] = str(data['acme_directory_url'] or '').strip() | ||
| if settings.get('acme_provider') != 'custom': | ||
| settings['acme_directory_url'] = '' |
There was a problem hiding this comment.
1. Custom provider le fallback 🐞 Bug ≡ Correctness
update_server_settings() can persist acme_provider='custom' with an empty acme_directory_url; the renewal loop then passes an empty directory_url and get_directory_url() silently falls back to Let’s Encrypt. This can renew/issue from the wrong CA or cause confusing renewal failures when the operator intended a custom CA.
Agent Prompt
## Issue description
`acme_provider='custom'` can be saved with an empty `acme_directory_url`, and the renewal thread will then fall back to Let’s Encrypt because `get_directory_url()` defaults when the URL is blank.
## Issue Context
This can lead to certificates being renewed/issued from the wrong CA or repeated renewal failures.
## Fix Focus Areas
- pegaprox/api/settings.py[969-982]
- pegaprox/api/settings.py[1272-1279]
- pegaprox/app.py[671-688]
- pegaprox/core/acme.py[117-123]
## Suggested fix
- In both JSON and form-data paths of `update_server_settings()`: if `acme_provider == 'custom'`, require a non-empty `acme_directory_url` (and validate it) or reject the update with 400.
- In the renewal loop: only pass/use `directory_url` when provider is `custom`; otherwise force empty.
- In `get_directory_url()`: consider failing (raising/returning error) when provider is custom but URL missing, rather than defaulting to Let’s Encrypt.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+117
to
141
| def get_directory_url(staging=False, directory_url=None): | ||
| """Resolve the ACME directory URL, defaulting to Let's Encrypt.""" | ||
| custom_url = (directory_url or '').strip() | ||
| if custom_url: | ||
| return custom_url | ||
| return ACME_DIRECTORY_STAGING if staging else ACME_DIRECTORY_PROD | ||
|
|
||
|
|
||
| def request_certificate(domain, email, ssl_dir, staging=False, directory_url=None): | ||
| """ | ||
| Request a Let's Encrypt certificate via HTTP-01 challenge. | ||
| Request an ACME certificate via HTTP-01 challenge. | ||
|
|
||
| Returns dict with 'success', 'message', optionally 'cert_path', 'key_path', 'expires'. | ||
| The caller must ensure /.well-known/acme-challenge/<token> is served on port 80. | ||
| """ | ||
| acme_url = ACME_DIRECTORY_STAGING if staging else ACME_DIRECTORY_PROD | ||
| acme_url = get_directory_url(staging=staging, directory_url=directory_url) | ||
| account_key_path = os.path.join(ssl_dir, 'acme_account.key') | ||
|
|
||
| try: | ||
| # Step 1: Load directory | ||
| logging.info(f"[ACME] Starting certificate request for {domain} ({'staging' if staging else 'production'})") | ||
| environment = 'custom' if (directory_url or '').strip() else ('staging' if staging else 'production') | ||
| logging.info(f"[ACME] Starting certificate request for {domain} ({environment}) via {acme_url}") | ||
| dir_resp = requests.get(acme_url, timeout=15) | ||
| dir_resp.raise_for_status() | ||
| directory = dir_resp.json() |
There was a problem hiding this comment.
2. Acme directory ssrf 🐞 Bug ⛨ Security
Custom acme_directory_url is fetched server-side and the returned directory JSON controls subsequent HEAD/POST request targets (newNonce/newAccount/newOrder/etc.), enabling SSRF to attacker-chosen URLs (including internal services). This can be abused to reach internal-only endpoints from the PegaProx host network context.
Agent Prompt
## Issue description
The ACME directory URL is user-configurable, and the directory response controls additional URLs that the server will call (`newNonce`, `newAccount`, `newOrder`, etc.). This enables SSRF because the server will make HTTP requests to attacker-provided URLs.
## Issue Context
Even if the initial `directory_url` looks benign, a malicious directory can return endpoint URLs pointing to internal IPs/hosts.
## Fix Focus Areas
- pegaprox/api/settings.py[1491-1524]
- pegaprox/core/acme.py[117-142]
- pegaprox/core/acme.py[107-110]
- pegaprox/core/acme.py[84-104]
## Suggested fix
- Parse and validate `directory_url` using `urllib.parse`.
- Enforce a strict policy for all directory-provided endpoints:
- same scheme/host/port as the original `directory_url` (same-origin), OR an explicit allowlist.
- disallow redirects (`allow_redirects=False`) when fetching the directory and/or for subsequent calls.
- resolve hostnames and block private/loopback/link-local ranges.
- Consider storing a validated, normalized URL and rejecting unsafe ones at save time.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Yes and when running an internal CA we of course need to access private/internal (RFC1918) addresses which are not public accessible. A personal CA must not be operated on public facing interfaces/addresses. It can also be operated on non standard ports which makes it non-sense to make further validations as https://myca.company.dev:8443 (pointing to 192.168.0.10) would also be legit.
Comment on lines
+346
to
+357
| def check_and_renew(domain, email, ssl_dir, staging=False, days_before=30, directory_url=None): | ||
| """Check if cert needs renewal and renew if so. Returns True if renewed.""" | ||
| info = get_cert_info(ssl_dir) | ||
| if not info: | ||
| return False | ||
|
|
||
| if not info['is_letsencrypt']: | ||
| return False | ||
|
|
||
| if info['days_left'] > days_before: | ||
| logging.debug(f"[ACME] Cert still valid for {info['days_left']} days, no renewal needed") | ||
| return False | ||
|
|
||
| logging.info(f"[ACME] Cert expires in {info['days_left']} days, renewing...") | ||
| result = request_certificate(domain, email, ssl_dir, staging) | ||
| result = request_certificate(domain, email, ssl_dir, staging=staging, directory_url=directory_url) |
There was a problem hiding this comment.
3. Renewal overwrites self-signed 🐞 Bug ☼ Reliability
check_and_renew() no longer gates renewals by issuer and does not check is_self_signed, so once ACME is enabled it may attempt renewal and replace even self-signed/manual certificates when they near expiry. This can cause unexpected certificate changes and downtime if ACME prerequisites (port 80 routing/domain) aren’t met.
Agent Prompt
## Issue description
ACME auto-renewal now runs for self-signed/manual certs because issuer-based gating was removed and there is no replacement check.
## Issue Context
This can lead to unexpected certificate replacement or repeated renewal failures.
## Fix Focus Areas
- pegaprox/core/acme.py[297-363]
- pegaprox/app.py[671-688]
## Suggested fix
- Add an explicit guard in `check_and_renew()`:
- `if info.get('is_self_signed'): return False`
- Optionally also require that ACME is "managed" (e.g., a stored flag set only after a successful ACME issuance) before renewing.
- In the renewal loop, validate provider configuration (custom requires directory_url) before calling `check_and_renew()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
MrMasterbay
added a commit
that referenced
this pull request
Apr 6, 2026
- Client Portal plugin: self-service VM management for hosting customers - Public Status Page plugin: cluster health for monitoring screens (#126) - Integrated syslog server (UDP/TCP port 1514) with log viewer (PR #257, gyptazy) - External ACME CA support — custom directory URLs e.g. StepCA (#249, PR #258, gyptazy) - Markdown VM descriptions with edit/preview toggle (PR #263, newtscamander2) - Inline tag selector with Proxmox tag dropdown + validation (PR #263) - Node/tag filter dropdowns in Resource Management (PR #263) - Plugin config editor (JSON textarea in Settings UI) - portal_only user flag, VNC audit logging, portal actions in task bar - fix: setReconfigureCluster prop missing in sidebar (PR #261, newtscamander2) - fix: OIDC skip JWT verification not persisted (#188) - fix: list view table too wide in modern layout - security: timing-safe auth key, TOTP rate limit, absolute session timeout - security: admin pw change revokes all sessions, plugin path traversal fix - security: file upload magic bytes, API token escalation fix, DB key permissions
Contributor
|
Close den Pull da manuell implementiert :) . Auch erwähnt natürlich |
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
2 participants
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.
feature: Add external ACME CA support
This feature adds support for external CAs that provide ACME based certification issuing.
Fixes: #249
Tests
Tests have been performed with StepCA and ACME endpoint. Please do not wonder, I run very short-living certificates by only 24hrs which might not be used. But adjusting the renewal is also possible in PegaProx.
Adding external ACME CA Endpoint
After issued certificate by CA