fix(ingestor): assign explicit, collision-resistant MQTT client IDs

[dborup]

Relates to #118

Plan and design

The user asked for autonomous work, so the plan is written here instead of waiting for sign-off (AGENTS.md rule 5).

Commits:

1. 78fd9c56: tests (they do not compile on master, because MQTTSource has no ClientID).
2. b493dada: the fix.
3. a0f190cf (review round 1): no MQTT log line contains credentials from the broker URL, including the tag of an unnamed source and a broker without a scheme.
4. 8698783d: test helper rename.
5. 0f40395 (review round 2): the client-ID uniqueness test no longer flakes (see "Tests").
6. 07f3481 (review round 2): credentials out of /api/mqtt/status, /api/healthz, the stats file and the tags; see "Credentials in status data" below.
7. 2721749 (review round 3): names the operator chose are kept, masked credentials show as ****@, known secrets are masked in errors, and a foreign stats tmp file is refused, also as root. See "Review round 3".
8. 3ff6050 (review round 3): two tests that catch the mutants that survived 2721749.

Claims in the issue, verified against master d264716c

• MQTTSource has no clientId field (cmd/ingestor/config.go).
• buildMQTTOpts() never calls SetClientID (cmd/ingestor/main.go). paho v1.5.0 therefore connects with a zero-length client ID and CleanSession=true.

The change

Configured ID. New optional mqttSources[].clientId, used verbatim. It is documented as needing to be unique among the broker's concurrent clients. A whitespace-only value counts as unset, so a blank template value cannot become a shared ID.

Generated default (new file cmd/ingestor/mqtt_client_id.go): corescope-<name>-<8 hex>.

• <name> is the sanitized source name: lowercased, [a-z0-9] kept, every other run of characters turned into one -, capped at 32 characters.
• If the name is empty, <name> is the host of the masked broker (no port). Since round 2 the host comes from the masked broker, because url.Parse takes the user name for the host when a password contains an unescaped /, ? or #.
• If that is empty too, the ID is just corescope-<8 hex>.
• The suffix is 32 random bits from crypto/rand. Go 1.22 is pinned in go.mod, so crypto/rand can still return an error. In that case a clock + process-counter fallback keeps IDs unique within the process.

Stable across reconnects. The default is generated once per client construction, inside buildMQTTOpts:

• paho reuses the options for its own auto-reconnect.
• The watchdog force-reconnect (buildForceReconnectFn) reuses the same client.

So the ID is stable for the process lifetime but differs between sources and processes.

Logging (credential-free).

• The connect log line is MQTT [tag] connected to <broker> as client <id>.
• Broker in logs. brokerForLog replaces user-info with **** and drops query and fragment (brokerurl.Mask, since round 3). A broker without a scheme is read as tcp://, as paho's AddBroker does.
• The connected, disconnected, reconnecting and connection attempt #N lines and the watchdog lines (via the liveness state's Broker) all use the masked broker. Disconnect and connect errors are masked too, in case they quote a URL or one of the source's secrets.

Documentation. config.example.json documents clientId in _comment_mqttSources without a literal value, because copied deployments must not share an ID. cmd/ingestor/README.md documents the key, the tag of an unnamed source and what the stats file contains.

Unchanged. Topics, credentials, CleanSession, TLS, keepalive and the watchdog.

Credentials in status data (review round 2)

The public /api/mqtt/status (no API key) served the ingestor's raw broker URL. The server only masked scheme://user:pass@, so these went out unmasked: user:secret@host:1883 (no scheme), tcp://token@host (user name or token only), wss://host/mqtt?token=abc (query), and tcp://user:p%zz@host (bad %-escape, url.Parse fails). The problem existed before this PR. It is fixed at both layers:

• Shared package internal/brokerurl (new; Mask, MaskText, and since round 3 Secrets; Strip was removed in round 3 together with its last caller), used by the ingestor and the server.
  • It does not use url.Parse. Everything up to the last @ is user-info, and the query and fragment are dropped.
  • If in doubt, it shows too little: a broker with an @ in its path or query shows only what follows that @, after ****@.
  • Wired into both go.mod files, both Dockerfile builder stages (check-dockerfile-internal-pkgs.sh passes) and the CI step that tests the shared packages.
• Ingestor (fixes the source; the server layer is described below).
  • RegisterSourceStatus stores the masked broker (brokerForLog, the same function the log uses), whoever calls it. main passes the masked one as well.
  • A disconnect error is masked before it is stored or logged.
  • So the stats file (source_statuses[].broker/name/lastError and the source_liveness keys) carries no credentials.
  • The existing TestSourceStatus_BasicLifecycle pinned raw passthrough ("server masks"). It now expects the masked broker, with the reason in a comment.
• Server. It masks again, whatever the ingestor sent.
  • broker: all user-info becomes **** (a lone user name too), and the query and fragment are dropped, also without a scheme and on parse errors.
  • name and the /api/healthz ingest_liveness keys: an older ingestor tagged an unnamed source with its raw broker. Since round 3 only such raw-broker names are masked (see below). Masked keys that coincide get (2), so no entry is lost.
  • lastError: each broker URL or user-info in the text is masked, and the rest of the message is kept.
  • The existing server tests expected the user name to be shown (mqtt://u:****@host). They now expect mqtt://****@host: the endpoint is public, and a user name alone is often the credential.
• Other endpoints. I checked all of cmd/server. Only /api/mqtt/status and /api/healthz (tags as keys) show broker data. /api/perf/write-sources shows counters only, and no endpoint exposes the mqttSources config.
• Stats file permissions. Mode is 0o600. A stale tmp file used to keep its own mode through the rename, so the writer now forces 0o600. Since round 3 a tmp file owned by another user is refused before anything is changed, also as root (see below).
• brokerForLog used to leak when an unescaped password contained /, ? or # and the part before it was digits only.
  • tcp://user:2024/secret@host:1883 was logged unchanged; it now becomes tcp://****@host:1883.
  • tcp://user:1234?abc@host became tcp://user:1234; it now becomes tcp://****@host.
  • The suggested rule "fall back if an @ is still in the result after parsing" would not catch the second case, because its @ ends up in the dropped query. That is why the package always cuts at the last @.
• Unique tags. Two unnamed sources on one host with different credentials had the same tag. The second got a "tag collision" ERROR, no watchdog tracking, and merged counters.
  • Now an unnamed source whose tag is taken gets (2), (3), … Tags of named sources are reserved first.
  • A duplicate name is left alone: that stays a config error, reported as before.
• Runtime coverage. main's per-source setup (options, status, liveness, the connect, connection-lost and reconnecting handlers) moved unchanged to prepareMQTTSource.
  • A test runs it against the loopback broker with credentials in the URL: connect, a broker-side drop, paho's reconnect and resubscribe.
  • It checks every log line, the status registry and the liveness keys.
  • This catches any path from the raw broker to a log line, including fmt.Sprint(source.Broker) in the disconnect handler.

Review round 3

1. Names the operator chose are kept (N1). Round 2 masked every name with @ or ://, so obs@north became ****@north, Feed @ CPH became ****@ CPH, and in /api/healthz names that masked to the same value got (2). Now a name or liveness key is masked only when it is a raw broker URL:

   • it equals one of the raw brokers in the stats file's source_statuses (an older ingestor wrote the raw broker as the tag of an unnamed source), or
   • it contains ://.

   /api/mqtt/status and /api/healthz (on a cache refresh) build the same set of raw brokers from the stats file (rawBrokerSet).

2. ****@ instead of a silent cut (N2). The ingestor's logs, tags, status registry and stats file use brokerurl.Mask instead of Strip:

   • mqtt://u:p@broker:1883 becomes mqtt://****@broker:1883.
   • wss://host/mqtt?u=me@x.org becomes wss://****@x.org instead of wss://x.org.

   The operator can see that the URL carries credentials and that the host may be cut short. Strip had no caller left and was removed.

3. Known secrets masked in errors. errForLog first replaces the source's secrets with ****, longest first, then runs MaskText. The secrets are password, username, and the user-info, query and fragment of the broker URL as configured (new brokerurl.Secrets). Only non-empty values count. The disconnect handler (log and lastError) and main's "connection failed" line pass them. This closes the gap where a query token quoted without its scheme passed MaskText.

4. Stats tmp file as root (N3). The round-2 text said the writer "gives up for a tmp file it does not own". That held only because chmod failed, which does not happen as root, and both binaries run as root in Docker. The writer now:

   • opens without O_TRUNC,
   • checks the owner with fstat against geteuid (fileOwnerUID, with a Unix and a Windows file),
   • refuses another user's file before anything is changed,
   • and only then sets chmod 0o600 and truncates.

   The code comment and cmd/ingestor/README.md now describe this.

5. Nit. A deterministic test injects clientIDRandom with de ad be ef and requires the suffix deadbeef.

Perf. No hot path is touched: MarkPacket is unchanged. The ingestor masks once per source at startup. Each disconnect makes one sort and one replace over a handful of secrets. The server masks per /api/mqtt/status request: one entry per source, typically 1–5, plus one set of the same size. For /api/healthz it masks only when the liveness cache is refreshed, not per probe; it decodes the already-read file a second time for the raw brokers.

Legacy Mosquitto and MQTT 3.1

• The legacy single-broker mqtt block becomes source default and gets corescope-default-<8 hex> (26 characters).
• MQTT 3.1.1, which paho tries first, only guarantees 1–23 characters, although Mosquitto and EMQX accept longer IDs over 3.1.1.
• paho falls back to MQTT 3.1 after a failed first handshake, and a strict 3.1 broker rejects IDs over 23 characters.
• Before this change the fallback sent an empty ID, which 3.1 forbids as well, so nothing gets worse.
• For such brokers the documentation says to set a short clientId. The length is not capped automatically, because that would cut the readable source name to 6 characters for everyone.

How this differs from upstream Kpa-clawbot/CoreScope#2016

Upstream is read as a reference only; nothing was cherry-picked.

	Upstream	This PR
Suffix	24 bits (6 hex)	32 bits (8 hex)
Name part	Mixed case	Lowercased, collapsed separators, length cap
Whitespace-only clientId	Used as the ID	Treated as unset
crypto/rand failure	Not handled	Clock + counter fallback
Logged broker and tag	Broker URL as configured	User-info as ****, no query or fragment in any line, tag, status entry or stats file

Both use a loopback broker test with auto-reconnect and watchdog force-reconnect. This PR additionally checks that a second client for the same source gets a different ID.

Acceptance criteria

Criterion	Status	Evidence
Optional mqttSources[].clientId, used verbatim and documented as unique	Met	TestMQTTClientIDConfiguredIsVerbatim_118, README, config.example.json
Omitted: non-empty, collision-resistant ID from the sanitized name → broker host → corescope, plus crypto/rand	Met	TestMQTTClientIDDefaultShape_118 (8 cases), TestMQTTClientIDUniqueAcrossConstructions_118, TestMQTTClientIDRandomFailureStillUnique_118, TestMQTTClientIDBaseHasNoCredentials_118, TestMQTTClientIDSuffixUsesAllRandomBytes_118
Generated once per construction; stable across paho auto-reconnect and watchdog force-reconnect; unique across sources and processes	Met	TestMQTTClientIDStableAcrossReconnects_118 (loopback broker)
Client ID logged on connect without credentials or tokens	Met	TestMQTTConnectedLogLine_118, TestMQTTLogLinesHaveNoCredentials_118, TestMQTTSourceWiringLeaksNoCredentials_118, TestMainLogsNoRawBroker_118, TestBrokerForLogAmbiguousPassword_118, TestBrokerForLogMarksRemovedUserinfo_118
No literal default ID in config.example.json	Met	TestConfigExampleHasNoLiteralClientID_118
Topics, credentials, clean session, TLS and watchdog preserved	Met	TestMQTTClientIDKeepsOtherOptions_118; existing TestBuildMQTTOpts_* and force-reconnect tests pass
Legacy MQTT 3.1 length constraints documented	Met	mqtt_client_id.go header, README, config.example.json
Round 2: no credentials in /api/mqtt/status, /api/healthz or the stats file; unique tags	Met	internal/brokerurl tests, mqtt_status_118_test.go (server), mqtt_status_credentials_118_test.go (ingestor)
Round 3: chosen names kept, ****@ marker, known secrets masked in errors, foreign tmp refused as root	Met	TestMqttStatusKeepsChosenNames_118, TestIngestLivenessKeepsChosenNames_118, TestMaskSourceName_118, TestErrForLogMasksKnownSecrets_118, TestDisconnectErrorMasksKnownSecrets_118, TestWriteStatsAtomicRefusesForeignTmp_118, TestWriteStatsAtomicRefusesForeignTmpAsRoot_118, TestWriteStatsAtomicTruncatesOwnStaleTmp_118, TestSecrets

Tests

New tests in round 2 (all red before the fix, for the right reasons; green after):

• internal/brokerurl/brokerurl_test.go: Mask and MaskText tables, and idempotence.
• cmd/server/mqtt_status_118_test.go:
  • The four forms above and the ambiguous passwords.
  • lastError/name text.
  • The handler, fed a stats file as an older ingestor writes it.
  • The ingest_liveness keys.
  • maskSourceName.
• cmd/ingestor/mqtt_status_credentials_118_test.go:
  • brokerForLog for both P3 examples.
  • The client-ID host.
  • Unique tags.
  • The status registry and lastError.
  • The prepareMQTTSource runtime test against the loopback broker.
  • The stats file, written by StartStatsFileWriter: its content, and mode 600 with a stale 644 tmp file present.
• The go/ast test now also checks mqtt_source.go and rejects RegisterSourceStatus(…, X.Broker).

Round 3 (2721749, 3ff6050):

• New: cmd/ingestor/mqtt_credentials_r3_118_test.go, three server tests (TestMqttStatusKeepsChosenNames_118, TestIngestLivenessKeepsChosenNames_118, a rewritten TestMaskSourceName_118), and TestSecrets. TestStrip was folded into TestMask, now with the Mask results.
• Updated: the round 2 expectations for brokers and tags (tcp://host → tcp://****@host, and so on), plus the loopback test, which now requires connected to tcp://****@<addr>.
• Red before the fix (the signatures were scaffolded with the old behaviour so the tests compiled):
  • 12 ingestor tests, TestSourceStatus_BasicLifecycle, 3 server tests and TestSecrets.
  • For example, maskSourceName("obs@north") = "****@north", brokerForLog("wss://host/mqtt?u=me@x.org") = "wss://x.org", errForLog("dial host/mqtt?token=abc: refused") unchanged, and writeStatsAtomic accepted a tmp file owned by another user (as root).
  • TestMQTTClientIDSuffixUsesAllRandomBytes_118 was green on the existing code, as intended; mutants back it up.

Adjusted tests:

• TestMain_StartsRouteMaskBackfillAfterBufferReady read c.Subscribe( from main.go as its marker for "MQTT is set up before Ready()". That code moved to prepareMQTTSource, so the marker is now the call to it, and the test checks that the function subscribes.
• TestMQTTClientIDUniqueAcrossConstructions_118 made 5000 draws of a 32-bit suffix. By the birthday bound that repeats in about 1 run in 350: the -race -count=10 run hit it, and -count=200 reproduced it 3 out of 3 times. It now makes 200 draws, which puts the chance near 5·10⁻⁶ (-count=2000: ok).
• The loopback test broker answers SUBSCRIBE. Log capture uses a locked buffer: the first -race run flagged the test reading while paho logged.

Mutants, round 2: 23, one per point at least, 22 caught by the tests.

• The survivor is the main-side call RegisterSourceStatus(tag, source.Broker). It is equivalent, because the function masks the broker itself. The static test now rejects it as well.
• Caught examples:
  • Status registry keeps the raw broker.
  • Server broker unmasked, query kept, first @ used instead of the last, scheme-less user-info kept.
  • name or lastError unmasked.
  • ingest_liveness keys unmasked or merged.
  • brokerForLog back on url.Parse.
  • Client-ID host from raw url.Parse.
  • No tag suffix; named tags not reserved.
  • fmt.Sprint(source.Broker) in the disconnect log.
  • Raw broker in the reconnect or connect line.
  • No chmod.

Mutants, round 3: 21, at least two per point. All are caught on 3ff6050.

Point	Mutants
1	@ rule back; no raw-broker set (:// only); healthz without the set; handler with an empty set
2	brokerForLog drops the ****@; Mask without the marker
3	Secrets ignored; disconnect status without secrets; disconnect log without secrets; no longest-first order; Secrets without the query; password not among the secrets
4	No owner check; owner unknown on Unix; O_TRUNC back before the check; no Truncate
5	Suffix from 3 bytes; 4th byte replaced

• On 2721749, two mutants survived: no longest-first order, and no Truncate. 3ff6050 adds a test for each:
  • A user name that starts the URL's user-info.
  • A long stale tmp file of the ingestor's own.
• A first variant of the "handler with an empty set" mutant failed only at compile time, so it was replaced by rawBrokerSet(nil), which the tests catch.

Runs on 3ff6050:

• On a merge with origin/master be35eefb (clean, no conflicts; deploy.yml fork-guard lines unchanged):
  • go test -race -count=1 ./...: ingestor ok (619 s), server ok (787 s).
  • New and affected tests with -race -count=10: ingestor ok, server ok.
  • internal/brokerurl: go test ./... ok, -race -count=10 ok.
• The same -count=10 runs on the branch itself: ok.
• go vet is clean for the ingestor (linux and darwin; windows builds), the server and internal/brokerurl.
  • GOOS=windows go vet stops at the existing channel_proposals_test.go (syscall.Kill), which this PR does not touch.
• New and changed files are gofmt-clean. source_status.go and perf_io.go have the same gofmt diffs as before; none of them come from this PR.
• The root test TestWriteStatsAtomicRefusesForeignTmpAsRoot_118 ran as root here. In CI, where it is not root, it is skipped, and TestWriteStatsAtomicRefusesForeignTmp_118 (an injected euid) covers the check instead.

Not verified

• The staging test against both brokers follows after this round and is not part of this PR round. It covers whether the generated IDs are accepted, whether they collide with other clients or the broker's ACL/auth setup, and whether reconnects keep the ID. Only a loopback broker was used here; no live broker was contacted.
• Behaviour against a strict MQTT 3.1-only broker.
• MaskText limitation: user-info without a scheme that contains whitespace cannot be told apart from free text, so only its last part is masked. This only affects lastError. broker and a raw-broker name are masked as whole URLs, the source's own secrets are now replaced literally first, and paho's disconnect errors do not quote the broker.
• Literal secret masking over-masks short values. A one-letter user name masks every such letter in the error text. That is the safe side, but it can make an error hard to read.
• Change of deployment user. If the deployment user changes from a non-root user to root, a stale tmp file from the old user is now refused. The stats file then stays stale, and every write logs [stats-file] write …: … belongs to uid N, until that file is removed. Before round 3, a root ingestor took the file over. The reverse direction (root to non-root) failed before as well.
• A hard link at the tmp path to a root-owned file is not checked against (nlink). This predates the PR, and fs.protected_hardlinks usually prevents it.

Overlap with other open PRs

• cmd/ingestor/config.go and cmd/ingestor/main.go: PR fix(ingestor): warn, throttled and bounded, when observerIATAWhitelist drops a region #134 (ingestor: warn when observerIATAWhitelist drops a region without unbounded throttle state #110, IATA whitelist warning, since merged) edited them in different hunks. The merge with master is clean; its new IATA-drop warning logs only the tag, which is now credential-free.
• config.example.json: fix(ingestor): warn, throttled and bounded, when observerIATAWhitelist drops a region #134 documents iataWarnIntervalSec; this PR extends _comment_mqttSources. Different lines.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UNatSvATYtHfy9XDaFaC1d

[dborup]

Independent review of b493dada

Verdict: APPROVE with nits. This is a recommendation only; merging is the owner's call.

Reviewed head: b493dada6236be2d539e4840703d6a610e80e6c5 (unchanged before and after the review). Work done on a git archive of the head, of commit A 78fd9c56, of origin/master ad011021, of the merge-base d264716c, and of a stacked merge tree with PR #134.

Labels: [F] freshly verified by me · [T] taken from the PR text · [A] assumption · [K] known limitation.

Findings

1. P3. The connect log line still leaks credentials in two cases cmd/ingestor/main.go:139-142, cmd/ingestor/main.go:163, cmd/ingestor/mqtt_client_id.go:108-118. The PR says credentials or device tokens in the broker URL "do not reach the log". That holds only for the connected to <broker> part:

   • Unnamed source. When mqttSources[].name is empty, tag = source.Broker, and the tag is printed raw. A reviewer probe (with made-up credentials) printed MQTT [tcp://dev-user:tok3n-secret@broker.example:1883] connected to tcp://broker.example:1883 as client corescope-broker-example-5748c91a [F].
   • Scheme-less broker. brokerForLog("dev-user:tok3n-secret@broker.example:1883") returns the string unchanged. url.Parse reads dev-user as the scheme, so u.User is nil. paho accepts this form, because AddBroker prepends tcp:// [F].
   • Query-string tokens. A token in the query string (wss://…/mqtt?token=…) is also logged unchanged [F]. I found no device-auth config in this fork that uses one, so this one is [A] low risk.

   The same raw tag and broker already appear in the disconnected, reconnecting and connection attempt lines on master. That part is pre-existing and the PR's "Not verified" section admits it [K]. The criterion covers the connect line, though, and the claim in the PR body overstates what that line guarantees. Cheap fix: sanitize the fallback tag with brokerForLog when the name is empty, and normalize scheme-less input the same way paho does. TestMQTTConnectedLogLine_118 covers only a named tag and a URL that has a scheme.

2. P3. The main.go wiring of the connect log is untested cmd/ingestor/main.go:145,163. Mutant M11 put back the old connected to %s line with no client ID, and every test still passed [F]. Only the helper mqttConnectedLogLine is tested. This is low risk, since it is one line inside main().

3. nit. The ID-length claim in the PR body is wrong, and the 3.1.1 caveat could be stated more plainly. The body says the legacy source gets corescope-default-<8 hex> "(24 characters)". It is 26. Measured lengths [F]:

   Source	Length
   default (legacy block)	26
   local (no config)	24
   env	22
   name of 4 characters	23

   Default IDs also contain -, which is outside the 1–23 [0-9a-zA-Z] set that MQTT 3.1.1 servers must accept ([MQTT-3.1.3-5]). Longer IDs and other characters are only a "MAY". The code header mqtt_client_id.go:30-35 states this correctly. README.md:94 frames the limit as a strict-MQTT-3.1 issue only. Mosquitto, EMQX, HiveMQ and VerneMQ accept these IDs [A]. Staging is listed as not verified [T].

4. nit. The legacy mqtt block and the MQTT_BROKER env source cannot set clientId cmd/ingestor/config.go:44-47,310-320. The docs tell strict-broker users to set a short clientId. Legacy-block and env users can only do that by moving to mqttSources, and the docs do not say so [F].

No P1 or P2 findings.

Metadata

• CI on b493dada is complete: Go Build & Test, Playwright E2E and Docker are SUCCESS; the rest are SKIPPED [F].

• Head at start and at end: b493dada6236be2d539e4840703d6a610e80e6c5 (gh pr view 141 --json headRefOid) [F].

• Commits in origin/master..head [F]. Both have author and committer dborup <kontakt@meshview.dk>:

  • 78fd9c56 test(ingestor): pin explicit, collision-resistant MQTT client IDs (ingestor: assign explicit, collision-resistant MQTT client IDs #118)
  • b493dada fix(ingestor): explicit, collision-resistant MQTT client IDs (ingestor: assign explicit, collision-resistant MQTT client IDs #118)

• Files changed [F]:

  • cmd/ingestor/README.md (+1)
  • cmd/ingestor/config.go (+4)
  • cmd/ingestor/main.go (+6/-2)
  • cmd/ingestor/mqtt_client_id.go (new, 118 lines)
  • cmd/ingestor/mqtt_client_id_118_test.go (new, 319 lines)
  • config.example.json (+1/-1)

• .github/workflows/deploy.yml and cmd/server are not touched [F].

• The merge-base is d264716c, 3 commits behind origin/master ad011021. Those commits touch only public/, test-all.sh, deploy.yml and one JS test [F].

• git merge-tree --write-tree origin/master b493dada is clean (tree 4cdc1d0c) [F].

• Stacked with PR fix(ingestor): warn, throttled and bounded, when observerIATAWhitelist drops a region #134 (origin/codex/issue-110-iata-whitelist-warning d9f4c328) [F]:

  • master → fix(ingestor): warn, throttled and bounded, when observerIATAWhitelist drops a region #134 → this PR is clean.
  • master → this PR → fix(ingestor): warn, throttled and bounded, when observerIATAWhitelist drops a region #134 is clean.
  • Both orders give the identical tree 263ab6a4.
  • On that tree, go vet is clean and all _118 and IATA tests from both PRs pass. config.example.json still parses, because TestConfigExampleHasNoLiteralClientID_118 passes.

• PR body [F]:

  • It says "Relates to ingestor: assign explicit, collision-resistant MQTT client IDs #118", with no closing keyword, and closingIssuesReferences is empty.
  • No @mentions.
  • Upstream appears only as code-formatted Kpa-clawbot/CoreScope#2016, with no github.com/Kpa-clawbot URL.
  • The PR is a draft.

Acceptance criteria (issue #118)

Criterion	Result
Optional mqttSources[].clientId, used verbatim, documented as unique	Met [F]. config.go:28-31, mqtt_client_id.go:50-52, README.md:94, config.example.json:373. A whitespace-only value counts as unset (M4 caught). A value with padding such as " edge " is kept verbatim, spaces included, which matches the criterion.
Omitted: non-empty, collision-resistant ID from sanitized name → broker hostname → corescope, plus crypto/rand	Met [F]. mqtt_client_id.go:53-99. The suffix is 32 bits of crypto/rand. On a failure, a clock+counter fallback kicks in (M7 caught). The hostname comes from u.Hostname(), with no port and no user-info (M8 and M9 caught). No credentials reach the ID (test at _test.go:53). Collision odds with 1,000 concurrent same-name instances are about 1.2e-4 (birthday bound) [F, calculated].
Generated once per construction; stable across paho auto-reconnect and watchdog force-reconnect; unique across sources and processes	Met [F]. The ID is set in buildMQTTOpts (main.go:554), and paho copies the options into the client (client.go:154). The loopback test covers first connect, auto-reconnect, force-reconnect and a second client. Mutant M10, which regenerates the ID in OnReconnecting, is caught. I also ran -race -count=10 on that test: 10/10 pass. The ID changes on restart. That is harmless because CleanSession=true stays and subscriptions are QoS 0 (main.go), so no persistent session is lost [F].
Log the selected client ID on connect without credentials or tokens	Partially met [F]. The client ID is logged and user-info is stripped from the broker part. Credentials still leak through the tag for unnamed sources and through scheme-less or query-token URLs (finding 1).
No literal default ID in config.example.json	Met [F]. The file only documents the key in _comment_mqttSources, and a test enforces this.
Topics, credentials, clean-session, TLS and watchdog preserved	Met [F]. Only the SetClientID link was added to the chain. TestMQTTClientIDKeepsOtherOptions_118 covers this (M13 CleanSession=false caught). The existing TestBuildMQTTOpts_* and ForceReconnect tests pass.
Document legacy MQTT 3.1 length constraints	Met [F]. mqtt_client_id.go:30-35, README.md:94, config.example.json:373. For wording nits, see findings 3 and 4. I checked paho v1.5.0 client.go:412-424: it tries 3.1.1, falls back to 3.1 only when the first handshake fails, then pins the version. That matches the PR text.

Test-first and mutants

• Commit A (78fd9c56), which is master plus the new test file, does not compile: unknown field ClientID in struct literal of type MQTTSource and more [F].
• A behavioural red comes from mutant M1, a revert of SetClientID. It fails 6 of the 11 new tests, including the loopback test ("the client connected with an empty client id") [F].
• Tests changed between A and head [F]. TestMQTTClientIDRandomFailureStillUnique_118 was strengthened: it now freezes the clock through the new clientIDNow hook and checks 2000 constructions instead of 2. That is justified, because it now tests the counter fallback on a coarse clock.
• All mutants were run on a copy of the head with -run '_118|BuildMQTTOpts|ForceReconnect'. Afterwards, each file was restored and checked with shasum against git show b493dada:<path>: they match for main.go, config.go and mqtt_client_id.go [F].

#	Mutant	Result
M1	Remove SetClientID (revert the fix)	Caught (6 tests, including the loopback test)
M2	Constant suffix 00000000	Caught (Unique, StableAcrossReconnects, RandomFailure)
M3	Configured clientId ignored	Caught (ConfiguredIsVerbatim)
M4	Whitespace-only clientId used verbatim	Caught (BlankConfiguredIsGenerated)
M5	u.User = nil removed (user-info logged)	Caught (ConnectedLogLine)
M6	Length cap removed	Caught (LongNameIsCapped)
M7	Fallback counter removed	Caught (RandomFailureStillUnique)
M8	Hostname fallback removed	Caught (DefaultShape)
M9	u.Host instead of u.Hostname()	Caught (DefaultShape)
M10	ID regenerated on every reconnect (OnReconnecting)	Caught (StableAcrossReconnects)
M11	main.go connect log reverted to the old line	Survived: a real test gap (finding 2); main() wiring is not covered
M12	No - separators in the sanitizer	Caught (DefaultShape)
M13	SetCleanSession(false) added	Caught (KeepsOtherOptions)

Suites run locally

• Head: cd cmd/ingestor && go test -race -count=1 -timeout 60m ./... returned ok github.com/corescope/ingestor 776.731s (exit 0) [F]. The PR reports 1074 s on its machine [T].
• Master: I did not run the full suite on master, because nothing failed on the head.
• Stacked tree (fix(ingestor): warn, throttled and bounded, when observerIATAWhitelist drops a region #134 + this PR): go vet is clean, and the _118, IATA and Drop tests pass [F].
• go vet and gofmt -l on the changed Go files are clean [F].

Performance and security

• Performance. The ID is built once per client construction, at startup: at most 4 bytes of crypto/rand plus string work. The ingest path is not touched, so no proof of performance is needed [F].
• Bounded state. The only new global state is an atomic.Uint64 counter and two test hooks (clientIDRandom, clientIDNow). There are no maps, timers or goroutines [F].
• The loopback test broker closes its listener, its connections and its WaitGroup in t.Cleanup [F].
• Interfaces. No new map[string]interface{} (0 occurrences in the diff) [F].
• Read-only invariant. cmd/server is not touched [F].
• What the ID contains. It holds only the operator-chosen source name or the broker hostname, and it is sent only to that broker. No credentials end up in it: tested, and M8/M9 were caught [F]. The source name can become visible to other clients where a broker exposes client IDs, for example through $SYS or a dashboard. I consider that acceptable, since the name is a label and not a secret [A].
• Log redaction. For the remaining gaps, see finding 1.

Not verified

• Behaviour against the real staging brokers: ID acceptance, ACLs, clientid_prefixes-style restrictions and reconnects. No broker other than the in-test loopback broker was contacted [K], as the PR itself states [T].
• Behaviour against a strict MQTT 3.1 or strict 3.1.1 broker [K].
• The full cmd/ingestor suite on master, which was not needed because the head was green.
• The PR's "Not verified" section is honest. It leaves out the unnamed-source tag leak and the scheme-less case in finding 1, and its "24 characters" figure is wrong (finding 3).

[dborup]

Review feedback addressed (commit a0f190cf)

1. Tag of an unnamed source. tag = source.Broker (main.go, in main() and in buildMQTTOpts) is replaced by mqttSourceTag(source). That is the name, or for an unnamed source the broker without credentials. Named sources are unchanged.
2. Broker without a scheme. brokerForLog now reads a scheme-less broker as tcp://, as paho's AddBroker does, before it strips user-info. It also drops query and fragment. For unparseable input it keeps only what follows the last @.
3. All other log lines.
   • The disconnected, reconnecting and connection attempt #N lines log brokerForLog(...).
   • The liveness state gets the sanitized broker, because the watchdog lines print it.
   • The source-status registry still gets the raw broker. That is not a log line, and the existing test pins "raw URL passthrough (server masks)".
4. Tests. New file cmd/ingestor/mqtt_log_credentials_118_test.go:
   • Tag cases: an unnamed source with user:pass@ in the URL, with and without a scheme; a ?password= query; an unparseable URL.
   • A loopback-broker connect of both unnamed variants, with every captured log line checked.
   • A go/ast check of main.go for raw X.Broker in log calls, in the tag and in the liveness Broker. This also covers the review's M11-style gap in the main() wiring.
   • Before the fix: 4 of 4 new tests red, e.g. MQTT [tcp://dev-user:s3cret-pass@127.0.0.1:…] connection attempt #1 to tcp://dev-user:s3cret-pass@….
   • After the fix: 15/15 _118 tests green.
   • Mutants caught: no scheme normalization, raw broker in the attempt line, raw broker in the disconnected line, and query kept.
   • cd cmd/ingestor && go test -count=1 ./...: ok (135.8 s).
5. PR text. It now says that the staging test against both brokers is done separately afterwards. The "24 characters" figure is corrected to 26.

---

Generated by Claude Code

[dborup]

Review feedback addressed (commit 8698783d)

1. CI fix. a0f190cf failed Go Build & Test on the merge with master. The package test build broke because mqtt_log_credentials_118_test.go declared captureLog, which iata_drop_log_test.go (from fix(ingestor): warn, throttled and bounded, when observerIATAWhitelist drops a region #134, now on master) already declares. I had tested only the branch, not its merge with master.
2. The fix. The helper is renamed to captureLog118. There is no other change.
3. Verified on a local merge of 8698783d with origin/master.
   • go vet is clean.
   • -run '_118|IATA|Drop': ok.
   • Full cd cmd/ingestor && go test -count=1 ./...: ok (140.7 s).

---

Generated by Claude Code

[dborup]

Review feedback addressed (commits 0f40395, 07f3481)

1. P2: public /api/mqtt/status leaked broker credentials (existed before this PR). Fixed at both layers with a new shared package, internal/brokerurl. It does not use url.Parse: everything up to the last @ is user-info, and the query and fragment are dropped.
   • a) The ingestor registers the stripped broker (brokerForLog, the same function the log uses). RegisterSourceStatus strips it itself too, and lastError is masked. The test that pinned raw passthrough (TestSourceStatus_BasicLifecycle) now expects the stripped broker, with the reason in a comment.
   • b) The server masks all user-info (a lone user name or token too), the query and the fragment, also without a scheme and when url.Parse fails. The old tests expected u:****@; they now expect ****@.
   • c) name and lastError are masked as well. Among the other /api endpoints, /api/healthz uses tags as ingest_liveness keys, so those are masked too; colliding keys get (2). No other endpoint shows broker data.
2. P3: brokerForLog. Both examples (user:2024/secret@… and user:1234?abc@…) are now tested. The suggested rule ("an @ is still in the result") would not have caught the second, because its @ ends up in the dropped query. That is why the new code always cuts at the last @. The client-ID host also comes from the stripped broker now.
3. P3: tag collision. An unnamed source whose tag is taken gets (2), (3), … Named tags are reserved first, and a duplicate name stays a config error. Tested, including the watchdog registration of both sources and separate counters.
4. P3: runtime coverage. main's per-source setup moved unchanged to prepareMQTTSource. A test runs it against the loopback broker with credentials in the URL: connect, a broker-side drop, reconnect and resubscribe. It checks every log line, the registry and the liveness keys. fmt.Sprint(source.Broker) in the disconnect handler is caught. The AST test also covers mqtt_source.go and RegisterSourceStatus.
5. Stats file. It is written stripped as a consequence of 1a (tested through StartStatsFileWriter). Mode 0o600 is now forced on a stale tmp file as well; before, a 644 tmp file survived the rename.

Also found and fixed:

• TestMQTTClientIDUniqueAcrossConstructions_118 (from round 1) flaked about 1 run in 350: a 32-bit birthday collision over 5000 draws. It now makes 200 draws.
• A data race in the new test's log buffer.
• The c.Subscribe marker in TestMain_StartsRouteMaskBackfillAfterBufferReady, which moved with the refactor.

Verification:

• All new tests were red before the fix.
• 23 mutants were run, at least one per point. 22 were caught by the tests; the survivor was equivalent and is now caught by the static test.
• On a merge with origin/master 571fb2c8: go test -race -count=1 ./... passes for the ingestor and the server; the new tests pass with -race -count=10.

The staging test against both brokers follows after this round.

---

Generated by Claude Code

[dborup]

Review feedback addressed (commit 2721749, tests completed in 3ff6050)

1. N1: names the operator chose stay as they are. Server maskSourceName and maskLivenessKeys now mask only names that are a raw broker URL:

   • in /api/mqtt/status: a name equal to a raw broker in the stats file's source_statuses (an older ingestor), or one that contains ://;
   • in /api/healthz: the same, based on that file's source_statuses.

   obs@north, obs@south and Feed @ CPH are served unchanged and no longer get (2). Tests: TestMqttStatusKeepsChosenNames_118, TestIngestLivenessKeepsChosenNames_118 and TestMaskSourceName_118.

2. N2: brokerurl.Mask instead of Strip. The ingestor uses it in logs, tags, the status registry and the stats file, so a cut host always shows ****@ in front.

   • mqtt://u:p@broker:1883 becomes mqtt://****@broker:1883.
   • wss://host/mqtt?u=me@x.org becomes wss://****@x.org.

   Tested in the ingestor (TestBrokerForLogMarksRemovedUserinfo_118 and the loopback test) and on the server. Strip had no caller left and was removed.

3. Known secrets in errForLog. It first replaces password, username and the broker URL's user-info, query and fragment with **** (new brokerurl.Secrets). Only non-empty values count, and the longest is replaced first. Then it runs MaskText. Wired into the disconnect handler (log and lastError) and main's "connection failed" line. Tests: TestErrForLogMasksKnownSecrets_118 and TestDisconnectErrorMasksKnownSecrets_118.

4. N3: stats tmp file as root. The writer now opens without O_TRUNC and checks the owner with fstat against geteuid. It refuses another user's file before anything is changed, and only then sets chmod 0o600 and truncates. The code comment and cmd/ingestor/README.md are corrected. Tests:

   • an injected euid, which runs in CI;
   • a real foreign owner via chown, which ran here as root;
   • truncation of a long stale tmp file of the ingestor's own.

5. Nit. TestMQTTClientIDSuffixUsesAllRandomBytes_118 injects de ad be ef and requires deadbeef.

Verification

• The new and changed tests were red before the fix and are green after.
• 21 mutants, at least two per point, all caught. Two that survived on 2721749 led to the tests in 3ff6050.
• On a merge with origin/master be35eefb:
  • go test -race -count=1 ./...: ingestor ok (619 s), server ok (787 s).
  • internal/brokerurl: ok.
  • The new tests with -race -count=10: ok.
• go vet and gofmt are clean on the changed files.

---

Generated by Claude Code

[dborup]

CI on 3ff6050: 🎭 Playwright E2E Tests failed in test-analytics-fluid-charts.js at AC3: layout reflows on resize … precondition failed: expected ≥2 cols at 1300px; got 1. Go Build & Test passed.

I don't think this failure comes from this PR:

• The test is a self-contained file:// harness. It uses only public/style.css, and this PR changes neither that file nor the test: git diff origin/master...HEAD -- public/ test-*.js is empty.
• In the same run, the identical 1300px case passed in an earlier step (viewport 1440 / wrapper 1300px → side-by-side (≥2 cols)).
• CI on master be35eefb is green.
• The harness waits only for domcontentloaded before measuring, so it can measure before the stylesheet is applied. That looks like a timing flake in the test.

No fix exists for it yet. I don't want to widen this PR, so here is a proposal for a separate change: in load(), wait for waitUntil: 'load' instead of 'domcontentloaded'.

I have re-run the failed job once.

---

Generated by Claude Code

[dborup]

Review feedback addressed (commit 41675d98)

Relates to #118. Round 4, finding R1 only; R2, R3, R4 and the FIFO hang are left for separate issues.

1. R1, /api/healthz could serve a scheme-less raw broker tag (a369c239 test, 41675d98 fix). When the stats file has source_liveness but no source_statuses (an ingestor built 2026-06-07..06-12), the raw-broker list was empty and maskSourceName only masked keys holding ://. maskLivenessKeys now takes whether source_statuses was present. When it is absent or empty, every key holding @ is masked with brokerurl.Mask. When it is present, behaviour is unchanged, so names such as obs@north stay as they are. Colliding keys still get (2), (3).
2. Tests (through handleHealthz):
   • TestHealthzMasksBrokerTagsWithoutStatuses_118: source_statuses absent and empty. user:secret@host:1883 and mqtt://u:p@h:1883; the body holds no secret, u:p or user:, and all three keys keep an entry.
   • TestHealthzKeepsChosenNamesWithStatuses_118: with source_statuses present, obs@north is unchanged.
   • TestHealthzMaskedTagCollisionsWithoutStatuses_118: three tags that mask to ****@host:1883 keep an entry each, with (2) and (3).
   • The first and third fail on 3ff60504. Removing the fallback makes them fail again. Applying the fallback with statuses present makes TestHealthzKeepsChosenNamesWithStatuses_118 and the existing TestIngestLivenessKeepsChosenNames_118 fail.
3. Local runs: cmd/server go test ./... ok (2611 passed, 3 skipped) and with -race ok (2611 passed, 403 s); cmd/ingestor go test ./... ok (848 passed, 2 skipped) and with -race ok (848 passed, 358 s). go vet is clean. .github/workflows/deploy.yml is unchanged.