[LinuxDT] Fix LoadKeyPair Function

LoadKeyPair was not reading the contents of the key file properly. It
was "over-reading" it, leading to having the contents of the file along
with undefined characters in the buffer up to kMaxBufferSize. In turn,
the browser was never able to load the key properly, making it think
that there was no key on the device (hence always re-creating it).

- Changed the implementation to make use of file reading utils,
- Added unit tests reading from a real test file,
- Removed NOTIMPLEMENTED from GetTpmBackedKeyProvider for both Linux and
  Mac, as not having a TPM is a handled case.

Fixed: b:228488796,b:228492179
Change-Id: Ic47a8d5aa752a30b2341264cd653270583c9166e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3579124
Auto-Submit: Sebastien Lalancette <seblalancette@chromium.org>
Reviewed-by: Dominique Fauteux-Chapleau <domfc@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#990991}

chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/BUILD.gn

@@ -42,6 +42,8 @@ source_set("persistence") {
       "//chrome/browser/enterprise/connectors/device_trust/key_management/core:constants",
       "//third_party/abseil-cpp:absl",
     ]
+
+    friend = [ ":unit_tests" ]
   }
 }
 
@@ -70,3 +72,17 @@ source_set("test_support") {
     "//crypto",
   ]
 }
+
+if (is_linux) {
+  source_set("unit_tests") {
+    testonly = true
+    sources = [ "linux_key_persistence_delegate_unittest.cc" ]
+
+    deps = [
+      ":persistence",
+      "//base",
+      "//components/policy/proto",
+      "//testing/gtest",
+    ]
+  }
+}

chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/linux_key_persistence_delegate.cc

@@ -18,6 +18,7 @@
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
+#include "base/no_destructor.h"
 #include "base/notreached.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/syslog_logging.h"
@@ -52,7 +53,16 @@ base::FilePath::CharType kDirPolicyPath[] =
     FILE_PATH_LITERAL("/etc/chromium/policies");
 #endif
 
+absl::optional<base::FilePath>& GetTestFilePathStorage() {
+  static base::NoDestructor<absl::optional<base::FilePath>> storage;
+  return *storage;
+}
+
 base::FilePath GetSigningKeyFilePath() {
+  auto& storage = GetTestFilePathStorage();
+  if (storage) {
+    return storage.value();
+  }
   base::FilePath path(kDirPolicyPath);
   return path.Append(constants::kSigningKeyFilePath);
 }
@@ -142,20 +152,14 @@ bool LinuxKeyPersistenceDelegate::StoreKeyPair(
 }
 
 KeyPersistenceDelegate::KeyInfo LinuxKeyPersistenceDelegate::LoadKeyPair() {
-  base::File file =
-      OpenSigningKeyFile(base::File::FLAG_OPEN | base::File::FLAG_READ);
-  if (!file.IsValid())
-    return invalid_key_info();
-
-  // Read key info.
-  char keyinfo_str[kMaxBufferSize];
-  int bytes_read = file.ReadAtCurrentPos(keyinfo_str, kMaxBufferSize);
-  if (bytes_read <= 0) {
+  std::string file_content;
+  if (!base::ReadFileToStringWithMaxSize(GetSigningKeyFilePath(), &file_content,
+                                         kMaxBufferSize)) {
     return invalid_key_info();
   }
 
   // Get dictionary key info.
-  auto keyinfo = base::JSONReader::Read(keyinfo_str);
+  auto keyinfo = base::JSONReader::Read(file_content);
   if (!keyinfo || !keyinfo->is_dict()) {
     return invalid_key_info();
   }
@@ -189,8 +193,15 @@ KeyPersistenceDelegate::KeyInfo LinuxKeyPersistenceDelegate::LoadKeyPair() {
 
 std::unique_ptr<crypto::UnexportableKeyProvider>
 LinuxKeyPersistenceDelegate::GetTpmBackedKeyProvider() {
-  NOTIMPLEMENTED();  // TODO (http://b/210343211)
+  // TODO (http://b/210343211)
   return nullptr;
 }
 
+// static
+void LinuxKeyPersistenceDelegate::SetFilePathForTesting(
+    const base::FilePath& file_path) {
+  auto& storage = GetTestFilePathStorage();
+  storage.emplace(file_path);
+}
+
 }  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/linux_key_persistence_delegate.h

@@ -9,6 +9,10 @@
 #include "chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/key_persistence_delegate.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
+namespace base {
+class FilePath;
+}  // namespace base
+
 namespace enterprise_connectors {
 
 // Linux implementation of the KeyPersistenceDelegate interface.
@@ -26,6 +30,12 @@ class LinuxKeyPersistenceDelegate : public KeyPersistenceDelegate {
       override;
 
  private:
+  friend class LinuxKeyPersistenceDelegateTest;
+
+  // Statically sets a file path to be used by LinuxKeyPersistenceDelegate
+  // instances when retrieve the key file path.
+  static void SetFilePathForTesting(const base::FilePath& file_path);
+
   // Signing key file instance used for handling concurrency during the
   // key rotation process.
   absl::optional<base::File> locked_file_;

chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/linux_key_persistence_delegate_unittest.cc

@@ -0,0 +1,172 @@
+// Copyright 2022 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/linux_key_persistence_delegate.h"
+
+#include "base/base64.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/strings/stringprintf.h"
+#include "components/policy/proto/device_management_backend.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+base::FilePath::CharType kFileName[] = FILE_PATH_LITERAL("test_file");
+
+// Represents an OS key.
+constexpr char kValidKeyWrappedBase64[] =
+    "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg3VGyKUYrI0M5VOGIw0dh3D0s26"
+    "0xeKGcOKZ76A+LTQuhRANCAAQ8rmn96lycvM/"
+    "WTQn4FZnjucsKdj2YrUkcG42LWoC2WorIp8BETdwYr2OhGAVBmSVpg9iyi5gtZ9JGZzMceWOJ";
+
+// String containing invalid base64 characters, like % and the whitespace.
+constexpr char kInvalidBase64String[] = "? %";
+
+constexpr char kValidTPMKeyFileContent[] =
+    "{\"signingKey\":"
+    "\"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg3VGyKUYrI0M5VOGIw0dh3D0s"
+    "260xeKGcOKZ76A+LTQuhRANCAAQ8rmn96lycvM/"
+    "WTQn4FZnjucsKdj2YrUkcG42LWoC2WorIp8BETdwYr2OhGAVBmSVpg9iyi5gtZ9JGZzMceWOJ"
+    "\",\"trustLevel\":1}";
+constexpr char kValidOSKeyFileContent[] =
+    "{\"signingKey\":"
+    "\"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg3VGyKUYrI0M5VOGIw0dh3D0s"
+    "260xeKGcOKZ76A+LTQuhRANCAAQ8rmn96lycvM/"
+    "WTQn4FZnjucsKdj2YrUkcG42LWoC2WorIp8BETdwYr2OhGAVBmSVpg9iyi5gtZ9JGZzMceWOJ"
+    "\",\"trustLevel\":2}";
+constexpr char kInvalidTrustLevelKeyFileContent[] =
+    "{\"signingKey\":"
+    "\"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg3VGyKUYrI0M5VOGIw0dh3D0s"
+    "260xeKGcOKZ76A+LTQuhRANCAAQ8rmn96lycvM/"
+    "WTQn4FZnjucsKdj2YrUkcG42LWoC2WorIp8BETdwYr2OhGAVBmSVpg9iyi5gtZ9JGZzMceWOJ"
+    "\",\"trustLevel\":100}";
+
+std::vector<uint8_t> ParseKeyWrapped(base::StringPiece encoded_wrapped) {
+  std::string decoded_key;
+  if (!base::Base64Decode(encoded_wrapped, &decoded_key)) {
+    return std::vector<uint8_t>();
+  }
+
+  return std::vector<uint8_t>(decoded_key.begin(), decoded_key.end());
+}
+
+}  // namespace
+
+namespace enterprise_connectors {
+
+using BPKUR = enterprise_management::BrowserPublicKeyUploadRequest;
+
+class LinuxKeyPersistenceDelegateTest : public testing::Test {
+ public:
+  LinuxKeyPersistenceDelegateTest() {
+    EXPECT_TRUE(scoped_dir_.CreateUniqueTempDir());
+    LinuxKeyPersistenceDelegate::SetFilePathForTesting(GetKeyFilePath());
+  }
+
+ protected:
+  base::FilePath GetKeyFilePath() {
+    return scoped_dir_.GetPath().Append(kFileName);
+  }
+
+  bool CreateFile(base::StringPiece content) {
+    return base::WriteFile(GetKeyFilePath(), content);
+  }
+
+  base::ScopedTempDir scoped_dir_;
+  LinuxKeyPersistenceDelegate persistence_delegate_;
+};
+
+// Tests trying to load a key when there is no file.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_NoKeyFile) {
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::KEY_TRUST_LEVEL_UNSPECIFIED);
+  EXPECT_TRUE(wrapped.empty());
+}
+
+// Tests loading a valid OS key from a key file.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_ValidOSKeyFile) {
+  ASSERT_TRUE(CreateFile(kValidOSKeyFileContent));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::CHROME_BROWSER_OS_KEY);
+  EXPECT_FALSE(wrapped.empty());
+  EXPECT_EQ(wrapped, ParseKeyWrapped(kValidKeyWrappedBase64));
+}
+
+// Tests loading a valid TPM key from a key file.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_ValidTPMKeyFile) {
+  ASSERT_TRUE(CreateFile(kValidTPMKeyFileContent));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::CHROME_BROWSER_TPM_KEY);
+  EXPECT_FALSE(wrapped.empty());
+  EXPECT_EQ(wrapped, ParseKeyWrapped(kValidKeyWrappedBase64));
+}
+
+// Tests loading a key from a key file with an invalid trust level.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_InvalidTrustLevel) {
+  ASSERT_TRUE(CreateFile(kInvalidTrustLevelKeyFileContent));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::KEY_TRUST_LEVEL_UNSPECIFIED);
+  EXPECT_TRUE(wrapped.empty());
+}
+
+// Tests loading a key from a key file when the signing key property is missing.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_MissingSigningKey) {
+  const char file_content[] = "{\"trustLevel\":\"2\"}";
+
+  ASSERT_TRUE(CreateFile(file_content));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::KEY_TRUST_LEVEL_UNSPECIFIED);
+  EXPECT_TRUE(wrapped.empty());
+}
+
+// Tests loading a key from a key file when the trust level property is missing.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_MissingTrustLevel) {
+  const std::string file_content =
+      base::StringPrintf("{\"signingKey\":\"%s\"}", kValidKeyWrappedBase64);
+
+  ASSERT_TRUE(CreateFile(file_content));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::KEY_TRUST_LEVEL_UNSPECIFIED);
+  EXPECT_TRUE(wrapped.empty());
+}
+
+// Tests loading a key from a key file when the file content is invalid (not a
+// JSON dictionary).
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_InvalidContent) {
+  const char file_content[] = "just some text";
+
+  ASSERT_TRUE(CreateFile(file_content));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::KEY_TRUST_LEVEL_UNSPECIFIED);
+  EXPECT_TRUE(wrapped.empty());
+}
+
+// Tests loading a key from a key file when there is a valid key, but the key
+// file contains random trailing values.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_TrailingGibberish) {
+  const std::string file_content = base::StringPrintf(
+      "{\"signingKey\":\"%s\",\"trustLevel\":\"2\"}someother random content",
+      kValidKeyWrappedBase64);
+
+  ASSERT_TRUE(CreateFile(file_content));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::KEY_TRUST_LEVEL_UNSPECIFIED);
+  EXPECT_TRUE(wrapped.empty());
+}
+
+// Tests loading a key from a key file when the key value is not a valid
+// base64 encoded string.
+TEST_F(LinuxKeyPersistenceDelegateTest, LoadKeyPair_KeyNotBase64) {
+  const std::string file_content = base::StringPrintf(
+      "{\"signingKey\":\"%s\",\"trustLevel\":\"2\"}", kInvalidBase64String);
+
+  ASSERT_TRUE(CreateFile(file_content));
+  auto [trust_level, wrapped] = persistence_delegate_.LoadKeyPair();
+  EXPECT_EQ(trust_level, BPKUR::KEY_TRUST_LEVEL_UNSPECIFIED);
+  EXPECT_TRUE(wrapped.empty());
+}
+
+}  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/mac_key_persistence_delegate.cc

@@ -28,7 +28,7 @@ KeyPersistenceDelegate::KeyInfo MacKeyPersistenceDelegate::LoadKeyPair() {
 
 std::unique_ptr<crypto::UnexportableKeyProvider>
 MacKeyPersistenceDelegate::GetTpmBackedKeyProvider() {
-  NOTIMPLEMENTED();
+  // Mac OS does not expose TPM support.
   return nullptr;
 }
 

chrome/test/BUILD.gn

@@ -8019,6 +8019,10 @@ test("unit_tests") {
     ]
   }
 
+  if (is_linux) {
+    deps += [ "../browser/enterprise/connectors/device_trust/key_management/core/persistence:unit_tests" ]
+  }
+
   if (is_chromeos_ash) {
     sources += [ "../browser/enterprise/connectors/device_trust/attestation/ash/ash_attestation_service_unittest.cc" ]
     deps += [