fix(openapi_plugin): block Azure WireServer and IPv6-embedded private addresses in server URL validation

[Diwakar Ray Yadav (Diwak4r)]

Description

The OpenAPI plugin server URL validator could be bypassed to reach cloud metadata endpoints in two ways:

1. Azure WireServer (168.63.129.16) is publicly routable. Unlike the AWS (169.254.169.254) and GCP (metadata.google.internal) endpoints, the Azure WireServer IP falls in no private range, so try_categorize_non_public_address treated it as public and validate_server_url allowed it.
2. IPv4 addresses embedded in IPv6 were classified purely as IPv6. A link-local or private IPv4 smuggled inside a NAT64 (64:ff9b::/96, 64:ff9b:1::/48), 6to4 (2002::/16), or Teredo (2001::/32) address looked like a benign public IPv6 address and passed validation.

Changes

• Denylist the Azure WireServer endpoint in _try_classify_ipv4, and enforce it even when allow_private_network_access=True — being allowed to reach your own network is not the same as being allowed to reach the host agent's credential endpoint. The private-mode check only inspects literal IP hosts (private mode deliberately does not resolve hostnames).
• Decode IPv4 addresses embedded in IPv6 (sixtofour, teredo, and the RFC 6052 NAT64 prefixes) in try_categorize_non_public_address so the embedded address is classified with the IPv4 rules.
• Add tests covering the WireServer endpoint (literal, DNS-resolved, NAT64- and 6to4-embedded), all three IPv6 embedding forms, and the private-network-access override.

Test plan

pytest tests/unit/connectors/openapi_plugin/test_server_url_validator.py — 62 passed (10 new tests added).

Fixes #14240

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

Automated Code Review

Reviewers: 5 | Confidence: 74% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach

---

Automated review by Diwak4r's agents

[Diwakar Ray Yadav (Diwak4r)]

Just verifying this is still in the review queue. All checks green, branch is mergeable. Happy to address any feedback.

[ErenAta16 (ErenAta16)]

Heads up on one thing here, because I hit exactly the same bug in #14242 and only found it by measuring.

_NAT64_PREFIXES includes 64:ff9b:1::/48 and _extract_embedded_ipv4 reads address.packed[-4:] for anything inside it. RFC 6052 section 2.2 only puts the embedded IPv4 in the last four bytes for a /96 embedding; for /48, /56 and /64 those bytes are the suffix, which the RFC says SHOULD be zero. So an ordinary public target embedded at any length but /96 decodes as 0.0.0.0 and gets blocked as unspecified:

8.8.8.8 via /96   64:ff9b:1::808:808        -> 8.8.8.8    allowed
8.8.8.8 via /48   64:ff9b:1:808:8:800::     -> 0.0.0.0    blocked
1.1.1.1 via /48   64:ff9b:1:101:1:100::     -> 0.0.0.0    blocked

The tempting fix, only decoding when the address really is /96-shaped, swings it the other way: 169.254.169.254 embedded at /48, /56 or /64 then passes the check entirely, which is worse than the false positive. Decoding all four lengths does not work either, because each wrong reading still produces some IPv4 and those land in blocked ranges often enough to be noise.

The reason none of it works is RFC 6052 section 3.3: the prefix length is configuration held by the translator and is not carried in the address, so there is nothing to infer it from.

What I ended up with, if it is useful to you: keep the decode for 64:ff9b::/96, whose length RFC 6052 section 2.1 fixes, and treat the RFC 8215 local-use /48 as non-public in its own right rather than guessing at its contents. That is uniform in both directions, and it states the policy instead of arriving at it through a zero suffix.

Separately, thank you for address.sixtofour / address.teredo. Mine was doing the offset arithmetic and the Teredo XOR by hand, and yours is plainly better since those properties return None outside their prefixes and carry the membership test with them. I checked the two against each other on 40000 random addresses per form, found no disagreement, and have switched to the standard library ones with a pointer back to this PR.

There is real overlap between this and #14242, which I opened on 29 July. I am not asking you to close anything, that is for the maintainers to sort out, but the NAT64 detail is worth fixing in whichever one moves.

[Diwakar Ray Yadav (Diwak4r)]

Thanks for the careful read, you're right, and I've applied exactly the approach you suggested.

_extract_embedded_ipv4 now decodes only the well-known 64:ff9b::/96 prefix (RFC 6052 fixes its length, so the last 32 bits are always the embedded IPv4), and the local-use 64:ff9b:1::/48 range (RFC 8215) is blocked as its own non-public category - NAT64 local-use prefix (RFC 8215) - rather than decoded. That closes the /48 smuggling hole (a WireServer or link-local address embedded at /48 is still blocked, as a range policy) while the /48 0.0.0.0 false positives disappear.

Added regression tests for all of it: the local-use range blocks (including 64:ff9b:1:a83f:8110::, WireServer embedded at /48), and 64:ff9b::808:808 / 64:ff9b::101:101 stay allowed through the /96 decode. The address.sixtofour / address.teredo properties are unchanged - agreed they carry the membership test for free.

And thanks for the pointer on #14242 - happy to defer to the maintainers on how to handle the overlap; this is the behavior I'd keep in whichever PR moves forward.

[ErenAta16 (ErenAta16)]

That is the right split, and putting the RFC reasoning in the comment rather than only in the PR body is what will stop someone "simplifying" the two branches back into one later.

Measured the behaviour at d5bd363b rather than reading it:

ranges overlap?  False

64:ff9b::169.254.169.254   /96=True   /48=False   -> 169.254.169.254
64:ff9b::a83f:8110         /96=True   /48=False   -> 168.63.129.16
64:ff9b::808:808           /96=True   /48=False   -> 8.8.8.8
64:ff9b:1::                /96=False  /48=True
64:ff9b:1:a83f:8110::      /96=False  /48=True
64:ff9b:1:101:1:100::      /96=False  /48=True

Two things worth having on the record.

The prefixes are disjoint, so neither check can shadow the other and the order of the two branches is not load-bearing. That is worth knowing because it is the kind of thing a later refactor might reorder without thinking, and here it is safe.

More importantly, and this is the one I would put in a comment if it is not already: the decoded WireServer address is not caught by any generic rule.

168.63.129.16   is_private=False   is_link_local=False

So the explicit WireServer entry is not redundant with the private-range check, and anyone reviewing this who assumes "the private-address branch already covers Azure metadata" would be wrong. 169.254.169.254 decodes to link-local and would be caught either way; 168.63.129.16 would sail through. That asymmetry is the whole reason the WireServer rule has to exist separately, and it is invisible from reading the diff.

64:ff9b::808:808 decoding to 8.8.8.8 and staying allowed is the control I wanted to see, and it behaves.

No further concerns from me on the NAT64 half.

[Diwakar Ray Yadav (Diwak4r)]

Thanks for measuring it at the head commit - that table is exactly the evidence this kind of change needs.

Both points are now on the record in the code: the WireServer comment block spells out that 168.63.129.16 has is_private=False and is_link_local=False (so no generic rule catches it, not even after NAT64 decoding via 64:ff9b::a83f:8110), and that the explicit entry is load-bearing rather than redundant with the range checks. The prefix-disjointness point is covered by the existing RFC citations on each prefix constant.

Comment-only commit: 96d71d7 (+6 lines).

[ErenAta16 (ErenAta16)]

That comment is better than what I asked for.

Naming the wrong assumption outright, "do not remove it on the assumption that
the private-address branch already covers Azure metadata", is what makes it
survive a refactor. A comment that only states the fact tends to get deleted by
whoever is tidying up, precisely because the fact reads as redundant to someone
who already holds the wrong belief.

Nothing further from me on the NAT64 or WireServer half.

[Diwakar Ray Yadav (Diwak4r)]

Thanks for the close read-through - glad the comment lands the way you wanted. I will leave the PR as is then, and watch for the workflow-approval gate on your side. Happy to rebase if main moves before then.