diff --git a/CHANGELOG.md b/CHANGELOG.md index d8f4a63546..3a26a21604 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ All notable changes to this project will be documented in this file. ### Changes +- Serviceability + - The RFC-27 sentinel exemption from IP ownership proof validation is narrowed to the shape the shred-oracle actually creates: a `UserType::Multicast` user whose owner is not the transaction payer. Previously any creation paid for by `globalstate.sentinel_authority_pk` could omit a proof, so a compromised sentinel key could bind an arbitrary `client_ip` to a unicast user or to a user it owned itself. Those now require a proof like any other registrant, and the sentinel can obtain one for an address it operates. `doublezero permission audit` reports the exemption and the key holding it whenever `require-ip-ownership-proof` is set, so the remaining privilege is not invisible. (#4215) - SDK - The Go serviceability `Executor.CreateUser` now emits the RFC-27 `ip_proof` Option discriminant explicitly, so its payload matches the Rust `user_create_args` fixture byte for byte instead of relying on `BorshDeserializeIncremental` to default the missing field. It also refuses to submit when an environment has `require-ip-ownership-proof` set, with a message saying why, rather than spending a transaction per user to collect `IpOwnershipProofRequired` (105): the verifier signs only the address it observes a request originate from, so the synthetic addresses the device-stress orchestrator names cannot be proven at all. Serviceability custom error codes 91-118 gained names, and the RFC-27 rejection classes (105-118) are matchable with `errors.Is` via `ClassifyProgramError`. (#4225) - The TypeScript and Python `GlobalState` deserializers expose `ip_verifier_authority_pk`, the RFC-27 trust root the Go SDK and the Rust state already carried, so those consumers can read which key signs IP ownership proofs. The field is appended, so an account written before the upgrade decodes it as the default pubkey rather than failing. (#4231) diff --git a/e2e/ip_ownership_proof_enforcement_test.go b/e2e/ip_ownership_proof_enforcement_test.go index 21c9244f56..2c7ce62446 100644 --- a/e2e/ip_ownership_proof_enforcement_test.go +++ b/e2e/ip_ownership_proof_enforcement_test.go @@ -132,18 +132,22 @@ func TestE2E_IPOwnershipProof_Enforced(t *testing.T) { requireNoUserForIP(t, dn, unverified.CYOANetworkIP) }) - // The sentinel exemption, which is what keeps enforcement from breaking the shred-oracle. + // The bound on the sentinel exemption, after #4215 narrowed it. // - // The oracle provisions multicast publishers owned by validators, for addresses the - // verification service never sees a request from, so there is no proof it could obtain. The - // program waives the *requirement* for a creation paid for by - // `globalstate.sentinel_authority_pk`. + // The exemption exists so enforcement does not break the shred-oracle, which provisions + // multicast publishers owned by validators for addresses the verification service never sees a + // request from. It is now scoped to exactly that shape — `UserType::Multicast` with an owner + // other than the payer — rather than to any creation the sentinel pays for. // - // In this devnet the manager is that authority (`smartcontract_init.go` runs - // `authority set --sentinel-authority me`), and `doublezero user create` never attaches a - // proof at all, so a manager-side create is the exemption in action. The contrast with - // wildcard_pass_without_a_proof is the transaction payer, which is what `is_sentinel` compares. - t.Run("sentinel_authority_is_exempt", func(t *testing.T) { + // In this devnet the manager is the sentinel authority (`smartcontract_init.go` runs + // `authority set --sentinel-authority me`) and `doublezero user create` attaches no proof, owns + // the user itself, and is unicast. Before the narrowing that combination was admitted; it is + // the privilege #4215 removed, so it must now be refused like any other unproven creation. + // + // The oracle shape itself is covered onchain by + // `test_sentinel_exemption_covers_the_oracle_shape`, which can construct a validator-owned + // multicast create directly; reproducing it here would mean standing up the sentinel service. + t.Run("sentinel_authority_is_not_exempt_for_a_unicast_self_owned_create", func(t *testing.T) { // An address the manager owns a pass for. It has no container behind it; this subtest is // about whether the creation is admitted, not about tunnels. const sentinelUserIP = "9.0.0.9" @@ -153,15 +157,18 @@ func TestE2E_IPOwnershipProof_Enforced(t *testing.T) { sentinelUserIP + " --user-payer me"}) require.NoError(t, err) - log.Info("==> Creating a user as the sentinel authority, with no proof") + log.Info("==> Creating a unicast user as the sentinel authority, with no proof") out, err := dn.Manager.Exec(t.Context(), []string{"bash", "-c", "doublezero user create --device " + device.Spec.Code + " --client-ip " + sentinelUserIP + " 2>&1"}) - log.Info("==> User create output", "output", string(out)) - require.NoError(t, err, "the sentinel authority must be exempt from the proof requirement: %s", string(out)) + output := string(out) + log.Info("==> User create output", "output", output) + require.Error(t, err, + "a sentinel-paid unicast create owns its own user, so the narrowed exemption must not cover it: %s", + output) + require.Contains(t, output, "An IP ownership proof is required to create a user", + "the refusal must be the program's IpOwnershipProofRequired, not an incidental failure") - users, err := dn.Manager.Exec(t.Context(), []string{"bash", "-c", "doublezero user list"}) - require.NoError(t, err) - require.Contains(t, string(users), sentinelUserIP) + requireNoUserForIP(t, dn, sentinelUserIP) }) } diff --git a/rfcs/rfc27-ip-verification.md b/rfcs/rfc27-ip-verification.md index 44c33dbe83..ebd1e451c4 100644 --- a/rfcs/rfc27-ip-verification.md +++ b/rfcs/rfc27-ip-verification.md @@ -258,8 +258,8 @@ The program MUST reject when any of the following holds: - no verifier public key is configured in global state; - the proof is malformed. -A proof that is *absent* is rejected only when the flag is set and the payer is not the sentinel -authority; see Backward Compatibility. +A proof that is *absent* is rejected only when the flag is set and the creation is not the narrow +oracle shape the sentinel authority is exempt for; see Backward Compatibility. Each class has its own `DoubleZeroError` variant, so an operator can tell a stale proof from a rotated verifier key from a client that never attached the Ed25519 instruction. @@ -331,8 +331,9 @@ set per environment through the existing `SetFeatureFlags` instruction. working. A creation that *does* supply one is still validated in full: a client attaching a broken proof is broken now, not at rollout, and letting it through would hide that until the flag flips. - **Flag set.** Every user creation requires a valid proof — wildcard and specific-IP passes alike, - and on the idempotent rerun path as well as on first creation. One exception: a creation paid for - by `globalstate.sentinel_authority_pk` may omit the proof. + and on the idempotent rerun path as well as on first creation. One narrow exception: a creation + paid for by `globalstate.sentinel_authority_pk` may omit the proof **when it is the shape the + shred-oracle produces** — a `UserType::Multicast` user whose owner is not the payer. **The sentinel exemption.** The shred-oracle provisions multicast publishers owned by validators, so the proof would have to name the validator for an address the verification service never sees a @@ -340,10 +341,30 @@ request from — there is no proof the oracle could obtain, and without the exem would break that path outright. Unlike the wildcard-pass gap this RFC closes, the exemption is not reachable by a registrant: it requires a DoubleZero-operated key. It waives the *requirement* only; a proof the sentinel does attach is still validated in full, so the oracle can start carrying real -proofs without a program change. The residual risk is that a compromised sentinel key can bind any -IP, and that `InitGlobalState` seeds `sentinel_authority_pk` to whoever initialized global state, so -in a fresh environment the exemption belongs to the deployer until the key is rotated. Replacing it -is tracked in issue #4215. +proofs without a program change. + +**Scoped to the oracle shape (#4215).** The exemption is *not* a blanket waiver for anything the +sentinel pays for. The predicate is + +```rust +is_sentinel && effective_owner != payer && user_type == UserType::Multicast +``` + +which is exactly what `crates/sentinel/src/dz_ledger_writer.rs` builds (`owner` set to the +validator, `UserType::Multicast`, `publisher: true`). A sentinel-paid creation of any other shape — +a unicast user, or a multicast user the sentinel owns itself — requires a proof like any other +registrant, and for those shapes the sentinel operates the address and so can obtain one. + +The residual risk that remains is confined to that shape: a compromised sentinel key can bind an +arbitrary `client_ip` to a validator-owned multicast publisher. It can no longer bind an address to +a unicast user or to a user it owns. Note also that `InitGlobalState` seeds `sentinel_authority_pk` +to whoever initialized global state, so in a fresh environment even the narrowed exemption belongs +to the deployer until the key is rotated. `doublezero permission audit` reports the exemption and +the key that holds it. + +Fully removing it needs the oracle to relay a proof the validator obtained for its own address, +which is work in `doublezero-shreds` rather than here; the program already validates a proof the +sentinel supplies, so no program change is required when that lands. The proof is optional on the wire rather than on the instruction: `BorshDeserializeIncremental` decodes an older client's shorter payload as `None`, and whether `None` is acceptable is the flag's diff --git a/smartcontract/cli/src/permission/audit.rs b/smartcontract/cli/src/permission/audit.rs index cf3bd33b34..b485fdf905 100644 --- a/smartcontract/cli/src/permission/audit.rs +++ b/smartcontract/cli/src/permission/audit.rs @@ -27,6 +27,10 @@ const NON_MIGRATED_SUBSYSTEMS: &[&str] = &[ via the USER_ADMIN legacy mapping, Permission holders directly — so only the sentinel_authority \ direct check remains un-migrated; qa_allowlist/foundation_allowlist still bypass device status & \ seat limits)", + "user create RFC-27 IP ownership proof (sentinel_authority may omit a proof for the shred-oracle \ + shape — UserType::Multicast owned by somebody other than the payer — via a direct \ + sentinel_authority_pk check, not through authorize(); see the IP ownership proof section above \ + for whether it is currently active)", ]; /// A legacy key that authorizes a migrated instruction today but lacks an equivalent @@ -48,9 +52,22 @@ struct Gap { /// `true` and drop the rendered paragraph when the builder-side append is re-enabled. const SDK_ATTACHES_PERMISSION_ACCOUNTS: bool = false; +/// The RFC-27 sentinel exemption from IP ownership proof validation, reported so a +/// privileged path is never invisible. `None` means enforcement is off, so the +/// exemption grants nothing today. +#[derive(Serialize, Debug, PartialEq)] +struct SentinelIpProofExemption { + /// The key the exemption belongs to (`globalstate.sentinel_authority_pk`). + key: String, + /// The creation shape it covers. Anything else needs a proof. + scope: String, +} + #[derive(Serialize, Debug)] struct AuditReport { strict_mode_enabled: bool, + require_ip_ownership_proof_enabled: bool, + sentinel_ip_proof_exemption: Option, /// See [`SDK_ATTACHES_PERMISSION_ACCOUNTS`]. sdk_attaches_permission_accounts: bool, gaps: Vec, @@ -70,6 +87,20 @@ fn build_report(gs: &GlobalState, permissions: &HashMap) -> let strict_mode_enabled = is_feature_enabled(gs.feature_flags, FeatureFlag::RequirePermissionAccounts); + // RFC-27 (#4215). The exemption is narrowed to the shape the shred-oracle creates, but a + // narrowed privilege is still a privilege, so it is reported whenever it can be exercised. + let require_ip_ownership_proof_enabled = + is_feature_enabled(gs.feature_flags, FeatureFlag::RequireIpOwnershipProof); + let sentinel_ip_proof_exemption = (require_ip_ownership_proof_enabled + && gs.sentinel_authority_pk != Pubkey::default()) + .then(|| SentinelIpProofExemption { + key: gs.sentinel_authority_pk.to_string(), + scope: "may omit an IP ownership proof for a UserType::Multicast creation whose owner is \ + not the payer (the shred-oracle shape); every other creation it pays for needs a \ + proof" + .to_string(), + }); + // Coverage gaps for every flag routed through `authorize()`. The set of gated // flags and the legacy→key mapping both come from the serviceability crate, so // this can never drift from the on-chain authorization. @@ -133,6 +164,8 @@ fn build_report(gs: &GlobalState, permissions: &HashMap) -> AuditReport { strict_mode_enabled, + require_ip_ownership_proof_enabled, + sentinel_ip_proof_exemption, sdk_attaches_permission_accounts: SDK_ATTACHES_PERMISSION_ACCOUNTS, gaps, permission_admin_holders, @@ -269,6 +302,33 @@ fn render_text(out: &mut W, report: &AuditReport) -> eyre::Result<()> )?; writeln!(out)?; } + writeln!( + out, + "IP ownership proof (require-ip-ownership-proof): {}", + if report.require_ip_ownership_proof_enabled { + "ON" + } else { + "OFF" + } + )?; + match &report.sentinel_ip_proof_exemption { + Some(e) => { + writeln!(out, " Sentinel exemption ACTIVE — held by {}", e.key)?; + writeln!(out, " Scope: {}", e.scope)?; + } + None if report.require_ip_ownership_proof_enabled => { + writeln!(out, " No sentinel authority set, so no exemption is held.")?; + } + None => { + writeln!( + out, + " Enforcement is off, so no proof is required of anyone and the sentinel" + )?; + writeln!(out, " exemption grants nothing.")?; + } + } + writeln!(out)?; + writeln!( out, "foundation_allowlist ({} member(s)): {}", @@ -402,6 +462,46 @@ mod tests { assert!(pks.contains(&feed)); } + #[test] + fn test_sentinel_ip_proof_exemption_is_reported_only_while_enforced() { + // RFC-27 (#4215). The exemption is a privileged path, so the audit must name the key that + // holds it — but only when the flag makes it exercisable, or the report cries wolf in + // every environment that has not turned enforcement on. + let sentinel = Pubkey::new_unique(); + + // Enforcement off: nothing to report even though a sentinel is set. + let mut gs = globalstate_with_foundation(vec![], 0); + gs.sentinel_authority_pk = sentinel; + let report = build_report(&gs, &HashMap::new()); + assert!(!report.require_ip_ownership_proof_enabled); + assert_eq!(report.sentinel_ip_proof_exemption, None); + + // Enforcement on: the holder is named. + let mut gs = + globalstate_with_foundation(vec![], FeatureFlag::RequireIpOwnershipProof.to_mask()); + gs.sentinel_authority_pk = sentinel; + let report = build_report(&gs, &HashMap::new()); + assert!(report.require_ip_ownership_proof_enabled); + let exemption = report + .sentinel_ip_proof_exemption + .as_ref() + .expect("an active exemption must be reported"); + assert_eq!(exemption.key, sentinel.to_string()); + assert!(exemption.scope.contains("Multicast")); + + // Enforcement on with no sentinel set: nobody holds it. + let gs = + globalstate_with_foundation(vec![], FeatureFlag::RequireIpOwnershipProof.to_mask()); + let report = build_report(&gs, &HashMap::new()); + assert_eq!(report.sentinel_ip_proof_exemption, None); + + // The rendered text has to say so, not just the JSON. + let mut out = Vec::new(); + render_text(&mut out, &report).unwrap(); + let text = String::from_utf8(out).unwrap(); + assert!(text.contains("require-ip-ownership-proof")); + } + #[test] fn test_foundation_not_flagged_for_permission_admin() { // A foundation member with no Permission account is safe for PERMISSION_ADMIN diff --git a/smartcontract/programs/doublezero-serviceability/src/ip_proof.rs b/smartcontract/programs/doublezero-serviceability/src/ip_proof.rs index f1bd685069..4cd720de16 100644 --- a/smartcontract/programs/doublezero-serviceability/src/ip_proof.rs +++ b/smartcontract/programs/doublezero-serviceability/src/ip_proof.rs @@ -69,16 +69,22 @@ pub fn split_trailing_instructions_sysvar<'a, 'info>( /// enforcement has been switched on, and letting it through would mask that until the flag flips. /// With the flag set, a proof is required for every user creation, wildcard and specific-IP passes /// alike (#4192 item 3: a legacy path that skips the proof is exactly the hole the RFC exists to -/// close) — with one exception, `payer_is_sentinel`. +/// close) — with one narrow exception, `sentinel_exemption`. /// -/// `payer_is_sentinel` waives the *requirement* only, never validation. The shred-oracle +/// `sentinel_exemption` waives the *requirement* only, never validation. The shred-oracle /// provisions multicast publishers owned by validators (`crates/sentinel`), so the proof would have /// to name the validator for an address the oracle never observes a request from; there is no proof /// the oracle could obtain today. Unlike the wildcard-pass hole this RFC closes, the exemption is /// not reachable by a registrant: it requires `globalstate.sentinel_authority_pk`, a -/// DoubleZero-operated key. The residual risk is that a compromised sentinel key can bind any IP, -/// which it already could. A proof the sentinel *does* attach is still checked in full, so the -/// oracle can start carrying real proofs without a program change (#4215). +/// DoubleZero-operated key. A proof the sentinel *does* attach is still checked in full, so the +/// oracle can start carrying real proofs without a program change. +/// +/// #4215 narrowed it. The caller must decide the exemption applies only for the *shape* the oracle +/// actually creates — a multicast publisher owned by somebody other than the payer — not for any +/// creation the sentinel happens to pay for. A blanket exemption let a compromised sentinel key +/// bind any `client_ip` to any user type, including a plain unicast user for itself; narrowing it +/// leaves only the case that would otherwise break outright. See +/// `create_user_core` for the predicate. /// /// `user_type` binds the proof to the account it authorizes: the User PDA is /// `f(client_ip, user_type)`, so `client_ip` alone would leave a proof reusable for a different @@ -92,12 +98,12 @@ pub fn validate_ip_ownership_proof( client_ip: &Ipv4Addr, user_type: u8, current_epoch: u64, - payer_is_sentinel: bool, + sentinel_exemption: bool, ) -> Result<(), ProgramError> { let proof = match proof { Some(proof) => proof, None => { - return if payer_is_sentinel { + return if sentinel_exemption { Ok(()) } else if is_feature_enabled( globalstate.feature_flags, diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs b/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs index dbe79f28a8..c5bf55d45b 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs @@ -183,6 +183,22 @@ pub fn create_user_core( // who must have demonstrated control of `client_ip` — which is also the identity the AccessPass // is keyed on (`accesspass.user_payer` below). On the ordinary path the two are the same // account. + // + // #4215. The exemption is narrowed to the shape the shred-oracle actually creates: a multicast + // publisher owned by somebody other than the payer. `crates/sentinel` builds exactly that — + // `owner` set to the validator, `UserType::Multicast`, `publisher: true` — and it is the only + // creation for which no obtainable proof exists, because the verifier signs only the address it + // observes a request originate from and the oracle never sends one from the validator's + // address. + // + // Everything else a sentinel-paid transaction could create now needs a proof like any other + // registrant. A blanket `is_sentinel` waiver let a compromised sentinel key bind any client_ip + // to any user type, including a plain unicast user for itself, which is the privilege this + // narrowing removes. + let sentinel_exemption = is_sentinel + && effective_owner != *core.payer_account.key + && user_type == UserType::Multicast; + validate_ip_ownership_proof( core.instructions_sysvar_account, ip_proof, @@ -191,7 +207,7 @@ pub fn create_user_core( &client_ip, user_type as u8, clock.epoch, - is_sentinel, + sentinel_exemption, )?; // Check account Types diff --git a/smartcontract/programs/doublezero-serviceability/tests/user_ip_proof_test.rs b/smartcontract/programs/doublezero-serviceability/tests/user_ip_proof_test.rs index 664ef20d9c..1c853b356c 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/user_ip_proof_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/user_ip_proof_test.rs @@ -859,13 +859,14 @@ async fn test_owner_override_proof_naming_the_payer_is_rejected() { } #[tokio::test] -async fn test_sentinel_payer_may_create_without_a_proof_while_the_flag_is_set() { +async fn test_sentinel_exemption_covers_the_oracle_shape() { let mut f = setup().await; f.require_proof().await; - // The shred-oracle provisions multicast publishers owned by validators, so a proof would have - // to name the validator for an address the oracle never observes a request from — there is no - // proof it could obtain. The exemption is keyed on the sentinel authority, a + // The shape `crates/sentinel` actually creates: a multicast user owned by somebody other than + // the payer. A proof would have to name the validator for an address the oracle never observes + // a request from, so there is no proof it could obtain, and without the exemption setting the + // flag would break this path outright. The exemption is keyed on the sentinel authority, a // DoubleZero-operated key, so it is not a path a registrant can reach. let sentinel = f.payer.pubkey(); f.set_sentinel_authority(sentinel).await; @@ -875,19 +876,56 @@ async fn test_sentinel_payer_may_create_without_a_proof_while_the_flag_is_set() f.create_subscribe_with_owner(owner, accesspass, CLIENT_IP, None) .await - .expect("the sentinel authority is exempt from the proof requirement"); + .expect("the oracle shape is exempt from the proof requirement"); assert!(f.user_exists(CLIENT_IP, UserType::Multicast).await); } +#[tokio::test] +async fn test_sentinel_payer_is_not_exempt_for_a_unicast_create() { + let mut f = setup().await; + f.require_proof().await; + + // #4215. A unicast create is not a shape the oracle produces, so the sentinel gets no waiver + // for it even though it is the payer. Before the narrowing this succeeded, which meant a + // compromised sentinel key could bind any client_ip to an IBRL user of its own. + let sentinel = f.payer.pubkey(); + f.set_sentinel_authority(sentinel).await; + + let result = f.create_user(None, &[], false).await; + assert_rejected(result, DoubleZeroError::IpOwnershipProofRequired); + assert!(!f.user_exists(CLIENT_IP, UserType::IBRL).await); +} + +#[tokio::test] +async fn test_sentinel_payer_is_not_exempt_when_it_owns_the_user_itself() { + let mut f = setup().await; + f.require_proof().await; + + // The other half of the narrowing: multicast, but owned by the payer rather than a validator. + // The oracle always names a separate owner, so a self-owned creation has no claim on the + // exemption — and the sentinel can obtain a proof for an address it does operate. + let sentinel = f.payer.pubkey(); + f.set_sentinel_authority(sentinel).await; + + let accesspass = f.provision_owner(sentinel, CLIENT_IP).await; + + let result = f + .create_subscribe_with_owner(sentinel, accesspass, CLIENT_IP, None) + .await; + assert_rejected(result, DoubleZeroError::IpOwnershipProofRequired); + assert!(!f.user_exists(CLIENT_IP, UserType::Multicast).await); +} + #[tokio::test] async fn test_sentinel_payer_still_has_a_supplied_proof_validated() { let mut f = setup().await; f.require_proof().await; - // The exemption waives the requirement, not validation. Keeping a supplied proof checked is - // what lets the oracle start carrying real proofs without a program change, and it means a - // broken one is a visible error rather than a silent bypass. + // The exemption waives the requirement, not validation, so this uses the oracle shape and + // still gets its proof checked. That is what lets the oracle start carrying real proofs + // without a program change, and it means a broken one is a visible error rather than a silent + // bypass. let sentinel = f.payer.pubkey(); f.set_sentinel_authority(sentinel).await;