Update device trust service flow to use new signals dict

This updates the entire signals flow to use a base::Value::Dict instead of a signals proto. This cl focuses on changing the device trust signals flow to use a signals dictionary instead of a signals protobuf however a mapping is needed since VA does not support this yet. The purpose of this change is to decrease the complexity of adding new signals since CrOS and device trust protos will no longer need to be updated when changes to signals occurs. The eventual outcome is to no longer support protobufs for signals collection. There will be a follow up CL when VA is updated to support this signals collection change to convert signals to a JSON string and send this to VA and to delete the mapping. Bug: b/229973698 Change-Id: I2ee79ae374f397ff1dfbca379aa6c106fdc97c69 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3606542 Reviewed-by: Matthew Denton <mpdenton@chromium.org> Reviewed-by: Sebastien Lalancette <seblalancette@chromium.org> Commit-Queue: Hamda Mare <hmare@google.com> Cr-Commit-Position: refs/heads/main@{#1001658}
chromium · May 10, 2022 · e1773a4 · e1773a4
1 parent bd11c29
commit e1773a4
Show file tree

Hide file tree

Showing 40 changed files with 241 additions and 296 deletions.
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -5989,6 +5989,7 @@ static_library("browser") {
       "//chrome/browser/enterprise/connectors/device_trust/attestation/common/proto:interface_proto",
       "//chrome/browser/enterprise/connectors/device_trust/key_management/core",
       "//chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence",
+      "//components/device_signals/core/common",
     ]
   }
 

diff --git a/chrome/browser/enterprise/connectors/device_trust/attestation/ash/BUILD.gn b/chrome/browser/enterprise/connectors/device_trust/attestation/ash/BUILD.gn
@@ -15,6 +15,7 @@ source_set("ash") {
     "//chrome/browser/ash",
     "//chrome/browser/enterprise/connectors/device_trust/common",
     "//chrome/browser/profiles:profile",
+    "//components/device_signals/core/common",
   ]
 
   public_deps = [

diff --git a/chrome/browser/enterprise/connectors/device_trust/attestation/ash/ash_attestation_service.cc b/chrome/browser/enterprise/connectors/device_trust/attestation/ash/ash_attestation_service.cc
@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/check.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "chrome/browser/ash/attestation/tpm_challenge_key_result.h"
 #include "chrome/browser/ash/attestation/tpm_challenge_key_with_timeout.h"
 #include "chrome/browser/enterprise/connectors/device_trust/attestation/common/attestation_utils.h"
@@ -46,9 +47,8 @@ AshAttestationService::~AshAttestationService() = default;
 
 void AshAttestationService::BuildChallengeResponseForVAChallenge(
     const std::string& serialized_signed_challenge,
-    std::unique_ptr<attestation::DeviceTrustSignals> signals,
+    base::Value::Dict signals,
     AttestationCallback callback) {
-  DCHECK(signals);
   auto tpm_key_challenger =
       std::make_unique<ash::attestation::TpmChallengeKeyWithTimeout>();
   auto* tpm_key_challenger_ptr = tpm_key_challenger.get();
@@ -58,7 +58,8 @@ void AshAttestationService::BuildChallengeResponseForVAChallenge(
                      weak_factory_.GetWeakPtr(), std::move(tpm_key_challenger),
                      std::move(callback)),
       serialized_signed_challenge, /*register_key=*/false,
-      /*key_name_for_spkac=*/std::string(), /*signals=*/*signals);
+      /*key_name_for_spkac=*/std::string(),
+      /*signals=*/*DictionarySignalsToProtobufSignals(signals));
 }
 
 void AshAttestationService::ReturnResult(

diff --git a/chrome/browser/enterprise/connectors/device_trust/attestation/ash/ash_attestation_service.h b/chrome/browser/enterprise/connectors/device_trust/attestation/ash/ash_attestation_service.h
@@ -32,7 +32,7 @@ class AshAttestationService : public AttestationService {
   // AttestationService:
   void BuildChallengeResponseForVAChallenge(
       const std::string& serialized_signed_challenge,
-      std::unique_ptr<attestation::DeviceTrustSignals> signals,
+      base::Value::Dict signals,
       AttestationCallback callback) override;
 
  private:

diff --git a/...er/enterprise/connectors/device_trust/attestation/ash/ash_attestation_service_unittest.cc b/...er/enterprise/connectors/device_trust/attestation/ash/ash_attestation_service_unittest.cc
@@ -23,6 +23,7 @@
 #include "chromeos/dbus/attestation/attestation_ca.pb.h"
 #include "chromeos/dbus/attestation/attestation_client.h"
 #include "chromeos/dbus/constants/attestation_constants.h"
+#include "components/device_signals/core/common/signals_constants.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -109,10 +110,11 @@ class AshAttestationServiceTest : public testing::Test {
         std::make_unique<AshAttestationService>(&test_profile_);
   }
 
-  std::unique_ptr<attestation::DeviceTrustSignals> CreateSignals() {
-    auto signals = std::make_unique<attestation::DeviceTrustSignals>();
-    signals->set_device_id(kDeviceId);
-    signals->set_obfuscated_customer_id(kObfuscatedCustomerId);
+  base::Value::Dict CreateSignals() {
+    base::Value::Dict signals;
+    signals.Set(device_signals::names::kDeviceId, kDeviceId);
+    signals.Set(device_signals::names::kObfuscatedCustomerId,
+                kObfuscatedCustomerId);
     return signals;
   }
 
@@ -124,8 +126,6 @@ class AshAttestationServiceTest : public testing::Test {
 };
 
 TEST_F(AshAttestationServiceTest, BuildChallengeResponse_Success) {
-  auto signals = CreateSignals();
-
   base::RunLoop run_loop;
   auto callback =
       base::BindLambdaForTesting([&](const std::string& challenge_response) {
@@ -150,7 +150,7 @@ TEST_F(AshAttestationServiceTest, BuildChallengeResponse_Success) {
               kFakeResponse)));
 
   attestation_service_->BuildChallengeResponseForVAChallenge(
-      protoChallenge, std::move(signals), std::move(callback));
+      protoChallenge, CreateSignals(), std::move(callback));
   run_loop.Run();
 }
 

diff --git a/chrome/browser/enterprise/connectors/device_trust/attestation/common/attestation_service.h b/chrome/browser/enterprise/connectors/device_trust/attestation/common/attestation_service.h
@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/callback.h"
+#include "base/values.h"
 #include "chrome/browser/enterprise/connectors/device_trust/attestation/common/signals_type.h"
 
 namespace enterprise_connectors {
@@ -27,7 +28,7 @@ class AttestationService {
   // `callback` with an empty string.
   virtual void BuildChallengeResponseForVAChallenge(
       const std::string& serialized_signed_challenge,
-      std::unique_ptr<SignalsType> signals,
+      base::Value::Dict signals,
       AttestationCallback callback) = 0;
 };
 

diff --git a/.../browser/enterprise/connectors/device_trust/attestation/common/mock_attestation_service.h b/.../browser/enterprise/connectors/device_trust/attestation/common/mock_attestation_service.h
@@ -7,8 +7,8 @@
 
 #include <string>
 
+#include "base/values.h"
 #include "chrome/browser/enterprise/connectors/device_trust/attestation/common/attestation_service.h"
-#include "chrome/browser/enterprise/connectors/device_trust/attestation/common/signals_type.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace enterprise_connectors {
@@ -23,9 +23,7 @@ class MockAttestationService : public AttestationService {
 
   MOCK_METHOD(void,
               BuildChallengeResponseForVAChallenge,
-              (const std::string&,
-               std::unique_ptr<SignalsType>,
-               AttestationCallback),
+              (const std::string&, base::Value::Dict, AttestationCallback),
               (override));
 };
 

diff --git a/...ser/enterprise/connectors/device_trust/attestation/desktop/desktop_attestation_service.cc b/...ser/enterprise/connectors/device_trust/attestation/desktop/desktop_attestation_service.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/enterprise/connectors/device_trust/attestation/common/proto/device_trust_attestation_ca.pb.h"
 #include "chrome/browser/enterprise/connectors/device_trust/attestation/desktop/crypto_utility.h"
 #include "chrome/browser/enterprise/connectors/device_trust/common/metrics_utils.h"
+#include "components/device_signals/core/common/signals_constants.h"
 #include "components/enterprise/browser/device_trust/device_trust_key_manager.h"
 #include "crypto/random.h"
 #include "crypto/unexportable_key.h"
@@ -102,16 +103,14 @@ DesktopAttestationService::~DesktopAttestationService() = default;
 // - Reply to callback.
 void DesktopAttestationService::BuildChallengeResponseForVAChallenge(
     const std::string& serialized_signed_challenge,
-    std::unique_ptr<DeviceTrustSignals> signals,
+    base::Value::Dict signals,
     AttestationCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(signals);
 
   // Signals have to at least have the non-empty device ID and obfuscated
   // customer ID.
-  if (!signals || !signals->has_device_id() || signals->device_id().empty() ||
-      !signals->has_obfuscated_customer_id() ||
-      signals->obfuscated_customer_id().empty()) {
+  if (!signals.FindString(device_signals::names::kDeviceId) ||
+      !signals.FindString(device_signals::names::kObfuscatedCustomerId)) {
     LogAttestationResult(DTAttestationResult::kMissingCoreSignals);
     std::move(callback).Run(std::string());
     return;
@@ -125,7 +124,7 @@ void DesktopAttestationService::BuildChallengeResponseForVAChallenge(
 
 void DesktopAttestationService::OnPublicKeyExported(
     const std::string& serialized_signed_challenge,
-    std::unique_ptr<DeviceTrustSignals> signals,
+    base::Value::Dict signals,
     AttestationCallback callback,
     absl::optional<std::string> exported_key) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -160,7 +159,7 @@ void DesktopAttestationService::OnPublicKeyExported(
 void DesktopAttestationService::OnChallengeValidated(
     const SignedData& signed_data,
     const std::string& exported_public_key,
-    std::unique_ptr<DeviceTrustSignals> signals,
+    base::Value::Dict signals,
     AttestationCallback callback,
     bool is_va_challenge) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -177,9 +176,14 @@ void DesktopAttestationService::OnChallengeValidated(
   KeyInfo key_info;
   key_info.set_key_type(CBCM);
   key_info.set_browser_instance_public_key(exported_public_key);
-  key_info.set_device_id(signals->device_id());
-  key_info.set_customer_id(signals->obfuscated_customer_id());
-  key_info.set_allocated_device_trust_signals(signals.release());
+  key_info.set_device_id(*signals.FindString(device_signals::names::kDeviceId));
+  key_info.set_customer_id(
+      *signals.FindString(device_signals::names::kObfuscatedCustomerId));
+
+  // VA currently only accepts the signals in a protobuf format.
+  std::unique_ptr<DeviceTrustSignals> signals_proto =
+      DictionarySignalsToProtobufSignals(signals);
+  key_info.set_allocated_device_trust_signals(signals_proto.release());
 
   std::string serialized_key_info;
   if (!key_info.SerializeToString(&serialized_key_info)) {

diff --git a/...wser/enterprise/connectors/device_trust/attestation/desktop/desktop_attestation_service.h b/...wser/enterprise/connectors/device_trust/attestation/desktop/desktop_attestation_service.h
@@ -20,7 +20,6 @@
 namespace enterprise_connectors {
 
 class DeviceTrustKeyManager;
-class DeviceTrustSignals;
 
 // This class is in charge of handling the key pair used for attestation. Also
 // provides the methods needed in the handshake between Chrome, an IdP and
@@ -33,22 +32,22 @@ class DesktopAttestationService : public AttestationService {
   // AttestationService:
   void BuildChallengeResponseForVAChallenge(
       const std::string& serialized_signed_challenge,
-      std::unique_ptr<DeviceTrustSignals> signals,
+      base::Value::Dict signals,
       AttestationCallback callback) override;
 
  private:
   void OnChallengeParsed(AttestationCallback callback,
-                         std::unique_ptr<DeviceTrustSignals> signals,
+                         base::Value::Dict signals,
                          const std::string& serialized_signed_challenge);
 
   void OnPublicKeyExported(const std::string& serialized_signed_challenge,
-                           std::unique_ptr<DeviceTrustSignals> signals,
+                           base::Value::Dict signals,
                            AttestationCallback callback,
                            absl::optional<std::string> exported_key);
 
   void OnChallengeValidated(const SignedData& signed_data,
                             const std::string& exported_public_key,
-                            std::unique_ptr<DeviceTrustSignals> signals,
+                            base::Value::Dict signals,
                             AttestationCallback callback,
                             bool is_va_challenge);
 

diff --git a/...prise/connectors/device_trust/attestation/desktop/desktop_attestation_service_unittest.cc b/...prise/connectors/device_trust/attestation/desktop/desktop_attestation_service_unittest.cc
@@ -17,6 +17,7 @@
 #include "chrome/browser/enterprise/connectors/device_trust/key_management/browser/device_trust_key_manager_impl.h"
 #include "chrome/browser/enterprise/connectors/device_trust/key_management/browser/mock_key_rotation_launcher.h"
 #include "chrome/browser/enterprise/connectors/device_trust/key_management/core/persistence/scoped_key_persistence_delegate_factory.h"
+#include "components/device_signals/core/common/signals_constants.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -96,10 +97,11 @@ class DesktopAttestationServiceTest : public testing::Test {
         std::make_unique<DesktopAttestationService>(key_manager_.get());
   }
 
-  std::unique_ptr<DeviceTrustSignals> CreateSignals() {
-    auto signals = std::make_unique<DeviceTrustSignals>();
-    signals->set_device_id(kDeviceId);
-    signals->set_obfuscated_customer_id(kObfuscatedCustomerId);
+  base::Value::Dict CreateSignals() {
+    base::Value::Dict signals;
+    signals.Set(device_signals::names::kDeviceId, kDeviceId);
+    signals.Set(device_signals::names::kObfuscatedCustomerId,
+                kObfuscatedCustomerId);
     return signals;
   }
 
@@ -112,7 +114,6 @@ class DesktopAttestationServiceTest : public testing::Test {
 TEST_F(DesktopAttestationServiceTest, BuildChallengeResponse_Success) {
   // TODO(crbug.com/1208881): Add signals and validate they effectively get
   // added to the signed data.
-  auto signals = CreateSignals();
 
   base::RunLoop run_loop;
   auto callback = base::BindLambdaForTesting(
@@ -126,7 +127,7 @@ TEST_F(DesktopAttestationServiceTest, BuildChallengeResponse_Success) {
       });
 
   attestation_service_->BuildChallengeResponseForVAChallenge(
-      GetSerializedSignedChallenge(), std::move(signals), std::move(callback));
+      GetSerializedSignedChallenge(), CreateSignals(), std::move(callback));
   run_loop.Run();
 }
 

diff --git a/chrome/browser/enterprise/connectors/device_trust/device_trust_service.cc b/chrome/browser/enterprise/connectors/device_trust/device_trust_service.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/enterprise/connectors/device_trust/device_trust_service.h"
 
 #include "base/base64.h"
+#include "base/values.h"
 #include "chrome/browser/enterprise/connectors/connectors_prefs.h"
 #include "chrome/browser/enterprise/connectors/device_trust/attestation/common/attestation_service.h"
 #include "chrome/browser/enterprise/connectors/device_trust/attestation/common/attestation_utils.h"
@@ -102,7 +103,7 @@ void DeviceTrustService::GetSignals(CollectSignalsCallback callback) {
 void DeviceTrustService::OnSignalsCollected(
     const std::string& serialized_signed_challenge,
     AttestationCallback callback,
-    std::unique_ptr<SignalsType> signals) {
+    base::Value::Dict signals) {
   LogAttestationFunnelStep(DTAttestationFunnelStep::kSignalsCollected);
 
   attestation_service_->BuildChallengeResponseForVAChallenge(

diff --git a/chrome/browser/enterprise/connectors/device_trust/device_trust_service.h b/chrome/browser/enterprise/connectors/device_trust/device_trust_service.h
@@ -10,7 +10,7 @@
 
 #include "base/callback.h"
 #include "base/memory/raw_ptr.h"
-#include "chrome/browser/enterprise/connectors/device_trust/attestation/common/signals_type.h"
+#include "base/values.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "services/data_decoder/public/cpp/data_decoder.h"
 
@@ -55,8 +55,7 @@ class DeviceTrustService : public KeyedService {
   virtual bool Watches(const GURL& url) const;
 
   // Collects device trust signals and returns them via `callback`.
-  void GetSignals(
-      base::OnceCallback<void(std::unique_ptr<SignalsType>)> callback);
+  void GetSignals(base::OnceCallback<void(const base::Value::Dict)> callback);
 
   // Parses the `challenge` response and returns it via a `callback`.
   void ParseJsonChallenge(const std::string& challenge,
@@ -71,7 +70,7 @@ class DeviceTrustService : public KeyedService {
                          const std::string& serialized_signed_challenge);
   void OnSignalsCollected(const std::string& challenge,
                           AttestationCallback callback,
-                          std::unique_ptr<SignalsType> signals);
+                          base::Value::Dict signals);
 
   std::unique_ptr<AttestationService> attestation_service_;
   std::unique_ptr<SignalsService> signals_service_;

diff --git a/chrome/browser/enterprise/connectors/device_trust/device_trust_service_unittest.cc b/chrome/browser/enterprise/connectors/device_trust/device_trust_service_unittest.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/enterprise/connectors/device_trust/device_trust_connector_service.h"
 #include "chrome/browser/enterprise/connectors/device_trust/device_trust_features.h"
 #include "chrome/browser/enterprise/connectors/device_trust/signals/mock_signals_service.h"
+#include "components/device_signals/core/common/signals_constants.h"
 #include "components/prefs/testing_pref_service.h"
 #include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -152,22 +153,22 @@ TEST_P(DeviceTrustServiceTest, BuildChallengeResponse) {
 
   std::string fake_device_id = "fake_device_id";
   EXPECT_CALL(*mock_signals_service_, CollectSignals(_))
-      .WillOnce(
-          Invoke([&fake_device_id](
-                     base::OnceCallback<void(std::unique_ptr<SignalsType>)>
-                         signals_callback) {
-            auto fake_signals = std::make_unique<SignalsType>();
-            fake_signals->set_device_id(fake_device_id);
-            std::move(signals_callback).Run(std::move(fake_signals));
+      .WillOnce(Invoke(
+          [&fake_device_id](
+              base::OnceCallback<void(base::Value::Dict)> signals_callback) {
+            auto fake_signals = std::make_unique<base::Value::Dict>();
+            fake_signals->Set(device_signals::names::kDeviceId, fake_device_id);
+            std::move(signals_callback).Run(std::move(*fake_signals));
           }));
 
   EXPECT_CALL(*mock_attestation_service_,
               BuildChallengeResponseForVAChallenge(
-                  GetSerializedSignedChallenge(kJsonChallenge), NotNull(), _))
+                  GetSerializedSignedChallenge(kJsonChallenge), _, _))
       .WillOnce(Invoke([&fake_device_id](const std::string& challenge,
-                                         std::unique_ptr<SignalsType> signals,
+                                         const base::Value::Dict signals,
                                          AttestationCallback callback) {
-        EXPECT_EQ(signals->device_id(), fake_device_id);
+        EXPECT_EQ(signals.FindString(device_signals::names::kDeviceId)->c_str(),
+                  fake_device_id);
         std::move(callback).Run(challenge);
       }));
 

diff --git a/...rowser/enterprise/connectors/device_trust/signals/decorators/ash/ash_signals_decorator.cc b/...rowser/enterprise/connectors/device_trust/signals/decorators/ash/ash_signals_decorator.cc
@@ -6,9 +6,11 @@
 
 #include "base/check.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "chrome/browser/ash/policy/core/browser_policy_connector_ash.h"
 #include "chrome/browser/enterprise/connectors/device_trust/signals/decorators/common/metrics_utils.h"
 #include "chrome/browser/enterprise/connectors/device_trust/signals/decorators/common/signals_decorator.h"
+#include "components/device_signals/core/common/signals_constants.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 
 namespace enterprise_connectors {
@@ -29,16 +31,17 @@ AshSignalsDecorator::AshSignalsDecorator(
 
 AshSignalsDecorator::~AshSignalsDecorator() = default;
 
-void AshSignalsDecorator::Decorate(attestation::DeviceTrustSignals& signals,
+void AshSignalsDecorator::Decorate(base::Value::Dict& signals,
                                    base::OnceClosure done_closure) {
   auto start_time = base::TimeTicks::Now();
 
   // Directory API ID is the same thing as permanent device ID, which is generated by the server.
-  signals.set_device_id(browser_policy_connector_->GetDirectoryApiID());
-  signals.set_obfuscated_customer_id(
-      browser_policy_connector_->GetObfuscatedCustomerID());
-  signals.set_enrollment_domain(
-      browser_policy_connector_->GetEnterpriseDomainManager());
+  signals.Set(device_signals::names::kDeviceId,
+              browser_policy_connector_->GetDirectoryApiID());
+  signals.Set(device_signals::names::kObfuscatedCustomerId,
+              browser_policy_connector_->GetObfuscatedCustomerID());
+  signals.Set(device_signals::names::kEnrollmentDomain,
+              browser_policy_connector_->GetEnterpriseDomainManager());
 
   LogSignalsCollectionLatency(kLatencyHistogramVariant, start_time);