Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727713003:
Update FWMP in TPM (Closed)
DescriptionUpdate FWMP in TPM
As part of enrollment, the firmware management parameters (FWMP)
partition from TPM has to be set including the flags to mark if the
devmode is blocked. The update has to be done before the TPM is locked
but after the policy is retrieved.
It is implemented by including additional step in enrollment process
that makes the D-Bus call to cryptohome to set the data in FWMP.
Similarly when the device is deprovisioned, the firmware management
parameters are removed from TPM when it is established that it is a
consumer owned device.
BUG=685144
Review-Url: https://codereview.chromium.org/2727713003
Cr-Commit-Position: refs/heads/master@{#462886}
Committed: https://chromium.googlesource.com/chromium/src/+/d6dbbe9a3f1a62bc6ec0414bd4bbd5d4c2d9f833
Patch Set 1 #Patch Set 2 : Added unit tests #Patch Set 3 : Implementation of remove Fwmp in auto-enrollment for consumer owned device #Patch Set 4 : Test fixed #
Total comments: 48
Patch Set 5 : Fixed review comments #Patch Set 6 : Small Nits #Patch Set 7 : Nit #
Total comments: 14
Patch Set 8 : Fixed review comments #
Total comments: 29
Patch Set 9 : Fixed review comments #Patch Set 10 : Nit #
Total comments: 8
Patch Set 11 : Fixed review comments #Patch Set 12 : Fixed review comments #Patch Set 13 : Nit #Patch Set 14 : Fixed reenrollment case #
Total comments: 6
Patch Set 15 : Nits #Patch Set 16 : Nit #
Total comments: 2
Patch Set 17 : Nits #
Total comments: 6
Patch Set 18 : Nits #Messages
Total messages: 51 (23 generated)
Description was changed from ========== Set FWMP in TPM on enrollment As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark is the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. BUG=685144 ========== to ========== Set FWMP in TPM on enrollment As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. BUG=685144 ==========
Description was changed from ========== Set FWMP in TPM on enrollment As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. BUG=685144 ========== to ========== Update FWMP in TPM As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. Similarly when the device is deprovisioned, the firmware management parameters are removed from TPM when it is established that it is a consumer owned device. BUG=685144 ==========
igorcov@chromium.org changed reviewers: + atwilson@chromium.org, tnagel@chromium.org
PTAL.
derat@chromium.org changed reviewers: + derat@chromium.org
https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:307: DCHECK(state_ == policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT); use DCHECK_EQ so the actual value is logged on failure https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:309: std::unique_ptr<chromeos::InstallAttributes> install_attributes = nit: auto (since the type is obvious from the MakeUnique call) https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:20: nit: i think we usually omit these blank lines when forward-declaring something in a namespace, e.g. namespace cryptohome { class BaseReply; } // namespace cryptohome https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:95: // D-Bus call to cryptohome to remove the firmware management parameters from nit: "Makes a D-Bus call ..." https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:97: void RemoveFwmp(); mind also renaming this to RemoveFirmwareManagementParameters? https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:99: // Callback after the firmware management parameters is removed. nit: s/is/are/, but maybe just: // Callback for RemoveFirmwareManagementParameters(). instead? https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:100: void OnFwmpRemoved(chromeos::DBusMethodCallStatus call_status, ditto here https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:401: DCHECK_EQ(STEP_SET_FWMP_DATA, enrollment_step_); nit: i think the order used for DCHECK_EQ is typically (actual, expected), e.g. DCHECK_EQ(enrollment_step_, STEP_SET_FWMP_DATA); https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:412: DCHECK_EQ(STEP_SET_FWMP_DATA, enrollment_step_); nit: same here https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:417: // get devmode and set the data. nit: s/get/Get/ https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:424: std::unique_ptr<em::PolicyData> policy_data(new em::PolicyData()); nit: use auto and MakeUnique https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:425: policy_data->ParseFromString(policy_->policy_data()); shouldn't you be checking the return value here to make sure it was parsed correctly before using it? https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:148: void SetFwmpData(); nit: mind expanding the acronym in these names too? https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/install_attributes.h (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/settings/install_attributes.h:81: void RemoveFwmpInTpm( nit: same comment about expanding acronym here https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/install_attributes_unittest.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/settings/install_attributes_unittest.cc:312: loop.Run(); nit: just do: base::RunLoop().Run(); ? https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.cc:932: dbus::MethodCall method_call(cryptohome::kCryptohomeInterface, method_name); it looks like a bunch of these methods are all copy-and-pasted versions of each other with different method names. would you mind moving this to an internal helper method that takes a method name, MessageLite, and callback so that there isn't so much duplication? https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.h:41: } nit: add // namespace protobuf https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.h:564: virtual void AsyncTpmUpdateFirmwareManagementParameters( add a comment documenting what this does. can you also drop "Async" from the name? some of the async methods in this class have that prefix while others don't. i think that the callback param makes it pretty clear that this is async, so a shorter name is probably better. https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.h:565: const std::string& method_name, i think that this leaks too many of the IPC implementation details to the caller. please add separate methods for updating and removing that take the concrete protobuf type instead of this generic version.
https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:299: RemoveFwmp(); Document that RemoveFwmp() will handle calling the various callbacks and stopping safeguard_timer_ since it's not documented that these are side effects. What happens if the safeguard_timer_ triggers while we are in the middle of removing FWMP? https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:307: DCHECK(state_ == policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT); DCHECK_EQ https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:310: base::MakeUnique<chromeos::InstallAttributes>( This is something Thiemo should look at - I don't know the right pattern for using InstallAttributes (are we really supposed to free it right after calling?) so I can't check if it's OK. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:314: client_start_weak_factory_.GetWeakPtr())); Why do we only call progress callbacks and stop the safeguard timer in OnFwmpRemoved, instead of here? We should explain this somewhere. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:322: LOG(ERROR) << "Failed to remove firmware management parameters"; Any other info we can log here (does it return a more detailed error?) https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:20: nit: this is OK as-is, but typically I avoid blank lines in namespace blocks https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:99: // Callback after the firmware management parameters is removed. nit: is->are
https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:299: RemoveFwmp(); On 2017/03/07 11:16:30, Andrew T Wilson (Slow) wrote: > Document that RemoveFwmp() will handle calling the various callbacks and > stopping safeguard_timer_ since it's not documented that these are side effects. > What happens if the safeguard_timer_ triggers while we are in the middle of > removing FWMP? It would possibly request to remove FWMP twice, which is not nice. I've fixed now the behavior to catch this case. Thanks. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:307: DCHECK(state_ == policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT); On 2017/03/06 21:18:25, Daniel Erat wrote: > use DCHECK_EQ so the actual value is logged on failure Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:310: base::MakeUnique<chromeos::InstallAttributes>( On 2017/03/07 11:16:30, Andrew T Wilson (Slow) wrote: > This is something Thiemo should look at - I don't know the right pattern for > using InstallAttributes (are we really supposed to free it right after calling?) > so I can't check if it's OK. I've changed this to use directly the CryptohomeClient. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:314: client_start_weak_factory_.GetWeakPtr())); On 2017/03/07 11:16:30, Andrew T Wilson (Slow) wrote: > Why do we only call progress callbacks and stop the safeguard timer in > OnFwmpRemoved, instead of here? We should explain this somewhere. Included in the comments of RemoveFirmwareManagementParameters() function. Thanks. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:322: LOG(ERROR) << "Failed to remove firmware management parameters"; On 2017/03/07 11:16:30, Andrew T Wilson (Slow) wrote: > Any other info we can log here (does it return a more detailed error?) Added the reply error. We can't use much from call_status as that is a two value enum (success/fail). Hopefully reply error has more details, but normally cryptohomed logs details about the failed operation. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:20: On 2017/03/06 21:18:25, Daniel Erat wrote: > nit: i think we usually omit these blank lines when forward-declaring something > in a namespace, e.g. > > namespace cryptohome { > class BaseReply; > } // namespace cryptohome Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:95: // D-Bus call to cryptohome to remove the firmware management parameters from On 2017/03/06 21:18:25, Daniel Erat wrote: > nit: "Makes a D-Bus call ..." Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:97: void RemoveFwmp(); On 2017/03/06 21:18:25, Daniel Erat wrote: > mind also renaming this to RemoveFirmwareManagementParameters? Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:99: // Callback after the firmware management parameters is removed. On 2017/03/06 21:18:25, Daniel Erat wrote: > nit: s/is/are/, but maybe just: > > // Callback for RemoveFirmwareManagementParameters(). > > instead? Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:100: void OnFwmpRemoved(chromeos::DBusMethodCallStatus call_status, On 2017/03/06 21:18:25, Daniel Erat wrote: > ditto here Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:401: DCHECK_EQ(STEP_SET_FWMP_DATA, enrollment_step_); On 2017/03/06 21:18:25, Daniel Erat wrote: > nit: i think the order used for DCHECK_EQ is typically (actual, expected), e.g. > > DCHECK_EQ(enrollment_step_, STEP_SET_FWMP_DATA); This would make my code inconsistent with the rest of the code from this class. Why would DCHECK_EQ be different from the standard we use in EXPECT_EQ from unit tests? (https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?dr=C...) https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:417: // get devmode and set the data. On 2017/03/06 21:18:25, Daniel Erat wrote: > nit: s/get/Get/ Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:424: std::unique_ptr<em::PolicyData> policy_data(new em::PolicyData()); On 2017/03/06 21:18:25, Daniel Erat wrote: > nit: use auto and MakeUnique Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:425: policy_data->ParseFromString(policy_->policy_data()); On 2017/03/06 21:18:26, Daniel Erat wrote: > shouldn't you be checking the return value here to make sure it was parsed > correctly before using it? Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:148: void SetFwmpData(); On 2017/03/06 21:18:26, Daniel Erat wrote: > nit: mind expanding the acronym in these names too? Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/install_attributes.h (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/settings/install_attributes.h:81: void RemoveFwmpInTpm( On 2017/03/06 21:18:26, Daniel Erat wrote: > nit: same comment about expanding acronym here Done. https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/install_attributes_unittest.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/settings/install_attributes_unittest.cc:312: loop.Run(); On 2017/03/06 21:18:26, Daniel Erat wrote: > nit: just do: > > base::RunLoop().Run(); > > ? Done. https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.cc:932: dbus::MethodCall method_call(cryptohome::kCryptohomeInterface, method_name); On 2017/03/06 21:18:26, Daniel Erat wrote: > it looks like a bunch of these methods are all copy-and-pasted versions of each > other with different method names. would you mind moving this to an internal > helper method that takes a method name, MessageLite, and callback so that there > isn't so much duplication? Done, thanks for suggestion. https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.h:41: } On 2017/03/06 21:18:26, Daniel Erat wrote: > nit: add // namespace protobuf Done. https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.h:564: virtual void AsyncTpmUpdateFirmwareManagementParameters( On 2017/03/06 21:18:26, Daniel Erat wrote: > add a comment documenting what this does. can you also drop "Async" from the > name? some of the async methods in this class have that prefix while others > don't. i think that the callback param makes it pretty clear that this is async, > so a shorter name is probably better. Done. https://codereview.chromium.org/2727713003/diff/60001/chromeos/dbus/cryptohom... chromeos/dbus/cryptohome_client.h:565: const std::string& method_name, On 2017/03/06 21:18:26, Daniel Erat wrote: > i think that this leaks too many of the IPC implementation details to the > caller. please add separate methods for updating and removing that take the > concrete protobuf type instead of this generic version. Done.
https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:401: DCHECK_EQ(STEP_SET_FWMP_DATA, enrollment_step_); On 2017/03/09 12:22:57, igorcov wrote: > On 2017/03/06 21:18:25, Daniel Erat wrote: > > nit: i think the order used for DCHECK_EQ is typically (actual, expected), > e.g. > > > > DCHECK_EQ(enrollment_step_, STEP_SET_FWMP_DATA); > > This would make my code inconsistent with the rest of the code from this class. > Why would DCHECK_EQ be different from the standard we use in EXPECT_EQ from > unit tests? > (https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?dr=C...) because they come from completely different places. :-/ DCHECK_EQ is from chrome's base/logging.h, while EXPECT_EQ and ASSERT_EQ are from googletest, which is modeled on junit. leaving it like this is fine if this file already lists things in the opposite order, though.
thanks for cleaning up the existing code! looks fine to me with a few more comments, but i'll let others more familiar with this code approve it. https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:150: // If a client is being created or already existing, bail out. i'm not familiar with this code, but can you update the CL description to mention why this is being moved earlier in the method? is it to fix a bug? https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:325: LOG(ERROR) << "Failed to remove firmware management parameters, error: " the style guide requires curly brackets around multi-line if/else bodies https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:427: bool block_devmode = false; nit: move this down to just above the block that sets it instead of declaring it at the top of the method (see https://google.github.io/styleguide/cppguide.html#Local_Variables) https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:442: const em::SystemSettingsProto& container(payload->system_settings()); nit: i think it's more common to use ' = ' when initializing shorthand references like this https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/install_attributes.h (right): https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/settings/install_attributes.h:73: // Updates the firmware management parameters from TPM, storring the devmode nit: "storing" https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:27: class RemoveFirmwareManagementParametersRequest; keep this list alphabetized https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/fake_cry... File chromeos/dbus/fake_cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/fake_cry... chromeos/dbus/fake_cryptohome_client.h:198: nit: delete the blank line here and below so all the overridden methods are grouped together
Description was changed from ========== Update FWMP in TPM As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. Similarly when the device is deprovisioned, the firmware management parameters are removed from TPM when it is established that it is a consumer owned device. BUG=685144 ========== to ========== Update FWMP in TPM As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. Similarly when the device is deprovisioned, the firmware management parameters are removed from TPM when it is established that it is a consumer owned device. The safeguard timer from AutoEnrollmentController, that takes care to timeout when the enrollment checks take too long, includes now also the time required for FWMP to be removed. This is the reason, its initialization is moved up in the code, so that it covers that time. BUG=685144 ==========
https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:150: // If a client is being created or already existing, bail out. On 2017/03/09 22:15:22, Daniel Erat wrote: > i'm not familiar with this code, but can you update the CL description to > mention why this is being moved earlier in the method? is it to fix a bug? Updated the CL description. Basically this change is done so that safeguard_timer_ is started before we possibly trigger the remove FWMP operation. https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:325: LOG(ERROR) << "Failed to remove firmware management parameters, error: " On 2017/03/09 22:15:22, Daniel Erat wrote: > the style guide requires curly brackets around multi-line if/else bodies Done. https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:427: bool block_devmode = false; On 2017/03/09 22:15:22, Daniel Erat wrote: > nit: move this down to just above the block that sets it instead of declaring it > at the top of the method (see > https://google.github.io/styleguide/cppguide.html#Local_Variables) Done. https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:442: const em::SystemSettingsProto& container(payload->system_settings()); On 2017/03/09 22:15:22, Daniel Erat wrote: > nit: i think it's more common to use ' = ' when initializing shorthand > references like this Done. https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/settings/install_attributes.h (right): https://codereview.chromium.org/2727713003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/settings/install_attributes.h:73: // Updates the firmware management parameters from TPM, storring the devmode On 2017/03/09 22:15:22, Daniel Erat wrote: > nit: "storing" Done. https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:27: class RemoveFirmwareManagementParametersRequest; On 2017/03/09 22:15:23, Daniel Erat wrote: > keep this list alphabetized Done. https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/fake_cry... File chromeos/dbus/fake_cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/120001/chromeos/dbus/fake_cry... chromeos/dbus/fake_cryptohome_client.h:198: On 2017/03/09 22:15:23, Daniel Erat wrote: > nit: delete the blank line here and below so all the overridden methods are > grouped together Done.
https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:301: return; Please don't mix return and break in the same switch statement as this is easily overlooked when reading the code. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:317: client_start_weak_factory_.GetWeakPtr())); Have you checked the side effects of using client_start_weak_factory_? https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:329: if (safeguard_timer_.IsRunning()) { This condition is always true because Timeout() invalidates the weak pointers. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:343: state_ = policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT; It seems unclean to update the state manually. Maybe it would be cleaner to cancel the timeout before running the FWMP update? Are you sure we need that safeguard timer to cover FWMP update? Is that not covered by a D-Bus timeout? (Please try to simplify the code. It's become so complex that it's hard to convince oneself of its correctness.) https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:352: safeguard_timer_.Stop(); What's the point in stopping the timer here? Isn't it stopped already when Timeout() is called? https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:101: void RemoveFirmwareManagementParameters(); Nit: I'd suggest to add "Start" to the beginning of the method name to make clear that this is non-blocking. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:427: auto policy_data = base::MakeUnique<em::PolicyData>(); Please add a CHECK or a DCHECK that policy_ is not unset. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:433: std::unique_ptr<em::ChromeDeviceSettingsProto> payload( What's the purpose of the unique_ptr here? I think it can be dropped. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:118: STEP_SET_FWMP_DATA = 8, // Setting the firmware management parameters. Why not immediately before LOCK_DEVICE? It would seem more logical to have the TMP-related steps next to each other... https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:147: // according to devmode. "according to devmode" seems unclear to me. Please clarify. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:156: // Returns whether block_devmode is set. Can be invoked after the policy is s/Can be/Must only be/ s/is retrieved/has been retrieved/ https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:158: bool GetBlockDevmode(); Nit: I'd suggest to make clearer that this is based on the policy. Either by renaming to something like GetBlockDevmodeFromPolicy() or maybe even better by making it a free function (anonymous namespace) and passing in the PolicyFetchResponse as a parameter. https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.cc:1186: // Makes an asynchronous D-Bus call, using cryptohome interface. Nit: Please wrap comments at 80 characters. https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:564: virtual void RemoveFirmwareManagementParametersInTpm( Nit: FromTpm would probably be better language. https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:565: const cryptohome::RemoveFirmwareManagementParametersRequest& request, The request parameter seems to serve no purpose. Can we drop it?
https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:301: return; On 2017/03/23 17:23:52, Thiemo Nagel wrote: > Please don't mix return and break in the same switch statement as this is easily > overlooked when reading the code. Done. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:343: state_ = policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT; On 2017/03/23 17:23:52, Thiemo Nagel wrote: > It seems unclean to update the state manually. Maybe it would be cleaner to > cancel the timeout before running the FWMP update? Are you sure we need that > safeguard timer to cover FWMP update? Is that not covered by a D-Bus timeout? > > (Please try to simplify the code. It's become so complex that it's hard to > convince oneself of its correctness.) I've removed the FWMP update operation from being covered by safeguard timeout, as it has its own D-Bus timeout indeed. This allowed for code to be simplified, thanks. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:101: void RemoveFirmwareManagementParameters(); On 2017/03/23 17:23:52, Thiemo Nagel wrote: > Nit: I'd suggest to add "Start" to the beginning of the method name to make > clear that this is non-blocking. Done. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:427: auto policy_data = base::MakeUnique<em::PolicyData>(); On 2017/03/23 17:23:52, Thiemo Nagel wrote: > Please add a CHECK or a DCHECK that policy_ is not unset. Done. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:433: std::unique_ptr<em::ChromeDeviceSettingsProto> payload( On 2017/03/23 17:23:52, Thiemo Nagel wrote: > What's the purpose of the unique_ptr here? I think it can be dropped. Done. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.h (right): https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:118: STEP_SET_FWMP_DATA = 8, // Setting the firmware management parameters. On 2017/03/23 17:23:52, Thiemo Nagel wrote: > Why not immediately before LOCK_DEVICE? It would seem more logical to have the > TMP-related steps next to each other... Done. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:147: // according to devmode. On 2017/03/23 17:23:52, Thiemo Nagel wrote: > "according to devmode" seems unclear to me. Please clarify. Done. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:156: // Returns whether block_devmode is set. Can be invoked after the policy is On 2017/03/23 17:23:52, Thiemo Nagel wrote: > s/Can be/Must only be/ > s/is retrieved/has been retrieved/ Done. https://codereview.chromium.org/2727713003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:158: bool GetBlockDevmode(); On 2017/03/23 17:23:52, Thiemo Nagel wrote: > Nit: I'd suggest to make clearer that this is based on the policy. Either by > renaming to something like GetBlockDevmodeFromPolicy() or maybe even better by > making it a free function (anonymous namespace) and passing in the > PolicyFetchResponse as a parameter. Done. https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.cc:1186: // Makes an asynchronous D-Bus call, using cryptohome interface. On 2017/03/23 17:23:52, Thiemo Nagel wrote: > Nit: Please wrap comments at 80 characters. This is formatted with arguments to the function explained separately. https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:564: virtual void RemoveFirmwareManagementParametersInTpm( On 2017/03/23 17:23:52, Thiemo Nagel wrote: > Nit: FromTpm would probably be better language. Done. https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:565: const cryptohome::RemoveFirmwareManagementParametersRequest& request, On 2017/03/23 17:23:52, Thiemo Nagel wrote: > The request parameter seems to serve no purpose. Can we drop it? It is used, being passed to cryptohome_proxy for CallMethod. Do you suggest creating it inside this function? It would look different from other functions and also at risk of changing back to this state in case some parameter inside the |request| will be added in the future.
https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.cc:1186: // Makes an asynchronous D-Bus call, using cryptohome interface. > This is formatted with arguments to the function explained separately. True, but this does not match the prevalent style of function comments. Cf. cryptohome_client.h for examples. https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:324: base::Unretained(this))); base::Unretained() seems unsafe. Are you sure this usage is correct? https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:80: // Returns whether block_devmode is set. Must only be invoked after the policy Nit: The 2nd sentence is obsolete now. https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:83: enterprise_management::PolicyFetchResponse* policy) { Nit: const
https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/140001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.cc:1186: // Makes an asynchronous D-Bus call, using cryptohome interface. On 2017/03/27 17:21:47, Thiemo Nagel wrote: > > This is formatted with arguments to the function explained separately. > > True, but this does not match the prevalent style of function comments. Cf. > cryptohome_client.h for examples. Done. https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:324: base::Unretained(this))); On 2017/03/27 17:21:47, Thiemo Nagel wrote: > base::Unretained() seems unsafe. Are you sure this usage is correct? Not sure if it counts as "check", but safeguard_timer_ used it since forever. And the reason it was introduced is the fact that the flow is blocked until progress_callbacks_ are notified. I've checked in the code, this object lives inside login_display_host_impl and there's a chain leading from notification being sent to the impl_ object but it is linked through some weak_ptr in binds, so I couldn't find a way to prove it theoretically. https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right): https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:80: // Returns whether block_devmode is set. Must only be invoked after the policy On 2017/03/27 17:21:47, Thiemo Nagel wrote: > Nit: The 2nd sentence is obsolete now. Done. https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:83: enterprise_management::PolicyFetchResponse* policy) { On 2017/03/27 17:21:47, Thiemo Nagel wrote: > Nit: const Done.
https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:324: base::Unretained(this))); I think unless specified in the contract for the class (i.e. the class-level comment), it should be possible to destroy the class at any time without causing crashes due to pending callbacks. Thus imho the use of Unretained for safeguard_timer_ was wrong from the start. As a quick fix, you could add a (second) weak_ptr_factory_. In the long run, I'd like to merge the two, but that's maybe best done in a separate CL.
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Update FWMP in TPM As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. Similarly when the device is deprovisioned, the firmware management parameters are removed from TPM when it is established that it is a consumer owned device. The safeguard timer from AutoEnrollmentController, that takes care to timeout when the enrollment checks take too long, includes now also the time required for FWMP to be removed. This is the reason, its initialization is moved up in the code, so that it covers that time. BUG=685144 ========== to ========== Update FWMP in TPM As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. Similarly when the device is deprovisioned, the firmware management parameters are removed from TPM when it is established that it is a consumer owned device. BUG=685144 ==========
The CQ bit was unchecked by igorcov@chromium.org
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:324: base::Unretained(this))); On 2017/03/29 10:47:21, Thiemo Nagel wrote: > I think unless specified in the contract for the class (i.e. the class-level > comment), it should be possible to destroy the class at any time without causing > crashes due to pending callbacks. Thus imho the use of Unretained for > safeguard_timer_ was wrong from the start. > > As a quick fix, you could add a (second) weak_ptr_factory_. In the long run, > I'd like to merge the two, but that's maybe best done in a separate CL. Done.
Lgtm. https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:142: weak_ptr_factory_(this) {} Nit: While you're here, I'd suggest to move initialization to the .h file. (E.g. weak_ptr_factory_{this};) https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:277: client_.reset(new policy::AutoEnrollmentClient( Nit: While you're here, I'd suggest to replace "new" by MakeUnqiue<> for a more modern style. https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:132: base::WeakPtrFactory<AutoEnrollmentController> client_start_weak_factory_; Nit: Add TODO() to merge the two factories.
https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:142: weak_ptr_factory_(this) {} On 2017/03/29 15:46:20, Thiemo Nagel wrote: > Nit: While you're here, I'd suggest to move initialization to the .h file. > (E.g. weak_ptr_factory_{this};) Done. https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:277: client_.reset(new policy::AutoEnrollmentClient( On 2017/03/29 15:46:20, Thiemo Nagel wrote: > Nit: While you're here, I'd suggest to replace "new" by MakeUnqiue<> for a more > modern style. Done. https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h (right): https://codereview.chromium.org/2727713003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h:132: base::WeakPtrFactory<AutoEnrollmentController> client_start_weak_factory_; On 2017/03/29 15:46:20, Thiemo Nagel wrote: > Nit: Add TODO() to merge the two factories. Done.
One more nit... https://codereview.chromium.org/2727713003/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:140: safeguard_timer_(false, false) {} Nit: For consistency, I'd suggest to move the other two initializations inside the class definition as well.
https://codereview.chromium.org/2727713003/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc (right): https://codereview.chromium.org/2727713003/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:140: safeguard_timer_(false, false) {} On 2017/03/29 16:09:40, Thiemo Nagel wrote: > Nit: For consistency, I'd suggest to move the other two initializations inside > the class definition as well. Done.
derat@ Could you please check again, and lgtm if all good in chromeos/dbus/*? Thanks,
//chromeos/dbus lgtm with a few nits. thanks again for cleaning up the repetitive code. https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:586: // RemoveFirmwareManagementParameters removes the firmware management nit: probably don't need this sentence; the method name already makes it very clear that this Removes Firmware Management Parameters From TPM. :-) https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/fake_cry... File chromeos/dbus/fake_cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/fake_cry... chromeos/dbus/fake_cryptohome_client.cc:591: cryptohome::BaseReply reply; nit: if you're not planning to set things in the reply later, just inline this: ReturnProtobufMethodCallback(cryptohome::BaseReply(), callback); https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/fake_cry... chromeos/dbus/fake_cryptohome_client.cc:598: cryptohome::BaseReply reply; nit: same here
Still lgtm.
https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/cryptoho... File chromeos/dbus/cryptohome_client.h (right): https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/cryptoho... chromeos/dbus/cryptohome_client.h:586: // RemoveFirmwareManagementParameters removes the firmware management On 2017/03/29 16:50:08, Daniel Erat wrote: > nit: probably don't need this sentence; the method name already makes it very > clear that this Removes Firmware Management Parameters From TPM. :-) Done. https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/fake_cry... File chromeos/dbus/fake_cryptohome_client.cc (right): https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/fake_cry... chromeos/dbus/fake_cryptohome_client.cc:591: cryptohome::BaseReply reply; On 2017/03/29 16:50:08, Daniel Erat wrote: > nit: if you're not planning to set things in the reply later, just inline this: > > ReturnProtobufMethodCallback(cryptohome::BaseReply(), callback); Done. https://codereview.chromium.org/2727713003/diff/320001/chromeos/dbus/fake_cry... chromeos/dbus/fake_cryptohome_client.cc:598: cryptohome::BaseReply reply; On 2017/03/29 16:50:08, Daniel Erat wrote: > nit: same here Done.
After offline conversation: I don't think the requires_tpm_update_ flag is necessary. In your local tests GetMode() returned a different value than it would in an official build and that has skewed your conclusions.
Patchset #20 (id:380001) has been deleted
Patchset #19 (id:360001) has been deleted
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2727713003/#ps340001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/04/07 13:38:46, Thiemo Nagel wrote: > After offline conversation: I don't think the requires_tpm_update_ flag is > necessary. In your local tests GetMode() returned a different value than it > would in an official build and that has skewed your conclusions. You're right, I've tested again simulating GetMove() as official version and it works as expected. Thanks.
PS #18 still lgtm.
The CQ bit was checked by igorcov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1491581221695790,
"parent_rev": "77a23bfb8de76d6678851aed78b008986fb4edbc", "commit_rev":
"d6dbbe9a3f1a62bc6ec0414bd4bbd5d4c2d9f833"}
Message was sent while issue was closed.
Description was changed from ========== Update FWMP in TPM As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. Similarly when the device is deprovisioned, the firmware management parameters are removed from TPM when it is established that it is a consumer owned device. BUG=685144 ========== to ========== Update FWMP in TPM As part of enrollment, the firmware management parameters (FWMP) partition from TPM has to be set including the flags to mark if the devmode is blocked. The update has to be done before the TPM is locked but after the policy is retrieved. It is implemented by including additional step in enrollment process that makes the D-Bus call to cryptohome to set the data in FWMP. Similarly when the device is deprovisioned, the firmware management parameters are removed from TPM when it is established that it is a consumer owned device. BUG=685144 Review-Url: https://codereview.chromium.org/2727713003 Cr-Commit-Position: refs/heads/master@{#462886} Committed: https://chromium.googlesource.com/chromium/src/+/d6dbbe9a3f1a62bc6ec0414bd4bb... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/d6dbbe9a3f1a62bc6ec0414bd4bb... |
