Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
171773006:
Add a histogram to measure the frequency of OCSP stapling support by servers (Closed)
|
Created:
6 years, 10 months ago by Ryan Sleevi Modified:
6 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a histogram to measure the frequency of OCSP stapling support by servers
BUG=none
R=wtc
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252765
Patch Set 1 #
Total comments: 7
Patch Set 2 : Review feedback #
Total comments: 1
Messages
Total messages: 15 (0 generated)
wtc: implementation jar: histogram
Patch set 1 LGTM. https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_nss.cc:2402: ocsp_requested && ocsp_responses_present); It should be unnecessary to test ocsp_requested. NSS doesn't allow a server to send us an unsolicited extension, so if OCSP responses are present, we must have requested OCSP stapling. https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_nss.cc:3205: ssl_config_.signed_cert_timestamps_enabled)); Perhaps we should save this boolean in a data member, for later use in Core::UpdateStapledOCSPResponse. Unfortunately there are two classes involved, so it may not be that straightforward.
https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_nss.cc:2402: ocsp_requested && ocsp_responses_present); 1. I guess what you have in mind is this: if (ocsp_requested) UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled", ocsp_responses_present); That is, the histogram is only updated when we requested OCSP stapling. 2. We can call SSL_OptionGet(nss_fd_, SSL_ENABLE_OCSP_STAPLING) to get the value of ocsp_requested. We will need to declare ocsp_requested as a PRBool. https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_nss.cc:3205: ssl_config_.signed_cert_timestamps_enabled)); On 2014/02/19 21:09:19, wtc wrote: > > Perhaps we should save this boolean in a data member, for later use in > Core::UpdateStapledOCSPResponse. Unfortunately there are two classes involved, > so it may not be that straightforward. Please ignore this comment. I found a better solution.
Patch set 1, histograms.xml LGTM (WanTeh will comment on code). I also had some very optional suggestions below, which you should feel very free to ignore. https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_nss.cc:2401: UMA_HISTOGRAM_BOOLEAN("Net.OCSPResponseStapled", This looks fine... but considering how little space you're using for a boolean... you *could* break out the 4 cases into an UMA_HISTOGRAM_ENUMERATION, rather than aggregating them via the && on the next line. YMMV... your choice. https://codereview.chromium.org/171773006/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/171773006/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:9270: +<histogram name="Net.OCSPResponseStapled" enum="BooleanSuccess"> This is ok.... but sometimes the histogram is more readable if you create your own custom enum, with choices like: No stapling found Contains stapled OCSP In this case, your boolean might be clear enough as as. Again, your choice.
Updated for review feedback from both wtc and jar https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/171773006/diff/1/net/socket/ssl_client_socket... net/socket/ssl_client_socket_nss.cc:2402: ocsp_requested && ocsp_responses_present); On 2014/02/19 21:09:19, wtc wrote: > > It should be unnecessary to test ocsp_requested. NSS doesn't allow a server to > send us an unsolicited extension, so if OCSP responses are present, we must have > requested OCSP stapling. Thanks for pointing this out. I've instead chosen to only record this histogram when we've actually *requested* OCSP stapling. With CT, this is effectively becoming "always", but at least it keeps the histogram clean.
Patch set 2 LGTM. https://codereview.chromium.org/171773006/diff/80001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/171773006/diff/80001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2397: SSL_OptionGet(nss_fd_, SSL_ENABLE_OCSP_STAPLING, &ocsp_requested); Nit: some tool may warn that we ignore the return value. We can say: PRBool ocsp_requested; if (SSL_OptionGet(nss_fd_, SSL_ENABLE_OCSP_STAPLING, &ocsp_requested) != SECSuccess) ocsp_requested = PR_FALSE;
On 2014/02/19 23:37:02, wtc wrote: > Patch set 2 LGTM. > > https://codereview.chromium.org/171773006/diff/80001/net/socket/ssl_client_so... > File net/socket/ssl_client_socket_nss.cc (right): > > https://codereview.chromium.org/171773006/diff/80001/net/socket/ssl_client_so... > net/socket/ssl_client_socket_nss.cc:2397: SSL_OptionGet(nss_fd_, > SSL_ENABLE_OCSP_STAPLING, &ocsp_requested); > > Nit: some tool may warn that we ignore the return value. We can say: > > PRBool ocsp_requested; > if (SSL_OptionGet(nss_fd_, SSL_ENABLE_OCSP_STAPLING, &ocsp_requested) != > SECSuccess) > ocsp_requested = PR_FALSE; Under the Chromium warning levels, we're fine.
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/171773006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/171773006/80001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsleevi@chromium.org/171773006/80001
Message was sent while issue was closed.
Change committed as 252765 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
