Change WebRTC SslCipher to be exposed as number only.

webrtc/base/sslstreamadapter.h

Index: webrtc/base/sslstreamadapter.h
diff --git a/webrtc/base/sslstreamadapter.h b/webrtc/base/sslstreamadapter.h
index 4fb238a290482d13d00569bc57ccc5cdc45389f8..d546a14f7a37d9d0fb903ce2252817880fddcb26 100644
--- a/webrtc/base/sslstreamadapter.h
+++ b/webrtc/base/sslstreamadapter.h
@@ -19,6 +19,27 @@
 
 namespace rtc {
 
+// This follows https://tools.ietf.org/html/rfc5764#section-4.1.2.

[Ryan Sleevi, 2015/09/28 17:24:17]

// DTLS-SRTP protection profiles, as described in
https://tools.ietf.org/html/rfc5764#section-4.1.2

[guoweis_webrtc, 2015/09/30 04:09:47]

Done.

+enum SrtpCipherType {

[Ryan Sleevi, 2015/09/28 17:24:17]

Is this really a Cipher type? According to the RFC, it's a protection profile,
which itself matches an SDP Crypto Suite.

[pthatcher1, 2015/09/29 22:25:17]

In https://tools.ietf.org/html/rfc4568#section-6.2, it's called "SRTP crypto
suites".  I think it would make sense to call the enum "SrtpCryptoSuite" (no
need for Type on the end) with prefix "SRTP_".  Then the names would match the
constants in https://tools.ietf.org/html/rfc5764#section-4.1.2.

[guoweis_webrtc, 2015/09/30 04:09:47]

Yes, it should be named cryptos. However, it's consistently misused as Cipher
across webrtc code. I will put a TODO there.

[pthatcher1, 2015/09/30 05:46:22]

Can you at least name the new enum type you are adding to what we want it to be
long-term?  I still would prefer SrtpCryptoSuite with SRTP_ as the prefix rather
than SrtpCipher_.

+  SrtpCipher_Unknown = 0,
+  SrtpCipher_AES_CM_128_HMAC_SHA1_80 = 1,
+  SrtpCipher_AES_CM_128_HMAC_SHA1_32 = 2,
+  SrtpCipher_NULL_HMAC_SHA1_80 = 5,
+  SrtpCipher_NULL_HMAC_SHA1_32 = 6,

[pthatcher1, 2015/09/29 22:25:16]

Do we actually use these anywhere?  If not, can we remove them?

[guoweis_webrtc, 2015/09/30 04:09:47]

Done.

+};
+
+// Convert SRTP Cipher's RFC names to its ID. TODO(guoweis): Investigate whether
+// there is existing definition already.

[Ryan Sleevi, 2015/09/28 17:24:17]

This comment is quite confusing. There's lots of RFCs at play, as I called out
earlier.

Perhaps this might be better stated

// Returns the DTLS-SRTP protection profile ID, as defined in
https://tools.ietf.org/html/rfc5764#section-4.1.2,
// for the given SDP Crypto-suite, as defined in
https://tools.ietf.org/html/rfc4568#section-6.2 

[pthatcher1, 2015/09/29 22:25:16]

I agree we should mention both RFCs in the comment.  But 4568 calls them "SRTP
crypto suites", not "SDP crypto suites", which happens to be expressed in SDP as
an SDP attribute called the "SDP crypto attribute". 

[guoweis_webrtc, 2015/09/30 04:09:47]

Done.

+SrtpCipherType GetSrtpCipherType(const std::string& cipher_rfc_name);
+
+// Cipher suite to use for SRTP. Typically a 80-bit HMAC will be used, except
+// in applications (voice) where the additional bandwidth may be significant.

[Ryan Sleevi, 2015/09/28 17:24:17]

The comment starting with "typically" seems to be describing too much of the
implementation

[pthatcher1, 2015/09/29 22:25:16]

I think it's more saying how these are typically used in the industry.  Plus, I
think he just moved this comment from the previous location.  I think it's still
accurate.

+// A 80-bit HMAC is always used for SRTCP.
+// 128-bit AES with 80-bit SHA-1 HMAC.
+extern const char CS_AES_CM_128_HMAC_SHA1_80[];
+// 128-bit AES with 32-bit SHA-1 HMAC.
+extern const char CS_AES_CM_128_HMAC_SHA1_32[];
+
 // SSLStreamAdapter : A StreamInterfaceAdapter that does SSL/TLS.
 // After SSL has been started, the stream will only open on successful
 // SSL verification of certificates, and the communication is
@@ -135,7 +156,7 @@ class SSLStreamAdapter : public StreamAdapterInterface {
 
   // Retrieves the name of the cipher suite used for the connection
   // (e.g. "TLS_RSA_WITH_AES_128_CBC_SHA").
-  virtual bool GetSslCipher(std::string* cipher);
+  virtual bool GetSslCipher(uint16_t* cipher);
 
   // Key Exporter interface from RFC 5705
   // Arguments are:
@@ -167,9 +188,10 @@ class SSLStreamAdapter : public StreamAdapterInterface {
 
   // Returns the default Ssl cipher used between streams of this class
   // for the given protocol version. This is used by the unit tests.
-  // TODO(torbjorng@webrtc.org): Fix callers to avoid default parameter.
-  static std::string GetDefaultSslCipher(SSLProtocolVersion version,
-                                         KeyType key_type = KT_DEFAULT);
+  static uint16_t GetDefaultSslCipherForTest(SSLProtocolVersion version,
+                                             KeyType key_type);
+
+  static std::string GetRfcSslCipherName(uint16_t cipher);

[Ryan Sleevi, 2015/09/28 17:24:17]

DESIGN nit: This is not really for this CL, but this sort of design makes things
really weird. It 'seems' as if the idea of the SSLStreamAdapter base class is
meant to make inheritance easier and abstract away the platform implementation,
but as seen in the .cc file, you end up coupling tightly to the derived classes
by virtue of needing to use #ifdefs.

Presumably you'd want this to be non-static - indeed, having static methods on a
virtual base class really strikes as weird design and violation of
encapsulation.

This stood out in this CL, but I'd suggest it's more something to consider for
the future rather than something that requires immediate attention.

[guoweis_webrtc, 2015/09/30 04:09:47]

added a TODO.

 
  private:
   // If true, the server certificate need not match the configured