New issue
Advanced search Search tips

Issue 668716 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Ensure test coverage for handling key rotation for different policy types

Project Member Reported by emaxx@chromium.org, Nov 25 2016

Issue description

As per  issue 663870 , the policy test server didn't perform key rotations previously despite its documentation said otherwise. This means that there are possible gaps in test coverage of handling the key rotations.

In particular, user, device and device-local account policy for Chrome OS and user policy for desktop have to be checked.
 

Comment 1 by emaxx@chromium.org, Nov 25 2016

Status: Assigned (was: Untriaged)

Comment 2 by emaxx@chromium.org, Dec 6 2016

Status: Started (was: Assigned)
Looks like the test coverage is not very well with regards to end-to-end testing of the whole policy stack.
The first probable issue has been discovered:  issue 671659 .
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Sign in to add a comment