Device enterprise registration with a certificate.

DM server client flow to register a device with an enrollment (a.k.a. registration) certificate.

See go/zero-touch-chrome for design. (The author hasn't updated the name of the protos, but the document explains the signed request.)

BUG=624187
TEST=unit tests

Committed: https://crrev.com/26a5d631d5bc4f18e604d9ea9dc12c2dc5990437
Cr-Commit-Position: refs/heads/master@{#414236}

[The one and only Dr. Crash, 2016-08-19 20:40:49]

Description was changed from

==========
Initial registration with a certificate.


Added RegisterWithCertificate().


Initial prototype.


DM server API: enrollment with a registration certificate.

Brings up the protos to the same set of messages as what is server
side for DM server.

BUG=624187
TEST=none
==========

to

==========
Initial registration with a certificate.


Added RegisterWithCertificate().


Initial prototype.


DM server API: enrollment with a registration certificate.

Brings up the protos to the same set of messages as what is server
side for DM server.

BUG=624187
TEST=none
==========

[The one and only Dr. Crash, 2016-08-19 20:40:49]

drcrash@chromium.org changed reviewers:
+ bartfab@chromium.org, pastarmovj@chromium.org

[The one and only Dr. Crash, 2016-08-20 04:31:44]

Description was changed from

==========
Initial registration with a certificate.


Added RegisterWithCertificate().


Initial prototype.


DM server API: enrollment with a registration certificate.

Brings up the protos to the same set of messages as what is server
side for DM server.

BUG=624187
TEST=none
==========

to

==========
DM server client flow to register a device with an enrollment (a.k.a.
registration) certificate.

BUG=624187
TEST=unit tests
==========

[The one and only Dr. Crash, 2016-08-20 04:35:15]

drcrash@chromium.org changed reviewers:
+ achuith@chromium.org

[The one and only Dr. Crash, 2016-08-20 15:42:28]

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

[commit-bot: I haz the power, 2016-08-20 15:42:38]

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

[commit-bot: I haz the power, 2016-08-20 15:44:27]

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

[commit-bot: I haz the power, 2016-08-20 15:44:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[The one and only Dr. Crash, 2016-08-20 15:52:52]

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

[commit-bot: I haz the power, 2016-08-20 15:52:57]

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

[commit-bot: I haz the power, 2016-08-20 15:55:26]

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

[commit-bot: I haz the power, 2016-08-20 15:55:27]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[The one and only Dr. Crash, 2016-08-20 16:09:44]

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

[commit-bot: I haz the power, 2016-08-20 16:09:52]

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

[commit-bot: I haz the power, 2016-08-20 16:12:55]

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

[commit-bot: I haz the power, 2016-08-20 16:12:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[The one and only Dr. Crash, 2016-08-21 00:29:07]

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

[commit-bot: I haz the power, 2016-08-21 00:29:19]

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

[commit-bot: I haz the power, 2016-08-21 01:18:17]

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

[commit-bot: I haz the power, 2016-08-21 01:18:18]

Dry run: This issue passed the CQ dry run.

[The one and only Dr. Crash, 2016-08-21 23:59:48]

Description was changed from

==========
DM server client flow to register a device with an enrollment (a.k.a.
registration) certificate.

BUG=624187
TEST=unit tests
==========

to

==========
DM server client flow to register a device with an enrollment (a.k.a.
registration) certificate.

See go/zero-touch-chrome for design. (The author hasn't updated the name of the
protos, but the document explains the signed request.)

BUG=624187
TEST=unit tests
==========

[The one and only Dr. Crash, 2016-08-22 00:34:01]

drcrash@chromium.org changed reviewers:
+ dkrahn@chromium.org

[The one and only Dr. Crash, 2016-08-22 00:34:23]

+dkrahn for AttestationFlow.

[The one and only Dr. Crash, 2016-08-22 14:42:40]

The CQ bit was checked by drcrash@chromium.org

[The one and only Dr. Crash, 2016-08-22 14:42:40]

The CQ bit was unchecked by drcrash@chromium.org

[pastarmovj, 2016-08-22 15:09:43]

mostly nits.

https://codereview.chromium.org/2261763002/diff/120001/chromeos/attestation/a...
File chromeos/attestation/attestation_flow.h (right):

https://codereview.chromium.org/2261763002/diff/120001/chromeos/attestation/a...
chromeos/attestation/attestation_flow.h:60: static AttestationKeyType
GetKeyTypeForProfile(
If those are becoming public now, please document them.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:169: if
(!signing_service_) {
Can you explain why is this not a DCHECK or CHECK even? Shouldn't it be the
caller that needs to know beforehand that he can do cert based registration or
not?

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:204: if (success) {
nit: maybe reverse this like if (!success) { error; return; } ...
this way the bulk of the code will not be indented :)

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:91: SigningCallback;
nit: please add a new line between the two.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:92: virtual void
SignData(const std::string& data,
Please document this function so that whoever needs to implement it know what to
do.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:101:
CloudPolicyClient(
Just thinking aloud here... could the signing service be specified by a setter
instead of in the constructor? It doesn't seem like it is needed at construction
time and most instantiations doesn't need it it seems?

https://codereview.chromium.org/2261763002/diff/120001/components/policy/prot...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2261763002/diff/120001/components/policy/prot...
components/policy/proto/device_management_backend.proto:18: // (e.g. a nonce to
avoid proxy attacks of the signing service).
out of curiosity, can you explain this comment to me?

[The one and only Dr. Crash, 2016-08-22 16:00:02]

https://codereview.chromium.org/2261763002/diff/120001/chromeos/attestation/a...
File chromeos/attestation/attestation_flow.h (right):

https://codereview.chromium.org/2261763002/diff/120001/chromeos/attestation/a...
chromeos/attestation/attestation_flow.h:60: static AttestationKeyType
GetKeyTypeForProfile(
On 2016/08/22 15:09:43, pastarmovj wrote:
> If those are becoming public now, please document them.

Done.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:169: if
(!signing_service_) {
On 2016/08/22 15:09:43, pastarmovj wrote:
> Can you explain why is this not a DCHECK or CHECK even? Shouldn't it be the
> caller that needs to know beforehand that he can do cert based registration or
> not?

I started with a DCHECK, but thought that since there typically is a backup
enrollment method, return an error that can be recovered from is friendlier.

This being said I am fine either way and if a DCHECK fits better with the rest
of the code I'll revert to it.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:204: if (success) {
On 2016/08/22 15:09:43, pastarmovj wrote:
> nit: maybe reverse this like if (!success) { error; return; } ...
> this way the bulk of the code will not be indented :)

Done.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:91: SigningCallback;
On 2016/08/22 15:09:43, pastarmovj wrote:
> nit: please add a new line between the two.

Done.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:92: virtual void
SignData(const std::string& data,
On 2016/08/22 15:09:43, pastarmovj wrote:
> Please document this function so that whoever needs to implement it know what
to
> do.

Done.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:101:
CloudPolicyClient(
On 2016/08/22 15:09:43, pastarmovj wrote:
> Just thinking aloud here... could the signing service be specified by a setter
> instead of in the constructor? It doesn't seem like it is needed at
construction
> time and most instantiations doesn't need it it seems?

I think the constructor patterns fits the code style better (I did consider both
a setter and passing it to the new API), and we expect to authenticate more
methods like this over time.

https://codereview.chromium.org/2261763002/diff/120001/components/policy/prot...
File components/policy/proto/device_management_backend.proto (right):

https://codereview.chromium.org/2261763002/diff/120001/components/policy/prot...
components/policy/proto/device_management_backend.proto:18: // (e.g. a nonce to
avoid proxy attacks of the signing service).
On 2016/08/22 15:09:43, pastarmovj wrote:
> out of curiosity, can you explain this comment to me?

The signing code adds a nonce at the end of the data before it signs it
(https://cs.corp.google.com/chromeos_public/src/platform2/cryptohome/attestati...).
This is to avoid proxy attacks where someone could get another service to sign
something and pretend it was the one doing the signing. Because of TPM 2.0 the
size of the nonce is now variable
(https://cs.corp.google.com/chromeos_public/src/platform2/cryptohome/attestati...)
so it cannot be hardcoded in the server anymore as it has been up to now. so we
compute it and pass it. (It could be passed by the attestation code eventually
if dkrahn agrees to that idea).

[pastarmovj, 2016-08-23 08:46:02]

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/2261763002/diff/120001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:169: if
(!signing_service_) {
On 2016/08/22 16:00:01, The one and only Dr. Crash wrote:
> On 2016/08/22 15:09:43, pastarmovj wrote:
> > Can you explain why is this not a DCHECK or CHECK even? Shouldn't it be the
> > caller that needs to know beforehand that he can do cert based registration
or
> > not?
> 
> I started with a DCHECK, but thought that since there typically is a backup
> enrollment method, return an error that can be recovered from is friendlier.
> 
> This being said I am fine either way and if a DCHECK fits better with the rest
> of the code I'll revert to it.

+Daren, what do you think?

https://codereview.chromium.org/2261763002/diff/140001/chromeos/attestation/a...
File chromeos/attestation/attestation_flow.h (right):

https://codereview.chromium.org/2261763002/diff/140001/chromeos/attestation/a...
chromeos/attestation/attestation_flow.h:63: //   certificate_profile - Specifies
what kind of certificate the key is for.
nit: s/certificate_profile/profile/...or rename the argument.

https://codereview.chromium.org/2261763002/diff/140001/chromeos/attestation/a...
chromeos/attestation/attestation_flow.h:66: // Returns the name of the key for a
given certificate profile. The
Add a new line before this one.

[pastarmovj, 2016-08-23 15:03:43]

lgtm for policy files.

[The one and only Dr. Crash, 2016-08-23 15:05:10]

https://codereview.chromium.org/2261763002/diff/140001/chromeos/attestation/a...
File chromeos/attestation/attestation_flow.h (right):

https://codereview.chromium.org/2261763002/diff/140001/chromeos/attestation/a...
chromeos/attestation/attestation_flow.h:63: //   certificate_profile - Specifies
what kind of certificate the key is for.
On 2016/08/23 08:46:02, pastarmovj wrote:
> nit: s/certificate_profile/profile/...or rename the argument.

Done.

https://codereview.chromium.org/2261763002/diff/140001/chromeos/attestation/a...
chromeos/attestation/attestation_flow.h:66: // Returns the name of the key for a
given certificate profile. The
On 2016/08/23 08:46:02, pastarmovj wrote:
> Add a new line before this one.

Done.

[The one and only Dr. Crash, 2016-08-23 15:16:51]

+dkrahn, could you have a look at attestation_flow.h and attestation_flow.cc?
(Exposed the GetKey* methods since we need to request signature with the key of
the registration certificate.)

+achuith, could you have a look at the CL?

[achuithb, 2016-08-23 18:40:28]

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:61:
device_mode_(DEVICE_MODE_NOT_SET),
Move this to header

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:73:
CloudPolicyClient::CloudPolicyClient(
Get rid of this

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:107: if
(client_id.empty()) {
use ternary operator instead

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:193:
base::Unretained(this)));
Why base::Unretained? Couldn't you use a weak ptr instead? Here and elsewhere

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:199:
em::DeviceManagementResponse response;
const

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:203:
policy_fetch_request_job_.reset(
newline before this

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:89: typedef
base::Callback<void(bool success,
use using here instead.
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:95: SigningCallback
callback) = 0;
pass const reference instead.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:111:
CloudPolicyClient(
we don't prefer multiple ctors. Please just use the other one and change call
sites to use signing_service=nullptr

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client_unittest.cc
(right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:86: // A
mock SigningService
period at the end of comment

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:91: bool
success = !ShouldSignDataFail();
const

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:92: if
(success) {
drop {}

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:97: static
void SignRegistrationData(
statics first

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:107:
private:
newline before this line

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:108: static
void SignData(const std::string& data, em::SignedData* signed_data) {
Use a different name - we don't allow overloaded function names.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:144: //
TODO(drcrash): Use FLAVOR_ATTESTATION after 2186623002 has landed.
reference bug

[The one and only Dr. Crash, 2016-08-24 05:53:33]

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

[The one and only Dr. Crash, 2016-08-24 05:53:45]

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:61:
device_mode_(DEVICE_MODE_NOT_SET),
On 2016/08/23 18:40:28, achuithb wrote:
> Move this to header

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:73:
CloudPolicyClient::CloudPolicyClient(
On 2016/08/23 18:40:28, achuithb wrote:
> Get rid of this

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:107: if
(client_id.empty()) {
On 2016/08/23 18:40:28, achuithb wrote:
> use ternary operator instead

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:193:
base::Unretained(this)));
On 2016/08/23 18:40:28, achuithb wrote:
> Why base::Unretained? Couldn't you use a weak ptr instead? Here and elsewhere

Again, that's me matching the style of the file.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:199:
em::DeviceManagementResponse response;
On 2016/08/23 18:40:28, achuithb wrote:
> const

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:203:
policy_fetch_request_job_.reset(
On 2016/08/23 18:40:28, achuithb wrote:
> newline before this

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:89: typedef
base::Callback<void(bool success,
On 2016/08/23 18:40:28, achuithb wrote:
> use using here instead.
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:95: SigningCallback
callback) = 0;
On 2016/08/23 18:40:28, achuithb wrote:
> pass const reference instead.

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:111:
CloudPolicyClient(
On 2016/08/23 18:40:28, achuithb wrote:
> we don't prefer multiple ctors. Please just use the other one and change call
> sites to use signing_service=nullptr

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client_unittest.cc
(right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:86: // A
mock SigningService
On 2016/08/23 18:40:28, achuithb wrote:
> period at the end of comment

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:91: bool
success = !ShouldSignDataFail();
On 2016/08/23 18:40:28, achuithb wrote:
> const

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:92: if
(success) {
On 2016/08/23 18:40:28, achuithb wrote:
> drop {}

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:97: static
void SignRegistrationData(
On 2016/08/23 18:40:28, achuithb wrote:
> statics first

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:107:
private:
On 2016/08/23 18:40:28, achuithb wrote:
> newline before this line

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:108: static
void SignData(const std::string& data, em::SignedData* signed_data) {
On 2016/08/23 18:40:28, achuithb wrote:
> Use a different name - we don't allow overloaded function names.

Done.

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client_unittest.cc:144: //
TODO(drcrash): Use FLAVOR_ATTESTATION after 2186623002 has landed.
On 2016/08/23 18:40:28, achuithb wrote:
> reference bug

Made it link to CL.

[commit-bot: I haz the power, 2016-08-24 05:54:04]

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

[commit-bot: I haz the power, 2016-08-24 05:56:50]

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

[commit-bot: I haz the power, 2016-08-24 05:56:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[achuithb, 2016-08-24 06:05:36]

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:193:
base::Unretained(this)));
On 2016/08/24 05:53:44, The one and only Dr. Crash wrote:
> On 2016/08/23 18:40:28, achuithb wrote:
> > Why base::Unretained? Couldn't you use a weak ptr instead? Here and
elsewhere
> 
> Again, that's me matching the style of the file.

Could you please switch to using weak_ptr_factory? base::Unretained can cause
access to freed memory during shutdown which is a security problem.

[The one and only Dr. Crash, 2016-08-24 08:19:16]

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/2261763002/diff/160001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.cc:193:
base::Unretained(this)));
On 2016/08/24 06:05:36, achuithb wrote:
> On 2016/08/24 05:53:44, The one and only Dr. Crash wrote:
> > On 2016/08/23 18:40:28, achuithb wrote:
> > > Why base::Unretained? Couldn't you use a weak ptr instead? Here and
> elsewhere
> > 
> > Again, that's me matching the style of the file.
> 
> Could you please switch to using weak_ptr_factory? base::Unretained can cause
> access to freed memory during shutdown which is a security problem.

Sure.

[The one and only Dr. Crash, 2016-08-24 08:27:11]

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

[commit-bot: I haz the power, 2016-08-24 08:27:26]

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

[commit-bot: I haz the power, 2016-08-24 09:28:54]

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

[commit-bot: I haz the power, 2016-08-24 09:28:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[The one and only Dr. Crash, 2016-08-24 15:57:42]

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

[commit-bot: I haz the power, 2016-08-24 15:58:00]

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

[commit-bot: I haz the power, 2016-08-24 18:14:59]

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

[commit-bot: I haz the power, 2016-08-24 18:15:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Darren Krahn, 2016-08-24 19:48:14]

On 2016/08/24 18:30:09, The one and only Dr. Crash wrote:

lgm for chromeos/attestation

[Darren Krahn, 2016-08-24 19:48:41]

On 2016/08/24 19:48:14, Darren Krahn wrote:
> On 2016/08/24 18:30:09, The one and only Dr. Crash wrote:
> 
> lgm for chromeos/attestation

that is -- lgtm for chromeos/attestation

[achuithb, 2016-08-24 21:46:18]

lgtm

https://codereview.chromium.org/2261763002/diff/240001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/2261763002/diff/240001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:409:
DeviceManagementService* service_;
= nullptr

https://codereview.chromium.org/2261763002/diff/240001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:412: SigningService*
signing_service_;
= nullptr;

[The one and only Dr. Crash, 2016-08-24 22:17:52]

https://codereview.chromium.org/2261763002/diff/240001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/2261763002/diff/240001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:409:
DeviceManagementService* service_;
On 2016/08/24 21:46:18, achuithb wrote:
> = nullptr

Done.

https://codereview.chromium.org/2261763002/diff/240001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_client.h:412: SigningService*
signing_service_;
On 2016/08/24 21:46:17, achuithb wrote:
> = nullptr;

Done.

[The one and only Dr. Crash, 2016-08-24 22:18:15]

The CQ bit was checked by drcrash@chromium.org

[The one and only Dr. Crash, 2016-08-24 22:18:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pastarmovj@chromium.org, achuith@chromium.org, dkrahn@chromium.org
Link to the patchset: https://codereview.chromium.org/2261763002/#ps260001
(title: "Initialize pointers to nullptr.")

[commit-bot: I haz the power, 2016-08-24 22:19:35]

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

[commit-bot: I haz the power, 2016-08-24 23:30:47]

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

[commit-bot: I haz the power, 2016-08-24 23:30:48]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[The one and only Dr. Crash, 2016-08-24 23:34:06]

The CQ bit was checked by drcrash@chromium.org

[commit-bot: I haz the power, 2016-08-24 23:35:32]

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

[commit-bot: I haz the power, 2016-08-25 00:58:45]

Description was changed from

==========
DM server client flow to register a device with an enrollment (a.k.a.
registration) certificate.

See go/zero-touch-chrome for design. (The author hasn't updated the name of the
protos, but the document explains the signed request.)

BUG=624187
TEST=unit tests
==========

to

==========
DM server client flow to register a device with an enrollment (a.k.a.
registration) certificate.

See go/zero-touch-chrome for design. (The author hasn't updated the name of the
protos, but the document explains the signed request.)

BUG=624187
TEST=unit tests
==========

[commit-bot: I haz the power, 2016-08-25 00:58:46]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-25 01:01:28]

Description was changed from

==========
DM server client flow to register a device with an enrollment (a.k.a.
registration) certificate.

See go/zero-touch-chrome for design. (The author hasn't updated the name of the
protos, but the document explains the signed request.)

BUG=624187
TEST=unit tests
==========

to

==========
DM server client flow to register a device with an enrollment (a.k.a.
registration) certificate.

See go/zero-touch-chrome for design. (The author hasn't updated the name of the
protos, but the document explains the signed request.)

BUG=624187
TEST=unit tests

Committed: https://crrev.com/26a5d631d5bc4f18e604d9ea9dc12c2dc5990437
Cr-Commit-Position: refs/heads/master@{#414236}
==========

[commit-bot: I haz the power, 2016-08-25 01:01:30]

Patchset 14 (id:??) landed as
https://crrev.com/26a5d631d5bc4f18e604d9ea9dc12c2dc5990437
Cr-Commit-Position: refs/heads/master@{#414236}