fido/mac: simplify CredentialMetadata interface

Change the SealCredentialId(), EncodeRpId(), EncodeRpIdAndUserId() and
SealLegacyV0CredentialIdForTestingOnly() methods to not return base::Optional
in order to make them less clunky. No functional changes intended.

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

device/fido/mac/credential_metadata.h

@@ -76,7 +76,7 @@ std::string GenerateCredentialMetadataSecret();
 //
 // Credential IDs have following format:
 //
-//    | version  |    nonce   | AEAD(pt=CBOR(metadata), |
+//    | version  |    nonce   | AEAD(pt=CBOR(metadata),    |
 //    | (1 byte) | (12 bytes) |      nonce=nonce,          |
 //    |          |            |      ad=(version, rpID))   |
 //
@@ -86,10 +86,9 @@ std::string GenerateCredentialMetadataSecret();
 // The |user_name| and |user_display_name| fields may be truncated before
 // encryption. The truncated values are guaranteed to be valid UTF-8.
 COMPONENT_EXPORT(DEVICE_FIDO)
-base::Optional<std::vector<uint8_t>> SealCredentialId(
-    const std::string& secret,
-    const std::string& rp_id,
-    const CredentialMetadata& metadata);
+std::vector<uint8_t> SealCredentialId(const std::string& secret,
+                                      const std::string& rp_id,
+                                      const CredentialMetadata& metadata);
 
 // UnsealCredentialId attempts to decrypt a CredentialMetadata from a credential
 // id.
@@ -105,15 +104,13 @@ base::Optional<CredentialMetadata> UnsealCredentialId(
 // This encoding allows lookup of credentials for a given RP and user but
 // without the credential ID.
 COMPONENT_EXPORT(DEVICE_FIDO)
-base::Optional<std::string> EncodeRpIdAndUserId(
-    const std::string& secret,
-    const std::string& rp_id,
-    base::span<const uint8_t> user_id);
+std::string EncodeRpIdAndUserId(const std::string& secret,
+                                const std::string& rp_id,
+                                base::span<const uint8_t> user_id);
 
 // EncodeRpId encodes the given RP ID for storage in the macOS keychain.
 COMPONENT_EXPORT(DEVICE_FIDO)
-base::Optional<std::string> EncodeRpId(const std::string& secret,
-                                       const std::string& rp_id);
+std::string EncodeRpId(const std::string& secret, const std::string& rp_id);
 
 // DecodeRpId attempts to decode a given RP ID from the keychain.
 //
@@ -126,7 +123,7 @@ base::Optional<std::string> DecodeRpId(const std::string& secret,
 
 // Seals a legacy V0 credential ID.
 COMPONENT_EXPORT(DEVICE_FIDO)
-base::Optional<std::vector<uint8_t>> SealLegacyV0CredentialIdForTestingOnly(
+std::vector<uint8_t> SealLegacyV0CredentialIdForTestingOnly(
     const std::string& secret,
     const std::string& rp_id,
     const std::vector<uint8_t>& user_id,

device/fido/mac/credential_metadata_unittest.cc

@@ -35,7 +35,7 @@ class CredentialMetadataTest : public ::testing::Test {
   }
 
   std::vector<uint8_t> SealCredentialId(CredentialMetadata user) {
-    return *device::fido::mac::SealCredentialId(key_, rp_id_, std::move(user));
+    return device::fido::mac::SealCredentialId(key_, rp_id_, std::move(user));
   }
 
   CredentialMetadata UnsealCredentialId(
@@ -44,11 +44,11 @@ class CredentialMetadataTest : public ::testing::Test {
   }
 
   std::string EncodeRpIdAndUserId(base::StringPiece user_id) {
-    return *device::fido::mac::EncodeRpIdAndUserId(key_, rp_id_,
-                                                   to_bytes(user_id));
+    return device::fido::mac::EncodeRpIdAndUserId(key_, rp_id_,
+                                                  to_bytes(user_id));
   }
   std::string EncodeRpId() {
-    return *device::fido::mac::EncodeRpId(key_, rp_id_);
+    return device::fido::mac::EncodeRpId(key_, rp_id_);
   }
 
   std::string DecodeRpId(const std::string& ct) {
@@ -70,11 +70,11 @@ TEST_F(CredentialMetadataTest, CredentialId) {
 
 TEST_F(CredentialMetadataTest, LegacyCredentialId) {
   auto user = DefaultUser();
-  auto id = SealLegacyV0CredentialIdForTestingOnly(
+  std::vector<uint8_t> id = SealLegacyV0CredentialIdForTestingOnly(
       key_, rp_id_, user.user_id, user.user_name, user.user_display_name);
-  EXPECT_EQ(0, (*id)[0]);
-  EXPECT_EQ(54u, id->size());
-  EXPECT_TRUE(MetadataEq(UnsealCredentialId(*id), DefaultUser()));
+  EXPECT_EQ(0, id[0]);
+  EXPECT_EQ(54u, id.size());
+  EXPECT_TRUE(MetadataEq(UnsealCredentialId(id), DefaultUser()));
 }
 
 TEST_F(CredentialMetadataTest, CredentialId_IsRandom) {
@@ -106,26 +106,26 @@ TEST_F(CredentialMetadataTest, EncodeRpIdAndUserId) {
   EXPECT_EQ(EncodeRpIdAndUserId("user"), EncodeRpIdAndUserId("user"));
   EXPECT_NE(EncodeRpIdAndUserId("userA"), EncodeRpIdAndUserId("userB"));
   EXPECT_NE(EncodeRpIdAndUserId("user"),
-            *device::fido::mac::EncodeRpIdAndUserId(key_, "notacme.com",
-                                                    to_bytes("user")));
+            device::fido::mac::EncodeRpIdAndUserId(key_, "notacme.com",
+                                                   to_bytes("user")));
   EXPECT_NE(EncodeRpIdAndUserId("user"),
-            *device::fido::mac::EncodeRpIdAndUserId(wrong_key_, rp_id_,
-                                                    to_bytes("user")));
+            device::fido::mac::EncodeRpIdAndUserId(wrong_key_, rp_id_,
+                                                   to_bytes("user")));
 }
 
 TEST_F(CredentialMetadataTest, EncodeRpId) {
   EXPECT_EQ(48u, EncodeRpId().size());
 
   EXPECT_EQ(EncodeRpId(), EncodeRpId());
-  EXPECT_NE(EncodeRpId(), *device::fido::mac::EncodeRpId(key_, "notacme.com"));
-  EXPECT_NE(EncodeRpId(), *device::fido::mac::EncodeRpId(wrong_key_, rp_id_));
+  EXPECT_NE(EncodeRpId(), device::fido::mac::EncodeRpId(key_, "notacme.com"));
+  EXPECT_NE(EncodeRpId(), device::fido::mac::EncodeRpId(wrong_key_, rp_id_));
 }
 
 TEST_F(CredentialMetadataTest, DecodeRpId) {
   EXPECT_EQ(rp_id_, DecodeRpId(EncodeRpId()));
   EXPECT_NE(rp_id_,
             *device::fido::mac::DecodeRpId(
-                key_, *device::fido::mac::EncodeRpId(key_, "notacme.com")));
+                key_, device::fido::mac::EncodeRpId(key_, "notacme.com")));
   EXPECT_FALSE(device::fido::mac::DecodeRpId(wrong_key_, EncodeRpId()));
 }
 

device/fido/mac/get_assertion_operation.mm

@@ -46,12 +46,7 @@ const std::string& GetAssertionOperation::RpId() const {
 }
 
 void GetAssertionOperation::Run() {
-  if (!Init()) {
-    std::move(callback())
-        .Run(CtapDeviceResponseCode::kCtap2ErrOther, base::nullopt);
-    return;
-  }
-
+  Init();
   // Display the macOS Touch ID prompt.
   PromptTouchId(l10n_util::GetStringFUTF16(IDS_WEBAUTHN_TOUCH_ID_PROMPT_REASON,
                                            base::UTF8ToUTF16(RpId())));

device/fido/mac/keychain.mm

@@ -87,19 +87,14 @@ static std::list<Credential> FindCredentialsImpl(
     const std::string& rp_id,
     const std::set<std::vector<uint8_t>>& allowed_credential_ids,
     LAContext* authentication_context) API_AVAILABLE(macosx(10.12.2)) {
-  base::Optional<std::string> encoded_rp_id =
-      EncodeRpId(metadata_secret, rp_id);
-  if (!encoded_rp_id) {
-    return {};
-  }
-
   base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
       CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
   CFDictionarySetValue(query, kSecClass, kSecClassKey);
   CFDictionarySetValue(query, kSecAttrAccessGroup,
                        base::SysUTF8ToNSString(keychain_access_group));
-  CFDictionarySetValue(query, kSecAttrLabel,
-                       base::SysUTF8ToNSString(*encoded_rp_id));
+  CFDictionarySetValue(
+      query, kSecAttrLabel,
+      base::SysUTF8ToNSString(EncodeRpId(metadata_secret, rp_id)));
   if (authentication_context) {
     CFDictionarySetValue(query, kSecUseAuthenticationContext,
                          authentication_context);

device/fido/mac/make_credential_operation.h

@@ -59,10 +59,6 @@ class API_AVAILABLE(macosx(10.12.2))
   // OperationBase:
   const std::string& RpId() const override;
   void PromptTouchIdDone(bool success) override;
-
-  // Generates a credential ID by invoking SealCredentialId() with parameters
-  // for the request. Note that results are non-deterministic.
-  base::Optional<std::vector<uint8_t>> GenerateCredentialIdForRequest() const;
 };
 
 }  // namespace mac

device/fido/mac/make_credential_operation.mm

@@ -49,12 +49,7 @@ const std::string& MakeCredentialOperation::RpId() const {
 }
 
 void MakeCredentialOperation::Run() {
-  if (!Init()) {
-    std::move(callback())
-        .Run(CtapDeviceResponseCode::kCtap2ErrOther, base::nullopt);
-    return;
-  }
-
+  Init();
   // Verify pubKeyCredParams contains ES-256, which is the only algorithm we
   // support.
   auto is_es256 =
@@ -113,17 +108,12 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
   //
   // Note that because the rk bit is not encoded here, a resident credential
   // may overwrite a non-resident credential and vice versa.
-  base::Optional<std::string> encoded_rp_id_user_id =
+  const std::string encoded_rp_id_user_id =
       EncodeRpIdAndUserId(metadata_secret(), RpId(), request().user.id);
-  if (!encoded_rp_id_user_id) {
-    std::move(callback())
-        .Run(CtapDeviceResponseCode::kCtap2ErrOther, base::nullopt);
-    return;
-  }
   {
     ScopedCFTypeRef<CFMutableDictionaryRef> query = DefaultKeychainQuery();
     CFDictionarySetValue(query, kSecAttrApplicationTag,
-                         base::SysUTF8ToNSString(*encoded_rp_id_user_id));
+                         base::SysUTF8ToNSString(encoded_rp_id_user_id));
     OSStatus status = Keychain::GetInstance().ItemDelete(query);
     if (status != errSecSuccess && status != errSecItemNotFound) {
       OSSTATUS_DLOG(ERROR, status) << "SecItemDelete failed";
@@ -134,14 +124,10 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
   }
 
   // Generate the new key pair.
-  base::Optional<std::vector<uint8_t>> credential_id =
-      GenerateCredentialIdForRequest();
-  if (!credential_id) {
-    FIDO_LOG(ERROR) << "GenerateCredentialIdForRequest failed";
-    std::move(callback())
-        .Run(CtapDeviceResponseCode::kCtap2ErrOther, base::nullopt);
-    return;
-  }
+  const std::vector<uint8_t> credential_id =
+      SealCredentialId(metadata_secret(), RpId(),
+                       CredentialMetadata::FromPublicKeyCredentialUserEntity(
+                           request().user, request().resident_key_required));
 
   ScopedCFTypeRef<CFMutableDictionaryRef> params(
       CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
@@ -160,10 +146,10 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
   CFDictionarySetValue(private_key_params, kSecUseAuthenticationContext,
                        authentication_context());
   CFDictionarySetValue(private_key_params, kSecAttrApplicationTag,
-                       base::SysUTF8ToNSString(*encoded_rp_id_user_id));
+                       base::SysUTF8ToNSString(encoded_rp_id_user_id));
   CFDictionarySetValue(private_key_params, kSecAttrApplicationLabel,
-                       [NSData dataWithBytes:credential_id->data()
-                                      length:credential_id->size()]);
+                       [NSData dataWithBytes:credential_id.data()
+                                      length:credential_id.size()]);
 
   ScopedCFTypeRef<CFErrorRef> cferr;
   ScopedCFTypeRef<SecKeyRef> private_key(
@@ -187,7 +173,7 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
   // Create attestation object. There is no separate attestation key pair, so
   // we perform self-attestation.
   base::Optional<AttestedCredentialData> attested_credential_data =
-      MakeAttestedCredentialData(*credential_id,
+      MakeAttestedCredentialData(credential_id,
                                  SecKeyRefToECPublicKey(public_key));
   if (!attested_credential_data) {
     FIDO_LOG(ERROR) << "MakeAttestedCredentialData failed";
@@ -216,13 +202,6 @@ void MakeCredentialOperation::PromptTouchIdDone(bool success) {
       .Run(CtapDeviceResponseCode::kSuccess, std::move(response));
 }
 
-base::Optional<std::vector<uint8_t>>
-MakeCredentialOperation::GenerateCredentialIdForRequest() const {
-  return SealCredentialId(metadata_secret(), RpId(),
-                          CredentialMetadata::FromPublicKeyCredentialUserEntity(
-                              request().user, request().resident_key_required));
-}
-
 }  // namespace mac
 }  // namespace fido
 }  // namespace device

device/fido/mac/operation_base.h

@@ -43,15 +43,7 @@ class API_AVAILABLE(macosx(10.12.2)) OperationBase : public Operation {
 
  protected:
   // Subclasses must call Init() at the beginning of Run().
-  bool Init() {
-    base::Optional<std::string> encoded_rp_id =
-        EncodeRpId(metadata_secret(), RpId());
-    if (!encoded_rp_id)
-      return false;
-
-    encoded_rp_id_ = std::move(*encoded_rp_id);
-    return true;
-  }
+  void Init() { encoded_rp_id_ = EncodeRpId(metadata_secret(), RpId()); }
 
   // PromptTouchId triggers a Touch ID consent dialog with the given reason
   // string. Subclasses implement the PromptTouchIdDone callback to receive the