Remove the "not_after" validation of policy timestamps

Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection on the client side
against policy rollbacks.

However, the decision now is made that this validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
Review-Url: https://codereview.chromium.org/2820063005
Cr-Commit-Position: refs/heads/master@{#465964}
Committed: https://chromium.googlesource.com/chromium/src/+/5a159859f7164e629c68d8212db34a711fc5245d

[emaxx, 2017-04-18 19:46:39]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 19:47:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-04-18 20:09:04]

Description was changed from

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading the policy
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending out wrong and too
big timestamps. This could be a theoretical problem, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection against rollbacks.

However, the decision now is made that such validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

to

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending out wrong and too
big timestamps. This could be a theoretical problem, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection against rollbacks.

However, the decision now is made that such validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

[emaxx, 2017-04-18 20:09:42]

Description was changed from

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending out wrong and too
big timestamps. This could be a theoretical problem, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection against rollbacks.

However, the decision now is made that such validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

to

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection against rollbacks.

However, the decision now is made that such validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

[emaxx, 2017-04-18 20:10:01]

Description was changed from

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection against rollbacks.

However, the decision now is made that such validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

to

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection on the client side
against policy rollbacks.

However, the decision now is made that such validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

[emaxx, 2017-04-18 20:10:14]

Description was changed from

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection on the client side
against policy rollbacks.

However, the decision now is made that such validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

to

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection on the client side
against policy rollbacks.

However, the decision now is made that this validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

[commit-bot: I haz the power, 2017-04-18 20:15:18]

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

[commit-bot: I haz the power, 2017-04-18 20:15:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emaxx, 2017-04-18 20:17:31]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 20:18:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 21:04:19]

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

[commit-bot: I haz the power, 2017-04-18 21:04:20]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-04-18 21:05:29]

emaxx@chromium.org changed reviewers:
+ tnagel@chromium.org

[emaxx, 2017-04-18 21:05:30]

Thiemo, please take a look.

[Thiemo Nagel, 2017-04-19 11:43:09]

Lgtm with nits.

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

https://codereview.chromium.org/2820063005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:203:
CloudPolicyValidatorBase::TIMESTAMP_VALIDATED);
Nit: IIUC it would be clearer to set TIMESTAMP_NOT_VALIDATED.

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
(right):

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:48:
base::TimeDelta::FromMilliseconds(PolicyBuilder::kFakeTimestamp)),
Nit: Using FromJsTime(...::kFakeTimestamp) would allow dropping the UnixEpoch()
summand.

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:180:
base::Time timestamp(timestamp_ + base::TimeDelta::FromMinutes(5));
Nit: Suggest to use FromHours(3) so that the test would fail against the old
version of the code.

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:182:
(timestamp - base::Time::UnixEpoch()).InMilliseconds());
Nit: Using timestamp.ToJavaTime() would seem cleaner to me.

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:216:
(timestamp - base::Time::UnixEpoch()).InMilliseconds());
Same here: Using timestamp.ToJavaTime() would seem cleaner to me.

[emaxx, 2017-04-19 20:48:30]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[emaxx, 2017-04-19 20:48:46]

Thiemo, PTAL.

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

https://codereview.chromium.org/2820063005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:203:
CloudPolicyValidatorBase::TIMESTAMP_VALIDATED);
On 2017/04/19 11:43:09, Thiemo Nagel wrote:
> Nit: IIUC it would be clearer to set TIMESTAMP_NOT_VALIDATED.

Current code still validates something: it checks that the timestamp is present.
So I'd better leave this in place.

But I updated that the docs for ValidateTimestamp as they were misleading by not
mentioning the check of the _presence_ of the timestamp field.

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
(right):

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:48:
base::TimeDelta::FromMilliseconds(PolicyBuilder::kFakeTimestamp)),
On 2017/04/19 11:43:09, Thiemo Nagel wrote:
> Nit: Using FromJsTime(...::kFakeTimestamp) would allow dropping the
UnixEpoch()
> summand.

Thanks for pointing at the already existing primitive for doing this kind of
conversion. But I don't like it a little bit as they are called "JsTime" or
"JavaTime", which has no semantic connection with the policy protocol (note that
the similar conversions are performed in a number of policy-related source
files).

I actually had a different idea: to introduce a special pair of functions
somewhere under policy/core/common/ that would perform the conversion between
policy timestamps and base::Time. WDYT? (Of course, then it will be done in a
separate CL.)

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:180:
base::Time timestamp(timestamp_ + base::TimeDelta::FromMinutes(5));
On 2017/04/19 11:43:09, Thiemo Nagel wrote:
> Nit: Suggest to use FromHours(3) so that the test would fail against the old
> version of the code.

Done.

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:182:
(timestamp - base::Time::UnixEpoch()).InMilliseconds());
On 2017/04/19 11:43:09, Thiemo Nagel wrote:
> Nit: Using timestamp.ToJavaTime() would seem cleaner to me.

Same as above: do you agree with deferring this for a follow-up CL that would
introduce special helpers under the policy/ source tree?

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:216:
(timestamp - base::Time::UnixEpoch()).InMilliseconds());
On 2017/04/19 11:43:09, Thiemo Nagel wrote:
> Same here: Using timestamp.ToJavaTime() would seem cleaner to me.

Acknowledged.

[commit-bot: I haz the power, 2017-04-19 20:49:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 20:53:49]

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

[commit-bot: I haz the power, 2017-04-19 20:53:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[emaxx, 2017-04-20 09:11:20]

The CQ bit was checked by emaxx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 09:12:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 10:03:06]

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

[commit-bot: I haz the power, 2017-04-20 10:03:07]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-04-20 10:08:27]

Still lgtm.

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator_unittest.cc
(right):

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:48:
base::TimeDelta::FromMilliseconds(PolicyBuilder::kFakeTimestamp)),
> Thanks for pointing at the already existing primitive for doing this kind of
> conversion. But I don't like it a little bit as they are called "JsTime" or
> "JavaTime", which has no semantic connection with the policy protocol (note
that
> the similar conversions are performed in a number of policy-related source
> files).
> 
> I actually had a different idea: to introduce a special pair of functions
> somewhere under policy/core/common/ that would perform the conversion between
> policy timestamps and base::Time. WDYT? (Of course, then it will be done in a
> separate CL.)

Imho that would be unnecessary overhead.  The server is Java code and by virtue
of that is using Java timestamps thus using Java conversion functions on the
client to me seems exactly the right way to do it.

How about documenting the use of Java time more explicitly in the proto file,
e.g. instead of

"[timestamp] is milliseconds since Epoch in UTC timezone."

maybe write something like

"[timestamp] is milliseconds since Epoch in UTC timezone (Java time)."

?

https://codereview.chromium.org/2820063005/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator_unittest.cc:182:
(timestamp - base::Time::UnixEpoch()).InMilliseconds());
> Same as above: do you agree with deferring this for a follow-up CL 

Sure!

[emaxx, 2017-04-20 10:20:35]

The CQ bit was checked by emaxx@chromium.org

[commit-bot: I haz the power, 2017-04-20 10:20:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 10:25:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492683635607320,
"parent_rev": "d589ff736dbbaf58bfb5352468dab4fb8b3b33d7", "commit_rev":
"5a159859f7164e629c68d8212db34a711fc5245d"}

[commit-bot: I haz the power, 2017-04-20 10:26:03]

Description was changed from

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection on the client side
against policy rollbacks.

However, the decision now is made that this validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045
==========

to

==========
Remove the "not_after" validation of policy timestamps

This CL disables the validation of policy timestamps against the current
device clocks, which was previously there to prevent loading policies
"from the future".

The disabled validation was originally intended as a protection against
a potential bug on DMServer side if it started sending wrongly big
timestamps. This could be a problem in theory, as after such a bug
occurs on the server side and is fixed then, the clients will refuse the
subsequent policy updates due to the protection on the client side
against policy rollbacks.

However, the decision now is made that this validation brings more
drawbacks than benefits as the device's clocks are quite often wrong.

BUG=701045

Review-Url: https://codereview.chromium.org/2820063005
Cr-Commit-Position: refs/heads/master@{#465964}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a159859f7164e629c68d8212db3...
==========

[commit-bot: I haz the power, 2017-04-20 10:26:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5a159859f7164e629c68d8212db3...

[emaxx, 2017-04-20 10:29:08]

On 2017/04/20 10:08:27, Thiemo Nagel wrote:
> Imho that would be unnecessary overhead.  The server is Java code and by
virtue
> of that is using Java timestamps thus using Java conversion functions on the
> client to me seems exactly the right way to do it.
> 
> How about documenting the use of Java time more explicitly in the proto file,
> e.g. instead of
> 
> "[timestamp] is milliseconds since Epoch in UTC timezone."
> 
> maybe write something like
> 
> "[timestamp] is milliseconds since Epoch in UTC timezone (Java time)."
> 
> ?

Thanks, sounds reasonable. Will do in a follow-up CL.