PairingAuthenticator implementation and plumbing.

remoting/protocol/pairing_client_authenticator.cc

Index: remoting/protocol/pairing_client_authenticator.cc
diff --git a/remoting/protocol/pairing_client_authenticator.cc b/remoting/protocol/pairing_client_authenticator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ed6d65a0d895d414954207a006bbdb93791d1fea
--- /dev/null
+++ b/remoting/protocol/pairing_client_authenticator.cc
@@ -0,0 +1,111 @@
+// 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/pairing_client_authenticator.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "remoting/base/constants.h"
+#include "remoting/base/rsa_key_pair.h"
+#include "remoting/protocol/authentication_method.h"
+#include "remoting/protocol/channel_authenticator.h"
+#include "remoting/protocol/v2_authenticator.h"
+#include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
+
+namespace remoting {
+namespace protocol {
+
+namespace {
+const buzz::StaticQName kPairingQName =

[rmsousa, 2013/05/15 01:25:16]

Nit: We use kPairingTag for tags inside an authenticator.

[rmsousa, 2013/05/15 01:25:16]

Nit: I'd prefer if these were in a shared place, although there aren't enough
commonalities between client and host to justify a PairingAuthenticatorBase, and
there isn't anywhere else that seems appropriate... so I suppose leave a comment
in each file pointing out that it's in sync with its antipode?

[Jamie, 2013/05/15 23:41:08]

Done.

[Jamie, 2013/05/15 23:41:08]

Done.

+    { kChromotingXmlNamespace, "pairing-info" };
+const buzz::StaticQName kClientIdAttributeQName =
+    { "", "client-id" };
+const buzz::StaticQName kPairingFailedQName =
+    { kChromotingXmlNamespace, "pairing-failed" };
+const buzz::StaticQName kPairingErrorQName =
+    { "", "error" };
+}
+
+PairingClientAuthenticator::PairingClientAuthenticator(
+    const std::string& client_id,
+    const std::string& shared_secret,
+    const FetchSecretCallback& fetch_secret_callback,
+    const std::string& authentication_tag)
+    : initial_message_sent_(false),
+      client_id_(client_id),
+      shared_secret_(shared_secret),
+      fetch_secret_callback_(fetch_secret_callback),
+      authentication_tag_(authentication_tag),
+      pairing_failed_(false),
+      weak_factory_(this) {
+  v2_authenticator_ = V2Authenticator::CreateForClient(

[rmsousa, 2013/05/15 01:25:16]

There is no need to create this underlying authenticator in the WAITING_MESSAGE
state here. It won't be used to send the first message, and it may end up being
deleted and recreated when you process the next message - you might as well wait
for the next ProcessMessage, and create the appropriate authenticator when you
receive the message from the host and actually know which authenticator/state to
create (PIN Authenticator in MESSAGE_READY state if it's a failure, SharedSecret
Authenticator in WAITING_MESSAGE state if it's a success).

[Jamie, 2013/05/15 23:41:08]

Done.

+      shared_secret_, WAITING_MESSAGE);
+}
+
+Authenticator::State PairingClientAuthenticator::state() const {
+  if (!initial_message_sent_) {
+    return MESSAGE_READY;
+  } else if (!v2_authenticator_) {
+    return PROCESSING_MESSAGE;
+  }
+  return v2_authenticator_->state();
+}
+
+Authenticator::RejectionReason
+PairingClientAuthenticator::rejection_reason() const {
+  DCHECK(v2_authenticator_);
+  return v2_authenticator_->rejection_reason();
+}
+
+void PairingClientAuthenticator::ProcessMessage(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback) {
+  const buzz::XmlElement* pairing_failed_tag =
+      message->FirstNamed(kPairingFailedQName);
+  if (pairing_failed_tag) {
+    // If pairing failed, prompt the user for the PIN and try again.
+    std::string error = pairing_failed_tag->Attr(kPairingErrorQName);
+    LOG(INFO) << "Pairing failed: " << error;
+    v2_authenticator_.reset();
+    SecretFetchedCallback callback = base::Bind(
+        &PairingClientAuthenticator::CreateV2AuthenticatorWithSecret,
+        weak_factory_.GetWeakPtr(), resume_callback);

[rmsousa, 2013/05/16 22:16:37]

(Comment for a future CL, when you implement the UI)
It might be a good idea to pass the pairing error in the fetch_secret_callback.
If I connect to a host to which I'm supposed to be paired, and end up getting a
PIN dialog, I'd like to know if that's because my local Chrome isn't paired, the
host doesn't recognize my pairing ID, or the host's registry is corrupted.

[Jamie, 2013/05/16 22:57:46]

Good point.

+    fetch_secret_callback_.Run(callback);
+    return;
+  }
+  v2_authenticator_->ProcessMessage(message, resume_callback);
+}
+
+scoped_ptr<buzz::XmlElement> PairingClientAuthenticator::GetNextMessage() {
+  // If the initial message has not yet been sent, return it now.
+  if (!initial_message_sent_) {

[rmsousa, 2013/05/15 01:25:16]

This is wrong when the host sends the first message to the (unpaired) client -
the host is already using PIN authentication.

if you delay the v2_authenticator_ creation, its existence seems like a better
check here (and initial_message_sent_ isn't necessary at all).

[Jamie, 2013/05/15 23:41:08]

My understanding is that the client always sends the first message--the
optimistic client-id payload on the supported-methods tag. Is that not the case?

In any case, I've modified this method in light of your previous comment to have
an explicit pairing_state_ member, which may address your concerns.

[rmsousa, 2013/05/16 00:46:25]

No. If the client does not have a client ID, the code on
NegotiatingClientAuthenticator does not pick a method or create an
authenticator. This will cause the host to pick the spake2_pair method, and
create PairingHostAuthenticator in the MESSAGE_READY state (meaning it will send
the first message of the PIN-based V2Authenticator). Then the client will
receive that message, and create a PairingClientAuthenticator in the
WAITING_MESSAGE state.

[Jamie, 2013/05/16 19:30:16]

Okay, I think we're talking at cross-purposes here. I agree that in this case,
the host sends the first message in the underlying authentication protocol.
However, in this case we won't be using this class at all.

[rmsousa, 2013/05/16 20:12:59]

Oh. I just realized that - CreateAuthenticator will create a V2Authenticator for
this method. In that case, you have a different problem - if you use that
authenticator, it will use the default pin fetcher (without the checkbox). I
think the better way to fix that is to use this authenticator for every case
where method = spake2_pair.

[Jamie, 2013/05/16 21:09:09]

I don't think that's true. The PairingSupportedButNotPaired test covers that
case, and it verifies that the method name exposed to the client is Spake2Pair.
Do you think it's worth changing the implementation anyway to make it clearer
that the pairing authenticator is always used for Spake2Pair (even if it's just
passing messages through to the v2 authenticator)? I worry that it might make
the implementation more complicated than it needs to be, but I haven't tried it
yet.

[rmsousa, 2013/05/16 22:16:37]

Oh, I think I misunderstood your design. I was assuming that you added
current_method() to NegotiatingAuthenticator just for the unit tests.

If you were you planning to have the client go back to the negotiating
authenticator when it receives a fetch_secret_callback_, to check the current
method, and use that to decide whether to show the checkbox, I don't think that
would work - the client has no pointer to the authenticator after the session is
started (and that is by design). That information will need to be passed as a
parameter in the callback itself.

As far as putting the logic inside this class: we should keep a coherent mapping
of authentication method to authenticator class, and the logic for an
authentication method should be entirely contained in its class, without
spilling on the negotiating authenticator. We shouldn't have a method where we
don't know what authenticator class is currently being used, or where the host
may be talking to two different client classes - that makes future backwards
compatibility much more difficult. 

(You may notice that V2Authenticator is an anti-pattern to pretty much
everything I just said - there's quite a bit of its logic living on the
NegotiatingAuthenticator. But that actually reinforces this point - the only
reason why it's so messy is because previous clients/hosts could have ambiguous
V2Authenticator states for the same method name, and when we added the async pin
flow (which also had to use the same method name to be backwards compatible), we
had to deal with all this ambiguity to keep it backwards compatible with older
webapps/hosts).

[Jamie, 2013/05/16 22:57:46]

That's correct.
The way this is working in my uncommitted branch right now is that when the
negotiating authenticator asks the client to prompt for a PIN, it passes a bool
which it sets to (current_method_ == Spake2Pair). In the rare cases when the
pairing authenticator prompts directly (when pairing fails), it just passes
true. That flag controls whether or not to show the "remember me" checkbox.

The way this interacts with the unit tests is that right now I want to be able
to verify that the correct protocol was used. Once the callback has a bool
parameter, I'll be able to check that directly, and add checks to make sure that
the callback isn't called at all when I don't expect it to be. At that point, I
can probably remove the current_method accessor again.
Yes, I think I agree. Let me see what the code looks like if it always uses the
pairing authenticators for Spake2Pair.

[rmsousa, 2013/05/16 23:22:24]

Oh, OK, we're in the same page, then. 
If it ends up too ugly, another option to at least keep them symmetric between
client and host is to change negotiating_host_authenticator::CreateAuthenticator
to create a V2Autehnticator for method=spake2_pair, state=MESSAGE_READY, and a
PairingAutehnticator for method=spake2_pair, state = WAITING_MESSAGE.
That also cleans up some of the initial_state handling in
PairingHostAuthentciator.

I'm fine with either approach.

[Jamie, 2013/05/17 17:55:45]

I tried using the pairing authenticator for the exchange even if the client is
not already paired, but as I feared it makes the code a lot more complex and
breaks one of your previous points about having the host always initiate the
Spake exchange. You second option of making the two sides symmetric is much
simpler, so I've gone with that.

+    scoped_ptr<buzz::XmlElement> result = CreateEmptyAuthenticatorMessage();
+    buzz::XmlElement* pairing_tag = new buzz::XmlElement(kPairingQName);
+    pairing_tag->AddAttr(kClientIdAttributeQName, client_id_);
+    result->AddElement(pairing_tag);
+    initial_message_sent_ = true;
+    return result.Pass();
+  }
+  // In all other cases, defer to the underlying authenticator.
+  return v2_authenticator_->GetNextMessage();
+}
+
+scoped_ptr<ChannelAuthenticator>
+PairingClientAuthenticator::CreateChannelAuthenticator() const {
+  return v2_authenticator_->CreateChannelAuthenticator();
+}
+
+void PairingClientAuthenticator::CreateV2AuthenticatorWithSecret(
+    const base::Closure& resume_callback,
+    const std::string& shared_secret) {
+  v2_authenticator_ = V2Authenticator::CreateForClient(
+      AuthenticationMethod::ApplyHashFunction(
+          AuthenticationMethod::HMAC_SHA256,
+          authentication_tag_, shared_secret),
+      MESSAGE_READY);
+  resume_callback.Run();
+}
+
+}  // namespace protocol
+}  // namespace remoting