Don't disable extensions immediately if verification is out of date

Don't disable extensions immediately if verification is out of date

If the install verification signature we have stored in our prefs is
older than the install time of an extension, we don't want to
immediately disable it. Instead, we want to do another server request to
update our signature, and then disable if appropriate.

BUG=342165
R=finnur@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250855

[asargent_no_longer_on_chrome, 2014-02-12 08:31:45]

This is to fix the bug we discussed at the meeting this morning (or last
evening, depending on your perspective).

[Finnur, 2014-02-12 13:54:56]

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:623:
extensions::ExtensionSet::const_iterator i;
nit: Like you, I have a tendency to move large declarations of variables out of
the for loop for readability reasons but I've gotten pushback on that from
reviewers because then you are changing the scope of things, in this case |i|,
to live outside the scope of the for loop. I don't have a strong opinion, I'll
leave it up to you.

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:627:
extension_prefs_->GetInstallTime(extension.id()) >= timestamp) {
You seem to have thought about this problem elsewhere in the code, but just to
be sure: Is this code block robust enough to handle when the system clock
changes manually? For example, if I set my system clock backward and/or
forwards, will I end up in a state where I always/never bootstrap, depending on
what time I've set? (always being better than never, I suppose).

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:634: VerifyAllExtensions(true); 
// bootstrap=true.
Refresh my memory... Bootstrap means: Fetch the signatures again and verify, not
just fetch signatures and verify on next boot, right?

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/install_signer.cc (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/install_signer.cc:456: result->timestamp =
base::Time::Now();
The timestamp you want is when you sent the request, not when you received a
reply, right? What happens if an extension gets installed in between sending and
receiving a reply?

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/install_verifier.cc (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/install_verifier.cc:397: return true;
Am I understanding it correctly that this is just for backwards compatibility
and will not be needed at some point in the future (when all have started
recording the signature timestamp).

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/install_verifier.h:57: base::Time
signature_timestamp();
Shouldn't this technically be SignatureTimestamp()?

[asargent_no_longer_on_chrome, 2014-02-12 16:27:55]

Thanks for quick review! PTAL

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:623:
extensions::ExtensionSet::const_iterator i;
On 2014/02/12 13:54:57, Finnur wrote:
> nit: Like you, I have a tendency to move large declarations of variables out
of
> the for loop for readability reasons but I've gotten pushback on that from
> reviewers because then you are changing the scope of things, in this case |i|,
> to live outside the scope of the for loop. I don't have a strong opinion, I'll
> leave it up to you.
Ah, that's actually a good argument - fixed it.

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:627:
extension_prefs_->GetInstallTime(extension.id()) >= timestamp) {
On 2014/02/12 13:54:57, Finnur wrote:
> You seem to have thought about this problem elsewhere in the code, but just to
> be sure: Is this code block robust enough to handle when the system clock
> changes manually? For example, if I set my system clock backward and/or
> forwards, will I end up in a state where I always/never bootstrap, depending
on
> what time I've set? (always being better than never, I suppose).

We have code elsewhere that forces a request for a new signature whenever new
extensions are installed/uninstalled, so you're right in your comment elsewhere
that this code just makes sure during the rollout period that we don't
accidentally disable things for users who have an outdated signature as
discussed in the bug and at the meeting. Eventually we can move to assuming
there's always a valid signature and get rid of this.

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:634: VerifyAllExtensions(true); 
// bootstrap=true.
On 2014/02/12 13:54:57, Finnur wrote:
> Refresh my memory... Bootstrap means: Fetch the signatures again and verify,
not
> just fetch signatures and verify on next boot, right?

Right - inside the callback function  FinishVerifyAllExtensions, if we
successfully got a result back from the server, down at line 683 we proceed to
call CheckManagementPolicy which, because the signature's timestamp will now be
updated, should properly enforce the results we got back from the server and
this time *not* skip any extensions based on their install time inside
InstallVerifier::MustRemainDisabled.

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/install_signer.cc (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/install_signer.cc:456: result->timestamp =
base::Time::Now();
On 2014/02/12 13:54:57, Finnur wrote:
> The timestamp you want is when you sent the request, not when you received a
> reply, right? What happens if an extension gets installed in between sending
and
> receiving a reply?

This is technically correct about the timestamp, and I've fixed it to avoid any
future problems. 

(It turns out not to matter in the current code since the act of installing an
extension will trigger another signature request to be queued up).

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/install_verifier.cc (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/install_verifier.cc:397: return true;
On 2014/02/12 13:54:57, Finnur wrote:
> Am I understanding it correctly that this is just for backwards compatibility
> and will not be needed at some point in the future (when all have started
> recording the signature timestamp).

Yep - at some point we should be able to get rid of this code, when we're always
keeping the signature up to date and we remove the code used for incremental
rollout.

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/install_verifier.h (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/install_verifier.h:57: base::Time
signature_timestamp();
On 2014/02/12 13:54:57, Finnur wrote:
> Shouldn't this technically be SignatureTimestamp()?

Yes, good catch. Fixed.

[Finnur, 2014-02-12 16:56:25]

LGTM, one minor nit and a comment.

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/160313002/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:634: VerifyAllExtensions(true); 
// bootstrap=true.
Good. I was worried that someone could force Chrome to be always in bootstrap
mode and thereby avoiding the enforcement. Seems like you got that covered.

https://codereview.chromium.org/160313002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/install_signer.h (right):

https://codereview.chromium.org/160313002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/install_signer.h:114: base::Time request_start_time_; 
// Not set until the request starts.
nit: Probably would prefer to have this not lumped in with the other members
here (add linebreak).

[asargent_no_longer_on_chrome, 2014-02-12 18:32:26]

https://codereview.chromium.org/160313002/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/install_signer.h (right):

https://codereview.chromium.org/160313002/diff/90001/chrome/browser/extension...
chrome/browser/extensions/install_signer.h:114: base::Time request_start_time_; 
// Not set until the request starts.
On 2014/02/12 16:56:26, Finnur wrote:
> nit: Probably would prefer to have this not lumped in with the other members
> here (add linebreak).

Done.

https://codereview.chromium.org/160313002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/160313002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:630: install_time <
base::Time::Now() && install_time >= timestamp) {
Note: I added the same "ignore times in the future" check here to avoid
extraneous server requests.

[asargent_no_longer_on_chrome, 2014-02-12 18:32:56]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2014-02-12 18:36:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/160313002/160001

[commit-bot: I haz the power, 2014-02-12 20:14:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 20:14:07]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[asargent_no_longer_on_chrome, 2014-02-13 00:02:32]

Committed patchset #3 manually as r250855 (presubmit successful).