From 0ba0e2f14cb9a05027c4aacec5d7b3a153584a30 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 22 Apr 2024 12:27:31 -0600 Subject: [PATCH] Only permit transparent-only Unified keys & addresses with associated metadata. --- .../src/data_api/testing/orchard.rs | 5 +- zcash_keys/CHANGELOG.md | 6 + zcash_keys/src/address.rs | 62 +++-- zcash_keys/src/keys.rs | 226 ++++++++++++++---- 4 files changed, 236 insertions(+), 63 deletions(-) diff --git a/zcash_client_backend/src/data_api/testing/orchard.rs b/zcash_client_backend/src/data_api/testing/orchard.rs index f90c66330..e00b842d7 100644 --- a/zcash_client_backend/src/data_api/testing/orchard.rs +++ b/zcash_client_backend/src/data_api/testing/orchard.rs @@ -75,6 +75,7 @@ impl ShieldedPoolTester for OrchardPoolTester { None, None, ) + .expect("Address construction is valid") .into() } @@ -154,7 +155,9 @@ impl ShieldedPoolTester for OrchardPoolTester { return result.map(|(note, addr, memo)| { ( Note::Orchard(note), - UnifiedAddress::from_receivers(Some(addr), None, None).into(), + UnifiedAddress::from_receivers(Some(addr), None, None) + .expect("Address construction is valid") + .into(), MemoBytes::from_bytes(&memo).expect("correct length"), ) }); diff --git a/zcash_keys/CHANGELOG.md b/zcash_keys/CHANGELOG.md index 481d9726e..3fcbf044f 100644 --- a/zcash_keys/CHANGELOG.md +++ b/zcash_keys/CHANGELOG.md @@ -6,9 +6,15 @@ and this library adheres to Rust's notion of ## [Unreleased] +### Added +- `zcash_keys::keys::UnifiedKeyError` + ### Changed - MSRV is now 1.77.0. +### Removed +- `zcash_keys::keys::DerivationError` use `UnifiedKeyError` instead. + ## [0.4.0] - 2024-10-04 ### Added diff --git a/zcash_keys/src/address.rs b/zcash_keys/src/address.rs index 541b96edf..df81a6f03 100644 --- a/zcash_keys/src/address.rs +++ b/zcash_keys/src/address.rs @@ -93,7 +93,7 @@ impl TryFrom for UnifiedAddress { } } - Ok(Self { + Self::from_checked_parts( #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] @@ -103,7 +103,7 @@ impl TryFrom for UnifiedAddress { expiry_height, expiry_time, unknown_metadata, - }) + ).ok_or("Transparent-only unified addresses without additional metadata fields are not permitted.") } } @@ -118,36 +118,55 @@ impl UnifiedAddress { #[cfg(feature = "orchard")] orchard: Option, #[cfg(feature = "sapling")] sapling: Option, transparent: Option, - ) -> Self { - Self::new_internal( + ) -> Option { + Self::from_checked_parts( #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] sapling, transparent, + vec![], None, None, + vec![], ) } - pub(crate) fn new_internal( + pub(crate) fn from_checked_parts( #[cfg(feature = "orchard")] orchard: Option, #[cfg(feature = "sapling")] sapling: Option, transparent: Option, + unknown_data: Vec<(u32, Vec)>, expiry_height: Option, expiry_time: Option, - ) -> Self { - Self { + unknown_metadata: Vec<(u32, Vec)>, + ) -> Option { + #[allow(unused_mut)] + let mut has_shielded = false; + #[cfg(feature = "sapling")] + { + has_shielded = has_shielded || sapling.is_some(); + } + #[cfg(feature = "orchard")] + { + has_shielded = has_shielded || orchard.is_some(); + } + + let has_unknown = !unknown_data.is_empty(); + let has_metadata = + expiry_height.is_some() || expiry_time.is_some() || !unknown_metadata.is_empty(); + + (has_shielded || has_unknown || (transparent.is_some() && has_metadata)).then(|| Self { #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] sapling, transparent, - unknown_data: vec![], + unknown_data, expiry_height, expiry_time, - unknown_metadata: vec![], - } + unknown_metadata, + }) } /// Returns whether this address has an Orchard receiver. @@ -592,15 +611,25 @@ mod tests { let transparent = None; #[cfg(all(feature = "orchard", feature = "sapling"))] - let ua = UnifiedAddress::new_internal(orchard, sapling, transparent, None, None); + let ua = UnifiedAddress::from_checked_parts( + orchard, + sapling, + transparent, + vec![], + None, + None, + vec![], + ); #[cfg(all(not(feature = "orchard"), feature = "sapling"))] - let ua = UnifiedAddress::new_internal(sapling, transparent, None, None); + let ua = + UnifiedAddress::from_checked_parts(sapling, transparent, vec![], None, None, vec![]); #[cfg(all(feature = "orchard", not(feature = "sapling")))] - let ua = UnifiedAddress::new_internal(orchard, transparent, None, None); + let ua = + UnifiedAddress::from_checked_parts(orchard, transparent, vec![], None, None, vec![]); - let addr = Address::from(ua); + let addr = Address::from(ua.expect("test UAs are constructed in valid configurations")); let addr_str = addr.encode(&MAIN_NETWORK); assert_eq!(Address::decode(&MAIN_NETWORK, &addr_str), Some(addr)); } @@ -609,7 +638,10 @@ mod tests { #[cfg(not(any(feature = "orchard", feature = "sapling")))] fn ua_round_trip() { let transparent = None; - assert_eq!(UnifiedAddress::new_internal(transparent, None, None), None) + assert_eq!( + UnifiedAddress::from_checked_parts(transparent, vec![], None, None, vec![]), + None + ) } #[test] diff --git a/zcash_keys/src/keys.rs b/zcash_keys/src/keys.rs index aad412543..106b9e947 100644 --- a/zcash_keys/src/keys.rs +++ b/zcash_keys/src/keys.rs @@ -92,29 +92,38 @@ fn to_transparent_child_index(j: DiversifierIndex) -> Option) -> fmt::Result { match self { #[cfg(feature = "orchard")] - DerivationError::Orchard(e) => write!(_f, "Orchard error: {}", e), + UnifiedKeyError::Orchard(e) => write!(_f, "Orchard key derivation error: {}", e), + #[cfg(feature = "transparent-inputs")] + UnifiedKeyError::Transparent(e) => { + write!(_f, "Transparent key derivation error: {}", e) + } #[cfg(feature = "transparent-inputs")] - DerivationError::Transparent(e) => write!(_f, "Transparent error: {}", e), + UnifiedKeyError::TransparentOnlyNotSupported => write!( + _f, + "Unified keys without shielded receivers must contain metadata items." + ), #[cfg(not(any(feature = "orchard", feature = "transparent-inputs")))] other => { - unreachable!("Unhandled DerivationError variant {:?}", other) + unreachable!("Unhandled UnifiedKeyError variant {:?}", other) } } } } -impl std::error::Error for DerivationError {} +impl std::error::Error for UnifiedKeyError {} /// A version identifier for the encoding of unified spending keys. /// @@ -147,8 +156,11 @@ pub enum DecodingError { LengthMismatch(Typecode, u32), #[cfg(feature = "unstable")] InsufficientData(Typecode), - /// The key data could not be decoded from its string representation to a valid key. + /// The key data for the given key type could not be decoded from its string representation to + /// a valid key. KeyDataInvalid(Typecode), + /// Decoding resulted in a value that would violate validity constraints. + ConstraintViolation(String), } impl std::fmt::Display for DecodingError { @@ -177,12 +189,35 @@ impl std::fmt::Display for DecodingError { write!(f, "Insufficient data for typecode {:?}", t) } DecodingError::KeyDataInvalid(t) => write!(f, "Invalid key data for key type {:?}", t), + DecodingError::ConstraintViolation(s) => { + write!(f, "Decoding produced an invalid value: {}", s) + } } } } impl std::error::Error for DecodingError {} +impl From for DecodingError { + fn from(value: UnifiedKeyError) -> Self { + match value { + #[cfg(feature = "orchard")] + UnifiedKeyError::Orchard(_) => { + Self::KeyDataInvalid(Typecode::Data(unified::DataTypecode::Orchard)) + } + #[cfg(feature = "transparent-inputs")] + UnifiedKeyError::Transparent(_) => { + Self::KeyDataInvalid(Typecode::Data(unified::DataTypecode::P2pkh)) + } + #[cfg(feature = "transparent-inputs")] + UnifiedKeyError::TransparentOnlyNotSupported => Self::ConstraintViolation( + "Transparent-only Unified Containers must contain additional address metadata." + .to_owned(), + ), + } + } +} + #[cfg(feature = "unstable")] impl Era { /// Returns the unique identifier for the era. @@ -218,7 +253,7 @@ impl UnifiedSpendingKey { _params: &P, seed: &[u8], _account: AccountId, - ) -> Result { + ) -> Result { if seed.len() < 32 { panic!("ZIP 32 seeds MUST be at least 32 bytes"); } @@ -226,12 +261,12 @@ impl UnifiedSpendingKey { UnifiedSpendingKey::from_checked_parts( #[cfg(feature = "transparent-inputs")] legacy::AccountPrivKey::from_seed(_params, seed, _account) - .map_err(DerivationError::Transparent)?, + .map_err(UnifiedKeyError::Transparent)?, #[cfg(feature = "sapling")] sapling::spending_key(seed, _params.coin_type(), _account), #[cfg(feature = "orchard")] orchard::keys::SpendingKey::from_zip32_seed(seed, _params.coin_type(), _account) - .map_err(DerivationError::Orchard)?, + .map_err(UnifiedKeyError::Orchard)?, ) } @@ -241,11 +276,19 @@ impl UnifiedSpendingKey { #[cfg(feature = "transparent-inputs")] transparent: legacy::AccountPrivKey, #[cfg(feature = "sapling")] sapling: sapling::ExtendedSpendingKey, #[cfg(feature = "orchard")] orchard: orchard::keys::SpendingKey, - ) -> Result { + ) -> Result { // Verify that FVK and IVK derivation succeed; we don't want to construct a USK // that can't derive transparent addresses. #[cfg(feature = "transparent-inputs")] - let _ = transparent.to_account_pubkey().derive_external_ivk()?; + { + let _ = transparent.to_account_pubkey().derive_external_ivk()?; + + // We don't permit the construction of transparent-only USKs, because we can't derive + // U*VKs or addresses from them. + if !(cfg!(feature = "sapling") || cfg!(feature = "orchard")) { + return Err(UnifiedKeyError::TransparentOnlyNotSupported); + } + } Ok(UnifiedSpendingKey { #[cfg(feature = "transparent-inputs")] @@ -458,7 +501,7 @@ impl UnifiedSpendingKey { #[cfg(feature = "orchard")] orchard.unwrap(), ) - .map_err(|_| DecodingError::KeyDataInvalid(Typecode::P2PKH)); + .map_err(DecodingError::from); } } } @@ -574,7 +617,9 @@ impl UnifiedAddressRequest { expiry_height: Option, expiry_time: Option, ) -> Option { - if !(has_sapling || has_orchard || has_p2pkh) { + let has_shielded = has_sapling || has_orchard; + let has_metadata = expiry_height.is_some() && expiry_time.is_some(); + if !(has_shielded || (has_p2pkh && has_metadata)) { None } else { Some(Self { @@ -627,14 +672,15 @@ impl UnifiedAddressRequest { /// Construct a new unified address request from its constituent parts. /// - /// Panics: at least one of `has_orchard`, `has_sapling`, or `has_p2pkh` must be `true`. + /// Panics: at least one of `has_orchard` or `has_sapling` must be `true`. P2pkh-only + /// addresses without additional metadata are currently disallowed by ZIP 316. pub const fn unsafe_new_without_expiry( has_orchard: bool, has_sapling: bool, has_p2pkh: bool, ) -> Self { - if !(has_orchard || has_sapling || has_p2pkh) { - panic!("At least one receiver must be requested.") + if !(has_orchard || has_sapling) { + panic!("At least one shielded receiver must be requested.") } Self { @@ -648,9 +694,9 @@ impl UnifiedAddressRequest { } #[cfg(feature = "transparent-inputs")] -impl From for DerivationError { +impl From for UnifiedKeyError { fn from(e: bip32::Error) -> Self { - DerivationError::Transparent(e) + UnifiedKeyError::Transparent(e) } } @@ -681,7 +727,7 @@ impl UnifiedFullViewingKey { #[cfg(feature = "sapling")] sapling: Option, #[cfg(feature = "orchard")] orchard: Option, // TODO: Implement construction of UFVKs with metadata items. - ) -> Result { + ) -> Result { Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] transparent, @@ -701,7 +747,7 @@ impl UnifiedFullViewingKey { #[cfg(feature = "unstable-frost")] pub fn from_orchard_fvk( orchard: orchard::keys::FullViewingKey, - ) -> Result { + ) -> Result { Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] None, @@ -721,7 +767,7 @@ impl UnifiedFullViewingKey { #[cfg(all(feature = "sapling", feature = "unstable"))] pub fn from_sapling_extended_full_viewing_key( sapling: ExtendedFullViewingKey, - ) -> Result { + ) -> Result { Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] None, @@ -748,16 +794,41 @@ impl UnifiedFullViewingKey { expiry_height: Option, expiry_time: Option, unknown_metadata: Vec<(u32, Vec)>, - ) -> Result { + ) -> Result { // Verify that IVK derivation succeeds; we don't want to construct a UFVK // that can't derive transparent addresses. #[cfg(feature = "transparent-inputs")] - let _ = transparent - .as_ref() - .map(|t| t.derive_external_ivk()) - .transpose()?; + { + let _ = transparent + .as_ref() + .map(|t| t.derive_external_ivk()) + .transpose()?; + + if transparent.is_some() { + #[allow(unused_mut)] + let mut has_shielded = false; + #[cfg(feature = "sapling")] + { + has_shielded = has_shielded || sapling.is_some(); + } + #[cfg(feature = "orchard")] + { + has_shielded = has_shielded || orchard.is_some(); + } + + // a transparent address must be accompanied either by another data or metadata item. + if !has_shielded + && expiry_height.is_none() + && expiry_time.is_none() + && unknown_data.is_empty() + && unknown_metadata.is_empty() + { + return Err(UnifiedKeyError::TransparentOnlyNotSupported); + } + } + } - Ok(UnifiedFullViewingKey { + Ok(Self { #[cfg(feature = "transparent-inputs")] transparent, #[cfg(feature = "sapling")] @@ -1102,6 +1173,55 @@ impl UnifiedIncomingViewingKey { } } + fn from_checked_parts( + #[cfg(feature = "transparent-inputs")] transparent: Option< + zcash_primitives::legacy::keys::ExternalIvk, + >, + #[cfg(feature = "sapling")] sapling: Option<::sapling::zip32::IncomingViewingKey>, + #[cfg(feature = "orchard")] orchard: Option, + unknown_data: Vec<(u32, Vec)>, + expiry_height: Option, + expiry_time: Option, + unknown_metadata: Vec<(u32, Vec)>, + ) -> Result { + // Verify that IVK derivation succeeds; we don't want to construct a UFVK + // that can't derive transparent addresses. + #[cfg(feature = "transparent-inputs")] + if transparent.is_some() { + #[allow(unused_mut)] + let mut has_shielded = false; + #[cfg(feature = "sapling")] + { + has_shielded = has_shielded || sapling.is_some(); + } + #[cfg(feature = "orchard")] + { + has_shielded = has_shielded || orchard.is_some(); + } + + if !has_shielded + && expiry_height.is_none() + && expiry_time.is_none() + && unknown_metadata.is_empty() + { + return Err(UnifiedKeyError::TransparentOnlyNotSupported); + } + } + + Ok(Self { + #[cfg(feature = "transparent-inputs")] + transparent, + #[cfg(feature = "sapling")] + sapling, + #[cfg(feature = "orchard")] + orchard, + unknown_data, + expiry_height, + expiry_time, + unknown_metadata, + }) + } + /// Parses a `UnifiedFullViewingKey` from its [ZIP 316] string encoding. /// /// [ZIP 316]: https://zips.z.cash/zip-0316 @@ -1186,7 +1306,7 @@ impl UnifiedIncomingViewingKey { } } - Ok(Self { + Ok(Self::from_checked_parts( #[cfg(feature = "transparent-inputs")] transparent, #[cfg(feature = "sapling")] @@ -1197,7 +1317,7 @@ impl UnifiedIncomingViewingKey { expiry_height, expiry_time, unknown_metadata, - }) + )?) } /// Returns the string encoding of this `UnifiedFullViewingKey` for the given network. @@ -1376,15 +1496,18 @@ impl UnifiedIncomingViewingKey { #[cfg(not(feature = "transparent-inputs"))] let transparent = None; - Ok(UnifiedAddress::new_internal( + Ok(UnifiedAddress::from_checked_parts( #[cfg(feature = "orchard")] orchard, #[cfg(feature = "sapling")] sapling, transparent, + self.unknown_data.clone(), std::cmp::min(self.expiry_height, request.expiry_height), std::cmp::min(self.expiry_time, request.expiry_time), - )) + self.unknown_metadata.clone(), + ) + .expect("UIVK validity constraints are checked at construction.")) } /// Searches the diversifier space starting at diversifier index `j` for one which will @@ -1395,7 +1518,7 @@ impl UnifiedIncomingViewingKey { /// required to satisfy the unified address request are not properly enabled. pub fn find_address( &self, - mut j: DiversifierIndex, + mut _j: DiversifierIndex, request: UnifiedAddressRequest, ) -> Result<(UnifiedAddress, DiversifierIndex), AddressGenerationError> { // If we need to generate a transparent receiver, check that the user has not @@ -1404,21 +1527,21 @@ impl UnifiedIncomingViewingKey { #[cfg(feature = "transparent-inputs")] if request.has_p2pkh && self.transparent.is_some() - && to_transparent_child_index(j).is_none() + && to_transparent_child_index(_j).is_none() { - return Err(AddressGenerationError::InvalidTransparentChildIndex(j)); + return Err(AddressGenerationError::InvalidTransparentChildIndex(_j)); } // Find a working diversifier and construct the associated address. loop { - let res = self.address(j, request); + let res = self.address(_j, request); match res { Ok(ua) => { - return Ok((ua, j)); + return Ok((ua, _j)); } #[cfg(feature = "sapling")] Err(AddressGenerationError::InvalidSaplingDiversifierIndex(_)) => { - if j.increment().is_err() { + if _j.increment().is_err() { return Err(AddressGenerationError::DiversifierSpaceExhausted); } else { continue; @@ -1514,13 +1637,11 @@ mod tests { #[cfg(feature = "transparent-inputs")] use { - crate::{address::Address, encoding::AddressCodec}, - zcash_address::test_vectors, + crate::encoding::AddressCodec, zcash_primitives::legacy::keys::{AccountPrivKey, IncomingViewingKey}, - zip32::DiversifierIndex, }; - #[cfg(feature = "unstable")] + #[cfg(all(feature = "unstable", any(feature = "sapling", feature = "orchard")))] use super::{testing::arb_unified_spending_key, Era, UnifiedSpendingKey}; #[cfg(all(feature = "orchard", feature = "unstable"))] @@ -1677,9 +1798,14 @@ mod tests { } #[test] - #[cfg(feature = "transparent-inputs")] + #[cfg(all( + feature = "transparent-inputs", + any(feature = "orchard", feature = "sapling") + ))] fn ufvk_derivation() { - use crate::keys::UnifiedAddressRequest; + use crate::{address::Address, keys::UnifiedAddressRequest}; + use zcash_address::test_vectors; + use zip32::DiversifierIndex; use super::UnifiedSpendingKey; @@ -1863,9 +1989,14 @@ mod tests { } #[test] - #[cfg(feature = "transparent-inputs")] + #[cfg(all( + feature = "transparent-inputs", + any(feature = "sapling", feature = "orchard") + ))] fn uivk_derivation() { - use crate::keys::UnifiedAddressRequest; + use crate::{address::Address, keys::UnifiedAddressRequest}; + use zcash_address::test_vectors; + use zip32::DiversifierIndex; use super::UnifiedSpendingKey; @@ -1925,7 +2056,7 @@ mod tests { proptest! { #[test] - #[cfg(feature = "unstable")] + #[cfg(all(feature = "unstable", any(feature = "orchard", feature = "sapling")))] fn prop_usk_roundtrip(usk in arb_unified_spending_key(zcash_protocol::consensus::Network::MainNetwork)) { let encoded = usk.to_bytes(Era::Orchard); @@ -1954,6 +2085,7 @@ mod tests { #[cfg(feature = "orchard")] assert!(bool::from(decoded.orchard().ct_eq(usk.orchard()))); + #[cfg(feature = "sapling")] assert_eq!(decoded.sapling(), usk.sapling()); #[cfg(feature = "transparent-inputs")]