Change Authenticator interface to properly handle protocol errors.

Previously all error of the auth protocol were interpreted as invalid 
credentials even when the error was due to some other reason, e.g. 
incompatible protocol.

BUG=105214


Review URL: http://codereview.chromium.org/9270052

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@118894 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 88 insertions, 14 deletions

diff --git a/remoting/protocol/authenticator.h b/remoting/protocol/authenticator.h
index 1a0931a..4ed7fa5 100644
--- a/remoting/protocol/authenticator.h
+++ b/remoting/protocol/authenticator.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -55,6 +55,11 @@ class Authenticator {
     REJECTED,
   };
 
+  enum RejectionReason {
+    INVALID_CREDENTIALS,
+    PROTOCOL_ERROR,
+  };
+
   // Returns true if |message| is an Authenticator message.
   static bool IsAuthenticatorMessage(const buzz::XmlElement* message);
 
@@ -72,6 +77,9 @@ class Authenticator {
   // Returns current state of the authenticator.
   virtual State state() const = 0;
 
+  // Returns rejection reason. Can be called only when in REJECTED state.
+  virtual RejectionReason rejection_reason() const = 0;
+
   // Called in response to incoming message received from the peer.
   // Should only be called when in WAITING_MESSAGE state. Caller
   // retains ownership of |message|.
diff --git a/remoting/protocol/fake_authenticator.cc b/remoting/protocol/fake_authenticator.cc
index 4be9b74..9e63cf7 100644
--- a/remoting/protocol/fake_authenticator.cc
+++ b/remoting/protocol/fake_authenticator.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -89,6 +89,11 @@ Authenticator::State FakeAuthenticator::state() const{
   }
 }
 
+Authenticator::RejectionReason FakeAuthenticator::rejection_reason() const {
+  EXPECT_EQ(REJECTED, state());
+  return INVALID_CREDENTIALS;
+}
+
 void FakeAuthenticator::ProcessMessage(const buzz::XmlElement* message) {
   EXPECT_EQ(WAITING_MESSAGE, state());
   std::string id =
diff --git a/remoting/protocol/fake_authenticator.h b/remoting/protocol/fake_authenticator.h
index a1a6db5..d9750a5 100644
--- a/remoting/protocol/fake_authenticator.h
+++ b/remoting/protocol/fake_authenticator.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -53,6 +53,7 @@ class FakeAuthenticator : public Authenticator {
 
   // Authenticator interface.
   virtual State state() const OVERRIDE;
+  virtual RejectionReason rejection_reason() const OVERRIDE;
   virtual void ProcessMessage(const buzz::XmlElement* message) OVERRIDE;
   virtual scoped_ptr<buzz::XmlElement> GetNextMessage() OVERRIDE;
   virtual scoped_ptr<ChannelAuthenticator>
diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index 1b3c1a9..b05805d 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -422,7 +422,8 @@ void JingleSession::AcceptConnection() {
   DCHECK(authenticator_->state() == Authenticator::WAITING_MESSAGE);
   authenticator_->ProcessMessage(auth_message);
   if (authenticator_->state() == Authenticator::REJECTED) {
-    CloseInternal(net::ERR_CONNECTION_FAILED, AUTHENTICATION_FAILED);
+    CloseInternal(net::ERR_CONNECTION_FAILED,
+                  RejectionReasonToError(authenticator_->rejection_reason()));
     return;
   }
 
@@ -454,7 +455,8 @@ void JingleSession::ProcessAuthenticationStep() {
   if (authenticator_->state() == Authenticator::ACCEPTED) {
     SetState(AUTHENTICATED);
   } else if (authenticator_->state() == Authenticator::REJECTED) {
-    CloseInternal(net::ERR_CONNECTION_ABORTED, AUTHENTICATION_FAILED);
+    CloseInternal(net::ERR_CONNECTION_FAILED,
+                  RejectionReasonToError(authenticator_->rejection_reason()));
   }
 }
 
@@ -520,6 +522,19 @@ void JingleSession::SetState(State new_state) {
 }
 
 // static
+Session::Error JingleSession::RejectionReasonToError(
+    Authenticator::RejectionReason reason) {
+  switch (reason) {
+    case Authenticator::INVALID_CREDENTIALS:
+      return AUTHENTICATION_FAILED;
+    case Authenticator::PROTOCOL_ERROR:
+      return INCOMPATIBLE_PROTOCOL;
+  }
+  NOTREACHED();
+  return UNKNOWN_ERROR;
+}
+
+// static
 scoped_ptr<cricket::SessionDescription> JingleSession::CreateSessionDescription(
     scoped_ptr<CandidateSessionConfig> config,
     scoped_ptr<buzz::XmlElement> authenticator_message) {
diff --git a/remoting/protocol/jingle_session.h b/remoting/protocol/jingle_session.h
index 3230f57..caa46d2 100644
--- a/remoting/protocol/jingle_session.h
+++ b/remoting/protocol/jingle_session.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -8,6 +8,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "net/base/completion_callback.h"
+#include "remoting/protocol/authenticator.h"
 #include "remoting/protocol/session.h"
 #include "third_party/libjingle/source/talk/base/sigslot.h"
 #include "third_party/libjingle/source/talk/p2p/base/session.h"
@@ -15,7 +16,6 @@
 namespace remoting {
 namespace protocol {
 
-class Authenticator;
 class JingleChannelConnector;
 class JingleSessionManager;
 
@@ -108,6 +108,8 @@ class JingleSession : public protocol::Session,
 
   void SetState(State new_state);
 
+  static Error RejectionReasonToError(Authenticator::RejectionReason reason);
+
   static scoped_ptr<cricket::SessionDescription> CreateSessionDescription(
       scoped_ptr<CandidateSessionConfig> candidate_config,
       scoped_ptr<buzz::XmlElement> authenticator_message);
diff --git a/remoting/protocol/pepper_session.cc b/remoting/protocol/pepper_session.cc
index 5751aba..ae2dd66 100644
--- a/remoting/protocol/pepper_session.cc
+++ b/remoting/protocol/pepper_session.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -347,7 +347,14 @@ void PepperSession::ProcessAuthenticationStep() {
   if (authenticator_->state() == Authenticator::ACCEPTED) {
     SetState(AUTHENTICATED);
   } else if (authenticator_->state() == Authenticator::REJECTED) {
-    OnError(AUTHENTICATION_FAILED);
+    switch (authenticator_->rejection_reason()) {
+      case Authenticator::INVALID_CREDENTIALS:
+        OnError(AUTHENTICATION_FAILED);
+        break;
+      case Authenticator::PROTOCOL_ERROR:
+        OnError(INCOMPATIBLE_PROTOCOL);
+        break;
+    }
   }
 }
 
@@ -357,7 +364,7 @@ void PepperSession::OnSessionInfoResponse(const buzz::XmlElement* response) {
     LOG(ERROR) << "Received error in response to session-info message: \""
                << response->Str()
                << "\". Terminating the session.";
-    OnError(AUTHENTICATION_FAILED);
+    OnError(INCOMPATIBLE_PROTOCOL);
   }
 }
 
diff --git a/remoting/protocol/v1_authenticator.cc b/remoting/protocol/v1_authenticator.cc
index 5811eff..aa000e8 100644
--- a/remoting/protocol/v1_authenticator.cc
+++ b/remoting/protocol/v1_authenticator.cc
@@ -28,7 +28,8 @@ V1ClientAuthenticator::V1ClientAuthenticator(
     const std::string& shared_secret)
     : local_jid_(local_jid),
       shared_secret_(shared_secret),
-      state_(MESSAGE_READY) {
+      state_(MESSAGE_READY),
+      rejection_reason_(INVALID_CREDENTIALS) {
 }
 
 V1ClientAuthenticator::~V1ClientAuthenticator() {
@@ -38,6 +39,11 @@ Authenticator::State V1ClientAuthenticator::state() const {
   return state_;
 }
 
+Authenticator::RejectionReason V1ClientAuthenticator::rejection_reason() const {
+  DCHECK_EQ(state_, REJECTED);
+  return rejection_reason_;
+}
+
 void V1ClientAuthenticator::ProcessMessage(const XmlElement* message) {
   DCHECK_EQ(state_, WAITING_MESSAGE);
 
@@ -54,6 +60,7 @@ void V1ClientAuthenticator::ProcessMessage(const XmlElement* message) {
 
   if (remote_cert_.empty()) {
     state_ = REJECTED;
+    rejection_reason_ = PROTOCOL_ERROR;
   } else {
     state_ = ACCEPTED;
   }
@@ -93,7 +100,8 @@ V1HostAuthenticator::V1HostAuthenticator(
       local_private_key_(local_private_key.Copy()),
       shared_secret_(shared_secret),
       remote_jid_(remote_jid),
-      state_(WAITING_MESSAGE) {
+      state_(WAITING_MESSAGE),
+      rejection_reason_(INVALID_CREDENTIALS) {
 }
 
 V1HostAuthenticator::~V1HostAuthenticator() {
@@ -103,15 +111,27 @@ Authenticator::State V1HostAuthenticator::state() const {
   return state_;
 }
 
+Authenticator::RejectionReason V1HostAuthenticator::rejection_reason() const {
+  DCHECK_EQ(state_, REJECTED);
+  return rejection_reason_;
+}
+
 void V1HostAuthenticator::ProcessMessage(const XmlElement* message) {
   DCHECK_EQ(state_, WAITING_MESSAGE);
 
   std::string auth_token =
       message->TextNamed(buzz::QName(kChromotingXmlNamespace, kAuthTokenTag));
 
+  if (auth_token.empty()) {
+    state_ = REJECTED;
+    rejection_reason_ = PROTOCOL_ERROR;
+    return;
+  }
+
   if (!protocol::VerifySupportAuthToken(
           remote_jid_, shared_secret_, auth_token)) {
     state_ = REJECTED;
+    rejection_reason_ = INVALID_CREDENTIALS;
   } else {
     state_ = MESSAGE_READY;
   }
diff --git a/remoting/protocol/v1_authenticator.h b/remoting/protocol/v1_authenticator.h
index db06cf5..aceafe6 100644
--- a/remoting/protocol/v1_authenticator.h
+++ b/remoting/protocol/v1_authenticator.h
@@ -25,6 +25,7 @@ class V1ClientAuthenticator : public Authenticator {
 
   // Authenticator interface.
   virtual State state() const OVERRIDE;
+  virtual RejectionReason rejection_reason() const OVERRIDE;
   virtual void ProcessMessage(const buzz::XmlElement* message) OVERRIDE;
   virtual scoped_ptr<buzz::XmlElement> GetNextMessage() OVERRIDE;
   virtual scoped_ptr<ChannelAuthenticator>
@@ -35,6 +36,7 @@ class V1ClientAuthenticator : public Authenticator {
   std::string shared_secret_;
   std::string remote_cert_;
   State state_;
+  RejectionReason rejection_reason_;
 
   DISALLOW_COPY_AND_ASSIGN(V1ClientAuthenticator);
 };
@@ -50,6 +52,7 @@ class V1HostAuthenticator : public Authenticator {
 
   // Authenticator interface.
   virtual State state() const OVERRIDE;
+  virtual RejectionReason rejection_reason() const OVERRIDE;
   virtual void ProcessMessage(const buzz::XmlElement* message) OVERRIDE;
   virtual scoped_ptr<buzz::XmlElement> GetNextMessage() OVERRIDE;
   virtual scoped_ptr<ChannelAuthenticator>
@@ -61,6 +64,7 @@ class V1HostAuthenticator : public Authenticator {
   std::string shared_secret_;
   std::string remote_jid_;
   State state_;
+  RejectionReason rejection_reason_;
 
   DISALLOW_COPY_AND_ASSIGN(V1HostAuthenticator);
 };
diff --git a/remoting/protocol/v2_authenticator.cc b/remoting/protocol/v2_authenticator.cc
index 14bced4..9e1184c 100644
--- a/remoting/protocol/v2_authenticator.cc
+++ b/remoting/protocol/v2_authenticator.cc
@@ -59,7 +59,8 @@ V2Authenticator::V2Authenticator(
     const std::string& shared_secret)
     : certificate_sent_(false),
       key_exchange_impl_(type, shared_secret),
-      state_(MESSAGE_READY) {
+      state_(MESSAGE_READY),
+      rejection_reason_(INVALID_CREDENTIALS) {
   pending_messages_.push(key_exchange_impl_.GetMessage());
 }
 
@@ -72,6 +73,11 @@ Authenticator::State V2Authenticator::state() const {
   return state_;
 }
 
+Authenticator::RejectionReason V2Authenticator::rejection_reason() const {
+  DCHECK_EQ(state(), REJECTED);
+  return rejection_reason_;
+}
+
 void V2Authenticator::ProcessMessage(const buzz::XmlElement* message) {
   DCHECK_EQ(state(), WAITING_MESSAGE);
 
@@ -88,6 +94,7 @@ void V2Authenticator::ProcessMessage(const buzz::XmlElement* message) {
   if (!is_host_side() && remote_cert_.empty()) {
     LOG(WARNING) << "No valid host certificate.";
     state_ = REJECTED;
+    rejection_reason_ = PROTOCOL_ERROR;
     return;
   }
 
@@ -95,6 +102,7 @@ void V2Authenticator::ProcessMessage(const buzz::XmlElement* message) {
   if (!eke_element) {
     LOG(WARNING) << "No eke-message found.";
     state_ = REJECTED;
+    rejection_reason_ = PROTOCOL_ERROR;
     return;
   }
 
@@ -105,6 +113,7 @@ void V2Authenticator::ProcessMessage(const buzz::XmlElement* message) {
         !base::Base64Decode(base64_message, &spake_message)) {
       LOG(WARNING) << "Failed to decode auth message received from the peer.";
       state_ = REJECTED;
+      rejection_reason_ = PROTOCOL_ERROR;
       return;
     }
 
@@ -117,6 +126,7 @@ void V2Authenticator::ProcessMessage(const buzz::XmlElement* message) {
 
       case P224EncryptedKeyExchange::kResultFailed:
         state_ = REJECTED;
+        rejection_reason_ = INVALID_CREDENTIALS;
         return;
 
       case P224EncryptedKeyExchange::kResultSuccess:
diff --git a/remoting/protocol/v2_authenticator.h b/remoting/protocol/v2_authenticator.h
index 01e8a67..cd492b1 100644
--- a/remoting/protocol/v2_authenticator.h
+++ b/remoting/protocol/v2_authenticator.h
@@ -37,6 +37,7 @@ class V2Authenticator : public Authenticator {
 
   // Authenticator interface.
   virtual State state() const OVERRIDE;
+  virtual RejectionReason rejection_reason() const OVERRIDE;
   virtual void ProcessMessage(const buzz::XmlElement* message) OVERRIDE;
   virtual scoped_ptr<buzz::XmlElement> GetNextMessage() OVERRIDE;
   virtual scoped_ptr<ChannelAuthenticator>
@@ -61,6 +62,7 @@ class V2Authenticator : public Authenticator {
   // Used for both HOST and CLIENT authenticators.
   crypto::P224EncryptedKeyExchange key_exchange_impl_;
   State state_;
+  RejectionReason rejection_reason_;
   std::queue<std::string> pending_messages_;
   std::string auth_key_;