Move trial token code to content/common.

Move trial token code to content/common.

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient, to
enable verification of trial tokens from the browser process.

BUG=590873
Committed: https://crrev.com/67f486d991d883a5b3438f8ca820860e9979170d
Cr-Commit-Position: refs/heads/master@{#380823}

[Marijn Kruisselbrink, 2016-02-27 07:26:17]

Description was changed from

==========
WIP move trial token to content/common, and cleanup code

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

to

==========
WIP move trial token to content/common, and cleanup code

StringPiece should be passed by value rather than const ref.
Feature names should be passed as StringPiece rather than std::string.
Origin might as well be represented/passed as url::Origin rather than
GURL in some places and std::string in other.
Storing expiry time as base::Time seems more sensible than storing it as int64.
C-style casts are against the style guide. And in this case the entire cast was
unnecessary.

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

[Marijn Kruisselbrink, 2016-02-27 07:26:58]

Description was changed from

==========
WIP move trial token to content/common, and cleanup code

StringPiece should be passed by value rather than const ref.
Feature names should be passed as StringPiece rather than std::string.
Origin might as well be represented/passed as url::Origin rather than
GURL in some places and std::string in other.
Storing expiry time as base::Time seems more sensible than storing it as int64.
C-style casts are against the style guide. And in this case the entire cast was
unnecessary.

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

to

==========
WIP move trial token to content/common, and cleanup code

StringPiece should be passed by value rather than const ref.
Feature names should be passed as StringPiece rather than std::string.
Origin might as well be represented/passed as url::Origin rather than
GURL in some places and std::string in other.
Storing expiry time as base::Time seems more sensible than storing it as int64.
C-style casts are against the style guide. And in this case the entire cast was
unnecessary.
Fix order of methods in origin_token.cc to match that in origin_token.h

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

[Marijn Kruisselbrink, 2016-02-27 07:27:18]

Description was changed from

==========
WIP move trial token to content/common, and cleanup code

StringPiece should be passed by value rather than const ref.
Feature names should be passed as StringPiece rather than std::string.
Origin might as well be represented/passed as url::Origin rather than
GURL in some places and std::string in other.
Storing expiry time as base::Time seems more sensible than storing it as int64.
C-style casts are against the style guide. And in this case the entire cast was
unnecessary.
Fix order of methods in origin_token.cc to match that in origin_token.h

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

to

==========
WIP move trial token to content/common, and cleanup code

StringPiece should be passed by value rather than const ref.
Feature names should be passed as StringPiece rather than std::string.
Origin might as well be represented/passed as url::Origin rather than
GURL in some places and std::string in other.
Storing expiry time as base::Time seems more sensible than storing it as int64.
C-style casts are against the style guide. And in this case the entire cast was
unnecessary.
Fix order of methods in trial_token.cc to match that in trial_token.h

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

[Marijn Kruisselbrink, 2016-02-29 21:50:32]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2016-02-29 21:50:42]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2016-02-29 21:51:40]

Description was changed from

==========
WIP move trial token to content/common, and cleanup code

StringPiece should be passed by value rather than const ref.
Feature names should be passed as StringPiece rather than std::string.
Origin might as well be represented/passed as url::Origin rather than
GURL in some places and std::string in other.
Storing expiry time as base::Time seems more sensible than storing it as int64.
C-style casts are against the style guide. And in this case the entire cast was
unnecessary.
Fix order of methods in trial_token.cc to match that in trial_token.h

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

to

==========
WIP move trial token to content/common

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient.

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

[Marijn Kruisselbrink, 2016-02-29 22:06:08]

Description was changed from

==========
WIP move trial token to content/common

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient.

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

to

==========
Move trial token code to content/common.

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient.

BUG=590873
==========

[Marijn Kruisselbrink, 2016-02-29 22:06:24]

Description was changed from

==========
Move trial token code to content/common.

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient.

BUG=590873
==========

to

==========
Move trial token code to content/common.

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient, to
enable verification of trial tokens from the browser process.

BUG=590873
==========

[Marijn Kruisselbrink, 2016-02-29 22:07:08]

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

[Marijn Kruisselbrink, 2016-03-04 00:27:44]

Updated this CL to include the changes I described in
https://codereview.chromium.org/1752463002 (move token validation logic to a new
content/common/ TrialTokenValidator namespace, and rename existing
content::TrialTokenValidator to content::WebTrialTokenValidatorImpl)

[iclelland, 2016-03-07 16:40:23]

This move lgtm now; sorry for the lag.

https://codereview.chromium.org/1742053002/diff/60001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.h (right):

https://codereview.chromium.org/1742053002/diff/60001/chrome/common/chrome_co...
chrome/common/chrome_content_client.h:93: OriginTrialKeyManager
origin_trial_key_manager_;
This is the only data member on this object -- besides the other clients
pointers that are part of the ContentClient base.

As long as it's okay to add this here, I think we'll just need to be careful
about thread-safety of the GetOriginTrialPublicKey method. It's fine right now,
but I need to remember to keep it so, if we make the key handling more complex.

[Marijn Kruisselbrink, 2016-03-08 20:32:56]

https://codereview.chromium.org/1742053002/diff/60001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.h (right):

https://codereview.chromium.org/1742053002/diff/60001/chrome/common/chrome_co...
chrome/common/chrome_content_client.h:93: OriginTrialKeyManager
origin_trial_key_manager_;
On 2016/03/07 at 16:40:23, iclelland wrote:
> This is the only data member on this object -- besides the other clients
pointers that are part of the ContentClient base.

Yes, that's true. I don't think this should be problematic, but if it is we can
always remove the data member until there actually is a need to keep state (and
at that point figure out how that state should be shared between renderer and
browser processes).

> As long as it's okay to add this here, I think we'll just need to be careful
about thread-safety of the GetOriginTrialPublicKey method. It's fine right now,
but I need to remember to keep it so, if we make the key handling more complex.

I'm not sure how that is relevant to this CL though? All the thread-safety
issues already existed in the old code. Anyway, I added a comment to
origin_trial_key_manager.h to document the (pre-existing) threading
requirements.

[iclelland, 2016-03-08 20:47:21]

https://codereview.chromium.org/1742053002/diff/60001/chrome/common/chrome_co...
File chrome/common/chrome_content_client.h (right):

https://codereview.chromium.org/1742053002/diff/60001/chrome/common/chrome_co...
chrome/common/chrome_content_client.h:93: OriginTrialKeyManager
origin_trial_key_manager_;
On 2016/03/08 20:32:56, Marijn Kruisselbrink wrote:
> On 2016/03/07 at 16:40:23, iclelland wrote:
> > This is the only data member on this object -- besides the other clients
> pointers that are part of the ContentClient base.
> 
> Yes, that's true. I don't think this should be problematic, but if it is we
can
> always remove the data member until there actually is a need to keep state
(and
> at that point figure out how that state should be shared between renderer and
> browser processes).
> 
> > As long as it's okay to add this here, I think we'll just need to be careful
> about thread-safety of the GetOriginTrialPublicKey method. It's fine right
now,
> but I need to remember to keep it so, if we make the key handling more
complex.
> 
> I'm not sure how that is relevant to this CL though? All the thread-safety
> issues already existed in the old code. Anyway, I added a comment to
> origin_trial_key_manager.h to document the (pre-existing) threading
> requirements.

Sorry -- not relevant here, you're right. Reading it just made me worried that
without such a comment, it would be easier now to accidentally break things. The
comment's perfect, thanks.

[Marijn Kruisselbrink, 2016-03-08 22:20:20]

mek@chromium.org changed reviewers:
+ jam@chromium.org

[Marijn Kruisselbrink, 2016-03-08 22:20:21]

+jam for content/ and chrome/ OWNERS

[Marijn Kruisselbrink, 2016-03-11 20:33:56]

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

[Marijn Kruisselbrink, 2016-03-11 20:33:57]

jam seems to be unavailable, so
+avi for content/ OWNERS
+thakis for chrome/ OWNERS

[Avi (use Gerrit), 2016-03-11 21:38:47]

This looks like a pretty straightforward code move; lgtm

[Nico, 2016-03-11 21:49:06]

chrome lgtm

This is only dealing with trusted data that's fine to run outside the sandbox,
right?

https://codereview.chromium.org/1742053002/diff/100001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1742053002/diff/100001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.h:12: class
OriginTrialKeyManager {
(why is this an instantiated class instead of just a function?)

[Marijn Kruisselbrink, 2016-03-11 21:56:36]

On 2016/03/11 at 21:49:06, thakis wrote:
> This is only dealing with trusted data that's fine to run outside the sandbox,
right?

The tokens being parsed/validated aren't exactly trusted data. They are provided
by websites. So I imagine the question is if it is safe to run ED25519_verify
using a trusted key, but untrusted data/signature outside of the sandbox (the
rest of the code is all pretty straightforward). I would hope that is the case,
but I can see how there might be some concerns about that. This CL doesn't
actually run any code in the browser process yet at least, but in a future CL I
hope to be able to verify these signatures without having to bounce through some
other process.

https://codereview.chromium.org/1742053002/diff/100001/chrome/common/origin_t...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/1742053002/diff/100001/chrome/common/origin_t...
chrome/common/origin_trials/origin_trial_key_manager.h:12: class
OriginTrialKeyManager {
On 2016/03/11 at 21:49:05, Nico wrote:
> (why is this an instantiated class instead of just a function?)

I'm not sure. I think iclelland@ had some future plans to do things like caching
signature verification results, if that turned out to be a bottleneck, so I
assume this is instantiated to be able to store that state.

[Nico, 2016-03-11 22:09:48]

On 2016/03/11 21:56:36, Marijn Kruisselbrink wrote:
> On 2016/03/11 at 21:49:06, thakis wrote:
> > This is only dealing with trusted data that's fine to run outside the
sandbox,
> right?
> 
> The tokens being parsed/validated aren't exactly trusted data. They are
provided
> by websites. So I imagine the question is if it is safe to run ED25519_verify
> using a trusted key, but untrusted data/signature outside of the sandbox (the
> rest of the code is all pretty straightforward). I would hope that is the
case,
> but I can see how there might be some concerns about that. This CL doesn't
> actually run any code in the browser process yet at least, but in a future CL
I
> hope to be able to verify these signatures without having to bounce through
some
> other process.

Hm, we generally don't allow attacker-controlled data in the browser process, so
you probably still want to use an utility process for this. (But if it's in a
different process type than renderer, you still need to move this code to common
for that)

[Marijn Kruisselbrink, 2016-03-11 22:45:59]

On 2016/03/11 at 22:09:48, thakis wrote:
> On 2016/03/11 21:56:36, Marijn Kruisselbrink wrote:
> > On 2016/03/11 at 21:49:06, thakis wrote:
> > > This is only dealing with trusted data that's fine to run outside the
sandbox,
> > right?
> > 
> > The tokens being parsed/validated aren't exactly trusted data. They are
provided
> > by websites. So I imagine the question is if it is safe to run
ED25519_verify
> > using a trusted key, but untrusted data/signature outside of the sandbox
(the
> > rest of the code is all pretty straightforward). I would hope that is the
case,
> > but I can see how there might be some concerns about that. This CL doesn't
> > actually run any code in the browser process yet at least, but in a future
CL I
> > hope to be able to verify these signatures without having to bounce through
some
> > other process.
> 
> Hm, we generally don't allow attacker-controlled data in the browser process,
so you probably still want to use an utility process for this. (But if it's in a
different process type than renderer, you still need to move this code to common
for that)

Hmm, good point... Although there definitely seems to be quite a bit of
crypto/hashing of untrusted data being done in the browser process today? And
ED25519_verify looks small/simple enough to be unlikely to contains bugs (and
all the untrusted data is used for is to calculate a SHA512 hash). So it seems
like it should be safe to do this with untrusted data in the browser process,
but I can see how there is always a trade-off to be made as to what is and isn't
okay to do with untrusted data outside of sandboxes.

[Marijn Kruisselbrink, 2016-03-12 00:34:35]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-03-12 00:34:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/1742053002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-12 00:34:57]

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

[Nico, 2016-03-12 00:43:17]

On 2016/03/11 22:45:59, Marijn Kruisselbrink wrote:
> On 2016/03/11 at 22:09:48, thakis wrote:
> > On 2016/03/11 21:56:36, Marijn Kruisselbrink wrote:
> > > On 2016/03/11 at 21:49:06, thakis wrote:
> > > > This is only dealing with trusted data that's fine to run outside the
> sandbox,
> > > right?
> > > 
> > > The tokens being parsed/validated aren't exactly trusted data. They are
> provided
> > > by websites. So I imagine the question is if it is safe to run
> ED25519_verify
> > > using a trusted key, but untrusted data/signature outside of the sandbox
> (the
> > > rest of the code is all pretty straightforward). I would hope that is the
> case,
> > > but I can see how there might be some concerns about that. This CL doesn't
> > > actually run any code in the browser process yet at least, but in a future
> CL I
> > > hope to be able to verify these signatures without having to bounce
through
> some
> > > other process.
> > 
> > Hm, we generally don't allow attacker-controlled data in the browser
process,
> so you probably still want to use an utility process for this. (But if it's in
a
> different process type than renderer, you still need to move this code to
common
> for that)
> 
> Hmm, good point... Although there definitely seems to be quite a bit of
> crypto/hashing of untrusted data being done in the browser process today? And

Where?

> ED25519_verify looks small/simple enough to be unlikely to contains bugs (and
> all the untrusted data is used for is to calculate a SHA512 hash). So it seems
> like it should be safe to do this with untrusted data in the browser process,
> but I can see how there is always a trade-off to be made as to what is and
isn't
> okay to do with untrusted data outside of sandboxes.

Definitely get an ok from security team if you want to pursue doing this in the
browser process. While ED25519_verify might look fine, not allowing untrusted
data in the browser process is part of our defense it depth approach.

[Marijn Kruisselbrink, 2016-03-12 01:06:09]

On 2016/03/12 at 00:43:17, thakis wrote:
> On 2016/03/11 22:45:59, Marijn Kruisselbrink wrote:
> > On 2016/03/11 at 22:09:48, thakis wrote:
> > > On 2016/03/11 21:56:36, Marijn Kruisselbrink wrote:
> > > > On 2016/03/11 at 21:49:06, thakis wrote:
> > > > > This is only dealing with trusted data that's fine to run outside the
> > sandbox,
> > > > right?
> > > > 
> > > > The tokens being parsed/validated aren't exactly trusted data. They are
> > provided
> > > > by websites. So I imagine the question is if it is safe to run
> > ED25519_verify
> > > > using a trusted key, but untrusted data/signature outside of the sandbox
> > (the
> > > > rest of the code is all pretty straightforward). I would hope that is
the
> > case,
> > > > but I can see how there might be some concerns about that. This CL
doesn't
> > > > actually run any code in the browser process yet at least, but in a
future
> > CL I
> > > > hope to be able to verify these signatures without having to bounce
through
> > some
> > > > other process.
> > > 
> > > Hm, we generally don't allow attacker-controlled data in the browser
process,
> > so you probably still want to use an utility process for this. (But if it's
in a
> > different process type than renderer, you still need to move this code to
common
> > for that)
> > 
> > Hmm, good point... Although there definitely seems to be quite a bit of
> > crypto/hashing of untrusted data being done in the browser process today?
And
> 
> Where?

Actually I'm not entirely sure, since it's often hard to tell what data is and
isn't trusted. But it seems that content/browser/download/base_file calculates
SHA256 hashes of untrusted data for one. And depending on if you consider
extensions trusted data lots of hashing and crypto is done in various places
there as well.
And unless I'm misunderstanding the process model, I thought most of the network
level SSL support is currently also done in the browser process (but that's of
course planned to change, exactly for this reason).


> > ED25519_verify looks small/simple enough to be unlikely to contains bugs
(and
> > all the untrusted data is used for is to calculate a SHA512 hash). So it
seems
> > like it should be safe to do this with untrusted data in the browser
process,
> > but I can see how there is always a trade-off to be made as to what is and
isn't
> > okay to do with untrusted data outside of sandboxes.
> 
> Definitely get an ok from security team if you want to pursue doing this in
the browser process. While ED25519_verify might look fine, not allowing
untrusted data in the browser process is part of our defense it depth approach.

Yes, will definitely make sure to get somebody from the security team to OK this
before actually landing code that calls this from the browser process. (and I
assume you meant "not allowing [certain operations on] untrusted data in the
browser process", since all untrusted data has to go through the browser process
at some point).

[commit-bot: I haz the power, 2016-03-12 01:34:40]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-03-12 01:36:56]

Description was changed from

==========
Move trial token code to content/common.

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient, to
enable verification of trial tokens from the browser process.

BUG=590873
==========

to

==========
Move trial token code to content/common.

Moves content/renderer/origin_trials/trial_token.* to content/common, and
moves ContentRendererClient::GetOriginTrialPublicKey to ContentClient, to
enable verification of trial tokens from the browser process.

BUG=590873

Committed: https://crrev.com/67f486d991d883a5b3438f8ca820860e9979170d
Cr-Commit-Position: refs/heads/master@{#380823}
==========

[commit-bot: I haz the power, 2016-03-12 01:36:57]

Patchset 4 (id:??) landed as
https://crrev.com/67f486d991d883a5b3438f8ca820860e9979170d
Cr-Commit-Position: refs/heads/master@{#380823}