Change origin trial token format

Change origin trial token format

This CL introduces a new format for origin trial tokens. The tokens are JSON-encoded data, wrapped in a base64-encoded signed binary structure.

The token format is documented at https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M

Additionally, this CL changes the order of operations when processing tokens. Now signature verification is always performed as part of parsing, and the token's applicability can be checked after it has been verified and parsed.

BUG=601090
Committed: https://crrev.com/0709f4ee3881c0c97c7e765ba824b20d5464771f
Cr-Commit-Position: refs/heads/master@{#387333}

[iclelland, 2016-04-06 17:31:06]

Description was changed from

==========
Change origin trial token format

BUG=
==========

to

==========
Change origin trial token format

This CL introduces a new format for origin trial tokens. The tokens are
JSON-encoded data, wrapped in a base64-encoded signed binary structure.

The token format is documented at
https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M

Additionally, this CL changes the order of operations when processing tokens.
Now signature verification is always performed as part of parsing, and the
token's applicability can be checked after it has been verified and parsed.

BUG=601090
==========

[iclelland, 2016-04-06 17:31:06]

iclelland@chromium.org changed reviewers:
+ chase@chromium.org, estark@chromium.org, palmer@chromium.org

[iclelland, 2016-04-06 20:49:51]

iclelland@chromium.org changed reviewers:
+ chasej@chromium.org
- chase@chromium.org

[palmer, 2016-04-07 00:03:53]

Might have more comments later; zooming to a meeting.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
File content/common/origin_trials/trial_token.h (right):

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.h:39: static scoped_ptr<TrialToken>
From(const std::string& token_text,
Nit: The |From|, |Extract|, |Parse| suite of functions form an interface that is
not obvious to me.  (For example, clearly I must call them in a certain order
and hook up outputs to inputs in some way, but what is it exactly?)

My instinct is to specify the interface as just a constructor, like this:

  TrialToken(vector<uint8_t> token_bytes, vector<uint8_t> public_key_bytes);

and if the |token_bytes| or |public_key_bytes| are invalid in any way, the
constructed |TrialToken| is invalid or empty or whatever.

Does that not suffice for some reason that isn't apparent to me yet? (I haven't
read any of the rest of the code, so.)

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.h:54: bool IsAppropriate(const
url::Origin& origin,
Nit: Similarly, I'd declare simply:

  bool IsValidForFeature(const url::Origin& origin,
                         base::StringPiece feature_name,
                         const base::Time& now) const;

[iclelland, 2016-04-07 14:55:13]

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
File content/common/origin_trials/trial_token.h (right):

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.h:39: static scoped_ptr<TrialToken>
From(const std::string& token_text,
On 2016/04/07 00:03:53, palmer wrote:
> Nit: The |From|, |Extract|, |Parse| suite of functions form an interface that
is
> not obvious to me.  (For example, clearly I must call them in a certain order
> and hook up outputs to inputs in some way, but what is it exactly?)
> 
> My instinct is to specify the interface as just a constructor, like this:
> 
>   TrialToken(vector<uint8_t> token_bytes, vector<uint8_t> public_key_bytes);
> 
> and if the |token_bytes| or |public_key_bytes| are invalid in any way, the
> constructed |TrialToken| is invalid or empty or whatever.
> 
> Does that not suffice for some reason that isn't apparent to me yet? (I
haven't
> read any of the rest of the code, so.)

From is intended to be the entry point -- it could really just be a constructor,
but it seemed like a more common idiom in Chrome to name conversion functions
"Create" or "From", and have the actual constructor, which just initializes data
members, be private. I'm fine either way.

Extract and Parse represent the two distinct phases of getting the token data
out of the string -- one unwraps the binary, checks the signature, and returns
the JSON payload; the other deserializes the JSON and pulls the important pieces
out of it. They are separated for ease of testing; there's no real need for them
to be a public API.

Removed those two from the public API.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.h:54: bool IsAppropriate(const
url::Origin& origin,
On 2016/04/07 00:03:53, palmer wrote:
> Nit: Similarly, I'd declare simply:
> 
>   bool IsValidForFeature(const url::Origin& origin,
>                          base::StringPiece feature_name,
>                          const base::Time& now) const;

Done.

[chasej, 2016-04-07 15:52:31]

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:36: const size_t kVersionOffset = 0;
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.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:39: const size_t kPayloadOffset =
kPayloadLengthOffset + 4;
Should/add use a constant for kPayloadLengthBytesLength? Name is kind of gross,
but you get the idea. Maybe could refer to Payload Size instead?

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:69: if (token_contents.length() <
kPayloadOffset) {
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.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:74: uint8_t version =
token_contents[0];
Should use the kVersionOffset constant here, not hard-coded zero.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:93:
token_contents.substr(kVersionOffset, kVersionBytesLength);
Nit: Again, this part is not version-specific.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:101: std::string signed_data =
version_string + payload_length_string + payload;
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.

https://codereview.chromium.org/1858763003/diff/40001/tools/origin_trials/gen...
File tools/origin_trials/generate_token.py (right):

https://codereview.chromium.org/1858763003/diff/40001/tools/origin_trials/gen...
tools/origin_trials/generate_token.py:6: """Utility for generating experimental
API tokens
Nit: There are a couple usages of "API" instead of "feature" in this file.
Ideally, this would be consistent throughout.

https://codereview.chromium.org/1858763003/diff/40001/tools/origin_trials/gen...
tools/origin_trials/generate_token.py:82: def GenerateTokenData(origin,
api_name, expiry):
Ditto for "API" vs "feature".

https://codereview.chromium.org/1858763003/diff/40001/tools/origin_trials/gen...
tools/origin_trials/generate_token.py:95: struct.pack(">I",len(data)) + data)
Nit: Could pull out the logic to pack the length + data into a separate
function, e.g. GeneratePayload(). I had to look twice before I realized that
GenerateTokenData() and FormatToken() weren't exactly the same. I think that
would make the differences between the functions clearer (in addition to the DRY
micro-improvement).

https://codereview.chromium.org/1858763003/diff/40001/tools/origin_trials/gen...
tools/origin_trials/generate_token.py:99: description="Generate tokens for
enabling experimental APIs")
Ditto for "API" vs "feature".

https://codereview.chromium.org/1858763003/diff/40001/tools/origin_trials/gen...
tools/origin_trials/generate_token.py:101: help="Origin for which to enable the
API. This can be "
Ditto for "API" vs "feature".

https://codereview.chromium.org/1858763003/diff/40001/tools/origin_trials/gen...
tools/origin_trials/generate_token.py:138: signature = Sign(private_key,
data_to_sign)
Nit: Add a comment linking to the design doc somewhere here/in this file. I
think that would help explain what the code is doing.

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:17: #include
"base/strings/utf_string_conversions.h"
Which of these string headers are still needed? Now, with parsing as binary
and/or typed values retrieved via json, it seems many headers could be removed.

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:55: scoped_ptr<std::string>
TrialToken::Extract(const std::string& token_payload,
Move to match new declaration order.

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:112: scoped_ptr<TrialToken>
TrialToken::Parse(const std::string& token_json) {
Move to match new declaration order.

[iclelland, 2016-04-07 20:26:45]

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:36: const size_t kVersionOffset = 0;
On 2016/04/07 15:52:30, chasej wrote:
> 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.

Done.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:39: const size_t kPayloadOffset =
kPayloadLengthOffset + 4;
On 2016/04/07 15:52:30, chasej wrote:
> Should/add use a constant for kPayloadLengthBytesLength? Name is kind of
gross,
> but you get the idea. Maybe could refer to Payload Size instead?

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

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:69: if (token_contents.length() <
kPayloadOffset) {
On 2016/04/07 15:52:30, chasej wrote:
> 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.

Good catch. Done.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:74: uint8_t version =
token_contents[0];
On 2016/04/07 15:52:30, chasej wrote:
> Should use the kVersionOffset constant here, not hard-coded zero.

Done.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:93:
token_contents.substr(kVersionOffset, kVersionBytesLength);
On 2016/04/07 15:52:30, chasej wrote:
> Nit: Again, this part is not version-specific.

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.

https://codereview.chromium.org/1858763003/diff/40001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:101: std::string signed_data =
version_string + payload_length_string + payload;
On 2016/04/07 15:52:30, chasej wrote:
> Could/should the substrings use StringPiece to avoid allocations? (I'm not an
> expert, but this was suggested in a previous iteration).

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.

> 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);

Done. (Of course, we could have done that with std::strings as well)

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

Done.

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:17: #include
"base/strings/utf_string_conversions.h"
On 2016/04/07 15:52:30, chasej wrote:
> Which of these string headers are still needed? Now, with parsing as binary
> and/or typed values retrieved via json, it seems many headers could be
removed.

Good catch -- none of them!
Done.

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:55: scoped_ptr<std::string>
TrialToken::Extract(const std::string& token_payload,
On 2016/04/07 15:52:30, chasej wrote:
> Move to match new declaration order.

Done.

https://codereview.chromium.org/1858763003/diff/60001/content/common/origin_t...
content/common/origin_trials/trial_token.cc:112: scoped_ptr<TrialToken>
TrialToken::Parse(const std::string& token_json) {
On 2016/04/07 15:52:30, chasej wrote:
> Move to match new declaration order.

Done.

[chasej, 2016-04-08 03:23:08]

lgtm

[iclelland, 2016-04-08 15:22:48]

estark - Does this CL address the concerns you had with the previous string
parsing code?

[estark, 2016-04-08 17:23:37]

On 2016/04/08 15:22:48, iclelland wrote:
> estark - Does this CL address the concerns you had with the previous string
> parsing code?

Yes, this lgtm, thanks. One question though, have you generated any tokens
already with the previous format? Would it be safer to increment the version
number?

[iclelland, 2016-04-08 17:30:46]

On 2016/04/08 17:23:37, estark wrote:
> On 2016/04/08 15:22:48, iclelland wrote:
> > estark - Does this CL address the concerns you had with the previous string
> > parsing code?
> 
> Yes, this lgtm, thanks. One question though, have you generated any tokens
> already with the previous format? Would it be safer to increment the version
> number?

We have generated tokens, though not for any features which have launched yet. I
had been considering bumping the version, even though there is no possibility of
misinterpretation (ASCII vs base64-encoded binary). I'll change it to 2 so as to
avoid any confusion for people.

[iclelland, 2016-04-08 20:50:38]

iclelland@chromium.org changed reviewers:
+ avi@chromium.org, mek@chromium.org

[iclelland, 2016-04-08 20:51:17]

palmer -- ping -- did you manage to take a closer look?

+r avi for content/
+r mek for tools/

Thanks!

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

tools looks good, but have several comments about the rest of the code

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:28: // Version 2 is the only token
version currently supported. Version 1 was
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.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:88: // Extract the length of the
signed data (Big-endian).
Can you use something like base::BigEndianReader to do all this parsing? That
seems safer than implementing your own parsing and length checks...

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:90:
((uint8_t)token_contents[kPayloadLengthOffset] << 24) +
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).

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:104: base::StringPiece
payload_piece(token_bytes + kPayloadLengthOffset,
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.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
File content/common/origin_trials/trial_token.h (right):

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.h:38: // correctly formatted and
signed, and can be parsed.)
nit: Maybe make it explicit in the comment that "signed" is related to the
|public_key| passed in?

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.h:39: static scoped_ptr<TrialToken>
From(const std::string& token_text,
All these scoped_ptr's should be std::unique_ptr

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.h:55: // Returns the payload of a
signed token, or nullptr if the token is not
nit: I think it's more typical to return a bool for valid/invalid and have a
std::string* output parameter to pass the actual string, or something like that
to avoid the unnecessary heap allocations. Or base::optional, but that hasn't
landed yet. Maybe just using unique_ptr here is fine after all, it does match
base::Value related methods after all...

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.h:57: static scoped_ptr<std::string>
Extract(const std::string& token_text,
nit: "Extract" doesn't really seem like a name that makes it clear that this
method extracts data, and also does signature validation

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
File content/common/origin_trials/trial_token_unittest.cc (right):

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token_unittest.cc:196: /* Test verification
of signature and extraction of token JSON from signed token
// is much more common to use for comments. And the style guide says be
consistent, so just use // here and below

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token_unittest.cc:249: scoped_ptr<TrialToken>
empty_token = Parse(kInvalidTokens[i]);
nit: not really part of this CL, but it would be nice to use the gtest
functionality for parameterized tests rather than manually looping over the
various test cases
(https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...)

[Marijn Kruisselbrink, 2016-04-08 21:20:09]

On 2016/04/08 at 21:19:30, Marijn Kruisselbrink wrote:
> tools looks good, but have several comments about the rest of the code

oh, and it seems there is some bug somewhere, since those layout test failures
look real

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

This looks reasonable, and with the other reviewer's comments addressed, and
with properly delimited sentences, lgtm.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:24: // Version is a 1-byte field at
offset 0
Make comments full sentences and end them with a full stop/period.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:33: // Version 2 field sizes and
offsets
.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:100: // Extract the version-specific
contents of the token
.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:107: // The data which is covered by
the signature is (version + length + payload)
.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:111: // Validate the signature on
the data before proceeding
.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:122: // signed_data is JSON-encoded
.

etc

[iclelland, 2016-04-11 16:59:38]

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:24: // Version is a 1-byte field at
offset 0
On 2016/04/08 21:34:24, Avi wrote:
> Make comments full sentences and end them with a full stop/period.

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:28: // Version 2 is the only token
version currently supported. Version 1 was
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> 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.

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:33: // Version 2 field sizes and
offsets
On 2016/04/08 21:34:24, Avi wrote:
> .

Done. And turned into a full sentence.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:88: // Extract the length of the
signed data (Big-endian).
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> Can you use something like base::BigEndianReader to do all this parsing? That
> seems safer than implementing your own parsing and length checks...

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.)

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:90:
((uint8_t)token_contents[kPayloadLengthOffset] << 24) +
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> 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).

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

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:100: // Extract the version-specific
contents of the token
On 2016/04/08 21:34:24, Avi wrote:
> .

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:104: base::StringPiece
payload_piece(token_bytes + kPayloadLengthOffset,
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> 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.

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

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:107: // The data which is covered by
the signature is (version + length + payload)
On 2016/04/08 21:34:24, Avi wrote:
> .

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:111: // Validate the signature on
the data before proceeding
On 2016/04/08 21:34:23, Avi wrote:
> .

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.cc:122: // signed_data is JSON-encoded
On 2016/04/08 21:34:24, Avi wrote:
> .
> 
> etc

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
File content/common/origin_trials/trial_token.h (right):

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.h:38: // correctly formatted and
signed, and can be parsed.)
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> nit: Maybe make it explicit in the comment that "signed" is related to the
> |public_key| passed in?

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.h:39: static scoped_ptr<TrialToken>
From(const std::string& token_text,
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> All these scoped_ptr's should be std::unique_ptr

Yes, I've been following those CLs over the weekend. Thanks for the reminder,
though.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token.h:57: static scoped_ptr<std::string>
Extract(const std::string& token_text,
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> nit: "Extract" doesn't really seem like a name that makes it clear that this
> method extracts data, and also does signature validation

I'm open to any other suggestions; It's only an internal method, so even
something as long as ValidateSignatureAndExtractPayload() would work, if it's
clearer.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
File content/common/origin_trials/trial_token_unittest.cc (right):

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token_unittest.cc:196: /* Test verification
of signature and extraction of token JSON from signed token
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> // is much more common to use for comments. And the style guide says be
> consistent, so just use // here and below

Done.

https://codereview.chromium.org/1858763003/diff/100001/content/common/origin_...
content/common/origin_trials/trial_token_unittest.cc:249: scoped_ptr<TrialToken>
empty_token = Parse(kInvalidTokens[i]);
On 2016/04/08 21:19:30, Marijn Kruisselbrink wrote:
> nit: not really part of this CL, but it would be nice to use the gtest
> functionality for parameterized tests rather than manually looping over the
> various test cases
>
(https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...)

Thanks for that pointer -- done.

[palmer, 2016-04-11 20:25:58]

https://codereview.chromium.org/1858763003/diff/160001/content/common/origin_...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/160001/content/common/origin_...
content/common/origin_trials/trial_token.cc:121: std::unique_ptr<TrialToken>
TrialToken::Parse(const std::string& token_json) {
What you call |token_json| here is called |token_payload| in the .h file.
Probably better to use |token_json| in both places.

https://codereview.chromium.org/1858763003/diff/160001/content/common/origin_...
content/common/origin_trials/trial_token.cc:183: reinterpret_cast<const
uint8_t*>(public_key.data()));
These casts indicate to me that the StringPiece and string interfaces might not
be quite right. The caller should do these conversions, if necessary. (And, they
may not need to — perhaps they had to convert the uint8_t*s they had into the
wrapper types.)

[iclelland, 2016-04-12 04:09:32]

https://codereview.chromium.org/1858763003/diff/160001/content/common/origin_...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/160001/content/common/origin_...
content/common/origin_trials/trial_token.cc:121: std::unique_ptr<TrialToken>
TrialToken::Parse(const std::string& token_json) {
On 2016/04/11 20:25:57, palmer wrote:
> What you call |token_json| here is called |token_payload| in the .h file.
> Probably better to use |token_json| in both places.

Done.

https://codereview.chromium.org/1858763003/diff/160001/content/common/origin_...
content/common/origin_trials/trial_token.cc:183: reinterpret_cast<const
uint8_t*>(public_key.data()));
On 2016/04/11 20:25:57, palmer wrote:
> These casts indicate to me that the StringPiece and string interfaces might
not
> be quite right. The caller should do these conversions, if necessary. (And,
they
> may not need to — perhaps they had to convert the uint8_t*s they had into the
> wrapper types.)

That's a really good point... I recall that this signature came out of
discussions with davidben, and we figured that StringPiece made more sense than
the uint8_t*s that I was passing around, if only because the strings encapsulate
their length with the data.

This predates the proposal to allow the use of std::vector::data() though, which
may be a better fit conceptually. I could switch these to std::vector<uint8_t>,
but updating the public keys everywhere will require updating much more code
than what's in this CL already. Are you okay with that being in a subsequent CL?

[palmer, 2016-04-12 17:48:31]

> This predates the proposal to allow the use of std::vector::data() though,
which
> may be a better fit conceptually. I could switch these to
std::vector<uint8_t>,
> but updating the public keys everywhere will require updating much more code
> than what's in this CL already. Are you okay with that being in a subsequent
CL?

Sure, that's fine. LGTM.

[iclelland, 2016-04-13 17:18:21]

On 2016/04/08 21:20:09, Marijn Kruisselbrink wrote:
> On 2016/04/08 at 21:19:30, Marijn Kruisselbrink wrote:
> > tools looks good, but have several comments about the rest of the code
> 
> oh, and it seems there is some bug somewhere, since those layout test failures
> look real

The layout test failures were happening because I had changed the version number
in the tokens, but hadn't regenerated the tokens embedded in the test html.

The rest of the issues should be resolved, I think -- can you take a look?

[Marijn Kruisselbrink, 2016-04-13 23:07:38]

Sorry for the delayed response, lgtm with some minor nits.

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token.cc:46: // Static
nit: you use lowercase "static" in comments below, so probably keep this
lowercase too

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token.cc:122: // signed_data is JSON-encoded.
nit: signed_data should probably be token_json? Although since that already has
json in the name the comment probably doesn't add much.

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
File content/common/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token_validator.cc:15: base::StringPiece
featureName) {
nit (and unrelated to this CL): featureName should be feature_name in chromium
style

[iclelland, 2016-04-14 16:15:22]

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
File content/common/origin_trials/trial_token.cc (right):

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token.cc:46: // Static
On 2016/04/13 23:07:37, Marijn Kruisselbrink wrote:
> nit: you use lowercase "static" in comments below, so probably keep this
> lowercase too

Done.

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token.cc:122: // signed_data is JSON-encoded.
On 2016/04/13 23:07:37, Marijn Kruisselbrink wrote:
> nit: signed_data should probably be token_json? Although since that already
has
> json in the name the comment probably doesn't add much.

Thanks, missed that. Removed redundant comment.

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
File content/common/origin_trials/trial_token_validator.cc (right):

https://codereview.chromium.org/1858763003/diff/180001/content/common/origin_...
content/common/origin_trials/trial_token_validator.cc:15: base::StringPiece
featureName) {
On 2016/04/13 23:07:37, Marijn Kruisselbrink wrote:
> nit (and unrelated to this CL): featureName should be feature_name in chromium
> style

Totally should. It must have snuck in from the blink-side WebTrialTokenValidator
somehow. Fixed.

[iclelland, 2016-04-14 16:15:34]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-04-14 16:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org,
estark@chromium.org, avi@chromium.org, palmer@chromium.org, mek@chromium.org
Link to the patchset: https://codereview.chromium.org/1858763003/#ps200001
(title: "Nits")

[commit-bot: I haz the power, 2016-04-14 16:15:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1858763003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1858763003/200001

[commit-bot: I haz the power, 2016-04-14 16:21:30]

Description was changed from

==========
Change origin trial token format

This CL introduces a new format for origin trial tokens. The tokens are
JSON-encoded data, wrapped in a base64-encoded signed binary structure.

The token format is documented at
https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M

Additionally, this CL changes the order of operations when processing tokens.
Now signature verification is always performed as part of parsing, and the
token's applicability can be checked after it has been verified and parsed.

BUG=601090
==========

to

==========
Change origin trial token format

This CL introduces a new format for origin trial tokens. The tokens are
JSON-encoded data, wrapped in a base64-encoded signed binary structure.

The token format is documented at
https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M

Additionally, this CL changes the order of operations when processing tokens.
Now signature verification is always performed as part of parsing, and the
token's applicability can be checked after it has been verified and parsed.

BUG=601090
==========

[commit-bot: I haz the power, 2016-04-14 16:21:31]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-04-14 16:22:53]

Description was changed from

==========
Change origin trial token format

This CL introduces a new format for origin trial tokens. The tokens are
JSON-encoded data, wrapped in a base64-encoded signed binary structure.

The token format is documented at
https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M

Additionally, this CL changes the order of operations when processing tokens.
Now signature verification is always performed as part of parsing, and the
token's applicability can be checked after it has been verified and parsed.

BUG=601090
==========

to

==========
Change origin trial token format

This CL introduces a new format for origin trial tokens. The tokens are
JSON-encoded data, wrapped in a base64-encoded signed binary structure.

The token format is documented at
https://docs.google.com/document/d/1v5fi0EUV_QHckVHVF2K4P72iNywnrJtNhNZ6i2NPt0M

Additionally, this CL changes the order of operations when processing tokens.
Now signature verification is always performed as part of parsing, and the
token's applicability can be checked after it has been verified and parsed.

BUG=601090

Committed: https://crrev.com/0709f4ee3881c0c97c7e765ba824b20d5464771f
Cr-Commit-Position: refs/heads/master@{#387333}
==========

[commit-bot: I haz the power, 2016-04-14 16:22:54]

Patchset 11 (id:??) landed as
https://crrev.com/0709f4ee3881c0c97c7e765ba824b20d5464771f
Cr-Commit-Position: refs/heads/master@{#387333}