Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
233423002:
Don't upload extension IDs in the cloud policy protocol. (Closed)
|
Created:
6 years, 8 months ago by Joao da Silva Modified:
6 years, 7 months ago Reviewers:
Mattias Nissler (ping if slow) CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDon't upload extension IDs in the cloud policy protocol.
The cloud policy protocol can also be used to fetch policy for extensions; this change makes an update to the protocol such that extension IDs are never uploaded to the server.
Note that this feature is still disabled by default behind the --enable-component-cloud-policy flag.
BUG=361156
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266857
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix chromeos clang build #
Total comments: 14
Patch Set 4 : addressed comments #Patch Set 5 : send a PolicyFetchRequest for extensions #
Total comments: 7
Patch Set 6 : addressed comments #
Total comments: 4
Patch Set 7 : fixed nits #
Messages
Total messages: 28 (0 generated)
PTAL, thanks!
Code looks good, so I'll just nitpick a bit on the test server changes :) https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/cl... File chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc (right): https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/cl... chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc:91: bool Base64Encode(const std::string& value, std::string* encoded) { side note: The name of this function is misleading, should be Base64UrlEncode https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:471: 'google/chrome/user')): Wouldn't the correct implementation be to check whether *any* of the incoming requests was for user policy? https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:476: # in the response. Regarding signatures, have you thought about what the right behavior is regarding key rotations? I suspect we should just not perform any rotations for extension policy blobs. Does it make sense to implement that here? If not - please add a comment explaining what features of ProcessCloudPolicy we may get but don't actually need. https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:1077: """Returns a list of settings entity id that have a configuration file. IDs (consistent with spelling below) https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:1084: A list of settings entity ID for the given |policy_type| that have a IDs https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:1090: return [ x[len_base_name:x.rfind('.')] for x in files ] s/x/file/
Thanks for the quick review :-) PTAL https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/cl... File chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc (right): https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/cl... chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc:91: bool Base64Encode(const std::string& value, std::string* encoded) { On 2014/04/24 12:20:36, Mattias Nissler wrote: > side note: The name of this function is misleading, should be Base64UrlEncode Done. https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:471: 'google/chrome/user')): On 2014/04/24 12:20:36, Mattias Nissler wrote: > Wouldn't the correct implementation be to check whether *any* of the incoming > requests was for user policy? I'm not sure. Since we don't send PolicyFetchRequests for extension policy, we use the signing parameters from the user PolicyFetchRequest. If there is more than one then which one triggers the fetch of policy for extensions? Alternatively, we could do this for each user PolicyFetchRequest. In that case the order of the responses will be significant. I'd rather not make any assumptions about that at this stage, since we're still in a simple world where there's 1 user PolicyFetchRequest per request :-) https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:476: # in the response. On 2014/04/24 12:20:36, Mattias Nissler wrote: > Regarding signatures, have you thought about what the right behavior is > regarding key rotations? I suspect we should just not perform any rotations for > extension policy blobs. Does it make sense to implement that here? If not - > please add a comment explaining what features of ProcessCloudPolicy we may get > but don't actually need. Correct, we don't do rotations for extension policy blobs. These blobs are verified with the same key as the main user policy blob; if that key is rotated for the user policy blob that it is automatically rotated for extension policy blobs too. For completeness, here is what happens: when the server sends down a user policy blob with a new key it should also send down the extension policy blobs signed with the new key. The component-policy code reacts when the user CloudPolicyStore is updated (indicating that the rotation was successful), and then picks up the new extension policy blobs and caches them. However, the remote policy data is not fetched again, since their hashes should still be the same. I've updated the code to just remove any fields related to key updates from these responses. https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:1077: """Returns a list of settings entity id that have a configuration file. On 2014/04/24 12:20:36, Mattias Nissler wrote: > IDs (consistent with spelling below) Done. https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:1084: A list of settings entity ID for the given |policy_type| that have a On 2014/04/24 12:20:36, Mattias Nissler wrote: > IDs Done. https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:1090: return [ x[len_base_name:x.rfind('.')] for x in files ] On 2014/04/24 12:20:36, Mattias Nissler wrote: > s/x/file/ Done.
Feel free to catch me on chat if you want to discuss more. https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:471: 'google/chrome/user')): On 2014/04/24 13:22:24, Joao da Silva wrote: > On 2014/04/24 12:20:36, Mattias Nissler wrote: > > Wouldn't the correct implementation be to check whether *any* of the incoming > > requests was for user policy? > > I'm not sure. Since we don't send PolicyFetchRequests for extension policy, we > use the signing parameters from the user PolicyFetchRequest. If there is more > than one then which one triggers the fetch of policy for extensions? > > Alternatively, we could do this for each user PolicyFetchRequest. In that case > the order of the responses will be significant. > > I'd rather not make any assumptions about that at this stage, since we're still > in a simple world where there's 1 user PolicyFetchRequest per request :-) IIUC, the only parameter you rely on from the policy fetch is the signature type, correct? Another variant here could be to send an explicit policy fetch request for google/chrome/extension/*. Doing so would accomplish to goals: (1) We signal that we actually want extension policy blobs so the server doesn't have to send them if they aren't requested and (2) we can specify the parameters for the response, such as signature type. I acknowledge that this would be another (minor) protocol change. Ignoring any other factors, do you think that's a better approach from a technical perspective? https://codereview.chromium.org/233423002/diff/40001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:476: # in the response. On 2014/04/24 13:22:24, Joao da Silva wrote: > On 2014/04/24 12:20:36, Mattias Nissler wrote: > > Regarding signatures, have you thought about what the right behavior is > > regarding key rotations? I suspect we should just not perform any rotations > for > > extension policy blobs. Does it make sense to implement that here? If not - > > please add a comment explaining what features of ProcessCloudPolicy we may get > > but don't actually need. > > Correct, we don't do rotations for extension policy blobs. These blobs are > verified with the same key as the main user policy blob; if that key is rotated > for the user policy blob that it is automatically rotated for extension policy > blobs too. > > For completeness, here is what happens: when the server sends down a user policy > blob with a new key it should also send down the extension policy blobs signed > with the new key. The component-policy code reacts when the user > CloudPolicyStore is updated (indicating that the rotation was successful), and > then picks up the new extension policy blobs and caches them. However, the > remote policy data is not fetched again, since their hashes should still be the > same. Ah, so this was a bit surprising to me - we do request user policy to be signed with the old key, but extension policy comes back in the reply with the new key. I guess that makes sense though as you should have the extension policy blobs always signed with the *current* (as determined by the server) user policy key. > > I've updated the code to just remove any fields related to key updates from > these responses.
This has been updated after the discussions from today. I've verified it works with the testserver and the current DMServer bare-bones implementation of policy for extensions. I've asked the KIR team to add similar support for public accounts policy fetches, because those will break when we enable this in the client... but that's another CL. PTAL, thanks! https://codereview.chromium.org/233423002/diff/80001/components/policy/core/c... File components/policy/core/common/cloud/component_cloud_policy_service_unittest.cc (left): https://codereview.chromium.org/233423002/diff/80001/components/policy/core/c... components/policy/core/common/cloud/component_cloud_policy_service_unittest.cc:170: // invalidations are available, or a timeout elapses). In case you wonder, this was logging "unexpected calls" and was caused by a recent change to the invalidator by Steve; it's not related to the other changes in this CL.
https://codereview.chromium.org/233423002/diff/80001/chrome/browser/policy/te... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/233423002/diff/80001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:450: if (request.policy_type not in The logic here is rather convoluted now. Can't we just remove support for the 'ping' request type here (we're not using it anyways) and then make the code do this: if one-of-the-standard-policy-types: ProcessCloudPolicy elif extension-policy: ProcessCloudPolicy for extensions else: error https://codereview.chromium.org/233423002/diff/80001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:471: # type in the response. Why not just use |request|? https://codereview.chromium.org/233423002/diff/80001/components/policy/core/c... File components/policy/core/common/cloud/component_cloud_policy_service.cc (right): https://codereview.chromium.org/233423002/diff/80001/components/policy/core/c... components/policy/core/common/cloud/component_cloud_policy_service.cc:299: for (int d = POLICY_DOMAIN_EXTENSIONS; d < POLICY_DOMAIN_SIZE; ++d) { This seems a rather convoluted way to execute the code below just for POLICY_DOMAIN_EXTENSIONS. Am I missing something? I'd rather go with a an explicit list of policy domains that you want to handle here.
Ready for another look. PTAL, thanks! https://codereview.chromium.org/233423002/diff/80001/chrome/browser/policy/te... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/233423002/diff/80001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:450: if (request.policy_type not in On 2014/04/25 11:51:46, Mattias Nissler wrote: > The logic here is rather convoluted now. Can't we just remove support for the > 'ping' request type here (we're not using it anyways) and then make the code do > this: > > if one-of-the-standard-policy-types: > ProcessCloudPolicy > elif extension-policy: > ProcessCloudPolicy for extensions > else: > error Done. https://codereview.chromium.org/233423002/diff/80001/chrome/browser/policy/te... chrome/browser/policy/test/policy_testserver.py:471: # type in the response. On 2014/04/25 11:51:46, Mattias Nissler wrote: > Why not just use |request|? Done. https://codereview.chromium.org/233423002/diff/80001/components/policy/core/c... File components/policy/core/common/cloud/component_cloud_policy_service.cc (right): https://codereview.chromium.org/233423002/diff/80001/components/policy/core/c... components/policy/core/common/cloud/component_cloud_policy_service.cc:299: for (int d = POLICY_DOMAIN_EXTENSIONS; d < POLICY_DOMAIN_SIZE; ++d) { On 2014/04/25 11:51:46, Mattias Nissler wrote: > This seems a rather convoluted way to execute the code below just for > POLICY_DOMAIN_EXTENSIONS. Am I missing something? I'd rather go with a an > explicit list of policy domains that you want to handle here. Done.
LGTM https://codereview.chromium.org/233423002/diff/100001/chrome/browser/policy/t... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/233423002/diff/100001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:639: request: the PolicyFetchRequest that triggered this handler. nit: Capitalized The after colon (also below) https://codereview.chromium.org/233423002/diff/100001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:641: PolicyFetchResponses will be appened to this message. appended
Thanks for reviewing! https://codereview.chromium.org/233423002/diff/100001/chrome/browser/policy/t... File chrome/browser/policy/test/policy_testserver.py (right): https://codereview.chromium.org/233423002/diff/100001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:639: request: the PolicyFetchRequest that triggered this handler. On 2014/04/28 12:02:48, Mattias Nissler wrote: > nit: Capitalized The after colon (also below) Done. https://codereview.chromium.org/233423002/diff/100001/chrome/browser/policy/t... chrome/browser/policy/test/policy_testserver.py:641: PolicyFetchResponses will be appened to this message. On 2014/04/28 12:02:48, Mattias Nissler wrote: > appended Done.
The CQ bit was checked by joaodasilva@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/233423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
The CQ bit was checked by joaodasilva@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/233423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
The CQ bit was checked by joaodasilva@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/233423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
The CQ bit was checked by joaodasilva@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/233423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
The CQ bit was checked by joaodasilva@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/233423002/120001
Message was sent while issue was closed.
Change committed as 266857 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
