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..adc48e828f835303f71957680624dc62bbceaa86 100644
--- a/webrtc/base/sslidentity.h
+++ b/webrtc/base/sslidentity.h
@@ -107,25 +107,95 @@ 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.
+// The WebRTC RFC draft mandates KT_ECDSA and KT_RSA2048.
 // 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 };

[hbos, 2015/09/29 13:53:18]

Is KT_DEFAULT still used/something we want to have given KeyTypeFull::Default()?

[juberti, 2015/09/29 16:44:05]

We don't usually use _LAST enums. Is there a specific need for this?

[torbjorng (webrtc), 2015/10/01 11:43:18]

It is still used in one place, associated to the identitystore.
Will add a TODO for now.

[torbjorng (webrtc), 2015/10/01 11:43:19]

It is intended for array decls and loops, and is resilient to adding
new enum tags. It is currently used for a vector declaration.

 
+enum { RSA_MOD_SIZE_DEFAULT = 1024 };

[juberti, 2015/09/29 16:44:05]

These should be const ints, not enums. They should both have RSA_ prefix (or
kRsa for const int)

[torbjorng (webrtc), 2015/10/01 11:43:18]

Done.

+enum { PUB_EXP_DEFAULT = 65537 };
+
+struct RSAParams {
+  int mod_size;
+  int pub_exp;
+};
+
+enum ECCurve { EC_NIST_P256, /* EC_FANCY, */ EC_LAST };
+
+union KeyParams {
+  RSAParams rsa;
+  ECCurve curve;
+};
+
+class KeyTypeFull {

[hbos, 2015/09/29 13:53:18]

Document supported parameter ranges here or in Generate implementations. I
suppose the supported ranges could be implementation-specific. Now there is only
one implementation? Would be best if all implementations are required to support
the same parameter sets so that you can be completely agnostic about what
implementation is used when you call Generate.

Assuming all implementations should support the same parameter sets consider
adding a method for checking if a KeyTypeFull is a valid parameter set (boolean
isValid() inside KeyTypeFull).
Blink/chromium code will have to know if its generateCertificate input is valid
or not, it would be nice if we didn't have validate parameters code in multiple
places.

[juberti, 2015/09/29 16:44:05]

I think KeyParams would be a better name here. You could use a nameless union
instead of using union for KeyParams, and that will also make the code less
verbose.

[torbjorng (webrtc), 2015/10/01 11:43:18]

Added an isValid() method.

[torbjorng (webrtc), 2015/10/01 11:43:19]

Agreed.

+ public:
+  explicit KeyTypeFull() {

[hbos, 2015/09/29 13:53:18]

Remove explicit keyword here. I would make this constructor private in favor of
using Default() publicly unless there is a reason for having a parameterless
constructor.

[juberti, 2015/09/29 16:44:05]

I would just remove this constructor altogether. I don't see a need for a
default ctor on this object given that we have KT_DEFAULT already.

[torbjorng (webrtc), 2015/10/01 11:43:18]

Sure, but then we need a constructor for SSLIdentityParams (which includes a
KeyParams).  I'm adding that.

However, a default constructor is required by gtest, so I put it back.

+    type_ = KT_RSA;
+    params_.rsa.mod_size = RSA_MOD_SIZE_DEFAULT;
+    params_.rsa.pub_exp = PUB_EXP_DEFAULT;
+  }
+
+  explicit KeyTypeFull(KeyType key_type) {

[hbos, 2015/09/29 13:53:18]

I suggest replacing this with a factory function "static KeyTypeFull
Default(KeyType key_type)", that way you don't have to have a mixture of factory
functions and constructors, and the name hints what type of construction it
does.

[torbjorng (webrtc), 2015/10/05 12:03:05]

Acknowledged.

+    if (key_type == KT_ECDSA) {
+      type_ = KT_ECDSA;
+      params_.curve = EC_NIST_P256;
+    } else {
+      type_ = KT_RSA;
+      params_.rsa.mod_size = RSA_MOD_SIZE_DEFAULT;
+      params_.rsa.pub_exp = PUB_EXP_DEFAULT;
+    }
+  }
+
+  static KeyTypeFull RSA(int mod_size = RSA_MOD_SIZE_DEFAULT,

[juberti, 2015/09/29 16:44:05]

Default parameters are discouraged by the style guide. I think if you use this
form (as opposed to just passing KeyType), you should be expected to fill in all
the parameters.

[torbjorng (webrtc), 2015/10/01 11:43:18]

The style guide allows it for ctors, I assumed that factory functions are like
ctors in this respect.  Apparently not, so I will change this.

+                         int pub_exp = PUB_EXP_DEFAULT) {

[hbos, 2015/09/29 13:53:18]

I'm happy with calling them RSA, ECDSA and Default. Factory functions are
usually called CreateBlah but I couldn't find anything about this in the style
guide. "Create" sounds a bit heavy for such a light-weight class, and its a more
cumbersome name. As you and/or other reviewers please.

[torbjorng (webrtc), 2015/10/05 12:03:05]

Acknowledged.

+    KeyTypeFull kt(KT_RSA);
+    kt.params_.rsa.mod_size = mod_size;
+    kt.params_.rsa.pub_exp = pub_exp;
+    return kt;
+  }
+
+  static KeyTypeFull ECDSA(ECCurve curve = EC_NIST_P256) {

[juberti, 2015/09/29 16:44:05]

Same here.

[torbjorng (webrtc), 2015/10/01 11:43:19]

Done.

+    KeyTypeFull kt(KT_ECDSA);
+    kt.params_.curve = curve;
+    return kt;
+  }
+
+  static KeyTypeFull Default() { return RSA(); }

[juberti, 2015/09/29 16:44:05]

Not sure we need this

[torbjorng (webrtc), 2015/10/01 11:43:18]

Removed.

+
+  RSAParams rsa_params() const {
+    // DCHECK(type_ == KT_RSA);
+    return params_.rsa;
+  }
+
+  ECCurve ec_params() const {

[hbos, 2015/09/29 13:53:18]

Does it make sense to have an ECParams struct containing the curve in case we
ever want to add a second parameter to the EC case? And so that KeyParams is a
list of "<Algorithm>Params" members

[torbjorng (webrtc), 2015/10/01 11:43:19]

I think "named curve" is the only reasonable parameterization of ECDSA.
Else, we'll need the polynomial coefficients and the coefficient field
explicitly defined, which would be a bunch of bignum params.

[hbos, 2015/10/01 14:42:43]

Acknowledged.

+    // DCHECK(type_ == KT_ECDSA);
+    return params_.curve;
+  }
+
+  KeyType type() const { return type_; }
+
+ private:
+  KeyType type_;
+  KeyParams 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);

[hbos, 2015/09/29 13:53:18]

This file is already pretty long with multiple classes.
Now that the key type stuff is more than a one line enum, consider moving all
the key type-related stuff to a new file.

[torbjorng (webrtc), 2015/10/05 12:03:05]

Let's consider that for a separate CL.
(Incidentally, I'm reducing this file's size by purging SCHANNEL.)

 
-// 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 an identity for. 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 {
   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.
+  KeyTypeFull key_type;
 };
 
 // Our identity in an SSL negotiation: a keypair and certificate (both
@@ -139,7 +209,7 @@ 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);
+                               KeyTypeFull key_type);

[juberti, 2015/09/29 16:44:05]

as this is taking a structure, it should be const KeyParams&, rather than
passing by value

[torbjorng (webrtc), 2015/10/01 11:43:19]

This surely is always a good idea for large structs/classes.
For a like this class which is <= 64 bits, things can live in
registers...

Anyway, this is not time-critical code, so I changed it.

 
   // Generates an identity with the specified validity period.
   static SSLIdentity* GenerateForTest(const SSLIdentityParams& params);