Change WebRTC SslCipher to be exposed as number only.

webrtc/base/sslstreamadapter.h

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_BASE_SSLSTREAMADAPTER_H_
 #define WEBRTC_BASE_SSLSTREAMADAPTER_H_
 
 #include <string>
 #include <vector>
 
 #include "webrtc/base/stream.h"
 #include "webrtc/base/sslidentity.h"
 
 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
 // encrypted of course.
 //
 // This class was written with SSLAdapter as a starting point. It
 // offers a similar interface, with two differences: there is no
 // support for a restartable SSL connection, and this class has a
 // peer-to-peer mode.
 //
@@ skipping 96 matching lines @@
                                         const unsigned char* digest_val,
                                         size_t digest_len) = 0;
 
   // Retrieves the peer's X.509 certificate, if a connection has been
   // established. It returns the transmitted over SSL, including the entire
   // chain. The returned certificate is owned by the caller.
   virtual bool GetPeerCertificate(SSLCertificate** cert) const = 0;
 
   // 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:
   // label               -- the exporter label.
   //                        part of the RFC defining each exporter
   //                        usage (IN)
   // context/context_len -- a context to bind to for this connection;
   //                        optional, can be NULL, 0 (IN)
   // use_context         -- whether to use the context value
   //                        (needed to distinguish no context from
@@ skipping 11 matching lines @@
   virtual bool SetDtlsSrtpCiphers(const std::vector<std::string>& ciphers);
   virtual bool GetDtlsSrtpCipher(std::string* cipher);
 
   // Capabilities testing
   static bool HaveDtls();
   static bool HaveDtlsSrtp();
   static bool HaveExporter();
 
   // 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
   // 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_;
 };
 
 }  // namespace rtc
 
 #endif  // WEBRTC_BASE_SSLSTREAMADAPTER_H_