Set Token-Binding HTTP header

Set Token-Binding HTTP header

Adds a new method to SSLClientSocket to get the Token Binding from an SSL
connection where Token Binding was negotiated, and uses that to add the
Set-Token-Binding HTTP header (only when Token Binding was negotiated).

BUG=467312
Committed: https://crrev.com/b7441ef2effe86324798710a82d8a006f5eb1395
Cr-Commit-Position: refs/heads/master@{#371347}

[nharper, 2015-09-30 21:53:33]

Fix usage of ScopedEVP_MD_CTX

[nharper, 2015-09-30 22:18:40]

rebase

[nharper, 2015-09-30 22:47:26]

nharper@chromium.org changed reviewers:
+ davidben@chromium.org, mattm@chromium.org, rsleevi@chromium.org

[nharper, 2015-09-30 22:47:27]

This is dependent on https://chromiumcodereview.appspot.com/1360633002/.

[mattm, 2015-10-01 01:53:01]

Is it possible to add tests?


(note: this is just comments from a first pass.)

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/http/http_s...
File net/http/http_stream.h (right):

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/http/http_s...
net/http/http_stream.h:156: // string.
document return value

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/socket/ssl_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/socket/ssl_...
net/socket/ssl_client_socket_openssl.cc:2374:
&TokenBindingExtension::ClientParseCallback, nullptr) != 0;
should this change be in the parent CL?

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/ssl/token_b...
File net/ssl/token_binding_openssl.cc (right):

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/ssl/token_b...
net/ssl/token_binding_openssl.cc:19: size_t out_len;
move these down to where they are first used

[nharper, 2015-10-01 20:25:18]

Minor changes

[nharper, 2015-10-01 20:25:47]

> Is it possible to add tests?

I knew I forgot something. I'm working on those now.

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/http/http_s...
File net/http/http_stream.h (right):

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/http/http_s...
net/http/http_stream.h:156: // string.
On 2015/10/01 01:53:01, mattm wrote:
> document return value

Done.

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/socket/ssl_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/socket/ssl_...
net/socket/ssl_client_socket_openssl.cc:2374:
&TokenBindingExtension::ClientParseCallback, nullptr) != 0;
On 2015/10/01 01:53:01, mattm wrote:
> should this change be in the parent CL?

Done.

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/ssl/token_b...
File net/ssl/token_binding_openssl.cc (right):

https://chromiumcodereview.appspot.com/1378613004/diff/100001/net/ssl/token_b...
net/ssl/token_binding_openssl.cc:19: size_t out_len;
On 2015/10/01 01:53:01, mattm wrote:
> move these down to where they are first used

Done.

[Ryan Sleevi, 2015-10-01 23:44:25]

As discussed in person, going to sit on this until we get some of the
SSLClientSocket bit sorted. Ping me when it's ready again for a thorough review
:)

[davidben, 2015-10-15 20:37:17]

(Reviewing the other CL now and not doing a full review here yet. This just
occurred to me as something we'll need to think about somewhat, unless we limit
the feature to HTTP/2.)

https://codereview.chromium.org/1378613004/diff/120001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/120001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:526: if (!SSL_get_tls_unique(ssl_, buf,
&len, arraysize(buf)))
Both this function and SSL_export_keying_material can do extremely stupid things
if we are mid-renegotiation. It's really not even clear what their semantics
are. We're going to need to be a little careful here. (Probably part of that
will fall on BoringSSL to just make those functions fail in the renego case, but
we should also think a bit on how everything else interacts.)

[nharper, 2015-10-31 01:23:45]

Add missing tlslite patch

[nharper, 2015-10-31 01:27:09]

Major changes: add commandline switch; do key lookup in HttpNetworkTransaction,
add tests

[nharper, 2015-10-31 01:29:00]

Patchset #10 (id:180001) has been deleted

[nharper, 2015-10-31 01:29:37]

Major changes: add commandline switch; do key lookup in HttpNetworkTransaction,
add tests

[nharper, 2015-10-31 01:30:22]

Patchset #10 (id:200001) has been deleted

[nharper, 2015-10-31 01:32:07]

Major changes: add commandline switch; do key lookup in HttpNetworkTransaction,
add tests

[nharper, 2015-10-31 01:33:22]

Patchset #9 (id:160001) has been deleted

[nharper, 2015-10-31 01:33:28]

Patchset #8 (id:140001) has been deleted

[nharper, 2015-10-31 01:38:46]

Description was changed from

==========
Set Sec-Token-Binding HTTP header

Adds a new method to SSLClientSocket to get the Token Binding from an SSL
connection where Token Binding was negotiated, and uses that to add the
Set-Token-Binding HTTP header (only when Token Binding was negotiated).

BUG=467312
==========

to

==========
Set Token-Binding HTTP header

Adds a new method to SSLClientSocket to get the Token Binding from an SSL
connection where Token Binding was negotiated, and uses that to add the
Set-Token-Binding HTTP header (only when Token Binding was negotiated).

BUG=467312
==========

[nharper, 2015-10-31 01:42:02]

This CL has been updated and mostly rewritten. Comparing patchset 8 to previous
ones probably isn't very useful.

Let me know if you'd like the tlslite changes broken out into a separate CL.

[nharper, 2015-11-02 17:54:46]

Remove extra SHA256 from signing

[nharper, 2015-11-04 02:59:19]

update to match change to SSLConfig in dependent patchset

[nharper, 2015-11-06 00:17:50]

rebase

[nharper, 2015-11-07 02:18:07]

Add UMA logging of Token Binding support and NetLog event for Token Binding key
lookup

[davidben, 2015-11-17 21:04:12]

(Didn't get to this today. Will look at it tomorrow.)

[davidben, 2015-11-18 20:49:01]

Phew. Large CL. Here's a first pass over it. I think it's basically good as far
as the shape of it. I haven't looked too closely at the unit tests yet.

https://codereview.chromium.org/1378613004/diff/300001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:550: // handshake.
I would probably say

// Enables token binding (draft-ietf-tokbind-protocol-02).

and not mention TLS. Maybe we'll do QUIC. And the important part is that the
HTTP code does it, not the TLS code.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:656: supported = UNSUPPORTED_KEY_PARAM;
This can never happen, no? You'll only negotiate the ones you set up in line
205. Also probably bad to look at this before the bool below.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:963:
base::Bind(&HttpNetworkTransaction::OnIOComplete, base::Unretained(this)),
io_callback_

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:974: rv = ERR_CHANNEL_ID_IMPORT_FAILED;
Is this possible?

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:1015: if (token_binding_key_) {
An HttpNetworkSession may run through a request multiple times (see
ResetStateForRestart). The server may change its mind on whether to negotiate TB
or whatever. You should probably reset everything in ResetStateForRestart,
otherwise this check is invalid and some other stuff might be weird.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:1054: return "";
If this or below fails, it should probably be fatal to the connection.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:1064: base::ReplaceChars(base64_header,
"/", "_", &base64_header);
base/base64url.h

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.h:17: #include "crypto/ec_private_key.h"
I think you can still forward-decl this.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h
File net/http/http_stream.h (right):

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:18: #include "crypto/ec_private_key.h"
Forward decl

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:153: 
Documentation?

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:154: virtual int GetProvidedTokenBindingWithKey(
Nit: Maybe WithKey -> ForKey? Not sure. That seems marginally better. WithKey
sounds like it might return binding + key.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:154: virtual int GetProvidedTokenBindingWithKey(
So, I'm a little confused about this "provided" vs "referred" business. Since
the signature doesn't include the type anyway (a little bit suspicious in
itself), why not make HttpStream just pass up GetSignedEKMForTokenBinding. We
can wrap it in "provided" outside.

The other motivation is if we push it up as high as assembling the
TokenBindingMessage, we can save an allocation and copy. (Honestly, I'm really
confused what's the point of that too. You can already emit a header multiple
times, so why not use that...)

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:155: const scoped_ptr<crypto::ECPrivateKey>& key,
crypto::ECPrivateKey* is how you spell a non-owning pointer. :-P I'm not sure
const references to scoped_ptr actually make sense.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:500:
tb_signed_ekm_map_(SignedEkmMap::NO_AUTO_EVICT),
With NO_AUTO_EVICT and no explicit evictions, this means we're not actually
using the MRU-ness, right? Seems we'd want to just set a fairly conservative one
and leave it at that.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:558: std::vector<uint8_t>* out) {
I think this deserves a comment like:

  // The same key will be asserted for multiple requests, so look up a
  // signature in the cache first.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:566: }
Nit: Newline here, probably.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:569: const char tb_ekm_label[] =
"EXPORTER-Token-Binding";
Nit: static const char kTbEkmLabel[]. Or maybe kTokenBindingEkmLabel[] or even
kTokenBindingExporterLabel[] to cut down on abbreviation soup?

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:573: size_t ekm_label_length =
arraysize(tb_ekm_label) - 1;
Rather than that long comment, how about just using strlen? I'm sure the
compiler will be smart enough to avoid the function call.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:586: if (!EVP_PKEY_sign(pctx.get(),
&out->front(), &sig_len, tb_ekm_buf,
vector_as_array(out) from base/stl_util.h, very soon to be replaced with
out->data().

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding.h
File net/ssl/token_binding.h (right):

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:18: // error.
You can return a net::Error to make that clearer since you're not muxing in an
output size with it.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:44: //     provided_token_binding(0),
referred_token_binding(1), (255)
This is somewhat less related, but what exactly is this provided vs. referred
thing supposed to be for? Whatever it is, we don't actually sign the type.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:75: bool ParseTokenBindingMessage(const std::string&
token_binding_message,
This doesn't appear to have any callers.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:77: std::string* signature);
StringPiece to avoid all the copies? This function could just take in
StringPiece and spit out StringPieces

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:84: bool VerifyEKMSignature(const std::string& ec_point,
This doesn't appear to have any callers.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
File net/ssl/token_binding_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:36: if (i2d_PublicKey(pkey, NULL) !=
kExpectedKeyLength)
It's kind of confusing what type you're using when you call i2d_PublicKey. It's
even kind of screwy because, despite the name, you aren't serializing to DER.

I'll get a better API later, but for now I'd use:
https://commondatastorage.googleapis.com/chromium-boringssl-docs/ec_key.h.htm...

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:45: }
You can save the extra copy by doing:

  uint8_t *buf;
  if (i2d_Foo(pkey, nullptr) != kExpectedKeyLength ||
      !CBB_add_space(&ec_point, &buf, kExpectedKeyLength) ||
      i2d_Foo(pkey, &buf) != kExpectedKeyLength ||
      !CBB_flush(out)) {
    return false;
  }

Which is generalizable to cases where you don't know the expected length by
using what the first call returned. (It's not generalizable to the EVP_PKEY_sign
case where you only get an upper-bound initially, but I'll add a CBB_reserve +
CBB_advance pair at some point when we need it.)

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:54: CBB tb_message, tb_messages;
tb_message vs tb_messages is kind of confusing. I'd probably just call the
second one 'child'.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:63:
reinterpret_cast<uint8_t*>(const_cast<char*>(token_binding.data())),
No need for the const_cast

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:90: !CBB_add_bytes(&token_binding,
&signed_ekm[0], signed_ekm.size()) ||
vector_as_array(&signed_ekm)

https://codereview.chromium.org/1378613004/diff/300001/third_party/tlslite/tl...
File third_party/tlslite/tlslite/session.py (right):

https://codereview.chromium.org/1378613004/diff/300001/third_party/tlslite/tl...
third_party/tlslite/tlslite/session.py:71: self.serverRandom = serverRandom
I don't think storing the client/server random on the Session is quite right. If
you resume a session, you will have different randoms on the connection, but the
same session.

https://codereview.chromium.org/1378613004/diff/300001/third_party/tlslite/tl...
third_party/tlslite/tlslite/session.py:135: """Returns the exported keying
material as defined in RFC 5705."""
Ditto. The client and server random used in the exporter are different on
session resumption. This probably wants to be a method on the connection.

https://codereview.chromium.org/1378613004/diff/300001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1378613004/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:47965: +<histogram
name="TokenBinding.Support" enum="TokenBinding.Support">
Probably wants a Net prefix.

https://codereview.chromium.org/1378613004/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:74718: +<enum
name="TokenBinding.Support" type="int">
I don't think enum names typically have dots in them.

[nharper, 2015-11-30 22:31:20]

rebase

[nharper, 2015-12-04 01:42:12]

reply to comments

[nharper, 2015-12-04 01:42:21]

https://codereview.chromium.org/1378613004/diff/300001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:550: // handshake.
On 2015/11/18 20:49:00, davidben (slow) wrote:
> I would probably say
> 
> // Enables token binding (draft-ietf-tokbind-protocol-02).
> 
> and not mention TLS. Maybe we'll do QUIC. And the important part is that the
> HTTP code does it, not the TLS code.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:656: supported = UNSUPPORTED_KEY_PARAM;
On 2015/11/18 20:49:00, davidben (slow) wrote:
> This can never happen, no? You'll only negotiate the ones you set up in line
> 205. Also probably bad to look at this before the bool below.

My thought here was if the server can negotiate token binding, but not with a
key param that we support, then record it here. Clearly that doesn't work. The
easier thing to do is drop UNSUPPORTED_KEY_PARAM from this enum altogether;
otherwise I'll need to plumb more info up into SSLInfo to expose what happened
in the extension at the ssl layer.

I've removed UNSUPPORTED_KEY_PARAM for now.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:963:
base::Bind(&HttpNetworkTransaction::OnIOComplete, base::Unretained(this)),
On 2015/11/18 20:49:00, davidben (slow) wrote:
> io_callback_

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:974: rv = ERR_CHANNEL_ID_IMPORT_FAILED;
On 2015/11/18 20:49:00, davidben (slow) wrote:
> Is this possible?

I can't see how this would be possible (GetOrCreateChannelID's documentation
indicates that token_binding_key_ will be filled if it returns success), but
SSLClientSocketOpenSSL has a similar check when looking up the channel ID key.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:1015: if (token_binding_key_) {
On 2015/11/18 20:49:00, davidben (slow) wrote:
> An HttpNetworkSession may run through a request multiple times (see
> ResetStateForRestart). The server may change its mind on whether to negotiate
TB
> or whatever. You should probably reset everything in ResetStateForRestart,
> otherwise this check is invalid and some other stuff might be weird.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:1054: return "";
On 2015/11/18 20:49:00, davidben (slow) wrote:
> If this or below fails, it should probably be fatal to the connection.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.cc:1064: base::ReplaceChars(base64_header,
"/", "_", &base64_header);
On 2015/11/18 20:49:00, davidben (slow) wrote:
> base/base64url.h

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_network_...
net/http/http_network_transaction.h:17: #include "crypto/ec_private_key.h"
On 2015/11/18 20:49:00, davidben (slow) wrote:
> I think you can still forward-decl this.

Yes I can. Done.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h
File net/http/http_stream.h (right):

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:18: #include "crypto/ec_private_key.h"
On 2015/11/18 20:49:00, davidben (slow) wrote:
> Forward decl

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:153: 
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> Documentation?

This method is now gone, and a passthru to
SSLClientSocket::GetSignedEKMForTokenBinding added in its place, with
documentation.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:154: virtual int GetProvidedTokenBindingWithKey(
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> Nit: Maybe WithKey -> ForKey? Not sure. That seems marginally better. WithKey
> sounds like it might return binding + key.

Method is gone, so naming is moot.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:154: virtual int GetProvidedTokenBindingWithKey(
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> So, I'm a little confused about this "provided" vs "referred" business. Since
> the signature doesn't include the type anyway (a little bit suspicious in
> itself), why not make HttpStream just pass up GetSignedEKMForTokenBinding. We
> can wrap it in "provided" outside.
> 
> The other motivation is if we push it up as high as assembling the
> TokenBindingMessage, we can save an allocation and copy. (Honestly, I'm really
> confused what's the point of that too. You can already emit a header multiple
> times, so why not use that...)

Pushing this up higher did clean up some stuff.

https://codereview.chromium.org/1378613004/diff/300001/net/http/http_stream.h...
net/http/http_stream.h:155: const scoped_ptr<crypto::ECPrivateKey>& key,
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> crypto::ECPrivateKey* is how you spell a non-owning pointer. :-P I'm not sure
> const references to scoped_ptr actually make sense.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:500:
tb_signed_ekm_map_(SignedEkmMap::NO_AUTO_EVICT),
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> With NO_AUTO_EVICT and no explicit evictions, this means we're not actually
> using the MRU-ness, right? Seems we'd want to just set a fairly conservative
one
> and leave it at that.

Correct. I've changed it to 10.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:558: std::vector<uint8_t>* out) {
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> I think this deserves a comment like:
> 
>   // The same key will be asserted for multiple requests, so look up a
>   // signature in the cache first.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:566: }
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> Nit: Newline here, probably.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:569: const char tb_ekm_label[] =
"EXPORTER-Token-Binding";
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> Nit: static const char kTbEkmLabel[]. Or maybe kTokenBindingEkmLabel[] or even
> kTokenBindingExporterLabel[] to cut down on abbreviation soup?

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:573: size_t ekm_label_length =
arraysize(tb_ekm_label) - 1;
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> Rather than that long comment, how about just using strlen? I'm sure the
> compiler will be smart enough to avoid the function call.

I forgot about strlen. Done.

https://codereview.chromium.org/1378613004/diff/300001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:586: if (!EVP_PKEY_sign(pctx.get(),
&out->front(), &sig_len, tb_ekm_buf,
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> vector_as_array(out) from base/stl_util.h, very soon to be replaced with
> out->data().

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding.h
File net/ssl/token_binding.h (right):

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:18: // error.
On 2015/11/18 20:49:00, davidben (behind on reviews) wrote:
> You can return a net::Error to make that clearer since you're not muxing in an
> output size with it.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:44: //     provided_token_binding(0),
referred_token_binding(1), (255)
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> This is somewhat less related, but what exactly is this provided vs. referred
> thing supposed to be for? Whatever it is, we don't actually sign the type.

The type (provided vs referred) is for the federated use case (which I haven't
implemented yet). A provided token binding on a connection between client and
server A is when the client uses its keypair for server A to sign, while a
referred token binding is when the client connects to server B and uses its
keypair for server A (as well as sending a provided token binding using the
keypair for B). This is so that server B can generate a token containing the
public key for server A that the client will present to server A.

In terms of not signing the type, I can't think of a danger of that (so long as
the exported keying material can't be replicated on another connection, but if
that's the case, then there are much larger protocol problems with token
binding).

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:75: bool ParseTokenBindingMessage(const std::string&
token_binding_message,
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> This doesn't appear to have any callers.

This is called in url_request_unittest.cc

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:77: std::string* signature);
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> StringPiece to avoid all the copies? This function could just take in
> StringPiece and spit out StringPieces

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding....
net/ssl/token_binding.h:84: bool VerifyEKMSignature(const std::string& ec_point,
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> This doesn't appear to have any callers.

This is called in url_request_unittest.cc

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
File net/ssl/token_binding_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:36: if (i2d_PublicKey(pkey, NULL) !=
kExpectedKeyLength)
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> It's kind of confusing what type you're using when you call i2d_PublicKey.
It's
> even kind of screwy because, despite the name, you aren't serializing to DER.
> 
> I'll get a better API later, but for now I'd use:
>
https://commondatastorage.googleapis.com/chromium-boringssl-docs/ec_key.h.htm...

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:45: }
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> You can save the extra copy by doing:
> 
>   uint8_t *buf;
>   if (i2d_Foo(pkey, nullptr) != kExpectedKeyLength ||
>       !CBB_add_space(&ec_point, &buf, kExpectedKeyLength) ||
>       i2d_Foo(pkey, &buf) != kExpectedKeyLength ||
>       !CBB_flush(out)) {
>     return false;
>   }
> 
> Which is generalizable to cases where you don't know the expected length by
> using what the first call returned. (It's not generalizable to the
EVP_PKEY_sign
> case where you only get an upper-bound initially, but I'll add a CBB_reserve +
> CBB_advance pair at some point when we need it.)

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:54: CBB tb_message, tb_messages;
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> tb_message vs tb_messages is kind of confusing. I'd probably just call the
> second one 'child'.

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:63:
reinterpret_cast<uint8_t*>(const_cast<char*>(token_binding.data())),
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> No need for the const_cast

Done.

https://codereview.chromium.org/1378613004/diff/300001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:90: !CBB_add_bytes(&token_binding,
&signed_ekm[0], signed_ekm.size()) ||
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> vector_as_array(&signed_ekm)

Done.

https://codereview.chromium.org/1378613004/diff/300001/third_party/tlslite/tl...
File third_party/tlslite/tlslite/session.py (right):

https://codereview.chromium.org/1378613004/diff/300001/third_party/tlslite/tl...
third_party/tlslite/tlslite/session.py:71: self.serverRandom = serverRandom
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> I don't think storing the client/server random on the Session is quite right.
If
> you resume a session, you will have different randoms on the connection, but
the
> same session.

Moved to the TLSConnection class. The clientRandom and serverRandom member
variables are the only member variables of that class (everything else is in the
parent class TLSRecordLayer).

https://codereview.chromium.org/1378613004/diff/300001/third_party/tlslite/tl...
third_party/tlslite/tlslite/session.py:135: """Returns the exported keying
material as defined in RFC 5705."""
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> Ditto. The client and server random used in the exporter are different on
> session resumption. This probably wants to be a method on the connection.

Done.

https://codereview.chromium.org/1378613004/diff/300001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1378613004/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:47965: +<histogram
name="TokenBinding.Support" enum="TokenBinding.Support">
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> Probably wants a Net prefix.

Done.

https://codereview.chromium.org/1378613004/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:74718: +<enum
name="TokenBinding.Support" type="int">
On 2015/11/18 20:49:01, davidben (behind on reviews) wrote:
> I don't think enum names typically have dots in them.

There are multiple enum names with dots in them - I modeled this after
DomainBoundCerts.Support.

If I'm changing the histogram name to have a 'Net.' prefix, should I change the
enum name as well?

[nharper, 2016-01-05 20:00:10]

rebase

[nharper, 2016-01-14 20:19:56]

rebase

[nharper, 2016-01-16 00:00:13]

Fix build errors

[nharper, 2016-01-16 00:17:38]

fix uint16 compile issue

[nharper, 2016-01-16 00:24:59]

Patchset #17 (id:400001) has been deleted

[nharper, 2016-01-16 00:46:31]

fix build issues

[nharper, 2016-01-16 01:09:38]

Patchset #17 (id:420001) has been deleted

[nharper, 2016-01-19 19:40:17]

fix build issues

[nharper, 2016-01-20 17:52:24]

fix build issues

[davidben, 2016-01-22 00:19:22]

So much plumbing. Looks good though. Just minor comments.

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction.cc:660: return true;
Nit: You could also just write

  return ssl_info.token_binding_negotiated &&
         ssl_info.token_binding_key_param == TB_PARAM_ECDSAP256 &&
         session_->params().channel_id_service;

(Alternatively, it's not actually possible for token_binding_negotiated to be
true and the others false, right? Could also just DCHECK the rest.)

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction.cc:995: rv = ERR_CHANNEL_ID_IMPORT_FAILED;
On 2015/12/04 01:42:19, nharper wrote:
> On 2015/11/18 20:49:00, davidben (slow) wrote:
> > Is this possible?
> 
> I can't see how this would be possible (GetOrCreateChannelID's documentation
> indicates that token_binding_key_ will be filled if it returns success), but
> SSLClientSocketOpenSSL has a similar check when looking up the channel ID key.

Pfft. Apparently this dates to
https://codereview.chromium.org/338093012/diff/60001/net/socket/ssl_client_so...
which is my fault. And then yours in
https://codereview.chromium.org/1076063002

Originally, the key would come out as a string rather than an ECPrivateKey.
Let's ditch it here and we also land
https://codereview.chromium.org/1615943003/.

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction.cc:1086: rv =
BuildTokenBindingMessageFromTokenBindings(token_bindings, &header);
You could probably fold this and BuildProvidedTokenBinding into one function.
Save a copy even. Or do you expect the other TB types to want them decomposed?

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15844:
base::MessageLoop::current()->RunUntilIdle();
[I have no clue what's going on with this block and am just assuming it's
correct. May wish to get a SPDY person to look at that file.]

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket.h:13: #include "crypto/ec_private_key.h"
Can be forward-declared.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket.h:149: // Returns a net error code of ERR_FAILED or
OK.
Nit: Also returns ERR_NOT_IMPLEMENTED, strictly speaking. Maybe have it return
an Error and then the type system catures it?

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:29: #include "base/stl_util.h"
Unused?

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:596: size_t tb_ekm_size = 32;
Nit: Could just use sizeof(tb_ekm_buf) everywhere I think.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:599: size_t ekm_label_length =
strlen(kTokenBindingExporterLabel);
Nit: Could just use strlen(kTokenBindingExporterLabel) at the call site I think.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:602: nullptr, 0, false)) {
Nit: false /* no context */

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.h:47: typedef base::MRUCache<std::string,
std::vector<uint8_t>> SignedEkmMap;
using SignedEkmMap = ....

https://codereview.chromium.org/1378613004/diff/480001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:51: #include "net/ssl/token_binding.h"
Not necessary?

https://codereview.chromium.org/1378613004/diff/480001/net/ssl/token_binding_...
File net/ssl/token_binding_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:42: !CBB_flush(out)) {
[Later there'll be an EC_POINT_point2cbb but apparently it's part of the giant
mass of unreviewed CLs I have in Adam's queue. :-) ]

https://codereview.chromium.org/1378613004/diff/480001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:19: #include "base/base64.h"
Unused?

https://codereview.chromium.org/1378613004/diff/480001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:3397: // TODO(svaldez): Update tests to
user EmbeddedTestServer.
Nit: user -> use?

[nharper, 2016-01-22 01:10:23]

Patchset #18 (id:460001) has been deleted

[nharper, 2016-01-22 01:10:37]

Patchset #17 (id:440001) has been deleted

[nharper, 2016-01-22 01:52:35]

rebase

[nharper, 2016-01-22 02:26:56]

address comments (and see what breaks on trybots)

[nharper, 2016-01-22 19:36:52]

+bnc

Bence, can you take a look at http_network_transaction_unittest.cc?

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction.cc:660: return true;
On 2016/01/22 00:19:20, davidben (slow) wrote:
> Nit: You could also just write
> 
>   return ssl_info.token_binding_negotiated &&
>          ssl_info.token_binding_key_param == TB_PARAM_ECDSAP256 &&
>          session_->params().channel_id_service;
> 
> (Alternatively, it's not actually possible for token_binding_negotiated to be
> true and the others false, right? Could also just DCHECK the rest.)

If there's a bug in the code elsewhere, it's possible for
session_->params().channel_id_service to be null and still have
token_binding_negotiated true. (That is to say, a channel_id_service is not
needed to negotiated token binding.)

token_binding_key_param could be DCHECK'd if token_binding_negotiated is true,
but that makes things complicated (and if the DCHECK fails, it would still need
to return the correct false in non-debug builds). I think the return statement
you wrote is the cleanest.

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction.cc:995: rv = ERR_CHANNEL_ID_IMPORT_FAILED;
On 2016/01/22 00:19:20, davidben (slow) wrote:
> On 2015/12/04 01:42:19, nharper wrote:
> > On 2015/11/18 20:49:00, davidben (slow) wrote:
> > > Is this possible?
> > 
> > I can't see how this would be possible (GetOrCreateChannelID's documentation
> > indicates that token_binding_key_ will be filled if it returns success), but
> > SSLClientSocketOpenSSL has a similar check when looking up the channel ID
key.
> 
> Pfft. Apparently this dates to
>
https://codereview.chromium.org/338093012/diff/60001/net/socket/ssl_client_so...
> which is my fault. And then yours in
> https://codereview.chromium.org/1076063002
> 
> Originally, the key would come out as a string rather than an ECPrivateKey.
> Let's ditch it here and we also land
> https://codereview.chromium.org/1615943003/.

This statement is now removed.

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction.cc:1086: rv =
BuildTokenBindingMessageFromTokenBindings(token_bindings, &header);
On 2016/01/22 00:19:20, davidben (slow) wrote:
> You could probably fold this and BuildProvidedTokenBinding into one function.
> Save a copy even. Or do you expect the other TB types to want them decomposed?

Once we support the federated use case (i.e. referred token bindings), we'll
need to call BuildTokenBindingMessageFromTokenBindings with a vector that has
both the provided and referred token bindings.

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15844:
base::MessageLoop::current()->RunUntilIdle();
On 2016/01/22 00:19:21, davidben (slow) wrote:
> [I have no clue what's going on with this block and am just assuming it's
> correct. May wish to get a SPDY person to look at that file.]

This should be mostly copypasta from another unittest.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket.h:13: #include "crypto/ec_private_key.h"
On 2016/01/22 00:19:21, davidben (slow) wrote:
> Can be forward-declared.

Done.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket.h:149: // Returns a net error code of ERR_FAILED or
OK.
On 2016/01/22 00:19:21, davidben (slow) wrote:
> Nit: Also returns ERR_NOT_IMPLEMENTED, strictly speaking. Maybe have it return
> an Error and then the type system catures it?

Done. I also changed the same method in HttpStream to return an Error instead of
an int (since I'm not returning a non-Error value with my int).

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:29: #include "base/stl_util.h"
On 2016/01/22 00:19:21, davidben (slow) wrote:
> Unused?

Removed.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:596: size_t tb_ekm_size = 32;
On 2016/01/22 00:19:21, davidben (slow) wrote:
> Nit: Could just use sizeof(tb_ekm_buf) everywhere I think.

Done.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:599: size_t ekm_label_length =
strlen(kTokenBindingExporterLabel);
On 2016/01/22 00:19:21, davidben (slow) wrote:
> Nit: Could just use strlen(kTokenBindingExporterLabel) at the call site I
think.

Done.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:602: nullptr, 0, false)) {
On 2016/01/22 00:19:21, davidben (slow) wrote:
> Nit: false /* no context */

I've made this change, but I'm not sure it's any clearer.

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/1378613004/diff/480001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.h:47: typedef base::MRUCache<std::string,
std::vector<uint8_t>> SignedEkmMap;
On 2016/01/22 00:19:21, davidben (slow) wrote:
> using SignedEkmMap = ....

Done.

https://codereview.chromium.org/1378613004/diff/480001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:51: #include "net/ssl/token_binding.h"
On 2016/01/22 00:19:21, davidben (slow) wrote:
> Not necessary?

Removed.

https://codereview.chromium.org/1378613004/diff/480001/net/ssl/token_binding_...
File net/ssl/token_binding_openssl.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/ssl/token_binding_...
net/ssl/token_binding_openssl.cc:42: !CBB_flush(out)) {
On 2016/01/22 00:19:22, davidben (slow) wrote:
> [Later there'll be an EC_POINT_point2cbb but apparently it's part of the giant
> mass of unreviewed CLs I have in Adam's queue. :-) ]

Added a TODO to use EC_POINT_point2cbb.

https://codereview.chromium.org/1378613004/diff/480001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/480001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:19: #include "base/base64.h"
On 2016/01/22 00:19:22, davidben (slow) wrote:
> Unused?

Removed.

https://codereview.chromium.org/1378613004/diff/480001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:3397: // TODO(svaldez): Update tests to
user EmbeddedTestServer.
On 2016/01/22 00:19:22, davidben (slow) wrote:
> Nit: user -> use?

Done.

[davidben, 2016-01-22 21:28:08]

lgtm

[nharper, 2016-01-22 22:18:51]

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

[nharper, 2016-01-22 22:18:53]

asargent, can you review chrome/browser/extensions?
rsleevi, can you review chrome/browser/io_thread.*?

[asargent_no_longer_on_chrome, 2016-01-23 00:08:10]

chrome/browser/extensions/ lgtm

[nharper, 2016-01-23 00:34:33]

Add #if !defined(OS_IOS) to a unittest

[nharper, 2016-01-23 00:36:41]

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

[nharper, 2016-01-23 00:36:42]

+mpearson to review tools/metrics/histograms/histograms.xml

davidben, can you take a look at the most recent change
(http_network_transaction_ssl_unittest.cc)?

[nharper, 2016-01-23 01:02:22]

Add another #if !defined(OS_IOS)

[Mark P, 2016-01-23 05:01:49]

histograms.xml lgtm with minor comments below

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction.cc:660: enum {
Please put the standard comment here that this enum is emitted to UMA and
element in it cannot be renumbered or removed (and thus at risk of being
reused).

https://codereview.chromium.org/1378613004/diff/560001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1378613004/diff/560001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:27522: +    Count of number of secure
HTTP requests broken down by support for Token
option nit: "Count of" -> "The"

[Bence, 2016-01-23 22:31:16]

bnc@chromium.org changed reviewers:
+ bnc@chromium.org

[Bence, 2016-01-23 22:31:18]

net/http/http_network_transaction_unittest.cc LGTM modulo nits.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:25: #include
"base/strings/string_number_conversions.h"
Sorry, I cannot seem to find what these two includes are for.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15832: scoped_ptr<SpdyFrame>
resp(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
Style guide recommends nullptr instead of NULL for new code, see
https://chromium-cpp.appspot.com.  Here and twice below.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15832: scoped_ptr<SpdyFrame>
resp(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
Optional: add an empty line above this one.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15834: MockRead reads[] =
{CreateMockRead(*resp, 1), CreateMockRead(*body, 2),
Don't you have to count mock data from 0?  I'd be surprised if this test passed,
I would expect a "index 0 not found" kind of error.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15840:
scoped_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
Optional: add an empty line above this one.

[nharper, 2016-01-25 18:53:43]

fix nits

[nharper, 2016-01-25 18:54:55]

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction.cc:660: enum {
On 2016/01/23 05:01:49, Mark P wrote:
> Please put the standard comment here that this enum is emitted to UMA and
> element in it cannot be renumbered or removed (and thus at risk of being
> reused).

Done.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:25: #include
"base/strings/string_number_conversions.h"
On 2016/01/23 22:31:17, Bence wrote:
> Sorry, I cannot seem to find what these two includes are for.

I don't remember what they were for and it compiles fine without them, so
they're gone now.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15832: scoped_ptr<SpdyFrame>
resp(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
On 2016/01/23 22:31:17, Bence wrote:
> Style guide recommends nullptr instead of NULL for new code, see
> https://chromium-cpp.appspot.com.  Here and twice below.

Done.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15832: scoped_ptr<SpdyFrame>
resp(spdy_util_.ConstructSpdyGetSynReply(NULL, 0, 1));
On 2016/01/23 22:31:17, Bence wrote:
> Optional: add an empty line above this one.

Done.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15834: MockRead reads[] =
{CreateMockRead(*resp, 1), CreateMockRead(*body, 2),
On 2016/01/23 22:31:17, Bence wrote:
> Don't you have to count mock data from 0?  I'd be surprised if this test
passed,
> I would expect a "index 0 not found" kind of error.

I don't know what the sequence number is supposed to be. From looking at a few
of the other tests, I see numbers (starting at 0) being used for both reads and
writes - generally 0 is the first write (e.g. a request). I tried various other
possibilities for the numbers (including using the same sequence number for all
of them, or having the numbers in reverse order), and the test still passed.

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15840:
scoped_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
On 2016/01/23 22:31:18, Bence wrote:
> Optional: add an empty line above this one.

Done.

https://codereview.chromium.org/1378613004/diff/560001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1378613004/diff/560001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:27522: +    Count of number of secure
HTTP requests broken down by support for Token
On 2016/01/23 05:01:49, Mark P wrote:
> option nit: "Count of" -> "The"

Done.

[Bence, 2016-01-25 21:45:19]

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15834: MockRead reads[] =
{CreateMockRead(*resp, 1), CreateMockRead(*body, 2),
On 2016/01/25 18:54:54, nharper wrote:
> On 2016/01/23 22:31:17, Bence wrote:
> > Don't you have to count mock data from 0?  I'd be surprised if this test
> passed,
> > I would expect a "index 0 not found" kind of error.
> 
> I don't know what the sequence number is supposed to be. From looking at a few
> of the other tests, I see numbers (starting at 0) being used for both reads
and
> writes - generally 0 is the first write (e.g. a request). I tried various
other
> possibilities for the numbers (including using the same sequence number for
all
> of them, or having the numbers in reverse order), and the test still passed.

Well, if the sequence numbers do not seem to matter, could you please omit them?
 I believe both CreateMockRead() and MockRead() has a variant without a sequence
number argument.  Calling them would better reflect the empirical fact that
sequence numbers are irrelevant.

[nharper, 2016-01-25 21:46:06]

nharper@chromium.org changed reviewers:
+ mmenke@chromium.org
- rsleevi@chromium.org

[nharper, 2016-01-25 21:46:07]

R+mmenke,-sleevi for chrome/browser/io_thread*

[mmenke, 2016-01-25 21:47:44]

On 2016/01/25 21:46:07, nharper wrote:
> R+mmenke,-sleevi for chrome/browser/io_thread*

io_thread LGTM

[nharper, 2016-01-25 22:06:06]

Remove sequence numbers from mock reads

[nharper, 2016-01-25 22:06:17]

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:15834: MockRead reads[] =
{CreateMockRead(*resp, 1), CreateMockRead(*body, 2),
On 2016/01/25 21:45:19, Bence wrote:
> On 2016/01/25 18:54:54, nharper wrote:
> > On 2016/01/23 22:31:17, Bence wrote:
> > > Don't you have to count mock data from 0?  I'd be surprised if this test
> > passed,
> > > I would expect a "index 0 not found" kind of error.
> > 
> > I don't know what the sequence number is supposed to be. From looking at a
few
> > of the other tests, I see numbers (starting at 0) being used for both reads
> and
> > writes - generally 0 is the first write (e.g. a request). I tried various
> other
> > possibilities for the numbers (including using the same sequence number for
> all
> > of them, or having the numbers in reverse order), and the test still passed.
> 
> Well, if the sequence numbers do not seem to matter, could you please omit
them?
>  I believe both CreateMockRead() and MockRead() has a variant without a
sequence
> number argument.  Calling them would better reflect the empirical fact that
> sequence numbers are irrelevant.

Done.

[nharper, 2016-01-25 22:06:27]

The CQ bit was checked by nharper@chromium.org

[nharper, 2016-01-25 22:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, davidben@chromium.org, bnc@chromium.org,
mpearson@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1378613004/#ps600001
(title: "Remove sequence numbers from mock reads")

[commit-bot: I haz the power, 2016-01-25 22:07:37]

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

[commit-bot: I haz the power, 2016-01-25 23:54:29]

Description was changed from

==========
Set Token-Binding HTTP header

Adds a new method to SSLClientSocket to get the Token Binding from an SSL
connection where Token Binding was negotiated, and uses that to add the
Set-Token-Binding HTTP header (only when Token Binding was negotiated).

BUG=467312
==========

to

==========
Set Token-Binding HTTP header

Adds a new method to SSLClientSocket to get the Token Binding from an SSL
connection where Token Binding was negotiated, and uses that to add the
Set-Token-Binding HTTP header (only when Token Binding was negotiated).

BUG=467312
==========

[commit-bot: I haz the power, 2016-01-25 23:54:30]

Committed patchset #23 (id:600001)

[commit-bot: I haz the power, 2016-01-25 23:55:28]

Description was changed from

==========
Set Token-Binding HTTP header

Adds a new method to SSLClientSocket to get the Token Binding from an SSL
connection where Token Binding was negotiated, and uses that to add the
Set-Token-Binding HTTP header (only when Token Binding was negotiated).

BUG=467312
==========

to

==========
Set Token-Binding HTTP header

Adds a new method to SSLClientSocket to get the Token Binding from an SSL
connection where Token Binding was negotiated, and uses that to add the
Set-Token-Binding HTTP header (only when Token Binding was negotiated).

BUG=467312

Committed: https://crrev.com/b7441ef2effe86324798710a82d8a006f5eb1395
Cr-Commit-Position: refs/heads/master@{#371347}
==========

[commit-bot: I haz the power, 2016-01-25 23:55:30]

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

[Bence, 2016-01-26 15:59:17]

On 2016/01/25 22:06:17, nharper wrote:
>
https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
> File net/http/http_network_transaction_unittest.cc (right):
> 
>
https://codereview.chromium.org/1378613004/diff/560001/net/http/http_network_...
> net/http/http_network_transaction_unittest.cc:15834: MockRead reads[] =
> {CreateMockRead(*resp, 1), CreateMockRead(*body, 2),
> On 2016/01/25 21:45:19, Bence wrote:
> > On 2016/01/25 18:54:54, nharper wrote:
> > > On 2016/01/23 22:31:17, Bence wrote:
> > > > Don't you have to count mock data from 0?  I'd be surprised if this test
> > > passed,
> > > > I would expect a "index 0 not found" kind of error.
> > > 
> > > I don't know what the sequence number is supposed to be. From looking at a
> few
> > > of the other tests, I see numbers (starting at 0) being used for both
reads
> > and
> > > writes - generally 0 is the first write (e.g. a request). I tried various
> > other
> > > possibilities for the numbers (including using the same sequence number
for
> > all
> > > of them, or having the numbers in reverse order), and the test still
passed.
> > 
> > Well, if the sequence numbers do not seem to matter, could you please omit
> them?
> >  I believe both CreateMockRead() and MockRead() has a variant without a
> sequence
> > number argument.  Calling them would better reflect the empirical fact that
> > sequence numbers are irrelevant.
> 
> Done.

Thanks.

[Ryan Sleevi, 2016-01-26 22:53:46]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-01-26 22:53:47]

chrome/browser/io_thread LGTM % nit (addressible in a follow-up CL/separately)

https://codereview.chromium.org/1378613004/diff/600001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1378613004/diff/600001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:850: if
(command_line.HasSwitch(switches::kEnableTokenBinding)) {
As command-line switches are generally inaccessible on Android/ChromeOS, are
there plans to add it to about://flags? Or via Policy? Or via Finch? Just
exploring here.

Note that this file appears to omit braces for one-line ifs, so I think local
style should dominate.

[nharper, 2016-01-26 23:29:30]

https://codereview.chromium.org/1378613004/diff/600001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1378613004/diff/600001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:850: if
(command_line.HasSwitch(switches::kEnableTokenBinding)) {
On 2016/01/26 22:53:47, Ryan Sleevi wrote:
> As command-line switches are generally inaccessible on Android/ChromeOS, are
> there plans to add it to about://flags? Or via Policy? Or via Finch? Just
> exploring here.
> 
> Note that this file appears to omit braces for one-line ifs, so I think local
> style should dominate.

There is a plan to add it to about:flags - see
https://codereview.chromium.org/1409323011/. That CL also fixes this style nit.