Gets the peer address (if available) in server requests.

Gets the peer address (if available) in server requests.

R=byungchul@chromium.org, lcwu@chromium.org, pfeldman@chromium.org, rsleevi@chromium.org
BUG=347770
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258424

[gunsch, 2014-03-03 17:02:39]

+rsleevi

[Ryan Sleevi, 2014-03-03 17:13:12]

Please provide more details on the linked bug.

Two (deep) concerns:
1) std::string is certainly the wrong data type for peer & port
2) Chromecast using this interface at all, which is not a
well-designed/well-supported piece of net/ infrastructure (eg: it blocks
threads) and not recommended for projects to use (it's only used by Devtools, as
a historical artifact of an incomplete migration away from it, AIUI)

I think we're (net/ OWNERS) happy to help figure out what's right for
Chromecast, and I'm happy to view this in a transitional light to getting to
that point. That said, the justification for this change needs better
documentation (eg: on the bug)

[gunsch, 2014-03-03 17:32:32]

+byungchul

Both reasonable concerns. From my side, I'm trying to upstream some changes
currently in Chromecast's local fork of Chromium as part of our process to
"unfork" the codebase.

Byungchul can comment on this much more deeply, but my understanding is that
net::HttpServer is currently a pretty integral part of our system. The cast
receiver code (for communicating with sender devices) relies fairly heavily on
it. We start an instance of net::HttpServer on its own thread and attach a
net::HttpServer::Delegate for most of our logic. So it's probably disconcerting
to know that it's considered a "historical artifact" from your side.

Viewing this from a transitional perspective, is HttpServer expected to stick
around? Is there other code within chromium that accomplishes roughly the same
that is recommended instead?

[Ryan Sleevi, 2014-03-03 17:41:58]

On 2014/03/03 17:32:32, gunsch wrote:
> +byungchul
> 
> Both reasonable concerns. From my side, I'm trying to upstream some changes
> currently in Chromecast's local fork of Chromium as part of our process to
> "unfork" the codebase.
> 
> Byungchul can comment on this much more deeply, but my understanding is that
> net::HttpServer is currently a pretty integral part of our system. The cast
> receiver code (for communicating with sender devices) relies fairly heavily on
> it. We start an instance of net::HttpServer on its own thread and attach a
> net::HttpServer::Delegate for most of our logic. So it's probably
disconcerting
> to know that it's considered a "historical artifact" from your side.
> 
> Viewing this from a transitional perspective, is HttpServer expected to stick
> around? Is there other code within chromium that accomplishes roughly the same
> that is recommended instead?

We have two HttpServers right now - this and the one in net/test/embedded_server

Neither is strongly owned by the net/ team (the migrations were to make the code
more accessible)
Neither are known to be robust against 'malicious' data (eg: they presume the
peer is a trusted entity who would never do bad)
Both block (as by virtue of StreamListenSocket implementation details)

My current understanding is that no one is working right now to do the migration
away, other than rough promises, and with the goal being meeting those above
three criteria. Let me talk to some of the other net/ OWNERS to make sure
nothing has changed recently.

Note: We're _not_ talking about doing away with HTTP serving - we still need
that for DevTools. But making sure our interfaces are robust, even under
hostile/remote connections, and _especially_ when in the browser process, is
very important.

[gunsch, 2014-03-03 18:45:18]

On 2014/03/03 17:41:58, Ryan Sleevi wrote:
> On 2014/03/03 17:32:32, gunsch wrote:
> > +byungchul
> > 
> > Both reasonable concerns. From my side, I'm trying to upstream some changes
> > currently in Chromecast's local fork of Chromium as part of our process to
> > "unfork" the codebase.
> > 
> > Byungchul can comment on this much more deeply, but my understanding is that
> > net::HttpServer is currently a pretty integral part of our system. The cast
> > receiver code (for communicating with sender devices) relies fairly heavily
on
> > it. We start an instance of net::HttpServer on its own thread and attach a
> > net::HttpServer::Delegate for most of our logic. So it's probably
> disconcerting
> > to know that it's considered a "historical artifact" from your side.
> > 
> > Viewing this from a transitional perspective, is HttpServer expected to
stick
> > around? Is there other code within chromium that accomplishes roughly the
same
> > that is recommended instead?
> 
> We have two HttpServers right now - this and the one in
net/test/embedded_server
> 
> Neither is strongly owned by the net/ team (the migrations were to make the
code
> more accessible)
> Neither are known to be robust against 'malicious' data (eg: they presume the
> peer is a trusted entity who would never do bad)
> Both block (as by virtue of StreamListenSocket implementation details)
> 
> My current understanding is that no one is working right now to do the
migration
> away, other than rough promises, and with the goal being meeting those above
> three criteria. Let me talk to some of the other net/ OWNERS to make sure
> nothing has changed recently.
> 
> Note: We're _not_ talking about doing away with HTTP serving - we still need
> that for DevTools. But making sure our interfaces are robust, even under
> hostile/remote connections, and _especially_ when in the browser process, is
> very important.

I spoke with Byungchul today on this; we're more than aware of the blocking
issue and we currently have an implementation in our fork that fixes blocking in
StreamListenSocket that we'd also like to upstream (obviously not this CL).

With regards to your second concern, is HttpServer currently running in a
different process for DevTools?

As far as this CL's implementation, you're right that std::string is silly; I
had just upstreamed what we had locally. I converted to an IPEndPoint.

[byungchul, 2014-03-05 03:13:46]

On 2014/03/03 18:45:18, gunsch wrote:
> On 2014/03/03 17:41:58, Ryan Sleevi wrote:
> > On 2014/03/03 17:32:32, gunsch wrote:
> > > +byungchul
> > > 
> > > Both reasonable concerns. From my side, I'm trying to upstream some
changes
> > > currently in Chromecast's local fork of Chromium as part of our process to
> > > "unfork" the codebase.
> > > 
> > > Byungchul can comment on this much more deeply, but my understanding is
that
> > > net::HttpServer is currently a pretty integral part of our system. The
cast
> > > receiver code (for communicating with sender devices) relies fairly
heavily
> on
> > > it. We start an instance of net::HttpServer on its own thread and attach a
> > > net::HttpServer::Delegate for most of our logic. So it's probably
> > disconcerting
> > > to know that it's considered a "historical artifact" from your side.
> > > 
> > > Viewing this from a transitional perspective, is HttpServer expected to
> stick
> > > around? Is there other code within chromium that accomplishes roughly the
> same
> > > that is recommended instead?
> > 
> > We have two HttpServers right now - this and the one in
> net/test/embedded_server
> > 
> > Neither is strongly owned by the net/ team (the migrations were to make the
> code
> > more accessible)
> > Neither are known to be robust against 'malicious' data (eg: they presume
the
> > peer is a trusted entity who would never do bad)
> > Both block (as by virtue of StreamListenSocket implementation details)
> > 
> > My current understanding is that no one is working right now to do the
> migration
> > away, other than rough promises, and with the goal being meeting those above
> > three criteria. Let me talk to some of the other net/ OWNERS to make sure
> > nothing has changed recently.
> > 
> > Note: We're _not_ talking about doing away with HTTP serving - we still need
> > that for DevTools. But making sure our interfaces are robust, even under
> > hostile/remote connections, and _especially_ when in the browser process, is
> > very important.
> 
> I spoke with Byungchul today on this; we're more than aware of the blocking
> issue and we currently have an implementation in our fork that fixes blocking
in
> StreamListenSocket that we'd also like to upstream (obviously not this CL).
> 
> With regards to your second concern, is HttpServer currently running in a
> different process for DevTools?
> 
> As far as this CL's implementation, you're right that std::string is silly; I
> had just upstreamed what we had locally. I converted to an IPEndPoint.

Chromecast uses this http server
1) for setup
2) for cast v1, DIAL and websocket between cast sender app and chromecast, and
between cast receiver app and chromecast
3) for cast v2, websocket between cast receiver app and chromecast

This upstream merge is to support other cast receiver device including molly
which don't need 1), and cast v1 will be shutdown soon. So, this web server is
mainly used for cast v2 websocket connected from local cast receiver app in the
perspective of chromium master source tree.
In cast v2, peer address here is used to determine if requests of websocket are
coming from local or not. No external api's are open in cast v2 case.

Even with setup and cast v2, since we use this http server with a set of urls
not modifiable, I don't think big security problems may exist here. For example,
POST requests for setup api (though they are only for chromecast) must be in
format of JSON, and cast v1 websocket data is just passed by up to javascript
app.

[Ryan Sleevi, 2014-03-05 03:20:47]

On 2014/03/05 03:13:46, byungchul wrote:
> Chromecast uses this http server
> 1) for setup
> 2) for cast v1, DIAL and websocket between cast sender app and chromecast, and
> between cast receiver app and chromecast
> 3) for cast v2, websocket between cast receiver app and chromecast
> 
> This upstream merge is to support other cast receiver device including molly
> which don't need 1), and cast v1 will be shutdown soon. So, this web server is
> mainly used for cast v2 websocket connected from local cast receiver app in
the
> perspective of chromium master source tree.
> In cast v2, peer address here is used to determine if requests of websocket
are
> coming from local or not. No external api's are open in cast v2 case.
> 
> Even with setup and cast v2, since we use this http server with a set of urls
> not modifiable, I don't think big security problems may exist here. For
example,
> POST requests for setup api (though they are only for chromecast) must be in
> format of JSON, and cast v1 websocket data is just passed by up to javascript
> app.

I'm sorry, this doesn't quite parse, so apologies that I'm not familiar with it.

chromecast is a separate device, on a separate IP, accessing the HTTP server on
the network. This is, by definition, an "external" source. DevTools' original
design _only_ accepted on 127.0.0.1 - which was a far greater security posture.

I don't think you've fully understood my concerns here, or perhaps I've
misunderstood your reply. I'm happy to meet and discuss further, but basically,
I take the view of "If this will be exposed on a network - regardless of if it's
a local network or not - then we need to be looking much more carefully at this
code."

I suspect a quick meeting can help us in net/ understand your design - which
seems to be that you want to support a Chromecast device ("receiver", if I'm not
mistaken in your terminology) connecting to a WebSocket endpoint (FYI: The
WebSocket code in net/ is equally 'not in an ideal state') in Chrome ("sender",
if I'm not mistaken in your terminology).

This is where a good bug description and a design doc can help us understand
what your constraints are, and how we can prioritize both clean-up and
enhancement of this thus-far-unloved APIs.

[byungchul, 2014-03-05 03:37:50]

On 2014/03/05 03:20:47, Ryan Sleevi wrote:
> On 2014/03/05 03:13:46, byungchul wrote:
> > Chromecast uses this http server
> > 1) for setup
> > 2) for cast v1, DIAL and websocket between cast sender app and chromecast,
and
> > between cast receiver app and chromecast
> > 3) for cast v2, websocket between cast receiver app and chromecast
> > 
> > This upstream merge is to support other cast receiver device including molly
> > which don't need 1), and cast v1 will be shutdown soon. So, this web server
is
> > mainly used for cast v2 websocket connected from local cast receiver app in
> the
> > perspective of chromium master source tree.
> > In cast v2, peer address here is used to determine if requests of websocket
> are
> > coming from local or not. No external api's are open in cast v2 case.
> > 
> > Even with setup and cast v2, since we use this http server with a set of
urls
> > not modifiable, I don't think big security problems may exist here. For
> example,
> > POST requests for setup api (though they are only for chromecast) must be in
> > format of JSON, and cast v1 websocket data is just passed by up to
javascript
> > app.
> 
> I'm sorry, this doesn't quite parse, so apologies that I'm not familiar with
it.
> 
> chromecast is a separate device, on a separate IP, accessing the HTTP server
on
> the network. This is, by definition, an "external" source. DevTools' original
> design _only_ accepted on 127.0.0.1 - which was a far greater security
posture.
> 
> I don't think you've fully understood my concerns here, or perhaps I've
> misunderstood your reply. I'm happy to meet and discuss further, but
basically,
> I take the view of "If this will be exposed on a network - regardless of if
it's
> a local network or not - then we need to be looking much more carefully at
this
> code."
> 
> I suspect a quick meeting can help us in net/ understand your design - which
> seems to be that you want to support a Chromecast device ("receiver", if I'm
not
> mistaken in your terminology) connecting to a WebSocket endpoint (FYI: The
> WebSocket code in net/ is equally 'not in an ideal state') in Chrome
("sender",
> if I'm not mistaken in your terminology).
> 
> This is where a good bug description and a design doc can help us understand
> what your constraints are, and how we can prioritize both clean-up and
> enhancement of this thus-far-unloved APIs.

Yes, let's have a short meeting.

[Ryan Sleevi, 2014-03-11 01:32:42]

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.cc
File net/server/http_server.cc (right):

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.c...
net/server/http_server.cc:139: request.peer = peer;
Why not socket->GetPeerAddress(&request.peer)?

Seems you're leaving a potentially stale peer here.

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.c...
net/server/http_server.cc:160: }
Unrelated?

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.c...
net/server/http_server.cc:289: info->proto = buffer;
ditto

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
File net/socket/stream_listen_socket.cc (right):

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:106: return MapSystemError(errno);
|errno| won't be set on Win - it would be WSAGetLastError()

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:108: return ERR_FAILED;
ERR_ADDRESS_INVALID

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:110: return OK;
don't use else after return statements

if (getpeername(socket_, storage.addr, &storage.addr_len)))
  return MapSystemError(errno);  // BUGGED as above

if (!address->FromSockAddr(storage.addr, storage.addr_len))
  return ERR_ADDRESS_INVALID;

return OK;

(see other examples of calls to getpeername() in net/

[gunsch, 2014-03-17 16:57:15]

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.cc
File net/server/http_server.cc (right):

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.c...
net/server/http_server.cc:139: request.peer = peer;
On 2014/03/11 01:32:43, Ryan Sleevi wrote:
> Why not socket->GetPeerAddress(&request.peer)?
> 
> Seems you're leaving a potentially stale peer here.

Done.

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.c...
net/server/http_server.cc:160: }
On 2014/03/11 01:32:43, Ryan Sleevi wrote:
> Unrelated?

This was in our local changes to this class, but I don't think is actually
necessary. Removing.

https://codereview.chromium.org/181063006/diff/40001/net/server/http_server.c...
net/server/http_server.cc:289: info->proto = buffer;
On 2014/03/11 01:32:43, Ryan Sleevi wrote:
> ditto

Done.

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
File net/socket/stream_listen_socket.cc (right):

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:106: return MapSystemError(errno);
On 2014/03/11 01:32:43, Ryan Sleevi wrote:
> |errno| won't be set on Win - it would be WSAGetLastError()

Done.

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:108: return ERR_FAILED;
On 2014/03/11 01:32:43, Ryan Sleevi wrote:
> ERR_ADDRESS_INVALID

Hm, why ERR_ADDRESS_INVALID? In GetLocalAddress above, ERR_FAILED is used here,
and same in UDBSocketLibEvent::GetPeerAddress.

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:110: return OK;
On 2014/03/11 01:32:43, Ryan Sleevi wrote:
> don't use else after return statements
> 
> if (getpeername(socket_, storage.addr, &storage.addr_len)))
>   return MapSystemError(errno);  // BUGGED as above
> 
> if (!address->FromSockAddr(storage.addr, storage.addr_len))
>   return ERR_ADDRESS_INVALID;
> 
> return OK;
> 
> (see other examples of calls to getpeername() in net/

Done.

[Ryan Sleevi, 2014-03-17 19:46:03]

https://codereview.chromium.org/181063006/diff/60001/net/server/http_server_r...
File net/server/http_server_request_info.h (right):

https://codereview.chromium.org/181063006/diff/60001/net/server/http_server_r...
net/server/http_server_request_info.h:29: std::string proto;
Did you mean to remove this?

https://codereview.chromium.org/181063006/diff/60001/net/socket/stream_listen...
File net/socket/stream_listen_socket.cc (right):

https://codereview.chromium.org/181063006/diff/60001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:118: }
Longer term, I suspect we're going to want to split this into
stream_listen_socket_win.cc vs stream_listen_socket_posix.cc

That would be a good clean-up effort/refactoring (hi mef@!)

[Ryan Sleevi, 2014-03-17 19:47:22]

Sorry, LGTM, mod the one nit re: proto (which we can do in a follow-up CL. My
first question will be "What about net/http/http_version.h" for when you start
upstreaming that)

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
File net/socket/stream_listen_socket.cc (right):

https://codereview.chromium.org/181063006/diff/40001/net/socket/stream_listen...
net/socket/stream_listen_socket.cc:108: return ERR_FAILED;
On 2014/03/17 16:57:15, gunsch wrote:
> On 2014/03/11 01:32:43, Ryan Sleevi wrote:
> > ERR_ADDRESS_INVALID
> 
> Hm, why ERR_ADDRESS_INVALID? In GetLocalAddress above, ERR_FAILED is used
here,
> and same in UDBSocketLibEvent::GetPeerAddress.

Matches the TCPClientSocket.

I suspect the UDPClientSocket/this class are the ones lagging.

[gunsch, 2014-03-17 20:26:29]

https://codereview.chromium.org/181063006/diff/60001/net/server/http_server_r...
File net/server/http_server_request_info.h (right):

https://codereview.chromium.org/181063006/diff/60001/net/server/http_server_r...
net/server/http_server_request_info.h:29: std::string proto;
On 2014/03/17 19:46:04, Ryan Sleevi wrote:
> Did you mean to remove this?

Yep, thanks for catching. Done.

[gunsch, 2014-03-17 22:57:22]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-03-17 23:01:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gunsch@chromium.org/181063006/80001

[commit-bot: I haz the power, 2014-03-17 23:12:42]

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

[commit-bot: I haz the power, 2014-03-17 23:12:43]

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

[gunsch, 2014-03-20 18:10:17]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:11:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gunsch@chromium.org/181063006/80001

[commit-bot: I haz the power, 2014-03-20 21:51:46]

Change committed as 258424