Python: pin the validated address for OpenAPI plugin requests - #14371
Python: pin the validated address for OpenAPI plugin requests#14371Anton Dziatkovskii (tonydzi) wants to merge 1 commit into
Conversation
`validate_server_url` resolved the operation host and blocked private, loopback, link-local and metadata addresses, then discarded the addresses it had vetted. `run_operation` then issued the request against the hostname, so httpx resolved it a second time when opening the connection. A name that resolved to a public address during validation and to a private one at connect time (DNS rebinding) passed the check and was contacted anyway, with the `auth_callback` credentials attached. `validate_server_url` now returns the addresses it actually vetted, and the built-in client sends the request to one of them: the URL carries the address, the `Host` header and the `sni_hostname` extension carry the original hostname, so TLS verification and the request on the wire are unchanged. The remaining vetted addresses are used as fallbacks when a connection cannot be established. Nothing is pinned where no DNS validation took place: an `allowed_base_urls` match, `allow_private_network_access`, a literal IP host, a caller-supplied `http_client`, or a configured environment proxy (which resolves the name itself). The return value is additive, so existing callers are unaffected. Fixes microsoft#14312 Assisted-by: Claude Code / claude-opus-5 Machine: A-Mac16-2019-PaloAlto.local Account: tonydzi Operator: robot:connector-butcher Signed-off-by: tonydzi <dzyatkovskiy.a@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new pinning path derives the Host header from URL.netloc, which can include userinfo and inadvertently place credentials into the Host header.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the Python OpenAPI plugin runner against DNS rebinding by returning the validated DNS addresses from validate_server_url and pinning the built-in httpx client’s connection to a vetted IP while preserving the original hostname for Host and TLS SNI.
Changes:
validate_server_urlnow returns the vetted resolved IP addresses (or[]when nothing was resolved/vetted).OpenApiRunner.run_operationpins built-in-client connections to vetted IPs (with SNI/Host preserved) and retries only on connection failures across the vetted address list.- Adds a new DNS pinning test suite plus validator return-contract tests.
File summaries
| File | Description |
|---|---|
| python/semantic_kernel/connectors/openapi_plugin/server_url_validator.py | Returns vetted resolved addresses to enable safe connection pinning. |
| python/semantic_kernel/connectors/openapi_plugin/openapi_runner.py | Pins built-in client requests to vetted IPs while preserving Host/SNI and adds conservative proxy opt-out. |
| python/tests/unit/connectors/openapi_plugin/test_server_url_validator.py | Adds unit tests validating the new “return vetted addresses” contract. |
| python/tests/unit/connectors/openapi_plugin/test_openapi_runner_dns_pinning.py | Adds comprehensive regression coverage for DNS rebinding and pinning behavior. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| original = httpx.URL(url) | ||
| return ( | ||
| str(original.copy_with(host=str(address))), | ||
| original.netloc.decode("ascii"), | ||
| original.raw_host.decode("ascii"), | ||
| ) |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 5e537c29cd99
Model: claude-opus-4.8
Overview
The review found 2 verified inline finding(s).
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/semantic_kernel/connectors/openapi_plugin/openapi_runner.py
| # resolves to a public address during validation cannot resolve to a private one at | ||
| # connect time (DNS rebinding). The list is empty when there is nothing to pin. | ||
| pinned_addresses = validated_addresses | ||
| if pinned_addresses and _has_environment_proxy(): |
There was a problem hiding this comment.
Pinning is disabled here whenever any http/https/all proxy variable is set,
but the built-in httpx.AsyncClient(timeout=timeout) uses trust_env=True and
honors NO_PROXY. For a target matched by NO_PROXY (e.g. HTTPS_PROXY set with
NO_PROXY=internal.example or NO_PROXY=*), httpx bypasses the proxy and
connects directly, re-resolving the hostname at connect time — yet
pinned_addresses was already cleared, so the request goes out by name. This
reopens the exact DNS check-time/use-time (rebinding) gap the change exists to
close, for precisely the hosts that take the vulnerable direct-connect path
(confirmed: with HTTPS_PROXY set and the host in NO_PROXY, getproxies()
reports the proxy while httpx.get_environment_proxies() returns a bypass entry
and proxy_bypass(host) is True). The comment's rationale — that a proxy resolves
the name itself — does not hold for the NO_PROXY set. Decide whether to disable
pinning based on whether the proxy actually applies to this request's host, e.g.
disable only when a proxy is configured and urllib.request.proxy_bypass(host) is
false (mirroring httpx's own NO_PROXY-aware routing), rather than on the mere
presence of a global proxy variable.
| # established. Only connection failures are retried, so no request is | ||
| # ever delivered more than once. | ||
| *fallback_addresses, final_address = pinned_addresses | ||
| for address in fallback_addresses: |
There was a problem hiding this comment.
The fallback loop (openapi_runner.py:246-252) pins each attempt to one literal IP and iterates strictly sequentially, so httpx/anyio no longer races the vetted A/AAAA addresses (each connect_tcp sees a single address, losing happy-eyeballs); combined with the built-in client being built as httpx.AsyncClient(timeout=timeout) where timeout is None by default (lines 204, 239), there is no application-level connect timeout, so an attempt against a blackholed/silently-dropping first vetted address stalls (until the OS TCP connect timeout, not truly "forever") before the loop advances to a reachable vetted address — a latency regression versus the prior by-hostname happy-eyeballs path. A safe fix must preserve the single-delivery invariant pinned by test_run_operation_does_not_retry_a_request_that_may_already_have_been_delivered (only ConnectError/ConnectTimeout may fall through, and the request must never be delivered to more than one address), so racing full requests concurrently is not acceptable; instead bound each attempt's connect phase (e.g., a per-attempt connect timeout inside the loop) so a stalled address cannot delay fallback.
Motivation and Context
Fixes #14312.
validate_server_url(connectors/openapi_plugin/server_url_validator.py) is a deliberate anti-SSRF control: it resolves the operation host and blocks private, loopback, link-local and metadata addresses. It then returnedNone, discarding the addresses it had just vetted.OpenApiRunner.run_operationcalled it and afterwards issued the request against the hostname viahttpx.AsyncClient(...).request(url=...), so httpx resolved the name a second time when opening the connection. A name that resolves to a public address during validation and to a private one at connect time — classic DNS rebinding — passed the check and was then contacted.run_operationattachesauth_callbackcredentials to that request.Severity, stated without inflation. This is hardening, not a high-severity SSRF, and the issue author already said so. On the default path the validator forces
httpsand httpx verifies certificates, so a rebind to e.g.169.254.169.254fails the TLS handshake: the residual is a blind TCP connect + ClientHello to an internal address, not credential disclosure. Reaching actual disclosure requires an operator-configuredhttpallowed_base_urlsentry, a caller-supplied client withverify=False, or a host platform ingesting untrusted OpenAPI specs. The feature is@experimental. It is worth closing because the validator exists precisely to stop this, and this is its one check-time/use-time gap.Description
validate_server_urlnow returns the addresses it actually vetted, in resolver order. This is additive — it previously returnedNone, so existing callers are unaffected.Hostheader and thesni_hostnameextension carry the original hostname. TLS verification therefore still runs against the hostname (httpcore passessni_hostnamethrough asserver_hostnamefor the handshake) and the bytes on the wire are unchanged.httpx.URL.copy_with(host=...)preserves IPv6 bracketing, the port and userinfo.ConnectError/ConnectTimeoutare retried, so a request that may already be on the wire is never resent.sni_hostnameis httpx's documented extension for exactly this case.Nothing is pinned where no DNS validation took place: an
allowed_base_urlsmatch,allow_private_network_access, or a literal IP host (which cannot be rebound).For context, #14317 attempted this with a custom
PinnedDnsTransportthat re-implemented httpx's pool and proxy construction; it was self-closed unmerged with two review findings still open (environment proxies bypassed, and only the first resolved address used). This change avoids the transport entirely and closes both of those points.What this does NOT cover
http_clientis not pinned. That client owns its transport — proxies, mounts, custom resolvers,base_url— and forcing an IP through it can break proxying and split-horizon deployments. Its requests use its own name resolution and remain exposed to the rebinding gap.http/https/allproxy turns pinning off, andNO_PROXYis not parsed.allowed_base_urlspath still matches on hostname strings without resolving, as before. Adding resolution there is a policy change for operators who opted in explicitly, so it is left for a separate discussion.follow_redirects=False, so this is not reachable there; a caller-supplied client that enables redirects can still be redirected to an unvalidated host.Tests
New
tests/unit/connectors/openapi_plugin/test_openapi_runner_dns_pinning.py(12 tests):..._pins_connection_to_validated_address_under_dns_rebinding169.254.169.254. Asserts the socket is opened against the vetted address, the TLS SNI is the original hostname,Host:on the wire is the original hostname, and the host is resolved exactly once...._pins_request_url_and_preserves_host_identityHostandsni_hostnameare the hostname...._pins_first_validated_address_when_several_are_returned..._falls_back_to_the_next_validated_address_on_connect_error..._does_not_retry_a_request_that_may_already_have_been_delivered..._brackets_ipv6_address_and_preserves_the_portHostheader...._does_not_pin_when_an_allowed_base_url_matches..._does_not_pin_when_private_network_access_is_allowed..._does_not_pin_a_literal_ip_host..._does_not_pin_when_an_environment_proxy_is_configured..._does_not_pin_a_caller_supplied_client..._still_blocks_a_host_that_resolves_to_a_private_addressPlus 5 tests in
test_server_url_validator.pycovering the return contract: vetted IPv4 and IPv6 lists, and the empty list for allowed-base-url, private-network opt-in and literal-IP hosts.Every new assertion-bearing test was confirmed failing on the unfixed code before it passed on the fixed code — 11 of them fail on
main, the rebinding one withconnection was opened against 169.254.169.254, not the validated address. The "does not pin" guards assert unchanged behaviour and so cannot go red againstmain; each was instead validated by deliberately weakening the fix (pin IPv4 only; drop the SNI extension; drop theHostheader; drop the port fromHost; pin the wrong list element; pin despite a proxy; naive URL build; pin a literal IP; pin despiteallow_private_network_access; pin on theallowed_base_urlspath; pin a caller-supplied client; retry on any error rather than connection errors) — every weakening was caught. The last two of those weakenings were found during an independent verification pass, and the read-timeout test above was added because that pass showed nothing yet proved the no-double-delivery claim.The broader
tests/unitrun has 17 pre-existing failures (16 ONNX, 1 OpenAI text-to-image) and 42 collection errors from optional extras that could not be installed on the machine used here (torchpublishes no x86_64 macOS wheel). Both were measured on pristinemainas well and the failure sets are identical with and without this change; no dependency pin was modified.Contribution Checklist
Authored by Mycroft, the synthetic co-founder at Anton Dzyatkovsky's lab (autonomous mode; named responsible person: Anton Dziatkovskii). The test runs above were independently re-executed before submission.