fix(connectors): warn when the runtime API is exposed without a key - #3804
fix(connectors): warn when the runtime API is exposed without a key#3804mlevkov wants to merge 3 commits into
Conversation
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.
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.
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3804 +/- ##
=============================================
- Coverage 75.72% 17.35% -58.38%
Complexity 969 969
=============================================
Files 1322 1320 -2
Lines 159363 137083 -22280
Branches 132746 110466 -22280
=============================================
- Hits 120684 23787 -96897
- Misses 35041 112840 +77799
+ Partials 3638 456 -3182
🚀 New features to boost your workflow:
|
On the Codecov comment aboveIt is a snapshot taken before this PR's Rust coverage upload landed, and Codecov All 22 The 3 genuinely uncovered added lines are the call site in 49 if is_unauthenticated_beyond_loopback(config) {
50 warn!(
54 }The predicate itself has four tests covering the decision table (loopback, IPv6 The |
|
Added the test, so the 3 lines are no longer uncovered. Reaching the warning requires a non-loopback address, and any non-loopback That constraint turns out to give the stronger assertion. The warning is only
The second is the one a plain "does it warn" test would have missed. The loopback case is covered too, asserting the shipped default produces no Capture is via a global subscriber installed once per test binary, because a No production code changed in this commit:
|
7bada32 to
68fd96a
Compare
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.
68fd96a to
5c28f90
Compare
|
/request-review @hubcio |
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
follow-up outside this diff: runtime/src/api/config.rs Debug impl hardcodes "[REDACTED]", which now duplicates the new REDACTED const. same literal is also hardcoded in core/common auth credentials, server state models and the s3_sink URL redaction - worth a small sweep to the const later.
| ``` | ||
|
|
||
| > [!IMPORTANT] | ||
| > **Treat this API as privileged.** The configuration endpoints return plugin |
There was a problem hiding this comment.
the exposure is not read-only. PUBLIC_PATHS in auth.rs is only / and /health, so POST /sinks/{key}/configs, PUT .../configs/active, DELETE .../configs and POST .../restart all sit behind the same empty key.
restart_connector() re-reads the stored config and calls init_sink() with its plugin_config and setup_sink_consumers() with its streams, so rewrite + restart repoints a sink at an attacker destination and forwards your topic data using the runtime's own credentials. the stored path gets dlopened on the next start too.
that changes the decision this doc informs: "local processes can read my secrets" is acceptable on a trusted network, "anyone reachable can repoint my sinks" is not. worth naming write and reconfiguration, not just disclosure.
| > 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. |
There was a problem hiding this comment.
true literally, but a browser is a local process. shipped config pairs [http.cors] enabled = false with allowed_origins = ["*"], configure_cors maps that to AllowOrigin::any(), and the CORS layer wraps outside auth. flip cors.enabled alone and any page the operator visits reads a config endpoint cross-origin - simple GET, no preflight, ACAO: *.
setting api_key closes it (attacker page cannot send the header, gets a 401). chrome's private network access blocks the public-origin case, firefox and safari do not, and a local-origin page bypasses it everywhere. worth one clause saying enabling [http.cors] voids this containment.
| > 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 |
There was a problem hiding this comment.
no mention of http.tls, which ships disabled. the key then travels as a cleartext api-key header, and so do the responses - the verbatim plugin configs this block exists to protect. follow this advice exactly (move address, set key, leave tls alone) and every connector secret goes out in the clear. worth naming http.tls next to api_key.
| > address resolves beyond loopback with no key configured, but nothing prevents | ||
| > it. | ||
|
|
||
| Currently, it does expose the following endpoints: |
There was a problem hiding this comment.
list is missing every mutating route: POST /sinks/{key}/restart, POST /sources/{key}/restart, and DELETE on both configs routes. restart appears nowhere in this file, and the DELETE lines at 127-128 belong to the config provider section. pre-existing, but a GET/POST/PUT-only list right under the new notice is what makes the read-only framing look right.
|
|
||
| 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.", |
There was a problem hiding this comment.
same two gaps as the README block. "can read every connector secret" undersells it - the config POST/PUT/DELETE routes and /restart are behind the same empty key. and "Set http.api_key" omits that the key and the responses cross in cleartext unless http.tls.enabled. message is already long, so maybe "read or rewrite every connector configuration" plus naming http.tls.
| /// 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() { |
There was a problem hiding this comment.
guard reads config.api_key, middleware enforces context.api_key. same immutable binding in main.rs, no reload path, so nothing diverges today - the warning just describes a value it does not read.
not free to change though: sourcing from context means the four cheap sync tests have to build a RuntimeContext (tempdir + async provider). fine as is, but the test helper hardcodes an empty context key with no parameter, so a future init test passing a key would get unauthenticated middleware while the predicate sees the key.
|
|
||
| fn free_port() -> u16 { | ||
| std::net::TcpListener::bind("127.0.0.1:0") | ||
| .expect("the loopback interface must offer a port") |
There was a problem hiding this comment.
bind, read port, drop, let init rebind is a race for no gain - lose it and init panics with an unrelated message. window is small and nothing realistically competes, so this is mostly about deleting code.
the test only asserts nothing warned, so it never needs a known port. "127.0.0.1:0" straight into config() works: parses as loopback so the predicate stays quiet, and the warn interpolates config.address verbatim so warned_about("127.0.0.1:0") still discriminates. deletes this helper and the race.
| fn capture_warnings() { | ||
| static INSTALLED: OnceLock<()> = OnceLock::new(); | ||
| INSTALLED.get_or_init(|| { | ||
| let subscriber = tracing_subscriber::registry().with(CaptureWarnings); |
There was a problem hiding this comment.
set_global_default claims the process-wide subscriber slot for the whole 128-test binary. any future test installing its own subscriber (anything going through init_logging, which calls .init()) hits the expect below. and the shared Vec is never drained, so warned_about on a negative assertion is hostage to warnings from anywhere in the crate.
a per-test Arc<Mutex<Vec<String>>> with a set_default guard drops both, plus the address filtering that only exists because the vec is shared. benchmark.rs has the shape (CaptureLayer + FieldVisitor, no statics), though its capture() takes a sync FnOnce() so it is not directly reusable.
separate, worth doing either way: CaptureWarnings implements neither max_level_hint nor enabled, so the global max level goes to TRACE and every trace!/debug!/info! callsite in the binary stops short-circuiting. scoping does not fix that - .with_filter(LevelFilter::WARN) does, and it deletes the hand-rolled level check below.
| /// 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. | ||
| const UNASSIGNABLE_ROUTABLE_ADDRESS: &str = "192.0.2.1:8081"; |
There was a problem hiding this comment.
holds on stock hosts but not with net.ipv4.ip_nonlocal_bind=1, normal on keepalived/haproxy vip boxes. there the bind succeeds, bind_failed is false, and the assert fires with a message that is now misleading. loud, not silent, so it is a contributor-machine hazard rather than a false green.
small correction to the comment: if the bind does succeed, init has already spawned the server, so the listener stays up for the rest of the test binary, not just this test.
| let address = format!("127.0.0.1:{}", free_port()); | ||
| let (context, _directory) = context().await; | ||
|
|
||
| init(&config(&address, ""), context).await; |
There was a problem hiding this comment.
pure negative assertion with no positive control, so nothing proves init reached the guard. it silently depends on HttpConfig::default().enabled being true, which lives in another file and is never asserted - flip that default (a plausible follow-up to this PR) and init early-returns, the test still passes, and the only in-init loopback coverage quietly disappears. the routable test has assert!(bind_failed) as its control; this one could assert the server came up.
keep the test regardless - it is the only one that kills "init warns unconditionally". mutate the if to if true and everything else stays green.
github-actions
Bot
added
S-waiting-on-author
2 participants
Addresses suggestions 2 and 3 of #3802. Deliberately does not close it —
suggestion 1 (redacting or gating the config responses) needs a maintainer
decision, explained at the bottom.
The gap
Two defaults compose badly.
api_key = ""meansresolve_api_keywaves everyrequest through:
and the configuration routes return plugin configuration exactly as parsed from
TOML — a database connection string, an S3 secret key, a webhook signing secret.
There is no redaction anywhere in
core/connectors/runtime/src.The shipped
address = "127.0.0.1:8081"confines that to local processes, whichis a defensible posture for an admin API and is why this is a hardening gap
rather than a disclosure. The problem is what happens next: changing
addressto0.0.0.0to reach the API from outside a container is an ordinary thing to do,and it silently turns "local processes can read the credentials" into "the
network can", with no signal at any layer.
What this changes
A startup warning when the API is enabled,
api_keyis empty, and theaddress resolves beyond loopback. Warns rather than refuses to start — refusing
would break deployments that are exposed today, and that is the maintainers'
call, not mine.
It resolves the address rather than parsing it, which matters more than it
looks:
HttpConfig::default()islocalhost:8081, which is loopback but is nota
SocketAddr. A parse-based check would fire on the default config and trainoperators to ignore the warning. An address that cannot resolve counts as
exposed, since it is about to fail the bind regardless.
Four unit tests cover the decision table (loopback / IPv6 loopback /
localhost/
0.0.0.0/ a routable IP / key configured / unresolvable). These are the firsttests in
src/api/, so they add amod teststhere. Mutation-checked: droppingthe
api_keyearly return fails the configured-key case.Documentation. The endpoint list said what each route returns but never that
the config routes return credentials, so a reader had no way to know exposing the
port exposes their secrets. Added an admonition under
## HTTP API, and theapi_keycomment inconfig.tomlnow says that empty disables authenticationrather than just calling the key "optional" — plus a note on why the default
address is loopback, which otherwise reads as an arbitrary default rather than
the control doing the confining.
Why suggestion 1 is not here
"Redact credential-bearing fields in the config responses" cannot be done
faithfully at this layer:
plugin_configis an opaqueserde_json::Value, so theruntime has no way to know which keys are credentials. A key-name heuristic
(
*password*,*token*,*secret*,*key*) would both miss fields and redactinnocent ones, and silently returning altered config from an API operators may
read programmatically is its own hazard.
The alternative in the issue — requiring
api_keyfor those specific routesregardless of the global default — is a clean fix but a breaking behaviour change
for anyone consuming those routes unauthenticated today.
Both are defensible; picking between them is a maintainer decision, so I have
left #3802 open rather than guess. Happy to implement either.
Related: #3801, which is why the plugin-side
SecretStringannotations do nothelp here — the runtime never routes through them.
Verification
cargo fmt --all --check,cargo sort --check --no-format --workspace,cargo clippy -p iggy-connectors --all-features --all-targets -- -D warnings,cargo test -p iggy-connectors(126 pass),taplo fmt --check,hawkeye check,typos,markdownlint— all exit 0.