Make socket permission also allow Pepper sockets

Make socket permission also allow Pepper sockets

BUG=116225
TEST=manual

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

[Jói, 2012-02-29 15:07:44]

LGTM

Content changes look good.

[miket_OOO, 2012-02-29 18:32:18]

I'm starting my review now, but beware that I'm not a committer, so my approval
means nothing.

[miket_OOO, 2012-02-29 18:34:16]

On 2012/02/29 18:32:18, miket wrote:
> I'm starting my review now, but beware that I'm not a committer, so my
approval
> means nothing.

LGTM, and thank you for the readability improvements in
ChromeContentBrowserClient::AllowSocketAPI.

[Dmitry Polukhin, 2012-03-01 06:15:42]

+ brettw for OWNER review in content/browser

Mike, please ping me when regex-capable whitelist for protocol/host is
implemented, it will need to be added here too. Also this whitelist most
probably should have support for TCP server sockets.

[jam, 2012-03-01 06:21:26]

content/browser lgtm with nits

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
File content/browser/mock_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
content/browser/mock_content_browser_client.cc:308: content::BrowserContext*
browser_context,
ditto

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
File content/browser/mock_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
content/browser/mock_content_browser_client.h:155: virtual bool
AllowSocketAPI(content::BrowserContext* browser_context,
nit: content:: not needed

https://chromiumcodereview.appspot.com/9539011/diff/1/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/public/browser/...
content/public/browser/content_browser_client.h:367: virtual bool
AllowSocketAPI(content::BrowserContext* browser_context,
ditto

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
File content/shell/shell_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
content/shell/shell_content_browser_client.cc:328: content::BrowserContext*
browser_context,
ditto

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
File content/shell/shell_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
content/shell/shell_content_browser_client.h:160: virtual bool
AllowSocketAPI(content::BrowserContext* browser_context,
ditto

[Mihai Parparita -not on Chrome, 2012-03-01 06:25:31]

https://chromiumcodereview.appspot.com/9539011/diff/1/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1445: if
(extension->HasAPIPermission(ExtensionAPIPermission::kSocket))
I may need some more context about this CL, but I believe the plan for the
extension socket API is to require both the socket API permission, and the host
permission to connect to the specific host that the socket is open to.

[Dmitry Polukhin, 2012-03-01 06:36:05]

https://chromiumcodereview.appspot.com/9539011/diff/1/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1445: if
(extension->HasAPIPermission(ExtensionAPIPermission::kSocket))
On 2012/03/01 06:25:31, Mihai Parparita wrote:
> I may need some more context about this CL, but I believe the plan for the
> extension socket API is to require both the socket API permission, and the
host
> permission to connect to the specific host that the socket is open to.

I'm sorry I'm not sure that I understand what do you mean by "host permission".
Perhaps it is the same as regex-capable whitelist for protocol/host but as far
as I can see it is not implemented yet.

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
File content/browser/mock_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
content/browser/mock_content_browser_client.cc:308: content::BrowserContext*
browser_context,
On 2012/03/01 06:21:26, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
File content/browser/mock_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/browser/mock_co...
content/browser/mock_content_browser_client.h:155: virtual bool
AllowSocketAPI(content::BrowserContext* browser_context,
On 2012/03/01 06:21:26, John Abd-El-Malek wrote:
> nit: content:: not needed

Done.

https://chromiumcodereview.appspot.com/9539011/diff/1/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/public/browser/...
content/public/browser/content_browser_client.h:367: virtual bool
AllowSocketAPI(content::BrowserContext* browser_context,
On 2012/03/01 06:21:26, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
File content/shell/shell_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
content/shell/shell_content_browser_client.cc:328: content::BrowserContext*
browser_context,
On 2012/03/01 06:21:26, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
File content/shell/shell_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9539011/diff/1/content/shell/shell_con...
content/shell/shell_content_browser_client.h:160: virtual bool
AllowSocketAPI(content::BrowserContext* browser_context,
On 2012/03/01 06:21:26, John Abd-El-Malek wrote:
> ditto

Done.

[Mihai Parparita -not on Chrome, 2012-03-01 06:41:12]

On Wed, Feb 29, 2012 at 10:36 PM, <dpolukhin@chromium.org> wrote:

> On 2012/03/01 06:25:31, Mihai Parparita wrote:
>
>> I may need some more context about this CL, but I believe the plan for
>>
> the
>
>> extension socket API is to require both the socket API permission, and
>>
> the host
>
>> permission to connect to the specific host that the socket is open to.
>>
>
> I'm sorry I'm not sure that I understand what do you mean by "host
> permission". Perhaps it is the same as regex-capable whitelist for
> protocol/host but as far as I can see it is not implemented yet.


I'm thinking something along the lines of the cookies API (
http://code.google.com/chrome/extensions/cookies.html), where to use it you
need to include in the extension manifest both the "cookies" permission and
the hosts for which you set cookies. The check is implemented here:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/e...

Host permissions support ports, and you could extend URLPattern to support
a "udp" and a "tcp" scheme so that for example an extension that wanted to
use this for SSH-ing to google.com hosts would include "tcp://*.
google.com:22" in its manifest.

Mike, were you thinking along those lines for permission support for the
socket API?

Mihai

[Dmitry Polukhin, 2012-03-01 06:54:00]

Yes, we are talking about the same thing. But it is not implemented for
sockets yet:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/extensions...

I could be tricky part of it because of IP addresses (IP v4 and IP v6) and
not fully qualified host names. Pepper allows making host resolution and
connection using IP address. Also we will need to invent something for TCP
server sockets (i.e. permission to listen TCP port on localhost)
and distinguish it from permission to connect to given port on
localhost. Perhaps for Pepper socket someone will need to specify "*://*:*"
i.e. everywhere because for most frequent use cases it will require anyway
(SSH client and other remote access like apps).

On Thu, Mar 1, 2012 at 10:41 AM, Mihai Parparita <mihaip@chromium.org>wrote:

> On Wed, Feb 29, 2012 at 10:36 PM, <dpolukhin@chromium.org> wrote:
>
>> On 2012/03/01 06:25:31, Mihai Parparita wrote:
>>
>>> I may need some more context about this CL, but I believe the plan for
>>>
>> the
>>
>>> extension socket API is to require both the socket API permission, and
>>>
>> the host
>>
>>> permission to connect to the specific host that the socket is open to.
>>>
>>
>> I'm sorry I'm not sure that I understand what do you mean by "host
>> permission". Perhaps it is the same as regex-capable whitelist for
>> protocol/host but as far as I can see it is not implemented yet.
>
>
> I'm thinking something along the lines of the cookies API (
> http://code.google.com/chrome/extensions/cookies.html), where to use it
> you need to include in the extension manifest both the "cookies" permission
> and the hosts for which you set cookies. The check is implemented here:
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/e...
>
> Host permissions support ports, and you could extend URLPattern to support
> a "udp" and a "tcp" scheme so that for example an extension that wanted to
> use this for SSH-ing to google.com hosts would include "tcp://*.
> google.com:22" in its manifest.
>
> Mike, were you thinking along those lines for permission support for the
> socket API?
>
> Mihai
>
>

[Mihai Parparita -not on Chrome, 2012-03-01 18:17:41]

LGTM so that I'm not holding this CL up.

On Wed, Feb 29, 2012 at 10:53 PM, Dmitry Polukhin <dpolukhin@chromium.org>wrote:

> Yes, we are talking about the same thing. But it is not implemented for
> sockets yet:
>
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/extensions...
>
> I could be tricky part of it because of IP addresses (IP v4 and IP v6) and
> not fully qualified host names. Pepper allows making host resolution and
> connection using IP address. Also we will need to invent something for TCP
> server sockets (i.e. permission to listen TCP port on localhost)
> and distinguish it from permission to connect to given port on
> localhost. Perhaps for Pepper socket someone will need to specify "*://*:*"
> i.e. everywhere because for most frequent use cases it will require anyway
> (SSH client and other remote access like apps).
>

Agreed about it being tricky. My only concern was that if we're currently
checking just for the socket API permission, and later we check for the
host permission, existing extensions will break (this doesn't apply to
Mike's JS API, since that's still in experimental, so no backwards
compatibility is required). If that's not a concern for for Pepper either,
then I see no issue with this landing.

Mihai

[Dmitry Polukhin, 2012-03-02 06:58:02]

I think it is fine if we change it later. Enabling sockets via extension
permission now will be used mostly for testing, for example, on Chrome OS
where it is hard to add command line switch --allow-nacl-socket-api.
Production things relay on whitelist.

On Thu, Mar 1, 2012 at 10:17 PM, Mihai Parparita <mihaip@chromium.org>wrote:

> LGTM so that I'm not holding this CL up.
>
> On Wed, Feb 29, 2012 at 10:53 PM, Dmitry Polukhin
<dpolukhin@chromium.org>wrote:
>
>> Yes, we are talking about the same thing. But it is not implemented for
>> sockets yet:
>>
>>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/extensions...
>>
>> I could be tricky part of it because of IP addresses (IP v4 and IP v6)
>> and not fully qualified host names. Pepper allows making host resolution
>> and connection using IP address. Also we will need to invent something for
>> TCP server sockets (i.e. permission to listen TCP port on localhost)
>> and distinguish it from permission to connect to given port on
>> localhost. Perhaps for Pepper socket someone will need to specify "*://*:*"
>> i.e. everywhere because for most frequent use cases it will require anyway
>> (SSH client and other remote access like apps).
>>
>
> Agreed about it being tricky. My only concern was that if we're currently
> checking just for the socket API permission, and later we check for the
> host permission, existing extensions will break (this doesn't apply to
> Mike's JS API, since that's still in experimental, so no backwards
> compatibility is required). If that's not a concern for for Pepper either,
> then I see no issue with this landing.
>
> Mihai
>

[commit-bot: I haz the power, 2012-03-02 06:58:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/9539011/7002