refactor(callouts): standardize target and credential policy - #911
refactor(callouts): standardize target and credential policy#911eoinfennessy wants to merge 11 commits into
Conversation
|
Missing Signed-off-by: ab4170c. All commits require sign-off (via |
Signed-off-by: Eoin Fennessy <efenness@redhat.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
ab4170c to
8681eef
Compare
033fa48 to
c271be9
Compare
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
c271be9 to
a5f6c2f
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
refactor(callouts): standardize target and credential policy
Assessment
Well-structured refactoring that centralizes outbound callout target validation and credential binding across all affected filters. The shared callout_target module is clean and the security model (config-time structural validation + connect-time address pinning) is sound. The credential-origin binding in ApiClient is a valuable security hardening.
Findings
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 1 |
| Medium | 3 |
Non-inline findings
[Large] callout_target::validate_configured_http_target no longer catches legacy/non-standard IPv4 literals (octal 0177.0.0.1, hex 0x7f000001, mixed-radix forms) that the removed parse_legacy_ipv4_host in url.rs previously handled. The url::Url crate and IpAddr::parse() both treat these as DNS hostnames, so they pass config-time validation and are deferred to connect-time resolution. Whether a given DNS resolver or libc getaddrinfo expands these into 127.0.0.1 is platform-dependent. The connect-time validate_resolved_addrs will catch it if the resolver expands the literal, but if it does not (returning NXDOMAIN), the request simply fails -- so it is not exploitable. Still, the explicit defense-in-depth layer was removed without replacement. Consider re-adding parse_legacy_ipv4_host (or a simplified version) to callout_target.rs and rejecting these forms at config time under PublicOnly, matching the prior behavior.
[Medium] callout_target::validate_configured_http_target strips brackets from host_str() via strip_prefix('[') / strip_suffix(']'), but url::Url::host_str() already returns IPv6 addresses without brackets (the url crate normalizes them). This branch is dead code -- the strip never matches. Remove the bracket stripping or add a code comment explaining why it is retained as a defensive measure against future url crate changes.
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
Signed-off-by: Eoin Fennessy <efenness@redhat.com>
leseb
left a comment
There was a problem hiding this comment.
Findings, these are mostly refactor regressions:
Major — static Host bypasses target-authority binding
filters/src/callout/mod.rs:233
A configured static Host survives because the shared executor only supplies the URL authority when Host is absent. Credentials may therefore reach a different virtual host. Reject static Host or overwrite it with the configured authority, with a wire-level regression test.
Minor — empty URL userinfo markers are accepted
apis/src/callout_target.rs:68
http://@example.com and http://:@example.com pass url::Url validation, but later http::Uri parsing preserves a divergent Host authority. Reject userinfo delimiters even when username/password are empty.
Minor — generated compact-filter examples are invalid
docs/filters/openai_responses_compact.md:22
Both examples use localhost without the now-required allow_private_inference_url: true. Update the source snippets and regenerate documentation.
Minor — new assertions lack diagnostic messages
apis/src/callout_target.rs:272
Several new bare assert! calls violate the attached review contract and declared workspace lint policy. The pattern also occurs in API-client, web-search, and callout tests.
Summary
Standardize outbound AI callout target resolution, validation, and credential handling across the affected filters. This establishes shared policy primitives while preserving the distinction between operator-configured upstreams and request-derived targets.
Changes
SubRequestClient.Security considerations
Credentials and forwarded headers remain bound to validated destinations. Redirects and destination changes must not disclose credentials to another origin, and request-derived targets are not weakened to share configured-upstream behavior.
Closes #696