Store Origin Trials public key in browser prefs

Store Origin Trials public key in browser prefs

It was pointed out during code review of another CL that the component updater
will not read the manifest for an updated Origin Trials public key in time to
set the command line for the renderer process. This CL solves the issue by
having the component updater store the new key in browser local state. The new
key will then not be used on the current browser run, but will take effect the
next time the browser is started.

The new preference is added as "origin_trials.public_key". Other origin trials
policy configuration will be added within the "origin_trials" namespace.

The command-line flag can still be used to temporarily override the preference
for a single browser run.

BUG=589830, 603588
Committed: https://crrev.com/27f4bcb37e897164fe60b13d7026057a1ee75003
Cr-Commit-Position: refs/heads/master@{#401304}

[iclelland, 2016-06-07 13:40:16]

Description was changed from

==========
Store Origin Trials public key in browser prefs

It was pointed out during code review of another CL that the component updater
will not read the manifest for an updated Origin Trials public key in time to
set the command line for the renderer process. This CL solves the issue by
having the component updater store the new key in browser local state. The new
key will then not be used on the current browser run, but will take effect the
next time the browser is started.

BUG=589830,603588
==========

to

==========
Store Origin Trials public key in browser prefs

It was pointed out during code review of another CL that the component updater
will not read the manifest for an updated Origin Trials public key in time to
set the command line for the renderer process. This CL solves the issue by
having the component updater store the new key in browser local state. The new
key will then not be used on the current browser run, but will take effect the
next time the browser is started.

The new preference is added as "origin_trials.public_key". Other origin trials
policy configuration will be added within the "origin_trials" namespace.

The command-line flag can still be used to temporarily override the preference
for a single browser run.

BUG=589830,603588
==========

[iclelland, 2016-06-07 13:58:11]

iclelland@chromium.org changed reviewers:
+ chasej@chromium.org, waffles@chromium.org, wfh@chromium.org

[iclelland, 2016-06-07 13:58:12]

I've split the introduction of preference logic off of the original CL
(https://codereview.chromium.org/1741783002/) to make the changes easier to
reason about.

I believe that this CL solves the issue of the CRX component data not getting to
the renderers quickly enough; I've rebased the other CL off of this one, and
extended the logic to handle the other features there.

chasej, PTAL for origin trials
waffles, PTAL for component code
wfh, PTAL at the persistence mechanism through prefs

I'll add any other needed OWNERS once I have confirmation that this approach
isn't completely insane :)

Thanks!

[chasej, 2016-06-07 15:09:40]

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/origin_trials_component_installer.cc:76:
local_state->Set("origin-trials.public-key",
Can this use the prefs::kOriginTrialPublicKey constant defined in
"chrome/common/pref_names.h"?

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:315: RegisterOriginTrialPrefs(registry);
All other registration methods seemed to be contained in specific
namespace/class (with one exception). That seems like a reasonable practice that
should be followed for origin trials as well.

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/origin...
File chrome/browser/prefs/origin_trial_prefs.cc (right):

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/origin...
chrome/browser/prefs/origin_trial_prefs.cc:13:
"fMS4mpO6buLQ/QMd+zJmxzty/VQ6B1EUZqoCU04zoRU=");
I assume this value is the base64-encoded version of the default public key in
OriginTrialKeyManager (kDefaultPublicKey)? 

If so, why do we need to store two copies of the same value? I think it would be
better to just register an empty string as the default value here. Besides
avoiding a copy, it also means that the default public key is defined in
OriginTrialKeyManager, where it is most likely to be looked for/found.

https://codereview.chromium.org/2047683002/diff/1/chrome/common/origin_trials...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/2047683002/diff/1/chrome/common/origin_trials...
chrome/common/origin_trials/origin_trial_key_manager.h:25: static void
RegisterPrefs(PrefRegistrySimple* registry);
I don't see a definition for this function. As per my comment in
browser_prefs.cc, this seems like the function I would expect called there.
Instead, the function provided in origin_trial_prefs.h/cc is used.

One of origin_trial_prefs.* or this method needs to be cleaned up, depending on
the resolution to my comment in browser_prefs.cc.

[iclelland, 2016-06-08 02:48:22]

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/component_up...
File chrome/browser/component_updater/origin_trials_component_installer.cc
(right):

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/component_up...
chrome/browser/component_updater/origin_trials_component_installer.cc:76:
local_state->Set("origin-trials.public-key",
On 2016/06/07 15:09:40, chasej wrote:
> Can this use the prefs::kOriginTrialPublicKey constant defined in
> "chrome/common/pref_names.h"?

It absolutely can, and should. Thanks. Done.

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:315: RegisterOriginTrialPrefs(registry);
On 2016/06/07 15:09:40, chasej wrote:
> All other registration methods seemed to be contained in specific
> namespace/class (with one exception). That seems like a reasonable practice
that
> should be followed for origin trials as well.

Done.

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/origin...
File chrome/browser/prefs/origin_trial_prefs.cc (right):

https://codereview.chromium.org/2047683002/diff/1/chrome/browser/prefs/origin...
chrome/browser/prefs/origin_trial_prefs.cc:13:
"fMS4mpO6buLQ/QMd+zJmxzty/VQ6B1EUZqoCU04zoRU=");
On 2016/06/07 15:09:40, chasej wrote:
> I assume this value is the base64-encoded version of the default public key in
> OriginTrialKeyManager (kDefaultPublicKey)? 
> 
> If so, why do we need to store two copies of the same value? I think it would
be
> better to just register an empty string as the default value here. Besides
> avoiding a copy, it also means that the default public key is defined in
> OriginTrialKeyManager, where it is most likely to be looked for/found.

Yes, one copy is much better. Done.

https://codereview.chromium.org/2047683002/diff/1/chrome/common/origin_trials...
File chrome/common/origin_trials/origin_trial_key_manager.h (right):

https://codereview.chromium.org/2047683002/diff/1/chrome/common/origin_trials...
chrome/common/origin_trials/origin_trial_key_manager.h:25: static void
RegisterPrefs(PrefRegistrySimple* registry);
On 2016/06/07 15:09:40, chasej wrote:
> I don't see a definition for this function. As per my comment in
> browser_prefs.cc, this seems like the function I would expect called there.
> Instead, the function provided in origin_trial_prefs.h/cc is used.
> 
> One of origin_trial_prefs.* or this method needs to be cleaned up, depending
on
> the resolution to my comment in browser_prefs.cc.

This unused declaration shouldn't have made it to code review :) The method was
originally here, but chrome/common isn't allowed to depend on
chrome/components/prefs. That's why the method was moved into chrome/browser.

Declaration removed.

[Will Harris, 2016-06-08 02:57:20]

Sorry one dumb question and one hopefully slightly less dumb question

1. can you explain what the origin trial public key is, and why it's needed
before renderer start.

2. does it matter if the public key gets updated while the browser is running,
does this have to be handled correctly?

[iclelland, 2016-06-08 13:48:47]

On 2016/06/08 02:57:20, Will Harris wrote:
> Sorry one dumb question and one hopefully slightly less dumb question
> 
> 1. can you explain what the origin trial public key is, and why it's needed
> before renderer start.

The key is used to validate the signature on origin trial tokens, to ensure that
they are valid.

We need the renderer to be aware of the key as early as possible, since trial
tokens can be encountered early: During http header response at the earliest
right now. We use the key to validate the tokens as we are setting up the V8
context, before scripts are run and before we even begin parsing the body.

Communicating this from the browser to the renderer via the command line seemed
the best way to ensure that it would always be available.

> 
> 2. does it matter if the public key gets updated while the browser is running,
> does this have to be handled correctly?

I don't think so; we're not considering this a "must abort everything and
reevaluate the page" kind of situation. Any running renderer, for instance,
wouldn't be affected no matter what happens to the browser policy, once it
starts. I'm pretty sure it's fine for it to take effect the next time the
browser restarts.

The situation I'm trying to avoid here is one where a new key is downloaded, but
when the browser starts up, the first renderer doesn't get it, and uses the
built-in default key. Then the second renderer gets the new key, and we have
inconsistent behaviour between two running renderers. This could repeat every
time the user restarts the browser, where no matter how out-of-date the default
key is, it's often used anyway.

[chasej, 2016-06-08 15:19:54]

lgtm

[Will Harris, 2016-06-08 21:35:27]

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
File chrome/browser/prefs/origin_trial_prefs.cc (right):

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
chrome/browser/prefs/origin_trial_prefs.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
chrome/browser/prefs/origin_trial_prefs.cc:12:
registry->RegisterStringPref(prefs::kOriginTrialPublicKey, "");
Is it okay for the default key to be "" - this will be the key used for first
run and until the component updater has run for the first time (normally a few
minutes into startup).

Do you consider hard coding a default key, or maybe that is done somewhere else?

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
File chrome/browser/prefs/origin_trial_prefs.h (right):

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
chrome/browser/prefs/origin_trial_prefs.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: no (c)

[iclelland, 2016-06-09 03:06:39]

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
File chrome/browser/prefs/origin_trial_prefs.cc (right):

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
chrome/browser/prefs/origin_trial_prefs.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/06/08 21:35:27, Will Harris wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
chrome/browser/prefs/origin_trial_prefs.cc:12:
registry->RegisterStringPref(prefs::kOriginTrialPublicKey, "");
On 2016/06/08 21:35:27, Will Harris wrote:
> Is it okay for the default key to be "" - this will be the key used for first
> run and until the component updater has run for the first time (normally a few
> minutes into startup).
> 
> Do you consider hard coding a default key, or maybe that is done somewhere
else?

It won't hurt -- there is a default hard-coded key (see
chrome/common/origin_trials/origin_trial_key_manager.cc), and attempting to set
the key to an empty string will just fail.

It's not great, though; I'd rather not deliberately call it with an invalid
value. I've changed it to test whether the value is an empty string, and if so,
not to set any command-line flag for it.

(Incidentally, the first patch in this CL had a default key here, but we
realized that it was unnecessarily duplicating the value)

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
File chrome/browser/prefs/origin_trial_prefs.h (right):

https://codereview.chromium.org/2047683002/diff/20001/chrome/browser/prefs/or...
chrome/browser/prefs/origin_trial_prefs.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/06/08 21:35:27, Will Harris wrote:
> nit: no (c)

Fixed, thanks. (That should teach me not to just steal the header from whatever
other file I have open)

[waffles, 2016-06-09 15:20:04]

lgtm

[Will Harris, 2016-06-09 16:13:23]

lgtm

[iclelland, 2016-06-09 16:21:06]

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

[iclelland, 2016-06-09 16:21:07]

Thanks, everyone!

jochen, can you PTAL? (There are two other followup CLs to this one as well)

[jochen (gone - plz use gerrit), 2016-06-13 15:28:31]

can you also rebase that one?

[iclelland, 2016-06-15 09:45:06]

Rebased this onto https://codereview.chromium.org/2049783002/

[iclelland, 2016-06-22 15:01:53]

On 2016/06/15 09:45:06, iclelland wrote:
> Rebased this onto https://codereview.chromium.org/2049783002/

Rebased again, after changes to parent issue. jochen, can you PTAL again?
Thanks!

[jochen (gone - plz use gerrit), 2016-06-22 15:03:22]

lgtm

[iclelland, 2016-06-22 16:21:57]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2016-06-22 16:21:58]

The patchset sent to the CQ was uploaded after l-g-t-m from chasej@chromium.org,
waffles@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2047683002/#ps80001
(title: "Rebase; update SetupOriginTrials method name")

[commit-bot: I haz the power, 2016-06-22 16:22:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047683002/80001

[commit-bot: I haz the power, 2016-06-22 16:41:19]

Description was changed from

==========
Store Origin Trials public key in browser prefs

It was pointed out during code review of another CL that the component updater
will not read the manifest for an updated Origin Trials public key in time to
set the command line for the renderer process. This CL solves the issue by
having the component updater store the new key in browser local state. The new
key will then not be used on the current browser run, but will take effect the
next time the browser is started.

The new preference is added as "origin_trials.public_key". Other origin trials
policy configuration will be added within the "origin_trials" namespace.

The command-line flag can still be used to temporarily override the preference
for a single browser run.

BUG=589830,603588
==========

to

==========
Store Origin Trials public key in browser prefs

It was pointed out during code review of another CL that the component updater
will not read the manifest for an updated Origin Trials public key in time to
set the command line for the renderer process. This CL solves the issue by
having the component updater store the new key in browser local state. The new
key will then not be used on the current browser run, but will take effect the
next time the browser is started.

The new preference is added as "origin_trials.public_key". Other origin trials
policy configuration will be added within the "origin_trials" namespace.

The command-line flag can still be used to temporarily override the preference
for a single browser run.

BUG=589830,603588
==========

[commit-bot: I haz the power, 2016-06-22 16:41:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-22 16:43:04]

Description was changed from

==========
Store Origin Trials public key in browser prefs

It was pointed out during code review of another CL that the component updater
will not read the manifest for an updated Origin Trials public key in time to
set the command line for the renderer process. This CL solves the issue by
having the component updater store the new key in browser local state. The new
key will then not be used on the current browser run, but will take effect the
next time the browser is started.

The new preference is added as "origin_trials.public_key". Other origin trials
policy configuration will be added within the "origin_trials" namespace.

The command-line flag can still be used to temporarily override the preference
for a single browser run.

BUG=589830,603588
==========

to

==========
Store Origin Trials public key in browser prefs

It was pointed out during code review of another CL that the component updater
will not read the manifest for an updated Origin Trials public key in time to
set the command line for the renderer process. This CL solves the issue by
having the component updater store the new key in browser local state. The new
key will then not be used on the current browser run, but will take effect the
next time the browser is started.

The new preference is added as "origin_trials.public_key". Other origin trials
policy configuration will be added within the "origin_trials" namespace.

The command-line flag can still be used to temporarily override the preference
for a single browser run.

BUG=589830,603588

Committed: https://crrev.com/27f4bcb37e897164fe60b13d7026057a1ee75003
Cr-Commit-Position: refs/heads/master@{#401304}
==========

[commit-bot: I haz the power, 2016-06-22 16:43:05]

Patchset 5 (id:??) landed as
https://crrev.com/27f4bcb37e897164fe60b13d7026057a1ee75003
Cr-Commit-Position: refs/heads/master@{#401304}