Set the type of certificates when uploading them.

This will allow certificates to be typed even if we send an empty
PEM certificate which the server can act on.

Bug: 778535
Test: unit_tests
Change-Id: Iadc2d94c4cdd558dc46086b7cea629f7f92e8ae7
Reviewed-on: https://chromium-review.googlesource.com/822164Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Yves Arrouye <drcrash@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524108}

chrome/browser/chromeos/attestation/attestation_policy_observer.cc

@@ -254,7 +254,7 @@ void AttestationPolicyObserver::CheckCertificateExpiry(
 
 void AttestationPolicyObserver::UploadCertificate(
     const std::string& pem_certificate_chain) {
-  policy_client_->UploadCertificate(
+  policy_client_->UploadEnterpriseMachineCertificate(
       pem_certificate_chain,
       base::Bind(&AttestationPolicyObserver::OnUploadComplete,
                  weak_factory_.GetWeakPtr()));

chrome/browser/chromeos/attestation/attestation_policy_observer.h

@@ -67,7 +67,7 @@ class AttestationPolicyObserver {
   // and, if so, gets a new one. If not renewing, calls CheckIfUploaded.
   void CheckCertificateExpiry(const std::string& pem_certificate_chain);
 
-  // Uploads a certificate to the policy server.
+  // Uploads a machine certificate to the policy server.
   void UploadCertificate(const std::string& pem_certificate_chain);
 
   // Checks if a certificate has already been uploaded and, if not, upload.

chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc

@@ -87,8 +87,8 @@ class AttestationPolicyObserverTest : public ::testing::Test {
     // status in the key payload matches the upload operation.
     bool new_key = (mock_options & MOCK_NEW_KEY);
     if (new_key || !key_uploaded) {
-      EXPECT_CALL(policy_client_,
-                  UploadCertificate(new_key ? "fake_cert" : certificate, _))
+      EXPECT_CALL(policy_client_, UploadEnterpriseMachineCertificate(
+                                      new_key ? "fake_cert" : certificate, _))
           .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
     }
 

chrome/browser/chromeos/attestation/enrollment_policy_observer.cc

@@ -159,7 +159,7 @@ void EnrollmentPolicyObserver::GetEnrollmentCertificate() {
 
 void EnrollmentPolicyObserver::UploadCertificate(
     const std::string& pem_certificate_chain) {
-  policy_client_->UploadCertificate(
+  policy_client_->UploadEnterpriseEnrollmentCertificate(
       pem_certificate_chain,
       base::Bind(&EnrollmentPolicyObserver::OnUploadComplete,
                  weak_factory_.GetWeakPtr()));

chrome/browser/chromeos/attestation/enrollment_policy_observer.h

@@ -57,7 +57,7 @@ class EnrollmentPolicyObserver {
   // Gets an enrollment certificate.
   void GetEnrollmentCertificate();
 
-  // Uploads a certificate to the policy server.
+  // Uploads an enrollment certificate to the policy server.
   void UploadCertificate(const std::string& pem_certificate_chain);
 
   // Called when a certificate upload operation completes.  On success, |status|

chrome/browser/chromeos/attestation/enrollment_policy_observer_unittest.cc

@@ -57,7 +57,8 @@ class EnrollmentPolicyObserverTest : public ::testing::Test {
   void SetupMocks() {
     EXPECT_CALL(attestation_flow_, GetCertificate(_, _, _, _, _))
         .WillOnce(WithArgs<4>(Invoke(CertCallbackSuccess)));
-    EXPECT_CALL(policy_client_, UploadCertificate("fake_cert", _))
+    EXPECT_CALL(policy_client_,
+                UploadEnterpriseEnrollmentCertificate("fake_cert", _))
         .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
   }
 

components/policy/core/common/cloud/cloud_policy_client.cc

@@ -345,26 +345,20 @@ void CloudPolicyClient::Unregister() {
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void CloudPolicyClient::UploadCertificate(
+void CloudPolicyClient::UploadEnterpriseMachineCertificate(
     const std::string& certificate_data,
     const CloudPolicyClient::StatusCallback& callback) {
-  CHECK(is_registered());
-  std::unique_ptr<DeviceManagementRequestJob> request_job(
-      service_->CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_CERTIFICATE,
-                          GetRequestContext()));
-  request_job->SetDMToken(dm_token_);
-  request_job->SetClientID(client_id_);
-
-  em::DeviceManagementRequest* request = request_job->GetRequest();
-  request->mutable_cert_upload_request()->set_device_certificate(
-      certificate_data);
-
-  const DeviceManagementRequestJob::Callback job_callback =
-      base::Bind(&CloudPolicyClient::OnCertificateUploadCompleted,
-                 weak_ptr_factory_.GetWeakPtr(), request_job.get(), callback);
+  UploadCertificate(certificate_data,
+                    em::DeviceCertUploadRequest::ENTERPRISE_MACHINE_CERTIFICATE,
+                    callback);
+}
 
-  request_jobs_.push_back(std::move(request_job));
-  request_jobs_.back()->Start(job_callback);
+void CloudPolicyClient::UploadEnterpriseEnrollmentCertificate(
+    const std::string& certificate_data,
+    const CloudPolicyClient::StatusCallback& callback) {
+  UploadCertificate(
+      certificate_data,
+      em::DeviceCertUploadRequest::ENTERPRISE_ENROLLMENT_CERTIFICATE, callback);
 }
 
 void CloudPolicyClient::UploadDeviceStatus(
@@ -559,6 +553,31 @@ int CloudPolicyClient::GetActiveRequestCountForTest() const {
   return request_jobs_.size();
 }
 
+void CloudPolicyClient::UploadCertificate(
+    const std::string& certificate_data,
+    em::DeviceCertUploadRequest::CertificateType certificate_type,
+    const CloudPolicyClient::StatusCallback& callback) {
+  CHECK(is_registered());
+  std::unique_ptr<DeviceManagementRequestJob> request_job(
+      service_->CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_CERTIFICATE,
+                          GetRequestContext()));
+  request_job->SetDMToken(dm_token_);
+  request_job->SetClientID(client_id_);
+
+  em::DeviceManagementRequest* request = request_job->GetRequest();
+  em::DeviceCertUploadRequest* upload_request =
+      request->mutable_cert_upload_request();
+  upload_request->set_device_certificate(certificate_data);
+  upload_request->set_certificate_type(certificate_type);
+
+  const DeviceManagementRequestJob::Callback job_callback = base::BindRepeating(
+      &CloudPolicyClient::OnCertificateUploadCompleted,
+      weak_ptr_factory_.GetWeakPtr(), request_job.get(), callback);
+
+  request_jobs_.push_back(std::move(request_job));
+  request_jobs_.back()->Start(job_callback);
+}
+
 void CloudPolicyClient::OnRetryRegister(DeviceManagementRequestJob* job) {
   DCHECK_EQ(policy_fetch_request_job_.get(), job);
   // If the initial request managed to get to the server but the response didn't

components/policy/core/common/cloud/cloud_policy_client.h

@@ -153,12 +153,21 @@ class POLICY_EXPORT CloudPolicyClient {
   // Sends an unregistration request to the server.
   virtual void Unregister();
 
-  // Upload a device certificate to the server.  Like FetchPolicy, this method
+  // Upload a machine certificate to the server.  Like FetchPolicy, this method
   // requires that the client is in a registered state.  |certificate_data| must
   // hold the X.509 certificate data to be sent to the server.  The |callback|
   // will be called when the operation completes.
-  virtual void UploadCertificate(const std::string& certificate_data,
-                                 const StatusCallback& callback);
+  virtual void UploadEnterpriseMachineCertificate(
+      const std::string& certificate_data,
+      const StatusCallback& callback);
+
+  // Upload an enrollment certificate to the server.  Like FetchPolicy, this
+  // method requires that the client is in a registered state.
+  // |certificate_data| must hold the X.509 certificate data to be sent to the
+  // server.  The |callback| will be called when the operation completes.
+  virtual void UploadEnterpriseEnrollmentCertificate(
+      const std::string& certificate_data,
+      const StatusCallback& callback);
 
   // Uploads device/session status to the server. As above, the client must be
   // in a registered state. If non-null, |device_status| and |session_status|
@@ -297,6 +306,16 @@ class POLICY_EXPORT CloudPolicyClient {
   // A set of (policy type, settings entity ID) pairs to fetch.
   typedef std::set<std::pair<std::string, std::string>> PolicyTypeSet;
 
+  // Upload a certificate to the server.  Like FetchPolicy, this method
+  // requires that the client is in a registered state.  |certificate_data| must
+  // hold the X.509 certificate data to be sent to the server.  The |callback|
+  // will be called when the operation completes.
+  void UploadCertificate(
+      const std::string& certificate_data,
+      enterprise_management::DeviceCertUploadRequest::CertificateType
+          certificate_type,
+      const StatusCallback& callback);
+
   // Callback for retries of registration requests.
   void OnRetryRegister(DeviceManagementRequestJob* job);
 

components/policy/core/common/cloud/cloud_policy_client_unittest.cc

@@ -45,7 +45,8 @@ const char kMachineID[] = "fake-machine-id";
 const char kMachineModel[] = "fake-machine-model";
 const char kOAuthToken[] = "fake-oauth-token";
 const char kDMToken[] = "fake-dm-token";
-const char kDeviceCertificate[] = "fake-device-certificate";
+const char kMachineCertificate[] = "fake-machine-certificate";
+const char kEnrollmentCertificate[] = "fake-enrollment-certificate";
 const char kRequisition[] = "fake-requisition";
 const char kStateKey[] = "fake-state-key";
 const char kPayload[] = "input_payload";
@@ -53,7 +54,6 @@ const char kResultPayload[] = "output_payload";
 const char kAssetId[] = "fake-asset-id";
 const char kLocation[] = "fake-location";
 const char kGcmID[] = "fake-gcm-id";
-const char kEnrollmentCertificate[] = "fake-certificate";
 
 const int64_t kAgeOfCommand = 123123123;
 const int64_t kLastCommandId = 123456789;
@@ -138,8 +138,16 @@ class CloudPolicyClientTest : public testing::Test {
 
     unregistration_request_.mutable_unregister_request();
     unregistration_response_.mutable_unregister_response();
-    upload_certificate_request_.mutable_cert_upload_request()->
-        set_device_certificate(kDeviceCertificate);
+    upload_machine_certificate_request_.mutable_cert_upload_request()
+        ->set_device_certificate(kMachineCertificate);
+    upload_machine_certificate_request_.mutable_cert_upload_request()
+        ->set_certificate_type(
+            em::DeviceCertUploadRequest::ENTERPRISE_MACHINE_CERTIFICATE);
+    upload_enrollment_certificate_request_.mutable_cert_upload_request()
+        ->set_device_certificate(kEnrollmentCertificate);
+    upload_enrollment_certificate_request_.mutable_cert_upload_request()
+        ->set_certificate_type(
+            em::DeviceCertUploadRequest::ENTERPRISE_ENROLLMENT_CERTIFICATE);
     upload_certificate_response_.mutable_cert_upload_response();
 
     upload_status_request_.mutable_device_status_report_request();
@@ -271,15 +279,14 @@ class CloudPolicyClientTest : public testing::Test {
                          MatchProto(unregistration_request_)));
   }
 
-  void ExpectUploadCertificate() {
+  void ExpectUploadCertificate(const em::DeviceManagementRequest& request) {
     EXPECT_CALL(service_,
                 CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_CERTIFICATE,
                           request_context_))
         .WillOnce(service_.SucceedJob(upload_certificate_response_));
-    EXPECT_CALL(service_,
-                StartJob(dm_protocol::kValueRequestUploadCertificate,
-                         std::string(), std::string(), kDMToken, client_id_,
-                         MatchProto(upload_certificate_request_)));
+    EXPECT_CALL(service_, StartJob(dm_protocol::kValueRequestUploadCertificate,
+                                   std::string(), std::string(), kDMToken,
+                                   client_id_, MatchProto(request)));
   }
 
   void ExpectUploadStatus() {
@@ -371,7 +378,8 @@ class CloudPolicyClientTest : public testing::Test {
   em::DeviceManagementRequest cert_based_registration_request_;
   em::DeviceManagementRequest policy_request_;
   em::DeviceManagementRequest unregistration_request_;
-  em::DeviceManagementRequest upload_certificate_request_;
+  em::DeviceManagementRequest upload_machine_certificate_request_;
+  em::DeviceManagementRequest upload_enrollment_certificate_request_;
   em::DeviceManagementRequest upload_status_request_;
   em::DeviceManagementRequest remote_command_request_;
   em::DeviceManagementRequest attribute_update_permission_request_;
@@ -774,28 +782,55 @@ TEST_F(CloudPolicyClientTest, PolicyFetchWithExtensionPolicy) {
   }
 }
 
-TEST_F(CloudPolicyClientTest, UploadCertificate) {
+TEST_F(CloudPolicyClientTest, UploadEnterpriseMachineCertificate) {
   Register();
 
-  ExpectUploadCertificate();
+  ExpectUploadCertificate(upload_machine_certificate_request_);
   EXPECT_CALL(callback_observer_, OnCallbackComplete(true)).Times(1);
-  CloudPolicyClient::StatusCallback callback = base::Bind(
-      &MockStatusCallbackObserver::OnCallbackComplete,
-      base::Unretained(&callback_observer_));
-  client_->UploadCertificate(kDeviceCertificate, callback);
+  CloudPolicyClient::StatusCallback callback =
+      base::BindRepeating(&MockStatusCallbackObserver::OnCallbackComplete,
+                          base::Unretained(&callback_observer_));
+  client_->UploadEnterpriseMachineCertificate(kMachineCertificate, callback);
+  EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
+}
+
+TEST_F(CloudPolicyClientTest, UploadEnterpriseEnrollmentCertificate) {
+  Register();
+
+  ExpectUploadCertificate(upload_enrollment_certificate_request_);
+  EXPECT_CALL(callback_observer_, OnCallbackComplete(true)).Times(1);
+  CloudPolicyClient::StatusCallback callback =
+      base::BindRepeating(&MockStatusCallbackObserver::OnCallbackComplete,
+                          base::Unretained(&callback_observer_));
+  client_->UploadEnterpriseEnrollmentCertificate(kEnrollmentCertificate,
+                                                 callback);
   EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
 }
 
-TEST_F(CloudPolicyClientTest, UploadCertificateEmpty) {
+TEST_F(CloudPolicyClientTest, UploadEnterpriseMachineCertificateEmpty) {
   Register();
 
   upload_certificate_response_.clear_cert_upload_response();
-  ExpectUploadCertificate();
+  ExpectUploadCertificate(upload_machine_certificate_request_);
   EXPECT_CALL(callback_observer_, OnCallbackComplete(false)).Times(1);
-  CloudPolicyClient::StatusCallback callback = base::Bind(
-      &MockStatusCallbackObserver::OnCallbackComplete,
-      base::Unretained(&callback_observer_));
-  client_->UploadCertificate(kDeviceCertificate, callback);
+  CloudPolicyClient::StatusCallback callback =
+      base::BindRepeating(&MockStatusCallbackObserver::OnCallbackComplete,
+                          base::Unretained(&callback_observer_));
+  client_->UploadEnterpriseMachineCertificate(kMachineCertificate, callback);
+  EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
+}
+
+TEST_F(CloudPolicyClientTest, UploadEnterpriseEnrollmentCertificateEmpty) {
+  Register();
+
+  upload_certificate_response_.clear_cert_upload_response();
+  ExpectUploadCertificate(upload_enrollment_certificate_request_);
+  EXPECT_CALL(callback_observer_, OnCallbackComplete(false)).Times(1);
+  CloudPolicyClient::StatusCallback callback =
+      base::BindRepeating(&MockStatusCallbackObserver::OnCallbackComplete,
+                          base::Unretained(&callback_observer_));
+  client_->UploadEnterpriseEnrollmentCertificate(kEnrollmentCertificate,
+                                                 callback);
   EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
 }
 
@@ -812,7 +847,7 @@ TEST_F(CloudPolicyClientTest, UploadCertificateFailure) {
   CloudPolicyClient::StatusCallback callback = base::Bind(
       &MockStatusCallbackObserver::OnCallbackComplete,
       base::Unretained(&callback_observer_));
-  client_->UploadCertificate(kDeviceCertificate, callback);
+  client_->UploadEnterpriseMachineCertificate(kMachineCertificate, callback);
   EXPECT_EQ(DM_STATUS_REQUEST_FAILED, client_->status());
 }
 
@@ -886,7 +921,7 @@ TEST_F(CloudPolicyClientTest, MultipleActiveRequests) {
   CloudPolicyClient::StatusCallback callback2 = base::Bind(
       &MockStatusCallbackObserver::OnCallbackComplete,
       base::Unretained(&callback_observer_));
-  client_->UploadCertificate(kDeviceCertificate, callback2);
+  client_->UploadEnterpriseMachineCertificate(kMachineCertificate, callback2);
   EXPECT_EQ(2, client_->GetActiveRequestCountForTest());
 
   // Now satisfy both active jobs.

components/policy/core/common/cloud/mock_cloud_policy_client.h

@@ -32,7 +32,9 @@ class MockCloudPolicyClient : public CloudPolicyClient {
            const std::string&));
   MOCK_METHOD0(FetchPolicy, void(void));
   MOCK_METHOD0(Unregister, void(void));
-  MOCK_METHOD2(UploadCertificate,
+  MOCK_METHOD2(UploadEnterpriseMachineCertificate,
+               void(const std::string&, const StatusCallback&));
+  MOCK_METHOD2(UploadEnterpriseEnrollmentCertificate,
                void(const std::string&, const StatusCallback&));
   MOCK_METHOD3(UploadDeviceStatus,
                void(const enterprise_management::DeviceStatusReportRequest*,