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..3d5eafe96a280c8711c4567e748970a1fd1d9472
--- /dev/null
+++ b/remoting/protocol/third_party_authenticator.h
@@ -0,0 +1,141 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

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

nit: 2013 please

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_PROTOCOL_TOKEN_AUTHENTICATOR_H_
+#define REMOTING_PROTOCOL_TOKEN_AUTHENTICATOR_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "crypto/p224_spake.h"
+#include "crypto/signature_creator.h"
+#include "remoting/protocol/authenticator.h"
+
+namespace remoting {
+namespace protocol {
+
+class KeyPair;
+
+class ThirdPartyAuthenticator : public Authenticator {

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

Add comment to explain what this class is for.

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

Done.

+ public:
+  class TokenFetcher {

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

Need virtual destructor for a class with virtual methods.

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

Done.

+   public:

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

Define a virtual dtor for the interface. Since it's lifetime is not
recipient-managed that probably needs to be protected.

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

Done.

+    // 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".
+    // |on_token_fetched| is called by the TokenFetcher once |token| and

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

Clarify which thread the callback is invoked on - must it be the thread the call
is made on?

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

Done.

+    // |shared_secret| are obtained, or called with empty strings on failure.
+    virtual void FetchThirdPartyToken(

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

I think you also need a way to cancel the request - for the case Authenticator
object gets deleted while this request is pending.

Also see my comment for ~TokenValidator()

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

You don't necessarily need a cancel method if the Callback is Bind()ed to a
WeakPtr to the Authenticator.

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

Still useful to have a way to cancel explicitly - e.g. in case this is doing an
URLFetch, we can proactively cancel the fetch instead of waiting for it to
finish to find that the callback is invalidated.

In fact, by having this own the fetcher/validator, we can actually do away with
the weakptr.

+        const std::string& token_url,

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

maybe use GURL type for the URLs?

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

Done.

+        const std::string& host_public_key,
+        const std::string& scope,
+        const base::Callback<void(

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

please add typedef for the callback - that would make this code more readable
and allow to document is separately.

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

Done.

+            const std::string& token,
+            const std::string& shared_secret)>&on_token_fetched) = 0;

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

space before argument name

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

nit: Space between & and member name.
nit: Best to define a typedef for the Callback (e.g. TokenFetcher::Callback)

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

nit: on_token_fetched -> fetch_callback or just callback

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

Done.

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

Done.

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

Done.

+  };
+
+  class TokenValidator {
+   public:
+    // Validates third party |token| with the server at |token_validation_url|,
+    // and exchanges it for a |shared_secret|. |host_public_key| is sent to the
+    // server to identify the host, and must match the one used by the client to
+    // request |token|, and the private key used for |token_signature|. |scope|
+    // is an attribute list (as above) that the token's scope must match.
+    // |on_token_validated| is called by the TokenFetcher once |shared_secret|
+    // is obtained, or called with an empty string on failure.
+    virtual void ValidateThirdPartyToken(

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

Also need a way to cancel the request.

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

Done.

+        const std::string& token_validation_url,
+        const std::string& token,
+        const std::string& host_public_key,
+        const std::string& token_signature,
+        const std::string& scope,

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

Do we expect the content of this string to be in some format? Maybe better to
add a class that parses a scope and use it here.

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

The only ones that need to know about the string contents are the validator and
the fetcher - the authenticator is only a pass-through for it, and needs it to
be a pretty much opaque string.

I've moved everything that can be determined at tokenvalidator construction time
into the validator (and out of this class's constructors). Namely: scope, urls,
keypair.

+        const base::Callback<void(

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

typedef for the callback please

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

Done.

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

Done.

+            const std::string& shared_secret)>& on_token_validated) = 0;

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

nit: on_token_validated -> validation_callback or just callback

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

Done.

+    virtual ~TokenValidator() {}

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

nit: does the destructor need to be public? if not make it protected.

If you make destructor public then you can use it as a way to cancel the
request, but then the Authenticator should take ownership of the implementation
of this interface. IMO that would be the best way to implement request
cancellation.

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

Done.

+  };
+
+  class TokenValidatorFactory {
+   public:
+    // Creates a TokenValidator instance.
+    virtual scoped_ptr<TokenValidator> CreateTokenValidator() = 0;
+  };
+
+  // Creates a third-party client authenticator, for the host with the given
+  // |host_public_key|. |token_fetcher| must outlive this authenticator.
+  static scoped_ptr<Authenticator> CreateForClient(
+      const std::string& host_public_key,
+      ThirdPartyAuthenticator::TokenFetcher* token_fetcher,

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

Does this need to out-live, or could it be scoped_ptr?

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

Done.

+      Authenticator::State initial_state);
+
+  // Creates a third-party host authenticator. |local_cert| is used to establish

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

Why do we need to pass local_cert? Can't it be generated using
KeyPair::GenerateCertificate().

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

Isn't generating certificates expensive? (If I understood our SSL code
correctly, in some OSes each generated certificate will actually have to go into
the system cert store)

The current code generates a certificate once when RemotingMe2MeHost starts, and
uses the same one for every connection - I just kept that behavior.

If we want to generate one certificate for every connection,  we should do it
for every authenticator (i.e. change v2_authenticator to also do it, and remove
local_cert from every constructor signature).

+  // the underlying SSL channels. |key_pair| is used for SSL, as well as to sign
+  // the token for |token_validator|. |token_url| is sent to the client, to be
+  // used by its |TokenFetcher|. |token_validation_url| is used by

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

Why are |token_url|, |token_validator|, |scope|, |key_pair| passed here rather
than to the TokenValidator implementation ctor, which the caller has already had
to call?

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

key_pair needs to be here to construct the underlying v2_authenticator (I don't
want to bake in an assumption that the token_validator necessarily has a key
pair, so I prefer that we pass an explicit reference to the keypair here, even
if it's ultimately the same)

For the token parameters, yeah, good point, I've moved them into the validator.

+  // |token_validator| to obtain the shared secret. |scope| is a list of
+  // connection attributes the host must send to the client, and require the
+  // token to match. This object may add/require its own attributes to |scope|
+  // (e.g. a nonce).
+  static scoped_ptr<Authenticator> CreateForHost(
+      const std::string& local_cert,

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

Why do we need local_cert here together with key_pair? Can't we generate
local_cert from the key pair?

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

see above

+      scoped_ptr<KeyPair> key_pair,
+      const std::string& token_url,
+      const std::string& token_validation_url,
+      const std::string& scope,

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

this argument is called token_scope in .cc file.

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

Done.

+      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) OVERRIDE;
+  virtual scoped_ptr<buzz::XmlElement> GetNextMessage() OVERRIDE;
+  virtual scoped_ptr<ChannelAuthenticator>
+      CreateChannelAuthenticator() const OVERRIDE;
+  virtual void PerformExternalAction(
+      const base::Closure& resume_callback) OVERRIDE;
+
+ private:
+  ThirdPartyAuthenticator(State initial_state);
+  void OnThirdPartyTokenFetched(const base::Closure& resume_callback,
+                                const std::string& third_party_token,
+                                const std::string& shared_secret);
+  void OnThirdPartyTokenValidated(const base::Closure& resume_callback,
+                                  const std::string& shared_secret);

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

nit: Separate these methods by blank lines and add a comment to each of the On*
methods to explain what they do (it's clear from the name when they are called
:)

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

Done.

+  bool is_host_side() const;
+
+  // Used only for host authenticators.
+  bool expecting_token_;
+  std::string local_cert_;
+  scoped_ptr<KeyPair> key_pair_;
+  scoped_ptr<TokenValidator> token_validator_;
+
+  // Used only for client authenticators.
+  TokenFetcher* token_fetcher_;
+  scoped_ptr<buzz::XmlElement> pending_message_;
+  std::string host_public_key_;
+
+  // Used for both host and client authenticators.
+  std::string token_url_;
+  std::string token_validation_url_;
+  std::string token_scope_;
+  std::string token_;
+  std::string token_signature_;
+  std::string shared_secret_;
+  scoped_ptr<Authenticator> underlying_;
+  State state_;
+  RejectionReason rejection_reason_;
+  base::WeakPtrFactory<ThirdPartyAuthenticator> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThirdPartyAuthenticator);
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_TOKEN_AUTHENTICATOR_H_