Chromium Code Reviews
Help | Chromium Project | Sign in
(4)

Issue 2704623002: Add non-application-data SSL messages to NetLog.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 3 days ago by davidben
Modified:
1 week, 2 days ago
Reviewers:
svaldez, eroman
CC:
chromium-reviews, net-reviews_chromium.org, cbentzel+watch_chromium.org, bnc+watch_chromium.org, mmenke (Out Feb 4 to March 5), eroman
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : . #

Total comments: 2

Messages

Total messages: 17 (10 generated)
davidben
eroman: What do you think? Too noisy?
1 week, 2 days ago (2017-02-17 21:04:42 UTC) #8
eroman
Can you include a sample log file generated with this data? I haven't reviewed the ...
1 week, 2 days ago (2017-02-17 21:30:18 UTC) #9
davidben
On 2017/02/17 21:30:18, eroman wrote: > Can you include a sample log file generated with ...
1 week, 2 days ago (2017-02-17 21:40:09 UTC) #10
davidben
Also +svaldez to get his views.
1 week, 2 days ago (2017-02-17 21:40:37 UTC) #12
svaldez
On 2017/02/17 21:40:37, davidben wrote: > Also +svaldez to get his views. Any chance type ...
1 week, 2 days ago (2017-02-17 22:06:15 UTC) #13
davidben
On 2017/02/17 22:06:15, svaldez wrote: > On 2017/02/17 21:40:37, davidben wrote: > > Also +svaldez ...
1 week, 2 days ago (2017-02-17 22:16:32 UTC) #14
eroman
1 week, 2 days ago (2017-02-17 22:16:51 UTC) #15
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?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd