From 7805fb7597138c176aff633cffcdc095961cb24e Mon Sep 17 00:00:00 2001 From: Paulo Date: Mon, 7 Sep 2026 12:54:36 +0200 Subject: [PATCH] Swap every header that carries a placeholder The proxy stopped at the first placeholder it found, so a request with two secret headers sent the second placeholder to its destination as it was. The addon now collects every placeholder from the original headers, authorizes each one for the host, and replaces them all at once. One refusal refuses the whole request, an exchange that gives no answer gets a 503 before anything is sent, and two placeholders that resolve to the same header refuse the request. A credential the exchange returns is never scanned as a placeholder. The security and architecture docs say so. Header names compare without case, as mitmproxy keeps them, and the placeholders come from every field of a repeated name, not from the folded value. --- deploy/proxy/swap.py | 49 ++++-- docs/architecture.md | 3 +- docs/security.md | 11 +- src/secrets_exchange/tests/test_swap.py | 216 +++++++++++++++++++++++- 4 files changed, 255 insertions(+), 24 deletions(-) diff --git a/deploy/proxy/swap.py b/deploy/proxy/swap.py index c6fe87a..41e8d63 100644 --- a/deploy/proxy/swap.py +++ b/deploy/proxy/swap.py @@ -172,31 +172,46 @@ async def requestheaders(self, flow: http.HTTPFlow) -> None: if not is_reachable(await self.resolve(flow.request.host)): flow.response = http.Response.make(403, b"destination refused\n") else: - for name, value in flow.request.headers.items(): - if placeholder := placeholder_in(value): - await self.swap(flow, name, placeholder) - break + found = [ + (name, placeholder) + for name, value in flow.request.headers.items(multi=True) + if (placeholder := placeholder_in(value)) + ] + if found: + await self.swap(flow, found) flow.request.stream = not flow.response - async def swap(self, flow: http.HTTPFlow, name: str, placeholder: str) -> None: + async def swap(self, flow: http.HTTPFlow, found: list[tuple[str, str]]) -> None: + """Every placeholder is authorized first. Then all are replaced at once, + so a refusal leaves the request untouched and unsent. Header names + compare without case, as mitmproxy keeps them.""" # The authority the client sends must match the CONNECT host. approved = flow.server_conn.sni or flow.request.host authority = urllib.parse.urlsplit(f"//{flow.request.host_header or ''}").hostname if authority != approved.lower(): flow.response = http.Response.make(403, b"host does not match the connection\n") return - try: - header, credential = await self.exchange.authorize(placeholder, approved) - except Refused: - logger.info("refused a placeholder for %s", approved) - flow.response = http.Response.make(403, b"placeholder refused\n") - return - except ExchangeUnavailable as exc: - logger.warning("the exchange gave no answer for %s: %s", approved, exc) - flow.response = http.Response.make(503, b"exchange unavailable\n") - return - del flow.request.headers[name] - flow.request.headers[header] = credential + credentials: dict[str, tuple[str, str]] = {} + for _, placeholder in found: + try: + header, credential = await self.exchange.authorize(placeholder, approved) + except Refused: + logger.info("refused a placeholder for %s", approved) + flow.response = http.Response.make(403, b"placeholder refused\n") + return + except ExchangeUnavailable as exc: + logger.warning("the exchange gave no answer for %s: %s", approved, exc) + flow.response = http.Response.make(503, b"exchange unavailable\n") + return + if header.lower() in credentials: + logger.info("two placeholders for one header on %s", approved) + flow.response = http.Response.make(403, b"placeholders conflict\n") + return + credentials[header.lower()] = (header, credential) + for name in {name.lower() for name, _ in found}: + del flow.request.headers[name] + for header, credential in credentials.values(): + flow.request.headers[header] = credential def responseheaders(self, flow: http.HTTPFlow) -> None: flow.response.stream = True diff --git a/docs/architecture.md b/docs/architecture.md index 04e2f0e..d7c0adb 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -144,7 +144,8 @@ It terminates TLS only for the hosts the exchange lists at `/upstreams`, the hosts with a registered secret, and tunnels every other host blind. For a request with a placeholder it asks the exchange at `/authorize`, with the placeholder and the destination host, for the header the upstream reads and -the real credential. It swaps that one header and streams the request on. +the real credential. It swaps every header that carries a placeholder and +streams the request on. One refusal refuses the whole request. A destination that resolves to a loopback, private, link-local, or metadata address is refused, so a sandbox cannot reach the exchange or the API through the proxy. Every connection goes to the address the proxy checked, and the diff --git a/docs/security.md b/docs/security.md index 5525e28..e028c42 100644 --- a/docs/security.md +++ b/docs/security.md @@ -97,11 +97,12 @@ database and never returned by the API. The placeholder, `drk...`, works only at the secrets proxy, and only for the host and the service it names. The entry stores a -fingerprint of it, so a database read cannot replay it. The proxy swaps that -one header, on HTTPS to a registered host, and touches nothing else. Plain -HTTP is forwarded unchanged. The proxy refuses a loopback, private, link-local, -or metadata destination, so a box cannot reach the exchange or the API through -it. It logs no credential. +fingerprint of it, so a database read cannot replay it. The proxy swaps every +header that carries a placeholder, on HTTPS to a registered host, and touches +nothing else. One placeholder it cannot resolve refuses the whole request. +Plain HTTP is forwarded unchanged. The proxy refuses a loopback, private, +link-local, or metadata destination, so a box cannot reach the exchange or the +API through it. It logs no credential. The real value is encrypted in the database. It passes through the exchange and the proxy for one request, and the exchange keeps an issuer's value in diff --git a/src/secrets_exchange/tests/test_swap.py b/src/secrets_exchange/tests/test_swap.py index 9c19cb9..87eae43 100644 --- a/src/secrets_exchange/tests/test_swap.py +++ b/src/secrets_exchange/tests/test_swap.py @@ -21,6 +21,38 @@ PLACEHOLDER = "drk.0123abcd.anthropic.s3cret" +class Headers: + """mitmproxy's Headers, as far as the addon uses them: names compare + without case, a repeated name keeps every field, and ``items()`` folds + the fields of one name into one value.""" + + def __init__(self, *fields: tuple[str, str]) -> None: + self.fields = list(fields) + + def items(self, multi: bool = False) -> list[tuple[str, str]]: + if multi: + return list(self.fields) + names: dict[str, str] = {} + values: dict[str, list[str]] = {} + for name, value in self.fields: + names.setdefault(name.lower(), name) + values.setdefault(name.lower(), []).append(value) + return [(name, ", ".join(values[key])) for key, name in names.items()] + + def __delitem__(self, name: str) -> None: + kept = [field for field in self.fields if field[0].lower() != name.lower()] + if len(kept) == len(self.fields): + raise KeyError(name) + self.fields = kept + + def __setitem__(self, name: str, value: str) -> None: + self.fields = [field for field in self.fields if field[0].lower() != name.lower()] + self.fields.append((name, value)) + + def __eq__(self, other: object) -> bool: + return dict(self.items()) == other + + class FakeRequest: def __init__(self, url: str, headers: dict[str, str], request_timeout: float) -> None: self.url = url @@ -126,7 +158,7 @@ def _flow(scheme: str, host: str, headers: dict[str, str], connect_host: str = " host="104.18.0.1" if scheme == "https" else host, host_header=f"{host}:443" if scheme == "https" else host, port=443, - headers=headers, + headers=Headers(*headers.items()), stream=False, ), server_conn=SimpleNamespace(sni=connect_host if scheme == "https" else None), @@ -352,6 +384,188 @@ async def test_a_basic_placeholder_is_swapped_the_same_way(swap: ModuleType) -> assert flow.request.headers == {"Authorization": basic} +OTHER = PLACEHOLDER.replace(".anthropic.", ".acme.") +THIRD = PLACEHOLDER.replace(".anthropic.", ".third.") + + +@pytest.mark.parametrize("reverse", [False, True]) +async def test_every_header_with_a_placeholder_is_swapped(swap: ModuleType, reverse: bool) -> None: + exchange = FakeExchange( + swap, + {"api.acme.test"}, + { + (PLACEHOLDER, "api.acme.test"): ("Authorization", "Bearer real-one"), + (OTHER, "api.acme.test"): ("X-Acme-Key", "real-two"), + }, + ) + addon = _swap(swap, exchange, "104.18.0.1") + headers = {"Authorization": f"Bearer {PLACEHOLDER}", "X-Acme-Key": OTHER, "Accept": "*/*"} + if reverse: + headers = dict(reversed(headers.items())) + flow = _flow("https", "api.acme.test", headers) + + await addon.requestheaders(flow) + + assert flow.request.headers == { + "Authorization": "Bearer real-one", + "X-Acme-Key": "real-two", + "Accept": "*/*", + } + assert sorted(exchange.asked) == sorted( + [(PLACEHOLDER, "api.acme.test"), (OTHER, "api.acme.test")] + ) + assert not flow.response + + +async def test_a_credential_is_never_scanned_as_a_placeholder(swap: ModuleType) -> None: + exchange = FakeExchange( + swap, + {"api.acme.test"}, + {(PLACEHOLDER, "api.acme.test"): ("Authorization", f"Bearer {OTHER}")}, + ) + addon = _swap(swap, exchange, "104.18.0.1") + flow = _flow("https", "api.acme.test", {"Authorization": f"Bearer {PLACEHOLDER}"}) + + await addon.requestheaders(flow) + + assert flow.request.headers == {"Authorization": f"Bearer {OTHER}"} + assert exchange.asked == [(PLACEHOLDER, "api.acme.test")] + + +async def test_two_placeholders_for_one_header_refuse_the_request(swap: ModuleType) -> None: + exchange = FakeExchange( + swap, + {"api.acme.test"}, + { + (PLACEHOLDER, "api.acme.test"): ("Authorization", "Bearer real-one"), + (OTHER, "api.acme.test"): ("Authorization", "Bearer real-two"), + }, + ) + addon = _swap(swap, exchange, "104.18.0.1") + headers = {"Authorization": f"Bearer {PLACEHOLDER}", "X-Acme-Key": OTHER} + flow = _flow("https", "api.acme.test", headers) + + await addon.requestheaders(flow) + + assert flow.response.status_code == 403 + assert flow.request.headers == headers + assert flow.request.stream is False + + +async def test_a_refused_second_placeholder_leaves_the_request_untouched( + swap: ModuleType, +) -> None: + exchange = FakeExchange( + swap, + {"api.acme.test"}, + {(PLACEHOLDER, "api.acme.test"): ("Authorization", "Bearer real-one")}, + ) + addon = _swap(swap, exchange, "104.18.0.1") + headers = {"Authorization": f"Bearer {PLACEHOLDER}", "X-Acme-Key": OTHER} + flow = _flow("https", "api.acme.test", headers) + + await addon.requestheaders(flow) + + assert flow.response.status_code == 403 + assert flow.request.headers == headers + assert flow.request.stream is False + + +@pytest.mark.parametrize( + ("first", "second"), + [("Authorization", "authorization"), ("Authorization", "Authorization")], +) +async def test_one_destination_twice_is_refused_whatever_the_case_or_the_value( + swap: ModuleType, first: str, second: str +) -> None: + exchange = FakeExchange( + swap, + {"api.acme.test"}, + { + (PLACEHOLDER, "api.acme.test"): (first, "Bearer real-one"), + (OTHER, "api.acme.test"): (second, "Bearer real-one"), + }, + ) + addon = _swap(swap, exchange, "104.18.0.1") + headers = {"Authorization": f"Bearer {PLACEHOLDER}", "X-Acme-Key": OTHER} + flow = _flow("https", "api.acme.test", headers) + + await addon.requestheaders(flow) + + assert flow.response.status_code == 403 + assert flow.request.headers == headers + + +async def test_every_field_of_a_repeated_name_carries_its_own_placeholder( + swap: ModuleType, +) -> None: + exchange = FakeExchange( + swap, + {"api.acme.test"}, + { + (PLACEHOLDER, "api.acme.test"): ("Authorization", "Bearer real-one"), + (OTHER, "api.acme.test"): ("X-Acme-Key", "real-two"), + (THIRD, "api.acme.test"): ("X-Acme-Key", "real-three"), + }, + ) + addon = _swap(swap, exchange, "104.18.0.1") + flow = _flow("https", "api.acme.test", {}) + fields = [ + ("X-Acme-Key", OTHER), + ("X-Acme-Key", THIRD), + ("Authorization", f"Bearer {PLACEHOLDER}"), + ] + flow.request.headers = Headers(*fields) + + await addon.requestheaders(flow) + + assert flow.response.status_code == 403 + assert flow.request.headers.items(multi=True) == fields + assert exchange.asked == [(OTHER, "api.acme.test"), (THIRD, "api.acme.test")] + + +async def test_a_repeated_field_without_a_placeholder_passes_as_it_is(swap: ModuleType) -> None: + exchange = FakeExchange(swap, {"api.acme.test"}, {}) + addon = _swap(swap, exchange, "104.18.0.1") + flow = _flow("https", "api.acme.test", {}) + flow.request.headers = Headers(("Accept", "text/plain"), ("Accept", "application/json")) + + await addon.requestheaders(flow) + + assert flow.request.headers.items(multi=True) == [ + ("Accept", "text/plain"), + ("Accept", "application/json"), + ] + assert exchange.asked == [] + + +class ExchangeDownAfterOneAnswer(FakeExchange): + async def authorize(self, placeholder: str, host: str) -> tuple[str, str]: + self.unavailable = bool(self.asked) + return await super().authorize(placeholder, host) + + +async def test_an_exchange_that_falls_silent_midway_gives_503_and_swaps_nothing( + swap: ModuleType, +) -> None: + exchange = ExchangeDownAfterOneAnswer( + swap, + {"api.acme.test"}, + { + (PLACEHOLDER, "api.acme.test"): ("Authorization", "Bearer real-one"), + (OTHER, "api.acme.test"): ("X-Acme-Key", "real-two"), + }, + ) + addon = _swap(swap, exchange, "104.18.0.1") + headers = {"Authorization": f"Bearer {PLACEHOLDER}", "X-Acme-Key": OTHER} + flow = _flow("https", "api.acme.test", headers) + + await addon.requestheaders(flow) + + assert flow.response.status_code == 503 + assert flow.request.headers == headers + + async def test_a_host_header_that_differs_from_the_connect_host_is_refused( swap: ModuleType, ) -> None: