From d82eee0675cf971cdc5f02340b5799bb303449e3 Mon Sep 17 00:00:00 2001 From: zhihuang Date: Fri, 26 Aug 2016 11:25:05 -0700 Subject: [PATCH] Log how often DTLS negotiation failed because of incompatible ciphersuites. Log the DTLS handshake error code in OpenSSLStreamAdapter. Forward the error code to WebRTCSession with the Signals. This part is only for the WebRTC native code. To make it work, need another CL for Chromium. BUG=webrtc:5959 Review-Url: https://codereview.webrtc.org/2167363002 Cr-Commit-Position: refs/heads/master@{#13940} --- webrtc/api/umametrics.h | 1 + webrtc/api/webrtcsession.cc | 10 ++++++++++ webrtc/api/webrtcsession.h | 2 ++ webrtc/base/opensslstreamadapter.cc | 6 ++++++ webrtc/base/sslstreamadapter.cc | 7 +++++++ webrtc/base/sslstreamadapter.h | 10 +++++++--- webrtc/p2p/base/dtlstransportchannel.cc | 7 +++++++ webrtc/p2p/base/dtlstransportchannel.h | 1 + webrtc/p2p/base/transportchannelimpl.h | 3 +++ webrtc/p2p/base/transportcontroller.cc | 6 ++++++ webrtc/p2p/base/transportcontroller.h | 4 ++++ 11 files changed, 54 insertions(+), 3 deletions(-) diff --git a/webrtc/api/umametrics.h b/webrtc/api/umametrics.h index 5374716133..8dbfa22716 100644 --- a/webrtc/api/umametrics.h +++ b/webrtc/api/umametrics.h @@ -32,6 +32,7 @@ enum PeerConnectionEnumCounterType { kEnumCounterVideoSslCipher, kEnumCounterDataSrtpCipher, kEnumCounterDataSslCipher, + kEnumCounterDtlsHandshakeError, kPeerConnectionEnumCounterMax }; diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc index 35a5fab4c6..f8a8f67836 100644 --- a/webrtc/api/webrtcsession.cc +++ b/webrtc/api/webrtcsession.cc @@ -493,6 +493,8 @@ WebRtcSession::WebRtcSession( this, &WebRtcSession::OnTransportControllerCandidatesGathered); transport_controller_->SignalCandidatesRemoved.connect( this, &WebRtcSession::OnTransportControllerCandidatesRemoved); + transport_controller_->SignalDtlsHandshakeError.connect( + this, &WebRtcSession::OnDtlsHandshakeError); } WebRtcSession::~WebRtcSession() { @@ -2073,4 +2075,12 @@ const std::string WebRtcSession::GetTransportName( } return channel->transport_name(); } + +void WebRtcSession::OnDtlsHandshakeError(rtc::SSLHandshakeError error) { + if (metrics_observer_) { + metrics_observer_->IncrementEnumCounter( + webrtc::kEnumCounterDtlsHandshakeError, static_cast(error), + static_cast(rtc::SSLHandshakeError::MAX_VALUE)); + } +} } // namespace webrtc diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h index 3174568b7b..c62ea9984e 100644 --- a/webrtc/api/webrtcsession.h +++ b/webrtc/api/webrtcsession.h @@ -461,6 +461,8 @@ class WebRtcSession : const std::string GetTransportName(const std::string& content_name); + void OnDtlsHandshakeError(rtc::SSLHandshakeError error); + rtc::Thread* const network_thread_; rtc::Thread* const worker_thread_; rtc::Thread* const signaling_thread_; diff --git a/webrtc/base/opensslstreamadapter.cc b/webrtc/base/opensslstreamadapter.cc index b856f3cdb3..4b40c38904 100644 --- a/webrtc/base/opensslstreamadapter.cc +++ b/webrtc/base/opensslstreamadapter.cc @@ -839,6 +839,12 @@ int OpenSSLStreamAdapter::ContinueSSL() { case SSL_ERROR_ZERO_RETURN: default: LOG(LS_VERBOSE) << " -- error " << code; + SSLHandshakeError ssl_handshake_err = SSLHandshakeError::UNKNOWN; + int err_code = ERR_peek_last_error(); + if (err_code != 0 && ERR_GET_REASON(err_code) == SSL_R_NO_SHARED_CIPHER) { + ssl_handshake_err = SSLHandshakeError::INCOMPATIBLE_CIPHERSUITE; + } + SignalSSLHandshakeError(ssl_handshake_err); return (ssl_error != 0) ? ssl_error : -1; } diff --git a/webrtc/base/sslstreamadapter.cc b/webrtc/base/sslstreamadapter.cc index c34fc90bf5..17e758e811 100644 --- a/webrtc/base/sslstreamadapter.cc +++ b/webrtc/base/sslstreamadapter.cc @@ -108,6 +108,13 @@ SSLStreamAdapter* SSLStreamAdapter::Create(StreamInterface* stream) { #endif // SSL_USE_OPENSSL } +SSLStreamAdapter::SSLStreamAdapter(StreamInterface* stream) + : StreamAdapterInterface(stream), + ignore_bad_cert_(false), + client_auth_enabled_(true) {} + +SSLStreamAdapter::~SSLStreamAdapter() {} + bool SSLStreamAdapter::GetSslCipherSuite(int* cipher_suite) { return false; } diff --git a/webrtc/base/sslstreamadapter.h b/webrtc/base/sslstreamadapter.h index 7fe113a602..a7ef23fc79 100644 --- a/webrtc/base/sslstreamadapter.h +++ b/webrtc/base/sslstreamadapter.h @@ -110,6 +110,9 @@ enum SSLProtocolVersion { // Errors for Read -- in the high range so no conflict with OpenSSL. enum { SSE_MSG_TRUNC = 0xff0001 }; +// Used to send back UMA histogram value. Logged when Dtls handshake fails. +enum class SSLHandshakeError { UNKNOWN, INCOMPATIBLE_CIPHERSUITE, MAX_VALUE }; + class SSLStreamAdapter : public StreamAdapterInterface { public: // Instantiate an SSLStreamAdapter wrapping the given stream, @@ -117,9 +120,8 @@ class SSLStreamAdapter : public StreamAdapterInterface { // Caller is responsible for freeing the returned object. static SSLStreamAdapter* Create(StreamInterface* stream); - explicit SSLStreamAdapter(StreamInterface* stream) - : StreamAdapterInterface(stream), ignore_bad_cert_(false), - client_auth_enabled_(true) { } + explicit SSLStreamAdapter(StreamInterface* stream); + ~SSLStreamAdapter() override; void set_ignore_bad_cert(bool ignore) { ignore_bad_cert_ = ignore; } bool ignore_bad_cert() const { return ignore_bad_cert_; } @@ -225,6 +227,8 @@ class SSLStreamAdapter : public StreamAdapterInterface { // depending on specific SSL implementation. static std::string SslCipherSuiteToName(int cipher_suite); + sigslot::signal1 SignalSSLHandshakeError; + private: // If true, the server certificate need not match the configured // server_name, and in fact missing certificate authority and other diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc index 6e8213c606..3dde8fea05 100644 --- a/webrtc/p2p/base/dtlstransportchannel.cc +++ b/webrtc/p2p/base/dtlstransportchannel.cc @@ -280,6 +280,8 @@ bool DtlsTransportChannelWrapper::SetupDtls() { dtls_->SetMaxProtocolVersion(ssl_max_version_); dtls_->SetServerRole(ssl_role_); dtls_->SignalEvent.connect(this, &DtlsTransportChannelWrapper::OnDtlsEvent); + dtls_->SignalSSLHandshakeError.connect( + this, &DtlsTransportChannelWrapper::OnDtlsHandshakeError); if (!dtls_->SetPeerCertificateDigest( remote_fingerprint_algorithm_, reinterpret_cast(remote_fingerprint_value_.data()), @@ -676,4 +678,9 @@ void DtlsTransportChannelWrapper::OnChannelStateChanged( SignalStateChanged(this); } +void DtlsTransportChannelWrapper::OnDtlsHandshakeError( + rtc::SSLHandshakeError error) { + SignalDtlsHandshakeError(error); +} + } // namespace cricket diff --git a/webrtc/p2p/base/dtlstransportchannel.h b/webrtc/p2p/base/dtlstransportchannel.h index 9cba249aca..9f6c563b62 100644 --- a/webrtc/p2p/base/dtlstransportchannel.h +++ b/webrtc/p2p/base/dtlstransportchannel.h @@ -220,6 +220,7 @@ class DtlsTransportChannelWrapper : public TransportChannelImpl { int last_sent_packet_id, bool ready_to_send); void OnChannelStateChanged(TransportChannelImpl* channel); + void OnDtlsHandshakeError(rtc::SSLHandshakeError error); rtc::Thread* worker_thread_; // Everything should occur on this thread. // Underlying channel, not owned by this class. diff --git a/webrtc/p2p/base/transportchannelimpl.h b/webrtc/p2p/base/transportchannelimpl.h index 59065a10a8..910ec3133e 100644 --- a/webrtc/p2p/base/transportchannelimpl.h +++ b/webrtc/p2p/base/transportchannelimpl.h @@ -100,6 +100,9 @@ class TransportChannelImpl : public TransportChannel { // Emitted whenever the transport channel state changed. sigslot::signal1 SignalStateChanged; + // Emitted whenever the Dtls handshake failed on some transport channel. + sigslot::signal1 SignalDtlsHandshakeError; + private: RTC_DISALLOW_COPY_AND_ASSIGN(TransportChannelImpl); }; diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index b5021ccecc..1a13ca14b4 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -191,6 +191,8 @@ TransportChannel* TransportController::CreateTransportChannel_n( this, &TransportController::OnChannelRoleConflict_n); channel->SignalStateChanged.connect( this, &TransportController::OnChannelStateChanged_n); + channel->SignalDtlsHandshakeError.connect( + this, &TransportController::OnDtlsHandshakeError); channels_.insert(channels_.end(), RefCountedChannel(channel))->AddRef(); // Adding a channel could cause aggregate state to change. UpdateAggregateStates_n(); @@ -685,4 +687,8 @@ void TransportController::UpdateAggregateStates_n() { } } +void TransportController::OnDtlsHandshakeError(rtc::SSLHandshakeError error) { + SignalDtlsHandshakeError(error); +} + } // namespace cricket diff --git a/webrtc/p2p/base/transportcontroller.h b/webrtc/p2p/base/transportcontroller.h index da3bab3fe4..a32620281a 100644 --- a/webrtc/p2p/base/transportcontroller.h +++ b/webrtc/p2p/base/transportcontroller.h @@ -119,6 +119,8 @@ class TransportController : public sigslot::has_slots<>, // for unit test const rtc::scoped_refptr& certificate_for_testing(); + sigslot::signal1 SignalDtlsHandshakeError; + protected: // Protected and virtual so we can override it in unit tests. virtual Transport* CreateTransport_n(const std::string& transport_name); @@ -202,6 +204,8 @@ class TransportController : public sigslot::has_slots<>, void UpdateAggregateStates_n(); + void OnDtlsHandshakeError(rtc::SSLHandshakeError error); + rtc::Thread* const signaling_thread_ = nullptr; rtc::Thread* const network_thread_ = nullptr; typedef std::map TransportMap;