Add split tunnel interface to vpnProvider

extensions/browser/api/vpn_provider/vpn_provider_api.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/api/vpn_provider/vpn_provider_api.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "extensions/browser/api/vpn_provider/vpn_service.h"
 #include "extensions/browser/api/vpn_provider/vpn_service_factory.h"
 #include "extensions/common/api/vpn_provider.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace extensions {
 
 namespace {
 
 namespace api_vpn = extensions::core_api::vpn_provider;
 
 const char kCIDRSeperator[] = "/";

[bartfab (slow), 2015/02/19 13:10:13]

Why is this a string and not a single-char constant?

[kaliamoorthi, 2015/02/19 16:20:05]

This is due to tokenize below.

 
+bool CheckIPCIDR(const std::string& value, bool cidr, bool ipv6) {

[not at google - send to devlin, 2015/02/18 19:14:04]

You should test this.

[kaliamoorthi, 2015/02/19 12:53:59]

I am working on the test.

[bartfab (slow), 2015/02/19 13:10:14]

I think it would be better to implement this differently:

* If |cidr=true|, split the string on '/'. Verify that you get 2 pieces.
* Check each piece separately:
 * Split on '.'. Verify that you get 4 pieces.
  * Check each piece separately:
   * Verify that you got 1-3 digits. For bonus points, verify that the value is
in the expected range.

No need to implement IPv6 if we do not support it for now anyway.

[kaliamoorthi, 2015/02/19 16:20:05]

I am doing a simple sanity check here in a single pass. The idea is to not pass
a buffer passed in by the user to a privileged process without some basic
vetting. Detailed analysis happens in shill.

+  int dots = !ipv6 ? 3 : 0;
+  int sep = cidr ? 1 : 0;
+  int colon = ipv6 ? 7 : 0;
+
+  for (auto elem : value) {

[bartfab (slow), 2015/02/19 13:10:14]

Nit: const auto&

[kaliamoorthi, 2015/02/19 16:20:05]

Done.

+    if (base::IsAsciiDigit(*elem)) {
+      continue;
+    } else if (*elem == '.' && !dots) {

[bartfab (slow), 2015/02/19 13:10:14]

Nit: No else after continue.

[kaliamoorthi, 2015/02/19 16:20:05]

Done.

+      return false;
+    } else if (*elem == '.') {

[bartfab (slow), 2015/02/19 13:10:14]

Nit: No else after return.

[kaliamoorthi, 2015/02/19 16:20:05]

Done.

+      dots--;
+    } else if (*elem == kCIDRSeperator[0] && !sep) {
+      return false;
+    } else if (*elem == kCIDRSeperator[0]) {

[bartfab (slow), 2015/02/19 13:10:13]

Nit: No else after return.

[kaliamoorthi, 2015/02/19 16:20:05]

Done.

+      sep--;
+    } else if (*elem == ':' && !colon) {
+      return false;
+    } else if (*elem == ':') {

[bartfab (slow), 2015/02/19 13:10:14]

Nit: No else after return.

[kaliamoorthi, 2015/02/19 16:20:05]

Done.

+      colon--;

[bartfab (slow), 2015/02/19 13:10:13]

You are not checking that IPv6 starts with a colon.

[kaliamoorthi, 2015/02/19 16:20:04]

It need not start with a colon.

+    } else if (ipv6 && base::IsHexDigit(*elem)) {
+      continue;
+    } else {

[bartfab (slow), 2015/02/19 13:10:14]

Nit: No else after continue.

[kaliamoorthi, 2015/02/19 16:20:05]

Done.

+      return false;
+    }
+  }
+  return !sep && !dots && (colon < 7);

[bartfab (slow), 2015/02/19 13:10:14]

Is the number of colons not fixed?

[kaliamoorthi, 2015/02/19 16:20:05]

Acknowledged.

+}
+
 void ConvertParameters(const api_vpn::Parameters& parameters,
                        base::DictionaryValue* parameter_value,
                        std::string* error) {
-  std::vector<std::string> cidr_parts;
-  if (Tokenize(parameters.address, kCIDRSeperator, &cidr_parts) != 2) {
+  if (!CheckIPCIDR(parameters.address, true, false)) {
     *error = "Invalid CIDR address.";
     return;
   }
 
+  for (auto address : parameters.exclusion_list) {
+    if (!CheckIPCIDR(*address, true, false)) {
+      *error = "Invalid CIDR in exclusion list.";
+      return;
+    }
+  }
+
+  for (auto address : parameters.inclusion_list) {
+    if (!CheckIPCIDR(*address, true, false)) {
+      *error = "Invalid CIDR in inclusion list.";
+      return;
+    }
+  }
+
+  for (auto address : parameters.dns_servers) {
+    if (!CheckIPCIDR(*address, false, false)) {
+      *error = "Invalid IP in DNS servers.";
+      return;
+    }
+  }

[not at google - send to devlin, 2015/02/18 19:14:04]

Could you pull each of these into a CheckIPCIDRList function?

[kaliamoorthi, 2015/02/19 12:53:59]

Done.

+
+  std::vector<std::string> cidr_parts;
+  Tokenize(parameters.address, kCIDRSeperator, &cidr_parts);
+
   parameter_value->SetStringWithoutPathExpansion(
       shill::kAddressParameterThirdPartyVpn, cidr_parts[0]);
 
   parameter_value->SetStringWithoutPathExpansion(
       shill::kSubnetPrefixParameterThirdPartyVpn, cidr_parts[1]);
 
   parameter_value->SetStringWithoutPathExpansion(
-      shill::kBypassTunnelForIpParameterThirdPartyVpn,
-      JoinString(parameters.bypass_tunnel_for_ip, shill::kIPDelimiter));
+      "exclusion_list",
+      JoinString(parameters.exclusion_list, shill::kIPDelimiter));
+
+  parameter_value->SetStringWithoutPathExpansion(
+      "inclusion_list",
+      JoinString(parameters.inclusion_list, shill::kIPDelimiter));
 
   if (parameters.mtu) {
     parameter_value->SetStringWithoutPathExpansion(
         shill::kMtuParameterThirdPartyVpn, *parameters.mtu);
   }
 
   if (parameters.broadcast_address) {
     parameter_value->SetStringWithoutPathExpansion(
         shill::kBroadcastAddressParameterThirdPartyVpn,
         *parameters.broadcast_address);
@@ skipping 172 matching lines @@
                      SignalCallCompletionSuccess,
                  this),
       base::Bind(&VpnProviderNotifyConnectionStateChangedFunction::
                      SignalCallCompletionFailure,
                  this));
 
   return RespondLater();
 }
 
 }  // namespace extensions