Repository navigation
Conversation
|
a65ec41 to
76279f5
Compare
46692e8 to
eb2db3f
Compare
politerealism
left a comment
politerealism
left a comment
There was a problem hiding this comment.
Summary
The design is sound and directly addresses both #3781 (Podman TLS-trust regression) and #3295 (general additional-destination-CA feature): trust material is scoped to the supervisor container only, root-store construction is fixed for both the bundled-ca-roots and native-store variants, validation is fail-closed end-to-end, and the gateway-mTLS trust boundary is provably untouched (new destination_ca_cannot_authenticate_gateway regression test). However, the branch as submitted (eb2db3f65) doesn't compile its own test suite, so CI can't pass yet.
Blocking
cargo test -p openshell-driver-podman -p openshell-driver-docker -p openshell-server (and mise run pre-commit's rust:lint step) fail to compile, all four attributable to this PR's own commits:
crates/openshell-driver-podman/src/container.rs:1833— new testadditional_destination_trust_is_delivered_only_to_supervisor_with_digestis missing the pre-existing required fieldrootlessonIsolationSpecInput.crates/openshell-driver-podman/src/container.rs:2020— existingrootful_specs = build_isolation_specs(...)wasn't updated with the new required fieldnetwork_trust.crates/openshell-driver-docker/src/tests.rs:135— setsdaemon_version: "28.0.0".to_string(), butDockerDriverRuntimeConfighas no such field.crates/openshell-server/src/compute/mod.rs:13576— new teststart_sandbox_request_uses_optional_wire_field_six_for_durable_snapshotis missing the pre-existing required fieldexpected_runtime_identityonStartSandboxRequest.
openshell-driver-kubernetes, openshell-driver-vm, and openshell-gateway build and test cleanly in isolation, so this is localized to four struct literals, not systemic.
Non-blocking notes
deploy/helm/openshell/templates/clusterrole.yaml:82-97/role.yaml:65-70: RBAC grants unscopedgeton ConfigMaps (in addition tocreate), sinceresourceNamescan't restrict dynamically digest-named generations. That lets the gateway service account read any ConfigMap in the bound scope, not just its own CA generations. Reasoning is documented in-line and in thedebug-openshell-clusterskill update, but worth a second look given it's a real (if bounded) privilege widening.crates/openshell-supervisor-network/src/run.rs:212-219: the additional-CA load path fails closed correctly but doesn't emit aConfigStateChangeBuilderOCSF event the way the adjacent TLS-termination code does atrun.rs:401-429. Not required by AGENTS.md's OCSF scope (that'sopenshell-sandbox-specific), but would be a nice consistency improvement for operator visibility.
Testing
cargo fmt --all -- --check passes. cargo test -p openshell-supervisor-network -p openshell-core -p openshell-supervisor passes (1344/1345, one pre-existing unrelated flake). Docker/Kubernetes/VM e2e lanes haven't been run (not available in my review environment) and remain genuinely unverified — recommend running those before merge, in addition to fixing the compile errors above.
eb2db3f to
60826fb
Compare
Since v0.1.2 the Podman supervisor runs as its own container, separate from the workload image, so it can no longer inherit a private CA baked into the workload's system trust store. Policy-allowed, inspected HTTPS requests to destinations signed by that CA fail upstream TLS establishment, and the only workaround is rebuilding the supervisor image with the CA baked in. Add a Podman-only additional_ca_bundle setting, independent of the existing proxy_ca_bundle (which requires https_proxy), that bind-mounts an operator PEM into the supervisor container and folds it into the upstream trust store alongside the system CA bundle. Validated on the gateway host at sandbox-create time, matching the existing corporate proxy CA bundle's validation, so a misconfigured path or certificate-free bundle fails closed immediately rather than producing a failed sandbox once the supervisor container starts. Scoped to Podman only, reusing the existing additive-bundle mechanism without new gateway.toml schema, proto fields, or cross-driver plumbing -- a narrower alternative to the in-flight cross-driver implementation in NVIDIA#3292, which remains the intended long-term direction. Fixes NVIDIA#3781. Signed-off-by: politerealism <[email protected]>
|
This pull request has had no activity for 14 days and is now marked stale. |
github-actions
Bot
added
the
state:stale
5672597 to
309301d
Compare
…ss (NVIDIA#3996) * fix(podman): trust an operator-supplied additional CA for direct egress Since v0.1.2 the Podman supervisor runs as its own container, separate from the workload image, so it can no longer inherit a private CA baked into the workload's system trust store. Policy-allowed, inspected HTTPS requests to destinations signed by that CA fail upstream TLS establishment, and the only workaround is rebuilding the supervisor image with the CA baked in. Add a Podman-only additional_ca_bundle setting, independent of the existing proxy_ca_bundle (which requires https_proxy), that bind-mounts an operator PEM into the supervisor container and folds it into the upstream trust store alongside the system CA bundle. Validated on the gateway host at sandbox-create time, matching the existing corporate proxy CA bundle's validation, so a misconfigured path or certificate-free bundle fails closed immediately rather than producing a failed sandbox once the supervisor container starts. Scoped to Podman only, reusing the existing additive-bundle mechanism without new gateway.toml schema, proto fields, or cross-driver plumbing -- a narrower alternative to the in-flight cross-driver implementation in NVIDIA#3292, which remains the intended long-term direction. Fixes NVIDIA#3781. Signed-off-by: politerealism <[email protected]> * fix(podman): keep additional CA mount on the supervisor build_isolation_specs() keeps only trusted_mount() destinations on the supervisor, and the additional CA path was not in that list. The bind mount stayed on the workload while the supervisor received --additional-ca-bundle for a file it did not have, so startup failed. Add the path to trusted_mount() and cover the final supervisor and workload specs with a test. Correct the docs and a code comment to say the bundle is validated when the driver starts, and add Podman troubleshooting guidance to the debug-openshell-cluster skill. Signed-off-by: politerealism <[email protected]> --------- Signed-off-by: politerealism <[email protected]>
github-actions
Bot
removed
the
state:stale
309301d to
aaf328b
Compare
Maintainer Convergence DecisionThe current head is rebase-equivalent to the last reviewed patch. No new code review was run, no findings were reopened, and Three carried obligations remain open. The feedback ledger reports the autonomous Warning budget exhausted; the decision needed is how to dispose of these existing findings, not whether to expand review. Root-cause findings:
Scope growth: None identified in this equivalent patch. Reviewer-quality signals: The ledger reports repeated finding IDs across review summaries; these are carried obligations, not new author defects. No new findings were proposed this cycle. Action required: @NVIDIA/openshell-maintainers, confirm which of the three carried findings require an author fix or explicitly waive each accepted risk in its existing thread. A maintainer must also approve the current-head Trivy Changes workflow. Exact-head test authorization is posted after verifying repository admin authority. Branch Checks, Helm Lint, and Branch E2E Checks are now running. DCO and PR Issue Acceptance pass. No test labels were applied. Gator metadata
|
|
/ok to test 83cf46d |
PR Review StatusThe critical-only follow-up confirms that the three carried findings are fixed. @jhjaggars, I checked your changes: Podman supervisor refresh now uses the retained workload's pinned image for identity and environment, standalone CA sources are staged into driver-owned read-only artifacts, and VM E2E checks host-delivered trust without requiring guest artifacts. I resolved those three Gator threads; Action required: a maintainer must approve pending workflow execution for this head, including required Trivy Changes. Dependency Review, Workflow Security Reports, and Branch Docs Preview also report that approval is required. The PR remains Blocking findings: None remain. Carried findings:
Gator metadata
|
|
/ok to test b7f666c |
PR Review StatusThe critical-only follow-up found no new blocking defects in the author-only rebase changes. The Kubernetes shared-supervisor combination now rejects additional destination trust explicitly rather than silently omitting it. All four prior Gator findings remain resolved; no maintainer convergence decision is needed. Action required: a maintainer must approve workflow execution for this head, including required Trivy Changes. Dependency Review, Workflow Security Reports, and Branch Docs Preview also require approval. The PR remains Blocking findings: None. Carried findings: Gator metadata
|
|
/ok to test f2194ea |
PR Review StatusThe current head is rebase-equivalent to the last reviewed patch. All four prior Gator findings remain resolved; no new reviewer run or maintainer convergence decision is needed. Issue #3295 remains accepted and Action required: a maintainer must approve workflow execution for this head, including required Trivy Changes. Dependency Review, Workflow Security Reports, and Branch Docs Preview also require approval. I verified repository admin authority and posted the standalone test authorization for the current head. The mirror matches; Branch Checks and Branch E2E Checks are queued, and Helm Lint is running. DCO and PR Issue Acceptance pass. The PR remains Blocking findings: None remain. Carried findings: Gator metadata
|
Signed-off-by: Jesse Jaggars <[email protected]> # Conflicts: # crates/openshell-gateway/src/lib.rs
Signed-off-by: Jesse Jaggars <[email protected]>
Signed-off-by: Jesse Jaggars <[email protected]>
Signed-off-by: Jesse Jaggars <[email protected]>
Signed-off-by: Jesse Jaggars <[email protected]>
Signed-off-by: Jesse Jaggars <[email protected]>
Resolve rebase fallout against current upstream main after the series was rebased 214 commits forward: - follow upstream NVIDIA#3110: supervisor→gateway TLS is CA-only, so configure_gateway_tls and its isolation tests drop the client identity - follow upstream tls_secret_names reduction to [String; 1] in the Podman driver while keeping the network trust bundle delivery - port [openshell.supervisor.network] rendering into the openshell.gatewayConfigToml serializer in _toml.tpl - adapt tests to upstream struct/function changes (DriverStartupContext, SandboxRunConfig, IsolationSpecInput, supervisor_pod, VM bootstrap image) - drop conformance-artifact-prefix plumbing removed upstream Signed-off-by: Jesse Jaggars <[email protected]>
Unify the additional destination CA series on the mechanism merged to main in NVIDIA#3996: one operator pre-assembled PEM bundle, the --additional-ca-bundle supervisor flag, and the /etc/openshell/tls/additional/ca-bundle.pem staging path. - Replace the gateway-level additional_ca_cert_paths list with a single additional_ca_bundle file validated and staged at gateway startup. - Restore the upstream supervisor argument path and fold-into-roots behavior; drop the --network-additional-ca-bundle and --network-additional-ca-digest contracts. - Keep gateway normalization bounds, content-addressed staging, digest-based driver reconciliation, and stop/start rotation; the freshness check now runs at the gateway/driver boundary instead of the supervisor boundary. - Reject additional_ca_bundle in operator driver JSON config; standalone drivers self-source the bundle from --sandbox-additional-ca-bundle with the same validation. - Extend the Kubernetes ConfigMap name digest segment to keep the full 63-byte budget after the prefix rename. - Box the persisted-sandbox reconciliation futures so callers stay under the clippy::large_futures threshold. Updates Docker, Podman, Kubernetes, and VM delivery, Helm chart and tests, e2e wiring, gateway configuration docs, and the cluster debugging skill to the single mechanism. Signed-off-by: Jesse Jaggars <[email protected]>
… lockfile The network trust startup tests asserted on raw substrings of gateway diagnostics, but the CLI renderer wraps output at a fixed width with continuation markers and hard-breaks long paths. Compare assertions on whitespace- and continuation-squashed text so they hold regardless of wrap position. Format the e2e workspace (separate cargo workspace, not covered by the root cargo fmt run) and refresh examples/isolation-backend-v8/Cargo.lock for the libc dependency added to the network trust crate. Signed-off-by: Jesse Jaggars <[email protected]>
The additional CA e2e never ran in CI before the first maintainer test dispatch, and its local container inspection was broken in two ways: it located the supervisor container through e2e network membership (the supervisor is host-networked or runtime-bridged, so the filter matched the CI job container instead), and it required the gateway mTLS CA as a bind mount (Podman delivers it as a container secret when user namespaces remap UIDs). Select the supervisor by managed labels and accept the secret delivery form. The lifecycle leg then exposed a driver bug: stopped-pair trust replacement re-created the per-sandbox TLS CA secret, colliding with the secret still owned by the renamed stopped pair. Mirror the token and resolver secret handling: require and reuse the existing secret during replacement instead of creating it. Validated end-to-end with the Podman additional CA e2e lane. Signed-off-by: Jesse Jaggars <[email protected]>
…ilures Recovery no longer marks sandboxes Error/InvalidPersistedSandbox when driver_start_request fails during the startup or lifecycle sweeps. Gateway configuration problems (for example a missing launch signer) must not permanently fail unrelated sandbox records; the sweep logs a warning and skips them, matching the behavior before the additional CA work. Sandbox-record failures surface through the normal start path. Signed-off-by: Jesse Jaggars <[email protected]>
Replace the stopped-pair trust reconciliation with supervisor-only recreation. The destination-trust generation label moves from the workload to the supervisor container, and start_sandbox recreates only the supervisor when its generation is missing or stale. The workload container, its volumes, its secrets, and its workspace data are never replaced: the additional CA bundle is mounted only into the supervisor. Running sandboxes are never gated on trust labels, and recovery without fresh launch credentials keeps the existing supervisor. The intentional -removal fence machinery in the watcher existed only for workload replacement and is removed with it. Signed-off-by: Jesse Jaggars <[email protected]>
…nges Delete the stopped-workload trust reconciliation path and all of its machinery: the trust-generation label on workload containers, the generation match check in the start fence, the stopped-sandbox replacement, the bounded workspace archive copy with its 512 MiB cap and regular-file-only validation, and the boundary-state rollback helpers. No replacement machinery is needed: the trusted supervisor container is already re-created from the gateway's current configuration on every start via ensure_control_process_for_container, so stop/start picks up the current additional CA bundle while the workload container, its volumes, and its workspace data are never replaced. Signed-off-by: Jesse Jaggars <[email protected]>
…ifecycle The stop/start lifecycle description already matches the supervisor-only refresh model; remove the two remaining references to restart reconciliation of workload containers. Signed-off-by: Jesse Jaggars <[email protected]>
Route the gateway TLS isolation test PKI, the destination CA source fixtures, and the gateway startup trust tests through openshell_crypto::pki now that rcgen access is centralized there: generate_keypair/self_signed/signed_by replace the direct rcgen calls and CertifiedKey field access follows the wrapper's shape. Signed-off-by: Jesse Jaggars <[email protected]>
The first maintainer test dispatch revealed two latent issues in the additional CA e2e that no local run had exercised: the Podman gateway CA secret helper referenced ContainerEngine without the container feature gate, breaking Kubernetes and VM builds of the test, and the Docker branch asserted a client ca.crt bind mount that the Docker driver never creates. Gate the helper like its callers, and assert Docker's actual delivery path instead: the gateway mTLS CA staged as tls/ca.pem inside the read-only supervisor state volume, selected through the OPENSHELL_TLS_CA environment pointer. Signed-off-by: Jesse Jaggars <[email protected]>
The helm:docs check regenerates the chart README with helm-docs, which orders rows alphabetically; a conflict resolution had placed supervisor.network.additionalCaConfigMapName after the supervisor.podSecurityContext rows. Regenerate with mise run helm:docs. Signed-off-by: Jesse Jaggars <[email protected]>
Signed-off-by: Jesse Jaggars <[email protected]>
Standalone Podman drivers accepting --sandbox-additional-ca-bundle previously kept the operator's source path as the trust artifact path. The immutable-artifact verification performed on every create/start requires exactly mode 0444, which an operator-owned source path cannot guarantee, so a readable-but-writable source failed reconciliation. Read the operator bundle with the same bounded, no-symlink validation, then stage the normalized material into a driver-owned artifact inside a private tempdir held for the driver process lifetime. The artifact is created 0600, synced, and only then restricted to 0444 so no observer ever sees a writable artifact. The operator source and its permissions are never referenced after startup. Addresses GATOR-291f43b2-02. Signed-off-by: Jesse Jaggars <[email protected]>
…resh Supervisor-only trust reconciliation re-entered create_sandbox_with_existing_token, which re-pulled and re-resolved the workload image tag even though the workload container is retained. If the tag moved to a different image between stop and start, identity resolution and child-environment construction would reflect the new image while the retained workload still runs the original one, leaving mode-0600 bootstrap files the retained user cannot read. During a refresh with a retained workload, inspect the stopped container and pin to the immutable image ID it was created from (Config.Image); never pull or resolve the request tag. The requested reference is still used for child metadata so the retained workload's environment is byte-identical to its original launch. Addresses GATOR-291f43b2-01. Signed-off-by: Jesse Jaggars <[email protected]>
The additional-CA lane's VM assertions read the destination CA out of the guest overlay and a guest-side supervisor-argv marker — artifacts from the guest-supervisor design that no longer exist now that the VM driver stages the CA host-only and runs the supervisor on the host. Inspect the driver-staged host artifact (digest-compared against the source), assert the guest overlay contains no CA material at the old mount paths, and keep the serial-console no-disclosure check. The workload validation scripts also ran curl, which exists only in the container-engine fixture image; the Kubernetes and VM lanes use the uv-based workload image and died instantly with "canonical main process already finished". Rewrite the main-process, exec, and lifecycle validation scripts in python3 (present in every lane's workload image). The network-policy binary identity is the resolved /proc/<pid>/exe target, so the versioned python3.12 binaries are allowlisted rather than the unversioned symlinks. The hostname mismatch check keeps its semantics: same reachable fixture, mismatched TLS SNI, and the failure must specifically be hostname verification. Addresses GATOR-291f43b2-04. Signed-off-by: Jesse Jaggars <[email protected]>
|
/ok to test f1422cd |
PR Review StatusThe current head is rebase-equivalent to the last reviewed patch. All four prior Gator findings remain resolved; no new reviewer run or maintainer convergence decision is needed. Issue #3295 remains accepted and Action required: a maintainer must approve workflow execution for this head, including required Trivy Changes. Dependency Review, Workflow Security Reports, and Branch Docs Preview also require approval. I verified repository admin authority and posted the standalone test authorization for the current head. Branch Checks, Helm Lint, and Branch E2E Checks now have queued current-head runs. DCO and PR Issue Acceptance pass. The PR remains Blocking findings: None remain. Carried findings: Gator metadata
|
Summary
Add a global network-supervisor configuration for additional destination CA certificates so sandbox egress can trust private PKI without replacing default roots or changing gateway control-plane trust.
This series adopts the mechanism merged to main in #3996 (from #3781) and extends it to every first-party compute driver: one operator pre-assembled PEM bundle, the
--additional-ca-bundlesupervisor flag, and the/etc/openshell/tls/additional/ca-bundle.pemstaging path.Related Issue
Fixes #3295
Changes
[openshell.supervisor.network].additional_ca_bundle(single pre-assembled PEM file) with strict, bounded startup validation and fail-closed capability-based driver handling.additional_ca_bundlein operator driver JSON config so the global gateway setting stays the single source; standalone drivers self-source the same bundle from--sandbox-additional-ca-bundlewith identical validation.Why lifecycle reconciliation is included
Additional CA sources are startup-only gateway configuration. Updating the source file or Helm source ConfigMap does not hot-reload running supervisors. To roll certificates, an operator updates the source, restarts or redeploys the gateway, and explicitly stop/starts each affected sandbox.
Each runtime records the immutable trust generation it was created with. Without reconciliation, ordinary stop/start cannot reliably move an existing sandbox to the new generation: Docker and Podman retain old container metadata, Kubernetes retains the old content-addressed ConfigMap mount, and VM retains the old writable overlay. Replacing only the staged file would conflict with the recorded digest and correctly fail closed.
The only existing manual alternative is to delete and recreate each affected sandbox, or create a replacement sandbox and move work to it. Sandbox deletion normally removes its driver-owned mutable filesystem storage—Docker/Podman workspace volumes, the Kubernetes sandbox PVC, or the VM writable overlay. Operators would therefore need to back up and restore required
/sandboxcontents, use external persistent storage, and re-establish any sandbox-specific state. The reconciliation implemented here avoids that migration by rebuilding stale runtime resources during stop/start while preserving the sandbox record and supported durable workspace data.This lifecycle work can be split into a follow-up only if destructive replacement plus explicit data migration is an acceptable interim certificate-rotation procedure.
Testing
mise run pre-commitpassesmise run testtask run as a single validation commandChecklist