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}
diff --git a/device/fido/credential_management.cc b/device/fido/credential_management.cc
index ed56dc12..87d7877 100644
--- a/device/fido/credential_management.cc
+++ b/device/fido/credential_management.cc
@@ -114,11 +114,11 @@
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 @@
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_)
diff --git a/device/fido/credential_management.h b/device/fido/credential_management.h
index 2718c38..beaa193 100644
--- a/device/fido/credential_management.h
+++ b/device/fido/credential_management.h
@@ -110,7 +110,7 @@
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 @@
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:
diff --git a/device/fido/credential_management_handler.cc b/device/fido/credential_management_handler.cc
index d8eacea..06d85b7 100644
--- a/device/fido/credential_management_handler.cc
+++ b/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 @@
}
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 @@
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) {
diff --git a/device/fido/credential_management_handler.h b/device/fido/credential_management_handler.h
index 2497cbc..f67532b8 100644
--- a/device/fido/credential_management_handler.h
+++ b/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 @@
// 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 @@
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_);
diff --git a/device/fido/credential_management_handler_unittest.cc b/device/fido/credential_management_handler_unittest.cc
index c3b8710..85b40589 100644
--- a/device/fido/credential_management_handler_unittest.cc
+++ b/device/fido/credential_management_handler_unittest.cc
@@ -88,7 +88,7 @@
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();
diff --git a/device/fido/fido_authenticator.cc b/device/fido/fido_authenticator.cc
index 1f5acb8..9894c44 100644
--- a/device/fido/fido_authenticator.cc
+++ b/device/fido/fido_authenticator.cc
@@ -78,7 +78,7 @@
void FidoAuthenticator::DeleteCredential(
base::span<const uint8_t> pin_token,
- base::span<const uint8_t> credential_id,
+ const PublicKeyCredentialDescriptor& credential_id,
DeleteCredentialCallback callback) {
NOTREACHED();
}
diff --git a/device/fido/fido_authenticator.h b/device/fido/fido_authenticator.h
index 446d779..d91d873 100644
--- a/device/fido/fido_authenticator.h
+++ b/device/fido/fido_authenticator.h
@@ -164,9 +164,10 @@
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);
diff --git a/device/fido/fido_device_authenticator.cc b/device/fido/fido_device_authenticator.cc
index 25009de..a15be8e 100644
--- a/device/fido/fido_device_authenticator.cc
+++ b/device/fido/fido_device_authenticator.cc
@@ -477,7 +477,7 @@
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 @@
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);
}
diff --git a/device/fido/fido_device_authenticator.h b/device/fido/fido_device_authenticator.h
index 1660294..6e54bdc 100644
--- a/device/fido/fido_device_authenticator.h
+++ b/device/fido/fido_device_authenticator.h
@@ -76,7 +76,7 @@
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;
diff --git a/device/fido/virtual_ctap2_device.cc b/device/fido/virtual_ctap2_device.cc
index f158067..0add793 100644
--- a/device/fido/virtual_ctap2_device.cc
+++ b/device/fido/virtual_ctap2_device.cc
@@ -1336,15 +1336,19 @@
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;
}
