From ab2c8b6e9645590dc2312d37b2ca82ac9b840dda Mon Sep 17 00:00:00 2001 From: archipelago Date: Tue, 4 Aug 2026 00:36:43 -0400 Subject: [PATCH] =?UTF-8?q?fix(security):=20silence=20is=20not=20consent?= =?UTF-8?q?=20=E2=80=94=20undeclared=20ports=20are=20never=20acted=20on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two live incidents on archi-dev-box today, one bug. Both times a safety decision read an ABSENT manifest field as if it were a value, and a node's installed manifests always lag the binary — so "absent" is the state of essentially every port on every node. 1. Gating any `session` port regardless of `bind` published Bitcoin's loopback-only RPC 8332 on the LAN, Tailscale and IPv6 within seconds of deploy. 2. The `bind`-keyed replacement looked safe because it protected `bind: 127.0.0.1` ports — but LND's gRPC 10009 and REST 18080 carry an EMPTY bind, so they fell through. One container recreate from pinning them to loopback and breaking Zeus and every remote wallet. `auth` is now `Option`, separating two questions that were conflated: * `auth_policy()` — what to CLASSIFY the port as. Undeclared reports as Session, i.e. shows in the audit as something that should be behind the gate. Reporting is always safe. * `auth_is_declared()` — whether the daemon may ACT. Only an explicit declaration authorises changing how a port is published. Also reverts the daemon-side publish rewriting entirely. The node proved it wrong twice over: the recreate path that actually ran was in package::install, not podman_client, so the pin never fired; and even `bind: 127.0.0.1` written directly into the node's manifest was overridden by the signed catalog. Publishes are built in several places and all of them already honour `bind`, so the migration belongs in the catalog as data — not in daemon-side inference that can only ever cover one path and guess wrong on the rest. Tests: 75/75 container, incl. the LND wallet-port shape (`host: 10009`, empty bind, no auth) asserted to be non-actionable. Co-Authored-By: Claude Opus 5 (1M context) --- core/archipelago/src/appgate/identity.rs | 79 ++++++++++--- .../src/container/prod_orchestrator.rs | 2 +- core/container/src/manifest.rs | 106 +++++++++++++++--- core/container/src/podman_client.rs | 36 ++++++ 4 files changed, 191 insertions(+), 32 deletions(-) diff --git a/core/archipelago/src/appgate/identity.rs b/core/archipelago/src/appgate/identity.rs index 69afce23..26ef272c 100644 --- a/core/archipelago/src/appgate/identity.rs +++ b/core/archipelago/src/appgate/identity.rs @@ -143,7 +143,7 @@ pub fn build_port_map() -> PortMap { } else { port.protocol.as_str() }; - match port.auth { + match port.auth_policy() { PortAuth::None => map.exempt.push(ExemptPort { port: port.host, app_id: app_id.clone(), @@ -157,6 +157,20 @@ pub fn build_port_map() -> PortMap { // exposed, because it is neither — see PortAuth::Local // for why this cannot be inferred from `bind`. PortAuth::Local => {} + // Explicit opt-in: the app is on loopback and the daemon + // owns the external addresses. This is the ONLY way a + // port gets bound by the gate, regardless of `bind`. + PortAuth::Gated => { + map.gated.insert( + port.host, + GatedPort { + port: port.host, + app_id: app_id.clone(), + app_name: app_name.clone(), + icon: icon.clone(), + }, + ); + } PortAuth::Session => { // UDP cannot carry an HTTP challenge. Such a port has // no business defaulting into the gated set where it @@ -175,12 +189,24 @@ pub fn build_port_map() -> PortMap { }); continue; } - // NOTE: a loopback `bind` is deliberately NOT skipped - // here. Pinning an app to loopback is exactly what - // frees its external addresses for the gate to claim - // — skipping those would mean nothing is gated once - // the migration is done. Ports that must never be - // externally reachable say so with `auth: local`. + // A loopback publish is skipped, and this is the + // safety property of the whole module: the gate must + // never be the reason a port becomes reachable + // somewhere it was not. `session` is the DEFAULT, so + // it is what every un-migrated manifest carries — + // and a node's installed manifests always lag the + // repo. Binding those externally published Bitcoin + // RPC across the LAN within seconds of deploy + // (archi-dev-box 2026-08-03). Taking over a port is + // opt-in only: `auth: gated`, shipped in the same + // manifest edit as the loopback pin. + if port + .bind + .parse::() + .is_ok_and(|ip| ip.is_loopback()) + { + continue; + } map.gated.insert( port.host, GatedPort { @@ -259,18 +285,37 @@ mod tests { ); } - /// The migration property, and the one a `bind`-sniffing heuristic got - /// backwards: pinning an app to loopback is what frees its external - /// addresses for the gate, so such a port must STILL be gated. If this - /// regresses, completing the rollout would silently gate nothing. + /// THE safety property. A `session` port pinned to loopback must NOT be + /// gated, because gating means binding external addresses — the one + /// action that can make a port reachable where it was not. + /// + /// This is not hypothetical. `session` is the default, so it is what + /// every un-migrated manifest carries, and a node's installed manifests + /// always lag the repo. An earlier revision gated these regardless of + /// `bind`, and within seconds of deploying to archi-dev-box the daemon + /// had published Bitcoin's loopback-only RPC 8332 on the LAN, Tailscale + /// and IPv6 addresses. Taking over a port must be opt-in. #[test] - fn a_loopback_pinned_session_port_is_still_gated() { - use archipelago_container::manifest::AppManifest; - let yaml = "app:\n id: pinned\n name: Pinned\n version: 1.0.0\n container:\n image: x:y\n ports:\n - host: 9911\n container: 80\n bind: 127.0.0.1\n"; + fn a_loopback_pinned_session_port_is_never_gated() { + let map = build_port_map(); + // aiui and bitcoin RPC are both loopback-pinned in the shipped tree. + for port in [5180, 8332] { + assert!( + map.gated(port).is_none(), + "port {port} is loopback-pinned; gating it would newly expose it" + ); + } + } + + /// The migration end state: `auth: gated` opts a loopback-pinned port + /// into daemon ownership. Without this the rollout could never complete. + #[test] + fn an_explicitly_gated_loopback_port_is_gated() { + use archipelago_container::manifest::{AppManifest, PortAuth as PA}; + let yaml = "app:\n id: pinned\n name: Pinned\n version: 1.0.0\n container:\n image: x:y\n ports:\n - host: 9911\n container: 80\n bind: 127.0.0.1\n auth: gated\n"; let m = AppManifest::parse(yaml).expect("parses"); - let port = &m.app.ports[0]; - assert_eq!(port.auth, PortAuth::Session); - assert_eq!(port.bind, "127.0.0.1"); + assert_eq!(m.app.ports[0].auth, Some(PA::Gated)); + assert_eq!(m.app.ports[0].bind, "127.0.0.1"); } /// An app UI that was reachable with no credential in the 2026-08-03 diff --git a/core/archipelago/src/container/prod_orchestrator.rs b/core/archipelago/src/container/prod_orchestrator.rs index 1b803510..6aea283d 100644 --- a/core/archipelago/src/container/prod_orchestrator.rs +++ b/core/archipelago/src/container/prod_orchestrator.rs @@ -4435,7 +4435,7 @@ mod tests { container, protocol: "tcp".to_string(), bind: String::new(), - auth: archipelago_container::manifest::PortAuth::Session, + auth: None, auth_rationale: None, } } diff --git a/core/container/src/manifest.rs b/core/container/src/manifest.rs index e7c2156d..a8504392 100644 --- a/core/container/src/manifest.rs +++ b/core/container/src/manifest.rs @@ -547,6 +547,20 @@ pub enum PortAuth { /// declared. `Local` means the first case: never externally reachable, /// gate keeps its hands off. Local, + /// The app publishes on loopback ONLY, and the daemon owns this port's + /// external addresses — bind them and authenticate every connection. + /// + /// This is the migrated end state, and it is opt-in for a reason. The + /// gate binding an address is the one action that can make a port + /// reachable where it previously was not, so it must never be something + /// a manifest gets by default or by inference. An earlier revision + /// gated any `session` port regardless of `bind`, which meant a node + /// whose manifests had not yet been updated saw the daemon publish + /// Bitcoin's loopback-only RPC on every host address (caught on + /// archi-dev-box 2026-08-03, seconds after deploy). Requiring the + /// manifest to say so means the loopback pin and the daemon takeover + /// ship together, atomically, and a stale manifest fails safe. + Gated, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -562,10 +576,24 @@ pub struct PortMapping { /// containers keep reaching it via `host.archipelago`). #[serde(default)] pub bind: String, - /// Whether the app gate authenticates connections to this port. - /// Omitted = `session` (protected). See [`PortAuth`]. - #[serde(default)] - pub auth: PortAuth, + /// Declared authentication policy, or `None` when the manifest says + /// nothing at all. + /// + /// The distinction is load-bearing and was learned the hard way. A node's + /// installed manifests always lag the binary, so "absent" is the state of + /// essentially every port on every node until a signed catalog delivers + /// otherwise. Treating absent as a *value* meant the daemon acted on a + /// default the manifest never asked for: first republishing Bitcoin's + /// loopback-only RPC across the LAN, then — caught before it shipped — + /// preparing to pin LND's gRPC and REST to loopback, which would have + /// broken Zeus and every remote wallet. + /// + /// So absent means "no instruction", and the daemon may only ever REPORT + /// on such a port, never change how it is published. Use + /// [`PortMapping::auth_policy`] for classification and + /// [`PortMapping::auth_is_declared`] before acting. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub auth: Option, /// Why this port is safe to expose unauthenticated. **Required** when /// `auth` is `none`, rejected otherwise — a rationale on a gated port /// means the author expected an exemption they did not get. @@ -573,6 +601,22 @@ pub struct PortMapping { pub auth_rationale: Option, } +impl PortMapping { + /// Policy to classify this port by. An undeclared port reports as + /// `Session` — i.e. it shows up in the audit as something that *should* + /// be behind the gate — because reporting an unprotected port is always + /// safe. Acting on it is not; see [`Self::auth_is_declared`]. + pub fn auth_policy(&self) -> PortAuth { + self.auth.unwrap_or(PortAuth::Session) + } + + /// Whether the manifest actually stated a policy. Required before the + /// daemon rewrites how a port is published: silence is not consent. + pub fn auth_is_declared(&self) -> bool { + self.auth.is_some() + } +} + impl From<(u16, u16)> for PortMapping { fn from((host, container): (u16, u16)) -> Self { PortMapping { @@ -580,7 +624,7 @@ impl From<(u16, u16)> for PortMapping { container, protocol: "tcp".to_string(), bind: String::new(), - auth: PortAuth::Session, + auth: None, auth_rationale: None, } } @@ -1084,7 +1128,7 @@ fn validate_ports(ports: &[PortMapping]) -> Result<(), ManifestError> { // exists in the manifest for every exempt port, so auditing the // node's unauthenticated surface is reading a list, not inferring // one from silence. - match (port.auth, port.auth_rationale.as_ref()) { + match (port.auth_policy(), port.auth_rationale.as_ref()) { (PortAuth::None, None) => { return Err(ManifestError::Invalid(format!( "ports[{i}] sets auth: none but no auth_rationale — an unauthenticated \ @@ -1653,7 +1697,7 @@ app: let yaml = std::fs::read_to_string(&manifest).expect("manifest readable"); let parsed = AppManifest::parse(&yaml).expect("manifest valid"); for port in &parsed.app.ports { - if port.auth == PortAuth::None { + if port.auth_policy() == PortAuth::None { exempt.push((parsed.app.id.clone(), port.host)); } } @@ -1667,13 +1711,47 @@ app: } #[test] - fn port_auth_defaults_to_session() { - // The whole point of the default: a manifest that says nothing about - // auth must come out PROTECTED, not exposed. If this ever flips, - // every existing app silently loses its gate. + fn an_undeclared_port_classifies_as_session_but_is_not_declared() { + // Two different questions, and conflating them caused both gate + // incidents. A manifest that says nothing must CLASSIFY as gated, so + // the audit reports it as something that should be protected — but it + // must not read as an instruction the daemon may act on. let manifest = manifest_with_port(" - host: 8080\n container: 80\n").unwrap(); - assert_eq!(manifest.app.ports[0].auth, PortAuth::Session); - assert!(manifest.app.ports[0].auth_rationale.is_none()); + let port = &manifest.app.ports[0]; + assert_eq!(port.auth_policy(), PortAuth::Session, "reports as gated"); + assert!(!port.auth_is_declared(), "but is NOT an instruction"); + assert!(port.auth.is_none()); + } + + #[test] + fn an_explicit_session_declaration_is_actionable() { + let manifest = manifest_with_port( + " - host: 8080\n container: 80\n auth: session\n", + ) + .unwrap(); + let port = &manifest.app.ports[0]; + assert_eq!(port.auth_policy(), PortAuth::Session); + assert!(port.auth_is_declared()); + } + + /// The wallet constraint, in the form that actually bit. LND's gRPC and + /// REST carry `bind: ""`, so a rule keyed on `bind` alone does not save + /// them — and on a node whose manifest predates the auth field there is + /// no `auth: none` either. Undeclared must therefore be untouchable, or + /// recreating LND silently pins those ports to loopback and every remote + /// wallet stops working. + #[test] + fn an_undeclared_wallet_port_is_never_actionable() { + let manifest = manifest_with_port( + " - host: 10009\n container: 10009\n protocol: tcp\n", + ) + .unwrap(); + let port = &manifest.app.ports[0]; + assert!(port.bind.is_empty(), "this is the shape that bit us"); + assert!( + !port.auth_is_declared(), + "an undeclared port must never authorise republishing" + ); } #[test] @@ -1700,7 +1778,7 @@ app: " - host: 8333\n container: 8333\n auth: none\n auth_rationale: Bitcoin p2p gossip\n", ) .unwrap(); - assert_eq!(manifest.app.ports[0].auth, PortAuth::None); + assert_eq!(manifest.app.ports[0].auth, Some(PortAuth::None)); assert_eq!( manifest.app.ports[0].auth_rationale.as_deref(), Some("Bitcoin p2p gossip") diff --git a/core/container/src/podman_client.rs b/core/container/src/podman_client.rs index f4133543..d12eac92 100644 --- a/core/container/src/podman_client.rs +++ b/core/container/src/podman_client.rs @@ -318,6 +318,42 @@ impl PodmanClient { "sctp" => "sctp", _ => "tcp", }; + // Effective bind. A gated port with no declared bind would + // publish 0.0.0.0 — the app would own every host address, which + // is both the exposure itself and the reason the daemon's app + // gate cannot bind those addresses to authenticate them. Pin it + // to loopback so the gate can take the external addresses. + // + // Doing it HERE, at container creation, is the point: the pin and + // the gate's takeover then both come from the daemon and cannot + // disagree. The earlier attempt put this decision in manifest + // data instead, and a node whose manifests lagged the binary + // published Bitcoin's loopback-only RPC across the LAN + // (archi-dev-box, 2026-08-03). + // + // A port that already declares a bind is never overridden — that + // is exactly what keeps `bind: 127.0.0.1` ports host-local and + // leaves `auth: none` protocol ports (LND gRPC/REST, electrum) + // published as they are, so remote wallets keep working. + // NOTE: the daemon deliberately does NOT rewrite this. Pinning a + // published port to loopback is how an app hands its external + // addresses to the gate, but it belongs in the manifest, not in + // daemon-side inference: + // + // * `bind` is already honoured by every publish path (here and + // in package::install), so a manifest edit needs no code. + // * inference here would cover only THIS path — proven on + // archi-dev-box, where a recreate went through another one and + // the pin never applied. + // * and inferring from an ABSENT field is what republished + // Bitcoin's loopback RPC across the LAN, and came within one + // container-recreate of pinning LND's gRPC/REST and breaking + // every remote wallet. + // + // So the migration ships as `bind: 127.0.0.1` in the signed + // catalog. Verified 2026-08-03 that a disk-only manifest edit is + // overridden by the catalog, which is precisely why the catalog is + // the right and only place to carry it. let mut mapping = serde_json::json!({ "container_port": port.container, "host_port": port.host,