mDNS host discovery and remote multihost pairing - #3429
Conversation
# Conflicts: # packages/cli/src/lib/setup/setupMultihost.ts
The line came in with master but was dropped again while resolving the conflict in 4d48e57, because both sides had touched the same block. Without it every object view comes back empty on macOS and the standard tests end at 377 passing / 101 failing. Becomes unnecessary once the view scripts compare bytes instead of relying on the server collation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QcdCSrTTHNY2SRMUo8aoPc
Apollon77
left a comment
Apollon77
left a comment
There was a problem hiding this comment.
Reviewed with a focus on the network-facing side. tsc --project packages/controller/tsconfig.check.json is clean on this branch (my first run failed only because my local packages/common/build was stale — rebuilding @iobroker/js-controller-common clears it, so the claim in the description holds).
The engineering is careful in the places that are easy to get wrong: re-publishing only on real change instead of on a timer, the optional dependency behind a dynamic import() with local interfaces so it still compiles, isUnclaimed() evaluated against this host's actual state rather than trusting the sender, keeping UDP alongside mDNS for bridged networks. The write-up is also unusually honest about what is missing.
My concern is that the unauthenticated join is filed as "known and deliberately out of scope", and I think the description understates it on both axes:
Reach. pairing defaults to true when absent, and the new pairingOnly listener starts whenever the objects database is local and the multihost service is off. That is the standard single-host installation. So after an upgrade, hosts that previously had nothing listening on UDP 50005 start listening, and "narrows this to hosts without a second host in their system" describes the majority of installations rather than an edge case. The user is never asked.
Impact. "Re-point its databases and restart" undersells the consequence. The objects database is where adapter configuration and installedFrom live, so a host pointed at an attacker's database fetches instance objects from the attacker and the controller then starts and installs adapters accordingly — code execution as the iobroker user, from one unauthenticated UDP packet on the LAN.
There is also a second, independent path to the same outcome that is not in the description: MHClient.connect() accepts the browse answer from any source address and correlates only on a message id that starts at 1, and joinMaster builds a fresh client for every attempt so the first request is always id 2. Detail inline.
My suggestion would be to split this: the announcement, the discovery, identify and the read-only browse are useful on their own and could go in once the spam handling below is addressed. The join write path is the part that needs an anchor first — a post-install pairing window or a pairing code shown on the master — and pairing should default to false until it has one.
Separately, nothing on the UDP or mDNS side is rate limited or bounded, and several handlers do real work per packet: a synchronous read-modify-write of a JSON file, a full getObjectViewAsync('system', 'host'), a log line, a states write. Those are individually cheap to fix and are marked inline.
Everything below is from reading the code on this branch; where I say "no check" I mean I grepped for it and it is not there.
| _config.multihostService?.pairing !== false && | ||
| (await isLocalObjectsDbServer(_config.objects.type, _config.objects.host)) | ||
| ) { | ||
| _startMultihost(_config, false, { pairingOnly: true }); |
There was a problem hiding this comment.
This is the line that decides the blast radius, so it deserves the most attention in the PR.
pairing is absent from every existing iobroker.json, so !== false makes it enabled, and the second condition — a local objects database — is true for every standalone installation. The result is that upgrading to this version opens UDP 50005 on hosts that had nothing listening there before, with no prompt and no changelog-visible opt-in. Combined with the unauthenticated join handler, one packet from anywhere that can route to the host is enough to make it fetch its database configuration from the sender and restart.
I would default pairing to false and let the ready-made-image use case turn it on explicitly (the images ship their own iobroker.json anyway, so they can). That keeps the feature available where it is wanted without changing the exposure of existing installations.
Also: the comment above says "browse is refused in this mode, so nothing is disclosed", but MHServer answers browse in pairingOnly mode with hostname, info (os, ostype, cpus, memory, node version) and uuid to any unauthenticated sender. The MHServerOptions.pairingOnly JSDoc describes it correctly — this comment does not, and it is the one a reader checking the security posture will find first.
| const id = `${rinfo.address}:${rinfo.port}`; | ||
|
|
||
| switch (msg.cmd) { | ||
| case 'join': { |
There was a problem hiding this comment.
The join handler runs before anything has proven who the sender is. msg.password is only forwarded outwards; nothing here verifies it, and masterUuid is only consulted against the local decline list. So the guard is isUnclaimed() alone, which is true for every host that has a local database and no second host — a fresh installation and an ordinary single-host installation.
What happens next is worth spelling out in the description, because "re-point its databases" reads more benign than it is: onJoin pulls objects/states from rinfo.address, writes them to iobroker.json and restarts. From then on the host reads system.adapter.* from a database the attacker controls, which is where installedFrom and every instance's native live. The controller will install and start what it finds there. That is remote code execution as the iobroker user, reachable with a single UDP datagram.
The anchor you sketch in the description — a pairing window after installation, or a code the master must present — is the right shape. Either one also removes the need for isUnclaimed() to carry the whole weight. Until then I would not ship this handler enabled by default.
One more thing on this path: there is no in-flight guard. See the note on joinMaster.
| ): Promise<ReceivedMessage> { | ||
| return new Promise((resolve, reject) => { | ||
| let answered = false; | ||
| const requestId = ++this.id; |
There was a problem hiding this comment.
Anchoring here because the id counter is the part this PR touches, but the finding is about connect() (line 267 in this file), which joinMaster now feeds straight into iobroker.json.
Two gaps there:
- No source check.
startServer's message handler receivesrinfobut never compares it against theipthe request went to, so abrowseanswer from any address is accepted and itsmsg.objects/msg.statesare handed to the callback at line 277. - Predictable id.
idis initialised to1(line 67) and bothconnect()andsendCommand()send++this.id.joinMasterconstructs a freshMHClientper attempt, so the first request is always id 2.
So an attacker on the network who lands a datagram on the ephemeral socket during the ~2 s window — {"cmd":"browse","id":2,"result":"ok","objects":{…},"states":{…}} — beats the real master and decides which databases the host joins. The port is the only unknown, and it is a single 16-bit guess against a window the attacker can trigger repeatedly.
This is pre-existing code, but it used to be reachable only from iobroker multihost connect typed by a human on the box. onJoin makes it remotely triggerable, which is what brings it into scope here.
Both fixes are small — in the connect() handler:
(msg, rinfo) => {
if (rinfo.address !== ip) {
return false; // not the host we asked
}
…and seeding id from crypto.randomInt instead of 1, which also hardens sendCommand on this line.
| return []; | ||
| } | ||
|
|
||
| private addDeclined(masterUuid: string): void { |
There was a problem hiding this comment.
decline is unauthenticated and this is the most abusable handler of the four.
Per packet it does readDeclined() + writeJSONSync() — a synchronous read-modify-write of a file next to iobroker.json, on the event loop, inside the UDP handler. masterUuid is any string of any length, and list has no cap. So a flood of decline packets with random masterUuid values grows the file without bound and stalls the controller on synchronous I/O for as long as the flood lasts. No rate limit, no size limit, no format check.
revoke: true is the same handler and equally unauthenticated, so anyone can also remove a decline the user made — which is the one decision this file exists to remember.
Minimum changes I would want: validate masterUuid against a UUID shape, cap the list length, and move to the async fs API with the write coalesced (or hold the list in memory and flush on a timer). Rate limiting per source address would cover all four commands at once.
| * considers every host with a local database free, which is why it is not used when the | ||
| * controller provides something better. | ||
| */ | ||
| private async isUnclaimed(): Promise<boolean> { |
There was a problem hiding this comment.
isUnclaimed() is called from both the browse and the join handler, and the injected implementation (isHostUnclaimed in main.ts) does getObjectViewAsync('system', 'host') every time, with no caching.
So one unauthenticated UDP datagram costs one full objects-database view query. That is a comfortable amplification factor for an attacker and it needs no reply to be delivered — a flood of browse packets is enough to keep the objects database busy.
The answer changes rarely (only when a host joins or leaves the system), so caching it for a few seconds — or recomputing it on the objectChange for system.host.* and answering from a cached boolean — removes the amplification entirely.
| await publishDiscoveredHosts([]); | ||
| } | ||
|
|
||
| if (!hostDiscovery.isAvailable()) { |
There was a problem hiding this comment.
This return sits before the waitForUuid() kick-off at line 447, so when mDNS is unavailable the UUID is never picked up later.
The sequence on a fresh installation without bonjour-service — the Windows case the description explicitly supports:
updateHostAnnouncement()runs once,system.meta.uuiddoes not exist yet,ownUuidstays'';isAvailable()is false, sohostDiscovery = nulland the function returns;waitForUuid()never starts, andupdateHostAnnouncement()returns early forever after becausehostDiscoveryis null;getUuid: () => ownUuidtherefore keeps returning''for the whole lifetime of the process, so thepairingOnlybrowseanswer has nouuid;- the master cannot key a decline to that host, and the
!host.uuidfilter shows it regardless.
Since ownUuid is also what the UDP path needs, it should not depend on mDNS being usable at all. Resolving it before the availability check — or moving it out of updateHostAnnouncement into its own small resolver — decouples the two.
| config.states.host = replaceListenAll(config.states.host); | ||
|
|
||
| try { | ||
| fs.copyFileSync(configFile, `${configFile}.bak`); |
There was a problem hiding this comment.
There is no in-flight guard on joinMaster, and the join handler can be entered concurrently: it answers ok and then awaits onJoin, so a second datagram arriving in that window starts a second join.
The consequence lands exactly on the safety net this backup is meant to be. Run one: iobroker.json.bak holds the original, iobroker.json holds the master's configuration. Run two overlaps and copies the already rewritten iobroker.json over iobroker.json.bak — so both files now point at the master, and the recovery path the description promises ("without a screen and without SSH a host that cannot reach its master after the restart would otherwise be unrecoverable") is gone.
Two small things fix it: a module-level joinInProgress flag that makes the second join answer error, and refusing to overwrite ${configFile}.bak if one already exists (or writing .bak only when the current config still has a local database).
| if ((entry.uuid && declined.includes(entry.uuid)) || (entry.ip && seen.has(entry.ip))) { | ||
| continue; | ||
| } | ||
| hosts.push({ ...entry, source: 'udp' }); |
There was a problem hiding this comment.
{ ...entry } forwards the whole browse answer to the caller of multihostBrowse. For a master that runs with multihostService.secure: false, MHServer answers browse with objects: this.config.objects and states: this.config.states — database type, host and, for Redis, the credentials in those sections.
So this reply can carry another host's database configuration to whoever sent the host message. The Admin needs hostname, ip, port, unclaimed, uuid, version and source; picking those explicitly instead of spreading keeps the rest from travelling. The mDNS branch above already only has the safe fields, so the two branches would also become symmetric.
Separately, msg.message?.timeout at line 3295 is unbounded and caller-controlled. MHClient.browse resolves only when the timeout expires, and this handler awaits it, so a large value parks the handler and holds the socket for that long. Clamping to something like 10 s is enough.
| return; | ||
| } | ||
|
|
||
| if (txt.uuid && this.announced?.uuid === txt.uuid && hostname === this.hostname) { |
There was a problem hiding this comment.
Both self-filters can miss, so a host can list itself.
ownFqdn is only assigned after a successful publish(), and this.announced is null until then. startDiscovery() and announce() are independent entry points, so if browsing wins the race the host's own announcement passes both checks and is stored under its own fqdn key.
It then stays: once announce() completes, addHost returns at line 359 on the fqdn match, so the stale self-entry is never refreshed and never corrected — it just sits in the list until ENTRY_TTL expires it five minutes later, and in the meantime the Admin offers the host to itself.
Comparing against this.hostname and the resolved ownUuid independently of this.announced, or simply deleting this.hosts.get(fqdn) when ownFqdn is assigned, closes it.
| * | ||
| * A host that is switched off does not always manage to send a goodbye. | ||
| */ | ||
| private expire(): void { |
There was a problem hiding this comment.
On tests — the description says the feature cannot be tested because pairing spans two hosts and ends in a restart, and that is true for the end-to-end flow. But the parts where the bugs above actually live are not the end-to-end flow, and most of them are reachable without any network:
addHost/expire/getKey/removeHosttake a plainMdnsServiceobject. Feeding them a hand-written service (missingproto, no IPv4, duplicate fqdn, attacker-length strings) needs no mDNS at all and would pin the filtering the JSDoc claims.announce()'s change detection — the property that a re-publish only happens on real change, which the description calls out as important — is testable with a stub responder.MHServer's command dispatch can be driven over a loopback UDP socket withisUnclaimed/onJoinstubbed. That coversalready claimed,declined, thepairingOnlyanswer shape and, most usefully, thatbrowseinpairingOnlymode never containsobjects/states— a regression there is silent and serious.
Those three groups are cheap and they guard the security-relevant branches, which is where I would want a test to exist even if the full pairing flow stays manual.
Addresses the review findings on the mDNS discovery / remote pairing PR. Authentication and trust: - join: demand a well-formed masterUuid, so a master the user declined cannot get back in by simply omitting the field - join: refuse unless this host runs in pairing mode - a host that is a multihost master itself is not a pairing target, even while no second host has joined it yet - MHClient.connect/sendCommand: only accept an answer from the address that was asked, and seed the message id randomly instead of from 1 Resource use - all four UDP commands are unauthenticated: - rate limit per sender in front of the command dispatch - decline: validate the uuid, cap the list, and keep it in memory with an async write instead of a sync read-modify-write per datagram - isUnclaimed: cache the answer, it cost one objects view per datagram - identify: log at most once per sender and window mDNS: - addHost: enforce what the JSDoc already promised - protocol version, plausible uuid, bounded string lengths - and cap the number of entries - debounce the change notification, so the state write rate no longer follows whatever the network announces - drop our own announcement from the list once the fqdn is known Correctness: - do not offer a host without a uuid: the decline list is keyed by it, so "No" would silently do nothing and the host would keep coming back - resolve system.meta.uuid independently of mDNS, the UDP browse answer needs it just as much - keep iobroker.json.bak from being overwritten by a second join, and guard joinMaster against concurrent runs - multihostBrowse: pick the fields explicitly, since a browse answer of an unsecured master carries its database credentials; clamp the timeout Pairing stays enabled by default: a freshly installed host has to be attachable from an existing system without a shell on it. Tests: 27 unit tests for the command dispatch, the mDNS filtering and the client, wired into CI on all platforms via cross-env - which also brings the existing adapter unit tests into CI for the first time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
[feature]: mDNS host discovery and remote multihost pairing
Link the feature issue which is closed by this PR
Implementation details
Today a new host can only be attached to an existing multihost system from that host:
iobroker multihost connectneeds a shell on the machine. On a ready-made image, in a container oron a headless device that is exactly what the user does not have.
This PR makes a host announce itself on the network, lets a master list the hosts that belong to no
system yet, and lets the user attach or reject one of them from the Admin — without ever logging
into the new host.
The flow this enables:
restarts. Nothing has to be confirmed on the new host.
1. Host discovery — new module
packages/controller/src/lib/hostDiscovery.tsAnnounces this host via mDNS/DNS-SD and collects the announcements of the others.
_iobroker._tcpon port 50005 — the port the multihost service already listenson, so whoever sees the announcement can talk to that host straight away.
uuid(installation id),host,unclaimed,master,v(controller version),proto(TXT layout version).re-publish as "gone and back again", so doing it on a timer would make every host flap in every
master's list. The periodic part sits on the browsing side instead: the browser re-queries every
60 seconds and every host answers. The same tick asks the controller whether its own announcement
is still accurate — a host stops being unclaimed the moment another host joins it.
a host that is switched off does not always manage to send a goodbye.
The mDNS library is an optional dependency.
bonjour-servicebinds UDP 5353, which does notwork on every system — notably not on some Windows setups. It is declared in
optionalDependenciesand loaded with a dynamic
import(). When it is missing or the responder cannot be started, thecontroller writes one info line and carries on: hosts are then only found over the multihost UDP
protocol, and
discoveredHostsstays empty. So that the module also compiles without the package,the used surface is described by local interfaces rather than a type import.
The discovered hosts are published as JSON in the new state
system.host.<hostname>.discoveredHosts, so the Admin can subscribe instead of polling.2.
MHClientmoved to@iobroker/js-controller-commonpackages/cli/src/lib/setup/multihostClient.ts→packages/common/src/lib/common/multihostClient.ts,exported together with the
ReceivedMessage/BrowseResultEntrytypes. The controller needs thesame client the CLI setup uses, and the controller must not depend on the CLI package.
sendCommand(ip, cmd, payload, timeout)— a unicast command to a single host.browse()returned the server's answer unchanged forresult: 'ok', and that answercarries no address — only the
not authenticatedbranch addedip. Unclaimed hosts always answerok, so the whole pairing flow had no IP to send anything to. The address now comes from thepacket itself.
uuidadded toReceivedMessage;infore-typed, it is an object and was declared asstring.3. New UDP commands in
MHServerbrowsewas read-only: a client asks, the host answers. These three go the other way — a mastertells a host what to do. That host has no states database connection, so this socket is the only way
to reach it.
joinalready claimedordeclined.declinedeclined-masters.jsonand ignores its futurejoins.revoke: trueundoes it.identify4. Pairing mode
When the multihost service is off but the host still uses local databases,
MHServerstarts inpairingOnlymode:browseanswers with hostname, static info,unclaimedanduuid— neverwith the database configuration. UDP is kept alongside mDNS on purpose, because mDNS does not
survive a Docker bridge or a subnet border.
Switchable with the new
"multihostService": { "pairing": false }iniobroker.json(defaulttrue; added toiobroker-dist.json, toIoBJsoninpackages/types-dev/config.d.ts, andschemas/iobroker.jsonregenerated).5. What counts as "unclaimed"
A remote database means the host already belongs to a system. A local database alone is not enough
to call it free — a master runs on a local database too. What disqualifies it is a second host in
the same system: then somebody already joined it, and taking it over would cut that host off from
its data.
Installed adapter instances are deliberately not part of the check. A ready-made image usually
ships with admin and a backup adapter already set up, and that is still a host nobody has claimed.
The check needs the objects database, which
MHServerhas no access to, so the controller injects itvia the new
MHServerOptions.isUnclaimed. The old configuration-only check remains as the fallback.6. Ignore list on the master
declineused to be stored only on the other host, keyed by the master's UUID. That loses thedecision whenever the other host is switched off or the packet is lost.
The master now keeps its own list in
system.meta.discovery(native.declined), keyed by theUUID the remote host announces.
multihostPairwithcmd: 'decline'stores locally first andnotifies the other host afterwards — if it cannot be reached, it stays hidden anyway.
revoke: truetakes it back. Both sources of the discovery list are filtered against it.
7. Host messages
multihostBrowsesource: 'mdns' | 'udp'), filtered against the ignore list.multihostPairjoin/decline/identify. Passes the master'ssystem.meta.uuidso a decline can be assigned.multihostConnectmultihostPairwithjoinruns two preflight checks before sending anything, because ajoinmakes the other host fetch the configuration from us: if this host is not a multihost master, or
its databases only listen locally, there is nothing to fetch. Without the checks the other side
failed seconds later with an unhelpful "invalid configuration" — in a log the master cannot read,
because that host is not part of the system. It now returns a message the Admin can act on.
8.
joinMaster()— shared by both directionsRuns the existing handshake (
browse→auth→browse), stores the receivedobjects/statessections and restarts the controller. Safeguards:
iobroker.jsonis kept asiobroker.json.bak— without a screen and without SSH, ahost that cannot reach its master after the restart would otherwise be unrecoverable;
0.0.0.0/::) delivered by the master is replaced with its actual IP.9. Startup ordering fix
setMeta()createssystem.meta.uuidinside a database callback which it does not await, so on afresh installation the UUID does not exist yet when the discovery starts. The host would have
announced itself without one for a full refresh interval — and without it a master can neither tell
two hosts apart nor remember a rejected one.
waitForUuid()picks it up every 2 s for up to 30 s andcorrects the announcement.
10. Removed again
@iobroker/plugin-mdnsand itscommon.plugins.mdnsentry, both added in an earlier commit on thisbranch, are gone. That package is adapter-oriented: it has no discovery side at all, its dependency
@homebridge/ciaois advertise-only, it aborts without a configured port, and its namespace parsingdoes not match a controller. Discovery is inseparable from
MHServer/MHClient, so it lives in thecontroller now.
Security considerations
never handed out before it is claimed.
joinverifies against the actual state of this host whether it is free, it does not trust thesender.
declined by one master stays reachable for another — a host invisible to everybody could never be
brought back without reflashing it. A declined host keeps announcing for the same reason.
multihostService.pairing: false.Known and deliberately out of scope for this PR:
joinis not authenticated on the receivingside. There is no secret check in the
joinpath —passwordis only forwarded outwards to themaster. A single UDP packet to port 50005 is enough to make a host with a local database re-point its
databases and restart. The
unclaimedcheck narrows this to hosts without a second host in theirsystem, but does not close it. Since confirming on the new host is explicitly not wanted, a different
anchor is needed — a pairing window after installation, or a pairing code the master has to send.
Tests
If no tests added, please specify why it was not possible
Pairing runs over mDNS and UDP multicast between two hosts and ends in a controller restart, which
the existing Mocha integration setup does not cover.
Manually verified:
tsc --project packages/controller/tsconfig.check.jsonnpm run lintdb-objects-redis)npm run buildunclaimed: true, own announcement filtered out, flipping tounclaimed: falsepropagates, clean shutdownbonjour-serviceremovedDocumentation
New
Host discoverysection underFeature Overview, plusmultihostBrowse,multihostPairandmultihostConnectunderFeature Overview/js-controller Host Messages.Open points
discoveredHosts, the yes/nodialog, and enabling the multihost service on the master (write
iobroker.json, thenupdateMultihost; rebinding the databases to0.0.0.0still needs a controller restart).iobroker; after the join theirsystem.host.<name>objects collide. mDNS resolves the conflictfor the announced instance name only, not for the host object.
masterflag in the announcement comes from the in-memory config and stays stale afterupdateMultihostuntil the next restart. Cosmetic — onlyunclaimeddrives the pairing logic.