Merge pull request #51 from 1Password/progdrasil/rust-and-clippy-updates

Updates from internal usage

Author: Progdrasil

CHANGELOG.md

@@ -1,7 +1,23 @@
 # Changelog
 
 ## Unreleased
-- Added: support for controlling generated credential's ID length to passkey-authenticator ([#49](https://github.com/1Password/passkey-rs/pull/49))
+
+## Passkey v0.4.0
+### passkey-authenticator v0.4.0
+
+- Added: support for controlling generated credential's ID length to Authenticator ([#49](https://github.com/1Password/passkey-rs/pull/49))
+- ⚠ BREAKING: Removal of `Authenticator::set_display_name` and `Authenticator::display_name` methods ([#51](https://github.com/1Password/passkey-rs/pull/51))
+
+### passkey-client v0.4.0
+- ⚠ BREAKING: Update android asset link verification ([#51](https://github.com/1Password/passkey-rs/pull/51))
+  - Change `asset_link_url` parameter in `UnverifiedAssetLink::new` to be required rather than optional.
+  - Remove internal string in `ValidationError::InvalidAssetLinkUrl` variant.
+- Remove special casing of responses for specific RPs ([#51](https://github.com/1Password/passkey-rs/pull/51))
+- Added `RpIdValidator::is_valid_rp_id` to verify that an rp_id is valid to be used as such ([#51](https://github.com/1Password/passkey-rs/pull/51))
+
+### passkey-types v0.4.0
+- ⚠ BREAKING: Removal of `CredentialPropertiesOutput::authenticator_display_name` ([#51](https://github.com/1Password/passkey-rs/pull/51))
+
 
 ## Passkey v0.3.0
 ### passkey-authenticator v0.3.0

passkey-authenticator/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "passkey-authenticator"
-version = "0.3.0"
+version = "0.4.0"
 description = "A webauthn authenticator supporting passkeys."
 include = ["src/", "../LICENSE-APACHE", "../LICENSE-MIT"]
 readme = "README.md"
@@ -25,14 +25,14 @@ coset = "0.3"
 log = "0.4"
 mockall = { version = "0.11", optional = true }
 p256 = { version = "0.13", features = ["pem", "arithmetic", "jwk"] }
-passkey-types = { path = "../passkey-types", version = "0.3" }
+passkey-types = { path = "../passkey-types", version = "0.4" }
 rand = "0.8"
 tokio = { version = "1", features = ["sync"], optional = true }
 
 [dev-dependencies]
 mockall = { version = "0.11" }
-passkey-types = { path = "../passkey-types", version = "0.3", features = [
-    "testable",
+passkey-types = { path = "../passkey-types", version = "0.4", features = [
+  "testable",
 ] }
 tokio = { version = "1", features = ["sync", "macros", "rt"] }
 generic-array = { version = "0.14", default-features = false }

passkey-authenticator/src/authenticator.rs

@@ -118,16 +118,6 @@ where
         }
     }
 
-    /// Set the authenticator's display name which will be returned if [`webauthn::CredentialPropertiesOutput`] is requested.
-    pub fn set_display_name(&mut self, name: String) {
-        self.extensions.display_name.replace(name);
-    }
-
-    /// Get a reference to the authenticators display name to return in [`webauthn::CredentialPropertiesOutput`].
-    pub fn display_name(&self) -> Option<&String> {
-        self.extensions.display_name.as_ref()
-    }
-
     /// Set whether the authenticator should save new credentials with a signature counter.
     ///
     /// NOTE: Using a counter with a credential that will sync is not recommended and can cause friction

passkey-authenticator/src/authenticator/extensions.rs

@@ -28,9 +28,6 @@ use crate::Authenticator;
 #[derive(Debug, Default)]
 #[non_exhaustive]
 pub(super) struct Extensions {
-    /// The display name given when a [`webauthn::CredentialPropertiesOutput`] is requested
-    pub display_name: Option<String>,
-
     /// Extension to retrieve a symmetric secret from the authenticator.
     pub hmac_secret: Option<HmacSecretConfig>,
 }

passkey-authenticator/src/authenticator/get_assertion.rs

@@ -5,7 +5,6 @@ use passkey_types::{
         AuthenticatorData, Ctap2Error, Flags, StatusCode,
     },
     webauthn::PublicKeyCredentialUserEntity,
-    Passkey,
 };
 
 use crate::{private_key_from_cose_key, Authenticator, CredentialStore, UserValidationMethod};
@@ -14,7 +13,6 @@ impl<S: CredentialStore + Sync, U> Authenticator<S, U>
 where
     S: CredentialStore + Sync,
     U: UserValidationMethod<PasskeyItem = <S as CredentialStore>::PasskeyItem> + Sync,
-    Passkey: TryFrom<<S as CredentialStore>::PasskeyItem> + Clone,
 {
     /// This method is used by a host to request cryptographic proof of user authentication as well
     /// as user consent to a given transaction, using a previously generated credential that is

passkey-authenticator/src/user_validation.rs

@@ -15,7 +15,6 @@ pub struct UserCheck {
 
 /// Pluggable trait for the [`Authenticator`] to do user interaction and verification.
 #[cfg_attr(any(test, feature = "testable"), mockall::automock(type PasskeyItem = Passkey;))]
-#[cfg_attr(any(test, feature = "testable"), allow(clippy::unused_async))] // Generated by the `mockall` macro.
 #[async_trait::async_trait]
 pub trait UserValidationMethod {
     /// The type of the passkey item that can be used to display additional information about the operation to the user.

passkey-client/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "passkey-client"
-version = "0.3.0"
+version = "0.4.0"
 description = "Webauthn client in Rust."
 include = ["src/", "../LICENSE-APACHE", "../LICENSE-MIT"]
 readme = "README.md"
@@ -22,8 +22,8 @@ android-asset-validation = ["dep:nom"]
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-passkey-authenticator = { path = "../passkey-authenticator", version = "0.3" }
-passkey-types = { path = "../passkey-types", version = "0.3" }
+passkey-authenticator = { path = "../passkey-authenticator", version = "0.4" }
+passkey-types = { path = "../passkey-types", version = "0.4" }
 public-suffix = { path = "../public-suffix", version = "0.1" }
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"
@@ -39,8 +39,8 @@ nom = { version = "7", features = ["alloc"], optional = true }
 [dev-dependencies]
 coset = "0.3"
 mockall = { version = "0.11" }
-passkey-authenticator = { path = "../passkey-authenticator", version = "0.3", features = [
-    "tokio",
-    "testable",
+passkey-authenticator = { path = "../passkey-authenticator", version = "0.4", features = [
+  "tokio",
+  "testable",
 ] }
 tokio = { version = "1", features = ["macros", "rt"] }

passkey-client/src/android.rs

@@ -8,7 +8,7 @@ use nom::{
 use std::{borrow::Cow, fmt::Debug, str::from_utf8};
 use url::Url;
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 /// An Unverified asset link.
 pub struct UnverifiedAssetLink<'a> {
     /// Application package name.
@@ -27,19 +27,21 @@ impl<'a> UnverifiedAssetLink<'a> {
         package_name: impl Into<Cow<'a, str>>,
         sha256_cert_fingerprint: &str,
         host: impl Into<Cow<'a, str>>,
-        asset_link_url: Option<Url>,
+        asset_link_url: Url,
     ) -> Result<Self, ValidationError> {
+        // Is this correct?
+        // It looks like you can set your own url path.
+        // https://developers.google.com/digital-asset-links/v1/statements#scaling-to-dozens-of-statements-or-more
+        if !valid_asset_link_url(&asset_link_url) {
+            return Err(ValidationError::InvalidAssetLinkUrl);
+        }
         let host = host.into();
-        let url = match asset_link_url {
-            Some(u) => u,
-            None => Url::parse(&format!("https://{host}/.well-known/assetlinks.json",))
-                .map_err(|e| ValidationError::InvalidAssetLinkUrl(e.to_string()))?,
-        };
+
         valid_fingerprint(sha256_cert_fingerprint).map(|sha256_cert_fingerprint| Self {
             package_name: package_name.into(),
             sha256_cert_fingerprint,
             host,
-            asset_link_url: url,
+            asset_link_url,
         })
     }
 
@@ -71,8 +73,8 @@ pub enum ValidationError {
     ParseFailed(String),
     /// The fingerprint had an invalid length.
     InvalidLength,
-    /// The asset link url could not be parsed.
-    InvalidAssetLinkUrl(String),
+    /// The asset link url is not secure or incorrect path.
+    InvalidAssetLinkUrl,
 }
 
 impl<T> From<nom::Err<nom::error::Error<T>>> for ValidationError {
@@ -120,10 +122,15 @@ pub fn valid_fingerprint(fingerprint: &str) -> Result<Vec<u8>, ValidationError>
         .ok_or(ValidationError::InvalidLength)
 }
 
+/// Check for secure and expected path.
+fn valid_asset_link_url(url: &Url) -> bool {
+    url.scheme() == "https" && url.path() == "/.well-known/assetlinks.json"
+}
+
 #[cfg(test)]
 mod test {
-    use super::valid_fingerprint;
-    use crate::ValidationError;
+    use super::{valid_asset_link_url, valid_fingerprint, ValidationError};
+    use url::Url;
 
     #[test]
     fn check_valid_fingerprint() {
@@ -154,4 +161,22 @@ mod test {
             "Should be valid fingerprint"
         );
     }
+
+    #[test]
+    fn asset_link_url_ok() {
+        let url = Url::parse("https://www.facebook.com/.well-known/assetlinks.json").unwrap();
+        assert!(valid_asset_link_url(&url));
+    }
+
+    #[test]
+    fn asset_link_url_not_secure() {
+        let url = Url::parse("http://www.facebook.com/.well-known/assetlinks.json").unwrap();
+        assert!(!valid_asset_link_url(&url));
+    }
+
+    #[test]
+    fn asset_link_url_unexpected_path() {
+        let url = Url::parse("https://www.facebook.com/assetlinks.json").unwrap();
+        assert!(!valid_asset_link_url(&url));
+    }
 }

passkey-client/src/extensions.rs

@@ -17,7 +17,6 @@ use passkey_types::{
         AuthenticationExtensionsClientInputs, AuthenticationExtensionsClientOutputs,
         CredentialPropertiesOutput, PublicKeyCredentialRequestOptions,
     },
-    Passkey,
 };
 
 use crate::{Client, WebauthnError};
@@ -29,7 +28,6 @@ where
     S: CredentialStore + Sync,
     U: UserValidationMethod + Sync,
     P: public_suffix::EffectiveTLDProvider + Sync + 'static,
-    Passkey: TryFrom<<S as CredentialStore>::PasskeyItem>,
 {
     /// Create the extension inputs to be passed to an authenticator over CTAP2
     /// during a registration request.
@@ -55,7 +53,6 @@ where
 
             Some(CredentialPropertiesOutput {
                 discoverable: Some(discoverable),
-                authenticator_display_name: self.authenticator.display_name().cloned(),
             })
         } else {
             None

passkey-client/src/lib.rs

@@ -17,7 +17,7 @@
 mod client_data;
 pub use client_data::*;
 
-use std::{borrow::Cow, fmt::Display};
+use std::{borrow::Cow, fmt::Display, ops::ControlFlow};
 
 use ciborium::{cbor, value::Value};
 use coset::{iana::EnumI64, Algorithm};
@@ -35,8 +35,6 @@ use typeshare::typeshare;
 use url::Url;
 
 mod extensions;
-mod quirks;
-use quirks::QuirkyRp;
 
 #[cfg(feature = "android-asset-validation")]
 mod android;
@@ -160,7 +158,6 @@ where
     S: CredentialStore + Sync,
     U: UserValidationMethod + Sync,
     P: public_suffix::EffectiveTLDProvider + Sync + 'static,
-    Passkey: TryFrom<<S as CredentialStore>::PasskeyItem>,
 {
     authenticator: Authenticator<S, U>,
     rp_id_verifier: RpIdVerifier<P>,
@@ -187,7 +184,6 @@ where
     S: CredentialStore + Sync,
     U: UserValidationMethod<PasskeyItem = <S as CredentialStore>::PasskeyItem> + Sync,
     P: public_suffix::EffectiveTLDProvider + Sync + 'static,
-    Passkey: TryFrom<<S as CredentialStore>::PasskeyItem>,
 {
     /// Create a `Client` with a given `Authenticator` and a custom TLD provider
     /// that implements `[public_suffix::EffectiveTLDProvider]`.
@@ -353,9 +349,7 @@ where
             client_extension_results,
         };
 
-        // Sanitize output before sending it back to the RP
-        let maybe_quirky_rp = QuirkyRp::from_rp_id(rp_id);
-        Ok(maybe_quirky_rp.map_create_credential(response))
+        Ok(response)
     }
 
     /// Authenticate a Webauthn request.
@@ -549,30 +543,59 @@ where
             effective_domain = rp_id;
         }
 
-        // guard against localhost effective domain, return early
-        if effective_domain == "localhost" {
-            return if self.allows_insecure_localhost {
-                Ok(effective_domain)
-            } else {
-                Err(WebauthnError::InsecureLocalhostNotAllowed)
-            };
+        // Guard against local host and assert rp_id is not part of the public suffix list
+        if let ControlFlow::Break(res) = self.assert_valid_rp_id(effective_domain) {
+            return res;
         }
 
         // Make sure origin uses https://
         if !(origin.scheme().eq_ignore_ascii_case("https")) {
             return Err(WebauthnError::UnprotectedOrigin);
         }
 
+        Ok(effective_domain)
+    }
+
+    fn assert_valid_rp_id<'a>(
+        &self,
+        rp_id: &'a str,
+    ) -> ControlFlow<Result<&'a str, WebauthnError>, ()> {
+        // guard against localhost effective domain, return early
+        if rp_id == "localhost" {
+            return if self.allows_insecure_localhost {
+                ControlFlow::Break(Ok(rp_id))
+            } else {
+                ControlFlow::Break(Err(WebauthnError::InsecureLocalhostNotAllowed))
+            };
+        }
+
         // assert rp_id is not part of the public suffix list and is a registerable domain.
-        if decode_host(effective_domain)
+        if decode_host(rp_id)
             .as_ref()
             .and_then(|s| self.tld_provider.effective_tld_plus_one(s).ok())
             .is_none()
         {
-            return Err(WebauthnError::InvalidRpId);
+            return ControlFlow::Break(Err(WebauthnError::InvalidRpId));
         }
 
-        Ok(effective_domain)
+        ControlFlow::Continue(())
+    }
+
+    /// Parse a given Relying Party ID and assert that it is valid to act as such.
+    ///
+    /// This method is only to assert that an RP ID passes the required checks.
+    /// In order to ensure that a request's origin is in accordance with it's claimed RP ID,
+    /// [`Self::assert_domain`] should be used.
+    ///
+    /// There are several checks that an RP ID must pass:
+    /// 1. An RP ID set to `localhost` is only allowed when explicitly enabled with [`Self::allows_insecure_localhost`].
+    /// 1. An RP ID must not be part of the [public suffix list],
+    ///    since that would allow it to act as a credential for unrelated services by other entities.
+    pub fn is_valid_rp_id(&self, rp_id: &str) -> bool {
+        match self.assert_valid_rp_id(rp_id) {
+            ControlFlow::Continue(_) | ControlFlow::Break(Ok(_)) => true,
+            ControlFlow::Break(Err(_)) => false,
+        }
     }
 
     #[cfg(feature = "android-asset-validation")]