client: honor connect --client-ip for ip-bound access passes - #4341
juan-malbeclabs wants to merge 18 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 901aa53. Configure here.
`connect --client-ip` was accepted and silently discarded, so a host whose discovered address differs from the one its AccessPass names could only connect by editing the packaged doublezerod unit and restarting the daemon as root. The flag is honored again, but only when the payer holds an AccessPass at the exact (client_ip, user_payer) PDA and the host actually holds the address. A dynamic pass is deliberately not enough: it authorizes any globally routable address, so accepting a caller-chosen one against it is the squatting hole RFC-27 exists to close. The lookup goes through a new GetExactAccessPassCommand rather than GetAccessPassCommand, whose wildcard-first resolution would answer such a query with the dynamic pass. The pin also has to reach the reconciler, which matches onchain users on the client IP and uses it verbatim as the IBRL tunnel source: POST /enable takes an optional client_ip, re-verifies local assignment itself, reconciles even when already enabled, and persists the pin so a restart does not revert the host to its discovered address.
Two ways the pin could go missing without the operator being told. A body-less /enable — an ordinary `connect` with no flag, or `doublezero enable` — persisted an empty client_ip while leaving the running daemon on the pinned address. The divergence surfaced only at the next restart, as a tunnel torn down because discovery had taken over. Every state write now persists the pin in effect rather than the request being served, and the pin outlives a disable: it describes which address the host presents to DoubleZero, not anything about one session. A refused pin was also excused whenever the reconciler happened to be on already, because a plain enable is a no-op in that state. A pin is not: the daemon rejects an address the host does not hold, so the connect went on to wait for a tunnel that would never be built for a user it had just created. `connect` now requires the daemon to report the pinned address, and the bare path sends the pin with its up-front enable so a refusal stops the run before anything is created onchain.
58bdb13 to
6b6fb0a
Compare
… pass With require-ip-ownership-proof set, a creation carrying no proof was rejected for every access pass alike. That left a host whose pass names an address it cannot originate from — asymmetric routing, NAT, a verifier unreachable from that source — with no path to a connection at all: no proof for the address is obtainable, and the only workaround was reissuing the pass as a wildcard one, which trades the attestation away for the shape RFC-27 treats as weaker. A pass stored at its own client_ip PDA and not flagged allow_multiple_ip now waives the requirement. Such a pass is already an attestation: SetAccessPass is permissioned, so a privileged party asserted that this payer may use this address, which is the same reasoning by which RFC-27 scopes its threat to wildcard passes. The waiver covers a missing proof only; a supplied one is still validated in full. The trust boundary moves onto issuance with it. A tenant administrator may issue passes, so one could pin a third party's address to their own payer and create a user there with no proof; that is accepted deliberately, with a per-pass waiver flag as the lever if it needs tightening. The proof check moves below the access-pass checks, which is what makes the pass shape available and already matched against the address being created. It stays above the already_exists return, so a rerun is still covered. Four tests asserted rejection on what is now an IP-bound pass; they move to wildcard passes, the shape where the requirement still bites.
elitegreg
left a comment
elitegreg
left a comment
There was a problem hiding this comment.
Reviewed the full diff. Code quality is high — the comments explain why rather than
what, the concurrency work in manager.go is disciplined, and onchain test coverage is
strong. My concerns are mostly about scope and disclosure rather than craft.
1. The description says the onchain waiver is not in this PR, but it is (High)
The Follow-up section states Part 2 of #4333 "is not in this PR. It is an onchain change
in validate_ip_ownership_proof plus an amendment to RFC-27, and the issue's open
question ... is undecided."
That change is here: ip_proof.rs, create_core.rs, user_ip_proof_test.rs (+167)
and a 34-line RFC-27 amendment. Neither Summary of Changes nor Testing Verification
mentions it. The highest-risk change in the PR is described as absent in the text a
reviewer reads first. It also makes the Diff Breakdown table stale (19 files / +1588 vs.
an actual 24 / +1845).
Could you update the description, or split the onchain half out?
2. The waiver is reachable by any tenant administrator (High)
The logic itself is sound — I checked both ways it could break and neither does.
accesspass_is_ip_bound is only reachable with accesspass.client_ip == client_ip,
because the guard above rejects a mismatch, and the client_ip field cannot diverge from
the PDA seed (set.rs:116 derives from value.client_ip, :192/:253 store it).
Moving the validate_ip_ownership_proof call below the access-pass checks is what makes
that hold, and the tests cover allow_multiple_ip and wildcard-PDA passes still
requiring a proof, on both first creation and rerun.
The issue is the trust boundary. In set.rs:142-163 a caller is authorized if
is_privileged or is_tenant_admin (just tenant.administrators.contains(payer)),
and nothing constrains value.client_ip. So any tenant administrator can issue a pass at
(someone_elses_ip, their_own_payer) and then create a user at that address with no
proof — the squatting RFC-27 exists to close.
The doc comment and RFC amendment both acknowledge this and accept it, pointing at a
per-pass waiver flag as the future lever. Given the PR body says the waiver option is
still an open question, this reads like a design decision that got settled in-flight.
Worth an explicit call rather than a code comment — and the narrower ACCESS_PASS_ADMIN-only
waiver flag may be worth doing now rather than later.
3. A restored pin is never re-validated (Medium)
/enable gates a pin on IPv4 parse plus isLocallyAssigned. The restart path applies
neither: run.go passes the persisted pin into DiscoverClientIP, whose explicit branch
(runtime/clientip.go:84-96) only parses, with no local-assignment check and no fallback
to discovery.
So a host whose pinned address goes away (DHCP change, NIC swap, re-addressing) comes back
up pinned to an address the kernel does not hold, matches no onchain user, and builds no
tunnel. With no un-pin path, recovery is hand-editing the state file and restarting.
Re-running IsLocallyAssigned on the restored pin and falling back to discovery with a
warning would close it.
4. The waiver's justification contradicts the client gate (Medium)
The RFC amendment justifies the waiver because an attested address "is not always one the
host can originate from — under asymmetric routing, behind NAT". But the client gate this
PR adds requires the address be assigned to a local up+running interface. Behind NAT the
host does not hold its public address, so connect --client-ip rejects exactly the hosts
the onchain waiver exists to serve. The two halves address disjoint sets of hosts; the
amendment should probably say so.
5. The CLI and program gates test different predicates (Low)
The CLI checks "an exact-PDA pass exists"; the program checks
"exact PDA and !allow_multiple_ip()". A pass at PDA(X, payer) with
allow_multiple_ip set passes the CLI gate, so connect --client-ip X proceeds, but
onchain accesspass_is_ip_bound is false and a proof is still required. For the flag's
target case (no obtainable proof) that fails late at create_user with
IpOwnershipProofRequired instead of early with the clear "No AccessPass is pinned to X"
message. Fails safe, so UX rather than a hole, but the predicates should match.
6. The daemon does not check globally-routable (Low)
The description says "Both sides verify the address is globally routable and assigned to
an interface that is up and running." IsLocallyAssigned is possession-only and there is
no is_global equivalent in http.go. The CLI and the program both catch it, so this is
just a description inaccuracy.
Things I specifically liked
GetExactAccessPassCommand— the pre-existing wildcard-first resolution in
GetAccessPassCommandwould have admitted a payer holding both passes on the strength
of the dynamic one. ReturningOk(None)for anUNSPECIFIEDquery rather than
resolving it is a good call.- Adopting the address synchronously in
SetReconcilerStateand sending only
clientIPChangedover the channel, sotunnelSrcCachestays owned by the reconciler
goroutine while/v2/statuscan report the pin the moment/enablereturns. - Snapshotting
clientIP := n.ClientIP()once perreconcile()pass, so a concurrent
pin cannot match a user against one address and provision against another. - "Persist the pin in effect, not the request being served" — the subtlest invariant
here, and correct. A body-less/enableblanking a live pin would only surface at the
next reboot.
A pin restored from the state file went straight into DiscoverClientIP's explicit branch, which only parses it. An address that left while the daemon was down — DHCP change, NIC swap, re-addressing — brought the host back up pinned to something the kernel does not hold, matching no onchain user and building no tunnel, with no un-pin path short of editing the state file. The check /enable makes is now repeated at startup. A pin the host no longer holds falls back to discovery with a warning and is kept on disk, so it applies again once the address returns and a late interface does not cost the operator their configuration. An enumeration failure keeps the pin: it is a failure to check, not a failed check.
The --client-ip gate asked only whether a pass existed at the exact PDA, while create_user additionally requires !allow_multiple_ip() before it treats the pass as attesting the address. A pass at (X, payer) with the flag set cleared the CLI and then failed onchain with IpOwnershipProofRequired — late, and precisely for the host the flag exists to serve, one with no obtainable proof. The two predicates now match.
The amendment justified the waiver with hosts behind NAT, but behind plain NAT the service observes the same address discovery finds, so a proof is obtainable. The cases that actually need it are a host reaching the service from an address other than the attested one, or unable to reach it from that source at all. Also records that the waiver is reached through the daemon's configured or discovered address rather than through connect --client-ip, which independently requires local possession, and says which of the CLI's two checks the daemon repeats and why it does not repeat the other.
…iver The merge of main resolved CHANGELOG.md by keeping this branch's Unreleased section wholesale, which dropped four entries main had added: the feed lifecycle SDK commands and three RFC-28 serviceability bullets. Main has since cut v0.41.0 and moved them there, so this takes main's file and adds this branch's entries under Unreleased, where they belong now that v0.41.0 is out. The connect bullet no longer says the onchain waiver is separate work — it is in this branch — and the waiver gets a Serviceability entry of its own.
|
Thanks — all six are addressed. Point by point: 1. Description claimed the onchain waiver was absent (High). Correct, and the worst of the six. The description is rewritten: the waiver is in the opening paragraph, the Summary, Testing Verification and Key files, and the Follow-up section no longer claims it is separate work. The table is recomputed from the real diff (25 files, +1988/−219). I kept the two halves together — either alone leaves the flag unusable for the host that asked for it — but the size note now names the onchain half as the clean split if you would rather take it separately. The CHANGELOG carried the same stale sentence and is fixed too. 2. Waiver reachable by any tenant administrator (High). Accepted, and now an explicit call rather than a code comment: the description has a "Security decision this PR makes" section naming The narrower lever is recorded in Follow-up with the shape it would take: stamp a bit in the existing 3. Restored pin never re-validated (Medium). Fixed —
Worth noting this was inherited rather than introduced — the daemon's own 4. Justification contradicts the client gate (Medium). Right, and the NAT example was weaker than either of us said: behind plain NAT the service observes the same address discovery finds, so a proof is obtainable. The amendment is rewritten around the cases that actually hold — a host reaching the service from an address other than the attested one, or unable to reach it from that source at all — and now states the thing that resolves the contradiction: the waiver is reached through the address the daemon is configured with or discovers, never through 5. CLI and program test different predicates (Low). Fixed — the gate filters on 6. Daemon does not check globally-routable (Low). Fixed as the description inaccuracy it was, rather than by adding the check: globality is a property of the address rather than of the host, so it cannot change between the CLI's check and the daemon's, and One thing outside the review. The merge of main at |
The waiver read the stored `client_ip` field, which is not on its own an attestation. Between #1608 and #3859 `create_user` wrote the first address a dynamic pass connected from into that field and serialized it back, and #3859 removed the lock-in without migrating the accounts. Such a pass still sits at the UNSPECIFIED PDA while its field names an address the user picked, so the waiver accepted an address no authority ever vouched for — the squatting RFC-27 exists to close, reached through exactly the wildcard passes it names as its threat. The PDA seed is what an issuing authority chose, so the waiver is keyed on it. A legacy pass still authorizes the creation; it only stops waiving the proof. Also gives the sentinel test its teeth back: it provisioned a pinned pass, which the waiver now satisfies on its own, so it passed with the sentinel exemption deleted entirely. It uses an allow_multiple_ip pass instead.
The gate asked `get_accesspass_exact`, but user creation resolves the account with `GetAccessPassCommand`, which prefers the dynamic (0.0.0.0) PDA whenever one exists. A payer holding both passes cleared the gate on the exact one and then had the dynamic one sent, where the pass does not attest the address and the proof is required after all — the late IpOwnershipProofRequired the gate exists to turn into an early refusal. It now refuses up front and says why. Two diagnostics alongside it. GetExactAccessPassCommand folded every error into Ok(None), so an unreachable or wrong-cluster ledger read as "this payer holds no pass" and sent an operator to have a live one reissued; only AccountNotFound is absence now. And the daemon client's enable/disable discarded the response body, which for /enable is the only place the reason exists — four distinct rejections all surfaced as "400 Bad Request".
reconcile() reads the address once per pass under a comment saying a concurrent pin must not split it between matching one user and provisioning against another, but the snapshot was a local that stopped there: buildProvisionRequest re-read n.ClientIP(), and for plain IBRL that value becomes the GRE tunnel source verbatim. A pin landing between the two would source the tunnel from an address the ledger and the device both bind elsewhere, with the service marked up. The snapshot is now passed down like devicesByPK and allPrefixes already are.
…lost Two holes on either side of the pin's lifetime. /enable checked only possession. create_user is not the backstop for the rest: it rejects the onchain user, while what /enable writes is the daemon's pin, and a pin no user can exist at is the worst shape to hold — it matches nothing, tears down the services the host had, persists, and survives every restart with no un-pin path short of editing the state file. IsPublicIPv4 moves to the manager package so the discovered address and the pinned one share one definition, and the pin is now checked against it. A restored pin the host no longer holds fell back to discovery but stayed in memory as the pin in effect, so the next state write put it back and a later restart could apply it once the address happened to return — a silent address migration, and a state file disagreeing with the daemon in the meantime. It is dropped instead, and the warning says to re-pin.
It filtered the host's own addresses to the globally routable ones, and CI and developer machines are NAT'd, so the loop had no iterations: a test structurally incapable of failing where it runs. It now covers every enumerated address and asserts that a private one is refused for not being global rather than for not being assigned, which is the agreement it exists to guard.
Follows the program: the seed is what an issuing authority chose, and a pass predating the RFC may carry an address its own holder first connected from. Also records the two client-side refusals added alongside it.
|
A second self-review pass turned up six more, five of which I'd rank above some of the first round. All fixed and pushed; each has a regression test that fails without the fix. 1. The waiver was reachable by a legacy wildcard pass. The waiver is now keyed on the PDA seed, which is what an issuing authority chose. A legacy pass still authorizes the creation; it only stops waiving the proof. Worth a check against live ledger state to see whether the population is non-empty — I could not verify that from here. 2. The 3. The single-read invariant stopped halfway. 4. 5. A restored pin the host had lost was still held as the pin in effect. Falling back to discovery but keeping it in memory meant the next state write put it back, and a later restart could apply it once the address happened to return. That is a silent address migration, and it contradicts the invariant this PR claims — that the daemon and the state file cannot disagree. Dropped instead, with the warning saying to re-pin. @elitegreg, this is your original suggestion; I traded it away last round for boot-race tolerance and the trade was wrong. 6. Two tests were vacuous, one newly so. Two diagnostics went with them: Still open and unfixed, flagged rather than silently carried: version skew (a daemon predating this PR 200s the pin and discards it, so a CLI upgrade without a daemon restart exits 0 with an orphan onchain user); |
ben-malbeclabs
left a comment
ben-malbeclabs
left a comment
There was a problem hiding this comment.
Verdict: 2 important, 7 minor. The two important ones want addressing before merge.
Reviewed at 8f8d28c. CI is green on that commit including all six e2e shards, so the connect --client-ip invocations the description calls out as unverified do pass: the e2e CYOA subnet is 9.128.0.0/9, which is_global accepts, and the passes are issued at the exact PDA with no dynamic pass alongside.
Findings
| # | Sev | Location |
|---|---|---|
| 1 | Important | runtime/run.go:131 — a runtime /enable pin loses to -client-ip on the next restart |
| 2 | Important | CHANGELOG.md:12 — behaviour change is breaking, filed under Changes |
| 3 | Minor | CHANGELOG.md:18 — entry length and PR IDs |
| 4 | Minor | manager/manager.go:523 — tunnel-src cache clear has no reachable effect |
| 5 | Minor | runtime/run.go:51 — restored pin skips the globality check |
| 6 | Minor | connect.rs:232 — rejection names the wrong condition |
| 7 | Minor | rfc27-ip-verification.md:363 — states a program property the program does not enforce |
| 8 | Minor | create_core.rs:234 — allow_multiple_ip rationale is false against the assert above it |
| 9 | Minor | accesspass/get.rs:81 — absence decided by matching error text |
Rationale without a single anchor
The onchain waiver itself checks out. accesspass_is_ip_bound keys on accesspass_account.key == &accesspass_pda, which is seeded on the client_ip being created, so a legacy pass whose stored field its own holder chose cannot buy the waiver, and the test for that shape is present. Moving validate_ip_ownership_proof below the access-pass checks weakens no other path: every check that ran before still runs, only the error a bad caller sees changes.
The tenant-administrator escalation is accurately described. add_administrator is foundation-gated so administrators are appointed, but they hold no ACCESS_PASS_ADMIN and nothing constrains value.client_ip in set.rs. One property the description omits: the waiver is unconditional once RequireIpOwnershipProof is set, so no configuration enforces RFC-27 strictly any more. If the follow-up per-pass flag slips, that is what was lost.
Finding 1 is the one worth blocking on. The changelog states the previous workaround was -client-ip in the packaged unit, so the hosts carrying that flag are exactly the population this feature targets; on them the pin works until the next restart and then reverts silently. /v2/status reports the flag address while PinnedClientIP() reports the dormant pin, and restoreClientIPPin is skipped in that branch, so the dormant pin is never validated either.
| // host to its discovered address and tearing the tunnel down. | ||
| effectiveClientIP := clientIP | ||
| pinnedClientIP := state.ClientIP | ||
| if clientIP == "" && state.ClientIP != "" { |
There was a problem hiding this comment.
Important — the persisted pin is only consulted when -client-ip is unset, and the manager is never told the flag was set, so /enable accepts and persists a pin that the next restart overrides with the flag's address, leaving the host on an address no onchain user matches. Pass the flag's presence through to the manager and have ServeEnable refuse a differing pin, naming the flag.
| ### Changes | ||
|
|
||
| - CLI | ||
| - `doublezero connect --client-ip <ip>` is honored again instead of being accepted, warned about and discarded. A host whose discovered address differs from the one its AccessPass names could previously only connect by adding the flag to the packaged `doublezerod` unit and restarting the daemon as root. The flag is honored only when the payer holds an AccessPass at the exact `(client_ip, user_payer)` PDA: a dynamic pass authorizes any globally routable address, so accepting a caller-chosen one against it is the address squatting RFC-27 exists to prevent. The flag is also refused while the payer holds a dynamic pass, because user creation attaches that one and the gate would otherwise promise something the transaction does not deliver. The address must also be globally routable and assigned to an interface that is up on this host — possession, not ownership, which is what the local configuration needs and all the CLI can check; RFC-27 proofs remain the answer to who holds an address on the public internet. The value now parses as an IPv4 address rather than a string, so a malformed one is refused instead of silently ignored. A daemon that refuses the pin fails the connect: on the bare path the refusal stops the run before anything is created onchain, and elsewhere it is reported rather than written off as a redundant enable, which would otherwise leave the operator waiting on a tunnel that could never be built for a user already created at that address. A pass flagged `allow_multiple_ip` is refused for the same reason a dynamic one is, and for the same predicate `create_user` applies, so the refusal arrives up front rather than as a late `IpOwnershipProofRequired`. (#4333) |
There was a problem hiding this comment.
Important — --client-ip goes from accepted-and-ignored to a hard failure, so an existing invocation by a payer holding a dynamic pass now aborts the connect. This belongs under the empty ### Breaking heading above.
| - SDK | ||
| - `GetExactAccessPassCommand` in the Rust SDK reads the AccessPass at an exact `(client_ip, user_payer)` PDA. `GetAccessPassCommand` resolves the dynamic `0.0.0.0` pass first and falls back to the exact one, so a payer holding both is answered with the dynamic pass — right for a connect that takes the address it is given, wrong for authorizing a caller-chosen one, where the distinction is the whole check. | ||
| - Serviceability | ||
| - With `RequireIpOwnershipProof` set, a user creation carrying no RFC-27 proof is accepted when its AccessPass is bound to the address being claimed — stored at that address's PDA and not flagged `allow_multiple_ip`. The PDA seed is what an issuing authority chose, so the waiver is keyed on it rather than on the stored `client_ip` field: a pass issued between #1608 and #3859 can carry the first address its own holder connected from, which attests nothing. Such a pass is itself an attestation: `SetAccessPass` is permissioned, so a registrant cannot self-issue one, and a pass at `(client_ip, user_payer)` means an issuing authority asserted that this payer may use this address. The flag and the pass together left a host unable to connect at all when no proof was obtainable for its pinned address — the service signs the source it observes, so a host reaching it from some other address gets a proof it cannot use, and one that cannot reach it from that source gets none. The waiver covers *absence* only; a supplied proof is still validated in full, and a proof naming another address, payer, user type or epoch is rejected as before. Wildcard passes and passes flagged `allow_multiple_ip` still require one, including a legacy wildcard pass whose stored field names an address: neither has an authority's seed behind the address being claimed, which is the squatting RFC-27 exists to close. The trust boundary moves with it, onto issuance: `SetAccessPass` authorizes tenant administrators as well as `ACCESS_PASS_ADMIN` holders, so a tenant administrator can pin a third party's address to their own payer and create a user there with no proof. Accepted deliberately; a per-pass waiver flag set by a narrower authority is the lever if it needs tightening. RFC-27 is amended to match. (#4333) |
There was a problem hiding this comment.
Minor — these four entries run 200-400 words each and carry (#4333). The convention here is a TL;DR with no issue or PR IDs.
| // resolved for the old source. Cleared here because the cache belongs to | ||
| // this goroutine. | ||
| if cmd.clientIPChanged { | ||
| n.tunnelSrcCache = make(map[string]net.IP) |
There was a problem hiding this comment.
Minor — entries are ResolveTunnelSrc(tunnelDst), a function of the routing table alone, and are read only for IBRLWithAllocatedIP and Multicast, where clientIP is not the source; a pin change neither invalidates them nor is described by the comment above. Drop the clear, or state what it actually protects.
| // | ||
| // An enumeration failure keeps the pin: that is a failure to check, not a failed check. | ||
| func restoreClientIPPin(pinned string, isAssigned func(net.IP) (bool, error)) string { | ||
| ip := net.ParseIP(pinned) |
There was a problem hiding this comment.
Minor — a restored pin is re-checked for parseability, IPv4-ness and local assignment but not IsPublicIPv4, which /enable applies, so a non-global pin is adopted and matches no onchain user: the lockout this function exists to prevent.
| if pinned.is_none() { | ||
| writeln!( | ||
| out, | ||
| "❌ No AccessPass is pinned to {client_ip} for UserPayer: {}", |
There was a problem hiding this comment.
Minor — an exact pass flagged allow_multiple_ip reaches this line through the filter above, and the message says no pass is pinned to the address when one is.
| host unable to connect. It waives the *requirement* only; a supplied proof is still validated in | ||
| full. | ||
|
|
||
| This exemption is reached through the address the client daemon is configured with or discovers, |
There was a problem hiding this comment.
Minor — nothing onchain enforces the local-interface check, so doublezero user create --client-ip and any SDK caller reach the waiver without it; this states a program property that only the daemon CLI provides.
| // is keyed on (`accesspass.user_payer` above). On the ordinary path the two are the same | ||
| // account. | ||
| // | ||
| // A pass at the UNSPECIFIED PDA authorizes any address, and `allow_multiple_ip` says the same |
There was a problem hiding this comment.
Minor — the assert! above restricts accesspass_account to PDA(client_ip) or PDA(0.0.0.0), so a pass stored at a specific address can only ever be presented for that address and allow_multiple_ip cannot make it authorize another; keep the clause as an issuer opt-out, but the stated reason is wrong. Same claim in ip_proof.rs, tests/user_ip_proof_test.rs:236 and connect.rs:191.
| // behaviour this replaces, and the transport errors worth retrying have already | ||
| // been retried by the client. | ||
| Err(err) => { | ||
| if format!("{err:#}").contains("AccountNotFound") { |
There was a problem hiding this comment.
Minor — absence is decided by matching upstream error text, so a message change turns the ordinary no-pinned-pass case into a hard error and breaks connect --client-ip. DoubleZeroClient::get_multiple_accounts already returns Vec<Option<Account>> for typed absence.
|
Re-reviewed at I agree with Ben's review. Both of his important findings match what I see, and I traced I'm satisfied with the onchain waiver as scoped. The predicate keying on the PDA seed rather One nit nobody has mentioned: Leaving this as comments rather than a verdict — Ben's two important items are the ones |
Resolves: #4333
Two halves, both of #4333. The client half honors
connect --client-ip; the onchain half waives the RFC-27 proof requirement for a pass whose address an authority pinned. They ship together because either alone leaves the flag unusable for the host that asked for it: without the waiver a pinned host still fails when a verifier is configured, and without the client half there is no way to ask for the pinned address.Summary of Changes
connect --client-ipis honored again instead of being parsed, warned about and discarded. It is accepted only when the payer holds an AccessPass at the exact(client_ip, user_payer)PDA and that pass is not flaggedallow_multiple_ip: a pass that authorizes any address does not authorize a caller-chosen one, which is the squatting hole RFC-27 exists to close. This is the same predicatecreate_userapplies, so a refusal arrives up front rather than as a lateIpOwnershipProofRequired.RequireIpOwnershipProofset, a creation carrying no proof is now accepted when the AccessPass is bound to the address being claimed — stored at thatclient_ipand not flaggedallow_multiple_ip. Such a pass is itself an attestation:SetAccessPassis permissioned, so a registrant cannot self-issue one. The waiver covers absence only; a supplied proof is still validated in full.validate_ip_ownership_proofmoves below the access-pass checks increate_user_core, which is what establishes that the pass names this address and this owner before the waiver can rely on it. RFC-27 is amended to match.GetExactAccessPassCommandin the Rust SDK does the CLI's lookup.GetAccessPassCommandresolves the wildcard pass first and falls back to the exact one, so a payer holding both would have been wrongly admitted.POST /enablenow takes an optionalclient_ip, reconciles even when already enabled, and clears the tunnel-src cache on a change.create_userenforces it onchain for every path in. The CLI check is best-effort (an enumeration failure defers to the daemon); the daemon's is fatal, since nothing downstream is left to catch it.-client-ipflag, then a persisted pin, then discovery./enablemade says nothing about now — an address can leave with a DHCP lease, a NIC swap or a re-addressing while the daemon is down — and a host pinned to an address the kernel no longer holds would match no onchain user and build no tunnel, with no un-pin path short of editing the state file. A pin that no longer holds falls back to discovery with a warning and stays on disk, so it applies again once the address returns.Ipv4Addrrather thanString, so a malformed value is rejected instead of silently accepted.Security decision this PR makes
The waiver moves the trust boundary from "the verification service observed this address" to "every
ACCESS_PASS_ADMINand every tenant administrator names addresses correctly".set.rsauthorizesSetAccessPassonis_privileged || is_tenant_admin, whereis_tenant_adminis justtenant.administrators.contains(payer), and nothing constrainsvalue.client_ip. So a tenant administrator can issue a pass at(someone_else's_ip, their_own_payer)and then create a user at that address with no proof — a path RFC-27 previously closed for them.This is accepted deliberately, as the price of making pinned passes usable, and it is called out here rather than left in a code comment because it wants explicit sign-off. A per-pass waiver flag set by a narrower authority is the lever if it needs tightening; see Follow-up.
Diff Breakdown
Test-heavy: roughly two thirds of the diff is tests. Core logic is +768 new lines, above the ~500-line guideline; the onchain half (
ip_proof.rs,create_core.rs, the RFC amendment anduser_ip_proof_test.rs, ~+270 including its tests) is the clean split if a reviewer would rather take it separately. The Rust files appear in both Core logic and Tests, since their tests are inline; the totals row counts each file once.Key files (click to expand)
smartcontract/programs/doublezero-serviceability/src/ip_proof.rs—accesspass_is_ip_boundas a second exemption alongsidepayer_is_sentinel, and why it existssmartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs— the proof check moved below the access-pass checks, and the predicate derived from the passcrates/doublezero-daemon-cli/src/connect.rs—resolve_connect_client_ip: the local-possession and exact-pass gates, the flag flowing through toenable, and the refused-pin handling inuser_activatedclient/doublezerod/internal/manager/manager.go—clientIPand the pin behind anRWMutex,reconcilerCmdon the enable channel, pin delivered even when already enabled, tunnel-src cache dropped on changecrates/doublezero-daemon-cli/src/clientip.rs— new: validates a caller-supplied address againstis_globaland this host's up+running interfaces, with a named rejection per caseclient/doublezerod/internal/manager/http.go—/enableaccepts an optionalclient_ip, validated and re-checked before anything is written; both handlers persist the pin in effectclient/doublezerod/internal/runtime/run.go— startup precedence, andrestoreClientIPPinre-validating a pin loaded from diskclient/doublezerod/internal/manager/state.go—State.ClientIPpersisted;LoadOrMigrateState/WriteStatecarry aStateinstead of a boolsmartcontract/sdk/rs/src/commands/accesspass/get.rs—GetExactAccessPassCommand, which never falls back to the dynamic passrfcs/rfc27-ip-verification.md— the amendment: the exemption, what it does and does not cover, and the trust boundary it movesTesting Verification
create_subscribe_user, since both sharecreate_user_core. It stays rejected for a pass at the wildcard PDA and for a pinned pass flaggedallow_multiple_ip, on first creation and on the idempotent rerun path alike.get_accesspasswould have admitted), refused for an exact pass flaggedallow_multiple_ip(the case that would otherwise fail late onchain), refused for an address this host does not hold and for a non-global one, and ignored entirely when absent — the ledger is never consulted about a pin in that case.connect: three combinations that must all create a user — dynamic pass with a discovered and certified address, pinned pass with a discovered address and no proof, and pinned pass with--client-ipand no proof. The last asserts the pinned address is what reaches provisioning, and that every downstream pass lookup is keyed on it rather than the discovered one.connect --client-ipagainst a daemon that refuses the pin stops with nocreate_usercall at all./enablepins and reports the address on/v2/statusimmediately; a body-less request leaves both the address in use and the persisted pin alone; a malformed or non-local address changes nothing (no state write, no pin); an interface-enumeration failure fails the request; a pin on an already-enabled daemon is still delivered; the daemon and the state file agree at every step of pin → disconnect → reconnect-without-the-flag;State.ClientIPround-trips through the state file.--client-iptoconnect(e2e/device_maxusers_rollover_test.go,e2e/multi_client_ibrl_allocated_ip_test.go), where it was previously ignored and now takes effect. They pin the AccessPass to the exact CYOA address and that address is assigned to the client container, so both gates should pass, but this wants a CI run to confirm.Follow-up
SetAccessPassauthorizes, tenant administrators included (see Security decision above). Recording at issuance whether the issuer heldACCESS_PASS_ADMIN— a bit in the existingflagsbyte, so no layout change — and waiving only for those passes is the contained way to tighten it. Deliberately not in this PR: it would make tenant-issued pinned passes require a proof, which is a product call about how tenants provision rather than a correctness fix.client_ipfrom the state file and restarting; an--client-ip=-style clear would need the daemon to re-run discovery on demand, which is worth doing separately if operators ask for it.--client-ipis refused while the payer holds a dynamic pass. User creation attaches the dynamic pass, so the flag cannot be honored against the pinned one without changing which pass is charged for the seat — a larger change than this flag should make. The gate says so up front instead of promising something the transaction does not deliver.connectstill treats that as fatal, before the onchain waiver is ever reached. A host that needs the waiver gets there through the address the daemon is configured with or discovers.