From 0d28ea5f8316e357734b8d7c8ac2d9a2d02c30b0 Mon Sep 17 00:00:00 2001 From: bnc Date: Mon, 13 Oct 2014 08:15:38 -0700 Subject: [PATCH] Add histogram to track NPN/ALPN. Add histogram to track * which TLS extension (NPN or ALPN) is used for protocol negotiation, * in case of NPN, is there overlap or do we fall back, * which protocol is selected. This CL is draft, I still do not know how to call set_protocol_negotiation() in NSS. I would love to do that in SSLClientSocketNSS::Core::HandshakeSucceeded(), but that might not be the on the same thread. Review URL: https://codereview.chromium.org/590513002 Cr-Commit-Position: refs/heads/master@{#299303} --- net/socket/next_proto.h | 6 ++++-- net/socket/ssl_client_socket.cc | 35 ++++++++++++++++++++++++++++++++- net/socket/ssl_client_socket.h | 15 ++++++++++++++ net/socket/ssl_client_socket_nss.cc | 25 +++++++++++++++++++++++ net/socket/ssl_client_socket_openssl.cc | 2 ++ net/socket/ssl_client_socket_pool.cc | 4 +++- tools/metrics/histograms/histograms.xml | 30 ++++++++++++++++++++++++++++ 7 files changed, 113 insertions(+), 4 deletions(-) diff --git a/net/socket/next_proto.h b/net/socket/next_proto.h index 08e680153279..e715aca3a170 100644 --- a/net/socket/next_proto.h +++ b/net/socket/next_proto.h @@ -14,8 +14,10 @@ namespace net { // Next Protocol Negotiation (NPN), if successful, results in agreement on an // application-level string that specifies the application level protocol to // use over the TLS connection. NextProto enumerates the application level -// protocols that we recognise. Do not change or reuse values, because they -// are used to collect statistics on UMA. +// protocols that we recognize. Do not change or reuse values, because they +// are used to collect statistics on UMA. Also, values must be in [0,499), +// because of the way TLS protocol negotiation extension information is added to +// UMA histogram. enum NextProto { kProtoUnknown = 0, kProtoHTTP11 = 1, diff --git a/net/socket/ssl_client_socket.cc b/net/socket/ssl_client_socket.cc index 4aacbc8d42db..da2d6ba0c8d8 100644 --- a/net/socket/ssl_client_socket.cc +++ b/net/socket/ssl_client_socket.cc @@ -5,6 +5,7 @@ #include "net/socket/ssl_client_socket.h" #include "base/metrics/histogram.h" +#include "base/metrics/sparse_histogram.h" #include "base/strings/string_util.h" #include "crypto/ec_private_key.h" #include "net/base/host_port_pair.h" @@ -19,7 +20,8 @@ SSLClientSocket::SSLClientSocket() protocol_negotiated_(kProtoUnknown), channel_id_sent_(false), signed_cert_timestamps_received_(false), - stapled_ocsp_response_received_(false) { + stapled_ocsp_response_received_(false), + negotiation_extension_(kExtensionUnknown) { } // static @@ -124,6 +126,11 @@ void SSLClientSocket::set_protocol_negotiated(NextProto protocol_negotiated) { protocol_negotiated_ = protocol_negotiated; } +void SSLClientSocket::set_negotiation_extension( + SSLNegotiationExtension negotiation_extension) { + negotiation_extension_ = negotiation_extension; +} + bool SSLClientSocket::WasChannelIDSent() const { return channel_id_sent_; } @@ -232,4 +239,30 @@ std::vector SSLClientSocket::SerializeNextProtos( return wire_protos; } +void SSLClientSocket::RecordNegotiationExtension() { + if (negotiation_extension_ == kExtensionUnknown) + return; + std::string proto; + SSLClientSocket::NextProtoStatus status = GetNextProto(&proto); + if (status == kNextProtoUnsupported) + return; + // Convert protocol into numerical value for histogram. + NextProto protocol_negotiated = SSLClientSocket::NextProtoFromString(proto); + base::HistogramBase::Sample sample = + static_cast(protocol_negotiated); + // In addition to the protocol negotiated, we want to record which TLS + // extension was used, and in case of NPN, whether there was overlap between + // server and client list of supported protocols. + if (negotiation_extension_ == kExtensionNPN) { + if (status == kNextProtoNoOverlap) { + sample += 1000; + } else { + sample += 500; + } + } else { + DCHECK_EQ(kExtensionALPN, negotiation_extension_); + } + UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSLProtocolNegotiation", sample); +} + } // namespace net diff --git a/net/socket/ssl_client_socket.h b/net/socket/ssl_client_socket.h index 98eb7741d7e5..8f40f8492e99 100644 --- a/net/socket/ssl_client_socket.h +++ b/net/socket/ssl_client_socket.h @@ -79,6 +79,13 @@ class NET_EXPORT SSLClientSocket : public SSLSocket { // the first protocol in our list. }; + // TLS extension used to negotiate protocol. + enum SSLNegotiationExtension { + kExtensionUnknown, + kExtensionALPN, + kExtensionNPN, + }; + // StreamSocket: virtual bool WasNpnNegotiated() const override; virtual NextProto GetNegotiatedProtocol() const override; @@ -150,6 +157,8 @@ class NET_EXPORT SSLClientSocket : public SSLSocket { virtual void set_protocol_negotiated(NextProto protocol_negotiated); + void set_negotiation_extension(SSLNegotiationExtension negotiation_extension); + // Returns the ChannelIDService used by this socket, or NULL if // channel ids are not supported. virtual ChannelIDService* GetChannelIDService() const = 0; @@ -162,6 +171,10 @@ class NET_EXPORT SSLClientSocket : public SSLSocket { // Public for ssl_client_socket_openssl_unittest.cc. virtual bool WasChannelIDSent() const; + // Record which TLS extension was used to negotiate protocol and protocol + // chosen in a UMA histogram. + void RecordNegotiationExtension(); + protected: virtual void set_channel_id_sent(bool channel_id_sent); @@ -219,6 +232,8 @@ class NET_EXPORT SSLClientSocket : public SSLSocket { bool signed_cert_timestamps_received_; // True if a stapled OCSP response was received. bool stapled_ocsp_response_received_; + // Protocol negotiation extension used. + SSLNegotiationExtension negotiation_extension_; }; } // namespace net diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc index 26725353384d..ef2e29d5d466 100644 --- a/net/socket/ssl_client_socket_nss.cc +++ b/net/socket/ssl_client_socket_nss.cc @@ -408,6 +408,7 @@ struct HandshakeState { void Reset() { next_proto_status = SSLClientSocket::kNextProtoUnsupported; next_proto.clear(); + negotiation_extension_ = SSLClientSocket::kExtensionUnknown; channel_id_sent = false; server_cert_chain.Reset(NULL); server_cert = NULL; @@ -422,6 +423,9 @@ struct HandshakeState { SSLClientSocket::NextProtoStatus next_proto_status; std::string next_proto; + // TLS extension used for protocol negotiation. + SSLClientSocket::SSLNegotiationExtension negotiation_extension_; + // True if a channel ID was sent. bool channel_id_sent; @@ -760,6 +764,8 @@ class SSLClientSocketNSS::Core : public base::RefCountedThreadSafe { // UpdateNextProto gets any application-layer protocol that may have been // negotiated by the TLS connection. void UpdateNextProto(); + // Record TLS extension used for protocol negotiation (NPN or ALPN). + void UpdateExtensionUsed(); //////////////////////////////////////////////////////////////////////////// // Methods that are ONLY called on the network task runner: @@ -1641,6 +1647,7 @@ void SSLClientSocketNSS::Core::HandshakeSucceeded() { UpdateStapledOCSPResponse(); UpdateConnectionStatus(); UpdateNextProto(); + UpdateExtensionUsed(); // Update the network task runners view of the handshake state whenever // a handshake has completed. @@ -2496,6 +2503,23 @@ void SSLClientSocketNSS::Core::UpdateNextProto() { } } +void SSLClientSocketNSS::Core::UpdateExtensionUsed() { + PRBool negotiated_extension; + SECStatus rv = SSL_HandshakeNegotiatedExtension(nss_fd_, + ssl_app_layer_protocol_xtn, + &negotiated_extension); + if (rv == SECSuccess && negotiated_extension) { + nss_handshake_state_.negotiation_extension_ = kExtensionALPN; + } else { + rv = SSL_HandshakeNegotiatedExtension(nss_fd_, + ssl_next_proto_nego_xtn, + &negotiated_extension); + if (rv == SECSuccess && negotiated_extension) { + nss_handshake_state_.negotiation_extension_ = kExtensionNPN; + } + } +} + void SSLClientSocketNSS::Core::RecordChannelIDSupportOnNSSTaskRunner() { DCHECK(OnNSSTaskRunner()); if (nss_handshake_state_.resumed_handshake) @@ -3328,6 +3352,7 @@ int SSLClientSocketNSS::DoHandshakeComplete(int result) { !core_->state().sct_list_from_tls_extension.empty()); set_stapled_ocsp_response_received( !core_->state().stapled_ocsp_response.empty()); + set_negotiation_extension(core_->state().negotiation_extension_); LeaveFunction(result); return result; diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc index f341a9fc3775..aa45a9d3a830 100644 --- a/net/socket/ssl_client_socket_openssl.cc +++ b/net/socket/ssl_client_socket_openssl.cc @@ -915,6 +915,7 @@ int SSLClientSocketOpenSSL::DoHandshake() { if (alpn_len > 0) { npn_proto_.assign(reinterpret_cast(alpn_proto), alpn_len); npn_status_ = kNextProtoNegotiated; + set_negotiation_extension(kExtensionALPN); } } @@ -1669,6 +1670,7 @@ int SSLClientSocketOpenSSL::SelectNextProtoCallback(unsigned char** out, npn_proto_.assign(reinterpret_cast(*out), *outlen); DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_; + set_negotiation_extension(kExtensionNPN); return SSL_TLSEXT_ERR_OK; } diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc index 6356783144b2..a866b6c9513e 100644 --- a/net/socket/ssl_client_socket_pool.cc +++ b/net/socket/ssl_client_socket_pool.cc @@ -452,8 +452,10 @@ int SSLConnectJob::DoSSLConnectComplete(int result) { // GetNextProto will fail and and trigger a NOTREACHED if we pass in a socket // that hasn't had SSL_ImportFD called on it. If we get a certificate error // here, then we know that we called SSL_ImportFD. - if (result == OK || IsCertificateError(result)) + if (result == OK || IsCertificateError(result)) { status = ssl_socket_->GetNextProto(&proto); + ssl_socket_->RecordNegotiationExtension(); + } // If we want spdy over npn, make sure it succeeded. if (status == SSLClientSocket::kNextProtoNegotiated) { diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 2161333e9943..6446d031a849 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -18220,6 +18220,15 @@ Therefore, the affected-histogram name has to have at least one dot in it. Time to complete a speculative certificate verification. + + bnc@chromium.org + + TLS extension used to negotiate protocol (ALPN or NPN); in case of NPN, + whether the protocol is indeed supported by both the client and the server + or is a fallback because of no overlap; and the negotiated protocol itself. + + + @@ -52378,6 +52387,27 @@ To add a new entry, add it with any value and run test to compute valid value. + + + + + + + + + + + + + + + + + + + + + -- 2.11.4.GIT