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}

rtc_base/opensslstreamadapter.cc

@@ -852,7 +852,7 @@ int OpenSSLStreamAdapter::ContinueSSL() {
       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 @@ SSL_CTX* OpenSSLStreamAdapter::SetupSSLContext() {
 #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 @@ bool OpenSSLStreamAdapter::IsAcceptableCipher(const std::string& cipher,
   return false;
 }
 
-void OpenSSLStreamAdapter::enable_time_callback_for_testing() {
+void OpenSSLStreamAdapter::EnableTimeCallbackForTesting() {
   g_use_time_callback_for_testing = true;
 }
 

rtc_base/opensslstreamadapter.h

@@ -120,7 +120,7 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
 
   // 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 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
   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 {

rtc_base/ssladapter_unittest.cc

@@ -267,7 +267,7 @@ class SSLAdapterTestDummyServer : public sigslot::has_slots<> {
     // (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());
 

rtc_base/sslstreamadapter.cc

@@ -94,9 +94,7 @@ SSLStreamAdapter* SSLStreamAdapter::Create(StreamInterface* stream) {
 }
 
 SSLStreamAdapter::SSLStreamAdapter(StreamInterface* stream)
-    : StreamAdapterInterface(stream),
-      ignore_bad_cert_(false),
-      client_auth_enabled_(true) {}
+    : StreamAdapterInterface(stream) {}
 
 SSLStreamAdapter::~SSLStreamAdapter() {}
 
@@ -135,8 +133,13 @@ bool SSLStreamAdapter::IsAcceptableCipher(const std::string& cipher,
 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();
 }
 
 ///////////////////////////////////////////////////////////////////////////////

rtc_base/sslstreamadapter.h

@@ -116,12 +116,6 @@ class SSLStreamAdapter : public StreamAdapterInterface {
   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 @@ class SSLStreamAdapter : public StreamAdapterInterface {
   // 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

rtc_base/unittest_main.cc

@@ -117,7 +117,7 @@ int main(int argc, char* argv[]) {
 
   // 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);