Store/Retrieve CrOS user policy in session_manager.

Store/Retrieve CrOS user policy in session_manager.

BUG=chromium-os:15189
TEST=unit tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132195

[Mattias Nissler (ping if slow), 2011-06-22 11:01:42]

Hi guys, I'd like to start the review on this now so the chance remains that
I'll be able to land it by Friday ;) Note that the try jobs won't compile due to
the associated libcros change not being in the tree yet.

[gfeher, 2011-06-22 12:41:31]

Some of my comments are just a cries for help...

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:94: callback));
How about collapsing these arguments into fewer lines? Do whatever seems nicer.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:112: reinterpret_cast<const
uint8*>(&signature[0]),
Nit: indent.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:208: delete this;
Here and all the similar cases: please make sure the destructor is private.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:214: void
CrosUserPolicyCache::StorePolicyOperation::Cancel() {
delete this?

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:281: void
CrosUserPolicyCache::RetrievePolicyOperation::Cancel() {
delete this?

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:310:
scoped_ptr<RetrievePolicyOperation> scoped_killer(this);
Please also try and implement this in a way that the destructor ends up being
private.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:413: if (result) {
So after storing a policy you immediately retrieve it to trigger some stuff?
Please add a comment to explain why.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:437:
legacy_token_cache_->Load();
Do we get a SetPolicy callback if this succeeds?

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.h:79:
scoped_refptr<UserPolicyTokenCache> legacy_token_cache_;
Add a TODO to remove the above two when all the legacy caches are supposed to be
imported?

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
File chrome/browser/policy/cros_user_policy_identity_strategy.h (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_identity_strategy.h:34: const
std::string& token);
Is this the device's dmtoken or the user's dmtoken?

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_identity_strategy.h (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.h:46: virtual void
LoadTokenCache();
OVERRIDE?

[Mattias Nissler (ping if slow), 2011-06-22 17:17:35]

I've addressed or answered Gabor's comments. There's still the issue with the
legacy policy not being read, which I'd like to fix, buy you can take another
look at this in the meanwhile.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:94: callback));
On 2011/06/22 12:41:31, gfeher wrote:
> How about collapsing these arguments into fewer lines? Do whatever seems
nicer.

Done.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:112: reinterpret_cast<const
uint8*>(&signature[0]),
On 2011/06/22 12:41:31, gfeher wrote:
> Nit: indent.

Done.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:208: delete this;
On 2011/06/22 12:41:31, gfeher wrote:
> Here and all the similar cases: please make sure the destructor is private.

Done.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:214: void
CrosUserPolicyCache::StorePolicyOperation::Cancel() {
On 2011/06/22 12:41:31, gfeher wrote:
> delete this?

Actually no, since the operation may be in flight. Technically, there's the
state after construction where the policy has not be Run() yet, at which we
leak. I've changed to code to make Run() a factory function now, so Run() also
creates the operation. This also gives a nicer interface to the caller.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:281: void
CrosUserPolicyCache::RetrievePolicyOperation::Cancel() {
On 2011/06/22 12:41:31, gfeher wrote:
> delete this?

See above.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:310:
scoped_ptr<RetrievePolicyOperation> scoped_killer(this);
On 2011/06/22 12:41:31, gfeher wrote:
> Please also try and implement this in a way that the destructor ends up being
> private.

Done.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:413: if (result) {
On 2011/06/22 12:41:31, gfeher wrote:
> So after storing a policy you immediately retrieve it to trigger some stuff?
> Please add a comment to explain why.

Oh, the idea is to only have one read path and use session_manager as the only
policy source. This way we're consistent even if session_manager fails and
manipulates the policy.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:437:
legacy_token_cache_->Load();
On 2011/06/22 12:41:31, gfeher wrote:
> Do we get a SetPolicy callback if this succeeds?

No, we just pull down policy again. I'm not trying to recover also the policy,
since we cannot signature-check it. Also, I think that window during which
policy is not applied will hardly be noticed by anybody. But then again, there's
the startup pages. Oh well. WDYT? Is it worth also loading the legacy policy
cache?

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.h:79:
scoped_refptr<UserPolicyTokenCache> legacy_token_cache_;
On 2011/06/22 12:41:31, gfeher wrote:
> Add a TODO to remove the above two when all the legacy caches are supposed to
be
> imported?

Done.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
File chrome/browser/policy/cros_user_policy_identity_strategy.h (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_identity_strategy.h:34: const
std::string& token);
On 2011/06/22 12:41:31, gfeher wrote:
> Is this the device's dmtoken or the user's dmtoken?

It's the user token. I thought that's obvious since this code is for cros _user_
policy?

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_identity_strategy.h (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.h:46: virtual void
LoadTokenCache();
On 2011/06/22 12:41:31, gfeher wrote:
> OVERRIDE?

Done.

[gfeher, 2011-06-22 22:18:17]

I am starting to understand what's going on, here are some more comments.

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/1/chrome/browser/policy/cros_user...
chrome/browser/policy/cros_user_policy_cache.cc:437:
legacy_token_cache_->Load();
I was formulating my arguments against wasting time on dealing with the legacy
policy cache... But, it came to my mind, that forced extensions would be
uninstalled if Chrome ever starts up without having policies. They would then
only  be reinstalled when the policies come back. I am not sure if all their
state would be erased by this, but I know some extensions that pop up their
welcome screens after such incidents. Because of this, I believe it'd be better
to somehow try to migrate the old policy blob. But then we would need an
exceptional case when policies are accepted without signatures, arghhhh.

p.s.: Did I just describe a race between the policy subsystem and the extension
subsystem?

http://codereview.chromium.org/7233006/diff/1012/chrome/browser/policy/cros_u...
chrome/browser/policy/cros_user_policy_cache.cc:33: // Manages the policy key
and implements signature checking on the key.
on the key -> with the key?

http://codereview.chromium.org/7233006/diff/1012/chrome/browser/policy/cros_u...
chrome/browser/policy/cros_user_policy_cache.cc:44: // Checks whether
|signature| is a valid over |data|. The status will be
is a valid -> is valid

http://codereview.chromium.org/7233006/diff/1012/chrome/browser/policy/cros_u...
chrome/browser/policy/cros_user_policy_cache.cc:198: const StatusCallback&
callback)
Possible nit: if you would do this check at the beginning of this method, then
you could save the redundant new+delete of StorePolicyOperation.

http://codereview.chromium.org/7233006/diff/1012/chrome/browser/policy/cros_u...
chrome/browser/policy/cros_user_policy_cache.cc:316: void
CrosUserPolicyCache::RetrievePolicyOperation::RetrievePolicyCallback(
We would get this error printed after SetUnmanaged() is called, right?

http://codereview.chromium.org/7233006/diff/1012/chrome/browser/policy/cros_u...
chrome/browser/policy/cros_user_policy_cache.cc:421: base::Unretained(this)));
What's the relation of the signature check in this file and the signature check
in session_manager?

[Mattias Nissler (ping if slow), 2011-07-20 16:09:07]

Hi guys, can you look at this now? It turns out we now have a secure place to
store policy in, so maybe we can get the user policy security stuff for M14
after all.

[gfeher, 2011-07-21 12:24:22]

Looks pretty good. I think I can give you the L*TM soon if you explain me the
magic you did to fix the build.

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/login_library.h (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/login_library.h:25: virtual void
RequestRetrievePolicy(RetrievePolicyCallback callback,
Is this about device policies? Then please rename to
RequestRetrieveDevicePolicy.

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/login_library.h:31: virtual void
RequestStorePolicy(const std::string& policy,
Same here.

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:25:
"/home/chronos/user/.enterprise/key";
How about moving this to chrome/common/chrome_paths_???.cc?

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:230: class
CrosUserPolicyCache::RetrievePolicyOperation {
I took a look at DevicePolicyCache and it seems like it uses
SignedSettingsHelper to retrieve policies. Wouldn't it be possible (and make
sense) to use that here as well?

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:68: // loaded token and device ID
to the identity strategy.
Nit: identity strategy -> data store

[Mattias Nissler (ping if slow), 2011-07-21 14:20:25]

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/login_library.h (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/login_library.h:25: virtual void
RequestRetrievePolicy(RetrievePolicyCallback callback,
On 2011/07/21 12:24:23, gfeher wrote:
> Is this about device policies? Then please rename to
> RequestRetrieveDevicePolicy.

You are right in that changing the names would be nicer from the Chrome
perspective. However, then the naming would be out of sync with what libcros and
DBus uses. Changing there is more complicated, due to the multiple repositories
involved. So at this point you can pick: Change here and have an inconsistency
or not change and live with the slightly inaccurate naming. Which do you choose?
:)

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/login_library.h:31: virtual void
RequestStorePolicy(const std::string& policy,
On 2011/07/21 12:24:23, gfeher wrote:
> Same here.

Same reply.

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:25:
"/home/chronos/user/.enterprise/key";
On 2011/07/21 12:24:23, gfeher wrote:
> How about moving this to chrome/common/chrome_paths_???.cc?

Done.

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:230: class
CrosUserPolicyCache::RetrievePolicyOperation {
On 2011/07/21 12:24:23, gfeher wrote:
> I took a look at DevicePolicyCache and it seems like it uses
> SignedSettingsHelper to retrieve policies. Wouldn't it be possible (and make
> sense) to use that here as well?

I thought about that but rejected that idea for several reasons:
- We call a different set of functions, so we'd have to duplicate stuff in
SignedSettingsHelper
- We're not actually handling owner data
- The signature checking in SignedSettings uses a different key, which doesn't
fit well with how the signature checking is done in SignedSettings
- SignedSettings is desperately needing a rewrite, adding unrelated stuff to it
will make it only harder.

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:68: // loaded token and device ID
to the identity strategy.
On 2011/07/21 12:24:23, gfeher wrote:
> Nit: identity strategy -> data store

Done.

[Mattias Nissler (ping if slow), 2011-07-21 15:10:40]

On 2011/07/21 14:20:25, Mattias Nissler wrote:
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
> File chrome/browser/chromeos/cros/login_library.h (right):
> 
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros/login_library.h:25: virtual void
> RequestRetrievePolicy(RetrievePolicyCallback callback,
> On 2011/07/21 12:24:23, gfeher wrote:
> > Is this about device policies? Then please rename to
> > RequestRetrieveDevicePolicy.
> 
> You are right in that changing the names would be nicer from the Chrome
> perspective. However, then the naming would be out of sync with what libcros
and
> DBus uses. Changing there is more complicated, due to the multiple
repositories
> involved. So at this point you can pick: Change here and have an inconsistency
> or not change and live with the slightly inaccurate naming. Which do you
choose?
> :)
> 
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros/login_library.h:31: virtual void
> RequestStorePolicy(const std::string& policy,
> On 2011/07/21 12:24:23, gfeher wrote:
> > Same here.
> 
> Same reply.
> 
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
> File chrome/browser/policy/cros_user_policy_cache.cc (right):
> 
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
> chrome/browser/policy/cros_user_policy_cache.cc:25:
> "/home/chronos/user/.enterprise/key";
> On 2011/07/21 12:24:23, gfeher wrote:
> > How about moving this to chrome/common/chrome_paths_???.cc?
> 
> Done.
> 
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
> chrome/browser/policy/cros_user_policy_cache.cc:230: class
> CrosUserPolicyCache::RetrievePolicyOperation {
> On 2011/07/21 12:24:23, gfeher wrote:
> > I took a look at DevicePolicyCache and it seems like it uses
> > SignedSettingsHelper to retrieve policies. Wouldn't it be possible (and make
> > sense) to use that here as well?
> 
> I thought about that but rejected that idea for several reasons:
> - We call a different set of functions, so we'd have to duplicate stuff in
> SignedSettingsHelper
> - We're not actually handling owner data
> - The signature checking in SignedSettings uses a different key, which doesn't
> fit well with how the signature checking is done in SignedSettings
> - SignedSettings is desperately needing a rewrite, adding unrelated stuff to
it
> will make it only harder.
> 
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
> File chrome/browser/policy/cros_user_policy_cache.h (right):
> 
>
http://codereview.chromium.org/7233006/diff/14001/chrome/browser/policy/cros_...
> chrome/browser/policy/cros_user_policy_cache.h:68: // loaded token and device
ID
> to the identity strategy.
> On 2011/07/21 12:24:23, gfeher wrote:
> > Nit: identity strategy -> data store
> 
> Done.

new version is up.

[gfeher, 2011-07-21 23:37:14]

LGTM
live long and prosper

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/login_library.h (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/login_library.h:25: virtual void
RequestRetrievePolicy(RetrievePolicyCallback callback,
My choice: leave it like this for now and do the full rename on the day when all
our bigger problems become resolved.

Okay, wait: please add the word "device" to the comments.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:172: // Cancels the operation,
making sure that any pending callbacks get killed.
This doesn't really cancel the operation, just kills, the callbacks, right?

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:242: // Cancels the operation,
disengaging all pending callbacks.
Same here.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:406:
policy_data.set_timestamp((now - base::Time::UnixEpoch()).InMilliseconds());
I don't think it makes sense to stuff that into this CL, but maybe we should
have functions somewhere for the time-conversion operations that are used widely
in the policy code.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:26: class
CrosUserPolicyIdentityStrategy;
Obsolete, please remove.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:68: // loaded token and device ID
to the identity strategy.
Nit: identity strategy -> data store

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:107: 
Nit: extra newline.

[Mattias Nissler (ping if slow), 2011-07-22 11:29:47]

So long, and thanks for all the fish!

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/login_library.h (right):

http://codereview.chromium.org/7233006/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/login_library.h:25: virtual void
RequestRetrievePolicy(RetrievePolicyCallback callback,
On 2011/07/21 23:37:14, gfeher wrote:
> My choice: leave it like this for now and do the full rename on the day when
all
> our bigger problems become resolved.
> 
> Okay, wait: please add the word "device" to the comments.

Done.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:172: // Cancels the operation,
making sure that any pending callbacks get killed.
On 2011/07/21 23:37:15, gfeher wrote:
> This doesn't really cancel the operation, just kills, the callbacks, right?

Done.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:242: // Cancels the operation,
disengaging all pending callbacks.
On 2011/07/21 23:37:15, gfeher wrote:
> Same here.

Done.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:406:
policy_data.set_timestamp((now - base::Time::UnixEpoch()).InMilliseconds());
On 2011/07/21 23:37:15, gfeher wrote:
> I don't think it makes sense to stuff that into this CL, but maybe we should
> have functions somewhere for the time-conversion operations that are used
widely
> in the policy code.

Makes sense :) I'll keep it in mind when doing more refactoring.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:26: class
CrosUserPolicyIdentityStrategy;
On 2011/07/21 23:37:15, gfeher wrote:
> Obsolete, please remove.

Done.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:68: // loaded token and device ID
to the identity strategy.
On 2011/07/21 23:37:15, gfeher wrote:
> Nit: identity strategy -> data store

Done.

http://codereview.chromium.org/7233006/diff/19002/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:107: 
On 2011/07/21 23:37:15, gfeher wrote:
> Nit: extra newline.

Done.

[Mattias Nissler (ping if slow), 2011-10-13 13:54:20]

Rebased and updated version, please take another look.

Chris: FYI. I dropped the signature checks since the key file is not accessible
through the file system from Chrome any longer, so we have to trust whatever
reply we get from session_manager through DBus. I think for user policy that is
acceptable, just wanted to make sure you're aware.

[pastarmovj, 2011-10-13 14:42:13]

LGTM. 
I concentrated mainly on the changes since the last LGTM and they seem ok to me.

[Chris Masone, 2011-10-13 17:25:56]

On 2011/10/13 13:54:20, Mattias Nissler wrote:
> Rebased and updated version, please take another look.
> 
> Chris: FYI. I dropped the signature checks since the key file is not
accessible
> through the file system from Chrome any longer, so we have to trust whatever
> reply we get from session_manager through DBus. I think for user policy that
is
> acceptable, just wanted to make sure you're aware.

That doesn't sound acceptable to me, off the top of my head.  What are the
security guarantees we're trying to provide for these policies?

[Mattias Nissler (ping if slow), 2011-10-13 17:35:15]

On Thu, Oct 13, 2011 at 7:25 PM, <cmasone@chromium.org> wrote:

> On 2011/10/13 13:54:20, Mattias Nissler wrote:
>
>> Rebased and updated version, please take another look.
>>
>
>  Chris: FYI. I dropped the signature checks since the key file is not
>>
> accessible
>
>> through the file system from Chrome any longer, so we have to trust
>> whatever
>> reply we get from session_manager through DBus. I think for user policy
>> that
>>
> is
>
>> acceptable, just wanted to make sure you're aware.
>>
>
> That doesn't sound acceptable to me, off the top of my head.  What are the
> security guarantees we're trying to provide for these policies?


To make sure we're on the same page, let me recap the situation: Currently,
they're stored in a chronos-owned file that Chrome reads and writes, and
there are no signature checks at all, so there is no protection against
filesystem exploits. The code I implemented sends the policy blobs to
session_manager, which keeps the policy and corresponding key per user in
the infamous root-owned cryptohome location. session_manager also does
signature checks to make sure the policy blob is good.

The problem is that when we ship policy from session_manager to Chrome via
DBus, Chrome cannot re-check the policy signature because it doesn't know
the key to check against (the root-owned location is unfortunately not
accessible to chronos, so we cannot read the key file. Maybe we need to fix
that?). Asking session_manager for the key via DBus isn't good either, since
then we could just trust that the policy blob we receive via DBus is good.

[Chris Masone, 2011-10-13 17:37:22]

On Thu, Oct 13, 2011 at 10:34 AM, Mattias Nissler <mnissler@chromium.org>wrote:

>
>
> On Thu, Oct 13, 2011 at 7:25 PM, <cmasone@chromium.org> wrote:
>
>> On 2011/10/13 13:54:20, Mattias Nissler wrote:
>>
>>> Rebased and updated version, please take another look.
>>>
>>
>>  Chris: FYI. I dropped the signature checks since the key file is not
>>>
>> accessible
>>
>>> through the file system from Chrome any longer, so we have to trust
>>> whatever
>>> reply we get from session_manager through DBus. I think for user policy
>>> that
>>>
>> is
>>
>>> acceptable, just wanted to make sure you're aware.
>>>
>>
>> That doesn't sound acceptable to me, off the top of my head.  What are the
>> security guarantees we're trying to provide for these policies?
>
>
> To make sure we're on the same page, let me recap the situation: Currently,
> they're stored in a chronos-owned file that Chrome reads and writes, and
> there are no signature checks at all, so there is no protection against
> filesystem exploits. The code I implemented sends the policy blobs to
> session_manager, which keeps the policy and corresponding key per user in
> the infamous root-owned cryptohome location. session_manager also does
> signature checks to make sure the policy blob is good.
>

Thanks for the recap.  What, though, are the guarantees we want to be
providing here?  That's what I don't know, exactly.


>
> The problem is that when we ship policy from session_manager to Chrome via
> DBus, Chrome cannot re-check the policy signature because it doesn't know
> the key to check against (the root-owned location is unfortunately not
> accessible to chronos, so we cannot read the key file. Maybe we need to fix
> that?). Asking session_manager for the key via DBus isn't good either, since
> then we could just trust that the policy blob we receive via DBus is good.
>

[Mattias Nissler (ping if slow), 2012-04-04 19:39:58]

Guys, can you take another look? I'd like to land this really soon now :)

Regarding Chris' question from the last review, the security intention here is
to not store user policy in plain files where it can be accessed and modified by
everybody with a filesystem exploit.

[Chris Masone, 2012-04-04 20:33:17]

This issue is closed, and the CL has compile failures, it looks like...what's up
here?

[Mattias Nissler (ping if slow), 2012-04-04 20:51:55]

On 2012/04/04 20:33:17, Chris Masone wrote:
> This issue is closed, and the CL has compile failures, it looks like...what's
up
> here?

I closed it accidentally and the tryjob failures are due to me failing to
include the prerequisite CL https://chromiumcodereview.appspot.com/9961014/ in
the tryjob. Sorry for that.

[Chris Masone, 2012-04-04 22:24:40]

On 2012/04/04 20:51:55, Mattias Nissler wrote:
> On 2012/04/04 20:33:17, Chris Masone wrote:
> > This issue is closed, and the CL has compile failures, it looks
like...what's
> up
> > here?
> 
> I closed it accidentally and the tryjob failures are due to me failing to
> include the prerequisite CL https://chromiumcodereview.appspot.com/9961014/ in
> the tryjob. Sorry for that.

Ok.  Well, it LGTM as long as you can get the tryjob issue cleared up

[pastarmovj, 2012-04-05 09:56:21]

LGTM with a few nits.

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:65: // static
The constructor is static?

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:113: explicit
RetrievePolicyOperation(const ResultCallback& callback);
Is this one needed/implemented?

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:127: // static
Ditto.

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:335:
policy_data.username().empty() ||
Shouldn't those lines aline with the ( of the if?

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:110: // the number of pre-M18
clients drops back to zero.
I guess you'll need to update this to Pre-M20 here.

[Mattias Nissler (ping if slow), 2012-04-05 10:26:04]

Thanks for the reviews. I uploaded a new version that fixes the nits and the
failing tests, adding Nikita for chrome/browser/chromeos/login/OWNERS

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.cc (right):

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:65: // static
On 2012/04/05 09:56:21, pastarmovj wrote:
> The constructor is static?

Done.

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:113: explicit
RetrievePolicyOperation(const ResultCallback& callback);
On 2012/04/05 09:56:21, pastarmovj wrote:
> Is this one needed/implemented?

No. Done.

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:127: // static
On 2012/04/05 09:56:21, pastarmovj wrote:
> Ditto.

Done.

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.cc:335:
policy_data.username().empty() ||
On 2012/04/05 09:56:21, pastarmovj wrote:
> Shouldn't those lines aline with the ( of the if?

Absolutely. Done.

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
File chrome/browser/policy/cros_user_policy_cache.h (right):

http://codereview.chromium.org/7233006/diff/52001/chrome/browser/policy/cros_...
chrome/browser/policy/cros_user_policy_cache.h:110: // the number of pre-M18
clients drops back to zero.
On 2012/04/05 09:56:21, pastarmovj wrote:
> I guess you'll need to update this to Pre-M20 here.

Done.

[Nikita (slow), 2012-04-05 11:30:27]

chromeos/login lgtm

[commit-bot: I haz the power, 2012-04-13 14:34:09]

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

[commit-bot: I haz the power, 2012-04-13 15:58:30]

Change committed as 132195