Add OSEP-0012 for Credential Vault#955
Conversation
jwx0925
requested review from
Generalwin,
Pangjiping,
Spground,
fengcone,
hittyt,
kevinlynx and
ninan-nn
as code owners
![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: 7eacd21760
ℹ️ 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".
| |----|-------------|----------| | ||
| | R1 | Users can attach credential bindings to a sandbox at creation time | Must Have | | ||
| | R2 | Plaintext credentials are not exposed through sandbox env vars, files, lifecycle API responses, command output, or diagnostic APIs | Must Have | | ||
| | R3 | Credential Proxy injects credentials only for matching FQDN, HTTP method, and path scope | Must Have | |
There was a problem hiding this comment.
R2 promises that plaintext credentials are not exposed through command output, but the proposed transparent header injection only keeps the secret out of the sandbox on the request path. For any allowed binding whose upstream endpoint echoes request headers or includes them in an error/debug response, a sandbox process can run curl and receive the injected Authorization value in stdout, violating this requirement unless the design also redacts response bodies/headers or narrows the guarantee to non-echoing services.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7c9a44. I narrowed R2 so it only promises that OpenSandbox-managed surfaces do not expose plaintext credentials, added an explicit Response Redaction and Echo Handling section, and added tests/risks for upstream services that echo credential-bearing headers. The proposal now treats response redaction as best effort and calls out that arbitrary upstream echo behavior cannot be an absolute secrecy guarantee.
| 1. **Kubernetes Secret** | ||
| - Available only for Kubernetes runtime. | ||
| - The OpenSandbox server reads the referenced secret through its existing service account permissions. | ||
| - Credential Proxy does not receive Kubernetes API permissions by default. | ||
| - The resolved value is passed to the proxy through a sandbox-scoped secret volume or bootstrap channel. |
There was a problem hiding this comment.
With this source model, the sandbox creator supplies the Kubernetes Secret name/key and the server reads it with its service account, but the design does not define any namespace scoping, allowlist, or authorization check tying that Secret to the requester or sandbox. In Kubernetes deployments where the server service account can read shared or high-privilege Secrets, any lifecycle API caller who can create a sandbox could bind one of those Secrets to an allowed host and use it through the proxy, so the source must be constrained to configured/authorized Secret references rather than arbitrary names from the request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7c9a44. Kubernetes Secret sources are now explicitly resolved through configured source providers rather than arbitrary namespace/name input from sandbox creators. The OSEP now requires namespace scope, allowlists or selectors, and requester authorization before the server resolves any Kubernetes Secret.
There was a problem hiding this comment.
Updated in f68309d. The design no longer exposes Kubernetes Secret as a user-facing credential source in the MVP. Kubernetes Secret may only be used as a sandbox-scoped runtime delivery artifact generated by OpenSandbox and mounted into the egress sidecar; sandbox creators cannot reference arbitrary pre-existing Kubernetes Secret names. Any future user-selectable Kubernetes Secret provider would need a separate authorization model, namespace/scope policy, allowlists/selectors, requester authorization, and source-to-destination constraints.
|
|
||
| Limitations: | ||
|
|
||
| - Requires Linux network namespace support and `CAP_NET_ADMIN` for the egress sidecar. |
There was a problem hiding this comment.
This limitation needs an explicit fail-closed requirement for credential-enabled sandboxes: the current egress sidecar is documented in components/egress/README.md as gracefully disabling enforcement when CAP_NET_ADMIN is missing, but without the transparent iptables redirect outbound 80/443 traffic bypasses Credential Proxy entirely. In clusters or Docker setups that omit this capability, a sandbox with credentialVault.bindings could start without injection, auditing, or the intended credential policy mediation, violating R9 unless creation/readiness fails when the redirect cannot be installed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7c9a44. Credential-enabled startup now has an explicit fail-closed requirement: sandbox creation/readiness must fail if transparent redirect, mitm readiness, CA bootstrap, credential addon loading, or egress API auth cannot be configured. This is intentionally stricter than ordinary egress graceful degradation.
| - Extend `components/egress` transparent mitmproxy support with a first-party credential addon. | ||
| - Load credential binding bootstrap config into the egress sidecar. | ||
| - Implement binding evaluation, header injection, redaction, and audit events in the mitm addon path. | ||
| - Keep the existing system addon behavior for streaming and user addon loading. |
There was a problem hiding this comment.
Keeping arbitrary user mitm addon loading in the same transparent mitmproxy process is incompatible with the credential secrecy guarantees: the existing egress transparent mode supports OPENSANDBOX_EGRESS_MITMPROXY_SCRIPT, loaded after the system addon, and such an addon can inspect or log flow.request.headers after the first-party credential addon injects Authorization. For credential-enabled sandboxes, user addons need to be disabled, isolated, or otherwise prevented from observing credential-bearing flows; otherwise R2/R7 can be bypassed by the proxy extension path itself.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7c9a44. The OSEP now disables user-provided mitm addon loading for credential-enabled sandboxes until an isolation model exists that prevents addons from observing credential-bearing flows. It keeps only the existing system addon behavior plus the first-party credential addon path.
There was a problem hiding this comment.
Updated in f68309d. I clarified the current trust boundary: egress mitm addons are operator/platform-controlled sidecar configuration, not a sandbox-user API. That is not a credential leak by itself because operator-configured addons are part of the platform trust boundary. The OSEP now requires preserving this boundary for credential-enabled sandboxes by rejecting any future sandbox-supplied addon mechanism and ensuring addon script paths do not come from app-container-writable locations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88567c1d32
ℹ️ 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".
| targets: | ||
| type: array | ||
| items: | ||
| type: string | ||
| description: FQDN or wildcard domain targets, for example api.github.com or *.example.com. |
There was a problem hiding this comment.
With the scope limited to an FQDN/wildcard target, the same binding applies to both https://host/... and http://host/... because the proposed transparent proxy intercepts TCP 80/443. In any environment where the sandbox can call http://api.github.com/... or another bound host on port 80, Credential Proxy would inject Authorization into a cleartext request before any upstream redirect, exposing the credential on the network. The binding schema needs a scheme/port constraint or an HTTPS-only default for credential injection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7c9a44. CredentialScope now includes schemes and ports, with defaults of schemes: [https] and ports: [443]. The injection rules now state that HTTP or non-443 injection requires explicit opt-in and should be rejected by default platform policy unless the operator enables it.
| When both `credentialVault` and `networkPolicy` are present, the server should validate destination consistency. | ||
|
|
There was a problem hiding this comment.
Because credential bindings can be supplied without networkPolicy, this “when both are present” validation leaves credential-only sandboxes unspecified. The proposed runtime still enables the egress sidecar for bindings, but the current sidecar treats an omitted/empty initial policy as default-deny (components/egress/pkg/policy/policy.go), so a valid sandbox with only credentialVault.bindings can start and then have DNS/egress blocked for every target instead of being usable. The proposal should either require networkPolicy, synthesize matching allow rules, or explicitly configure an allow-all policy for this mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7c9a44. Credential-enabled sandboxes now require explicit networkPolicy.egress; missing policy, empty egress, or binding targets not covered by allow rules fail creation with HTTP 400. I chose explicit policy over synthesized allow rules to keep the network boundary auditable and fail-closed.
| - Persist credential binding metadata without plaintext credential values. | ||
| - Resolve or prepare sandbox-scoped credential material during sandbox creation. | ||
| - Redact `inlineEphemeral.value` from request logging, validation errors, persisted metadata, and lifecycle responses. | ||
| - Enable egress transparent mitmproxy and credential addon bootstrap for Docker and Kubernetes runtimes when bindings are present. |
There was a problem hiding this comment.
When bindings are present this starts the egress sidecar even if no networkPolicy was requested, but the proposal does not require provisioning the sidecar's runtime API token in that path. In the current implementation the /policy endpoint is unauthenticated when OPENSANDBOX_EGRESS_TOKEN is empty, and the server only generates that token when request.network_policy is set, so a credential-only sandbox sharing the sidecar netns could call 127.0.0.1:18080/policy and rewrite egress policy unless credential-enabled startup also always configures auth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e7c9a44. Credential-enabled sidecars must now always provision OPENSANDBOX_EGRESS_TOKEN, including when credentials are the reason the egress sidecar starts. The application container must not receive that token, so sandbox code cannot rewrite /policy.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7c9a44762
ℹ️ 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".
| 2. **Server-local source** | ||
| - Available for Docker and local development. | ||
| - Configured in server TOML, for example: | ||
|
|
||
| ```toml | ||
| [credential_vault] | ||
| enabled = true | ||
|
|
||
| [[credential_vault.sources]] | ||
| type = "server_local" | ||
| name = "github-readonly-token" | ||
| value_env = "OPENSANDBOX_GITHUB_READONLY_TOKEN" | ||
| ``` |
There was a problem hiding this comment.
When a shared server has multiple serverLocal credentials configured, this section lets the sandbox request pick a source by sourceRef.name but does not require the requester authorization/allowlist checks that the Kubernetes Secret source gets above. Any lifecycle caller who can create a sandbox and guess a configured source name could bind a more privileged server-local token and have Credential Proxy inject it, so server-local sources need per-source ACLs or opaque operator-approved binding aliases before they are selectable from requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. I removed user-facing serverLocal source selection from the MVP entirely. The public API now only accepts an inline write-only credential value at sandbox creation time, with no type, name, or key discriminator. Platform-level credentials are no longer selectable by sandbox requests; if a platform wants create-once/reference-many credentials, it should expose them through operator-configured binding templates and enforce template usage authorization at the platform layer.
| - Requires Linux network namespace support and `CAP_NET_ADMIN` for the egress sidecar. | ||
| - Requires the sandbox to trust the mitmproxy CA for HTTPS interception. | ||
| - Applies to HTTP/HTTPS traffic on `80/443`; non-HTTP protocols need future designs. | ||
| - In `ignore_hosts` pass-through mode, Credential Proxy cannot inspect or inject credentials for those hosts. | ||
| - Credential-enabled startup must fail closed if mitmproxy, `iptables` redirect, CA bootstrap, credential addon loading, or egress API authentication cannot be configured. |
There was a problem hiding this comment.
In environments where egress.disable_ipv6 is set false or IPv6 remains enabled, the current transparent MITM redirect only installs IPv4 iptables OUTPUT rules (components/egress/pkg/iptables/transparent.go uses iptables, and components/egress/docs/mitmproxy-transparent.md documents that IPv6 is not handled). Credential-enabled sandboxes in those configurations can send HTTP(S) over IPv6 without Credential Proxy inspection, injection, or audit, so the fail-closed startup requirements should also require IPv6 to be disabled or covered by equivalent ip6/nft redirection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. The OSEP now explicitly fails closed unless IPv6 HTTP(S) egress is disabled or covered by equivalent transparent redirect. It also records the longer-term direction: add IPv6 transparent redirect support through ip6tables/nftables with the same test coverage as IPv4.
| - redirects outbound `TCP 80/443` traffic to the mitmproxy listener using `iptables`, | ||
| - loads system and user mitm addons, | ||
| - exports the mitmproxy root CA, | ||
| - exposes health readiness so sandboxes do not start before interception is ready. |
There was a problem hiding this comment.
The existing Kubernetes helper appends egress as a normal container (server/opensandbox_server/services/k8s/egress_helper.py), and the Docker path only starts the sidecar container before immediately creating the sandbox container, so the current health endpoint/readiness does not actually prevent sandbox code from running before DNS/HTTP redirects and mitmproxy are installed. For credential-enabled sandboxes, that startup window can send requests without Credential Proxy injection or audit, so the proposal needs an explicit init/startup gate rather than relying on readiness alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 28d1488. I made the startup gate explicit: for credential-enabled sandboxes, application code must not run until the egress sidecar readiness signal confirms redirect rules, mitmproxy, credential addon loading, CA material, upstream TLS verification, IPv6 handling, and egress API auth are ready. The Kubernetes and Docker component notes now both require releasing/starting the application only after that signal is ready.
| - Credential Proxy must inject only for HTTPS on port 443 by default. | ||
| - Any HTTP or non-443 injection requires explicit `scope.schemes` and `scope.ports` opt-in and should be rejected by default platform policy unless the operator enables it. |
There was a problem hiding this comment.
The egress launcher currently supports OPENSANDBOX_EGRESS_MITMPROXY_SSL_INSECURE=true, which passes ssl_insecure=true to mitmproxy and skips upstream certificate verification. In that configuration, a sandbox can direct a scoped HTTPS hostname to an attacker-controlled IP while preserving the matching Host/SNI, causing Credential Proxy to inject the credential into a connection whose peer was not authenticated as the target host; credential-enabled startup should reject this mode or require verified upstream TLS before header injection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. The proposal now requires credential-enabled startup to reject upstream TLS-insecure mode such as OPENSANDBOX_EGRESS_MITMPROXY_SSL_INSECURE=true, so Credential Proxy only injects credentials when upstream TLS verification remains enabled.
| CredentialSourceRef: | ||
| type: object | ||
| required: [type] | ||
| properties: | ||
| type: | ||
| type: string | ||
| enum: [kubernetesSecret, serverLocal, inlineEphemeral] | ||
| name: | ||
| type: string | ||
| description: Provider-local credential source name. Required for kubernetesSecret and serverLocal; omitted for inlineEphemeral. | ||
| key: | ||
| type: string | ||
| description: Provider-local key name for multi-key sources. | ||
| value: | ||
| type: string | ||
| writeOnly: true | ||
| description: Inline ephemeral credential value accepted only at sandbox creation time. Never returned, logged, or persisted as plaintext. | ||
| additionalProperties: false |
There was a problem hiding this comment.
This schema only requires type, so OpenAPI validation would accept unusable or ambiguous sources such as {"type":"inlineEphemeral"} with no value, or {"type":"serverLocal","value":"..."} even though the surrounding text says name is required for configured providers and inline values are write-only create input. Without oneOf/conditional required fields, SDKs and server validators generated from the contract can admit credential bindings that fail later at runtime or put plaintext inline values on the wrong source type, so the contract should encode the per-type requirements directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d by simplifying the source model instead of adding per-type conditionals. The public MVP source schema now has only one valid shape: { "value": "..." }, with value required and write-only. There is no longer a type, name, or key, so OpenAPI/SDK validation cannot accept mixed or ambiguous serverLocal/inlineEphemeral combinations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffc4ee54ce
ℹ️ 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".
| - `{{ credential }}` is the only supported template variable in the MVP. | ||
| - Credential Proxy must reject templates that do not include `{{ credential }}`. |
There was a problem hiding this comment.
The MVP renderer is specified to support only the bare {{ credential }} variable, but several advertised built-in templates render {{ credential | base64 }} or concatenate before base64-encoding, such as git-https-basic, generic-basic, and pypi-token. With this rule, those built-ins either have to be rejected by the proxy/template validator or will be rendered incorrectly, so the proposal should either include the required filters/operations in the MVP renderer or remove/replace the templates that depend on them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. I kept the MVP renderer limited to the bare {{ credential }} variable and changed Basic-auth templates to require callers or upper-layer platforms to pass a pre-encoded value, e.g. base64(username:token). The built-in catalog and expansion examples now render Authorization: Basic {{ credential }} instead of using base64 filters or basic() helpers.
| params: | ||
| type: object | ||
| additionalProperties: | ||
| type: string | ||
| description: Non-sensitive template parameters. Sensitive values must use credential, not params. |
There was a problem hiding this comment.
The template params schema only accepts string values, but the built-in templates expose methods as an optional param that expands into CredentialScope.methods, which is an array. In a request that tries to narrow a generic template to multiple methods, there is no valid JSON array form for params.methods; a comma-separated string would either fail after expansion or become a single invalid method token. The schema should allow template-specific typed params or otherwise define how string params are parsed into arrays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. I removed methods from the generic template params and documented that built-in templates intentionally do not accept typed template params in the MVP. Method lists and other typed behavior are fixed by the built-in template type or can be represented by operator-configured templates later.
|
|
||
| ### Response Redaction and Echo Handling | ||
|
|
||
| Credential Proxy should redact known credential values from upstream response headers and text-like response bodies where practical. This protects against common debug endpoints and error handlers that echo request headers. |
There was a problem hiding this comment.
This redaction target is too narrow for the built-in Basic-auth templates because an upstream echo of Authorization: Basic ... contains the base64-rendered credential, not the raw username:token value. Fresh evidence is that the current OSEP now adds response redaction but only names known credential values, while the same proposal advertises transformed header renderings; in those cases a mock/debug upstream can still return a trivially decodable secret to the sandbox unless the proxy redacts every rendered injected header value as well as the source credential.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. The redaction section now requires redacting both the source credential material and rendered injected header values, including Bearer <token> and Basic <base64(username:token)>. The test plan also includes coverage for rendered injected credential values, not only raw source values.
| - Credential Proxy must inject only for HTTPS on port 443 by default. | ||
| - Any HTTP or non-443 injection requires explicit `scope.schemes` and `scope.ports` opt-in and should be rejected by default platform policy unless the operator enables it. |
There was a problem hiding this comment.
Allowing explicit non-443 injection creates configurations the proposed runtime cannot honor: transparent mode is described as intercepting only outbound TCP 80/443, so a binding for https://host:8443 can pass this rule and egress target validation but will not be inspected, injected, or audited by Credential Proxy. The API should reject non-80/443 ports for transparent mode, or the runtime plan should include redirecting and testing the additional ports before accepting such scopes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. Transparent mode now accepts credential injection only for destination ports configured as transparent MITM intercept ports. The MVP default is [80, 443]; any binding scope using another port is rejected unless the operator explicitly configures that port and the runtime can install matching redirect rules. Unit/integration tests were added to the plan for rejecting non-configured ports.
| - In `ignore_hosts` pass-through mode, Credential Proxy cannot inspect or inject credentials for those hosts. | ||
| - Credential-enabled startup must fail closed if mitmproxy, `iptables` redirect, CA bootstrap, credential addon loading, or egress API authentication cannot be configured. |
There was a problem hiding this comment.
This notes that ignore_hosts prevents Credential Proxy from inspecting or injecting, but the validation and fail-closed requirements never reject a credential binding whose target is covered by that pass-through list. In deployments with OPENSANDBOX_EGRESS_MITMPROXY_IGNORE_HOSTS configured for a bound host, the sandbox can reach the allowed destination without credential injection or audit rather than failing closed, so credential-enabled startup should reject matching ignore-host patterns or disallow ignore_hosts entirely for credential-bound targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d. The runtime limitations and integration tests now require credential-bound targets not to match ignore_hosts. Because ignore-hosts are pass-through TLS, Credential Proxy cannot inspect, inject, redact, or audit those flows, so credential-enabled startup must reject bindings that match configured ignore-host patterns.
| - A `CredentialBinding` must use exactly one of these forms: | ||
| - **Inline full binding**: `sourceRef`, `scope`, and `injection`. | ||
| - **Template binding**: `templateRef` and `credential`. |
There was a problem hiding this comment.
The full-binding form lets the request pair any usable sourceRef with an arbitrary scope and injection, but the source authorization described later only decides whether the requester may bind that source at all. In a shared deployment where a requester is allowed to use a provider-managed token for one service, the same requester can create a full binding that injects that token into attacker.example unless the source provider or template also enforces approved targets, methods, paths, and header renderings for that source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f68309d by removing user-selectable provider-managed sources from the public MVP API. Full bindings can only carry sandbox-scoped inline credential material supplied at creation time. Platform-managed credentials must be hidden behind operator-configured templates, and those templates must constrain targets, methods, paths, schemes, ports, and injection rendering. That prevents a request from freely pairing a platform credential source with an arbitrary attacker-controlled destination.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28d1488ee6
ℹ️ 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".
| CredentialBinding: | ||
| type: object | ||
| required: [name] | ||
| properties: |
There was a problem hiding this comment.
Because the proposed public OpenAPI schema only requires name, generated validators and SDKs will accept bindings like { "name": "x" } or bindings that mix templateRef with sourceRef/scope/injection, even though the validation rules and tests below require exactly one full-binding or template-binding form. This leaves the contract out of sync with the server behavior the proposal depends on, so the schema should model the two alternatives with oneOf/mutual exclusion and the per-form required fields.
Useful? React with 👍 / 👎.
| bindings: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/CredentialBinding' |
There was a problem hiding this comment.
If a sandbox request contains two bindings with the same name but different credentials or scopes, the proxy can still make an injection decision by scope while audit logs and persisted metadata only report the duplicated binding name. That makes R7's binding-usage audit trail ambiguous exactly when operators need to know which credential was used or revoked, so creation validation should reject duplicate binding names within a sandbox.
Useful? React with 👍 / 👎.
| paths: | ||
| type: array | ||
| items: | ||
| type: string | ||
| default: ["/*"] |
There was a problem hiding this comment.
When a binding narrows credentials to a path prefix, matching the raw request path lets inputs such as dot segments or encoded separators satisfy a scoped pattern while the upstream framework may normalize them to a different endpoint. For example, a request under /foo/bar.git/%2e%2e/admin can match the "/foo/bar.git/*" rule but be handled upstream outside that repo prefix, so the design needs a canonical path matching rule or must reject ambiguous encoded/relative paths before injecting credentials.
Useful? React with 👍 / 👎.
| Credential-enabled sandboxes must include `networkPolicy.egress`. The server must validate destination consistency before sandbox creation. | ||
|
|
||
| Required validation: | ||
|
|
||
| - Every credential binding target must be covered by an allow rule in `networkPolicy.egress`. | ||
| - More formally, the set of credential binding targets must be a subset of the egress allow targets after template expansion and wildcard normalization. | ||
| - `networkPolicy.defaultAction` should be `deny`; if `allow` is accepted for compatibility, the server must still require explicit allow coverage for every binding target and warn that broad outbound access coexists with credential-bearing traffic. | ||
| - If `networkPolicy` is omitted, if `egress` is empty, or if a binding target is not reachable under egress policy, sandbox creation must fail with HTTP 400. |
There was a problem hiding this comment.
The required strict egress coverage is only specified for sandbox creation, but the current SDK/server surface supports patching and deleting egress rules after startup. In credential-enabled sandboxes, a later policy update can remove the explicit allow rule or switch to broad default-allow while the credential binding remains active, so the same coverage check should run on egress policy mutations or those mutations should be rejected while credentials are bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b20320fa44
ℹ️ 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".
| - `GET /sandboxes/{sandbox_id}/credential-vault/bindings`: returns sanitized binding metadata and the current acknowledged revision. | ||
| - `POST /sandboxes/{sandbox_id}/credential-vault/bindings`: adds a new sandbox-local binding name. |
There was a problem hiding this comment.
When a sandbox is created without initial credential bindings, this advertised POST path has no safe runtime to update: the proposal only enables transparent Credential Proxy when credentialVault.bindings is present at startup, and the CA install/startup gate cannot be retroactively applied after user code is already running. The E2E plan also creates a sandbox with no initial bindings and then adds one at runtime, so the design needs to either bootstrap a credential-ready proxy with an empty binding set at creation or reject runtime adds on non-credential-enabled sandboxes.
Useful? React with 👍 / 👎.
| /sandboxes/{sandbox_id}/credential-vault/bindings/{binding_name}: | ||
| put: | ||
| summary: Replace a credential binding on a running sandbox. | ||
| requestBody: | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/CredentialBindingMutationRequest' |
There was a problem hiding this comment.
For PUT /bindings/{binding_name}, the request body reuses CredentialBinding, which has its own required name, but the validation rules never say that the body name must equal the path parameter. If callers send /bindings/github with { "name": "npm", ... }, the server/proxy could replace one binding while returning metadata or audit events under another name, undermining the duplicate-name and mutation semantics; the API should reject mismatches or remove name from replace bodies.
Useful? React with 👍 / 👎.
| - A `CredentialBinding` must use exactly one of these forms: | ||
| - **Inline full binding**: `sourceRef`, `scope`, and `injection`. | ||
| - **Template binding**: `templateRef` and `credential`. |
There was a problem hiding this comment.
When an operator-configured template is meant to use an internally selected platform-managed credential, this validation still requires every template binding to include an inline credential. That makes the documented platform-managed template path unusable without passing a dummy/user-supplied secret, so the binding forms need to distinguish inline-credential templates from operator-managed credential templates and authorize the latter explicitly.
Useful? React with 👍 / 👎.
| - The server prepares sandbox-scoped runtime material, sends the complete sanitized binding set and source material handles to Credential Proxy, and waits for an acknowledgement for that revision. | ||
| - Credential Proxy validates and loads the revision into an immutable in-memory snapshot, then atomically swaps the active snapshot. | ||
| - The API returns success only after Credential Proxy acknowledges the revision. The response includes the acknowledged revision and sanitized binding metadata. | ||
| - If Credential Proxy is unavailable, rejects the update, or times out, the server must not report success. The previous acknowledged revision remains active. | ||
| - If the server already persisted pending mutation metadata before delivery, it must mark the mutation as failed or roll it back before returning an error so lifecycle responses do not claim an inactive binding. |
There was a problem hiding this comment.
If a runtime add or replace prepares sandbox-scoped material and then Credential Proxy rejects or times out, this failure path keeps the previous revision active but never requires deleting the newly created candidate material. In Kubernetes that can leave a generated Secret containing a credential for a mutation the API reported as failed, so failed-delivery rollback should also clean up any runtime material created for the rejected revision.
Useful? React with 👍 / 👎.
| - Every credential binding target must be covered by an allow rule in `networkPolicy.egress`. | ||
| - More formally, the set of credential binding targets must be a subset of the egress allow targets after template expansion and wildcard normalization. |
There was a problem hiding this comment.
This coverage check only compares binding targets to allow-rule targets, but the existing egress policy is first-match and can also include deny rules/overlays, so an allow target is not necessarily reachable. For example, deny api.github.com before allow *.github.com would pass a set-based allow-subset check while Credential Proxy can never send the credentialed request; the validation should evaluate each expanded binding target against the effective egress policy and require an allow decision.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
No code tests run; documentation-only proposal.