Third Party authentication protocol.

remoting/protocol/third_party_authenticator.h

Index: remoting/protocol/third_party_authenticator.h
diff --git a/remoting/protocol/third_party_authenticator.h b/remoting/protocol/third_party_authenticator.h
new file mode 100644
index 0000000000000000000000000000000000000000..77aaa2b08ea7d7e5882a1b93599d12bd791505f9
--- /dev/null
+++ b/remoting/protocol/third_party_authenticator.h
@@ -0,0 +1,136 @@
+// 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.
+
+#ifndef REMOTING_PROTOCOL_THIRD_PARTY_AUTHENTICATOR_H_
+#define REMOTING_PROTOCOL_THIRD_PARTY_AUTHENTICATOR_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"

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

Remove this include.

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

Done.

+#include "googleurl/src/gurl.h"
+#include "remoting/protocol/authenticator.h"
+
+namespace remoting {
+
+class RsaKeyPair;
+
+namespace protocol {
+
+// Implements an authentication method that relies on a third party server for
+// authentication of both client and host.

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

Suggest "Authenticator implementation that authenticates client and host via a
third-party."

+// When third party authentication is being used, the client must request both a
+// token and a shared secret from a third-party server (which may require the
+// user to authenticate themselves). The client then sends only the token to the
+// host. The host signs the token, then contacts the third-party server to
+// exchange the token for the shared secret. Once both client and host have the
+// shared secret, they use an underlying |V2Authenticator| (SPAKE2) to negotiate
+// an authentication key, which is used to establish the connection.

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

nit: This looks like it should break down into a set of steps, e.g:

  C->S: Request token authentication from Server.
  S->C: Return token & secret.
  C->H: Pass token to Host.
  H->S: Sign token & pass to Server.
  S->H: Pass secret to Host.
  C<->H: SPAKE2 handshake.

(Except with better wording ;)
Expressing things this way makes it clearer how many round-trips are involved,
for example.

+class ThirdPartyAuthenticator : public Authenticator {
+ public:
+  class TokenFetcher {
+   public:
+    // Callback passed to |FetchThirdPartyToken|, and called once the client
+    // authentication finishes. |token| is an opaque string that should be sent
+    // directly to the host. |shared_secret| should be used by the client to

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

nit: "... will be passed ..."

+    // create a V2Authenticator. In case of failure, the callback is called with

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

nit: "... will be used at the client to create a V2Authenticator."?

i.e. document this as just another part of the ThirdPartyAuthenticator
interface, so document what ThirdPartyAuthenticator requires of the TokenFetcher
implementation.

+    // an empty |token| and |shared_secret|.
+    typedef base::Callback<void(
+        const std::string& token,
+        const std::string& shared_secret)> TokenFetchedCallback;
+
+    virtual ~TokenFetcher() {};
+
+    // Fetches a third party token from |token_url|. |host_public_key| is sent
+    // to the server so it can later authenticate the host. |scope| is a string
+    // with a space-separated list of attributes for this connection (e.g.
+    // "hostjid:abc@example.com/123 clientjid:def@example.org/456".
+    // |token_fetched_callback| is called when the client authentication ends,
+    // in the same thread |FetchThirdPartyToken| was originally called.
+    // The request is canceled if the TokenFetcher is destroyed.

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

See above, re documenting in terms of what the ThirdyPartyAuthenticator expects
of the TokenFetcher impl.

+    virtual void FetchThirdPartyToken(
+        const std::string& token_url,
+        const std::string& host_public_key,
+        const std::string& scope,
+        const TokenFetchedCallback& token_fetched_callback) = 0;
+  };
+
+  class TokenValidator {
+   public:
+    // Callback passed to |ValidateThirdPartyToken|, and called once the host
+    // authentication finishes. |shared_secret| should be used by the host to
+    // create a V2Authenticator. In case of failure, the callback is called with
+    // an empty |shared_secret|.

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

See above re comments.

+    typedef base::Callback<void(
+                const std::string& shared_secret)> TokenValidatedCallback;
+
+    virtual ~TokenValidator() {}
+
+    // Validates |token| with the server and exchanges it for a |shared_secret|.
+    // |token_validated_callback| is called when the host authentication ends,
+    // in the same thread |ValidateThirdPartyToken| was originally called.
+    // The request is canceled if this object is destroyed.
+    virtual void ValidateThirdPartyToken(
+        const std::string& token,
+        const TokenValidatedCallback& token_validated_callback) = 0;
+
+    // URL sent to the client, to be used by its |TokenFetcher| to get a token.
+    virtual const GURL& token_url() const = 0;

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

nit: Blank line after this.

+    // Space-separated list of connection attributes the host must send to the
+    // client, and require the token received in response to match.
+    virtual const std::string& token_scope() const = 0;
+  };
+
+  // Creates a third-party client authenticator, for the host with the given
+  // |host_public_key|. |token_fetcher| must outlive this authenticator.

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

|token_fetcher| is being passed, so it's lifetime is out of the caller's
control.

nit: Document the use of |initial_state|.

+  static scoped_ptr<Authenticator> CreateForClient(
+      const std::string& host_public_key,
+      scoped_ptr<ThirdPartyAuthenticator::TokenFetcher> token_fetcher,
+      Authenticator::State initial_state);
+
+  // Creates a third-party host authenticator. |local_cert| and |key_pair| are
+  // used by the underlying V2Authenticator to create the SSL channels.
+  // |token_validator| contains the token parameters to be sent to the client
+  // and is used to obtain the shared secret.

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

nit: "... used by ... " -> "... passed to ..."

You don't really need the comment re |token_validator| - that should be implicit
from the comments on the TokenValidator interface.

nit: Document the use of |initial_state|.

+  static scoped_ptr<Authenticator> CreateForHost(
+      const std::string& local_cert,
+      scoped_refptr<RsaKeyPair> key_pair,
+      scoped_ptr<ThirdPartyAuthenticator::TokenValidator> token_validator,
+      Authenticator::State initial_state);
+
+  virtual ~ThirdPartyAuthenticator();
+
+  // Authenticator interface.
+  virtual State state() const OVERRIDE;
+  virtual RejectionReason rejection_reason() const OVERRIDE;
+  virtual void ProcessMessage(const buzz::XmlElement* message,
+                              const base::Closure& resume_callback) OVERRIDE;
+  virtual scoped_ptr<buzz::XmlElement> GetNextMessage() OVERRIDE;
+  virtual scoped_ptr<ChannelAuthenticator>
+      CreateChannelAuthenticator() const OVERRIDE;
+
+ protected:
+  explicit ThirdPartyAuthenticator(State initial_state);

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

nit: Blank line after this to separate it from the methods

+  void ProcessUnderlyingMessage(
+      const buzz::XmlElement* message,
+      const base::Closure& resume_callback);
+  virtual void ProcessMessageInternal(
+      const buzz::XmlElement* message,
+      const base::Closure& resume_callback) = 0;
+  virtual void GetNextMessageInternal(buzz::XmlElement* message) = 0;

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

nit: Add comments documenting how/why these are overridden.

+
+  // Used for both host and client authenticators.

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

nit: Suggest "State comment to client and host authenticators"

+  scoped_ptr<Authenticator> underlying_;
+  State state_;
+  RejectionReason rejection_reason_;

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

nit: Should these be private and accessible only via protected setters?

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ThirdPartyAuthenticator);
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_THIRD_PARTY_AUTHENTICATOR_H_