net: add low-entropy, shared secret authentication protocol.

crypto/mutual_auth.h

+// Copyright (c) 2011 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 CRYPTO_MUTUAL_AUTH_H_
+#define CRYPTO_MUTUAL_AUTH_H_

[Wez, 2011/11/09 22:43:46]

"mutual auth" seems a very generic name for this, given that it implements a
specific algorithm.  How would you feel about crypto/p224_eke.*, for example?

[agl, 2011/11/11 16:05:17]

Done: p224_spake.cc

[Wez, 2011/11/12 00:50:23]

Technically SPAKE2...? ;)

+#pragma once
+
+#include <base/string_piece.h>
+#include <crypto/p224.h>
+#include <crypto/sha2.h>
+
+namespace crypto {
+
+// SharedSecretMutualAuthentication provides a means to authenticate an
+// encrypted transport using a low-entropy, shared secret.
+//
+// You need a value derived from the master secret of the connection in order
+// to bind the authentication to the encrypted channel. It's the |session|
+// argument to the constructor and can be of any length.
+//
+// The password can be low entropy as authenticating with an attacker only
+// gives the attacker a one-shot password oracle. No other information about
+// the password is leaked. (However, you must be sure to limit the number of
+// permitted authentication attempts otherwise they get many one-shot oracles.)
+//
+// The protocol requires several RTTs (actually two, but you shouldn't assume
+// that.) To use the object, call GetMessage() and pass that message to the
+// peer. Get a message from the peer and feed it into ProcessMessage. Then
+// examine the return value of ProcessMessage:
+//   kResultPending: another round is required. Call GetMessage and repeat.

[Wez, 2011/11/09 22:43:46]

nit: Capitalize "another", and similarly for Failed and Success.

[agl, 2011/11/11 16:05:17]

I don't think my English teacher would agree, but whatever. Done.

+//   kResultFailed: the authentication has failed. You can get a human readable
+//       error message by calling error().
+//   kResultSuccess: the authentication was successful.
+//
+// In each exchange, each peer always sends a message.
+class CRYPTO_EXPORT SharedSecretMutualAuthentication {

[Wez, 2011/11/09 22:43:46]

Would EncryptedKeyExchange or something of that ilk be a better name for this?

[agl, 2011/11/11 16:05:17]

Well, if the filename is algorithm specific then so should this be. So it's
P224EncryptedKeyExchange because P224SPAKE looked too ALLCAPS.

+ public:
+  enum Result {
+    kResultPending,
+    kResultFailed,
+    kResultSuccess,
+  };
+
+  // PeerType's values are named client and server due to convention. But
+  // they could be called "A" and "B" as far as the protocol is concerned so
+  // long as the two parties don't both get the same label.
+  enum PeerType {
+    kClient,
+    kServer,
+  };

[Wez, 2011/11/09 22:43:46]

nit: Should these be kPeerTypeClient and kPeerTypeServer?

The comment doesn't really explain to be what it _means_ to be the client, or
the server.  Why, as the caller, do I care, especially given it's mutual-auth?

If you has the PeerType actually be a p224::Point and defined kClient and
kServer via the M and N values then you could do away with |is_server_|...

[agl, 2011/11/11 16:05:17]

Done.
I think the comment is pretty clear and I certainly don't want to expose a
P224::Point in this API.

+
+  // peer_type: the type of the local authentication party.
+  // password: a, possibly low-entropy, mutually known password.
+  // session: a value securely derived from the connection's master secret.
+  //     Both parties to the authentication must pass the same value. For the
+  //     case of a TLS connection, see RFC 5705.
+  SharedSecretMutualAuthentication(PeerType peer_type,
+                                   const base::StringPiece& password,
+                                   const base::StringPiece& session);

[Wez, 2011/11/09 22:43:46]

Are there any size limits that we should enforce on |password| or |session|, as
crypto::HMAC does, for example?

[agl, 2011/11/11 16:05:17]

Nope

+
+  // GetMessage returns a byte string which must be passed to the other party
+  // in the authentication.
+  const std::string& GetMessage();

[Wez, 2011/11/09 22:43:46]

What happens if the caller calls this twice between ProcessMessage calls?

GSSAPI has a single process message call that both processes the message and
returns the next message to send, if any.  Would that make sense here?  (You'd
need callers to make a start-of-day call to the function with an empty message,
which is a little ugly, of course.)

[agl, 2011/11/11 16:05:17]

It LOG(FATAL)s.
Seems more ugly.

+
+  // ProcessMessage processes a message which must have been generated by a
+  // call to GetMessage() by the other party.
+  Result ProcessMessage(const base::StringPiece& message);
+
+  // In the event that ProcessMessage() returns kResultFailed, error will
+  // return a human readable error message.
+  const std::string& error() const;

[Wez, 2011/11/09 22:43:46]

Wot no numeric error code that calling code can switch on if necessary?

[agl, 2011/11/11 16:05:17]

Nope, because the higher level code should't act differently based on the error.
This is just a debugging aid.

[Wez, 2011/11/12 00:50:23]

nit: Add the ", to aid debugging" to the comment, then?

+
+ private:
+  // The authentication state machine is very simple and each party proceeds
+  // through each of these states, in order.
+  enum State {
+    kStateInitial,
+    kStateRecvDH,
+    kStateSendHash,
+    kStateRecvHash,
+    kStateDone,
+  };
+
+  const bool is_server_;
+
+  State state_;

[Wez, 2011/11/09 22:43:46]

nit: Blank line after |state_|, please.

[agl, 2011/11/11 16:05:17]

moot after moving.

+  // x_ is the secret Diffie-Hellman exponent.
+  uint8 x_[p224::kScalarBytes];
+  // pw_ is SHA256(P(password), P(session))[:28] where P() prepends a uint32,
+  // big-endian length prefix.

[Wez, 2011/11/09 22:43:46]

nit: Are "x" and "pw" the EKE terminology?  If so then perhaps a comment above
these fields indicating that fact would give the reader context for
understanding the names.

[agl, 2011/11/11 16:05:17]

Done.

+  uint8 pw_[p224::kScalarBytes];
+  // expected_authenticator_ is used to store the hash value expected from the
+  // other party.

[Wez, 2011/11/09 22:43:46]

nit: If it's the expected hash value, why not |expected_hash_|?

[agl, 2011/11/11 16:05:17]

Because it's less specific.

+  uint8 expected_authenticator_[kSHA256Length];
+  // next_message_ contains a value for GetMessage() to return.
+  std::string next_message_;
+  std::string error_;

[Wez, 2011/11/09 22:43:46]

nit: Consider moving these implementation-detail fields to join |is_server_| and
|state_|, rather than bundling them with the EKE fields above?

[agl, 2011/11/11 16:05:17]

Done.

+};
+
+}  // namespace crypto
+
+#endif  // MUTUAL_AUTH_H_