From 191f729bcd1b936ec52c5e128c24e1c9df67d1bb Mon Sep 17 00:00:00 2001 From: Alexander Cyon <116169792+CyonAlexRDX@users.noreply.github.com> Date: Fri, 20 Dec 2024 14:49:41 +0100 Subject: [PATCH] Upgrade `ShieldBuilder` to validate `FactorSourceKind` for ROLA factors (#314) set_authentication_signing_factor now validates the FactorSourceKind, and we can query shield builder for which FactorSourceKinds are supported for it --- Cargo.lock | 4 +- crates/sargon-uniffi/Cargo.toml | 2 +- .../security_shield_builder.rs | 52 ++++++++++++ crates/sargon/Cargo.toml | 2 +- .../security_shield_builder.rs | 84 ++++++++++++++++++- .../v100/factors/factor_source_kind.rs | 22 +++-- 6 files changed, 156 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9f9f88052..719a7dba9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2761,7 +2761,7 @@ dependencies = [ [[package]] name = "sargon" -version = "1.1.93" +version = "1.1.94" dependencies = [ "actix-rt", "aes-gcm", @@ -2816,7 +2816,7 @@ dependencies = [ [[package]] name = "sargon-uniffi" -version = "1.1.93" +version = "1.1.94" dependencies = [ "actix-rt", "assert-json-diff", diff --git a/crates/sargon-uniffi/Cargo.toml b/crates/sargon-uniffi/Cargo.toml index 4bba6a821..f6305e707 100644 --- a/crates/sargon-uniffi/Cargo.toml +++ b/crates/sargon-uniffi/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "sargon-uniffi" # Don't forget to update version in crates/sargon/Cargo.toml -version = "1.1.93" +version = "1.1.94" edition = "2021" build = "build.rs" diff --git a/crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs b/crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs index 7077057a8..4504cdfa3 100644 --- a/crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs +++ b/crates/sargon-uniffi/src/profile/mfa/security_structures/security_shield_builder.rs @@ -252,6 +252,29 @@ impl SecurityShieldBuilder { #[uniffi::export] impl SecurityShieldBuilder { + /// "Statically" queries which FactorSourceKinds are disallowed for authentication signing. + pub fn disallowed_factor_source_kinds_for_authentication_signing( + &self, + ) -> Vec { + sargon::SecurityShieldBuilder::disallowed_factor_source_kinds_for_authentication_signing().into_type() + } + + /// "Statically" queries which FactorSourceKinds are allowed for authentication signing. + pub fn allowed_factor_source_kinds_for_authentication_signing( + &self, + ) -> Vec { + sargon::SecurityShieldBuilder::allowed_factor_source_kinds_for_authentication_signing().into_type() + } + + /// "Statically" queries if `factor_source_kind`` is allowed for authentication signing. + pub fn is_allowed_factor_source_kind_for_authentication_signing( + &self, + factor_source_kind: FactorSourceKind, + ) -> bool { + sargon::SecurityShieldBuilder::is_allowed_factor_source_kind_for_authentication_signing( + factor_source_kind.clone().into()) + } + pub fn addition_of_factor_source_of_kind_to_primary_threshold_is_fully_valid( &self, factor_source_kind: FactorSourceKind, @@ -561,6 +584,35 @@ mod tests { #[allow(clippy::upper_case_acronyms)] type SUT = SecurityShieldBuilder; + #[test] + fn rola() { + let sut = SUT::new(); + assert_eq!(sut.disallowed_factor_source_kinds_for_authentication_signing().len(), sargon::SecurityShieldBuilder::disallowed_factor_source_kinds_for_authentication_signing().len()); + + assert_eq!(sut.allowed_factor_source_kinds_for_authentication_signing().len(), sargon::SecurityShieldBuilder::allowed_factor_source_kinds_for_authentication_signing().len()); + + assert!( + sut.is_allowed_factor_source_kind_for_authentication_signing( + FactorSourceKind::Device + ) + ); + assert!( + !sut.is_allowed_factor_source_kind_for_authentication_signing( + FactorSourceKind::Password + ) + ); + assert!( + !sut.is_allowed_factor_source_kind_for_authentication_signing( + FactorSourceKind::TrustedContact + ) + ); + assert!( + !sut.is_allowed_factor_source_kind_for_authentication_signing( + FactorSourceKind::SecurityQuestions + ) + ); + } + #[test] fn test() { let sut = SUT::new(); diff --git a/crates/sargon/Cargo.toml b/crates/sargon/Cargo.toml index 10708a6ac..622891933 100644 --- a/crates/sargon/Cargo.toml +++ b/crates/sargon/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "sargon" # Don't forget to update version in crates/sargon-uniffi/Cargo.toml -version = "1.1.93" +version = "1.1.94" edition = "2021" build = "build.rs" diff --git a/crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs b/crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs index 5f4a63088..799ef8618 100644 --- a/crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs +++ b/crates/sargon/src/profile/mfa/security_structures/security_shield_builder.rs @@ -120,11 +120,26 @@ impl SecurityShieldBuilder { self } + /// Sets the ROLA (authentication signing) factor to `new` if and only if + /// `new` is not Some(invalid), where invalid is defined by `allowed_factor_source_kinds_for_authentication_signing`, + /// that is, it checks the `FactorSourceKind` of the factor, according to the + /// rules defined in [doc][doc]. + /// + /// [doc]: https://radixdlt.atlassian.net/wiki/spaces/AT/pages/3758063620/MFA+Rules+for+Factors+and+Security+Shield pub fn set_authentication_signing_factor( &self, new: impl Into>, ) -> &Self { - *self.authentication_signing_factor.write().unwrap() = new.into(); + let new = new.into(); + if let Some(new) = new.as_ref() { + if !Self::is_allowed_factor_source_kind_for_authentication_signing( + new.get_factor_source_kind(), + ) { + warn!("Invalid FactorSourceKind for ROLA"); + return self; + } + } + *self.authentication_signing_factor.write().unwrap() = new; self } @@ -295,6 +310,30 @@ impl SecurityShieldBuilder { } impl SecurityShieldBuilder { + pub fn disallowed_factor_source_kinds_for_authentication_signing( + ) -> IndexSet { + IndexSet::from_iter([ + FactorSourceKind::Password, + FactorSourceKind::SecurityQuestions, + FactorSourceKind::TrustedContact, + ]) + } + + pub fn allowed_factor_source_kinds_for_authentication_signing( + ) -> IndexSet { + let all = FactorSourceKind::all(); + let disallowed = + Self::disallowed_factor_source_kinds_for_authentication_signing(); + all.difference(&disallowed).cloned().collect() + } + + pub fn is_allowed_factor_source_kind_for_authentication_signing( + factor_source_kind: FactorSourceKind, + ) -> bool { + Self::allowed_factor_source_kinds_for_authentication_signing() + .contains(&factor_source_kind) + } + /// Returns `true` for `Ok` and `Err(NotYetValid)`. pub fn addition_of_factor_source_of_kind_to_primary_threshold_is_valid_or_can_be( &self, @@ -583,6 +622,49 @@ mod tests { assert_eq!(sut.get_threshold(), 42); } + #[test] + fn allowed_rola() { + let allowed = + SUT::allowed_factor_source_kinds_for_authentication_signing(); + assert_eq!( + allowed, + IndexSet::::from_iter([ + FactorSourceKind::LedgerHQHardwareWallet, + FactorSourceKind::ArculusCard, + FactorSourceKind::OffDeviceMnemonic, + FactorSourceKind::Device, + ]) + ); + } + + #[test] + fn is_allowed_rola() { + let disallowed = + SUT::disallowed_factor_source_kinds_for_authentication_signing(); + assert!(disallowed.iter().all(|k| { + !SUT::is_allowed_factor_source_kind_for_authentication_signing(*k) + })); + } + + #[test] + fn test_invalid_rola_kind_does_not_change_rola() { + let sut = SUT::new(); + assert!(sut.get_authentication_signing_factor().is_none()); + let valid = FactorSourceID::sample_device(); + sut.set_authentication_signing_factor(valid); + assert_eq!(sut.get_authentication_signing_factor().unwrap(), valid); + + let invalid_factors = vec![ + FactorSourceID::sample_password(), + FactorSourceID::sample_security_questions(), + FactorSourceID::sample_trusted_contact(), + ]; + for invalid in invalid_factors { + sut.set_authentication_signing_factor(invalid); // should not have changed anything + } + assert_eq!(sut.get_authentication_signing_factor().unwrap(), valid); + } + #[test] fn test() { let sut = SUT::default(); diff --git a/crates/sargon/src/profile/v100/factors/factor_source_kind.rs b/crates/sargon/src/profile/v100/factors/factor_source_kind.rs index 037f8a62e..c881b4ba3 100644 --- a/crates/sargon/src/profile/v100/factors/factor_source_kind.rs +++ b/crates/sargon/src/profile/v100/factors/factor_source_kind.rs @@ -11,6 +11,7 @@ use crate::prelude::*; Eq, Hash, PartialOrd, + enum_iterator::Sequence, Ord, )] pub enum FactorSourceKind { @@ -93,6 +94,13 @@ pub enum FactorSourceKind { TrustedContact, } +impl FactorSourceKind { + /// All FactorSourceKind + pub fn all() -> IndexSet { + enum_iterator::all::().collect() + } +} + impl FactorSourceKind { pub fn discriminant(&self) -> String { // We do `to_value.as_str` instead of `to_string(_pretty)` to avoid unwanted quotation marks around the string. @@ -168,6 +176,15 @@ mod tests { #[allow(clippy::upper_case_acronyms)] type SUT = FactorSourceKind; + #[test] + fn ord() { + assert!(SUT::Device < SUT::TrustedContact); + let unsorted = SUT::all(); // is in fact sorted + let mut sorted = unsorted.clone(); + sorted.sort(); + assert_eq!(unsorted, sorted); + } + #[test] fn string_roundtrip() { use FactorSourceKind::*; @@ -214,11 +231,6 @@ mod tests { ); } - #[test] - fn ord() { - assert!(SUT::Device < SUT::TrustedContact); - } - #[test] fn discriminant() { assert_eq!(SUT::Device.discriminant(), "device");