Add IsAcceptableCipher, use instead of GetDefaultCipher.

webrtc/base/opensslstreamadapter.cc

Index: webrtc/base/opensslstreamadapter.cc
diff --git a/webrtc/base/opensslstreamadapter.cc b/webrtc/base/opensslstreamadapter.cc
index b2afebb75be6c32e0b770349fde2b4d8180e8f66..d7ed6b883628c57f80253f836873da9a8b85e61f 100644
--- a/webrtc/base/opensslstreamadapter.cc
+++ b/webrtc/base/opensslstreamadapter.cc
@@ -147,51 +147,6 @@ static const SslCipherMapEntry kSslCipherMap[] = {
 #pragma warning(disable : 4310)
 #endif  // defined(_MSC_VER)
 
-// Default cipher used between OpenSSL/BoringSSL stream adapters.
-// This needs to be updated when the default of the SSL library changes.
-// static_cast<uint16_t> causes build warnings on windows platform.
-static int kDefaultSslCipher10 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_AES_256_CBC_SHA);
-static int kDefaultSslEcCipher10 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_AES_256_CBC_SHA);
-#ifdef OPENSSL_IS_BORINGSSL
-static int kDefaultSslCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_AES_128_GCM_SHA256);
-static int kDefaultSslEcCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256);
-// Fallback cipher for DTLS 1.2 if hardware-accelerated AES-GCM is unavailable.
-
-#ifdef TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
-// This ciphersuite was added in boringssl 13414b3a..., changing the fallback
-// ciphersuite.  For compatibility during a transitional period, support old
-// boringssl versions.  TODO(torbjorng): Remove this.
-static int kDefaultSslCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256);
-#else
-static int kDefaultSslCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_CHACHA20_POLY1305_OLD);
-#endif
-
-#ifdef TLS1_CK_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
-// This ciphersuite was added in boringssl 13414b3a..., changing the fallback
-// ciphersuite.  For compatibility during a transitional period, support old
-// boringssl versions.  TODO(torbjorng): Remove this.
-static int kDefaultSslEcCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256);
-#else
-static int kDefaultSslEcCipher12NoAesGcm =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_CHACHA20_POLY1305_OLD);
-#endif
-
-#else  // !OPENSSL_IS_BORINGSSL
-// OpenSSL sorts differently than BoringSSL, so the default cipher doesn't
-// change between TLS 1.0 and TLS 1.2 with the current setup.
-static int kDefaultSslCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_RSA_WITH_AES_256_CBC_SHA);
-static int kDefaultSslEcCipher12 =
-    static_cast<uint16_t>(TLS1_CK_ECDHE_ECDSA_WITH_AES_256_CBC_SHA);
-#endif  // OPENSSL_IS_BORINGSSL
-
 #if defined(_MSC_VER)
 #pragma warning(pop)
 #endif  // defined(_MSC_VER)
@@ -1147,46 +1102,77 @@ bool OpenSSLStreamAdapter::HaveExporter() {
 #endif
 }
 
-int OpenSSLStreamAdapter::GetDefaultSslCipherForTest(SSLProtocolVersion version,
-                                                     KeyType key_type) {
-  if (key_type == KT_RSA) {
-    switch (version) {
-      case SSL_PROTOCOL_TLS_10:
-      case SSL_PROTOCOL_TLS_11:
-        return kDefaultSslCipher10;
-      case SSL_PROTOCOL_TLS_12:
-      default:
-#ifdef OPENSSL_IS_BORINGSSL
-        if (EVP_has_aes_hardware()) {
-          return kDefaultSslCipher12;
-        } else {
-          return kDefaultSslCipher12NoAesGcm;
-        }
-#else  // !OPENSSL_IS_BORINGSSL
-        return kDefaultSslCipher12;
+#define CDEF(X) \
+  { static_cast<uint16_t>(TLS1_CK_##X & 0xffff), "TLS_" #X }
+
+struct cipher_list {
+  uint16_t cipher;
+  const char* cipher_str;
+};
+
+// TODO(torbjorng): Perhaps add more cipher suites to these lists.
+static const cipher_list OK_RSA_ciphers[] = {
+  CDEF(ECDHE_RSA_WITH_AES_128_CBC_SHA),
+  CDEF(ECDHE_RSA_WITH_AES_256_CBC_SHA),
+  CDEF(ECDHE_RSA_WITH_AES_128_GCM_SHA256),
+#ifdef TLS1_CK_ECDHE_RSA_WITH_AES_256_GCM_SHA256
+  CDEF(ECDHE_RSA_WITH_AES_256_GCM_SHA256),
 #endif
-    }
-  } else if (key_type == KT_ECDSA) {
-    switch (version) {
-      case SSL_PROTOCOL_TLS_10:
-      case SSL_PROTOCOL_TLS_11:
-        return kDefaultSslEcCipher10;
-      case SSL_PROTOCOL_TLS_12:
-      default:
-#ifdef OPENSSL_IS_BORINGSSL
-        if (EVP_has_aes_hardware()) {
-          return kDefaultSslEcCipher12;
-        } else {
-          return kDefaultSslEcCipher12NoAesGcm;
-        }
-#else  // !OPENSSL_IS_BORINGSSL
-        return kDefaultSslEcCipher12;
+  CDEF(ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256),
+};
+
+static const cipher_list OK_ECDSA_ciphers[] = {
+  CDEF(ECDHE_ECDSA_WITH_AES_128_CBC_SHA),
+  CDEF(ECDHE_ECDSA_WITH_AES_256_CBC_SHA),
+  CDEF(ECDHE_ECDSA_WITH_AES_128_GCM_SHA256),
+#ifdef TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA256
+  CDEF(ECDHE_ECDSA_WITH_AES_256_GCM_SHA256),
 #endif
+  CDEF(ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256),
+};
+
+bool OpenSSLStreamAdapter::IsAcceptableCipher(int cipher,
+                                              KeyType key_type) {

[davidben_webrtc, 2016/03/08 20:29:05]

What is the test actually trying to check? Are you trying to make sure the key
type was honored or...? Previously it seems you were somehow interested in the
protocol version, but this one isn't.

I'm asking because there may be more direct ways to check what you want. You
could assert on the actual protocol version, etc.

[torbjorng (webrtc), 2016/03/09 13:10:31]

Mainly I am trying to assert we got a reasonable cipher suite. Separating RSA
and ECDSA sanity checks the logic of our code (since we separate them there).

I think it makes sense to whitelist cipher suites like this, even at the price
of making our test code somewhat sensitive to boringssl default changes. Now the
only change we will need is to add a line or two in our whitelist. I'd be
interested if there is a better way.

You're right that the old code somewhat indirectly checked that the expected
protocol version was used. We should check that again, with a definitive check
(by means of SSL_version(const SSL *ssl), (which unfortunately is
BoringSSL-specific and we still pretend to support plain OpenSSL).

[davidben_webrtc, 2016/03/09 20:30:26]

I think it depends on what you're interested in checking. If you want to assert
something about the cipher suite, you're probably stuck with this. I suppose you
can query the certificate out of the SSL* and then check the key type there? I
dunno. I can get you an API to categorize ciphers one way or another, but it
won't work in OpenSSL.
SSL_version's been in OpenSSL since forever. :-)

[torbjorng (webrtc), 2016/03/10 21:29:03]

Thanks, let's consider that for the future, but stick with my solution for now.
(This CL was not planned, I need to move on.)

It is doubtful whether WebRTC can be compiled against plain OpenSSL. We
conditionalize for it, but none of the bots test that it still works, which
probably means it doesn't.
Right.  I realised that when I implemented the checks yesterday.

[davidben_webrtc, 2016/03/10 21:40:05]

Really? I've been under the impression you all need to support it. If not, that
would be wonderful!

+  if (key_type == KT_RSA) {
+    for (const cipher_list &c : OK_RSA_ciphers) {
+      if (cipher == c.cipher)
+        return true;
+    }
+  }
+
+  if (key_type == KT_ECDSA) {
+    for (const cipher_list &c : OK_ECDSA_ciphers) {
+      if (cipher == c.cipher)
+        return true;
+    }
+  }
+
+  // TODO(torbjorng): Remove before landing.
+  LOG(LS_ERROR) << "Attempted use of truly terrible cipher suite: "
+                << OpenSSLStreamAdapter::SslCipherSuiteToName(cipher) << "("
+                << cipher << ")";
+  return false;
+}
+
+bool OpenSSLStreamAdapter::IsAcceptableCipher(std::string cipher,
+                                              KeyType key_type) {
+  if (key_type == KT_RSA) {
+    for (const cipher_list &c : OK_RSA_ciphers) {
+      if (cipher == c.cipher_str)
+        return true;
     }
-  } else {
-    RTC_NOTREACHED();
-    return kDefaultSslEcCipher12;
   }
+
+  if (key_type == KT_ECDSA) {
+    for (const cipher_list &c : OK_ECDSA_ciphers) {
+      if (cipher == c.cipher_str)
+        return true;
+    }
+  }
+
+  // TODO(torbjorng): Remove before landing.
+  LOG(LS_ERROR) << "Attempted use of truly terrible cipher suite: " << cipher;
+  return false;
 }
 
 }  // namespace rtc