Check whether to enable token binding in preconnect.cc

Check whether to enable token binding in preconnect.cc

BUG=604486
Committed: https://crrev.com/8cdb0fb12346304fb53e51bd97a41fd22be63d67
Cr-Commit-Position: refs/heads/master@{#389250}

[nharper, 2016-04-01 00:01:27]

nharper@chromium.org changed reviewers:
+ mattm@chromium.org

[mattm, 2016-04-01 01:27:18]

https://codereview.chromium.org/1850043002/diff/1/chrome/browser/net/preconne...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1850043002/diff/1/chrome/browser/net/preconne...
chrome/browser/net/preconnect.cc:110:
ssl_config.token_binding_params.push_back(net::TB_PARAM_ECDSAP256);
Should this check privacy mode? Should it set ssl_config.channel_id_enabled
also?

[nharper, 2016-04-01 19:33:06]

nharper@chromium.org changed reviewers:
+ davidben@chromium.org

[nharper, 2016-04-01 19:33:06]

Added a privacy mode check to preconnect.cc. I also fixed the privacy mode check
in http_network_transaction.cc since we weren't disabling Token Binding in
privacy mode there.

https://codereview.chromium.org/1850043002/diff/1/chrome/browser/net/preconne...
File chrome/browser/net/preconnect.cc (right):

https://codereview.chromium.org/1850043002/diff/1/chrome/browser/net/preconne...
chrome/browser/net/preconnect.cc:110:
ssl_config.token_binding_params.push_back(net::TB_PARAM_ECDSAP256);
On 2016/04/01 01:27:18, mattm wrote:
> Should this check privacy mode? Should it set ssl_config.channel_id_enabled
> also?

ssl_config.channel_id_enabled is true by default, but this should probably check
privacy mode and disable channel ID and token binding if that's the case.

[davidben, 2016-04-01 20:51:57]

davidben@chromium.org changed reviewers:
+ mmenke@chromium.org

[davidben, 2016-04-01 20:51:58]

+mmenke. It feels like this logic shouldn't be duplicated between
//chrome/browser/net and //net, but I'm not familiar enough with these bits to
know if there's any hope for that.

[mmenke, 2016-04-01 21:07:22]

On 2016/04/01 20:51:58, davidben wrote:
> +mmenke. It feels like this logic shouldn't be duplicated between
> //chrome/browser/net and //net, but I'm not familiar enough with these bits to
> know if there's any hope for that.

+1 to this.

Also...Why do we enabled token binding by setting token_binding_params on a
per-request basis, instead of disabling it by setting enable_token_binding to
false, and making what token binding "params" we support a
per-HttpNetworkSession thing?

[nharper, 2016-04-02 02:45:45]

On 2016/04/01 21:07:22, mmenke wrote:
> On 2016/04/01 20:51:58, davidben wrote:
> > +mmenke. It feels like this logic shouldn't be duplicated between
> > //chrome/browser/net and //net, but I'm not familiar enough with these bits
to
> > know if there's any hope for that.
> 
> +1 to this.
> 
> Also...Why do we enabled token binding by setting token_binding_params on a
> per-request basis, instead of disabling it by setting enable_token_binding to
> false, and making what token binding "params" we support a
> per-HttpNetworkSession thing?

The reason why it's set up so that SSLConfig::token_binding_params is flipped on
per-request, instead of setting the list of token_binding_params on a
per-HttpNetworkSession thing and then flipping it off per-request was so that
there's only one member in SSLConfig instead of two. I'd be fine doing this
differently if that makes other bits easier (like what I mention next).

For both token binding and channel ID, the only per-request check that I'm
seeing done for whether to enable it or not is whether privacy mode is enabled.
Since SSL socket pools are sharded by whether privacy mode is enabled (among
other things), would it make sense to do the check for disabling channel ID (and
token binding) at the same time? I'm thinking something like in SSLConnectJob
checking privacy mode, shoving that into the SSLClientSocketContext, and
checking that in the SSLClientSocket.

[mmenke, 2016-04-04 15:54:20]

On 2016/04/02 02:45:45, nharper (OOO thru 4-8) wrote:
> On 2016/04/01 21:07:22, mmenke wrote:
> > On 2016/04/01 20:51:58, davidben wrote:
> > > +mmenke. It feels like this logic shouldn't be duplicated between
> > > //chrome/browser/net and //net, but I'm not familiar enough with these
bits
> to
> > > know if there's any hope for that.
> > 
> > +1 to this.
> > 
> > Also...Why do we enabled token binding by setting token_binding_params on a
> > per-request basis, instead of disabling it by setting enable_token_binding
to
> > false, and making what token binding "params" we support a
> > per-HttpNetworkSession thing?
> 
> The reason why it's set up so that SSLConfig::token_binding_params is flipped
on
> per-request, instead of setting the list of token_binding_params on a
> per-HttpNetworkSession thing and then flipping it off per-request was so that
> there's only one member in SSLConfig instead of two. I'd be fine doing this
> differently if that makes other bits easier (like what I mention next).
> 
> For both token binding and channel ID, the only per-request check that I'm
> seeing done for whether to enable it or not is whether privacy mode is
enabled.
> Since SSL socket pools are sharded by whether privacy mode is enabled (among
> other things), would it make sense to do the check for disabling channel ID
(and
> token binding) at the same time? I'm thinking something like in SSLConnectJob
> checking privacy mode, shoving that into the SSLClientSocketContext, and
> checking that in the SSLClientSocket.

What I'd really like to do:

1) Remove ssl_config as an argument from PreconnectStreams.
2) Remove all the duplicate logic to set up SSLConfigs.
3) Make the SSLConfig actually populate all fields in SSLConfigs.  As it, it's
more of a PartialSSLConfigService.

Not going to block progress here on doing all of this, but I think preconnect
knowing about "TB_PARAM_ECDSAP256" is a step in the wrong direction.

Full disclaimer: I know very little about SSLConfig.  David:  I don't suppose
you know who would be best to make calls here?

[nharper, 2016-04-12 21:09:04]

On 2016/04/04 15:54:20, mmenke wrote:
> What I'd really like to do:
> 
> 1) Remove ssl_config as an argument from PreconnectStreams.
> 2) Remove all the duplicate logic to set up SSLConfigs.
> 3) Make the SSLConfig actually populate all fields in SSLConfigs.  As it, it's
> more of a PartialSSLConfigService.
> 
> Not going to block progress here on doing all of this, but I think preconnect
> knowing about "TB_PARAM_ECDSAP256" is a step in the wrong direction.
> 
> Full disclaimer: I know very little about SSLConfig.  David:  I don't suppose
> you know who would be best to make calls here?

I've done 1 and 2. 3 (changing SSLConfigService to do something more useful)
sounds like a good idea, but it isn't immediately clear on how to move the
de-duplication from #2 into SSLConfigService.

[mmenke, 2016-04-13 16:21:05]

Thanks for doing this, looks much better to me!

Could you please file a bug for this CL?

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:227: SSLConfigService* ssl_config_service() {
return ssl_config_service_.get(); }
Do we still need to expose this?

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:126:
session->GetNpnProtos(&server_ssl_config_.npn_protos);
I think these three lines are no longer needed?

[nharper, 2016-04-18 22:22:44]

Description was changed from

==========
Check whether to enable token binding in preconnect.cc

BUG=
==========

to

==========
Check whether to enable token binding in preconnect.cc

BUG=604486
==========

[nharper, 2016-04-18 22:22:55]

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:227: SSLConfigService* ssl_config_service() {
return ssl_config_service_.get(); }
On 2016/04/13 16:21:04, mmenke wrote:
> Do we still need to expose this?

There are a few other places still using this. BidirectionalStream uses it, but
if the new HttpNetworkSession::GetSSLConfig method is made to take in an
HttpRequestInfo pointer, it can pass in nullptr and use the new method.
(BidirectionalStream shares the same initial 3 lines of ssl_config_service(),
GetAlpnProtos, and GetNpnProtos.) There are other places outside of //net that
are also using it.

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1850043002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:126:
session->GetNpnProtos(&server_ssl_config_.npn_protos);
On 2016/04/13 16:21:05, mmenke wrote:
> I think these three lines are no longer needed?

If I fix two NPN tests in http_network_transaction_unittest.cc, net_unittests
passes with these lines removed. I'm not entirely sure that nothing uses
HttpNetworkTransaction between construction and the call to Start.

[mmenke, 2016-04-22 16:54:13]

LGTM, assuming the changed proxy behavior isn't a problem.  Sorry for slowness -
took two days off, and returned to be buried in reviews.

https://codereview.chromium.org/1850043002/diff/80001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1850043002/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:158: }
nit:  Remove braces

https://codereview.chromium.org/1850043002/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:159: proxy_ssl_config_ =
server_ssl_config_;
Hrm...We didn't enable channel ID on the proxy's config before, or enable
TB_PARAM_WHATEVER.  Is changing that a problem?  We're doing the same thing in
the preconnect path as well.

[nharper, 2016-04-22 19:05:02]

ptal

https://codereview.chromium.org/1850043002/diff/80001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1850043002/diff/80001/net/http/http_network_t...
net/http/http_network_transaction.cc:159: proxy_ssl_config_ =
server_ssl_config_;
On 2016/04/22 16:54:13, mmenke wrote:
> Hrm...We didn't enable channel ID on the proxy's config before, or enable
> TB_PARAM_WHATEVER.  Is changing that a problem?  We're doing the same thing in
> the preconnect path as well.

It looks like we unconditionally enabled channel ID on the proxy's config - the
old code here never touched procy_ssl_config_.channel_id_enabled, which defaults
to true. I feel most comfortable with not changing the behavior of it, by having
HttpNetworkSession::GetSSLConfig fill both the server and proxy SSLConfigs.

[mmenke, 2016-04-22 19:31:33]

On 2016/04/22 19:05:02, nharper wrote:
> ptal
> 
>
https://codereview.chromium.org/1850043002/diff/80001/net/http/http_network_t...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1850043002/diff/80001/net/http/http_network_t...
> net/http/http_network_transaction.cc:159: proxy_ssl_config_ =
> server_ssl_config_;
> On 2016/04/22 16:54:13, mmenke wrote:
> > Hrm...We didn't enable channel ID on the proxy's config before, or enable
> > TB_PARAM_WHATEVER.  Is changing that a problem?  We're doing the same thing
in
> > the preconnect path as well.
> 
> It looks like we unconditionally enabled channel ID on the proxy's config -
the
> old code here never touched procy_ssl_config_.channel_id_enabled, which
defaults
> to true. I feel most comfortable with not changing the behavior of it, by
having
> HttpNetworkSession::GetSSLConfig fill both the server and proxy SSLConfigs.

Ahh, assumed it defaulted to false.

LGTM!

[nharper, 2016-04-22 19:38:50]

The CQ bit was checked by nharper@chromium.org

[commit-bot: I haz the power, 2016-04-22 19:39:08]

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

[commit-bot: I haz the power, 2016-04-22 21:35:09]

Description was changed from

==========
Check whether to enable token binding in preconnect.cc

BUG=604486
==========

to

==========
Check whether to enable token binding in preconnect.cc

BUG=604486
==========

[commit-bot: I haz the power, 2016-04-22 21:35:11]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-22 21:36:30]

Description was changed from

==========
Check whether to enable token binding in preconnect.cc

BUG=604486
==========

to

==========
Check whether to enable token binding in preconnect.cc

BUG=604486

Committed: https://crrev.com/8cdb0fb12346304fb53e51bd97a41fd22be63d67
Cr-Commit-Position: refs/heads/master@{#389250}
==========

[commit-bot: I haz the power, 2016-04-22 21:36:31]

Patchset 7 (id:??) landed as
https://crrev.com/8cdb0fb12346304fb53e51bd97a41fd22be63d67
Cr-Commit-Position: refs/heads/master@{#389250}