Provide RSA2048 as per RFC

webrtc/base/sslidentity.h

Index: webrtc/base/sslidentity.h
diff --git a/webrtc/base/sslidentity.h b/webrtc/base/sslidentity.h
index 3a1bbd08563bf5d58faaaf184633220fecb45709..af6cf3dccb8f9ec5108e5fb95656de14c1964f21 100644
--- a/webrtc/base/sslidentity.h
+++ b/webrtc/base/sslidentity.h
@@ -18,6 +18,7 @@
 #include <vector>
 
 #include "webrtc/base/buffer.h"
+#include "webrtc/base/checks.h"
 #include "webrtc/base/messagedigest.h"
 
 namespace rtc {
@@ -107,25 +108,125 @@ class SSLCertChain {
   RTC_DISALLOW_COPY_AND_ASSIGN(SSLCertChain);
 };
 
+// KT_DEFAULT is currently an alias for KT_RSA.  This is likely to change.
+// KT_LAST is intended for vector declarations and loops over all key types;
+// it does not represent any key type in itself.
 // TODO(hbos,torbjorng): Don't change KT_DEFAULT without first updating
 // PeerConnectionFactory_nativeCreatePeerConnection's certificate generation
 // code.
-enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_DEFAULT = KT_RSA };
+enum KeyType { KT_RSA, KT_ECDSA, KT_LAST, KT_INVALID, KT_DEFAULT = KT_RSA };

[juberti, 2015/10/07 06:35:22]

I didn't see any usage of KT_LAST in this CL. I could be persuaded if we
actually used it, but is it serving a purpose?

[hbos, 2015/10/07 10:04:40]

It is used today by dtlsidentitystore.h. It is probably the only usage and after
the store is updated (different CL) I'm guessing it will no longer be needed.

[torbjorng (webrtc), 2015/10/07 13:30:03]

I have the habit of putting in a _LAST in order for people not to
use any temporal last active enum tag as the enum's dimension.

And since it is used for that purpose at the moment, I'll leave
it in.

+
+static const int kRsaDefaultModSize = 1024;
+static const int kRsaDefaultExponent = 0x10001;  // = 2^16+1 = 65537
+
+struct RSAParams {
+  unsigned int mod_size;
+  unsigned int pub_exp;
+};
+
+enum ECCurve { EC_NIST_P256, /* EC_FANCY, */ EC_LAST };
+
+class KeyParams {
+ public:
+  // Default ctor only needed by gtest, it would be nice to avoid this.
+  // We set grossly invalid parameters to discourage its use.
+  KeyParams() {

[juberti, 2015/10/07 06:35:22]

This seems wrong to me. Why not init with KT_DEFAULT here? Then you can avoid
KT_INVALID altogether. (See comment below - you can probably just add a default
argument of KT_DEFAULT to the ctor below).

[hbos, 2015/10/07 10:04:40]

(I would also prefer default constructor to create something default and usable
and remove KT_INVALID)

[torbjorng (webrtc), 2015/10/07 13:30:04]

OK, I'll revert to that form.

+    type_ = KT_INVALID;                       // Bad type.
+    memset(&params_, 0xff, sizeof(params_));  // Bad values.
+  }
+
+  // Generate a KeyParams object from a simple KeyType, using default params.
+  explicit KeyParams(KeyType key_type) {
+    if (key_type == KT_ECDSA) {
+      type_ = KT_ECDSA;
+      params_.curve = EC_NIST_P256;
+    } else if (key_type == KT_RSA) {
+      type_ = KT_RSA;
+      params_.rsa.mod_size = kRsaDefaultModSize;
+      params_.rsa.pub_exp = kRsaDefaultExponent;
+    } else {
+      RTC_NOTREACHED();
+    }
+  }
+
+  // Generate a a KeyParams for RSA with explicit parameters.
+  static KeyParams RSA(int mod_size, int pub_exp) {

[juberti, 2015/10/07 06:35:22]

I looked and you are correct; the style guide does allow for default arguments
in ctors (and I presume, factory functions). I think this class will be simpler
if you use default arguments, so go for it.

[hbos, 2015/10/07 10:04:40]

Yay!

[torbjorng (webrtc), 2015/10/07 13:30:03]

Great!  The style guide is not 100% clear here.

+    KeyParams kt(KT_RSA);
+    kt.params_.rsa.mod_size = mod_size;
+    kt.params_.rsa.pub_exp = pub_exp;
+    return kt;
+  }
+
+  // Generate a a KeyParams for RSA defaulting parameters.
+  static KeyParams RSA() {
+    KeyParams kt(KT_RSA);
+    kt.params_.rsa.mod_size = kRsaDefaultModSize;
+    kt.params_.rsa.pub_exp = kRsaDefaultExponent;
+    return kt;
+  }
+
+  // Generate a a KeyParams for ECDSA specifying the curve.
+  static KeyParams ECDSA(ECCurve curve) {
+    KeyParams kt(KT_ECDSA);
+    kt.params_.curve = curve;
+    return kt;
+  }
+
+  // Generate a a KeyParams for ECDSA defaulting the curve.
+  static KeyParams ECDSA() {
+    KeyParams kt(KT_ECDSA);
+    kt.params_.curve = EC_NIST_P256;
+    return kt;
+  }
+
+  // Check validity of a KeyParams object. Since the factory functions have
+  // no way of returning errors, this function can be called after creation
+  // to make sure the parameters are OK.
+  bool IsValid() {
+    if (type_ == KT_RSA) {
+      return (params_.rsa.mod_size >= 1024 && params_.rsa.mod_size <= 8192 &&

[juberti, 2015/10/07 06:35:22]

These should probably be constants.

[torbjorng (webrtc), 2015/10/07 13:30:04]

Done.

+              params_.rsa.pub_exp > params_.rsa.mod_size);
+    } else  if (type_ == KT_ECDSA) {
+      return (params_.curve == EC_NIST_P256);
+    }
+    return false;
+  }
+
+  RSAParams rsa_params() const {
+    RTC_DCHECK(type_ == KT_RSA);
+    return params_.rsa;
+  }
+
+  ECCurve ec_curve() const {
+    RTC_DCHECK(type_ == KT_ECDSA);
+    return params_.curve;
+  }
+
+  KeyType type() const { return type_; }
+
+ private:
+  KeyType type_;
+  union {
+    RSAParams rsa;
+    ECCurve curve;
+  } params_;
+};
 
 // TODO(hbos): Remove once rtc::KeyType (to be modified) and
 // blink::WebRTCKeyType (to be landed) match. By using this function in Chromium
 // appropriately we can change KeyType enum -> class without breaking Chromium.
 KeyType IntKeyTypeFamilyToKeyType(int key_type_family);
 
-// Parameters for generating an identity for testing. If common_name is
-// non-empty, it will be used for the certificate's subject and issuer name,
-// otherwise a random string will be used. |not_before| and |not_after| are
-// offsets to the current time in number of seconds.
+// Parameters for generating a certificate. If |common_name| is non-empty, it
+// will be used for the certificate's subject and issuer name, otherwise a
+// random string will be used.
 struct SSLIdentityParams {
+  SSLIdentityParams(const KeyParams& key_params) : key_params(key_params) {}

[juberti, 2015/10/07 06:35:22]

It seems odd to have a ctor that only initializes one parameter. I would either
make it initialize everything, or nothing.

[torbjorng (webrtc), 2015/10/07 13:30:03]

Ack.  It ended up like this since the KeyParams default ctor was
voted against.  :-)

I'm removing this ugly SSLIdentityParams ctor and let its init
rely on the default KeyParams ctor.

+
   std::string common_name;
-  int not_before;  // in seconds.
-  int not_after;  // in seconds.
-  KeyType key_type;
+  int not_before;  // offset from current time in seconds.
+  int not_after;   // offset from current time in seconds.
+  KeyParams key_params;
 };
 
 // Our identity in an SSL negotiation: a keypair and certificate (both
@@ -139,7 +240,11 @@ class SSLIdentity {
   // Returns NULL on failure.
   // Caller is responsible for freeing the returned object.
   static SSLIdentity* Generate(const std::string& common_name,
-                               KeyType key_type);
+                               const KeyParams& key_param);
+  static SSLIdentity* Generate(const std::string& common_name,
+                               KeyType key_type) {
+    return Generate(common_name, KeyParams(key_type));
+  }
 
   // Generates an identity with the specified validity period.
   static SSLIdentity* GenerateForTest(const SSLIdentityParams& params);