Fix/ssrf alerts target#1665
Open
ygndotgg wants to merge 4 commits into
Open
Conversation
Contributor
WalkthroughThis PR introduces an outbound egress policy system for alert targets. The system validates Slack webhooks, generic webhooks, and AlertManager instances against configurable allow/deny domain and CIDR rules, enforcing TLS policies, blocking sensitive headers, and pinning DNS-resolved addresses before connection. Includes admin HTTP endpoints for viewing and updating the policy. ChangesOutbound Alert Target Egress Policy
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin API
participant Handler as alert_target_policy handlers
participant Validator as validate_policy
participant Policy as ALERT_TARGET_POLICY
Admin->>Handler: PUT /admin/alert-target-policy
Handler->>Validator: validate_policy(&config)
Validator-->>Handler: Result<(), OutboundPolicyError>
alt validation passes
Handler->>Policy: replace_policy(config)
Policy-->>Handler: Ok(())
Handler-->>Admin: 200 JSON(config)
else validation fails
Validator-->>Handler: OutboundPolicyError
Handler-->>Admin: 400 JSON(error)
end
sequenceDiagram
participant AlertSender as Alert delivery
participant Target as TargetType::call
participant Prepare as prepare_alert_target
participant DNS as DNS Resolver
participant Validator as Address/Domain Validator
participant Client as reqwest::Client
AlertSender->>Target: send(alert)
Target->>Prepare: prepare_alert_target(endpoint, kind, headers)
Prepare->>Prepare: validate scheme & TLS rules
Prepare->>DNS: resolve host -> addrs
DNS-->>Prepare: resolved addresses
Prepare->>Validator: check denied CIDRs & private IPs
alt policy allows
Prepare-->>Target: PreparedAlertTarget
Target->>Client: POST with prepared client & headers/authorization
Client-->>Target: response
else policy rejects
Prepare-->>Target: OutboundPolicyError
Target->>Target: log rejection & return
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Cargo.toml (1)
84-91: 💤 Low value
ipnetaddition not reflected in summary; otherwise dependency changes look correct.The
netfeature ontokio("^1.43") is valid and required for DNS resolution/socket operations in the policy engine. However, line 91 also addsipnet = "2"(needed for CIDR parsing in the allow/deny policy), which contradicts the summary's claim that no other dependencies were altered.Note that
ipnetis placed under the "Async and Runtime" section though it's a networking/CIDR utility — consider moving it nearerurl/networking deps for clarity, but this is purely cosmetic.🤖 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 `@Cargo.toml` around lines 84 - 91, Update the PR summary to explicitly mention the new ipnet = "2" dependency (used for CIDR parsing in allow/deny policy) in addition to the tokio feature change; in the Cargo.toml diff, ensure the ipnet addition is clearly associated with networking/url deps rather than the "Async and Runtime" block by moving the ipnet = "2" line next to other networking dependencies (the tokio features block and url dependency) so reviewers can easily see it's a networking/CIDR utility; keep the existing tokio feature changes (sync, macros, fs, rt-multi-thread, net) unchanged.src/alerts/outbound_http_policy.rs (1)
167-169: 💤 Low valueConsider propagating client build errors instead of panicking.
While the
ClientBuilderconfiguration is deterministic and unlikely to fail in practice, using.expect()in production code paths is less defensive than propagating the error. If future changes introduce fallible configuration (e.g., loading certificates), this would need updating.🛡️ Suggested fix
Add a new error variant and propagate:
+ #[error("failed to build HTTP client: {0}")] + ClientBuildFailed(String),let client = builder .build() - .expect("alert target HTTP client can be constructed"); + .map_err(|e| OutboundPolicyError::ClientBuildFailed(e.to_string()))?;🤖 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 `@src/alerts/outbound_http_policy.rs` around lines 167 - 169, Replace the panic-causing .expect("alert target HTTP client can be constructed") on builder.build() with proper error propagation: add a new error variant (e.g., OutboundHttpClientBuildError or ClientBuildError) to the module's error enum, change the containing function's return type to Result<..., ErrorType>, and replace builder.build().expect(...) with builder.build().map_err(|e| ErrorType::OutboundHttpClientBuildError(e.into()))? (or equivalent) so the client construction failure is returned to callers; update any call sites and tests to handle the propagated Result accordingly.
🤖 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 `@src/alerts/outbound_http_policy.rs`:
- Around line 306-323: In denied_ipv6, add explicit checks to reject IPv6
tunneling prefixes 6to4 (2002::/16) and Teredo (2001::/32) before falling back
to mapped-IPv4 handling: inspect ip.segments()[0] and return true when it equals
0x2002 (6to4) or equals 0x2001 (Teredo /32), then continue with the existing
mapped-IPv4 call to denied_ipv4 and other checks; this ensures embedded IPv4
addresses in those tunneling ranges cannot bypass the private-IP blocking.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 84-91: Update the PR summary to explicitly mention the new ipnet =
"2" dependency (used for CIDR parsing in allow/deny policy) in addition to the
tokio feature change; in the Cargo.toml diff, ensure the ipnet addition is
clearly associated with networking/url deps rather than the "Async and Runtime"
block by moving the ipnet = "2" line next to other networking dependencies (the
tokio features block and url dependency) so reviewers can easily see it's a
networking/CIDR utility; keep the existing tokio feature changes (sync, macros,
fs, rt-multi-thread, net) unchanged.
In `@src/alerts/outbound_http_policy.rs`:
- Around line 167-169: Replace the panic-causing .expect("alert target HTTP
client can be constructed") on builder.build() with proper error propagation:
add a new error variant (e.g., OutboundHttpClientBuildError or ClientBuildError)
to the module's error enum, change the containing function's return type to
Result<..., ErrorType>, and replace builder.build().expect(...) with
builder.build().map_err(|e| ErrorType::OutboundHttpClientBuildError(e.into()))?
(or equivalent) so the client construction failure is returned to callers;
update any call sites and tests to handle the propagated Result accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 307e02c4-1433-45cc-90f4-a9ee10f8c67c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlsrc/alerts/mod.rssrc/alerts/outbound_http_policy.rssrc/alerts/target.rssrc/handlers/http/alert_target_policy.rssrc/handlers/http/mod.rssrc/handlers/http/modal/query_server.rssrc/handlers/http/modal/server.rssrc/handlers/http/targets.rs
ygndotgg
force-pushed
the
fix/ssrf-alerts-target
branch
from
fa45266 to
b8f9628
Compare
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.
Fixes #XXXX.
Description
This PR hardens alert target delivery against server-side request forgery by adding an outbound policy for alert-target HTTP
requests.
Previously, authenticated users with alert target permissions could configure webhook, Slack, or AlertManager targets that
caused the Parseable server to send outbound HTTP requests to arbitrary URLs from the server’s network position. That meant
private/internal destinations, loopback services, link-local metadata endpoints, and other network-restricted HTTP services
could be reached if routable from the Parseable host.
The chosen fix is to centralize alert-target outbound networking behind a policy gate. Instead of validating only the submitted
URL string, Parseable now resolves the destination, validates the resolved IPs, applies admin-owned allow/deny policy, disables
redirects/proxies, pins the resolved DNS result into the HTTP client, and revalidates again immediately before alert delivery.
Key changes made in this patch:
Added alert target outbound policy configuration with:
Added super-admin APIs for reading, updating, and validating the alert target policy.
Applied outbound policy validation during target creation and update.
Revalidated outbound policy during alert delivery because stored targets and DNS answers can change after creation.
Blocked private, loopback, link-local, multicast, reserved, and other non-public destinations by default.
Allowed private/internal destinations only when explicitly enabled by admin policy and matched by an allowlist.
Made denied CIDRs/domains take precedence over allowlists.
Disabled redirects and proxy use for alert target delivery.
Pinned resolved DNS addresses into reqwest before sending.
Added focused unit tests for the important policy decision paths.
This PR has:
Summary by CodeRabbit
New Features
Bug Fixes