fido: support bulk deletion of credentials

This adds a DeleteCredentials() method to CredentialManagementHandler to
support deletion of multiple credentials identified by the
CBOR-serialized PublicKeyCredentialDescriptor.

It also extends EnumerateCredentialsResponse with the CBOR-serialized
PublicKeyCredentialDescriptor such that the UI can use it as an opaque
identifier for the credentials without having to do CBOR-serialization.
The FidoAuthenticator::DeleteCredential is changed to take
PublicKeyCredentialDescriptor rather than a sequence of bytes to
identify the to-be-deleted credential.

On a CTAP2 level, credentials are identified for deletion not just via
their credential ID but via the full PublicKeyCredentialDescriptor. The
spec is unclear on whether the non-ID related fields ('transports' in
particular) are significant or not. Hence, it's probably wise to just
echo the descriptor as it was received during credential enumeration,
rather than send an empty descriptor with only the credential ID.

Bug: 955859
Change-Id: Id1b7a9094876c701b21000399870bf439de4d8b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1674411
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672229}

device/fido/credential_management.cc

@@ -114,11 +114,11 @@ CredentialManagementRequest::ForEnumerateCredentialsGetNext(Version version) {
 CredentialManagementRequest CredentialManagementRequest::ForDeleteCredential(
     Version version,
     base::span<const uint8_t> pin_token,
-    std::vector<uint8_t> credential_id) {
+    const PublicKeyCredentialDescriptor& credential_id) {
   cbor::Value::MapValue params_map;
   params_map.emplace(
       static_cast<int>(CredentialManagementRequestParamKey::kCredentialID),
-      std::move(credential_id));
+      AsCBOR(credential_id));
   base::Optional<std::vector<uint8_t>> pin_auth_bytes =
       cbor::Writer::Write(cbor::Value(params_map));
   DCHECK(pin_auth_bytes);
@@ -313,7 +313,9 @@ EnumerateCredentialsResponse::EnumerateCredentialsResponse(
     size_t credential_count_)
     : user(std::move(user_)),
       credential_id(std::move(credential_id_)),
-      credential_count(credential_count_) {}
+      credential_count(credential_count_) {
+  credential_id_cbor_bytes = *cbor::Writer::Write(AsCBOR(credential_id));
+}
 
 AggregatedEnumerateCredentialsResponse::AggregatedEnumerateCredentialsResponse(
     PublicKeyCredentialRpEntity rp_)

device/fido/credential_management.h

@@ -110,7 +110,7 @@ struct CredentialManagementRequest {
   static CredentialManagementRequest ForDeleteCredential(
       Version version,
       base::span<const uint8_t> pin_token,
-      std::vector<uint8_t> credential_id);
+      const PublicKeyCredentialDescriptor& credential_id);
 
   CredentialManagementRequest(CredentialManagementRequest&&);
   CredentialManagementRequest& operator=(CredentialManagementRequest&&);
@@ -176,6 +176,10 @@ struct EnumerateCredentialsResponse {
 
   PublicKeyCredentialUserEntity user;
   PublicKeyCredentialDescriptor credential_id;
+  // For convenience, also return the serialized |credential_id| so that the UI
+  // doesn't have to do CBOR serialization. (It only cares about the opaque byte
+  // string.)
+  std::vector<uint8_t> credential_id_cbor_bytes;
   size_t credential_count;
 
  private:

device/fido/credential_management_handler.cc

@@ -8,11 +8,13 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "components/cbor/reader.h"
 #include "components/cbor/values.h"
 #include "components/cbor/writer.h"
 #include "device/fido/fido_authenticator.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/pin.h"
+#include "device/fido/public_key_credential_descriptor.h"
 
 namespace device {
 
@@ -205,7 +207,7 @@ static void OnDeleteCredential(
 }
 
 void CredentialManagementHandler::DeleteCredential(
-    base::span<const uint8_t> credential_id,
+    const PublicKeyCredentialDescriptor& credential_id,
     DeleteCredentialCallback callback) {
   DCHECK(state_ == State::kReady && !get_credentials_callback_);
   if (!authenticator_) {
@@ -220,6 +222,61 @@ void CredentialManagementHandler::DeleteCredential(
       base::BindOnce(&OnDeleteCredential, std::move(callback)));
 }
 
+void CredentialManagementHandler::OnDeleteCredentials(
+    std::vector<std::vector<uint8_t>> remaining_credential_ids,
+    CredentialManagementHandler::DeleteCredentialCallback callback,
+    CtapDeviceResponseCode status,
+    base::Optional<DeleteCredentialResponse> response) {
+  if (status != CtapDeviceResponseCode::kSuccess ||
+      remaining_credential_ids.empty()) {
+    std::move(callback).Run(status);
+    return;
+  }
+
+  if (!authenticator_) {
+    // |authenticator_| could have been removed during a bulk deletion.  The
+    // observer would have already gotten an AuthenticatorRemoved() call, so no
+    // need to resolve |callback|.
+    return;
+  }
+
+  auto credential_id = *PublicKeyCredentialDescriptor::CreateFromCBORValue(
+      *cbor::Reader::Read(remaining_credential_ids.back()));
+  remaining_credential_ids.pop_back();
+  authenticator_->DeleteCredential(
+      *pin_token_, credential_id,
+      base::BindOnce(&CredentialManagementHandler::OnDeleteCredentials,
+                     weak_factory_.GetWeakPtr(),
+                     std::move(remaining_credential_ids), std::move(callback)));
+}
+
+void CredentialManagementHandler::DeleteCredentials(
+    std::vector<std::vector<uint8_t>> credential_ids,
+    DeleteCredentialCallback callback) {
+  DCHECK(state_ == State::kReady && !get_credentials_callback_);
+  if (!authenticator_) {
+    // AuthenticatorRemoved() may have been called, but the observer would have
+    // seen a FidoAuthenticatorRemoved() call.
+    NOTREACHED();
+    return;
+  }
+  DCHECK(pin_token_);
+
+  if (credential_ids.empty()) {
+    std::move(callback).Run(CtapDeviceResponseCode::kSuccess);
+    return;
+  }
+
+  auto credential_id = *PublicKeyCredentialDescriptor::CreateFromCBORValue(
+      *cbor::Reader::Read(credential_ids.back()));
+  credential_ids.pop_back();
+  authenticator_->DeleteCredential(
+      *pin_token_, credential_id,
+      base::BindOnce(&CredentialManagementHandler::OnDeleteCredentials,
+                     weak_factory_.GetWeakPtr(), std::move(credential_ids),
+                     std::move(callback)));
+}
+
 void CredentialManagementHandler::OnCredentialsMetadata(
     CtapDeviceResponseCode status,
     base::Optional<CredentialsMetadataResponse> response) {

device/fido/credential_management_handler.h

@@ -6,6 +6,7 @@
 #define DEVICE_FIDO_CREDENTIAL_MANAGEMENT_HANDLER_H_
 
 #include <memory>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/component_export.h"
@@ -69,9 +70,17 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CredentialManagementHandler
 
   // DeleteCredential attempts to delete the credential with the given
   // |credential_id|.
-  void DeleteCredential(base::span<const uint8_t> credential_id,
+  void DeleteCredential(const PublicKeyCredentialDescriptor& credential_id,
                         DeleteCredentialCallback callback);
 
+  // DeleteCredentials deletes a list of credentials. Each entry in
+  // |credential_ids| must be a CBOR-serialized PublicKeyCredentialDescriptor.
+  // If any individual deletion fails, |callback| is invoked with the
+  // respective error, and deletion of the remaining credentials will be
+  // aborted (but others may have been deleted successfully already).
+  void DeleteCredentials(std::vector<std::vector<uint8_t>> credential_ids,
+                         DeleteCredentialCallback callback);
+
  private:
   enum class State {
     kWaitingForTouch,
@@ -108,6 +117,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) CredentialManagementHandler
       CtapDeviceResponseCode status,
       base::Optional<std::vector<AggregatedEnumerateCredentialsResponse>>
           responses);
+  void OnDeleteCredentials(
+      std::vector<std::vector<uint8_t>> remaining_credential_ids,
+      CredentialManagementHandler::DeleteCredentialCallback callback,
+      CtapDeviceResponseCode status,
+      base::Optional<DeleteCredentialResponse> response);
 
   SEQUENCE_CHECKER(sequence_checker_);
 

device/fido/credential_management_handler_unittest.cc

@@ -88,7 +88,7 @@ TEST_F(CredentialManagementHandlerTest, Test) {
   EXPECT_EQ(*num_remaining, 99u);
 
   handler->DeleteCredential(
-      opt_response->front().credentials.front().credential_id.id(),
+      opt_response->front().credentials.front().credential_id,
       delete_callback_.callback());
 
   delete_callback_.WaitForCallback();

device/fido/fido_authenticator.cc

@@ -78,7 +78,7 @@ void FidoAuthenticator::EnumerateCredentials(
 
 void FidoAuthenticator::DeleteCredential(
     base::span<const uint8_t> pin_token,
-    base::span<const uint8_t> credential_id,
+    const PublicKeyCredentialDescriptor& credential_id,
     DeleteCredentialCallback callback) {
   NOTREACHED();
 }

device/fido/fido_authenticator.h

@@ -164,9 +164,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAuthenticator {
                                       GetCredentialsMetadataCallback callback);
   virtual void EnumerateCredentials(base::span<const uint8_t> pin_token,
                                     EnumerateCredentialsCallback callback);
-  virtual void DeleteCredential(base::span<const uint8_t> pin_token,
-                                base::span<const uint8_t> credential_id,
-                                DeleteCredentialCallback callback);
+  virtual void DeleteCredential(
+      base::span<const uint8_t> pin_token,
+      const PublicKeyCredentialDescriptor& credential_id,
+      DeleteCredentialCallback callback);
 
   // Biometric enrollment commands.
   virtual void GetModality(BioEnrollmentCallback callback);

device/fido/fido_device_authenticator.cc

@@ -477,7 +477,7 @@ void FidoDeviceAuthenticator::OnEnumerateCredentialsDone(
 
 void FidoDeviceAuthenticator::DeleteCredential(
     base::span<const uint8_t> pin_token,
-    base::span<const uint8_t> credential_id,
+    const PublicKeyCredentialDescriptor& credential_id,
     DeleteCredentialCallback callback) {
   DCHECK(Options()->supports_credential_management ||
          Options()->supports_credential_management_preview);
@@ -487,7 +487,7 @@ void FidoDeviceAuthenticator::DeleteCredential(
           Options()->supports_credential_management
               ? CredentialManagementRequest::kDefault
               : CredentialManagementRequest::kPreview,
-          pin_token, fido_parsing_utils::Materialize(credential_id)),
+          pin_token, credential_id),
       std::move(callback), base::BindOnce(&DeleteCredentialResponse::Parse),
       /*string_fixup_predicate=*/nullptr);
 }

device/fido/fido_device_authenticator.h

@@ -76,7 +76,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceAuthenticator
   void EnumerateCredentials(base::span<const uint8_t> pin_token,
                             EnumerateCredentialsCallback callback) override;
   void DeleteCredential(base::span<const uint8_t> pin_token,
-                        base::span<const uint8_t> credential_id,
+                        const PublicKeyCredentialDescriptor& credential_id,
                         DeleteCredentialCallback callback) override;
 
   void GetModality(BioEnrollmentCallback callback) override;

device/fido/virtual_ctap2_device.cc

@@ -1336,15 +1336,19 @@ CtapDeviceResponseCode VirtualCtap2Device::OnCredentialManagement(
       const auto credential_id_it = params.find(cbor::Value(static_cast<int>(
           CredentialManagementRequestParamKey::kCredentialID)));
       if (credential_id_it == params.end() ||
-          !credential_id_it->second.is_bytestring()) {
+          !credential_id_it->second.is_map()) {
         return CtapDeviceResponseCode::kCtap2ErrCBORUnexpectedType;
       }
-      const std::vector<uint8_t>& credential_id =
-          credential_id_it->second.GetBytestring();
-      if (!base::Contains(mutable_state()->registrations, credential_id)) {
+      auto credential_id = PublicKeyCredentialDescriptor::CreateFromCBORValue(
+          cbor::Value(credential_id_it->second.GetMap()));
+      if (!credential_id) {
+        return CtapDeviceResponseCode::kCtap2ErrCBORUnexpectedType;
+      }
+      if (!base::Contains(mutable_state()->registrations,
+                          credential_id->id())) {
         return CtapDeviceResponseCode::kCtap2ErrNoCredentials;
       }
-      mutable_state()->registrations.erase(credential_id);
+      mutable_state()->registrations.erase(credential_id->id());
       *response = {};
       return CtapDeviceResponseCode::kSuccess;
     }