MAC Cookies (patch 2 of N)

net/http/http_mac_signature.cc

+// 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.
+
+#include "net/http/http_mac_signature.h"
+
+#include "base/base64.h"
+#include "base/rand_util.h"
+#include "base/string_number_conversions.h"
+#include "base/string_util.h"
+#include "base/time.h"
+#include "crypto/hmac.h"
+
+namespace net {
+
+namespace {
+
+const char kSHA1Name[] = "hmac-sha-1";
+const char kSHA256Name[] = "hmac-sha-256";
+const int kNonceLength = 256 / 8;
+
+size_t LengthForHMACAlgorithm(crypto::HMAC::HashAlgorithm algorithm) {

[cbentzel, 2011/04/29 14:44:49]

I'm kind of surprised this isn't an HMAC method. Should it be? 

[abarth-chromium, 2011/04/29 18:16:01]

Yes.  I'll add it.

+  if (algorithm == crypto::HMAC::SHA1)
+    return 20;
+  if (algorithm == crypto::HMAC::SHA256)
+    return 32;
+  NOTREACHED();
+  return 20;
+}
+
+bool IsPlainStringCharacter(char character) {
+  return character == 0x20 || character == 0x21 ||
+    (character >= 0x23 && character <= 0x5B) ||
+    (character >= 0x5D && character <= 0x7E);
+}
+
+bool IsPlainString(const std::string& string) {
+  for (size_t i = 0; i < string.size(); ++i) {
+    if (!IsPlainStringCharacter(string[i]))
+      return false;
+  }
+  return true;
+}
+
+std::string GenerateNonce() {
+  std::string nonce;
+  bool result = base::Base64Encode(base::RandString(kNonceLength), &nonce);
+  DCHECK(result);
+  return nonce;
+}
+
+}
+
+HttpMacSignature::HttpMacSignature()
+    : mac_algorithm_(crypto::HMAC::SHA1)
+{

[cbentzel, 2011/04/29 14:44:49]

Nit: { typically on same line as the last initializer list item.

[abarth-chromium, 2011/04/29 18:16:01]

Fixed.  (WebKit style leaking in.)

+}
+
+HttpMacSignature::~HttpMacSignature() {
+}
+
+bool HttpMacSignature::AddStateInfo(const std::string& id,

[cbentzel, 2011/04/29 14:44:49]

Should this be callable multiple times? 

[abarth-chromium, 2011/04/29 18:16:01]

Not if it returns true.  DCHECK added.

+                                    const std::string& mac_key,
+                                    const std::string& mac_algorithm,
+                                    const std::string& issuer) {
+  if (!IsPlainString(id) || id.empty()
+    || !IsPlainString(mac_key) || mac_key.empty()
+    || !IsPlainString(issuer) || issuer.empty())
+    return false;
+
+  if (mac_algorithm == kSHA1Name)

[cbentzel, 2011/04/29 14:44:49]

Very tiny issue: maybe do input validity checking on mac_algorithm in the
IsPlainString() section above, to be consistent with the other parameters? 

[abarth-chromium, 2011/04/29 18:16:01]

Do you mean checking for IsPlainString ?  The mac_algorithm isn't actually
required to be a plain string.  (Now that I look at it, neither is mac_key.) 
I've added a check for empty() to the above block.

[cbentzel, 2011/04/29 18:37:56]

Sorry, I just meant moving this into

if (!IsPlainString(id) || id.empty() ...
    || !(mac_algorithm == kSHA1Name || mac_algorithm == kSHA256Name))
  return false;

at the top of the function, rather than split input validation to two locations.

+    mac_algorithm_ = crypto::HMAC::SHA1;
+  else if (mac_algorithm == kSHA256Name)
+    mac_algorithm_ = crypto::HMAC::SHA256;
+  else
+    return false;
+
+  id_ = id;
+  mac_key_ = mac_key;
+  issuer_ = issuer;
+  return true;
+}
+
+bool HttpMacSignature::AddHttpInfo(const std::string& method,
+                                   const std::string& request_uri,
+                                   const std::string& host,
+                                   int port) {
+  if (!IsPlainString(method) || method.empty()
+    || !IsPlainString(request_uri) || request_uri.empty()
+    || !IsPlainString(host) || host.empty()
+    || port <= 0)

[cbentzel, 2011/04/29 14:44:49]

Add a port > 65535 check as well. 

[abarth-chromium, 2011/04/29 18:16:01]

Done.

+    return false;
+
+  method_ = StringToUpperASCII(method);
+  request_uri_ = request_uri;
+  host_ = StringToLowerASCII(host);
+  port_ = base::IntToString(port);
+  return true;
+}
+
+std::string HttpMacSignature::GenerateAuthorizationHeader() {
+  DCHECK(!id_.empty()) << "Call AddStateInfo first.";
+  DCHECK(port_.empty()) << "Call AddHttpInfo first.";

[cbentzel, 2011/04/29 14:44:49]

!port_.empty()

[abarth-chromium, 2011/04/29 18:16:01]

I've changed this to used method_, actually, which is slightly more aesthetic.

+
+  std::string timestamp = base::IntToString((base::Time::Now() -
+      base::Time::UnixEpoch()).InSeconds());
+  std::string nonce = GenerateNonce();
+  std::string mac = GenerateMAC(timestamp, nonce);
+
+  DCHECK(IsPlainString(timestamp));
+  DCHECK(IsPlainString(nonce));
+  DCHECK(IsPlainString(mac));
+
+  std::string header = "MAC id=\"" + id_ +
+      "\", issuer=\"" + issuer_ +
+      "\", timestamp=\"" + timestamp +
+      "\", nonce=\"" + nonce +
+      "\", mac=\"" + mac + "\"";
+
+  return header;
+}
+
+std::string HttpMacSignature::GenerateNormalizedRequest(
+    const std::string& timestamp,
+    const std::string& nonce) {
+  static const std::string kNewLine = "\n";
+
+  std::string normalized_request = id_ + kNewLine;
+  normalized_request += issuer_ + kNewLine;
+  normalized_request += timestamp + kNewLine;
+  normalized_request += nonce + kNewLine;
+  normalized_request += method_ + kNewLine;
+  normalized_request += request_uri_ + kNewLine;
+  normalized_request += host_ + kNewLine;
+  normalized_request += port_ + kNewLine;
+
+  return normalized_request;
+}
+
+std::string HttpMacSignature::GenerateMAC(const std::string& timestamp,

[cbentzel, 2011/04/29 14:44:49]

Should these be returning a bool in case hmac.Sign/Base64Encode fail? 

[abarth-chromium, 2011/04/29 18:16:01]

Base64Encode can't fail in these cases because they only fail during overflow
and we've controlled the size of all inputs, which rules out overflow.

It's less clear to me whether Sign can fail.  Do you think a DCHECK is
sufficient, or do think think we should do something more drastic (e.g., CHECK)
?

[cbentzel, 2011/04/29 18:37:56]

DCHECK seems fine, although I have not looked at what would cause hmac.Sign to
fail. 

Another option is to change the signature to bool and propagate the errors along
to the caller, but I think it's fine to contain to DCHECKs inside this function.

+                                          const std::string& nonce) {
+  std::string request = GenerateNormalizedRequest(timestamp, nonce);
+
+  crypto::HMAC hmac(mac_algorithm_);
+  hmac.Init(mac_key_);
+
+  std::string signature;
+  size_t length = LengthForHMACAlgorithm(mac_algorithm_);
+  char* buffer = WriteInto(&signature, length);
+  hmac.Sign(request, reinterpret_cast<unsigned char*>(buffer), length);

[cbentzel, 2011/04/29 14:44:49]

You should check the result of hmac.Sign

[abarth-chromium, 2011/04/29 18:16:01]

I've added a DCHECK.

+
+  std::string encoded_signature;
+  bool result = base::Base64Encode(signature, &encoded_signature);
+  DCHECK(result);
+  return encoded_signature;
+}
+
+}  // namespace net