Skip to content

fix(start-os): reject a port range that overlaps a port the host binds - #3642

Open
helix-nine wants to merge 1 commit into
masterfrom
fix/reject-overlapping-port-claims
Open

fix(start-os): reject a port range that overlaps a port the host binds#3642
helix-nine wants to merge 1 commit into
masterfrom
fix/reject-overlapping-port-claims

Conversation

@helix-nine

Copy link
Copy Markdown
Contributor

Summary

Per @dr-bonez: a range overlapping a binding should be invalid.

A Host keeps single-port bindings and port ranges in two independent maps
(bindings keyed by internal port, binding_ranges keyed by internal start
port), and neither writer ever consulted the other. Only external ports were
protected, by the box-wide AvailablePorts allocator — so a range covering an
internal port the same host also bound singly, or covered by another range, was
accepted silently.

Why it isn't merely redundant

  • A range's forward hardcodes ForwardRequirements.secure = true
    (net_controller.rs:649-653 — deliberate, ranges carry no Security option),
    and update_addresses applies the gateway-secure filter to bindings but not
    to ranges. So an overlapping range serves that internal port on gateways the
    binding's own BindOptions.secure excluded.
  • BindId carries only (host, internal port) and clear_bindings tests one
    except set against both maps (net_controller.rs:1235-1256), so a range and
    a binding sharing a start port shield each other from being disabled.

The change

Model<Host>::add_binding and add_binding_range reject an overlapping claim
before allocating anything, naming both spans. These are the only key-creating
writers to either map, so this also covers the OS's own os_bindings.

The check counts only enabled entries. create_service runs
clear_bindings(∅) before every init pass (net_controller.rs:220), so at bind
time "enabled" means "claimed by this generation"; nothing ever prunes dormant
entries, so counting them would permanently block moving existing single ports
into a range — the ZMQ 28332/28333 migration the packaging book itself
documents — failing on every existing install while passing on a fresh one.

Computing the claimed span also bounds it server-side for the first time:
internalStartPort + numberOfPorts past 65535 was checked only in the SDK
(Host.ts:212) and otherwise failed late in forward-port.

Blast radius

coturn-startos is the only bindPortRange user across all 123 non-archived
*-startos repos in both orgs, and its 3478 binding is disjoint from its
42000-42499 range. The OS's admin host binds a single port and can never hold
a range. The 0.3.x compat shim binds single ports only. No migration needed —
because the check reads only enabled entries, existing db state cannot make a
bind fail.

Tests

8 new: internal_span bounds, enabled-only occupancy, self-skip on idempotent
re-bind (net/host/binding.rs); and the wiring — range blocked by an enabled
binding with nothing allocated, dormant binding not blocking the range that
supersedes it, single bind blocked inside an enabled range, coturn's shape
accepted, overflow rejected (net/host/mod.rs).

cargo test -p start-core --features=test net:: → 208 passed.
make start-core-format-check clean.

Reviewer calls

  • i18n. The new error uses plain eyre!, matching all 11 errors in
    net/host/ (zero use t!, including "domain {domain} is already in use",
    which is squarely user-facing). start-core/AGENTS.md:39 says error strings
    must be localized. I followed the module; say the word and I'll add the keys.
  • Scope is one host. Two hosts of the same package share a container IP, so
    a range in host A covering host B's port has the same problem. Rejecting that
    is a bigger policy call (two hosts binding the same single port looks like a
    legitimate existing pattern), so it's left alone.
  • Eviction vs error. A claim could instead evict a conflicting dormant
    sibling and free its external ports, which would make the port-preserving
    singles→range migration work (today try_alloc_range hard-errors when the
    dormant binding holds an external port inside the requested span). That's a
    behavior change on its own; not included here.

Separate from #3639 — no code overlap, only a neighbouring CHANGELOG.md
bullet.

A host keeps single-port bindings and port ranges in two independent maps,
and neither writer consulted the other, so a range covering an internal port
the same host also bound singly (or covered by another range) was accepted
silently. Only external ports were protected, by AvailablePorts.

Two claims on one container socket are not merely redundant. A range's
forward hardcodes secure: true and update_addresses applies no gateway-secure
filtering to ranges, so an overlapping range serves a port on gateways that
port's own BindOptions.secure excluded. It also leaves clear_bindings unable
to tell the two apart: BindId carries only (host, internal port), so a range
and a binding sharing a start port shield each other from being disabled.

add_binding and add_binding_range now reject an overlapping claim before
allocating anything, naming both spans. The check counts only enabled entries
— clear_bindings disables every binding before a setup pass runs, and nothing
prunes dormant ones, so counting them would permanently block moving existing
single ports into a range. Computing the claimed span also bounds it server
side for the first time; only the SDK checked it before.

coturn-startos is the sole bindPortRange user in either org and its 3478
binding is disjoint from its 42000-42499 range, so no published package is
affected and no migration is needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant