diff --git a/docs/dashpay/INTEROP_DESK_CHECK.md b/docs/dashpay/INTEROP_DESK_CHECK.md index 4a42145d843..ab67a726b67 100644 --- a/docs/dashpay/INTEROP_DESK_CHECK.md +++ b/docs/dashpay/INTEROP_DESK_CHECK.md @@ -471,17 +471,18 @@ The alignment recommendation above is superseded on the receive side by contacts established through the legacy Android/dashj client had 27 inbound requests that reference the recipient's AUTHENTICATION or TRANSFER key, and the documents are immutable, so rejecting them left those contacts unpayable. The -rules now are (rs-sdk `platform/dashpay/contact_request.rs`): +rules now are: -- **Send (documents we create):** unchanged. The sender key must be ENCRYPTION - (`sender_key_purpose_is_valid`) and the recipient key DECRYPTION or ENCRYPTION +- **Send (documents we create; rs-sdk `platform/dashpay/contact_request.rs`):** + unchanged. The sender key must be ENCRYPTION (`sender_key_purpose_is_valid`) + and the recipient key DECRYPTION or ENCRYPTION (`recipient_key_purpose_is_valid`). -- **Receive (documents already on chain):** the recipient key may be DECRYPTION, - ENCRYPTION, AUTHENTICATION or TRANSFER - (`recipient_key_purpose_is_acceptable_on_receive`), and the sender key - ENCRYPTION or AUTHENTICATION (`sender_key_purpose_is_acceptable_on_receive`). - Any other purpose is a mismatch that is skipped and retried, never a - permanently broken channel. +- **Receive (documents already on chain; rs-platform-wallet + `wallet/identity/crypto/validation.rs`, `validate_contact_request`):** key + purpose is not checked. Only the key type is: the sender key must be + ECDSA_SECP256K1, and the recipient key ECDSA_SECP256K1 or ECDSA_HASH160. A + key of another type is a permanent error; a key id absent from the identity + is retryable, since identities gain keys; a disabled key is only a warning. ## Re-checking against the reference clients diff --git a/packages/rs-dpp/src/errors/protocol_error.rs b/packages/rs-dpp/src/errors/protocol_error.rs index 1cc5106db22..90cb7b50392 100644 --- a/packages/rs-dpp/src/errors/protocol_error.rs +++ b/packages/rs-dpp/src/errors/protocol_error.rs @@ -15,6 +15,7 @@ use crate::document::errors::*; ))] use crate::state_transition::errors::InvalidIdentityPublicKeyTypeError; +use crate::identity::key_disable_rule::LastAuthenticationKeyError; #[cfg(all(feature = "state-transitions", feature = "validation"))] use crate::state_transition::errors::StateTransitionError; @@ -324,6 +325,12 @@ pub enum ProtocolError { using: u16, msg: &'static str, }, + + /// An identity update would disable the last valid authentication key of a security + /// level (DIP-11). Refused by the signing builder before anything is signed; this is + /// client-side policy, not a consensus rule, so it is never a `ConsensusError`. + #[error(transparent)] + LastAuthenticationKey(#[from] LastAuthenticationKeyError), } impl From<&str> for ProtocolError { diff --git a/packages/rs-dpp/src/identity/identity_public_key/key_type.rs b/packages/rs-dpp/src/identity/identity_public_key/key_type.rs index 02df74d6347..3747497cf3f 100644 --- a/packages/rs-dpp/src/identity/identity_public_key/key_type.rs +++ b/packages/rs-dpp/src/identity/identity_public_key/key_type.rs @@ -99,6 +99,21 @@ impl KeyType { } } + /// Can a key of this type sign an identity-signed state transition? + /// + /// Mirrors the `SUPPORTED_KEY_TYPES` set drive-abci checks in + /// `validate_state_transition_identity_signed` (ECDSA_SECP256K1, BLS12_381 and + /// ECDSA_HASH160); `StateTransition::sign` refuses the other types. Keep the two in step. + pub fn can_sign_state_transitions(&self) -> bool { + match self { + KeyType::ECDSA_SECP256K1 => true, + KeyType::BLS12_381 => true, + KeyType::ECDSA_HASH160 => true, + KeyType::BIP13_SCRIPT_HASH => false, + KeyType::EDDSA_25519_HASH160 => false, + } + } + /// Can this key type be understood as an address on the Core chain? pub fn is_core_address_key_type(&self) -> bool { match self { diff --git a/packages/rs-dpp/src/identity/key_disable_rule.rs b/packages/rs-dpp/src/identity/key_disable_rule.rs new file mode 100644 index 00000000000..ebec25c36ac --- /dev/null +++ b/packages/rs-dpp/src/identity/key_disable_rule.rs @@ -0,0 +1,495 @@ +//! The DIP-11 rule on disabling an identity's authentication keys. +//! +//! DIP-11: "Keys can only be disabled if another valid key is enabled in the same security +//! level. This can happen atomically in the same state transition." +//! +//! **This is signing-side policy, not consensus.** Drive enforces it for MASTER keys only and +//! accepts an identity update that disables the last key of any other level, so this module +//! never produces a `ConsensusError` and nothing on a block-execution path calls it. Clients +//! enforce it: `IdentityUpdateTransition::try_from_identity_with_signer` (the +//! `state-transition-signing` builder) refuses such an update before signing anything, and an +//! interface with a clock can call [`check_keys_can_be_disabled`] to offer only the keys it may +//! disable. + +use crate::identity::accessors::IdentityGettersV0; +use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; +use crate::identity::identity_public_key::accessors::v1::IdentityPublicKeyGettersV1; +use crate::identity::{Identity, IdentityPublicKey, KeyID, Purpose, SecurityLevel}; +use crate::prelude::TimestampMillis; + +/// An identity update would disable the last valid authentication key of a security level, +/// which DIP-11 forbids. Signing-side policy, not a consensus error: Platform would accept the +/// update for every level but MASTER. +/// +/// A security level is a minimum: a key of a stronger level stands in for it, except MASTER, +/// which signs identity updates and key-limits updates only. So a CRITICAL key keeps a HIGH key +/// disableable, a MASTER key does not, and a MASTER key needs another MASTER key. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +#[error( + "cannot disable authentication key {key_id}: no other enabled, unexpired authentication key \ + at security level {security_level} or one standing in for it would remain" +)] +pub struct LastAuthenticationKeyError { + /// The key the update would disable. + pub key_id: KeyID, + /// The security level that would be left without a valid authentication key. + pub security_level: SecurityLevel, +} + +/// Whether an identity update that adds `add_public_keys` and disables `disable_public_keys` +/// keeps a valid authentication key at the security level of every authentication key it +/// disables. +/// +/// A valid key is an enabled authentication key that has not expired and whose key type can sign +/// state transitions ([`KeyType::can_sign_state_transitions`](crate::identity::KeyType::can_sign_state_transitions)): `ECDSA_HASH160` keys, which +/// identities created by older Dash Evo Tool versions carry, count; `BIP13_SCRIPT_HASH` and +/// `EDDSA_25519_HASH160` keys do not. The keys the identity keeps count, and so do the keys the +/// same update adds. A key stands in for the disabled one only where it can sign everything the +/// disabled one could: +/// +/// - **Budget.** A key with a `total_budget` never counts: what is left of the budget lives in +/// Drive, and an exhausted key can no longer sign. +/// - **Contract bounds.** An unbounded key is replaced only by an unbounded key; a bound key by +/// an unbounded key or one with identical bounds. Platform refuses a bound key outside its +/// bounds. +/// +/// `now_ms` is the caller's current time in Unix milliseconds. dpp reads no clock, so `None` +/// judges conservatively: a key with any `expires_at` does not count as remaining. With +/// `Some(now)`, a key counts until `now` reaches its `expires_at`. Each of these is stricter +/// than DIP-11 and therefore still compliant. +/// +/// Ids the identity does not hold, and keys of other purposes, are not judged here: Platform +/// refuses unknown ids, and DIP-11 speaks of authentication keys. +pub fn check_keys_can_be_disabled( + identity: &Identity, + add_public_keys: &[IdentityPublicKey], + disable_public_keys: &[KeyID], + now_ms: Option, +) -> Result<(), LastAuthenticationKeyError> { + let remaining_valid_keys = || { + identity + .public_keys() + .values() + .filter(|key| !disable_public_keys.contains(&key.id())) + .chain(add_public_keys) + .filter(|key| { + key.purpose() == Purpose::AUTHENTICATION + && key.key_type().can_sign_state_transitions() + && key.disabled_at().is_none() + && key.total_budget().is_none() + && is_unexpired(key, now_ms) + }) + }; + for key_id in disable_public_keys { + let Some(key) = identity.public_keys().get(key_id) else { + continue; + }; + if key.purpose() != Purpose::AUTHENTICATION { + continue; + } + if !remaining_valid_keys().any(|remaining| stands_in_for(remaining, key)) { + return Err(LastAuthenticationKeyError { + key_id: *key_id, + security_level: key.security_level(), + }); + } + } + Ok(()) +} + +/// Whether `key` is unexpired at `now_ms`; without a time, only a key with no expiry is. +fn is_unexpired(key: &IdentityPublicKey, now_ms: Option) -> bool { + match now_ms { + Some(now_ms) => !key.is_expired_at(now_ms), + None => key.expires_at().is_none(), + } +} + +/// Whether `candidate` can sign whatever `disabled` could, as far as level and contract bounds +/// go: the same level, or a stronger one other than MASTER, which signs identity updates and +/// key-limits updates only; and no bounds, or the same bounds as `disabled`. The single place +/// these comparisons of the rule are made. +fn stands_in_for(candidate: &IdentityPublicKey, disabled: &IdentityPublicKey) -> bool { + let (candidate_level, required_level) = (candidate.security_level(), disabled.security_level()); + let level_suffices = candidate_level == required_level + || (candidate_level != SecurityLevel::MASTER + && candidate_level.stronger_security_than(required_level)); + let bounds_suffice = match candidate.contract_bounds() { + None => true, + bounds => bounds == disabled.contract_bounds(), + }; + level_suffices && bounds_suffice +} + +#[cfg(test)] +pub(crate) mod tests { + use super::*; + use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeySettersV0; + use crate::identity::identity_public_key::contract_bounds::ContractBounds; + use crate::identity::identity_public_key::v0::IdentityPublicKeyV0; + use crate::identity::v0::IdentityV0; + use crate::identity::KeyType; + use platform_value::{BinaryData, Identifier}; + use std::collections::BTreeMap; + + const NOW_MS: TimestampMillis = 1_800_000_000_000; + + pub(crate) fn key( + id: KeyID, + purpose: Purpose, + security_level: SecurityLevel, + key_type: KeyType, + ) -> IdentityPublicKey { + let data = match key_type { + KeyType::ECDSA_HASH160 => vec![id as u8; 20], + _ => vec![id as u8; 33], + }; + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose, + security_level, + contract_bounds: None, + key_type, + read_only: false, + data: BinaryData::new(data), + disabled_at: None, + }) + } + + pub(crate) fn authentication_key( + id: KeyID, + security_level: SecurityLevel, + ) -> IdentityPublicKey { + key( + id, + Purpose::AUTHENTICATION, + security_level, + KeyType::ECDSA_SECP256K1, + ) + } + + pub(crate) fn identity(keys: Vec) -> Identity { + IdentityV0 { + id: Identifier::from([1; 32]), + public_keys: keys + .into_iter() + .map(|key| (key.id(), key)) + .collect::>(), + balance: 0, + revision: 0, + } + .into() + } + + /// MASTER 0, CRITICAL 1, HIGH 2, MEDIUM 3: one authentication key per level. + pub(crate) fn one_key_per_level() -> Identity { + identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + authentication_key(2, SecurityLevel::HIGH), + authentication_key(3, SecurityLevel::MEDIUM), + ]) + } + + #[test] + fn should_refuse_disabling_the_only_critical_key() { + let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[1], Some(NOW_MS)); + assert_eq!( + result, + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + } + + #[test] + fn should_refuse_disabling_the_only_master_key_without_adding_one() { + let result = check_keys_can_be_disabled(&one_key_per_level(), &[], &[0], Some(NOW_MS)); + assert_eq!( + result, + Err(LastAuthenticationKeyError { + key_id: 0, + security_level: SecurityLevel::MASTER, + }) + ); + } + + #[test] + fn should_let_a_stronger_non_master_key_stand_in_for_a_weaker_level() { + // CRITICAL 1 still meets HIGH and MEDIUM requirements. + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[], &[2, 3], Some(NOW_MS)), + Ok(()) + ); + } + + #[test] + fn should_not_let_a_master_key_stand_in_for_critical() { + // MASTER signs identity and key-limits updates only, so it cannot replace the CRITICAL key. + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + ]); + assert!(check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)).is_err()); + } + + #[test] + fn should_not_count_a_key_type_that_cannot_sign_as_remaining() { + for key_type in [KeyType::BIP13_SCRIPT_HASH, KeyType::EDDSA_25519_HASH160] { + let unsignable = key( + 4, + Purpose::AUTHENTICATION, + SecurityLevel::CRITICAL, + key_type, + ); + let refused = Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }); + + let held = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + unsignable.clone(), + ]); + assert_eq!( + check_keys_can_be_disabled(&held, &[], &[1], Some(NOW_MS)), + refused, + "a held {key_type:?} key must not stand in" + ); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[unsignable], &[1], Some(NOW_MS)), + refused, + "an added {key_type:?} key must not stand in" + ); + } + } + + #[test] + fn should_allow_disabling_a_key_replaced_in_the_same_update() { + let replacement = authentication_key(4, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[replacement], &[1], Some(NOW_MS)), + Ok(()) + ); + let master_replacement = authentication_key(4, SecurityLevel::MASTER); + assert_eq!( + check_keys_can_be_disabled( + &one_key_per_level(), + &[master_replacement], + &[0], + Some(NOW_MS) + ), + Ok(()) + ); + } + + #[test] + fn should_allow_disabling_one_of_two_keys_of_a_level_but_not_both() { + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + authentication_key(2, SecurityLevel::CRITICAL), + ]); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)), + Ok(()) + ); + assert!(check_keys_can_be_disabled(&identity, &[], &[1, 2], Some(NOW_MS)).is_err()); + } + + #[test] + fn should_not_count_disabled_or_expired_keys_as_remaining() { + let mut disabled = authentication_key(2, SecurityLevel::CRITICAL); + disabled.set_disabled_at(NOW_MS - 1); + let expired = + authentication_key(3, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS)); + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + authentication_key(1, SecurityLevel::CRITICAL), + disabled, + expired, + ]); + assert!(check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)).is_err()); + + let expired_replacement = + authentication_key(4, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS)); + assert!( + check_keys_can_be_disabled(&identity, &[expired_replacement], &[1], Some(NOW_MS)) + .is_err() + ); + + let unexpired = + authentication_key(5, SecurityLevel::CRITICAL).with_limits(None, Some(NOW_MS + 1)); + assert_eq!( + check_keys_can_be_disabled(&identity, &[unexpired], &[1], Some(NOW_MS)), + Ok(()) + ); + } + + #[test] + fn should_not_count_a_key_with_any_expiry_without_a_clock() { + // Without a time the rule cannot tell an expiring key from an expired one, so it counts + // neither: stricter than DIP-11, never looser. + let far_future = + authentication_key(4, SecurityLevel::CRITICAL).with_limits(None, Some(u64::MAX)); + let mut identity = one_key_per_level(); + identity.add_public_key(far_future.clone()); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], None), + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + assert!( + check_keys_can_be_disabled(&one_key_per_level(), &[far_future], &[1], None).is_err() + ); + // A key without limits counts the same either way. + let replacement = authentication_key(5, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[replacement], &[1], None), + Ok(()) + ); + } + + #[test] + fn should_not_count_a_key_with_a_budget_as_remaining() { + // What is left of a budget lives in Drive: an exhausted budgeted key can no longer + // sign, and the rule cannot tell it from a fresh one. + let budgeted = authentication_key(4, SecurityLevel::CRITICAL).with_limits(Some(1), None); + let mut identity = one_key_per_level(); + identity.add_public_key(budgeted.clone()); + for now_ms in [Some(NOW_MS), None] { + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], now_ms), + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + assert!(check_keys_can_be_disabled( + &one_key_per_level(), + std::slice::from_ref(&budgeted), + &[1], + now_ms + ) + .is_err()); + } + // An unbudgeted replacement added in the same update still counts. + let replacement = authentication_key(5, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&identity, &[replacement], &[1], Some(NOW_MS)), + Ok(()) + ); + } + + fn bound_to(key: IdentityPublicKey, contract: u8) -> IdentityPublicKey { + let IdentityPublicKey::V0(mut v0) = key else { + panic!("test keys are V0"); + }; + v0.contract_bounds = Some(ContractBounds::SingleContract { + id: Identifier::from([contract; 32]), + }); + v0.into() + } + + #[test] + fn should_not_let_a_contract_bound_key_stand_in_for_an_unbounded_one() { + // A bound key signs only inside its contract, so it cannot do the unbounded key's work. + let bound = bound_to(authentication_key(4, SecurityLevel::CRITICAL), 7); + let mut identity = one_key_per_level(); + identity.add_public_key(bound.clone()); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], Some(NOW_MS)), + Err(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }) + ); + assert!( + check_keys_can_be_disabled(&one_key_per_level(), &[bound], &[1], Some(NOW_MS)).is_err() + ); + } + + #[test] + fn should_let_an_unbounded_or_identically_bound_key_stand_in_for_a_bound_one() { + let bound = bound_to(authentication_key(4, SecurityLevel::CRITICAL), 7); + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + bound.clone(), + ]); + let unbounded = authentication_key(5, SecurityLevel::CRITICAL); + assert_eq!( + check_keys_can_be_disabled(&identity, &[unbounded], &[4], Some(NOW_MS)), + Ok(()) + ); + let same_bounds = bound_to(authentication_key(5, SecurityLevel::CRITICAL), 7); + assert_eq!( + check_keys_can_be_disabled(&identity, &[same_bounds], &[4], Some(NOW_MS)), + Ok(()) + ); + let other_bounds = bound_to(authentication_key(5, SecurityLevel::CRITICAL), 8); + assert_eq!( + check_keys_can_be_disabled(&identity, &[other_bounds], &[4], Some(NOW_MS)), + Err(LastAuthenticationKeyError { + key_id: 4, + security_level: SecurityLevel::CRITICAL, + }) + ); + } + + #[test] + fn should_not_judge_keys_of_other_purposes_or_unknown_ids() { + let identity = identity(vec![ + authentication_key(0, SecurityLevel::MASTER), + key( + 1, + Purpose::TRANSFER, + SecurityLevel::CRITICAL, + KeyType::ECDSA_SECP256K1, + ), + key( + 2, + Purpose::ENCRYPTION, + SecurityLevel::MEDIUM, + KeyType::ECDSA_SECP256K1, + ), + ]); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1, 2, 9], Some(NOW_MS)), + Ok(()) + ); + // A non-authentication key does not keep an authentication level valid. + let identity_with_critical = { + let mut identity = identity; + identity.add_public_key(authentication_key(3, SecurityLevel::CRITICAL)); + identity + }; + assert!( + check_keys_can_be_disabled(&identity_with_critical, &[], &[3], Some(NOW_MS)).is_err() + ); + } + + #[test] + fn should_count_hash160_keys_as_valid() { + // Identities created by older Dash Evo Tool versions carry ECDSA_HASH160 keys. + let hash160 = key( + 4, + Purpose::AUTHENTICATION, + SecurityLevel::CRITICAL, + KeyType::ECDSA_HASH160, + ); + let mut identity = one_key_per_level(); + identity.add_public_key(hash160.clone()); + assert_eq!( + check_keys_can_be_disabled(&identity, &[], &[1], None), + Ok(()) + ); + assert_eq!( + check_keys_can_be_disabled(&one_key_per_level(), &[hash160], &[1], None), + Ok(()) + ); + } +} diff --git a/packages/rs-dpp/src/identity/mod.rs b/packages/rs-dpp/src/identity/mod.rs index 14ddeb8fd2c..77fc41a00ae 100644 --- a/packages/rs-dpp/src/identity/mod.rs +++ b/packages/rs-dpp/src/identity/mod.rs @@ -26,6 +26,7 @@ mod identity_facade; #[cfg(feature = "factories")] pub mod identity_factory; pub mod identity_nonce; +pub mod key_disable_rule; pub mod methods; #[cfg(feature = "random-identities")] pub mod random; diff --git a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs index 9c0d00c3f80..e06373d6c75 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/methods/v0/mod.rs @@ -15,6 +15,14 @@ use crate::{identity::KeyID, ProtocolError}; use platform_version::version::PlatformVersion; pub trait IdentityUpdateTransitionMethodsV0 { + /// Builds and signs an identity update that adds `add_public_keys` and disables + /// `disable_public_keys`, signed by the MASTER key `master_public_key_id`. + /// + /// Refuses, before signing anything, an update that would disable the last valid + /// authentication key of a security level, with [`ProtocolError::LastAuthenticationKey`] + /// (DIP-11 signing-side policy; see + /// [`check_keys_can_be_disabled`](crate::identity::key_disable_rule::check_keys_can_be_disabled)). + /// The builder has no clock, so a key with any expiry does not count as remaining. #[cfg(feature = "state-transition-signing")] #[allow(clippy::too_many_arguments)] async fn try_from_identity_with_signer>( diff --git a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs index a975040c95f..1296c71242f 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs @@ -19,6 +19,8 @@ use crate::identity::{Identity, IdentityPublicKey}; use crate::identity::accessors::IdentityGettersV0; #[cfg(feature = "state-transition-signing")] use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; +#[cfg(feature = "state-transition-signing")] +use crate::identity::key_disable_rule::check_keys_can_be_disabled; use crate::prelude::IdentityNonce; #[cfg(feature = "state-transition-signing")] use crate::prelude::UserFeeIncrease; @@ -52,6 +54,13 @@ impl IdentityUpdateTransitionMethodsV0 for IdentityUpdateTransitionV0 { _platform_version: &PlatformVersion, _version: Option, ) -> Result { + // DIP-11 signing-side policy, checked before anything is signed. This is an in-place edit + // of a generation every protocol version selects, and it is inert for consensus at all of + // them: only clients build transitions here (no Drive or drive-abci code path calls this + // builder), and a request the rule accepts yields the same transition as before. dpp + // reads no clock, so a key with any expiry does not count as remaining (`None`). + check_keys_can_be_disabled(identity, &add_public_keys, &disable_public_keys, None)?; + let add_public_keys_in_creation = add_public_keys .iter() .map(|public_key| public_key.into()) @@ -159,3 +168,216 @@ impl IdentityUpdateTransitionAccessorsV0 for IdentityUpdateTransitionV0 { &self.disable_public_keys } } + +#[cfg(all(test, feature = "state-transition-signing"))] +mod tests { + use crate::address_funds::AddressWitness; + use crate::identity::accessors::IdentityGettersV0; + use crate::identity::key_disable_rule::tests::{authentication_key, key, one_key_per_level}; + use crate::identity::key_disable_rule::LastAuthenticationKeyError; + use crate::identity::signer::Signer; + use crate::identity::{Identity, IdentityPublicKey, KeyID, KeyType, Purpose, SecurityLevel}; + use crate::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; + use crate::state_transition::identity_update_transition::v0::IdentityUpdateTransitionV0; + use crate::state_transition::identity_update_transition::IdentityUpdateTransition; + use crate::state_transition::public_key_in_creation::accessors::IdentityPublicKeyInCreationV0Setters; + use crate::state_transition::public_key_in_creation::IdentityPublicKeyInCreation; + use crate::state_transition::StateTransition; + use crate::ProtocolError; + use assert_matches::assert_matches; + use async_trait::async_trait; + use platform_value::BinaryData; + use platform_version::version::{PlatformVersion, PLATFORM_VERSIONS}; + use std::sync::atomic::{AtomicUsize, Ordering}; + + const SIGNATURE: [u8; 65] = [7; 65]; + + /// Returns [`SIGNATURE`] for every request and counts them. + #[derive(Debug, Default)] + struct CountingSigner { + signatures: AtomicUsize, + } + + impl CountingSigner { + fn signatures(&self) -> usize { + self.signatures.load(Ordering::SeqCst) + } + } + + #[async_trait] + impl Signer for CountingSigner { + async fn sign( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + self.signatures.fetch_add(1, Ordering::SeqCst); + Ok(SIGNATURE.to_vec().into()) + } + + async fn sign_create_witness( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(ProtocolError::Generic( + "identity updates create no address witnesses".to_string(), + )) + } + + fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { + true + } + } + + async fn build( + identity: &Identity, + add_public_keys: Vec, + disable_public_keys: Vec, + signer: &CountingSigner, + platform_version: &PlatformVersion, + ) -> Result { + IdentityUpdateTransition::try_from_identity_with_signer( + identity, + &0, + add_public_keys, + disable_public_keys, + 5, + 0, + signer, + platform_version, + None, + ) + .await + } + + #[tokio::test] + async fn should_refuse_disabling_the_last_critical_key_before_signing_anything() { + let signer = CountingSigner::default(); + // The added key is a unique key type, so a build that got past the rule would sign it. + let added = authentication_key(4, SecurityLevel::MEDIUM); + let result = build( + &one_key_per_level(), + vec![added], + vec![1], + &signer, + PlatformVersion::latest(), + ) + .await; + assert_matches!( + result, + Err(ProtocolError::LastAuthenticationKey( + LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + } + )) + ); + assert_eq!(signer.signatures(), 0, "nothing may be signed"); + } + + #[tokio::test] + async fn should_not_count_an_expiring_key_as_remaining_in_the_builder() { + // The builder has no clock: even an expiry far in the future does not count. + let mut identity = one_key_per_level(); + identity.add_public_key( + authentication_key(4, SecurityLevel::CRITICAL).with_limits(None, Some(u64::MAX)), + ); + let signer = CountingSigner::default(); + let result = build( + &identity, + vec![], + vec![1], + &signer, + PlatformVersion::latest(), + ) + .await; + assert_matches!( + result, + Err(ProtocolError::LastAuthenticationKey( + LastAuthenticationKeyError { key_id: 1, .. } + )) + ); + assert_eq!(signer.signatures(), 0); + + // The same key without the expiry keeps key 1 disableable. + identity.add_public_key(authentication_key(4, SecurityLevel::CRITICAL)); + assert!(build( + &identity, + vec![], + vec![1], + &signer, + PlatformVersion::latest() + ) + .await + .is_ok()); + } + + #[tokio::test] + async fn should_count_a_hash160_key_as_remaining_in_the_builder() { + let mut identity = one_key_per_level(); + identity.add_public_key(key( + 4, + Purpose::AUTHENTICATION, + SecurityLevel::CRITICAL, + KeyType::ECDSA_HASH160, + )); + let signer = CountingSigner::default(); + let transition = build( + &identity, + vec![], + vec![1], + &signer, + PlatformVersion::latest(), + ) + .await + .expect("an ECDSA_HASH160 CRITICAL key keeps the CRITICAL level valid"); + assert_matches!(transition, StateTransition::IdentityUpdate(_)); + } + + /// The in-place edit of this shipped generation is inert for a request the rule accepts: + /// at every protocol version, the last shipped one included, the builder produces exactly + /// the transition it produced before the rule existed. + #[tokio::test] + async fn should_build_an_unchanged_transition_for_a_compliant_update_at_every_version() { + let identity = one_key_per_level(); + let added = authentication_key(4, SecurityLevel::HIGH); + let mut added_in_creation: IdentityPublicKeyInCreation = (&added).into(); + added_in_creation.set_signature(SIGNATURE.to_vec().into()); + let expected: StateTransition = IdentityUpdateTransitionV0 { + identity_id: identity.id(), + revision: identity.revision(), + nonce: 5, + add_public_keys: vec![added_in_creation], + // CRITICAL 1 stands in for HIGH 2. + disable_public_keys: vec![2], + user_fee_increase: 0, + signature_public_key_id: 0, + signature: SIGNATURE.to_vec().into(), + } + .into(); + + for platform_version in PLATFORM_VERSIONS { + let signer = CountingSigner::default(); + let transition = build( + &identity, + vec![added.clone()], + vec![2], + &signer, + platform_version, + ) + .await + .unwrap_or_else(|e| { + panic!( + "protocol version {}: {e}", + platform_version.protocol_version + ) + }); + assert_eq!( + transition, expected, + "protocol version {}", + platform_version.protocol_version + ); + } + } +} diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index d14d197ddb2..47d7010b508 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -1094,6 +1094,13 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::InvalidParameter(..) => { PlatformWalletFFIResultCode::ErrorInvalidParameter } + // An identity update that would disable the last valid + // authentication key of a security level (DIP-11), refused by the + // dpp builder before anything is signed: the caller picked keys it + // may not disable, the same class as the arm above. + PlatformWalletError::Sdk(dash_sdk::Error::Protocol( + dpp::ProtocolError::LastAuthenticationKey(_), + )) => PlatformWalletFFIResultCode::ErrorInvalidParameter, // A second producer of code 31 (the arm above is the first), // reached without any marker inspection at this layer: message // signing found no signable account for the address, or the signer @@ -2044,6 +2051,30 @@ mod tests { ); } + /// The DIP-11 last-key refusal of an identity update is a caller-argument + /// error, not an internal failure: hosts branch on it to say which key + /// cannot be disabled instead of showing ErrorUnknown. + #[test] + fn last_authentication_key_refusal_maps_to_invalid_parameter() { + use dpp::identity::key_disable_rule::LastAuthenticationKeyError; + use dpp::identity::SecurityLevel; + let err = PlatformWalletError::Sdk(dash_sdk::Error::Protocol( + dpp::ProtocolError::LastAuthenticationKey(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }), + )); + let result: PlatformWalletFFIResult = err.into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorInvalidParameter + ); + let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_string_lossy() + .into_owned(); + assert!(msg.contains("cannot disable authentication key 1"), "{msg}"); + } + /// The four DPNS-marketplace trade rejections each map to their own /// dedicated code rather than flattening to `ErrorUnknown`, and the /// not-found case rides the existing `NotFound`. Hosts branch on these diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 0dbca35e9b1..8eac3d17cfb 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -684,6 +684,29 @@ pub async fn test_platform_wallet_manager() -> ( pub async fn test_platform_wallet_manager_with

( persister: Arc

, ) -> (Arc>, WalletId) +where + P: crate::changeset::PlatformWalletPersistence + 'static, +{ + let sdk = dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"); + platform_wallet_manager_over(sdk, persister).await +} + +/// [`test_platform_wallet_manager`] over a caller-built mock SDK, for tests that +/// set mock expectations (an identity nonce, say) before the manager takes it. +#[cfg(test)] +pub(crate) async fn test_platform_wallet_manager_with_sdk( + sdk: dash_sdk::Sdk, +) -> ( + Arc>, + WalletId, +) { + platform_wallet_manager_over(sdk, Arc::new(NoopTestPersister)).await +} + +async fn platform_wallet_manager_over

( + sdk: dash_sdk::Sdk, + persister: Arc

, +) -> (Arc>, WalletId) where P: crate::changeset::PlatformWalletPersistence + 'static, { @@ -694,7 +717,7 @@ where const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ abandon abandon abandon abandon abandon about"; - let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let sdk = Arc::new(sdk); let event_handler: Arc = Arc::new(NoopTestEventHandler); let manager = Arc::new(crate::PlatformWalletManager::new( diff --git a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs index 8b46ce08955..0e4ac9507ff 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs @@ -1,14 +1,33 @@ -//! Pre-send validation for DashPay contact requests. +//! Key validation for DashPay contact requests. //! -//! Validates that the sender and recipient identities have the correct key -//! types and purposes before a contact request is submitted to the platform. +//! [`validate_contact_request`] judges a request already on chain: liberal, +//! because the document is immutable. What DIP-15 asks of a request we are +//! about to mint on top of it (both keys valid, i.e. not disabled) is +//! enforced once, by `dash_sdk::Sdk::create_contact_request`. -use dash_sdk::platform::dashpay::{ - recipient_key_purpose_is_acceptable_on_receive, sender_key_purpose_is_acceptable_on_receive, -}; use dpp::identity::accessors::IdentityGettersV0; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dpp::identity::{Identity, KeyType}; +use std::fmt; + +/// The side of a contact request a key index refers to, as named in +/// validation messages. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ContactRequestKeySide { + /// The identity that sent the request (`senderKeyIndex`). + Sender, + /// The identity the request is addressed to (`recipientKeyIndex`). + Recipient, +} + +impl fmt::Display for ContactRequestKeySide { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(match self { + Self::Sender => "Sender", + Self::Recipient => "Recipient", + }) + } +} /// Result of validating a contact request before it is sent. #[derive(Debug, Clone)] @@ -19,28 +38,9 @@ pub struct ContactRequestValidation { pub errors: Vec, /// Non-fatal warnings the caller may want to surface. pub warnings: Vec, - /// `true` when a key-PURPOSE mismatch was seen (e.g. a legacy 2024 doc - /// referencing an AUTHENTICATION key). - /// - /// This classification is load-bearing for the sync sweep / accept paths: - /// a purpose mismatch must NOT mark the payment channel - /// **permanently** broken — on-chain history demonstrably contains - /// nonconforming-but-honest documents, and our acceptance policy (not the - /// immutable request) is what might change. A purpose-only failure is a - /// non-permanent skip (log + retry next sweep); a key-TYPE / missing-key / - /// disabled-key failure stays permanent. - /// - /// **Read [`is_purpose_only`](Self::is_purpose_only), not this field, to - /// decide skip-vs-break.** This flag alone is `true` even when a hard - /// (non-purpose) error is *also* present; downgrading to a skip in that - /// case would mask a genuinely permanent failure (a disabled / wrong-type - /// key) into a retry-forever loop. - pub purpose_mismatch: bool, - /// `true` when at least one *non-purpose* hard error was recorded (missing - /// key, wrong key type, disabled key). Distinguishes "purpose mismatch is - /// the sole cause" (downgradable to a skip) from "purpose mismatch plus a - /// genuinely permanent fault" (must stay permanent). See - /// [`is_purpose_only`](Self::is_purpose_only). + /// `true` when at least one permanent error was recorded (a wrong key + /// type, or a disabled key on the send path). See + /// [`is_permanent`](Self::is_permanent). pub hard_error: bool, } @@ -50,7 +50,6 @@ impl Default for ContactRequestValidation { is_valid: true, errors: Vec::new(), warnings: Vec::new(), - purpose_mismatch: false, hard_error: false, } } @@ -62,9 +61,8 @@ impl ContactRequestValidation { Self::default() } - /// Add a hard (non-purpose) error: sets `is_valid = false` AND flags - /// `hard_error` so a co-occurring purpose mismatch can't downgrade this - /// genuinely-permanent fault to a skip. + /// Add a hard (permanent) error: sets `is_valid = false` AND flags + /// `hard_error`. pub fn add_error(&mut self, error: String) { self.errors.push(error); self.is_valid = false; @@ -79,29 +77,11 @@ impl ContactRequestValidation { self.is_valid = false; } - /// Add a key-PURPOSE error: sets `is_valid = false` AND flags - /// `purpose_mismatch` so callers can downgrade a *purpose-only* failure - /// to a non-permanent skip rather than a permanent broken-channel mark. - /// Does NOT set `hard_error`. - pub fn add_purpose_error(&mut self, error: String) { - self.errors.push(error); - self.is_valid = false; - self.purpose_mismatch = true; - } - /// Add a non-fatal warning. pub fn add_warning(&mut self, warning: String) { self.warnings.push(warning); } - /// Whether the *sole* cause of invalidity is a key-purpose mismatch — - /// the only case that may be downgraded to a non-permanent skip. - /// A purpose mismatch that co-occurs with a hard error (disabled / - /// wrong-type key) is NOT purpose-only and must stay permanent. - pub fn is_purpose_only(&self) -> bool { - self.purpose_mismatch && !self.hard_error - } - /// Whether this failure can never resolve on its own — the only kind that /// may permanently break a contact's payment channel. /// @@ -115,9 +95,11 @@ impl ContactRequestValidation { /// the user cannot repair — only a fresh request from the CONTACT clears /// the flag. /// - /// So an absent key is retryable, alongside a purpose mismatch. What stays + /// So an absent key is retryable. What stays /// permanent is what immutable facts make impossible: a key whose *type* - /// cannot do ECDH, and a key we have deliberately disabled. + /// cannot do ECDH. A disabled key is not among them: DIP-11 keeps disabled + /// keys usable for verification, and the request named the key while it + /// was valid, so the receive validators only warn about it. pub fn is_permanent(&self) -> bool { self.hard_error } @@ -129,9 +111,6 @@ impl ContactRequestValidation { if !other.is_valid { self.is_valid = false; } - if other.purpose_mismatch { - self.purpose_mismatch = true; - } if other.hard_error { self.hard_error = true; } @@ -145,8 +124,11 @@ impl ContactRequestValidation { /// validator is *liberal on receive* by necessity. Rejecting a document is not /// a retry, it is a permanent sentence on a contact relationship the user /// cannot renegotiate. What it keeps strict is what actually carries weight: -/// the ECDSA key-*type* gate (ECDH needs the full secp256k1 key) and the -/// disabled-key check. +/// the ECDSA key-*type* gate (ECDH needs the full secp256k1 key). A key +/// disabled since the request was sent is only a warning: DIP-11 keeps +/// disabled keys usable for verification, and DIP-15's "valid key" rule +/// binds the sender when minting, which +/// `dash_sdk::Sdk::create_contact_request` enforces for our own requests. /// /// Three live cohorts are known: /// - **Newest** — bound `ENCRYPTION(sender)` / `DECRYPTION(recipient)`, our @@ -167,27 +149,18 @@ impl ContactRequestValidation { /// **Sender key:** /// - Key at `sender_key_index` exists on the sender identity. /// - Key type is `ECDSA_SECP256K1` (required for ECDH). -/// - Key purpose is `ENCRYPTION` or `AUTHENTICATION` (bound or unbound) — -/// anything else is flagged as a `purpose_mismatch` (non-permanent). -/// - Key is not disabled. +/// - A disabled key is a warning. /// /// **Recipient key (our key):** /// - Key at `recipient_key_index` exists on the recipient identity. /// - Key type is compatible (`ECDSA_SECP256K1` or `ECDSA_HASH160`). -/// - Key purpose is `DECRYPTION`, `ENCRYPTION`, `AUTHENTICATION` or -/// `TRANSFER` — the node-operational purposes (`SYSTEM`, `VOTING`, -/// `OWNER`) are flagged as a `purpose_mismatch`. -/// - Key is not disabled. -/// -/// The accepted sets live in `dash_sdk::platform::dashpay` as -/// `*_key_purpose_is_acceptable_on_receive`, deliberately separate from the -/// stricter `recipient_key_purpose_is_valid` that governs the requests we -/// *mint*: accepting history is not the same decision as choosing a key for a -/// new document, and only the latter can still practice key separation. +/// - A disabled key is a warning. /// -/// A failure whose *only* cause is a purpose mismatch sets -/// [`ContactRequestValidation::purpose_mismatch`], signalling callers to skip -/// (and retry) rather than permanently break the channel. +/// Key purpose is not checked: DIP-15 constrains these keys by type only, and +/// ECDH is defined over the secp256k1 keypair whatever its purpose. Purpose +/// policy belongs to the requests we *mint* +/// (`dash_sdk::platform::dashpay::recipient_key_purpose_is_valid` and the +/// own-key selector), where key separation can still be practiced. pub fn validate_contact_request( sender_identity: &Identity, sender_key_index: u32, @@ -220,37 +193,28 @@ pub(crate) fn validate_sender_key( // Must be ECDSA_SECP256K1 for ECDH. if key.key_type() != KeyType::ECDSA_SECP256K1 { validation.add_error(format!( - "Sender key {} has type {:?}, but ECDSA_SECP256K1 is required for ECDH", + "{} key {} has type {:?}, but ECDSA_SECP256K1 is required for ECDH", + ContactRequestKeySide::Sender, sender_key_index, key.key_type(), )); } - // ENCRYPTION is the modern convention; legacy dashj documents - // reference an AUTHENTICATION key. Both are accepted on receive — - // the document is immutable, so rejecting it is a permanent - // sentence on a contact the user cannot appeal. Anything else is - // still a non-permanent purpose mismatch (skip and retry). - if !sender_key_purpose_is_acceptable_on_receive(key.purpose()) { - validation.add_purpose_error(format!( - "Sender key {} has purpose {:?}, but ENCRYPTION or AUTHENTICATION is \ - required for contact requests", - sender_key_index, - key.purpose(), - )); - } - - // Must not be disabled. + // Disabled since the request was sent: still usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { - validation.add_error(format!( - "Sender key {} is disabled (at timestamp {})", - sender_key_index, disabled_at, + validation.add_warning(format!( + "{} key {} is disabled (at timestamp {}); accepted, as the request \ + named it before", + ContactRequestKeySide::Sender, + sender_key_index, + disabled_at, )); } } None => { validation.add_absent_key_error(format!( - "Sender key index {} not found on identity {}", + "{} key index {} not found on identity {}", + ContactRequestKeySide::Sender, sender_key_index, sender_identity.id(), )); @@ -265,25 +229,25 @@ pub(crate) fn validate_sender_key( /// /// Split out because the deferred-crypto drain would otherwise pay a Platform /// round trip (`Identity::fetch` of the contact) before it could discover that -/// the request is unusable for a reason it could have known locally. A -/// purpose-rejected entry stays queued by design — the policy, not the -/// immutable document, is what might change — so that fetch was repeating on -/// every sweep, forever. Mainnet logs from one wallet show 27 contacts and 396 -/// such fetch-then-reject cycles in a single session. Running this half first +/// the request is unusable for a reason it could have known locally. An entry +/// naming a key our identity does not have yet stays queued by design — the +/// identity can gain it — so that fetch would repeat on every sweep. (Mainnet +/// logs from one wallet once showed 27 contacts and 396 such +/// fetch-then-reject cycles in a single session.) Running this half first /// costs nothing and removes the network entirely from that loop. /// /// # What this changes for a MIXED failure /// /// Deciding on this half alone is a real policy change, not just a reordering. -/// When our key is purpose-rejected AND the sender's key carries a hard fault -/// (missing / disabled / wrong type), the composed [`validate_contact_request`] -/// would merge both, see `hard_error`, and mark the channel permanently -/// broken. Stopping here classifies it purpose-only and leaves it queued. +/// When our key is absent AND the sender's key carries a hard fault (a key +/// type that cannot do ECDH), the composed [`validate_contact_request`] would +/// merge both, see `hard_error`, and mark the channel permanently broken. +/// Stopping here classifies it retryable and leaves it queued. /// /// That is the intended outcome. The `hard_error` precedence exists to stop a /// genuinely permanent fault from becoming a retry-forever loop — but the /// forever-loop it guards against was expensive precisely because each retry -/// fetched. With the fetch gone, a purpose-rejected entry costs a map lookup +/// fetched. With the fetch gone, a retryable entry costs a map lookup /// per sweep, while marking the channel broken is unappealable by the user: /// only a fresh request from the CONTACT clears it. Deferring the broken mark /// until the fault is one we can see locally trades a cheap retry for an @@ -304,51 +268,38 @@ pub(crate) fn validate_recipient_key( } KeyType::ECDSA_HASH160 => { validation.add_warning(format!( - "Recipient key {} is ECDSA_HASH160; full public key is needed for ECDH — \ + "{} key {} is ECDSA_HASH160; full public key is needed for ECDH — \ ensure the full key is available", + ContactRequestKeySide::Recipient, recipient_key_index, )); } other => { validation.add_error(format!( - "Recipient key {} has type {:?}, which is not compatible with ECDH", - recipient_key_index, other, + "{} key {} has type {:?}, which is not compatible with ECDH", + ContactRequestKeySide::Recipient, + recipient_key_index, + other, )); } } - // Four honest cohorts reach this point: the newest references our - // DECRYPTION key, the mobile population our ENCRYPTION key, and - // the legacy Android/dashj population our AUTHENTICATION (key ids - // 0-2) or TRANSFER (key id 3) key. Purpose is not a security - // boundary here — ECDH is defined over the secp256k1 keypair and - // DIP-9 indexes the identity-key tree by type and id, never by - // purpose — so the type and disabled-key gates around this block - // are what actually protect the derivation. The node-operational - // purposes (SYSTEM/VOTING/OWNER) stay out — nothing on chain - // references them for DashPay — and remain a non-permanent purpose - // mismatch: skip and retry, never break the channel, so a later - // evidence-driven widening can still pick those contacts up. - if !recipient_key_purpose_is_acceptable_on_receive(key.purpose()) { - validation.add_purpose_error(format!( - "Recipient key {} has purpose {:?}, which is not accepted for contact \ - requests", - recipient_key_index, - key.purpose(), - )); - } - - // Must not be disabled. + // Disabled since the request was sent: we still derive its + // scalar, so the request stays usable (DIP-11). if let Some(disabled_at) = key.disabled_at() { - validation.add_error(format!( - "Recipient key {} is disabled (at timestamp {})", - recipient_key_index, disabled_at, + validation.add_warning(format!( + "{} key {} is disabled (at timestamp {}); accepted, as the request \ + named it before", + ContactRequestKeySide::Recipient, + recipient_key_index, + disabled_at, )); } } None => { validation.add_absent_key_error(format!( - "Recipient key index {} not found on identity {}", + "{} key index {} not found on identity {}", + ContactRequestKeySide::Recipient, recipient_key_index, recipient_identity.id(), )); @@ -479,24 +430,6 @@ mod tests { assert!(result.errors.iter().any(|e| e.contains("ECDSA_SECP256K1"))); } - #[test] - fn test_sender_wrong_purpose() { - // VOTING, not AUTHENTICATION: the legacy dashj cohort pairs an - // AUTHENTICATION sender key with an AUTHENTICATION recipient key and - // is accepted on receive, so AUTHENTICATION does not exercise - // the sender-side rejection this test is about. - let sender = make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, Purpose::VOTING)]); - let recipient = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::DECRYPTION, - )]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!(result.errors.iter().any(|e| e.contains("ENCRYPTION"))); - } - #[test] fn test_recipient_key_missing() { let sender = make_identity(vec![make_key( @@ -531,11 +464,19 @@ mod tests { .any(|e| e.contains("not compatible with ECDH"))); } - #[test] - fn test_disabled_sender_key() { - let mut key = make_key(0, KeyType::ECDSA_SECP256K1, Purpose::ENCRYPTION); + fn disabled_key(id: u32, purpose: Purpose) -> IdentityPublicKey { + let mut key = make_key(id, KeyType::ECDSA_SECP256K1, purpose); key.set_disabled_at(12345); - let sender = make_identity(vec![key]); + key + } + + /// DIP-11: disabled keys "can be used for signature verification". A + /// received request is immutable history that named a key valid when it + /// was sent; the sender disabling that key since must not lock us out of + /// the channel. It is a warning, never a failure. + #[test] + fn should_accept_a_received_request_naming_a_since_disabled_sender_key() { + let sender = make_identity(vec![disabled_key(0, Purpose::ENCRYPTION)]); let recipient = make_identity(vec![make_key( 0, KeyType::ECDSA_SECP256K1, @@ -543,8 +484,36 @@ mod tests { )]); let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!(result.errors.iter().any(|e| e.contains("disabled"))); + assert!(result.is_valid, "errors: {:?}", result.errors); + assert!(!result.is_permanent()); + assert!(result.warnings.iter().any(|w| w.contains("disabled"))); + + let sender_half = validate_sender_key(&sender, 0); + assert!(sender_half.is_valid, "errors: {:?}", sender_half.errors); + } + + /// Our own key disabled after the request arrived: we still derive its + /// scalar, so the request stays usable. + #[test] + fn should_accept_a_received_request_naming_our_since_disabled_recipient_key() { + let sender = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + Purpose::ENCRYPTION, + )]); + let recipient = make_identity(vec![disabled_key(0, Purpose::DECRYPTION)]); + + let result = validate_contact_request(&sender, 0, &recipient, 0); + assert!(result.is_valid, "errors: {:?}", result.errors); + assert!(result.warnings.iter().any(|w| w.contains("disabled"))); + + let recipient_half = validate_recipient_key(&recipient, 0); + assert!( + recipient_half.is_valid, + "errors: {:?}", + recipient_half.errors + ); + assert!(!recipient_half.is_permanent()); } #[test] @@ -569,19 +538,14 @@ mod tests { // newest cohort uses bound ENCRYPTION(sender)/DECRYPTION(recipient). // Consensus enforces neither purpose nor boundedness, and mainnet adds a // third, legacy Android/dashj cohort referencing AUTHENTICATION/TRANSFER - // keys. So the validator accepts ENCRYPTION-or-AUTHENTICATION for the - // sender and everything but the node-operational purposes for the - // recipient, while keeping the - // ECDSA type gate and the disabled-key check — those are the checks that - // actually protect the ECDH. + // keys. DIP-15 constrains these keys by type only, so the validator + // accepts every purpose on receive and keeps the ECDSA type gate — the + // check that actually protects the ECDH. // ----------------------------------------------------------------------- /// Mobile-cohort shape: sender references an ENCRYPTION key, recipient /// (our key) is ALSO an ENCRYPTION key (mobile identities have no - /// DECRYPTION key). This must pass. The companion AUTHENTICATION test - /// below pins the recipient-purpose gate: without that gate an - /// AUTHENTICATION recipient key is silently accepted (it "passes" for - /// the wrong reason). + /// DECRYPTION key). This must pass. #[test] fn mobile_cohort_recipient_encryption_key_is_accepted() { let sender = make_identity(vec![make_key( @@ -601,7 +565,6 @@ mod tests { "mobile-cohort ENC/ENC request must validate, errors: {:?}", result.errors ); - assert!(!result.purpose_mismatch); } /// Legacy Android/dashj shape: the inbound request references OUR @@ -631,7 +594,6 @@ mod tests { document, errors: {:?}", result.errors ); - assert!(!result.purpose_mismatch); } } @@ -652,7 +614,6 @@ mod tests { let result = validate_contact_request(&sender, 1, &recipient, 1); assert!(result.is_valid, "errors: {:?}", result.errors); - assert!(!result.purpose_mismatch); } /// A key id the identity does not have **yet** must not be permanent. @@ -700,34 +661,77 @@ mod tests { ); } - /// The node-operational purposes are the ones still refused for a - /// recipient key — and they must stay a non-permanent purpose mismatch, so - /// a future evidence-driven widening can still pick those contacts up - /// instead of finding them broken. + const ALL_PURPOSES: [Purpose; 7] = [ + Purpose::AUTHENTICATION, + Purpose::ENCRYPTION, + Purpose::DECRYPTION, + Purpose::TRANSFER, + Purpose::SYSTEM, + Purpose::VOTING, + Purpose::OWNER, + ]; + + /// DIP-15 constrains the keys of a contact request by type only (ECDH + /// needs secp256k1), never by purpose, so a received request must not be + /// refused for the purpose of either key. #[test] - fn recipient_node_operational_key_is_rejected_as_purpose_mismatch() { - let sender = make_identity(vec![make_key( + fn should_accept_a_received_request_whatever_the_key_purposes() { + for sender_purpose in ALL_PURPOSES { + for recipient_purpose in ALL_PURPOSES { + let sender = + make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, sender_purpose)]); + let recipient = make_identity(vec![make_key( + 0, + KeyType::ECDSA_SECP256K1, + recipient_purpose, + )]); + + let result = validate_contact_request(&sender, 0, &recipient, 0); + assert!( + result.is_valid, + "{sender_purpose:?} sender / {recipient_purpose:?} recipient: {:?}", + result.errors + ); + } + } + } + + /// The type gate is the genuine one and stays: a key that cannot do + /// secp256k1 ECDH fails permanently, whatever its purpose. + #[test] + fn should_still_refuse_a_key_type_that_cannot_do_ecdh() { + let enabled = make_identity(vec![make_key( 0, KeyType::ECDSA_SECP256K1, Purpose::ENCRYPTION, )]); - for purpose in [Purpose::SYSTEM, Purpose::VOTING, Purpose::OWNER] { - let recipient = make_identity(vec![make_key(0, KeyType::ECDSA_SECP256K1, purpose)]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!( - !result.is_valid, - "a {purpose:?} recipient key must be rejected" - ); - assert!( - result.purpose_mismatch && !result.hard_error, - "{purpose:?} must be a PURPOSE mismatch (non-permanent skip), not a hard failure" - ); + for purpose in ALL_PURPOSES { + let bls = make_identity(vec![make_key(0, KeyType::BLS12_381, purpose)]); + for result in [ + validate_contact_request(&bls, 0, &enabled, 0), + validate_contact_request(&enabled, 0, &bls, 0), + ] { + assert!(!result.is_valid, "{purpose:?}"); + assert!(result.is_permanent(), "{purpose:?}"); + } } } + /// `merge` must carry the `hard_error` flag so a hard fault in a merged + /// sub-result can't be lost. + #[test] + fn should_propagate_a_hard_error_through_merge() { + let mut a = ContactRequestValidation::new(); + a.add_absent_key_error("absent".into()); + let mut b = ContactRequestValidation::new(); + b.add_error("hard".into()); + a.merge(b); + assert!(!a.is_valid); + assert!(a.is_permanent()); + } + /// Sender ENCRYPTION + recipient DECRYPTION (our existing convention, - /// the newest 2026 cohort) still validates and is not a purpose mismatch. + /// the newest 2026 cohort) still validates. #[test] fn bound_convention_enc_dec_still_validates() { let sender = make_identity(vec![make_key( @@ -743,102 +747,6 @@ mod tests { let result = validate_contact_request(&sender, 4, &recipient, 5); assert!(result.is_valid, "errors: {:?}", result.errors); - assert!(!result.purpose_mismatch); - } - - /// A sender purpose outside the accepted set stays a purpose mismatch — - /// the classification flag must be set so the sweep/accept paths skip - /// rather than permanently break the channel. TRANSFER stands in for - /// AUTHENTICATION here: the latter is now an accepted legacy shape, but - /// no observed document puts TRANSFER on the sender side. - #[test] - fn unaccepted_sender_purpose_is_a_purpose_mismatch() { - let sender = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::TRANSFER, - )]); - let recipient = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::DECRYPTION, - )]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!( - result.purpose_mismatch, - "a sender purpose mismatch must be flagged so the channel is not permanently broken" - ); - } - - /// A NON-purpose failure (wrong key type) must NOT set `purpose_mismatch` - /// — it stays a hard/permanent failure that breaks the channel. - #[test] - fn wrong_key_type_is_not_a_purpose_mismatch() { - let sender = make_identity(vec![make_key(0, KeyType::BLS12_381, Purpose::ENCRYPTION)]); - let recipient = make_identity(vec![make_key( - 0, - KeyType::ECDSA_SECP256K1, - Purpose::DECRYPTION, - )]); - - let result = validate_contact_request(&sender, 0, &recipient, 0); - assert!(!result.is_valid); - assert!( - !result.purpose_mismatch, - "a key-TYPE failure is permanent, not a purpose mismatch" - ); - } - - /// **#5 — a purpose mismatch that co-occurs with a hard error must NOT be - /// downgraded to a skip.** `add_purpose_error` flags `purpose_mismatch` - /// even when a genuinely-permanent hard error (disabled / missing / - /// wrong-type key) is also present; reading the bare flag to decide - /// skip-vs-break would mask that permanent fault into a retry-forever - /// loop. `is_purpose_only()` is the correct gate. - #[test] - fn purpose_mismatch_with_hard_error_is_not_purpose_only() { - let mut v = ContactRequestValidation::new(); - v.add_purpose_error("recipient key purpose is AUTHENTICATION".into()); - v.add_error("sender key is disabled".into()); - - assert!(!v.is_valid); - assert!(v.purpose_mismatch, "the purpose flag is still raised"); - assert!( - !v.is_purpose_only(), - "a purpose mismatch alongside a hard error is NOT purpose-only — must stay permanent" - ); - } - - /// A lone purpose mismatch IS purpose-only → skippable. - #[test] - fn lone_purpose_mismatch_is_purpose_only() { - let mut v = ContactRequestValidation::new(); - v.add_purpose_error("recipient key purpose is AUTHENTICATION".into()); - assert!(v.is_purpose_only()); - } - - /// A lone hard error is never purpose-only. - #[test] - fn lone_hard_error_is_not_purpose_only() { - let mut v = ContactRequestValidation::new(); - v.add_error("sender key is disabled".into()); - assert!(!v.is_purpose_only()); - } - - /// `merge` must carry the `hard_error` flag so a hard fault in a merged - /// sub-result can't be lost (which would re-open the masking bug). - #[test] - fn merge_propagates_hard_error() { - let mut a = ContactRequestValidation::new(); - a.add_purpose_error("purpose".into()); - let mut b = ContactRequestValidation::new(); - b.add_error("hard".into()); - a.merge(b); - assert!(a.purpose_mismatch); - assert!(a.hard_error); - assert!(!a.is_purpose_only()); } // ----------------------------------------------------------------------- diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index 4d9e3e2eafb..b4fefc00d4a 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -18,6 +18,7 @@ use super::sdk_writer::SendContactRequestParams; use super::*; use crate::broadcaster::TransactionBroadcaster; use crate::error::PlatformWalletError; +use crate::wallet::identity::crypto::validation::validate_contact_request; use crate::wallet::identity::types::dashpay::contact_request::ContactRequest; use crate::wallet::identity::types::dashpay::established_contact::EstablishedContact; use crate::wallet::identity::ManagedIdentity; @@ -467,21 +468,21 @@ impl DashPayView<'_, B> { // required for ECDH. Shared selector — same enabled-only policy // as the contactInfo publish path, so both DashPay surfaces // agree on the ECDH root and a disabled (rotated-away) first - // key can't hard-fail the pre-send validator below while an + // key can't hard-fail the SDK's disabled-key refusal while an // enabled replacement exists. let sender_encryption_key = select_own_encryption_key(&sender_identity)?.clone(); let sender_key_index = sender_encryption_key.id(); let recipient_key_index = select_recipient_key_index(&recipient_identity)?; - // 3b. Gate the selected key pair through the same validator - // the receive/accept paths use, BEFORE any ECDH or - // broadcast. The selectors above pick plausible indices; - // the validator pins the full contract (key types, not - // disabled, purpose policy) so a malformed identity can't - // reach the encrypt-and-broadcast stage with a key that - // would poison the channel. - let validation = crate::wallet::identity::crypto::validation::validate_contact_request( + // 3b. Gate the selected key pair through the receive validator + // BEFORE any ECDH or broadcast. The selectors above pick + // plausible indices; the validator pins the key types so a + // malformed identity can't reach the encrypt-and-broadcast + // stage with a key that would poison the channel. DIP-15's + // send-side rule (neither key disabled) is enforced by + // `Sdk::create_contact_request`, its single owner. + let validation = validate_contact_request( &sender_identity, sender_key_index, &recipient_identity, @@ -1192,9 +1193,8 @@ pub(super) fn external_account_needs_rebuild( /// /// That mainnet's legacy Android/dashj population *does* reference /// AUTHENTICATION/TRANSFER keys is a fact about immutable history, handled by -/// the wider receive-side policy -/// ([`dash_sdk::platform::dashpay::recipient_key_purpose_is_acceptable_on_receive`]). -/// It must not relax what we mint: this selector calls +/// the receive side, which does not judge purpose at all (DIP-15 constrains +/// these keys by type only). It must not relax what we mint: this selector calls /// [`dash_sdk::platform::dashpay::recipient_key_purpose_is_valid`] for /// membership, so the cohort cannot drift from the SDK's request-creation /// gate. Only the preference ORDER below (DECRYPTION first, ENCRYPTION second) @@ -1208,7 +1208,7 @@ fn select_recipient_key_index(recipient_identity: &Identity) -> Result = recipient_identity .public_keys() .iter() @@ -1236,10 +1236,10 @@ fn select_recipient_key_index(recipient_identity: &Identity) -> Result Result<&IdentityPublicKey, PlatformWalletError> { @@ -2223,8 +2223,8 @@ impl DashPayView<'_, B> { // knowing that. Reading it back off an exported log beats asking the // user to query Platform. On-chain public metadata only; no key data. // Gated on the level: the block allocates a set, a string per key and a - // join, and takes the wallet-manager read lock. Purpose-rejected - // entries stay queued and revisit this path every sweep, so leaving + // join, and takes the wallet-manager read lock. Retryable entries + // stay queued and revisit this path every sweep, so leaving // that work unconditional would add recurring cost to the very path // this change exists to make cheap. if tracing::enabled!(tracing::Level::INFO) { @@ -2271,14 +2271,14 @@ impl DashPayView<'_, B> { } let mut cleared: Vec = Vec::new(); - // Distinct key-purpose rejections seen this drain, and how many entries - // each blocked — summarised once at the end instead of one WARN per - // entry. A purpose-rejected entry stays queued by design (the policy, - // not the immutable document, is what might change), so per-entry - // WARNing repeats every sweep for the life of the wallet: mainnet logs - // from one wallet show 396 such lines for 27 contacts in a single - // session. Every reason still reaches the log, once, with its count. - let mut policy_blocked: std::collections::BTreeMap = + // Distinct retryable rejections (a referenced key we do not have yet) + // seen this drain, and how many entries each blocked — summarised once + // at the end instead of one WARN per entry. Such an entry stays queued + // by design (the identity can gain the key), so per-entry WARNing + // would repeat every sweep: mainnet logs from one wallet once showed + // 396 such lines for 27 contacts in a single session. Every reason + // still reaches the log, once, with its count. + let mut retry_blocked: std::collections::BTreeMap = std::collections::BTreeMap::new(); // How much of `cleared` is already dequeued + persisted, and the running // total actually removed. Bookkeeping lands per entry, so at most one @@ -2398,13 +2398,12 @@ impl DashPayView<'_, B> { // Validate OUR key first — it needs nothing but the // resident identity, so a request that can never be used is // rejected before spending a Platform round trip on the - // contact. This is the dominant rejection in practice - // (legacy documents reference our AUTHENTICATION/TRANSFER - // key), and because such an entry stays queued the fetch - // below would otherwise repeat on every sweep, forever. + // contact. A request naming a key we do not have yet stays + // queued, so the fetch below would otherwise repeat on + // every sweep. // - // Deciding here means a MIXED failure — our key - // purpose-rejected and the contact's key hard-faulted — + // Deciding here means a MIXED failure — our key absent + // and the contact's key hard-faulted — // leaves the entry queued where a composed validator would // mark the channel broken. Deliberate: see // `validate_recipient_key`. Marking broken is unappealable @@ -2425,9 +2424,8 @@ impl DashPayView<'_, B> { ); continue; }; - // Did only the widened receive-side policy let this request - // through — i.e. does either referenced key name a purpose - // we would never mint ourselves? That marks it as the + // Does either referenced key name a purpose we would never + // mint ourselves? That marks it as the // legacy dashj cohort, whose ECDH/AES byte compatibility // with our implementation has not been cross-validated // against a dashj-produced payload. Used far below to keep @@ -2453,7 +2451,7 @@ impl DashPayView<'_, B> { .apply_drain_validation_failure( entry, recipient_validation, - &mut policy_blocked, + &mut retry_blocked, ) .await { @@ -2461,6 +2459,9 @@ impl DashPayView<'_, B> { } continue; } + for warning in &recipient_validation.warnings { + tracing::warn!(owner = %entry.owner_identity_id, contact = %entry.contact_id, warning, "drain: contact request accepted with a key warning"); + } // Fetch the contact identity (transient on failure → leave). // Bounded: a Platform round trip, and nothing in this entry @@ -2495,11 +2496,10 @@ impl DashPayView<'_, B> { } }; - // The sender side of the legacy classification — the - // widening moved the sender rule from ENCRYPTION-only to - // ENCRYPTION-or-AUTHENTICATION too, so an AUTHENTICATION - // sender against a mint-valid recipient is just as much an - // unverified legacy payload. + // The sender side of the legacy classification — we mint + // ENCRYPTION senders only, so a sender key of any other + // purpose against a mint-valid recipient is just as much + // an unverified legacy payload. let accepted_by_legacy_widening = recipient_widened || contact_identity .get_public_key_by_id(*contact_encryption_key_index) @@ -2515,13 +2515,16 @@ impl DashPayView<'_, B> { ); if !validation.is_valid { if self - .apply_drain_validation_failure(entry, validation, &mut policy_blocked) + .apply_drain_validation_failure(entry, validation, &mut retry_blocked) .await { cleared.push(entry.key()); } continue; } + for warning in &validation.warnings { + tracing::warn!(owner = %entry.owner_identity_id, contact = %entry.contact_id, warning, "drain: contact request accepted with a key warning"); + } // The contact's encryption pubkey (peer). A malformed/missing // key is a permanent fault — re-deriving won't help. @@ -2750,19 +2753,19 @@ impl DashPayView<'_, B> { .flush_drained_contact_crypto(&entries, &cleared[flushed..]) .await; - // One line for every entry the key-purpose policy turned away, instead - // of one per entry per sweep. Kept at WARN and carrying the distinct + // One line for every entry left queued as retryable, instead of one + // per entry per sweep. Kept at WARN and carrying the distinct // reasons: this is the signal that a live on-chain cohort is failing // our acceptance policy, which is exactly how the legacy dashj cohort // was found — it must stay visible in an exported log, just not 27 // times a pass. - if !policy_blocked.is_empty() { - let blocked: usize = policy_blocked.values().sum(); + if !retry_blocked.is_empty() { + let blocked: usize = retry_blocked.values().sum(); tracing::warn!( entries = blocked, - reasons = ?policy_blocked, - "drain: contact requests left queued by the key-purpose policy \ - (not marking broken; they retry when the policy changes)" + reasons = ?retry_blocked, + "drain: contact requests left queued on a retryable key-index failure \ + (not marking broken; they retry every sweep)" ); } // One-line verdict for the pass. "Did the legacy contacts build?" is @@ -2781,19 +2784,19 @@ impl DashPayView<'_, B> { /// The drain's validation-failure policy, shared by the recipient-half and /// sender-half checks so both halves classify identically. /// - /// - A failure that can still resolve — a purpose mismatch (our acceptance - /// policy might change) or an absent key id (identities gain keys) — is - /// counted into `policy_blocked` and left queued. The `contactRequest` + /// - A failure that can still resolve — an absent key id (identities gain + /// keys) — is counted into `retry_blocked` and left queued. The `contactRequest` /// cleared consensus and is immutable; a channel marked broken here needs /// a superseding request from the CONTACT to heal, an appeal the user /// cannot file. /// - Only a fault that immutable facts make permanent — a key type that - /// cannot do ECDH, a key we disabled — breaks the channel, so the sweep - /// stops collecting it. + /// cannot do ECDH — breaks the channel, so the sweep stops collecting + /// it. A key disabled since the request was sent is not one: DIP-11 + /// keeps it usable, so the validators only warn about it. /// /// Returns `true` when the caller should clear the entry from the queue. /// - /// Takes `validation` by value: a purpose-rejected entry stays queued by + /// Takes `validation` by value: a retryable entry stays queued by /// design and comes back through here on every sweep, so cloning its /// reasons into the summary would allocate once per contact per pass for /// the life of the wallet. Moving them costs nothing — neither caller uses @@ -2802,16 +2805,16 @@ impl DashPayView<'_, B> { &self, entry: &crate::changeset::PendingContactCrypto, validation: crate::wallet::identity::crypto::validation::ContactRequestValidation, - policy_blocked: &mut std::collections::BTreeMap, + retry_blocked: &mut std::collections::BTreeMap, ) -> bool { if !validation.is_permanent() { tracing::debug!( owner = %entry.owner_identity_id, contact = %entry.contact_id, errors = ?validation.errors, - "drain: contact request key-purpose mismatch; leaving queued (not marking broken)" + "drain: contact request names a key not found yet; leaving queued (not marking broken)" ); for reason in validation.errors { - *policy_blocked.entry(reason).or_default() += 1; + *retry_blocked.entry(reason).or_default() += 1; } return false; } @@ -3727,6 +3730,9 @@ impl DashPayView<'_, B> { validation.errors ))); } + for warning in &validation.warnings { + tracing::warn!(owner = %our_identity_id, contact = %contact_id, warning, "accept: contact request accepted with a key warning"); + } // Seedless ECDH: our decryption-key scalar (at the Rust-built path) // against the contact's encryption pubkey, computed in the signer. The @@ -5754,8 +5760,8 @@ mod recipient_key_selection_tests { /// **Own-key selector: a disabled first ENCRYPTION key is skipped in /// favour of the enabled replacement.** After a disable-and-replace key /// rotation the lowest-id ENCRYPTION key is disabled; selecting it - /// would hard-fail the pre-send validator on every new outgoing contact - /// request ("Sender key N is disabled") even though an enabled + /// would fail every new outgoing contact request in the SDK + /// ("Sender key at index N is disabled") even though an enabled /// replacement exists — while the contactInfo path (which already /// filtered disabled keys) would use the replacement, splitting the two /// surfaces' notion of the ECDH root. Was red against the send path's diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index 0f396096316..f85cb390bb0 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -11227,8 +11227,9 @@ mod tests { ); } - /// An unaccepted recipient PURPOSE — the actual repeating case — is decided - /// locally, and a co-occurring sender-side hard fault does not change that. + /// A request naming a key of ours we do not have yet — the repeating case + /// — is decided locally, and a co-occurring sender-side hard fault does + /// not change that. /// /// This is the discriminating test for both halves of the change: /// @@ -11238,13 +11239,13 @@ mod tests { /// mark the channel broken and clear the entry. Asserting the entry is /// still queued and the channel still intact therefore proves the fetch /// never happened; a "hard faults only" short-circuit that still fetched - /// for purpose mismatches would fail here. - /// * It pins the deliberate mixed-failure policy change: purpose-rejected - /// on our side wins, and the entry stays recoverable. + /// for retryable failures would fail here. + /// * It pins the deliberate mixed-failure policy: a retryable failure on + /// our side wins, and the entry stays recoverable. /// /// Drained twice, because the cost avoided is per sweep, not once. #[tokio::test] - async fn unaccepted_recipient_purpose_never_fetches_and_stays_recoverable() { + async fn absent_recipient_key_never_fetches_and_stays_recoverable() { use crate::changeset::{PendingContactCrypto, PendingContactCryptoOp}; use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; @@ -11253,11 +11254,11 @@ mod tests { let owner = Identifier::from([0xAA; 32]); let contact = Identifier::from([0xBB; 32]); - // Our key at the referenced index: valid ECDSA, but a purpose the - // receive-side policy does not accept — a purpose-only rejection. + // Our identity holds key 0 only; the request names our key 1, which + // we do not have yet — a retryable rejection. let our_key = IdentityPublicKey::V0(IdentityPublicKeyV0 { id: 0, - purpose: Purpose::VOTING, + purpose: Purpose::DECRYPTION, security_level: SecurityLevel::HIGH, contract_bounds: None, key_type: KeyType::ECDSA_SECP256K1, @@ -11341,7 +11342,7 @@ mod tests { contact_id: contact, op: PendingContactCryptoOp::RegisterExternal { encrypted_public_key: vec![7u8; 96], - our_decryption_key_index: 0, + our_decryption_key_index: 1, contact_encryption_key_index: 0, }, enqueued_at_ms: 0, @@ -11356,7 +11357,7 @@ mod tests { .await; assert_eq!( drained, 0, - "pass {pass}: a purpose-rejected entry must stay queued, not be cleared" + "pass {pass}: a retryable entry must stay queued, not be cleared" ); } @@ -11370,7 +11371,7 @@ mod tests { assert_eq!( managed.dashpay().pending_contact_crypto.len(), 1, - "the entry must survive repeated drains so a policy change can still pick it up" + "the entry must survive repeated drains so the key, once added, can still pick it up" ); assert!( !managed @@ -11397,7 +11398,7 @@ mod tests { /// ordering fetched first, failed transiently, and left the channel /// intact. /// - /// That ordering is what keeps a purpose-rejected entry — which stays + /// That ordering is what keeps a retryable entry — which stays /// queued by design, and so is retried on every sweep forever — from /// spending a network round trip each time. #[tokio::test] diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs index e8a3777fb63..d2e7b5a3d79 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/update.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/update.rs @@ -75,6 +75,12 @@ impl IdentityWallet { /// identity-update state transition requires (DPP gates this on /// MASTER specifically — HIGH/CRITICAL aren't accepted). /// + /// An update that would disable the last valid authentication key of a + /// security level is refused by the transition builder before anything is + /// signed or broadcast, with + /// `PlatformWalletError::Sdk(dash_sdk::Error::Protocol(ProtocolError::LastAuthenticationKey(_)))` + /// (DIP-11); keys added in the same update count as valid. + /// /// NOTE: callers that ADD keys via `add_public_keys` are /// responsible for pre-persisting the new keys' private material /// to whatever store the supplied signer reads from (iOS Keychain @@ -188,11 +194,16 @@ impl IdentityWallet { None, ) .await - .map_err(|e| { - PlatformWalletError::InvalidIdentityData(format!( + .map_err(|e| match e { + // The builder's DIP-11 refusal stays typed, as on the + // caller-supplied-identity entry point. + refusal @ ProtocolError::LastAuthenticationKey(_) => { + PlatformWalletError::Sdk(dash_sdk::Error::Protocol(refusal)) + } + e => PlatformWalletError::InvalidIdentityData(format!( "Failed to create identity update transition: {}", e - )) + )), })?; if let Err(e) = state_transition @@ -419,6 +430,12 @@ impl IdentityWallet { /// caller receives the chain's error. See /// `docs/protocol/authentication-key-limits.md`. /// + /// An update that would disable the last valid authentication key of a + /// security level is refused by the transition builder before anything is + /// signed or broadcast, with + /// `dash_sdk::Error::Protocol(ProtocolError::LastAuthenticationKey(_))` + /// (DIP-11). + /// /// Returns the [`StateTransitionProofResult`] from the broadcast so callers /// can inspect proof-verified outcomes (e.g. updated keys, balance). pub async fn update_identity_with_signer>( @@ -919,3 +936,151 @@ mod update_identity_tests { #[cfg(test)] #[path = "lagging_fetch_tests.rs"] mod lagging_fetch_tests; + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_support::{test_platform_wallet_manager_with_sdk, NoopTestPersister}; + use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + use crate::wallet::platform_wallet::WalletId; + use dash_sdk::query_types::IdentityNonceFetcher; + use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dpp::identity::v0::IdentityV0; + use std::collections::BTreeMap; + use std::sync::Arc; + + /// Signs nothing; holds every key so the MASTER key is selectable. + #[derive(Debug)] + struct HoldsEveryKey; + + #[async_trait] + impl Signer for HoldsEveryKey { + async fn sign( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(ProtocolError::Generic( + "not signing in this test".to_string(), + )) + } + + async fn sign_create_witness( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + Err(ProtocolError::Generic( + "not signing in this test".to_string(), + )) + } + + fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { + true + } + } + + /// MASTER 0, CRITICAL 1, HIGH 2: one authentication key per level. + fn identity() -> Identity { + let keys = [ + (0, SecurityLevel::MASTER), + (1, SecurityLevel::CRITICAL), + (2, SecurityLevel::HIGH), + ] + .map(|(id, security_level)| { + IdentityPublicKey::V0(IdentityPublicKeyV0 { + id, + purpose: Purpose::AUTHENTICATION, + security_level, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new(vec![id as u8 + 2; 33]), + disabled_at: None, + }) + }); + IdentityV0 { + id: Identifier::from([1; 32]), + public_keys: keys + .into_iter() + .map(|key| (key.id(), key)) + .collect::>(), + balance: 0, + revision: 0, + } + .into() + } + + /// A wallet over a mock SDK that answers only the identity-nonce query, so + /// an update that got past the builder would fail its broadcast with a + /// different error. + async fn manager_answering_only_the_nonce() -> ( + Arc>, + WalletId, + ) { + let mut sdk = dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"); + sdk.mock() + .expect_fetch::( + Identifier::from([1; 32]), + Some(IdentityNonceFetcher(0)), + ) + .await + .expect("nonce expectation"); + test_platform_wallet_manager_with_sdk(sdk).await + } + + #[tokio::test] + async fn should_refuse_disabling_the_last_critical_key_before_sending_anything() { + let (manager, wallet_id) = manager_answering_only_the_nonce().await; + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + { + let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); + let mut wm = manager.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + info.identity_manager + .add_identity(identity(), 0, wallet_id, &persister) + .expect("add identity"); + } + + let result = wallet + .identity() + .update_identity_with_external_signer( + &Identifier::from([1; 32]), + vec![], + vec![1], + &HoldsEveryKey, + None, + ) + .await; + assert!( + matches!( + result, + Err(PlatformWalletError::Sdk(dash_sdk::Error::Protocol( + ProtocolError::LastAuthenticationKey(ref refusal) + ))) + if refusal.key_id == 1 && refusal.security_level == SecurityLevel::CRITICAL + ), + "{result:?}" + ); + } + + /// The caller-supplied-identity entry point refuses the same way. + #[tokio::test] + async fn should_refuse_disabling_the_last_critical_key_with_a_supplied_identity() { + let (manager, wallet_id) = manager_answering_only_the_nonce().await; + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + + let result = wallet + .identity() + .update_identity_with_signer(&identity(), &0, vec![], vec![1], &HoldsEveryKey, None) + .await; + assert!( + matches!( + result, + Err(dash_sdk::Error::Protocol(ProtocolError::LastAuthenticationKey(ref refusal))) + if refusal.key_id == 1 && refusal.security_level == SecurityLevel::CRITICAL + ), + "{result:?}" + ); + } +} diff --git a/packages/rs-sdk/src/platform/dashpay/contact_request.rs b/packages/rs-sdk/src/platform/dashpay/contact_request.rs index 3f528a4bba6..45bcbbd66a5 100644 --- a/packages/rs-sdk/src/platform/dashpay/contact_request.rs +++ b/packages/rs-sdk/src/platform/dashpay/contact_request.rs @@ -152,8 +152,7 @@ pub struct SendContactRequestResult { /// request **we are about to mint**. The sender always references its own /// ENCRYPTION key. /// -/// Mint-side only — see [`sender_key_purpose_is_acceptable_on_receive`] for -/// what we accept from documents already on chain. +/// Mint-side only: documents already on chain are not judged by purpose. fn sender_key_purpose_is_valid(purpose: Purpose) -> bool { purpose == Purpose::ENCRYPTION } @@ -171,55 +170,80 @@ fn sender_key_purpose_is_valid(purpose: Purpose) -> bool { /// ECDH is poor key separation, and no new document needs to. /// /// It is NOT the acceptance policy for inbound documents — a `contactRequest` -/// is immutable, so history cannot be re-minted to fit this rule. See -/// [`recipient_key_purpose_is_acceptable_on_receive`]. +/// is immutable, and DIP-15 constrains its keys by type only, so received +/// documents are not judged by purpose at all. pub fn recipient_key_purpose_is_valid(purpose: Purpose) -> bool { matches!(purpose, Purpose::DECRYPTION | Purpose::ENCRYPTION) } -/// Whether `purpose` on the `recipientKeyIndex` key of an **inbound, already -/// on-chain** contact request is acceptable for the ECDH that unwraps the -/// sender's `encryptedPublicKey`. +/// The sender's and the recipient's keys a contact request **we are about to +/// mint** names, once both pass DIP-15's send-side checks: each exists, has a +/// purpose [`sender_key_purpose_is_valid`] / [`recipient_key_purpose_is_valid`] +/// accepts, and is valid, i.e. not disabled. Encrypting the payment xpub to a +/// disabled key would hand it to whoever holds the revoked private half. /// -/// Strictly wider than [`recipient_key_purpose_is_valid`], and deliberately -/// so. `contactRequest` documents are immutable and consensus enforces no -/// purpose constraint on these integer fields, so the acceptance policy is the -/// *only* thing standing between a user and their own payment history. -/// Mainnet device logs (2026-08, a 29-contact wallet whose contacts were -/// established through the legacy Android/dashj client) show 27 of 29 inbound -/// requests referencing the recipient's AUTHENTICATION (key ids 0-2) or -/// TRANSFER (key id 3) key — under the mint-side rule every one of those -/// contacts is unpayable forever, with no action the user can take. -/// -/// Purpose carries no cryptographic weight here: ECDH is defined over the -/// secp256k1 keypair, and DIP-9's identity-key tree is indexed by key *type* -/// and id, never by purpose, so the same derivation reaches all of them. The -/// gates that do carry weight — `ECDSA_SECP256K1` key type and the -/// disabled-key check — are enforced separately by the caller and are -/// unaffected by this predicate. -/// -/// The node-operational purposes (SYSTEM, VOTING, OWNER) stay rejected: -/// nothing on chain references them for DashPay, and they have no business in -/// a payment-channel handshake. -pub fn recipient_key_purpose_is_acceptable_on_receive(purpose: Purpose) -> bool { - matches!( - purpose, - Purpose::DECRYPTION | Purpose::ENCRYPTION | Purpose::AUTHENTICATION | Purpose::TRANSFER - ) -} +/// The single owner of these checks. Received documents are not judged here: a +/// `contactRequest` is immutable, and one naming a key disabled since it was +/// sent stays usable (DIP-11). +fn contact_request_keys<'a>( + sender_identity: &'a Identity, + sender_key_index: u32, + recipient_identity: &'a Identity, + recipient_key_index: u32, +) -> Result<(&'a IdentityPublicKey, &'a IdentityPublicKey), Error> { + let sender_key = sender_identity + .public_keys() + .get(&sender_key_index) + .ok_or_else(|| { + Error::Generic(format!( + "Sender identity does not have encryption key at index {}", + sender_key_index + )) + })?; + + // Sender always references its own ENCRYPTION key (the live + // convention of both on-chain cohorts). + if !sender_key_purpose_is_valid(sender_key.purpose()) { + return Err(Error::Generic(format!( + "Sender key at index {} is not an encryption key", + sender_key_index + ))); + } -/// Receive-side counterpart of [`sender_key_purpose_is_valid`]: whether -/// `purpose` on the `senderKeyIndex` key of an **inbound, already on-chain** -/// contact request is acceptable for ECDH. -/// -/// Same reasoning as [`recipient_key_purpose_is_acceptable_on_receive`]. The -/// legacy cohort is narrower on this side — the observed mainnet documents -/// pair an AUTHENTICATION sender key with an AUTHENTICATION recipient key — so -/// only AUTHENTICATION is added. A sender referencing any other purpose has -/// not been seen and stays a purpose mismatch (skip-and-retry, never a -/// permanently broken channel), leaving room to widen again on evidence. -pub fn sender_key_purpose_is_acceptable_on_receive(purpose: Purpose) -> bool { - matches!(purpose, Purpose::ENCRYPTION | Purpose::AUTHENTICATION) + let recipient_key = recipient_identity + .public_keys() + .get(&recipient_key_index) + .ok_or_else(|| { + Error::Generic(format!( + "Recipient identity does not have a key at index {}", + recipient_key_index + )) + })?; + + // Accept either a DECRYPTION key (newest cohort / our original + // convention) OR an ENCRYPTION key (the dominant mobile cohort, whose + // identities carry no DECRYPTION key and reference their ENCRYPTION + // key for recipientKeyIndex). + if !recipient_key_purpose_is_valid(recipient_key.purpose()) { + return Err(Error::Generic(format!( + "Recipient key at index {} is not a decryption or encryption key", + recipient_key_index + ))); + } + + for (side, key_index, key) in [ + ("Sender", sender_key_index, sender_key), + ("Recipient", recipient_key_index, recipient_key), + ] { + if let Some(disabled_at) = key.disabled_at() { + return Err(Error::Generic(format!( + "{side} key at index {key_index} is disabled (at timestamp {disabled_at}); a new \ + contact request needs a valid key" + ))); + } + } + + Ok((sender_key, recipient_key)) } impl Sdk { @@ -249,7 +273,8 @@ impl Sdk { /// Returns an error if: /// - The DashPay contract cannot be fetched /// - The contactRequest document type is not found - /// - The sender or recipient doesn't have the required encryption keys + /// - The sender or recipient doesn't have the required encryption keys, or either key is + /// disabled /// - ECDH encryption fails /// - The shared secret, private key, or extended public key cannot be retrieved pub async fn create_contact_request( @@ -287,48 +312,12 @@ impl Sdk { } }; - // Verify sender has the encryption key at the specified index - let sender_key = input - .sender_identity - .public_keys() - .get(&input.sender_key_index) - .ok_or_else(|| { - Error::Generic(format!( - "Sender identity does not have encryption key at index {}", - input.sender_key_index - )) - })?; - - // Sender always references its own ENCRYPTION key (the live - // convention of both on-chain cohorts). - if !sender_key_purpose_is_valid(sender_key.purpose()) { - return Err(Error::Generic(format!( - "Sender key at index {} is not an encryption key", - input.sender_key_index - ))); - } - - // Verify recipient has the referenced key at the specified index. - let recipient_key = recipient_identity - .public_keys() - .get(&input.recipient_key_index) - .ok_or_else(|| { - Error::Generic(format!( - "Recipient identity does not have a key at index {}", - input.recipient_key_index - )) - })?; - - // Accept either a DECRYPTION key (newest cohort / our original - // convention) OR an ENCRYPTION key (the dominant mobile cohort, whose - // identities carry no DECRYPTION key and reference their ENCRYPTION - // key for recipientKeyIndex). - if !recipient_key_purpose_is_valid(recipient_key.purpose()) { - return Err(Error::Generic(format!( - "Recipient key at index {} is not a decryption or encryption key", - input.recipient_key_index - ))); - } + let (sender_key, recipient_key) = contact_request_keys( + &input.sender_identity, + input.sender_key_index, + &recipient_identity, + input.recipient_key_index, + )?; // Get the recipient's public key data for ECDH let recipient_public_key_data = recipient_key.data(); @@ -708,8 +697,8 @@ mod tests { fn mint_side_still_refuses_authentication_and_transfer() { // What we CREATE stays strict: reusing a signing or fund-authorizing // key for ECDH is poor key separation, and no new document needs to. - // Widening the receive-side acceptance below must never leak into the - // key we pick for our own outgoing requests. + // Accepting every purpose on receive must never leak into the key we + // pick for our own outgoing requests. assert!(!recipient_key_purpose_is_valid(Purpose::AUTHENTICATION)); assert!(!recipient_key_purpose_is_valid(Purpose::TRANSFER)); } @@ -723,49 +712,68 @@ mod tests { assert!(!sender_key_purpose_is_valid(Purpose::AUTHENTICATION)); } - #[test] - fn receive_side_accepts_the_legacy_dashj_cohort() { - // Regression guard for the mainnet legacy cohort: inbound requests - // minted by the Android/dashj client reference the recipient's - // AUTHENTICATION (key ids 0-2) or TRANSFER (key id 3) key. Rejecting - // them made every pre-iOS contact permanently unpayable — the document - // is immutable, so no user action could ever fix it. - for purpose in [ - Purpose::DECRYPTION, - Purpose::ENCRYPTION, - Purpose::AUTHENTICATION, - Purpose::TRANSFER, - ] { - assert!( - recipient_key_purpose_is_acceptable_on_receive(purpose), - "{purpose:?} recipient key must be accepted from an on-chain document" - ); + fn identity_with_key(purpose: Purpose, disabled_at: Option) -> Identity { + use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; + use dpp::identity::v0::IdentityV0; + use dpp::identity::{KeyType, SecurityLevel}; + use dpp::platform_value::BinaryData; + let (_, public_key) = secp256k1::generate_keypair(&mut rand::rng()); + let key = IdentityPublicKey::V0(IdentityPublicKeyV0 { + id: 0, + purpose, + security_level: SecurityLevel::MEDIUM, + contract_bounds: None, + key_type: KeyType::ECDSA_SECP256K1, + read_only: false, + data: BinaryData::new(public_key.serialize().to_vec()), + disabled_at, + }); + IdentityV0 { + id: Identifier::from([2; 32]), + public_keys: BTreeMap::from([(0, key)]), + balance: 0, + revision: 0, } - // The sender side of the same legacy documents pairs AUTHENTICATION - // with AUTHENTICATION; ENCRYPTION remains the modern convention. - assert!(sender_key_purpose_is_acceptable_on_receive( - Purpose::ENCRYPTION - )); - assert!(sender_key_purpose_is_acceptable_on_receive( - Purpose::AUTHENTICATION - )); + .into() } #[test] - fn receive_side_still_refuses_node_operational_purposes() { - // Not observed on chain for DashPay — widening is evidence-driven, so - // these stay out until something real needs them. A rejection here is - // a skip-and-retry purpose mismatch, never a permanently broken - // channel, so a later widening can still recover those contacts. - for purpose in [Purpose::SYSTEM, Purpose::VOTING, Purpose::OWNER] { - assert!(!recipient_key_purpose_is_acceptable_on_receive(purpose)); - assert!(!sender_key_purpose_is_acceptable_on_receive(purpose)); - } - // TRANSFER is accepted for the recipient (legacy key id 3) but has - // never been seen on the sender side. - assert!(!sender_key_purpose_is_acceptable_on_receive( - Purpose::TRANSFER - )); + fn should_accept_enabled_sender_and_recipient_keys() { + let sender = identity_with_key(Purpose::ENCRYPTION, None); + let recipient = identity_with_key(Purpose::DECRYPTION, None); + let (sender_key, recipient_key) = + contact_request_keys(&sender, 0, &recipient, 0).expect("both keys are valid"); + assert_eq!(sender_key, sender.public_keys().get(&0).unwrap()); + assert_eq!(recipient_key, recipient.public_keys().get(&0).unwrap()); + } + + #[test] + fn should_refuse_a_disabled_sender_key() { + let sender = identity_with_key(Purpose::ENCRYPTION, Some(1_700_000_000_000)); + let recipient = identity_with_key(Purpose::DECRYPTION, None); + let error = contact_request_keys(&sender, 0, &recipient, 0) + .expect_err("a disabled sender key must be refused"); + assert!( + error + .to_string() + .contains("Sender key at index 0 is disabled"), + "{error}" + ); + } + + #[test] + fn should_refuse_a_disabled_recipient_key() { + // Encrypting the payment xpub to a revoked key hands it to whoever holds the key. + let sender = identity_with_key(Purpose::ENCRYPTION, None); + let recipient = identity_with_key(Purpose::ENCRYPTION, Some(1_700_000_000_000)); + let error = contact_request_keys(&sender, 0, &recipient, 0) + .expect_err("a disabled recipient key must be refused"); + assert!( + error + .to_string() + .contains("Recipient key at index 0 is disabled"), + "{error}" + ); } #[test] diff --git a/packages/rs-sdk/src/platform/dashpay/mod.rs b/packages/rs-sdk/src/platform/dashpay/mod.rs index 3789065cd31..34ac250db7c 100644 --- a/packages/rs-sdk/src/platform/dashpay/mod.rs +++ b/packages/rs-sdk/src/platform/dashpay/mod.rs @@ -10,9 +10,8 @@ mod invitation; mod profile; pub use contact_request::{ - recipient_key_purpose_is_acceptable_on_receive, recipient_key_purpose_is_valid, - sender_key_purpose_is_acceptable_on_receive, ContactRequestInput, ContactRequestResult, - EcdhProvider, RecipientIdentity, SendContactRequestInput, SendContactRequestResult, + recipient_key_purpose_is_valid, ContactRequestInput, ContactRequestResult, EcdhProvider, + RecipientIdentity, SendContactRequestInput, SendContactRequestResult, }; pub use contact_request_queries::ContactRequestDocuments; pub use invitation::{ diff --git a/packages/strategy-tests/src/transitions.rs b/packages/strategy-tests/src/transitions.rs index 751b4f28b89..2d531dd85d3 100644 --- a/packages/strategy-tests/src/transitions.rs +++ b/packages/strategy-tests/src/transitions.rs @@ -59,6 +59,7 @@ use dpp::native_bls::NativeBlsModule; use dpp::state_transition::identity_topup_transition::methods::IdentityTopUpTransitionMethodsV0; use dpp::state_transition::identity_topup_transition::IdentityTopUpTransition; use dpp::state_transition::identity_update_transition::methods::IdentityUpdateTransitionMethodsV0; +use dpp::state_transition::identity_update_transition::v0::IdentityUpdateTransitionV0; use dpp::state_transition::identity_update_transition::IdentityUpdateTransition; use dpp::state_transition::{GetDataContractSecurityLevelRequirementFn, StateTransition}; use dpp::version::PlatformVersion; @@ -571,12 +572,12 @@ pub async fn create_identity_update_transition_disable_keys( } }); - let master_key_id = *identity + let master_key = identity .public_keys() - .iter() - .find(|(_, key)| key.security_level() == MASTER) + .values() + .find(|key| key.security_level() == MASTER) .expect("expected to have a master key") - .0; + .clone(); let identity_nonce = identity_nonce_counter.entry(identity.id()).or_default(); *identity_nonce += 1; @@ -584,19 +585,38 @@ pub async fn create_identity_update_transition_disable_keys( // Bump revision before creating state transition - the transition uses identity.revision() directly identity.bump_revision(); - let state_transition = IdentityUpdateTransition::try_from_identity_with_signer( - identity, - &master_key_id, - vec![], - key_ids_to_disable, - *identity_nonce, - 0, - signer, - platform_version, - None, - ) - .await - .expect("expected to create a DisableKeys transition"); + // Built by hand rather than through `IdentityUpdateTransition::try_from_identity_with_signer`: + // that builder applies DIP-11's client-side policy and refuses to disable the last key of a + // security level, which consensus accepts for every level but MASTER and these tests must + // keep exercising. The fields and signature match what the builder produces otherwise. + let serialization_version = platform_version + .dpp + .state_transition_serialization_versions + .identity_update_state_transition + .default_current_version; + assert_eq!( + serialization_version, 0, + "strategy tests build identity updates in serialization version 0 only" + ); + let mut state_transition: StateTransition = IdentityUpdateTransitionV0 { + signature: Default::default(), + signature_public_key_id: 0, + identity_id: identity.id(), + revision: identity.revision(), + nonce: *identity_nonce, + add_public_keys: vec![], + disable_public_keys: key_ids_to_disable, + user_fee_increase: 0, + } + .into(); + state_transition + .sign_external( + &master_key, + signer, + None::, + ) + .await + .expect("expected to sign a DisableKeys transition"); Some(state_transition) } @@ -1291,3 +1311,82 @@ pub async fn create_state_transitions_for_identities_and_proofs( } results } + +#[cfg(test)] +mod tests { + use super::*; + use dpp::state_transition::identity_update_transition::accessors::IdentityUpdateTransitionAccessorsV0; + use rand::SeedableRng; + + /// The hand-built DisableKeys transition is byte-for-byte what dpp's builder signs whenever + /// the builder's DIP-11 client policy lets the update through, so bypassing that policy + /// changes nothing else about the seeded runs. + #[tokio::test] + async fn should_build_the_dpp_builder_transition_whenever_the_builder_accepts_the_disable() { + let platform_version = PlatformVersion::latest(); + let mut compared = 0; + for seed in 0..32 { + let mut rng = StdRng::seed_from_u64(seed); + let (identity, private_keys): (Identity, BTreeMap) = + Identity::random_identity_with_main_keys_with_private_key( + 5, + &mut rng, + platform_version, + ) + .expect("expected a random identity"); + let mut signer = SimpleSigner::default(); + signer.add_identity_public_keys(private_keys); + + let mut updated = identity.clone(); + let Some(hand_built) = create_identity_update_transition_disable_keys( + &mut updated, + 1, + &mut BTreeMap::new(), + 1_000, + &mut signer, + &mut rng, + platform_version, + ) + .await + else { + continue; + }; + let StateTransition::IdentityUpdate(update) = &hand_built else { + panic!("expected an identity update, got {hand_built:?}"); + }; + + let master_key_id = *identity + .public_keys() + .iter() + .find(|(_, key)| key.security_level() == MASTER) + .expect("expected a master key") + .0; + let mut bumped = identity.clone(); + bumped.bump_revision(); + match IdentityUpdateTransition::try_from_identity_with_signer( + &bumped, + &master_key_id, + vec![], + update.public_key_ids_to_disable().to_vec(), + 1, + 0, + &signer, + platform_version, + None, + ) + .await + { + Ok(built) => { + assert_eq!(built, hand_built, "seed {seed}"); + compared += 1; + } + Err(ProtocolError::LastAuthenticationKey(_)) => {} + Err(e) => panic!("seed {seed}: {e}"), + } + } + assert!( + compared > 0, + "no seed produced an update the builder accepts" + ); + } +} diff --git a/packages/wasm-sdk/src/error.rs b/packages/wasm-sdk/src/error.rs index 5ab2a8d9625..855a06d51ee 100644 --- a/packages/wasm-sdk/src/error.rs +++ b/packages/wasm-sdk/src/error.rs @@ -323,8 +323,14 @@ impl From for WasmSdkError { } impl From for WasmSdkError { fn from(err: ProtocolError) -> Self { + // A DIP-11 refusal judges the caller's request, not the protocol: retrying cannot + // change it. + let kind = match err { + ProtocolError::LastAuthenticationKey(_) => WasmSdkErrorKind::InvalidArgument, + _ => WasmSdkErrorKind::Protocol, + }; let code = consensus_error_code(&err); - Self::new(WasmSdkErrorKind::Protocol, err.to_string(), code, false) + Self::new(kind, err.to_string(), code, false) } } @@ -440,6 +446,8 @@ mod tests { use dash_sdk::dpp::consensus::state::contract_moderation::ContractUserBannedError; use dash_sdk::dpp::consensus::ConsensusError; use dash_sdk::dpp::data_contract::errors::DataContractError; + use dash_sdk::dpp::identity::key_disable_rule::LastAuthenticationKeyError; + use dash_sdk::dpp::identity::SecurityLevel; use dash_sdk::dpp::platform_value::Identifier; use dash_sdk::dpp::serialization::PlatformSerializableWithPlatformVersion; use dash_sdk::dpp::version::PlatformVersion; @@ -646,6 +654,24 @@ mod tests { assert_eq!(error.code(), 10231); } + #[test] + fn should_report_a_dip11_last_key_refusal_as_an_invalid_argument() { + let refusal = ProtocolError::from(LastAuthenticationKeyError { + key_id: 1, + security_level: SecurityLevel::CRITICAL, + }); + let message = refusal.to_string(); + let error = WasmSdkError::with_context("Failed to create update transition", refusal); + + assert_eq!(error.kind(), WasmSdkErrorKind::InvalidArgument); + assert_eq!(error.code(), -1); + assert!(!error.is_retriable()); + assert_eq!( + error.message(), + format!("Failed to create update transition: {message}") + ); + } + #[test] fn should_report_no_code_for_a_protocol_error_that_is_not_a_consensus_error() { let error = WasmSdkError::from(ProtocolError::Generic("not consensus".to_string())); diff --git a/packages/wasm-sdk/src/state_transitions/identity.rs b/packages/wasm-sdk/src/state_transitions/identity.rs index 6aacefde7e8..fdc65584aff 100644 --- a/packages/wasm-sdk/src/state_transitions/identity.rs +++ b/packages/wasm-sdk/src/state_transitions/identity.rs @@ -537,7 +537,12 @@ export interface IdentityUpdateOptions { /** * Array of key IDs to disable. - * Cannot disable master, critical auth, or transfer keys. + * The update is refused before signing if it would disable the last valid authentication + * key of a security level (DIP-11). A valid key is enabled, has no expiry and no budget, and + * has a key type that can sign (not BIP13_SCRIPT_HASH or EDDSA_25519_HASH160). It stands in + * at its own level or, unless it is MASTER, at any weaker level, and only for keys with no + * contract bounds or the same bounds as its own. Keys added in the same update count. Keys of + * other purposes are not restricted. */ disablePublicKeys?: number[]; @@ -585,9 +590,10 @@ impl WasmSdk { /// /// This method handles the complete update flow: /// 1. Validates the master key for signing - /// 2. Validates keys to add/disable - /// 3. Builds and signs the identity update transition - /// 4. Broadcasts and waits for confirmation + /// 2. Builds and signs the identity update transition; the builder refuses, before + /// signing, to disable the last valid authentication key of a security level + /// (DIP-11), counting keys added in the same update + /// 3. Broadcasts and waits for confirmation /// /// @param options - Update options including identity, keys to add/disable, and signer /// @returns Promise that resolves when the update is complete