From b0bd8cd4bd6931ccf0082724ad5c567aca4415e9 Mon Sep 17 00:00:00 2001 From: Shekhar Mudarapu Date: Thu, 30 Jul 2026 21:37:16 -0400 Subject: [PATCH] =?UTF-8?q?fix(runtime):=20vend=20secrets=20over=20mTLS=20?= =?UTF-8?q?=E2=80=94=20the=20boot=20contract=20now=20passes=20end=20to=20e?= =?UTF-8?q?nd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EXPECT_VENDSECRET=1` passes on real KVM. All four assertions green, twice, with no leaked TAPs. The security-critical part of the boot contract — a per-VM client cert presented over mTLS, checked against the manager's allowlist — is exercised on every boot instead of being skipped. Three faults, each found only after the previous one was cleared. 1. tonic silently replaced the TLS config. The generated client's `connect()` routes through `tonic::transport::Endpoint::new`, which for an https:// URI does: return me.tls_config(ClientTlsConfig::new().with_enabled_roots()); overwriting the configured TLS, and `with_enabled_roots` discards its own receiver. With neither roots feature enabled the result is an EMPTY root store, no client identity, and no domain — which is exactly what the guest showed: UnknownIssuer (no roots), "client auth requested but no cert available" (no identity), and verification against the URI's address (no domain). Three symptoms, one cause. Connect via `endpoint.connect()` and build the client from the channel; that path does not go through `Endpoint::new`, so the configured TLS survives. 2. Declared secrets never reached the guest. The Firecracker backend never put `spec.secrets` in the boot args, so the harness saw none declared and its cache refused every request — a spec declaring a secret behaved exactly like one declaring none. The manager now emits one `lantern.secret.=` per secret and the harness reassembles the JSON. One key each rather than a JSON blob: kernel args are whitespace-separated and quotes are stripped from values, so serialised JSON arrives mangled. 3. Nothing ever populated the secret cache. `refresh_loop` only renews entries already cached and `get_or_vend` is driven by the workload asking over the socket, so a VM with no workload never exercised the vend path at all — precisely the path worth verifying. The harness now prefetches declared secrets at boot, best-effort and spawned so a slow manager cannot delay boot. This does hold a declared-but-unused secret in guest memory for the VM's life; that is already true of any secret the workload touches once, and declared secrets are by definition ones it may use. Verified: 2 consecutive runs, 4/4 assertions each, 0 leaked TAPs. 153 harness + 346 manager tests pass, clippy and rustfmt clean on both. Fault (1) is a tonic footgun worth knowing about repo-wide: any code doing `SomeClient::connect(endpoint)` over https with a custom ClientTlsConfig is silently getting default TLS instead. --- services/harness/src/bootenv.rs | 45 ++++++++++ services/harness/src/main.rs | 7 ++ services/harness/src/manager_client.rs | 87 ++++++++++++++----- services/harness/src/secrets.rs | 38 ++++++++ .../src/backends/firecracker.rs | 21 ++++- 5 files changed, 173 insertions(+), 25 deletions(-) diff --git a/services/harness/src/bootenv.rs b/services/harness/src/bootenv.rs index d75d0925..9f4cfb94 100644 --- a/services/harness/src/bootenv.rs +++ b/services/harness/src/bootenv.rs @@ -47,9 +47,54 @@ pub fn bootstrap() { report_entropy_source(); mount_cert_drive(); hydrate_env_from_cmdline(); + assemble_declared_secrets(); pin_manager_hostname(); } +/// Rebuild `LANTERN_DECLARED_SECRETS` from the per-secret boot-args keys. +/// +/// The manager emits one `lantern.secret.=` per declared secret +/// (a JSON blob cannot survive the kernel command line: args are +/// whitespace-separated and quotes are stripped from values). The rest of the +/// harness reads a JSON array, so assemble it here. +/// +/// Without this the secret cache sees nothing declared and refuses every +/// request, so a spec declaring secrets behaves exactly like one declaring +/// none — VendSecret is never reached. +fn assemble_declared_secrets() { + // An explicitly-provided value wins, as everywhere else in this module. + if std::env::var_os("LANTERN_DECLARED_SECRETS").is_some() { + return; + } + let Ok(cmdline) = std::fs::read_to_string(CMDLINE_PATH) else { + return; + }; + + let declared: Vec = cmdline + .split_whitespace() + .filter_map(|token| token.split_once('=')) + .filter_map(|(key, uri)| { + key.strip_prefix("lantern.secret.") + .filter(|name| !name.is_empty() && !uri.is_empty()) + .map(|name| serde_json::json!({ "env_name": name, "secret_uri": uri })) + }) + .collect(); + + if declared.is_empty() { + return; + } + let count = declared.len(); + match serde_json::to_string(&declared) { + Ok(json) => { + // SAFETY: called from main before any thread is spawned. + unsafe { std::env::set_var("LANTERN_DECLARED_SECRETS", &json) }; + // Names only — a secret URI can identify sensitive material. + tracing::info!(count, "bootenv: assembled declared secrets from boot-args"); + } + Err(e) => tracing::warn!(error = %e, "bootenv: could not encode declared secrets"), + } +} + /// Name the manager is reached by from inside the guest. /// /// Must match a DNS SAN on the manager's server certificate. diff --git a/services/harness/src/main.rs b/services/harness/src/main.rs index f8e8d3d2..6fac6e87 100644 --- a/services/harness/src/main.rs +++ b/services/harness/src/main.rs @@ -155,6 +155,13 @@ async fn main() -> Result<()> { tracing::error!(error = %e, "secrets: server exited"); } }); + // Warm the cache before the workload can ask, so the mTLS vend path + // runs on every boot rather than only when something requests a + // secret. Spawned: a slow or unreachable manager must not delay boot. + let s = Arc::clone(&secrets); + tokio::spawn(async move { + s.prefetch_declared().await; + }); let s = Arc::clone(&secrets); tokio::spawn(async move { s.refresh_loop().await; diff --git a/services/harness/src/manager_client.rs b/services/harness/src/manager_client.rs index d2ed86c2..5206564b 100644 --- a/services/harness/src/manager_client.rs +++ b/services/harness/src/manager_client.rs @@ -149,14 +149,27 @@ impl ManagerClient { endpoint }; - let mut client = pb::runtime_harness_client::RuntimeHarnessClient::connect(endpoint) - .await - .with_context(|| { - format!( - "heartbeat: could not connect to manager at {}", - self.manager_addr - ) - })?; + // NOTE: connect via `endpoint.connect()`, NOT `Client::connect(endpoint)`. + // The generated client's `connect` routes through + // `tonic::transport::Endpoint::new`, which for an https:// URI does: + // + // return me.tls_config(ClientTlsConfig::new().with_enabled_roots()); + // + // — silently REPLACING the TLS config set above with a default one. And + // `with_enabled_roots` discards its receiver, so with neither + // `tls-native-roots` nor `tls-webpki-roots` enabled the result is an + // EMPTY root store, no client identity, and no domain name. That is + // exactly what the guest saw: UnknownIssuer (no roots), "client auth + // requested but no cert available" (no identity), and verification + // against the URI's address (no domain). `Endpoint::connect` does not + // go through `Endpoint::new`, so the configured TLS survives. + let channel = endpoint.connect().await.with_context(|| { + format!( + "heartbeat: could not connect to manager at {}", + self.manager_addr + ) + })?; + let mut client = pb::runtime_harness_client::RuntimeHarnessClient::new(channel); // Outbound channel: heartbeat.rs pushes HeartbeatRequest here; we // forward each one (converted to pb) into the tonic stream. @@ -281,14 +294,27 @@ impl ManagerClient { endpoint }; - let mut client = pb::runtime_harness_client::RuntimeHarnessClient::connect(endpoint) - .await - .with_context(|| { - format!( - "report: could not connect to manager at {}", - self.manager_addr - ) - })?; + // NOTE: connect via `endpoint.connect()`, NOT `Client::connect(endpoint)`. + // The generated client's `connect` routes through + // `tonic::transport::Endpoint::new`, which for an https:// URI does: + // + // return me.tls_config(ClientTlsConfig::new().with_enabled_roots()); + // + // — silently REPLACING the TLS config set above with a default one. And + // `with_enabled_roots` discards its receiver, so with neither + // `tls-native-roots` nor `tls-webpki-roots` enabled the result is an + // EMPTY root store, no client identity, and no domain name. That is + // exactly what the guest saw: UnknownIssuer (no roots), "client auth + // requested but no cert available" (no identity), and verification + // against the URI's address (no domain). `Endpoint::connect` does not + // go through `Endpoint::new`, so the configured TLS survives. + let channel = endpoint.connect().await.with_context(|| { + format!( + "report: could not connect to manager at {}", + self.manager_addr + ) + })?; + let mut client = pb::runtime_harness_client::RuntimeHarnessClient::new(channel); // Convert internal HarnessReport → pb on the fly as the stream is // consumed. ReceiverStream + map keeps this allocation-light: no @@ -373,14 +399,27 @@ impl ManagerClient { endpoint }; - let mut client = pb::runtime_harness_client::RuntimeHarnessClient::connect(endpoint) - .await - .with_context(|| { - format!( - "vend_secret: could not connect to manager at {}", - self.manager_addr - ) - })?; + // NOTE: connect via `endpoint.connect()`, NOT `Client::connect(endpoint)`. + // The generated client's `connect` routes through + // `tonic::transport::Endpoint::new`, which for an https:// URI does: + // + // return me.tls_config(ClientTlsConfig::new().with_enabled_roots()); + // + // — silently REPLACING the TLS config set above with a default one. And + // `with_enabled_roots` discards its receiver, so with neither + // `tls-native-roots` nor `tls-webpki-roots` enabled the result is an + // EMPTY root store, no client identity, and no domain name. That is + // exactly what the guest saw: UnknownIssuer (no roots), "client auth + // requested but no cert available" (no identity), and verification + // against the URI's address (no domain). `Endpoint::connect` does not + // go through `Endpoint::new`, so the configured TLS survives. + let channel = endpoint.connect().await.with_context(|| { + format!( + "vend_secret: could not connect to manager at {}", + self.manager_addr + ) + })?; + let mut client = pb::runtime_harness_client::RuntimeHarnessClient::new(channel); let ttl_proto = if req.ttl_secs > 0 { Some(prost_types::Duration { diff --git a/services/harness/src/secrets.rs b/services/harness/src/secrets.rs index d43b7a7f..71ad826a 100644 --- a/services/harness/src/secrets.rs +++ b/services/harness/src/secrets.rs @@ -379,6 +379,44 @@ impl SecretCache { } /// Background refresh sweep — keeps active secrets ahead of expiry. + /// Vend every declared secret once, at boot. + /// + /// `refresh_loop` only renews entries ALREADY cached, and `get_or_vend` is + /// driven by the workload asking over the secrets socket — so nothing ever + /// populated the cache on its own. A VM with no workload (a boot or + /// verification VM) therefore never exercised the vend path at all, which + /// is precisely the security-critical part of the boot contract: the + /// per-VM client cert presented over mTLS and the manager's allowlist + /// check. Warming here makes that path run on every boot, and removes the + /// first-use stall for workloads that do ask. + /// + /// Best-effort by design: a secret that cannot be vended must not stop the + /// VM from booting — the workload gets the error when it asks. + /// + /// Note this does mean a declared-but-unused secret is held in guest memory + /// for the life of the VM. That is already true of any secret the workload + /// touches once, and declared secrets are by definition ones it may use. + pub async fn prefetch_declared(self: &Arc) { + let names: Vec = self.declared.iter().map(|e| e.key().clone()).collect(); + if names.is_empty() { + return; + } + let total = names.len(); + let mut ok = 0usize; + for name in names { + match self.get_or_vend(&name).await { + Ok(_) => ok += 1, + // Names only — never the value or the URI. + Err(e) => tracing::warn!(env = %name, error = %e, "secrets: prefetch failed"), + } + } + tracing::info!( + vended = ok, + declared = total, + "secrets: prefetched declared secrets" + ); + } + pub async fn refresh_loop(self: Arc) { let mut ticker = tokio::time::interval(Duration::from_secs(15)); loop { diff --git a/services/runtime-manager/src/backends/firecracker.rs b/services/runtime-manager/src/backends/firecracker.rs index 785db754..611d81d6 100644 --- a/services/runtime-manager/src/backends/firecracker.rs +++ b/services/runtime-manager/src/backends/firecracker.rs @@ -543,6 +543,25 @@ pub fn build_boot_args( let ip_param = net.kernel_ip_param(); let manager_addr = manager_addr_for_guest(net); + // Declared secrets, one key each. The harness needs these to know which + // secrets a workload may request — with none declared its cache refuses + // every request, so VendSecret is never reached and a spec that declares a + // secret behaves exactly like one that declares none. + // + // One key per secret rather than a JSON blob: the harness assembles the + // JSON itself. Kernel args are whitespace-separated and build_boot_args + // strips quotes from values, so serialised JSON would arrive mangled. + let declared_secrets: String = req + .secrets + .iter() + .filter(|s| !s.env_var.is_empty() && !s.vault_ref.is_empty()) + .map(|s| { + let env = s.env_var.replace([' ', '"', '\'', '='], "_"); + let uri = s.vault_ref.replace([' ', '"', '\''], "_"); + format!(" lantern.secret.{env}={uri}") + }) + .collect(); + format!( "console=ttyS0 reboot=k panic=1 pci=off \ {ip_param} \ @@ -551,7 +570,7 @@ pub fn build_boot_args( lantern.tls_cert={tls_cert_path} \ lantern.tls_key={tls_key_path} \ lantern.manager_ca={manager_ca_path} \ - lantern.env.LANTERN_MANAGER_ADDR={manager_addr}{extra_env}" + lantern.env.LANTERN_MANAGER_ADDR={manager_addr}{declared_secrets}{extra_env}" ) }