fix(execd): reload rotated mitmproxy CA - #1378
Conversation
github-actions
Bot
added
component/execd
documentation
![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: 1aac0bc6a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| refresh: func(refreshCtx context.Context) error { | ||
| commandCtx, cancel := context.WithTimeout(refreshCtx, refreshTimeout) | ||
| defer cancel() | ||
| output, refreshErr := exec.CommandContext(commandCtx, "/bin/sh", bootstrapPath, refreshOnlyArg).CombinedOutput() |
There was a problem hiding this comment.
When this refresh path handles the first usable CA after the initial bootstrap timed out, it runs bootstrap.sh --refresh-mitm-ca-trust in a separate shell, so the export REQUESTS_CA_BUNDLE / SSL_CERT_FILE / NODE_EXTRA_CA_CERTS done by refresh_mitm_ca is discarded when that child exits. Execd keeps its original environment and future Python/Node commands launched through execd still won't see the CA bundle even though the watcher reports a refresh; persist those values through EXECD_ENVS or have execd update its own environment after a successful refresh.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
…tated-mitm-ca-1370 # Conflicts: # docs/components/egress.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc78604a75
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| panic("mitmca watcher requires a positive poll interval") | ||
| } | ||
|
|
||
| trusted, hasTrusted, err := readCertificateFingerprint(cfg.caPath) |
There was a problem hiding this comment.
When the CA rotates after bootstrap finishes installing the old certificate but before this initial snapshot, trusted is initialized to the new fingerprint even though none of the trust stores contain it, so the watcher will never invoke refresh until another rotation. The same problem occurs when bootstrap's best-effort installation encounters a transient failure; initialize without accepting the on-disk fingerprint, or otherwise pass the successfully installed fingerprint from bootstrap.
Useful? React with 👍 / 👎.
| OPENSANDBOX_EGRESS_MITMPROXY_TRANSPARENT=true \ | ||
| EXECD="$TEST_DIR/execd" \ | ||
| PATH="$MOCK_BIN" \ | ||
| sh "$TEST_BOOTSTRAP" --refresh-mitm-ca-trust |
There was a problem hiding this comment.
When a developer or CI runner has JAVA_HOME set, restricting PATH here does not isolate the JDK scan: trust_mitm_ca_jdk still visits that real JDK, deletes any existing opensandbox-mitmproxy alias, and imports the test bundle into its live cacerts. Unset JAVA_HOME (and prevent the fallback scan) so this regression test cannot modify the host toolchain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3f8a31118
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| refresh: func(refreshCtx context.Context) error { | ||
| commandCtx, cancel := context.WithTimeout(refreshCtx, refreshTimeout) | ||
| defer cancel() | ||
| output, refreshErr := exec.CommandContext(commandCtx, "/bin/sh", bootstrapPath, refreshOnlyArg).CombinedOutput() |
There was a problem hiding this comment.
When the two-minute timeout or daemon context cancellation occurs while bootstrap.sh is waiting on a child such as update-ca-certificates, certutil, keytool, or sudo, exec.CommandContext kills only the /bin/sh process. The child can continue modifying trust stores and, because it retains the combined-output descriptors, can keep CombinedOutput and the watcher blocked beyond the advertised timeout; on daemon shutdown it may also survive as an orphan. Start the refresh in its own process group and terminate that group when the context expires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04c0934766
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {name: "NODE_EXTRA_CA_CERTS", value: defaultCAPath}, | ||
| {name: "REQUESTS_CA_BUNDLE", value: defaultMergedCAPath}, | ||
| {name: "SSL_CERT_FILE", value: defaultMergedCAPath}, |
There was a problem hiding this comment.
When transparent MITM is enabled and EXECD_BOOTSTRAP_PRE_SCRIPT intentionally exports a custom CA bundle, the script is sourced after the initial trust setup so its values are inherited by execd. However, the watcher refreshes the currently installed certificate on its first polling tick and these unconditional assignments overwrite that customization even when no rotation occurred; subsequent commands and PTY sessions launched through execd then lose the configured corporate or composite CA bundle. Preserve explicit post-bootstrap overrides, or only replace values that still contain the bootstrap-managed paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 284fad7d34
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if refreshErr != nil { | ||
| return fmt.Errorf("bootstrap refresh failed: %w output=%q", refreshErr, compactOutput(output)) | ||
| } | ||
| return setProcessTrustEnvironment(os.Setenv) |
There was a problem hiding this comment.
When the CA first appears after bootstrap timed out, any NSS or JDK import failure makes refresh_mitm_ca 1 exit nonzero even if the merged bundle was created successfully. This early return therefore skips setProcessTrustEnvironment, so future Node and certifi-based processes still lack the CA variables, while the watcher repeats the expensive full refresh every polling cycle. Update the process environment after the merged bundle succeeds while continuing to retry the failed trust stores separately.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c38873b502
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if getenv(variable.name) != "" { | ||
| continue |
There was a problem hiding this comment.
When the initial bootstrap finds the MITM certificate but cannot create the merged bundle, bootstrap.sh exports REQUESTS_CA_BUNDLE and SSL_CERT_FILE as the MITM-only certificate path. If a later watcher refresh successfully creates the merged bundle, this nonempty check treats those bootstrap-managed fallback values as custom overrides and leaves them unchanged, so future Python/OpenSSL processes still lack the normal public root set and can fail on HTTPS connections that bypass interception. Preserve genuine custom values, but replace the known managed fallback path with defaultMergedCAPath after recovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 310347db31
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
This new regression script is not referenced by components/execd/Makefile or .github/workflows/execd-test.yml, whose Linux jobs currently run only the existing smoke, SIGTERM, and bwrap scripts. Consequently, changes that break refresh-only bootstrap behavior can still pass every execd CI job; add this script to the Linux workflow or a test target invoked there.
AGENTS.md reference: AGENTS.md:L49-L51
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 931bd4147f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| {name: "REQUESTS_CA_BUNDLE", value: defaultMergedCAPath, managedFallback: defaultCAPath}, | ||
| {name: "SSL_CERT_FILE", value: defaultMergedCAPath, managedFallback: defaultCAPath}, |
There was a problem hiding this comment.
When bootstrap initially falls back to the MITM-only path and a persistent bash session is created before a later successful recovery, these assignments update only execd's process environment. newBashSession in components/execd/pkg/runtime/bash_session.go caches the old values, and buildWrappedScript re-exports that snapshot over the updated inherited environment on every subsequent run_in_session, leaving REQUESTS_CA_BUNDLE and SSL_CERT_FILE without public roots indefinitely. Fresh evidence beyond the earlier process-environment issue is this separate session-level cache; replace known managed fallbacks there as well so processes launched after recovery use the merged bundle promised by the updated documentation.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86579d92b5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| updatedEnv := parseExportDump(envLines) | ||
| s.mu.Lock() | ||
| if len(updatedEnv) > 0 { | ||
| applyManagedEnvironmentUpdates(updatedEnv, s.managedEnvironment) |
There was a problem hiding this comment.
After a managed CA update has been recorded, any later session command that runs unset REQUESTS_CA_BUNDLE or exports it as an empty value has that explicit override silently replaced here, because a missing/empty entry is treated as needing the managed value whenever the environment dump is persisted. This breaks normal bash-session environment persistence and contradicts the documented promise to preserve explicit environment overrides; distinguish an explicitly cleared variable from an absent bootstrap-managed value instead of unconditionally restoring it.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
| c.bashSessionClientMap.Range(func(_, sessionValue any) bool { | ||
| if session, ok := sessionValue.(*bashSession); ok { | ||
| session.updateManagedEnvironment(name, value, managedFallback) |
There was a problem hiding this comment.
When a bash session is created concurrently with fallback recovery, newBashSession can snapshot the old process environment before os.Setenv, while its later Store can occur after this Range has already passed; that session is then absent from the callback and retains the MITM-only fallback indefinitely. Keep the latest managed updates on the controller and apply them while publishing new sessions, or otherwise synchronize creation with this traversal, so every session created around a refresh receives the merged bundle as documented.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d454993f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| s.managedEnvironment = make(map[string]managedEnvironmentUpdate) | ||
| } | ||
| s.managedEnvironment[name] = update | ||
| applyManagedEnvironmentUpdate(s.env, name, update, true) |
There was a problem hiding this comment.
When a session explicitly unsets one of these variables before the watcher records its first successful update—for example, during the initial two-second polling window or while waiting for a late CA—replaceEmpty=true treats the missing value as bootstrap-managed and silently restores it. The later persistence logic only preserves clears made after managedEnvironment has been registered, so this remains a distinct path beyond the earlier clear-handling comment; track whether the session originally inherited the managed fallback instead of replacing every absent value, as the updated egress documentation promises to preserve explicit overrides.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d57268210b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err := setenv(variable.name, variable.value); err != nil { | ||
| return fmt.Errorf("set child-process trust environment %s: %w", variable.name, err) | ||
| } | ||
| if onUpdate != nil { | ||
| onUpdate(variable.name, variable.value, variable.managedFallback) | ||
| } |
There was a problem hiding this comment.
When bootstrap times out before the CA appears, or initially falls back to the MITM-only bundle, this updates only execd's own environment. runJupyter delegates kernel creation to the separately running Jupyter server, which retains its original missing or fallback values, so even newly created Python or Node kernels after recovery do not inherit the corrected trust environment. Distinct from the earlier process-environment issue, the fresh evidence is the external Jupyter parent process in pkg/runtime/jupyter.go; recovery must update or restart that runtime as well.
AGENTS.md reference: AGENTS.md:L42-L44
Useful? React with 👍 / 👎.
| for name, previousValue := range previous { | ||
| currentValue, exists := current[name] | ||
| if !exists || (previousValue != "" && currentValue == "") { | ||
| cleared[name] = true |
There was a problem hiding this comment.
When a bash session inherits an exported value larger than maxPersistedEnvValueSize, parseExportDump intentionally omits it, but this loop interprets that omission as an explicit unset. The next wrapped command therefore emits unset NAME, causing an unchanged inherited variable—such as a large credential or certificate payload—to disappear after the first session command; restrict clear tracking to variables that were actually eligible for persistence or to the managed CA variables.
Useful? React with 👍 / 👎.
1 participant
Summary
Closes #1370. This completes scenario B after #1371 fixed scenario C's co-restart race.
Behavior
The egress container atomically replaces
/opt/opensandbox/mitmproxy-ca-cert.pemafter regenerating its CA. Bootstrap previously installed that certificate only once.When transparent MITM is enabled, execd now fingerprints the parsed X.509 DER and ignores missing, empty, malformed, or non-CA files. A changed fingerprint invokes an internal bootstrap refresh mode, reusing the existing trust-store logic instead of duplicating it in Go.
Refresh mode atomically replaces
merged-ca-certificates.pem, reports applicable installation failures, and never starts another execd. The watcher accepts the fingerprint only after refresh succeeds and the source remains unchanged. It has a two-minute command timeout and exits with execd.Only the public certificate remains on the shared volume; the private key stays in egress. Future execd child processes use the refreshed trust. Long-running applications that cache TLS contexts must recreate them or restart.
Verification
go test -race ./pkg/...go vet ./...Focused tests cover unchanged certificates, late arrival, invalid/non-CA input, failed refresh retry, a second rotation during refresh, and context cancellation.