[remoting] Remove signature and sequence ID from HeartbeatSender

This CL removes signature and sequence ID from the heartbeat request
since they are no longer required by the server. It also rewrites
HeartbeatSenderTest to mock the HeartbeatClient directly instead of
creating in process channel and mocking the server, so that the tests
can be more deterministic.

Bug: 971363
Change-Id: I3b9be0819172d06da657536c56744532df39a75e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1653602
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669093}

remoting/host/heartbeat_sender.cc

@@ -75,39 +75,35 @@ const net::BackoffEntry::Policy kBackoffPolicy = {
     false,
 };
 
-using HeartbeatResponseCallback =
-    base::OnceCallback<void(const grpc::Status&,
-                            const apis::v1::HeartbeatResponse&)>;
-
 }  // namespace
 
-class HeartbeatSender::HeartbeatClient final {
+class HeartbeatSender::HeartbeatClientImpl final
+    : public HeartbeatSender::HeartbeatClient {
  public:
-  HeartbeatClient(OAuthTokenGetter* oauth_token_getter);
-  ~HeartbeatClient();
+  explicit HeartbeatClientImpl(OAuthTokenGetter* oauth_token_getter);
+  ~HeartbeatClientImpl() override;
 
   void Heartbeat(const apis::v1::HeartbeatRequest& request,
-                 HeartbeatResponseCallback callback);
-  void CancelPendingRequests();
-  void SetChannelForTest(GrpcChannelSharedPtr channel);
+                 HeartbeatResponseCallback callback) override;
+  void CancelPendingRequests() override;
 
  private:
   using DirectoryService = apis::v1::RemotingDirectoryService;
   std::unique_ptr<DirectoryService::Stub> directory_;
   GrpcAuthenticatedExecutor executor_;
-  DISALLOW_COPY_AND_ASSIGN(HeartbeatClient);
+  DISALLOW_COPY_AND_ASSIGN(HeartbeatClientImpl);
 };
 
-HeartbeatSender::HeartbeatClient::HeartbeatClient(
+HeartbeatSender::HeartbeatClientImpl::HeartbeatClientImpl(
     OAuthTokenGetter* oauth_token_getter)
     : executor_(oauth_token_getter) {
   directory_ = DirectoryService::NewStub(CreateSslChannelForEndpoint(
       ServiceUrls::GetInstance()->remoting_server_endpoint()));
 }
 
-HeartbeatSender::HeartbeatClient::~HeartbeatClient() = default;
+HeartbeatSender::HeartbeatClientImpl::~HeartbeatClientImpl() = default;
 
-void HeartbeatSender::HeartbeatClient::Heartbeat(
+void HeartbeatSender::HeartbeatClientImpl::Heartbeat(
     const apis::v1::HeartbeatRequest& request,
     HeartbeatResponseCallback callback) {
   std::string host_offline_reason_or_empty_log =
@@ -129,16 +125,11 @@ void HeartbeatSender::HeartbeatClient::Heartbeat(
   executor_.ExecuteRpc(std::move(async_request));
 }
 
-void HeartbeatSender::HeartbeatClient::CancelPendingRequests() {
+void HeartbeatSender::HeartbeatClientImpl::CancelPendingRequests() {
   executor_.CancelPendingRequests();
 }
 
-void HeartbeatSender::HeartbeatClient::SetChannelForTest(
-    GrpcChannelSharedPtr channel) {
-  directory_ = DirectoryService::NewStub(channel);
-}
-
-// end of HeartbeatSender::HeartbeatClient
+// end of HeartbeatSender::HeartbeatClientImpl
 
 HeartbeatSender::HeartbeatSender(
     base::OnceClosure on_heartbeat_successful_callback,
@@ -146,7 +137,6 @@ HeartbeatSender::HeartbeatSender(
     base::OnceClosure on_auth_error,
     const std::string& host_id,
     MuxingSignalStrategy* signal_strategy,
-    const scoped_refptr<const RsaKeyPair>& host_key_pair,
     OAuthTokenGetter* oauth_token_getter,
     LogToServer* log_to_server)
     : on_heartbeat_successful_callback_(
@@ -155,12 +145,10 @@ HeartbeatSender::HeartbeatSender(
       on_auth_error_(std::move(on_auth_error)),
       host_id_(host_id),
       signal_strategy_(signal_strategy),
-      host_key_pair_(host_key_pair),
-      client_(std::make_unique<HeartbeatClient>(oauth_token_getter)),
+      client_(std::make_unique<HeartbeatClientImpl>(oauth_token_getter)),
       log_to_server_(log_to_server),
       oauth_token_getter_(oauth_token_getter),
       backoff_(&kBackoffPolicy) {
-  DCHECK(host_key_pair_.get());
   DCHECK(log_to_server_);
 
   signal_strategy_->AddListener(this);
@@ -188,10 +176,6 @@ void HeartbeatSender::SetHostOfflineReason(
   }
 }
 
-void HeartbeatSender::SetGrpcChannelForTest(GrpcChannelSharedPtr channel) {
-  client_->SetChannelForTest(channel);
-}
-
 void HeartbeatSender::OnSignalStrategyStateChange(SignalStrategy::State state) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   switch (state) {
@@ -250,7 +234,6 @@ void HeartbeatSender::SendHeartbeat() {
   client_->Heartbeat(
       CreateHeartbeatRequest(),
       base::BindOnce(&HeartbeatSender::OnResponse, base::Unretained(this)));
-  ++sequence_id_;
 
   // Send a heartbeat log
   std::unique_ptr<ServerLogEntry> log_entry = MakeLogEntryForHeartbeat();
@@ -362,11 +345,9 @@ apis::v1::HeartbeatRequest HeartbeatSender::CreateHeartbeatRequest() {
     }
   }
   heartbeat.set_host_id(host_id_);
-  heartbeat.set_sequence_id(sequence_id_);
   if (!host_offline_reason_.empty()) {
     heartbeat.set_host_offline_reason(host_offline_reason_);
   }
-  heartbeat.set_signature(CreateSignature(signaling_id));
   // Append host version.
   heartbeat.set_host_version(STRINGIZE(VERSION));
   // If we have not recorded a heartbeat success, continue sending host OS info.
@@ -379,10 +360,4 @@ apis::v1::HeartbeatRequest HeartbeatSender::CreateHeartbeatRequest() {
   return heartbeat;
 }
 
-std::string HeartbeatSender::CreateSignature(const std::string& signaling_id) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  std::string message = signaling_id + ' ' + base::NumberToString(sequence_id_);
-  return host_key_pair_->SignMessage(message);
-}
-
 }  // namespace remoting

remoting/host/heartbeat_sender.h

@@ -11,12 +11,9 @@
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
 #include "base/sequence_checker.h"
 #include "base/timer/timer.h"
 #include "net/base/backoff_entry.h"
-#include "remoting/base/grpc_support/grpc_channel.h"
-#include "remoting/base/rsa_key_pair.h"
 #include "remoting/proto/remoting/v1/directory_messages.pb.h"
 #include "remoting/signaling/muxing_signal_strategy.h"
 
@@ -32,7 +29,6 @@ namespace remoting {
 
 class LogToServer;
 class OAuthTokenGetter;
-class RsaKeyPair;
 
 // HeartbeatSender periodically sends heartbeat to the directory service. See
 // the HeartbeatRequest message in directory_messages.proto for more details.
@@ -45,18 +41,6 @@ class RsaKeyPair;
 // host_exit_codes.cc (i.e. "INVALID_HOST_CONFIGURATION" string) or one of
 // kHostOfflineReasonXxx constants (i.e. "POLICY_READ_ERROR" string).
 //
-// The sequence_id field of the heartbeat is a zero-based incrementally
-// increasing integer unique to each heartbeat from a single host.
-// The server checks the value, and if it is incorrect, includes the
-// correct value in the result stanza. The host should then send another
-// heartbeat, with the correct sequence-id, and increment the sequence-id in
-// susbequent heartbeats.
-// The signature is a base-64 encoded SHA-1 hash, signed with the host's
-// private RSA key. The message being signed is the full Jid concatenated with
-// the sequence-id, separated by one space. For example, for the heartbeat
-// stanza above, the message that is signed is
-// "user@gmail.com/chromoting_ftl_abc123 456".
-//
 // The sends a HeartbeatResponse in response to each successful heartbeat.
 class HeartbeatSender final : public SignalStrategy::Listener {
  public:
@@ -78,7 +62,6 @@ class HeartbeatSender final : public SignalStrategy::Listener {
                   base::OnceClosure on_auth_error,
                   const std::string& host_id,
                   MuxingSignalStrategy* signal_strategy,
-                  const scoped_refptr<const RsaKeyPair>& host_key_pair,
                   OAuthTokenGetter* oauth_token_getter,
                   LogToServer* log_to_server);
   ~HeartbeatSender() override;
@@ -97,11 +80,22 @@ class HeartbeatSender final : public SignalStrategy::Listener {
       base::OnceCallback<void(bool success)> ack_callback);
 
  private:
-  class HeartbeatClient;
+  class HeartbeatClient {
+   public:
+    using HeartbeatResponseCallback =
+        base::OnceCallback<void(const grpc::Status&,
+                                const apis::v1::HeartbeatResponse&)>;
 
-  friend class HeartbeatSenderTest;
+    virtual ~HeartbeatClient() = default;
+
+    virtual void Heartbeat(const apis::v1::HeartbeatRequest& request,
+                           HeartbeatResponseCallback callback) = 0;
+    virtual void CancelPendingRequests() = 0;
+  };
 
-  void SetGrpcChannelForTest(GrpcChannelSharedPtr channel);
+  class HeartbeatClientImpl;
+
+  friend class HeartbeatSenderTest;
 
   // SignalStrategy::Listener interface.
   void OnSignalStrategyStateChange(SignalStrategy::State state) override;
@@ -120,14 +114,12 @@ class HeartbeatSender final : public SignalStrategy::Listener {
 
   // Helper methods used by DoSendStanza() to generate heartbeat stanzas.
   apis::v1::HeartbeatRequest CreateHeartbeatRequest();
-  std::string CreateSignature(const std::string& signaling_id);
 
   base::OnceClosure on_heartbeat_successful_callback_;
   base::OnceClosure on_unknown_host_id_error_;
   base::OnceClosure on_auth_error_;
   std::string host_id_;
   MuxingSignalStrategy* const signal_strategy_;
-  scoped_refptr<const RsaKeyPair> host_key_pair_;
   std::unique_ptr<HeartbeatClient> client_;
   LogToServer* const log_to_server_;
   OAuthTokenGetter* const oauth_token_getter_;
@@ -136,7 +128,6 @@ class HeartbeatSender final : public SignalStrategy::Listener {
 
   net::BackoffEntry backoff_;
 
-  int sequence_id_ = 0;
   bool heartbeat_succeeded_ = false;
 
   // Fields to send and indicate completion of sending host-offline-reason.

remoting/host/remoting_me2me_host.cc

@@ -1511,8 +1511,8 @@ void HostProcess::InitializeSignaling() {
         base::BindOnce(&HostProcess::OnUnknownHostIdError,
                        base::Unretained(this)),
         base::BindOnce(&HostProcess::OnAuthFailed, base::Unretained(this)),
-        host_id_, muxing_signal_strategy.get(), key_pair_,
-        oauth_token_getter_.get(), log_to_server_.get());
+        host_id_, muxing_signal_strategy.get(), oauth_token_getter_.get(),
+        log_to_server_.get());
     ftl_host_change_notification_listener_ =
         std::make_unique<FtlHostChangeNotificationListener>(
             this, muxing_signal_strategy->ftl_signal_strategy());

remoting/proto/remoting/v1/directory_messages.proto

@@ -45,22 +45,27 @@ message GetHostListResponse {
 
 // Sent from a Me2Me host to update it's info and online presence.
 message HeartbeatRequest {
-  // An encypted token used to validate the request's origin.
-  optional string signature = 1;
+  // Obsolete fields reserved to prevent reuse.
+  reserved 1, 5;
+
   // Host identity. Normally a UUID.
   optional string host_id = 2;
+
   // Current ID for the GoogleTalk service, if applicable.
   optional string jabber_id = 3;
+
   // Current ID for the Tachyon service, if applicable.
   optional string tachyon_id = 4;
-  // Monotonically increasing value which is used to prevent replay attacks.
-  optional int32 sequence_id = 5;
+
   // Version of the Me2Me host software installed.
   optional string host_version = 6;
+
   // A reason for the host reporting itself as offline.
   optional string host_offline_reason = 7;
+
   // Operating system type the host is running on.
   optional string host_os_name = 8;
+
   // Operating system version the host is running on.
   optional string host_os_version = 9;
 }