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..6eb08849aa09c8dee3ae99ec70cabcb37b430d0c
--- /dev/null
+++ b/remoting/protocol/third_party_authenticator.cc
@@ -0,0 +1,306 @@
+// Copyright 2013 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 "remoting/base/constants.h"
+#include "remoting/base/rsa_key_pair.h"
+#include "remoting/protocol/channel_authenticator.h"
+#include "remoting/protocol/v2_authenticator.h"
+#include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
+
+namespace {
+
+const buzz::StaticQName kTokenUrlTag = { remoting::kChromotingXmlNamespace,
+                                              "third-party-token-url" };

[Sergey Ulanov, 2013/03/05 04:09:11]

nit: indent

[rmsousa, 2013/03/06 00:54:16]

Done.

+const buzz::StaticQName kTokenScopeTag = { remoting::kChromotingXmlNamespace,
+                                           "third-party-token-scope" };
+const buzz::StaticQName kTokenTag = { remoting::kChromotingXmlNamespace,
+                                      "third-party-token" };
+}  // namespace
+
+namespace remoting {
+namespace protocol {
+
+class ClientThirdPartyAuthenticator : public ThirdPartyAuthenticator {

[Sergey Ulanov, 2013/03/05 04:09:11]

Why does this need to be in .cc file? I suggest renaming ThirdPartyAuthenticator
to ThirdPartyAuthenticatorBase and this class should be called
ThirdPartyClientAuthenticator and placed in
third_party_client_authenticator.[cc|h]. Similarly for the host side.

Then you also won't need static Create() methods as these classes can be created
directly.

[rmsousa, 2013/03/06 00:54:16]

Done.

+ public:
+  ClientThirdPartyAuthenticator(const std::string& host_public_key,
+                                scoped_ptr<TokenFetcher> token_fetcher,
+                                Authenticator::State initial_state);

[Wez, 2013/03/05 22:55:53]

nit: Need public virtual dtor.

[rmsousa, 2013/03/06 00:54:16]

Done.

+
+ protected:
+  // ThirdPartyAuthenticator implementation.
+  virtual void ProcessMessageInternal(
+      const buzz::XmlElement* message,
+      const base::Closure& resume_callback) OVERRIDE;
+  virtual void GetNextMessageInternal(buzz::XmlElement* message) OVERRIDE;
+
+ private:
+  void OnThirdPartyTokenFetched(const base::Closure& resume_callback,
+                                const std::string& third_party_token,
+                                const std::string& shared_secret);
+
+  std::string host_public_key_;
+  std::string token_;
+  scoped_ptr<TokenFetcher> token_fetcher_;
+};
+
+class HostThirdPartyAuthenticator : public ThirdPartyAuthenticator {

[Wez, 2013/03/05 22:55:53]

nit: I'd normally define one class and implement its methods, then define the
other and implement it, and finally follow with the public API implementations.

[rmsousa, 2013/03/06 00:54:16]

separated into different files.

+ public:
+  HostThirdPartyAuthenticator(const std::string& local_cert,
+                              scoped_refptr<RsaKeyPair> key_pair,
+                              scoped_ptr<TokenValidator> token_validator,
+                              Authenticator::State initial_state);

[Wez, 2013/03/05 22:55:53]

nit: Need public virtual dtor.

+
+ protected:
+  // ThirdPartyAuthenticator implementation.
+  virtual void ProcessMessageInternal(
+      const buzz::XmlElement* message,
+      const base::Closure& resume_callback) OVERRIDE;
+  virtual void GetNextMessageInternal(buzz::XmlElement* message) OVERRIDE;
+
+ private:
+  void OnThirdPartyTokenValidated(const buzz::XmlElement* message,
+                                  const base::Closure& resume_callback,
+                                  const std::string& shared_secret);

[Wez, 2013/03/05 22:55:53]

nit: Blank line after this.

+  bool has_sent_urls_;
+  std::string local_cert_;
+  scoped_refptr<RsaKeyPair> key_pair_;
+  scoped_ptr<TokenValidator> token_validator_;
+};
+
+// static
+scoped_ptr<Authenticator> ThirdPartyAuthenticator::CreateForClient(
+    const std::string& host_public_key,
+    scoped_ptr<ThirdPartyAuthenticator::TokenFetcher> token_fetcher,
+    Authenticator::State initial_state) {
+  scoped_ptr<Authenticator> result(new ClientThirdPartyAuthenticator(
+      host_public_key, token_fetcher.Pass(), initial_state));
+  return result.Pass();
+}
+
+// static
+scoped_ptr<Authenticator> ThirdPartyAuthenticator::CreateForHost(
+    const std::string& local_cert,
+    scoped_refptr<RsaKeyPair> key_pair,
+    scoped_ptr<HostThirdPartyAuthenticator::TokenValidator> token_validator,
+    Authenticator::State initial_state) {
+  scoped_ptr<Authenticator> result(new HostThirdPartyAuthenticator(
+      local_cert, key_pair, token_validator.Pass(), initial_state));
+  return result.Pass();
+}
+
+ThirdPartyAuthenticator::ThirdPartyAuthenticator(
+    Authenticator::State initial_state)
+    : state_(initial_state),
+      rejection_reason_(INVALID_CREDENTIALS) {
+}
+
+ThirdPartyAuthenticator::~ThirdPartyAuthenticator() {
+}
+
+Authenticator::State ThirdPartyAuthenticator::state() const {
+  if (state_ == ACCEPTED) {
+    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,
+    const base::Closure& resume_callback) {
+  DCHECK_EQ(state(), WAITING_MESSAGE);
+
+  if (state_ == WAITING_MESSAGE) {
+    ProcessMessageInternal(message, resume_callback);
+  } else {
+    DCHECK(state_ == ACCEPTED);
+    DCHECK(underlying_);
+    DCHECK(underlying_->state() == WAITING_MESSAGE);

[Wez, 2013/03/05 22:55:53]

If these are DCHECKs then will ProcessMessage() do the right thing if
unexpectedly called in Release code?

[rmsousa, 2013/03/06 00:54:16]

The DCHECK means (and documents) that this code is only  statically reachable in
that particular state. If any Release code calls ProcessMessage() unexpectedly,
that code has a bug.

+    underlying_->ProcessMessage(message, resume_callback);
+  }
+}
+
+scoped_ptr<buzz::XmlElement> ThirdPartyAuthenticator::GetNextMessage() {
+  DCHECK_EQ(state(), MESSAGE_READY);
+
+  scoped_ptr<buzz::XmlElement> message;
+  if (underlying_ && underlying_->state() == MESSAGE_READY) {
+    message = underlying_->GetNextMessage().Pass();
+  } else {
+    message = CreateEmptyAuthenticatorMessage();

[Wez, 2013/03/05 22:55:53]

How can we be in the MESSAGE_READY state with no message ready?

[rmsousa, 2013/03/06 00:54:16]

There are two states, the state for the token negotiation, and the state for the
underlying V2Authenticator. The aggregate state is MESSAGE_READY if *one* of
them has a message to send.

+  }
+  GetNextMessageInternal(message.get());
+  return message.Pass();
+}
+
+scoped_ptr<ChannelAuthenticator>
+ThirdPartyAuthenticator::CreateChannelAuthenticator() const {
+  DCHECK_EQ(state(), ACCEPTED);
+
+  return underlying_->CreateChannelAuthenticator();
+}
+
+ClientThirdPartyAuthenticator::ClientThirdPartyAuthenticator(
+    const std::string& host_public_key,
+    scoped_ptr<TokenFetcher> token_fetcher,
+    Authenticator::State initial_state)
+  : ThirdPartyAuthenticator(initial_state),
+    host_public_key_(host_public_key),
+    token_fetcher_(token_fetcher.Pass()) {
+}
+
+void ClientThirdPartyAuthenticator::ProcessMessageInternal(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback) {
+  std::string token_url = message->TextNamed(kTokenUrlTag);

[Wez, 2013/03/05 22:55:53]

What happens if we manage to see two consecutive messages, unexpectedly, e.g.
host mis-behaves?  Do we have a test for that case for the code that drives the
Authenticator?

[rmsousa, 2013/03/06 00:54:16]

JingleSession has code that explicitly rejects messages if the authenticator is
not in the WAITING_MESSAGE state.

There's no unit test for it.

+  std::string token_scope = message->TextNamed(kTokenScopeTag);
+  if (!token_url.empty() && !token_scope.empty()) {
+    state_ = PROCESSING_MESSAGE;
+    // |token_fetcher_| is owned, so Unretained() is safe here.

[Wez, 2013/03/05 22:55:53]

nit: Blank line before this comment.

+    token_fetcher_->FetchThirdPartyToken(
+        token_url, host_public_key_, token_scope, base::Bind(
+            &ClientThirdPartyAuthenticator::OnThirdPartyTokenFetched,
+            base::Unretained(this), resume_callback));
+    return;
+  }
+
+  LOG(WARNING) << "Missing token issue URL/verification URL/scope.";
+  state_ = REJECTED;
+  rejection_reason_ = PROTOCOL_ERROR;
+  resume_callback.Run();
+  return;

[Wez, 2013/03/05 22:55:53]

nit: No need for return here.

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

[Wez, 2013/03/05 22:55:53]

Again, we shouldn't ever reach the MESSAGE_READY state if there's no message
ready...

[rmsousa, 2013/03/06 00:54:16]

Please read the previous comments and replies about this.

+    }
+  }
+}
+
+void ClientThirdPartyAuthenticator::OnThirdPartyTokenFetched(
+    const base::Closure& resume_callback, const std::string& third_party_token,
+    const std::string& shared_secret) {
+  token_ = third_party_token;
+  if (!token_.empty() && !shared_secret.empty()) {
+    state_ = MESSAGE_READY;
+    underlying_ = V2Authenticator::CreateForClient(
+        shared_secret, MESSAGE_READY);
+  } else {
+    state_ = REJECTED;
+    rejection_reason_ = INVALID_CREDENTIALS;
+  }
+  resume_callback.Run();
+}
+
+HostThirdPartyAuthenticator::HostThirdPartyAuthenticator(
+    const std::string& local_cert,
+    scoped_refptr<RsaKeyPair> key_pair,
+    scoped_ptr<TokenValidator> token_validator,
+    Authenticator::State initial_state)
+  : ThirdPartyAuthenticator(initial_state),
+    has_sent_urls_(false),
+    local_cert_(local_cert),
+    key_pair_(key_pair),
+    token_validator_(token_validator.Pass()) {
+}
+
+void HostThirdPartyAuthenticator::ProcessMessageInternal(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback) {
+  if (!has_sent_urls_) {
+    // The host hasn't sent the token URLs to the client yet, so ignore the
+    // first message and send the URLs to the client.

[Wez, 2013/03/05 22:55:53]

Why are we receiving a first message if we're not expecting one?  Sounds like we
should be starting in the MESSAGE_READY state?

[rmsousa, 2013/03/06 00:54:16]

NegotiatingAuthenticator has baked in assumptions about what state each side
starts in. Changing that will be a large-ish CL (propagating all the way to the
client JS code if done correctly). I'll prepare that separately.

+    DCHECK(!underlying_);
+
+    state_ = MESSAGE_READY;
+    resume_callback.Run();
+    return;
+  }
+  // Host has already sent the URL and expects a token from the client.

[Wez, 2013/03/05 22:55:53]

Blank line before this comment.

+  std::string token = message->TextNamed(kTokenTag);
+  if (!token.empty()) {
+    state_ = PROCESSING_MESSAGE;
+    // This message also contains the client's first SPAKE message. Copy the

[Wez, 2013/03/05 22:55:53]

nit: Blank line before this comment.

+    // message into the callback, so that OnThirdPartyTokenValidated can give it
+    // to the underlying SPAKE authenticator that will be created.
+    // |token_validator_| is owned, so Unretained() is safe here.

[Wez, 2013/03/05 22:55:53]

Is there ever a possibility that we'd want to have the token & SPAKE messages
separate, in which case there would be a race between the first SPAKE message
and validation completing?

[rmsousa, 2013/03/06 00:54:16]

No.

There are two possibilities here:
(1) The client is able to create its V2authenticator, so it is able to include
the first SPAKE message together with the token, and it would be a waste of a
roundtrip not to do so.
(2) The client is unable to create the V2Authenticator, in which it doesn't
matter what the host does, because it knows it doesn't have anything else to
send in a reply to the client that would help it create the V2Authenticator.

If there's any case where the client doesn't create the V2Authenticator when it
gets the token, it means it is using a different, incompatible protocol. There's
nothing we can do here except reject (which is what will happen when the
V2Authenticator on the host processes the empty SPAKE message)

+    token_validator_->ValidateThirdPartyToken(token, base::Bind(
+            &HostThirdPartyAuthenticator::OnThirdPartyTokenValidated,
+            base::Unretained(this),
+            base::Owned(new buzz::XmlElement(*message)),
+            resume_callback));
+    return;
+  }
+
+  LOG(WARNING) << "Missing token.";
+  state_ = REJECTED;
+  rejection_reason_ = PROTOCOL_ERROR;
+  resume_callback.Run();
+  return;

[Wez, 2013/03/05 22:55:53]

No need for return here.

+}
+
+void HostThirdPartyAuthenticator::GetNextMessageInternal(
+    buzz::XmlElement* message) {
+  if (state_ == MESSAGE_READY) {
+    DCHECK(token_validator_->token_url().is_valid());
+    DCHECK(!token_validator_->token_scope().empty());
+
+    buzz::XmlElement* token_url_tag = new buzz::XmlElement(
+        kTokenUrlTag);
+    token_url_tag->SetBodyText(token_validator_->token_url().spec());
+    message->AddElement(token_url_tag);
+    buzz::XmlElement* token_scope_tag = new buzz::XmlElement(
+        kTokenScopeTag);
+    token_scope_tag->SetBodyText(token_validator_->token_scope());
+    message->AddElement(token_scope_tag);
+    has_sent_urls_ = true;
+    state_ = WAITING_MESSAGE;
+  }
+}
+
+void HostThirdPartyAuthenticator::OnThirdPartyTokenValidated(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback,
+    const std::string& shared_secret) {
+  if (!shared_secret.empty()) {
+    // The other side already started the SPAKE authentication.

[Wez, 2013/03/05 22:55:53]

nit: Clarify that we know this because the message containing the token also
contains the first SPAKE message.

[rmsousa, 2013/03/06 00:54:16]

We aren't inspecting the message to see what it contains. We know it because
it's part of the protocol. We send the message to the V2Authenticator, and if
the V2Authenticator doesn't find what it's looking for it's a protocol error.

+    state_ = ACCEPTED;
+    underlying_ = V2Authenticator::CreateForHost(
+        local_cert_, key_pair_, shared_secret, WAITING_MESSAGE);
+    underlying_->ProcessMessage(message, resume_callback);
+  } else {
+    state_ = REJECTED;
+    rejection_reason_ = INVALID_CREDENTIALS;
+    resume_callback.Run();
+  }
+}
+
+}  // namespace protocol
+}  // namespace remoting