fix(sdk): enable TLS interception when modify adds a secret

[IIITManjeet]

Adding a secret through modify() persisted the binding but left network.tls.enabled false, so the guest's placeholder reached the upstream unsubstituted. The modification path now upholds the same secrets-require-TLS invariant that SandboxBuilder::secret_entry already enforced at create time.

Description

• Add ensure_tls_for_secrets in sdk/rust/lib/sandbox/config.rs, beside the existing network-config helpers. It is pub(crate), so this adds no public API; both call sites live in the same crate.
• Call it from apply_secret_patch_to_config in sdk/rust/lib/sandbox/modify.rs, so modify persists interception alongside the secret.
• Replace the inline flip in SandboxBuilder::secret_entry with the shared helper. No behavior change; it removes the duplication that let the two paths drift.
• Emit a tls change from push_secret_changes when a patch declares secrets and interception is off. Persisting the flag alone is not enough: a config that already carries secrets with interception off, which is the state this bug produced, can take a rotate or allowed-hosts update that classifies as live, and apply mirrors those into the active config. Flipping the flag there would make inspect report interception the running proxy does not have. The planner's emission condition matches the persist step exactly, so the plan cannot disagree with what is written.
• The new change routes through the existing spec_disposition rules, so it is restart-backed: cannot apply modification: tls requires restart under the default policy, an actual restart under restart, and next start when stopped. The CLI renders it through the existing free-form field path as tls requires restart: interception disabled -> interception enabled, so no CLI change is needed.
• The invariant is deliberately one-way. Removing the last secret leaves interception on, because TlsConfig records no provenance and callers enable it independently of secrets, for bypass and inspection policy or for DNS-over-TLS.
• Update docs/sandboxes/secrets.mdx and docs/sandboxes/tuning.mdx, including a tls row in the change-disposition table.

Compatibility

No format change. tls.enabled is a pre-existing field, and tls is a new value for the already free-form ConfigPlannedChange.field, which older CLIs render verbatim.

One behavior change is worth calling out: on configs already carrying secrets with interception off, a rotate or allowed-hosts update that previously classified as live now requires a restart. That live update never actually worked, since without interception nothing was substituted, but scripts calling msb modify --secret without --restart against such a sandbox will now get an explicit error instead of a silent no-op.

Out of scope

NetworkBuilder::build() (the .network(|n| n.secret(...)) create path) and SandboxBuilder::from_spec_json can still produce secrets with interception off. Auto-enabling there would change behavior for deliberately plain-HTTP secrets, since require_tls_identity: false is supported, so that seems like a maintainer call rather than something to fold into this fix. Happy to open a follow-up issue.

Test Plan

cargo fmt --all -- --check
cargo test -p microsandbox --lib sandbox::modify
cargo test -p microsandbox --lib ensure_tls_for_secrets
cargo test -p microsandbox-network --lib
cargo test -p microsandbox --no-default-features --features prebuilt
cargo clippy --workspace --all-targets -- -D warnings

New tests in sdk/rust/lib/sandbox/config.rs:

• ensure_tls_for_secrets_enables_interception_for_a_non_empty_set — flips once for a non-empty set, then reports no further change.
• ensure_tls_for_secrets_leaves_an_empty_set_alone — an empty set neither enables nor disables interception.

New tests in sdk/rust/lib/sandbox/modify.rs:

• adding_first_secret_enables_tls_in_durable_config — the direct regression for this issue.
• first_secret_plans_tls_change_and_forces_restart — covers all three policies, including that restart yields plan_requires_restart, which is what rebuilds the active config from the TLS-enabled durable one.
• live_secret_change_on_tls_disabled_config_requires_restart — covers the legacy secrets-without-TLS config and asserts the exact error.
• removing_last_secret_keeps_tls_enabled — locks the one-way asymmetry.
• secret_change_with_tls_already_enabled_plans_no_tls_change — guards against plan noise.

The config_with_secret test fixture was itself building a secret-without-TLS config, a shape no real entry point can now produce, so it now mirrors the invariant. A separate config_with_secret_and_tls_disabled fixture keeps the legacy shape available for the test that needs it.

Environment notes:

• Developed on Windows, so integration tests requiring Linux with KVM or macOS Apple Silicon were not run. The reporter's suggested assertion that the active config shows TLS after a restart needs such a test; the restart-forcing logic it depends on is covered by the unit tests above.
• cargo test --workspace reports 4 failures on this host, all pre-existing Windows path and platform assumptions unrelated to this change: test_create_local_validates_direct_config_mounts, test_runtime_name_validation_uses_explicit_backend_paths, test_sandbox_cli_args_flat_oci_attaches_one_raw_root_disk, and test_builder_rejects_remote_named_pipe_and_datagram. Verified identical on clean main (601 passed / 4 failed) and on this branch (606 passed / 4 failed, the +5 being the new modify.rs tests).

Fixes #1422

[greptile-apps]

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Secret modification patch] --> B[Build modification plan]
    B --> C{Secrets declared and TLS disabled?}
    C -- No --> D[Use normal secret disposition]
    C -- Yes --> E[Add restart-backed tls change]
    E --> F{Sandbox status and policy}
    F -- Running without restart --> G[Reject: TLS requires restart]
    F -- Running with restart --> H[Stop sandbox]
    F -- Stopped --> I[Apply on next start]
    H --> J[Apply secret patch to durable config]
    I --> J
    D --> J
    J --> K[Enable TLS when resulting secret set is non-empty]
    K --> L[Persist configuration]
    L --> M[Restart when required]

Loading

_{Reviews (1): Last reviewed commit: "fix(sdk): enable TLS interception when m..." | Re-trigger Greptile}

[greptile-apps]

Sequence Diagram

sequenceDiagram
    participant User
    participant Modify as modify()
    participant Planner
    participant Config as Durable config
    participant Runtime

    User->>Modify: Add or update secret
    Modify->>Planner: Build modification plan
    Planner->>Config: Check tls.enabled
    alt TLS interception is disabled
        Planner-->>Modify: Add restart-backed tls change
        Modify->>Config: Persist secret and enable TLS
        alt Sandbox is running with restart policy
            Modify->>Runtime: Restart using durable config
        else Sandbox is stopped or next-start policy
            Modify-->>Runtime: Apply on next start
        end
    else TLS interception is already enabled
        Planner-->>Modify: Plan secret change normally
        Modify->>Config: Persist secret update
    end

Loading

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into fix-modify-secr..." | Re-trigger Greptile}

[IIITManjeet]

Hey @appcypher can you please review this PR.
Thanks
Manjeet