From 6ba38c9df8274f557600d55120ba7f79a6a81123 Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Tue, 28 Jul 2026 10:49:47 -0400 Subject: [PATCH 1/9] fix(core): trust public root CAs alongside the sandbox mTLS CA The supervisor gRPC client only trusted the CA configured via OPENSHELL_TLS_CA, since tonic ClientTlsConfig starts with an empty root store unless with_native_roots()/with_webpki_roots() is also enabled. Deployments where the gateway server certificate is issued by a public CA (e.g. cert-manager against an ACME issuer) caused every supervisor connection to fail the TLS handshake with "UnknownCA", since the sandbox mTLS CA and the server cert issuer were no longer the same. Enable both native and webpki roots in addition to the configured CA. tonic root store is a union of all configured sources, so this does not weaken verification for existing self-signed deployments. webpki-roots (compiled in) is enabled alongside native-roots since the supervisor binary may run in minimal sandbox images without a populated system CA bundle. Signed-off-by: Jesse Jaggars --- Cargo.lock | 1 + crates/openshell-core/Cargo.toml | 2 +- crates/openshell-core/src/grpc_client.rs | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 9f3f7dcdca..03aa8386e1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6942,6 +6942,7 @@ dependencies = [ "tower-layer", "tower-service", "tracing", + "webpki-roots 1.0.7", ] [[package]] diff --git a/crates/openshell-core/Cargo.toml b/crates/openshell-core/Cargo.toml index e138e1eee1..c9eaeca388 100644 --- a/crates/openshell-core/Cargo.toml +++ b/crates/openshell-core/Cargo.toml @@ -14,7 +14,7 @@ repository.workspace = true glob = { workspace = true } prost = { workspace = true } prost-types = { workspace = true } -tonic = { workspace = true, features = ["channel", "tls-native-roots"] } +tonic = { workspace = true, features = ["channel", "tls-native-roots", "tls-webpki-roots"] } tonic-prost = { workspace = true } tokio = { workspace = true } thiserror = { workspace = true } diff --git a/crates/openshell-core/src/grpc_client.rs b/crates/openshell-core/src/grpc_client.rs index 579ee4a5b3..5f70a2771e 100644 --- a/crates/openshell-core/src/grpc_client.rs +++ b/crates/openshell-core/src/grpc_client.rs @@ -167,8 +167,16 @@ async fn build_plain_channel(endpoint: &str) -> Result { .into_diagnostic() .wrap_err_with(|| format!("failed to read client key from {key_path}"))?; + // Trust the configured CA (self-signed deployments sign the gateway's + // cert with it) *in addition to* the public root stores, rather than + // instead of them — tonic's root store is a union of all configured + // sources, so this also covers deployments where the gateway's server + // cert comes from a public CA (e.g. an ACME issuer) instead of the + // same private CA that signs this client's identity. let mut tls_config = ClientTlsConfig::new() .ca_certificate(Certificate::from_pem(ca_pem)) + .with_native_roots() + .with_webpki_roots() .identity(Identity::from_pem(cert_pem, key_pem)); if let Ok(server_name) = std::env::var(sandbox_env::GATEWAY_TLS_SERVER_NAME) && !server_name.is_empty() From 2714a3ea0eae8cf06eab6231a9131af91f8e9589 Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Tue, 28 Jul 2026 10:50:00 -0400 Subject: [PATCH 2/9] feat(helm): support external cert-manager issuers and OpenShift Route passthrough Add certManager.serverIssuerRef/clientIssuerRef so the gateway and mTLS client certificates can be issued by a real Issuer/ClusterIssuer (e.g. ACME) instead of only the chart built-in self-signed CA. Add openshiftRoute template for exposing the gateway via a TLS passthrough Route so the gateway keeps terminating its own TLS/mTLS. The server Certificate excludes internal-only SANs (cluster-local, localhost, loopback) when an external issuer is configured, since ACME issuers reject those per CA/Browser Forum baseline requirements. A template-time fail guard catches the misconfiguration at helm install time rather than asynchronously at cert-manager issuance time. Includes Helm unittest coverage for both issuerRef overrides and Route rendering, plus a CI values overlay for lint coverage. Signed-off-by: Jesse Jaggars --- .../values-openshift-route-cert-manager.yaml | 33 ++++ .../openshell/templates/cert-manager-pki.yaml | 47 +++++- deploy/helm/openshell/templates/route.yaml | 28 ++++ .../tests/cert_manager_pki_test.yaml | 150 ++++++++++++++++++ deploy/helm/openshell/tests/route_test.yaml | 62 ++++++++ deploy/helm/openshell/values.yaml | 36 ++++- 6 files changed, 352 insertions(+), 4 deletions(-) create mode 100644 deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml create mode 100644 deploy/helm/openshell/templates/route.yaml create mode 100644 deploy/helm/openshell/tests/cert_manager_pki_test.yaml create mode 100644 deploy/helm/openshell/tests/route_test.yaml diff --git a/deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml b/deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml new file mode 100644 index 0000000000..d06283ee15 --- /dev/null +++ b/deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml @@ -0,0 +1,33 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# Render-coverage overlay for cert-manager issuing the server certificate from +# an external Issuer/ClusterIssuer (e.g. a real ACME issuer), plus an +# OpenShift Route with TLS passthrough. Merge after values.yaml: +# helm lint deploy/helm/openshell -f ci/values-openshift-route-cert-manager.yaml +# +# The ClusterIssuer name below is a placeholder for render coverage; a real +# deployment must reference an Issuer/ClusterIssuer that's actually installed +# and Ready in the target cluster. See docs/kubernetes/managing-certificates.mdx. + +server: + disableTls: false + grpcEndpoint: "https://openshell.example.com:443" + tls: + # Must name a secret containing the actual client CA when + # clientCaFromServerTlsSecret is false — here, the same CA secret + # cert-manager creates for signing the client (mTLS) certificate. + clientCaSecretName: openshell-ca-tls + +certManager: + enabled: true + clientCaFromServerTlsSecret: false + serverIssuerRef: + name: letsencrypt-prod + kind: ClusterIssuer + serverDnsNames: + - openshell.example.com + +openshiftRoute: + enabled: true + host: openshell.example.com diff --git a/deploy/helm/openshell/templates/cert-manager-pki.yaml b/deploy/helm/openshell/templates/cert-manager-pki.yaml index fdd702a305..c43268096d 100644 --- a/deploy/helm/openshell/templates/cert-manager-pki.yaml +++ b/deploy/helm/openshell/templates/cert-manager-pki.yaml @@ -42,6 +42,15 @@ spec: ca: secretName: {{ .Values.certManager.caSecretName | quote }} --- +{{- $externalServerIssuer := .Values.certManager.serverIssuerRef.name }} +{{- if $externalServerIssuer }} +{{- range .Values.certManager.serverDnsNames }} +{{- /* Single-label names (e.g. "openshell") are also rejected by ACME CAs but are intentionally not checked here — the guard targets recognisable internal-network patterns. */ -}} +{{- if or (eq . "localhost") (hasSuffix ".localhost" .) (hasSuffix ".svc.cluster.local" .) (hasSuffix ".svc" .) (eq . "host.docker.internal") (eq . "host.containers.internal") }} +{{- fail (printf "certManager.serverIssuerRef.name is set (external issuer) but certManager.serverDnsNames contains %q — external CAs (e.g. ACME / Let's Encrypt) reject internal-only names per CA/Browser Forum baseline requirements. Override certManager.serverDnsNames with your externally-resolvable hostname(s)." .) }} +{{- end }} +{{- end }} +{{- end }} apiVersion: cert-manager.io/v1 kind: Certificate metadata: @@ -53,7 +62,30 @@ spec: secretName: {{ .Values.server.tls.certSecretName | quote }} duration: {{ .Values.certManager.certificateDuration | quote }} renewBefore: {{ .Values.certManager.certificateRenewBefore | quote }} - commonName: openshell-server + {{- if $externalServerIssuer }} + # External issuers (e.g. ACME) reject internal-only names (cluster-local, + # localhost, loopback IPs) per CA/Browser Forum baseline requirements, and + # require any commonName to also appear in dnsNames. Only request the + # externally-resolvable SANs the operator configured — no defaults, no IPs. + {{- if .Values.certManager.serverDnsNames }} + commonName: {{ first .Values.certManager.serverDnsNames | quote }} + {{- end }} + dnsNames: + {{- range .Values.certManager.serverDnsNames }} + - {{ . | quote }} + {{- end }} + {{- else }} + # The chart fullname is always the first entry of defaultServerDnsNames + # below, so it's always valid as a commonName. + # + # Upgrade note: this replaces the previously-hardcoded "openshell-server" + # commonName. The ACME commonName-must-be-a-SAN constraint this exists for + # only applies to the external-issuer branch above, but this branch was + # changed too for consistency. Existing self-signed/internal-CA deployments + # will get a new server cert with a different Subject CN on next reissue + # after upgrading — SAN-based hostname verification (what TLS clients + # actually check) is unaffected. + commonName: {{ include "openshell.fullname" . }} dnsNames: {{- range (include "openshell.defaultServerDnsNames" . | fromYamlArray) }} - {{ . | quote }} @@ -65,6 +97,7 @@ spec: ipAddresses: {{- toYaml .Values.certManager.serverIpAddresses | nindent 4 }} {{- end }} + {{- end }} privateKey: algorithm: ECDSA size: 256 @@ -73,9 +106,15 @@ spec: - digital signature - key encipherment issuerRef: + {{- if .Values.certManager.serverIssuerRef.name }} + name: {{ .Values.certManager.serverIssuerRef.name }} + kind: {{ .Values.certManager.serverIssuerRef.kind | default "ClusterIssuer" }} + group: {{ .Values.certManager.serverIssuerRef.group | default "cert-manager.io" }} + {{- else }} name: {{ include "openshell.fullname" . }}-ca-issuer kind: Issuer group: cert-manager.io + {{- end }} --- apiVersion: cert-manager.io/v1 kind: Certificate @@ -97,7 +136,13 @@ spec: - digital signature - key encipherment issuerRef: + {{- if .Values.certManager.clientIssuerRef.name }} + name: {{ .Values.certManager.clientIssuerRef.name }} + kind: {{ .Values.certManager.clientIssuerRef.kind | default "ClusterIssuer" }} + group: {{ .Values.certManager.clientIssuerRef.group | default "cert-manager.io" }} + {{- else }} name: {{ include "openshell.fullname" . }}-ca-issuer kind: Issuer group: cert-manager.io + {{- end }} {{- end }} diff --git a/deploy/helm/openshell/templates/route.yaml b/deploy/helm/openshell/templates/route.yaml new file mode 100644 index 0000000000..26262ee7c6 --- /dev/null +++ b/deploy/helm/openshell/templates/route.yaml @@ -0,0 +1,28 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +{{- if .Values.openshiftRoute.enabled }} +apiVersion: route.openshift.io/v1 +kind: Route +metadata: + name: {{ include "openshell.fullname" . }} + namespace: {{ .Release.Namespace }} + labels: + {{- include "openshell.labels" . | nindent 4 }} + {{- with .Values.openshiftRoute.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} +spec: + {{- if .Values.openshiftRoute.host }} + host: {{ .Values.openshiftRoute.host }} + {{- end }} + to: + kind: Service + name: {{ include "openshell.fullname" . }} + port: + targetPort: grpc + tls: + termination: passthrough + wildcardPolicy: None +{{- end }} diff --git a/deploy/helm/openshell/tests/cert_manager_pki_test.yaml b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml new file mode 100644 index 0000000000..f64458d1b7 --- /dev/null +++ b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml @@ -0,0 +1,150 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +suite: cert-manager PKI issuerRef overrides +templates: + - templates/cert-manager-pki.yaml +release: + name: openshell + namespace: my-namespace + +tests: + - it: defaults both server and client Certificates to the chart's own CA issuer + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + asserts: + - equal: + path: spec.issuerRef.name + value: openshell-ca-issuer + documentIndex: 3 + - equal: + path: spec.issuerRef.kind + value: Issuer + documentIndex: 3 + - equal: + path: spec.issuerRef.name + value: openshell-ca-issuer + documentIndex: 4 + - equal: + path: spec.issuerRef.kind + value: Issuer + documentIndex: 4 + + - it: default server Certificate includes internal SANs, IPs, and a fixed commonName + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + asserts: + - equal: + path: spec.commonName + value: openshell + documentIndex: 3 + - contains: + path: spec.dnsNames + content: openshell.my-namespace.svc.cluster.local + documentIndex: 3 + - contains: + path: spec.dnsNames + content: localhost + documentIndex: 3 + - equal: + path: spec.ipAddresses[0] + value: 127.0.0.1 + documentIndex: 3 + + - it: overrides the server Certificate issuerRef when serverIssuerRef is set + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + certManager.serverIssuerRef.name: letsencrypt-prod + certManager.serverIssuerRef.kind: ClusterIssuer + certManager.serverDnsNames: + - openshell.example.com + asserts: + - equal: + path: spec.issuerRef.name + value: letsencrypt-prod + documentIndex: 3 + - equal: + path: spec.issuerRef.kind + value: ClusterIssuer + documentIndex: 3 + - equal: + path: spec.issuerRef.group + value: cert-manager.io + documentIndex: 3 + + - it: scopes server Certificate SANs to only the configured external hostnames when serverIssuerRef is set + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + certManager.serverIssuerRef.name: letsencrypt-prod + certManager.serverIssuerRef.kind: ClusterIssuer + certManager.serverDnsNames: + - openshell.example.com + asserts: + - equal: + path: spec.commonName + value: openshell.example.com + documentIndex: 3 + - equal: + path: spec.dnsNames + value: + - openshell.example.com + documentIndex: 3 + - notExists: + path: spec.ipAddresses + documentIndex: 3 + - notContains: + path: spec.dnsNames + content: localhost + documentIndex: 3 + - notContains: + path: spec.dnsNames + content: openshell.my-namespace.svc.cluster.local + documentIndex: 3 + + - it: overrides the client Certificate issuerRef when clientIssuerRef is set + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + certManager.clientIssuerRef.name: internal-pki-issuer + certManager.clientIssuerRef.kind: ClusterIssuer + asserts: + - equal: + path: spec.issuerRef.name + value: internal-pki-issuer + documentIndex: 4 + - equal: + path: spec.issuerRef.kind + value: ClusterIssuer + documentIndex: 4 + + - it: fails when serverIssuerRef is set but serverDnsNames contains internal-only names + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + certManager.serverIssuerRef.name: letsencrypt-prod + # serverDnsNames is left at the default which contains "openshell.openshell.svc", "localhost", etc. + asserts: + - failedTemplate: + errorMessage: "certManager.serverIssuerRef.name is set (external issuer) but certManager.serverDnsNames contains \"openshell.openshell.svc\" \u2014 external CAs (e.g. ACME / Let's Encrypt) reject internal-only names per CA/Browser Forum baseline requirements. Override certManager.serverDnsNames with your externally-resolvable hostname(s)." + + - it: client Certificate issuerRef is unaffected by serverIssuerRef + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + certManager.serverIssuerRef.name: letsencrypt-prod + certManager.serverIssuerRef.kind: ClusterIssuer + certManager.serverDnsNames: + - openshell.example.com + asserts: + - equal: + path: spec.issuerRef.name + value: openshell-ca-issuer + documentIndex: 4 + - equal: + path: spec.issuerRef.kind + value: Issuer + documentIndex: 4 diff --git a/deploy/helm/openshell/tests/route_test.yaml b/deploy/helm/openshell/tests/route_test.yaml new file mode 100644 index 0000000000..b218bf3264 --- /dev/null +++ b/deploy/helm/openshell/tests/route_test.yaml @@ -0,0 +1,62 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +suite: OpenShift Route +templates: + - templates/route.yaml +release: + name: openshell + namespace: my-namespace + +tests: + - it: renders nothing by default + asserts: + - hasDocuments: + count: 0 + + - it: renders a passthrough Route when enabled + set: + openshiftRoute.enabled: true + openshiftRoute.host: openshell.apps.example.com + asserts: + - isKind: + of: Route + - equal: + path: apiVersion + value: route.openshift.io/v1 + - equal: + path: spec.host + value: openshell.apps.example.com + - equal: + path: spec.to.kind + value: Service + - equal: + path: spec.to.name + value: openshell + - equal: + path: spec.port.targetPort + value: grpc + - equal: + path: spec.tls.termination + value: passthrough + - equal: + path: spec.wildcardPolicy + value: None + + - it: omits host when not set + set: + openshiftRoute.enabled: true + asserts: + - notExists: + path: spec.host + + - it: renders custom annotations + set: + openshiftRoute.enabled: true + openshiftRoute.host: openshell.apps.example.com + openshiftRoute.annotations: + haproxy.router.openshift.io/balance: roundrobin + asserts: + - equal: + path: metadata.annotations["haproxy.router.openshift.io/balance"] + value: roundrobin diff --git a/deploy/helm/openshell/values.yaml b/deploy/helm/openshell/values.yaml index 0525ed475d..859c2f8a28 100644 --- a/deploy/helm/openshell/values.yaml +++ b/deploy/helm/openshell/values.yaml @@ -345,7 +345,7 @@ pkiInitJob: serverIpAddresses: [] # cert-manager Certificate/Issuer resources (requires cert-manager CRDs in-cluster). -# Uses namespaced Issuers only (no ClusterIssuer). Does not install cert-manager itself. +# Does not install cert-manager itself. certManager: # -- Create cert-manager Issuer and Certificate resources. When enabled, # cert-manager owns TLS and the chart runs a JWT-only certgen hook to create @@ -353,9 +353,27 @@ certManager: enabled: false # -- Secret created for the intermediate CA (Certificate with isCA: true). caSecretName: openshell-ca-tls + # -- Override the issuerRef for the server Certificate (e.g. a real ACME + # ClusterIssuer for a publicly-trusted cert on an external hostname). Leave + # name empty to use the chart's own self-signed CA issuer (default). + serverIssuerRef: + name: "" + kind: "" + group: "" + # -- Override the issuerRef for the client (mTLS) Certificate. Client certs + # don't need public trust; leave empty to use the chart's own CA issuer + # unless an internal PKI issuer is preferred. + clientIssuerRef: + name: "" + kind: "" + group: "" # -- Mount gateway client CA from the server TLS secret's ca.crt (populated by - # cert-manager for certs issued by a CA Issuer). Avoids a separate - # openshell-server-client-ca Secret. + # cert-manager for certs issued by a CA Issuer). Set to false when + # serverIssuerRef points at an external issuer (its secret's ca.crt would be + # that issuer's chain, not the CA that signs the client cert). When false, + # also set server.tls.clientCaSecretName to a secret containing the actual + # client CA — for example caSecretName's value, since that's the CA the + # client certificate above is issued from by default. clientCaFromServerTlsSecret: true # -- Duration for cert-manager-issued certificates. certificateDuration: 8760h @@ -415,3 +433,15 @@ grpcRoute: # or the existing openshell-server-tls Secret (its SANs must include the # external hostname). certificateRefs: [] + +# OpenShift Route with TLS passthrough. The gateway terminates its own +# TLS/mTLS; the router only forwards based on SNI, so it never sees plaintext +# or the client certificate. Requires server.disableTls=false and a server +# cert whose SANs include the Route host (see certManager.serverIssuerRef). +openshiftRoute: + # -- Create an OpenShift Route with TLS passthrough. + enabled: false + # -- Hostname for the Route. Must match a SAN on the gateway's server cert. + host: "" + # -- Extra annotations on the Route (e.g. haproxy.router.openshift.io/*). + annotations: {} From fe79a10af2ea149cf4423beb95422657a07b892e Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Tue, 28 Jul 2026 10:50:09 -0400 Subject: [PATCH 3/9] docs: document cert-manager external issuer and OpenShift Route Update managing-certificates.mdx with the serverIssuerRef workflow and install-time validation behavior. Add a production section to the OpenShift guide covering passthrough Route with a real certificate. Regenerate Helm README for new certManager and openshiftRoute values. Sync debug-openshell-cluster skill with new troubleshooting steps for ACME issuance failures and supervisor UnknownCA from mismatched CAs. Signed-off-by: Jesse Jaggars --- .../skills/debug-openshell-cluster/SKILL.md | 33 ++++++++++++ deploy/helm/openshell/README.md | 7 ++- docs/kubernetes/managing-certificates.mdx | 54 ++++++++++++++++++- docs/kubernetes/openshift.mdx | 51 +++++++++++++++++- 4 files changed, 141 insertions(+), 4 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index cbff462d45..d4e39ca7b9 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -268,6 +268,37 @@ If the gateway exits with `failed to read sandbox JWT signing key from `sandbox-jwt` secret at `/etc/openshell-jwt`. The sandbox JWT mount is required even when local Helm values disable TLS. +If `certManager.serverIssuerRef` points the server certificate at an external +Issuer or ClusterIssuer (for example an ACME issuer, for a publicly-trusted +cert on an OpenShift `Route` with TLS passthrough — see +`openshiftRoute.enabled`), check the `Certificate`/`CertificateRequest`/ +`Challenge` resources directly when the secret never becomes Ready: + +```bash +kubectl -n openshell get certificate,certificaterequest,challenge +kubectl -n openshell describe certificate openshell-server +oc -n openshell get route +``` + +ACME issuers reject certificate requests that include internal-only names +(`*.svc.cluster.local`, `localhost`, loopback IPs) and require the +`commonName` to also be a SAN — the server `Certificate` only requests the +hostnames in `certManager.serverDnsNames` when `serverIssuerRef` is set, for +exactly this reason. + +If sandbox supervisors fail their TLS handshake to the gateway with +`UnknownCA` after configuring `serverIssuerRef`, the client (mTLS) certificate +and the server certificate now come from different CAs, and the gateway's +client-verification CA is misconfigured. Set +`certManager.clientCaFromServerTlsSecret=false` and +`server.tls.clientCaSecretName` to a secret that actually contains the CA that +signs the client certificate (`certManager.caSecretName`'s value by default): + +```bash +helm -n openshell get values openshell | grep -E 'clientCaFromServerTlsSecret|clientCaSecretName|serverIssuerRef|caSecretName' +kubectl -n openshell get secret openshell-ca-tls -o jsonpath='{.data.ca\.crt}' | base64 -d | openssl x509 -noout -subject +``` + If `server.providerTokenGrants.spiffe.enabled=true`, the gateway should still render `[openshell.gateway.gateway_jwt]` and mount the `sandbox-jwt` Secret. SPIRE is used only by sandbox pods for dynamic provider token grants. Verify @@ -452,6 +483,8 @@ openshell logs | `K8s namespace not ready` with `envoy-gateway-openshell.yaml: the server could not find the requested resource` | Optional Gateway API manifest was applied without Envoy Gateway CRDs, or k3s Helm controller startup exceeded the namespace wait | Apply `deploy/kube/manifests/envoy-gateway-openshell.yaml` manually only after Envoy Gateway is installed and `grpcRoute` is enabled | | HTTPS ingress (`grpcRoute.gateway.listener.protocol=HTTPS`) connection resets or TLS handshake hangs | Envoy terminates TLS but the gateway pod still expects TLS, so the plaintext backend hop fails | Set `server.disableTls=true` so Envoy forwards plaintext to the pod; verify the listener `certificateRefs` Secret exists in the release namespace and `openshell status` over `https://` | | HTTPS ingress returns `Unauthenticated` after connecting | TLS terminates at Envoy, so the gateway never sees a client cert; no OIDC issuer is configured for identity | Configure `server.oidc.issuer` and register with `openshell gateway add https:// --oidc-issuer `, or set `server.auth.allowUnauthenticatedUsers=true` for a trusted-proxy/dev cluster | +| Server `Certificate` never becomes Ready with `certManager.serverIssuerRef` set | ACME issuer rejected internal-only SANs, a loopback IP, or a `commonName` absent from the SANs | `kubectl -n openshell describe certificate openshell-server`; confirm `certManager.serverDnsNames` lists only real, externally-resolvable hostnames | +| Sandbox supervisors fail TLS handshake with `UnknownCA` after configuring `certManager.serverIssuerRef` | Client (mTLS) cert and server cert now come from different CAs, and `server.tls.clientCaSecretName` still points at the wrong (or default, unpopulated) secret | Set `certManager.clientCaFromServerTlsSecret=false` and `server.tls.clientCaSecretName` to the secret named by `certManager.caSecretName` | ## Reporting diff --git a/deploy/helm/openshell/README.md b/deploy/helm/openshell/README.md index d4310cb9a7..62fac5a428 100644 --- a/deploy/helm/openshell/README.md +++ b/deploy/helm/openshell/README.md @@ -144,10 +144,12 @@ add `ci/values-spire.yaml` to the OpenShell release values files. | certManager.caSecretName | string | `"openshell-ca-tls"` | Secret created for the intermediate CA (Certificate with isCA: true). | | certManager.certificateDuration | string | `"8760h"` | Duration for cert-manager-issued certificates. | | certManager.certificateRenewBefore | string | `"720h"` | Renewal window for cert-manager-issued certificates. | -| certManager.clientCaFromServerTlsSecret | bool | `true` | Mount gateway client CA from the server TLS secret's ca.crt (populated by cert-manager for certs issued by a CA Issuer). Avoids a separate openshell-server-client-ca Secret. | +| certManager.clientCaFromServerTlsSecret | bool | `true` | Mount gateway client CA from the server TLS secret's ca.crt (populated by cert-manager for certs issued by a CA Issuer). Set to false when serverIssuerRef points at an external issuer (its secret's ca.crt would be that issuer's chain, not the CA that signs the client cert). When false, also set server.tls.clientCaSecretName to a secret containing the actual client CA — for example caSecretName's value, since that's the CA the client certificate above is issued from by default. | +| certManager.clientIssuerRef | object | `{"group":"","kind":"","name":""}` | Override the issuerRef for the client (mTLS) Certificate. Client certs don't need public trust; leave empty to use the chart's own CA issuer unless an internal PKI issuer is preferred. | | certManager.enabled | bool | `false` | Create cert-manager Issuer and Certificate resources. When enabled, cert-manager owns TLS and the chart runs a JWT-only certgen hook to create the sandbox JWT signing Secret that cert-manager does not manage. | | certManager.serverDnsNames | list | `["openshell","openshell.openshell.svc","openshell.openshell.svc.cluster.local","localhost","openshell.localhost","*.openshell.localhost","host.docker.internal"]` | DNS SANs on the cert-manager-issued server certificate. | | certManager.serverIpAddresses | list | `["127.0.0.1"]` | IP SANs on the cert-manager-issued server certificate. | +| certManager.serverIssuerRef | object | `{"group":"","kind":"","name":""}` | Override the issuerRef for the server Certificate (e.g. a real ACME ClusterIssuer for a publicly-trusted cert on an external hostname). Leave name empty to use the chart's own self-signed CA issuer (default). | | fullnameOverride | string | `""` | Override the full generated resource name. | | grpcRoute.enabled | bool | `false` | Create a Gateway API GRPCRoute for the gateway service. | | grpcRoute.gateway.className | string | `"eg"` | GatewayClass to reference. Envoy Gateway installs one named "eg". | @@ -166,6 +168,9 @@ add `ci/values-spire.yaml` to the OpenShell release values files. | nameOverride | string | `"openshell"` | Override the chart name used in generated resource names. | | networkPolicy.enabled | bool | `true` | Create a NetworkPolicy restricting SSH ingress on sandbox pods to the gateway. | | nodeSelector | object | `{}` | Node selector for the gateway pod. | +| openshiftRoute.annotations | object | `{}` | Extra annotations on the Route (e.g. haproxy.router.openshift.io/*). | +| openshiftRoute.enabled | bool | `false` | Create an OpenShift Route with TLS passthrough. | +| openshiftRoute.host | string | `""` | Hostname for the Route. Must match a SAN on the gateway's server cert. | | pkiInitJob.enabled | bool | `true` | Run a pre-install/pre-upgrade Job that creates gateway and client mTLS Secrets. When certManager.enabled=true, cert-manager owns TLS and this same hook runs in JWT-only mode even if pkiInitJob.enabled remains true. | | pkiInitJob.serverDnsNames | list | `[]` | Extra DNS SANs to append to the server certificate. | | pkiInitJob.serverIpAddresses | list | `[]` | Extra IP SANs to append to the server certificate. | diff --git a/docs/kubernetes/managing-certificates.mdx b/docs/kubernetes/managing-certificates.mdx index b66419b505..1fcf8a3a59 100644 --- a/docs/kubernetes/managing-certificates.mdx +++ b/docs/kubernetes/managing-certificates.mdx @@ -60,6 +60,58 @@ The chart also runs a pre-install hook in JWT-only mode to create the gateway's sandbox JWT signing Secret. That Secret is separate from the cert-manager TLS certificate Secrets and is mounted at `/etc/openshell-jwt`. +## Using a real Issuer for the server certificate + +By default, cert-manager issues both the server and client certificates from +a self-signed CA the chart creates — this rotates automatically, but the +server certificate is still not publicly trusted. `certManager.serverIssuerRef` +overrides the `issuerRef` on the server `Certificate` resource to point at a +real `Issuer` or `ClusterIssuer` instead, for example an ACME issuer: + +```shell +helm upgrade --install openshell \ + oci://ghcr.io/nvidia/openshell/helm-chart \ + --version \ + --namespace openshell \ + --set certManager.enabled=true \ + --set certManager.clientCaFromServerTlsSecret=false \ + --set server.tls.clientCaSecretName=openshell-ca-tls \ + --set certManager.serverIssuerRef.name=letsencrypt-prod \ + --set certManager.serverIssuerRef.kind=ClusterIssuer \ + --set certManager.serverDnsNames[0]=openshell.example.com +``` + + +Public CAs such as Let's Encrypt reject certificate requests that include +internal-only names (`*.svc.cluster.local`, `localhost`, loopback IPs) per +CA/Browser Forum baseline requirements. The chart validates this at install +time and fails with an actionable error if `certManager.serverDnsNames` +contains internal-only entries while `serverIssuerRef` is set. When +`serverIssuerRef` is set, the server certificate's SANs are limited to the +hostnames in `certManager.serverDnsNames` — the chart's usual internal +cluster-local SANs are omitted. Sandboxes calling back into the gateway must then use the same +external hostname; see `server.grpcEndpoint` in +[Gateway Configuration](/reference/gateway-config). + + +Set `certManager.clientCaFromServerTlsSecret=false` whenever `serverIssuerRef` +is set, and set `server.tls.clientCaSecretName` to name a secret that actually +contains the client CA — by default that's the same secret named by +`certManager.caSecretName` (`openshell-ca-tls`), since that's the CA the chart +issues the client certificate from unless you've also overridden +`clientIssuerRef`. The client (mTLS) certificate used by sandbox supervisors +stays on the chart's own internal CA — it doesn't need public trust, and +getting a public CA to sign it isn't practical: ACME only validates domain +identifiers, not arbitrary workload identity, and issuing one certificate per +sandbox would put a public CA's issuance rate limits directly in the +sandbox-creation path. + +`certManager.clientIssuerRef` is also available if you'd rather sign the +client certificate from an internal PKI issuer of your own instead of the +chart's self-signed CA — most deployments can leave it unset. + ## Next Steps -Return to [Setup](/kubernetes/setup) to complete the installation. +Return to [Setup](/kubernetes/setup) to complete the installation. For +exposing the gateway externally on OpenShift with a real certificate, see +[OpenShift](/kubernetes/openshift). diff --git a/docs/kubernetes/openshift.mdx b/docs/kubernetes/openshift.mdx index 7512eaa65e..d947ea3efc 100644 --- a/docs/kubernetes/openshift.mdx +++ b/docs/kubernetes/openshift.mdx @@ -87,8 +87,55 @@ openshell gateway add http://127.0.0.1:8080 --local --name openshift openshell status ``` +## Production: expose externally with a real certificate + +The steps above run the gateway over plaintext HTTP for quick evaluation. For +a real deployment, cert-manager can issue the gateway's server certificate +from a real Issuer or ClusterIssuer (for example, an ACME issuer), and an +OpenShift Route with TLS passthrough exposes it externally while the gateway +keeps terminating its own TLS and mTLS. + +Install cert-manager and configure a working `ClusterIssuer` first, then see +[Managing Certificates](/kubernetes/managing-certificates) for the +`certManager.serverIssuerRef` details. Install the chart with: + +```shell +helm install openshell oci://ghcr.io/nvidia/openshell/helm-chart \ + --version \ + --namespace openshell \ + --set podSecurityContext.fsGroup=null \ + --set securityContext.runAsUser=null \ + --set server.disableTls=false \ + --set server.grpcEndpoint=https://:443 \ + --set certManager.enabled=true \ + --set certManager.clientCaFromServerTlsSecret=false \ + --set server.tls.clientCaSecretName=openshell-ca-tls \ + --set certManager.serverIssuerRef.name= \ + --set certManager.serverIssuerRef.kind=ClusterIssuer \ + --set certManager.serverDnsNames[0]= \ + --set openshiftRoute.enabled=true \ + --set openshiftRoute.host= +``` + +| Override | Reason | +|---|---| +| `server.grpcEndpoint` | Sandboxes call back into the gateway using this hostname. A cert from an external issuer only carries the external SANs you configure, not the internal cluster-local ones, so sandboxes must use the same externally-valid hostname to pass TLS verification. | +| `certManager.clientCaFromServerTlsSecret=false` + `server.tls.clientCaSecretName` | The server cert's issuer no longer shares a CA with the client (mTLS) cert, so this points the gateway's client-verification CA at `certManager.caSecretName` (the chart's own CA secret, `openshell-ca-tls` by default) instead of the server secret. | +| `certManager.serverIssuerRef` | Points the server certificate at your real Issuer or ClusterIssuer instead of the chart's built-in self-signed CA. | +| `openshiftRoute.enabled` / `openshiftRoute.host` | Creates an OpenShift Route with TLS passthrough — the router forwards the encrypted connection by SNI without decrypting, so the gateway keeps terminating its own TLS and mTLS. | + +Register the gateway with the CLI over OIDC. Remote gateways authenticate CLI +users via OIDC, not mTLS — see [Access Control](/kubernetes/access-control): + +```shell +openshell gateway add https:// \ + --name openshift \ + --oidc-issuer +openshell gateway login openshift +``` + ## Next Steps -- For TLS-enabled deployments, refer to [Managing Certificates](/kubernetes/managing-certificates). -- To expose the gateway externally, refer to [Ingress](/kubernetes/ingress). +- For more on certificate provisioning modes, refer to [Managing Certificates](/kubernetes/managing-certificates). +- To expose the gateway externally through the Kubernetes Gateway API instead of a Route, refer to [Ingress](/kubernetes/ingress). - To configure OIDC authentication, refer to [Access Control](/kubernetes/access-control). From 3b88851f06b2cdeb1d42dbef8e1c1af8cbf32539 Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Mon, 3 Aug 2026 10:32:53 -0400 Subject: [PATCH 4/9] fix(helm,core): address PR review feedback on cert-manager external issuer Addresses all five blocking review items from #2468: 1. Remove .with_native_roots() from supervisor gRPC client -- the supervisor runs inside the user-selected sandbox image, so the image CA bundle is not operator-controlled. Keep .with_webpki_roots() (compiled-in, not user-controlled) alongside the configured CA. 2. Fail at render time when serverIssuerRef.name is set but clientCaFromServerTlsSecret is still true. Add negative Helm test. 3. Remove clientIssuerRef -- changing only clientIssuerRef breaks both directions because trust bundles are not modeled separately. Change serverIssuerRef.kind default from ClusterIssuer to Issuer. 4. Add server.oidc.issuer and server.oidc.audience to the documented OpenShift production Helm command. Add Access Control prerequisite. 5. Fail at render time when openshiftRoute.enabled and disableTls are both true. Add negative Helm test. Signed-off-by: Jesse Jaggars --- .../skills/debug-openshell-cluster/SKILL.md | 4 +++- crates/openshell-core/Cargo.toml | 2 +- crates/openshell-core/src/grpc_client.rs | 19 ++++++++++----- deploy/helm/openshell/README.md | 1 - .../openshell/templates/cert-manager-pki.yaml | 11 ++++----- deploy/helm/openshell/templates/route.yaml | 3 +++ .../tests/cert_manager_pki_test.yaml | 23 ++++++++++--------- deploy/helm/openshell/tests/route_test.yaml | 8 +++++++ deploy/helm/openshell/values.yaml | 7 ------ docs/kubernetes/managing-certificates.mdx | 4 ---- docs/kubernetes/openshift.mdx | 12 +++++++--- 11 files changed, 53 insertions(+), 41 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index d4e39ca7b9..caa2cfe3c4 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -289,7 +289,9 @@ exactly this reason. If sandbox supervisors fail their TLS handshake to the gateway with `UnknownCA` after configuring `serverIssuerRef`, the client (mTLS) certificate and the server certificate now come from different CAs, and the gateway's -client-verification CA is misconfigured. Set +client-verification CA is misconfigured. The chart now fails at render time if +`clientCaFromServerTlsSecret` is still true when `serverIssuerRef` is set, but +this can still occur with pre-existing releases or manual overrides. Set `certManager.clientCaFromServerTlsSecret=false` and `server.tls.clientCaSecretName` to a secret that actually contains the CA that signs the client certificate (`certManager.caSecretName`'s value by default): diff --git a/crates/openshell-core/Cargo.toml b/crates/openshell-core/Cargo.toml index c9eaeca388..658ebff3f5 100644 --- a/crates/openshell-core/Cargo.toml +++ b/crates/openshell-core/Cargo.toml @@ -14,7 +14,7 @@ repository.workspace = true glob = { workspace = true } prost = { workspace = true } prost-types = { workspace = true } -tonic = { workspace = true, features = ["channel", "tls-native-roots", "tls-webpki-roots"] } +tonic = { workspace = true, features = ["channel", "tls-webpki-roots"] } tonic-prost = { workspace = true } tokio = { workspace = true } thiserror = { workspace = true } diff --git a/crates/openshell-core/src/grpc_client.rs b/crates/openshell-core/src/grpc_client.rs index 5f70a2771e..c4272bdbe6 100644 --- a/crates/openshell-core/src/grpc_client.rs +++ b/crates/openshell-core/src/grpc_client.rs @@ -168,14 +168,21 @@ async fn build_plain_channel(endpoint: &str) -> Result { .wrap_err_with(|| format!("failed to read client key from {key_path}"))?; // Trust the configured CA (self-signed deployments sign the gateway's - // cert with it) *in addition to* the public root stores, rather than - // instead of them — tonic's root store is a union of all configured - // sources, so this also covers deployments where the gateway's server - // cert comes from a public CA (e.g. an ACME issuer) instead of the - // same private CA that signs this client's identity. + // cert with it) *in addition to* the compiled-in WebPKI roots — this + // covers deployments where the gateway's server cert comes from a + // public CA (e.g. an ACME issuer) instead of the same private CA that + // signs this client's identity. + // + // Do NOT add `.with_native_roots()` here: the supervisor runs inside + // the user-selected sandbox image (Docker/Podman drivers), so the + // image's CA bundle is not operator-controlled. An attacker who can + // influence the image and DNS/routing could install their own CA and + // intercept the supervisor→gateway TLS connection. Additionally, + // tonic returns NativeCertsNotFound when the native store is empty, + // which would break minimal BYOC images even though OPENSHELL_TLS_CA + // is valid. let mut tls_config = ClientTlsConfig::new() .ca_certificate(Certificate::from_pem(ca_pem)) - .with_native_roots() .with_webpki_roots() .identity(Identity::from_pem(cert_pem, key_pem)); if let Ok(server_name) = std::env::var(sandbox_env::GATEWAY_TLS_SERVER_NAME) diff --git a/deploy/helm/openshell/README.md b/deploy/helm/openshell/README.md index 62fac5a428..e8b660df25 100644 --- a/deploy/helm/openshell/README.md +++ b/deploy/helm/openshell/README.md @@ -145,7 +145,6 @@ add `ci/values-spire.yaml` to the OpenShell release values files. | certManager.certificateDuration | string | `"8760h"` | Duration for cert-manager-issued certificates. | | certManager.certificateRenewBefore | string | `"720h"` | Renewal window for cert-manager-issued certificates. | | certManager.clientCaFromServerTlsSecret | bool | `true` | Mount gateway client CA from the server TLS secret's ca.crt (populated by cert-manager for certs issued by a CA Issuer). Set to false when serverIssuerRef points at an external issuer (its secret's ca.crt would be that issuer's chain, not the CA that signs the client cert). When false, also set server.tls.clientCaSecretName to a secret containing the actual client CA — for example caSecretName's value, since that's the CA the client certificate above is issued from by default. | -| certManager.clientIssuerRef | object | `{"group":"","kind":"","name":""}` | Override the issuerRef for the client (mTLS) Certificate. Client certs don't need public trust; leave empty to use the chart's own CA issuer unless an internal PKI issuer is preferred. | | certManager.enabled | bool | `false` | Create cert-manager Issuer and Certificate resources. When enabled, cert-manager owns TLS and the chart runs a JWT-only certgen hook to create the sandbox JWT signing Secret that cert-manager does not manage. | | certManager.serverDnsNames | list | `["openshell","openshell.openshell.svc","openshell.openshell.svc.cluster.local","localhost","openshell.localhost","*.openshell.localhost","host.docker.internal"]` | DNS SANs on the cert-manager-issued server certificate. | | certManager.serverIpAddresses | list | `["127.0.0.1"]` | IP SANs on the cert-manager-issued server certificate. | diff --git a/deploy/helm/openshell/templates/cert-manager-pki.yaml b/deploy/helm/openshell/templates/cert-manager-pki.yaml index c43268096d..fd70ffdcf9 100644 --- a/deploy/helm/openshell/templates/cert-manager-pki.yaml +++ b/deploy/helm/openshell/templates/cert-manager-pki.yaml @@ -43,6 +43,9 @@ spec: secretName: {{ .Values.certManager.caSecretName | quote }} --- {{- $externalServerIssuer := .Values.certManager.serverIssuerRef.name }} +{{- if and $externalServerIssuer .Values.certManager.clientCaFromServerTlsSecret }} +{{- fail "certManager.serverIssuerRef.name is set but certManager.clientCaFromServerTlsSecret is still true \u2014 the server cert Secret's ca.crt comes from the external issuer, not the CA that signs the client (mTLS) certificate. Set certManager.clientCaFromServerTlsSecret=false and set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls)." }} +{{- end }} {{- if $externalServerIssuer }} {{- range .Values.certManager.serverDnsNames }} {{- /* Single-label names (e.g. "openshell") are also rejected by ACME CAs but are intentionally not checked here — the guard targets recognisable internal-network patterns. */ -}} @@ -108,7 +111,7 @@ spec: issuerRef: {{- if .Values.certManager.serverIssuerRef.name }} name: {{ .Values.certManager.serverIssuerRef.name }} - kind: {{ .Values.certManager.serverIssuerRef.kind | default "ClusterIssuer" }} + kind: {{ .Values.certManager.serverIssuerRef.kind | default "Issuer" }} group: {{ .Values.certManager.serverIssuerRef.group | default "cert-manager.io" }} {{- else }} name: {{ include "openshell.fullname" . }}-ca-issuer @@ -136,13 +139,7 @@ spec: - digital signature - key encipherment issuerRef: - {{- if .Values.certManager.clientIssuerRef.name }} - name: {{ .Values.certManager.clientIssuerRef.name }} - kind: {{ .Values.certManager.clientIssuerRef.kind | default "ClusterIssuer" }} - group: {{ .Values.certManager.clientIssuerRef.group | default "cert-manager.io" }} - {{- else }} name: {{ include "openshell.fullname" . }}-ca-issuer kind: Issuer group: cert-manager.io - {{- end }} {{- end }} diff --git a/deploy/helm/openshell/templates/route.yaml b/deploy/helm/openshell/templates/route.yaml index 26262ee7c6..7bef6e58c2 100644 --- a/deploy/helm/openshell/templates/route.yaml +++ b/deploy/helm/openshell/templates/route.yaml @@ -2,6 +2,9 @@ # SPDX-License-Identifier: Apache-2.0 {{- if .Values.openshiftRoute.enabled }} +{{- if .Values.server.disableTls }} +{{- fail "openshiftRoute.enabled=true requires TLS (server.disableTls must be false) \u2014 a passthrough Route forwards encrypted traffic by SNI, so the gateway must terminate its own TLS." }} +{{- end }} apiVersion: route.openshift.io/v1 kind: Route metadata: diff --git a/deploy/helm/openshell/tests/cert_manager_pki_test.yaml b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml index f64458d1b7..b123d2d780 100644 --- a/deploy/helm/openshell/tests/cert_manager_pki_test.yaml +++ b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml @@ -59,6 +59,7 @@ tests: certManager.enabled: true certManager.serverIssuerRef.name: letsencrypt-prod certManager.serverIssuerRef.kind: ClusterIssuer + certManager.clientCaFromServerTlsSecret: false certManager.serverDnsNames: - openshell.example.com asserts: @@ -81,6 +82,7 @@ tests: certManager.enabled: true certManager.serverIssuerRef.name: letsencrypt-prod certManager.serverIssuerRef.kind: ClusterIssuer + certManager.clientCaFromServerTlsSecret: false certManager.serverDnsNames: - openshell.example.com asserts: @@ -105,27 +107,25 @@ tests: content: openshell.my-namespace.svc.cluster.local documentIndex: 3 - - it: overrides the client Certificate issuerRef when clientIssuerRef is set + - it: fails when serverIssuerRef is set but clientCaFromServerTlsSecret is true template: templates/cert-manager-pki.yaml set: certManager.enabled: true - certManager.clientIssuerRef.name: internal-pki-issuer - certManager.clientIssuerRef.kind: ClusterIssuer + certManager.serverIssuerRef.name: letsencrypt-prod + certManager.serverIssuerRef.kind: ClusterIssuer + certManager.clientCaFromServerTlsSecret: true + certManager.serverDnsNames: + - openshell.example.com asserts: - - equal: - path: spec.issuerRef.name - value: internal-pki-issuer - documentIndex: 4 - - equal: - path: spec.issuerRef.kind - value: ClusterIssuer - documentIndex: 4 + - failedTemplate: + errorMessage: "certManager.serverIssuerRef.name is set but certManager.clientCaFromServerTlsSecret is still true \u2014 the server cert Secret's ca.crt comes from the external issuer, not the CA that signs the client (mTLS) certificate. Set certManager.clientCaFromServerTlsSecret=false and set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls)." - it: fails when serverIssuerRef is set but serverDnsNames contains internal-only names template: templates/cert-manager-pki.yaml set: certManager.enabled: true certManager.serverIssuerRef.name: letsencrypt-prod + certManager.clientCaFromServerTlsSecret: false # serverDnsNames is left at the default which contains "openshell.openshell.svc", "localhost", etc. asserts: - failedTemplate: @@ -137,6 +137,7 @@ tests: certManager.enabled: true certManager.serverIssuerRef.name: letsencrypt-prod certManager.serverIssuerRef.kind: ClusterIssuer + certManager.clientCaFromServerTlsSecret: false certManager.serverDnsNames: - openshell.example.com asserts: diff --git a/deploy/helm/openshell/tests/route_test.yaml b/deploy/helm/openshell/tests/route_test.yaml index b218bf3264..e12b7ce2b7 100644 --- a/deploy/helm/openshell/tests/route_test.yaml +++ b/deploy/helm/openshell/tests/route_test.yaml @@ -50,6 +50,14 @@ tests: - notExists: path: spec.host + - it: fails when passthrough Route is enabled with TLS disabled + set: + openshiftRoute.enabled: true + server.disableTls: true + asserts: + - failedTemplate: + errorMessage: "openshiftRoute.enabled=true requires TLS (server.disableTls must be false) \u2014 a passthrough Route forwards encrypted traffic by SNI, so the gateway must terminate its own TLS." + - it: renders custom annotations set: openshiftRoute.enabled: true diff --git a/deploy/helm/openshell/values.yaml b/deploy/helm/openshell/values.yaml index 859c2f8a28..c12d093d0b 100644 --- a/deploy/helm/openshell/values.yaml +++ b/deploy/helm/openshell/values.yaml @@ -360,13 +360,6 @@ certManager: name: "" kind: "" group: "" - # -- Override the issuerRef for the client (mTLS) Certificate. Client certs - # don't need public trust; leave empty to use the chart's own CA issuer - # unless an internal PKI issuer is preferred. - clientIssuerRef: - name: "" - kind: "" - group: "" # -- Mount gateway client CA from the server TLS secret's ca.crt (populated by # cert-manager for certs issued by a CA Issuer). Set to false when # serverIssuerRef points at an external issuer (its secret's ca.crt would be diff --git a/docs/kubernetes/managing-certificates.mdx b/docs/kubernetes/managing-certificates.mdx index 1fcf8a3a59..1b74fd04ab 100644 --- a/docs/kubernetes/managing-certificates.mdx +++ b/docs/kubernetes/managing-certificates.mdx @@ -106,10 +106,6 @@ identifiers, not arbitrary workload identity, and issuing one certificate per sandbox would put a public CA's issuance rate limits directly in the sandbox-creation path. -`certManager.clientIssuerRef` is also available if you'd rather sign the -client certificate from an internal PKI issuer of your own instead of the -chart's self-signed CA — most deployments can leave it unset. - ## Next Steps Return to [Setup](/kubernetes/setup) to complete the installation. For diff --git a/docs/kubernetes/openshift.mdx b/docs/kubernetes/openshift.mdx index d947ea3efc..23022e13cd 100644 --- a/docs/kubernetes/openshift.mdx +++ b/docs/kubernetes/openshift.mdx @@ -95,9 +95,12 @@ from a real Issuer or ClusterIssuer (for example, an ACME issuer), and an OpenShift Route with TLS passthrough exposes it externally while the gateway keeps terminating its own TLS and mTLS. -Install cert-manager and configure a working `ClusterIssuer` first, then see +Install cert-manager and configure a working `ClusterIssuer` first — see [Managing Certificates](/kubernetes/managing-certificates) for the -`certManager.serverIssuerRef` details. Install the chart with: +`certManager.serverIssuerRef` details. Configure an OIDC provider as described +in [Access Control](/kubernetes/access-control) — remote gateways authenticate +CLI users via OIDC, not mTLS, so the gateway must know the OIDC issuer URL. +Install the chart with: ```shell helm install openshell oci://ghcr.io/nvidia/openshell/helm-chart \ @@ -114,7 +117,9 @@ helm install openshell oci://ghcr.io/nvidia/openshell/helm-chart \ --set certManager.serverIssuerRef.kind=ClusterIssuer \ --set certManager.serverDnsNames[0]= \ --set openshiftRoute.enabled=true \ - --set openshiftRoute.host= + --set openshiftRoute.host= \ + --set server.oidc.issuer= \ + --set server.oidc.audience= ``` | Override | Reason | @@ -123,6 +128,7 @@ helm install openshell oci://ghcr.io/nvidia/openshell/helm-chart \ | `certManager.clientCaFromServerTlsSecret=false` + `server.tls.clientCaSecretName` | The server cert's issuer no longer shares a CA with the client (mTLS) cert, so this points the gateway's client-verification CA at `certManager.caSecretName` (the chart's own CA secret, `openshell-ca-tls` by default) instead of the server secret. | | `certManager.serverIssuerRef` | Points the server certificate at your real Issuer or ClusterIssuer instead of the chart's built-in self-signed CA. | | `openshiftRoute.enabled` / `openshiftRoute.host` | Creates an OpenShift Route with TLS passthrough — the router forwards the encrypted connection by SNI without decrypting, so the gateway keeps terminating its own TLS and mTLS. | +| `server.oidc.issuer` / `server.oidc.audience` | Configures server-side OIDC validation. Without these, the gateway expects mTLS client certificates and rejects OIDC-only CLI connections. See [Access Control](/kubernetes/access-control). | Register the gateway with the CLI over OIDC. Remote gateways authenticate CLI users via OIDC, not mTLS — see [Access Control](/kubernetes/access-control): From aac65a995674a6e554d3a295562cc7701b014ba6 Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Wed, 5 Aug 2026 15:15:32 -0400 Subject: [PATCH 5/9] fix(drivers): strip GATEWAY_TLS_SERVER_NAME from Docker and Podman env Signed-off-by: Jesse Jaggars --- crates/openshell-driver-docker/src/lib.rs | 5 +++++ crates/openshell-driver-podman/src/container.rs | 5 +++++ docs/kubernetes/managing-certificates.mdx | 3 +-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index dd4d9ef0f0..599ca661e8 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -2250,6 +2250,11 @@ fn build_environment_for_oci_user( environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); + // Prevent user-supplied environment from overriding the TLS server name + // the supervisor verifies — with WebPKI roots trusted, a sandbox user who + // can redirect the gateway hostname could otherwise present a publicly + // valid certificate for a name they control and intercept the sandbox JWT. + environment.remove(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME); environment.insert( openshell_core::sandbox_env::OCI_IMAGE_USER.to_string(), oci_user.to_string(), diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 005f688a19..a8b0d3ee0e 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -483,6 +483,11 @@ fn build_env( env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); + // Prevent user-supplied environment from overriding the TLS server name + // the supervisor verifies — with WebPKI roots trusted, a sandbox user who + // can redirect the gateway hostname could otherwise present a publicly + // valid certificate for a name they control and intercept the sandbox JWT. + env.remove(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME); env.insert( openshell_core::sandbox_env::OCI_IMAGE_USER.into(), oci_user.to_string(), diff --git a/docs/kubernetes/managing-certificates.mdx b/docs/kubernetes/managing-certificates.mdx index 1b74fd04ab..3aadd0b002 100644 --- a/docs/kubernetes/managing-certificates.mdx +++ b/docs/kubernetes/managing-certificates.mdx @@ -98,8 +98,7 @@ Set `certManager.clientCaFromServerTlsSecret=false` whenever `serverIssuerRef` is set, and set `server.tls.clientCaSecretName` to name a secret that actually contains the client CA — by default that's the same secret named by `certManager.caSecretName` (`openshell-ca-tls`), since that's the CA the chart -issues the client certificate from unless you've also overridden -`clientIssuerRef`. The client (mTLS) certificate used by sandbox supervisors +issues the client certificate from. The client (mTLS) certificate used by sandbox supervisors stays on the chart's own internal CA — it doesn't need public trust, and getting a public CA to sign it isn't practical: ACME only validates domain identifiers, not arbitrary workload identity, and issuing one certificate per From 85ec2140686ec25a6ec833897697c718ae840a50 Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Thu, 6 Aug 2026 09:41:33 -0400 Subject: [PATCH 6/9] fix(helm,drivers): guard default clientCaSecretName and add env-strip tests Signed-off-by: Jesse Jaggars --- Cargo.lock | 2 +- crates/openshell-driver-docker/src/tests.rs | 20 +++++++++++++++++++ .../openshell-driver-podman/src/container.rs | 19 ++++++++++++++++++ .../openshell/templates/cert-manager-pki.yaml | 3 +++ .../tests/cert_manager_pki_test.yaml | 13 ++++++++++++ .../tests/statefulset_client_ca_test.yaml | 3 ++- 6 files changed, 58 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 03aa8386e1..3c719e0e14 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6942,7 +6942,7 @@ dependencies = [ "tower-layer", "tower-service", "tracing", - "webpki-roots 1.0.7", + "webpki-roots", ] [[package]] diff --git a/crates/openshell-driver-docker/src/tests.rs b/crates/openshell-driver-docker/src/tests.rs index ac525c705c..786cb0ac63 100644 --- a/crates/openshell-driver-docker/src/tests.rs +++ b/crates/openshell-driver-docker/src/tests.rs @@ -594,6 +594,26 @@ fn build_environment_protects_oci_identity_metadata() { assert!(!env.iter().any(|entry| entry.ends_with("=9999"))); } +#[test] +fn build_environment_strips_gateway_tls_server_name() { + let mut sandbox = test_sandbox(); + let spec = sandbox.spec.as_mut().unwrap(); + spec.environment.insert( + openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME.to_string(), + "evil.attacker.example.com".to_string(), + ); + + let env = build_environment(&sandbox, &runtime_config()); + + assert!( + !env.iter().any(|entry| entry.starts_with(&format!( + "{}=", + openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME + ))), + "GATEWAY_TLS_SERVER_NAME must be stripped from the supervisor environment" + ); +} + #[test] fn container_creation_uses_inspected_immutable_image() { let sandbox = test_sandbox(); diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index a8b0d3ee0e..2c49169690 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -1418,6 +1418,25 @@ mod tests { ); } + #[test] + fn build_env_strips_gateway_tls_server_name() { + let mut sandbox = test_sandbox("test-id", "test-name"); + let spec = sandbox.spec.get_or_insert_default(); + spec.environment.insert( + openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME.to_string(), + "evil.attacker.example.com".to_string(), + ); + + let container = build_container_spec(&sandbox, &test_config()); + + assert_eq!( + container["env"] + .get(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME), + None, + "GATEWAY_TLS_SERVER_NAME must be stripped from the supervisor environment" + ); + } + #[test] fn volume_name_uses_id() { assert_eq!( diff --git a/deploy/helm/openshell/templates/cert-manager-pki.yaml b/deploy/helm/openshell/templates/cert-manager-pki.yaml index fd70ffdcf9..65a109d499 100644 --- a/deploy/helm/openshell/templates/cert-manager-pki.yaml +++ b/deploy/helm/openshell/templates/cert-manager-pki.yaml @@ -43,6 +43,9 @@ spec: secretName: {{ .Values.certManager.caSecretName | quote }} --- {{- $externalServerIssuer := .Values.certManager.serverIssuerRef.name }} +{{- if and (not .Values.certManager.clientCaFromServerTlsSecret) (eq .Values.server.tls.clientCaSecretName "openshell-server-client-ca") }} +{{- fail "certManager.clientCaFromServerTlsSecret is false but server.tls.clientCaSecretName is still the default (openshell-server-client-ca), which nothing creates when cert-manager owns TLS. Set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls), or set it to empty to disable mTLS client verification." }} +{{- end }} {{- if and $externalServerIssuer .Values.certManager.clientCaFromServerTlsSecret }} {{- fail "certManager.serverIssuerRef.name is set but certManager.clientCaFromServerTlsSecret is still true \u2014 the server cert Secret's ca.crt comes from the external issuer, not the CA that signs the client (mTLS) certificate. Set certManager.clientCaFromServerTlsSecret=false and set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls)." }} {{- end }} diff --git a/deploy/helm/openshell/tests/cert_manager_pki_test.yaml b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml index b123d2d780..04c32ea0d7 100644 --- a/deploy/helm/openshell/tests/cert_manager_pki_test.yaml +++ b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml @@ -60,6 +60,7 @@ tests: certManager.serverIssuerRef.name: letsencrypt-prod certManager.serverIssuerRef.kind: ClusterIssuer certManager.clientCaFromServerTlsSecret: false + server.tls.clientCaSecretName: openshell-ca-tls certManager.serverDnsNames: - openshell.example.com asserts: @@ -83,6 +84,7 @@ tests: certManager.serverIssuerRef.name: letsencrypt-prod certManager.serverIssuerRef.kind: ClusterIssuer certManager.clientCaFromServerTlsSecret: false + server.tls.clientCaSecretName: openshell-ca-tls certManager.serverDnsNames: - openshell.example.com asserts: @@ -126,11 +128,21 @@ tests: certManager.enabled: true certManager.serverIssuerRef.name: letsencrypt-prod certManager.clientCaFromServerTlsSecret: false + server.tls.clientCaSecretName: openshell-ca-tls # serverDnsNames is left at the default which contains "openshell.openshell.svc", "localhost", etc. asserts: - failedTemplate: errorMessage: "certManager.serverIssuerRef.name is set (external issuer) but certManager.serverDnsNames contains \"openshell.openshell.svc\" \u2014 external CAs (e.g. ACME / Let's Encrypt) reject internal-only names per CA/Browser Forum baseline requirements. Override certManager.serverDnsNames with your externally-resolvable hostname(s)." + - it: fails when clientCaFromServerTlsSecret is false but clientCaSecretName is the default + template: templates/cert-manager-pki.yaml + set: + certManager.enabled: true + certManager.clientCaFromServerTlsSecret: false + asserts: + - failedTemplate: + errorMessage: "certManager.clientCaFromServerTlsSecret is false but server.tls.clientCaSecretName is still the default (openshell-server-client-ca), which nothing creates when cert-manager owns TLS. Set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls), or set it to empty to disable mTLS client verification." + - it: client Certificate issuerRef is unaffected by serverIssuerRef template: templates/cert-manager-pki.yaml set: @@ -138,6 +150,7 @@ tests: certManager.serverIssuerRef.name: letsencrypt-prod certManager.serverIssuerRef.kind: ClusterIssuer certManager.clientCaFromServerTlsSecret: false + server.tls.clientCaSecretName: openshell-ca-tls certManager.serverDnsNames: - openshell.example.com asserts: diff --git a/deploy/helm/openshell/tests/statefulset_client_ca_test.yaml b/deploy/helm/openshell/tests/statefulset_client_ca_test.yaml index a7b02310cf..1d744b35aa 100644 --- a/deploy/helm/openshell/tests/statefulset_client_ca_test.yaml +++ b/deploy/helm/openshell/tests/statefulset_client_ca_test.yaml @@ -53,13 +53,14 @@ tests: certManager.enabled: true certManager.clientCaFromServerTlsSecret: false pkiInitJob.enabled: true + server.tls.clientCaSecretName: openshell-ca-tls asserts: - equal: path: spec.template.spec.volumes[3].name value: tls-client-ca - equal: path: spec.template.spec.volumes[3].secret.secretName - value: openshell-server-client-ca + value: openshell-ca-tls - notExists: path: spec.template.spec.volumes[3].secret.items From 863c749a98596d3d1036d2a84c995d8d47f9a7ec Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Fri, 7 Aug 2026 15:29:34 -0400 Subject: [PATCH 7/9] feat(tls): SNI-based dual certificate for internal and external server TLS Split the gateway server certificate into two: an internal cert issued by the chart's own CA (for supervisor connections via cluster-local SANs) and an external cert issued by an operator-configured Issuer such as ACME/Let's Encrypt (for CLI and Route access via public SANs). The gateway uses SNI-based certificate selection: connections whose SNI hostname matches external_server_names receive the external cert; all others (including those with no SNI) receive the internal cert. Security improvement: remove .with_webpki_roots() from the supervisor gRPC client so supervisors trust only the chart CA, closing a MITM vector via publicly-trusted certificates in user-supplied container images. Key changes: - Add DualCertResolver with SNI-based cert selection and full test coverage - Add external_cert_path, external_key_path, external_server_names to TlsConfig - Validate partial external cert config (error on cert-without-key or vice versa) - Validate empty external_server_names when external cert is configured - Split cert-manager templates into internal + external Certificate resources - Add Helm guards for misconfigured external issuer (empty serverDnsNames, internal-only SANs with external issuer, conflicting clientCaFromServerTlsSecret) - Update gateway-config.mdx, managing-certificates.mdx, openshift.mdx docs - Update debug-openshell-cluster skill for dual-cert troubleshooting Signed-off-by: Pi Agent --- .../skills/debug-openshell-cluster/SKILL.md | 36 +- Cargo.lock | 1 - crates/openshell-core/Cargo.toml | 2 +- crates/openshell-core/src/config.rs | 18 + crates/openshell-core/src/grpc_client.rs | 24 +- .../openshell-driver-podman/src/container.rs | 3 +- crates/openshell-server/src/cli.rs | 16 + crates/openshell-server/src/lib.rs | 6 + .../openshell-server/src/service_routing.rs | 3 + crates/openshell-server/src/tls.rs | 422 +++++++++++++++++- .../tests/edge_tunnel_auth.rs | 15 + .../tests/multiplex_tls_integration.rs | 15 + .../values-openshift-route-cert-manager.yaml | 1 - .../openshell/templates/_gateway-workload.tpl | 10 + .../openshell/templates/cert-manager-pki.yaml | 71 +-- .../openshell/templates/gateway-config.yaml | 5 + .../tests/cert_manager_pki_test.yaml | 60 ++- docs/kubernetes/managing-certificates.mdx | 48 +- docs/kubernetes/openshift.mdx | 8 +- docs/reference/gateway-config.mdx | 10 + 20 files changed, 664 insertions(+), 110 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index caa2cfe3c4..8b0ef7242f 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -271,25 +271,38 @@ even when local Helm values disable TLS. If `certManager.serverIssuerRef` points the server certificate at an external Issuer or ClusterIssuer (for example an ACME issuer, for a publicly-trusted cert on an OpenShift `Route` with TLS passthrough — see -`openshiftRoute.enabled`), check the `Certificate`/`CertificateRequest`/ -`Challenge` resources directly when the secret never becomes Ready: +`openshiftRoute.enabled`), the chart creates **two** server certificates: an +internal one (chart CA, internal SANs) and an external one (from the configured +issuer, external SANs only). The gateway uses SNI to present the right cert. + +Check the external `Certificate`/`CertificateRequest`/`Challenge` resources +directly when the external secret never becomes Ready: ```bash kubectl -n openshell get certificate,certificaterequest,challenge -kubectl -n openshell describe certificate openshell-server +kubectl -n openshell describe certificate openshell-server-external oc -n openshell get route ``` ACME issuers reject certificate requests that include internal-only names (`*.svc.cluster.local`, `localhost`, loopback IPs) and require the -`commonName` to also be a SAN — the server `Certificate` only requests the -hostnames in `certManager.serverDnsNames` when `serverIssuerRef` is set, for -exactly this reason. +`commonName` to also be a SAN — the external `Certificate` only requests the +hostnames in `certManager.serverDnsNames`, for exactly this reason. If sandbox supervisors fail their TLS handshake to the gateway with -`UnknownCA` after configuring `serverIssuerRef`, the client (mTLS) certificate -and the server certificate now come from different CAs, and the gateway's -client-verification CA is misconfigured. The chart now fails at render time if +`UnknownCA` after configuring `serverIssuerRef`, the most likely cause is +`server.grpcEndpoint` set to the external hostname. This forces supervisors +to connect via the external hostname, receiving the ACME cert (via SNI) which +they cannot verify against the chart CA. Remove `server.grpcEndpoint` or set +it to the internal service name so supervisors receive the internal cert: + +```bash +helm -n openshell get values openshell | grep -E 'grpcEndpoint|clientCaFromServerTlsSecret|clientCaSecretName|serverIssuerRef|caSecretName' +# server.grpcEndpoint should be unset or point to internal service name +``` + +Less commonly, `UnknownCA` can occur if the gateway's client-verification CA +is misconfigured. The chart fails at render time if `clientCaFromServerTlsSecret` is still true when `serverIssuerRef` is set, but this can still occur with pre-existing releases or manual overrides. Set `certManager.clientCaFromServerTlsSecret=false` and @@ -297,7 +310,6 @@ this can still occur with pre-existing releases or manual overrides. Set signs the client certificate (`certManager.caSecretName`'s value by default): ```bash -helm -n openshell get values openshell | grep -E 'clientCaFromServerTlsSecret|clientCaSecretName|serverIssuerRef|caSecretName' kubectl -n openshell get secret openshell-ca-tls -o jsonpath='{.data.ca\.crt}' | base64 -d | openssl x509 -noout -subject ``` @@ -485,8 +497,8 @@ openshell logs | `K8s namespace not ready` with `envoy-gateway-openshell.yaml: the server could not find the requested resource` | Optional Gateway API manifest was applied without Envoy Gateway CRDs, or k3s Helm controller startup exceeded the namespace wait | Apply `deploy/kube/manifests/envoy-gateway-openshell.yaml` manually only after Envoy Gateway is installed and `grpcRoute` is enabled | | HTTPS ingress (`grpcRoute.gateway.listener.protocol=HTTPS`) connection resets or TLS handshake hangs | Envoy terminates TLS but the gateway pod still expects TLS, so the plaintext backend hop fails | Set `server.disableTls=true` so Envoy forwards plaintext to the pod; verify the listener `certificateRefs` Secret exists in the release namespace and `openshell status` over `https://` | | HTTPS ingress returns `Unauthenticated` after connecting | TLS terminates at Envoy, so the gateway never sees a client cert; no OIDC issuer is configured for identity | Configure `server.oidc.issuer` and register with `openshell gateway add https:// --oidc-issuer `, or set `server.auth.allowUnauthenticatedUsers=true` for a trusted-proxy/dev cluster | -| Server `Certificate` never becomes Ready with `certManager.serverIssuerRef` set | ACME issuer rejected internal-only SANs, a loopback IP, or a `commonName` absent from the SANs | `kubectl -n openshell describe certificate openshell-server`; confirm `certManager.serverDnsNames` lists only real, externally-resolvable hostnames | -| Sandbox supervisors fail TLS handshake with `UnknownCA` after configuring `certManager.serverIssuerRef` | Client (mTLS) cert and server cert now come from different CAs, and `server.tls.clientCaSecretName` still points at the wrong (or default, unpopulated) secret | Set `certManager.clientCaFromServerTlsSecret=false` and `server.tls.clientCaSecretName` to the secret named by `certManager.caSecretName` | +| External server `Certificate` never becomes Ready with `certManager.serverIssuerRef` set | ACME issuer rejected internal-only SANs, a loopback IP, or a `commonName` absent from the SANs | `kubectl -n openshell describe certificate openshell-server-external`; confirm `certManager.serverDnsNames` lists only real, externally-resolvable hostnames | +| Sandbox supervisors fail TLS handshake with `UnknownCA` after configuring `certManager.serverIssuerRef` | `server.grpcEndpoint` is set to the external hostname, forcing supervisors to receive the ACME cert (via SNI) which they can't verify against chart CA | Remove `server.grpcEndpoint` or set it to the internal service name; supervisors should connect via internal service name to receive the internal cert | ## Reporting diff --git a/Cargo.lock b/Cargo.lock index 3c719e0e14..9f3f7dcdca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6942,7 +6942,6 @@ dependencies = [ "tower-layer", "tower-service", "tracing", - "webpki-roots", ] [[package]] diff --git a/crates/openshell-core/Cargo.toml b/crates/openshell-core/Cargo.toml index 658ebff3f5..fe9972e9f3 100644 --- a/crates/openshell-core/Cargo.toml +++ b/crates/openshell-core/Cargo.toml @@ -14,7 +14,7 @@ repository.workspace = true glob = { workspace = true } prost = { workspace = true } prost-types = { workspace = true } -tonic = { workspace = true, features = ["channel", "tls-webpki-roots"] } +tonic = { workspace = true, features = ["channel"] } tonic-prost = { workspace = true } tokio = { workspace = true } thiserror = { workspace = true } diff --git a/crates/openshell-core/src/config.rs b/crates/openshell-core/src/config.rs index 5d4cceeab3..f012cdd25f 100644 --- a/crates/openshell-core/src/config.rs +++ b/crates/openshell-core/src/config.rs @@ -546,6 +546,24 @@ pub struct TlsConfig { /// When `false`, client certificates are accepted but not required. #[serde(default)] pub require_client_auth: bool, + + /// Path to an external TLS certificate file (e.g. ACME/publicly-trusted). + /// When set, the server uses SNI-based certificate selection: connections + /// whose SNI hostname matches `external_server_names` receive this cert, + /// all others receive the primary (internal) cert. + #[serde(default)] + pub external_cert_path: Option, + + /// Path to the private key for the external TLS certificate. + #[serde(default)] + pub external_key_path: Option, + + /// Hostnames that should be served with the external certificate. + /// Connections whose SNI matches one of these names receive the external + /// cert; all other connections (including those with no SNI) receive the + /// primary (internal) cert. + #[serde(default)] + pub external_server_names: Vec, } /// OIDC (`OpenID` Connect) configuration for JWT-based authentication. diff --git a/crates/openshell-core/src/grpc_client.rs b/crates/openshell-core/src/grpc_client.rs index c4272bdbe6..aa2344d7f7 100644 --- a/crates/openshell-core/src/grpc_client.rs +++ b/crates/openshell-core/src/grpc_client.rs @@ -167,23 +167,19 @@ async fn build_plain_channel(endpoint: &str) -> Result { .into_diagnostic() .wrap_err_with(|| format!("failed to read client key from {key_path}"))?; - // Trust the configured CA (self-signed deployments sign the gateway's - // cert with it) *in addition to* the compiled-in WebPKI roots — this - // covers deployments where the gateway's server cert comes from a - // public CA (e.g. an ACME issuer) instead of the same private CA that - // signs this client's identity. + // Trust only the configured CA — this is the chart's internal CA + // that signs both the gateway's internal server certificate and + // this client's identity certificate. The gateway uses SNI-based + // certificate selection to present this internal cert to supervisor + // connections, so no public root trust is needed here. // - // Do NOT add `.with_native_roots()` here: the supervisor runs inside - // the user-selected sandbox image (Docker/Podman drivers), so the - // image's CA bundle is not operator-controlled. An attacker who can - // influence the image and DNS/routing could install their own CA and - // intercept the supervisor→gateway TLS connection. Additionally, - // tonic returns NativeCertsNotFound when the native store is empty, - // which would break minimal BYOC images even though OPENSHELL_TLS_CA - // is valid. + // Do NOT add `.with_native_roots()` or `.with_webpki_roots()` here: + // the supervisor runs inside the user-selected sandbox image + // (Docker/Podman drivers), and broadening the trust store would let + // an attacker who controls the image + DNS present a publicly valid + // certificate and intercept the supervisor→gateway TLS connection. let mut tls_config = ClientTlsConfig::new() .ca_certificate(Certificate::from_pem(ca_pem)) - .with_webpki_roots() .identity(Identity::from_pem(cert_pem, key_pem)); if let Ok(server_name) = std::env::var(sandbox_env::GATEWAY_TLS_SERVER_NAME) && !server_name.is_empty() diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 2c49169690..c4f8b781f7 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -1430,8 +1430,7 @@ mod tests { let container = build_container_spec(&sandbox, &test_config()); assert_eq!( - container["env"] - .get(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME), + container["env"].get(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME), None, "GATEWAY_TLS_SERVER_NAME must be stripped from the supervisor environment" ); diff --git a/crates/openshell-server/src/cli.rs b/crates/openshell-server/src/cli.rs index 8b18034947..1ac77367ee 100644 --- a/crates/openshell-server/src/cli.rs +++ b/crates/openshell-server/src/cli.rs @@ -294,11 +294,27 @@ fn prepare_server_config(args: &mut RunArgs, matches: &ArgMatches) -> Result, require_client_auth: bool, + external_cert_path: Option, + external_key_path: Option, + external_server_names: Vec, reload_spawned: Arc, } @@ -64,14 +68,28 @@ impl TlsAcceptor { key_path: &Path, client_ca_path: Option<&Path>, require_client_auth: bool, + external_cert_path: Option<&Path>, + external_key_path: Option<&Path>, + external_server_names: Vec, ) -> Result { - let config = build_server_config(cert_path, key_path, client_ca_path, require_client_auth)?; + let config = build_server_config( + cert_path, + key_path, + client_ca_path, + require_client_auth, + external_cert_path, + external_key_path, + &external_server_names, + )?; Ok(Self { config: Arc::new(ArcSwap::from(config)), cert_path: cert_path.to_path_buf(), key_path: key_path.to_path_buf(), client_ca_path: client_ca_path.map(Path::to_path_buf), require_client_auth, + external_cert_path: external_cert_path.map(Path::to_path_buf), + external_key_path: external_key_path.map(Path::to_path_buf), + external_server_names, reload_spawned: Arc::new(AtomicBool::new(false)), }) } @@ -87,6 +105,9 @@ impl TlsAcceptor { &self.key_path, self.client_ca_path.as_deref(), self.require_client_auth, + self.external_cert_path.as_deref(), + self.external_key_path.as_deref(), + &self.external_server_names, )?; self.config.store(new_config); @@ -144,10 +165,22 @@ impl TlsAcceptor { } if let Some(ref ca) = self.client_ca_path { let ca_dir = ca.parent().unwrap_or_else(|| Path::new(".")); - if ca_dir != cert_dir && ca_dir != key_dir { + if !dirs.contains(&ca_dir.to_path_buf()) { dirs.push(ca_dir.to_path_buf()); } } + if let Some(ref ext_cert) = self.external_cert_path { + let ext_dir = ext_cert.parent().unwrap_or_else(|| Path::new(".")); + if !dirs.contains(&ext_dir.to_path_buf()) { + dirs.push(ext_dir.to_path_buf()); + } + } + if let Some(ref ext_key) = self.external_key_path { + let ext_dir = ext_key.parent().unwrap_or_else(|| Path::new(".")); + if !dirs.contains(&ext_dir.to_path_buf()) { + dirs.push(ext_dir.to_path_buf()); + } + } let debounce = Duration::from_secs(1); @@ -244,12 +277,87 @@ impl TlsAcceptor { } } +/// SNI-based certificate resolver that presents an external (e.g. ACME) +/// certificate for configured hostnames and the internal (chart CA) certificate +/// for everything else, including connections with no SNI. +struct DualCertResolver { + internal: Arc, + external: Arc, + external_names: Vec, +} + +impl ResolvesServerCert for DualCertResolver { + fn resolve(&self, client_hello: ClientHello<'_>) -> Option> { + if let Some(name) = client_hello.server_name() + && self.external_names.iter().any(|n| n == name) + { + return Some(self.external.clone()); + } + Some(self.internal.clone()) + } +} + +impl std::fmt::Debug for DualCertResolver { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("DualCertResolver") + .field("external_names", &self.external_names) + .finish() + } +} + +/// Build a `CertifiedKey` from certificate and key file paths. +fn load_certified_key(cert_path: &Path, key_path: &Path) -> Result> { + let certs = load_certs(cert_path)?; + let key = load_key(key_path)?; + let signing_key = sign::any_supported_type(&key) + .map_err(|e| Error::tls(format!("unsupported private key type: {e}")))?; + Ok(Arc::new(CertifiedKey::new(certs, signing_key))) +} + +/// Build an SNI-based cert resolver when an external certificate is configured. +/// Returns `None` when no external cert is configured (single-cert mode). +fn build_cert_resolver( + cert_path: &Path, + key_path: &Path, + external_cert_path: Option<&Path>, + external_key_path: Option<&Path>, + external_server_names: &[String], +) -> Result>> { + match (external_cert_path, external_key_path) { + (None, None) => Ok(None), + (Some(_), None) => Err(Error::tls( + "external_cert_path is set but external_key_path is missing", + )), + (None, Some(_)) => Err(Error::tls( + "external_key_path is set but external_cert_path is missing", + )), + (Some(ext_cert_path), Some(ext_key_path)) => { + if external_server_names.is_empty() { + return Err(Error::tls( + "external certificate is configured but external_server_names is empty — \ + the external cert would never be served", + )); + } + let internal = load_certified_key(cert_path, key_path)?; + let external = load_certified_key(ext_cert_path, ext_key_path)?; + Ok(Some(Arc::new(DualCertResolver { + internal, + external, + external_names: external_server_names.to_vec(), + }))) + } + } +} + /// Build a `ServerConfig` from certificate, key, and optional client CA files. fn build_server_config( cert_path: &Path, key_path: &Path, client_ca_path: Option<&Path>, require_client_auth: bool, + external_cert_path: Option<&Path>, + external_key_path: Option<&Path>, + external_server_names: &[String], ) -> Result> { let certs = load_certs(cert_path)?; let key = load_key(key_path)?; @@ -259,6 +367,14 @@ fn build_server_config( sign::any_supported_type(&key) .map_err(|e| Error::tls(format!("unsupported private key type: {e}")))?; + let resolver = build_cert_resolver( + cert_path, + key_path, + external_cert_path, + external_key_path, + external_server_names, + )?; + let mut config = if let Some(ca_path) = client_ca_path { let ca_certs = load_certs(ca_path)?; let mut root_store = rustls::RootCertStore::empty(); @@ -277,15 +393,23 @@ fn build_server_config( .build() .map_err(|e| Error::tls(format!("failed to build client verifier: {e}")))?; - ServerConfig::builder() - .with_client_cert_verifier(verifier) - .with_single_cert(certs, key) - .map_err(|e| Error::tls(format!("failed to create TLS config: {e}")))? + let builder = ServerConfig::builder().with_client_cert_verifier(verifier); + if let Some(resolver) = resolver { + builder.with_cert_resolver(resolver) + } else { + builder + .with_single_cert(certs, key) + .map_err(|e| Error::tls(format!("failed to create TLS config: {e}")))? + } } else { - ServerConfig::builder() - .with_no_client_auth() - .with_single_cert(certs, key) - .map_err(|e| Error::tls(format!("failed to create TLS config: {e}")))? + let builder = ServerConfig::builder().with_no_client_auth(); + if let Some(resolver) = resolver { + builder.with_cert_resolver(resolver) + } else { + builder + .with_single_cert(certs, key) + .map_err(|e| Error::tls(format!("failed to create TLS config: {e}")))? + } }; config @@ -402,6 +526,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), false, + None, + None, + &[], ) .expect("failed to build server config"); @@ -420,6 +547,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), false, + None, + None, + Vec::new(), ) .expect("failed to build acceptor"); @@ -438,6 +568,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), false, + None, + None, + Vec::new(), ) .expect("failed to build acceptor"); @@ -468,6 +601,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), false, + None, + None, + Vec::new(), ) .expect("failed to build acceptor"); @@ -560,6 +696,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), false, + None, + None, + Vec::new(), ) .expect("failed to build acceptor"); @@ -633,6 +772,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), false, + None, + None, + Vec::new(), ) .expect("failed to build acceptor"); @@ -664,6 +806,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), false, + None, + None, + Vec::new(), ) .expect("failed to build acceptor"); @@ -752,6 +897,9 @@ mod tests { &dir.path().join("server-key.pem"), Some(&dir.path().join("ca.pem")), true, // require mTLS + None, + None, + Vec::new(), ) .expect("failed to build acceptor with mTLS"); @@ -905,4 +1053,256 @@ mod tests { server_task.await.expect("server task failed"); } + + /// Generate a cert+key pair with given SANs, signed by the provided CA, + /// and write them to the specified files in `dir`. + fn generate_named_cert( + ca_cert: &rcgen::Certificate, + ca_key: &KeyPair, + dir: &Path, + cert_file: &str, + key_file: &str, + san: &str, + ) { + let params = + CertificateParams::new(vec![san.to_string()]).expect("failed to create cert params"); + let key = KeyPair::generate().expect("failed to generate key"); + let cert = params + .signed_by(&key, ca_cert, ca_key) + .expect("failed to sign cert"); + write_test_file(dir, cert_file, cert.pem().as_bytes()); + write_test_file(dir, key_file, key.serialize_pem().as_bytes()); + } + + #[test] + fn test_build_cert_resolver_returns_none_when_no_external() { + install_rustls_provider(); + let dir = tempfile::tempdir().expect("failed to create tempdir"); + generate_test_certs_with_ca(dir.path()); + + let result = build_cert_resolver( + &dir.path().join("server-cert.pem"), + &dir.path().join("server-key.pem"), + None, + None, + &[], + ) + .expect("build_cert_resolver should succeed"); + assert!(result.is_none(), "should return None when no external cert"); + } + + #[test] + fn test_build_cert_resolver_errors_on_cert_without_key() { + install_rustls_provider(); + let dir = tempfile::tempdir().expect("failed to create tempdir"); + generate_test_certs_with_ca(dir.path()); + + let result = build_cert_resolver( + &dir.path().join("server-cert.pem"), + &dir.path().join("server-key.pem"), + Some(&dir.path().join("server-cert.pem")), + None, + &["example.com".to_string()], + ); + let err = result.expect_err("should error when key is missing"); + assert!( + err.to_string().contains("external_key_path is missing"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_build_cert_resolver_errors_on_key_without_cert() { + install_rustls_provider(); + let dir = tempfile::tempdir().expect("failed to create tempdir"); + generate_test_certs_with_ca(dir.path()); + + let result = build_cert_resolver( + &dir.path().join("server-cert.pem"), + &dir.path().join("server-key.pem"), + None, + Some(&dir.path().join("server-key.pem")), + &["example.com".to_string()], + ); + let err = result.expect_err("should error when cert is missing"); + assert!( + err.to_string().contains("external_cert_path is missing"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_build_cert_resolver_errors_on_empty_server_names() { + install_rustls_provider(); + let dir = tempfile::tempdir().expect("failed to create tempdir"); + let (ca_cert, ca_key) = generate_test_certs_with_ca(dir.path()); + generate_named_cert( + &ca_cert, + &ca_key, + dir.path(), + "ext-cert.pem", + "ext-key.pem", + "external.example.com", + ); + + let result = build_cert_resolver( + &dir.path().join("server-cert.pem"), + &dir.path().join("server-key.pem"), + Some(&dir.path().join("ext-cert.pem")), + Some(&dir.path().join("ext-key.pem")), + &[], + ); + let err = result.expect_err("should error when server names are empty"); + assert!( + err.to_string().contains("external_server_names is empty"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_dual_cert_resolver_returns_external_on_sni_match() { + install_rustls_provider(); + let dir = tempfile::tempdir().expect("failed to create tempdir"); + let (ca_cert, ca_key) = generate_test_certs_with_ca(dir.path()); + generate_named_cert( + &ca_cert, + &ca_key, + dir.path(), + "ext-cert.pem", + "ext-key.pem", + "external.example.com", + ); + + let internal = load_certified_key( + &dir.path().join("server-cert.pem"), + &dir.path().join("server-key.pem"), + ) + .expect("load internal"); + let external = load_certified_key( + &dir.path().join("ext-cert.pem"), + &dir.path().join("ext-key.pem"), + ) + .expect("load external"); + + let internal_der = internal.cert[0].as_ref().to_vec(); + let external_der = external.cert[0].as_ref().to_vec(); + + // `ClientHello` cannot be constructed directly in tests, so + // SNI-based selection is exercised in the async integration test + // below. Here we verify the certs are distinct so the integration + // test's DER comparisons are meaningful. + assert_ne!( + internal_der, external_der, + "internal and external certs should be distinct" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn test_dual_cert_resolver_sni_selects_correct_cert() { + install_rustls_provider(); + + let dir = tempfile::tempdir().expect("failed to create tempdir"); + let (ca_cert, ca_key) = generate_test_certs_with_ca(dir.path()); + generate_named_cert( + &ca_cert, + &ca_key, + dir.path(), + "ext-cert.pem", + "ext-key.pem", + "external.example.com", + ); + + // Snapshot the DER of the internal and external leaf certs. + let internal_der = load_certs(&dir.path().join("server-cert.pem")) + .expect("load internal certs")[0] + .as_ref() + .to_vec(); + let external_der = load_certs(&dir.path().join("ext-cert.pem")) + .expect("load external certs")[0] + .as_ref() + .to_vec(); + + let acceptor = TlsAcceptor::from_files( + &dir.path().join("server-cert.pem"), + &dir.path().join("server-key.pem"), + Some(&dir.path().join("ca.pem")), + false, + Some(&dir.path().join("ext-cert.pem")), + Some(&dir.path().join("ext-key.pem")), + vec!["external.example.com".to_string()], + ) + .expect("failed to build acceptor"); + + let client_config = build_test_client_config(&dir.path().join("ca.pem")); + + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("failed to bind"); + let listen_addr = listener.local_addr().expect("failed to get local addr"); + + // --- Connection 1: SNI matches external name → external cert --- + let acceptor_srv = acceptor.clone(); + let server_task = tokio::spawn(async move { + let (stream, _) = listener.accept().await.expect("accept failed"); + acceptor_srv + .acceptor() + .accept(stream) + .await + .expect("TLS accept failed") + }); + + let connector = tokio_rustls::TlsConnector::from(client_config.clone()); + let tcp = TcpStream::connect(listen_addr) + .await + .expect("connect failed"); + let server_name = "external.example.com" + .try_into() + .expect("invalid server name"); + let tls = connector + .connect(server_name, tcp) + .await + .expect("TLS connect failed"); + + assert_eq!( + peer_cert_der(&tls), + external_der, + "SNI matching external name should serve external cert" + ); + drop(tls); + let _ = server_task.await; + + // --- Connection 2: SNI = "localhost" → internal cert --- + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("failed to bind"); + let listen_addr = listener.local_addr().expect("failed to get local addr"); + + let acceptor_srv = acceptor.clone(); + let server_task = tokio::spawn(async move { + let (stream, _) = listener.accept().await.expect("accept failed"); + acceptor_srv + .acceptor() + .accept(stream) + .await + .expect("TLS accept failed") + }); + + let connector = tokio_rustls::TlsConnector::from(client_config); + let tcp = TcpStream::connect(listen_addr) + .await + .expect("connect failed"); + let server_name = "localhost".try_into().expect("invalid server name"); + let tls = connector + .connect(server_name, tcp) + .await + .expect("TLS connect failed"); + + assert_eq!( + peer_cert_der(&tls), + internal_der, + "SNI not matching external name should serve internal cert" + ); + drop(tls); + let _ = server_task.await; + } } diff --git a/crates/openshell-server/tests/edge_tunnel_auth.rs b/crates/openshell-server/tests/edge_tunnel_auth.rs index 4df221f117..be70e45675 100644 --- a/crates/openshell-server/tests/edge_tunnel_auth.rs +++ b/crates/openshell-server/tests/edge_tunnel_auth.rs @@ -168,6 +168,9 @@ async fn mtls_valid_client_cert_accepted() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); @@ -209,6 +212,9 @@ async fn no_client_cert_accepted_with_ca_configured() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); @@ -252,6 +258,9 @@ async fn bearer_header_reaches_server_without_client_cert() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); @@ -283,6 +292,9 @@ async fn rogue_cert_rejected() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); @@ -329,6 +341,9 @@ async fn https_only_no_client_cert_required() { &temp.path().join("server-key.pem"), None, false, + None, + None, + Vec::new(), ) .unwrap(); diff --git a/crates/openshell-server/tests/multiplex_tls_integration.rs b/crates/openshell-server/tests/multiplex_tls_integration.rs index 4e17fdef97..3447aad517 100644 --- a/crates/openshell-server/tests/multiplex_tls_integration.rs +++ b/crates/openshell-server/tests/multiplex_tls_integration.rs @@ -62,6 +62,9 @@ async fn serves_grpc_and_http_over_tls_on_same_port() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); @@ -101,6 +104,9 @@ async fn mtls_valid_client_cert_accepted() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); @@ -129,6 +135,9 @@ async fn no_client_cert_accepted_with_ca() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); @@ -165,6 +174,9 @@ async fn no_client_cert_rejected_when_required() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), true, + None, + None, + Vec::new(), ) .unwrap(); @@ -202,6 +214,9 @@ async fn mtls_wrong_ca_client_cert_rejected() { &temp.path().join("server-key.pem"), Some(temp.path().join("ca.pem").as_path()), false, + None, + None, + Vec::new(), ) .unwrap(); diff --git a/deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml b/deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml index d06283ee15..f1e8b4f7ef 100644 --- a/deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml +++ b/deploy/helm/openshell/ci/values-openshift-route-cert-manager.yaml @@ -12,7 +12,6 @@ server: disableTls: false - grpcEndpoint: "https://openshell.example.com:443" tls: # Must name a secret containing the actual client CA when # clientCaFromServerTlsSecret is false — here, the same CA secret diff --git a/deploy/helm/openshell/templates/_gateway-workload.tpl b/deploy/helm/openshell/templates/_gateway-workload.tpl index 5931047e5f..d455cefab3 100644 --- a/deploy/helm/openshell/templates/_gateway-workload.tpl +++ b/deploy/helm/openshell/templates/_gateway-workload.tpl @@ -84,6 +84,11 @@ spec: - name: tls-cert mountPath: /etc/openshell-tls/server readOnly: true + {{- if .Values.certManager.serverIssuerRef.name }} + - name: tls-external-cert + mountPath: /etc/openshell-tls/server-external + readOnly: true + {{- end }} {{- if or .Values.server.tls.clientCaSecretName (and .Values.pkiInitJob.enabled (not .Values.certManager.enabled)) (and .Values.certManager.enabled .Values.certManager.clientCaFromServerTlsSecret) }} - name: tls-client-ca mountPath: /etc/openshell-tls/client-ca @@ -144,6 +149,11 @@ spec: - name: tls-cert secret: secretName: {{ .Values.server.tls.certSecretName }} + {{- if .Values.certManager.serverIssuerRef.name }} + - name: tls-external-cert + secret: + secretName: {{ include "openshell.fullname" . }}-server-external-tls + {{- end }} {{- if or .Values.server.tls.clientCaSecretName (and .Values.pkiInitJob.enabled (not .Values.certManager.enabled)) (and .Values.certManager.enabled .Values.certManager.clientCaFromServerTlsSecret) }} - name: tls-client-ca secret: diff --git a/deploy/helm/openshell/templates/cert-manager-pki.yaml b/deploy/helm/openshell/templates/cert-manager-pki.yaml index 65a109d499..cd9da7ebab 100644 --- a/deploy/helm/openshell/templates/cert-manager-pki.yaml +++ b/deploy/helm/openshell/templates/cert-manager-pki.yaml @@ -50,6 +50,9 @@ spec: {{- fail "certManager.serverIssuerRef.name is set but certManager.clientCaFromServerTlsSecret is still true \u2014 the server cert Secret's ca.crt comes from the external issuer, not the CA that signs the client (mTLS) certificate. Set certManager.clientCaFromServerTlsSecret=false and set server.tls.clientCaSecretName to the secret containing the client CA (e.g. the value of certManager.caSecretName, which defaults to openshell-ca-tls)." }} {{- end }} {{- if $externalServerIssuer }} +{{- if not .Values.certManager.serverDnsNames }} +{{- fail "certManager.serverIssuerRef.name is set but certManager.serverDnsNames is empty — the external certificate requires at least one externally-resolvable DNS name." }} +{{- end }} {{- range .Values.certManager.serverDnsNames }} {{- /* Single-label names (e.g. "openshell") are also rejected by ACME CAs but are intentionally not checked here — the guard targets recognisable internal-network patterns. */ -}} {{- if or (eq . "localhost") (hasSuffix ".localhost" .) (hasSuffix ".svc.cluster.local" .) (hasSuffix ".svc" .) (eq . "host.docker.internal") (eq . "host.containers.internal") }} @@ -57,6 +60,9 @@ spec: {{- end }} {{- end }} {{- end }} +# Internal server certificate — always issued by the chart’s own CA with +# internal SANs. Supervisors connect via internal hostnames and verify +# this cert against the chart CA they already trust. apiVersion: cert-manager.io/v1 kind: Certificate metadata: @@ -68,42 +74,20 @@ spec: secretName: {{ .Values.server.tls.certSecretName | quote }} duration: {{ .Values.certManager.certificateDuration | quote }} renewBefore: {{ .Values.certManager.certificateRenewBefore | quote }} - {{- if $externalServerIssuer }} - # External issuers (e.g. ACME) reject internal-only names (cluster-local, - # localhost, loopback IPs) per CA/Browser Forum baseline requirements, and - # require any commonName to also appear in dnsNames. Only request the - # externally-resolvable SANs the operator configured — no defaults, no IPs. - {{- if .Values.certManager.serverDnsNames }} - commonName: {{ first .Values.certManager.serverDnsNames | quote }} - {{- end }} - dnsNames: - {{- range .Values.certManager.serverDnsNames }} - - {{ . | quote }} - {{- end }} - {{- else }} - # The chart fullname is always the first entry of defaultServerDnsNames - # below, so it's always valid as a commonName. - # - # Upgrade note: this replaces the previously-hardcoded "openshell-server" - # commonName. The ACME commonName-must-be-a-SAN constraint this exists for - # only applies to the external-issuer branch above, but this branch was - # changed too for consistency. Existing self-signed/internal-CA deployments - # will get a new server cert with a different Subject CN on next reissue - # after upgrading — SAN-based hostname verification (what TLS clients - # actually check) is unaffected. commonName: {{ include "openshell.fullname" . }} dnsNames: {{- range (include "openshell.defaultServerDnsNames" . | fromYamlArray) }} - {{ . | quote }} {{- end }} + {{- if not $externalServerIssuer }} {{- range .Values.certManager.serverDnsNames }} - {{ . | quote }} {{- end }} + {{- end }} {{- if .Values.certManager.serverIpAddresses }} ipAddresses: {{- toYaml .Values.certManager.serverIpAddresses | nindent 4 }} {{- end }} - {{- end }} privateKey: algorithm: ECDSA size: 256 @@ -112,15 +96,44 @@ spec: - digital signature - key encipherment issuerRef: - {{- if .Values.certManager.serverIssuerRef.name }} - name: {{ .Values.certManager.serverIssuerRef.name }} - kind: {{ .Values.certManager.serverIssuerRef.kind | default "Issuer" }} - group: {{ .Values.certManager.serverIssuerRef.group | default "cert-manager.io" }} - {{- else }} name: {{ include "openshell.fullname" . }}-ca-issuer kind: Issuer group: cert-manager.io +{{- if $externalServerIssuer }} +--- +# External server certificate — issued by the operator-configured issuer +# (e.g. ACME/Let’s Encrypt) with only externally-resolvable SANs. +# The gateway uses SNI to present this cert for external hostnames. +apiVersion: cert-manager.io/v1 +kind: Certificate +metadata: + name: {{ include "openshell.fullname" . }}-server-external + namespace: {{ .Release.Namespace }} + labels: + {{- include "openshell.labels" . | nindent 4 }} +spec: + secretName: {{ include "openshell.fullname" . }}-server-external-tls + duration: {{ .Values.certManager.certificateDuration | quote }} + renewBefore: {{ .Values.certManager.certificateRenewBefore | quote }} + {{- if .Values.certManager.serverDnsNames }} + commonName: {{ first .Values.certManager.serverDnsNames | quote }} + {{- end }} + dnsNames: + {{- range .Values.certManager.serverDnsNames }} + - {{ . | quote }} {{- end }} + privateKey: + algorithm: ECDSA + size: 256 + usages: + - server auth + - digital signature + - key encipherment + issuerRef: + name: {{ .Values.certManager.serverIssuerRef.name }} + kind: {{ .Values.certManager.serverIssuerRef.kind | default "Issuer" }} + group: {{ .Values.certManager.serverIssuerRef.group | default "cert-manager.io" }} +{{- end }} --- apiVersion: cert-manager.io/v1 kind: Certificate diff --git a/deploy/helm/openshell/templates/gateway-config.yaml b/deploy/helm/openshell/templates/gateway-config.yaml index 0c2fc3bbd4..1fcff7996b 100644 --- a/deploy/helm/openshell/templates/gateway-config.yaml +++ b/deploy/helm/openshell/templates/gateway-config.yaml @@ -81,6 +81,11 @@ data: cert_path = "/etc/openshell-tls/server/tls.crt" key_path = "/etc/openshell-tls/server/tls.key" client_ca_path = "/etc/openshell-tls/client-ca/ca.crt" + {{- if .Values.certManager.serverIssuerRef.name }} + external_cert_path = "/etc/openshell-tls/server-external/tls.crt" + external_key_path = "/etc/openshell-tls/server-external/tls.key" + external_server_names = [{{- range $i, $name := .Values.certManager.serverDnsNames }}{{ if $i }}, {{ end }}{{ $name | quote }}{{- end }}] + {{- end }} {{- end }} {{- if .Values.server.auth.allowUnauthenticatedUsers }} diff --git a/deploy/helm/openshell/tests/cert_manager_pki_test.yaml b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml index 04c32ea0d7..089b3b5037 100644 --- a/deploy/helm/openshell/tests/cert_manager_pki_test.yaml +++ b/deploy/helm/openshell/tests/cert_manager_pki_test.yaml @@ -53,7 +53,7 @@ tests: value: 127.0.0.1 documentIndex: 3 - - it: overrides the server Certificate issuerRef when serverIssuerRef is set + - it: internal server cert keeps chart CA issuer and internal SANs when serverIssuerRef is set template: templates/cert-manager-pki.yaml set: certManager.enabled: true @@ -64,20 +64,35 @@ tests: certManager.serverDnsNames: - openshell.example.com asserts: + # Internal cert (doc 3) stays on chart CA - equal: path: spec.issuerRef.name - value: letsencrypt-prod + value: openshell-ca-issuer documentIndex: 3 - equal: path: spec.issuerRef.kind - value: ClusterIssuer + value: Issuer documentIndex: 3 + # Internal cert has internal SANs - equal: - path: spec.issuerRef.group - value: cert-manager.io + path: spec.commonName + value: openshell + documentIndex: 3 + - contains: + path: spec.dnsNames + content: openshell.my-namespace.svc.cluster.local + documentIndex: 3 + - contains: + path: spec.dnsNames + content: localhost + documentIndex: 3 + # Internal cert does NOT include external-only hostnames + - notContains: + path: spec.dnsNames + content: openshell.example.com documentIndex: 3 - - it: scopes server Certificate SANs to only the configured external hostnames when serverIssuerRef is set + - it: creates external server Certificate from serverIssuerRef with external SANs only template: templates/cert-manager-pki.yaml set: certManager.enabled: true @@ -88,26 +103,38 @@ tests: certManager.serverDnsNames: - openshell.example.com asserts: + # External cert (doc 4) uses the external issuer + - equal: + path: spec.issuerRef.name + value: letsencrypt-prod + documentIndex: 4 + - equal: + path: spec.issuerRef.kind + value: ClusterIssuer + documentIndex: 4 + - equal: + path: spec.issuerRef.group + value: cert-manager.io + documentIndex: 4 + # External cert has only external SANs - equal: path: spec.commonName value: openshell.example.com - documentIndex: 3 + documentIndex: 4 - equal: path: spec.dnsNames value: - openshell.example.com - documentIndex: 3 + documentIndex: 4 + # External cert has no IP addresses - notExists: path: spec.ipAddresses - documentIndex: 3 + documentIndex: 4 + # External cert has no internal names - notContains: path: spec.dnsNames content: localhost - documentIndex: 3 - - notContains: - path: spec.dnsNames - content: openshell.my-namespace.svc.cluster.local - documentIndex: 3 + documentIndex: 4 - it: fails when serverIssuerRef is set but clientCaFromServerTlsSecret is true template: templates/cert-manager-pki.yaml @@ -154,11 +181,12 @@ tests: certManager.serverDnsNames: - openshell.example.com asserts: + # Client cert is now doc 5 (after internal + external server certs) - equal: path: spec.issuerRef.name value: openshell-ca-issuer - documentIndex: 4 + documentIndex: 5 - equal: path: spec.issuerRef.kind value: Issuer - documentIndex: 4 + documentIndex: 5 diff --git a/docs/kubernetes/managing-certificates.mdx b/docs/kubernetes/managing-certificates.mdx index 3aadd0b002..39de163cbe 100644 --- a/docs/kubernetes/managing-certificates.mdx +++ b/docs/kubernetes/managing-certificates.mdx @@ -81,29 +81,41 @@ helm upgrade --install openshell \ --set certManager.serverDnsNames[0]=openshell.example.com ``` +### Dual certificate architecture + +When `serverIssuerRef` is set, the chart creates **two** server certificates: + +1. **Internal certificate** (`openshell-server-tls`): signed by the chart CA + with internal SANs (`*.svc.cluster.local`, `localhost`, etc.). +2. **External certificate** (`openshell-server-external-tls`): signed by the + configured issuer (e.g. ACME) with only the hostnames from + `certManager.serverDnsNames`. + +The gateway uses **SNI** to select which certificate to present: +supervisors connect via internal service names and receive the internal +certificate (verified against the chart CA they already trust), while CLI +users connecting through a Route or ingress use the external hostname and +receive the ACME certificate. This keeps supervisor trust pinned to only +the operator's chart CA — no WebPKI root trust is needed. + Public CAs such as Let's Encrypt reject certificate requests that include -internal-only names (`*.svc.cluster.local`, `localhost`, loopback IPs) per -CA/Browser Forum baseline requirements. The chart validates this at install -time and fails with an actionable error if `certManager.serverDnsNames` -contains internal-only entries while `serverIssuerRef` is set. When -`serverIssuerRef` is set, the server certificate's SANs are limited to the -hostnames in `certManager.serverDnsNames` — the chart's usual internal -cluster-local SANs are omitted. Sandboxes calling back into the gateway must then use the same -external hostname; see `server.grpcEndpoint` in -[Gateway Configuration](/reference/gateway-config). +internal-only names per CA/Browser Forum baseline requirements. The chart +validates this at install time and fails with an actionable error if +`certManager.serverDnsNames` contains internal-only entries while +`serverIssuerRef` is set. + +You do **not** need to set `server.grpcEndpoint` to the external hostname. +Supervisors connect via the internal service name automatically. Setting +`server.grpcEndpoint` to an external hostname would cause supervisors to +receive the ACME certificate (via SNI) which they cannot verify against the +chart CA. Set `certManager.clientCaFromServerTlsSecret=false` whenever `serverIssuerRef` -is set, and set `server.tls.clientCaSecretName` to name a secret that actually -contains the client CA — by default that's the same secret named by -`certManager.caSecretName` (`openshell-ca-tls`), since that's the CA the chart -issues the client certificate from. The client (mTLS) certificate used by sandbox supervisors -stays on the chart's own internal CA — it doesn't need public trust, and -getting a public CA to sign it isn't practical: ACME only validates domain -identifiers, not arbitrary workload identity, and issuing one certificate per -sandbox would put a public CA's issuance rate limits directly in the -sandbox-creation path. +is set, and set `server.tls.clientCaSecretName` to a secret containing the +client CA — by default that's `certManager.caSecretName` (`openshell-ca-tls`), +the CA the chart issues the client certificate from. ## Next Steps diff --git a/docs/kubernetes/openshift.mdx b/docs/kubernetes/openshift.mdx index 23022e13cd..43666b9410 100644 --- a/docs/kubernetes/openshift.mdx +++ b/docs/kubernetes/openshift.mdx @@ -109,7 +109,6 @@ helm install openshell oci://ghcr.io/nvidia/openshell/helm-chart \ --set podSecurityContext.fsGroup=null \ --set securityContext.runAsUser=null \ --set server.disableTls=false \ - --set server.grpcEndpoint=https://:443 \ --set certManager.enabled=true \ --set certManager.clientCaFromServerTlsSecret=false \ --set server.tls.clientCaSecretName=openshell-ca-tls \ @@ -124,10 +123,9 @@ helm install openshell oci://ghcr.io/nvidia/openshell/helm-chart \ | Override | Reason | |---|---| -| `server.grpcEndpoint` | Sandboxes call back into the gateway using this hostname. A cert from an external issuer only carries the external SANs you configure, not the internal cluster-local ones, so sandboxes must use the same externally-valid hostname to pass TLS verification. | -| `certManager.clientCaFromServerTlsSecret=false` + `server.tls.clientCaSecretName` | The server cert's issuer no longer shares a CA with the client (mTLS) cert, so this points the gateway's client-verification CA at `certManager.caSecretName` (the chart's own CA secret, `openshell-ca-tls` by default) instead of the server secret. | -| `certManager.serverIssuerRef` | Points the server certificate at your real Issuer or ClusterIssuer instead of the chart's built-in self-signed CA. | -| `openshiftRoute.enabled` / `openshiftRoute.host` | Creates an OpenShift Route with TLS passthrough — the router forwards the encrypted connection by SNI without decrypting, so the gateway keeps terminating its own TLS and mTLS. | +| `certManager.clientCaFromServerTlsSecret=false` + `server.tls.clientCaSecretName` | Points the gateway's client-verification CA at `certManager.caSecretName` (the chart's own CA secret, `openshell-ca-tls` by default). Supervisors connect via internal service names and receive the internal certificate signed by this CA. | +| `certManager.serverIssuerRef` | Creates a second server certificate from your Issuer or ClusterIssuer for external clients. The gateway uses SNI to present this cert for the external hostname while continuing to present the internal (chart CA) cert to supervisors. | +| `openshiftRoute.enabled` / `openshiftRoute.host` | Creates an OpenShift Route with TLS passthrough — the router forwards the encrypted connection by SNI without decrypting, so the gateway uses the SNI hostname to select the external certificate. | | `server.oidc.issuer` / `server.oidc.audience` | Configures server-side OIDC validation. Without these, the gateway expects mTLS client certificates and rejects OIDC-only CLI connections. See [Access Control](/kubernetes/access-control). | Register the gateway with the CLI over OIDC. Remote gateways authenticate CLI diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index 4db3c9c472..05735a8377 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -133,6 +133,10 @@ cert_path = "/etc/openshell/certs/gateway.pem" key_path = "/etc/openshell/certs/gateway-key.pem" client_ca_path = "/etc/openshell/certs/client-ca.pem" require_client_auth = false +# Optional: SNI-based dual certificate for external (e.g. ACME) TLS. +# external_cert_path = "/etc/openshell/certs/external.pem" +# external_key_path = "/etc/openshell/certs/external-key.pem" +# external_server_names = ["gateway.example.com"] [openshell.gateway.gateway_jwt] signing_key_path = "/etc/openshell/jwt/signing.pem" @@ -184,6 +188,8 @@ phases = ["validate"] Local Docker, Podman, and VM gateways can also set `[openshell.gateway.mtls_auth] enabled = true` to authenticate CLI callers from verified client certificates. Kubernetes deployments must leave this unset and use OIDC or a trusted access proxy; the Helm chart does not render this table. +`[openshell.gateway.tls]` supports optional SNI-based dual-certificate mode for deployments that need separate internal and external server certificates. Set `external_cert_path` and `external_key_path` to point at the external (e.g. ACME/publicly-trusted) certificate and key. List the hostnames that should be served with the external certificate in `external_server_names`. Connections whose TLS SNI hostname matches one of those names receive the external certificate; all other connections (including those with no SNI) receive the primary internal certificate from `cert_path`/`key_path`. Both fields must be set together — providing only one is a configuration error. On Kubernetes with the Helm chart, the external certificate is managed automatically when `certManager.serverIssuerRef.name` is set; the chart populates these fields from the cert-manager-issued external server certificate. + `[openshell.gateway] policy_validation_failure_mode` controls what sandbox supervisors do when a complete candidate policy fails runtime validation. The default, `fail_closed`, deactivates the previous network policy, closes relays pinned to it, and denies new egress until a valid generation loads. `retain_last_valid` leaves the previous valid generation active. Both modes reject the candidate atomically; startup always fails closed when no previous valid generation exists. Gateway mutation paths that can preflight a known effective scope reject invalid candidates before persistence and leave the active policy unchanged regardless of this setting. Changing the value requires restarting the gateway so it can reload `gateway.toml` and distribute the new posture to sandbox supervisors. `[openshell.gateway.gateway_jwt] ttl_secs` controls gateway-minted sandbox JWT lifetime. When omitted, it defaults to `0`: the token `exp` claim and `expires_at_ms` response field become `0`, and the sandbox JWT does not expire. Use that default only for local single-player Docker, Podman, or VM gateways. Kubernetes and other shared deployments should set a positive TTL; Helm renders `3600` seconds by default, and the gateway logs a warning when a Kubernetes gateway uses `0`. @@ -315,6 +321,10 @@ compute_drivers = ["kubernetes"] cert_path = "/etc/openshell-tls/server/tls.crt" key_path = "/etc/openshell-tls/server/tls.key" client_ca_path = "/etc/openshell-tls/client-ca/ca.crt" +# When cert-manager serverIssuerRef is configured, these are populated by Helm: +# external_cert_path = "/etc/openshell-tls/server-external/tls.crt" +# external_key_path = "/etc/openshell-tls/server-external/tls.key" +# external_server_names = ["gateway.example.com"] [openshell.drivers.kubernetes] namespace = "agents" From 4443506ab990ded612159077461ae0e1db2f5522 Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Fri, 7 Aug 2026 16:03:06 -0400 Subject: [PATCH 8/9] fix(drivers): strip GATEWAY_TLS_SERVER_NAME in VM driver and correct comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the same GATEWAY_TLS_SERVER_NAME environment stripping to the VM compute driver that Docker, Podman, and Kubernetes drivers already perform. Without this, a sandbox user on the VM driver could override the TLS server name the supervisor verifies. Fix stale comments in Docker and Podman drivers that referenced 'with WebPKI roots trusted' — WebPKI roots are explicitly not trusted after the tls-webpki-roots removal. Use tls-ring instead of bare channel for tonic in openshell-core so the TLS API (ClientTlsConfig, Endpoint::tls_config) is available without pulling in any root certificate store. Signed-off-by: Pi Agent --- crates/openshell-core/Cargo.toml | 2 +- crates/openshell-driver-docker/src/lib.rs | 6 ++-- .../openshell-driver-podman/src/container.rs | 6 ++-- crates/openshell-driver-vm/src/driver.rs | 35 +++++++++++++++++++ 4 files changed, 42 insertions(+), 7 deletions(-) diff --git a/crates/openshell-core/Cargo.toml b/crates/openshell-core/Cargo.toml index fe9972e9f3..380732ce7b 100644 --- a/crates/openshell-core/Cargo.toml +++ b/crates/openshell-core/Cargo.toml @@ -14,7 +14,7 @@ repository.workspace = true glob = { workspace = true } prost = { workspace = true } prost-types = { workspace = true } -tonic = { workspace = true, features = ["channel"] } +tonic = { workspace = true, features = ["channel", "tls-ring"] } tonic-prost = { workspace = true } tokio = { workspace = true } thiserror = { workspace = true } diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 599ca661e8..bc5e9adca1 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -2251,9 +2251,9 @@ fn build_environment_for_oci_user( environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); // Prevent user-supplied environment from overriding the TLS server name - // the supervisor verifies — with WebPKI roots trusted, a sandbox user who - // can redirect the gateway hostname could otherwise present a publicly - // valid certificate for a name they control and intercept the sandbox JWT. + // the supervisor verifies — a sandbox user who can redirect the gateway + // hostname could otherwise present a certificate for a name they control + // and intercept the sandbox JWT. environment.remove(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME); environment.insert( openshell_core::sandbox_env::OCI_IMAGE_USER.to_string(), diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index c4f8b781f7..df61a13e2d 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -484,9 +484,9 @@ fn build_env( env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); env.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); // Prevent user-supplied environment from overriding the TLS server name - // the supervisor verifies — with WebPKI roots trusted, a sandbox user who - // can redirect the gateway hostname could otherwise present a publicly - // valid certificate for a name they control and intercept the sandbox JWT. + // the supervisor verifies — a sandbox user who can redirect the gateway + // hostname could otherwise present a certificate for a name they control + // and intercept the sandbox JWT. env.remove(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME); env.insert( openshell_core::sandbox_env::OCI_IMAGE_USER.into(), diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index 9b6c0dd6ce..fd7b2c8ae5 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -4268,6 +4268,11 @@ fn build_guest_environment( ); environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN); environment.remove(openshell_core::sandbox_env::SANDBOX_TOKEN_FILE); + // Prevent user-supplied environment from overriding the TLS server name + // the supervisor verifies — a sandbox user who can redirect the gateway + // hostname could otherwise present a certificate for a name they control + // and intercept the sandbox JWT. + environment.remove(openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME); if sandbox .spec .as_ref() @@ -6716,6 +6721,36 @@ mod tests { ))); } + #[test] + fn build_guest_environment_strips_gateway_tls_server_name() { + let config = VmDriverConfig { + openshell_endpoint: "http://127.0.0.1:8080".to_string(), + ..Default::default() + }; + let sandbox = Sandbox { + id: "sandbox-123".to_string(), + name: "sandbox-123".to_string(), + spec: Some(SandboxSpec { + environment: HashMap::from([( + openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME.to_string(), + "evil.attacker.example.com".to_string(), + )]), + ..Default::default() + }), + ..Default::default() + }; + + let env = build_guest_environment(&sandbox, &config, None); + + assert!( + !env.iter().any(|v| v.starts_with(&format!( + "{}=", + openshell_core::sandbox_env::GATEWAY_TLS_SERVER_NAME + ))), + "GATEWAY_TLS_SERVER_NAME must be stripped from the guest environment" + ); + } + #[test] fn build_guest_environment_uses_deployment_telemetry_toggle() { let _guard = ENV_LOCK.lock().unwrap(); From 1362c65ae65b2cd972d930618d5bd05b5baec577 Mon Sep 17 00:00:00 2001 From: Jesse Jaggars Date: Fri, 7 Aug 2026 16:10:54 -0400 Subject: [PATCH 9/9] fix(tls,helm): wildcard SNI matching and Route host validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add RFC 6125 single-level wildcard matching to DualCertResolver so external_server_names entries like *.example.com correctly match SNI hostnames like gw.example.com. Previously only exact matches worked, silently falling back to the internal cert for wildcard configurations. Add a Helm fail guard in route.yaml that rejects openshiftRoute.host values not listed in certManager.serverDnsNames when an external issuer is configured — catches cert/route hostname mismatches at install time instead of at TLS connect time. Quote the host field in route.yaml for robustness. Signed-off-by: Pi Agent --- crates/openshell-server/src/tls.rs | 36 +++++++++++++++++++++- deploy/helm/openshell/templates/route.yaml | 5 ++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/crates/openshell-server/src/tls.rs b/crates/openshell-server/src/tls.rs index 24bbae7100..aa627746ce 100644 --- a/crates/openshell-server/src/tls.rs +++ b/crates/openshell-server/src/tls.rs @@ -286,10 +286,25 @@ struct DualCertResolver { external_names: Vec, } +/// Check whether `sni` matches a configured external name. +/// +/// Supports exact matches and single-level wildcard matches per RFC 6125: +/// `*.example.com` matches `foo.example.com` but not `bar.foo.example.com` +/// or `example.com` itself. +fn sni_matches(pattern: &str, sni: &str) -> bool { + pattern.strip_prefix("*.").map_or(pattern == sni, |suffix| { + // Wildcard: SNI must have exactly one label before the suffix. + // e.g. "foo." for "foo.example.com" against "*.example.com" + sni.strip_suffix(suffix).is_some_and(|prefix| { + prefix.ends_with('.') && !prefix[..prefix.len() - 1].contains('.') + }) + }) +} + impl ResolvesServerCert for DualCertResolver { fn resolve(&self, client_hello: ClientHello<'_>) -> Option> { if let Some(name) = client_hello.server_name() - && self.external_names.iter().any(|n| n == name) + && self.external_names.iter().any(|n| sni_matches(n, name)) { return Some(self.external.clone()); } @@ -1074,6 +1089,25 @@ mod tests { write_test_file(dir, key_file, key.serialize_pem().as_bytes()); } + #[test] + fn test_sni_matches_exact() { + assert!(sni_matches("example.com", "example.com")); + assert!(!sni_matches("example.com", "other.com")); + assert!(!sni_matches("example.com", "sub.example.com")); + } + + #[test] + fn test_sni_matches_wildcard() { + assert!(sni_matches("*.example.com", "foo.example.com")); + assert!(sni_matches("*.example.com", "bar.example.com")); + // Must not match bare domain. + assert!(!sni_matches("*.example.com", "example.com")); + // Must not match nested subdomains (RFC 6125). + assert!(!sni_matches("*.example.com", "sub.foo.example.com")); + // Must not match unrelated domain with same suffix. + assert!(!sni_matches("*.example.com", "notexample.com")); + } + #[test] fn test_build_cert_resolver_returns_none_when_no_external() { install_rustls_provider(); diff --git a/deploy/helm/openshell/templates/route.yaml b/deploy/helm/openshell/templates/route.yaml index 7bef6e58c2..292b71b67a 100644 --- a/deploy/helm/openshell/templates/route.yaml +++ b/deploy/helm/openshell/templates/route.yaml @@ -5,6 +5,9 @@ {{- if .Values.server.disableTls }} {{- fail "openshiftRoute.enabled=true requires TLS (server.disableTls must be false) \u2014 a passthrough Route forwards encrypted traffic by SNI, so the gateway must terminate its own TLS." }} {{- end }} +{{- if and .Values.openshiftRoute.host .Values.certManager.serverIssuerRef.name .Values.certManager.serverDnsNames (not (has .Values.openshiftRoute.host .Values.certManager.serverDnsNames)) }} +{{- fail (printf "openshiftRoute.host %q is not listed in certManager.serverDnsNames %v — the Route will forward SNI for a hostname the external certificate does not cover, causing TLS verification failures for CLI clients." .Values.openshiftRoute.host .Values.certManager.serverDnsNames) }} +{{- end }} apiVersion: route.openshift.io/v1 kind: Route metadata: @@ -18,7 +21,7 @@ metadata: {{- end }} spec: {{- if .Values.openshiftRoute.host }} - host: {{ .Values.openshiftRoute.host }} + host: {{ .Values.openshiftRoute.host | quote }} {{- end }} to: kind: Service