Protocol / client plugin changes to support interactively asking for a PIN

remoting/protocol/negotiating_authenticator.h

Index: remoting/protocol/negotiating_authenticator.h
diff --git a/remoting/protocol/negotiating_authenticator.h b/remoting/protocol/negotiating_authenticator.h
index a87d543e3ca98606ea4265013c0e39cff0056f50..e2fc083bbacdda1d71c19887352cb2af95b0145c 100644
--- a/remoting/protocol/negotiating_authenticator.h
+++ b/remoting/protocol/negotiating_authenticator.h
@@ -11,6 +11,7 @@
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "remoting/protocol/authenticator.h"
 #include "remoting/protocol/authentication_method.h"
 
@@ -20,16 +21,56 @@ class RsaKeyPair;
 
 namespace protocol {
 
+typedef base::Callback<void(const std::string& pin)> PinFetchedCallback;

[Wez, 2013/03/20 01:36:31]

nit: SecretFetchedCallback - PIN is specific to Me2Me.

[rmsousa, 2013/03/20 04:54:54]

Done.

+typedef base::Callback<void(const PinFetchedCallback& pin_fetched_callback)>
+    FetchPinCallback;

[Wez, 2013/03/20 01:36:31]

nit: Ditto here.

[rmsousa, 2013/03/20 04:54:54]

Done.

+
+// This class provides a meta-authenticator that allows clients and hosts that
+// support multiple authentication methods to negotiate a method to use.
+//
+// The typical flow is:
+//  * Client sends a message to host with its supported methods.
+//      (clients may also pick a method and send its first message here).

[Wez, 2013/03/20 01:36:31]

nit: also -> additionally

[rmsousa, 2013/03/20 04:54:54]

Done.

+//  * Host picks a method and sends its first message (if any).
+//      (if a supported method/message was sent by a client, it is processed).

[Wez, 2013/03/20 01:36:31]

nit: second "a" -> "the"

[rmsousa, 2013/03/20 04:54:54]

Done.

[rmsousa, 2013/03/20 04:54:54]

Done.

+//  * Client creates the authenticator selected by the host. If the method
+//      starts with a message from the host, it is processed.
+//  * Client and host exchange messages until the authentication is ACCEPTED or
+//      REJECTED.
+//
+// The details:

[Wez, 2013/03/20 01:36:31]

This entire block is rather verbose. It looks like it could be simplified
considerably.  Key points seem to be:

* Some Autheticators start with a client->host message, others with
host->client.
* Some are symmetric, and can start in either direction.
* Only sequential exchanges are supported (i.e. no simultaneous send/receive).
* When an Authenticator is created it is given a hint as to whether client or
host initiated is preferred, which it may ignore.
* NegotiatingAuthenticator can use this to optimistically initiate a chosen
Authenticator during the negotiation handshake.

[rmsousa, 2013/03/20 04:54:54]

These only describe the 3rd and 4th bulletpoint.
These are not really related. Some authenticators can start in any state, and
some authenticators can be initiated during the handshake, but these are
orthogonal properties. The PIN authenticator can be created in any state, but we
don't want to ask the user for a PIN until we know we need to.

+//  * The underlying message processing may be asynchronous (i.e. require user
+//      interaction based on the message contents), so it receives a callback to
+//      resume processing when done.

[Wez, 2013/03/20 01:36:31]

This seems to be an inherent property of the Authenticator interface.  Why are
you mentioning it here?

[rmsousa, 2013/03/20 04:54:54]

This text is describing the authentication negotiation protocol as a whole, not
just this object. That property is relevant because this object must deal
appropriately with being the middle-man in that situation (i.e. it must keep its
own state consistent when that happens).

+//  * Creating an authenticator may also be asynchronous (i.e. require user
+//      interaction to determine initial parameters, like PIN), so it also
+//      receives a callback.  If the authenticator has received a message to

[Wez, 2013/03/20 01:36:31]

What receives a callback?  The Create*() function? Or something else?

[rmsousa, 2013/03/20 04:54:54]

CreateAuthenticator.

+//      process, the message processing code must also be on that callback.

[Wez, 2013/03/20 01:36:31]

Not sure what "... must also be on that callback" means here?

[rmsousa, 2013/03/20 04:54:54]

If the authenticator is creating an authenticator to process a message, the
callback passed to CreateAuthenticator must include a call to ProcessMessage in
the newly created authenticator.

+//  * Some authentication methods may have a specific starting direction (e.g.
+//      host always sends the first message), while others are versatile (e.g.
+//      SPAKE, where either side can send the first message). So when an
+//      authenticator is created, it is given a "preferred" (context-dependent)
+//      initial state, but it may ignore it, and the negotiating authenticator

[Wez, 2013/03/20 01:36:31]

nit: "but it may ignore it" -> "which the authenticator may ignore"

[rmsousa, 2013/03/20 04:54:54]

Done.

+//      must deal with that, by sending a blank message if the method has none
+//      to send, and ignoring such blank message on the receiving end. This

[Wez, 2013/03/20 01:36:31]

What does it mean to send a "blank message"? You mean we actually add an extra
empty stanza, or that we just don't include the stanza at all?

[rmsousa, 2013/03/20 04:54:54]

An <authenticator> stanza is part of the protocol, and must be always be
included. If there is no message to be sent, that stanza is blank.

+//      relies on the assumption that each method must either be versatile on
+//      both ends, or name an explicit direction to start.

[Wez, 2013/03/20 01:36:31]

This sentence is confusing and I don't think you need it - it wouldn't make
sense for an authentication method to support both starting directions but for
the peer not to.

[rmsousa, 2013/03/20 04:54:54]

Not quite... The V2 authentication is a good example - now that we only want to
ask the client for a pin if we need to, it would have been convenient to declare
that the client side can only start in the WAITING_MESSAGE state (i.e. we only
ask for PIN after we receive the first SPAKE message). At the same time, the
host can still start in any state, since it doesn't need to prompt the user.

If we did that, however, it would break the blank message mechanism (the host
side would try to process that blank message).

+//  * The client may pick a method on its first message (assuming it doesn't
+//      require user interaction to start), and the host may not support that
+//      method, in which case it must discard that message and create an
+//      authenticator for a mutually supported method.
+//  * The host never sends its own supported methods back to the client, so once
+//      the host picks a method from the client's list, it's final.

[Wez, 2013/03/20 01:36:31]

Why do we not return to the handshake and let the host choose another method if
it wants?

[rmsousa, 2013/03/20 04:54:54]

I'm not sure what you mean here. This is just describing the existing protocol
(both in this change, and in the previous code). The sequence is:
1) Client sends host supported-methods="A,B,C" (and maybe a message with
method="A")
2) Host picks one method, and sends either a reply with method="A", or a new
message with method="B".
3) Client continues with the method the host selected (and never sending the
supported-methods again).

I suppose we could, in a future change, let the client decide it doesn't really
want the method that the host picked, and try again with, say,
supported-methods="C" method="C". There are specific cases that won't work with
older host code, but whoever implements it can work around that.

+//  * Any change in this class must maintain compatibility between any version
+//      mix of webapp, client plugin and host, for both Me2Me and IT2Me.

[Wez, 2013/03/20 01:36:31]

This is a good point, although it's not "any" version mix, just the
currently-supported protocols.

[rmsousa, 2013/03/20 04:54:54]

What do you mean currently-supported protocols? This class describes a single
("meta-")protocol. By any version mix I mean that any combination of old/new
host, old/new plugin, old/new webapp must work, both for IT2Me and Me2Me. Any
change to this class must be verified to work in all 16 combinations of current
and previous code (although, hopefully, the (old, old, old, *) combinations can
be assumed to work...)

 class NegotiatingAuthenticator : public Authenticator {
  public:
   virtual ~NegotiatingAuthenticator();
 
-  static bool IsNegotiableMessage(const buzz::XmlElement* message);
-
   // Creates a client authenticator for the given methods.
   static scoped_ptr<Authenticator> CreateForClient(
       const std::string& authentication_tag,
-      const std::string& shared_secret,
+      const FetchPinCallback& fetch_pin_callback,
       const std::vector<AuthenticationMethod>& methods);
 
   // Creates a host authenticator, using a fixed shared secret/PIN hash.
@@ -49,12 +90,31 @@ class NegotiatingAuthenticator : public Authenticator {
       CreateChannelAuthenticator() const OVERRIDE;
 
  private:
-  NegotiatingAuthenticator(Authenticator::State initial_state);
+  explicit NegotiatingAuthenticator(Authenticator::State initial_state);
 
   void AddMethod(const AuthenticationMethod& method);
-  void CreateAuthenticator(State initial_state);
+
+  // (Asynchronously) creates an authenticator. Authenticators that can be
+  // started in either state will be created in |preferred_initial_state|.
+  // |resume_callback| is called when the authenticator is ready.

[Wez, 2013/03/20 01:36:31]

Where does the Authenticator end up?  |current_authenticator_|?

[rmsousa, 2013/03/20 04:54:54]

Done.

+  void CreateAuthenticator(Authenticator::State preferred_initial_state,
+                           const base::Closure& resume_callback);
+
+  // Processes a message in the current authenticator. |message| and

[Wez, 2013/03/20 01:36:31]

nit: Suggest "Calls |current_authenticator_| to process |message|, passing the
supplied |resume_callback|."

[rmsousa, 2013/03/20 04:54:54]

Done.

+  // |resume_callback| are passed to the underlying ProcessMessage call.
+  void ProcessMessageInternal(const buzz::XmlElement* message,
+                              const base::Closure& resume_callback);
+
+  // Updates |state_| to reflect the current underlying authenticator state.

[Wez, 2013/03/20 01:36:31]

Why does this need a |resume_callback|? What is it looking at that "reflects the
state"?  Is it just updating |state_| based on
|current_authenticator_->state()|?

[rmsousa, 2013/03/20 04:54:54]

Yes, it's updating the negotiating_authenticator's state and rejection_reason to
match the ones in which the current_authenticator ended up after ProcessMessage
finished.

It's receiving a resume_callback because it's in the middle of a stack of
callbacks. The Session will give this object a resume_callback on its
ProcessMessage. This object will call the underlying ProcessMessage, and after
that finishes, it must update its state, then call the original resume_callback
received from the Session. So UpdateState is used as the underlying's
resume_callback, and wraps around the original resume_callback received from the
Session.

The same applies for when this is used as a callback from CreateAuthenticator -
CreateAuthentciator is called from inside this object's ProcessMessage - once
CreateAuthenticator finishes, it must update the state and call the originally
received resume_callback.

   void UpdateState(const base::Closure& resume_callback);
 
+  // Creates a V2Authenticator in state |initial_state| with the given
+  // |shared_secret|, then runs |resume_callback|.
+  void OnPinFetched(

[Wez, 2013/03/20 01:36:31]

Why is this called OnPinFetched if it actually creates a V2 authenticator?

[rmsousa, 2013/03/20 04:54:54]

Done.

+      Authenticator::State initial_state,
+      const base::Closure& resume_callback,
+      const std::string& shared_secret);
+
   bool is_host_side() const;
 
   // Used only for host authenticators.
@@ -64,7 +124,7 @@ class NegotiatingAuthenticator : public Authenticator {
 
   // Used only for client authenticators.
   std::string authentication_tag_;
-  std::string shared_secret_;
+  FetchPinCallback fetch_pin_callback_;
 
   // Used for both host and client authenticators.
   std::vector<AuthenticationMethod> methods_;
@@ -73,6 +133,8 @@ class NegotiatingAuthenticator : public Authenticator {
   State state_;
   RejectionReason rejection_reason_;
 
+  base::WeakPtrFactory<NegotiatingAuthenticator> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NegotiatingAuthenticator);
 };