New protocol and testserver for the Chrome-DMServer protocol

New protocol and testserver for the Chrome-DMServer protocol

New features:
-Message to ping DMServer and ask if a user is managed
-Signed policy responses
-Server assigns names to ChromeOS devices
A temporary version of cloud_policies.proto is also checked in (it will be auto-generated later).

BUG=chromium-os:11253, chromium-os:11254
TEST=none

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

[gfeher, 2011-01-12 16:22:41]

Hi Folks,
This is my current understanding of what we want for the next DMServer<->Chrome
protocol.
G

[danno, 2011-01-13 12:09:53]

here you go...

http://codereview.chromium.org/6161007/diff/1004/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/6161007/diff/1004/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:51: message
NewDevicePolicySetting {
s/NewDevicePolicy/DevicePolicySettings everywhere in this file.

http://codereview.chromium.org/6161007/diff/1004/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:51: message
NewDevicePolicySetting {
For consistency, move this right before NewDevicePolicyRequest.

http://codereview.chromium.org/6161007/diff/1004/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:55: optional string
policy_value = 2;
maybe "serialized_policy_proto"?

http://codereview.chromium.org/6161007/diff/1004/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_backend.proto:132: // identify key
to the settings: proxy etc.
Add the policy scope here, too.

[Mattias Nissler (ping if slow), 2011-01-25 11:05:57]

A few comments.

http://codereview.chromium.org/6161007/diff/25001/net/tools/testserver/device...
File net/tools/testserver/device_management.py (right):

http://codereview.chromium.org/6161007/diff/25001/net/tools/testserver/device...
net/tools/testserver/device_management.py:41:
logging.basicConfig(filename=LOG_FILENAME,level=logging.DEBUG)
I guess these two lines are not meant for committing?

http://codereview.chromium.org/6161007/diff/25001/net/tools/testserver/device...
net/tools/testserver/device_management.py:287: cloud_response.signature = 'TODO'
should have some actual signing here :) Maybe pass the certificate in via a
command line option? Also, we need pass the certificate back to the client.

[gfeher, 2011-01-27 15:39:50]

How about landing this?

cloud_policy.proto is a generated file, Jakob's CL  generates it. I can either
land this with a dummy cloud_policy.proto or rebase against his CL, depending on
which one of us gets there earlier.

Please review.

[Mattias Nissler (ping if slow), 2011-01-28 10:29:34]

looks pretty good, just some nits/minor comments.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
File chrome/browser/policy/proto/cloud_policy.proto (right):

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/cloud_policy.proto:23: optional PolicyMode
policy_mode = 1;
As discussed, let's make this a message wrapping meta data.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:13: // Protocol
buffers for the obsolete protocol:
Which buffers do you mean?

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:138: optional string
policy_scope = 1;
Need an additional field for the context's device token. When requesting user
policy, we'll also specify the device's token, in order to enable the server to
make decisions such as only-push-policy-to-managed-devices.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:145: // Signature of
the above data.
We should also mention that the signature algorithm is SHA1-RSA.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:147: // The chain of
DER-encoded X.509 certificates of the server's signing key.
As discussed, specify order here.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:151: // The
following three are necessary against replay attacks.
actually, it's the following two.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
File net/tools/testserver/device_management.py (right):

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:155:
self._headers.getheader('Authorization', ''))
Don't need that regex check any longer, no?

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:314:
group_message.__setattr__(field.name, field_value)
Could this also handle string lists? In other words, is the special casing for
repeated strings above really needed?

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:335: pass
What's this for?

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:345: recommended = True
can't you just use a PolicyMode enum directly?

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1528: 'used for signing.')
If you reverse the order, please update this comment.

[Jakob Kummerow, 2011-01-28 10:45:52]

A few more nits from me :-)

As discussed, the biggest issue I see is that the path to cloud_policy.proto
will change once we include automated generation of that file in the build
system. But we can fix that when that time comes.

http://codereview.chromium.org/6161007/diff/58001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6161007/diff/58001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2033:
'<(protoc_out_dir)/chrome/browser/policy/proto/cloud_policy.pb.cc',
I think you don't need these two lines.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
File net/tools/testserver/device_management.py (right):

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:320: signs it and constructs the
repsonse.
nit: response

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:340: # Indiactes if at least one field
was set in |group_message|.
nit: Indicates

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:367: signed_response.timestamp =
calendar.timegm(time.gmtime())
you can remove 'import calendar' and just use 'signed_response.timestamp =
int(time.time())' here.

[gfeher, 2011-01-28 13:42:09]

Thanks for the comments!
I've addressed them and did some extra cleanups.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
File chrome/browser/policy/proto/cloud_policy.proto (right):

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/cloud_policy.proto:23: optional PolicyMode
policy_mode = 1;
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> As discussed, let's make this a message wrapping meta data. 

Done.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:13: // Protocol
buffers for the obsolete protocol:
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> Which buffers do you mean?

Done.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:138: optional string
policy_scope = 1;
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> Need an additional field for the context's device token. When requesting user
> policy, we'll also specify the device's token, in order to enable the server
to
> make decisions such as only-push-policy-to-managed-devices.

Done.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:145: // Signature of
the above data.
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> We should also mention that the signature algorithm is SHA1-RSA.

Done.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:147: // The chain of
DER-encoded X.509 certificates of the server's signing key.
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> As discussed, specify order here.

Done.

http://codereview.chromium.org/6161007/diff/58001/chrome/browser/policy/proto...
chrome/browser/policy/proto/device_management_backend.proto:151: // The
following three are necessary against replay attacks.
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> actually, it's the following two.

Done.

http://codereview.chromium.org/6161007/diff/58001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6161007/diff/58001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2033:
'<(protoc_out_dir)/chrome/browser/policy/proto/cloud_policy.pb.cc',
I do, because device_management_backend.proto includes cloud_policy.proto, and
this code is required when linking the chrome binary.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
File net/tools/testserver/device_management.py (right):

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:155:
self._headers.getheader('Authorization', ''))
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> Don't need that regex check any longer, no?

Done.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:314:
group_message.__setattr__(field.name, field_value)
I get this error when I try to use __setattr__ to explicitly set a list to a
repeated list:

AttributeError: Assignment not allowed to repeated field "DisabledPlugins" in
protocol message object.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:320: signs it and constructs the
repsonse.
On 2011/01/28 10:45:52, jkummerow wrote:
> nit: response

Done.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:335: pass
Forgotten stuff -> I'll add support for device and user policy distinction.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:340: # Indiactes if at least one field
was set in |group_message|.
On 2011/01/28 10:45:52, jkummerow wrote:
> nit: Indicates

Done.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:345: recommended = True
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> can't you just use a PolicyMode enum directly?

Done.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/device...
net/tools/testserver/device_management.py:367: signed_response.timestamp =
calendar.timegm(time.gmtime())
On 2011/01/28 10:45:52, jkummerow wrote:
> you can remove 'import calendar' and just use 'signed_response.timestamp =
> int(time.time())' here.

Done.

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/6161007/diff/58001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1528: 'used for signing.')
On 2011/01/28 10:29:34, Mattias Nissler wrote:
> If you reverse the order, please update this comment.

Done.

[Mattias Nissler (ping if slow), 2011-01-28 14:00:28]

LGTM!

[Jakob Kummerow, 2011-01-28 14:22:36]

LGTM