From a3a8644735ab4dce0d3d06a31080216f8b948839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Sat, 12 Oct 2024 22:30:04 -0500 Subject: [PATCH 1/4] change(fc-pallet-pass): add tests to assert edge cases for authentication --- pallets/pass/src/tests.rs | 85 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/pallets/pass/src/tests.rs b/pallets/pass/src/tests.rs index 2bfd23d..61e09cf 100644 --- a/pallets/pass/src/tests.rs +++ b/pallets/pass/src/tests.rs @@ -224,6 +224,91 @@ mod authenticate { }); } + #[test] + fn fail_if_attested_with_credentials_from_a_different_device() { + new_test_ext().execute_with(|| { + assert_ok!(Balances::mint_into(&SIGNER, 2)); + + assert_ok!(Pass::register( + RuntimeOrigin::signed(SIGNER), + AccountNameA::get(), + PassDeviceAttestation::AuthenticatorB(authenticator_b::DeviceAttestation { + device_id: THE_DEVICE, + challenge: AuthenticatorB::generate(&THE_DEVICE), + }), + )); + assert_ok!(Pass::authenticate( + RuntimeOrigin::signed(SIGNER), + THE_DEVICE, + PassCredential::AuthenticatorB(authenticator_b::Credential { + user_id: AccountNameA::get(), + device_id: THE_DEVICE, + challenge: AuthenticatorB::generate(&THE_DEVICE), + }), + None, + )); + assert_ok!(Pass::add_device( + RuntimeOrigin::signed(SIGNER), + PassDeviceAttestation::AuthenticatorB(authenticator_b::DeviceAttestation { + device_id: OTHER_DEVICE, + challenge: AuthenticatorB::generate(&OTHER_DEVICE), + }), + )); + + assert_noop!( + Pass::authenticate( + RuntimeOrigin::signed(OTHER), + THE_DEVICE, + PassCredential::AuthenticatorB(authenticator_b::Credential { + user_id: AccountNameA::get(), + device_id: OTHER_DEVICE, + challenge: AuthenticatorB::generate(&OTHER_DEVICE), + }), + Some(DURATION), + ), + Error::::CredentialInvalid + ); + }); + } + + #[test] + fn fail_if_attested_with_credentials_from_a_different_user_device() { + new_test_ext().execute_with(|| { + assert_ok!(Balances::mint_into(&SIGNER, 2)); + + assert_ok!(Pass::register( + RuntimeOrigin::signed(SIGNER), + AccountNameA::get(), + PassDeviceAttestation::AuthenticatorB(authenticator_b::DeviceAttestation { + device_id: THE_DEVICE, + challenge: AuthenticatorB::generate(&THE_DEVICE), + }), + )); + assert_ok!(Pass::register( + RuntimeOrigin::signed(SIGNER), + AccountNameB::get(), + PassDeviceAttestation::AuthenticatorB(authenticator_b::DeviceAttestation { + device_id: OTHER_DEVICE, + challenge: AuthenticatorB::generate(&OTHER_DEVICE), + }), + )); + + assert_noop!( + Pass::authenticate( + RuntimeOrigin::signed(OTHER), + THE_DEVICE, + PassCredential::AuthenticatorB(authenticator_b::Credential { + user_id: AccountNameA::get(), + device_id: OTHER_DEVICE, + challenge: AuthenticatorB::generate(&OTHER_DEVICE), + }), + Some(DURATION), + ), + Error::::CredentialInvalid + ); + }); + } + #[test] fn it_works() { prepare(AccountNameA::get()).execute_with(|| { From f588a5048917cbd6926e6483a75170ba7755af50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Sat, 12 Oct 2024 23:13:23 -0500 Subject: [PATCH 2/4] fix(fc-pallet-pass): ensure credentials are verified by the device, not just by themselves --- pallets/pass/src/mock/authenticators.rs | 41 ++++++++++++++++++++-- pallets/pass/src/tests.rs | 45 ++++++++++++++----------- traits/authn/proc/src/lib.rs | 21 ++++++++++-- traits/authn/src/lib.rs | 5 ++- traits/authn/src/util.rs | 10 +++++- 5 files changed, 95 insertions(+), 27 deletions(-) diff --git a/pallets/pass/src/mock/authenticators.rs b/pallets/pass/src/mock/authenticators.rs index d87152d..5bec740 100644 --- a/pallets/pass/src/mock/authenticators.rs +++ b/pallets/pass/src/mock/authenticators.rs @@ -75,6 +75,11 @@ pub mod authenticator_a { fn device_id(&self) -> &DeviceId { &self.device_id } + + // Note: This authenticator should pass intentionally, to pass on simpler tests + fn verify_credential(&self, _: &Self::Credential) -> Option<()> { + Some(()) + } } impl DeviceChallengeResponse<()> for DeviceAttestation { @@ -134,9 +139,31 @@ pub mod authenticator_b { TypeInfo, DebugNoBound, EqNoBound, PartialEq, Clone, Encode, Decode, MaxEncodedLen, )] pub struct Credential { - pub(crate) device_id: DeviceId, pub(crate) user_id: HashedUserId, pub(crate) challenge: Challenge, + pub(crate) signature: Option<[u8; 32]>, + } + + impl Credential { + pub fn new(user_id: HashedUserId, challenge: Challenge) -> Self { + Self { + user_id, + challenge, + signature: None, + } + } + + pub fn sign(self, signer: &DeviceId) -> Self { + Self { + signature: Some(Self::signature(signer, &self)), + ..self + } + } + + // A dummy "signature", to test the signing capabilities + pub fn signature(signer: &DeviceId, credential: &Self) -> [u8; 32] { + blake2_256(&(signer, credential.user_id, credential.challenge).encode()) + } } impl Authenticator for AuthenticatorB { @@ -169,6 +196,14 @@ pub mod authenticator_b { fn device_id(&self) -> &DeviceId { &self.device_id } + + fn verify_credential(&self, credential: &Self::Credential) -> Option<()> { + credential.signature.and_then(|signature| { + Credential::signature(self.device_id(), &credential) + .eq(&signature) + .then_some(()) + }) + } } impl DeviceChallengeResponse for DeviceAttestation { @@ -191,11 +226,11 @@ pub mod authenticator_b { impl UserChallengeResponse for Credential { fn is_valid(&self) -> bool { - true + self.signature.is_some() } fn used_challenge(&self) -> (DeviceId, Challenge) { - (self.device_id, self.challenge) + (self.user_id, self.challenge) } fn authority(&self) -> AuthorityId { diff --git a/pallets/pass/src/tests.rs b/pallets/pass/src/tests.rs index 61e09cf..662f24c 100644 --- a/pallets/pass/src/tests.rs +++ b/pallets/pass/src/tests.rs @@ -212,11 +212,10 @@ mod authenticate { Pass::authenticate( RuntimeOrigin::signed(OTHER), THE_DEVICE, - PassCredential::AuthenticatorB(authenticator_b::Credential { - user_id: AccountNameA::get(), - device_id: THE_DEVICE, - challenge: AuthenticatorB::generate(&OTHER_DEVICE), - }), + PassCredential::AuthenticatorB(authenticator_b::Credential::new( + AccountNameA::get(), + AuthenticatorB::generate(&OTHER_DEVICE) + )), Some(DURATION), ), Error::::CredentialInvalid @@ -240,11 +239,13 @@ mod authenticate { assert_ok!(Pass::authenticate( RuntimeOrigin::signed(SIGNER), THE_DEVICE, - PassCredential::AuthenticatorB(authenticator_b::Credential { - user_id: AccountNameA::get(), - device_id: THE_DEVICE, - challenge: AuthenticatorB::generate(&THE_DEVICE), - }), + PassCredential::AuthenticatorB( + authenticator_b::Credential::new( + AccountNameA::get(), + AuthenticatorB::generate(&AccountNameA::get()), + ) + .sign(&THE_DEVICE) + ), None, )); assert_ok!(Pass::add_device( @@ -259,11 +260,13 @@ mod authenticate { Pass::authenticate( RuntimeOrigin::signed(OTHER), THE_DEVICE, - PassCredential::AuthenticatorB(authenticator_b::Credential { - user_id: AccountNameA::get(), - device_id: OTHER_DEVICE, - challenge: AuthenticatorB::generate(&OTHER_DEVICE), - }), + PassCredential::AuthenticatorB( + authenticator_b::Credential::new( + AccountNameA::get(), + AuthenticatorB::generate(&AccountNameA::get()) + ) + .sign(&OTHER_DEVICE) + ), Some(DURATION), ), Error::::CredentialInvalid @@ -297,11 +300,13 @@ mod authenticate { Pass::authenticate( RuntimeOrigin::signed(OTHER), THE_DEVICE, - PassCredential::AuthenticatorB(authenticator_b::Credential { - user_id: AccountNameA::get(), - device_id: OTHER_DEVICE, - challenge: AuthenticatorB::generate(&OTHER_DEVICE), - }), + PassCredential::AuthenticatorB( + authenticator_b::Credential::new( + AccountNameA::get(), + AuthenticatorB::generate(&AccountNameA::get()), + ) + .sign(&OTHER_DEVICE) + ), Some(DURATION), ), Error::::CredentialInvalid diff --git a/traits/authn/proc/src/lib.rs b/traits/authn/proc/src/lib.rs index e8c7666..efc61a2 100644 --- a/traits/authn/proc/src/lib.rs +++ b/traits/authn/proc/src/lib.rs @@ -161,7 +161,7 @@ pub fn composite_authenticator(input: TokenStream) -> TokenStream { } }); - let match_credentials = authenticators.clone().into_iter().map(|(id, _)| { + let match_verify_user = authenticators.clone().into_iter().map(|(id, _)| { quote! { ( #device::#id(device), @@ -169,6 +169,16 @@ pub fn composite_authenticator(input: TokenStream) -> TokenStream { ) => device.verify_user(credential) } }); + + let match_verify_credential = authenticators.clone().into_iter().map(|(id, _)| { + quote! { + ( + #device::#id(device), + #credential::#id(credential), + ) => device.verify_credential(credential) + } + }); + let match_user_id = authenticators.clone().into_iter().map(|(id, _)| { quote! { #credential::#id(credential) => credential.user_id() @@ -252,7 +262,14 @@ pub fn composite_authenticator(input: TokenStream) -> TokenStream { fn verify_user(&self, credential: &Self::Credential) -> Option<()> { match (self, credential) { - #(#match_credentials),*, + #(#match_verify_user),*, + _ => None, + } + } + + fn verify_credential(&self, credential: &Self::Credential) -> Option<()> { + match (self, credential) { + #(#match_verify_credential),*, _ => None, } } diff --git a/traits/authn/src/lib.rs b/traits/authn/src/lib.rs index 2527171..3845d22 100644 --- a/traits/authn/src/lib.rs +++ b/traits/authn/src/lib.rs @@ -89,9 +89,12 @@ pub trait UserAuthenticator: FullCodec + MaxEncodedLen + TypeInfo { .then_some(())?; let (cx, challenge) = credential.used_challenge(); Self::Challenger::check_challenge(&cx, &challenge)?; - credential.is_valid().then_some(()) + credential.is_valid().then_some(())?; + self.verify_credential(credential) } + fn verify_credential(&self, credential: &Self::Credential) -> Option<()>; + fn device_id(&self) -> &DeviceId; } diff --git a/traits/authn/src/util.rs b/traits/authn/src/util.rs index 779c533..1f18e44 100644 --- a/traits/authn/src/util.rs +++ b/traits/authn/src/util.rs @@ -38,6 +38,10 @@ where } } +trait VerifyCredential { + fn verify(&self, credential: &Cred) -> Option<()>; +} + /// Convenient auto-implemtator of the UserAuthenticator trait #[derive(Encode, Decode, TypeInfo)] #[scale_info(skip_type_params(A, Ch, Cred))] @@ -51,7 +55,7 @@ impl Dev { impl UserAuthenticator for Dev where - T: AsRef + FullCodec + MaxEncodedLen + TypeInfo + 'static, + T: VerifyCredential + AsRef + FullCodec + MaxEncodedLen + TypeInfo + 'static, A: Get + 'static, Ch: Challenger + 'static, Cred: UserChallengeResponse + 'static, @@ -63,6 +67,10 @@ where fn device_id(&self) -> &DeviceId { self.0.as_ref() } + + fn verify_credential(&self, credential: &Self::Credential) -> Option<()> { + self.0.verify(credential) + } } impl MaxEncodedLen for Dev { From 7ea6ae7a38cd6d2e886e612b9c6b1e3ccc2e3c32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Sat, 12 Oct 2024 23:36:25 -0500 Subject: [PATCH 3/4] change(fc-pallet-pass): add test to assert non-trivial `verify_credential` case --- pallets/pass/src/tests.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/pallets/pass/src/tests.rs b/pallets/pass/src/tests.rs index 662f24c..ddf7f18 100644 --- a/pallets/pass/src/tests.rs +++ b/pallets/pass/src/tests.rs @@ -338,6 +338,44 @@ mod authenticate { ); }); } + + #[test] + fn verify_credential_works() { + new_test_ext().execute_with(|| { + assert_ok!(Balances::mint_into(&SIGNER, 2)); + + assert_ok!(Pass::register( + RuntimeOrigin::signed(SIGNER), + AccountNameA::get(), + PassDeviceAttestation::AuthenticatorB(authenticator_b::DeviceAttestation { + device_id: THE_DEVICE, + challenge: AuthenticatorB::generate(&THE_DEVICE), + }), + )); + + assert_ok!(Pass::authenticate( + RuntimeOrigin::signed(SIGNER), + THE_DEVICE, + PassCredential::AuthenticatorB( + authenticator_b::Credential::new( + AccountNameA::get(), + AuthenticatorB::generate(&AccountNameA::get()) + ) + .sign(&THE_DEVICE) + ), + Some(DURATION), + )); + + let block_number = System::block_number(); + System::assert_has_event( + Event::::SessionCreated { + session_key: SIGNER, + until: block_number + DURATION, + } + .into(), + ); + }); + } } mod add_device { From cf37cd211d303a111d2618e70c79105333118fdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Sat, 12 Oct 2024 23:44:46 -0500 Subject: [PATCH 4/4] fix(fc-traits-authn): auxiliary trait `VerifyCredential` should be public --- traits/authn/src/util.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/traits/authn/src/util.rs b/traits/authn/src/util.rs index 1f18e44..c97a2ef 100644 --- a/traits/authn/src/util.rs +++ b/traits/authn/src/util.rs @@ -38,7 +38,7 @@ where } } -trait VerifyCredential { +pub trait VerifyCredential { fn verify(&self, credential: &Cred) -> Option<()>; }