-
Notifications
You must be signed in to change notification settings - Fork 0
fix(noema): fail closed at the credential egress boundary #1279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
seonghobae
wants to merge
5
commits into
main
Choose a base branch
from
codex/pr930-current-main-replacement-20260824
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+873
−25
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8c3375f
fix(noema): close credential egress boundary
seonghobae 111b3ae
fix(noema): pin credential egress to validated addresses
seonghobae 2adc8c4
test(noema): cover pinned connection failure paths
seonghobae 721a36f
fix(noema): pass IPv6 literals to socket connection
seonghobae 3feb583
Merge remote-tracking branch 'origin/main' into codex/pr930-current-m…
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # Noema credential-egress boundary | ||
|
|
||
| ## Customer outcome | ||
|
|
||
| Noema now rejects a misconfigured or attacker-influenced model endpoint before | ||
| placing its bearer credential on the request. A public service must use HTTPS, | ||
| must resolve entirely to globally routable unicast addresses, and must retain | ||
| the same complete DNS address set after the bounded response is received. | ||
| Resolver failures, empty answers, malformed addresses, redirects, special-use | ||
| addresses, and response bodies larger than 1 MiB fail closed. | ||
|
|
||
| The existing same-job contextual-orchestrator seam remains usable without | ||
| importing the orchestrator into this repository: literal `127.0.0.1` and `::1` | ||
| endpoints may use HTTP only when every resolver result is loopback. Hostnames | ||
| that merely resolve to loopback do not receive this exception. Provider routing, | ||
| model selection, and model-parameter translation remain upstream concerns. | ||
|
|
||
| ## Decision and trust boundary | ||
|
|
||
| The implementation reuses Python's `urllib.parse`, `socket.getaddrinfo`, and | ||
| `ipaddress` rather than adding a URL or address-classification dependency. | ||
| Before request construction it: | ||
|
|
||
| 1. rejects non-HTTP schemes, URL user information, and missing hostnames; | ||
| 2. restricts plaintext HTTP to the two literal loopback sidecar addresses; | ||
| 3. resolves the endpoint for its effective port and rejects failed, empty, or | ||
| malformed resolution evidence; and | ||
| 4. requires every non-loopback address to be globally routable unicast. | ||
|
|
||
| Redirects remain disabled, and the opener disables ambient proxies. Custom HTTP | ||
| and HTTPS connections connect only to the validated numeric addresses while | ||
| retaining the original hostname for HTTPS certificate validation. The response | ||
| reader requests at most one byte beyond the 1 MiB contract, rejects an | ||
| over-limit result before decoding JSON, and then re-resolves the same host and | ||
| port. A changed address set invalidates the result. Tests cover IPv4 and IPv6 | ||
| loopback, dual-stack public endpoints, pinned numeric TCP destinations, TLS SNI, | ||
| DNS rebinding evidence, resolver failures, malformed results, URL credentials, | ||
| special-use address classes, and the byte limit. | ||
|
|
||
| Trusted DNS and network egress controls remain defense-in-depth for production; | ||
| the application boundary now also prevents the request socket from performing | ||
| an unvalidated hostname resolution. | ||
|
|
||
| ## References | ||
|
|
||
| Cotton, M., Vegoda, L., Bonica, R., & Haberman, B. (2013). *Special-purpose IP | ||
| address registries* (RFC 6890). RFC Editor. https://doi.org/10.17487/RFC6890 | ||
|
|
||
| MITRE. (2026a). *CWE-400: Uncontrolled resource consumption*. | ||
| https://cwe.mitre.org/data/definitions/400.html | ||
|
|
||
| MITRE. (2026b). *CWE-918: Server-side request forgery (SSRF)*. | ||
| https://cwe.mitre.org/data/definitions/918.html | ||
|
|
||
| OWASP Foundation. (n.d.). *Server-side request forgery prevention cheat sheet*. | ||
| Retrieved August 24, 2026, from | ||
| https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| import argparse | ||
| import base64 | ||
| import http.client | ||
| import ipaddress | ||
| import json | ||
| import os | ||
|
|
@@ -41,6 +42,8 @@ | |
| MAX_FILE_CONTEXT_CHARS = 4000 | ||
| MAX_REVIEW_CONTEXT_CHARS = 24000 | ||
| MAX_THREAD_BODY_CHARS = 1200 | ||
| MAX_LLM_RESPONSE_BYTES = 1024 * 1024 | ||
| IpAddress = ipaddress.IPv4Address | ipaddress.IPv6Address | ||
|
|
||
| ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) | ||
| ORCHESTRATOR_BASE_ENV = "CONTEXTUAL_ORCHESTRATOR_BASE_URL" | ||
|
|
@@ -426,6 +429,177 @@ def redirect_request( | |
| raise urllib.error.HTTPError(req.full_url, code, msg, headers, fp) | ||
|
|
||
|
|
||
| def _socket_target(address: IpAddress, port: int) -> tuple[str, int]: | ||
| """Return a socket destination that contains only a validated IP literal.""" | ||
| return (str(address), port) | ||
|
|
||
|
|
||
| class _PinnedConnectionMixin: | ||
| """Connect only to the DNS addresses validated before the request.""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| *args: Any, | ||
| validated_addresses: frozenset[IpAddress], | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| """Store immutable DNS evidence before initializing the HTTP connection.""" | ||
| if not validated_addresses: | ||
| raise ValueError("Noema endpoint has no validated DNS addresses") | ||
| self._validated_addresses = tuple( | ||
| sorted(validated_addresses, key=lambda address: (address.version, address.packed)) | ||
| ) | ||
| super().__init__(*args, **kwargs) | ||
|
|
||
| def _connect_to_validated_address(self) -> None: | ||
| """Open a socket using a validated numeric destination, never the hostname.""" | ||
| last_error: OSError | None = None | ||
| for address in self._validated_addresses: | ||
| try: | ||
| self.sock = socket.create_connection( | ||
| _socket_target(address, self.port), | ||
| self.timeout, | ||
| self.source_address, | ||
| ) | ||
| return | ||
| except OSError as exc: | ||
| last_error = exc | ||
| raise OSError("Noema endpoint could not connect to validated DNS addresses") from last_error | ||
|
|
||
|
|
||
| class PinnedHTTPConnection(_PinnedConnectionMixin, http.client.HTTPConnection): | ||
| """HTTP connection that cannot resolve the configured hostname a second time.""" | ||
|
|
||
| def connect(self) -> None: | ||
| """Connect to a validated address and preserve proxy tunnel behavior.""" | ||
| self._connect_to_validated_address() | ||
| if self._tunnel_host: | ||
| self._tunnel() | ||
|
|
||
|
|
||
| class PinnedHTTPSConnection(_PinnedConnectionMixin, http.client.HTTPSConnection): | ||
| """HTTPS connection pinned to validated addresses while retaining hostname TLS.""" | ||
|
|
||
| def connect(self) -> None: | ||
| """Connect to a validated address, then verify the original hostname in TLS.""" | ||
| self._connect_to_validated_address() | ||
| if self._tunnel_host: | ||
| self._tunnel() | ||
| server_hostname = self._tunnel_host or self.host | ||
| self.sock = self._context.wrap_socket(self.sock, server_hostname=server_hostname) | ||
|
seonghobae marked this conversation as resolved.
seonghobae marked this conversation as resolved.
|
||
|
|
||
|
|
||
| class PinnedHTTPHandler(urllib.request.HTTPHandler): | ||
| """urllib handler that uses validated numeric destinations for HTTP requests.""" | ||
|
|
||
| def __init__(self, addresses: frozenset[IpAddress]) -> None: | ||
| """Bind this handler to one prevalidated DNS result set.""" | ||
| super().__init__() | ||
| self._addresses = addresses | ||
|
|
||
| def http_open(self, req: urllib.request.Request) -> Any: | ||
| """Open an HTTP request without resolving its hostname again.""" | ||
| return self.do_open( | ||
| lambda host, **kwargs: PinnedHTTPConnection( | ||
| host, validated_addresses=self._addresses, **kwargs | ||
| ), | ||
| req, | ||
| ) | ||
|
|
||
|
|
||
| class PinnedHTTPSHandler(urllib.request.HTTPSHandler): | ||
| """urllib handler that pins TCP while preserving HTTPS hostname verification.""" | ||
|
|
||
| def __init__(self, addresses: frozenset[IpAddress]) -> None: | ||
| """Bind this handler to one prevalidated DNS result set.""" | ||
| super().__init__() | ||
| self._addresses = addresses | ||
|
|
||
| def https_open(self, req: urllib.request.Request) -> Any: | ||
| """Open HTTPS using the validated address set and original URL hostname.""" | ||
| return self.do_open( | ||
| lambda host, **kwargs: PinnedHTTPSConnection( | ||
| host, validated_addresses=self._addresses, **kwargs | ||
| ), | ||
| req, | ||
| context=self._context, | ||
| ) | ||
|
Comment on lines
+492
to
+526
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
|
|
||
| def resolve_endpoint_addresses( | ||
| hostname: str, | ||
| port: int, | ||
| ) -> frozenset[IpAddress]: | ||
| """Resolve every stream address for an endpoint or fail closed.""" | ||
| try: | ||
| addrinfo = socket.getaddrinfo(hostname, port, type=socket.SOCK_STREAM) | ||
| except (OSError, UnicodeError) as exc: | ||
| raise ValueError("Noema endpoint DNS resolution failed") from exc | ||
| if not addrinfo: | ||
| raise ValueError("Noema endpoint DNS resolution returned no addresses") | ||
|
|
||
| addresses: set[IpAddress] = set() | ||
| for result in addrinfo: | ||
| try: | ||
| raw_address = result[4][0] | ||
| address = ipaddress.ip_address(raw_address) | ||
| except (IndexError, TypeError, ValueError) as exc: | ||
| raise ValueError("Noema endpoint DNS resolution returned a malformed address") from exc | ||
| addresses.add(address) | ||
| return frozenset(addresses) | ||
|
|
||
|
|
||
| def validate_endpoint( | ||
| api_url: str, | ||
| ) -> tuple[str, int, frozenset[IpAddress]]: | ||
| """Validate transport and pre-request DNS evidence for a model endpoint. | ||
|
|
||
| The narrow same-job loopback exception is the exact-origin | ||
| ``is_allowed_orchestrator_sidecar_url`` allowlist: a bare ``127.0.0.1`` or | ||
| ``::1`` literal is not enough on its own, it must also match the | ||
| configured ``CONTEXTUAL_ORCHESTRATOR_BASE_URL`` origin exactly. Every | ||
| other target — including any other loopback literal or port — must use | ||
| HTTPS and resolve only to globally routable unicast DNS evidence. | ||
| """ | ||
| if not (api_url.lower().startswith("http://") or api_url.lower().startswith("https://")): | ||
| raise ValueError( | ||
| "URL scheme must be http or https; NOEMA_LLM_API_URL must start " | ||
| "with http:// or https:// to prevent SSRF vulnerabilities" | ||
| ) | ||
| parsed = urllib.parse.urlparse(api_url) | ||
| scheme = parsed.scheme.lower() | ||
| if scheme not in {"http", "https"}: | ||
| raise ValueError( | ||
| "URL scheme must be http or https; NOEMA_LLM_API_URL must start " | ||
| "with http:// or https://" | ||
| ) | ||
| hostname = (parsed.hostname or "").lower() | ||
| if not hostname: | ||
| raise ValueError("URL must have a valid hostname") | ||
| if parsed.username is not None or parsed.password is not None: | ||
| raise ValueError("Noema endpoint URL cannot contain user information") | ||
|
|
||
| trusted_sidecar = is_allowed_orchestrator_sidecar_url(api_url) | ||
| if not trusted_sidecar: | ||
| if hostname in {"localhost", "localhost.localdomain"} or hostname.endswith(".localhost"): | ||
| raise ValueError("URL cannot target localhost") | ||
| if scheme != "https": | ||
| raise ValueError("Noema non-loopback endpoints must use HTTPS") | ||
|
|
||
| port = parsed.port or (443 if scheme == "https" else 80) | ||
| addresses = resolve_endpoint_addresses(hostname, port) | ||
| if trusted_sidecar: | ||
| if any(not address.is_loopback for address in addresses): | ||
| raise ValueError("Noema loopback endpoint DNS resolution left loopback") | ||
| elif any( | ||
| not address.is_global or address.is_multicast for address in addresses | ||
| ): | ||
|
seonghobae marked this conversation as resolved.
|
||
| raise ValueError( | ||
| "Noema non-loopback endpoint DNS must contain only globally routable unicast addresses" | ||
| ) | ||
| return hostname, port, addresses | ||
|
seonghobae marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def extract_json_object(text: str) -> dict[str, Any]: | ||
| """Extract a JSON object from a strict or lightly wrapped LLM response.""" | ||
| stripped = text.strip() | ||
|
|
@@ -539,7 +713,7 @@ def call_llm( | |
| model = os.environ.get("NOEMA_LLM_MODEL", "").strip() or "noema-default" | ||
| if not api_url or not api_key: | ||
| raise RuntimeError("Noema LLM review unavailable: NOEMA_LLM_API_URL or NOEMA_LLM_API_KEY is not configured.") | ||
| reject_private_llm_url(api_url) | ||
| hostname, port, addresses = validate_endpoint(api_url) | ||
|
|
||
| prompt = { | ||
| "role": "user", | ||
|
|
@@ -579,9 +753,21 @@ def call_llm( | |
| }, | ||
| method="POST", | ||
| ) | ||
| opener = urllib.request.build_opener(NoRedirectHandler()) | ||
| opener = urllib.request.build_opener( | ||
| # Force the pinned direct path; proxy destinations have not passed this | ||
| # endpoint's DNS policy and must not receive the bearer credential. | ||
| urllib.request.ProxyHandler({}), | ||
| PinnedHTTPHandler(addresses), | ||
| PinnedHTTPSHandler(addresses), | ||
| NoRedirectHandler(), | ||
| ) | ||
| with opener.open(request, timeout=120) as response: # nosec B310 | ||
| raw = response.read().decode("utf-8") | ||
| raw_bytes = response.read(MAX_LLM_RESPONSE_BYTES + 1) | ||
| if len(raw_bytes) > MAX_LLM_RESPONSE_BYTES: | ||
| raise RuntimeError("Noema LLM response exceeded the byte limit") | ||
| if resolve_endpoint_addresses(hostname, port) != addresses: | ||
| raise ValueError("Noema endpoint DNS addresses changed during the request") | ||
|
seonghobae marked this conversation as resolved.
Comment on lines
+768
to
+769
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| raw = raw_bytes.decode("utf-8") | ||
| data = json.loads(raw) | ||
| content = (((data.get("choices") or [{}])[0].get("message") or {}).get("content") or "").strip() | ||
| verdict = extract_json_object(content) | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: IPv6 destination shape is valid
socket.create_connectionaccepts a two-element host-and-port pair for IPv6 literals. The four-element form applies to lower-level socket addresses, not this API.Was this helpful? React with 👍 or 👎 to provide feedback.