Propagate exclude credential

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.

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<AuthenticatorResponse, Error> = loop {
-                    match &request {
+                let response: Result<CredentialResponse, Error> = 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<AuthenticatorResponse>),
+    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<HybridStateInternal> 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))
                 }
             },
         };

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<UsbState> for DeviceStateUpdate {
     }
 }
 
-fn complete_request(ctx: &Mutex<Option<RequestContext>>, response: CredentialResponse) {
+fn complete_request(
+    ctx: &Mutex<Option<RequestContext>>,
+    response: Result<CredentialResponse, CredentialServiceError>,
+) {
     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.")

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

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

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,
+        })
     }
 }