diff --git a/docs/sandboxes/secrets.mdx b/docs/sandboxes/secrets.mdx index 4c7e468b2..fb2f3d166 100644 --- a/docs/sandboxes/secrets.mdx +++ b/docs/sandboxes/secrets.mdx @@ -107,6 +107,8 @@ In the CLI form, `ENV@HOST[,HOST...]` records a host-side source reference: the Local-only Rotate or remove existing secrets without a restart. Adding a secret or changing its guest-visible placeholder requires a restart so the new environment reaches the guest. Later rotations keep that placeholder stable and only change the value injected at the network boundary. +Adding a secret to a sandbox that has TLS interception off turns it on, the same way `secret` does at create time — secrets are substituted by the TLS proxy, so interception is what makes them work at all. Interception cannot start on a running sandbox, so that change is restart-backed too and appears in the plan as `tls`. Removing every secret leaves interception on, since it may have been enabled for reasons of its own. + ```rust Rust let plan = sb.modify() diff --git a/docs/sandboxes/tuning.mdx b/docs/sandboxes/tuning.mdx index a7be1ce6f..a2c13c39f 100644 --- a/docs/sandboxes/tuning.mdx +++ b/docs/sandboxes/tuning.mdx @@ -116,6 +116,7 @@ The plan labels each change as `live`, `next start`, `requires restart`, or `uns | `labels` | Live | Host-side metadata; no guest process changes | | `env`, `workdir` | Future execs | Running processes keep what they already have | | `secrets` | Live for rotation | Placeholder changes need a restart | +| `tls` | Restart or next start | Configuring a secret turns interception on, and interception cannot start on a running sandbox | | Root disk size | Restart or next start | Managed and flat OCI disks grow only; tmpfs changes on next boot | | Other storage | Create or mount time | Named volumes, mount tmpfs, and user disk images are sized outside `modify` | @@ -332,6 +333,8 @@ msb modify worker --secret-rm OLD_TOKEN ``` +Configuring a secret on a sandbox with TLS interception off also turns it on, since the proxy is what substitutes the value. That shows up in the plan as a `tls` change and needs a restart or the next start. Removing every secret leaves interception on. + For the credential model and host allow lists, see [Secrets](/sandboxes/secrets). ## Storage diff --git a/sdk/rust/lib/sandbox/builder.rs b/sdk/rust/lib/sandbox/builder.rs index 83e26d99d..c765ae922 100644 --- a/sdk/rust/lib/sandbox/builder.rs +++ b/sdk/rust/lib/sandbox/builder.rs @@ -861,9 +861,7 @@ impl SandboxBuilder { match self.config.local_network_config() { Ok(mut network) => { network.secrets.secrets.push(entry); - if !network.tls.enabled { - network.tls.enabled = true; - } + super::config::ensure_tls_for_secrets(&mut network); if let Err(err) = self.config.set_local_network_config(network) && self.build_error.is_none() { diff --git a/sdk/rust/lib/sandbox/config.rs b/sdk/rust/lib/sandbox/config.rs index c45fcecfa..68d4327f6 100644 --- a/sdk/rust/lib/sandbox/config.rs +++ b/sdk/rust/lib/sandbox/config.rs @@ -583,6 +583,25 @@ pub(crate) fn network_config_from_spec( Ok(serde_json::from_value(serde_json::to_value(spec)?)?) } +/// Enforce the invariant that a non-empty secret set requires TLS +/// interception, returning whether `tls.enabled` had to be flipped. The +/// proxy is what substitutes secrets, so without interception the +/// placeholder reaches the upstream unchanged. +/// +/// One-way: emptying the secret set leaves interception on, since +/// `TlsConfig::enabled` records no provenance and callers enable it +/// independently of secrets. +#[cfg(feature = "net")] +pub(crate) fn ensure_tls_for_secrets( + network: &mut microsandbox_network::config::NetworkConfig, +) -> bool { + if network.secrets.secrets.is_empty() || network.tls.enabled { + return false; + } + network.tls.enabled = true; + true +} + #[cfg(feature = "net")] impl SandboxConfig { pub(crate) fn local_network_config( @@ -1739,6 +1758,42 @@ mod tests { //---------------------------------------------------------------------------------------------- // Tests: Secret source references (create path + spawn resolution) //---------------------------------------------------------------------------------------------- + #[cfg(feature = "net")] + #[test] + fn ensure_tls_for_secrets_enables_interception_for_a_non_empty_set() { + use microsandbox_network::secrets::config::{HostPattern, SecretEntry}; + + let mut network = microsandbox_network::config::NetworkConfig::default(); + assert!(!network.tls.enabled); + + network.secrets.secrets.push(SecretEntry { + env_var: "API_KEY".into(), + value: zeroize::Zeroizing::new(String::new()), + source: None, + placeholder: "$MSB_API_KEY".into(), + allowed_hosts: vec![HostPattern::Exact("api.example.com".into())], + injection: Default::default(), + on_violation: None, + require_tls_identity: true, + }); + assert!(super::ensure_tls_for_secrets(&mut network)); + assert!(network.tls.enabled); + assert!(!super::ensure_tls_for_secrets(&mut network)); + } + + /// One-way: an empty set never enables interception, and never disables + /// interception enabled for other reasons. + #[cfg(feature = "net")] + #[test] + fn ensure_tls_for_secrets_leaves_an_empty_set_alone() { + let mut network = microsandbox_network::config::NetworkConfig::default(); + assert!(!super::ensure_tls_for_secrets(&mut network)); + assert!(!network.tls.enabled); + + network.tls.enabled = true; + assert!(!super::ensure_tls_for_secrets(&mut network)); + assert!(network.tls.enabled); + } #[cfg(feature = "net")] const SECRET_SENTINEL: &str = "sentinel-secret-value"; diff --git a/sdk/rust/lib/sandbox/modify.rs b/sdk/rust/lib/sandbox/modify.rs index c46676f3c..916de662b 100644 --- a/sdk/rust/lib/sandbox/modify.rs +++ b/sdk/rust/lib/sandbox/modify.rs @@ -42,6 +42,9 @@ const FUTURE_EXECS_ONLY: &str = const SECRETS_UNAVAILABLE_WITHOUT_NET: &str = "secret modification requires a build with the net feature"; const SECRET_FIELD: &str = "secret"; +const TLS_FIELD: &str = "tls"; +const TLS_INTERCEPTION_REQUIRES_RESTART: &str = + "secrets require TLS interception, which cannot be enabled on a running sandbox"; const ROOT_DISK_FIELD: &str = "root_disk_size"; const ENV_FIELD: &str = "env"; const LABEL_FIELD: &str = "label"; @@ -294,6 +297,12 @@ impl SandboxModificationBuilder { /// socket; the durable config records host-side source references for /// source-based specs and persists the value for value-based specs (the /// same at-rest property as create's `secret_env`). + /// + /// Configuring a secret on a sandbox with TLS interception off also + /// turns interception on, matching create's `secret` — secrets are + /// substituted by the TLS proxy, so they do nothing without it. That is + /// planned as a `tls` change and, like every other restart-backed change, + /// needs `restart` or `next_start` on a running sandbox. pub async fn apply(self) -> MicrosandboxResult { let handle = self .backend @@ -967,6 +976,10 @@ fn apply_secret_patch_to_config( .secrets .secrets .retain(|entry| !patch.secrets_remove.contains(&entry.env_var)); + // Same invariant create upholds in `SandboxBuilder::secret_entry`. The + // planner emits a matching `tls` change under the same condition, so the + // plan cannot disagree with what is persisted. + super::config::ensure_tls_for_secrets(&mut network); // Enforce env-var and placeholder shape rules before anything persists; // validation errors carry entry indexes and sizes, never values. network.secrets.validate().map_err(|err| { @@ -1657,6 +1670,22 @@ fn push_secret_changes( reason, })); } + + // Surface the implied TLS enable as its own change rather than flipping + // a config field invisibly: it keeps the dry-run honest and routes the + // patch through the restart path, the only way interception can start. + // Removal-only patches enable nothing, so they emit nothing. + if !patch.secrets.is_empty() && !tls_interception_enabled(config) { + changes.push(spec_change( + TLS_FIELD, + ChangeKind::Updated, + Some("interception disabled".to_string()), + Some("interception enabled".to_string()), + status, + policy, + TLS_INTERCEPTION_REQUIRES_RESTART, + )); + } } /// Infer what a declarative secret spec changes by diffing it against the @@ -2237,6 +2266,23 @@ fn existing_secret_from_network_config( None } +/// Whether the config already has TLS interception on. An unreadable network +/// config counts as disabled: a redundant `tls` change beats a secret landing +/// without interception. +#[cfg(feature = "net")] +fn tls_interception_enabled(config: &SandboxConfig) -> bool { + config + .local_network_config() + .map(|network| network.tls.enabled) + .unwrap_or(false) +} + +/// Without `net`, secret changes already plan as unsupported; no `tls` on top. +#[cfg(not(feature = "net"))] +fn tls_interception_enabled(_config: &SandboxConfig) -> bool { + true +} + #[cfg(feature = "net")] fn format_host_pattern(host: microsandbox_network::secrets::config::HostPattern) -> String { match host { @@ -3438,6 +3484,18 @@ mod tests { on_violation: None, require_tls_identity: true, }); + // Mirror the invariant every real entry point upholds. + crate::sandbox::config::ensure_tls_for_secrets(&mut network); + config.set_local_network_config(network).unwrap(); + config + } + + /// The pre-fix shape this bug used to persist: a secret with TLS off. + #[cfg(feature = "net")] + fn config_with_secret_and_tls_disabled(name: &str, value: &str) -> SandboxConfig { + let mut config = config_with_secret(name, value); + let mut network = config.local_network_config().unwrap(); + network.tls.enabled = false; config.set_local_network_config(network).unwrap(); config } @@ -3474,6 +3532,15 @@ mod tests { } } + /// The `tls` change a secret patch emits when it must turn interception on. + #[cfg(feature = "net")] + fn tls_plan_change(plan: &SandboxModificationPlan) -> Option<&ConfigPlannedChange> { + plan.changes.iter().find_map(|change| match change { + PlannedChange::Config(change) if change.field == TLS_FIELD => Some(change), + _ => None, + }) + } + #[cfg(feature = "net")] fn secret_plan_dispositions(plan: &SandboxModificationPlan) -> Vec { plan.changes @@ -3851,6 +3918,167 @@ mod tests { assert!(conflicts[0].message.contains("needs a name")); } + /// Regression for #1422: the first secret left `tls.enabled` false, so the + /// placeholder reached the upstream unsubstituted. + #[cfg(feature = "net")] + #[test] + fn adding_first_secret_enables_tls_in_durable_config() { + let mut config = config(2, 1024); + assert!(!config.local_network_config().unwrap().tls.enabled); + + let patch = patch_with_specs(vec![source_spec("API_KEY", &["api.example.com"])]); + apply_secret_patch_to_config(&mut config, &patch).unwrap(); + + let network = config.local_network_config().unwrap(); + assert_eq!(network.secrets.secrets.len(), 1); + assert!(network.tls.enabled); + } + + /// Interception is restart-backed, so the first secret must show up in the + /// plan and drive the restart rather than flip silently under a running VM. + #[cfg(feature = "net")] + #[test] + fn first_secret_plans_tls_change_and_forces_restart() { + let config = config(2, 1024); + let patch = patch_with_specs(vec![source_spec("API_KEY", &["api.example.com"])]); + + // Stopped: lands on the next start. + let plan = build_plan( + "api".to_string(), + SandboxStatus::Stopped, + &config, + None, + LiveControl::default(), + patch.clone(), + ModificationPolicy::NoRestart, + ); + let tls = tls_plan_change(&plan).expect("expected a tls change"); + assert_eq!(tls.change, ChangeKind::Updated); + assert_eq!(tls.disposition, ModificationDisposition::NextStart); + assert!(validate_apply_supported(&plan).is_ok()); + + // Running under the default policy: an explicit error. + let plan = build_plan( + "api".to_string(), + SandboxStatus::Running, + &config, + None, + LiveControl { + resize: false, + secrets: true, + }, + patch.clone(), + ModificationPolicy::NoRestart, + ); + assert_eq!( + tls_plan_change(&plan).unwrap().disposition, + ModificationDisposition::RequiresRestart + ); + assert!(validate_apply_supported(&plan).is_err()); + + // Restart opted in: the restart rebuilds active from the durable config. + let plan = build_plan( + "api".to_string(), + SandboxStatus::Running, + &config, + None, + LiveControl { + resize: false, + secrets: true, + }, + patch, + ModificationPolicy::Restart, + ); + assert!(validate_apply_supported(&plan).is_ok()); + assert!(plan_requires_restart(&plan)); + } + + /// Rotating a secret on a legacy TLS-off config classifies as live, and + /// mirroring it would claim TLS the running proxy does not have. + #[cfg(feature = "net")] + #[test] + fn live_secret_change_on_tls_disabled_config_requires_restart() { + let config = config_with_secret_and_tls_disabled("API_KEY", SECRET_SENTINEL); + let patch = patch_with_specs(vec![source_spec("API_KEY", &[])]); + + let plan = build_plan( + "api".to_string(), + SandboxStatus::Running, + &config, + None, + LiveControl { + resize: false, + secrets: true, + }, + patch, + ModificationPolicy::NoRestart, + ); + + // The rotate itself is live-applicable, so tls is the only blocker. + let secret_dispositions: Vec<_> = plan + .changes + .iter() + .filter_map(|change| match change { + PlannedChange::Secret(change) => Some(change.disposition), + PlannedChange::Config(_) => None, + }) + .collect(); + assert_eq!(secret_dispositions, vec![ModificationDisposition::Live]); + assert_eq!( + tls_plan_change(&plan).unwrap().disposition, + ModificationDisposition::RequiresRestart + ); + let err = validate_apply_supported(&plan).unwrap_err().to_string(); + assert_eq!(err, "cannot apply modification: tls requires restart"); + } + + /// One-way: emptying the secret set must not turn interception off. + #[cfg(feature = "net")] + #[test] + fn removing_last_secret_keeps_tls_enabled() { + let mut config = config_with_secret("API_KEY", SECRET_SENTINEL); + let patch = SandboxModificationPatch { + secrets_remove: vec!["API_KEY".to_string()], + ..SandboxModificationPatch::default() + }; + + let plan = build_plan( + "api".to_string(), + SandboxStatus::Stopped, + &config, + None, + LiveControl::default(), + patch.clone(), + ModificationPolicy::NoRestart, + ); + assert!(tls_plan_change(&plan).is_none()); + + apply_secret_patch_to_config(&mut config, &patch).unwrap(); + + let network = config.local_network_config().unwrap(); + assert!(network.secrets.secrets.is_empty()); + assert!(network.tls.enabled); + } + + /// Interception already on: nothing to enable, so no extra plan noise. + #[cfg(feature = "net")] + #[test] + fn secret_change_with_tls_already_enabled_plans_no_tls_change() { + let config = config_with_secret("API_KEY", SECRET_SENTINEL); + let patch = patch_with_specs(vec![source_spec("API_KEY", &[])]); + + let plan = build_plan( + "api".to_string(), + SandboxStatus::Stopped, + &config, + None, + LiveControl::default(), + patch, + ModificationPolicy::NoRestart, + ); + + assert!(tls_plan_change(&plan).is_none()); + } #[cfg(feature = "net")] #[test] fn applying_new_source_spec_uses_create_placeholder_default() {