diff --git a/CHANGELOG.md b/CHANGELOG.md index cbe5e044..38072787 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,8 @@ run xdg-desktop-portal while we work on upstreaming the changes. - daemon: Validate related origins requests. - daemon: Add support for CTAP2 hybrid over BLE behind a feature flag. - daemon: Deduplicate USB state events emitted over D-Bus. -- daemon: Don't use hybrid when not available +- daemon: Don't use hybrid when not available. +- daemon: Return InvalidStateError to caller when credential is excluded. - ui: Add Georgian translations. (Thank you, @EkaterinePopova!) - ui: Add a portal backend API to credentialsd-ui. - ui: Allow setting client PIN during the flow when required. diff --git a/credentialsd/src/credential_service/hybrid.rs b/credentialsd/src/credential_service/hybrid.rs index 5f136346..df42b98a 100644 --- a/credentialsd/src/credential_service/hybrid.rs +++ b/credentialsd/src/credential_service/hybrid.rs @@ -3,27 +3,29 @@ use std::fmt::Debug; use async_stream::stream; use futures_lite::Stream; +use libwebauthn::{ + proto::CtapError, + transport::{ + Channel, ChannelSettings, Device, + cable::{ + channel::{CableUpdate, CableUxUpdate}, + qr_code_device::{CableQrCodeDevice, CableTransports, QrCodeOperationHint}, + }, + }, + webauthn::{WebAuthn, error::WebAuthnError}, +}; use tokio::sync::{ broadcast, mpsc::{self, Sender}, }; use tracing::{debug, error}; -use libwebauthn::transport::cable::channel::{CableUpdate, CableUxUpdate}; -use libwebauthn::transport::cable::qr_code_device::{ - CableQrCodeDevice, CableTransports, QrCodeOperationHint, -}; -use libwebauthn::transport::{Channel, ChannelSettings, Device}; -use libwebauthn::webauthn::{WebAuthn, error::WebAuthnError}; - use credentialsd_common::{ memfd::write_secret, model::{BackgroundEvent, Error}, }; -use crate::model::CredentialRequest; - -use super::AuthenticatorResponse; +use crate::model::{CredentialRequest, CredentialResponse}; pub(crate) trait HybridHandler { fn start( @@ -94,67 +96,61 @@ impl HybridHandler for InternalHybridHandler { }); tracing::debug!("Polling hybrid channel for updates."); - let response: Result = loop { - match &request { + let response: Result = loop { + let response = match &request { CredentialRequest::CreatePublicKeyCredentialRequest(make_request) => { - match channel.webauthn_make_credential(make_request).await { - Ok(response) => break Ok(response.into()), - Err(WebAuthnError::Ctap(ctap_error)) => { - if ctap_error.is_retryable_user_error() { - tracing::debug!( - "Retrying credential creation operation because of CTAP error: {:?}", - ctap_error - ); - continue; - } else { - tracing::error!( - "Received CTAP unrecoverable CTAP error: {:?}", - ctap_error - ); - break Err(Error::AuthenticatorError); - } - } - Err(err) => { - tracing::error!( - "Received unrecoverable error from authenticator: {:?}", - err - ); - break Err(Error::AuthenticatorError); - } - }; + channel.webauthn_make_credential(make_request).await.map( + |make_credential_response| { + CredentialResponse::from_make_credential( + &make_credential_response, + &["hybrid"], + "cross-platform", + ) + }, + ) + } + CredentialRequest::GetPublicKeyCredentialRequest(get_request) => channel + .webauthn_get_assertion(get_request) + .await + .map(|get_assertion_response| { + CredentialResponse::from_get_assertion( + // When doing hybrid, the authenticator is capable of displaying it's own UI. + // So we assume here, it only ever returns one assertion. + // In case this doesn't hold true, we have to implement credential selection here, + // like USB, for example. + &get_assertion_response.assertions[0], + "cross-platform", + ) + }), + }; + match response { + Ok(response) => { + tracing::debug!("Received credential from hybrid authenticator"); + break Ok(response); } - CredentialRequest::GetPublicKeyCredentialRequest(get_request) => { - match channel.webauthn_get_assertion(get_request).await { - Ok(response) => break Ok(response.into()), - Err(WebAuthnError::Ctap(ctap_error)) => { - if ctap_error.is_retryable_user_error() { - tracing::debug!( - "Retrying assertion operation because of CTAP error: {:?}", - ctap_error - ); - continue; - } else { - tracing::error!( - "Received CTAP unrecoverable CTAP error: {:?}", - ctap_error - ); - break Err(Error::AuthenticatorError); - } - } - Err(err) => { - tracing::error!( - "Received unrecoverable error from authenticator: {:?}", - err - ); - break Err(Error::AuthenticatorError); - } - }; + Err(WebAuthnError::Ctap(ctap_error)) + if ctap_error.is_retryable_user_error() => + { + tracing::debug!(%ctap_error, "Retrying WebAuthn operation"); + continue; + } + Err(err) => { + tracing::error!(%err, + "Failed to make/get credential with hybrid authenticator" + ); + break Err(err); } } - }; + } + .map_err(|err| match err { + WebAuthnError::Ctap(CtapError::PINAuthBlocked) => Error::PinAttemptsExhausted, + WebAuthnError::Ctap(CtapError::NoCredentials) => Error::NoCredentials, + WebAuthnError::Ctap(CtapError::CredentialExcluded) => Error::CredentialExcluded, + _ => Error::AuthenticatorError, + }); let terminal_state = match response { - Ok(auth_response) => HybridStateInternal::Completed(Box::new(auth_response)), - Err(_) => HybridStateInternal::Failed, + Ok(auth_response) => HybridStateInternal::Completed(auth_response), + Err(err) => HybridStateInternal::Failed(err), }; if let Err(err) = tx.send(terminal_state).await { tracing::error!("Failed to send caBLE update: {:?}", err) @@ -183,9 +179,9 @@ pub(super) enum HybridStateInternal { Connected, /// Authenticator data - Completed(Box), + Completed(CredentialResponse), - Failed, + Failed(Error), // TODO(cancellation) // This isn't actually sent from the server. #[allow(dead_code)] @@ -215,7 +211,7 @@ pub enum HybridState { Completed, /// Hybrid operation failed. - Failed, + Failed(Error), // This isn't actually sent from the server. UserCancelled, @@ -229,7 +225,7 @@ impl From for HybridState { HybridStateInternal::Connected => HybridState::Connected, HybridStateInternal::Completed(_) => HybridState::Completed, HybridStateInternal::UserCancelled => HybridState::UserCancelled, - HybridStateInternal::Failed => HybridState::Failed, + HybridStateInternal::Failed(err) => HybridState::Failed(err), } } } @@ -252,7 +248,13 @@ impl From<&HybridState> for BackgroundEvent { HybridState::Connected => BackgroundEvent::HybridConnected, HybridState::Completed => BackgroundEvent::CeremonyCompleted, HybridState::UserCancelled => BackgroundEvent::ErrorCancelled, - HybridState::Failed => BackgroundEvent::ErrorAuthenticator, + HybridState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator, + HybridState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials, + HybridState::Failed(Error::CredentialExcluded) => { + BackgroundEvent::ErrorCredentialExcluded + } + HybridState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator, + HybridState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal, } } } @@ -278,7 +280,7 @@ async fn handle_hybrid_updates( CableUpdate::Connected => Some(HybridStateInternal::Connected), CableUpdate::Error(transport_error) => { error!(?transport_error, "Hybrid transport error"); - Some(HybridStateInternal::Failed) + Some(HybridStateInternal::Failed(Error::AuthenticatorError)) } }, }; diff --git a/credentialsd/src/credential_service/mod.rs b/credentialsd/src/credential_service/mod.rs index 39435bf9..40c9ac03 100644 --- a/credentialsd/src/credential_service/mod.rs +++ b/credentialsd/src/credential_service/mod.rs @@ -282,27 +282,14 @@ where match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) { Poll::Pending => Poll::Pending, Poll::Ready(Some(HybridEvent { state })) => { - if let HybridStateInternal::Completed(hybrid_response) = &state { - let response = match &**hybrid_response { - AuthenticatorResponse::CredentialCreated(make_credential_response) => { - CredentialResponse::from_make_credential( - make_credential_response, - &["hybrid"], - "cross-platform", - ) - } - AuthenticatorResponse::CredentialsAsserted(get_assertion_response) => { - CredentialResponse::from_get_assertion( - // When doing hybrid, the authenticator is capable of displaying it's own UI. - // So we assume here, it only ever returns one assertion. - // In case this doesn't hold true, we have to implement credential selection here, - // as is done for USB. - &get_assertion_response.assertions[0], - "cross-platform", - ) - } - }; - complete_request(ctx, response.clone()); + match &state { + HybridStateInternal::Completed(response) => { + complete_request(ctx, Ok(response.clone())); + } + HybridStateInternal::Failed(err) => { + complete_request(ctx, Err(err.clone())); + } + _ => {} } Poll::Ready(Some(state.into())) } @@ -330,8 +317,14 @@ where match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) { Poll::Pending => Poll::Pending, Poll::Ready(Some(UsbEvent { state })) => { - if let UsbStateInternal::Completed(response) = &state { - complete_request(ctx, response.clone()); + match &state { + UsbStateInternal::Completed(response) => { + complete_request(ctx, Ok(response.clone())); + } + UsbStateInternal::Failed(err) => { + complete_request(ctx, Err(err.clone())); + } + _ => {} } Poll::Ready(Some(state.into())) } @@ -360,8 +353,14 @@ where match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) { Poll::Pending => Poll::Pending, Poll::Ready(Some(NfcEvent { state })) => { - if let NfcStateInternal::Completed(response) = &state { - complete_request(ctx, response.clone()); + match &state { + NfcStateInternal::Completed(response) => { + complete_request(ctx, Ok(response.clone())); + } + NfcStateInternal::Failed(err) => { + complete_request(ctx, Err(err.clone())); + } + _ => {} } Poll::Ready(Some(state.into())) } @@ -404,10 +403,13 @@ impl From for DeviceStateUpdate { } } -fn complete_request(ctx: &Mutex>, response: CredentialResponse) { +fn complete_request( + ctx: &Mutex>, + response: Result, +) { match ctx.lock().unwrap().take() { Some(ctx) => { - ctx.send_response(Ok(response)); + ctx.send_response(response); } _ => { tracing::error!("Tried to consume context to respond to caller, but none was found.") diff --git a/credentialsd/src/credential_service/nfc.rs b/credentialsd/src/credential_service/nfc.rs index 96b71021..01b631d4 100644 --- a/credentialsd/src/credential_service/nfc.rs +++ b/credentialsd/src/credential_service/nfc.rs @@ -487,7 +487,7 @@ impl From<&NfcState> for BackgroundEvent { NfcState::Completed => BackgroundEvent::CeremonyCompleted, NfcState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator, NfcState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials, - NfcState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorAuthenticator, + NfcState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorCredentialExcluded, NfcState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator, NfcState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal, } diff --git a/credentialsd/src/credential_service/usb.rs b/credentialsd/src/credential_service/usb.rs index 3ea66f07..870a34be 100644 --- a/credentialsd/src/credential_service/usb.rs +++ b/credentialsd/src/credential_service/usb.rs @@ -607,7 +607,7 @@ impl From<&UsbState> for BackgroundEvent { UsbState::Completed => BackgroundEvent::CeremonyCompleted, UsbState::Failed(Error::AuthenticatorError) => BackgroundEvent::ErrorAuthenticator, UsbState::Failed(Error::NoCredentials) => BackgroundEvent::ErrorNoCredentials, - UsbState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorAuthenticator, + UsbState::Failed(Error::CredentialExcluded) => BackgroundEvent::ErrorCredentialExcluded, UsbState::Failed(Error::PinAttemptsExhausted) => BackgroundEvent::ErrorAuthenticator, UsbState::Failed(Error::Internal(_)) => BackgroundEvent::ErrorInternal, } diff --git a/credentialsd/src/dbus/flow_control.rs b/credentialsd/src/dbus/flow_control.rs index f196a811..541fc521 100644 --- a/credentialsd/src/dbus/flow_control.rs +++ b/credentialsd/src/dbus/flow_control.rs @@ -330,15 +330,9 @@ impl CredentialRequestController for CredentialRequestControllerClient { tracing::error!("Credential response channel closed prematurely"); WebAuthnError::NotAllowedError })?; - // TODO: CredentialServiceError is returning the wrong errors types to the flow controller - // We need to be able to bubble up the InvalidStateError, when the - // selected authenticator has the credential known by the RP, and - // the user wants to let the RP know. - // All the other possible errors from the spec (AbortError, - // ConstraintError, SecurityError, TypeError) should be handled - // earlier by the gateway. - // Every other error should be squashed into NotAllowed as a catch-all - // For now, just squashing. - response.map_err(|_| WebAuthnError::NotAllowedError) + response.map_err(|err| match err { + CredentialServiceError::CredentialExcluded => WebAuthnError::InvalidStateError, + _ => WebAuthnError::NotAllowedError, + }) } }