From 320e92a104d34902518cbb282bcc437afbd12b45 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 2 Aug 2026 14:34:17 -0700 Subject: [PATCH 1/6] fix(connectors): warn when the runtime API is exposed without a key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authentication is off whenever `api_key` is empty, and the shipped `address` is loopback, so the default posture is "any local process may read every connector credential" — defensible for an admin API. What nothing catches is an operator moving `address` to reach the API from outside a container and getting an unauthenticated endpoint serving credentials, with no signal at any layer. Warns rather than refuses to start: refusing would break deployments that are exposed today, and that call is the maintainers' to make. Resolves the address rather than parsing it. The default is `localhost:8081`, which is loopback but is not a `SocketAddr`, so a parse check would warn on the shipped config and teach operators to ignore the warning. An address that cannot resolve counts as exposed — it is about to fail the bind anyway. --- core/connectors/runtime/src/api/mod.rs | 102 ++++++++++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/core/connectors/runtime/src/api/mod.rs b/core/connectors/runtime/src/api/mod.rs index 38cd8f6ed1..0e36f15796 100644 --- a/core/connectors/runtime/src/api/mod.rs +++ b/core/connectors/runtime/src/api/mod.rs @@ -22,9 +22,14 @@ use axum::{Json, Router, extract::State, middleware, routing::get}; use axum_server::tls_rustls::RustlsConfig; use config::{HttpConfig, configure_cors}; use iggy_connector_sdk::api::ConnectorRuntimeStats; -use std::{net::SocketAddr, path::PathBuf, sync::Arc}; +use secrecy::ExposeSecret; +use std::{ + net::{SocketAddr, ToSocketAddrs}, + path::PathBuf, + sync::Arc, +}; use tokio::spawn; -use tracing::{error, info}; +use tracing::{error, info, warn}; mod auth; pub mod config; @@ -41,6 +46,13 @@ pub async fn init(config: &HttpConfig, context: Arc) { return; } + if is_unauthenticated_beyond_loopback(config) { + warn!( + "{NAME} HTTP API is enabled on {} with no api_key configured. Its configuration endpoints return plugin configuration verbatim, credentials included, so anyone able to reach that address can read every connector secret. Set http.api_key, or bind the API to loopback.", + config.address + ); + } + let mut system_router = Router::new().route("/stats", get(get_stats)); if config.metrics.enabled { @@ -121,6 +133,30 @@ pub async fn init(config: &HttpConfig, context: Arc) { }); } +/// Whether the API would answer beyond loopback with no key required. +/// +/// The configuration endpoints return plugin configuration verbatim, so an +/// unauthenticated listener on a routable address hands out every credential an +/// operator put in their TOML. Loopback with no key is the shipped default and +/// a defensible posture for an admin API; moving only the address is the +/// combination no other layer catches. +/// +/// Resolves rather than parses, because `address` accepts a hostname and the +/// default is `localhost:8081` - which is loopback but is not a `SocketAddr`. +/// The bind that follows resolves the same string, so this classifies what will +/// actually be listened on. An address that cannot resolve counts as exposed: +/// it is about to fail the bind anyway, and staying quiet about an address we +/// could not classify is the wrong direction to be wrong in. +fn is_unauthenticated_beyond_loopback(config: &HttpConfig) -> bool { + if !config.api_key.expose_secret().is_empty() { + return false; + } + match config.address.to_socket_addrs() { + Ok(mut resolved) => !resolved.all(|address| address.ip().is_loopback()), + Err(_) => true, + } +} + async fn get_metrics(State(context): State>) -> String { context.metrics.get_formatted_output() } @@ -128,3 +164,65 @@ async fn get_metrics(State(context): State>) -> String { async fn get_stats(State(context): State>) -> Json { Json(stats::get_runtime_stats(&context).await) } + +#[cfg(test)] +mod tests { + use super::*; + use secrecy::SecretString; + + fn config(address: &str, api_key: &str) -> HttpConfig { + HttpConfig { + address: address.to_owned(), + api_key: SecretString::from(api_key.to_owned()), + ..HttpConfig::default() + } + } + + #[test] + fn given_loopback_address_and_no_key_when_checked_should_stay_quiet() { + // The shipped posture. Warning here would train operators to ignore it. + assert!(!is_unauthenticated_beyond_loopback(&config( + "127.0.0.1:8081", + "" + ))); + assert!(!is_unauthenticated_beyond_loopback(&config( + "[::1]:8081", + "" + ))); + assert!( + !is_unauthenticated_beyond_loopback(&config("localhost:8081", "")), + "the default address is a hostname, so parsing alone would misjudge it" + ); + } + + #[test] + fn given_routable_address_and_no_key_when_checked_should_report_it() { + assert!( + is_unauthenticated_beyond_loopback(&config("0.0.0.0:8081", "")), + "binding every interface to reach the API from outside a container \ + is the case this exists to catch" + ); + assert!(is_unauthenticated_beyond_loopback(&config( + "192.0.2.10:8081", + "" + ))); + } + + #[test] + fn given_configured_key_when_checked_should_stay_quiet_on_any_address() { + assert!(!is_unauthenticated_beyond_loopback(&config( + "0.0.0.0:8081", + "secret" + ))); + } + + #[test] + fn given_unresolvable_address_when_checked_should_report_it() { + // About to fail the bind regardless, so the warning costs nothing and + // the alternative is silence about an address we cannot classify. + assert!(is_unauthenticated_beyond_loopback(&config( + "not a valid address", + "" + ))); + } +} From 5ed1a33f4c9d38315f038a7d9a5f7cce205503f3 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 2 Aug 2026 14:34:17 -0700 Subject: [PATCH 2/6] docs(connectors): document the runtime control API as privileged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The endpoint list said what each route returns but not that the configuration routes return plugin configuration verbatim, credentials included, with no redaction layer anywhere in the runtime. An operator reading it had no way to know that exposing the port exposes every secret in their TOML. The `api_key` comment also described the key as optional without saying that leaving it empty disables authentication outright, and nothing explained why the default address is loopback — which made it look like an arbitrary default rather than the control that confines the exposure. --- core/connectors/runtime/README.md | 17 ++++++++++++++++- core/connectors/runtime/config.toml | 4 +++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/core/connectors/runtime/README.md b/core/connectors/runtime/README.md index 1c1339f49c..c3d71bd7af 100644 --- a/core/connectors/runtime/README.md +++ b/core/connectors/runtime/README.md @@ -136,8 +136,10 @@ Connector runtime has an optional HTTP API that can be enabled by setting the `e ```toml [http] # Optional HTTP API configuration enabled = true +# Loopback on purpose: the configuration endpoints return plugin credentials in +# plaintext. Set api_key in the same edit if you move this off loopback. address = "127.0.0.1:8081" -api_key = "" # Optional API key for authentication to be passed as `api-key` header +api_key = "" # Optional API key for authentication to be passed as `api-key` header; empty disables authentication [http.cors] # Optional CORS configuration for HTTP API enabled = false @@ -158,6 +160,19 @@ cert_file = "core/certs/iggy_cert.pem" key_file = "core/certs/iggy_key.pem" ``` +> [!IMPORTANT] +> **Treat this API as privileged.** The configuration endpoints return plugin +> configuration exactly as it was parsed from TOML, credentials included - a +> database connection string, an S3 secret key, a webhook signing secret. There +> is no redaction layer. `api_key` is empty by default, which means +> authentication is **off** by default; the loopback default `address` is what +> confines that to local processes. +> +> If you change `address` to reach the API from outside a container, set +> `api_key` in the same edit. The runtime logs a warning at startup when the +> address resolves beyond loopback with no key configured, but nothing prevents +> it. + Currently, it does expose the following endpoints: - `GET /`: welcome message. diff --git a/core/connectors/runtime/config.toml b/core/connectors/runtime/config.toml index 247a67e6b7..90e855d455 100644 --- a/core/connectors/runtime/config.toml +++ b/core/connectors/runtime/config.toml @@ -17,8 +17,10 @@ [http] # Optional HTTP API configuration enabled = true +# Loopback on purpose: the configuration endpoints return plugin credentials in +# plaintext. Set api_key in the same edit if you move this off loopback. address = "127.0.0.1:8081" -api_key = "" # Optional API key for authentication to be passed as `api-key` header +api_key = "" # Optional API key for authentication to be passed as `api-key` header; empty disables authentication [http.cors] # Optional CORS configuration for HTTP API enabled = false From 4b7bf9aef30b4b5ad293a41389c73b2f6b6b78d3 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sun, 2 Aug 2026 17:54:38 -0700 Subject: [PATCH 3/6] test(connectors): cover the warning wiring in the API init The four existing tests cover the guard's decision table but not that `init` consults it, so deleting the call left the suite green. These two drive the real `init` and close that. Reaching the warning needs a non-loopback address, and any such address that binds would open a port on every interface for the length of the test, which on macOS also trips the firewall prompt. The test uses a documentation-range address instead: `init` warns, then fails the bind. That turns the awkward constraint into the stronger assertion, because the warning is only observable if it precedes the bind, which is what an operator whose bind then fails depends on. Both mutations were checked: removing the call and moving it after the bind each fail the test. Captured through a global subscriber, since a warning is invisible to a test without one. Tests filter the captured lines by their own address so that events from tests running in parallel cannot be confused. The loopback case is here too. Warning on the shipped default would be worse than not warning at all, because operators learn to ignore it. --- core/connectors/runtime/src/api/mod.rs | 151 +++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/core/connectors/runtime/src/api/mod.rs b/core/connectors/runtime/src/api/mod.rs index 0e36f15796..5d161a64c9 100644 --- a/core/connectors/runtime/src/api/mod.rs +++ b/core/connectors/runtime/src/api/mod.rs @@ -168,7 +168,26 @@ async fn get_stats(State(context): State>) -> Json HttpConfig { HttpConfig { @@ -178,6 +197,138 @@ mod tests { } } + fn captured() -> &'static Mutex> { + static WARNINGS: OnceLock>> = OnceLock::new(); + WARNINGS.get_or_init(|| Mutex::new(Vec::new())) + } + + /// Installs the capture once for the whole test binary, since a global + /// subscriber can only be set once. Every test filters the captured lines + /// by its own address, so events from tests running in parallel cannot be + /// mistaken for each other. + fn capture_warnings() { + static INSTALLED: OnceLock<()> = OnceLock::new(); + INSTALLED.get_or_init(|| { + let subscriber = tracing_subscriber::registry().with(CaptureWarnings); + tracing::subscriber::set_global_default(subscriber) + .expect("no other test in this binary installs a subscriber"); + }); + } + + fn warned_about(address: &str) -> bool { + captured() + .lock() + .expect("the capture mutex is only held to push a line") + .iter() + .any(|warning| warning.contains(address)) + } + + struct CaptureWarnings; + + impl tracing_subscriber::Layer for CaptureWarnings { + fn on_event(&self, event: &tracing::Event<'_>, _context: LayerContext<'_, S>) { + if *event.metadata().level() != Level::WARN { + return; + } + let mut recorded = Recorded(String::new()); + event.record(&mut recorded); + captured() + .lock() + .expect("the capture mutex is only held to push a line") + .push(recorded.0); + } + } + + /// Every field the event carried, rendered into one line. + /// + /// Unconditional on purpose. These tests only ask whether a warning + /// mentioned a given address, so singling out the `message` field would add + /// a branch to the scaffolding whose other side nothing here would ever + /// take. `record_str` needs no impl either: it forwards here by default, + /// and a formatted `warn!` message arrives as `fmt::Arguments` regardless. + struct Recorded(String); + + impl Visit for Recorded { + fn record_debug(&mut self, _field: &Field, value: &dyn std::fmt::Debug) { + self.0.push_str(&format!("{value:?} ")); + } + } + + /// The cheapest context `init` will accept. Nothing here reaches Iggy: the + /// clients are never connected, and the warning is decided from the config + /// alone. + async fn context() -> (Arc, TempDir) { + let directory = tempfile::tempdir().expect("a temp dir must be available"); + let config_provider = + create_connectors_config_provider(&ConnectorsConfig::Local(LocalConnectorsConfig { + config_dir: directory.path().display().to_string(), + })) + .await + .expect("an empty config dir must initialize with no connectors"); + + let context = RuntimeContext { + sinks: SinkManager::new(vec![]), + sources: SourceManager::new(vec![]), + api_key: SecretString::from(String::new()), + config_provider: Arc::from(config_provider), + metrics: Arc::new(Metrics::init()), + start_time: IggyTimestamp::now(), + iggy_clients: Arc::new(IggyClients { + producer: IggyClient::default(), + consumer: IggyClient::default(), + }), + state_path: directory.path().display().to_string(), + }; + (Arc::new(context), directory) + } + + fn free_port() -> u16 { + std::net::TcpListener::bind("127.0.0.1:0") + .expect("the loopback interface must offer a port") + .local_addr() + .expect("a bound listener has an address") + .port() + } + + #[tokio::test] + async fn given_no_key_and_a_routable_address_when_initialized_should_warn_before_binding() { + capture_warnings(); + let (context, _directory) = context().await; + let config = config(UNASSIGNABLE_ROUTABLE_ADDRESS, ""); + + // `init` panics when the bind fails, which is what makes this the + // ordering test: the warning has to already be out by then, or an + // operator whose bind fails never learns the API was unauthenticated. + let bind_failed = tokio::spawn(async move { init(&config, context).await }) + .await + .is_err(); + + assert!( + bind_failed, + "a documentation-range address must not be bindable, or this test \ + would be exposing a port instead of exercising the warning" + ); + assert!( + warned_about(UNASSIGNABLE_ROUTABLE_ADDRESS), + "init must consult the guard and name the address it is exposing" + ); + } + + #[tokio::test] + async fn given_loopback_address_when_initialized_should_not_warn() { + capture_warnings(); + let address = format!("127.0.0.1:{}", free_port()); + let (context, _directory) = context().await; + + init(&config(&address, ""), context).await; + + assert!( + !warned_about(&address), + "the shipped posture is loopback with no key; warning about it \ + would teach operators to ignore the one that matters" + ); + } + #[test] fn given_loopback_address_and_no_key_when_checked_should_stay_quiet() { // The shipped posture. Warning here would train operators to ignore it. From b4ad6555c88a99601b9be15d3a956cff17e3671f Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 8 Aug 2026 14:15:42 -0700 Subject: [PATCH 4/6] fix(connectors): harden the API exposure warning after review The framing was too narrow. Only `/` and `/health` are exempt from the key, so the same empty string also guards `POST .../configs`, `PUT .../configs/active`, `DELETE .../configs` and `POST .../restart`. Since `restart_connector` re-reads the stored config and starts from it, a caller who can write one repoints a connector at a destination they chose and the runtime then forwards topic data there under its own Iggy credentials, with the stored plugin path `dlopen`ed on the next start. Both the warning and the README now say read *and* rewrite. Two more ways the loopback containment goes away, neither previously mentioned. `[http.cors]` ships `allowed_origins = ["*"]` and the CORS layer wraps outside authentication, so enabling it lets any page the operator visits read the config endpoints cross-origin, a browser being a local process. And `http.tls` ships disabled, so following the old advice exactly sent the key and the credential-bearing responses over the wire in clear. The endpoint list also omitted every mutating route, which is what made the read-only reading look right. `ToSocketAddrs` was a blocking `getaddrinfo` on a tokio worker that the already-spawned connector tasks share, so the predicate now uses `tokio::net::lookup_host`. Collecting the result also fixes `all` over an empty iterator reporting an unresolvable address as confined, inverting the stated policy. Resolution stays classification-only: binding what it returns would drop the `localhost` fallback on IPv6-disabled hosts. The stated reason for resolving was simply wrong. `HttpConfig::default()` is unreachable in production because the embedded `config.toml` is the first figment layer, so the effective default is `127.0.0.1:8081` and a parse would have handled it. Resolving is still right, because `address` is free-form and takes hostnames. Tests: the capture is per-test behind a `set_default` guard instead of claiming the process-wide subscriber slot with a never-drained shared buffer, and is level-filtered so the global max level stops being TRACE and callsites keep short-circuiting. The loopback case gains the positive control it lacked, since it asserted only an absence and would have kept passing if `enabled` ever defaulted to false. Dropped the `free_port` reserve-and-rebind race for `127.0.0.1:0`, and the context helper takes the api_key rather than hardcoding an empty one that could disagree with the config the guard reads. --- core/connectors/runtime/README.md | 45 ++++- core/connectors/runtime/src/api/mod.rs | 266 ++++++++++++++----------- 2 files changed, 180 insertions(+), 131 deletions(-) diff --git a/core/connectors/runtime/README.md b/core/connectors/runtime/README.md index c3d71bd7af..53cea5d084 100644 --- a/core/connectors/runtime/README.md +++ b/core/connectors/runtime/README.md @@ -161,17 +161,38 @@ key_file = "core/certs/iggy_key.pem" ``` > [!IMPORTANT] -> **Treat this API as privileged.** The configuration endpoints return plugin -> configuration exactly as it was parsed from TOML, credentials included - a -> database connection string, an S3 secret key, a webhook signing secret. There -> is no redaction layer. `api_key` is empty by default, which means -> authentication is **off** by default; the loopback default `address` is what -> confines that to local processes. +> **Treat this API as privileged. It reads and it writes.** > -> If you change `address` to reach the API from outside a container, set -> `api_key` in the same edit. The runtime logs a warning at startup when the -> address resolves beyond loopback with no key configured, but nothing prevents -> it. +> The configuration endpoints return plugin configuration exactly as stored, +> credentials included - a database connection string, an S3 secret key, a +> webhook signing secret. There is no redaction layer anywhere in the runtime. +> +> The exposure is not limited to disclosure. `POST /{sinks,sources}/{key}/configs` +> followed by `PUT .../configs/active` and `POST .../restart` is enough to +> repoint a connector at a destination of the caller's choosing: `restart` +> re-reads the stored configuration and starts the connector from it, so the +> runtime then forwards your topic data using its own Iggy credentials. The +> stored plugin `path` is `dlopen`ed on the next start as well. `DELETE +> .../configs` is on the same footing. +> +> `api_key` is empty by default, which means authentication is **off** by +> default. Only `/` and `/health` are exempt once it is set, so everything above +> sits behind that one empty string, and the loopback default `address` is what +> confines it to local processes. +> +> Three ways that containment goes away: +> +> - **Moving `address` off loopback.** Set `api_key` in the same edit. The +> runtime warns at startup when the address resolves beyond loopback with no +> key configured, but nothing prevents it. +> - **Enabling `[http.cors]`.** It ships `allowed_origins = ["*"]`, which becomes +> `AllowOrigin::any()`, and the CORS layer wraps *outside* authentication. A +> browser is a local process, so with CORS enabled and no key, any page the +> operator visits can read the configuration endpoints cross-origin. Setting +> `api_key` closes it, since an attacker's page cannot supply the header. +> - **Leaving `http.tls.enabled = false`.** It ships disabled, so the `api-key` +> header and the responses carrying your credentials both travel in cleartext. +> Enable TLS alongside `api_key` whenever this API leaves loopback. Currently, it does expose the following endpoints: @@ -183,19 +204,23 @@ Currently, it does expose the following endpoints: - `GET /sinks/{key}`: sink details. - `GET /sinks/{key}/configs`: list of configuration versions for the sink. - `POST /sinks/{key}/configs`: add a new configuration version for the sink. +- `DELETE /sinks/{key}/configs`: delete configuration versions for the sink. - `GET /sinks/{key}/configs/{version}`: configuration details for a specific version. - `GET /sinks/{key}/configs/active`: active configuration details. - `PUT /sinks/{key}/configs/active`: activate a specific configuration version for the sink. - `GET /sinks/{key}/configs/plugin`: sink plugin config, including the optional `format` query parameter to specify the config format. +- `POST /sinks/{key}/restart`: stop the sink and start it again from its stored active configuration. - `GET /sinks/{key}/transforms`: sink transforms to be applied to the fields. - `GET /sources`: list of sources. - `GET /sources/{key}`: source details. - `GET /sources/{key}/configs`: list of configuration versions for the source. - `POST /sources/{key}/configs`: add a new configuration version for the source. +- `DELETE /sources/{key}/configs`: delete configuration versions for the source. - `GET /sources/{key}/configs/{version}`: configuration details for a specific version. - `GET /sources/{key}/configs/active`: active configuration details. - `PUT /sources/{key}/configs/active`: activate a specific configuration version for the source. - `GET /sources/{key}/configs/plugin`: source plugin config, including the optional `format` query parameter to specify the config format. +- `POST /sources/{key}/restart`: stop the source and start it again from its stored active configuration. - `GET /sources/{key}/transforms`: source transforms to be applied to the fields. ## Telemetry diff --git a/core/connectors/runtime/src/api/mod.rs b/core/connectors/runtime/src/api/mod.rs index 5d161a64c9..fa1539fe3f 100644 --- a/core/connectors/runtime/src/api/mod.rs +++ b/core/connectors/runtime/src/api/mod.rs @@ -23,11 +23,8 @@ use axum_server::tls_rustls::RustlsConfig; use config::{HttpConfig, configure_cors}; use iggy_connector_sdk::api::ConnectorRuntimeStats; use secrecy::ExposeSecret; -use std::{ - net::{SocketAddr, ToSocketAddrs}, - path::PathBuf, - sync::Arc, -}; +use std::{net::SocketAddr, path::PathBuf, sync::Arc}; +use tokio::net::lookup_host; use tokio::spawn; use tracing::{error, info, warn}; @@ -46,9 +43,9 @@ pub async fn init(config: &HttpConfig, context: Arc) { return; } - if is_unauthenticated_beyond_loopback(config) { + if is_unauthenticated_beyond_loopback(config).await { warn!( - "{NAME} HTTP API is enabled on {} with no api_key configured. Its configuration endpoints return plugin configuration verbatim, credentials included, so anyone able to reach that address can read every connector secret. Set http.api_key, or bind the API to loopback.", + "{NAME} HTTP API is enabled on {} with no api_key configured. Anyone able to reach that address can read or rewrite every connector configuration, credentials included, and restart connectors from it. Set http.api_key, and http.tls unless the key and the responses may cross in cleartext, or bind the API to loopback.", config.address ); } @@ -135,26 +132,36 @@ pub async fn init(config: &HttpConfig, context: Arc) { /// Whether the API would answer beyond loopback with no key required. /// -/// The configuration endpoints return plugin configuration verbatim, so an -/// unauthenticated listener on a routable address hands out every credential an -/// operator put in their TOML. Loopback with no key is the shipped default and -/// a defensible posture for an admin API; moving only the address is the +/// The configuration endpoints return plugin configuration verbatim and also +/// accept writes, so an unauthenticated listener on a routable address hands +/// out every credential an operator put in their config and lets a caller +/// repoint a connector. Loopback with no key is the shipped default and a +/// defensible posture for an admin API; moving only the address is the /// combination no other layer catches. /// -/// Resolves rather than parses, because `address` accepts a hostname and the -/// default is `localhost:8081` - which is loopback but is not a `SocketAddr`. -/// The bind that follows resolves the same string, so this classifies what will -/// actually be listened on. An address that cannot resolve counts as exposed: -/// it is about to fail the bind anyway, and staying quiet about an address we -/// could not classify is the wrong direction to be wrong in. -fn is_unauthenticated_beyond_loopback(config: &HttpConfig) -> bool { +/// Resolves rather than parses because `address` is a free-form `String` that +/// takes a hostname, as `[iggy] address` does in the same file. Not because the +/// default needs it: the embedded `config.toml` is the first figment layer, so +/// the effective default is `127.0.0.1:8081` and would parse. An address that +/// cannot resolve counts as exposed, since it is about to fail the bind anyway +/// and staying quiet about one we could not classify is the wrong direction to +/// be wrong in. +/// +/// Classification only. Do not bind what this resolves: `TcpListener::bind` +/// walks every resolved address and takes the first that works, so collapsing +/// to one would drop the `localhost` -> `[::1, 127.0.0.1]` fallback on hosts +/// with IPv6 disabled. +async fn is_unauthenticated_beyond_loopback(config: &HttpConfig) -> bool { if !config.api_key.expose_secret().is_empty() { return false; } - match config.address.to_socket_addrs() { - Ok(mut resolved) => !resolved.all(|address| address.ip().is_loopback()), - Err(_) => true, - } + let Ok(resolved) = lookup_host(&config.address).await else { + return true; + }; + let addresses: Vec = resolved.collect(); + // Empty is reported as exposed rather than confined: `all` over nothing is + // vacuously true, which would quietly invert the policy above. + addresses.is_empty() || !addresses.iter().all(|address| address.ip().is_loopback()) } async fn get_metrics(State(context): State>) -> String { @@ -177,18 +184,28 @@ mod tests { use iggy::prelude::IggyClient; use iggy_common::IggyTimestamp; use secrecy::SecretString; - use std::sync::{Mutex, OnceLock}; + use std::sync::Mutex; use tempfile::TempDir; use tracing::Level; use tracing::field::{Field, Visit}; + use tracing::subscriber::DefaultGuard; + use tracing_subscriber::Layer as _; + use tracing_subscriber::filter::LevelFilter; use tracing_subscriber::layer::{Context as LayerContext, SubscriberExt}; - /// Reserved for documentation (RFC 5737), so it is never assignable on a - /// real host. Used to reach the warning without binding: any non-loopback - /// address that binds successfully would expose a port on every interface - /// for the duration of the test. + /// Reserved for documentation by RFC 5737, so no host routes it and the + /// bind fails. That is what lets the test reach the warning without + /// listening anywhere: a non-loopback address that binds successfully would + /// put a port on every interface for the life of the test binary. const UNASSIGNABLE_ROUTABLE_ADDRESS: &str = "192.0.2.1:8081"; + /// Port 0 rather than a port reserved by binding and dropping first. The + /// tests never need to know which port it lands on, and reserving one is a + /// race that buys nothing. + const EPHEMERAL_LOOPBACK_ADDRESS: &str = "127.0.0.1:0"; + + type Captured = Arc>>; + fn config(address: &str, api_key: &str) -> HttpConfig { HttpConfig { address: address.to_owned(), @@ -197,55 +214,72 @@ mod tests { } } - fn captured() -> &'static Mutex> { - static WARNINGS: OnceLock>> = OnceLock::new(); - WARNINGS.get_or_init(|| Mutex::new(Vec::new())) + /// Captures events for the current thread only, for as long as the returned + /// guard lives. + /// + /// Deliberately not a global subscriber: that slot is process-wide, so + /// claiming it would break any later test that installs its own, and a + /// shared buffer would leave every negative assertion hostage to warnings + /// from elsewhere in the binary. + /// + /// `#[tokio::test]` builds a current-thread runtime, so a task spawned by + /// the test body runs on this thread and sees this subscriber. Under a + /// multi-thread flavour the capture would come back empty and these + /// assertions would fail rather than quietly pass. + fn capture_events() -> (DefaultGuard, Captured) { + let captured: Captured = Arc::new(Mutex::new(Vec::new())); + // Filtered rather than checking the level inside `on_event`: a layer + // with no filter reports no `max_level_hint`, which pushes the global + // max level to TRACE and stops every callsite in the binary from + // short-circuiting. + let layer = CaptureEvents { + captured: Arc::clone(&captured), + } + .with_filter(LevelFilter::INFO); + let guard = tracing::subscriber::set_default(tracing_subscriber::registry().with(layer)); + (guard, captured) } - /// Installs the capture once for the whole test binary, since a global - /// subscriber can only be set once. Every test filters the captured lines - /// by its own address, so events from tests running in parallel cannot be - /// mistaken for each other. - fn capture_warnings() { - static INSTALLED: OnceLock<()> = OnceLock::new(); - INSTALLED.get_or_init(|| { - let subscriber = tracing_subscriber::registry().with(CaptureWarnings); - tracing::subscriber::set_global_default(subscriber) - .expect("no other test in this binary installs a subscriber"); - }); + fn warned_about(captured: &Captured, address: &str) -> bool { + captured + .lock() + .expect("the capture mutex is only held to push a line") + .iter() + .any(|(level, message)| *level == Level::WARN && message.contains(address)) } - fn warned_about(address: &str) -> bool { - captured() + /// Whether `init` got as far as serving. The positive control for tests + /// whose real assertion is that nothing was warned about. + fn started_serving(captured: &Captured) -> bool { + captured .lock() .expect("the capture mutex is only held to push a line") .iter() - .any(|warning| warning.contains(address)) + .any(|(level, message)| *level == Level::INFO && message.contains("Started")) } - struct CaptureWarnings; + struct CaptureEvents { + captured: Captured, + } - impl tracing_subscriber::Layer for CaptureWarnings { + impl tracing_subscriber::Layer for CaptureEvents { fn on_event(&self, event: &tracing::Event<'_>, _context: LayerContext<'_, S>) { - if *event.metadata().level() != Level::WARN { - return; - } let mut recorded = Recorded(String::new()); event.record(&mut recorded); - captured() + self.captured .lock() .expect("the capture mutex is only held to push a line") - .push(recorded.0); + .push((*event.metadata().level(), recorded.0)); } } /// Every field the event carried, rendered into one line. /// - /// Unconditional on purpose. These tests only ask whether a warning - /// mentioned a given address, so singling out the `message` field would add + /// Unconditional on purpose. These tests only ask whether an event + /// mentioned a given string, so singling out the `message` field would add /// a branch to the scaffolding whose other side nothing here would ever /// take. `record_str` needs no impl either: it forwards here by default, - /// and a formatted `warn!` message arrives as `fmt::Arguments` regardless. + /// and a formatted message arrives as `fmt::Arguments` regardless. struct Recorded(String); impl Visit for Recorded { @@ -257,7 +291,12 @@ mod tests { /// The cheapest context `init` will accept. Nothing here reaches Iggy: the /// clients are never connected, and the warning is decided from the config /// alone. - async fn context() -> (Arc, TempDir) { + /// + /// `api_key` is a parameter rather than always empty because the guard + /// reads `config.api_key` while the middleware enforces `context.api_key`. + /// They come from one binding in `main.rs` today, but a test that set only + /// one of them would be exercising a state the runtime cannot reach. + async fn context(api_key: &str) -> (Arc, TempDir) { let directory = tempfile::tempdir().expect("a temp dir must be available"); let config_provider = create_connectors_config_provider(&ConnectorsConfig::Local(LocalConnectorsConfig { @@ -269,7 +308,7 @@ mod tests { let context = RuntimeContext { sinks: SinkManager::new(vec![]), sources: SourceManager::new(vec![]), - api_key: SecretString::from(String::new()), + api_key: SecretString::from(api_key.to_owned()), config_provider: Arc::from(config_provider), metrics: Arc::new(Metrics::init()), start_time: IggyTimestamp::now(), @@ -282,18 +321,43 @@ mod tests { (Arc::new(context), directory) } - fn free_port() -> u16 { - std::net::TcpListener::bind("127.0.0.1:0") - .expect("the loopback interface must offer a port") - .local_addr() - .expect("a bound listener has an address") - .port() + #[tokio::test] + async fn given_loopback_address_and_no_key_when_checked_should_stay_quiet() { + // The shipped posture. Warning here would train operators to ignore it. + assert!(!is_unauthenticated_beyond_loopback(&config("127.0.0.1:8081", "")).await); + assert!(!is_unauthenticated_beyond_loopback(&config("[::1]:8081", "")).await); + assert!( + !is_unauthenticated_beyond_loopback(&config("localhost:8081", "")).await, + "`address` accepts a hostname, and parsing alone would misjudge one" + ); + } + + #[tokio::test] + async fn given_routable_address_and_no_key_when_checked_should_report_it() { + assert!( + is_unauthenticated_beyond_loopback(&config("0.0.0.0:8081", "")).await, + "binding every interface to reach the API from outside a container \ + is the case this exists to catch" + ); + assert!(is_unauthenticated_beyond_loopback(&config("192.0.2.10:8081", "")).await); + } + + #[tokio::test] + async fn given_configured_key_when_checked_should_stay_quiet_on_any_address() { + assert!(!is_unauthenticated_beyond_loopback(&config("0.0.0.0:8081", "secret")).await); + } + + #[tokio::test] + async fn given_unresolvable_address_when_checked_should_report_it() { + // About to fail the bind regardless, so the warning costs nothing and + // the alternative is silence about an address we cannot classify. + assert!(is_unauthenticated_beyond_loopback(&config("not a valid address", "")).await); } #[tokio::test] async fn given_no_key_and_a_routable_address_when_initialized_should_warn_before_binding() { - capture_warnings(); - let (context, _directory) = context().await; + let (_capture, captured) = capture_events(); + let (context, _directory) = context("").await; let config = config(UNASSIGNABLE_ROUTABLE_ADDRESS, ""); // `init` panics when the bind fails, which is what makes this the @@ -305,75 +369,35 @@ mod tests { assert!( bind_failed, - "a documentation-range address must not be bindable, or this test \ - would be exposing a port instead of exercising the warning" + "a documentation-range address is expected to be unbindable. A host with \ + net.ipv4.ip_nonlocal_bind=1, which keepalived and haproxy boxes set, binds \ + it instead, and this test has then started a listener that outlives the run" ); assert!( - warned_about(UNASSIGNABLE_ROUTABLE_ADDRESS), + warned_about(&captured, UNASSIGNABLE_ROUTABLE_ADDRESS), "init must consult the guard and name the address it is exposing" ); } #[tokio::test] - async fn given_loopback_address_when_initialized_should_not_warn() { - capture_warnings(); - let address = format!("127.0.0.1:{}", free_port()); - let (context, _directory) = context().await; + async fn given_loopback_address_when_initialized_should_serve_without_warning() { + let (_capture, captured) = capture_events(); + let (context, _directory) = context("").await; - init(&config(&address, ""), context).await; + init(&config(EPHEMERAL_LOOPBACK_ADDRESS, ""), context).await; + // Positive control first. Without it the assertion below passes for any + // reason `init` might return early, including `enabled` defaulting to + // false, and the only in-`init` loopback coverage would disappear + // silently. assert!( - !warned_about(&address), - "the shipped posture is loopback with no key; warning about it \ - would teach operators to ignore the one that matters" + started_serving(&captured), + "init must reach the listener, or the assertion below proves nothing" ); - } - - #[test] - fn given_loopback_address_and_no_key_when_checked_should_stay_quiet() { - // The shipped posture. Warning here would train operators to ignore it. - assert!(!is_unauthenticated_beyond_loopback(&config( - "127.0.0.1:8081", - "" - ))); - assert!(!is_unauthenticated_beyond_loopback(&config( - "[::1]:8081", - "" - ))); assert!( - !is_unauthenticated_beyond_loopback(&config("localhost:8081", "")), - "the default address is a hostname, so parsing alone would misjudge it" - ); - } - - #[test] - fn given_routable_address_and_no_key_when_checked_should_report_it() { - assert!( - is_unauthenticated_beyond_loopback(&config("0.0.0.0:8081", "")), - "binding every interface to reach the API from outside a container \ - is the case this exists to catch" + !warned_about(&captured, EPHEMERAL_LOOPBACK_ADDRESS), + "the shipped posture is loopback with no key; warning about it \ + would teach operators to ignore the one that matters" ); - assert!(is_unauthenticated_beyond_loopback(&config( - "192.0.2.10:8081", - "" - ))); - } - - #[test] - fn given_configured_key_when_checked_should_stay_quiet_on_any_address() { - assert!(!is_unauthenticated_beyond_loopback(&config( - "0.0.0.0:8081", - "secret" - ))); - } - - #[test] - fn given_unresolvable_address_when_checked_should_report_it() { - // About to fail the bind regardless, so the warning costs nothing and - // the alternative is silence about an address we cannot classify. - assert!(is_unauthenticated_beyond_loopback(&config( - "not a valid address", - "" - ))); } } From b86b5675134c5ef8ed3ae892a2097fd6365dd1f5 Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 8 Aug 2026 14:29:50 -0700 Subject: [PATCH 5/6] docs(connectors): correct the config-rewrite chain in the API notice The notice put `PUT .../configs/active` in the middle of the rewrite path, which is not what the default provider does. `restart_connector` asks for `get_sink_config(key, None)`, and the local provider resolves that to `max_by_key(version)` rather than the active version, so publishing a config is enough on its own and the activate call is not part of the chain. Describing it by effect rather than by call sequence keeps the notice true under both providers, since the HTTP one does resolve `None` to the active config. --- core/connectors/runtime/README.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/core/connectors/runtime/README.md b/core/connectors/runtime/README.md index 53cea5d084..c828ef61f3 100644 --- a/core/connectors/runtime/README.md +++ b/core/connectors/runtime/README.md @@ -167,13 +167,13 @@ key_file = "core/certs/iggy_key.pem" > credentials included - a database connection string, an S3 secret key, a > webhook signing secret. There is no redaction layer anywhere in the runtime. > -> The exposure is not limited to disclosure. `POST /{sinks,sources}/{key}/configs` -> followed by `PUT .../configs/active` and `POST .../restart` is enough to -> repoint a connector at a destination of the caller's choosing: `restart` -> re-reads the stored configuration and starts the connector from it, so the -> runtime then forwards your topic data using its own Iggy credentials. The -> stored plugin `path` is `dlopen`ed on the next start as well. `DELETE -> .../configs` is on the same footing. +> The exposure is not limited to disclosure. Publishing a configuration with +> `POST /{sinks,sources}/{key}/configs` and then calling `POST .../restart` is +> enough to repoint a connector at a destination of the caller's choosing, +> because `restart` re-reads the stored configuration and starts the connector +> from it. The runtime then forwards your topic data using its own Iggy +> credentials, and the stored plugin `path` is `dlopen`ed on the next start. +> `PUT .../configs/active` and `DELETE .../configs` sit behind the same key. > > `api_key` is empty by default, which means authentication is **off** by > default. Only `/` and `/health` are exempt once it is set, so everything above From b84590a2dc95640dae9013b0ad9bc558d293dacb Mon Sep 17 00:00:00 2001 From: mlevkov Date: Sat, 8 Aug 2026 14:48:29 -0700 Subject: [PATCH 6/6] fix(connectors): warn on every documented exposure path, not just one A self-review turned up that the previous commit documented three ways the API's containment goes away and then warned about one of them. An operator who enabled `[http.cors]` on the shipped loopback default, or who set `api_key` and left `http.tls` disabled off loopback, got silence from the very control added to catch exactly that. `warn_on_weak_containment` now emits one warning per path, because they compose independently and closing one is not closing the others. The early return on a configured key is gone: having a key says nothing about whether the key and the credential-bearing responses cross the wire in clear. The address predicate loses its key check and becomes `resolves_beyond_loopback`, which is the only thing it was classifying. That also fixes the remediation clause, which told operators to set `http.tls` whether or not it was already set, and read backwards on first pass. TLS now has its own warning, conditioned on being disabled. `example_config/config.toml` was carrying the old comments. It is the file operators copy, so leaving the guidance only in the embedded default and the README missed the audience it was written for. Tests: the two new paths, the disabled case, and a real assertion in place of the loopback one. Matching a warning against the address went vacuous the moment the message was reworded, so it now asserts no warning at all, which the per-test capture makes safe. All four were mutation-checked. --- .../runtime/example_config/config.toml | 5 +- core/connectors/runtime/src/api/mod.rs | 237 +++++++++++------- 2 files changed, 153 insertions(+), 89 deletions(-) diff --git a/core/connectors/runtime/example_config/config.toml b/core/connectors/runtime/example_config/config.toml index 4fad587f22..ce84c8d0c7 100644 --- a/core/connectors/runtime/example_config/config.toml +++ b/core/connectors/runtime/example_config/config.toml @@ -17,8 +17,11 @@ [http] # Optional HTTP API configuration enabled = true +# Loopback on purpose: the configuration endpoints return plugin credentials in +# plaintext and also accept writes. Set api_key in the same edit if you move +# this off loopback, and http.tls unless cleartext is acceptable. address = "127.0.0.1:8081" -api_key = "" # Optional API key for authentication to be passed as `api-key` header +api_key = "" # Optional API key for authentication to be passed as `api-key` header; empty disables authentication [http.cors] # Optional CORS configuration for HTTP API enabled = false diff --git a/core/connectors/runtime/src/api/mod.rs b/core/connectors/runtime/src/api/mod.rs index fa1539fe3f..01c5b1fd51 100644 --- a/core/connectors/runtime/src/api/mod.rs +++ b/core/connectors/runtime/src/api/mod.rs @@ -24,8 +24,7 @@ use config::{HttpConfig, configure_cors}; use iggy_connector_sdk::api::ConnectorRuntimeStats; use secrecy::ExposeSecret; use std::{net::SocketAddr, path::PathBuf, sync::Arc}; -use tokio::net::lookup_host; -use tokio::spawn; +use tokio::{net::lookup_host, spawn}; use tracing::{error, info, warn}; mod auth; @@ -43,12 +42,7 @@ pub async fn init(config: &HttpConfig, context: Arc) { return; } - if is_unauthenticated_beyond_loopback(config).await { - warn!( - "{NAME} HTTP API is enabled on {} with no api_key configured. Anyone able to reach that address can read or rewrite every connector configuration, credentials included, and restart connectors from it. Set http.api_key, and http.tls unless the key and the responses may cross in cleartext, or bind the API to loopback.", - config.address - ); - } + warn_on_weak_containment(config).await; let mut system_router = Router::new().route("/stats", get(get_stats)); @@ -130,14 +124,41 @@ pub async fn init(config: &HttpConfig, context: Arc) { }); } -/// Whether the API would answer beyond loopback with no key required. +/// Warns once for each way this API is less contained than its defaults look. /// -/// The configuration endpoints return plugin configuration verbatim and also -/// accept writes, so an unauthenticated listener on a routable address hands -/// out every credential an operator put in their config and lets a caller -/// repoint a connector. Loopback with no key is the shipped default and a -/// defensible posture for an admin API; moving only the address is the -/// combination no other layer catches. +/// Separate warnings rather than one, because the three compose independently +/// and an operator who closes one has not necessarily closed the others. All +/// three are the paths the runtime README documents. +async fn warn_on_weak_containment(config: &HttpConfig) { + let unauthenticated = config.api_key.expose_secret().is_empty(); + let beyond_loopback = resolves_beyond_loopback(&config.address).await; + + if unauthenticated && beyond_loopback { + warn!( + "{NAME} HTTP API is enabled on {} with no api_key configured. Anyone able to reach that address can read or rewrite every connector configuration, credentials included, and restart connectors from it. Set http.api_key, or bind the API to loopback.", + config.address + ); + } + + // Loopback does not contain this one. A browser is a local process, and the + // CORS layer wraps outside authentication, so the shipped + // `allowed_origins = ["*"]` lets any page the operator visits read these + // endpoints cross-origin. + if unauthenticated && config.cors.enabled { + warn!( + "{NAME} HTTP API has http.cors enabled with no api_key configured. Any page the operator visits can read the configuration endpoints, credentials included, cross-origin. Set http.api_key, or disable http.cors." + ); + } + + if beyond_loopback && !config.tls.enabled { + warn!( + "{NAME} HTTP API is enabled on {} with http.tls disabled. The api-key header and the configuration responses carrying connector credentials both cross the network in cleartext. Enable http.tls, or bind the API to loopback.", + config.address + ); + } +} + +/// Whether `address` resolves to anything outside loopback. /// /// Resolves rather than parses because `address` is a free-form `String` that /// takes a hostname, as `[iggy] address` does in the same file. Not because the @@ -150,12 +171,10 @@ pub async fn init(config: &HttpConfig, context: Arc) { /// Classification only. Do not bind what this resolves: `TcpListener::bind` /// walks every resolved address and takes the first that works, so collapsing /// to one would drop the `localhost` -> `[::1, 127.0.0.1]` fallback on hosts -/// with IPv6 disabled. -async fn is_unauthenticated_beyond_loopback(config: &HttpConfig) -> bool { - if !config.api_key.expose_secret().is_empty() { - return false; - } - let Ok(resolved) = lookup_host(&config.address).await else { +/// with IPv6 disabled. The cost is resolving twice at startup, which is the +/// trade for keeping that fallback. +async fn resolves_beyond_loopback(address: &str) -> bool { + let Ok(resolved) = lookup_host(address).await else { return true; }; let addresses: Vec = resolved.collect(); @@ -193,15 +212,10 @@ mod tests { use tracing_subscriber::filter::LevelFilter; use tracing_subscriber::layer::{Context as LayerContext, SubscriberExt}; - /// Reserved for documentation by RFC 5737, so no host routes it and the - /// bind fails. That is what lets the test reach the warning without - /// listening anywhere: a non-loopback address that binds successfully would - /// put a port on every interface for the life of the test binary. + /// Reserved for documentation by RFC 5737, so the bind fails and the test + /// reaches the warning without listening anywhere. A routable address that + /// binds would put a port on every interface for the life of the binary. const UNASSIGNABLE_ROUTABLE_ADDRESS: &str = "192.0.2.1:8081"; - - /// Port 0 rather than a port reserved by binding and dropping first. The - /// tests never need to know which port it lands on, and reserving one is a - /// race that buys nothing. const EPHEMERAL_LOOPBACK_ADDRESS: &str = "127.0.0.1:0"; type Captured = Arc>>; @@ -214,24 +228,19 @@ mod tests { } } - /// Captures events for the current thread only, for as long as the returned - /// guard lives. - /// - /// Deliberately not a global subscriber: that slot is process-wide, so - /// claiming it would break any later test that installs its own, and a - /// shared buffer would leave every negative assertion hostage to warnings - /// from elsewhere in the binary. + /// Captures events for the current thread only, for as long as the guard + /// lives. Not a global subscriber: that slot is process-wide, and a shared + /// buffer would leave negative assertions hostage to the rest of the binary. /// /// `#[tokio::test]` builds a current-thread runtime, so a task spawned by - /// the test body runs on this thread and sees this subscriber. Under a - /// multi-thread flavour the capture would come back empty and these - /// assertions would fail rather than quietly pass. + /// the test body sees this subscriber. Under a multi-thread flavour the + /// capture would come back empty and these tests would fail, not pass. + /// + /// Filtered rather than checking the level in `on_event`: a layer with no + /// filter reports no `max_level_hint`, which pushes the global max level to + /// TRACE and stops every callsite in the binary short-circuiting. fn capture_events() -> (DefaultGuard, Captured) { let captured: Captured = Arc::new(Mutex::new(Vec::new())); - // Filtered rather than checking the level inside `on_event`: a layer - // with no filter reports no `max_level_hint`, which pushes the global - // max level to TRACE and stops every callsite in the binary from - // short-circuiting. let layer = CaptureEvents { captured: Arc::clone(&captured), } @@ -240,16 +249,18 @@ mod tests { (guard, captured) } - fn warned_about(captured: &Captured, address: &str) -> bool { + fn warnings(captured: &Captured) -> Vec { captured .lock() .expect("the capture mutex is only held to push a line") .iter() - .any(|(level, message)| *level == Level::WARN && message.contains(address)) + .filter(|(level, _)| *level == Level::WARN) + .map(|(_, message)| message.clone()) + .collect() } /// Whether `init` got as far as serving. The positive control for tests - /// whose real assertion is that nothing was warned about. + /// whose real assertion is that something was not warned about. fn started_serving(captured: &Captured) -> bool { captured .lock() @@ -273,13 +284,8 @@ mod tests { } } - /// Every field the event carried, rendered into one line. - /// - /// Unconditional on purpose. These tests only ask whether an event - /// mentioned a given string, so singling out the `message` field would add - /// a branch to the scaffolding whose other side nothing here would ever - /// take. `record_str` needs no impl either: it forwards here by default, - /// and a formatted message arrives as `fmt::Arguments` regardless. + /// Unconditional on purpose: singling out the `message` field would add a + /// branch whose other side nothing here takes. struct Recorded(String); impl Visit for Recorded { @@ -288,14 +294,11 @@ mod tests { } } - /// The cheapest context `init` will accept. Nothing here reaches Iggy: the - /// clients are never connected, and the warning is decided from the config - /// alone. + /// The cheapest context `init` will accept. Nothing here reaches Iggy. /// - /// `api_key` is a parameter rather than always empty because the guard - /// reads `config.api_key` while the middleware enforces `context.api_key`. - /// They come from one binding in `main.rs` today, but a test that set only - /// one of them would be exercising a state the runtime cannot reach. + /// `api_key` is a parameter because the guard reads `config.api_key` while + /// the middleware enforces `context.api_key`; a test that set only one + /// would exercise a state the runtime cannot reach. async fn context(api_key: &str) -> (Arc, TempDir) { let directory = tempfile::tempdir().expect("a temp dir must be available"); let config_provider = @@ -322,36 +325,26 @@ mod tests { } #[tokio::test] - async fn given_loopback_address_and_no_key_when_checked_should_stay_quiet() { - // The shipped posture. Warning here would train operators to ignore it. - assert!(!is_unauthenticated_beyond_loopback(&config("127.0.0.1:8081", "")).await); - assert!(!is_unauthenticated_beyond_loopback(&config("[::1]:8081", "")).await); + async fn given_loopback_addresses_when_classified_should_report_contained() { + assert!(!resolves_beyond_loopback("127.0.0.1:8081").await); + assert!(!resolves_beyond_loopback("[::1]:8081").await); assert!( - !is_unauthenticated_beyond_loopback(&config("localhost:8081", "")).await, + !resolves_beyond_loopback("localhost:8081").await, "`address` accepts a hostname, and parsing alone would misjudge one" ); } #[tokio::test] - async fn given_routable_address_and_no_key_when_checked_should_report_it() { + async fn given_routable_or_unresolvable_addresses_when_classified_should_report_exposed() { assert!( - is_unauthenticated_beyond_loopback(&config("0.0.0.0:8081", "")).await, + resolves_beyond_loopback("0.0.0.0:8081").await, "binding every interface to reach the API from outside a container \ is the case this exists to catch" ); - assert!(is_unauthenticated_beyond_loopback(&config("192.0.2.10:8081", "")).await); - } - - #[tokio::test] - async fn given_configured_key_when_checked_should_stay_quiet_on_any_address() { - assert!(!is_unauthenticated_beyond_loopback(&config("0.0.0.0:8081", "secret")).await); - } - - #[tokio::test] - async fn given_unresolvable_address_when_checked_should_report_it() { - // About to fail the bind regardless, so the warning costs nothing and - // the alternative is silence about an address we cannot classify. - assert!(is_unauthenticated_beyond_loopback(&config("not a valid address", "")).await); + assert!(resolves_beyond_loopback("192.0.2.10:8081").await); + // About to fail the bind regardless, so staying quiet about an address + // we cannot classify is the wrong direction to be wrong in. + assert!(resolves_beyond_loopback("not a valid address").await); } #[tokio::test] @@ -374,7 +367,13 @@ mod tests { it instead, and this test has then started a listener that outlives the run" ); assert!( - warned_about(&captured, UNASSIGNABLE_ROUTABLE_ADDRESS), + !started_serving(&captured), + "the bind must not have completed, or this proves nothing about ordering" + ); + assert!( + warnings(&captured) + .iter() + .any(|warning| warning.contains(UNASSIGNABLE_ROUTABLE_ADDRESS)), "init must consult the guard and name the address it is exposing" ); } @@ -386,18 +385,80 @@ mod tests { init(&config(EPHEMERAL_LOOPBACK_ADDRESS, ""), context).await; - // Positive control first. Without it the assertion below passes for any - // reason `init` might return early, including `enabled` defaulting to - // false, and the only in-`init` loopback coverage would disappear - // silently. + // Positive control first: without it the assertion below passes for any + // reason `init` returns early, including `enabled` ever defaulting to + // false, and the only in-`init` loopback coverage disappears silently. assert!( started_serving(&captured), "init must reach the listener, or the assertion below proves nothing" ); + // Any warning at all, not one matching this address: matching on the + // address goes vacuous the moment the message is reworded. + assert!( + warnings(&captured).is_empty(), + "the shipped posture is loopback with no key; warning about it would \ + teach operators to ignore the ones that matter: {:?}", + warnings(&captured) + ); + } + + #[tokio::test] + async fn given_cors_enabled_and_no_key_when_initialized_should_warn_despite_loopback() { + let (_capture, captured) = capture_events(); + let (context, _directory) = context("").await; + let mut config = config(EPHEMERAL_LOOPBACK_ADDRESS, ""); + config.cors.enabled = true; + + init(&config, context).await; + + assert!(started_serving(&captured)); + assert!( + warnings(&captured) + .iter() + .any(|warning| warning.contains("http.cors")), + "loopback does not contain CORS: a browser is a local process and the \ + layer wraps outside authentication" + ); + } + + #[tokio::test] + async fn given_a_key_but_no_tls_beyond_loopback_when_initialized_should_still_warn() { + let (_capture, captured) = capture_events(); + let (context, _directory) = context("configured").await; + let config = config(UNASSIGNABLE_ROUTABLE_ADDRESS, "configured"); + + let _ = tokio::spawn(async move { init(&config, context).await }).await; + + let warnings = warnings(&captured); + assert!( + warnings.iter().any(|warning| warning.contains("http.tls")), + "setting a key does not stop the key and the credential-bearing \ + responses crossing the network in cleartext" + ); + assert!( + !warnings + .iter() + .any(|warning| warning.contains("no api_key")), + "and the key that was set must not still be reported as missing" + ); + } + + #[tokio::test] + async fn given_a_disabled_api_when_initialized_should_warn_about_nothing() { + let (_capture, captured) = capture_events(); + let (context, _directory) = context("").await; + // Routable, keyless and untrusting in every direction, but switched off. + let mut config = config(UNASSIGNABLE_ROUTABLE_ADDRESS, ""); + config.enabled = false; + config.cors.enabled = true; + + init(&config, context).await; + assert!( - !warned_about(&captured, EPHEMERAL_LOOPBACK_ADDRESS), - "the shipped posture is loopback with no key; warning about it \ - would teach operators to ignore the one that matters" + warnings(&captured).is_empty(), + "an API that is not listening exposes nothing, and warning about one \ + is the false positive that teaches operators to ignore the rest: {:?}", + warnings(&captured) ); } }