e2e: run an ip-verifier in every devnet and cover the RFC-27 proof outcomes - #4232
Conversation
e4bcc1c to
d357846
Compare
3740095 to
6122f69
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the local devnet (dev/dzctl and e2e devnet harness) to run an RFC-27 IP ownership verifier container and wire local client containers to use it, so the local connect flow matches production behavior.
Changes:
- Add an
ip-verifiercontainer to the devnet, generate a devnet-only keypair, and write its pubkey intoGlobalState.ip_verifier_authority_pkbefore container startup. - Attach the verifier to both the default network and the CYOA network, and set
DZ_IP_VERIFIER_URLin client containers to the verifier CYOA address. - Add build and CI wiring for the new image plus documentation and unit tests for verifier spec validation.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/internal/devnet/smartcontract_init.go | Add helpers to set the verifier authority and toggle the RFC-27 enforcement feature flag onchain. |
| e2e/internal/devnet/ip_verifier.go | Add the IPVerifier container manager and spec validation for the local devnet verifier. |
| e2e/internal/devnet/ip_verifier_test.go | Add unit tests for IPVerifierSpec.Validate. |
| e2e/internal/devnet/devnet.go | Add devnet spec and startup orchestration for the verifier plus keypair generation. |
| e2e/internal/devnet/cmd/devnet.go | Enable the verifier by default for the local devnet configuration. |
| e2e/internal/devnet/client.go | Wire DZ_IP_VERIFIER_URL into client containers to reach the verifier on CYOA. |
| e2e/internal/devnet/builder.go | Build the ip-verifier Docker image as part of the devnet build step. |
| e2e/docs/IP_VERIFIER_LOCAL_DEVNET.md | Document local devnet topology, feature flag, and debugging commands. |
| e2e/docker/ip-verifier/Dockerfile | Add a runtime image for doublezero-ip-verifier. |
| e2e/docker/base.dockerfile | Include the doublezero-ip-verifier binary in the base build stage output. |
| e2e/.env.local | Add DZ_IP_VERIFIER_IMAGE for local image tagging. |
| CLAUDE.md | Document the new dz-local-ip-verifier container in local devnet docs. |
| .github/workflows/e2e.yml | Push the ip-verifier image in the e2e workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
RFC-27 has connect obtain an IP ownership proof, so the local devnet needs a verifier to reach or the flow diverges from production. dzctl start now brings up a dz-local-ip-verifier container with a keypair generated per deploy, writes its pubkey to GlobalState.ip_verifier_authority_pk before the container starts (the service exits if the ledger names another key), and points every client at it. The container sits on the CYOA network, not only the default network, and clients are pointed at its CYOA address. The service signs the source address it observes and connect refuses a proof for any other address than the one it is provisioning, which for a local client is its CYOA address; reached over the default network the two would never agree. Same class of problem as the proxy handling in production. Enforcement stays off: the require-ip-ownership-proof feature flag is clear by default locally, so a proof is attached but not demanded.
The verifier was built and pushed on every e2e run but never started: only dev/dzctl set IPVerifierSpec.Enabled, so the Go e2e suite deployed no verifier and no client was pointed at one. RFC-27's connect path had no e2e coverage at all. Flip the field to Disabled so the zero value runs it. Every existing e2e test now obtains a real proof during connect and hands it to the program, which validates it. Enforcement stays off: require-ip-ownership-proof is clear, so a create without a proof is still accepted. Add ip_ownership_proof_test.go for the three outcomes the shared path can have and that no other test distinguishes: a proof the program accepts, no proof at all, and a proof signed by a key the program does not trust. The last needs a verifier that keeps signing after the trust root moves, so IPVerifierSpec.AuthorityRefreshSecs pins how often the service re-reads GlobalState.ip_verifier_authority_pk. Set it past the test and rotate the authority and the service never notices, leaving a proof signed by a key GlobalState no longer names. That is refused by the SDK pre-flight, before the transaction is paid for, so it does not reach the program's own precompile check. ClientSpec.NoIPVerifier leaves DZ_IP_VERIFIER_URL unset for one client, which is what a client with no configured verifier looks like.
Three fixes from review. The host ID bounds now match the ones ClientSpec and DeviceSpec enforce, which reject the network and broadcast addresses. The old check used 2^hostBits rather than 2^hostBits-1, so it accepted the broadcast host ID while its error message claimed a range that excluded it. Not reachable in practice — the CYOA Docker network is a /23 while CIDRPrefix is 24, so the bound was already conservative — but the divergence and the untrue message were both worth removing. A verifier this harness restarts now gets the same /health gate a fresh one gets from testcontainers. StartIfNotRunning called ContainerStart and returned, so a container that exits on startup left the devnet reporting success with clients pointed at a dead service. The authority can rotate while the container is stopped, and the service refuses to start when GlobalState no longer names its key, so this is the component where that gap actually bites. Ledger is the precedent: it reuses its create-path readiness check on the restart path. The other components share the gap and are left alone. A cloned-state devnet no longer runs a verifier unless it names a keypair. Its GlobalState came from a remote cluster and the local manager cannot write to it, so a generated key would never be the one the program trusts and the container would exit at startup. Latent until now: both SkipProgramDeploy callers hand-start components and never reach Devnet.Start.
Pull the ip-verifier image in the shard job alongside every other e2e image, rather than leaving it to testcontainers' unretried implicit pull. Give the service an explicit --serviceability-program-id (DZ_IP_VERIFIER_SERVICEABILITY_PROGRAM_ID) and pass the devnet's deployed program, so the startup authority check reads the GlobalState the devnet actually has. Under DZ_IP_VERIFIER_ENV=local it resolved the environment constant instead, and in a devnet with an unpinned program keypair that PDA does not exist: the check only warned and /health still answered 200. Check container state before the port wait in waitForHealthy. Docker drops the port bindings of a stopped container, so a verifier that exits on startup lost the exit-code diagnostic to a port-wait timeout. Run the doc's proof curl through a shell in the container, so DZ_IP_VERIFIER_URL is expanded there and not by the host shell.
8792223 to
54442d0
Compare
juan-malbeclabs
left a comment
There was a problem hiding this comment.
Approving. The design holds up and the parts that are easy to get wrong are right.
- Start ordering is load-bearing and correct: ledger → manager → CYOA network →
SetIPVerifierAuthority→ container. The service?s oncheck_at_startup(main.rs:59), so writing the authority first genuinely matters, andDevnet.Startsequences it that way. - The CYOA-attachment argument is real, not decorative:
connecthard-fails on an observed/provisioned address mismatch (connect.rs:430), so reaching the verifier over the default network would break every connect. /healthis readiness, not liveness (server.rs:228) — 503 on a stale epoch or an authority mismatch — so gating on it proves whatip_verifier.goclaims it does. The new--serviceability-program-idflag is what closes the hole whereAuthorityStatus::Unknown(noGlobalStateunder the environment's constant program ID) would answer 200 and make the gate vacuous. Good catch.- Verified the wiring end to end:
--ip-verifier-authority(globalconfig/authority/set.rs:30),feature-flags set --enable/--disable,solana.GenerateKeypairJSON,poll.Until,waitForContainerPortExposedall exist;e2e.ymlgot all three image lines (env / push / pull); teardown is label-based so the container is cleaned up with no change needed.go vet -tags e2e ./e2e/...is clean, and the newIPVerifierSpecValidatesubtests plus the twosettings.rstests pass locally.
Two non-blocking inline notes below, plus one thing worth fixing in the description:
The PR body is stale. It says "Entirely test-infrastructure … No production code is touched" with a 14-file / +754 table, but the branch is now 15 files, +920 / −7 and includes crates/doublezero-ip-verifier/src/settings.rs (+52 / −7) — a new production CLI flag on the verifier binary. The change is additive, defaults to the previous behavior, and has two unit tests, so it is fine on the merits; it just should not be invisible to the next reader.
What I would want as follow-up (agreeing with your own note): the enforcement path with require-ip-ownership-proof on. It is the path the whole RFC exists for and it is currently only manually verified. Worth watching the first full e2e run for memory pressure too, given the start-time gate is fatal — a flake in this one container turns every e2e test red, not just the new file.
Give the health probe its own client with a 5s timeout. poll.Until runs its condition synchronously and the request carries the caller's context, so a probe that hung rather than being refused would block past the 60s startup budget instead of failing within it. Refuse a nil-URL verifier when starting a client instead of falling through to the no-proof path. That state means Prepare never ran, which Devnet.Start makes unreachable; tolerating it would let every connecting test quietly stop covering the proof while still passing.
Closes #4205. Part of RFC-27 ([`rfcs/rfc27-ip-verification.md`](https://github.com/malbeclabs/doublezero/blob/main/rfcs/rfc27-ip-verification.md), tracker #4194). ## Summary of Changes - **`require-ip-ownership-proof` had no e2e coverage at all.** It was never set in any test, and `devnet.SetIPOwnershipProofFeatureFlag` had zero callers. One test now runs with the flag set, as four subtests over a single devnet. - **Wildcard access passes get their first e2e coverage of any kind.** All 72 `access-pass set` call sites under `e2e/` name a `--client-ip`, so the case RFC-27 actually exists for was untested. A pass at the `0.0.0.0` PDA — the shape the shred-oracle issues — authorizes its payer for any routable address, which makes the proof the only thing binding `client_ip`. Covered both ways: with a proof the user binds the observed address and reaches BGP, without one the create is rejected onchain. - **The sentinel exemption is covered**, because enforcement must not break the oracle path. The manager is the sentinel authority in a local devnet, so a manager-side `user create` still succeeds under enforcement while a client-paid one does not — the difference being the transaction payer, which is what `is_sentinel` compares. - **An address mismatch is asserted client-side**, where the guard actually lives: `connect` binds its proof request to the address it provisions and refuses a proof for any other. - One harness addition: `ClientSpec.DaemonClientIP` overrides the daemon's `-client-ip`, validated in `ClientSpec.Validate` alongside the other address-shaped fields. Needed because `doublezero connect --client-ip` is deprecated and explicitly ignored — the address comes from the daemon. ## Scope: what is *not* here, and why Two facts bound what e2e can usefully assert, and they cut several of the issue's bullets: **Most bad-proof cases never reach the chain.** The Rust SDK pre-flights version, payer, `client_ip`, `user_type` and the signature before it builds a transaction, and the runtime's Ed25519 precompile rejects bad signatures before the program runs. Of the program's proof errors only `IpOwnershipProofRequired` (105) and `IpProofEpochOutOfWindow` (110) are reachable end to end. Testing the others here would assert CLI strings while duplicating program tests. **The program already covers them.** `smartcontract/programs/doublezero-serviceability/tests/user_ip_proof_test.rs` has 36 tests spanning every rejection condition, epoch window included, using `warp_to_epoch` — a `solana-program-test` facility with no equivalent against a real validator. So this PR asserts 105 onchain and puts the rest of its weight on integration: a real verifier, a real ledger, a real tunnel, real BGP. **Not covered — a valid proof on a specific-IP pass.** The flag is read at exactly one site, inside the `proof == None` arm (`ip_proof.rs`). A supplied valid proof takes a byte-identical program path with the flag set or clear, so such a subtest would duplicate `TestE2E_IPOwnershipProof_ValidProof` for the price of a client and a 90s `WaitForTunnelUp`. The "enforcement disturbs nothing downstream" assertion lives on `wildcard_pass_with_a_proof` instead, which is not a duplicate of anything. **Deferred — stale epoch (issue bullet 5).** Feasible, but it needs two harness additions and is better as its own change: `LedgerSpec.SlotsPerEpoch` plumbed through the ledger entrypoint as `--slots-per-epoch 32` (silently ignored if the ledger volume already exists), plus `IPVerifierSpec` knobs for `epoch_refresh_secs`/`max_epoch_age_secs` so the service keeps signing a stale epoch while the ledger moves on. Today the devnet validator uses the default 432,000-slot epoch, so the epoch never advances — every existing proof test lives inside epoch 0, where the `{epoch-1, epoch}` window degenerates to `{0}`. Error 110 is the only other reachable onchain rejection, so this is the one remaining case with real e2e value; happy to file a follow-up issue. **Deferred — rotated verifier key (bullet 7).** Already covered by `TestE2E_IPOwnershipProof_UntrustedSigner` from #4232, and it can only ever be a client-side assertion: the SDK's `verify` catches it before the transaction is built. ## Diff Breakdown | Category | Files | Lines (+/-) | Net | |-------------------|-------|-----------------|--------| | Tests | 1 | +221 / -0 | +221 | | Docs | 2 | +27 / -2 | +25 | | Core logic | 1 | +19 / -1 | +18 | | **Total** | 4 | +267 / -3 | +264 | Almost entirely tests; the harness change is a spec field, a validation check and a two-line branch. <details> <summary>Key files (click to expand)</summary> - `e2e/ip_ownership_proof_enforcement_test.go` — the enforcement test and its four subtests - `e2e/docs/IP_VERIFIER_LOCAL_DEVNET.md` — a "testing enforcement" section, including the two traps below - `e2e/internal/devnet/client.go` — `ClientSpec.DaemonClientIP` and its validation </details> ## Testing Verification `TestE2E_IPOwnershipProof_Enforced` passes in CI (e2e shard 3, 142.52s) and locally. - **`wildcard_pass_with_a_proof`** — the one that matters. The pass names no address, so the proof is the only thing that could bind `client_ip`: `IP ownership verified for <addr>`, `✅ User Provisioned`, the user appears in `user list` at the observed address, and `WaitForTunnelUp` reaches `BGP Session Up`. Enforcement disturbs nothing downstream of the proof. - **`wildcard_pass_without_a_proof`** — the same wildcard pass does not rescue a client with no verifier to reach. The program rejects it: ``` Program log: Instruction: CreateUser(... client_ip: 9.210.238.101, ... ip_proof: false) Program log: IP ownership proof required but none supplied Program 7CTniUa88iJKUHTrCkB4TjAoG6TD7AMivhQeuqN2LPtX failed: custom program error: 0x69 ``` - **`sentinel_authority_is_exempt`** — manager-side `user create` with no proof succeeds under enforcement. - **`proof_for_a_different_address_is_refused`** — daemon set to an address the container does not own, so the verifier observes the real CYOA source: ``` ❌ The verification service observed this host at 9.130.200.100, but the daemon is provisioning 9.0.0.7. ``` Also re-ran `TestE2E_IPOwnershipProof_ValidProof` locally, since `DaemonClientIP` touches shared client startup — passes (173s). ## Notes for the reviewer - One devnet for the whole file. The two things a client cannot change after it starts — whether it has a verifier to reach, and what address its daemon provisions — are per-`ClientSpec`, so each costs one small container rather than a second ledger + manager + controller + cEOS device. - Every rejection assertion keys on the *specific* error string rather than just a non-zero exit. A test that passes because connect broke for an unrelated reason was the main risk in this set. - Two traps I hit and wrote into the docs, in case they save someone else the time: the manager is the sentinel authority locally, so "flag on rejects everything" is false for manager-driven creates; and `doublezero connect --client-ip` is deprecated and ignored, so the daemon flag is the only way to move the provisioned address.
Closes #4204. Part of RFC-27 (
rfcs/rfc27-ip-verification.md, tracker #4194). Depends on #4198.Summary of Changes
dzctland the Go e2e suite alike, soconnectobtains a real RFC-27 proof and attaches it to user creation instead of diverging from production.IPVerifierSpec.Disabledis the opt-out; the zero value runs one.connectin every test obtains a proof and the program validates it.DZ_IP_VERIFIER_URLpointing at its CYOA address. This is the substance of the change, not a detail: the service signs the source address it observes, andconnecthard-fails on a proof for any address other than the one it is provisioning. A local client provisions its CYOA address, so the request has to arrive over the CYOA network for the two to agree — reached over the default network, the observed address would be the client's default-network address and every connect would fail on the mismatch. Same class of problem as the proxy handling in production.GlobalState.ip_verifier_authority_pkbefore the container starts. The service reads the authority at startup and exits if it does not name its own key, so the ordering is load-bearing.require-ip-ownership-proofis clear, so a proof is attached but not demanded.devnet.SetIPOwnershipProofFeatureFlag(ctx, bool)turns it on.e2e/ip_ownership_proof_test.gocovers the three outcomes the shared connect path can have and that no other test distinguishes: a proof the program accepts, no proof at all, and a proof signed by a key the program does not trust.ClientSpec.NoIPVerifierleavesDZ_IP_VERIFIER_URLunset for one client, andIPVerifierSpec.AuthorityRefreshSecspins how often the service re-reads the onchain authority — set it past the test, rotate the authority, and the service keeps signing with a keyGlobalStateno longer names.e2e/docs/IP_VERIFIER_LOCAL_DEVNET.md, covering the topology, the flag, and how to poke at the service.Diff Breakdown
Entirely test-infrastructure: the "core logic" here is the devnet harness that brings the verifier up and wires clients to it. No production code is touched.
Key files (click to expand)
e2e/internal/devnet/ip_verifier.go— the container: spec, CYOA addressing, keypair,/healthgate, and theAuthorityRefreshSecsoverridee2e/ip_ownership_proof_test.go— the three proof outcomes, each on its own single-device devnete2e/internal/devnet/devnet.go— start ordering: CYOA network, then authority onchain, then the containere2e/internal/devnet/smartcontract_init.go—SetIPVerifierAuthorityandSetIPOwnershipProofFeatureFlage2e/internal/devnet/client.go—DZ_IP_VERIFIER_URLwiring and theNoIPVerifieropt-oute2e/internal/devnet/ip_verifier_test.go—IPVerifierSpec.Validate: host-ID defaulting and range, relative keypair pathTesting Verification
New e2e tests, all passing against real devnets:
TestE2E_IPOwnershipProof_ValidProof— connect reportsIP ownership verified for 9.236.198.100(the client's CYOA address, not its default-network address), user provisioned, tunnel up. Asserts the run did not silently fall back to no proof.TestE2E_IPOwnershipProof_NoProof— a client withNoIPVerifierreports no proof and is provisioned anyway, which is the path an environment takes before its verifier exists.TestE2E_IPOwnershipProof_UntrustedSigner— withAuthorityRefreshSecs: 3600the service never observes the rotation, so it still issues a proof (asserted, so the test cannot pass vacuously by the service simply refusing), and the create is then refused:IP ownership proof does not verify against the onchain verifier dKD6f2cN.... No user is left onchain.Where the untrusted-signer refusal lands: the SDK checks the proof against the onchain verifier key before building the transaction (
smartcontract/sdk/rs/src/commands/user/mod.rs:86), so it fails client-side. That is the intended "refused before the transaction is paid for" pre-flight, but it does mean this test does not reach the program's own Ed25519 precompile check — reaching that needs a client that skips the pre-flight, whichconnectgives no way to do. Noted in the test's doc comment so it is not later mistaken for onchain coverage.Regression check for turning the verifier on everywhere:
TestE2E_IBRLandTestE2E_Connect_AllModesFromAccessPassboth pass. The second is the one that mattered — a bareconnectruns two legs and brings up two tunnels, so it is where a proof/tunnel address disagreement would have surfaced.Manual devnet run (
dzctl build→start→add-device→add-client):/healthreturns{"epoch":0,"status":"ok","verifier_key":"matches"}; withrequire-ip-ownership-proofon and the verifier stopped, connect fails onchain withProgram log: IP ownership proof required but none supplied/custom program error: 0x69, and restarting the verifier restores it. That negative control is what shows the proof is load-bearing rather than the flag being inert.Notes for the reviewer
IPVerifier: devnet.IPVerifierSpec{Disabled: true}.rate_limitedrefusals unrelated to what is being tested. If you would rather tests confront the limiter, it is a one-line change.