diff --git a/CHANGELOG.md b/CHANGELOG.md index d3a2a5a..75e76cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ run xdg-desktop-portal while we work on upstreaming the changes. - daemon: Deduplicate USB state events emitted over D-Bus. - daemon: Don't use hybrid when not available - daemon: Cancel other transports, if one succeeded/failed +- 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 be6bb64..4d18cac 100644 --- a/credentialsd/src/credential_service/hybrid.rs +++ b/credentialsd/src/credential_service/hybrid.rs @@ -3,6 +3,17 @@ 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}, @@ -10,24 +21,12 @@ use tokio::sync::{ use tokio_util::sync::CancellationToken; use tracing::{debug, error}; -use libwebauthn::transport::cable::qr_code_device::{ - CableQrCodeDevice, CableTransports, QrCodeOperationHint, -}; -use libwebauthn::transport::{Channel, ChannelSettings, Device}; -use libwebauthn::webauthn::{WebAuthn, error::WebAuthnError}; -use libwebauthn::{ - proto::CtapError, - transport::cable::channel::{CableUpdate, CableUxUpdate}, -}; - 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( @@ -101,18 +100,31 @@ impl HybridHandler for InternalHybridHandler { let wait_for_response_fut = async { loop { - let response: Result = match &request { + let response: Result = match &request { CredentialRequest::CreatePublicKeyCredentialRequest(make_request) => { - channel - .webauthn_make_credential(make_request) - .await - .map(|response| response.into()) + 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(|response| response.into()) + 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 { @@ -159,8 +171,8 @@ impl HybridHandler for InternalHybridHandler { }; 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) @@ -189,9 +201,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)] @@ -221,7 +233,7 @@ pub enum HybridState { Completed, /// Hybrid operation failed. - Failed, + Failed(Error), // This isn't actually sent from the server. UserCancelled, @@ -235,7 +247,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), } } } @@ -258,7 +270,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, } } } @@ -284,7 +302,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 24cc952..686e80e 100644 --- a/credentialsd/src/credential_service/mod.rs +++ b/credentialsd/src/credential_service/mod.rs @@ -139,7 +139,11 @@ impl .unwrap() .start(request, cancellation.clone()); let ctx = self.ctx.clone(); - Box::pin(HybridStateStream { inner: stream, ctx }) + Box::pin(HybridStateStream { + inner: stream, + ctx, + cancellation_token: cancellation.clone(), + }) } else { tracing::error!( "Attempted to start hybrid credential flow, but no request context was found." @@ -162,7 +166,11 @@ impl .unwrap() .start(request, cancellation.clone()); let ctx = self.ctx.clone(); - Box::pin(UsbStateStream { inner: stream, ctx }) + Box::pin(UsbStateStream { + inner: stream, + ctx, + cancellation_token: cancellation.clone(), + }) } else { tracing::error!( "Attempted to start usb credential flow, but no request context was found." @@ -185,7 +193,11 @@ impl .unwrap() .start(request, cancellation.clone()); let ctx = self.ctx.clone(); - Box::pin(NfcStateStream { inner: stream, ctx }) + Box::pin(NfcStateStream { + inner: stream, + ctx, + cancellation_token: cancellation.clone(), + }) } else { tracing::error!( "Attempted to start nfc credential flow, but no request context was found." @@ -311,6 +323,7 @@ impl Manage pub struct HybridStateStream { inner: H, ctx: Arc>>, + cancellation_token: CancellationToken, } impl Stream for HybridStateStream @@ -324,34 +337,19 @@ where cx: &mut std::task::Context<'_>, ) -> Poll> { let ctx = &self.ctx.clone(); + let cancellation_token = self.cancellation_token.clone(); match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) { Poll::Pending => Poll::Pending, Poll::Ready(Some(HybridEvent { state })) => { + if cancellation_token.is_cancelled() { + return Poll::Ready(None); + } match &state { - HybridStateInternal::Completed(hybrid_response) => { - 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", - ) - } - }; + HybridStateInternal::Completed(response) => { complete_request(ctx, Ok(response.clone())); } - HybridStateInternal::Failed => { - complete_request(ctx, Err(CredentialServiceError::AuthenticatorError)); + HybridStateInternal::Failed(err) => { + complete_request(ctx, Err(err.clone())); } _ => {} } @@ -365,6 +363,7 @@ where struct UsbStateStream { inner: H, ctx: Arc>>, + cancellation_token: CancellationToken, } impl Stream for UsbStateStream @@ -378,9 +377,13 @@ where cx: &mut std::task::Context<'_>, ) -> Poll> { let ctx = &self.ctx.clone(); + let cancellation_token = self.cancellation_token.clone(); match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) { Poll::Pending => Poll::Pending, Poll::Ready(Some(UsbEvent { state })) => { + if cancellation_token.is_cancelled() { + return Poll::Ready(None); + } match &state { UsbStateInternal::Completed(response) => { complete_request(ctx, Ok(response.clone())); @@ -401,6 +404,7 @@ where struct NfcStateStream { inner: H, ctx: Arc>>, + cancellation_token: CancellationToken, } impl Stream for NfcStateStream @@ -414,9 +418,14 @@ where cx: &mut std::task::Context<'_>, ) -> Poll> { let ctx = &self.ctx.clone(); + let cancellation_token = self.cancellation_token.clone(); match Box::pin(Box::pin(self).as_mut().inner.next()).poll(cx) { Poll::Pending => Poll::Pending, Poll::Ready(Some(NfcEvent { state })) => { + if cancellation_token.is_cancelled() { + return Poll::Ready(None); + } + match &state { NfcStateInternal::Completed(response) => { complete_request(ctx, Ok(response.clone())); @@ -770,15 +779,24 @@ mod tests { { Box::pin(async_stream::stream! { let Some(mut rx) = rx else { return; }; + // This allows to simulate when the handler detected cancellation, + // but still emit a single event after cancellation to simulate a + // race. + let mut cancel_detected = false; loop { tokio::select! { biased; - _ = cancellation.cancelled() => { + _ = cancellation.cancelled(), if !cancel_detected => { + cancel_detected = true; cancelled.store(true, Ordering::SeqCst); - break; } maybe = rx.recv() => match maybe { - Some(state) => yield wrap(state), + Some(state) => { + yield wrap(state) + if cancel_detected { + break; + } + }, None => break, // all senders dropped } } @@ -1018,6 +1036,10 @@ mod tests { cancellation_token.is_cancelled(), "Cancellation token should be triggered after cancel_request" ); + // Add explicit post-cancellation message. + usb_ref.shift_state(UsbStateInternal::Failed(CredentialServiceError::Internal( + "Cancelled".to_string(), + ))); // biased select! polls cancellation first, discarding the queued states let usb_remaining: Vec<_> = usb_stream.collect().await; diff --git a/credentialsd/src/credential_service/nfc.rs b/credentialsd/src/credential_service/nfc.rs index 44ab0a7..bd30d78 100644 --- a/credentialsd/src/credential_service/nfc.rs +++ b/credentialsd/src/credential_service/nfc.rs @@ -547,7 +547,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 5406b24..3cd4362 100644 --- a/credentialsd/src/credential_service/usb.rs +++ b/credentialsd/src/credential_service/usb.rs @@ -683,7 +683,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 9c69b36..61362a2 100644 --- a/credentialsd/src/dbus/flow_control.rs +++ b/credentialsd/src/dbus/flow_control.rs @@ -352,15 +352,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, + }) } }