Private API for extensions like ssh-client that need access to TCP.

Private API for extensions like ssh-client that need access to websocket-to-tcp proxy.

Access to TCP is obtained in following way:
(1) extension requests authentication token via call to private API like:
    chrome.webSocketProxyPrivate.getPassportForTCP('netbsd.org', 25, callback);
    if API validates this request
    then extension obtains some string token (in callback).
(2) open websocket connection to local websocket-to-tcp proxy ws://127.0.0.1:10101/tcpproxy
(3) pass header containing hostname, port and token obtained at step (1)
(4) communicate (in base64 encoding at this moment).

Proxy (running in chrome process) verifies those tokens by calls to InternalAuthVerification::VerifyPassport

Passports are one-time; no passport can be reused.
Passports expire in short period of time (20 seconds).

BUG=chromium-os:9667
TEST=unit_test,apitest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85757

[Denis Lagno, 2011-03-29 00:13:57]

please take first look.

(it is still not very polished and lacks unittest).

[zel, 2011-03-29 05:31:33]

http://codereview.chromium.org/6683060/diff/3018/base/crypto/symmetric_key_ma...
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/6683060/diff/3018/base/crypto/symmetric_key_ma...
base/crypto/symmetric_key_mac.cc:73: std::copy(random_data.Data,
random_data.Data + num_bytes, out);
you should avoid data copy here

change the function to:

GenerateRandomBytes(size_t num_bytes, uint8** out)

and allocate memory inside, don't copy+delete for no good reason

http://codereview.chromium.org/6683060/diff/3018/base/crypto/symmetric_key_ma...
base/crypto/symmetric_key_mac.cc:86: std::fill(random_bytes.begin(),
random_bytes.end(), 0u);
why cleaning here? the data is already in memory somewhere else

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.cc (right):

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.cc:40:
std::vector<std::string> arg_list;
why string array here? aren't you just passing hostname (string) and port (int)?

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.h (right):

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.h:17:
DECLARE_EXTENSION_FUNCTION_NAME("internalAuth.getToken")
the is 'private' API and we should pick a better name for it since the current
one really does not tell much - how about:

cryptoPrivate.getWebProxyToken ?

[Aaron Boodman, 2011-03-29 18:38:34]

It seems like this extension function is not wired up yet. It still needs to be
hooked into extension_api.json and extension_function_dispatcher.cc.

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_apitest.h (right):

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_apitest.h:121: bool enable_incognito,
thanks!

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.cc (right):

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.cc:21: char
k90ttyExtensionId[] = "cigbjabahnfnnmplhmjeolnhobhfjggp";
Why is this hard-coded? The convention is to just restrict private APIs to
component extensions.

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.cc:26: if
(!browser::ConvertArgListToVarValueMap(arg_list, &map))
Where is this defined? I don't see it in the current source or in your patch.

It looks really weird. Chrome extensions APIs support named parameters. Why not
just use that? Like:

chrome.internalWhatever.doThing({foo:"bar", hot:"dog"});

Or, depending on your needs, just use normal parameters:

chrome.internalWhatever.doThing("bar", "dog");

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.h (right):

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Doesn't there need to be a change to extensions_api.json too?

[Denis Lagno, 2011-04-04 18:18:02]

http://codereview.chromium.org/6683060/diff/3018/base/crypto/symmetric_key_ma...
File base/crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/6683060/diff/3018/base/crypto/symmetric_key_ma...
base/crypto/symmetric_key_mac.cc:73: std::copy(random_data.Data,
random_data.Data + num_bytes, out);
On 2011/03/29 05:31:34, zel wrote:
> you should avoid data copy here
> 
> change the function to:
> 
> GenerateRandomBytes(size_t num_bytes, uint8** out)
> 
> and allocate memory inside, don't copy+delete for no good reason

mac was the only platform that performed this copying.
I've adjusted code to avoid copy.

http://codereview.chromium.org/6683060/diff/3018/base/crypto/symmetric_key_ma...
base/crypto/symmetric_key_mac.cc:86: std::fill(random_bytes.begin(),
random_bytes.end(), 0u);
On 2011/03/29 05:31:34, zel wrote:
> why cleaning here? the data is already in memory somewhere else

yes, in memory somewhere else.  But looking into implementation for other
platforms it is clear that we expect that after key destruction it will be wiped
out from memory completely to avoid leak.  If we would not clean here then after
key destruction its data may still left here.

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.cc (right):

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.cc:40:
std::vector<std::string> arg_list;
On 2011/03/29 05:31:34, zel wrote:
> why string array here? aren't you just passing hostname (string) and port
(int)?

Done.

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.h (right):

http://codereview.chromium.org/6683060/diff/3018/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.h:17:
DECLARE_EXTENSION_FUNCTION_NAME("internalAuth.getToken")
On 2011/03/29 05:31:34, zel wrote:
> the is 'private' API and we should pick a better name for it since the current
> one really does not tell much - how about:
> 
> cryptoPrivate.getWebProxyToken ?

Done.

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.cc (right):

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.cc:21: char
k90ttyExtensionId[] = "cigbjabahnfnnmplhmjeolnhobhfjggp";
On 2011/03/29 18:38:34, Aaron Boodman wrote:
> Why is this hard-coded? The convention is to just restrict private APIs to
> component extensions.

I removed it.  However it may reappear in future -- this API is expected to be
provided to several extensions and we may want to restrict port range in
different ways for different extensions.

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.cc:26: if
(!browser::ConvertArgListToVarValueMap(arg_list, &map))
On 2011/03/29 18:38:34, Aaron Boodman wrote:
> Where is this defined? I don't see it in the current source or in your patch.

It was declared in chrome/browser/internal_auth.h and defined in
chrome/browser/internal_auth.cc
However it is not needed anymore and was removed.

> It looks really weird. Chrome extensions APIs support named parameters. Why
not
> just use that? Like:
> 
> chrome.internalWhatever.doThing({foo:"bar", hot:"dog"});
> 
> Or, depending on your needs, just use normal parameters:
> 
> chrome.internalWhatever.doThing("bar", "dog");

Done.

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_internal_auth_api.h (right):

http://codereview.chromium.org/6683060/diff/8023/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_internal_auth_api.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2011/03/29 18:38:34, Aaron Boodman wrote:
> Doesn't there need to be a change to extensions_api.json too?

Done.

[zel, 2011-04-05 14:59:24]

LGTM

with a nit below:

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_proxy_private_api.cc (right):

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:33: std::string id
= extension_id();
const std::string&

[Aaron Boodman, 2011-04-06 03:49:00]

Am I missing something, or will this be enabled for every extension? That would
be bad.

It should be limited to component extensions only. See extension.cc for other
examples of such APIs.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_proxy_private_api.cc (right):

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:14: // Elaborate
this check for future needs.
The convention is: TODO(<yourusername>): ....

But in this case, I don't think the comment is useful. This TODO could be
applied to any code.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:16: const
std::string& extension_id,
This wrapping is not necessary (the params would fit on less lines).

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:19: int
kLowestUnprivelegedPort = 1024;
const int ...

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:21: return
!extension_id.empty() && !hostname.empty() &&
Kinda weird to pass in extension_id and hostname and then only ensure that they
are non-empty.

The extension system will guarantee that extension_id is always non-empty, there
is no need to check it (everything else will explode if that is not true). If
you are concerned about host_name being non-empty, you can set minLength to 1 in
extension_api.json, and then again, the extension system will guarantee that
before it gets to you.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:31: int port;
Always initialize primitives.

[Denis Lagno, 2011-04-06 18:09:22]

On 2011/04/06 03:49:00, Aaron Boodman wrote:
> Am I missing something, or will this be enabled for every extension? That
would
> be bad.
> It should be limited to component extensions only. See extension.cc for other
> examples of such APIs.

yes, it is for component extensions only.
I've made necessary changes.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_proxy_private_api.cc (right):

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:14: // Elaborate
this check for future needs.
On 2011/04/06 03:49:00, Aaron Boodman wrote:
> The convention is: TODO(<yourusername>): ....
> 
> But in this case, I don't think the comment is useful. This TODO could be
> applied to any code.

Intention was to express the notion that this function is more like piece of
configuration rather than piece of code.  It configures access restrictions. 
Reformulated.

Oh, I see, problem seems to be in that you probably consider access to
particular private API as atomic piece of permission, so we should not add
additional restrictions depending on extension that uses it..

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:16: const
std::string& extension_id,
On 2011/04/06 03:49:00, Aaron Boodman wrote:
> This wrapping is not necessary (the params would fit on less lines).

Done.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:19: int
kLowestUnprivelegedPort = 1024;
On 2011/04/06 03:49:00, Aaron Boodman wrote:
> const int ...

Done.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:21: return
!extension_id.empty() && !hostname.empty() &&
On 2011/04/06 03:49:00, Aaron Boodman wrote:
> Kinda weird to pass in extension_id and hostname and then only ensure that
they
> are non-empty.

as I said above, this function is just placeholder for real access restrictions.

Done.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:31: int port;
On 2011/04/06 03:49:00, Aaron Boodman wrote:
> Always initialize primitives.

Done.

http://codereview.chromium.org/6683060/diff/33042/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_proxy_private_api.cc:33: std::string id
= extension_id();
On 2011/04/05 14:59:25, zel wrote:
> const std::string&

Done.

[Aaron Boodman, 2011-04-07 03:17:01]

http://codereview.chromium.org/6683060/diff/43025/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6683060/diff/43025/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4686: "namespace":
"webproxyPrivate",
I just realized this: we already have a 'proxy' API in the extension system that
this might get confused with. Can you think of a better name?

What about just renaming chromeOSInfoPrivate.get to chromeosPrivate.getInfo, and
then renaming this to chromeOSPrivate.getTCPProxyToken() ?

[zel, 2011-04-07 03:59:42]

chromeosPrivate will be used for different purpose than this API - we are
expecting that only our help app will use this while this web socket proxy
business has a special purpose (which I will explain offline),

I would rather call this API as something else - networkPrivate?



On Wed, Apr 6, 2011 at 8:17 PM, <aa@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6683060/diff/43025/chrome/common/extensions/ap...
> File chrome/common/extensions/api/extension_api.json (right):
>
>
>
http://codereview.chromium.org/6683060/diff/43025/chrome/common/extensions/ap...
> chrome/common/extensions/api/extension_api.json:4686: "namespace":
> "webproxyPrivate",
> I just realized this: we already have a 'proxy' API in the extension
> system that this might get confused with. Can you think of a better
> name?
>
> What about just renaming chromeOSInfoPrivate.get to
> chromeosPrivate.getInfo, and then renaming this to
> chromeOSPrivate.getTCPProxyToken() ?
>
>
> http://codereview.chromium.org/6683060/
>

[Aaron Boodman, 2011-04-09 23:58:45]

On 2011/04/07 03:59:42, zel wrote:
> chromeosPrivate will be used for different purpose than this API - we are
> expecting that only our help app will use this while this web socket proxy
> business has a special purpose (which I will explain offline),
> 
> I would rather call this API as something else - networkPrivate?

networkPrivate is a pretty general name. If the point of this API is to proxy
websocket, then how about webSocketProxyPrivate ?

[Aaron Boodman, 2011-04-10 00:04:43]

Also, you need tests.

http://codereview.chromium.org/6683060/diff/43025/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webproxy_private_api.cc (right):

http://codereview.chromium.org/6683060/diff/43025/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webproxy_private_api.cc:15: bool
CheckCredentials(
I think you should just remove this and add it back when it is needed.

http://codereview.chromium.org/6683060/diff/43025/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webproxy_private_api.cc:23: std::string
hostname;
I think it would be good to add a CHECK(extension_->location() ==
Extension::COMPONENT) here, just for safety.

[Denis Lagno, 2011-05-13 22:02:52]

I've reflected all comments (and added apitest).

AFAIK Aaron is on extended leave so if he does not read this now then I assume
he is happy about those changes.

http://codereview.chromium.org/6683060/diff/43025/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webproxy_private_api.cc (right):

http://codereview.chromium.org/6683060/diff/43025/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webproxy_private_api.cc:15: bool
CheckCredentials(
On 2011/04/10 00:04:43, Aaron Boodman wrote:
> I think you should just remove this and add it back when it is needed.

Placed some real content here

http://codereview.chromium.org/6683060/diff/43025/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webproxy_private_api.cc:23: std::string
hostname;
On 2011/04/10 00:04:43, Aaron Boodman wrote:
> I think it would be good to add a CHECK(extension_->location() ==
> Extension::COMPONENT) here, just for safety.

Done.

http://codereview.chromium.org/6683060/diff/43025/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6683060/diff/43025/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4686: "namespace":
"webproxyPrivate",
On 2011/04/07 03:17:01, Aaron Boodman wrote:
> I just realized this: we already have a 'proxy' API in the extension system
that
> this might get confused with. Can you think of a better name?
> 
> What about just renaming chromeOSInfoPrivate.get to chromeosPrivate.getInfo,
and
> then renaming this to chromeOSPrivate.getTCPProxyToken() ?

I've renamed to webSocketProxyPrivate.getPassportForTCP

[Dmitry Polukhin, 2011-05-16 09:41:39]

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_controller.cc (right):

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:46: // Configure allowed
origins. Empty vector allows any origin.
Perhaps it make sense to add command line option to enable the proxy for
extension ID specified in command line - it may be required for testing.

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.cc
(right):

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:60:
delay_response = false;
It looks like some debug code.

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.h (right):

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_socket_proxy_private_api.h:18: virtual
~WebSocketProxyPrivateGetPassportForTCPFunction() {}
Please definition to .cc file. Clang bot will complain.

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/internal_auth.h
File chrome/browser/internal_auth.h (right):

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/internal_aut...
chrome/browser/internal_auth.h:71: friend class
::InternalAuthTest_BasicGeneration_Test;
Please use FRIEND_TEST macro.

http://codereview.chromium.org/6683060/diff/71065/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html
(right):

http://codereview.chromium.org/6683060/diff/71065/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html:7:
chrome.test.assertEq(msg.data, 'YWxvaGEK'); /* base64-encoded string "aloha\n"
*/
Please move comment on separate line.

[Denis Lagno, 2011-05-16 10:40:36]

Adding Chang as reviewer for crypto/ changes

[Denis Lagno, 2011-05-16 18:03:45]

What is the rule for naming javascript api/permission private?

In current codebase private apis are exactly apis restricted to component
extensions.

api in this CL cannot be restricted to component extensions since we have
non-component users, but still it is restricted to selected extensions.

Should this api/permission still be called Private?

[Erik does not do reviews, 2011-05-16 20:27:58]

If it's restricted so that a random third party developer couldn't develop
against it, then it should be named with Private.

What mechanism are you using to restrict access?  A hard-coded list of ids?
 Why can't they be component extensions?  One of the reason for requiring
component extensions is that we can be more aggressive about changing the
APIs without worrying about breaking compatibility.  Another reason is that
it's a simpler mechanism that's controlled through one place, so we're a bit
more clear about the risk and likelihood of accidentally leaking the
permission outside of the intended reach.

Erik


On Mon, May 16, 2011 at 11:03 AM, <dilmah@chromium.org> wrote:

> What is the rule for naming javascript api/permission private?
>
> In current codebase private apis are exactly apis restricted to component
> extensions.
>
> api in this CL cannot be restricted to component extensions since we have
> non-component users, but still it is restricted to selected extensions.
>
> Should this api/permission still be called Private?
>
>
>
> http://codereview.chromium.org/6683060/
>

[Denis Lagno, 2011-05-16 21:14:21]

On 2011/05/16 20:27:58, Erik Kay wrote:
> If it's restricted so that a random third party developer couldn't develop
> against it, then it should be named with Private.

ok

> What mechanism are you using to restrict access?  A hard-coded list of ids?

yes

>  Why can't they be component extensions?

AFAIU this extension is not tiny and not universally needed so it may be
unacceptable to bundle it with chrome.
Another reason may be that being component extension implies that its developers
should be chromium committers and it may be burdensome from
administrative/licensing point of view.

[Erik does not do reviews, 2011-05-16 21:34:08]

On Mon, May 16, 2011 at 2:14 PM, <dilmah@chromium.org> wrote:

> On 2011/05/16 20:27:58, Erik Kay wrote:
>
>> If it's restricted so that a random third party developer couldn't develop
>> against it, then it should be named with Private.
>>
>
> ok
>
>
>  What mechanism are you using to restrict access?  A hard-coded list of
>> ids?
>>
>
> yes
>
>
>   Why can't they be component extensions?
>>
>
> AFAIU this extension is not tiny and not universally needed so it may be
> unacceptable to bundle it with chrome.
> Another reason may be that being component extension implies that its
> developers
> should be chromium committers and it may be burdensome from
> administrative/licensing point of view.


OK.  If we're going to be supporting some restricted third party apps, then
I agree that they shouldn't be component extensions, since that would give
them access to other private APIs that we may not want them to have access
to.

Erik

[wtc, 2011-05-16 23:23:29]

Review comments on the crypto/* files:

1. The changes to crypto/hmac* are good.

2. Please revert the changes to crypto/symmetric_key*.
Please use the recently-added base::RandBytes function in
"base/rand_util.h" instead.

http://codereview.chromium.org/6683060/diff/80006/crypto/hmac.h
File crypto/hmac.h (right):

http://codereview.chromium.org/6683060/diff/80006/crypto/hmac.h#newcode55
crypto/hmac.h:55: const std::string& data, unsigned char* digest, int
digest_length) const;
Nit: the Style Guide seems to recommend we format this as
follows since the first parameter fits on the first line:

  bool Sign(const std::string& data,
            unsigned char* digest,
            int digest_length) const;

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/6683060/diff/80006/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/6683060/diff/80006/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:37: static bool GenerateRandomBytes(size_t size_in_bits,
uint8* out);
Please use the recently-added base::RandBytes function in
"base/rand_util.h".

[Dmitry Polukhin, 2011-05-17 10:58:39]

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy.h (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy.h:25: static const size_t
kConnPoolLimit = 200;
I think it should be even much smaller 20.

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_controller.cc (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:23: const
std::vector<std::string> kAllowedOriginsVector(
static objects with c-tors are prohibited in style guide
http://dev.chromium.org/developers/coding-style#Static_Variables

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:88: const char*
kAllowedOrigins[] = {
Redefinition?

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:123:
DCHECK(kAllowedOriginsVector.size() < 7) << "consider binary search";
Compile time assert with arraysize(kAllowedOrigins) is much better.

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:330:
Extension::kChromeosInfoPrivatePermission,
It shouldn't be a component extension.

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
chrome/common/extensions/extension_unittest.cc:1038:
skip.insert(Extension::kWebSocketProxyPrivatePermission);
Same here.

http://codereview.chromium.org/6683060/diff/80006/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html
(right):

http://codereview.chromium.org/6683060/diff/80006/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html:7:
chrome.test.assertEq(msg.data, 'YWxvaGEK'); /* base64-encoded string "aloha\n"
*/
Please use atob/btoa to make code more readable. And Don't split comment in 2
lines.

[Denis Lagno, 2011-05-17 22:15:07]

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_controller.cc (right):

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:46: // Configure allowed
origins. Empty vector allows any origin.
On 2011/05/16 09:41:39, Dmitry Polukhin wrote:
> Perhaps it make sense to add command line option to enable the proxy for
> extension ID specified in command line - it may be required for testing.

Done.

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.h (right):

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_socket_proxy_private_api.h:18: virtual
~WebSocketProxyPrivateGetPassportForTCPFunction() {}
On 2011/05/16 09:41:39, Dmitry Polukhin wrote:
> Please definition to .cc file. Clang bot will complain.

Done.

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/internal_auth.h
File chrome/browser/internal_auth.h (right):

http://codereview.chromium.org/6683060/diff/71065/chrome/browser/internal_aut...
chrome/browser/internal_auth.h:71: friend class
::InternalAuthTest_BasicGeneration_Test;
On 2011/05/16 09:41:39, Dmitry Polukhin wrote:
> Please use FRIEND_TEST macro.

Done.

http://codereview.chromium.org/6683060/diff/71065/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html
(right):

http://codereview.chromium.org/6683060/diff/71065/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html:7:
chrome.test.assertEq(msg.data, 'YWxvaGEK'); /* base64-encoded string "aloha\n"
*/
On 2011/05/16 09:41:39, Dmitry Polukhin wrote:
> Please move comment on separate line.

Done.

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy.h (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy.h:25: static const size_t
kConnPoolLimit = 200;
On 2011/05/17 10:58:39, Dmitry Polukhin wrote:
> I think it should be even much smaller 20.

Done.

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_controller.cc (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:23: const
std::vector<std::string> kAllowedOriginsVector(
On 2011/05/17 10:58:39, Dmitry Polukhin wrote:
> static objects with c-tors are prohibited in style guide
> http://dev.chromium.org/developers/coding-style#Static_Variables

Done.

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:88: const char*
kAllowedOrigins[] = {
On 2011/05/17 10:58:39, Dmitry Polukhin wrote:
> Redefinition?

Done.

http://codereview.chromium.org/6683060/diff/80006/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:123:
DCHECK(kAllowedOriginsVector.size() < 7) << "consider binary search";
On 2011/05/17 10:58:39, Dmitry Polukhin wrote:
> Compile time assert with arraysize(kAllowedOrigins) is much better.

Done.

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:330:
Extension::kChromeosInfoPrivatePermission,
On 2011/05/17 10:58:39, Dmitry Polukhin wrote:
> It shouldn't be a component extension.

it is different api

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/6683060/diff/80006/chrome/common/extensions/ex...
chrome/common/extensions/extension_unittest.cc:1038:
skip.insert(Extension::kWebSocketProxyPrivatePermission);
On 2011/05/17 10:58:39, Dmitry Polukhin wrote:
> Same here.

here comment is misleading.  Edited.

http://codereview.chromium.org/6683060/diff/80006/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html
(right):

http://codereview.chromium.org/6683060/diff/80006/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/web_socket_proxy_private/background.html:7:
chrome.test.assertEq(msg.data, 'YWxvaGEK'); /* base64-encoded string "aloha\n"
*/
On 2011/05/17 10:58:39, Dmitry Polukhin wrote:
> Please use atob/btoa to make code more readable. And Don't split comment in 2
> lines.

Done.

http://codereview.chromium.org/6683060/diff/80006/crypto/hmac.h
File crypto/hmac.h (right):

http://codereview.chromium.org/6683060/diff/80006/crypto/hmac.h#newcode55
crypto/hmac.h:55: const std::string& data, unsigned char* digest, int
digest_length) const;
On 2011/05/16 23:23:29, wtc wrote:
> Nit: the Style Guide seems to recommend we format this as
> follows since the first parameter fits on the first line:
> 
>   bool Sign(const std::string& data,
>             unsigned char* digest,
>             int digest_length) const;
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/6683060/diff/80006/crypto/symmetric_key.h
File crypto/symmetric_key.h (right):

http://codereview.chromium.org/6683060/diff/80006/crypto/symmetric_key.h#newc...
crypto/symmetric_key.h:37: static bool GenerateRandomBytes(size_t size_in_bits,
uint8* out);
On 2011/05/16 23:23:29, wtc wrote:
> Please use the recently-added base::RandBytes function in
> "base/rand_util.h".

Done.

[wtc, 2011-05-17 23:37:52]

LGTM on the change to crypto/*.

I have suggested changes for crypto/symmetric_key_mac.cc.

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc
File crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc...
crypto/symmetric_key_mac.cc:42: if (err != CSSM_OK) {
Although I prefer this form, it's more important to be
consistent.  So I prefer we not change this, or we should
change all instances of
  if (err)
in this file.

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc...
crypto/symmetric_key_mac.cc:81: std::fill(random_bytes, random_bytes +
key_size_in_bytes, 0);
You can use
  memset(random_bytes, 0, key_size_in_bytes);
here instead.  Then you don't need to change CreateRandomBytes
to return uint8_t*.

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc...
crypto/symmetric_key_mac.cc:147: : key_(static_cast<const char*>(key_data),
key_size_in_bits / 8) {
I was told to use reinterpret_cast to cast void* to foo*.
I don't remember why, but it may be the recommendations
of the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

So I prefer that we not change this cast.

[Dmitry Polukhin, 2011-05-18 06:50:47]

LGTM

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_controller.cc (right):

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:34: class Clearance {
Class name is strange to me. What about OriginValidator?

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.cc
(right):

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:22: #if
defined(OS_CHROMEOS)
Perhaps later we will have to move proxy in about:flags to be configurable on
other platforms as well to be able to test user code not only on Chrome OS.

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:52:
delay_response = false;
Please file P1 R13 issue about it. It needs to be fixed because there is race
condition here. So client code will be flaky (perhaps test also will be flaky).

[Denis Lagno, 2011-05-18 11:53:32]

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc
File crypto/symmetric_key_mac.cc (right):

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc...
crypto/symmetric_key_mac.cc:42: if (err != CSSM_OK) {
On 2011/05/17 23:37:52, wtc wrote:
> Although I prefer this form, it's more important to be
> consistent.  So I prefer we not change this, or we should
> change all instances of
>   if (err)
> in this file.

Done.

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc...
crypto/symmetric_key_mac.cc:81: std::fill(random_bytes, random_bytes +
key_size_in_bytes, 0);
On 2011/05/17 23:37:52, wtc wrote:
> You can use
>   memset(random_bytes, 0, key_size_in_bytes);
> here instead.  Then you don't need to change CreateRandomBytes
> to return uint8_t*.

better leave std::fill for consistency

http://codereview.chromium.org/6683060/diff/85029/crypto/symmetric_key_mac.cc...
crypto/symmetric_key_mac.cc:147: : key_(static_cast<const char*>(key_data),
key_size_in_bits / 8) {
On 2011/05/17 23:37:52, wtc wrote:
> I was told to use reinterpret_cast to cast void* to foo*.
> I don't remember why, but it may be the recommendations
> of the Style Guide:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting
> 
> So I prefer that we not change this cast.

Done.

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/chromeos/web...
File chrome/browser/chromeos/web_socket_proxy_controller.cc (right):

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/chromeos/web...
chrome/browser/chromeos/web_socket_proxy_controller.cc:34: class Clearance {
On 2011/05/18 06:50:47, Dmitry Polukhin wrote:
> Class name is strange to me. What about OriginValidator?

Done.

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_web_socket_proxy_private_api.cc
(right):

http://codereview.chromium.org/6683060/diff/88034/chrome/browser/extensions/e...
chrome/browser/extensions/extension_web_socket_proxy_private_api.cc:52:
delay_response = false;
On 2011/05/18 06:50:47, Dmitry Polukhin wrote:
> Please file P1 R13 issue about it. It needs to be fixed because there is race
> condition here. So client code will be flaky (perhaps test also will be
flaky).

ok