Added support for signed policy blobs on desktop.

Added support for signed policy blobs on desktop.

PolicyFetchRequests on desktop now request signed policy blobs. Also added
plumbing for injecting a google-supplied key to verify the blob signing keys to
protect against local tampering.

BUG=275291
TBR=joaodasilva@chromium.org

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

[Andrew T Wilson (Slow), 2014-01-25 00:18:12]

PTAL

[Mattias Nissler (ping if slow), 2014-01-27 13:52:12]

Here's the initial review.

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:51:
GetPolicyVerificationKey(),
So we actually do verification for Chrome OS? Didn't you mention elsewhere that
it is not?

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:518: if
(!base::ReadFileToString(path, key) ||
nit: I'd prefer ReadFile over ReadFileToString since the former uses only
limited memory (strictly speaking, there's a race here where the file size might
have increased since the GetFileSize() call above).

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
(right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation_unittest.cc:271: 
nit: remove newline?

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/user_policy_signin_service_base.cc (right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/policy/c...
chrome/browser/policy/cloud/user_policy_signin_service_base.cc:55:
DCHECK(manager);
remove, we'll crash anyways.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_constants.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_constants.cc:81: const char
kPolicyVerificationKeyHash[] = "1:356l7w";
This doesn't look like a sha-256 hash?

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.h:52: const
std::string& verification_key_hash,
Does this make sense here? The class itself doesn't use the value at all.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:42: // actual
policy modification by malware).
Nit: Or bugs :) I'd just phrase this in a generic way, i.e. "if the policy
cached on disk has been modified".

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:246: bool
CloudPolicyValidatorBase::CheckNewPublicKeyVerificationSignature() {
nit: Please keep function declaration/definition order consistent with the
header.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:258: DLOG(ERROR)
<< "Policy is missing public_key_verification_signature";
I think this should actually be LOG(ERROR), so logs contain the reason for
policy validation failure. This has proven to be useful on Chrome OS a number of
times.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:292: // signed
data.
Do you want to add this now or adjust in a follow-up CL?

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:309: return
VALIDATION_BAD_SIGNATURE;
It may make sense to add another enum value that we use to flag problems with
key certification. Depends on whether you see value in being able to distinguish
policy signature verification failures from key certification failures in
about:policy (and potentially elsewhere).

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:320: // If the
client has included a signature for the base signing key, then
Unclear what "the client" means here - I think you mean CloudPolicyClient. If
so, I'd prefer to phrase this as "if a key certification signature is
available".

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:182:
policy_.clear_new_public_key_signature();
The problem with this code is that it makes the builder unusable for testing
error scenarios. Say you want to write a test case that needs to set up
new_public_key values manually. If you clear() here, the test case can't create
the situation it needs. That's why I have generally opted to move this kind of
code into the caller. That also generally makes it easier to read the test case,
because PolicyBuilder does less magic when preparing the policy blob.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:220: std::string
PolicyBuilder::CreateTestSigningKeySignature() {
These should probably be named GetXXX(), given that we actually don't sign
anything here but return canned data?

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.h (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.h:62: void UnsetSigningKey();
nit: blank line before comment.

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

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:91:
result.key.has_signing_key());
indentation

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:317:
DCHECK(policy_key_.empty());
AFAICS, this code path can theoretically be hit when you call Load() more than
once. Since the public interface of the class allows that and policy_key_
already being present is not an error, this DCHECK should probably be removed.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:349: }
nit blank line for visual separation.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:377:
InstallPolicy(validator->policy_data().Pass(), validator->payload().Pass());
nit: newline before comment.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc
(right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:186: 
nit: remove newline

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:202:
Sequence s;
Side note for Bartosz: It seems like testing this external data manager sequence
stuff on each and every test case adds a lot of noise - why don't we have a
single test case that verifies stuff happens in the right order?

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:208: 
nit: remove 2nd blank line

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:221:
Sequence s;
can just use a different variable name instead of a nested scope.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:300:
Mock::VerifyAndClearExpectations(&observer_);
So much boilerplate... can we separate this out into a separate function for
now?

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:535: 
Can you add another test that verifies we detect tampered disk state such as
policy is correctly signed on disk but doesn't check out against the
verification key?

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:180: // or out-of-date
Chrome client.
This comment should say how the hash is computed.

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:307: //
new_public_key_verification_signature.
We should not only verify new_public_key (i.e. the rotation case), but also
against the current public key (i.e. no rotation). I think your code already
does that, so this comment is just inaccurate?

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
File components/policy/proto/policy_signing_key.proto (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/policy_signing_key.proto:18: optional bytes
signing_key_signature = 2;
Suggestion: Rename this to signing_key_certificate? IMHO, that'd be closer to
the standard terms and thus pressing the right buttons in one's mental state.

[Andrew T Wilson (Slow), 2014-01-30 17:10:30]

PTAL

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:51:
GetPolicyVerificationKey(),
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> So we actually do verification for Chrome OS? Didn't you mention elsewhere
that
> it is not?

The plumbing is in place, but GetPolicyVerificationKey() currently returns "no
key" on ChromeOS. At some point in the future we can turn it on by changing that
routine.

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:518: if
(!base::ReadFileToString(path, key) ||
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit: I'd prefer ReadFile over ReadFileToString since the former uses only
> limited memory (strictly speaking, there's a race here where the file size
might
> have increased since the GetFileSize() call above).

Done.

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
(right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation_unittest.cc:271: 
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit: remove newline?

Done.

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/user_policy_signin_service_base.cc (right):

https://codereview.chromium.org/116273002/diff/490001/chrome/browser/policy/c...
chrome/browser/policy/cloud/user_policy_signin_service_base.cc:55:
DCHECK(manager);
Reverted this change entirely.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_constants.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_constants.cc:81: const char
kPolicyVerificationKeyHash[] = "1:356l7w";
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> This doesn't look like a sha-256 hash?

Doesn't matter what the format is - it's just an opaque value that I pass back
to the server (This is the exact string that dkalin gave me).

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.h:52: const
std::string& verification_key_hash,
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> Does this make sense here? The class itself doesn't use the value at all.

Yeah, I had this idea that CPM would be the holder for the verification key hash
(so you could inject it into classes and classes could fetch it directly from
CPM). I think now that it's in CloudPolicyConstants this doesn't make much
sense, so I'll remove this and change the callers to fetch it from
CloudPolicyConstants.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:42: // actual
policy modification by malware).
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> Nit: Or bugs :) I'd just phrase this in a generic way, i.e. "if the policy
> cached on disk has been modified".

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:246: bool
CloudPolicyValidatorBase::CheckNewPublicKeyVerificationSignature() {
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit: Please keep function declaration/definition order consistent with the
> header.

Well, it's done...ish. Note that not all of the other routines are in
header-matching order. But I updated the header to move my routines after
RunChecks() and I take no responsibility for the shenanigans of other methods in
the file.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:258: DLOG(ERROR)
<< "Policy is missing public_key_verification_signature";
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> I think this should actually be LOG(ERROR), so logs contain the reason for
> policy validation failure. This has proven to be useful on Chrome OS a number
of
> times.

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:292: // signed
data.
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> Do you want to add this now or adjust in a follow-up CL?

I will add it later - it requires adding a new proto etc etc and it's not worth
adding to this. I'll add the proto *plus* signatures for example.com in a
followup.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:309: return
VALIDATION_BAD_SIGNATURE;
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> It may make sense to add another enum value that we use to flag problems with
> key certification. Depends on whether you see value in being able to
distinguish
> policy signature verification failures from key certification failures in
> about:policy (and potentially elsewhere).

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:320: // If the
client has included a signature for the base signing key, then
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> Unclear what "the client" means here - I think you mean CloudPolicyClient. If
> so, I'd prefer to phrase this as "if a key certification signature is
> available".

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:182:
policy_.clear_new_public_key_signature();
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> The problem with this code is that it makes the builder unusable for testing
> error scenarios. Say you want to write a test case that needs to set up
> new_public_key values manually. If you clear() here, the test case can't
create
> the situation it needs.

Yes it can - it can just inject them after calling PolicyBuilder::Build().

> That's why I have generally opted to move this kind of
> code into the caller. That also generally makes it easier to read the test
case,
> because PolicyBuilder does less magic when preparing the policy blob.

I disagree - you ought to be able to do this:

PolicyBuilder::SetDefaultSigningKey();
PolicyBuilder::Build();   <- Build blob with new_public_key
...tweak policy payload for second policy blob...
PolicyBuilder::Build();  <- Build blob without new_public_key

Having to manually clear the new_public_key and new_public_key_signature seems
like it's exposing too much about the internals. This is doubly true since
PolicyBuilder by default calls SetDefaultSigningKey() in its constructor. This
means that by default, if you just do this:

PolicyBuilder::Build();
...pass policy to Store for validation...
PolicyBuilder::Build();
...pass second policy to same Store for validation...

...the second policy will fail validation since it's trying to do an invalid key
rotation (set the public key twice).

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:220: std::string
PolicyBuilder::CreateTestSigningKeySignature() {
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> These should probably be named GetXXX(), given that we actually don't sign
> anything here but return canned data?

Done. I named this as CreateXXXX to match
CreateTestSigningKey()/CreateTestOtherSigningKey(). These are basically pairs
with those two routines, except they return the signature for those keys. But
I'm OK with calling these GetXXX() instead.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.h (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.h:62: void UnsetSigningKey();
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit: blank line before comment.

Done.

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

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:91:
result.key.has_signing_key());
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> indentation

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:317:
DCHECK(policy_key_.empty());
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> AFAICS, this code path can theoretically be hit when you call Load() more than
> once. Since the public interface of the class allows that and policy_key_
> already being present is not an error, this DCHECK should probably be removed.


Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:349: }
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit blank line for visual separation.

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store.cc:377:
InstallPolicy(validator->policy_data().Pass(), validator->payload().Pass());
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit: newline before comment.

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc
(right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:186: 
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:202:
Sequence s;
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> Side note for Bartosz: It seems like testing this external data manager
sequence
> stuff on each and every test case adds a lot of noise - why don't we have a
> single test case that verifies stuff happens in the right order?

Should I do anything to make sure Bartosz sees this comment?

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:208: 
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> nit: remove 2nd blank line

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:221:
Sequence s;
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> can just use a different variable name instead of a nested scope.

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:300:
Mock::VerifyAndClearExpectations(&observer_);
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> So much boilerplate... can we separate this out into a separate function for
> now?

Good call. Done!

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_store_unittest.cc:535: 
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> Can you add another test that verifies we detect tampered disk state such as
> policy is correctly signed on disk but doesn't check out against the
> verification key?

I've added a TODO for this. I intentionally haven't written that test yet - we
don't actually verify signing keys and produce a failure, since the production
server doesn't properly generate a per-domain signature yet for these keys.

I'll add that once I have integrated the proper verification keys from Dennis
and we've updated the signature verification on the server and test servers -
see the TODOs in CloudPolicyValidator.

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:180: // or out-of-date
Chrome client.
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> This comment should say how the hash is computed.

Why? We don't use it locally, so the actual algorithm used by the server
shouldn't be part of the public contract? This is basically a verbatim copy of
the already-landed server-side proto edit
(https://cs.corp.google.com/?verification_key_hash=#piper///depot/google3/wire...)

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:307: //
new_public_key_verification_signature.
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> We should not only verify new_public_key (i.e. the rotation case), but also
> against the current public key (i.e. no rotation). I think your code already
> does that, so this comment is just inaccurate?

Not sure I understand your question? new_public_key_verification_signature is
only present/stored when a new public key is sent down. The fact that we cache a
key locally and so have to cache the new_public_key_verification_signature to
verify that key on reload doesn't seem pertinent to this proto?

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
File components/policy/proto/policy_signing_key.proto (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/policy_signing_key.proto:18: optional bytes
signing_key_signature = 2;
On 2014/01/27 13:52:13, Mattias Nissler wrote:
> Suggestion: Rename this to signing_key_certificate? IMHO, that'd be closer to
> the standard terms and thus pressing the right buttons in one's mental state.

I'll add a comment. We already using _signature in other places in our protos
for similar situations (think of new_public_key_signature) so I'll keep this
name for now.

[Mattias Nissler (ping if slow), 2014-01-31 21:00:34]

This LGTM, improvements can be done in subsequent CLs.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:182:
policy_.clear_new_public_key_signature();
On 2014/01/30 17:10:31, Andrew T Wilson wrote:
> On 2014/01/27 13:52:13, Mattias Nissler wrote:
> > The problem with this code is that it makes the builder unusable for testing
> > error scenarios. Say you want to write a test case that needs to set up
> > new_public_key values manually. If you clear() here, the test case can't
> create
> > the situation it needs.
> 
> Yes it can - it can just inject them after calling PolicyBuilder::Build().
> 
> > That's why I have generally opted to move this kind of
> > code into the caller. That also generally makes it easier to read the test
> case,
> > because PolicyBuilder does less magic when preparing the policy blob.
> 
> I disagree - you ought to be able to do this:
> 
> PolicyBuilder::SetDefaultSigningKey();
> PolicyBuilder::Build();   <- Build blob with new_public_key
> ...tweak policy payload for second policy blob...
> PolicyBuilder::Build();  <- Build blob without new_public_key
> 
> Having to manually clear the new_public_key and new_public_key_signature seems
> like it's exposing too much about the internals. This is doubly true since
> PolicyBuilder by default calls SetDefaultSigningKey() in its constructor. This
> means that by default, if you just do this:
> 
> PolicyBuilder::Build();
> ...pass policy to Store for validation...
> PolicyBuilder::Build();
> ...pass second policy to same Store for validation...
> 
> ...the second policy will fail validation since it's trying to do an invalid
key
> rotation (set the public key twice).

Note that this will even fail for the code you have, unless you
SetSigningKey(GetNewSigningKey()) and UnsetNewSigningKey().

However, that's actually the behavior to I prefer - Build() should deliver
stable results if you call it multiple times in a row without modifying
PolicyBuilder state. I had not realized that you're not clearing builder state,
but build result state. What you have is great.

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:180: // or out-of-date
Chrome client.
On 2014/01/30 17:10:31, Andrew T Wilson wrote:
> On 2014/01/27 13:52:13, Mattias Nissler wrote:
> > This comment should say how the hash is computed.
> 
> Why? We don't use it locally, so the actual algorithm used by the server
> shouldn't be part of the public contract? This is basically a verbatim copy of
> the already-landed server-side proto edit
>
(https://cs.corp.google.com/?verification_key_hash=#piper///depot/google3/wire...)

You're right, I was confused. Maybe we should name this verification_key_id
instead and change the comment to say it's opaque instead of saying it's a hash
(which it probably isn't given dkalin's value)?

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:307: //
new_public_key_verification_signature.
On 2014/01/30 17:10:31, Andrew T Wilson wrote:
> On 2014/01/27 13:52:13, Mattias Nissler wrote:
> > We should not only verify new_public_key (i.e. the rotation case), but also
> > against the current public key (i.e. no rotation). I think your code already
> > does that, so this comment is just inaccurate?
> 
> Not sure I understand your question? new_public_key_verification_signature is
> only present/stored when a new public key is sent down. The fact that we cache
a
> key locally and so have to cache the new_public_key_verification_signature to
> verify that key on reload doesn't seem pertinent to this proto?

Technically, you're right. The comment phrasing suggests though that the trust
chain checks only happen if new_public_key is present, when in fact we also
verify the trust chain for signatures on cached and newly-received blobs that
don't contain the new_public_key* fields.

https://codereview.chromium.org/116273002/diff/690001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/116273002/diff/690001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:323:
!CheckVerificationKeySignature(key_, verification_key_, key_signature_)) {
Wouldn't we fail here for the case where the verification key gets rotated on
Chrome update and we load a cached blob? Should the initial load after startup
for that case fail or not? Do we need to handle this more graceful?

I guess could keep the old key in the binary as well and allow that for
verification of cached blobs for a while. If we decide to do that, it makes
sense to write code (and tests!) for this already, s.t. the key rotation can be
accomplished by just changing the hard-coded constants. Probably material for a
follow-up CL though.

[Andrew T Wilson (Slow), 2014-02-02 11:31:57]

Done, landing, will push a separate CL to address any further comments.

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
File components/policy/core/common/cloud/policy_builder.cc (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/core/...
components/policy/core/common/cloud/policy_builder.cc:182:
policy_.clear_new_public_key_signature();
On 2014/01/31 21:00:34, Mattias Nissler wrote:
> On 2014/01/30 17:10:31, Andrew T Wilson wrote:
> > On 2014/01/27 13:52:13, Mattias Nissler wrote:
> > > The problem with this code is that it makes the builder unusable for
testing
> > > error scenarios. Say you want to write a test case that needs to set up
> > > new_public_key values manually. If you clear() here, the test case can't
> > create
> > > the situation it needs.
> > 
> > Yes it can - it can just inject them after calling PolicyBuilder::Build().
> > 
> > > That's why I have generally opted to move this kind of
> > > code into the caller. That also generally makes it easier to read the test
> > case,
> > > because PolicyBuilder does less magic when preparing the policy blob.
> > 
> > I disagree - you ought to be able to do this:
> > 
> > PolicyBuilder::SetDefaultSigningKey();
> > PolicyBuilder::Build();   <- Build blob with new_public_key
> > ...tweak policy payload for second policy blob...
> > PolicyBuilder::Build();  <- Build blob without new_public_key
> > 
> > Having to manually clear the new_public_key and new_public_key_signature
seems
> > like it's exposing too much about the internals. This is doubly true since
> > PolicyBuilder by default calls SetDefaultSigningKey() in its constructor.
This
> > means that by default, if you just do this:
> > 
> > PolicyBuilder::Build();
> > ...pass policy to Store for validation...
> > PolicyBuilder::Build();
> > ...pass second policy to same Store for validation...
> > 
> > ...the second policy will fail validation since it's trying to do an invalid
> key
> > rotation (set the public key twice).
> 
> Note that this will even fail for the code you have, unless you
> SetSigningKey(GetNewSigningKey()) and UnsetNewSigningKey().
> 
> However, that's actually the behavior to I prefer - Build() should deliver
> stable results if you call it multiple times in a row without modifying
> PolicyBuilder state. I had not realized that you're not clearing builder
state,
> but build result state. What you have is great.

Done.

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:180: // or out-of-date
Chrome client.
On 2014/01/31 21:00:34, Mattias Nissler wrote:
> On 2014/01/30 17:10:31, Andrew T Wilson wrote:
> > On 2014/01/27 13:52:13, Mattias Nissler wrote:
> > > This comment should say how the hash is computed.
> > 
> > Why? We don't use it locally, so the actual algorithm used by the server
> > shouldn't be part of the public contract? This is basically a verbatim copy
of
> > the already-landed server-side proto edit
> >
>
(https://cs.corp.google.com/?verification_key_hash=#piper///depot/google3/wire...)
> 
> You're right, I was confused. Maybe we should name this verification_key_id
> instead and change the comment to say it's opaque instead of saying it's a
hash
> (which it probably isn't given dkalin's value)?

I'll do this in a followup CL.

https://codereview.chromium.org/116273002/diff/490001/components/policy/proto...
components/policy/proto/device_management_backend.proto:307: //
new_public_key_verification_signature.
On 2014/01/31 21:00:34, Mattias Nissler wrote:
> On 2014/01/30 17:10:31, Andrew T Wilson wrote:
> > On 2014/01/27 13:52:13, Mattias Nissler wrote:
> > > We should not only verify new_public_key (i.e. the rotation case), but
also
> > > against the current public key (i.e. no rotation). I think your code
already
> > > does that, so this comment is just inaccurate?
> > 
> > Not sure I understand your question? new_public_key_verification_signature
is
> > only present/stored when a new public key is sent down. The fact that we
cache
> a
> > key locally and so have to cache the new_public_key_verification_signature
to
> > verify that key on reload doesn't seem pertinent to this proto?
> 
> Technically, you're right. The comment phrasing suggests though that the trust
> chain checks only happen if new_public_key is present, when in fact we also
> verify the trust chain for signatures on cached and newly-received blobs that
> don't contain the new_public_key* fields.

Technically we don't do any verification on newly-received blobs that don't
contain the new_public_key fields - it's only used on key rotation, initial
provisioning of the initial key, and loading from cache. I guess I just don't
understand what comment you want here - I could put "clients that store the
new_public_key in insecure storage will also cache the
new_public_key_verification_signature so they can verify the key has not been
modified when loading" but this seems like the totally wrong place to put this.
It's pretty much inherent that if you have data + a signature, and you're
caching that data and want to verify the data on reload, then you need to cache
the signature too.

I think I might just be misunderstanding what you want, though - what comment
would you propose to make this clearer? I'm happy to push a separate CL through
for that.

https://codereview.chromium.org/116273002/diff/690001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.cc (right):

https://codereview.chromium.org/116273002/diff/690001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.cc:323:
!CheckVerificationKeySignature(key_, verification_key_, key_signature_)) {
On 2014/01/31 21:00:35, Mattias Nissler wrote:
> Wouldn't we fail here for the case where the verification key gets rotated on
> Chrome update and we load a cached blob? Should the initial load after startup
> for that case fail or not? Do we need to handle this more graceful?
> 
> I guess could keep the old key in the binary as well and allow that for
> verification of cached blobs for a while. If we decide to do that, it makes
> sense to write code (and tests!) for this already, s.t. the key rotation can
be
> accomplished by just changing the hard-coded constants. Probably material for
a
> follow-up CL though.

When we do key rotation, we'll need to add code on the client to support old +
new keys and plumb that through. I haven't done this yet, but it's not hard - I
was planning to defer this work until such time as we need to do our first key
rotation.

[Andrew T Wilson (Slow), 2014-02-02 11:32:32]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2014-02-02 11:32:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/116273002/690001

[commit-bot: I haz the power, 2014-02-02 11:57:27]

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

[commit-bot: I haz the power, 2014-02-02 11:57:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-02-02 11:57:30]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-02 11:57:33]

CQ bit was unchecked on CL. Ignoring.

[Andrew T Wilson (Slow), 2014-02-02 15:11:27]

On 2014/02/02 11:57:33, I haz the power (commit-bot) wrote:
> CQ bit was unchecked on CL. Ignoring.

Added joao due to odd presubmit error.

[Andrew T Wilson (Slow), 2014-02-02 15:11:39]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2014-02-02 15:11:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/116273002/690001

[commit-bot: I haz the power, 2014-02-02 17:28:47]

Change committed as 248416

[commit-bot: I haz the power, 2014-02-02 17:28:54]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-02 17:28:55]

CQ bit was unchecked on CL. Ignoring.

[Joao da Silva, 2014-02-03 09:11:34]

An approval from a "foo" owner is required when "foo" is added to "policy/DEPS".
The "policy/proto" includes come from generated files that have no OWNERS and
it's fine to bypass these errors, IMO.

checkdeps.py can probably be improved to avoid these false positives.