ip-proof: add a crate defining the IpOwnershipProof layout and signed message - #4206
ip-proof: add a crate defining the IpOwnershipProof layout and signed message#4206elitegreg wants to merge 3 commits into
Conversation
…essage RFC-27 foundation. The proof format is consumed by the serviceability program (BPF), the CLI, and the verification service, which must agree byte for byte or every proof fails validation. Defines it once. Closes #4195
juan-malbeclabs
left a comment
There was a problem hiding this comment.
Reviewed at high effort. Verified locally before commenting: cargo build -p doublezero-ip-proof --features signer succeeds standalone (solana-signature's verify feature arrives transitively via solana-keypair), cargo test -p doublezero-ip-proof --features signer,test-vectors passes all 11 tests, and make rust-test uses --all-features so the feature-gated tests do run in CI. The byte layout and offsets check out (11+1+32+4+8+32 = 88), both committed vectors decode and verify, borsh's Ipv4Addr impl is core::net-based so it's BPF-safe, and Signature::verify uses verify_strict, matching the Ed25519 precompile.
Two findings worth resolving before merge — the user_pubkey derivation and the un-updated RFC-27 proof spec. The other four are smaller. Each comment carries a concrete fix option.
| /// The User account this proof authorizes. Binding it stops a proof obtained for a routine | ||
| /// connect from being replayed into another operation on a different account in the same | ||
| /// epoch. | ||
| pub user_pubkey: Pubkey, |
There was a problem hiding this comment.
user_pubkey is the User PDA, and the User PDA is f(program_id, client_ip, user_type) (smartcontract/programs/doublezero-serviceability/src/pda.rs:58 — seeds are [SEED_PREFIX, SEED_USER, ip.octets(), user_type]). But the service is the party that discovers client_ip: RFC-27's request body is { "payer": ... } only (rfcs/rfc27-ip-verification.md:123), so the CLI has to send a PDA it derived from its own autodetected IP.
On a NATed or multi-homed host where the autodetected IP differs from the IP the service observes, the service signs client_ip = observed next to a user_pubkey derived from the other IP. The program re-derives the User PDA from proof.client_ip, they don't match, and the proof fails 100% of the time — indistinguishable from a bad signature. Neither sign() nor signed_message_for() checks the two fields agree, so the crate happily mints permanently-invalid proofs.
Two ways out:
- Add
user_typeto the RFC-27 request body and have the service derive the PDA itself from the IP it observed. The mismatch becomes structurally impossible. - Drop
user_pubkey.client_ipis already in the signed message, so the only new binding this field adds isuser_type(plus the program id) — narrower than the replay protection the doc comment and the PR body claim. Ifuser_typeis the thing worth binding, binduser_typedirectly.
Either way, please don't leave a field whose only valid value is one the issuer cannot compute.
|
|
||
| /// Domain-separation prefix. Keeps the signed bytes from colliding with any other DoubleZero | ||
| /// message a verifier key might ever be asked to sign. | ||
| pub const IP_PROOF_DOMAIN: &[u8; 11] = b"DZ_IP_PROOF"; |
There was a problem hiding this comment.
This message layout diverges from RFC-27 and the RFC isn't updated in this PR. RFC-27 §Proof Specification (rfcs/rfc27-ip-verification.md:149-172) specifies {payer, client_ip, epoch, signature} and tells the program to "reconstruct message = payer || client_ip || epoch" — no domain prefix, no version byte, no user_pubkey. That's 44 bytes against the 88 here.
The crate's whole premise is that three independent consumers agree byte for byte, and the verification service (#4198) is the consumer most likely to be written from the RFC rather than from this crate. A service implementing the RFC as written signs 44 bytes while the program reconstructs 88, and every proof is rejected.
The prefix and version byte are genuine improvements — keep them, and update RFC-27's proof spec in this PR to the 88-byte layout, pointing at signed_message_for as the normative definition.
|
|
||
| /// Length of the signed message: prefix(11) + version(1) + payer(32) + client_ip(4) + epoch(8) + | ||
| /// user_pubkey(32). | ||
| pub const SIGNED_MESSAGE_LEN: usize = 88; |
There was a problem hiding this comment.
88 is a literal while every other offset below is derived. A future v2 that changes the prefix or inserts a field, without also editing this literal, turns message[EPOCH_END..].copy_from_slice(user_pubkey.as_ref()) into a length-mismatch panic — onchain, a program abort rather than a compile error. The test pinning 88 doesn't help: the literal and the offsets move together in the author's head, not in the code.
| pub const SIGNED_MESSAGE_LEN: usize = 88; | |
| pub const SIGNED_MESSAGE_LEN: usize = EPOCH_END + 32; |
(Module-level consts are order-independent, so the forward reference to EPOCH_END is fine. If you'd rather keep the literal as documentation, const _: () = assert!(EPOCH_END + 32 == SIGNED_MESSAGE_LEN); gets the same build-time guarantee.)
|
|
||
| /// Layout version of the signed message. Bump for any change to the field set or ordering — for | ||
| /// example a v2 that carries an IPv6 client address. | ||
| pub const IP_PROOF_VERSION: u8 = 1; |
There was a problem hiding this comment.
The version byte doesn't travel with the proof, so it can't do what this doc comment says. It isn't a field of IpOwnershipProof and signed_message_for() takes no version argument, so signed_message() always reconstructs the current version's bytes.
The moment this constant becomes 2, every outstanding v1 proof stops verifying, and neither the program nor the service has any way to accept v1 and v2 at once. The rollout becomes an atomic cutover across the program upgrade, the service, and every deployed CLI, inside one epoch freshness window — which is the situation a version byte is supposed to prevent.
Fix option — carry it in the proof and let callers reconstruct any version:
pub struct IpOwnershipProof {
pub version: u8,
...
}
pub fn signed_message_for(
version: u8,
payer: &Pubkey,
...
)with IP_PROOF_VERSION as the value the issuer writes, and the program accepting a set of supported versions. Alternatively, if concurrent versions are explicitly not a goal, say so in the doc comment so nobody plans a migration around a capability that isn't there.
| #[derive(thiserror::Error, Debug, PartialEq, Eq)] | ||
| pub enum IpProofError { | ||
| #[error("ip ownership proof signature does not verify against the verifier key")] | ||
| InvalidSignature, |
There was a problem hiding this comment.
InvalidSignature is produced solely by signer::verify(). With default = [] the BPF build gets an error enum nothing can construct, plus the thiserror dependency the Cargo.toml comment is trying to keep out of the program.
| #[derive(thiserror::Error, Debug, PartialEq, Eq)] | |
| pub enum IpProofError { | |
| #[error("ip ownership proof signature does not verify against the verifier key")] | |
| InvalidSignature, | |
| #[cfg(feature = "signer")] | |
| #[derive(thiserror::Error, Debug, PartialEq, Eq)] | |
| pub enum IpProofError { | |
| #[error("ip ownership proof signature does not verify against the verifier key")] | |
| InvalidSignature, | |
| } |
Pair it with thiserror = { workspace = true, optional = true } and signer = ["dep:thiserror", ...] so the default surface really is just borsh + solana-program.
| solana-signer = { workspace = true, optional = true } | ||
|
|
||
| [features] | ||
| default = [] |
There was a problem hiding this comment.
With default = [], nothing behind the non-default features is ever linted. make rust-lint runs cargo hack clippy --workspace --all-targets ... -Dclippy::all -Dwarnings (Makefile:97) with no feature flags, and cargo hack without --each-feature/--feature-powerset behaves like plain cargo — default features only. So signer.rs and test_vectors.rs, roughly two thirds of this crate, are exempt from clippy; a violation there lands green and only ever surfaces as a compile error via cargo test --all-features.
Fix option — add a per-crate pass to the rust-lint target:
cargo hack clippy -p doublezero-ip-proof --each-feature --all-targets -- -Dclippy::all -Dwarnings(--workspace --each-feature over the whole tree would be far too slow; scoping it to this crate is cheap.)
Resolves: #4195
Part of RFC-27 (
rfcs/rfc27-ip-verification.md), tracker #4194.Summary of Changes
crates/doublezero-ip-proof, definingIpOwnershipProofand the exact bytes the IP verification service signs. Three independent consumers — the serviceability program (BPF), the CLI, and the verification service — must agree byte for byte or every proof fails validation, so the format is defined once rather than duplicated three times.DZ_IP_PROOFdomain-separation prefix, a version byte, thenpayer || client_ip || epoch || user_pubkey. No length prefixes and no Borsh on the signed portion, so the program can reconstruct it on the stack and compare against what the Ed25519 precompile instruction covers. The version byte leaves a clean path to a v2 layout (IPv6 is out of scope).user_pubkeybinds a proof to one target User account, so a proof obtained for a routine connect cannot be replayed into another operation on a different account within the same epoch.signerfeature, keeping the default build BPF-clean (borsh,solana-program'sPubkey, thiserror).test-vectorsfeature, for the program, CLI, and service tests to assert against as those land.Nothing consumes the crate yet; this is a foundation change with no behavior impact. Onchain validation (#4197), the
GlobalStateverifier key (#4196), the service (#4198), and the client path (#4200/#4201) build on it.Note on the issue text
The issue named
ed25519-dalekfor the sign/verify helpers. This usessolana-keypair/solana-signer/solana-signatureinstead — already resolved inCargo.lock, and the verification service will hold a Solana keypair JSON, sosign_message()/Signature::verify()are the natural fit with no new crypto dependency. Same intent: the helpers stay behind a non-default feature so nothing extra reaches the program. Test vectors are Rust constants rather than a JSON file, since all three consumers named in the acceptance criteria are Rust.Diff Breakdown
Two thirds of the diff is tests and committed vectors; the format itself is ~150 lines. (Core and test counts split within
lib.rsandsigner.rs, which carry inline#[cfg(test)]modules.)Key files (click to expand)
crates/doublezero-ip-proof/src/lib.rs— theIpOwnershipProofstruct, layout constants, andsigned_message_for(), plus byte-layout and Borsh round-trip testscrates/doublezero-ip-proof/src/test_vectors.rs— two committed vectors (specific IP, and a wildcard-pass connect with a high epoch exercising the little-endian upper bytes) signed by a fixed test keypaircrates/doublezero-ip-proof/src/signer.rs—sign()/verify()behind thesignerfeature, off-chain onlycrates/doublezero-ip-proof/Cargo.toml— feature split keeping the default build BPF-cleanCargo.toml— new workspace member and dependency entriesTesting Verification
signed_message()against a hardcoded expected vector, with offset assertions on the prefix, version byte, and little-endian epoch, so a future field reorder fails loudly rather than silently invalidating every proof.payer,client_ip,epoch, anduser_pubkeychanges the signed message, and thatsignaturedoes not.client_ipserializes in network order.doublezero-serviceabilitywith ause, rancargo build-sbf --tools-version v1.54successfully, then reverted. The program does not consume the crate until serviceability: validate IpOwnershipProof via the Ed25519 precompile during user creation #4197.