Skip to content

Index netmap peers by NodeID and preserve explicit endpoint clears #126

Description

@forkwright

Finding

dictyon has two different identities for the same live peer state. Full PeersChanged records update or append peers by public key, while PeersChangedPatch and numeric removals address peers by server-assigned NodeID.

That split breaks key rotation: a full changed record carrying the same NodeID and a new key is treated as a new peer, leaving the old-key record in the netmap. The patch path also collapses an explicit empty endpoint list into “unchanged,” so the server cannot revoke every previously advertised direct endpoint.

Verified against main 9aa9a56223afdd47d7723104cd16a5c07d5fc722.

Evidence

  • crates/dictyon/src/control/netmap.rs:115-149 handles PeersChanged by searching self.peers for p.key == changed_peer.key; a changed key with the same node ID therefore falls into the append path.
  • netmap.rs:151-160 applies removals and PeersChangedPatch separately; apply_peer_change at :173-176 finds the target by peer.id == change.node_id.
  • crates/mitos/src/types/mod.rs:345-429 models Node.id as the server-assigned numeric identity and Node.key as a rotatable public key. PeerChange at :431-479 is explicitly keyed by NodeID and can carry a new Key.
  • netmap.rs:186-195 applies endpoints only after change.endpoints.filter(|endpoints| !endpoints.is_empty()). None and Some(vec![]) therefore both mean “do nothing,” even though the wire shape can distinguish omitted from explicitly empty.
  • The netmap property test at crates/dictyon/src/control/tests/netmap_tests.rs:522-629 asserts only that peer keys are unique. It does not assert uniqueness by node ID or generate a same-ID/key-rotation update.
  • The reference implementation at tailscale/tailscale@33af330c8ed8a1cb02530eaf70d9179f9656db2c, control/controlclient/map.go, stores peers in a map keyed by NodeID; both full changes and patches update that same entry. It applies Endpoints whenever the patch field is non-nil, including an empty list that clears prior endpoints.
  • Build authoritative ACL policy state before advertising or activating the data plane #67 owns capability-map policy and revocation; this finding excludes CapMap and concerns the lower-level peer identity and endpoint-state contract.

Why this matters

NodeID, node key, endpoints, and patch/removal routing feed the eventual WireGuard peer configuration. A key rotation delivered as a full peer change can leave two records for one peer: one stale key and one current key. Different consumers may then select different records, retain obsolete routes, or configure duplicate peers.

Ignoring an explicit empty endpoint list preserves stale direct-connect addresses after the control plane has revoked them. The client can continue dialing an address the server deliberately removed, rather than falling back to current discovery or DERP state.

This is already-diverged ownership, not cosmetic DRY: one update path treats the key as identity, while the other treats the node ID as identity.

Desired correction

Give Netmap one authoritative peer index keyed by NodeID. Full peer lists, PeersChanged, patches, and removals must all mutate that owner. Public-key changes should update the existing peer record rather than create another peer.

Preserve tri-state patch semantics for collection fields: omitted means unchanged, present-empty means clear, and present-non-empty means validate and replace. Apply the same rule to other nullable patch collections as they become modeled, while leaving #67’s capability-policy semantics with that issue.

Done when:

  • one live peer record exists per non-zero NodeID;
  • a full PeersChanged entry with an existing ID and rotated key replaces that peer without increasing peer count;
  • key- and ID-based compatibility inputs resolve through the same canonical record;
  • Endpoints: [] clears previously stored endpoints, while an omitted field preserves them;
  • full-list ingestion rejects or deterministically resolves duplicate IDs; and
  • property/integration tests assert unique node IDs, key rotation, endpoint revocation, and subsequent removal of the rotated peer.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugUnintended behavior or production defectseverity:mediumNotable impact; workaround exists

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions