diff --git a/CHANGELOG.md b/CHANGELOG.md index 89870a39a1..51e7d051ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ All notable changes to this project will be documented in this file. ### Changes +- Onchain programs + - Restrict granting the `FOUNDATION` permission flag: a plain `PERMISSION_ADMIN` holder can no longer grant `FOUNDATION` (a privilege escalation). Only a `foundation_allowlist` member or an existing `FOUNDATION` holder may grant it, enforced independently of `RequirePermissionAccounts` so foundation members are never locked out. +- CLI + - Add `doublezero permission audit` to check legacy→Permission parity before enabling `require-permission-accounts`: it reports which legacy keys would lose access to migrated instructions (coverage gaps, non-zero exit), super-admin holders, and the non-migrated subsystems that still depend on the GlobalState allowlists. - Collector - Harden ledger writes against a slow/degraded RPC endpoint: bound each RPC request (default 15s, `--ledger-rpc-timeout`), size the connection pool above the submitter concurrency (default 128, `--ledger-rpc-max-conns`), and deadline each submission attempt so it fails fast and retries with a fresh blockhash instead of sending an expired one and failing preflight with `BlockhashNotFound`. (#3973) - E2E diff --git a/smartcontract/cli/src/cli/command.rs b/smartcontract/cli/src/cli/command.rs index ec845049ce..b66105ebda 100644 --- a/smartcontract/cli/src/cli/command.rs +++ b/smartcontract/cli/src/cli/command.rs @@ -184,6 +184,7 @@ impl ServiceabilityCommand { PermissionCommands::Delete(args) => args.execute(ctx, client, out).await, PermissionCommands::Get(args) => args.execute(ctx, client, out).await, PermissionCommands::List(args) => args.execute(ctx, client, out).await, + PermissionCommands::Audit(args) => args.execute(ctx, client, out).await, }, Self::Tenant(cmd) => match cmd.command { TenantCommands::Create(args) => args.execute(ctx, client, out).await, @@ -395,6 +396,17 @@ mod tests { )); } + #[test] + fn parses_permission_audit() { + let parsed = TestCli::try_parse_from(["test", "permission", "audit"]).unwrap(); + assert!(matches!( + parsed.command, + ServiceabilityCommand::Permission(PermissionCliCommand { + command: PermissionCommands::Audit(_), + }) + )); + } + // `hide = true` must not gate parsing - operators and automation rely on // these verbs being reachable even though they do not appear in --help. #[test] diff --git a/smartcontract/cli/src/cli/permission.rs b/smartcontract/cli/src/cli/permission.rs index d258d66e12..3733d60aaf 100644 --- a/smartcontract/cli/src/cli/permission.rs +++ b/smartcontract/cli/src/cli/permission.rs @@ -1,6 +1,6 @@ use clap::{Args, Subcommand}; -use crate::permission::{delete::*, get::*, list::*, resume::*, set::*, suspend::*}; +use crate::permission::{audit::*, delete::*, get::*, list::*, resume::*, set::*, suspend::*}; #[derive(Args, Debug)] pub struct PermissionCliCommand { @@ -28,4 +28,7 @@ pub enum PermissionCommands { /// List all permission accounts #[clap()] List(ListPermissionCliCommand), + /// Audit legacy→Permission parity before enabling require-permission-accounts + #[clap()] + Audit(AuditPermissionCliCommand), } diff --git a/smartcontract/cli/src/permission/audit.rs b/smartcontract/cli/src/permission/audit.rs new file mode 100644 index 0000000000..971231ee22 --- /dev/null +++ b/smartcontract/cli/src/permission/audit.rs @@ -0,0 +1,488 @@ +use crate::{doublezerocommand::CliCommand, permission::flags::bitmask_to_names}; +use clap::Args; +use doublezero_cli_core::CliContext; +use doublezero_sdk::{commands::permission::list::ListPermissionCommand, GetGlobalStateCommand}; +use doublezero_serviceability::state::{ + feature_flags::{is_feature_enabled, FeatureFlag}, + globalstate::GlobalState, + permission::{permission_flags, Permission, PermissionStatus}, +}; +use serde::Serialize; +use solana_sdk::pubkey::Pubkey; +use std::{collections::HashMap, io::Write}; + +/// Permission flags whose instructions call `authorize()`. Enabling +/// `RequirePermissionAccounts` only changes authorization for these flags — every +/// other flag's instructions still gate on `GlobalState` directly, so a Permission +/// account grants nothing there. Keep in sync with the migrated processors (see +/// `PERMISSION.md`). +const MIGRATED_FLAGS: &[(u128, &str)] = &[ + (permission_flags::PERMISSION_ADMIN, "permission-admin"), + (permission_flags::ACCESS_PASS_ADMIN, "access-pass-admin"), + (permission_flags::USER_ADMIN, "user-admin"), + (permission_flags::TOPOLOGY_ADMIN, "topology-admin"), + (permission_flags::RESOURCE_ADMIN, "resource-admin"), + (permission_flags::INDEX_ADMIN, "index-admin"), +]; + +/// Subsystems still gated on `GlobalState` allowlists/authorities (NOT migrated to +/// `authorize()`). A Permission account grants NOTHING for these — removing a key +/// from `foundation_allowlist`/authorities breaks them with no Permission fallback. +/// Static because the enforcement surface lives in the on-chain program, not the CLI. +const NON_MIGRATED_SUBSYSTEMS: &[&str] = &[ + "device (create/update/delete/resume/sethealth, interface/*)", + "link (create/update/delete/suspend/resume/sethealth)", + "exchange (create/update/delete/suspend/resume/setdevice)", + "location (create/update/delete/suspend/resume)", + "contributor (create/update/delete/suspend/resume)", + "tenant (create/update/delete/add|remove_administrator/update_payment_status)", + "multicastgroup (create/update/delete/suspend/reactivate, allowlist publisher|subscriber add/remove)", + "user (create_core/update)", + "governance (globalstate/*, globalconfig/set, allowlist/foundation|qa/*)", +]; + +/// Legacy keys that authorize `flag` today, mirroring +/// `doublezero_serviceability::authorize::check_legacy_any` for the migrated flags. +/// +/// IMPORTANT: this MUST stay in sync with `check_legacy_any` (authorize.rs). If the +/// on-chain legacy mapping changes and this does not, the audit will report wrong +/// gaps. See the maintenance note in `smartcontract/programs/CLAUDE.md`. +fn legacy_keys_for_flag(gs: &GlobalState, flag: u128) -> Vec<(Pubkey, &'static str)> { + let mut keys = Vec::new(); + // Foundation authorizes every migrated flag in legacy mode. + let foundation_flags = permission_flags::PERMISSION_ADMIN + | permission_flags::ACCESS_PASS_ADMIN + | permission_flags::USER_ADMIN + | permission_flags::TOPOLOGY_ADMIN + | permission_flags::RESOURCE_ADMIN + | permission_flags::INDEX_ADMIN; + if flag & foundation_flags != 0 { + for pk in &gs.foundation_allowlist { + keys.push((*pk, "foundation-allowlist")); + } + } + // ACCESS_PASS_ADMIN also honors the sentinel and feed authorities. + if flag & permission_flags::ACCESS_PASS_ADMIN != 0 { + if gs.sentinel_authority_pk != Pubkey::default() { + keys.push((gs.sentinel_authority_pk, "sentinel-authority")); + } + if gs.feed_authority_pk != Pubkey::default() { + keys.push((gs.feed_authority_pk, "feed-authority")); + } + } + keys +} + +/// A legacy key that authorizes a migrated instruction today but lacks an equivalent +/// Permission account — i.e. it would lose access if strict mode were enabled. +#[derive(Serialize, Debug, PartialEq)] +struct Gap { + key: String, + flag: String, + legacy_source: String, +} + +#[derive(Serialize, Debug)] +struct AuditReport { + strict_mode_enabled: bool, + gaps: Vec, + permission_admin_holders: Vec, + foundation_flag_holders: Vec, + suspended: Vec, + unknown_bit_accounts: Vec, + foundation_allowlist: Vec, + non_migrated_subsystems: Vec, +} + +fn build_report(gs: &GlobalState, permissions: &HashMap) -> AuditReport { + // Index permissions by the pubkey they grant rights to (one per user_payer). + let by_payer: HashMap = + permissions.values().map(|p| (p.user_payer, p)).collect(); + + let strict_mode_enabled = + is_feature_enabled(gs.feature_flags, FeatureFlag::RequirePermissionAccounts); + + // Coverage gaps for migrated flags. + let mut gaps = Vec::new(); + for (flag, flag_name) in MIGRATED_FLAGS { + for (key, source) in legacy_keys_for_flag(gs, *flag) { + // Foundation members can always exercise PERMISSION_ADMIN via the + // foundation recovery path, so they are never at risk for that flag. + if *flag == permission_flags::PERMISSION_ADMIN && gs.foundation_allowlist.contains(&key) + { + continue; + } + let covered = by_payer.get(&key).is_some_and(|p| { + p.status == PermissionStatus::Activated && p.permissions & flag != 0 + }); + if !covered { + gaps.push(Gap { + key: key.to_string(), + flag: (*flag_name).to_string(), + legacy_source: source.to_string(), + }); + } + } + } + gaps.sort_by(|a, b| (&a.key, &a.flag).cmp(&(&b.key, &b.flag))); + + // Super-admin holders, suspended accounts, and accounts carrying unknown bits. + let mut permission_admin_holders = Vec::new(); + let mut foundation_flag_holders = Vec::new(); + let mut suspended = Vec::new(); + let mut unknown_bit_accounts = Vec::new(); + for p in permissions.values() { + if p.status == PermissionStatus::Suspended { + suspended.push(p.user_payer.to_string()); + } + if p.status == PermissionStatus::Activated { + if p.permissions & permission_flags::PERMISSION_ADMIN != 0 { + permission_admin_holders.push(p.user_payer.to_string()); + } + if p.permissions & permission_flags::FOUNDATION != 0 { + foundation_flag_holders.push(p.user_payer.to_string()); + } + } + // Set bits with no known name (count of set bits exceeds named bits). + if p.permissions.count_ones() as usize > bitmask_to_names(p.permissions).len() { + unknown_bit_accounts.push(p.user_payer.to_string()); + } + } + permission_admin_holders.sort(); + foundation_flag_holders.sort(); + suspended.sort(); + unknown_bit_accounts.sort(); + + let mut foundation_allowlist: Vec = gs + .foundation_allowlist + .iter() + .map(|k| k.to_string()) + .collect(); + foundation_allowlist.sort(); + + AuditReport { + strict_mode_enabled, + gaps, + permission_admin_holders, + foundation_flag_holders, + suspended, + unknown_bit_accounts, + foundation_allowlist, + non_migrated_subsystems: NON_MIGRATED_SUBSYSTEMS + .iter() + .map(|s| s.to_string()) + .collect(), + } +} + +fn render_text(out: &mut W, report: &AuditReport) -> eyre::Result<()> { + writeln!(out, "Permission Activation Audit")?; + writeln!(out, "===========================")?; + writeln!(out)?; + writeln!( + out, + "Strict mode (require-permission-accounts): {}", + if report.strict_mode_enabled { + "ON" + } else { + "OFF" + } + )?; + if report.strict_mode_enabled { + writeln!( + out, + " Gaps below indicate keys ALREADY locked out of migrated instructions." + )?; + } else { + writeln!( + out, + " Gaps below indicate keys that would LOSE access to migrated instructions" + )?; + writeln!(out, " if the flag were enabled now.")?; + } + writeln!(out)?; + + writeln!( + out, + "Legacy keys missing Permission coverage: {}", + report.gaps.len() + )?; + for g in &report.gaps { + writeln!(out, " {} flag={} via {}", g.key, g.flag, g.legacy_source)?; + } + writeln!(out)?; + + writeln!( + out, + "Super-admin review (PERMISSION_ADMIN can grant ANY flag, incl. FOUNDATION):" + )?; + writeln!( + out, + " permission-admin holders: {}", + fmt_list(&report.permission_admin_holders) + )?; + writeln!( + out, + " foundation-flag holders: {}", + fmt_list(&report.foundation_flag_holders) + )?; + writeln!(out)?; + + if !report.suspended.is_empty() { + writeln!( + out, + "Suspended Permission accounts: {}", + fmt_list(&report.suspended) + )?; + } + if !report.unknown_bit_accounts.is_empty() { + writeln!( + out, + "Permission accounts with unknown bits: {}", + fmt_list(&report.unknown_bit_accounts) + )?; + } + writeln!(out)?; + + writeln!( + out, + "Legacy dependency warning — DO NOT remove these keys yet" + )?; + writeln!( + out, + "-------------------------------------------------------" + )?; + writeln!( + out, + "The following subsystems are NOT migrated to the Permission model; they still" + )?; + writeln!( + out, + "authorize via GlobalState allowlists/authorities. A Permission account grants" + )?; + writeln!( + out, + "nothing for them, so removing a key from foundation_allowlist/authorities breaks" + )?; + writeln!(out, "these with no fallback:")?; + for s in &report.non_migrated_subsystems { + writeln!(out, " - {s}")?; + } + writeln!(out)?; + writeln!( + out, + "foundation_allowlist ({} member(s)): {}", + report.foundation_allowlist.len(), + fmt_list(&report.foundation_allowlist) + )?; + writeln!(out)?; + + if report.gaps.is_empty() { + writeln!(out, "Result: no coverage gaps for migrated instructions.")?; + } else { + writeln!( + out, + "Result: {} gap(s) — provisioning incomplete.", + report.gaps.len() + )?; + } + Ok(()) +} + +fn fmt_list(items: &[String]) -> String { + if items.is_empty() { + "(none)".to_string() + } else { + items.join(", ") + } +} + +#[derive(Args, Debug, Default)] +pub struct AuditPermissionCliCommand { + /// Output as pretty JSON + #[arg(long, default_value_t = false)] + pub json: bool, + /// Output as compact JSON + #[arg(long, default_value_t = false)] + pub json_compact: bool, +} + +impl AuditPermissionCliCommand { + pub async fn execute( + self, + _ctx: &CliContext, + client: &C, + out: &mut W, + ) -> eyre::Result<()> { + let (_, globalstate) = client.get_globalstate(GetGlobalStateCommand)?; + let permissions = client.list_permission(ListPermissionCommand {})?; + + let report = build_report(&globalstate, &permissions); + + if self.json || self.json_compact { + let s = if self.json_compact { + serde_json::to_string(&report)? + } else { + serde_json::to_string_pretty(&report)? + }; + writeln!(out, "{s}")?; + } else { + render_text(out, &report)?; + } + + // Non-zero exit when provisioning is incomplete, so this is usable as a + // pre-activation gate in a runbook / CI. + if !report.gaps.is_empty() { + return Err(eyre::eyre!( + "{} permission gap(s) found — do not enable require-permission-accounts until \ + every legacy key has an equivalent Permission account", + report.gaps.len() + )); + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tests::utils::create_test_client; + use doublezero_cli_core::testing::{block_on, cli_context_default_for_tests}; + use doublezero_sdk::AccountType; + use mockall::predicate; + + fn globalstate_with_foundation(members: Vec, feature_flags: u128) -> GlobalState { + GlobalState { + foundation_allowlist: members, + feature_flags, + ..Default::default() + } + } + + fn permission(user_payer: Pubkey, permissions: u128, status: PermissionStatus) -> Permission { + Permission { + account_type: AccountType::Permission, + owner: Pubkey::new_unique(), + bump_seed: 255, + status, + user_payer, + permissions, + } + } + + #[test] + fn test_legacy_keys_for_access_pass_admin_includes_sentinel_and_feed() { + let sentinel = Pubkey::new_unique(); + let feed = Pubkey::new_unique(); + let foundation = Pubkey::new_unique(); + let mut gs = globalstate_with_foundation(vec![foundation], 0); + gs.sentinel_authority_pk = sentinel; + gs.feed_authority_pk = feed; + + let keys = legacy_keys_for_flag(&gs, permission_flags::ACCESS_PASS_ADMIN); + let pks: Vec = keys.iter().map(|(k, _)| *k).collect(); + assert!(pks.contains(&foundation)); + assert!(pks.contains(&sentinel)); + assert!(pks.contains(&feed)); + } + + #[test] + fn test_foundation_not_flagged_for_permission_admin() { + // A foundation member with no Permission account is safe for PERMISSION_ADMIN + // (recovery path) but IS a gap for the other migrated flags. + let foundation = Pubkey::new_unique(); + let gs = globalstate_with_foundation(vec![foundation], 0); + let permissions = HashMap::new(); + + let report = build_report(&gs, &permissions); + assert!(!report.gaps.iter().any(|g| g.flag == "permission-admin")); + // user-admin, topology-admin, resource-admin, index-admin, access-pass-admin + assert_eq!(report.gaps.len(), 5); + } + + #[test] + fn test_no_gaps_when_foundation_fully_provisioned() { + let foundation = Pubkey::new_unique(); + let gs = globalstate_with_foundation(vec![foundation], 0); + // Grant every migrated non-recovery flag. + let mask = permission_flags::USER_ADMIN + | permission_flags::TOPOLOGY_ADMIN + | permission_flags::RESOURCE_ADMIN + | permission_flags::INDEX_ADMIN + | permission_flags::ACCESS_PASS_ADMIN; + let pda = Pubkey::new_unique(); + let permissions = HashMap::from([( + pda, + permission(foundation, mask, PermissionStatus::Activated), + )]); + + let report = build_report(&gs, &permissions); + assert!(report.gaps.is_empty(), "unexpected gaps: {:?}", report.gaps); + } + + #[test] + fn test_suspended_permission_does_not_cover() { + let foundation = Pubkey::new_unique(); + let gs = globalstate_with_foundation(vec![foundation], 0); + let mask = permission_flags::USER_ADMIN + | permission_flags::TOPOLOGY_ADMIN + | permission_flags::RESOURCE_ADMIN + | permission_flags::INDEX_ADMIN + | permission_flags::ACCESS_PASS_ADMIN; + let pda = Pubkey::new_unique(); + let permissions = HashMap::from([( + pda, + permission(foundation, mask, PermissionStatus::Suspended), + )]); + + let report = build_report(&gs, &permissions); + // Suspended → still a gap for all 5 non-recovery flags. + assert_eq!(report.gaps.len(), 5); + assert_eq!(report.suspended, vec![foundation.to_string()]); + } + + #[test] + fn test_superadmin_holders_reported() { + let holder = Pubkey::new_unique(); + let gs = globalstate_with_foundation(vec![], 0); + let pda = Pubkey::new_unique(); + let permissions = HashMap::from([( + pda, + permission( + holder, + permission_flags::PERMISSION_ADMIN | permission_flags::FOUNDATION, + PermissionStatus::Activated, + ), + )]); + + let report = build_report(&gs, &permissions); + assert_eq!(report.permission_admin_holders, vec![holder.to_string()]); + assert_eq!(report.foundation_flag_holders, vec![holder.to_string()]); + } + + #[test] + fn test_cli_permission_audit_reports_gap_and_errors() { + let mut client = create_test_client(); + let foundation = Pubkey::new_unique(); + let gs = globalstate_with_foundation(vec![foundation], 0); + let gstate_pubkey = Pubkey::new_unique(); + + client + .expect_get_globalstate() + .with(predicate::eq(GetGlobalStateCommand)) + .returning(move |_| Ok((gstate_pubkey, gs.clone()))); + client + .expect_list_permission() + .returning(|_| Ok(HashMap::new())); + + let mut output = Vec::new(); + let ctx = cli_context_default_for_tests(); + let cmd = AuditPermissionCliCommand::default(); + let res = block_on(cmd.execute(&ctx, &client, &mut output)); + + assert!(res.is_err(), "expected non-zero exit on gaps"); + let output_str = String::from_utf8(output).unwrap(); + assert!(output_str.contains("Legacy keys missing Permission coverage: 5")); + assert!(output_str.contains("DO NOT remove these keys yet")); + } +} diff --git a/smartcontract/cli/src/permission/mod.rs b/smartcontract/cli/src/permission/mod.rs index 4955a08c59..aa775c0939 100644 --- a/smartcontract/cli/src/permission/mod.rs +++ b/smartcontract/cli/src/permission/mod.rs @@ -1,3 +1,4 @@ +pub mod audit; pub mod delete; pub mod flags; pub mod get; diff --git a/smartcontract/programs/CLAUDE.md b/smartcontract/programs/CLAUDE.md index cce863d13f..440001336a 100644 --- a/smartcontract/programs/CLAUDE.md +++ b/smartcontract/programs/CLAUDE.md @@ -50,5 +50,9 @@ 1. **Keep `doublezero resource verify` in sync**: Anytime a processor allocates from or deallocates against a `ResourceExtension` (any field that pulls from `SegmentRoutingIds`, `TunnelIds`, `LinkIds`, `UserTunnelBlock`, `DeviceTunnelBlock`, `DzPrefixBlock`, `MulticastGroupBlock`, `MulticastPublisherBlock`, etc.), update the corresponding `verify_*` function in `smartcontract/cli/src/resource/verify.rs` so that field is counted as "in use." Otherwise the verifier will report the allocation as `AllocatedButNotUsed` (or miss a leak when a deallocation is dropped). Add coverage in the verify test module for the new field. +### Permissions + +1. **Keep `doublezero permission audit` in sync with `authorize::check_legacy_any`**: The audit's `legacy_keys_for_flag` (in `smartcontract/cli/src/permission/audit.rs`) mirrors the flag→legacy-key mapping in `authorize.rs::check_legacy_any` for the migrated flags. If you change that mapping (or migrate a new instruction to `authorize()`), update `legacy_keys_for_flag` and the `MIGRATED_FLAGS` / `NON_MIGRATED_SUBSYSTEMS` lists — otherwise the audit reports wrong gaps and understates lockout risk before enabling `RequirePermissionAccounts`. + ### Pull Requests - Make sure `make rust-lint` and `make rust-test` both pass before posting pull requests \ No newline at end of file diff --git a/smartcontract/programs/doublezero-serviceability/src/authorize.rs b/smartcontract/programs/doublezero-serviceability/src/authorize.rs index eca86d8de0..54da076c23 100644 --- a/smartcontract/programs/doublezero-serviceability/src/authorize.rs +++ b/smartcontract/programs/doublezero-serviceability/src/authorize.rs @@ -139,6 +139,39 @@ fn foundation_permission_recovery( && globalstate.foundation_allowlist.contains(payer_key) } +/// Whether `payer_key` may GRANT the `FOUNDATION` flag on a Permission account. +/// +/// Granting `FOUNDATION` is privileged beyond ordinary `PERMISSION_ADMIN`: a plain +/// `PERMISSION_ADMIN` holder must NOT be able to escalate anyone (including +/// themselves) to `FOUNDATION`. Only a `foundation_allowlist` member, or an existing +/// holder of the `FOUNDATION` flag via their own Activated Permission account, may do +/// so. This is independent of `RequirePermissionAccounts` — foundation members remain +/// authoritative in both modes. +/// +/// `permission_account` is the caller's own trailing Permission account (the one the +/// SDK auto-appends), if present. It is bound to `payer_key` by re-deriving the PDA, +/// so a caller cannot pass someone else's FOUNDATION permission. +pub fn can_grant_foundation( + program_id: &Pubkey, + permission_account: Option<&AccountInfo>, + payer_key: &Pubkey, + globalstate: &GlobalState, +) -> bool { + if globalstate.foundation_allowlist.contains(payer_key) { + return true; + } + if let Some(acc) = permission_account { + let (expected_pda, _) = get_permission_pda(program_id, payer_key); + if acc.key == &expected_pda && acc.owner == program_id && !acc.data_is_empty() { + if let Ok(p) = Permission::try_from(acc) { + return p.status == PermissionStatus::Activated + && p.permissions & permission_flags::FOUNDATION != 0; + } + } + } + false +} + /// Returns true if `payer` satisfies at least one of the requested flags using legacy /// GlobalState fields. fn check_legacy_any(payer: &Pubkey, globalstate: &GlobalState, any_of: u128) -> bool { @@ -1468,4 +1501,115 @@ mod tests { ) .is_ok()); } + + // ── can_grant_foundation ───────────────────────────────────────────────── + + #[test] + fn test_can_grant_foundation_allowlist_member() { + let program_id = Pubkey::new_unique(); + let payer = Pubkey::new_unique(); + let gs = gs_with_foundation(&payer); + // Foundation members may grant FOUNDATION even without a Permission account. + assert!(can_grant_foundation(&program_id, None, &payer, &gs)); + } + + #[test] + fn test_can_grant_foundation_none_denied() { + let program_id = Pubkey::new_unique(); + let payer = Pubkey::new_unique(); + let gs = GlobalState::default(); + assert!(!can_grant_foundation(&program_id, None, &payer, &gs)); + } + + #[test] + fn test_can_grant_foundation_flag_holder_allowed() { + let program_id = Pubkey::new_unique(); + let payer = Pubkey::new_unique(); + let (pda, _, mut data) = make_permission_data( + &program_id, + &payer, + PermissionStatus::Activated, + permission_flags::FOUNDATION | permission_flags::PERMISSION_ADMIN, + ); + let mut lamports = 100_000u64; + let account = AccountInfo::new( + &pda, + false, + false, + &mut lamports, + &mut data, + &program_id, + false, + Epoch::default(), + ); + let gs = GlobalState::default(); + assert!(can_grant_foundation( + &program_id, + Some(&account), + &payer, + &gs + )); + } + + #[test] + fn test_can_grant_foundation_permission_admin_only_denied() { + // A plain PERMISSION_ADMIN holder (no FOUNDATION flag, not in the allowlist) + // must NOT be able to grant FOUNDATION. + let program_id = Pubkey::new_unique(); + let payer = Pubkey::new_unique(); + let (pda, _, mut data) = make_permission_data( + &program_id, + &payer, + PermissionStatus::Activated, + permission_flags::PERMISSION_ADMIN, + ); + let mut lamports = 100_000u64; + let account = AccountInfo::new( + &pda, + false, + false, + &mut lamports, + &mut data, + &program_id, + false, + Epoch::default(), + ); + let gs = GlobalState::default(); + assert!(!can_grant_foundation( + &program_id, + Some(&account), + &payer, + &gs + )); + } + + #[test] + fn test_can_grant_foundation_suspended_flag_holder_denied() { + let program_id = Pubkey::new_unique(); + let payer = Pubkey::new_unique(); + let (pda, _, mut data) = make_permission_data( + &program_id, + &payer, + PermissionStatus::Suspended, + permission_flags::FOUNDATION, + ); + let mut lamports = 100_000u64; + let account = AccountInfo::new( + &pda, + false, + false, + &mut lamports, + &mut data, + &program_id, + false, + Epoch::default(), + ); + let gs = GlobalState::default(); + assert!(!can_grant_foundation( + &program_id, + Some(&account), + &payer, + &gs + )); + } } diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/permission/create.rs b/smartcontract/programs/doublezero-serviceability/src/processors/permission/create.rs index 68f060060a..ad6d186574 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/permission/create.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/permission/create.rs @@ -1,5 +1,5 @@ use crate::{ - authorize::authorize, + authorize::{authorize, can_grant_foundation}, error::DoubleZeroError, pda::get_permission_pda, seeds::{SEED_PERMISSION, SEED_PREFIX}, @@ -70,14 +70,34 @@ pub fn process_create_permission( } let globalstate = GlobalState::try_from(globalstate_account)?; + + // The SDK appends the caller's own Permission PDA as the trailing account when one + // exists on-chain. Capture it so it can serve both the PERMISSION_ADMIN check and + // the stricter FOUNDATION-grant check below. + let caller_permission = accounts_iter.next(); + authorize( program_id, - accounts_iter, + &mut caller_permission.into_iter(), payer_account.key, &globalstate, permission_flags::PERMISSION_ADMIN, )?; + // Granting FOUNDATION is more privileged than PERMISSION_ADMIN: a plain + // PERMISSION_ADMIN holder must not be able to escalate to FOUNDATION. Only a + // foundation_allowlist member or an existing FOUNDATION-flag holder may grant it. + if value.permissions & permission_flags::FOUNDATION != 0 + && !can_grant_foundation( + program_id, + caller_permission, + payer_account.key, + &globalstate, + ) + { + return Err(DoubleZeroError::NotAllowed.into()); + } + let permission = Permission { account_type: AccountType::Permission, owner: *payer_account.key, diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/permission/update.rs b/smartcontract/programs/doublezero-serviceability/src/processors/permission/update.rs index 4af0060021..d58452031d 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/permission/update.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/permission/update.rs @@ -1,5 +1,5 @@ use crate::{ - authorize::authorize, + authorize::{authorize, can_grant_foundation}, error::DoubleZeroError, pda::get_permission_pda, processors::validation::validate_program_account, @@ -74,14 +74,34 @@ pub fn process_update_permission( } let globalstate = GlobalState::try_from(globalstate_account)?; + + // The SDK appends the caller's own Permission PDA as the trailing account when one + // exists on-chain. Capture it for both the PERMISSION_ADMIN check and the stricter + // FOUNDATION-grant check below. + let caller_permission = accounts_iter.next(); + authorize( program_id, - accounts_iter, + &mut caller_permission.into_iter(), payer_account.key, &globalstate, permission_flags::PERMISSION_ADMIN, )?; + // Adding FOUNDATION is more privileged than PERMISSION_ADMIN: a plain + // PERMISSION_ADMIN holder must not be able to escalate a Permission to FOUNDATION. + // Only a foundation_allowlist member or an existing FOUNDATION-flag holder may. + if value.add & permission_flags::FOUNDATION != 0 + && !can_grant_foundation( + program_id, + caller_permission, + payer_account.key, + &globalstate, + ) + { + return Err(DoubleZeroError::NotAllowed.into()); + } + permission.permissions = (permission.permissions | value.add) & !value.remove; try_acc_write(&permission, permission_account, payer_account, accounts)?; diff --git a/smartcontract/programs/doublezero-serviceability/tests/permission_test.rs b/smartcontract/programs/doublezero-serviceability/tests/permission_test.rs index bba5132c07..70b46a01cf 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/permission_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/permission_test.rs @@ -697,3 +697,217 @@ async fn test_delete_self_removal_rejected() { "Caller should not be able to delete their own permission" ); } + +// Grants admin a plain PERMISSION_ADMIN permission and returns (admin keypair, its PDA). +async fn grant_permission_admin( + banks_client: &mut BanksClient, + payer: &Keypair, + program_id: Pubkey, + globalstate_pubkey: Pubkey, + flags: u128, +) -> (Keypair, Pubkey) { + let admin = Keypair::new(); + transfer(banks_client, payer, &admin.pubkey(), 100_000_000).await; + let (admin_perm_pda, _) = get_permission_pda(&program_id, &admin.pubkey()); + let recent_blockhash = banks_client.get_latest_blockhash().await.unwrap(); + execute_transaction( + banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::CreatePermission(PermissionCreateArgs { + user_payer: admin.pubkey(), + permissions: flags, + }), + vec![ + AccountMeta::new(admin_perm_pda, false), + AccountMeta::new_readonly(globalstate_pubkey, false), + ], + payer, + ) + .await; + (admin, admin_perm_pda) +} + +#[tokio::test] +async fn test_permission_admin_cannot_grant_foundation_on_create() { + let (mut banks_client, payer, program_id, globalstate_pubkey, _) = + setup_program_with_globalconfig().await; + + // Foundation grants `admin` a plain PERMISSION_ADMIN (no FOUNDATION flag). + let (admin, admin_perm_pda) = grant_permission_admin( + &mut banks_client, + &payer, + program_id, + globalstate_pubkey, + permission_flags::PERMISSION_ADMIN, + ) + .await; + + let target = Pubkey::new_unique(); + let (target_pda, _) = get_permission_pda(&program_id, &target); + + // admin (PERMISSION_ADMIN only, not foundation) tries to grant FOUNDATION -> denied. + let rb = banks_client.get_latest_blockhash().await.unwrap(); + let denied = try_execute_transaction_with_extra_accounts( + &mut banks_client, + rb, + program_id, + DoubleZeroInstruction::CreatePermission(PermissionCreateArgs { + user_payer: target, + permissions: permission_flags::FOUNDATION, + }), + vec![ + AccountMeta::new(target_pda, false), + AccountMeta::new_readonly(globalstate_pubkey, false), + ], + &admin, + &[AccountMeta::new_readonly(admin_perm_pda, false)], + ) + .await; + assert!( + denied.is_err(), + "PERMISSION_ADMIN holder must not be able to grant FOUNDATION" + ); + + // Control: the same admin CAN grant a non-FOUNDATION flag on the same PDA. + let rb2 = banks_client.get_latest_blockhash().await.unwrap(); + execute_transaction_with_extra_accounts( + &mut banks_client, + rb2, + program_id, + DoubleZeroInstruction::CreatePermission(PermissionCreateArgs { + user_payer: target, + permissions: permission_flags::USER_ADMIN, + }), + vec![ + AccountMeta::new(target_pda, false), + AccountMeta::new_readonly(globalstate_pubkey, false), + ], + &admin, + &[AccountMeta::new_readonly(admin_perm_pda, false)], + ) + .await; + let perm = get_permission(&mut banks_client, program_id, &target).await; + assert_eq!(perm.permissions, permission_flags::USER_ADMIN); +} + +#[tokio::test] +async fn test_permission_admin_cannot_add_foundation_on_update() { + let (mut banks_client, payer, program_id, globalstate_pubkey, _) = + setup_program_with_globalconfig().await; + + let (admin, admin_perm_pda) = grant_permission_admin( + &mut banks_client, + &payer, + program_id, + globalstate_pubkey, + permission_flags::PERMISSION_ADMIN, + ) + .await; + + // Foundation creates a target permission with USER_ADMIN. + let target = Pubkey::new_unique(); + let (target_pda, _) = get_permission_pda(&program_id, &target); + let rb = banks_client.get_latest_blockhash().await.unwrap(); + execute_transaction( + &mut banks_client, + rb, + program_id, + DoubleZeroInstruction::CreatePermission(PermissionCreateArgs { + user_payer: target, + permissions: permission_flags::USER_ADMIN, + }), + vec![ + AccountMeta::new(target_pda, false), + AccountMeta::new_readonly(globalstate_pubkey, false), + ], + &payer, + ) + .await; + + // admin tries to ADD FOUNDATION via update -> denied. + let rb2 = banks_client.get_latest_blockhash().await.unwrap(); + let denied = try_execute_transaction_with_extra_accounts( + &mut banks_client, + rb2, + program_id, + DoubleZeroInstruction::UpdatePermission(PermissionUpdateArgs { + add: permission_flags::FOUNDATION, + remove: 0, + }), + vec![ + AccountMeta::new(target_pda, false), + AccountMeta::new_readonly(globalstate_pubkey, false), + ], + &admin, + &[AccountMeta::new_readonly(admin_perm_pda, false)], + ) + .await; + assert!( + denied.is_err(), + "PERMISSION_ADMIN holder must not be able to add FOUNDATION via update" + ); + + // Control: admin CAN add a non-FOUNDATION flag. + let rb3 = banks_client.get_latest_blockhash().await.unwrap(); + execute_transaction_with_extra_accounts( + &mut banks_client, + rb3, + program_id, + DoubleZeroInstruction::UpdatePermission(PermissionUpdateArgs { + add: permission_flags::NETWORK_ADMIN, + remove: 0, + }), + vec![ + AccountMeta::new(target_pda, false), + AccountMeta::new_readonly(globalstate_pubkey, false), + ], + &admin, + &[AccountMeta::new_readonly(admin_perm_pda, false)], + ) + .await; + let perm = get_permission(&mut banks_client, program_id, &target).await; + assert_eq!( + perm.permissions, + permission_flags::USER_ADMIN | permission_flags::NETWORK_ADMIN + ); +} + +#[tokio::test] +async fn test_foundation_flag_holder_can_grant_foundation() { + let (mut banks_client, payer, program_id, globalstate_pubkey, _) = + setup_program_with_globalconfig().await; + + // Only foundation can bootstrap a FOUNDATION-flag holder. + let (admin, admin_perm_pda) = grant_permission_admin( + &mut banks_client, + &payer, + program_id, + globalstate_pubkey, + permission_flags::FOUNDATION | permission_flags::PERMISSION_ADMIN, + ) + .await; + + // admin holds the FOUNDATION flag, so it CAN grant FOUNDATION to a target. + let target = Pubkey::new_unique(); + let (target_pda, _) = get_permission_pda(&program_id, &target); + let rb = banks_client.get_latest_blockhash().await.unwrap(); + execute_transaction_with_extra_accounts( + &mut banks_client, + rb, + program_id, + DoubleZeroInstruction::CreatePermission(PermissionCreateArgs { + user_payer: target, + permissions: permission_flags::FOUNDATION, + }), + vec![ + AccountMeta::new(target_pda, false), + AccountMeta::new_readonly(globalstate_pubkey, false), + ], + &admin, + &[AccountMeta::new_readonly(admin_perm_pda, false)], + ) + .await; + let perm = get_permission(&mut banks_client, program_id, &target).await; + assert_eq!(perm.permissions, permission_flags::FOUNDATION); +}