New policies: enable/disable relay; port range

New policies: enable/disable relay; port range

Add new Chromoting policies:

"RemoteAccessHostAllowRelayedConnection"
bool, default true
It can be set to false to disable the use of relay servers if NAT
traversal is enabled.

"RemoteAccessHostUdpPortRange"
string, default ""
Can be used to specify a range in the form "<min-port>-<max-port>" to
restrict the range of UDP ports available to the host for connections.
E.g. "12400-12409"

BUG=355168
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267234

[Sergey Ulanov, 2014-03-24 23:37:12]

https://codereview.chromium.org/209323002/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

https://codereview.chromium.org/209323002/diff/1/remoting/base/util.h#newcode101
remoting/base/util.h:101: bool ParsePortRange(const std::string& port_range,
int* out_min_port,
nit:move out_min_port to the next line (one argument per line)

https://codereview.chromium.org/209323002/diff/1/remoting/base/util.h#newcode101
remoting/base/util.h:101: bool ParsePortRange(const std::string& port_range,
int* out_min_port,
This doesn't belong in remoting/base (and I'd like to avoid adding anything new
in util.h in general). Move it somewhere in remoting/host or to
protocol::NetworkSettings?

https://codereview.chromium.org/209323002/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/209323002/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:80: const char
PolicyWatcher::kRelayPolicyName[] =
The new policies also need to be added in
chrome/app/policy/policy_templates.json .  See
https://chromiumcodereview.appspot.com/20864002 for example.

https://codereview.chromium.org/209323002/diff/10001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.h (right):

https://codereview.chromium.org/209323002/diff/10001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:23: NAT_TRAVERSAL_ENABLED_NO_RELAY,
This breaks LibjingleTransportFactory::EnsureFreshJingleInfo() which only
fetches STUN config in the NAT_TRAVERSAL_ENABLED mode. To avoid bugs like this,
it might be better to replace nat_traversal_mode with nat_traversal_flags and
separate flags for STUN, Relay, and outgoing connections.

[dcaiafa, 2014-03-26 15:29:16]

ptal

https://codereview.chromium.org/209323002/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

https://codereview.chromium.org/209323002/diff/1/remoting/base/util.h#newcode101
remoting/base/util.h:101: bool ParsePortRange(const std::string& port_range,
int* out_min_port,
On 2014/03/24 23:37:12, Sergey Ulanov wrote:
> nit:move out_min_port to the next line (one argument per line)

Done.

https://codereview.chromium.org/209323002/diff/1/remoting/base/util.h#newcode101
remoting/base/util.h:101: bool ParsePortRange(const std::string& port_range,
int* out_min_port,
On 2014/03/24 23:37:12, Sergey Ulanov wrote:
> This doesn't belong in remoting/base (and I'd like to avoid adding anything
new
> in util.h in general). Move it somewhere in remoting/host or to
> protocol::NetworkSettings?

Moved to network_settings.*

https://codereview.chromium.org/209323002/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/209323002/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:80: const char
PolicyWatcher::kRelayPolicyName[] =
On 2014/03/24 23:37:12, Sergey Ulanov wrote:
> The new policies also need to be added in
> chrome/app/policy/policy_templates.json .  See
> https://chromiumcodereview.appspot.com/20864002 for example.

Done.

https://codereview.chromium.org/209323002/diff/10001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.h (right):

https://codereview.chromium.org/209323002/diff/10001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:23: NAT_TRAVERSAL_ENABLED_NO_RELAY,
On 2014/03/24 23:37:12, Sergey Ulanov wrote:
> This breaks LibjingleTransportFactory::EnsureFreshJingleInfo() which only
> fetches STUN config in the NAT_TRAVERSAL_ENABLED mode. To avoid bugs like
this,
> it might be better to replace nat_traversal_mode with nat_traversal_flags and
> separate flags for STUN, Relay, and outgoing connections.

Good catch! Done.

[Sergey Ulanov, 2014-03-27 18:31:19]

https://codereview.chromium.org/209323002/diff/30001/chrome/test/data/policy/...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/209323002/diff/30001/chrome/test/data/policy/...
chrome/test/data/policy/policy_test_cases.json:251: { "pref":
"remote_access.host_allow_gnubby_auth" }
allow_relayed_connection

https://codereview.chromium.org/209323002/diff/30001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/209323002/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:693: 'desc': '''Restricts the
UDP port range used by the remote access host in this machine.
Mention that port range is restricted to 12400-12409 when NAT traversal is
disabled.

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.cc (right):

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.cc:19: int min_port =
atoi(port_range.substr(0, separator_index).c_str());
With atoi() this function will accept strings like "1foo-100bar". Please use
base::StringToUint() from base/strings/string_number_conversions.h . You may
also need base::TrimWhitespaceASCII() to trim whitespaces if necessary

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.h (right):

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:22: // Flags:
Define the flags in an enum instead of as a set of constants

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:41: NAT_TRAVERSAL_STUN |
NAT_TRAVERSAL_RELAY;
Should this also include NAT_TRAVERSAL_OUTGOING? or is the intent that outgoing
connections are implied by STUN/Relay flags?

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:65: bool ParsePortRange(const
std::string& port_range,
Make this a static method in NetworkSettings class?

https://codereview.chromium.org/209323002/diff/30001/remoting/protocol/libjin...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/209323002/diff/30001/remoting/protocol/libjin...
remoting/protocol/libjingle_transport_factory.cc:208: if
(network_settings_.flags == NetworkSettings::NAT_TRAVERSAL_DISABLED) {
channel_->set_incoming_only(!(flags & NetworkSettings::NAT_TRAVERSAL_OUTGOING));

[dcaiafa, 2014-03-27 19:32:23]

ptal

https://codereview.chromium.org/209323002/diff/30001/chrome/test/data/policy/...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/209323002/diff/30001/chrome/test/data/policy/...
chrome/test/data/policy/policy_test_cases.json:251: { "pref":
"remote_access.host_allow_gnubby_auth" }
On 2014/03/27 18:31:20, Sergey Ulanov wrote:
> allow_relayed_connection

Done.

https://codereview.chromium.org/209323002/diff/30001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/209323002/diff/30001/components/policy/resour...
components/policy/resources/policy_templates.json:693: 'desc': '''Restricts the
UDP port range used by the remote access host in this machine.
On 2014/03/27 18:31:20, Sergey Ulanov wrote:
> Mention that port range is restricted to 12400-12409 when NAT traversal is
> disabled.

Done.

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.cc (right):

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.cc:19: int min_port =
atoi(port_range.substr(0, separator_index).c_str());
On 2014/03/27 18:31:20, Sergey Ulanov wrote:
> With atoi() this function will accept strings like "1foo-100bar". Please use
> base::StringToUint() from base/strings/string_number_conversions.h . You may
> also need base::TrimWhitespaceASCII() to trim whitespaces if necessary

Done.

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.h (right):

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:22: // Flags:
On 2014/03/27 18:31:20, Sergey Ulanov wrote:
> Define the flags in an enum instead of as a set of constants

Done.

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:41: NAT_TRAVERSAL_STUN |
NAT_TRAVERSAL_RELAY;
On 2014/03/27 18:31:20, Sergey Ulanov wrote:
> Should this also include NAT_TRAVERSAL_OUTGOING? or is the intent that
outgoing
> connections are implied by STUN/Relay flags?

What do you think of this?

https://codereview.chromium.org/209323002/diff/30001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:65: bool ParsePortRange(const
std::string& port_range,
On 2014/03/27 18:31:20, Sergey Ulanov wrote:
> Make this a static method in NetworkSettings class?

Done.

https://codereview.chromium.org/209323002/diff/30001/remoting/protocol/libjin...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/209323002/diff/30001/remoting/protocol/libjin...
remoting/protocol/libjingle_transport_factory.cc:208: if
(network_settings_.flags == NetworkSettings::NAT_TRAVERSAL_DISABLED) {
On 2014/03/27 18:31:20, Sergey Ulanov wrote:
> channel_->set_incoming_only(!(flags &
NetworkSettings::NAT_TRAVERSAL_OUTGOING));

Done.

[Sergey Ulanov, 2014-03-27 19:52:25]

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.h (right):

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:33: NAT_TRAVERSAL_STUN = 0x1 |
NAT_TRAVERSAL_OUTGOING,
this should be 2, not 1.
I'd still prefer separate flags. Maybe add a DCHECK in the NetworkSettings
constructors to verify that NAT_TRAVERSAL_OUTGOING is always set when STUN or
Relay are enabled?

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:36: NAT_TRAVERSAL_RELAY = 0x2 |
NAT_TRAVERSAL_OUTGOING,
4

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:63: static bool ParsePortRange(const
std::string& port_range,
nit: this should be declared before |flags|.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

[Sergey Ulanov, 2014-03-27 19:52:52]

lgtm after my comments are addressed.

[dcaiafa, 2014-03-27 23:36:21]

Thanks!

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.h (right):

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:33: NAT_TRAVERSAL_STUN = 0x1 |
NAT_TRAVERSAL_OUTGOING,
On 2014/03/27 19:52:26, Sergey Ulanov wrote:
> this should be 2, not 1.
> I'd still prefer separate flags. Maybe add a DCHECK in the NetworkSettings
> constructors to verify that NAT_TRAVERSAL_OUTGOING is always set when STUN or
> Relay are enabled?

Ugh! Done.

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:36: NAT_TRAVERSAL_RELAY = 0x2 |
NAT_TRAVERSAL_OUTGOING,
On 2014/03/27 19:52:26, Sergey Ulanov wrote:
> 4

Done.

https://codereview.chromium.org/209323002/diff/70001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:63: static bool ParsePortRange(const
std::string& port_range,
On 2014/03/27 19:52:26, Sergey Ulanov wrote:
> nit: this should be declared before |flags|.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

[Sergey Ulanov, 2014-03-27 23:43:06]

LGTM. Thanks!

https://codereview.chromium.org/209323002/diff/90001/remoting/jingle_glue/net...
File remoting/jingle_glue/network_settings.h (right):

https://codereview.chromium.org/209323002/diff/90001/remoting/jingle_glue/net...
remoting/jingle_glue/network_settings.h:48: DCHECK(!(flags & (NAT_TRAVERSAL_STUN
| NAT_TRAVERSAL_RELAY)) ||
nit: maybe comment why this is needed.

[dcaiafa, 2014-04-25 19:22:30]

+mnissler for chrome policy related changes (everything not remoting/...)

[Mattias Nissler (ping if slow), 2014-04-28 11:40:44]

Deferring to Joao, as you'll need his LGTM for policy_templates.json anyways.

[Joao da Silva, 2014-04-28 12:38:28]

lgtm

See inline about the policy component: it should be possible for the remoting
code to use the PolicyService from //components/policy directly now, instead of
duplicating the policy loading logic and strings. That's for another CL though
:-)

https://codereview.chromium.org/209323002/diff/110001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/209323002/diff/110001/components/policy/resou...
components/policy/resources/policy_templates.json:675: If this setting is
enabled, then remote clients can use relay servers to connect to the this
machine when a direct connection is not available (e.g. due to firewall
restrictions).
to connect to this machine

https://codereview.chromium.org/209323002/diff/110001/remoting/host/policy_ha...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/209323002/diff/110001/remoting/host/policy_ha...
remoting/host/policy_hack/policy_watcher.cc:114: "RemoteAccessHostUdpPortRange";
FYI, the policy code now lives in a component at //components/policy, so maybe
you can reuse the policy strings from there.

You'd need to depend on the components.gyp:policy target, and then you can
#include "policy/policy_constants.h" to use the policy names. Note that this
file is generated, look at out/Debug/gen/policy/policy/policy_constants.h to see
it.

Also, you could maybe pass the PolicyService from
g_browser_process->policy_service() or from
ProfilePolicyConnector::policy_service() down to the remoting code, and read the
policies directly from there. The PolicyService is also in the component, so you
don't need to depend on //chrome code.

This is just a suggestion, it's fine as is for this CL.

[dcaiafa, 2014-04-29 18:40:30]

+Ilya for histograms.xml

https://codereview.chromium.org/209323002/diff/110001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/209323002/diff/110001/components/policy/resou...
components/policy/resources/policy_templates.json:675: If this setting is
enabled, then remote clients can use relay servers to connect to the this
machine when a direct connection is not available (e.g. due to firewall
restrictions).
On 2014/04/28 12:38:29, Joao da Silva wrote:
> to connect to this machine

Done.

https://codereview.chromium.org/209323002/diff/110001/remoting/host/policy_ha...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/209323002/diff/110001/remoting/host/policy_ha...
remoting/host/policy_hack/policy_watcher.cc:114: "RemoteAccessHostUdpPortRange";
On 2014/04/28 12:38:29, Joao da Silva wrote:
> FYI, the policy code now lives in a component at //components/policy, so maybe
> you can reuse the policy strings from there.
> 
> You'd need to depend on the components.gyp:policy target, and then you can
> #include "policy/policy_constants.h" to use the policy names. Note that this
> file is generated, look at out/Debug/gen/policy/policy/policy_constants.h to
see
> it.
> 
> Also, you could maybe pass the PolicyService from
> g_browser_process->policy_service() or from
> ProfilePolicyConnector::policy_service() down to the remoting code, and read
the
> policies directly from there. The PolicyService is also in the component, so
you
> don't need to depend on //chrome code.
> 
> This is just a suggestion, it's fine as is for this CL.

That's great! Thanks for letting us know. I opened crbug.com/368321 to
investigate using //components/policy.

[Ilya Sherman, 2014-04-29 20:03:07]

histograms.xml lgtm

[dcaiafa, 2014-04-29 20:24:07]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2014-04-29 20:43:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/209323002/130001

[commit-bot: I haz the power, 2014-04-29 20:58:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 20:58:16]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium

[dcaiafa, 2014-04-30 14:12:37]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2014-04-30 14:13:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/209323002/150001

[commit-bot: I haz the power, 2014-04-30 16:47:34]

Change committed as 267234