fix(egress): fail closed when active vault lookup fails - #1636
fix(egress): fail closed when active vault lookup fails#1636gx-hidemi-ito wants to merge 1 commit into
Conversation
github-actions
Bot
added
component/egress
size/S
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fa2b80068
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| vault = _load_active_vault() | ||
| try: | ||
| vault = _load_active_vault() | ||
| except ActiveVaultLookupError: |
There was a problem hiding this comment.
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 👍 / 👎.
| try: | ||
| vault = _load_active_vault() | ||
| except ActiveVaultLookupError: | ||
| _reject_request(flow, b"credential proxy temporarily unavailable\n", status=503) |
There was a problem hiding this comment.
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.
| _vault_cache = None | ||
| _vault_cache_loaded_at = now | ||
| return None | ||
| raise ActiveVaultLookupError("active vault lookup failed") from exc |
There was a problem hiding this comment.
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.
| _vault_cache = None | ||
| _vault_cache_loaded_at = now | ||
| return None | ||
| raise ActiveVaultLookupError(f"HTTP {response.status}") |
There was a problem hiding this comment.
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.
| 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: |
There was a problem hiding this comment.
Test coverage gaps:
- 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-numericrevision) raises outside thetryin_load_active_vault— payload parsing at lines 187-193 is not wrapped, so the resultingAttributeError/ValueErroris not anActiveVaultLookupErrorandrequestheadersdoes not install the 503, letting the flow continue fail-open. A test for this case would have caught it. - No test covers the streamed-body path, where
_reject_requestkills the flow instead of returning a 503.
2 participants
Summary
Why
The credential-injection proxy currently treats Unix-socket timeouts and non-200 responses as if no vault were active. The request is then forwarded upstream with unresolved credential placeholders, producing misleading provider authentication failures and allowing traffic to bypass the credential-vault enforcement path.
A 404 remains the explicit no-vault result and continues to pass through unchanged. All operational lookup failures now stop the request locally.
Tests
python3 -m unittest components.egress.tests.test_mitmscripts_system(54 tests)Please include this fix in the next official
opensandbox/egressrelease so downstream deployments can update their pinned multi-architecture digest.