diff --git a/webrtc/base/opensslstreamadapter.cc b/webrtc/base/opensslstreamadapter.cc index 89f628d3e8..b856f3cdb3 100644 --- a/webrtc/base/opensslstreamadapter.cc +++ b/webrtc/base/opensslstreamadapter.cc @@ -286,7 +286,6 @@ OpenSSLStreamAdapter::OpenSSLStreamAdapter(StreamInterface* stream) ssl_write_needs_read_(false), ssl_(NULL), ssl_ctx_(NULL), - custom_verification_succeeded_(false), ssl_mode_(SSL_MODE_TLS), ssl_max_version_(SSL_PROTOCOL_TLS_12) {} @@ -317,7 +316,6 @@ bool OpenSSLStreamAdapter::SetPeerCertificateDigest(const std::string size_t digest_len) { ASSERT(!peer_certificate_); ASSERT(peer_certificate_digest_algorithm_.size() == 0); - ASSERT(ssl_server_name_.empty()); size_t expected_len; if (!OpenSSLDigest::GetDigestSize(digest_alg, &expected_len)) { @@ -470,16 +468,21 @@ bool OpenSSLStreamAdapter::GetDtlsSrtpCryptoSuite(int* crypto_suite) { #endif } -int OpenSSLStreamAdapter::StartSSLWithServer(const char* server_name) { - ASSERT(server_name != NULL && server_name[0] != '\0'); - ssl_server_name_ = server_name; - return StartSSL(); -} +int OpenSSLStreamAdapter::StartSSL() { + ASSERT(state_ == SSL_NONE); -int OpenSSLStreamAdapter::StartSSLWithPeer() { - ASSERT(ssl_server_name_.empty()); - // It is permitted to specify peer_certificate_ only later. - return StartSSL(); + if (StreamAdapterInterface::GetState() != SS_OPEN) { + state_ = SSL_WAIT; + return 0; + } + + state_ = SSL_CONNECTING; + if (int err = BeginSSL()) { + Error("BeginSSL", err, false); + return err; + } + + return 0; } void OpenSSLStreamAdapter::SetMode(SSLMode mode) { @@ -732,36 +735,16 @@ void OpenSSLStreamAdapter::OnEvent(StreamInterface* stream, int events, StreamAdapterInterface::OnEvent(stream, events_to_signal, signal_error); } -int OpenSSLStreamAdapter::StartSSL() { - ASSERT(state_ == SSL_NONE); - - if (StreamAdapterInterface::GetState() != SS_OPEN) { - state_ = SSL_WAIT; - return 0; - } - - state_ = SSL_CONNECTING; - if (int err = BeginSSL()) { - Error("BeginSSL", err, false); - return err; - } - - return 0; -} - int OpenSSLStreamAdapter::BeginSSL() { ASSERT(state_ == SSL_CONNECTING); - // The underlying stream has open. If we are in peer-to-peer mode - // then a peer certificate must have been specified by now. - ASSERT(!ssl_server_name_.empty() || - !peer_certificate_digest_algorithm_.empty()); - LOG(LS_INFO) << "BeginSSL: " - << (!ssl_server_name_.empty() ? ssl_server_name_ : - "with peer"); + // The underlying stream has opened. + // A peer certificate digest must have been specified by now. + ASSERT(!peer_certificate_digest_algorithm_.empty()); + LOG(LS_INFO) << "BeginSSL with peer."; BIO* bio = NULL; - // First set up the context + // First set up the context. ASSERT(ssl_ctx_ == NULL); ssl_ctx_ = SetupSSLContext(); if (!ssl_ctx_) @@ -827,7 +810,7 @@ int OpenSSLStreamAdapter::ContinueSSL() { case SSL_ERROR_NONE: LOG(LS_VERBOSE) << " -- success"; - if (!SSLPostConnectionCheck(ssl_, ssl_server_name_.c_str(), NULL, + if (!SSLPostConnectionCheck(ssl_, NULL, peer_certificate_digest_algorithm_)) { LOG(LS_ERROR) << "TLS post connection check failed"; return -1; @@ -1094,36 +1077,12 @@ int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) { return 1; } -// This code is taken from the "Network Security with OpenSSL" -// sample in chapter 5 bool OpenSSLStreamAdapter::SSLPostConnectionCheck(SSL* ssl, - const char* server_name, const X509* peer_cert, const std::string &peer_digest) { - ASSERT(server_name != NULL); - bool ok; - if (server_name[0] != '\0') { // traditional mode - ok = OpenSSLAdapter::VerifyServerName(ssl, server_name, ignore_bad_cert()); - - if (ok) { - ok = (SSL_get_verify_result(ssl) == X509_V_OK || - custom_verification_succeeded_); - } - } else { // peer-to-peer mode - ASSERT((peer_cert != NULL) || (!peer_digest.empty())); - // no server name validation - ok = true; - } - - if (!ok && ignore_bad_cert()) { - LOG(LS_ERROR) << "SSL_get_verify_result(ssl) = " - << SSL_get_verify_result(ssl); - LOG(LS_INFO) << "Other TLS post connection checks failed."; - ok = true; - } - - return ok; + ASSERT((peer_cert != NULL) || (!peer_digest.empty())); + return true; } bool OpenSSLStreamAdapter::HaveDtls() { diff --git a/webrtc/base/opensslstreamadapter.h b/webrtc/base/opensslstreamadapter.h index 05e8102169..76dbad24a0 100644 --- a/webrtc/base/opensslstreamadapter.h +++ b/webrtc/base/opensslstreamadapter.h @@ -27,29 +27,26 @@ typedef struct x509_store_ctx_st X509_STORE_CTX; namespace rtc { // This class was written with OpenSSLAdapter (a socket adapter) as a -// starting point. It has similar structure and functionality, with -// the peer-to-peer mode added. +// starting point. It has similar structure and functionality, but uses a +// "peer-to-peer" mode, verifying the peer's certificate using a digest +// sent over a secure signaling channel. // // Static methods to initialize and deinit the SSL library are in -// OpenSSLAdapter. This class also uses -// OpenSSLAdapter::custom_verify_callback_ (a static field). These -// should probably be moved out to a neutral class. +// OpenSSLAdapter. These should probably be moved out to a neutral class. // -// In a few cases I have factored out some OpenSSLAdapter code into -// static methods so it can be reused from this class. Eventually that -// code should probably be moved to a common support -// class. Unfortunately there remain a few duplicated sections of -// code. I have not done more restructuring because I did not want to -// affect existing code that uses OpenSSLAdapter. +// In a few cases I have factored out some OpenSSLAdapter code into static +// methods so it can be reused from this class. Eventually that code should +// probably be moved to a common support class. Unfortunately there remain a +// few duplicated sections of code. I have not done more restructuring because +// I did not want to affect existing code that uses OpenSSLAdapter. // -// This class does not support the SSL connection restart feature -// present in OpenSSLAdapter. I am not entirely sure how the feature -// is useful and I am not convinced that it works properly. +// This class does not support the SSL connection restart feature present in +// OpenSSLAdapter. I am not entirely sure how the feature is useful and I am +// not convinced that it works properly. // -// This implementation is careful to disallow data exchange after an -// SSL error, and it has an explicit SSL_CLOSED state. It should not -// be possible to send any data in clear after one of the StartSSL -// methods has been called. +// This implementation is careful to disallow data exchange after an SSL error, +// and it has an explicit SSL_CLOSED state. It should not be possible to send +// any data in clear after one of the StartSSL methods has been called. // Look in sslstreamadapter.h for documentation of the methods. @@ -72,8 +69,9 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter { std::unique_ptr GetPeerCertificate() const override; - int StartSSLWithServer(const char* server_name) override; - int StartSSLWithPeer() override; + // Goes from state SSL_NONE to either SSL_CONNECTING or SSL_WAIT, depending + // on whether the underlying stream is already open or not. + int StartSSL() override; void SetMode(SSLMode mode) override; void SetMaxProtocolVersion(SSLProtocolVersion version) override; @@ -138,10 +136,6 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter { // on some other error cases, so it can't really be interpreted // unfortunately. - // Go from state SSL_NONE to either SSL_CONNECTING or SSL_WAIT, - // depending on whether the underlying stream is already open or - // not. - int StartSSL(); // Prepare SSL library, state is SSL_CONNECTING. int BeginSSL(); // Perform SSL negotiation steps. @@ -165,7 +159,7 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter { // SSL library configuration SSL_CTX* SetupSSLContext(); // SSL verification check - bool SSLPostConnectionCheck(SSL* ssl, const char* server_name, + bool SSLPostConnectionCheck(SSL* ssl, const X509* peer_cert, const std::string& peer_digest); // SSL certification verification error handler, called back from @@ -185,22 +179,15 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter { SSL* ssl_; SSL_CTX* ssl_ctx_; - // Our key and certificate, mostly useful in peer-to-peer mode. + // Our key and certificate. std::unique_ptr identity_; - // in traditional mode, the server name that the server's certificate - // must specify. Empty in peer-to-peer mode. - std::string ssl_server_name_; - // The certificate that the peer must present or did present. Initially - // null in traditional mode, until the connection is established. + // The certificate that the peer presented. Initially null, until the + // connection is established. std::unique_ptr peer_certificate_; - // In peer-to-peer mode, the digest of the certificate that - // the peer must present. + // The digest of the certificate that the peer must present. Buffer peer_certificate_digest_value_; std::string peer_certificate_digest_algorithm_; - // OpenSSLAdapter::custom_verify_callback_ result - bool custom_verification_succeeded_; - // The DtlsSrtp ciphers std::string srtp_ciphers_; diff --git a/webrtc/base/ssladapter_unittest.cc b/webrtc/base/ssladapter_unittest.cc index adce5f4f01..a6ec56ebc0 100644 --- a/webrtc/base/ssladapter_unittest.cc +++ b/webrtc/base/ssladapter_unittest.cc @@ -252,7 +252,7 @@ class SSLAdapterTestDummyServer : public sigslot::has_slots<> { ssl_stream_adapter_->SetPeerCertificateDigest(rtc::DIGEST_SHA_1, digest, digest_len); - ssl_stream_adapter_->StartSSLWithPeer(); + ssl_stream_adapter_->StartSSL(); ssl_stream_adapter_->SignalEvent.connect(this, &SSLAdapterTestDummyServer::OnSSLStreamAdapterEvent); diff --git a/webrtc/base/sslstreamadapter.h b/webrtc/base/sslstreamadapter.h index 4dbe457eba..7fe113a602 100644 --- a/webrtc/base/sslstreamadapter.h +++ b/webrtc/base/sslstreamadapter.h @@ -127,21 +127,18 @@ class SSLStreamAdapter : public StreamAdapterInterface { void set_client_auth_enabled(bool enabled) { client_auth_enabled_ = enabled; } bool client_auth_enabled() const { return client_auth_enabled_; } - // Specify our SSL identity: key and certificate. Mostly this is - // only used in the peer-to-peer mode (unless we actually want to - // provide a client certificate to a server). - // SSLStream takes ownership of the SSLIdentity object and will - // free it when appropriate. Should be called no more than once on a - // given SSLStream instance. + // Specify our SSL identity: key and certificate. SSLStream takes ownership + // of the SSLIdentity object and will free it when appropriate. Should be + // called no more than once on a given SSLStream instance. virtual void SetIdentity(SSLIdentity* identity) = 0; - // Call this to indicate that we are to play the server's role in - // the peer-to-peer mode. - // The default argument is for backward compatibility + // Call this to indicate that we are to play the server role (or client role, + // if the default argument is replaced by SSL_CLIENT). + // The default argument is for backward compatibility. // TODO(ekr@rtfm.com): rename this SetRole to reflect its new function virtual void SetServerRole(SSLRole role = SSL_SERVER) = 0; - // Do DTLS or TLS + // Do DTLS or TLS. virtual void SetMode(SSLMode mode) = 0; // Set maximum supported protocol version. The highest version supported by @@ -151,42 +148,29 @@ class SSLStreamAdapter : public StreamAdapterInterface { // next lower will be used. virtual void SetMaxProtocolVersion(SSLProtocolVersion version) = 0; - // The mode of operation is selected by calling either - // StartSSLWithServer or StartSSLWithPeer. - // Use of the stream prior to calling either of these functions will - // pass data in clear text. - // Calling one of these functions causes SSL negotiation to begin as - // soon as possible: right away if the underlying wrapped stream is - // already opened, or else as soon as it opens. + // StartSSL starts negotiation with a peer, whose certificate is verified + // using the certificate digest. Generally, SetIdentity() and possibly + // SetServerRole() should have been called before this. + // SetPeerCertificateDigest() must also be called. It may be called after + // StartSSLWithPeer() but must be called before the underlying stream opens. // - // These functions return a negative error code on failure. - // Returning 0 means success so far, but negotiation is probably not - // complete and will continue asynchronously. In that case, the - // exposed stream will open after successful negotiation and - // verification, or an SE_CLOSE event will be raised if negotiation - // fails. + // Use of the stream prior to calling StartSSL will pass data in clear text. + // Calling StartSSL causes SSL negotiation to begin as soon as possible: right + // away if the underlying wrapped stream is already opened, or else as soon as + // it opens. + // + // StartSSL returns a negative error code on failure. Returning 0 means + // success so far, but negotiation is probably not complete and will continue + // asynchronously. In that case, the exposed stream will open after + // successful negotiation and verification, or an SE_CLOSE event will be + // raised if negotiation fails. + virtual int StartSSL() = 0; - // StartSSLWithServer starts SSL negotiation with a server in - // traditional mode. server_name specifies the expected server name - // which the server's certificate needs to specify. - virtual int StartSSLWithServer(const char* server_name) = 0; - - // StartSSLWithPeer starts negotiation in the special peer-to-peer - // mode. - // Generally, SetIdentity() and possibly SetServerRole() should have - // been called before this. - // SetPeerCertificate() or SetPeerCertificateDigest() must also be called. - // It may be called after StartSSLWithPeer() but must be called before the - // underlying stream opens. - virtual int StartSSLWithPeer() = 0; - - // Specify the digest of the certificate that our peer is expected to use in - // peer-to-peer mode. Only this certificate will be accepted during - // SSL verification. The certificate is assumed to have been - // obtained through some other secure channel (such as the XMPP - // channel). Unlike SetPeerCertificate(), this must specify the - // terminal certificate, not just a CA. - // SSLStream makes a copy of the digest value. + // Specify the digest of the certificate that our peer is expected to use. + // Only this certificate will be accepted during SSL verification. The + // certificate is assumed to have been obtained through some other secure + // channel (such as the signaling channel). This must specify the terminal + // certificate, not just a CA. SSLStream makes a copy of the digest value. virtual bool SetPeerCertificateDigest(const std::string& digest_alg, const unsigned char* digest_val, size_t digest_len) = 0; diff --git a/webrtc/base/sslstreamadapter_unittest.cc b/webrtc/base/sslstreamadapter_unittest.cc index 07ee855458..341e09ff1e 100644 --- a/webrtc/base/sslstreamadapter_unittest.cc +++ b/webrtc/base/sslstreamadapter_unittest.cc @@ -385,10 +385,10 @@ class SSLStreamAdapterTestBase : public testing::Test, int rv; server_ssl_->SetServerRole(); - rv = server_ssl_->StartSSLWithPeer(); + rv = server_ssl_->StartSSL(); ASSERT_EQ(0, rv); - rv = client_ssl_->StartSSLWithPeer(); + rv = client_ssl_->StartSSL(); ASSERT_EQ(0, rv); // Now run the handshake diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc index a6b06361f4..6e8213c606 100644 --- a/webrtc/p2p/base/dtlstransportchannel.cc +++ b/webrtc/p2p/base/dtlstransportchannel.cc @@ -571,14 +571,14 @@ void DtlsTransportChannelWrapper::OnDtlsEvent(rtc::StreamInterface* dtls, void DtlsTransportChannelWrapper::MaybeStartDtls() { if (dtls_ && channel_->writable()) { - if (dtls_->StartSSLWithPeer()) { + if (dtls_->StartSSL()) { // This should never fail: // Because we are operating in a nonblocking mode and all // incoming packets come in via OnReadPacket(), which rejects // packets in this state, the incoming queue must be empty. We // ignore write errors, thus any errors must be because of // configuration and therefore are our fault. - RTC_DCHECK(false) << "StartSSLWithPeer failed."; + RTC_DCHECK(false) << "StartSSL failed."; LOG_J(LS_ERROR, this) << "Couldn't start DTLS handshake"; set_dtls_state(DTLS_TRANSPORT_FAILED); return;