fix(broker): install the broker CA inside a runner's sandbox, without sudo - #2452
Conversation
… sudo A brokered conversation on a self-hosted runner failed at the CA install: it ran `sudo install` and `update-ca-certificates`, and a runner sandbox is a directory on the tenant's machine run as the tenant, where sudo wants a password nobody is there to type. A Mac has no update-ca-certificates either, and the machine's trust store is not Fountain's to change. On a runner, the CA and a bundle of the machine's roots plus it are now written under the sandbox's HOME, and the CA variables name their real paths on the machine, since a runner maps /home/sprite in file paths and command arguments but not in env values. Every other provider is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
0337757 to
ffe7e48
Compare
jhgaylor
left a comment
There was a problem hiding this comment.
Automated adversarial agent review.
Adversarial correctness/QA review: REQUEST_CHANGES.
Reviewed the complete PR at ffe7e48e9e5b03d41c3ac8e05dcad07e3b1572a2 against 51c753734e3f00e169976b5be1b661f6c125b61f.
-
[P2] A transient runner lookup leaves a nonexistent CA bundle in the cached environment. In
Provisioning.broker_ca_files/1, lines 624–627, bothSandbox.host_path/2calls perform independent runnergetRPCs. The lockedmanagoat_runner0.2.4 helper silently returns its input on any error. If one lookup disconnects and the runner recovers for subsequent calls, provisioning successfully writes and installs the real files but caches/home/sprite/...in the CA environment. Process runners do not map environment values, so affected TLS clients cannot read their bundle; token refresh retains these values. I reproduced the second lookup returningrunner_disconnected, followed by the actual installation returning:okand a subsequent runner exec receivingSSL_CERT_FILE=/home/sprite/.fountain/broker/ca-bundle.crt, while the bundle existed only under the runner's real temporary sandbox directory. Resolve the runner root once through an error-aware/retried lookup before building the CA environment, propagate an unresolved lookup, and cover disconnect-then-recovery. A successful Firecracker lookup may legitimately return/home/sprite, so distinguish lookup failure from that valid result. -
[P2] Git is never told to trust the HOME-only CA. The new runner install at
provisioning.ex, lines 519–522 stops installing the CA into the OS store, butBroker.ca_env/1, lines 451–457 supplies no Git-specific trust setting.clone_https/5forwards these variables and only configures proxy authentication; Git does not consumeSSL_CERT_FILEas its CA configuration. A brokered HTTPS repository clone therefore still fails certificate verification, as do later Git fetches/pushes to intercepted hosts. The parallel compatibility review verified Apple Git 2.39.5 against an isolated TLS Git endpoint: the exact PR CA variables gave exit 128 with a certificate error; addingGIT_SSL_CAINFO=<bundle>gave exit 0. Supply the mapped bundle throughGIT_SSL_CAINFOor sandbox-scopedhttp.sslCAInfo, preserve tenant override precedence, and add a real HTTPS Git regression test.
Coverage: all seven changed paths (broker.ex, conversation_server.ex, egress.ex, fresh_provision.ex, provisioning.ex, provisioning_test.exs, and changelog.d/2057-runner-broker-ca.md), fresh provisioning, wake/reattach, session refresh, repository cloning, Process/Firecracker runner mapping, and the locked Sandbox/Runner dependencies. Read the base correctness policy, contributor guides, server conventions, and relevant runner ADR.
Verification: the isolated BEAM diagnostic loaded the exact changed Broker/Provisioning source and real locked Sandbox/Runner adapter and connection modules; its fake RPC peer injected one transient lookup error, then performed real mapped writes and ran the generated install script. The diagnostic reproduced finding 1 without starting the app or database. Finding 2's source trace was checked independently; the TLS experiment was run by the parallel compatibility reviewer. This reviewer did not rerun ExUnit or perform a live-runner launch. Source review is complete.
jhgaylor
left a comment
There was a problem hiding this comment.
Automated adversarial agent review.
Independent adversarial product/API compatibility review of head ffe7e48e9e5b03d41c3ac8e05dcad07e3b1572a2 against base 51c753734e3f00e169976b5be1b661f6c125b61f: REQUEST_CHANGES. Source review complete.
- [P2] Give Git the sandbox CA bundle when removing it from the OS trust store. In broker.ex:452–458, the environment still has no
GIT_SSL_CAINFO; the new runner installation only writes the CA under HOME and configureshttp.proxyAuthMethod. Git does not consume these existing CA variables on a supported Mac runner. A brokered environment containing an HTTPS repository therefore gets past CA installation and fails inProvisioning.clone_https/5with a certificate error. Agentgit fetch/pushhas the same problem. The old installation relied on putting this CA into the system trust store, which this branch explicitly stops doing. Supply the mapped full bundle throughGIT_SSL_CAINFO(and include that key in the overridable CA keys), or configure equivalent sandbox-localhttp.sslCAInfo; add an actual Git TLS regression test.
Verification: an isolated localhost HTTPS Git repository presented a leaf certificate signed by a temporary CA. The bundle contained /etc/ssl/cert.pem plus that CA, and the client received precisely the PR's five CA variables. On Apple Git 2.39.5, git -c http.proxyAuthMethod=basic ls-remote exited 128 with SSL certificate problem: unable to get local issuer certificate. Adding only GIT_SSL_CAINFO=<the same bundle> made it exit 0. This exercises certificate trust directly; it does not claim an end-to-end live broker or runner test.
Coverage: all seven changed paths (broker.ex, conversation_server.ex, egress.ex, fresh_provision.ex, provisioning.ex, provisioning_test.exs, and changelog.d/2057-runner-broker-ca.md). Traced fresh provisioning and wake/reattach, CA override precedence and session refresh, the repository clone caller, CLI process runner path/env mapping, Firecracker's in-guest path mapping, and the unchanged hosted-provider installation. Reviewed the trusted-base contribution/API guidance and relevant runner/broker ADRs. No endpoint, response schema, generated wire contract, or SDK API changes require regeneration. The added tests cover file creation and env path spelling but do not exercise Git's TLS client. No shared-database tests were run by this reviewer.
jhgaylor
left a comment
There was a problem hiding this comment.
Automated adversarial agent review.
Adversarial security and tenant-isolation review of ffe7e48e9e5b03d41c3ac8e05dcad07e3b1572a2 against 51c753734e3f00e169976b5be1b661f6c125b61f: REQUEST_CHANGES. Source review complete. I found no additional security defect, but independently inspected the source and diagnostic scripts supporting the other reviewers' two blocking correctness findings and agree they need repair before approval.
- [P2] Preserve runner path lookup failures.
Provisioning.broker_ca_files/1, lines 624–627 calls the locked runner helper twice; each call silently returns/home/sprite/...on an RPC failure. A disconnect followed by recovery can therefore leave an invalid CA path cached in the environment even though the actual CA installation succeeds at the runner's real directory. Runner environment values are not mapped. The correctness reviewer reproduced this with the exact changed source and locked adapter. Resolve the runner root once with error-aware retry/propagation, then derive both paths; add a disconnect/recovery regression test. - [P2] Give Git the HOME-only trust bundle.
Broker.ca_env/1, lines 452–458 does not configure Git trust after the runner branch stops adding the broker CA to the OS store. The compatibility reviewer reproduced Apple Git rejecting a CA-signed HTTPS endpoint with these exact CA variables and succeeding when onlyGIT_SSL_CAINFOwas added. This affects the existing HTTPS repository-cloning stage and agent Git operations. Provide the mapped bundle through an overridable Git CA setting and cover a real HTTPS Git request.
I inspected all seven changed paths and traced fresh provisioning and reattachment through tenant-scoped launch/attach lookups, runner selection and authenticated runner registration, the pinned runner/sandbox adapters, process path and environment mapping, and broker session/credential handling. The new files contain the public CA and root bundle; session tokens remain in the existing process environment path. CA defaults retain the existing tenant override precedence. The runner branch removes privileged trust-store changes and preserves the existing broker admission and direct-egress behavior. The process runner's pre-existing trusted-host model is not treated as a newly introduced isolation defect.
Verification: source review and read-only gh/Git inspection. Reviewed the new regression assertions and relevant dependency implementations (managoat_runner 0.2.4, managoat_sandbox 0.5.0), then inspected the peer diagnostics and their reported results. I did not execute those probes, database tests, a live runner, or TLS integration tests myself. CI status and peer test results are not claimed as locally executed here.
jhgaylor
left a comment
There was a problem hiding this comment.
Adversarial agent review — maintainability and verification: REQUEST_CHANGES on ffe7e48e9e5b03d41c3ac8e05dcad07e3b1572a2 against 51c753734e3f00e169976b5be1b661f6c125b61f.
The HOME-local CA approach and atomic file replacement fit the process runner. The remaining blocker is whether the tools consuming that trust configuration work. I concur with the compatibility review's Git finding; this is the same defect, not an additional issue.
[P2] Supply the HOME bundle to Git before claiming runner provisioning succeeds. In apps/fountain/lib/fountain/broker.ex:452–459, the CA defaults omit GIT_SSL_CAINFO. The new runner installer no longer installs into the OS trust store, and its only Git setting is http.proxyAuthMethod. Provisioning.clone_https/5 subsequently runs Git with this environment and no http.sslCAInfo. The compatibility review's isolated macOS TLS probe failed with the PR's environment and succeeded when only GIT_SSL_CAINFO was added. A brokered HTTPS repository can therefore still prevent the conversation from provisioning.
Add the Git trust setting and a consumer-level regression using a temporary CA and local HTTPS Git endpoint. If implemented as an environment default, include the name in Broker.ca_keys/0 so SpriteEnv.split_brokered/1 preserves the existing tenant-override precedence. The new file-content and path assertions should remain, but they do not detect this failure.
Coverage: all seven changed paths, fresh provisioning, reattachment, broker-session refresh, environment precedence, runner process/Firecracker path behavior, the pinned runner/sandbox adapters, and existing CA installation tests. No separate blocking complexity or style findings. git diff --check passed, and the current-head GitHub CI checks are green. I inspected the compatibility review's diagnostic and its results; I did not rerun it or the full database suite. This is a source/verification review, not a completed Review Loop service run.
Review of #2452 found two gaps. The runner root came from two `host_path` calls, each answering `/home/sprite` when the runner could not be reached. One dropped lookup followed by a recovered install left that unmapped path in the env for the conversation's life. The root is now looked up once, retried, and a lookup that keeps failing fails the launch. Only a brokered conversation asks. Git reads none of the CA variables the broker set; Apple's git ignores SSL_CERT_FILE. With the OS trust store no longer touched on a runner, a brokered HTTPS clone failed verification. GIT_SSL_CAINFO now names the bundle, as an overridable default like the rest, and a test runs git against a local HTTPS server signed by a temporary CA. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
|
Both review findings are addressed in b88818e.
|
…l runner get The certificates :public_key.pkix_test_data/1 makes pass LibreSSL but not GnuTLS, which Ubuntu's git uses, so the test failed in CI where it matters most. openssl now makes the CA and the leaf; checked against GnuTLS git 2.43 in ubuntu:24.04 before relying on it. A runner's get always names the sandbox directory (process.go and firecracker.go both return "path"); the platform-inference test's fake did not, which the root lookup now requires. Credo's two notes on the new test are fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>
jhgaylor
left a comment
There was a problem hiding this comment.
Automated adversarial agent review.
Adversarial correctness/QA review: APPROVE.
Reviewed the entire PR at c34672d0205878afa04f1de060e514dddef7be8e against 51c753734e3f00e169976b5be1b661f6c125b61f, including the original implementation and subsequent fixes. No remaining actionable medium-or-higher correctness defects found.
Both earlier blockers are resolved. The runner root now comes from a single error-aware lookup with retries; persistent failure cannot produce an apparently successful environment containing an unmapped CA path. Fresh provisioning and reattachment resolve it before building the environment. GIT_SSL_CAINFO now names the resolved bundle and belongs to the overridable CA defaults, including sudo preservation on hosted providers. Session refresh retains tenant CA overrides. The added tests exercise temporary and persistent lookup failure and real Git TLS verification.
Independent verification: an isolated BEAM diagnostic loaded this head's Broker/Provisioning source and the real locked Sandbox/Runner adapter and connection modules. A fake RPC peer injected a disconnect, then performed real mapped file writes and ran the generated installation script. The retry recovered; installation returned :ok; a subsequent exec received an existing CA bundle; and GIT_SSL_CAINFO matched that bundle. Additional assertions confirmed that persistent disconnect returns an error and a successful Firecracker /home/sprite root remains valid. No database was used. I did not rerun the ExUnit suite or launch a live runner; the source review and the focused diagnostics above are complete.
Coverage included fresh provisioning, wake/reattach and failure handling, broker session refresh, repository cloning, tenant override precedence, Process/Firecracker mapping, the locked dependencies, and all nine changed paths:
apps/fountain/lib/fountain/broker.exapps/fountain/lib/fountain/conversations/conversation_server.exapps/fountain/lib/fountain/conversations/egress.exapps/fountain/lib/fountain/conversations/fresh_provision.exapps/fountain/lib/fountain/conversations/provisioning.exapps/fountain/test/fountain/conversations/conversation_server_platform_inference_test.exsapps/fountain/test/fountain/conversations/provisioning_test.exschangelog.d/2057-runner-broker-ca.mddocs/concepts/secrets.md
jhgaylor
left a comment
There was a problem hiding this comment.
Automated adversarial agent review.
Independent adversarial product/API compatibility review of the entire PR at c34672d0205878afa04f1de060e514dddef7be8e, against base 51c753734e3f00e169976b5be1b661f6c125b61f: APPROVE. Source review complete; no remaining actionable medium-or-higher compatibility findings.
The previous Git defect is resolved: GIT_SSL_CAINFO names the full mapped bundle, belongs to the overridable CA keys, reaches the repository-clone environment, and survives broker session refresh. The documentation and hosted-provider sudo environment handling include it. Runner CA paths now derive together from a successful, retried get; lookup errors propagate before the environment is built. This removes the silent /home/sprite fallback while retaining Firecracker's legitimate in-guest /home/sprite and avoiding unnecessary lookups for unbrokered conversations.
Independent verification: extracted the six CA assignments from this head's Broker.ca_env source and used them against an isolated localhost HTTPS Git repository with a temporary-CA-signed leaf certificate and a bundle containing the machine roots plus that CA. Apple Git 2.39.5 ls-remote exited 0; removing only GIT_SSL_CAINFO reproduced exit 128 and unable to get local issuer certificate. Curl and Node HTTPS also succeeded with the same source-derived environment. Reviewed the added real-Git TLS regression test and the transient/permanent lookup assertions.
Coverage includes all nine changed paths: broker.ex, conversation_server.ex, egress.ex, fresh_provision.ex, provisioning.ex, conversation_server_platform_inference_test.exs, provisioning_test.exs, changelog.d/2057-runner-broker-ca.md, and docs/concepts/secrets.md. Traced fresh and wake/reattach paths, CA override precedence and refresh, clone callers, process-runner path/env mapping, Firecracker's guest path, and existing hosted-provider behavior. Reviewed the trusted-base contribution/API guidance and runner/broker ADRs. No endpoint, response schema, SDK surface, or generated wire contract changes require regeneration.
Verification limits: the independent probe exercised local macOS clients and certificate trust, not a live broker, full remote runner provisioning, or a Firecracker VM. I did not run shared-database tests or the full repository suite. Required CI remains a separate landing gate.
jhgaylor
left a comment
There was a problem hiding this comment.
Automated adversarial agent review.
Adversarial security and tenant-isolation re-review of the complete PR at c34672d0205878afa04f1de060e514dddef7be8e against 51c753734e3f00e169976b5be1b661f6c125b61f: APPROVE. Source review complete; no introduced defect requiring changes found.
The two previous blockers are addressed in the current source. Runner CA paths now derive from one successful, retried root lookup; errors propagate through both fresh provisioning and reattachment instead of becoming cached fallback paths. A successful Firecracker /home/sprite result remains valid. GIT_SSL_CAINFO names the full bundle and belongs to ca_keys, preserving tenant override precedence while leaving proxy variables authoritative. The added tests exercise lookup recovery/failure and Git certificate verification with and without that variable.
I inspected all nine changed paths: Broker, ConversationServer, Egress, FreshProvision, Provisioning, both changed test files, the changelog, and the secrets documentation. I traced the relevant tenant-scoped launch/attach and authenticated runner routing, process/Firecracker path mapping, shell/file operations, env serialization, credential custody, session refresh, retry/error handling, and cleanup. The public CA remains inside the runner sandbox and no privileged host trust-store write is introduced. The existing trusted runner/direct-egress model remains unchanged.
Verification: source and dependency inspection, comparison with the trusted base, and git diff --check. Reviewed the added TLS test and regression assertions, but did not execute ExUnit, a live runner, TLS integration probes, or shared-database tests in this review. This is a new review of the current complete diff, not reliance on the author's resolution claims or the previous verdict.
jhgaylor
left a comment
There was a problem hiding this comment.
Adversarial agent review — maintainability and verification: APPROVE at c34672d0205878afa04f1de060e514dddef7be8e against 51c753734e3f00e169976b5be1b661f6c125b61f.
Both blockers from the prior review round are addressed. Runner CA paths now come from one retried, error-aware root lookup before environment construction on both fresh provisioning and reattachment. Failed lookups propagate instead of silently caching /home/sprite; a successful Firecracker guest root remains valid. GIT_SSL_CAINFO names the same bundle and is included in ca_keys/0, preserving the documented tenant-override precedence.
I reviewed all nine changed paths, the full base-to-head diff and fix delta, relevant callers and pinned dependency behavior. The implementation keeps the provider distinction localized, preserves existing call defaults, and adds meaningful retry/failure and real TLS-client coverage. No remaining blocking maintainability or correctness finding from this pass.
Verification: git diff --check passed. I ran an isolated Apple curl check against a localhost HTTPS endpoint with a temporary-CA-signed leaf and the current six CA environment variables: exit 0, HTTP 200. Current-head GitHub CI, including all six test partitions, static analysis, coverage, and CI required, is green. The independent reviewers also verified the retry behavior and Git's positive/negative TLS controls. I did not run the full database suite or a live runner/broker deployment. This approval records the source review and stated verification, not a completed Review Loop service run.
Problem
With
BROKER_ALLOW_UNENFORCEDon, a runner conversation gets past the broker gate. It then fails at the CA install:install_broker_ca/2assumes a Linux sandbox with passwordless root. It runssudo installinto/usr/local/share/ca-certificates,update-ca-certificates, and a sudoers drop-in. A self-hosted runner's sandbox is a directory on the tenant's own machine, run as the tenant.sudowants a password nobody is there to type. A Mac has noupdate-ca-certificates. And even where the install could run, it would make every process on the tenant's machine trust the broker. The machine's trust store isn't Fountain's to change.Change
On a
:runnerhandle:install_broker_ca/2writes the CA to/home/sprite/.fountain/broker/ca.crt, which the runner maps into the sandbox directory. It then buildsca-bundle.crtfrom the machine's own roots plus the CA. The roots come from the first of Debian's, Fedora's or macOS's store that exists. Each file is staged beside its destination and renamed into place, so concurrent conversations on one sandbox never read half a file. It doesn't use sudo,update-ca-certificatesor sudoers. It still pinshttp.proxyAuthMethod basic, which writes to the sandbox's own.gitconfig.broker_ca_files/1resolves the runner's root once, with a retriedSandbox.get, and gives the CA variables their real paths under it. A runner maps/home/spritein file paths and command arguments, but env values reach it verbatim (cli/internal/runner/process.go:env()does no mapping). A lookup that keeps failing fails the launch. It doesn't fall back to/home/sprite, which is whathost_path/2returns on error and would leave every TLS client without its roots. Only a brokered conversation does the lookup (Egress.ca_files/2), and it's awithstep on both the fresh-provision and wake paths.GIT_SSL_CAINFOjoins the CA variables andca_keys/0. It names the same bundle asSSL_CERT_FILE, because git doesn't read that one (Apple git ignores it). Tenants can still override it throughenv_varsor a secret.Every other provider still installs into the OS trust store.
ca_env/0andsandbox_env/1keep their defaults, and the only change there is the extraGIT_SSL_CAINFOpair, which names the same system bundle.Not covered
Testing
provisioning_test.exs:update-ca-certificates.bashrun in a tmp dir that rewrites/home/spritethe way the runner does: the bundle keeps the machine's roots and ends with the CA, and the staging file is removed.GIT_SSL_CAINFO; non-runner files are unchanged.git ls-remoteagainst a local HTTPS server whose certificate a temporary CA signed. WithoutGIT_SSL_CAINFOit fails with a certificate error (the control). With the broker CA env it gets through the handshake to a 404.provisioning_test,egress_test,sprite_env_testandbroker_native_test: 130 tests, 0 failures, 1 skipped (the existing Linux-only trust-store test). Run locally on macOS.test/fountain/conversations/run locally failed with Ecto sandbox shared-mode ownership errors, apparently from another test run sharing the DB. I'm leaving that to CI.alecraso/hud) on a macOS runner against managoat.com. I'll verify end to end once this is deployed.🤖 Generated with Claude Code