Skip to content

fix(egress): fail closed when active vault lookup fails #1636

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.

Sign up for GitHub

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
gx-hidemi-ito wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(egress): fail closed when active vault lookup fails #1636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions components/egress/mitmscripts/system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ def __init__(
self.redactions = redactions


class ActiveVaultLookupError(RuntimeError):
"""The credential proxy could not determine the active vault state."""


_vault_cache: ActiveVault | None = None
_vault_cache_loaded_at = 0.0

Expand Down Expand Up @@ -172,18 +176,11 @@ def _load_active_vault() -> ActiveVault | None:
_vault_cache_loaded_at = now
return None
if response.status != 200:
ctx.log.warn(
f"credential proxy: active vault lookup failed with HTTP {response.status}"
)
_vault_cache = None
_vault_cache_loaded_at = now
return None
raise ActiveVaultLookupError(f"HTTP {response.status}")

@Pangjiping Pangjiping Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this raise is effectively dead code — it sits inside the try, so the except immediately catches it and re-raises a generic ActiveVaultLookupError("active vault lookup failed"), discarding the HTTP status from the exception (it only survives in the log line). Consider raising a single ActiveVaultLookupError that carries the original cause (e.g. include the status in the message), or handle the non-200 branch outside the generic except so the status is preserved.

payload = json.loads(body.decode("utf-8"))
except Exception as exc: # noqa: BLE001 - mitm addon must not crash traffic handling
except Exception as exc: # noqa: BLE001 - all lookup failures must fail closed
ctx.log.warn(f"credential proxy: active vault lookup failed: {exc}")
_vault_cache = None
_vault_cache_loaded_at = now
return None
raise ActiveVaultLookupError("active vault lookup failed") from exc

@Pangjiping Pangjiping Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On lookup failure the cache is no longer cleared: the expired _vault_cache (which still holds the previous revision's plaintext secret header values) stays in the module global until the next successful refresh, whereas the old code set _vault_cache = None and dropped the reference. The expired entry is never served (the TTL check still short-circuits), but for a fail-closed, security-sensitive path it would be more consistent to clear the cache here too — and it avoids keeping potentially revoked credentials resident in memory.

Also note: the PR description's "never reuse an expired cached vault after a failed refresh" was already true before this change (the old code cleared the cache); the real behavioral change is the fail-closed 503.

finally:
connection.close()

Expand Down Expand Up @@ -377,7 +374,7 @@ def _request_may_be_streamed(flow: http.HTTPFlow) -> bool:
return "transfer-encoding" in flow.request.headers


def _reject_request(flow: http.HTTPFlow, body: bytes) -> None:
def _reject_request(flow: http.HTTPFlow, body: bytes, status: int = 403) -> None:
"""Terminate a request before it is forwarded upstream.

mitmproxy 11.0.2 refuses to serve a locally-set response while a request
Expand All @@ -393,7 +390,7 @@ def _reject_request(flow: http.HTTPFlow, body: bytes) -> None:
if flow.killable:
flow.kill()
return
flow.response = http.Response.make(403, body, {"content-type": "text/plain"})
flow.response = http.Response.make(status, body, {"content-type": "text/plain"})


def _flow_rejected(flow: http.HTTPFlow) -> bool:
Expand Down Expand Up @@ -638,7 +635,12 @@ def requestheaders(flow: http.HTTPFlow) -> None:
made: with ``stream_large_bodies=1m`` the ``request`` hook fires only
after a body above 1 MiB has been streamed upstream.
"""
vault = _load_active_vault()
try:
vault = _load_active_vault()
except ActiveVaultLookupError:

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Convert payload-validation failures to lookup errors

When the socket returns syntactically valid but malformed JSON, _load_active_vault() can still raise exceptions other than ActiveVaultLookupError after its try block—for example, [] raises AttributeError at payload.get(...), while a nonnumeric revision raises ValueError. This handler therefore installs no 503 response, and mitmproxy can continue processing the request after the addon hook fails, defeating the intended fail-closed behavior. Wrap payload shape/type validation in ActiveVaultLookupError or catch these failures here as well.

Useful? React with 👍 / 👎.

_reject_request(flow, b"credential proxy temporarily unavailable\n", status=503)

@Pangjiping Pangjiping Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The global fail-closed impact of this path is not documented. When the credential proxy is down, this 503 (or a flow kill for streamed bodies) applies to all egress traffic — including requests to hosts outside any credential binding scope — so the sidecar's availability becomes a hard dependency for every request. That is a deliberate fail-closed trade-off, but it is an operator-visible behavior change; please document it (e.g. in components/egress/docs/ and the module docstring) so downstream deployments can plan around it.

ctx.log.warn("credential proxy: rejected request because active vault lookup failed")
return
if vault is None:
return

Expand Down
119 changes: 119 additions & 0 deletions components/egress/tests/test_mitmscripts_system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,125 @@ def close(self) -> None:
self.assertEqual(("request", "GET", system.ACTIVE_VAULT_PATH), calls[1])
self.assertEqual(("close", None, None), calls[-1])

def test_active_vault_timeout_rejects_request_without_upstream_forwarding(self) -> None:
system = _load_system_module()

class TimeoutConnection:
def __init__(self, socket_path: str, timeout: float) -> None:
pass

def request(self, method: str, path: str) -> None:
raise TimeoutError("timed out")

def close(self) -> None:
pass

system.UnixSocketHTTPConnection = TimeoutConnection
flow = _Flow()
flow.response = None

system.requestheaders(flow)

self.assertIsNotNone(flow.response)
self.assertEqual(503, flow.response.status_code)
self.assertNotIn("Private-Token", flow.request.headers._values)

def test_expired_vault_cache_is_not_reused_after_lookup_failure(self) -> None:
system = _load_system_module()
system._vault_cache = system.ActiveVault(
1,
[
{
"name": "gitlab-api",
"match": {"hosts": ["code.example.com"]},
"headers": [{"name": "Private-Token", "value": "stale-secret"}],
}
],
["stale-secret"],
)
system._vault_cache_loaded_at = 0.0

class TimeoutConnection:
def __init__(self, socket_path: str, timeout: float) -> None:
pass

def request(self, method: str, path: str) -> None:
raise TimeoutError("timed out")

def close(self) -> None:
pass

system.UnixSocketHTTPConnection = TimeoutConnection
flow = _Flow()
flow.response = None

system.requestheaders(flow)

self.assertIsNotNone(flow.response)
self.assertEqual(503, flow.response.status_code)
self.assertNotIn("Private-Token", flow.request.headers._values)

def test_active_vault_http_error_rejects_request_without_upstream_forwarding(self) -> None:

@Pangjiping Pangjiping Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage gaps:

  1. The PR description claims "malformed responses" fail closed, but a 200 response with syntactically valid yet structurally invalid JSON (e.g. [], null, or a non-numeric revision) raises outside the try in _load_active_vault — payload parsing at lines 187-193 is not wrapped, so the resulting AttributeError/ValueError is not an ActiveVaultLookupError and requestheaders does not install the 503, letting the flow continue fail-open. A test for this case would have caught it.
  2. No test covers the streamed-body path, where _reject_request kills the flow instead of returning a 503.

system = _load_system_module()

class ErrorResponse:
status = 500

def read(self) -> bytes:
return b""

class ErrorConnection:
def __init__(self, socket_path: str, timeout: float) -> None:
pass

def request(self, method: str, path: str) -> None:
pass

def getresponse(self) -> ErrorResponse:
return ErrorResponse()

def close(self) -> None:
pass

system.UnixSocketHTTPConnection = ErrorConnection
flow = _Flow()
flow.response = None

system.requestheaders(flow)

self.assertIsNotNone(flow.response)
self.assertEqual(503, flow.response.status_code)

def test_missing_active_vault_remains_an_authoritative_pass_through(self) -> None:
system = _load_system_module()

class MissingResponse:
status = 404

def read(self) -> bytes:
return b""

class MissingConnection:
def __init__(self, socket_path: str, timeout: float) -> None:
pass

def request(self, method: str, path: str) -> None:
pass

def getresponse(self) -> MissingResponse:
return MissingResponse()

def close(self) -> None:
pass

system.UnixSocketHTTPConnection = MissingConnection
flow = _Flow()
flow.response = None

system.requestheaders(flow)

self.assertIsNone(flow.response)

def test_request_injection_log_does_not_include_secret_value(self) -> None:
system = _load_system_module()
flow = _Flow()
Expand Down
Loading