Conversation
|
a65ec41 to
76279f5
Compare
46692e8 to
eb2db3f
Compare
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.
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
eb2db3f to
60826fb
Compare
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
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. Deliver the normalized trust bundle consistently through Docker, Podman, Kubernetes combined/sidecar, and VM compute paths, including lifecycle-safe certificate rollover for stopped sandboxes.
Related Issue
No linked accepted issue (process discrepancy): this feature was implemented and published by direct user request. An accepted issue is still required before this PR is ready to merge.
Changes
[openshell.supervisor.network].additional_ca_cert_pathswith strict, bounded startup validation, canonicalization, redacted metadata, and fail-closed capability-based driver handling.Why lifecycle reconciliation is included
Additional CA sources are startup-only gateway configuration. Updating a 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 supervisor's 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