fido: make CtapMakeCredentialRequest.exclude_credentials non-optional

While at the CTAP layer, the excludeList parameter is optional, omitting
it is equivalent to sending an empty array. Furthermore, an absent
PublicKeyCredentialCreationOptions.excludeCredentials at the Blink layer
is translated into an empty list at the Authenticator mojo interface
(where the corresponding field is non-optional), which demonstrates that
we don't care about the distinction anyway.

Note that while previously, a present-but-empty CTAP excludeList was
marshaled into an empty CBOR Array (inside `AsCTAPRequestValuePair(const
CtapMakeCredentialRequest& request)`), this CL changes it to be omitted
instead.

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

device/fido/ctap_make_credential_request.cc

@@ -48,9 +48,9 @@ AsCTAPRequestValuePair(const CtapMakeCredentialRequest& request) {
   cbor_map[cbor::Value(2)] = AsCBOR(request.rp);
   cbor_map[cbor::Value(3)] = AsCBOR(request.user);
   cbor_map[cbor::Value(4)] = AsCBOR(request.public_key_credential_params);
-  if (request.exclude_list) {
+  if (!request.exclude_list.empty()) {
     cbor::Value::ArrayValue exclude_list_array;
-    for (const auto& descriptor : *request.exclude_list) {
+    for (const auto& descriptor : request.exclude_list) {
       exclude_list_array.push_back(AsCBOR(descriptor));
     }
     cbor_map[cbor::Value(5)] = cbor::Value(std::move(exclude_list_array));

device/fido/ctap_make_credential_request.h

@@ -64,7 +64,7 @@ struct COMPONENT_EXPORT(DEVICE_FIDO) CtapMakeCredentialRequest {
   bool is_u2f_only = false;
   bool is_incognito_mode = false;
 
-  base::Optional<std::vector<PublicKeyCredentialDescriptor>> exclude_list;
+  std::vector<PublicKeyCredentialDescriptor> exclude_list;
   base::Optional<std::vector<uint8_t>> pin_auth;
   base::Optional<uint8_t> pin_protocol;
   AttestationConveyancePreference attestation_preference =

device/fido/mac/make_credential_operation.mm

@@ -86,29 +86,26 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
 
   // Evaluate that excludeList does not contain any credentials stored by this
   // authenticator.
-  if (request().exclude_list) {
-    for (auto& credential : *request().exclude_list) {
-      ScopedCFTypeRef<CFMutableDictionaryRef> query = DefaultKeychainQuery();
-      CFDictionarySetValue(query, kSecAttrApplicationLabel,
-                           [NSData dataWithBytes:credential.id().data()
-                                          length:credential.id().size()]);
-      OSStatus status = SecItemCopyMatching(query, nullptr);
-      if (status == errSecSuccess) {
-        // Excluded item found.
-        DVLOG(1) << "credential from excludeList found";
-        std::move(callback())
-            .Run(CtapDeviceResponseCode::kCtap2ErrCredentialExcluded,
-                 base::nullopt);
-        return;
-      }
-      if (status != errSecItemNotFound) {
-        // Unexpected keychain error.
-        OSSTATUS_DLOG(ERROR, status)
-            << "failed to check for excluded credential";
-        std::move(callback())
-            .Run(CtapDeviceResponseCode::kCtap2ErrOther, base::nullopt);
-        return;
-      }
+  for (auto& credential : request().exclude_list) {
+    ScopedCFTypeRef<CFMutableDictionaryRef> query = DefaultKeychainQuery();
+    CFDictionarySetValue(query, kSecAttrApplicationLabel,
+                         [NSData dataWithBytes:credential.id().data()
+                                        length:credential.id().size()]);
+    OSStatus status = SecItemCopyMatching(query, nullptr);
+    if (status == errSecSuccess) {
+      // Excluded item found.
+      DVLOG(1) << "credential from excludeList found";
+      std::move(callback())
+          .Run(CtapDeviceResponseCode::kCtap2ErrCredentialExcluded,
+               base::nullopt);
+      return;
+    }
+    if (status != errSecItemNotFound) {
+      // Unexpected keychain error.
+      OSSTATUS_DLOG(ERROR, status) << "failed to check for excluded credential";
+      std::move(callback())
+          .Run(CtapDeviceResponseCode::kCtap2ErrOther, base::nullopt);
+      return;
     }
   }
 

device/fido/make_credential_task.cc

@@ -79,7 +79,6 @@ CtapMakeCredentialRequest MakeCredentialTask::GetTouchRequest(
       PublicKeyCredentialParams(
           {{CredentialType::kPublicKey,
             base::strict_cast<int>(CoseAlgorithmIdentifier::kCoseEs256)}}));
-  req.exclude_list.reset();
 
   // If a device supports CTAP2 and has PIN support then setting an empty
   // pinAuth should trigger just a touch[1]. Our U2F code also understands
@@ -128,12 +127,11 @@ void MakeCredentialTask::StartTask() {
 }
 
 CtapGetAssertionRequest MakeCredentialTask::NextSilentSignRequest() {
-  DCHECK(request_.exclude_list &&
-         current_credential_ < request_.exclude_list->size());
+  DCHECK(current_credential_ < request_.exclude_list.size());
   CtapGetAssertionRequest request(
       probing_alternative_rp_id_ ? *request_.app_id : request_.rp.id,
       /*client_data_json=*/"");
-  request.allow_list = {{request_.exclude_list->at(current_credential_)}};
+  request.allow_list = {{request_.exclude_list.at(current_credential_)}};
   request.user_presence_required = false;
   request.user_verification = UserVerificationRequirement::kDiscouraged;
   return request;
@@ -144,9 +142,8 @@ void MakeCredentialTask::MakeCredential() {
   // authenticators rejecting lists over a certain size. Also do this if a
   // second RP ID needs to be tested because the site used the appidExclude
   // extension.
-  if (request_.exclude_list &&
-      (request_.exclude_list->size() > 1 ||
-       (!request_.exclude_list->empty() && request_.app_id))) {
+  if (request_.exclude_list.size() > 1 ||
+      (!request_.exclude_list.empty() && request_.app_id)) {
     silent_sign_operation_ = std::make_unique<Ctap2DeviceOperation<
         CtapGetAssertionRequest, AuthenticatorGetAssertionResponse>>(
         device(), NextSilentSignRequest(),
@@ -170,7 +167,7 @@ void MakeCredentialTask::MakeCredential() {
 void MakeCredentialTask::HandleResponseToSilentSignRequest(
     CtapDeviceResponseCode response_code,
     base::Optional<AuthenticatorGetAssertionResponse> response_data) {
-  DCHECK(request_.exclude_list && request_.exclude_list->size() > 0);
+  DCHECK(!request_.exclude_list.empty());
 
   if (canceled_) {
     return;
@@ -181,7 +178,7 @@ void MakeCredentialTask::HandleResponseToSilentSignRequest(
   // touch and and the CTAP2_ERR_CREDENTIAL_EXCLUDED error code.
   if (response_code == CtapDeviceResponseCode::kSuccess) {
     CtapMakeCredentialRequest request = request_;
-    request.exclude_list = {{request_.exclude_list->at(current_credential_)}};
+    request.exclude_list = {{request_.exclude_list.at(current_credential_)}};
     if (probing_alternative_rp_id_) {
       request.rp.id = *request_.app_id;
     }
@@ -216,7 +213,7 @@ void MakeCredentialTask::HandleResponseToSilentSignRequest(
   // The authenticator doesn't recognize this particular credential from the
   // exclude list. Try the next one.
   current_credential_++;
-  if (current_credential_ == request_.exclude_list->size() &&
+  if (current_credential_ == request_.exclude_list.size() &&
       !probing_alternative_rp_id_ && request_.app_id) {
     // All elements of |request_.exclude_list| have been tested, but there's a
     // second RP ID so they need to be tested again.
@@ -224,7 +221,7 @@ void MakeCredentialTask::HandleResponseToSilentSignRequest(
     probing_alternative_rp_id_ = true;
   }
 
-  if (current_credential_ < request_.exclude_list->size()) {
+  if (current_credential_ < request_.exclude_list.size()) {
     silent_sign_operation_ = std::make_unique<Ctap2DeviceOperation<
         CtapGetAssertionRequest, AuthenticatorGetAssertionResponse>>(
         device(), NextSilentSignRequest(),
@@ -240,7 +237,7 @@ void MakeCredentialTask::HandleResponseToSilentSignRequest(
   // register request may proceed but without the exclude list present in case
   // it exceeds the device's size limit.
   CtapMakeCredentialRequest request = request_;
-  request.exclude_list.reset();
+  request.exclude_list = {};
   register_operation_ = std::make_unique<Ctap2DeviceOperation<
       CtapMakeCredentialRequest, AuthenticatorMakeCredentialResponse>>(
       device(), std::move(request), std::move(callback_),

device/fido/u2f_register_operation.cc

@@ -33,8 +33,7 @@ U2fRegisterOperation::~U2fRegisterOperation() = default;
 void U2fRegisterOperation::Start() {
   DCHECK(IsConvertibleToU2fRegisterCommand(request()));
 
-  const auto& exclude_list = request().exclude_list;
-  if (exclude_list && !exclude_list->empty()) {
+  if (!request().exclude_list.empty()) {
     // First try signing with the excluded credentials to see whether this
     // device should be excluded.
     WinkAndTrySign();
@@ -115,14 +114,14 @@ void U2fRegisterOperation::OnCheckForExcludedKeyHandle(
       // Continue to iterate through the provided key handles in the exclude
       // list to check for already registered keys.
       current_key_handle_index_++;
-      if (current_key_handle_index_ == request().exclude_list->size() &&
+      if (current_key_handle_index_ == request().exclude_list.size() &&
           !probing_alternative_rp_id_ && request().app_id) {
         // All elements of |request().exclude_list| have been tested, but
         // there's a second AppID so they need to be tested again.
         probing_alternative_rp_id_ = true;
         current_key_handle_index_ = 0;
       }
-      if (current_key_handle_index_ < request().exclude_list->size()) {
+      if (current_key_handle_index_ < request().exclude_list.size()) {
         WinkAndTrySign();
       } else {
         // Reached the end of exclude list with no duplicate credential.
@@ -204,8 +203,8 @@ void U2fRegisterOperation::OnRegisterResponseReceived(
 }
 
 const std::vector<uint8_t>& U2fRegisterOperation::excluded_key_handle() const {
-  DCHECK_LT(current_key_handle_index_, request().exclude_list->size());
-  return request().exclude_list.value()[current_key_handle_index_].id();
+  DCHECK_LT(current_key_handle_index_, request().exclude_list.size());
+  return request().exclude_list[current_key_handle_index_].id();
 }
 
 }  // namespace device

device/fido/virtual_ctap2_device.cc

@@ -722,13 +722,13 @@ base::Optional<CtapDeviceResponseCode> VirtualCtap2Device::OnMakeCredential(
 
   // 6. Check for already registered credentials.
   const auto rp_id_hash = fido_parsing_utils::CreateSHA256Hash(request.rp.id);
-  if (request.exclude_list) {
+  if (!request.exclude_list.empty()) {
     if (config_.reject_large_allow_and_exclude_lists &&
-        request.exclude_list->size() > 1) {
+        request.exclude_list.size() > 1) {
       return CtapDeviceResponseCode::kCtap2ErrLimitExceeded;
     }
 
-    for (const auto& excluded_credential : *request.exclude_list) {
+    for (const auto& excluded_credential : request.exclude_list) {
       const RegistrationData* found =
           FindRegistrationData(excluded_credential.id(), rp_id_hash);
       if (found) {

device/fido/win/webauthn_api.cc

@@ -267,7 +267,7 @@ AuthenticatorMakeCredentialBlocking(WinWebAuthnApi* webauthn_api,
   // Note that entries in |exclude_list_credentials| hold pointers
   // into request.exclude_list.
   std::vector<WEBAUTHN_CREDENTIAL_EX> exclude_list_credentials =
-      ToWinCredentialExVector(&request.exclude_list.value());
+      ToWinCredentialExVector(&request.exclude_list);
   std::vector<WEBAUTHN_CREDENTIAL_EX*> exclude_list_ptrs;
   std::transform(
       exclude_list_credentials.begin(), exclude_list_credentials.end(),