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 6 commits into
Conversation
|
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3804 +/- ##
=============================================
- Coverage 76.58% 18.28% -58.30%
Complexity 1046 1046
=============================================
Files 1347 1345 -2
Lines 171018 146265 -24753
Branches 142372 117619 -24753
=============================================
- Hits 130967 26751 -104216
- Misses 36233 119033 +82800
+ Partials 3818 481 -3337
🚀 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:
|
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
|
All ten addressed in The exposure is not read-only. Correct, and I had the threat model wrong, not just the wording. I traced it: CORS. Confirmed the layer order: TLS. Also correct, and it made the old advice actively harmful: move the address, set the key, leave The endpoint list. Added Blocking Empty iterator. Collecting to The wrong rationale. This one I would not have caught:
Global subscriber. Now per-test:
The missing positive control. The best catch of the set. The loopback test asserted only an absence, so it would have kept passing if Re-mutated the wiring after the harness change, since I had rewritten the capture path: removing the guard from Gate: fmt, sort, taplo, hawkeye, typos, markdownlint, clippy Still open for your call, unchanged: whether #3802's redaction should be a key-name heuristic over opaque |
|
/ready |
github-actions
Bot
added
S-waiting-on-review
|
Ran a self-review over this branch before handing it back, and it caught something I had introduced in the previous commit rather than inherited. Pushed The fix documented three exposure paths and warned about one of them. That commit added the README block naming three ways the containment goes away, and the guard only covered the first. So:
That also resolves the remediation wording you flagged, from the other direction than I first took it. Conditioning TLS advice on
Tests. Added the CORS path, the key-without-TLS path, and All four mutation-checked: disabling either new warning block fails exactly its own test. Deliberately not changed, in case you would rather I did:
Separately, while re-checking a claim in the README block I filed #3848: Gate: fmt, sort, taplo, hawkeye, typos, markdownlint, clippy |
|
/ready |
|
/request-review @hubcio |
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.
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.
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.
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.
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.
e1a3a8a to
b84590a
Compare
|
Rebased onto A correction to my earlier comment on this PR. I explained the
That mechanism is wrong. The observation was real (head project 17.18% against a 75.72% base, hits 120684 to 23540 on a 40-line diff) and Two things from the rebase worth knowing:
Gate on this branch after the rebase: fmt, sort, clippy |
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.