diff --git a/CHANGELOG.md b/CHANGELOG.md index ad819cc..a50368a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,16 @@ handles client data and its hash. - ⚠ BREAKING: Changed: Custom client data hashes are now specified using `DefaultClientDataWithCustomHash(Vec)` instead of `Some(Vec)`. - Added: Additional fields can be added to the client data using `DefaultClientDataWithExtra(ExtraData)`. +- Added: The `Client` now has the ability to adjust the response for quirky relying parties + when a fully featured response would break their server side validation. ([#31](https://github.com/1Password/passkey-rs/pull/31)) +- ⚠ BREAKING: Added the `Origin` enum which is now the origin parameter for the following methods ([#32](https://github.com/1Password/passkey-rs/pull/27)): + - `Client::register` takes an `impl Into` instead of a `&Url` + - `Client::authenticate` takes an `impl Into` instead of a `&Url` + - `RpIdValidator::assert_domain` takes an `&Origin` instead of a `&Url` +- ⚠ BREAKING: The collected client data will now have the android app signature as the origin when a request comes from an app directly. ([#32](https://github.com/1Password/passkey-rs/pull/27)) + +## passkey-types + - `CollectedClientData` is now generic and supports additional strongly typed fields. - Changed: `CollectedClientData` has changed to `CollectedClientData` - The `Client` now returns `CredProps::rk` depending on the authenticator's capabilities. @@ -76,4 +86,4 @@ Most of these changes are adding fields to structs which are breaking changes du ### public-suffix v0.1.1 -- Update the public suffix list \ No newline at end of file +- Update the public suffix list diff --git a/passkey-authenticator/src/user_validation.rs b/passkey-authenticator/src/user_validation.rs index 843a73f..c289118 100644 --- a/passkey-authenticator/src/user_validation.rs +++ b/passkey-authenticator/src/user_validation.rs @@ -61,14 +61,12 @@ impl MockUserValidationMethod { /// Sets up the mock for returning true for the verification. pub fn verified_user(times: usize) -> Self { let mut user_mock = MockUserValidationMethod::new(); + user_mock.expect_is_presence_enabled().returning(|| true); user_mock .expect_is_verification_enabled() .returning(|| Some(true)) .times(..); - user_mock - .expect_is_presence_enabled() - .returning(|| true) - .times(..); + user_mock.expect_is_presence_enabled().returning(|| true); user_mock .expect_check_user() .with( diff --git a/passkey-client/Cargo.toml b/passkey-client/Cargo.toml index 78dec31..ef4e229 100644 --- a/passkey-client/Cargo.toml +++ b/passkey-client/Cargo.toml @@ -17,6 +17,7 @@ workspace = true [features] tokio = ["dep:tokio"] testable = ["dep:mockall"] +android-asset-validation = ["dep:nom"] # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html @@ -33,12 +34,13 @@ idna = "0.5" url = "2" coset = "0.3" tokio = { version = "1", features = ["sync"], optional = true } +nom = { version = "7", features = ["alloc"], optional = true } [dev-dependencies] coset = "0.3" mockall = { version = "0.11" } passkey-authenticator = { path = "../passkey-authenticator", features = [ - "tokio", - "testable", + "tokio", + "testable", ] } tokio = { version = "1", features = ["macros", "rt"] } diff --git a/passkey-client/src/android.rs b/passkey-client/src/android.rs new file mode 100644 index 0000000..21e10aa --- /dev/null +++ b/passkey-client/src/android.rs @@ -0,0 +1,157 @@ +use nom::{ + bytes::complete::{tag, take_while_m_n}, + character::is_hex_digit, + combinator::map_res, + multi::separated_list1, + IResult, +}; +use std::{borrow::Cow, fmt::Debug, str::from_utf8}; +use url::Url; + +#[derive(Debug)] +/// An Unverified asset link. +pub struct UnverifiedAssetLink<'a> { + /// Application package name. + package_name: Cow<'a, str>, + /// Fingerprint to compare. + sha256_cert_fingerprint: Vec, + /// Host to lookup the well known asset link. + host: Cow<'a, str>, + /// When sourced from the application statement list or parsed from host for passkeys. + asset_link_url: Url, +} + +impl<'a> UnverifiedAssetLink<'a> { + /// Create a new [`UnverifiedAssetLink`]. + pub fn new( + package_name: impl Into>, + sha256_cert_fingerprint: &str, + host: impl Into>, + asset_link_url: Option, + ) -> Result { + 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, + }) + } + + /// Fingerprint of the application's signing certificate + pub fn sha256_cert_fingerprint(&self) -> &[u8] { + self.sha256_cert_fingerprint.as_slice() + } + + /// The application's package name + pub fn package_name(&self) -> &str { + &self.package_name + } + + /// The host to lookup the well-known assetlinks + pub fn host(&self) -> &str { + &self.host + } + + /// Get the digital asset Url for validation + pub fn asset_link_url(&self) -> Url { + self.asset_link_url.clone() + } +} + +/// Digital asset fingerprint validation error. +#[derive(Debug)] +pub enum ValidationError { + /// The fingerprint could not be parsed. + ParseFailed(String), + /// The fingerprint had an invalid length. + InvalidLength, + /// The asset link url could not be parsed. + InvalidAssetLinkUrl(String), +} + +impl From>> for ValidationError { + fn from(value: nom::Err>) -> Self { + let code_msg = value.map(|err| format!("{:?}", err.code)); + let message = match code_msg { + nom::Err::Incomplete(_) => "Parsing incomplete".to_owned(), + nom::Err::Error(msg) => format!("Parsing error: {msg}"), + nom::Err::Failure(msg) => format!("Parsing failure: {msg}"), + }; + + ValidationError::ParseFailed(message) + } +} + +/// Make sure we have an expected fingerprint. Characters have to be uppercase. +/// +/// +/// * Having a lower case signature in assetlinks.json. The signature should be +/// in upper case. +pub fn valid_fingerprint(fingerprint: &str) -> Result, ValidationError> { + #[derive(Debug)] + enum HexError { + Utf8, + ParseInt, + } + + fn parse_fingerprint(input: &[u8]) -> IResult<&[u8], Vec> { + separated_list1( + tag(":"), + map_res( + take_while_m_n(2, 2, |c| is_hex_digit(c) && !c.is_ascii_lowercase()), + |hex| { + u8::from_str_radix(from_utf8(hex).map_err(|_| HexError::Utf8)?, 16) + .map_err(|_| HexError::ParseInt) + }, + ), + )(input) + } + + let (left, parsed) = parse_fingerprint(fingerprint.as_bytes())?; + + (left.is_empty() && parsed.len() == 32) + .then_some(parsed) + .ok_or(ValidationError::InvalidLength) +} + +#[cfg(test)] +mod test { + use super::valid_fingerprint; + use crate::ValidationError; + + #[test] + fn check_valid_fingerprint() { + assert!( + valid_fingerprint("B3:5B:68:D5:CE:84:50:55:7C:6A:55:FD:64:B5:1F:EA:C1:10:CB:36:D6:A3:52:1C:59:48:DB:3A:38:0A:34:A9").is_ok(), + "Should be valid fingerprint" + ); + } + + #[test] + fn check_invalid_fingerprint_lowercase() { + let result = valid_fingerprint("b3:5b:68:d5:ce:84:50:55:7c:6a:55:fd:64:b5:1f:ea:c1:10:cb:36:d6:a3:52:1c:59:48:db:3a:38:0a:34:a9"); + assert!(result.is_err(), "Should be invalid fingerprint"); + assert!(matches!(result, Err(ValidationError::ParseFailed(..)))) + } + + #[test] + fn check_invalid_fingerprint_length() { + let result = valid_fingerprint("B3:5B:68:D5:CE:84:50:55:7C:6A:55"); + assert!(result.is_err(), "Should be invalid fingerprint"); + assert!(matches!(result, Err(ValidationError::InvalidLength))) + } + + #[test] + fn check_invalid_fingerprint_non_hex() { + assert!( + valid_fingerprint("B3:5B:68:X5:CE:84:50:55:7C:6A:55:FD:64:B5:1F:EA:C1:10:CB:36:D6:A3:52:1C:59:48:DB:3A:38:0A:34:A9").is_err(), + "Should be valid fingerprint" + ); + } +} diff --git a/passkey-client/src/lib.rs b/passkey-client/src/lib.rs index d88cbde..2de9c78 100644 --- a/passkey-client/src/lib.rs +++ b/passkey-client/src/lib.rs @@ -17,7 +17,7 @@ mod client_data; pub use client_data::*; -use std::borrow::Cow; +use std::{borrow::Cow, fmt::Display}; use ciborium::{cbor, value::Value}; use coset::{iana::EnumI64, Algorithm}; @@ -37,6 +37,15 @@ use serde::Serialize; use typeshare::typeshare; use url::Url; +mod quirks; +use quirks::QuirkyRp; + +#[cfg(feature = "android-asset-validation")] +mod android; + +#[cfg(feature = "android-asset-validation")] +pub use self::android::{valid_fingerprint, UnverifiedAssetLink, ValidationError}; + #[cfg(test)] mod tests; @@ -95,6 +104,44 @@ fn decode_host(host: &str) -> Option> { } } +/// The origin of a WebAuthn request. +pub enum Origin<'a> { + /// A Url, meant for a request in the web browser. + Web(Cow<'a, Url>), + /// An android digital asset fingerprint. + /// Meant for a request coming from an android application. + #[cfg(feature = "android-asset-validation")] + Android(UnverifiedAssetLink<'a>), +} + +impl From for Origin<'_> { + fn from(value: Url) -> Self { + Origin::Web(Cow::Owned(value)) + } +} + +impl<'a> From<&'a Url> for Origin<'a> { + fn from(value: &'a Url) -> Self { + Origin::Web(Cow::Borrowed(value)) + } +} + +impl Display for Origin<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Origin::Web(url) => write!(f, "{}", url.as_str().trim_end_matches('/')), + #[cfg(feature = "android-asset-validation")] + Origin::Android(target_link) => { + write!( + f, + "android:apk-key-hash:{}", + encoding::base64url(target_link.sha256_cert_fingerprint()) + ) + } + } + } +} + /// A `Client` represents a Webauthn client. Users of this struct should supply a /// [`CredentialStore`], a [`UserValidationMethod`] and, optionally, an implementation of /// [`public_suffix::EffectiveTLDProvider`]. @@ -171,10 +218,12 @@ where /// Returns either a [`webauthn::CreatedPublicKeyCredential`] on success or some [`WebauthnError`] pub async fn register, E: Serialize + Clone>( &mut self, - origin: &Url, + origin: impl Into>, request: webauthn::CredentialCreationOptions, client_data: D, ) -> Result { + let origin = origin.into(); + // extract inner value of request as there is nothing else of value directly in CredentialCreationOptions let request = request.public_key; let auth_info = self.authenticator.get_info().await; @@ -193,12 +242,12 @@ where let rp_id = self .rp_id_verifier - .assert_domain(origin, request.rp.id.as_deref())?; + .assert_domain(&origin, request.rp.id.as_deref())?; let collected_client_data = webauthn::CollectedClientData:: { ty: webauthn::ClientDataType::Create, challenge: encoding::base64url(&request.challenge), - origin: origin.as_str().trim_end_matches('/').to_owned(), + origin: origin.to_string(), cross_origin: None, extra_data: client_data.extra_client_data(), unknown_keys: Default::default(), @@ -213,7 +262,7 @@ where let cred_props_requested = request.extensions.as_ref().and_then(|ext| ext.cred_props) == Some(true); - let rk = self.map_rk(&request.authenticator_selection); + let rk = self.map_rk(&request.authenticator_selection, &auth_info); let uv = request.authenticator_selection.map(|s| s.user_verification) != Some(UserVerificationRequirement::Discouraged); @@ -304,7 +353,9 @@ where client_extension_results: AuthenticatorExtensionsClientOutputs { cred_props }, }; - Ok(response) + // 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)) } /// Authenticate a Webauthn request. @@ -312,10 +363,12 @@ where /// Returns either an [`webauthn::AuthenticatedPublicKeyCredential`] on success or some [`WebauthnError`]. pub async fn authenticate, E: Serialize + Clone>( &mut self, - origin: &Url, + origin: impl Into>, request: webauthn::CredentialRequestOptions, client_data: D, ) -> Result { + let origin = origin.into(); + // extract inner value of request as there is nothing else of value directly in CredentialRequestOptions let request = request.public_key; @@ -328,12 +381,12 @@ where let rp_id = self .rp_id_verifier - .assert_domain(origin, request.rp_id.as_deref())?; + .assert_domain(&origin, request.rp_id.as_deref())?; let collected_client_data = webauthn::CollectedClientData:: { ty: webauthn::ClientDataType::Get, challenge: encoding::base64url(&request.challenge), - origin: origin.as_str().trim_end_matches('/').to_owned(), + origin: origin.to_string(), cross_origin: None, //Some(false), extra_data: client_data.extra_client_data(), unknown_keys: Default::default(), @@ -383,7 +436,13 @@ where }) } - fn map_rk(&self, criteria: &Option) -> bool { + fn map_rk( + &self, + criteria: &Option, + auth_info: &ctap2::get_info::Response, + ) -> bool { + let supports_rk = auth_info.options.as_ref().is_some_and(|o| o.rk); + match criteria.as_ref().unwrap_or(&Default::default()) { // > If pkOptions.authenticatorSelection.residentKey: // > is present and set to required @@ -394,18 +453,14 @@ where } => true, // > is present and set to preferred - // > And the authenticator is capable of client-side credential storage modality AuthenticatorSelectionCriteria { resident_key: Some(ResidentKeyRequirement::Preferred), .. - // > Let requireResidentKey be true. - } => true, - - // > is present and set to preferred - // > And is not capable of client-side credential storage modality, or if the client cannot determine authenticator capability, - // > Let requireResidentKey be false. - // `passkey-rs` requires the authenticator to be capable of client-side credential storage modality, - // so we do not need to check for this case. + // > And the authenticator is capable of client-side credential storage modality + // > Let requireResidentKey be true. + // > And the authenticator is not capable of client-side credential storage modality, or if the client cannot determine authenticator capability, + // > Let requireResidentKey be false. + } => supports_rk, // > is present and set to discouraged AuthenticatorSelectionCriteria { @@ -460,6 +515,18 @@ where /// /// Returns the effective domain on success or some [`WebauthnError`] pub fn assert_domain<'a>( + &self, + origin: &'a Origin, + rp_id: Option<&'a str>, + ) -> Result<&'a str, WebauthnError> { + match origin { + Origin::Web(url) => self.assert_web_rp_id(url, rp_id), + #[cfg(feature = "android-asset-validation")] + Origin::Android(unverified) => self.assert_android_rp_id(unverified, rp_id), + } + } + + fn assert_web_rp_id<'a>( &self, origin: &'a Url, rp_id: Option<&'a str>, @@ -499,6 +566,37 @@ where Ok(effective_domain) } + + #[cfg(feature = "android-asset-validation")] + fn assert_android_rp_id<'a>( + &self, + target_link: &'a UnverifiedAssetLink, + rp_id: Option<&'a str>, + ) -> Result<&'a str, WebauthnError> { + let mut effective_rp_id = target_link.host(); + + if let Some(rp_id) = rp_id { + // subset from assert_web_rp_id + if !effective_rp_id.ends_with(rp_id) { + return Err(WebauthnError::OriginRpMissmatch); + } + effective_rp_id = rp_id; + } + + if decode_host(effective_rp_id) + .as_ref() + .and_then(|s| self.tld_provider.effective_tld_plus_one(s).ok()) + .is_none() + { + return Err(WebauthnError::InvalidRpId); + } + + // TODO: Find an ergonomic and caching friendly way to fetch the remote + // assetlinks and validate them here. + // https://github.com/1Password/passkey-rs/issues/13 + + Ok(effective_rp_id) + } } #[cfg(test)] @@ -565,13 +663,28 @@ mod test { user_verification: UserVerificationRequirement::Discouraged, authenticator_attachment: None, }; + let auth_info = ctap2::get_info::Response { + versions: vec![], + extensions: None, + aaguid: ctap2::Aaguid::new_empty(), + options: Some(ctap2::get_info::Options { + rk: true, + uv: Some(true), + up: true, + plat: true, + client_pin: None, + }), + max_msg_size: None, + pin_protocols: None, + transports: None, + }; let client = Client::new(Authenticator::new( ctap2::Aaguid::new_empty(), MemoryStore::new(), - MockUserValidationMethod::new(), + MockUserValidationMethod::verified_user(0), )); - let result = client.map_rk(&Some(criteria)); + let result = client.map_rk(&Some(criteria), &auth_info); assert_eq!(result, test_case.expected_rk, "{:?}", test_case); } diff --git a/passkey-client/src/quirks.rs b/passkey-client/src/quirks.rs new file mode 100644 index 0000000..2950263 --- /dev/null +++ b/passkey-client/src/quirks.rs @@ -0,0 +1,64 @@ +//! The goal of this module is to address quirks with RP's different implementations. +//! We don't want to limit this library's functionality for all RPs because of only +//! a few RPs misbehave. + +use passkey_types::webauthn::CreatedPublicKeyCredential; + +/// List of quirky RPs, the default is [`Self::NotQuirky`] which maps to being a no-op +#[derive(Default)] +pub(crate) enum QuirkyRp { + /// The RP is not known to be quirky, thus the mapping methods will be no-ops. + #[default] + NotQuirky, + + /// Adobe crashes on their server when they encounter the key + /// [credProps.authenticatorDisplayName][adn] during key creation. + /// + /// RP_IDs: + /// * `adobe.com` + /// + /// [adn]: https://w3c.github.io/webauthn/#dom-credentialpropertiesoutput-authenticatordisplayname + Adobe, + + /// Hyatt returns an "invalid request" error when they encounter the key + /// [credProps.authenticatorDisplayName][adn] during key creation. + /// + /// RP_IDs: + /// * `hyatt.com` + /// + /// [adn]: https://w3c.github.io/webauthn/#dom-credentialpropertiesoutput-authenticatordisplayname + Hyatt, +} + +impl QuirkyRp { + pub fn from_rp_id(rp_id: &str) -> Self { + match rp_id { + "adobe.com" => QuirkyRp::Adobe, + "hyatt.com" => QuirkyRp::Hyatt, + _ => QuirkyRp::NotQuirky, + } + } + + /// Use this after creating the response but before returning it to the function caller + #[inline] + pub fn map_create_credential( + &self, + response: CreatedPublicKeyCredential, + ) -> CreatedPublicKeyCredential { + match self { + // no-op + Self::NotQuirky => response, + Self::Adobe | Self::Hyatt => remove_authenticator_display_name(response), + } + } +} + +#[inline] +fn remove_authenticator_display_name( + mut response: CreatedPublicKeyCredential, +) -> CreatedPublicKeyCredential { + if let Some(cp) = response.client_extension_results.cred_props.as_mut() { + cp.authenticator_display_name = None; + } + response +} diff --git a/passkey-client/src/tests/mod.rs b/passkey-client/src/tests/mod.rs index 393ace8..9aa8d79 100644 --- a/passkey-client/src/tests/mod.rs +++ b/passkey-client/src/tests/mod.rs @@ -57,8 +57,7 @@ fn uv_mock_with_creation(times: usize) -> MockUserValidationMethod { let mut user_mock = MockUserValidationMethod::new(); user_mock .expect_is_verification_enabled() - .returning(|| Some(true)) - .times(times + 1); + .returning(|| Some(true)); user_mock .expect_check_user() .with( @@ -73,10 +72,7 @@ fn uv_mock_with_creation(times: usize) -> MockUserValidationMethod { }) }) .times(times); - user_mock - .expect_is_presence_enabled() - .returning(|| true) - .times(1); + user_mock.expect_is_presence_enabled().returning(|| true); user_mock } @@ -159,7 +155,7 @@ async fn create_and_authenticate_with_extra_client_data() { .authenticate( &origin, auth_options, - DefaultClientDataWithExtra(extra_data.clone()), + DefaultClientDataWithExtra(extra_data), ) .await .expect("failed to authenticate with freshly created credential"); @@ -298,26 +294,26 @@ async fn create_and_authenticate_without_cred_params() { fn validate_rp_id() -> Result<(), ParseError> { let client = RpIdVerifier::new(public_suffix::DEFAULT_PROVIDER); - let example = "https://example.com".parse()?; + let example = Url::parse("https://example.com")?.into(); let com_tld = client.assert_domain(&example, Some("com")); assert_eq!(com_tld, Err(WebauthnError::InvalidRpId)); - let example_dots = "https://example...com".parse()?; + let example_dots = Url::parse("https://example...com")?.into(); let bunch_of_dots = client.assert_domain(&example_dots, Some("...com")); assert_eq!(bunch_of_dots, Err(WebauthnError::InvalidRpId)); - let future = "https://www.future.1password.com".parse()?; + let future = Url::parse("https://www.future.1password.com")?.into(); let sub_domain_ignored = client.assert_domain(&future, Some("future.1password.com")); assert_eq!(sub_domain_ignored, Ok("future.1password.com")); let use_effective_domain = client.assert_domain(&future, None); assert_eq!(use_effective_domain, Ok("www.future.1password.com")); - let not_protected = "http://example.com".parse()?; + let not_protected = Url::parse("http://example.com")?.into(); let not_https = client.assert_domain(¬_protected, Some("example.com")); assert_eq!(not_https, Err(WebauthnError::UnprotectedOrigin)); - let localhost = "http://localhost:8080".parse()?; + let localhost = Url::parse("http://localhost:8080")?.into(); let should_still_match = client.assert_domain(&localhost, Some("example.com")); assert_eq!(should_still_match, Err(WebauthnError::OriginRpMissmatch)); @@ -361,7 +357,7 @@ fn validate_domain_with_private_list_provider() -> Result<(), ParseError> { // Notice that, in this test, this is a legitimate origin/rp_id combination // We assert that this produces an error to prove that we are indeed using our // BrokenTLDProvider which always returns Err() regardless of the TLD. - let origin = "https://www.future.1password.com".parse()?; + let origin = Url::parse("https://www.future.1password.com")?.into(); let rp_id = "future.1password.com"; let result = client.assert_domain(&origin, Some(rp_id)); assert_eq!(result, Err(WebauthnError::InvalidRpId)); diff --git a/passkey/src/lib.rs b/passkey/src/lib.rs index d0bdad9..bb4e2c6 100644 --- a/passkey/src/lib.rs +++ b/passkey/src/lib.rs @@ -143,7 +143,10 @@ //! }; //! //! // Now create the credential. -//! let my_webauthn_credential = my_client.register(&origin, request, DefaultClientData).await.unwrap(); +//! let my_webauthn_credential = my_client +//! .register(&origin, request, DefaultClientData) +//! .await +//! .unwrap(); //! //! // Let's try and authenticate. //! // Create a challenge that would usually come from the RP.