From 6847738f27b897dfdf83c07bf8bc3deedae7d849 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Mon, 1 Jun 2026 15:54:11 -0700 Subject: [PATCH 1/2] fix(config): reject unknown fields in nested gateway config tables The gateway TOML loader silently ignored keys placed under the wrong table header. PR #1661 fixed one instance of this (supervisor_image under [openshell.gateway.gateway_jwt]) but the root cause remained: the nested gateway config tables did not deny unknown fields, so a misplaced key was accepted and dropped instead of erroring. Concretely, tasks/scripts/gateway.sh emitted `sandbox_namespace` right after the [openshell.gateway.gateway_jwt] heredoc, so it landed inside the gateway_jwt table rather than [openshell.gateway]. The k8s driver already receives the namespace via [openshell.drivers.kubernetes], so the stray line was dead config that parsed without complaint. Changes: - Add #[serde(deny_unknown_fields)] to the nested gateway config tables that are part of the config-file parse tree: TlsConfig, OidcConfig, MtlsAuthConfig, GatewayAuthConfig, GatewayJwtConfig. - Remove the misplaced sandbox_namespace line from gateway.sh. - Drop the unused Serialize/Deserialize derives from Config and ServiceRoutingConfig (see below). - Add a regression test asserting a key under the wrong nested table is rejected. Why Config and ServiceRoutingConfig no longer need serde: The on-disk schema is parsed into the gateway's ConfigFile / GatewayFileSection types (openshell-server::config_file), not into openshell_core::Config. Config is assembled programmatically in Config::new plus the gateway CLI, merging the parsed file with env vars and CLI flags. A repo-wide search found no call site that serializes or deserializes either struct (no toml/serde_json/serde_yml round-trip, no DB persistence -- database_url is just a connection string). The derives were vestigial from an earlier design (RFC 0003) where the file mapped directly onto Config before the dedicated loader types were introduced. Removing them makes the parse tree the single, unambiguous deserialization surface. The crates are not published, so this is not a public API break. --- crates/openshell-core/src/config.rs | 31 +++++++++++----------- crates/openshell-server/src/config_file.rs | 20 ++++++++++++++ tasks/scripts/gateway.sh | 1 - 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/crates/openshell-core/src/config.rs b/crates/openshell-core/src/config.rs index 98562c8a6..a0165fc9c 100644 --- a/crates/openshell-core/src/config.rs +++ b/crates/openshell-core/src/config.rs @@ -167,22 +167,25 @@ fn is_unix_socket(path: &Path) -> bool { } /// Server configuration. -#[derive(Debug, Clone, Serialize, Deserialize)] +/// +/// Built programmatically in [`crate::Config::new`] and the gateway CLI from +/// the parsed config file, env vars, and CLI flags. It is never deserialized +/// directly; the on-disk config schema lives in the gateway's `config_file` +/// module ([`crate::TlsConfig`] and the other nested tables carry their own +/// `Deserialize` impls for that purpose). +#[derive(Debug, Clone)] pub struct Config { /// Address to bind the server to. - #[serde(default = "default_bind_address")] pub bind_address: SocketAddr, /// Address to bind the unauthenticated health endpoint to. /// /// When `None`, the dedicated health listener is disabled. - #[serde(default)] pub health_bind_address: Option, /// Address to bind the Prometheus metrics endpoint to. /// /// When `None`, the dedicated metrics listener is disabled. - #[serde(default)] pub metrics_bind_address: Option, /// Additional bind addresses that serve the same multiplexed gRPC/HTTP @@ -191,36 +194,30 @@ pub struct Config { /// Compute drivers may register extra listeners during startup so that /// sandbox workloads can call back into the gateway over an interface /// that the operator-supplied `bind_address` does not expose. - #[serde(default)] pub extra_bind_addresses: Vec, /// Log level (trace, debug, info, warn, error). - #[serde(default = "default_log_level")] pub log_level: String, /// TLS configuration. When `None`, the server listens on plaintext HTTP. pub tls: Option, /// OIDC configuration. When `Some`, the server validates Bearer JWTs. - #[serde(default)] pub oidc: Option, /// Gateway user authentication behavior. - #[serde(default)] pub auth: GatewayAuthConfig, /// mTLS user authentication configuration. When enabled, a verified TLS /// client certificate can authenticate CLI/SDK callers as a /// `Principal::User`. This is for local single-user gateways only; /// sandbox identity is always carried by gateway-minted sandbox JWTs. - #[serde(default)] pub mtls_auth: MtlsAuthConfig, /// Gateway-minted sandbox JWT configuration. When `Some`, the gateway /// loads the signing key from disk and accepts gateway-issued sandbox /// JWTs as `Principal::Sandbox`. Required for the per-sandbox identity /// flow (issue #1354). - #[serde(default)] pub gateway_jwt: Option, /// Database URL for persistence. @@ -231,29 +228,26 @@ pub struct Config { /// The config shape allows multiple drivers so the gateway can evolve /// toward multi-backend routing. Current releases require exactly one /// configured driver. - #[serde(default)] pub compute_drivers: Vec, /// TTL for SSH session tokens, in seconds. 0 disables expiry. - #[serde(default = "default_ssh_session_ttl_secs")] pub ssh_session_ttl_secs: u64, /// Browser-facing sandbox service routing configuration. - #[serde(default)] pub service_routing: ServiceRoutingConfig, } /// Browser-facing sandbox service routing configuration. -#[derive(Debug, Clone, Serialize, Deserialize)] +/// +/// Part of the programmatically-built [`Config`]; never deserialized directly. +#[derive(Debug, Clone)] pub struct ServiceRoutingConfig { /// Base domains accepted for `sandbox--service.` routes. /// The first domain is used when the gateway prints endpoint URLs. - #[serde(default = "default_service_routing_domains")] pub base_domains: Vec, /// Enable TLS-enabled loopback gateway listeners to also accept plaintext /// HTTP for sandbox service hostnames. - #[serde(default = "default_enable_loopback_service_http")] pub enable_loopback_service_http: bool, } @@ -269,6 +263,7 @@ pub struct ServiceRoutingConfig { /// In both modes, authentication is handled at the application layer /// (e.g. OIDC bearer tokens). mTLS is an additional mechanism. #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct TlsConfig { /// Path to the TLS certificate file. pub cert_path: PathBuf, @@ -299,6 +294,7 @@ pub struct TlsConfig { /// - Entra ID / Okta: `roles` /// - Custom: any dot-separated path into the JWT claims #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct OidcConfig { /// OIDC issuer URL (e.g., `http://localhost:8180/realms/openshell`). pub issuer: String, @@ -333,6 +329,7 @@ pub struct OidcConfig { /// mTLS user authentication for local, single-user gateways. #[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct MtlsAuthConfig { /// When true, the gateway maps a verified TLS client certificate into a /// user principal. Keep disabled for Kubernetes deployments because @@ -343,6 +340,7 @@ pub struct MtlsAuthConfig { /// Gateway user authentication settings. #[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct GatewayAuthConfig { /// When true, unauthenticated user/CLI calls are accepted as a local /// developer principal. This is an unsafe local-development escape hatch @@ -363,6 +361,7 @@ const fn default_jwks_ttl_secs() -> u64 { /// signing key never leaves the gateway process; the public key is loaded /// by the same gateway so it can validate its own tokens. #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct GatewayJwtConfig { /// Path to the Ed25519 signing key (PKCS#8 PEM). pub signing_key_path: PathBuf, diff --git a/crates/openshell-server/src/config_file.rs b/crates/openshell-server/src/config_file.rs index d7852e8ee..11d7f70f4 100644 --- a/crates/openshell-server/src/config_file.rs +++ b/crates/openshell-server/src/config_file.rs @@ -420,6 +420,26 @@ nonsense = true assert!(matches!(err, ConfigFileError::Parse { .. })); } + #[test] + fn rejects_unknown_field_in_nested_gateway_jwt_table() { + // Regression guard for the class of silent-misconfig bug fixed in + // PR #1661: a key indented under the wrong table header (here, + // `sandbox_namespace` landing under `[openshell.gateway.gateway_jwt]` + // instead of `[openshell.gateway]`) must be rejected rather than + // silently ignored. + let toml = r#" +[openshell.gateway.gateway_jwt] +signing_key_path = "/tmp/jwt/signing.pem" +public_key_path = "/tmp/jwt/public.pem" +kid_path = "/tmp/jwt/kid" +sandbox_namespace = "agents" +"#; + let tmp = write_tmp(toml); + let err = + load(tmp.path()).expect_err("unknown field in nested gateway_jwt table must be rejected"); + assert!(matches!(err, ConfigFileError::Parse { .. })); + } + #[test] fn rejects_removed_ssh_endpoint_fields() { let toml = r" diff --git a/tasks/scripts/gateway.sh b/tasks/scripts/gateway.sh index 8d9e1d71e..11fbf9a05 100644 --- a/tasks/scripts/gateway.sh +++ b/tasks/scripts/gateway.sh @@ -333,7 +333,6 @@ EOF case "${DRIVER}" in kubernetes) cat >>"${CONFIG_PATH}" < Date: Mon, 1 Jun 2026 15:59:42 -0700 Subject: [PATCH 2/2] style: satisfy rustfmt in config_file test --- crates/openshell-server/src/config_file.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/openshell-server/src/config_file.rs b/crates/openshell-server/src/config_file.rs index 11d7f70f4..57037bcf5 100644 --- a/crates/openshell-server/src/config_file.rs +++ b/crates/openshell-server/src/config_file.rs @@ -435,8 +435,8 @@ kid_path = "/tmp/jwt/kid" sandbox_namespace = "agents" "#; let tmp = write_tmp(toml); - let err = - load(tmp.path()).expect_err("unknown field in nested gateway_jwt table must be rejected"); + let err = load(tmp.path()) + .expect_err("unknown field in nested gateway_jwt table must be rejected"); assert!(matches!(err, ConfigFileError::Parse { .. })); }