Remove unnecessary plumbing for policy verification key

Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing it
around.

BUG=none

Committed: https://crrev.com/aea27953b5a2898d62c9114a442dc33a962a98f1
Cr-Commit-Position: refs/heads/master@{#434652}

[Thiemo Nagel, 2016-11-17 10:11:47]

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

[commit-bot: I haz the power, 2016-11-17 10:12:09]

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

[Thiemo Nagel, 2016-11-17 10:12:21]

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

[Thiemo Nagel, 2016-11-17 10:12:22]

Drew, Maksim, could you please take a look?

Thank you,
Thiemo

[Thiemo Nagel, 2016-11-17 10:14:18]

If you agree with the change, I'll have to deprecate the corresponding UMA
entry, too.

[commit-bot: I haz the power, 2016-11-17 11:21:19]

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

[commit-bot: I haz the power, 2016-11-17 11:21:20]

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

[Thiemo Nagel, 2016-11-17 12:04:50]

https://codereview.chromium.org/2507423002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_validator.cc (left):

https://codereview.chromium.org/2507423002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_validator.cc:280: return true;
Actually, maybe best to just return false here so that UMA stays intact.  (BTW,
there's approximately zero counts for that branch anyways.)

[Andrew T Wilson (Slow), 2016-11-17 12:32:34]

https://codereview.chromium.org/2507423002/diff/1/components/policy/core/comm...
File components/policy/core/common/cloud/cloud_policy_validator.cc (left):

https://codereview.chromium.org/2507423002/diff/1/components/policy/core/comm...
components/policy/core/common/cloud/cloud_policy_validator.cc:277:
UMA_HISTOGRAM_ENUMERATION(kMetricPolicyKeyVerification,
Note that if verification_key_ is empty, CheckVerificationKeySignature() below
will fail a CHECK() - you can see this in the failed SAML tests from the
trybots.

Let's keep the check in place and just return false as you say, but be prepared
to fix up any tests that aren't setting up policy correctly.

UMA doesn't seem to show that any clients are actually hitting this, which is
good.

[emaxx, 2016-11-17 13:20:37]

Thiemo,

Won't this change break the --disable-policy-key-verification command line flag?
I think it's still required when doing local test runs against YAPS or policy
test server.

[Thiemo Nagel, 2016-11-21 14:27:57]

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

[commit-bot: I haz the power, 2016-11-21 14:28:09]

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

[Thiemo Nagel, 2016-11-21 14:44:10]

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

[commit-bot: I haz the power, 2016-11-21 14:44:18]

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, 2016-11-21 15:28:40]

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

[commit-bot: I haz the power, 2016-11-21 15:28:40]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-11-21 16:05:07]

Description was changed from

==========
Don't ingore missing verification key in CloudPolicyValidatorBase

Ignoring a missing key seems dangerous.

BUG=none
==========

to

==========
Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing the
key around.

BUG=none
==========

[Thiemo Nagel, 2016-11-21 16:07:38]

On 2016/11/17 13:20:37, emaxx wrote:
> Won't this change break the --disable-policy-key-verification command line
flag?
> I think it's still required when doing local test runs against YAPS or policy
> test server.

Thanks Maksim, I was missing that piece of information!  I've added a comment to
make it clear what's happening there.

I've also dropped a lot of unnecessary plumbing for the verification key.  Could
you maybe take another look?

[Thiemo Nagel, 2016-11-21 16:07:59]

Description was changed from

==========
Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing the
key around.

BUG=none
==========

to

==========
Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing it
around.

BUG=none
==========

[emaxx, 2016-11-21 16:42:17]

I'm fine with this change as it's anyway not clear to me how the verification
key rotation was expected to be organized.
So, assuming it's a constant, the code simplification makes sense.
LGTM

[Thiemo Nagel, 2016-11-21 17:02:16]

On 2016/11/21 16:42:17, emaxx wrote:
> I'm fine with this change as it's anyway not clear to me how the verification
> key rotation was expected to be organized.
> So, assuming it's a constant, the code simplification makes sense.

It is my understanding that the baked-in constant would be replaced by a
different one.

Drew, are you OK with this change?

[Andrew T Wilson (Slow), 2016-11-25 15:20:25]

LGTM, and correct, verification key rotation is done manually. There's not
really any facility for rotation as-is - what we'd end up having to do is build
special code in the validator to try a different key based on a verification key
id in the policy (also doesn't exist yet). Idea is that we'd do that if/when we
ever decide we need to rotate the key.

https://codereview.chromium.org/2507423002/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/user_cloud_policy_store.cc (right):

https://codereview.chromium.org/2507423002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:331:
base::Bind(&UserCloudPolicyStore::StorePolicyAfterValidation,
Should this be combined with prev line?

[Thiemo Nagel, 2016-11-28 13:17:08]

https://codereview.chromium.org/2507423002/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/user_cloud_policy_store.cc (right):

https://codereview.chromium.org/2507423002/diff/40001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:331:
base::Bind(&UserCloudPolicyStore::StorePolicyAfterValidation,
On 2016/11/25 15:20:25, Andrew T Wilson (Slow) wrote:
> Should this be combined with prev line?

Yes, thanks.  I've run "git cl format" which did some more adjustments.

[Thiemo Nagel, 2016-11-28 13:17:11]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-11-28 13:17:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2507423002/#ps60001
(title: "git cl format and rebase.")

[commit-bot: I haz the power, 2016-11-28 13:17:21]

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, 2016-11-28 14:13:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480339031781960,
"parent_rev": "4511c8a34ec02ffa0f9129c196ff6e11afd00bc8", "commit_rev":
"f2d922c6bf1826e2f0feceb8190ae152f66bd3e1"}

[commit-bot: I haz the power, 2016-11-28 14:14:18]

Description was changed from

==========
Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing it
around.

BUG=none
==========

to

==========
Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing it
around.

BUG=none
==========

[commit-bot: I haz the power, 2016-11-28 14:14:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-28 14:17:41]

Description was changed from

==========
Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing it
around.

BUG=none
==========

to

==========
Remove unnecessary plumbing for policy verification key

The key is hard-coded anyways, thus there's no benefit in passing it
around.

BUG=none

Committed: https://crrev.com/aea27953b5a2898d62c9114a442dc33a962a98f1
Cr-Commit-Position: refs/heads/master@{#434652}
==========

[commit-bot: I haz the power, 2016-11-28 14:17:42]

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