Update server-backed state key generation.

Update server-backed state key generation.

Include the disk serial number as a factor that is neither easily predictable nor known to the server side as well as a quantized time stamp, so the identifiers become invalid after a while.

BUG=chromium:352599
TEST=Forced re-enrollment still works.
R=joaodasilva@chromium.org, stevenjb@chromium.org, xiyuan@chromium.org

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

[Mattias Nissler (ping if slow), 2014-03-26 21:02:01]

Please review. Note that this depends on
https://chromium-review.googlesource.com/#/c/191730/

[stevenjb, 2014-03-26 22:27:44]

owner lgtm

[Joao da Silva, 2014-03-26 23:26:31]

lgtm

[Joao da Silva, 2014-03-26 23:28:23]

WizardControllerFlowTest.ControlFlowForcedReEnrollment is timing out.

[Mattias Nissler (ping if slow), 2014-03-27 15:09:07]

Joao, PTAL. I've added the time-based aspect to the CL and also included a unit
test.

[Joao da Silva, 2014-03-27 15:45:39]

The code lgtm bar simple nits, but I'm not familiar with the design behind the
formulas used. Has it been reviewed too?

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:535: virtual void
SetUpInProcessBrowserTestFixture() {
OVERRIDE

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:546: virtual void
TearDownInProcessBrowserTestFixture() {
OVERRIDE

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:103:
DeviceCloudPolicyManagerChromeOS::kDeviceStateKeyFutureQuanta;
IIRC this is not necessary for static const ints (they're always inlined by the
compiler)

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:412:
(timestamp - base::Time::UnixEpoch()).InSeconds() & ~(quantum_size - 1);
So quantum_size is 97 days, and quantized_time is always the same value for
every 97-day block. Is that right?

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:419:
quantized_time += quantum_size;
... and this is producing the hashes that will be generated for the next 5 * 97
days, is that right?

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:57: //
device becomes anonymous to the server again.
If I understood the implementation correctly then it will take about 5*97 days
== 40 months until the device becomes anonymous to the server again. Is that
what's intended?

Can you expand this comment to describe how this interval can be derived from
these 2 values?

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:272:
&statistics_provider_);
Remove this on TearDown?

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:318:
}
Suggestion: verify that a small time increase within the quantum size produces
the same values

[Mattias Nissler (ping if slow), 2014-03-27 16:12:41]

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller_browsertest.cc (right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:535: virtual void
SetUpInProcessBrowserTestFixture() {
On 2014/03/27 15:45:40, Joao da Silva wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller_browsertest.cc:546: virtual void
TearDownInProcessBrowserTestFixture() {
On 2014/03/27 15:45:40, Joao da Silva wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:103:
DeviceCloudPolicyManagerChromeOS::kDeviceStateKeyFutureQuanta;
On 2014/03/27 15:45:40, Joao da Silva wrote:
> IIRC this is not necessary for static const ints (they're always inlined by
the
> compiler)

I got link errors and checked the net, which pointed me at the spec: A
definition is required in the general case, and the value should be omitted.

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:412:
(timestamp - base::Time::UnixEpoch()).InSeconds() & ~(quantum_size - 1);
On 2014/03/27 15:45:40, Joao da Silva wrote:
> So quantum_size is 97 days, and quantized_time is always the same value for
> every 97-day block. Is that right?

Correct.

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:419:
quantized_time += quantum_size;
On 2014/03/27 15:45:40, Joao da Silva wrote:
> ... and this is producing the hashes that will be generated for the next 5 *
97
> days, is that right?

Correct.

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.h:57: //
device becomes anonymous to the server again.
On 2014/03/27 15:45:40, Joao da Silva wrote:
> If I understood the implementation correctly then it will take about 5*97 days
> == 40 months until the device becomes anonymous to the server again. Is that
> what's intended?

5 * 97 is closer to 16 months :) The goal was one year, so that's 4 quanta. Then
we add one to make up for the fact that the current quantum might be almost
over, yielding the magic 5 number here.

> 
> Can you expand this comment to describe how this interval can be derived from
> these 2 values?

Updated the comment.

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:272:
&statistics_provider_);
On 2014/03/27 15:45:40, Joao da Silva wrote:
> Remove this on TearDown?

I debated it as it's not strictly needed (i.e. test runner code shouldn't call
into StatisticsProvider), so I decided to leave it out for brevity.

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:318:
}
On 2014/03/27 15:45:40, Joao da Silva wrote:
> Suggestion: verify that a small time increase within the quantum size produces
> the same values

Good idea, done.

[Mattias Nissler (ping if slow), 2014-03-27 16:13:08]

On 2014/03/27 15:45:39, Joao da Silva wrote:
> The code lgtm bar simple nits, but I'm not familiar with the design behind the
> formulas used. Has it been reviewed too?

Yup, this is the result of my consultations with privacy and security folks.

[Mattias Nissler (ping if slow), 2014-03-27 16:16:46]

Nikita, can I get your OWNERS stamp for the test infrastructure update in
chrome/browser/chromeos/login/wizard_controller_browsertest.cc ?

[Joao da Silva, 2014-03-27 16:29:47]

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:272:
&statistics_provider_);
On 2014/03/27 16:12:41, Mattias Nissler wrote:
> On 2014/03/27 15:45:40, Joao da Silva wrote:
> > Remove this on TearDown?
> 
> I debated it as it's not strictly needed (i.e. test runner code shouldn't call
> into StatisticsProvider), so I decided to leave it out for brevity.

Note that this mock object will remain as the static global testing
StatisticsProvider, so subsequent unit tests may crash if they hit code that
reaches for the StatisticsProvider. I really think it's a good idea to reset it
to NULL.

[Mattias Nissler (ping if slow), 2014-03-27 17:04:08]

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/212653004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:272:
&statistics_provider_);
On 2014/03/27 16:29:48, Joao da Silva wrote:
> On 2014/03/27 16:12:41, Mattias Nissler wrote:
> > On 2014/03/27 15:45:40, Joao da Silva wrote:
> > > Remove this on TearDown?
> > 
> > I debated it as it's not strictly needed (i.e. test runner code shouldn't
call
> > into StatisticsProvider), so I decided to leave it out for brevity.
> 
> Note that this mock object will remain as the static global testing
> StatisticsProvider, so subsequent unit tests may crash if they hit code that
> reaches for the StatisticsProvider. I really think it's a good idea to reset
it
> to NULL.

Uh, good point. Fixed. Globals suck.

[xiyuan, 2014-03-28 21:11:55]

lgtm

[Mattias Nissler (ping if slow), 2014-03-28 23:03:13]

The CQ bit was checked by mnissler@chromium.org

[commit-bot: I haz the power, 2014-03-28 23:03:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/212653004/100001

[Mattias Nissler (ping if slow), 2014-03-28 23:36:57]

Committed patchset #6 manually as r260301 (presubmit successful).