Verify the signature on user cloud policy downloads.

Verify the signature on user cloud policy downloads.

The signature on the user cloud policy blob is already verified by the session
manager on ChromeOS, but should also be verified by Chrome before storing new
policy, and after loading policy from the cache.

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

[Joao da Silva, 2013-02-04 16:11:36]

Here's user policy signature validation on chromeos. Early comments welcome;
automated tests will come after trying to break this with manual testing.

[Mattias Nissler (ping if slow), 2013-02-04 16:44:56]

Looks reasonable to me

https://codereview.chromium.org/12183017/diff/1/chrome/browser/policy/user_cl...
File chrome/browser/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/12183017/diff/1/chrome/browser/policy/user_cl...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:39:
FILE_PATH_LITERAL("/var/run/user_policy/%s.pub");
can we make user_policy/%s a directory? That'd allow for future extensibility.

[Joao da Silva, 2013-02-06 00:34:37]

Added a bunch of tests, please have another look.

The dbus/ changes are under review separately at
https://codereview.chromium.org/12223011/, I'll rebase afterwards. The tests
work without that, but gmock outputs a lot of noise.

AFAICT --policy-key at the testserver is never used, so it's been removed and
rotations are controlled by a parameter in the json file.

I plan to write a couple more tests for this, in cloud_policy_browsertests.cc.
But this CL is big enough as is :-)

https://codereview.chromium.org/12183017/diff/1/chrome/browser/policy/user_cl...
File chrome/browser/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/12183017/diff/1/chrome/browser/policy/user_cl...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:39:
FILE_PATH_LITERAL("/var/run/user_policy/%s.pub");
On 2013/02/04 16:44:57, Mattias Nissler wrote:
> can we make user_policy/%s a directory? That'd allow for future extensibility.

In progress at https://gerrit.chromium.org/gerrit/#/c/42542/; this CL is
assuming that path.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.h:130: static void
SetRootPathForTesting(const FilePath::CharType* root_path);
Better ideas welcome :-)

I don't totally dislike this, since BrowserPolicyConnector() creates a lot of
stuff and passes paths into constructors; this may be handy for more things in
the future.

[Mattias Nissler (ping if slow), 2013-02-06 17:58:25]

I need to leave and didn't manage to review the entire patch, but here's my
feedback so far.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.h:130: static void
SetRootPathForTesting(const FilePath::CharType* root_path);
On 2013/02/06 00:34:38, Joao da Silva wrote:
> Better ideas welcome :-)
> 
> I don't totally dislike this, since BrowserPolicyConnector() creates a lot of
> stuff and passes paths into constructors; this may be handy for more things in
> the future.

How about getting the path from PathService and installing an override in tests?
That's the canonical way of handling this situation in Chrome AFAICT.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
File chrome/browser/policy/policy_builder.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
chrome/browser/policy/policy_builder.cc:148:
CHECK(signing_key_->ExportPublicKey(public_key));
What's the value in adding these helpers?

[Mattias Nissler (ping if slow), 2013-02-07 14:12:07]

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:189:
testserver_relative_docroot()));
any reason not to use an absolute path here?

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:271: return
temp_dir_.path().AppendASCII("testserver");
inline into testserver_device_management_file?

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:282: FilePath root_path() {
you probably want to rename this if you switch to PathService.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:302: FilePath::StringType
root_path_;
needed?

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:338: #if defined(OS_CHROMEOS)
It seems like everything that's really chromeos-specific here are the checks for
the key file, and those are actually mocked out. Does this test run successfully
on desktop actually?

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:200: // Start
validation. The Validator will free itself once validation is
nit: s/free/delete/

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos.h:94: void
EnsurePolicyKeyLoaded(const base::Closure& callback);
document.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:142: void
StorePolicy(const std::vector<uint8>* new_public_key,
Should be named PerformPolicyStore for consistency

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:156:
EXPECT_FALSE(store_->policy());
Shouldn't this be EXPECT_EQ(previous_value != NULL, store_->policy() != NULL);
without the if?

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:196:
loop_.RunUntilIdle();
Ah, I hate these snippets. Blocking pool sucks in testing.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:239:
StorePolicy(&new_public_key, NULL, kDefaultHomepage);
Wrap in ASSERT_NO_FATAL_FAILURE? (here and below)

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:320: 
remove redundant newline

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:388:
EXPECT_EQ(CloudPolicyStore::STATUS_VALIDATION_ERROR, store_->status());
seems like the last 4 lines are now frequent enough to warrant a helper
function.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:472:
TEST_F(UserCloudPolicyStoreChromeOSTest, MigrationAndStoreNew) {
This one is good, thanks for adding!

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:500: // Now
store a new policy using the new location.
*homepage location

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_key.h (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_policy_key.h:18: class UserPolicyKey : public
base::NonThreadSafe {
Not quite convinced putting this into its own class is justified after all. It's
more or less a glorified ReadFileToVector(), executing on a blocking bool
thread. How about just doing a static scoped_ptr<std::vector<uint8>>
UserPolicyStoreChromeOS::LoadPolicyKey(const FilePath& path) that you can fire
on the blocking pool?

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_policy_key.h:34: base::WeakPtrFactory<UserPolicyKey>
weak_ptr_factory_;
DISALLOw_COPY_AND_ASSIGN?

https://codereview.chromium.org/12183017/diff/14002/chromeos/dbus/mock_dbus_t...
File chromeos/dbus/mock_dbus_thread_manager.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chromeos/dbus/mock_dbus_t...
chromeos/dbus/mock_dbus_thread_manager.cc:192: .Times(AnyNumber());
The changes in this file should probably go into a separate CL.

https://codereview.chromium.org/12183017/diff/14002/net/tools/testserver/devi...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/12183017/diff/14002/net/tools/testserver/devi...
net/tools/testserver/device_management.py:564: key =
tlslite.api.generateRSAKey(512)
I'd hate to loose the ability to use keys passed on the command line - that
feature has been very handy when I needed to generate valid policy blobs for
seeding a client that couldn't register when working on the public accounts
stuff.

[Joao da Silva, 2013-02-07 16:31:59]

@Mattias: PTAL

@Jochen: please review the changes at chrome/common/

Thanks!

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.h:130: static void
SetRootPathForTesting(const FilePath::CharType* root_path);
On 2013/02/06 17:58:25, Mattias Nissler wrote:
> On 2013/02/06 00:34:38, Joao da Silva wrote:
> > Better ideas welcome :-)
> > 
> > I don't totally dislike this, since BrowserPolicyConnector() creates a lot
of
> > stuff and passes paths into constructors; this may be handy for more things
in
> > the future.
> 
> How about getting the path from PathService and installing an override in
tests?
> That's the canonical way of handling this situation in Chrome AFAICT.
> 

Good idea, done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:189:
testserver_relative_docroot()));
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> any reason not to use an absolute path here?

Yes, see comment in line 179.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:271: return
temp_dir_.path().AppendASCII("testserver");
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> inline into testserver_device_management_file?

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:282: FilePath root_path() {
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> you probably want to rename this if you switch to PathService.

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:302: FilePath::StringType
root_path_;
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:338: #if defined(OS_CHROMEOS)
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> It seems like everything that's really chromeos-specific here are the checks
for
> the key file, and those are actually mocked out. Does this test run
successfully
> on desktop actually?

They're mocked at the session_manager mock, which doesn't exist on the desktop.

This test doesn't have much value on the desktop, since the keys aren't stored
and there's no validation.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
File chrome/browser/policy/policy_builder.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
chrome/browser/policy/policy_builder.cc:148:
CHECK(signing_key_->ExportPublicKey(public_key));
On 2013/02/06 17:58:25, Mattias Nissler wrote:
> What's the value in adding these helpers?

They're used in the unit test.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos.cc (left):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:283: // Start
validation. The Validator will free itself once validation is
I think the comment was originally from here.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:200: // Start
validation. The Validator will free itself once validation is
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> nit: s/free/delete/

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos.cc:288: // Start
validation. The Validator will free itself once validation is
Changed here too for consistency.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos.h:94: void
EnsurePolicyKeyLoaded(const base::Closure& callback);
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> document.

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:142: void
StorePolicy(const std::vector<uint8>* new_public_key,
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> Should be named PerformPolicyStore for consistency

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:156:
EXPECT_FALSE(store_->policy());
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> Shouldn't this be EXPECT_EQ(previous_value != NULL, store_->policy() != NULL);
> without the if?

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:239:
StorePolicy(&new_public_key, NULL, kDefaultHomepage);
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> Wrap in ASSERT_NO_FATAL_FAILURE? (here and below)

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:320: 
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> remove redundant newline

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:388:
EXPECT_EQ(CloudPolicyStore::STATUS_VALIDATION_ERROR, store_->status());
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> seems like the last 4 lines are now frequent enough to warrant a helper
> function.

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:500: // Now
store a new policy using the new location.
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> *homepage location

Done.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
File chrome/browser/policy/user_policy_key.h (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/use...
chrome/browser/policy/user_policy_key.h:18: class UserPolicyKey : public
base::NonThreadSafe {
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> Not quite convinced putting this into its own class is justified after all.
It's
> more or less a glorified ReadFileToVector(), executing on a blocking bool
> thread. How about just doing a static scoped_ptr<std::vector<uint8>>
> UserPolicyStoreChromeOS::LoadPolicyKey(const FilePath& path) that you can fire
> on the blocking pool?

Done.

https://codereview.chromium.org/12183017/diff/14002/chromeos/dbus/mock_dbus_t...
File chromeos/dbus/mock_dbus_thread_manager.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chromeos/dbus/mock_dbus_t...
chromeos/dbus/mock_dbus_thread_manager.cc:192: .Times(AnyNumber());
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> The changes in this file should probably go into a separate CL.

Yea, already done (https://codereview.chromium.org/12223011/); this was meant to
be rebased on that.

https://codereview.chromium.org/12183017/diff/14002/net/tools/testserver/devi...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/12183017/diff/14002/net/tools/testserver/devi...
net/tools/testserver/device_management.py:564: key =
tlslite.api.generateRSAKey(512)
On 2013/02/07 14:12:07, Mattias Nissler wrote:
> I'd hate to loose the ability to use keys passed on the command line - that
> feature has been very handy when I needed to generate valid policy blobs for
> seeding a client that couldn't register when working on the public accounts
> stuff.

Reverted.

[Joao da Silva, 2013-02-07 16:42:52]

@Jochen: please review the changes at chrome/common/, thanks!

[jochen (gone - plz use gerrit), 2013-02-07 17:13:14]

lgtm

[Mattias Nissler (ping if slow), 2013-02-08 13:36:42]

Getting close. Regarding writing to the source dir, I'm not really sure what the
rules are. You might want to ask maruel@

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:189:
testserver_relative_docroot()));
On 2013/02/07 16:32:00, Joao da Silva wrote:
> On 2013/02/07 14:12:07, Mattias Nissler wrote:
> > any reason not to use an absolute path here?
> 
> Yes, see comment in line 179.

I don't think it's OK to write to the source dir though?

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
File chrome/browser/policy/policy_builder.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
chrome/browser/policy/policy_builder.cc:148:
CHECK(signing_key_->ExportPublicKey(public_key));
On 2013/02/07 16:32:00, Joao da Silva wrote:
> On 2013/02/06 17:58:25, Mattias Nissler wrote:
> > What's the value in adding these helpers?
> 
> They're used in the unit test.

Well, but the unit test could just call ExportPublicKey directly? The CHECKs in
146 and 147 are redundant anyways. If you feel strong about keeping the helpers,
you should probably also fix all code that makes these calls directly (I believe
there are a few).

https://codereview.chromium.org/12183017/diff/14007/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/12183017/diff/14007/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:474:
testing::Sequence seq;
Not used.

https://codereview.chromium.org/12183017/diff/14007/net/tools/testserver/devi...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/12183017/diff/14007/net/tools/testserver/devi...
net/tools/testserver/device_management.py:575: # Generate 2 private keys if no
private keys directory was specified.
nit: it's not a directory, you can specify the flag multiple times.

[Joao da Silva, 2013-02-08 16:47:04]

PTAL. Fixing the testserver issue at https://codereview.chromium.org/12210088.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
File chrome/browser/policy/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/clo...
chrome/browser/policy/cloud_policy_browsertest.cc:189:
testserver_relative_docroot()));
On 2013/02/08 13:36:42, Mattias Nissler wrote:
> On 2013/02/07 16:32:00, Joao da Silva wrote:
> > On 2013/02/07 14:12:07, Mattias Nissler wrote:
> > > any reason not to use an absolute path here?
> > 
> > Yes, see comment in line 179.
> 
> I don't think it's OK to write to the source dir though?

Yea, if the test crashes then it leaves a scoped_temp_dir around. I'll leave
this for now and fix in another CL, it requires bigger changes on the
testserver.

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
File chrome/browser/policy/policy_builder.cc (right):

https://codereview.chromium.org/12183017/diff/14002/chrome/browser/policy/pol...
chrome/browser/policy/policy_builder.cc:148:
CHECK(signing_key_->ExportPublicKey(public_key));
On 2013/02/08 13:36:42, Mattias Nissler wrote:
> On 2013/02/07 16:32:00, Joao da Silva wrote:
> > On 2013/02/06 17:58:25, Mattias Nissler wrote:
> > > What's the value in adding these helpers?
> > 
> > They're used in the unit test.
> 
> Well, but the unit test could just call ExportPublicKey directly? The CHECKs
in
> 146 and 147 are redundant anyways. If you feel strong about keeping the
helpers,
> you should probably also fix all code that makes these calls directly (I
believe
> there are a few).

That's right, reverted these helpers.

https://codereview.chromium.org/12183017/diff/14007/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/12183017/diff/14007/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:474:
testing::Sequence seq;
On 2013/02/08 13:36:42, Mattias Nissler wrote:
> Not used.

Done.

https://codereview.chromium.org/12183017/diff/14007/net/tools/testserver/devi...
File net/tools/testserver/device_management.py (right):

https://codereview.chromium.org/12183017/diff/14007/net/tools/testserver/devi...
net/tools/testserver/device_management.py:575: # Generate 2 private keys if no
private keys directory was specified.
On 2013/02/08 13:36:42, Mattias Nissler wrote:
> nit: it's not a directory, you can specify the flag multiple times.

Done.

[Mattias Nissler (ping if slow), 2013-02-08 17:06:30]

LGTM. I'm wondering whether we should land this before the branch point though
given that we don't have UMA that'd tell us whether there are actually
validation errors showing up now that we're more strict in validation. WDYT?

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:79:
policy_.signing_key()->ExportPublicKey(&public_key);
ASSERT_TRUE()

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:243:
policy_.new_signing_key()->ExportPublicKey(&new_public_key);
ditto

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:258:
policy_.new_signing_key()->ExportPublicKey(&new_public_key);
ditto

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:501:
policy_.new_signing_key()->ExportPublicKey(&new_public_key);
ditto

https://codereview.chromium.org/12183017/diff/14008/chrome/common/chrome_path...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/12183017/diff/14008/chrome/common/chrome_path...
chrome/common/chrome_paths.cc:406: cur =
base::FilePath(FILE_PATH_LITERAL(kDefaultAppOrderFileName));
If you feel like cleaning up: FWIW, this is wrong (wraps the argument in
FILE_PATH_LITERAL twice), here and elsewhere in the file. Feel free to ignore
this comment.

[Joao da Silva, 2013-02-13 16:22:05]

M26 has sailed, so barring any red bots I'll send this to the CQ later today.

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
File chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:79:
policy_.signing_key()->ExportPublicKey(&public_key);
On 2013/02/08 17:06:30, Mattias Nissler wrote:
> ASSERT_TRUE()

Done.

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:243:
policy_.new_signing_key()->ExportPublicKey(&new_public_key);
On 2013/02/08 17:06:30, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:258:
policy_.new_signing_key()->ExportPublicKey(&new_public_key);
On 2013/02/08 17:06:30, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/12183017/diff/14008/chrome/browser/policy/use...
chrome/browser/policy/user_cloud_policy_store_chromeos_unittest.cc:501:
policy_.new_signing_key()->ExportPublicKey(&new_public_key);
On 2013/02/08 17:06:30, Mattias Nissler wrote:
> ditto

Done.

https://codereview.chromium.org/12183017/diff/14008/chrome/common/chrome_path...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/12183017/diff/14008/chrome/common/chrome_path...
chrome/common/chrome_paths.cc:406: cur =
base::FilePath(FILE_PATH_LITERAL(kDefaultAppOrderFileName));
On 2013/02/08 17:06:30, Mattias Nissler wrote:
> If you feel like cleaning up: FWIW, this is wrong (wraps the argument in
> FILE_PATH_LITERAL twice), here and elsewhere in the file. Feel free to ignore
> this comment.

Done in https://codereview.chromium.org/12230014/

[commit-bot: I haz the power, 2013-02-13 17:52:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/12183017/22001

[commit-bot: I haz the power, 2013-02-13 19:13:35]

Change committed as 182279