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..ac4b5847b0ad972deba2dbb3cab93199fb63b2f7 100644
--- a/content/common/origin_trials/trial_token.cc
+++ b/content/common/origin_trials/trial_token.cc
@@ -9,12 +9,14 @@
 #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/time/time.h"
+#include "base/values.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -25,69 +27,121 @@ namespace {
 // Version 1 is the only token version currently supported
 const uint8_t kVersion1 = 1;
 
-const char* kFieldSeparator = "|";
+// Version is a 1-byte field
+const size_t kVersionBytesLength = 1;
+
+// Version 1 field sizes and offsets
+const size_t kSignatureBytesLength = 512 >> 3;  // get from curve25519 instead?
+
+const size_t kVersionOffset = 0;

[chasej, 2016/04/07 15:52:30]

Nit: The version offset is independent of version. Defining it here suggests
that it is specific to Version 1. I think it would help to make that more
obvious.

I know the header file links to the design doc with all the structure details.
Here, I still think it would help if the code/definitions showed that structure.

[iclelland, 2016/04/07 20:26:43]

Done.

+const size_t kSignatureOffset = kVersionOffset + kVersionBytesLength;
+const size_t kPayloadLengthOffset = kSignatureOffset + kSignatureBytesLength;
+const size_t kPayloadOffset = kPayloadLengthOffset + 4;

[chasej, 2016/04/07 15:52:30]

Should/add use a constant for kPayloadLengthBytesLength? Name is kind of gross,
but you get the idea. Maybe could refer to Payload Size instead?

[iclelland, 2016/04/07 20:26:43]

Yeah, if we have a named constant for 0, then why not one for 4? (I debated
including thinks like ASSERT(sizeof(uint32_t)==4), but that's also dumb.)

I'll change all field size constants to be named "<field>Size" for consistency

 
 }  // namespace
 
 TrialToken::~TrialToken() {}
 
-scoped_ptr<TrialToken> TrialToken::Parse(const std::string& token_text) {
+// Static
+scoped_ptr<TrialToken> TrialToken::From(const std::string& token_text,
+                                        base::StringPiece public_key) {
+  scoped_ptr<std::string> token_json = Extract(token_text, public_key);
+  if (!token_json) {
+    return nullptr;
+  }
+  return Parse(*token_json);
+}
+
+scoped_ptr<std::string> TrialToken::Extract(const std::string& token_text,
+                                            base::StringPiece public_key) {
   if (token_text.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_text, &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) {
+  // Token must be large enough to contain a version, signature, and payload
+  // length.
+  if (token_contents.length() < kPayloadOffset) {

[chasej, 2016/04/07 15:52:30]

Nit, but this is really a version-specific check, before the version is
known/validated. I think it would be cleaner to have the version check first.
All version-specific logic is together, and it's a single point to branch if
multiple versions are supported.

[iclelland, 2016/04/07 20:26:43]

Good catch. Done.

     return nullptr;
   }
 
-  // Only version 1 currently supported
+  // Only version 1 currently supported.
+  uint8_t version = token_contents[0];

[chasej, 2016/04/07 15:52:30]

Should use the kVersionOffset constant here, not hard-coded zero.

[iclelland, 2016/04/07 20:26:43]

Done.

   if (version != kVersion1) {
     return nullptr;
   }
 
+  // Extract the length of the signed data (Big-endian).
+  uint32_t payload_length =
+      ((uint8_t)token_contents[kPayloadLengthOffset] << 24) +
+      ((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
-  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) {
+  std::string version_string =
+      token_contents.substr(kVersionOffset, kVersionBytesLength);

[chasej, 2016/04/07 15:52:30]

Nit: Again, this part is not version-specific.

[iclelland, 2016/04/07 20:26:43]

It kind of is, in that we only *need* the version bytes as a std::string in
order to perform the V1 signature-validation. The version-agnostic code is just
the checks above, I think.

+  std::string signature =
+      token_contents.substr(kSignatureOffset, kSignatureBytesLength);
+  std::string payload_length_string =
+      token_contents.substr(kPayloadLengthOffset, 4);
+  std::string payload = token_contents.substr(kPayloadOffset, payload_length);
+
+  // The data which is covered by the signature is (version + length + payload)
+  std::string signed_data = version_string + payload_length_string + payload;

[chasej, 2016/04/07 15:52:30]

Could/should the substrings use StringPiece to avoid allocations? (I'm not an
expert, but this was suggested in a previous iteration).

If using StringPiece, is it necessary to extract the payload length and contents
separately? It seems like it could just extract one substring to concatenate
with the version string. e.g:
std:string data = token_contents.substr(kPayloadLengthOffset, payload_length +
kPayloadLengthBytesLength);

I know the payload is needed separately as a return value. Could substring then,
if it makes that far.

[iclelland, 2016/04/07 20:26:43]

Done; we change them back to strings afterwards for the concatenation, though
(except for the signature part), so I'm not sure how much we gain.
Done. (Of course, we could have done that with std::strings as well)
Done.

+
+  // Validate the signature on the data before proceeding
+  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(payload));
+}
 
-  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
+  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);
+  // Ensure that the feature name is a valid string
+  if (feature_name.empty()) {
+    return nullptr;
+  }
+
+  // Ensure that the expiry timestamp is a valid (positive) integer
+  if (expiry_timestamp <= 0) {
+    return nullptr;
+  }
 
-  return make_scoped_ptr(new TrialToken(version, signature, data, origin,
-                                        feature_name, expiry_timestamp));
+  return make_scoped_ptr(
+      new TrialToken(origin, feature_name, expiry_timestamp));
 }
 
 bool TrialToken::IsAppropriate(const url::Origin& origin,
@@ -95,11 +149,8 @@ bool TrialToken::IsAppropriate(const url::Origin& origin,
   return ValidateOrigin(origin) && ValidateFeatureName(feature_name);
 }
 
-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);
+bool TrialToken::IsValid(const base::Time& now) const {
+  return ValidateDate(now);
 }
 
 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(const std::string& 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)) {}