Change origin trial token format

content/common/origin_trials/trial_token.cc

Index: content/common/origin_trials/trial_token.cc
diff --git a/content/common/origin_trials/trial_token.cc b/content/common/origin_trials/trial_token.cc
index e44f6f046a2c712eaa6144f10d5aec51d9c4e9bf..658200c4fcb708b12e3f91120361a601e706b2d3 100644
--- a/content/common/origin_trials/trial_token.cc
+++ b/content/common/origin_trials/trial_token.cc
@@ -9,12 +9,11 @@
 #include <vector>
 
 #include "base/base64.h"
+#include "base/json/json_reader.h"
 #include "base/macros.h"
-#include "base/strings/string_number_conversions.h"
-#include "base/strings/string_split.h"
-#include "base/strings/string_util.h"
-#include "base/strings/utf_string_conversions.h"
+#include "base/strings/string_piece.h"
 #include "base/time/time.h"
+#include "base/values.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -22,84 +21,136 @@ namespace content {
 
 namespace {
 
-// Version 1 is the only token version currently supported
-const uint8_t kVersion1 = 1;
+// Version is a 1-byte field at offset 0

[Avi (use Gerrit), 2016/04/08 21:34:24]

Make comments full sentences and end them with a full stop/period.

[iclelland, 2016/04/11 16:59:37]

Done.

+const size_t kVersionOffset = 0;
+const size_t kVersionSize = 1;
 
-const char* kFieldSeparator = "|";
+// Version 2 is the only token version currently supported. Version 1 was

[Marijn Kruisselbrink, 2016/04/08 21:19:30]

nit: Would it make sense to keep the offsets and sizes of fields together and
move the definition of kVersion2 to be outside this block of constants? Either
before or after.

[iclelland, 2016/04/11 16:59:37]

Done.

+// introduced in Chrome M50, and removed in M51. There were no experiments
+// enabled in the stable M50 release which would have used those tokens.
+const uint8_t kVersion2 = 2;
+
+// Version 2 field sizes and offsets

[Avi (use Gerrit), 2016/04/08 21:34:24]

.

[iclelland, 2016/04/11 16:59:37]

Done. And turned into a full sentence.

+const size_t kSignatureOffset = kVersionOffset + kVersionSize;
+const size_t kSignatureSize = 64;
+const size_t kPayloadLengthOffset = kSignatureOffset + kSignatureSize;
+const size_t kPayloadLengthSize = 4;
+const size_t kPayloadOffset = kPayloadLengthOffset + kPayloadLengthSize;
 
 }  // namespace
 
 TrialToken::~TrialToken() {}
 
-scoped_ptr<TrialToken> TrialToken::Parse(const std::string& token_text) {
-  if (token_text.empty()) {
+// Static
+scoped_ptr<TrialToken> TrialToken::From(const std::string& token_text,
+                                        base::StringPiece public_key) {
+  scoped_ptr<std::string> token_payload = Extract(token_text, public_key);
+  if (!token_payload) {
+    return nullptr;
+  }
+  return Parse(*token_payload);
+}
+
+bool TrialToken::IsValidForFeature(const url::Origin& origin,
+                                   base::StringPiece feature_name,
+                                   const base::Time& now) const {
+  return ValidateOrigin(origin) && ValidateFeatureName(feature_name) &&
+         ValidateDate(now);
+}
+
+scoped_ptr<std::string> TrialToken::Extract(const std::string& token_payload,
+                                            base::StringPiece public_key) {
+  if (token_payload.empty()) {
     return nullptr;
   }
 
-  // Extract the version from the token. The version must be the first part of
-  // the token, separated from the remainder, as:
-  // version|<version-specific contents>
-  size_t version_end = token_text.find(kFieldSeparator);
-  if (version_end == std::string::npos) {
+  // Token is base64-encoded; decode first.
+  std::string token_contents;
+  if (!base::Base64Decode(token_payload, &token_contents)) {
     return nullptr;
   }
 
-  std::string version_string = token_text.substr(0, version_end);
-  unsigned int version = 0;
-  if (!base::StringToUint(version_string, &version) || version > UINT8_MAX) {
+  // Only version 2 currently supported.
+  if (token_contents.length() < (kVersionOffset + kVersionSize)) {
+    return nullptr;
+  }
+  uint8_t version = token_contents[kVersionOffset];
+  if (version != kVersion2) {
     return nullptr;
   }
 
-  // Only version 1 currently supported
-  if (version != kVersion1) {
+  // Token must be large enough to contain a version, signature, and payload
+  // length.
+  if (token_contents.length() < (kPayloadLengthOffset + kPayloadLengthSize)) {
+    return nullptr;
+  }
+
+  // Extract the length of the signed data (Big-endian).

[Marijn Kruisselbrink, 2016/04/08 21:19:30]

Can you use something like base::BigEndianReader to do all this parsing? That
seems safer than implementing your own parsing and length checks...

[iclelland, 2016/04/11 16:59:37]

Yes! Thanks, I was looking around for just the function (mostly with various
spellings of 'ntohl')

(Not a reader object, though -- seems like way too much overhead for a single
field. ReadBigEndian<uint32_t> is perfect, though.)

+  uint32_t payload_length =
+      ((uint8_t)token_contents[kPayloadLengthOffset] << 24) +

[Marijn Kruisselbrink, 2016/04/08 21:19:30]

C-style casts are against the style guide. Either static_cast (or if and when {}
style initialization is allowed in chromium use uint8_t{foo} like the style
guide suggests).

[iclelland, 2016/04/11 16:59:37]

Acknowledged. Thanks, switched to ReadBigEndian to avoid the whole issue.

+      ((uint8_t)token_contents[kPayloadLengthOffset + 1] << 16) +
+      ((uint8_t)token_contents[kPayloadLengthOffset + 2] << 8) +
+      ((uint8_t)token_contents[kPayloadLengthOffset + 3]);
+
+  // Validate that the stated length matches the actual payload length.
+  if (payload_length != token_contents.length() - kPayloadOffset) {
     return nullptr;
   }
 
   // Extract the version-specific contents of the token

[Avi (use Gerrit), 2016/04/08 21:34:24]

.

[iclelland, 2016/04/11 16:59:37]

Done.

-  std::string token_contents = token_text.substr(version_end + 1);
-
-  // The contents of a valid version 1 token should resemble:
-  // signature|origin|feature_name|expiry_timestamp
-  std::vector<std::string> parts =
-      SplitString(token_contents, kFieldSeparator, base::KEEP_WHITESPACE,
-                  base::SPLIT_WANT_ALL);
-  if (parts.size() != 4) {
+  const char* token_bytes = token_contents.data();
+  base::StringPiece version_piece(token_bytes + kVersionOffset, kVersionSize);
+  base::StringPiece signature(token_bytes + kSignatureOffset, kSignatureSize);
+  base::StringPiece payload_piece(token_bytes + kPayloadLengthOffset,

[Marijn Kruisselbrink, 2016/04/08 21:19:30]

nit: I was a bit confused on first reading that "payload_piece" represents both
the payload length and the payload itself. Fine to keep it this way, but maybe
have separate payload_length_piece and payload_piece variables? That way you're
also more robust against somebody changing the length&offset constants without
realizing that the code depends on payload length and payload are consecutive.

[iclelland, 2016/04/11 16:59:37]

I was trying to avoid extra string allocations during concatenation, following
chasej@s comments.

+                                  kPayloadLengthSize + payload_length);
+
+  // The data which is covered by the signature is (version + length + payload)

[Avi (use Gerrit), 2016/04/08 21:34:24]

.

[iclelland, 2016/04/11 16:59:37]

Done.

+  std::string signed_data =
+      version_piece.as_string() + payload_piece.as_string();
+
+  // Validate the signature on the data before proceeding

[Avi (use Gerrit), 2016/04/08 21:34:23]

.

[iclelland, 2016/04/11 16:59:37]

Done.

+  if (!TrialToken::ValidateSignature(signature, signed_data, public_key)) {
     return nullptr;
   }
 
-  const std::string& signature = parts[0];
-  const std::string& origin_string = parts[1];
-  const std::string& feature_name = parts[2];
-  const std::string& expiry_string = parts[3];
+  // Return just the payload, as a new string.
+  return make_scoped_ptr(
+      new std::string(token_contents, kPayloadOffset, payload_length));
+}
 
-  uint64_t expiry_timestamp;
-  if (!base::StringToUint64(expiry_string, &expiry_timestamp)) {
+scoped_ptr<TrialToken> TrialToken::Parse(const std::string& token_json) {
+  // signed_data is JSON-encoded

[Avi (use Gerrit), 2016/04/08 21:34:24]

.

etc

[iclelland, 2016/04/11 16:59:37]

Done.

+  scoped_ptr<base::DictionaryValue> datadict =
+      base::DictionaryValue::From(base::JSONReader::Read(token_json));
+  if (!datadict) {
     return nullptr;
   }
 
+  std::string origin_string;
+  std::string feature_name;
+  int expiry_timestamp = 0;
+  datadict->GetString("origin", &origin_string);
+  datadict->GetString("feature", &feature_name);
+  datadict->GetInteger("expiry", &expiry_timestamp);
+
   // Ensure that the origin is a valid (non-unique) origin URL
   url::Origin origin = url::Origin(GURL(origin_string));
   if (origin.unique()) {
     return nullptr;
   }
 
-  // Signed data is (origin + "|" + feature_name + "|" + expiry).
-  std::string data = token_contents.substr(signature.length() + 1);
-
-  return make_scoped_ptr(new TrialToken(version, signature, data, origin,
-                                        feature_name, expiry_timestamp));
-}
+  // Ensure that the feature name is a valid string
+  if (feature_name.empty()) {
+    return nullptr;
+  }
 
-bool TrialToken::IsAppropriate(const url::Origin& origin,
-                               base::StringPiece feature_name) const {
-  return ValidateOrigin(origin) && ValidateFeatureName(feature_name);
-}
+  // Ensure that the expiry timestamp is a valid (positive) integer
+  if (expiry_timestamp <= 0) {
+    return nullptr;
+  }
 
-bool TrialToken::IsValid(const base::Time& now,
-                         base::StringPiece public_key) const {
-  // TODO(iclelland): Allow for multiple signing keys, and iterate over all
-  // active keys here. https://crbug.com/543220
-  return ValidateDate(now) && ValidateSignature(public_key);
+  return make_scoped_ptr(
+      new TrialToken(origin, feature_name, expiry_timestamp));
 }
 
 bool TrialToken::ValidateOrigin(const url::Origin& origin) const {
@@ -114,23 +165,13 @@ bool TrialToken::ValidateDate(const base::Time& now) const {
   return expiry_time_ > now;
 }
 
-bool TrialToken::ValidateSignature(base::StringPiece public_key) const {
-  return ValidateSignature(signature_, data_, public_key);
-}
-
 // static
-bool TrialToken::ValidateSignature(const std::string& signature_text,
+bool TrialToken::ValidateSignature(base::StringPiece signature,
                                    const std::string& data,
                                    base::StringPiece public_key) {
   // Public key must be 32 bytes long for Ed25519.
   CHECK_EQ(public_key.length(), 32UL);
 
-  std::string signature;
-  // signature_text is base64-encoded; decode first.
-  if (!base::Base64Decode(signature_text, &signature)) {
-    return false;
-  }
-
   // Signature must be 64 bytes long
   if (signature.length() != 64) {
     return false;
@@ -143,16 +184,10 @@ bool TrialToken::ValidateSignature(const std::string& signature_text,
   return (result != 0);
 }
 
-TrialToken::TrialToken(uint8_t version,
-                       const std::string& signature,
-                       const std::string& data,
-                       const url::Origin& origin,
+TrialToken::TrialToken(const url::Origin& origin,
                        const std::string& feature_name,
                        uint64_t expiry_timestamp)
-    : version_(version),
-      signature_(signature),
-      data_(data),
-      origin_(origin),
+    : origin_(origin),
       feature_name_(feature_name),
       expiry_time_(base::Time::FromDoubleT(expiry_timestamp)) {}