Refactor: Removing IgnoreBadCert from SSLStreamAdapter. Make test methods more explicit.
We have several places in the SSL APIs where we will poke holes through the API
surface with boolean flags to enable scenarios like disabling authentication.
This isn't an ideal approach because it is error prone and confusing to the
API user. Instead authentication should be dependency injected with a default
secure component and a fake can be created for testing.
For now this CL just cleans up the left over unused test flags and renames the
remaining ones with a ForTesting postfix to make it very clear they shouldn't
be used in any production code.
Bug: webrtc:9860
Change-Id: I31f55cf85097bacb9cd895c16a6fad3773cd1c2b
Reviewed-on: https://webrtc-review.googlesource.com/c/107786
Commit-Queue: Benjamin Wright <benwright@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25377}diff --git a/rtc_base/opensslstreamadapter.cc b/rtc_base/opensslstreamadapter.cc
index 47ca125..3ee8fad 100644
--- a/rtc_base/opensslstreamadapter.cc
+++ b/rtc_base/opensslstreamadapter.cc
@@ -852,7 +852,7 @@
RTC_LOG(LS_VERBOSE) << " -- success";
// By this point, OpenSSL should have given us a certificate, or errored
// out if one was missing.
- RTC_DCHECK(peer_cert_chain_ || !client_auth_enabled());
+ RTC_DCHECK(peer_cert_chain_ || !GetClientAuthEnabled());
state_ = SSL_CONNECTED;
if (!waiting_to_verify_peer_certificate()) {
@@ -1050,7 +1050,7 @@
#endif
int mode = SSL_VERIFY_PEER;
- if (client_auth_enabled()) {
+ if (GetClientAuthEnabled()) {
// Require a certificate from the client.
// Note: Normally this is always true in production, but it may be disabled
// for testing purposes (e.g. SSLAdapter unit tests).
@@ -1238,7 +1238,7 @@
return false;
}
-void OpenSSLStreamAdapter::enable_time_callback_for_testing() {
+void OpenSSLStreamAdapter::EnableTimeCallbackForTesting() {
g_use_time_callback_for_testing = true;
}
diff --git a/rtc_base/opensslstreamadapter.h b/rtc_base/opensslstreamadapter.h
index d282932..927fe2b 100644
--- a/rtc_base/opensslstreamadapter.h
+++ b/rtc_base/opensslstreamadapter.h
@@ -120,7 +120,7 @@
// Use our timeutils.h source of timing in BoringSSL, allowing us to test
// using a fake clock.
- static void enable_time_callback_for_testing();
+ static void EnableTimeCallbackForTesting();
protected:
void OnEvent(StreamInterface* stream, int events, int err) override;
@@ -176,7 +176,7 @@
static int SSLVerifyCallback(X509_STORE_CTX* store, void* arg);
bool waiting_to_verify_peer_certificate() const {
- return client_auth_enabled() && !peer_certificate_verified_;
+ return GetClientAuthEnabled() && !peer_certificate_verified_;
}
bool has_peer_certificate_digest() const {
diff --git a/rtc_base/ssladapter_unittest.cc b/rtc_base/ssladapter_unittest.cc
index 8ed460f..c84c668 100644
--- a/rtc_base/ssladapter_unittest.cc
+++ b/rtc_base/ssladapter_unittest.cc
@@ -267,7 +267,7 @@
// (e.g. a WebRTC-based application and an RFC 5766 TURN server), where
// clients are not required to provide a certificate during handshake.
// Accordingly, we must disable client authentication here.
- ssl_stream_adapter_->set_client_auth_enabled(false);
+ ssl_stream_adapter_->SetClientAuthEnabledForTesting(false);
ssl_stream_adapter_->SetIdentity(ssl_identity_->GetReference());
diff --git a/rtc_base/sslstreamadapter.cc b/rtc_base/sslstreamadapter.cc
index fe42bf4..9c33a9c 100644
--- a/rtc_base/sslstreamadapter.cc
+++ b/rtc_base/sslstreamadapter.cc
@@ -94,9 +94,7 @@
}
SSLStreamAdapter::SSLStreamAdapter(StreamInterface* stream)
- : StreamAdapterInterface(stream),
- ignore_bad_cert_(false),
- client_auth_enabled_(true) {}
+ : StreamAdapterInterface(stream) {}
SSLStreamAdapter::~SSLStreamAdapter() {}
@@ -135,8 +133,13 @@
std::string SSLStreamAdapter::SslCipherSuiteToName(int cipher_suite) {
return OpenSSLStreamAdapter::SslCipherSuiteToName(cipher_suite);
}
-void SSLStreamAdapter::enable_time_callback_for_testing() {
- OpenSSLStreamAdapter::enable_time_callback_for_testing();
+
+///////////////////////////////////////////////////////////////////////////////
+// Test only settings
+///////////////////////////////////////////////////////////////////////////////
+
+void SSLStreamAdapter::EnableTimeCallbackForTesting() {
+ OpenSSLStreamAdapter::EnableTimeCallbackForTesting();
}
///////////////////////////////////////////////////////////////////////////////
diff --git a/rtc_base/sslstreamadapter.h b/rtc_base/sslstreamadapter.h
index dad8a4e..25f4f33 100644
--- a/rtc_base/sslstreamadapter.h
+++ b/rtc_base/sslstreamadapter.h
@@ -116,12 +116,6 @@
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_; }
-
- 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. 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.
@@ -235,22 +229,32 @@
// depending on specific SSL implementation.
static std::string SslCipherSuiteToName(int cipher_suite);
+ ////////////////////////////////////////////////////////////////////////////
+ // Testing only member functions
+ ////////////////////////////////////////////////////////////////////////////
+
// Use our timeutils.h source of timing in BoringSSL, allowing us to test
// using a fake clock.
- static void enable_time_callback_for_testing();
+ static void EnableTimeCallbackForTesting();
+
+ // Deprecated. Do not use this API outside of testing.
+ // Do not set this to false outside of testing.
+ void SetClientAuthEnabledForTesting(bool enabled) {
+ client_auth_enabled_ = enabled;
+ }
+
+ // Deprecated. Do not use this API outside of testing.
+ // Returns true by default, else false if explicitly set to disable client
+ // authentication.
+ bool GetClientAuthEnabled() const { return client_auth_enabled_; }
sigslot::signal1<SSLHandshakeError> SignalSSLHandshakeError;
private:
- // If true, the server certificate need not match the configured
- // server_name, and in fact missing certificate authority and other
- // verification errors are ignored.
- bool ignore_bad_cert_;
-
// If true (default), the client is required to provide a certificate during
// handshake. If no certificate is given, handshake fails. This applies to
// server mode only.
- bool client_auth_enabled_;
+ bool client_auth_enabled_ = true;
};
} // namespace rtc
diff --git a/rtc_base/unittest_main.cc b/rtc_base/unittest_main.cc
index 28a1bbb..5fd3a99 100644
--- a/rtc_base/unittest_main.cc
+++ b/rtc_base/unittest_main.cc
@@ -117,7 +117,7 @@
// Initialize SSL which are used by several tests.
rtc::InitializeSSL();
- rtc::SSLStreamAdapter::enable_time_callback_for_testing();
+ rtc::SSLStreamAdapter::EnableTimeCallbackForTesting();
#if defined(WEBRTC_IOS)
rtc::test::InitTestSuite(RUN_ALL_TESTS, argc, argv, false);