Third Party authentication protocol.

remoting/protocol/third_party_authenticator.cc

Index: remoting/protocol/third_party_authenticator.cc
diff --git a/remoting/protocol/third_party_authenticator.cc b/remoting/protocol/third_party_authenticator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..99f9ce42f57827e13227dc33691aebe36211bfbd
--- /dev/null
+++ b/remoting/protocol/third_party_authenticator.cc
@@ -0,0 +1,279 @@
+// 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.
+
+#include "remoting/protocol/third_party_authenticator.h"
+
+#include "base/base64.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/logging.h"
+#include "crypto/rsa_private_key.h"
+#include "remoting/base/constants.h"
+#include "remoting/protocol/channel_authenticator.h"
+#include "remoting/protocol/v2_authenticator.h"
+#include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
+
+#if defined(_WIN32) && defined(GetMessage)

[Sergey Ulanov, 2013/02/26 01:14:50]

Do you need this here?

[rmsousa, 2013/03/05 03:30:24]

Done.

+#undef GetMessage
+#endif
+
+namespace {
+
+const buzz::StaticQName kTokenUrlTag = { remoting::kChromotingXmlNamespace,
+                                              "third-party-token-url" };
+const buzz::StaticQName kTokenScopeTag = { remoting::kChromotingXmlNamespace,
+                                           "third-party-token-scope" };
+const buzz::StaticQName kTokenTag = { remoting::kChromotingXmlNamespace,
+                                      "third-party-token" };
+}  // namespace
+

[Sergey Ulanov, 2013/02/26 01:14:50]

remove extra empty line.

[rmsousa, 2013/03/05 03:30:24]

Done.

+
+namespace remoting {
+namespace protocol {
+
+// static
+scoped_ptr<Authenticator> ThirdPartyAuthenticator::CreateForClient(
+    const std::string& host_public_key,
+    ThirdPartyAuthenticator::TokenFetcher* token_fetcher,
+    Authenticator::State initial_state) {
+  scoped_ptr<ThirdPartyAuthenticator> result(new ThirdPartyAuthenticator(
+      initial_state));
+  result->host_public_key_ = host_public_key;
+  result->token_fetcher_ = token_fetcher;
+  return scoped_ptr<Authenticator>(result.Pass());
+}
+// static

[Wez, 2013/02/27 07:05:30]

nit: Blank line, plz

[rmsousa, 2013/03/05 03:30:24]

Done.

+scoped_ptr<Authenticator> ThirdPartyAuthenticator::CreateForHost(
+    const std::string& local_cert,
+    scoped_ptr<KeyPair> key_pair,
+    const std::string& token_url,
+    const std::string& token_validation_url,
+    const std::string& token_scope,
+    scoped_ptr<ThirdPartyAuthenticator::TokenValidator> token_validator,
+    Authenticator::State initial_state) {
+  scoped_ptr<ThirdPartyAuthenticator> result(new ThirdPartyAuthenticator(
+      initial_state));
+  result->local_cert_ = local_cert;
+  result->key_pair_ = key_pair.Pass();
+  result->host_public_key_ = result->key_pair_->GetPublicKey();
+  result->token_url_ = token_url;
+  result->token_validation_url_ = token_validation_url;
+  result->token_scope_ = token_scope;
+  result->token_validator_ = token_validator.Pass();
+  result->expecting_token_ = false;

[Sergey Ulanov, 2013/02/26 01:14:50]

don't need this - it should be already initialized in the constructor.

[rmsousa, 2013/03/05 03:30:24]

Done.

+  return scoped_ptr<Authenticator>(result.Pass());
+}
+
+ThirdPartyAuthenticator::ThirdPartyAuthenticator(

[Sergey Ulanov, 2013/02/26 01:14:50]

Would it make sense two have separate classes - one for Client and one for Host?
The protocol is not symmetric (unlike in V2Authenticator), so it may simplify
code significantly.  If you think that there is still a lot of code shared
between the two sides then it may make sense to define a class that contains
common logic (I would call it LayeredAuthenticator and put there everything
related to wrapping another Authenticator implementation).

[Wez, 2013/02/27 07:05:30]

Agreed.

You don't really need a public class for this at all, just a pair of constructor
functions, and internally a shared Base, and Client and Host implementations
derived from it.

Not sure that there's enough really generic common logic to pull out a
LayeredAuthenticator base, though?

[rmsousa, 2013/03/05 03:30:24]

Done.

[rmsousa, 2013/03/05 03:30:24]

I separated in one public class plus two local subclasses, but kept them all in
this file, since most of the implementation is common, and having the
client/host in the same file may make it easier to follow the protocol. Let me
know if you prefer them separate.

+    Authenticator::State initial_state)
+    : expecting_token_(false),
+      token_validator_(NULL),
+      token_fetcher_(NULL),
+      state_(initial_state),
+      rejection_reason_(INVALID_CREDENTIALS),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
+}
+
+ThirdPartyAuthenticator::~ThirdPartyAuthenticator() {
+}
+
+Authenticator::State ThirdPartyAuthenticator::state() const {
+  if (state_ == ACCEPTED) {

[Wez, 2013/02/27 07:05:30]

nit: No need for {} for simple single-line if.

+    return underlying_->state();
+  }
+  return state_;
+}
+
+Authenticator::RejectionReason ThirdPartyAuthenticator::rejection_reason()
+    const {
+  DCHECK_EQ(state(), REJECTED);
+  if (state_ == REJECTED) {
+    return rejection_reason_;
+  } else {
+    return underlying_->rejection_reason();
+  }
+}
+
+void ThirdPartyAuthenticator::ProcessMessage(const buzz::XmlElement* message) {
+  DCHECK_EQ(state(), WAITING_MESSAGE);
+
+  // The other side may have started the SPAKE negotiation.
+  // Keep a copy of the message until the V2 authenticator is ready.
+  const buzz::XmlElement* underlying_message =
+      Authenticator::FindAuthenticatorMessage(message);
+  if (underlying_message) {
+    if (underlying_) {
+      // Pass the message through the underlying authenticator.
+      DCHECK_EQ(underlying_->state(), WAITING_MESSAGE);
+      underlying_->ProcessMessage(underlying_message);
+    } else {
+      pending_message_.reset(new buzz::XmlElement(*underlying_message));
+    }
+  }
+
+  if (state_ == WAITING_MESSAGE) {

[Sergey Ulanov, 2013/02/26 01:14:50]

there is a DCHECK for this condition at the top, so you don't really need this
if here..

[Wez, 2013/02/27 07:05:30]

Is the caller responsible for never calling ProcessMessage() when we're not in
the WAITING_MESSAGE state? Or could an extra message sent by the client cause us
to be called in the wrong state?

[rmsousa, 2013/03/05 03:30:24]

the DCHECK is for state(), which is the authenticator's aggregate state (taking
into account the underlying state) - that is, ProcessMessage must be called when
*someone* has a message.
The if block then handles the case where the token autehtnicator itself is
waiting for a message.

In any case, hopefully the refactor made the distinction clearer.

[rmsousa, 2013/03/05 03:30:24]

The caller is responsible for not calling if state() is not WAITING_MESSAGE, but
see comment about the DCHECK below.

+    if (!is_host_side()) {
+      if (token_url_.empty()) {
+        // First message from the host, token URL and scope.

[Wez, 2013/02/27 07:05:30]

nit: Make the above a sentence.  Also, please move it after the DCHECK, to match
the layout below.

super-nit: I also prefer blocks of DCHECKs to be separated from code by a blank
line, here and elsewhere.

+        DCHECK(token_scope_.empty());
+        token_url_ = message->TextNamed(kTokenUrlTag);
+        token_scope_ = message->TextNamed(kTokenScopeTag);
+        if (!token_url_.empty() && !token_scope_.empty()) {
+          state_ = WAITING_EXTERNAL;
+          return;
+        }
+      }
+      LOG(WARNING) << "Missing token issue URL/verification URL/scope.";
+    } else {
+      if (!expecting_token_) {

[Wez, 2013/02/27 07:05:30]

Should this be |has_sent_urls| instead?

[rmsousa, 2013/03/05 03:30:24]

Done.

+        // The host hasn't send the token URLs to the client yet, so ignore the

[Wez, 2013/02/27 07:05:30]

typo: send -> sent

[rmsousa, 2013/03/05 03:30:24]

Done.

+        // first message and send the URLs to the client.
+        state_ = MESSAGE_READY;
+        return;
+      }
+      DCHECK(token_.empty());
+      DCHECK(token_signature_.empty());
+      // Host has already sent the URL and expects a token from the client.
+      token_ = message->TextNamed(kTokenTag);
+      token_signature_ = key_pair_->GetSignature(token_);
+      if (!token_.empty() && !token_signature_.empty()) {
+        state_ = WAITING_EXTERNAL;
+        return;
+      }
+      LOG(WARNING) << "Missing token.";
+    }
+    state_ = REJECTED;
+    rejection_reason_ = PROTOCOL_ERROR;
+    return;
+  }
+}
+
+scoped_ptr<buzz::XmlElement> ThirdPartyAuthenticator::GetNextMessage() {
+  DCHECK_EQ(state(), MESSAGE_READY);
+
+  scoped_ptr<buzz::XmlElement> message = CreateEmptyAuthenticatorMessage();
+  if (state_ == MESSAGE_READY) {

[Sergey Ulanov, 2013/02/26 01:14:50]

same here - there is a DCHECK for this condition.

+    if (!is_host_side()) {
+      if (!token_.empty()) {
+        buzz::XmlElement* token_tag = new buzz::XmlElement(kTokenTag);
+        token_tag->SetBodyText(token_);
+        message->AddElement(token_tag);
+      } else {
+        // The client doesn't really have anything to send yet, it's just
+        // waiting for the host to send the token_url.

[Wez, 2013/02/27 07:05:30]

Surely the client should never be in the MESSAGE_READY state if there's nothing
to send, so this is a NOTREACHED?

[rmsousa, 2013/03/05 03:30:24]

We expect an authentication message on the session-initiate, which comes from
the client. Since the client in this protocol doesn't have anything to send, it
needs to send an empty authentication message.

We can change negotiating_authenticator to not pick an algorithm in the client's
first message, but that's a large-ish separate change (it will also involve some
client JS code, to ask for the PIN after the connection is started rather than
before connecting)

+      }
+    } else {
+      if (token_.empty()) {
+        DCHECK(!token_url_.empty());
+        DCHECK(!token_scope_.empty());
+        buzz::XmlElement* token_url_tag = new buzz::XmlElement(
+            kTokenUrlTag);
+        token_url_tag->SetBodyText(token_url_);
+        message->AddElement(token_url_tag);
+        buzz::XmlElement* token_scope_tag = new buzz::XmlElement(
+            kTokenScopeTag);
+        token_scope_tag->SetBodyText(token_scope_);
+        message->AddElement(token_scope_tag);
+        expecting_token_ = true;
+      }
+    }
+  }
+  if (underlying_ && underlying_->state() == MESSAGE_READY) {

[Wez, 2013/02/27 07:05:30]

nit: Blank lines before & after this if...else... chunk

[rmsousa, 2013/03/05 03:30:24]

Done.

+    DCHECK(!shared_secret_.empty());
+    // This side already has the shared secret and can start the SPAKE
+    // negotiation in parallel with the remaining token messages.
+    message->AddElement(underlying_->GetNextMessage().release());
+    state_ = ACCEPTED;
+  } else {
+    state_ = WAITING_MESSAGE;
+  }
+  return message.Pass();
+}
+
+scoped_ptr<ChannelAuthenticator>
+ThirdPartyAuthenticator::CreateChannelAuthenticator() const {
+  DCHECK_EQ(state(), ACCEPTED);
+  return underlying_->CreateChannelAuthenticator();
+}
+
+bool ThirdPartyAuthenticator::is_host_side() const {
+  return !token_fetcher_;
+}
+
+void ThirdPartyAuthenticator::PerformExternalAction(
+    const base::Closure& resume_callback) {
+  DCHECK_EQ(state(), WAITING_EXTERNAL);
+  if (is_host_side()) {

[Sergey Ulanov, 2013/02/26 01:14:50]

Here the order of client and host parts is reversed from the one used in other
methods. Better make it consistent.

[rmsousa, 2013/03/05 03:30:24]

Done.

+    DCHECK(!token_signature_.empty());
+    DCHECK(!token_validation_url_.empty());
+    DCHECK(!token_.empty());
+    DCHECK(!host_public_key_.empty());
+    DCHECK(!token_scope_.empty());
+    token_validator_->ValidateThirdPartyToken(
+        token_validation_url_, token_, host_public_key_, token_signature_,
+        token_scope_, base::Bind(
+            &ThirdPartyAuthenticator::OnThirdPartyTokenValidated,
+            weak_factory_.GetWeakPtr(),
+            resume_callback));
+  } else {
+    DCHECK(token_.empty());
+    token_fetcher_->FetchThirdPartyToken(
+      token_url_, host_public_key_, token_scope_, base::Bind(
+          &ThirdPartyAuthenticator::OnThirdPartyTokenFetched,
+          weak_factory_.GetWeakPtr(),
+          resume_callback));
+  }
+}
+
+void ThirdPartyAuthenticator::OnThirdPartyTokenFetched(
+    const base::Closure& resume_callback, const std::string& third_party_token,

[Sergey Ulanov, 2013/02/26 01:14:50]

nit: one argument per line please when whole function definition doesn't fit in
one line.
http://dev.chromium.org/developers/coding-style#Code_Formatting

[rmsousa, 2013/03/05 03:30:24]

Done.

+    const std::string& shared_secret) {
+  DCHECK_EQ(state_, WAITING_EXTERNAL);
+  DCHECK(!is_host_side());
+  token_ = third_party_token;
+  if (!token_.empty() && !shared_secret.empty()) {
+    this->OnThirdPartyTokenValidated(resume_callback, shared_secret);
+    return;
+  }
+  state_ = REJECTED;
+  rejection_reason_ = INVALID_CREDENTIALS;
+  resume_callback.Run();
+}
+
+void ThirdPartyAuthenticator::OnThirdPartyTokenValidated(
+    const base::Closure& resume_callback, const std::string& shared_secret) {

[Sergey Ulanov, 2013/02/26 01:14:50]

one argument per line please

[rmsousa, 2013/03/05 03:30:24]

Done.

+  DCHECK_EQ(state_, WAITING_EXTERNAL);
+  shared_secret_ = shared_secret;
+  Authenticator::State initial_state;
+  if (!shared_secret_.empty()) {
+    if (pending_message_) {
+      // The other side already started the SPAKE authentication.
+      state_ = ACCEPTED;
+      initial_state = WAITING_MESSAGE;
+    } else {
+      // The other side still needs data from this side to start SPAKE.
+      state_ = MESSAGE_READY;
+      initial_state = MESSAGE_READY;
+    }
+    if (is_host_side()) {
+      underlying_ = V2Authenticator::CreateForHost(
+          local_cert_, key_pair_->Copy(), shared_secret_,
+          initial_state);
+    } else {
+      underlying_ = V2Authenticator::CreateForClient(
+          shared_secret_, initial_state);
+    }
+    if (pending_message_) {
+      underlying_->ProcessMessage(pending_message_.release());
+      DCHECK_NE(underlying_->state(), WAITING_MESSAGE);
+    }
+  } else {
+    state_ = REJECTED;
+    rejection_reason_ = INVALID_CREDENTIALS;
+  }
+  resume_callback.Run();
+}
+
+}  // namespace protocol
+}  // namespace remoting