Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2530023002:
Fix policy test server key rotation feature (Closed)
|
Created:
4 years ago by emaxx Modified:
4 years ago Reviewers:
Andrew T Wilson (Slow) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis introduces a special command line flag for the
local policy test server that enables the automatic
rotation of the signing keys with each received policy
fetch request.
The LocalPolicyTestServer class is extended to provide
ability to trigger this feature.
Also the existing documentation for the local policy test
server is corrected to correctly describe the default
behavior in which there are no cyclic key rotations.
BUG=663870
TEST=existing tests
Committed: https://crrev.com/26aa638c5ccdd27549493df2d2aff78db8fa8382
Cr-Commit-Position: refs/heads/master@{#434519}
Patch Set 1 #Patch Set 2 #Patch Set 3 #
Total comments: 4
Patch Set 4 : Rename, add comments #
Messages
Total messages: 38 (32 generated)
The CQ bit was checked by emaxx@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 checked by emaxx@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: This issue passed the CQ dry run.
The CQ bit was checked by emaxx@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 ========== [DONT REVIEW] Policy test server works BUG=663870 ========== to ========== This introduces a special command line flag for the local policy test server that enables the automatic rotation of the signing keys with each received policy fetch request. The LocalPolicyTestServer class is extended to provide ability to trigger this feature. Also the existing documentation for the local policy test server is corrected to correctly describe the default behavior in which there are no cyclic key rotations. BUG=663870 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== This introduces a special command line flag for the local policy test server that enables the automatic rotation of the signing keys with each received policy fetch request. The LocalPolicyTestServer class is extended to provide ability to trigger this feature. Also the existing documentation for the local policy test server is corrected to correctly describe the default behavior in which there are no cyclic key rotations. BUG=663870 ========== to ========== This introduces a special command line flag for the local policy test server that enables the automatic rotation of the signing keys with each received policy fetch request. The LocalPolicyTestServer class is extended to provide ability to trigger this feature. Also the existing documentation for the local policy test server is corrected to correctly describe the default behavior in which there are no cyclic key rotations. BUG=663870 TEST=existing tests ==========
The CQ bit was checked by emaxx@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 checked by emaxx@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_clobber_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 emaxx@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: This issue passed the CQ dry run.
emaxx@chromium.org changed reviewers: + atwilson@chromium.org
Drew, PTAL.
LGTM with a couple naming/other nits which you can fix or ignore at your discretion. https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... File chrome/browser/policy/test/local_policy_test_server.h (right): https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... chrome/browser/policy/test/local_policy_test_server.h:50: void SetSigningKeysAutomaticRotation(); I think "EnableAutomaticRotationOfSigningKeys()" is probably clearer than "SetXXXX". https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:1042: key specified in the config or the first key will be used for all This is fine. As we discussed, we might get slightly more robust test coverage if we rotate every time by default, but I'm OK with however you want to do it.
The CQ bit was checked by emaxx@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: This issue passed the CQ dry run.
Thanks for the review! I've fixed the nits and added a couple of more comments into the source code. https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... File chrome/browser/policy/test/local_policy_test_server.h (right): https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... chrome/browser/policy/test/local_policy_test_server.h:50: void SetSigningKeysAutomaticRotation(); On 2016/11/25 14:50:09, Andrew T Wilson (Slow) wrote: > I think "EnableAutomaticRotationOfSigningKeys()" is probably clearer than > "SetXXXX". Done. https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/2530023002/diff/40001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:1042: key specified in the config or the first key will be used for all On 2016/11/25 14:50:09, Andrew T Wilson (Slow) wrote: > This is fine. As we discussed, we might get slightly more robust test coverage > if we rotate every time by default, but I'm OK with however you want to do it. This is a valid concern. I think ideally it should be tackled from this perspective: the existing tests should test what they were intended for, while the gaps in the test coverage with regard to key rotation should be filled with new tests. I've filed a bug http://crbug.com/668716 to track this.
The CQ bit was checked by emaxx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/2530023002/#ps60001 (title: "Rename, add comments")
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": 60001, "attempt_start_ts": 1480090669041630,
"parent_rev": "3b464e482fd75e5be2cd06bba97f96a1a3cdd858", "commit_rev":
"e6498fdcf70548cf652aa08214799d96717a8cbb"}
Message was sent while issue was closed.
Description was changed from ========== This introduces a special command line flag for the local policy test server that enables the automatic rotation of the signing keys with each received policy fetch request. The LocalPolicyTestServer class is extended to provide ability to trigger this feature. Also the existing documentation for the local policy test server is corrected to correctly describe the default behavior in which there are no cyclic key rotations. BUG=663870 TEST=existing tests ========== to ========== This introduces a special command line flag for the local policy test server that enables the automatic rotation of the signing keys with each received policy fetch request. The LocalPolicyTestServer class is extended to provide ability to trigger this feature. Also the existing documentation for the local policy test server is corrected to correctly describe the default behavior in which there are no cyclic key rotations. BUG=663870 TEST=existing tests ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== This introduces a special command line flag for the local policy test server that enables the automatic rotation of the signing keys with each received policy fetch request. The LocalPolicyTestServer class is extended to provide ability to trigger this feature. Also the existing documentation for the local policy test server is corrected to correctly describe the default behavior in which there are no cyclic key rotations. BUG=663870 TEST=existing tests ========== to ========== This introduces a special command line flag for the local policy test server that enables the automatic rotation of the signing keys with each received policy fetch request. The LocalPolicyTestServer class is extended to provide ability to trigger this feature. Also the existing documentation for the local policy test server is corrected to correctly describe the default behavior in which there are no cyclic key rotations. BUG=663870 TEST=existing tests Committed: https://crrev.com/26aa638c5ccdd27549493df2d2aff78db8fa8382 Cr-Commit-Position: refs/heads/master@{#434519} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/26aa638c5ccdd27549493df2d2aff78db8fa8382 Cr-Commit-Position: refs/heads/master@{#434519} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
