Add split tunnel interface to vpnProvider

Add split tunnel interface to vpnProvider

This CL adds split tunnel capability to chrome.vpnProvider interface.
Furthermore, additional sanity checks are added to the input from the
extensions.

BUG=459609
Committed: https://crrev.com/cd7896d845d45087370716695b5026260f453d71
Cr-Commit-Position: refs/heads/master@{#317289}

[kaliamoorthi, 2015-02-18 18:18:07]

kaliamoorthi@chromium.org changed reviewers:
+ kalman@chromium.org

[kaliamoorthi, 2015-02-18 18:18:07]

PTAL.

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

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
File extensions/browser/api/vpn_provider/vpn_provider_api.cc (right):

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:27: bool
CheckIPCIDR(const std::string& value, bool cidr, bool ipv6) {
You should test this.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:83: }
Could you pull each of these into a CheckIPCIDRList function?

https://codereview.chromium.org/932063003/diff/1/extensions/common/api/vpn_pr...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/932063003/diff/1/extensions/common/api/vpn_pr...
extensions/common/api/vpn_provider.idl:23: // parameter can be used to setup
split tunnel.
Which takes precedence?

[kaliamoorthi, 2015-02-19 12:53:58]

kaliamoorthi@chromium.org changed reviewers:
+ bartfab@chromium.org

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

I am working on the unit tests.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
File extensions/browser/api/vpn_provider/vpn_provider_api.cc (right):

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:27: bool
CheckIPCIDR(const std::string& value, bool cidr, bool ipv6) {
On 2015/02/18 19:14:04, kalman wrote:
> You should test this.

I am working on the test.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:83: }
On 2015/02/18 19:14:04, kalman wrote:
> Could you pull each of these into a CheckIPCIDRList function?

Done.

https://codereview.chromium.org/932063003/diff/1/extensions/common/api/vpn_pr...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/932063003/diff/1/extensions/common/api/vpn_pr...
extensions/common/api/vpn_provider.idl:23: // parameter can be used to setup
split tunnel.
On 2015/02/18 19:14:04, kalman wrote:
> Which takes precedence?

I responded to this question in the comment. Its the most specific one with the
longest matching prefix.

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

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
File extensions/browser/api/vpn_provider/vpn_provider_api.cc (right):

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:25: const char
kCIDRSeperator[] = "/";
Why is this a string and not a single-char constant?

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:27: bool
CheckIPCIDR(const std::string& value, bool cidr, bool ipv6) {
On 2015/02/19 12:53:59, kaliamoorthi wrote:
> On 2015/02/18 19:14:04, kalman wrote:
> > You should test this.
> 
> I am working on the test.

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.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:32: for (auto elem :
value) {
Nit: const auto&

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:35: } else if (*elem ==
'.' && !dots) {
Nit: No else after continue.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:37: } else if (*elem ==
'.') {
Nit: No else after return.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:41: } else if (*elem ==
kCIDRSeperator[0]) {
Nit: No else after return.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:45: } else if (*elem ==
':') {
Nit: No else after return.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:46: colon--;
You are not checking that IPv6 starts with a colon.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:49: } else {
Nit: No else after continue.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:53: return !sep && !dots
&& (colon < 7);
Is the number of colons not fixed?

[kaliamoorthi, 2015-02-19 16:09:05]

Patchset #3 (id:40001) has been deleted

[kaliamoorthi, 2015-02-19 16:18:28]

Patchset #3 (id:60001) has been deleted

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

PTAL.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
File extensions/browser/api/vpn_provider/vpn_provider_api.cc (right):

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:25: const char
kCIDRSeperator[] = "/";
On 2015/02/19 13:10:13, bartfab wrote:
> Why is this a string and not a single-char constant?

This is due to tokenize below.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:27: bool
CheckIPCIDR(const std::string& value, bool cidr, bool ipv6) {
On 2015/02/19 13:10:14, bartfab wrote:
> On 2015/02/19 12:53:59, kaliamoorthi wrote:
> > On 2015/02/18 19:14:04, kalman wrote:
> > > You should test this.
> > 
> > I am working on the test.
> 
> 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.

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.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:32: for (auto elem :
value) {
On 2015/02/19 13:10:14, bartfab wrote:
> Nit: const auto&

Done.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:35: } else if (*elem ==
'.' && !dots) {
On 2015/02/19 13:10:14, bartfab wrote:
> Nit: No else after continue.

Done.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:37: } else if (*elem ==
'.') {
On 2015/02/19 13:10:14, bartfab wrote:
> Nit: No else after return.

Done.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:41: } else if (*elem ==
kCIDRSeperator[0]) {
On 2015/02/19 13:10:13, bartfab wrote:
> Nit: No else after return.

Done.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:45: } else if (*elem ==
':') {
On 2015/02/19 13:10:14, bartfab wrote:
> Nit: No else after return.

Done.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:46: colon--;
On 2015/02/19 13:10:13, bartfab wrote:
> You are not checking that IPv6 starts with a colon.

It need not start with a colon.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:49: } else {
On 2015/02/19 13:10:14, bartfab wrote:
> Nit: No else after continue.

Done.

https://codereview.chromium.org/932063003/diff/1/extensions/browser/api/vpn_p...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:53: return !sep && !dots
&& (colon < 7);
On 2015/02/19 13:10:14, bartfab wrote:
> Is the number of colons not fixed?

Acknowledged.

[not at google - send to devlin, 2015-02-19 19:06:24]

lgtm with nits. didn't look at logic.

https://codereview.chromium.org/932063003/diff/100001/extensions/browser/api/...
File extensions/browser/api/vpn_provider/vpn_provider_apitest.cc (right):

https://codereview.chromium.org/932063003/diff/100001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_provider_apitest.cc:28: {0, "1+++", ""},
            /*+ not allowed*/
Use // comments not /* ?

https://codereview.chromium.org/932063003/diff/100001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_provider_apitest.cc:82: LOG(ERROR) <<
"Expected message " << kErrorMessages[GetParam()->err_index];
Take out logging?

[kaliamoorthi, 2015-02-20 10:32:10]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

[kaliamoorthi, 2015-02-20 10:33:21]

Fixed nits.

https://codereview.chromium.org/932063003/diff/100001/extensions/browser/api/...
File extensions/browser/api/vpn_provider/vpn_provider_apitest.cc (right):

https://codereview.chromium.org/932063003/diff/100001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_provider_apitest.cc:28: {0, "1+++", ""},
            /*+ not allowed*/
On 2015/02/19 19:06:24, kalman wrote:
> Use // comments not /* ?

Done.

https://codereview.chromium.org/932063003/diff/100001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_provider_apitest.cc:82: LOG(ERROR) <<
"Expected message " << kErrorMessages[GetParam()->err_index];
On 2015/02/19 19:06:24, kalman wrote:
> Take out logging?

Done.

[kaliamoorthi, 2015-02-20 11:11:42]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2015-02-20 11:12:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/932063003/120001

[commit-bot: I haz the power, 2015-02-20 11:15:17]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-02-20 11:15:55]

Patchset 5 (id:??) landed as
https://crrev.com/cd7896d845d45087370716695b5026260f453d71
Cr-Commit-Position: refs/heads/master@{#317289}

[bartfab (slow), 2015-02-20 14:56:38]

https://codereview.chromium.org/932063003/diff/120001/extensions/browser/api/...
File extensions/browser/api/vpn_provider/vpn_provider_api.cc (right):

https://codereview.chromium.org/932063003/diff/120001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:31: bool hex_allowed =
ipv6;
Nit: const.

https://codereview.chromium.org/932063003/diff/120001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:50: colon = 0;
Since you reset |colon| here, the entire (colon < 7) check at the end of the
method becomes pointless.

https://codereview.chromium.org/932063003/diff/120001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:58: } else {
Nit: No else after return.