From 3716b6e9c31ff87a35e7a92c933f9d245fba6430 Mon Sep 17 00:00:00 2001 From: archipelago Date: Mon, 3 Aug 2026 18:41:29 -0400 Subject: [PATCH] fix(security): two gate bugs that would have made the rollout a no-op MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found while setting up the on-node test, and both fail silently in the same direction — the gate reports success while protecting nothing, which is the exact failure the module was written to prevent. 1. Loopback-pinned ports were skipped entirely. `identity.rs` dropped any port whose manifest sets `bind: 127.0.0.1`, reasoning that a loopback publish is not externally reachable. But `listener.rs` requires loopback-pinning as the PRECONDITION for gating — while an app holds 0.0.0.0: the kernel will not let the gate bind that port at all. So the two contradicted each other: pinning an app, the one action that lets the gate take over, was also what removed it from the gated set. Completing the entire migration would have gated nothing, and GateStatus would have reported zero unprotected ports while doing it. `bind` cannot carry this decision, because two unrelated intentions produce an identical loopback publish: Bitcoin's RPC 8332 is pinned so the LAN CANNOT reach it (fronting it would newly expose it on every host address, behind a login but exposed where it deliberately was not), whereas a migrated app is pinned precisely so the gate CAN. Inferring from `bind` breaks one or the other, so the intent is now declared: `PortAuth::Local` means the first case. The three ports that are host-local by intent (bitcoin-core/knots 8332, aiui 5180 — all already `bind: 127.0.0.1`) say so, and a loopback publish with `auth: session` stays gated. A test pins that property. 2. The port map was never refreshed. `AppGate::refresh()` existed, was documented as making catalog changes apply without a restart, and was called by nothing. The map was built once in `new()`, so an app installed while the daemon runs would never be gated — and would never appear in `unprotected` either, so the node would report itself fully enforced while serving a brand-new app to anyone who asked. The sweep now refreshes before classifying. Tests: 22/22 appgate, 73/73 archipelago-container. Co-Authored-By: Claude Opus 5 (1M context) --- apps/aiui/manifest.yml | 1 + apps/bitcoin-core/manifest.yml | 1 + apps/bitcoin-knots/manifest.yml | 1 + core/archipelago/src/appgate/identity.rs | 46 ++++++++++++++++++------ core/archipelago/src/appgate/listener.rs | 6 ++++ core/container/src/manifest.rs | 17 +++++++++ 6 files changed, 61 insertions(+), 11 deletions(-) diff --git a/apps/aiui/manifest.yml b/apps/aiui/manifest.yml index 4a07e1ba..d07ceca4 100644 --- a/apps/aiui/manifest.yml +++ b/apps/aiui/manifest.yml @@ -28,6 +28,7 @@ app: container: 80 protocol: tcp bind: 127.0.0.1 # Only accessible via nginx proxy, not externally + auth: local health_check: type: http diff --git a/apps/bitcoin-core/manifest.yml b/apps/bitcoin-core/manifest.yml index c9c3879b..6cdd5faa 100644 --- a/apps/bitcoin-core/manifest.yml +++ b/apps/bitcoin-core/manifest.yml @@ -85,6 +85,7 @@ app: container: 8332 protocol: tcp bind: 127.0.0.1 + auth: local - host: 8333 container: 8333 protocol: tcp diff --git a/apps/bitcoin-knots/manifest.yml b/apps/bitcoin-knots/manifest.yml index 884eb5a2..9d7967d4 100644 --- a/apps/bitcoin-knots/manifest.yml +++ b/apps/bitcoin-knots/manifest.yml @@ -85,6 +85,7 @@ app: container: 8332 protocol: tcp bind: 127.0.0.1 + auth: local - host: 8333 container: 8333 protocol: tcp diff --git a/core/archipelago/src/appgate/identity.rs b/core/archipelago/src/appgate/identity.rs index caaecbba..69afce23 100644 --- a/core/archipelago/src/appgate/identity.rs +++ b/core/archipelago/src/appgate/identity.rs @@ -153,6 +153,10 @@ pub fn build_port_map() -> PortMap { .unwrap_or_else(|| "(no rationale recorded)".to_string()), protocol: protocol.to_string(), }), + // Declared host-local. Not gated and not reported as + // exposed, because it is neither — see PortAuth::Local + // for why this cannot be inferred from `bind`. + PortAuth::Local => {} PortAuth::Session => { // UDP cannot carry an HTTP challenge. Such a port has // no business defaulting into the gated set where it @@ -171,13 +175,12 @@ pub fn build_port_map() -> PortMap { }); continue; } - // A publish pinned to loopback is not externally - // reachable, so the gate has nothing to stand in - // front of. Gating it would mean binding a port the - // app already holds and breaking in-node clients. - if port.bind.parse::().is_ok_and(|ip| ip.is_loopback()) { - 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`. map.gated.insert( port.host, GatedPort { @@ -242,11 +245,32 @@ mod tests { } } - /// Bitcoin's RPC is loopback-pinned, so the gate must leave it alone - /// even though its manifest does not declare an exemption. + /// Bitcoin's RPC is host-local by intent (`auth: local`), so the gate + /// must neither gate it nor report it as exposed — fronting it would + /// newly publish it on every host address, behind a login but reachable + /// where it deliberately was not. #[test] - fn loopback_pinned_ports_are_not_gated() { - assert!(build_port_map().gated(8332).is_none()); + fn host_local_ports_are_neither_gated_nor_reported() { + let map = build_port_map(); + assert!(map.gated(8332).is_none(), "bitcoin RPC must not be gated"); + assert!( + !map.exempt_ports().iter().any(|e| e.port == 8332), + "a host-local port is not an unauthenticated exposure" + ); + } + + /// 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. + #[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"; + 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"); } /// An app UI that was reachable with no credential in the 2026-08-03 diff --git a/core/archipelago/src/appgate/listener.rs b/core/archipelago/src/appgate/listener.rs index b02b2eaf..e789609e 100644 --- a/core/archipelago/src/appgate/listener.rs +++ b/core/archipelago/src/appgate/listener.rs @@ -163,6 +163,12 @@ async fn sweep( held: &mut HashMap<(u16, IpAddr), ()>, shutdown_rx: &tokio::sync::watch::Receiver, ) { + // Re-read the manifests every sweep rather than trusting the map built + // at construction. An app installed while the daemon is running would + // otherwise never be gated until the next restart — and it would not + // appear in `unprotected` either, so the node would report itself fully + // enforced while serving a brand-new app to anyone who asked. + gate.refresh().await; let port_map = gate.port_map().await; let addresses = host_addresses().await; if addresses.is_empty() { diff --git a/core/container/src/manifest.rs b/core/container/src/manifest.rs index c6c55434..e7c2156d 100644 --- a/core/container/src/manifest.rs +++ b/core/container/src/manifest.rs @@ -530,6 +530,23 @@ pub enum PortAuth { /// (Bitcoin p2p gossip, mDNS). Requires `auth_rationale`: an exemption /// nobody can explain is an exemption nobody reviewed. None, + /// Host-local by intent — the gate must not bind this port at all. + /// + /// This exists because `bind: 127.0.0.1` is ambiguous on its own, and + /// reading intent out of it would be wrong in both directions. Two + /// unrelated situations produce an identical loopback publish: + /// + /// * Bitcoin's RPC 8332 is loopback-pinned so that the LAN *cannot* + /// reach it. Fronting it with the gate would newly expose it on every + /// host address — behind a login, but exposed where it deliberately + /// was not. + /// * A gated app is loopback-pinned precisely *so that* the gate can + /// take over its external addresses; that is the whole migration. + /// + /// Inferring from `bind` would break one or the other, so the intent is + /// declared. `Local` means the first case: never externally reachable, + /// gate keeps its hands off. + Local, } #[derive(Debug, Clone, Serialize, Deserialize)]