tls: terminate policy field emits repeated deprecation warnings on every sandbox start instead of erroring or auto-migrating #3310

Activity

1. github-actions

   added

   state:triage-neededOpened without agent diagnostics and needs triageOpened without agent diagnostics and needs triage

   on Sep 14, 2026
2. johntmyers

   added

   state:acceptedA maintainer decided OpenShell should pursue this issueA maintainer decided OpenShell should pursue this issue

   and removed

   state:triage-neededOpened without agent diagnostics and needs triageOpened without agent diagnostics and needs triage

   on Sep 14, 2026
3. johntmyers

   added this to the OpenShell 0.1.0 milestone on Sep 14, 2026
4. johntmyers commented on Sep 14, 2026

   

   johntmyers

   on Sep 14, 2026

   Collaborator

   More actions

   We should just rip out any tls: terminate other values and not throw deprecation warnings. we currently support https://docs.nvidia.com/openshell/latest/security/best-practices#tls-handling. i'm not sure we need a migration command, we can just reject invalid tls values as part of 0.1.0
5. lunarwhite commented on Sep 15, 2026

   

   lunarwhite

   on Sep 15, 2026

   Contributor

   More actions

   I did some preliminary research around this, and noticed there is a sequencing issue:

   Rejecting tls: terminate also rejects the default policy baked into the community images at /etc/openshell/policy.yaml — 13 occurrences across base (3), droid (2), gemini (3), ollama (3), pi (2). Three test_sandbox_venv.py tests depend on that policy for their pypi rule, and a local check confirms the blast radius: mise run e2e:python gives 3 failed / 84 passed / 81 skipped, all three failures there, each a proxy 403 once disk-policy validation fails and discover_policy_from_path falls back to the restrictive default (crates/openshell-sandbox/src/lib.rs:2675-2698). The host reaches PyPI fine, so this is the rejection and not the environment.

   #3116 makes this disappear — a plain Alpine image ships no /etc/openshell/policy.yaml, so disk-policy discovery finds nothing — and it already names the baked policy as a dependency that must go. But #3236 is still a draft in conflict and touches no E2E tests.

   Land this issue behind #3116, or delete those 13 lines in the community repo as a stopgap, which is the suggested approach to proceed? The stopgap is throwaway work on an artifact we're retiring.
6. drew

   assigned

   lunarwhite

   on Sep 15, 2026
7. lunarwhite commented on Sep 17, 2026

   

   lunarwhite

   on Sep 17, 2026

   Contributor

   More actions

   🏗️ build-plan

   Implementation Plan

   Issue type: fix
   Complexity: Medium
   Confidence: High

   Summary

   tls: terminate and tls: passthrough are inert aliases for the default —
   parse_tls_mode already resolves both to Auto — so their only remaining
   effect is the MED-severity CONFIG:VALIDATION deprecation warning this issue
   reports. This closes the field to omitted or skip and deletes the warning
   machinery rather than upgrading it. Removing the value from any policy is
   exactly behavior-preserving.

   The check belongs in validate_l7_endpoint_semantics
   (crates/openshell-policy/src/l7_validate.rs), the nine-arm predicate all three
   policy validators already share so they cannot drift. As a tenth arm it reuses
   PolicyViolation::InvalidL7Endpoint, which carries the policy name and endpoint
   index, so no new violation variant is needed.

   Enforcement and data cleanup cannot land separately: this repo ships six
   tls: terminate lines in providers/aws-s3.yaml, compiled in via include_str!
   and unwrapped by builtin_profiles(), so the check alone would panic the
   gateway on first catalog access.

   Scope

   Enforcement

   • l7_validate.rs: add validate_tls_mode, call it as arm 10, and add the
     tls field to L7EndpointFields that the seam's three callers populate —
     validate_sandbox_policy (policy/src/lib.rs:1525, backing gateway admission
     and stored reads), validate_l7_policies (l7/mod.rs:1348, the supervisor's
     OPA-data ingress), and validate_profile_set (profiles.rs:2554).
   • policy_local.rs:1131: a fourth boundary that cannot use the seam.
     network_endpoint_from_json validates field-by-field before a
     NetworkEndpoint exists, so it calls validate_tls_mode directly. Without
     it an in-sandbox agent can author a value the gateway rejects.
   • l7/mod.rs: delete the deprecation arms in parse_l7_config (:300-326),
     which lets it delegate to parse_tls_mode, plus the warning at :1481 and
     the stale comments at :463 and :1501-1503. parse_tls_mode's lenient
     _ => Auto stays: Auto inspects, so an unvalidated path still fails closed.

   Data this repo ships

   • Three SPIFFE example profiles carry tls: none and are already invalid
     today, silently: validate_l7_policies only warns on the two named
     spellings.
   • Two e2e fixtures and roughly fifteen test fixtures drop an incidental value.

   Dead code the change creates

   • merge.rs:804-809: effective_tls's "auto" sentinel exists only to equate
     the legacy spellings with the default. Its caller (:700) is guarded by
     !proposed.tls.is_empty(), so once they are rejected it reduces to
     loaded.tls != proposed.tls. Delete it; effective_enforcement below stays
     load-bearing.
   • openshell-tui/src/lib.rs:1316,1330-1332: the unreachable "TLS terminate"
     annotation.

   Implementation Steps

   One commit — see the Summary for why it cannot be split.

   1. Strip the values from shipped data, fixtures, docs, and the skill.
   2. Add validate_tls_mode and arm 10; populate tls at the three seam callers.
   3. Wire the direct call in network_endpoint_from_json.
   4. Delete the deprecation machinery and the two dead paths.

   Test Plan

   • Unit tests: l7_validate.rs — omitted and skip accepted; terminate,
     passthrough, and an arbitrary value rejected with a message naming the value
     and the fix. One rejection test per boundary: lib.rs, profiles.rs
     (diagnostic at endpoints[0]), policy_local.rs. l7/mod.rs: replace the
     "terminate is deprecated and treated as Auto" test with the issue's acceptance
     criterion — an error naming the endpoint and no surviving deprecation
     warning. merge.rs: rework the coverage test to assert a legacy spelling now
     fails closed.
   • Integration tests: grpc/validation.rs returns InvalidArgument;
     opa.rs fails policy load with an error locating the endpoint — the path that
     previously only warned.
   • E2E tests: run the suite per the AGENTS.md sandbox-infrastructure rule. No
     new case; the rejection is pure validation already covered at all four
     boundaries. Grep the output including in-sandbox OCSF logs for
     unsupported tls value, CONFIG:VALIDATION, and deprecat — all three
     absent is the issue's acceptance criterion.

   Risks & Open Questions

   • Stored policies fail on read with FAILED_PRECONDITION.
     openshell policy set --global recovers, because it validates only the
     incoming policy. openshell policy delete --global does not:
     DeleteGlobalPolicySetting calls provider_policy_composition_enabled_in
     (grpc/policy.rs:3603) unconditionally, one line before removing the key, and
     that helper decodes the stored policy (:2218-2220) and propagates the error.
     Pre-existing and not specific to tls, so filed separately — but the
     migration note must point at set, not delete.
   • Silent normalization on stored reads was considered and rejected. It would
     make the upgrade invisible, but it rewrites an operator's stored policy
     without consent and hides that the policy was authored against a dead
     contract. Nothing self-heals here, by design.
   • No cross-repo dependency, despite appearances. The community images still
     ship tls: terminate — 13 occurrences across five images, and the base
     image's three match this report exactly. They are never read: RFC-0012 put the
     supervisor in its own container, and Dockerfile.supervisor is
     debian:bookworm-slim plus the binary with readonly_rootfs and no workload
     mount, so discover_policy_from_disk_or_default finds no file on any path.
     The in-repo aws-s3.yaml values do reach the supervisor, because
     compose_effective_policy inserts provider layers into the delivered policy.
     Deleting the emitters therefore fixes the symptom whatever its source.
     Cleaning the community repo is hygiene, or moot if
     #3116 retires those images.
   • Case canonicalization is out of scope. tls has no canonicalization step
     unlike protocol, and the sites comparing it disagree. tls: SKIP is
     rejected by the credential gate (grpc/policy.rs:3154, case-insensitive);
     after the documented allow_uninspected_credentials: true remedy,
     parse_tls_mode (l7/mod.rs:460-462, exact match) returns Auto and
     terminates TLS into a tunnel the operator declared uninspectable, while
     middleware.rs:271 never raises MiddlewareTlsSkipConflict. A real
     pre-existing defect, but fixing it here means either a second class of
     newly-invalid input or a behavior change for a spelling this issue never
     mentions.
   • The break is in-window. rfc/0014-release-stability/README.md:83-90 — the
     compatibility contract begins at 0.1.0, and :218-220 designates the
     pre-0.1.0 window for exactly this. Confirm no ! marker is wanted, since it
     would request a minor.
   • No LSM impact: policy-string validation in userspace, with no /proc access,
     process identity resolution, or exec.

   Documentation Impact

   • docs/reference/policy-schema.mdx:197 publishes "still accepted for backward
     compatibility", which becomes false.
   • docs/about/release-notes.mdx — a 0.1.0 migration entry: the closed set, the
     behavior-preserving removal, a warning against substituting tls: skip, and
     the stored-policy failure with its working recovery command.
   • skills/generate-sandbox-policy/SKILL.md tells agents tls: terminate is
     required for HTTPS plus L7 in seven places (:204, :208, :272, :376,
     :387, :554, :570), already contradicting examples.md:7-11. The
     decision table and "Critical" paragraph should collapse to prose rather than
     be edited in place; the decision is binary once the field is closed, on a
     surface paid every invocation.
   • examples/sandbox-policy-quickstart/README.md — two occurrences.
   • architecture/security-policy.md — the closed-set invariant, beside the
     existing L4/L7 transport discussion.
   • proto/sandbox.proto:116 omits skip even today; comment-only, with
     sdk/go/proto/sandboxv1/sandbox.pb.go regenerating.
   • Not required: docs/reference/gateway-config.mdx, since tls is a policy
     field rather than gateway TOML. docs/security/best-practices.mdx and
     docs/sandboxes/policies.mdx mention only skip and are already correct.

   ---

   Revision 1 — initial plan
8. lunarwhite commented on Sep 17, 2026

   

   lunarwhite

   on Sep 17, 2026

   Contributor

   More actions

   PR is ready for review: #3414
9. johntmyers

   closed this as completedin #3414on Sep 22, 2026