PairingAuthenticator implementation and plumbing.

remoting/protocol/pairing_host_authenticator.cc

Index: remoting/protocol/pairing_host_authenticator.cc
diff --git a/remoting/protocol/pairing_host_authenticator.cc b/remoting/protocol/pairing_host_authenticator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..22710925ba824585cf0ba12fb2406343f1a68f26
--- /dev/null
+++ b/remoting/protocol/pairing_host_authenticator.cc
@@ -0,0 +1,129 @@
+// 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_host_authenticator.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/pairing_registry.h"
+#include "remoting/protocol/v2_authenticator.h"
+#include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
+
+namespace remoting {
+namespace protocol {
+
+namespace {
+const buzz::StaticQName kPairingInfoQName =
+    { kChromotingXmlNamespace, "pairing-info" };
+const buzz::StaticQName kClientIdAttributeQName =
+    { "", "client-id" };
+const buzz::StaticQName kPairingFailedQName =
+    { kChromotingXmlNamespace, "pairing-failed" };
+const buzz::StaticQName kPairingErrorQName =
+    { "", "error" };
+}
+
+PairingHostAuthenticator::PairingHostAuthenticator(
+    scoped_refptr<PairingRegistry> pairing_registry,
+    const std::string& local_cert,
+    scoped_refptr<RsaKeyPair> key_pair,
+    const std::string& shared_secret,
+    State initial_state)
+    : pairing_registry_(pairing_registry),
+      local_cert_(local_cert),
+      key_pair_(key_pair),
+      shared_secret_(shared_secret),
+      initial_state_(initial_state) {
+  // If the client didn't specify an initial message, use the PIN as the shared

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

This implies a stronger protocol requirement (i.e. that should be part of the
description in the header):
If the client wants to use an existing client_id, it MUST send the initial
message in the protocol. If the host selects spake2_pair, it is too late, and
the client will have to use the PIN.

On the other hand, the server side seems a bit too flexible - the client has the
choice of either initiate by sending a blank spake2_pair message
(missing-client-id), or send no message and let the host pick this method from
the client's supported methods list.

I'm slightly bothered by this asymmetry - is there any reason why we don't
simply forbid a blank message from the client as PROTOCOL_ERROR?

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

Do my changes to the header comments address this concern?
I think you're right. I've modified ProcessMessage to explicitly flag this as an
error.

+  // secret. If it did, the authenticator will be created in ProcessMessage with
+  // the appropriate secret from the pairing registry.
+  if (initial_state_ != WAITING_MESSAGE) {

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

Nit: please DCHECK_EQ(initial_state_, MESSAGE_READY)

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

Done.

+    CreateV2AuthenticatorWithPIN();
+  }
+}
+
+Authenticator::State PairingHostAuthenticator::state() const {
+  if (!error_message_.empty()) {
+    return MESSAGE_READY;
+  }
+  if (v2_authenticator_) {
+    return v2_authenticator_->state();
+  }
+  return initial_state_;

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

Please either have an actual state_ variable, or use an implicit state (in this
case, if !v2_authenticator_, the state is WAITING_MESSAGE, always). Using
initial_state_ anywhere but the constructor is confusing (especially if it's not
trivial to determine that the piece of code using initial_state_ will really
only be used just after construction).

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

An explicit state variable makes less sense that it does in the client because
the REJECTED state (meaning pairing failed) is either a protocol error or a case
of deferring to the underlying authenticator. I've made it implicit and no
longer dependent on the initial state as you suggest.

+}
+
+Authenticator::RejectionReason
+PairingHostAuthenticator::rejection_reason() const {
+  DCHECK(v2_authenticator_);
+  return v2_authenticator_->rejection_reason();
+}
+
+void PairingHostAuthenticator::ProcessMessage(
+    const buzz::XmlElement* message,
+    const base::Closure& resume_callback) {

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

DCHECK_EQ(state(), WAITING_MESSAGE)

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

Done.

+  // If there's already an underlying authenticator, defer to it.
+  if (v2_authenticator_) {
+    v2_authenticator_->ProcessMessage(message, resume_callback);

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

please DCHECK the v2_authenticator_ state here.

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

Done.

+    return;
+  }
+
+  // If not, then create one based on the contents of the first message.
+  std::string client_id;
+  const buzz::XmlElement* pairing_tag = message->FirstNamed(kPairingInfoQName);
+  if (pairing_tag) {
+    client_id = pairing_tag->Attr(kClientIdAttributeQName);
+  }
+
+  if (client_id.empty()) {
+    error_message_ = "missing-client-id";
+    CreateV2AuthenticatorWithPIN();
+    resume_callback.Run();
+    return;
+  }
+
+  std::string paired_secret = pairing_registry_->GetSecret(client_id);
+
+  if (paired_secret.empty()) {
+    error_message_ = "unknown-client-id";
+    CreateV2AuthenticatorWithPIN();
+    resume_callback.Run();
+    return;
+  }
+
+  v2_authenticator_ = V2Authenticator::CreateForHost(
+      local_cert_, key_pair_, paired_secret, MESSAGE_READY);
+  resume_callback.Run();
+}
+
+scoped_ptr<buzz::XmlElement> PairingHostAuthenticator::GetNextMessage() {

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

DCHECK_EQ(state(), MESSAGE_READY)

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

Done.

+  if (!error_message_.empty()) {
+    scoped_ptr<buzz::XmlElement> result = CreateEmptyAuthenticatorMessage();
+    buzz::XmlElement* pairing_failed_tag =
+        new buzz::XmlElement(kPairingFailedQName);
+    pairing_failed_tag->AddAttr(kPairingErrorQName, error_message_);
+    result->AddElement(pairing_failed_tag);
+    error_message_.clear();
+    return result.Pass();
+  } else {
+    DCHECK(v2_authenticator_);
+    return v2_authenticator_->GetNextMessage();
+  }
+}
+
+scoped_ptr<ChannelAuthenticator>
+PairingHostAuthenticator::CreateChannelAuthenticator() const {
+  DCHECK(v2_authenticator_);
+  return v2_authenticator_->CreateChannelAuthenticator();
+}
+
+void PairingHostAuthenticator::CreateV2AuthenticatorWithPIN() {
+  DCHECK(!v2_authenticator_);
+  v2_authenticator_ = V2Authenticator::CreateForHost(
+      local_cert_, key_pair_, shared_secret_, initial_state_);

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

Using initial_state_ is weird here. There are several different flows happening,
with different V2Authenticator states on each side:

(1) Client sends client_id, host recognizes:
  C -> H client_id
  H -> C first spake message
  (...)
(2) Client sends client_id, host does not recognize:
  C -> H client_id
  H -> C error_message
  C -> H first spake message
  (...)
(3) Client does not send a client_id:
  C -> H blank
  H -> C first spake message

And then on the client there is the opposite mapping, of processing or not
processing the first host message based on initial_state_.

The host always has all the information required to send teh first SPAKE message
- It seems simpler to determine that it always does so.

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

It doesn't always send the first message. If the client has specified an
unrecognized id (the pairing has been revoked, for example), the host replies
with a pairing-specific message to tell it to request the PIN instead. In this
case, the client initiates the SPAKE exchange. I've removed the reference to
initial_state_ here though.

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

Yes, I was saying that this variation was confusing. There are two cases where
SPAKE is host initiated, and one where it is client initiated. The host already
has the information to start the SPAKE negotiation in the pairing rejected case,
there's no reason to wait for the client to start it. 

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

Okay, I understand now. I agree, your approach is better. Done.

+}
+
+}  // namespace protocol
+}  // namespace remoting