Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901033002:
Host port range policy is no longer ignored in it2me host (Closed)
|
Created:
3 years, 7 months ago by Gus Smith Modified:
3 years, 6 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionHost port range policy is no longer ignored in it2me host.
Adds UpdateHostUdpPortRangePolicy into It2MeHost, which is called whenever a policy update contains a port range setting. This port range setting is then stored, and is later used to set the appropriate fields in the NetworkSettings object passed into the TransportContext.
BUG=723093
Review-Url: https://codereview.chromium.org/2901033002
Cr-Commit-Position: refs/heads/master@{#475047}
Committed: https://chromium.googlesource.com/chromium/src/+/49ccc86fa6f0112e768dda3b7dd2d78529275ca8
Patch Set 1 #
Total comments: 3
Patch Set 2 : Host port range policy is no longer ignored in it2me host #Patch Set 3 : Host port range policy is no longer ignored in it2me host #
Total comments: 10
Patch Set 4 : Host port range policy is no longer ignored in it2me host #
Total comments: 3
Patch Set 5 : Host port range policy is no longer ignored in it2me host #
Total comments: 6
Patch Set 6 : Host port range policy is no longer ignored in it2me host #
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Host port range policy is no longer ignored in it2me host BUG=723093 ========== to ========== Host port range policy is no longer ignored in it2me host This is currently a draft. BUG=723093 ==========
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
Looking more at the code I think it's possible to unittest this change without connecting a client to the host. You just need to start the host (see StartHost() in the tests), and then verify that the port range is passed to the TransportContext in ChromotingHost. That would require adding a getter in ChromotingHost (e.g. transport_context_for_tests()) and then the test will need to be able to get the ChromotingHost object from It2MeHost (you can make the test a friend for It2MeHost, see https://codesearch.chromium.org/chromium/src/remoting/host/heartbeat_sender.h... for example). https://codereview.chromium.org/2901033002/diff/1/remoting/host/it2me/it2me_h... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/1/remoting/host/it2me/it2me_h... remoting/host/it2me/it2me_host.cc:397: // TODO(gusss) i copied this; need to figure out what it actually does! It verifies that this function is called on the right thread. https://codereview.chromium.org/2901033002/diff/1/remoting/host/it2me/it2me_h... remoting/host/it2me/it2me_host.cc:403: // TODO(gusss) how to handle formatting error in this case? I think we need to stop the host in this case. I think something like this should work: SetState(kError, "Invalid RemoteAccessHostUdpPortRange policy value."); DisconnectOnNetworkThread(); https://codereview.chromium.org/2901033002/diff/1/remoting/host/it2me/it2me_h... remoting/host/it2me/it2me_host.cc:406: // TODO(gusss) do we have to disconnect? Yes, same as in UpdateClientDomainListPolicy()
The CQ bit was checked by gusss@google.com 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 gusss@google.com
The CQ bit was checked by gusss@google.com
The CQ bit was unchecked by gusss@google.com
Description was changed from ========== Host port range policy is no longer ignored in it2me host This is currently a draft. BUG=723093 ========== to ========== Host port range policy is no longer ignored in it2me host. Adds UpdateHostUdpPortRangePolicy into It2MeHost, which is called whenever a policy update contains a port range setting. This port range setting is then stored, and is later used to set the appropriate fields in the NetworkSettings object passed into the TransportContext. BUG=723093 ==========
The CQ bit was checked by gusss@google.com to run a CQ dry run
Sergey - this is ready for another review.
Sergey - this is ready for another review.
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.
https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:183: if (!PortRange::Parse(udp_port_range_string_, &port_range)) { It looks like PolicyWatcher is supposed to verify that the port_range is in correct format, so this call is never expected to fail and we can just crash the host if it fails, same as in the me2me host: https://codesearch.chromium.org/chromium/src/remoting/host/remoting_me2me_hos... Also, I think parsing belongs to UpdateHostUdpPortRangePolicy() https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:146: void UpdateHostUdpPortRangePolicy(std::string port_range_string); use const reference to pass strings (const std::string&) https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:185: std::string udp_port_range_string_; It's better to store parsed PortRange value. https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:514: TEST_F(It2MeHostTest, ConnectionValidation_HostUdpPortRangePolicy_ValidRange) { This test name is too long. I don't think you need ConnectionValidation_ prefix. This test has nothing to do with connection validation. Same for other tests https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:541: ASSERT_EQ(0, port_range.max_port); nit: ASSERT_TRUE(port_range.is_null());
https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:183: if (!PortRange::Parse(udp_port_range_string_, &port_range)) { On 2017/05/25 19:34:53, Sergey Ulanov wrote: > It looks like PolicyWatcher is supposed to verify that the port_range is in > correct format, so this call is never expected to fail and we can just crash the > host if it fails, same as in the me2me host: > https://codesearch.chromium.org/chromium/src/remoting/host/remoting_me2me_hos... > > Also, I think parsing belongs to UpdateHostUdpPortRangePolicy() OK - I'll change it back. Originally I did the parsing in UpdateHostUdpPortRangePolicy(), but I was getting errors when I'd attempt to set the state to kError after getting an incorrect parse within UpdateHostUdpPortRangePolicy(). I moved it here because I saw that the domain list was being checked here, and was potentially setting an error state; I simply emulated that. I didn't realize that the policy watcher checked this - thanks https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:146: void UpdateHostUdpPortRangePolicy(std::string port_range_string); On 2017/05/25 19:34:53, Sergey Ulanov wrote: > use const reference to pass strings (const std::string&) Done. https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:185: std::string udp_port_range_string_; On 2017/05/25 19:34:53, Sergey Ulanov wrote: > It's better to store parsed PortRange value. Done. https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:514: TEST_F(It2MeHostTest, ConnectionValidation_HostUdpPortRangePolicy_ValidRange) { On 2017/05/25 19:34:53, Sergey Ulanov wrote: > This test name is too long. I don't think you need ConnectionValidation_ prefix. > This test has nothing to do with connection validation. Same for other tests Done. https://codereview.chromium.org/2901033002/diff/40001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host_unittest.cc:541: ASSERT_EQ(0, port_range.max_port); On 2017/05/25 19:34:53, Sergey Ulanov wrote: > nit: ASSERT_TRUE(port_range.is_null()); Done.
https://codereview.chromium.org/2901033002/diff/60001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/60001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:203: network_settings.port_range.min_port = Setting port range here makes sense only if it isn't already set by the policy. I.e. if (!udp_port_range_.is_null()) { network_settings.port_range = udp_port_range_; } else if (!nat_traversal_enabled_) { network_settings.port_range.min_port = protocol::NetworkSettings::kDefaultMinPort; network_settings.port_range.max_port = protocol::NetworkSettings::kDefaultMaxPort; }
https://codereview.chromium.org/2901033002/diff/60001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/60001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:203: network_settings.port_range.min_port = On 2017/05/25 22:24:38, Sergey Ulanov wrote: > Setting port range here makes sense only if it isn't already set by the policy. > I.e. > if (!udp_port_range_.is_null()) { > network_settings.port_range = udp_port_range_; > } else if (!nat_traversal_enabled_) { > network_settings.port_range.min_port = > protocol::NetworkSettings::kDefaultMinPort; > network_settings.port_range.max_port = > protocol::NetworkSettings::kDefaultMaxPort; > } Done. I don't think I fully understand what should take precedence over what, though - I assumed that NAT traversal being disabled should force the port range to the default port range to allow for firewall pin-holing as the comment states. Is this not the case?
Code looks good, just couple needs about the comments. https://codereview.chromium.org/2901033002/diff/60001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/60001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:203: network_settings.port_range.min_port = On 2017/05/25 22:55:58, Gus Smith wrote: > On 2017/05/25 22:24:38, Sergey Ulanov wrote: > > Setting port range here makes sense only if it isn't already set by the > policy. > > I.e. > > if (!udp_port_range_.is_null()) { > > network_settings.port_range = udp_port_range_; > > } else if (!nat_traversal_enabled_) { > > network_settings.port_range.min_port = > > protocol::NetworkSettings::kDefaultMinPort; > > network_settings.port_range.max_port = > > protocol::NetworkSettings::kDefaultMaxPort; > > } > > Done. I don't think I fully understand what should take precedence over what, > though - I assumed that NAT traversal being disabled should force the port range > to the default port range to allow for firewall pin-holing as the comment > states. Is this not the case? If an admin disables NAT traversal then they also have control of the port range policy. They can set the policy to the default range if that's what they want. This code was added long time ago when we didn't have PortRange policy. Now that we have it we should respect it regardless of the NAT traversal state. This behavior is consistent with the Me2Me host, see https://codesearch.chromium.org/chromium/src/remoting/host/remoting_me2me_hos... https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:199: // Update port range only if it has not already been set. This comment doesn't look correct. The code sets port_range in network_settings when the policy is set. I would just remove it. The code is clear enough without this comment. https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:200: // If NAT traversal is off then limit port range to allow firewall pin-holing. Please remove this comment to avoid confusion. Instead I suggest adding a comment in the !nat_traversal_enabled_ case, maybe just copy from the Me2Me host: https://codesearch.chromium.org/chromium/src/remoting/host/remoting_me2me_hos... https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:201: HOST_LOG << "NAT state: " << nat_traversal_enabled_; This line is not related to port range. I'd prefer to keep it where it was, i.e. before network_settings definition.
https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:199: // Update port range only if it has not already been set. On 2017/05/26 00:40:09, Sergey Ulanov wrote: > This comment doesn't look correct. The code sets port_range in network_settings > when the policy is set. I would just remove it. The code is clear enough without > this comment. Done. https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:200: // If NAT traversal is off then limit port range to allow firewall pin-holing. On 2017/05/26 00:40:09, Sergey Ulanov wrote: > Please remove this comment to avoid confusion. Instead I suggest adding a > comment in the !nat_traversal_enabled_ case, maybe just copy from the Me2Me > host: > https://codesearch.chromium.org/chromium/src/remoting/host/remoting_me2me_hos... Done. https://codereview.chromium.org/2901033002/diff/50005/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:201: HOST_LOG << "NAT state: " << nat_traversal_enabled_; On 2017/05/26 00:40:09, Sergey Ulanov wrote: > This line is not related to port range. I'd prefer to keep it where it was, i.e. > before network_settings definition. Done.
lgtm
The CQ bit was checked by gusss@google.com
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": 90001, "attempt_start_ts": 1495818381391870,
"parent_rev": "94c549fec3c9147e180141841576a6afc6baaa98", "commit_rev":
"49ccc86fa6f0112e768dda3b7dd2d78529275ca8"}
Message was sent while issue was closed.
Description was changed from ========== Host port range policy is no longer ignored in it2me host. Adds UpdateHostUdpPortRangePolicy into It2MeHost, which is called whenever a policy update contains a port range setting. This port range setting is then stored, and is later used to set the appropriate fields in the NetworkSettings object passed into the TransportContext. BUG=723093 ========== to ========== Host port range policy is no longer ignored in it2me host. Adds UpdateHostUdpPortRangePolicy into It2MeHost, which is called whenever a policy update contains a port range setting. This port range setting is then stored, and is later used to set the appropriate fields in the NetworkSettings object passed into the TransportContext. BUG=723093 Review-Url: https://codereview.chromium.org/2901033002 Cr-Commit-Position: refs/heads/master@{#475047} Committed: https://chromium.googlesource.com/chromium/src/+/49ccc86fa6f0112e768dda3b7dd2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://chromium.googlesource.com/chromium/src/+/49ccc86fa6f0112e768dda3b7dd2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
