fix: Re-allow private ip ranges for ACME#588
Draft
gyptazy wants to merge 1 commit into
Draft
Conversation
This patch is required to re-allow people using their own root CA instances (e.g., StepCA) on personal / private ip ranges. As a result, we define a new key for 'trusted_hosts' which is the given host from the ACME domain.
codeant-ai-for-open-source
Bot
added
the
size:M
PR Summary by QodoFix ACME SSRF guard to allow private CA hosts via trusted_hosts Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
1 rule 1. DNS-01 drops trusted_hosts
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+309
to
+316
| trusted_hosts = {urlparse(acme_url).hostname} | ||
| account_key_path = os.path.join(ssl_dir, 'acme_account.key') | ||
|
|
||
| env = 'custom' if (directory_url or '').strip() else ('staging' if staging else 'production') | ||
| logging.info(f"[ACME] Starting certificate request for {domain} ({env}) via {acme_url}") | ||
| try: | ||
| from pegaprox.utils.url_security import sanitize_outbound_url, SsrfError | ||
| sanitize_outbound_url(acme_url) | ||
| sanitize_outbound_url(acme_url, trusted_hosts=trusted_hosts) |
There was a problem hiding this comment.
1. trusted_hosts bypass for le 📘 Rule violation ⛨ Security
The PR introduces a trusted_hosts allowlist that bypasses private/loopback SSRF blocking, and it is applied unconditionally (including default Let's Encrypt/staging directory URLs). This expands the SSRF risk surface beyond the stated “custom/private CA” use-case and should be restricted to explicitly configured custom ACME endpoints only.
Agent Prompt
## Issue description
`trusted_hosts` is derived from `acme_url` and passed into `sanitize_outbound_url()` even when using the default Let's Encrypt/staging directory URL. Because `trusted_hosts` exempts private/loopback targets from SSRF blocking, this unintentionally broadens a security bypass beyond the intended custom/private CA scenario.
## Issue Context
This PR’s stated goal is to re-allow private IP ranges for custom ACME deployments (e.g., StepCA). The current implementation also trusts public CA hostnames, meaning private/loopback resolution for those hostnames would no longer be blocked.
## Fix Focus Areas
- pegaprox/core/acme.py[306-317]
- pegaprox/utils/url_security.py[148-179]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+213
to
+220
| def _validate_challenge(authz_url, challenge_url, account_key, nonce, kid, token=None, trusted_hosts=()): | ||
| """Notify ACME that the selected challenge is ready and poll authorization.""" | ||
| ch_resp = _signed_request(challenge_url, {}, account_key, nonce, kid) | ||
| ch_resp = _signed_request(challenge_url, {}, account_key, nonce, kid, trusted_hosts=trusted_hosts) | ||
| nonce = ch_resp.headers.get('Replay-Nonce', nonce) | ||
|
|
||
| for attempt in range(30): | ||
| time.sleep(2) | ||
| poll_resp = _signed_request(authz_url, None, account_key, nonce, kid) | ||
| poll_resp = _signed_request(authz_url, None, account_key, nonce, kid, trusted_hosts=trusted_hosts) |
There was a problem hiding this comment.
2. Dns-01 drops trusted_hosts 🐞 Bug ≡ Correctness
After this PR, _validate_challenge() and _finalize_order() rely on trusted_hosts to allow private ACME endpoints, but the DNS-01 flows call them without passing context['trusted_hosts']. DNS-01 issuance/validation against an ACME directory on RFC1918/loopback will still fail at the SSRF guard when _signed_request() validates challenge/authz/finalize URLs.
Agent Prompt
## Issue description
The PR introduces `trusted_hosts` to relax the SSRF private-IP guard for ACME follow-on URLs when using a private/internal ACME server. However, DNS-01 code paths still call `_validate_challenge()` and `_finalize_order()` without passing the `trusted_hosts` returned by `_create_order()`, so those requests will be guarded with an empty `trusted_hosts` set and be rejected for private IP ranges.
## Issue Context
- `_signed_request()` now calls `_guard_acme_url(url, trusted_hosts)`.
- `_guard_acme_url()` forwards `trusted_hosts` into `sanitize_outbound_url()`.
- `_create_order()` computes `trusted_hosts` from the configured directory URL and returns it in the context.
- DNS-01 paths should pass `trusted_hosts=context['trusted_hosts']` into `_validate_challenge()` and `_finalize_order()`.
## Fix Focus Areas
- pegaprox/core/acme.py[445-477]
- pegaprox/core/acme.py[502-531]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
|
|
||
| def _validate_challenge(authz_url, challenge_url, account_key, nonce, kid, token=None): | ||
| def _validate_challenge(authz_url, challenge_url, account_key, nonce, kid, token=None, trusted_hosts=()): |
There was a problem hiding this comment.
Suggestion: The trusted-host propagation is only implemented for the HTTP-01 path, while DNS-01 flows still call the same helpers without passing trusted_hosts, so they silently fall back to the empty default and reject private ACME endpoints. This breaks the stated goal of re-allowing private ACME servers for DNS challenge workflows. Thread context['trusted_hosts'] through prepare_dns01_challenge and complete_dns01_challenge calls to _validate_challenge and _finalize_order as well. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ DNS-01 issuance fails for private StepCA-style ACME servers.
- ⚠️ Admins cannot complete ACME DNS-01 challenges on LAN.
- ❌ Automatic renewals using dns-01 with private CA break.Steps of Reproduction ✅
1. An admin requests a certificate with DNS-01 using a private ACME endpoint by calling
`POST /api/settings/acme/request` (route at `pegaprox/api/settings.py:1874-1935`) with
JSON including `"challenge_type": "dns-01"`, `"provider": "custom"`, and `"directory_url":
"https://10.0.0.5/acme/directory"`.
2. `request_acme_certificate()` calls `request_certificate()` in
`pegaprox/core/acme.py:362`, which in turn calls `_create_order()` at
`pegaprox/core/acme.py:306`; `_create_order()` sets `trusted_hosts =
{urlparse(acme_url).hostname}` at line 309, validates the directory URL with
`sanitize_outbound_url(acme_url, trusted_hosts=trusted_hosts)` at line 316, and returns a
`context` dict that includes this `trusted_hosts` entry (returned at lines 349-359).
3. For RFC 2136 DNS-01, `prepare_dns01_challenge()` in `pegaprox/core/acme.py:423-99` is
called from `request_certificate()` (line 372); after creating the TXT record and sleeping
for propagation, it calls `_validate_challenge(context['authz_url'], dns_challenge['url'],
context['account_key'], context['nonce'], context['kid'])` at `pegaprox/core/acme.py:456`
without passing `trusted_hosts`, so `_validate_challenge()` (definition at line 213) uses
its default `trusted_hosts=()`.
4. `_validate_challenge()` calls `_signed_request(challenge_url, {}, account_key, nonce,
kid, trusted_hosts=trusted_hosts)` at `pegaprox/core/acme.py:215`; with the default empty
`trusted_hosts`, `_signed_request()` invokes `_guard_acme_url()` at lines 108-121, which
calls `sanitize_outbound_url(challenge_url, trusted_hosts=())`. Because the ACME host is
on a private IP and no hostname/IP is whitelisted, `is_safe_outbound_url()` in
`pegaprox/utils/url_security.py:86-181` rejects the URL and raises `SsrfError`, causing
`prepare_dns01_challenge()`'s outer try/except block (lines 425-99) to log the error and
return a `{'success': False, 'message': ...}` result to the caller.
5. For manual DNS-01 completion, `complete_dns01_challenge()` in
`pegaprox/core/acme.py:103-135` pulls the stored `context` (which contains
`'trusted_hosts'`) from `_pending_dns_challenges`, but then calls
`_validate_challenge(...)` at line 512 and `_finalize_order(...)` at line 523 without
passing `trusted_hosts`, so the same SSRF guard rejection occurs when the ACME host is
private, breaking DNS-01 flows for private/custom ACME servers.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pegaprox/core/acme.py
**Line:** 213:213
**Comment:**
*Incomplete Implementation: The trusted-host propagation is only implemented for the HTTP-01 path, while DNS-01 flows still call the same helpers without passing `trusted_hosts`, so they silently fall back to the empty default and reject private ACME endpoints. This breaks the stated goal of re-allowing private ACME servers for DNS challenge workflows. Thread `context['trusted_hosts']` through `prepare_dns01_challenge` and `complete_dns01_challenge` calls to `_validate_challenge` and `_finalize_order` as well.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+309
to
+316
| trusted_hosts = {urlparse(acme_url).hostname} | ||
| account_key_path = os.path.join(ssl_dir, 'acme_account.key') | ||
|
|
||
| env = 'custom' if (directory_url or '').strip() else ('staging' if staging else 'production') | ||
| logging.info(f"[ACME] Starting certificate request for {domain} ({env}) via {acme_url}") | ||
| try: | ||
| from pegaprox.utils.url_security import sanitize_outbound_url, SsrfError | ||
| sanitize_outbound_url(acme_url) | ||
| sanitize_outbound_url(acme_url, trusted_hosts=trusted_hosts) |
There was a problem hiding this comment.
Suggestion: This change unintentionally disables private-IP SSRF protection for the ACME directory host by always whitelisting that same host before validation. Because is_safe_outbound_url now skips private/loopback rejection when the hostname is in trusted_hosts, a poisoned/rebound DNS answer for the directory domain can route requests to internal addresses and still pass validation. Only allow this bypass for explicitly approved custom-private ACME mode (not by default for every directory host), or pin the initially resolved IPs instead of trusting the hostname itself. [ssrf]
Severity Level: Critical 🚨
- ❌ ACME client may reach private-network HTTP endpoints.
- ⚠️ SSRF guard bypass via trusted_hosts hostname whitelisting.
- ⚠️ Potential internal service exposure from DNS rebinding.Steps of Reproduction ✅
1. An admin triggers certificate issuance via the API endpoint `POST
/api/settings/acme/request` implemented in `pegaprox/api/settings.py:1874-1935`, which
calls `request_certificate()` in `pegaprox/core/acme.py:362`.
2. Inside `request_certificate()`, `_create_order()` is invoked at
`pegaprox/core/acme.py:379`, which computes `acme_url = _get_directory_url(...)` and then
sets `trusted_hosts = {urlparse(acme_url).hostname}` at `pegaprox/core/acme.py:309`.
3. `_create_order()` calls `sanitize_outbound_url(acme_url, trusted_hosts=trusted_hosts)`
at `pegaprox/core/acme.py:316`, which flows into `is_safe_outbound_url()` in
`pegaprox/utils/url_security.py:86-181`; because the hostname (for example
`acme-v02.api.letsencrypt.org` or a custom ACME host) is present in `trusted_hosts`, the
private/loopback rejection branch at `url_security.py:170-179` is skipped even if DNS
resolves the host to a private IP (e.g., `10.0.0.10`).
4. As a result, a DNS-rebound or poisoned resolution of the ACME directory hostname to an
internal/private address will still be accepted by the SSRF guard, and subsequent HTTP
calls such as `requests.get(acme_url, ...)` at `pegaprox/core/acme.py:320` and
`_signed_request()` calls guarded by `_guard_acme_url()` at `pegaprox/core/acme.py:89-121`
will reach that internal address, defeating the intended private-IP SSRF protection for
that hostname.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pegaprox/core/acme.py
**Line:** 309:316
**Comment:**
*Ssrf: This change unintentionally disables private-IP SSRF protection for the ACME directory host by always whitelisting that same host before validation. Because `is_safe_outbound_url` now skips private/loopback rejection when the hostname is in `trusted_hosts`, a poisoned/rebound DNS answer for the directory domain can route requests to internal addresses and still pass validation. Only allow this bypass for explicitly approved custom-private ACME mode (not by default for every directory host), or pin the initially resolved IPs instead of trusting the hostname itself.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
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
1 participant
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.
User description
fix: Re-allow private ip ranges for ACME
This patch is required to re-allow people using
their own root CA instances (e.g., StepCA) on
personal / private ip ranges. As a result, we
define a new key for 'trusted_hosts' which is the
given host from the ACME domain.
Note: Draft, needs to be tested furthermore.
CodeAnt-AI Description
Allow ACME certificates to work with trusted private servers
What Changed
Impact
✅ Internal ACME setups work again✅ Fewer certificate request failures on private networks✅ Safer outbound requests to untrusted addresses💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.