fix: [Traits for Authn] UserAuthenticator MUST verify credentials, not just let them validate themselves

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<DeviceId> for DeviceAttestation {
@@ -191,11 +226,11 @@ pub mod authenticator_b {
 
     impl UserChallengeResponse<DeviceId> 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 {

pallets/pass/src/tests.rs

@@ -212,11 +212,101 @@ 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::<Test>::CredentialInvalid
+            );
+        });
+    }
+
+    #[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::new(
+                        AccountNameA::get(),
+                        AuthenticatorB::generate(&AccountNameA::get()),
+                    )
+                    .sign(&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::new(
+                            AccountNameA::get(),
+                            AuthenticatorB::generate(&AccountNameA::get())
+                        )
+                        .sign(&OTHER_DEVICE)
+                    ),
+                    Some(DURATION),
+                ),
+                Error::<Test>::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::new(
+                            AccountNameA::get(),
+                            AuthenticatorB::generate(&AccountNameA::get()),
+                        )
+                        .sign(&OTHER_DEVICE)
+                    ),
                     Some(DURATION),
                 ),
                 Error::<Test>::CredentialInvalid
@@ -248,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::<Test>::SessionCreated {
+                    session_key: SIGNER,
+                    until: block_number + DURATION,
+                }
+                .into(),
+            );
+        });
+    }
 }
 
 mod add_device {

traits/authn/proc/src/lib.rs

@@ -161,14 +161,24 @@ 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),
                 #credential::#id(credential),
             ) => 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,
                 }
             }

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;
 }
 

traits/authn/src/util.rs

@@ -38,6 +38,10 @@ where
     }
 }
 
+pub trait VerifyCredential<Cred> {
+    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<T, A, Ch, Cred> Dev<T, A, Ch, Cred> {
 
 impl<T, A, Ch, Cred> UserAuthenticator for Dev<T, A, Ch, Cred>
 where
-    T: AsRef<DeviceId> + FullCodec + MaxEncodedLen + TypeInfo + 'static,
+    T: VerifyCredential<Cred> + AsRef<DeviceId> + FullCodec + MaxEncodedLen + TypeInfo + 'static,
     A: Get<AuthorityId> + 'static,
     Ch: Challenger + 'static,
     Cred: UserChallengeResponse<Ch::Context> + '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<T: MaxEncodedLen, A, Ch, Cr> MaxEncodedLen for Dev<T, A, Ch, Cr> {