Correctly handle device policy key rotation |
||||
Issue descriptionAFAICS from the current code, rotations of the device policy signing key won't be handled correctly. On the Chrome side, most consumers get the signing public key from DeviceSettingsService, which keeps it in the public_key_ member. The only place where this member is updated is DeviceSettingsService::HandleCompletedOperation(), and the new value is taken from SessionManagerOperation::public_key(). However, the latter will always return the same (original) key, given that the key reload may be forced only by setting force_key_load_ to true, while it's always false for StoreSettingsOperation. Moreover, the stored device policy with the rotated key will probably immediately fail the signature validation in the same StoreSettingsOperation - for the same reason as why SessionManagerOperation::public_key() will return the old value.
,
Dec 7 2016
I have a faint memory that back when I wrote this code, I did some testing to make sure key rotations worked. It's possible I'm mistaken on that though. Note that key reload is supposed to be triggered via the OwnerKeySet() signal emitted by session_manager. That triggers a reload with force_key_load = true, so at that point we should validate against a new key. Any reason why this would fail?
,
Dec 7 2016
Thanks Mattias. So eventually the key should be updated on the Chrome side. It didn't work for me in the test environment as the FakeSessionManagerClient doesn't update the key and doesn't generate the notifications. I'm working on an end-to-end test for this (actually, the whole investigation was triggered by working on tests in issue 668716). Also I think that as this separate OwnerKeySet notification happens asynchronously, with the existing code there will be some window during which the key would be still old, and the policy will fail validation (maybe with some other side effects - not sure about that).
,
Mar 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6581fa97d9ac35a9c541856a0acc04c8be2cfc20 commit 6581fa97d9ac35a9c541856a0acc04c8be2cfc20 Author: emaxx <emaxx@chromium.org> Date: Fri Mar 03 03:54:13 2017 Fix handling of device cloud signing policy key rotation This CL fixes issues with handling of the signing key rotation for the device policy. There were some issues with not loading the updated owner key right after the new policy is stored. Even though the key would eventually be loaded thanks to the OwnerKeySet signal emitted by the session manager, there may be some window during which the old key is still used on the Chrome side. Also the CL adds a browser end-to-end test for browser policy key rotation (using a local policy test server). Some fake testing-only stubs were also fixed in order to support changing the signing keys (similar to how this is handled by the session manager). BUG= 671659 ,668716 TEST=new browser test Review-Url: https://codereview.chromium.org/2558543003 Cr-Commit-Position: refs/heads/master@{#454506} [modify] https://crrev.com/6581fa97d9ac35a9c541856a0acc04c8be2cfc20/chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc [modify] https://crrev.com/6581fa97d9ac35a9c541856a0acc04c8be2cfc20/chrome/browser/chromeos/settings/session_manager_operation.cc [modify] https://crrev.com/6581fa97d9ac35a9c541856a0acc04c8be2cfc20/chromeos/BUILD.gn [modify] https://crrev.com/6581fa97d9ac35a9c541856a0acc04c8be2cfc20/chromeos/dbus/fake_session_manager_client.cc
,
Mar 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3fa1b666a54ad027289ec0c1752fcfb6d138b512 commit 3fa1b666a54ad027289ec0c1752fcfb6d138b512 Author: alph <alph@chromium.org> Date: Fri Mar 03 07:59:21 2017 Revert of Fix handling of device cloud signing policy key rotation (patchset #8 id:140001 of https://codereview.chromium.org/2558543003/ ) Reason for revert: Broke KeyRotationDeviceCloudPolicyTest.Basic test https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/34579 Original issue's description: > Fix handling of device cloud signing policy key rotation > > This CL fixes issues with handling of the signing key rotation for the > device policy. There were some issues with not loading the updated > owner key right after the new policy is stored. Even though the key > would eventually be loaded thanks to the OwnerKeySet signal emitted > by the session manager, there may be some window during which the old > key is still used on the Chrome side. > > Also the CL adds a browser end-to-end test for browser policy key > rotation (using a local policy test server). Some fake testing-only > stubs were also fixed in order to support changing the signing keys > (similar to how this is handled by the session manager). > > BUG= 671659 ,668716 > TEST=new browser test > > Review-Url: https://codereview.chromium.org/2558543003 > Cr-Commit-Position: refs/heads/master@{#454506} > Committed: https://chromium.googlesource.com/chromium/src/+/6581fa97d9ac35a9c541856a0acc04c8be2cfc20 TBR=atwilson@chromium.org,mnissler@chromium.org,stevenjb@chromium.org,emaxx@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 671659 ,668716 Review-Url: https://codereview.chromium.org/2727863004 Cr-Commit-Position: refs/heads/master@{#454541} [modify] https://crrev.com/3fa1b666a54ad027289ec0c1752fcfb6d138b512/chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc [modify] https://crrev.com/3fa1b666a54ad027289ec0c1752fcfb6d138b512/chrome/browser/chromeos/settings/session_manager_operation.cc [modify] https://crrev.com/3fa1b666a54ad027289ec0c1752fcfb6d138b512/chromeos/BUILD.gn [modify] https://crrev.com/3fa1b666a54ad027289ec0c1752fcfb6d138b512/chromeos/dbus/fake_session_manager_client.cc
,
Mar 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8aaab6cd51f40e6ca50c01423f64c49d5bbb150f commit 8aaab6cd51f40e6ca50c01423f64c49d5bbb150f Author: emaxx <emaxx@chromium.org> Date: Tue Mar 07 14:57:33 2017 Reland Fix handling of device cloud signing policy key rotation The original CL (crrev.com/2558543003) was reverted due to the failure of the added test KeyRotationDeviceCloudPolicyTest.Basic. This CL fixes this test by fixing the incorrect usage of DCHECK in the fake class for testing: the DCHECK erroneously contained an actual piece of logic. Original CL's description: > This CL fixes issues with handling of the signing key rotation for the > device policy. There were some issues with not loading the updated > owner key right after the new policy is stored. Even though the key > would eventually be loaded thanks to the OwnerKeySet signal emitted > by the session manager, there may be some window during which the old > key is still used on the Chrome side. > > Also the CL adds a browser end-to-end test for browser policy key > rotation (using a local policy test server). Some fake testing-only > stubs were also fixed in order to support changing the signing keys > (similar to how this is handled by the session manager). > > BUG= 671659 , 668716 > TEST=new browser test > > Review-Url: https://codereview.chromium.org/2558543003 > Cr-Commit-Position: refs/heads/master@{#454506} > Committed: https://chromium.googlesource.com/chromium/src/+/6581fa97d9ac35a9c541856a0acc04c8be2cfc20 BUG= 671659 , 668716 TEST=new browser test TBR=stevenjb@chromium.org, atwilson@chromium.org TBR_REASON=The CL is almost identical to the one that was already reviewed, except for fixing a small stupid bug in the test. Review-Url: https://codereview.chromium.org/2737483006 Cr-Commit-Position: refs/heads/master@{#455075} [modify] https://crrev.com/8aaab6cd51f40e6ca50c01423f64c49d5bbb150f/chrome/browser/chromeos/policy/device_cloud_policy_browsertest.cc [modify] https://crrev.com/8aaab6cd51f40e6ca50c01423f64c49d5bbb150f/chrome/browser/chromeos/settings/session_manager_operation.cc [modify] https://crrev.com/8aaab6cd51f40e6ca50c01423f64c49d5bbb150f/chromeos/BUILD.gn [modify] https://crrev.com/8aaab6cd51f40e6ca50c01423f64c49d5bbb150f/chromeos/dbus/fake_session_manager_client.cc
,
Mar 13 2017
,
May 30 2017
,
Jul 6 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by emaxx@chromium.org
, Dec 6 2016