Skip to content

feat(server)!: harden gateway peer transport and expand HA conformance - #3825

Open
EmilienM wants to merge 7 commits into
NVIDIA:mainfrom
EmilienM:feat/3529-ha-peer-transport/EmilienM
Open

EmilienM wants to merge 7 commits into
NVIDIA:mainfrom
EmilienM:feat/3529-ha-peer-transport/EmilienM

Conversation

@EmilienM

Copy link
Copy Markdown
Contributor

Summary

Gateway replicas on PostgreSQL relay whole supervisor sessions to each other and authenticate with a projected ServiceAccount token. Until now the dialer picked TLS from the scheme the remote owner advertised, fell back to platform trust roots when no peer CA was set, and a plaintext gateway accepted http:// peers with only a warning. This PR makes peer transport fail closed: HTTPS with an explicitly pinned peer CA, or plaintext only through one explicit, loud opt-out on a plaintext gateway. The chart refuses the insecure combination at render time. The PR also documents which operations work through a non-owner replica and adds an HA e2e suite that drives them through non-owner replicas across scale-up, scale-down, graceful and forced owner loss, and a rolling restart, over HTTPS peers.

The PR was drafted and adversarially reviewed by Opus 5.5 through multi-agent workflows (research, planning, per-commit review, and a final review gate). The design section and "How this was built" below describe that process and what it changed.

Related Issue

Closes #3529. It addresses all seven acceptance criteria, with one deliberate deviation on AC1 described under Design: plaintext is rejected by default, and one explicit opt-out remains for plaintext gateways (edge TLS termination, dev and CI).

The issue has no acceptance or agent-workflow labels; it is on the 0.1.5 milestone. Implementation was directly requested; labels are unchanged.

Breaking changes and upgrade

  1. A PostgreSQL release with server.disableTls=true stops rendering. Set server.peer.allowInsecureTransport=true to keep the current behavior on a trusted network. Do not turn on gateway TLS in the same upgrade: existing sandboxes keep the plaintext endpoint they received at creation and would be stranded.
  2. A PostgreSQL release with a custom server TLS Secret needs ca.crt in that Secret, and the server certificate must chain to it and carry <fullname>.<namespace>.svc.cluster.local. Otherwise the gateway refuses to start. Chart-generated PKI and cert-manager installs need no change.
  3. Non-chart PostgreSQL gateways with an http:// peer endpoint need OPENSHELL_PEER_ALLOW_INSECURE_TRANSPORT=true or TLS. HTTPS peers need OPENSHELL_PEER_TLS_CA_FILE; platform roots are no longer used.

Release note: Gateways on PostgreSQL now require HTTPS peer transport with an explicit peer CA. Plaintext peers need server.peer.allowInsecureTransport=true (Helm) or OPENSHELL_PEER_ALLOW_INSECURE_TRANSPORT=true.

Design

Where enforcement lives. Every peer RPC and relay goes through one dialer, PeerRouteCache::channel → build_peer_channel. The new peer_transport module owns an immutable PeerTransportPolicy, installed once at startup, and build_peer_channel checks it before any I/O. Enforcing at startup alone would not help: the replica that sends the token is the dialer, and it dials endpoints that other replicas wrote into the owner records (for example old plaintext pods during a disableTls flip). Rejecting on the receiving side cannot help either, because by then the token is already on the wire.

Dial rule. https:// requires OPENSHELL_PEER_TLS_CA_FILE; the native-roots fallback is deleted. That fallback mattered more than it looked: rustls-native-certs replaces the platform store with SSL_CERT_FILE, which the chart sets for the OIDC CA, so peers could end up trusting only the OIDC CA. http:// is dialed only with the opt-out on a gateway that itself serves plaintext. unix:, local:// and anything else are refused. Unit-test builds (cfg(test)) also dial numeric loopback so in-process fake peers keep working; production builds never do.

Startup. When peer routing is expected (PostgreSQL plus a peer endpoint, even with one replica, because a Deployment rollout surges a second pod), startup rejects a plaintext own endpoint without the opt-out. It parses the CA (absent, empty and non-PEM all fail), checks the client cert/key pair and the server name, and requires a peer client identity when the listener requires client certificates. It also verifies that this gateway's own server certificate, the one SNI actually selects for peers, would pass peer verification. Every replica mounts the same Secret, so a bad custom certificate becomes a clear boot error instead of a cross-replica handshake failure. Expired or not-yet-valid certificates only warn, and are still checked for chain and name inside their validity window, so certificate expiry never adds a crash class.

Why an opt-out instead of mandatory pod TLS. With server.disableTls=true behind edge TLS, the ingress-to-pod hop and the supervisor-to-gateway hop already carry sandbox JWTs, user bearer tokens and relay bytes in cleartext on the same pod network. Mandatory peer TLS without mandatory pod TLS buys little, and mandatory pod TLS would break the documented edge-termination recipe and single-replica PostgreSQL installs. The opt-out follows the proxy_auth_allow_insecure precedent: rejected by default, honored only on a plaintext gateway and only for http://, never derived from disableTls, ignored with a warning on a TLS gateway, and logged as a warning at every startup on PostgreSQL.

Relays fail fast on local refusals. open_routed_relay_with_message retried every error for 15 seconds, so a policy refusal would have looked like a stall ending in supervisor session not connected. One new match arm returns immediately when the local policy refuses the owner endpoint. It keys on the immutable local policy (refuses_dial), never on a status code a remote peer returned, so a peer cannot trigger it. Handshake failures (wrong CA, wrong name) keep retrying, since a certificate can be rotated, and the dialer now reports the rustls cause (UnknownIssuer, certificate not valid for name) instead of tonic's bare "transport error".

Peer identity. Two layers, now both tested. Transport: the owner certificate must chain to the pinned CA and carry the configured server name, so the token is never sent to an unverified peer. Application: the receiver checks TokenReview, audience, ServiceAccount, live pod UID, release labels, x-openshell-peer-replica against the token's pod, and PeerRelay requester against the principal. The peer TLS client certificate is not an identity: it is the shared openshell-client certificate, and it is absent with enableMtls=false.

Rejected alternatives. A dedicated peer listener (large, leaves the other hops plaintext, breaks #3661's redirect targeting). Mandatory x-openshell-peer-replica (adds nothing, since the principal comes from TokenReview). Envoy + BackendTLSPolicy for the TLS HA lane (TLS pods behind a plaintext Envoy listener break SSH-based upload, download and forward, because CreateSshSession derives the scheme from pod TLS; it also needs Gateway API CRDs only the Envoy chart installs and would not exercise the peer client certificate).

Changes

  • Peer transport policy (feat(server)!). New crates/openshell-server/src/peer_transport.rs (policy, dial rule, startup validation, SNI-aware own-certificate check, error chains, IPv6 endpoint bracketing for the chart's $(OPENSHELL_POD_IP)). PeerTlsClientConfig loses its native-roots branch and gains CA, identity and server-name checks. PeerRouteCache carries the policy; build_peer_channel enforces it. New env OPENSHELL_PEER_ALLOW_INSECURE_TRANSPORT.
  • Fail fast on locally refused relays (feat(server)). One arm in the relay retry loop, PeerTransportPolicy::preflight, and in-process two-replica TLS tests (PeerRelay, endpoint-status and provider-readiness forwarding over verified TLS with client certificates; replica and requester mismatch; missing identity).
  • Receiver identity tests (test(server)). validate_live_peer_pod extracted as a pure function with unchanged order, codes and messages; tests for audience, pod binding, pod UID, pod ServiceAccount, release labels and replica header mismatches.
  • Fail-closed chart (feat(helm)!). New _peer-transport.tpl guard: PostgreSQL (externalDbSecret or a postgres:///postgresql:// dbUrl) plus disableTls without server.peer.allowInsecureTransport fails to render, including --reuse-values upgrades; non-boolean values fail. OPENSHELL_PEER_TLS_CA_FILE is always wired when TLS is on. The opt-out env renders only on plaintext gateways; ci/values-skaffold.yaml sets it for dev and CI. New ci/values-high-availability-tls.yaml. New tests/peer_transport_test.yaml (23 tests) also asserts the whole peer env block and the peer client TLS mount.
  • HA operation inventory e2e (test(e2e)). New kubernetes_ha_operations e2e binary. One long-lived sandbox goes through setup, baseline, scale-up, scale-down, graceful owner loss, forced owner loss (--grace-period=0 --force on the identified owner) and kubectl rollout restart. After each step a recovery gate waits for Ready, then operations run through a replica that does not own the session: continuity check, exec (including more than 1 MiB of piped stdin, which switches to ExecSandboxInteractive), upload/download with checksums, forward start, forward service, policy update --wait, sandbox provider attach/detach --wait, and a fresh create/delete. The owner is read from its PostgreSQL record before and after each block; if it moved, the block runs once more, so a pass proves the operations crossed a peer. Retries are bounded and transient-only, non-idempotent operations reconcile before resending, and the test prints one summary line per phase.
  • TLS HA lane (ci(e2e)). Harness OPENSHELL_E2E_KUBE_POD_TLS mode (Service port-forward, verified mTLS handshake with the chart CA and client certificate, mTLS CLI registration; rejects Envoy and the DB scenarios). New mise run e2e:kubernetes:ha-tls and label-gated kubernetes-ha-tls-e2e CI job in the HA result gate.
  • Docs (docs): the peer transport contract in the gateway configuration reference; "Secure Peer Transport" (chart PKI, custom Secrets, ingress termination, the opt-out and its risk, upgrade guidance) and "Supported Operations" (routing class, non-owner availability, behavior after interruptions, HA test coverage) in the HA guide; architecture, Helm README and values docs, ingress and setup notes; public debug skill troubleshooting rows with the exact error strings; helm-dev skill TLS section.

Operational limits:

  • Sandbox logs and platform events are replica-local (logs reads only the serving replica's buffer). The HA guide documents this; making logs shared is out of scope.
  • After a sandbox's main process exits, only the owner accepts new SSH, file transfer and forwarding sessions.
  • sandbox connect does not retry FAILED_PRECONDITION "sandbox is not ready" during an owner move; the HA guide says to run it again.
  • Handshake failures (wrong CA or name) are retried and surface as supervisor session not connected after about 15 seconds, with the TLS cause in the gateway logs.
  • Rotated peer CA or client certificates reach new peer connections; cached HTTP/2 channels keep their session until an error evicts them.

Testing

Rebased on main at 1358941b.

  • mise run pre-commit: passed.
  • Server: 1,884 lib tests plus all integration targets passed, including 53 peer_transport tests (in-process TLS gateway with rcgen PKI: rogue CA, wrong server name, absent/empty/non-PEM CA, key mismatch, intermediate chains, SNI-selected external certificate, client identity, IPv6, opt-out on TLS and plaintext gateways, scheme refusals, two-replica relay and unary forwarding) and 8 new auth::peer tests. The C2 fail-fast tests ran 20 times in a row, including under 32 CPU burners on 16 cores.
  • Mutation checks: the fail-fast arm never firing, always firing, or keyed on status code each fail a test; removing the IPv6 or validity-window fixes fails their tests; removing the label check, the postgres:// chart branch, or the peer client TLS mount fails a test.
  • Helm: helm:lint (all variants), helm:test 226 gateway tests and 6 workspace tests, split ownership, helm:docs:check.
  • Docs: markdown:lint 0 errors, docs:build:strict 0 errors (3 warnings, same as main).
  • e2e static: fmt, feature clippy for kubernetes_ha_operations, 7 unit tests, shellcheck.
  • Kubernetes e2e on a local kind cluster (rootless Podman, kind v0.33.0, node v1.37.0, external PostgreSQL 17, images built from this branch):
    • mise run e2e:kubernetes:ha-tls (pod TLS + mTLS, HTTPS peers): all standalone conformance runs passed over mTLS; kubernetes_ha_operations passed in 125 seconds (8 tests: the lifecycle test plus 7 unit tests), every operation on its first attempt.
    • The same test in the plaintext Envoy lane (with the values-skaffold opt-out): passed in 149 seconds.
    • mise run e2e:kubernetes:ha-rebalancing (plaintext, unchanged tests from main): the final run passed conformance and both HA tests (2/2 in 236 seconds). Across the runs on this branch, sandbox_exec_rebalances_across_gateway_scale_and_rollout took 213-221 seconds against its 300-second limit and timed out once while a parallel workspace build loaded the host. sandbox_file_sync_survives_gateway_pod_rolls failed once with FAILED_PRECONDITION "sandbox is not ready": a CLI sync retry landed in the window where the sandbox reads Provisioning after its owner pod is replaced, and sync_error_is_retryable (crates/openshell-cli/src/ssh.rs:1709) does not retry that status. This PR does not touch the CLI or that path; it is a pre-existing race.
    • Owner moves observed in every run: graceful loss, forced loss and rollout each produced a new owner epoch. The forced delete took the release path (forced_path=released): the kubelet still sent SIGTERM, so the stale-owner-record branch (node failure) was not exercised end to end. The docs do not claim it.
  • Merge checks on throwaway trees (no refs touched): with feat(server): add HA capacity metrics, scoped locks, and graceful drain #3710, both merge orders give the same tree with no conflicts; the combined tree passes cargo check --workspace --all-targets, 2,003 server tests (including feat(server): add HA capacity metrics, scoped locks, and graceful drain #3710's fake-peer metrics tests), feat(server): add HA capacity metrics, scoped locks, and graceful drain #3710's PostgreSQL suite (17/17), 249 Helm tests and markdownlint. feat(server): place supervisor sessions by consistent hash #3661, feat(streams)!: define forwarding and relay lifecycle #3669 and test(conformance): cover relay reconnect recovery #3766 each compile with this branch and pass the peer transport, auth::peer and supervisor_session tests. fix(helm): scope OIDC CA trust to the OIDC client #3673, perf(kubernetes): use a TCP readiness probe for the supervisor #3700, perf(server): drop per-connection session tokens for TCP forwards #3734 and test(conformance): cover sandbox policy lifecycle #3777 merge cleanly.

Known environmental failures on this host, unchanged by this PR: openshell-binary-identity tests that read /proc/<pid>/exe.

The HA lanes need the test:e2e-kubernetes label and do not run in merge groups. Please add the label.

  • mise run pre-commit passes.
  • Unit tests added/updated.
  • E2E tests added/updated.

Checklist

  • Follows Conventional Commits (breaking changes marked with !).
  • Seven logical commits, each signed off for DCO. Review fixes were folded into the commit they belong to.
  • Architecture docs updated.
  • Published docs and related skills updated.

How this was built

Each phase ran as a separate multi-agent workflow on Opus 5.5, with the lead session reading every result, committing, and deciding. Nothing was posted on the issue.

  1. Research (8 agents). Six readers covered server transport, Helm topologies, the operation inventory, the e2e/conformance harness, the docs surface and 12 overlapping open PRs; a principal-engineer challenger and a completeness critic followed. The critic spot-checked 28 load-bearing claims (23 confirmed, 4 corrected, 1 unverifiable). Research found the dialer-side plaintext path, the SSL_CERT_FILE interaction, that "multi-replica" in code means PostgreSQL, that logs are replica-local, and that feat(helm): migrate gateway configuration to gatewayConfig #3384 silently drops the peer env block.
  2. Plan (12 agents). Four spec writers produced ready-to-apply patches, an architect and a principal engineer reviewed them, a contracts owner froze names, strings and tests with a disposition for every finding, the specs were revised, and a master planner ran a consistency pass. Review changed the design in six places: the TLS HA lane topology, the loopback exception becoming test-only, error chains for handshake failures, the fail-fast guard keyed on local policy, the startup self-check of the server certificate, and more realistic e2e retries and owner-stability proof.
  3. Implementation (one workflow per commit, 101 agents in total). Security, concurrency and chart commits had three review lenses with two skeptics per finding; the others had a code reviewer and an architect with one skeptic each. Surviving findings were fixed in the same commit. The first e2e draft was about 3,900 lines, so a dedicated simplification pass cut it to about 2,000 lines, and its review caught and fixed four regressions the simplification introduced.
  4. Final gate (47 agents). Eight reviewers covered the full diff: server security, server concurrency, Helm, e2e/CI, docs accuracy, architecture with merge-tree against open PRs, general code review, and an AC1-AC7 completeness critic. 16 of 19 findings survived two skeptics (none critical or high). They were folded into their owning commits with --fixup and autosquash.

In total about 170 agents and 19M subagent tokens. Every verification number above comes from commands run on this branch.

Merge coordination

Follow-ups worth tracking: shared sandbox logs; sandbox connect and CLI upload/download retrying "sandbox is not ready" during an owner move; deterministic forced owner loss (crictl stop) in e2e; service URL and sandbox connect HA e2e; the plaintext HA lane also running the operation test (after #3714); moving PeerTlsClientConfig into peer_transport.rs and folding validate_peer_endpoint_scheme into it (after #3710); owner lookup through #3710's session gauge; feature-gated e2e clippy in CI.

🤖 Generated with Claude Code

Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
…peer CA

Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Signed-off-by: Emilien Macchi <emacchi@redhat.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(ha): harden gateway peer transport and expand failover conformance

1 participant