Fix ACME SSRF - #5409
Merged
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDNS validation now rejects IPv4 and IPv6 literals. HTTP-01 validation no longer exposes mismatched response bodies, disables redirects, and uses scoped HTTP resource handling. ChangesDNS identifier validation
HTTP-01 validation hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@base/acme/src/main/java/org/dogtagpki/acme/server/ACMEIdentifierValidator.java`:
- Around line 52-61: Update ACMEIdentifierValidator’s DNS validation to remove a
leading wildcard prefix before calling isIpLiteral, while preserving the
existing malformed error for detected IP literals. Add a regression test
covering a wildcard-prefixed IPv4 identifier and verify it is rejected before
authorization storage or HTTP-01 processing.
In `@base/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java`:
- Around line 95-103: Update the invalid-response logging in HTTP01Validator,
including the log site near the later response handling, to stop logging the
remote-controlled response body. Log only bounded, escaped metadata such as
response length and a digest, reusing one sanitized representation consistently
and preserving the existing validation error behavior.
- Around line 117-124: Update HTTP01Validator’s validation request around
HttpClients.custom(), httpClient.execute(), and IOUtils.toString() to enforce
bounded connection, read, and request timeouts and a strict maximum
response-body size before buffering or comparison. Preserve redirect disabling
and ensure oversized or timed-out responses fail validation without exhausting
validation threads or memory.
- Around line 122-124: Update getResponse() to handle a null result from
httpResponse.getEntity() by treating it as an empty response, avoiding both
getContent() and EntityUtils.consume(entity) when no entity exists. Preserve the
existing trimming and response-processing behavior for non-null entities so
validateChallenge reports incorrectResponse rather than a connection error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b8ccb30-1f36-4fa1-bee2-35c727f21ace
📒 Files selected for processing (2)
base/acme/src/main/java/org/dogtagpki/acme/server/ACMEIdentifierValidator.javabase/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java
agaragna77
force-pushed
the
fix-acme-ssrf
branch
from
July 30, 2026 13:35
4dbecd4 to
49d2497
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
base/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java (1)
95-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the hardening contract.
Cover sanitized mismatch details, non-followed redirects, empty responses, and bounded slow/oversized responses.
Also applies to: 115-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java` around lines 95 - 103, Add regression tests for HTTP01Validator’s validation flow covering sanitized mismatch error details, redirects that are not followed, empty response bodies, and bounded handling of slow or oversized responses. Assert the ACME client receives no response-body disclosure while operator logging remains appropriate, and exercise the existing validation entry point rather than adding unrelated test coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java`:
- Around line 117-121: Update HTTP01Validator’s validationURL egress path before
httpClient.execute(httpGet) to resolve the target hostname and reject loopback,
private, link-local, and other disallowed addresses, including every resolved
address before connecting. Reuse existing network-egress isolation or
address-validation utilities if available; otherwise enforce the filtering in
HTTP01Validator while preserving normal public-address validation.
---
Nitpick comments:
In `@base/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java`:
- Around line 95-103: Add regression tests for HTTP01Validator’s validation flow
covering sanitized mismatch error details, redirects that are not followed,
empty response bodies, and bounded handling of slow or oversized responses.
Assert the ACME client receives no response-body disclosure while operator
logging remains appropriate, and exercise the existing validation entry point
rather than adding unrelated test coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9f5576b-f5a7-47f2-9a4f-67a7972d3c17
📒 Files selected for processing (2)
base/acme/src/main/java/org/dogtagpki/acme/server/ACMEIdentifierValidator.javabase/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java
agaragna77
force-pushed
the
fix-acme-ssrf
branch
from
July 30, 2026 20:36
49d2497 to
de4235e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@base/acme/src/main/java/org/dogtagpki/acme/server/ACMEIdentifierValidator.java`:
- Around line 133-141: Update the colon-containing branch in
ACMEIdentifierValidator to validate IPv6 and IPv4-mapped literals lexically
before any resolver call, rejecting malformed values such as “g:internal”. Do
not call InetAddress.getByName for arbitrary host input; only permit the
strictly validated literal forms and preserve rejection of invalid addresses.
- Around line 146-159: Update isIpv4Literal to use a non-resolving IP parser
that recognizes and rejects every IPv4 decimal notation accepted by the HTTP-01
downstream client, including dotted forms with fewer than four components and
single-integer forms such as 127.1, 127.0.1, and 2130706433. Extend the
validateSyntax regression tests to cover these literals and their
wildcard-prefixed variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c1c2dd4-4568-4cc2-a8a7-5587b192e55b
📒 Files selected for processing (2)
base/acme/src/main/java/org/dogtagpki/acme/server/ACMEIdentifierValidator.javabase/acme/src/main/java/org/dogtagpki/acme/validator/HTTP01Validator.java
agaragna77
force-pushed
the
fix-acme-ssrf
branch
from
July 31, 2026 06:46
de4235e to
0421b1e
Compare
edewata
approved these changes
Aug 4, 2026
edewata
left a comment
edewata
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Sorry for the delay. I just have some minor comments but feel free to update/merge. Thanks!
agaragna77
force-pushed
the
fix-acme-ssrf
branch
from
August 5, 2026 09:06
0421b1e to
c5274ec
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
Author
|
@edewata thanks! |
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.
Fix 5407
Summary by CodeRabbit