Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions .agents/skills/debug-openshell-cluster/SKILL.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -284,6 +284,52 @@ 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`), 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-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 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 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 default `clientCaFromServerTlsSecret=true` is correct
for all configurations — the internal server certificate is always signed by
the chart CA (the same CA that signs the client cert), so its `ca.crt` is
the right trust anchor. Only override this if you intentionally mount a
separate client CA via `server.tls.clientCaSecretName`. Verify the mounted
client CA matches the CA that signed the client certificate:

```bash
kubectl -n openshell get statefulset openshell -o jsonpath='{.spec.template.spec.volumes[?(@.name=="tls-client-ca")]}' | jq .
# Should show items filter for ca.crt from openshell-server-tls
```

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
Expand DownExpand Up@@ -469,6 +515,8 @@ openshell logs <sandbox-name>
| `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://<host>` |
| 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://<host> --oidc-issuer <url>`, or set `server.auth.allowUnauthenticatedUsers=true` for a trusted-proxy/dev cluster |
| 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

Expand Down
2 changes: 1 addition & 1 deletion crates/openshell-core/Cargo.toml
Original file line numberDiff line numberDiff line change
Expand Up@@ -15,7 +15,7 @@ async-trait = "0.1"
glob = { workspace = true }
prost = { workspace = true }
prost-types = { workspace = true }
tonic = { workspace = true, features = ["channel", "tls-native-roots"] }
tonic = { workspace = true, features = ["channel", "tls-ring"] }
tonic-prost = { workspace = true }
tokio = { workspace = true }
thiserror = { workspace = true }
Expand Down
18 changes: 18 additions & 0 deletions crates/openshell-core/src/config.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -547,6 +547,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<PathBuf>,

/// Path to the private key for the external TLS certificate.
#[serde(default)]
pub external_key_path: Option<PathBuf>,

/// 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<String>,
}

/// OIDC (`OpenID` Connect) configuration for JWT-based authentication.
Expand Down
11 changes: 11 additions & 0 deletions crates/openshell-core/src/grpc_client.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -167,6 +167,17 @@ async fn build_plain_channel(endpoint: &str) -> Result<Channel> {
.into_diagnostic()
.wrap_err_with(|| format!("failed to read client key from {key_path}"))?;

// 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()` 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))
.identity(Identity::from_pem(cert_pem, key_pem));
Expand Down
5 changes: 5 additions & 0 deletions crates/openshell-driver-docker/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -2453,6 +2453,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 — 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(),
oci_user.to_string(),
Expand Down
20 changes: 20 additions & 0 deletions crates/openshell-driver-docker/src/tests.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -595,6 +595,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();
Expand Down
23 changes: 23 additions & 0 deletions crates/openshell-driver-podman/src/container.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 — 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(),
oci_user.to_string(),
Expand DownExpand Up@@ -1413,6 +1418,24 @@ 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!(
Expand Down
35 changes: 35 additions & 0 deletions crates/openshell-driver-vm/src/driver.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -4447,6 +4447,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()
Expand DownExpand Up@@ -6961,6 +6966,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();
Expand Down
16 changes: 16 additions & 0 deletions crates/openshell-server/src/cli.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -294,11 +294,27 @@ fn prepare_server_config(args: &mut RunArgs, matches: &ArgMatches) -> Result<Ser
let key_path = args.tls_key.clone().ok_or_else(|| {
miette::miette!("--tls-key is required when TLS is enabled (use --disable-tls to skip)")
})?;
// External cert config (SNI-based dual cert) is only configurable
// via the TOML file, not CLI flags — it's a deployment-time setting.
let (ext_cert, ext_key, ext_names) = file
.as_ref()
.and_then(|f| f.openshell.gateway.tls.as_ref())
.map(|tls| {
(
tls.external_cert_path.clone(),
tls.external_key_path.clone(),
tls.external_server_names.clone(),
)
})
.unwrap_or_default();
Some(openshell_core::TlsConfig {
cert_path,
key_path,
require_client_auth: has_client_ca && !has_oidc,
client_ca_path: args.tls_client_ca.clone(),
external_cert_path: ext_cert,
external_key_path: ext_key,
external_server_names: ext_names,
})
};

Expand Down
6 changes: 6 additions & 0 deletions crates/openshell-server/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -564,6 +564,9 @@ pub(crate) async fn run_server(
&tls.key_path,
tls.client_ca_path.as_deref(),
tls.require_client_auth,
tls.external_cert_path.as_deref(),
tls.external_key_path.as_deref(),
tls.external_server_names.clone(),
)?;

// Spawn file-watcher-based TLS certificate reload worker.
Expand DownExpand Up@@ -1145,6 +1148,9 @@ mod tests {
&dir.path().join("server-key.pem"),
Some(&dir.path().join("ca.pem")),
false,
None,
None,
Vec::new(),
)
.expect("failed to build tls acceptor");

Expand Down
3 changes: 3 additions & 0 deletions crates/openshell-server/src/service_routing.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -856,6 +856,9 @@ mod tests {
key_path: "server.key".into(),
client_ca_path: Some("ca.crt".into()),
require_client_auth: false,
external_cert_path: None,
external_key_path: None,
external_server_names: Vec::new(),
}
}

Expand Down
Loading
Loading