e2e: run an ip-verifier in every devnet and cover the RFC-27 proof outcomes - #4232
e2e: run an ip-verifier in every devnet and cover the RFC-27 proof outcomes#4232elitegreg wants to merge 3 commits into
Conversation
e4bcc1c to
d357846
Compare
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.
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.
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.
elitegreg
changed the title
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.
| docker push ${{ env.DZ_IMAGE_REPO }}/device-health-oracle:${{ env.DZ_IMAGE_TAG }} | ||
| docker push ${{ env.DZ_IMAGE_REPO }}/geoprobe:${{ env.DZ_IMAGE_TAG }} | ||
| docker push ${{ env.DZ_IMAGE_REPO }}/sentinel:${{ env.DZ_IMAGE_TAG }} | ||
| docker push ${{ env.DZ_IMAGE_REPO }}/ip-verifier:${{ env.DZ_IMAGE_TAG }} |
There was a problem hiding this comment.
The image is built and pushed here, but the shard job's Pull pre-built images step (~line 439) was not updated -- ip-verifier ends up the only e2e image missing from that list. Since IPVerifierSpec.Disabled defaults to false, every shard devnet now needs .../ip-verifier:${DZ_IMAGE_TAG}, and will instead fall back to testcontainers' implicit pull: one unretried pull per devnet, racing at -parallel=12, outside the pull_with_retry wrapper every other image gets. Worth adding pull_with_retry ${{ env.DZ_IMAGE_REPO }}/ip-verifier:${{ env.DZ_IMAGE_TAG }} next to the sentinel line.
| } | ||
|
|
||
| env := map[string]string{ | ||
| "DZ_IP_VERIFIER_ENV": "local", |
There was a problem hiding this comment.
DZ_IP_VERIFIER_ENV=local makes the service resolve its serviceability program ID from ENV_LOCAL_SERVICEABILITY_PUBKEY (7CTniUa8...), not from the program this devnet actually deployed -- the client container passes DZ_SERVICEABILITY_PROGRAM_ID explicitly for exactly this reason, and the service has no equivalent flag.
devnet.New generates a random serviceability program keypair whenever ManagerSpec.ServiceabilityProgramKeypairPath is unset, and e2e/sentinel_multicast_publisher_test.go:30 does that. In that devnet Ledger::ip_verifier_authority() calls get_account_data on a GlobalState PDA that does not exist, so check_at_startup only warns and the status stays AuthorityStatus::Unknown -- which health() treats as servable (only Mismatch returns 503).
Net effect: the container comes up, /health returns 200, StartIfNotRunning reports success, and the invariant this file is built around ("the service reads GlobalState at startup and exits if the authority is not its own key", and "waiting on /health proves both") silently does not hold there -- it just logs a warning every 60s forever. Proofs still validate only because SetIPVerifierAuthority wrote the same key into the real program, i.e. by coincidence rather than by the check. Either pin that devnet to the fixed program keypair or give the service an explicit program-id override.
| // never will. /health is 200 only once the cached ledger epoch is fresh and the ledger names this | ||
| // container's key, so it proves the same two things the create path's wait strategy does. | ||
| func (v *IPVerifier) waitForHealthy(ctx context.Context, containerID string) error { | ||
| port, err := v.dn.waitForContainerPortExposed(ctx, containerID, ipVerifierInternalPort, 10*time.Second) |
There was a problem hiding this comment.
The port wait runs before the container-state check, and Docker drops NetworkSettings.Ports entries for a container that is no longer running. So when the restarted verifier exits quickly -- the case this whole path was added for, where the authority rotated while the container was stopped and check_at_startup bails after a single local-ledger RPC round trip (tens of ms) -- this can lose the race, burn the full 10s, and return failed to wait for ip-verifier port: ..., never reaching the carefully worded ip-verifier exited with code %d ... the usual cause is GlobalState.ip_verifier_authority_pk no longer naming its key branch below. Diagnostics only, but it defeats the message where it matters most; inspecting for !State.Running before waiting on the port would keep it.
|
|
||
| # A proof, as a client would ask for it. | ||
| docker exec dz-local-client-<pubkey> \ | ||
| curl -sS -X POST "$DZ_IP_VERIFIER_URL/v1/proof" \ |
There was a problem hiding this comment.
docker exec runs curl directly, with no shell in the container, so $DZ_IP_VERIFIER_URL is expanded by the host shell where it is unset. Copy-pasted, this becomes curl -sS -X POST /v1/proof ... and fails with a protocol error rather than hitting the verifier. It needs a shell inside the container:
docker exec dz-local-client-<pubkey> bash -c \
'curl -sS -X POST "$DZ_IP_VERIFIER_URL/v1/proof" \
-H "content-type: application/json" \
-d "{\"payer\":\"<pubkey>\",\"user_type\":0}"'
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.