Add non-application-data SSL messages to NetLog.

Add non-application-data SSL messages to NetLog.

Right now we usually ask the reporter to give us a full byte-level log,
but this unnecessarily leaks a lot of information. In TLS 1.2, these
messages are all (save the Finished, which is not interesting) sent in
the clear anyway. This also doesn't work for renegotiation which we've
usually been blind for.

In TLS 1.3, these messages are now encrypted, which means even our prior
strategy wouldn't work. It's also somewhat silly to ask users for full
byte logging when we're only interested in protocol-level information.

Per the bug, Certificate messages we send (client certs) are filtered
out unless full byte logging is enabled.

BUG=676353
Review-Url: https://codereview.chromium.org/2704623002
Cr-Commit-Position: refs/heads/master@{#465603}
Committed: https://chromium.googlesource.com/chromium/src/+/cef9e211832bf2d1f01f750296238fed28e416f0

[davidben, 2017-02-16 23:40:29]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 23:41:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 06:38:54]

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

[commit-bot: I haz the power, 2017-02-17 06:38:55]

Dry run: This issue passed the CQ dry run.

[davidben, 2017-02-17 21:03:55]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 21:04:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2017-02-17 21:04:41]

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

[davidben, 2017-02-17 21:04:42]

eroman: What do you think? Too noisy?

[eroman, 2017-02-17 21:30:18]

Can you include a sample log file generated with this data?

I haven't reviewed the change yet, but I generally ask to see a log as part of
the review for new stuff.

[davidben, 2017-02-17 21:40:09]

On 2017/02/17 21:30:18, eroman wrote:
> Can you include a sample log file generated with this data?
> 
> I haven't reviewed the change yet, but I generally ask to see a log as part of
> the review for new stuff.

Sample file: https://drive.google.com/open?id=0B2ImyA6KAoPURmdYQ1dJbGdIdEU

The TLS 1.3 sockets, in particular, are the main motivation. Even full byte
logging won't capture that part of the handshake. Though being able to see
ClientHello and ServerHello is handy.

(It occurs to me that, if we're worried about the size, we could filter out both
send and received Certificate messages unless full byte logging is enabled.)

[davidben, 2017-02-17 21:40:36]

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

[davidben, 2017-02-17 21:40:37]

Also +svaldez to get his views.

[svaldez, 2017-02-17 22:06:15]

On 2017/02/17 21:40:37, davidben wrote:
> Also +svaldez to get his views.

Any chance type could be moved to be the first thing in the log, possibly also
having it convert from number to label might be useful, though possibly too
complicated.

[davidben, 2017-02-17 22:16:32]

On 2017/02/17 22:06:15, svaldez wrote:
> On 2017/02/17 21:40:37, davidben wrote:
> > Also +svaldez to get his views.
> 
> Any chance type could be moved to be the first thing in the log, possibly also
> having it convert from number to label might be useful, though possibly too
> complicated.

It's a dictionary. I don't think we control the order. :-P I suppose we could
make the painter more complicated. Likewise, I don't believe we currently have
code which maintains the names for all the message type. Could be added, but I
dunno if it's worth it when we don't decode the contents. The type's was just an
afterthought since the message is sometimes redacted. Otherwise I wouldn't have
bothered.

Idle thought: maybe we could redact things by just truncating down to the 4-byte
message header?

[eroman, 2017-02-17 22:16:51]

Can you tell me more about what data will be in these blobs?

In particular I would like to ensure that any potentially sensitive information
(which we wouldn't want in a public bug report) is elided when
capture_mode.include_cookies_and_credentials()

I am less concerned with the size, however if you have averages on that would be
useful to hear.

Otherwise the coding implementation looks good.

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1960: base::Bind(&NetLogSSLAlertCallback,
base::Unretained(buf), len));
Is Unretained() needed here, or is it just for documentation purposes? Same
question below.

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.h (right):

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.h:221: void MessageCallback(int is_write,
Documentation?

[commit-bot: I haz the power, 2017-02-17 23:07:13]

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

[commit-bot: I haz the power, 2017-02-17 23:07:14]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[davidben, 2017-04-18 20:31:51]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 20:33:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2017-04-18 20:43:57]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 20:44:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2017-04-18 20:51:17]

On 2017/02/17 22:16:51, eroman wrote:
> Can you tell me more about what data will be in these blobs?
> 
> In particular I would like to ensure that any potentially sensitive
information
> (which we wouldn't want in a public bug report) is elided when
> capture_mode.include_cookies_and_credentials()

Sorry about the delay.

The things that are logged are TLS handshake messages. ClientHello, ServerHello,
etc. In TLS 1.2, every message but the Finished message is sent over the clear
anyway. In TLS 1.3, some of the logged messages are encrypted, but they aren't
especially sensitive. Today, the messages are:

ClientHello: This just contains connection parameters. The thing that varies at
all is hostname which is already in NetLog, and the session ID we're offering.

ServerHello: This just contains connection parameters.

HelloRetryRequest (TLS 1.3): This just contains connection parameters.

EncryptedExtensions (TLS 1.3): This just contains connection parameters.

server Certificate: This is the server certificate. On a successful connection,
we'll already log it. On a failed connection, this would let us see what was
wrong with the certificate. (mattm hit some issues here recently.) Notably, this
is useful if we cannot parse the certificate at all.

ServerKeyExchange: This just contains a fresh ECDH public key.

server CertificateVerify (TLS 1.3): This just contains a signature.

client Certificate: Now this message is actually potentially interesting. It
doesn't contain anything to let us impersonate the user, but it may contain
things like the user's name, email, etc. Whatever their deployment puts in
client certificates. Per the bug, we redact it unless include_socket_bytes[*] is
on.

client CertificateVerify: This just contains a signature.

ChannelID: This contains the Channel ID public key. See https://crbug.com/674377
for discussion. The public key itself does not convey anything, but it is
persistent.

alerts: These are just error codes.


[*] I did include_socket_bytes() rather than include_cookies_and_credentials()
because the latter is weird between net-internals and net-export (namely
net-internals reimplements it in JavaScript as a post-processing), so I'm a
little wary of using it. Or do we get to ignore it now? The UI is deprecated but
still there.

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1960: base::Bind(&NetLogSSLAlertCallback,
base::Unretained(buf), len));
On 2017/02/17 22:16:51, eroman wrote:
> Is Unretained() needed here, or is it just for documentation purposes? Same
> question below.

Oh, I didn't realize they weren't needed. Removed.

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.h (right):

https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.h:221: void MessageCallback(int is_write,
On 2017/02/17 22:16:51, eroman wrote:
> Documentation?

Done.

[davidben, 2017-04-18 20:53:43]

On 2017/04/18 20:51:17, davidben wrote:
> On 2017/02/17 22:16:51, eroman wrote:
> > Can you tell me more about what data will be in these blobs?
> > 
> > In particular I would like to ensure that any potentially sensitive
> information
> > (which we wouldn't want in a public bug report) is elided when
> > capture_mode.include_cookies_and_credentials()
> 
> Sorry about the delay.
> 
> The things that are logged are TLS handshake messages. ClientHello,
ServerHello,
> etc. In TLS 1.2, every message but the Finished message is sent over the clear
> anyway. In TLS 1.3, some of the logged messages are encrypted, but they aren't
> especially sensitive. Today, the messages are:
> 
> ClientHello: This just contains connection parameters. The thing that varies
at
> all is hostname which is already in NetLog, and the session ID we're offering.
> 
> ServerHello: This just contains connection parameters.
> 
> HelloRetryRequest (TLS 1.3): This just contains connection parameters.
> 
> EncryptedExtensions (TLS 1.3): This just contains connection parameters.
> 
> server Certificate: This is the server certificate. On a successful
connection,
> we'll already log it. On a failed connection, this would let us see what was
> wrong with the certificate. (mattm hit some issues here recently.) Notably,
this
> is useful if we cannot parse the certificate at all.
> 
> ServerKeyExchange: This just contains a fresh ECDH public key.
> 
> server CertificateVerify (TLS 1.3): This just contains a signature.
> 
> client Certificate: Now this message is actually potentially interesting. It
> doesn't contain anything to let us impersonate the user, but it may contain
> things like the user's name, email, etc. Whatever their deployment puts in
> client certificates. Per the bug, we redact it unless include_socket_bytes[*]
is
> on.
> 
> client CertificateVerify: This just contains a signature.
> 
> ChannelID: This contains the Channel ID public key. See
https://crbug.com/674377
> for discussion. The public key itself does not convey anything, but it is
> persistent.

Sorry, I missed a few:

ServerHelloDone: This is the empty string.

Finished: This just contains an HMAC.

NewSessionTicket: This contains the (public) ticket to pass to the server when
resuming the connection. It's an opaque blob that the server knows how to
process.

KeyUpdate (TLS 1.3): This contains a boolean for whether it's an request or
response KeyUpdate.

CertificateStatus (TLS 1.2): This contains the server's stapled OCSP response.

> 
> alerts: These are just error codes.
> 
> 
> [*] I did include_socket_bytes() rather than include_cookies_and_credentials()
> because the latter is weird between net-internals and net-export (namely
> net-internals reimplements it in JavaScript as a post-processing), so I'm a
> little wary of using it. Or do we get to ignore it now? The UI is deprecated
but
> still there.
> 
>
https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
> File net/socket/ssl_client_socket_impl.cc (right):
> 
>
https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
> net/socket/ssl_client_socket_impl.cc:1960: base::Bind(&NetLogSSLAlertCallback,
> base::Unretained(buf), len));
> On 2017/02/17 22:16:51, eroman wrote:
> > Is Unretained() needed here, or is it just for documentation purposes? Same
> > question below.
> 
> Oh, I didn't realize they weren't needed. Removed.
> 
>
https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
> File net/socket/ssl_client_socket_impl.h (right):
> 
>
https://codereview.chromium.org/2704623002/diff/20001/net/socket/ssl_client_s...
> net/socket/ssl_client_socket_impl.h:221: void MessageCallback(int is_write,
> On 2017/02/17 22:16:51, eroman wrote:
> > Documentation?
> 
> Done.

[eroman, 2017-04-18 21:07:23]

lgtm

> Or do we get to ignore it now? The UI is deprecated but
still there.

The JS version of net-log export will probably be removed in M60. Once that
happens you can safely assume that the C++ filtering is the only implementation
to worry about.

[commit-bot: I haz the power, 2017-04-18 21:59:23]

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

[commit-bot: I haz the power, 2017-04-18 21:59:24]

Dry run: This issue passed the CQ dry run.

[davidben, 2017-04-19 14:55:38]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2017-04-19 14:56:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 15:00:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492613738385440,
"parent_rev": "80adefe09301c91a0f93a768df1196a3a692fc0e", "commit_rev":
"cef9e211832bf2d1f01f750296238fed28e416f0"}

[commit-bot: I haz the power, 2017-04-19 15:00:34]

Description was changed from

==========
Add non-application-data SSL messages to NetLog.

Right now we usually ask the reporter to give us a full byte-level log,
but this unnecessarily leaks a lot of information. In TLS 1.2, these
messages are all (save the Finished, which is not interesting) sent in
the clear anyway. This also doesn't work for renegotiation which we've
usually been blind for.

In TLS 1.3, these messages are now encrypted, which means even our prior
strategy wouldn't work. It's also somewhat silly to ask users for full
byte logging when we're only interested in protocol-level information.

Per the bug, Certificate messages we send (client certs) are filtered
out unless full byte logging is enabled.

BUG=676353
==========

to

==========
Add non-application-data SSL messages to NetLog.

Right now we usually ask the reporter to give us a full byte-level log,
but this unnecessarily leaks a lot of information. In TLS 1.2, these
messages are all (save the Finished, which is not interesting) sent in
the clear anyway. This also doesn't work for renegotiation which we've
usually been blind for.

In TLS 1.3, these messages are now encrypted, which means even our prior
strategy wouldn't work. It's also somewhat silly to ask users for full
byte logging when we're only interested in protocol-level information.

Per the bug, Certificate messages we send (client certs) are filtered
out unless full byte logging is enabled.

BUG=676353

Review-Url: https://codereview.chromium.org/2704623002
Cr-Commit-Position: refs/heads/master@{#465603}
Committed:
https://chromium.googlesource.com/chromium/src/+/cef9e211832bf2d1f01f75029623...
==========

[commit-bot: I haz the power, 2017-04-19 15:00:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/cef9e211832bf2d1f01f75029623...