Revert of Provide RSA2048 as per RFC (patchset #9 id:200001 of https://codereview.webrtc.org/1329493005/ )
Reason for revert:
Breaks chrome.
Original issue's description:
> provide RSA2048 as per RFC
>
> BUG=webrtc:4972
>
> Committed: https://crrev.com/0df3eb03c9a6a8299d7e18c8c314ca58c2f0681e
> Cr-Commit-Position: refs/heads/master@{#10209}
TBR=hbos@webrtc.org,juberti@google.com,jbauch@webrtc.org,henrikg@webrtc.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:4972
Review URL: https://codereview.webrtc.org/1397703002
Cr-Commit-Position: refs/heads/master@{#10210}
diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc
index d7db041..2853ca4 100644
--- a/talk/app/webrtc/webrtcsession_unittest.cc
+++ b/talk/app/webrtc/webrtcsession_unittest.cc
@@ -596,9 +596,8 @@
rtc::ToString(rtc::CreateRandomId());
// Confirmed to work with KT_RSA and KT_ECDSA.
tdesc_factory_->set_certificate(rtc::RTCCertificate::Create(
- rtc::scoped_ptr<rtc::SSLIdentity>(
- rtc::SSLIdentity::Generate(identity_name, rtc::KT_DEFAULT))
- .Pass()));
+ rtc::scoped_ptr<rtc::SSLIdentity>(rtc::SSLIdentity::Generate(
+ identity_name, rtc::KT_DEFAULT)).Pass()));
tdesc_factory_->set_secure(cricket::SEC_REQUIRED);
}
diff --git a/webrtc/base/opensslidentity.cc b/webrtc/base/opensslidentity.cc
index feda674..de4e6a7 100644
--- a/webrtc/base/opensslidentity.cc
+++ b/webrtc/base/opensslidentity.cc
@@ -33,6 +33,9 @@
// We could have exposed a myriad of parameters for the crypto stuff,
// but keeping it simple seems best.
+// Strength of generated keys. Those are RSA.
+static const int KEY_LENGTH = 1024;
+
// Random bits for certificate serial number
static const int SERIAL_RAND_BITS = 64;
@@ -43,16 +46,15 @@
static const int CERTIFICATE_WINDOW = -60*60*24;
// Generate a key pair. Caller is responsible for freeing the returned object.
-static EVP_PKEY* MakeKey(const KeyParams& key_params) {
+static EVP_PKEY* MakeKey(KeyType key_type) {
LOG(LS_INFO) << "Making key pair";
EVP_PKEY* pkey = EVP_PKEY_new();
- if (key_params.type() == KT_RSA) {
- int key_length = key_params.rsa_params().mod_size;
+ if (key_type == KT_RSA) {
BIGNUM* exponent = BN_new();
RSA* rsa = RSA_new();
if (!pkey || !exponent || !rsa ||
- !BN_set_word(exponent, key_params.rsa_params().pub_exp) ||
- !RSA_generate_key_ex(rsa, key_length, exponent, NULL) ||
+ !BN_set_word(exponent, 0x10001) || // 65537 RSA exponent
+ !RSA_generate_key_ex(rsa, KEY_LENGTH, exponent, NULL) ||
!EVP_PKEY_assign_RSA(pkey, rsa)) {
EVP_PKEY_free(pkey);
BN_free(exponent);
@@ -62,23 +64,16 @@
}
// ownership of rsa struct was assigned, don't free it.
BN_free(exponent);
- } else if (key_params.type() == KT_ECDSA) {
- if (key_params.ec_curve() == EC_NIST_P256) {
- EC_KEY* ec_key = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
- if (!pkey || !ec_key || !EC_KEY_generate_key(ec_key) ||
- !EVP_PKEY_assign_EC_KEY(pkey, ec_key)) {
- EVP_PKEY_free(pkey);
- EC_KEY_free(ec_key);
- LOG(LS_ERROR) << "Failed to make EC key pair";
- return NULL;
- }
- // ownership of ec_key struct was assigned, don't free it.
- } else {
- // Add generation of any other curves here.
+ } else if (key_type == KT_ECDSA) {
+ EC_KEY* ec_key = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
+ if (!pkey || !ec_key || !EC_KEY_generate_key(ec_key) ||
+ !EVP_PKEY_assign_EC_KEY(pkey, ec_key)) {
EVP_PKEY_free(pkey);
- LOG(LS_ERROR) << "ECDSA key requested for unknown curve";
+ EC_KEY_free(ec_key);
+ LOG(LS_ERROR) << "Failed to make EC key pair";
return NULL;
}
+ // ownership of ec_key struct was assigned, don't free it.
} else {
EVP_PKEY_free(pkey);
LOG(LS_ERROR) << "Key type requested not understood";
@@ -160,8 +155,8 @@
}
}
-OpenSSLKeyPair* OpenSSLKeyPair::Generate(const KeyParams& key_params) {
- EVP_PKEY* pkey = MakeKey(key_params);
+OpenSSLKeyPair* OpenSSLKeyPair::Generate(KeyType key_type) {
+ EVP_PKEY* pkey = MakeKey(key_type);
if (!pkey) {
LogSSLErrors("Generating key pair");
return NULL;
@@ -384,7 +379,7 @@
OpenSSLIdentity* OpenSSLIdentity::GenerateInternal(
const SSLIdentityParams& params) {
- OpenSSLKeyPair* key_pair = OpenSSLKeyPair::Generate(params.key_params);
+ OpenSSLKeyPair* key_pair = OpenSSLKeyPair::Generate(params.key_type);
if (key_pair) {
OpenSSLCertificate* certificate =
OpenSSLCertificate::Generate(key_pair, params);
@@ -397,12 +392,12 @@
}
OpenSSLIdentity* OpenSSLIdentity::Generate(const std::string& common_name,
- const KeyParams& key_params) {
+ KeyType key_type) {
SSLIdentityParams params;
- params.key_params = key_params;
params.common_name = common_name;
params.not_before = CERTIFICATE_WINDOW;
params.not_after = CERTIFICATE_LIFETIME;
+ params.key_type = key_type;
return GenerateInternal(params);
}
diff --git a/webrtc/base/opensslidentity.h b/webrtc/base/opensslidentity.h
index f957ef2..d8ba138 100644
--- a/webrtc/base/opensslidentity.h
+++ b/webrtc/base/opensslidentity.h
@@ -32,7 +32,7 @@
ASSERT(pkey_ != NULL);
}
- static OpenSSLKeyPair* Generate(const KeyParams& key_params);
+ static OpenSSLKeyPair* Generate(KeyType key_type);
virtual ~OpenSSLKeyPair();
@@ -100,7 +100,7 @@
class OpenSSLIdentity : public SSLIdentity {
public:
static OpenSSLIdentity* Generate(const std::string& common_name,
- const KeyParams& key_params);
+ KeyType key_type);
static OpenSSLIdentity* GenerateForTest(const SSLIdentityParams& params);
static SSLIdentity* FromPEMStrings(const std::string& private_key,
const std::string& certificate);
diff --git a/webrtc/base/ssladapter_unittest.cc b/webrtc/base/ssladapter_unittest.cc
index 7869b6e..9b69451 100644
--- a/webrtc/base/ssladapter_unittest.cc
+++ b/webrtc/base/ssladapter_unittest.cc
@@ -131,10 +131,10 @@
class SSLAdapterTestDummyServer : public sigslot::has_slots<> {
public:
explicit SSLAdapterTestDummyServer(const rtc::SSLMode& ssl_mode,
- const rtc::KeyParams& key_params)
+ const rtc::KeyType key_type)
: ssl_mode_(ssl_mode) {
// Generate a key pair and a certificate for this host.
- ssl_identity_.reset(rtc::SSLIdentity::Generate(GetHostname(), key_params));
+ ssl_identity_.reset(rtc::SSLIdentity::Generate(GetHostname(), key_type));
server_socket_.reset(CreateSocket(ssl_mode_));
@@ -271,10 +271,10 @@
public sigslot::has_slots<> {
public:
explicit SSLAdapterTestBase(const rtc::SSLMode& ssl_mode,
- const rtc::KeyParams& key_params)
+ const rtc::KeyType key_type)
: ssl_mode_(ssl_mode),
ss_scope_(new rtc::VirtualSocketServer(NULL)),
- server_(new SSLAdapterTestDummyServer(ssl_mode_, key_params)),
+ server_(new SSLAdapterTestDummyServer(ssl_mode_, key_type)),
client_(new SSLAdapterTestDummyClient(ssl_mode_)),
handshake_wait_(kTimeout) {}
@@ -348,25 +348,25 @@
class SSLAdapterTestTLS_RSA : public SSLAdapterTestBase {
public:
SSLAdapterTestTLS_RSA()
- : SSLAdapterTestBase(rtc::SSL_MODE_TLS, rtc::KeyParams::RSA()) {}
+ : SSLAdapterTestBase(rtc::SSL_MODE_TLS, rtc::KT_RSA) {}
};
class SSLAdapterTestTLS_ECDSA : public SSLAdapterTestBase {
public:
SSLAdapterTestTLS_ECDSA()
- : SSLAdapterTestBase(rtc::SSL_MODE_TLS, rtc::KeyParams::ECDSA()) {}
+ : SSLAdapterTestBase(rtc::SSL_MODE_TLS, rtc::KT_ECDSA) {}
};
class SSLAdapterTestDTLS_RSA : public SSLAdapterTestBase {
public:
SSLAdapterTestDTLS_RSA()
- : SSLAdapterTestBase(rtc::SSL_MODE_DTLS, rtc::KeyParams::RSA()) {}
+ : SSLAdapterTestBase(rtc::SSL_MODE_DTLS, rtc::KT_RSA) {}
};
class SSLAdapterTestDTLS_ECDSA : public SSLAdapterTestBase {
public:
SSLAdapterTestDTLS_ECDSA()
- : SSLAdapterTestBase(rtc::SSL_MODE_DTLS, rtc::KeyParams::ECDSA()) {}
+ : SSLAdapterTestBase(rtc::SSL_MODE_DTLS, rtc::KT_ECDSA) {}
};
#if SSL_USE_OPENSSL
diff --git a/webrtc/base/sslidentity.cc b/webrtc/base/sslidentity.cc
index b3336ea..ce209dd 100644
--- a/webrtc/base/sslidentity.cc
+++ b/webrtc/base/sslidentity.cc
@@ -108,8 +108,8 @@
}
SSLIdentity* SSLIdentity::Generate(const std::string& common_name,
- const KeyParams& key_params) {
- return OpenSSLIdentity::Generate(common_name, key_params);
+ KeyType key_type) {
+ return OpenSSLIdentity::Generate(common_name, key_type);
}
SSLIdentity* SSLIdentity::GenerateForTest(const SSLIdentityParams& params) {
diff --git a/webrtc/base/sslidentity.h b/webrtc/base/sslidentity.h
index 99cbac8..3a1bbd0 100644
--- a/webrtc/base/sslidentity.h
+++ b/webrtc/base/sslidentity.h
@@ -18,7 +18,6 @@
#include <vector>
#include "webrtc/base/buffer.h"
-#include "webrtc/base/checks.h"
#include "webrtc/base/messagedigest.h"
namespace rtc {
@@ -108,105 +107,25 @@
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 };
-static const int kRsaDefaultModSize = 1024;
-static const int kRsaDefaultExponent = 0x10001; // = 2^16+1 = 65537
-static const int kRsaMinModSize = 1024;
-static const int kRsaMaxModSize = 8192;
-
-struct RSAParams {
- unsigned int mod_size;
- unsigned int pub_exp;
-};
-
-enum ECCurve { EC_NIST_P256, /* EC_FANCY, */ EC_LAST };
-
-class KeyParams {
- public:
- // Generate a KeyParams object from a simple KeyType, using default params.
- explicit KeyParams(KeyType key_type = KT_DEFAULT) {
- 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 = kRsaDefaultModSize,
- int pub_exp = kRsaDefaultExponent) {
- 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 ECDSA specifying the curve.
- static KeyParams ECDSA(ECCurve curve = EC_NIST_P256) {
- KeyParams kt(KT_ECDSA);
- kt.params_.curve = curve;
- 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 >= kRsaMinModSize &&
- params_.rsa.mod_size <= kRsaMaxModSize &&
- 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 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.
+// 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.
struct SSLIdentityParams {
std::string common_name;
- int not_before; // offset from current time in seconds.
- int not_after; // offset from current time in seconds.
- KeyParams key_params;
+ int not_before; // in seconds.
+ int not_after; // in seconds.
+ KeyType key_type;
};
// Our identity in an SSL negotiation: a keypair and certificate (both
@@ -220,11 +139,7 @@
// Returns NULL on failure.
// Caller is responsible for freeing the returned object.
static SSLIdentity* Generate(const std::string& common_name,
- const KeyParams& key_param);
- static SSLIdentity* Generate(const std::string& common_name,
- KeyType key_type) {
- return Generate(common_name, KeyParams(key_type));
- }
+ KeyType key_type);
// Generates an identity with the specified validity period.
static SSLIdentity* GenerateForTest(const SSLIdentityParams& params);
diff --git a/webrtc/base/sslstreamadapter_unittest.cc b/webrtc/base/sslstreamadapter_unittest.cc
index a3e8d9c..c65bb63 100644
--- a/webrtc/base/sslstreamadapter_unittest.cc
+++ b/webrtc/base/sslstreamadapter_unittest.cc
@@ -161,12 +161,11 @@
class SSLStreamAdapterTestBase : public testing::Test,
public sigslot::has_slots<> {
public:
- SSLStreamAdapterTestBase(
- const std::string& client_cert_pem,
- const std::string& client_private_key_pem,
- bool dtls,
- rtc::KeyParams client_key_type = rtc::KeyParams(rtc::KT_DEFAULT),
- rtc::KeyParams server_key_type = rtc::KeyParams(rtc::KT_DEFAULT))
+ SSLStreamAdapterTestBase(const std::string& client_cert_pem,
+ const std::string& client_private_key_pem,
+ bool dtls,
+ rtc::KeyType client_key_type = rtc::KT_DEFAULT,
+ rtc::KeyType server_key_type = rtc::KT_DEFAULT)
: client_buffer_(kFifoBufferSize),
server_buffer_(kFifoBufferSize),
client_stream_(
@@ -225,17 +224,17 @@
server_ssl_->SignalEvent.connect(this, &SSLStreamAdapterTestBase::OnEvent);
rtc::SSLIdentityParams client_params;
- client_params.key_params = rtc::KeyParams(rtc::KT_DEFAULT);
client_params.common_name = "client";
client_params.not_before = not_before;
client_params.not_after = not_after;
+ client_params.key_type = rtc::KT_DEFAULT;
client_identity_ = rtc::SSLIdentity::GenerateForTest(client_params);
rtc::SSLIdentityParams server_params;
- server_params.key_params = rtc::KeyParams(rtc::KT_DEFAULT);
server_params.common_name = "server";
server_params.not_before = not_before;
server_params.not_after = not_after;
+ server_params.key_type = rtc::KT_DEFAULT;
server_identity_ = rtc::SSLIdentity::GenerateForTest(server_params);
client_ssl_->SetIdentity(client_identity_);
@@ -463,7 +462,7 @@
class SSLStreamAdapterTestTLS
: public SSLStreamAdapterTestBase,
- public WithParamInterface<tuple<rtc::KeyParams, rtc::KeyParams>> {
+ public WithParamInterface<tuple<rtc::KeyType, rtc::KeyType>> {
public:
SSLStreamAdapterTestTLS()
: SSLStreamAdapterTestBase("",
@@ -571,7 +570,7 @@
class SSLStreamAdapterTestDTLS
: public SSLStreamAdapterTestBase,
- public WithParamInterface<tuple<rtc::KeyParams, rtc::KeyParams>> {
+ public WithParamInterface<tuple<rtc::KeyType, rtc::KeyType>> {
public:
SSLStreamAdapterTestDTLS()
: SSLStreamAdapterTestBase("",
@@ -979,10 +978,9 @@
ASSERT_TRUE(GetSslCipherSuite(false, &server_cipher));
ASSERT_EQ(client_cipher, server_cipher);
- ASSERT_EQ(
- rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
- rtc::SSL_PROTOCOL_DTLS_10, ::testing::get<1>(GetParam()).type()),
- server_cipher);
+ ASSERT_EQ(rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
+ rtc::SSL_PROTOCOL_DTLS_10, ::testing::get<1>(GetParam())),
+ server_cipher);
}
// Test getting the used DTLS 1.2 ciphers.
@@ -998,10 +996,9 @@
ASSERT_TRUE(GetSslCipherSuite(false, &server_cipher));
ASSERT_EQ(client_cipher, server_cipher);
- ASSERT_EQ(
- rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
- rtc::SSL_PROTOCOL_DTLS_12, ::testing::get<1>(GetParam()).type()),
- server_cipher);
+ ASSERT_EQ(rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
+ rtc::SSL_PROTOCOL_DTLS_12, ::testing::get<1>(GetParam())),
+ server_cipher);
}
// DTLS 1.2 enabled for client only -> DTLS 1.0 will be used.
@@ -1016,10 +1013,9 @@
ASSERT_TRUE(GetSslCipherSuite(false, &server_cipher));
ASSERT_EQ(client_cipher, server_cipher);
- ASSERT_EQ(
- rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
- rtc::SSL_PROTOCOL_DTLS_10, ::testing::get<1>(GetParam()).type()),
- server_cipher);
+ ASSERT_EQ(rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
+ rtc::SSL_PROTOCOL_DTLS_10, ::testing::get<1>(GetParam())),
+ server_cipher);
}
// DTLS 1.2 enabled for server only -> DTLS 1.0 will be used.
@@ -1034,30 +1030,16 @@
ASSERT_TRUE(GetSslCipherSuite(false, &server_cipher));
ASSERT_EQ(client_cipher, server_cipher);
- ASSERT_EQ(
- rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
- rtc::SSL_PROTOCOL_DTLS_10, ::testing::get<1>(GetParam()).type()),
- server_cipher);
+ ASSERT_EQ(rtc::SSLStreamAdapter::GetDefaultSslCipherForTest(
+ rtc::SSL_PROTOCOL_DTLS_10, ::testing::get<1>(GetParam())),
+ server_cipher);
}
-// The RSA keysizes here might look strange, why not include the RFC's size
-// 2048?. The reason is test case slowness; testing two sizes to exercise
-// parametrization is sufficient.
-INSTANTIATE_TEST_CASE_P(
- SSLStreamAdapterTestsTLS,
- SSLStreamAdapterTestTLS,
- Combine(Values(rtc::KeyParams::RSA(1024, 65537),
- rtc::KeyParams::RSA(1152, 65537),
- rtc::KeyParams::ECDSA(rtc::EC_NIST_P256)),
- Values(rtc::KeyParams::RSA(1024, 65537),
- rtc::KeyParams::RSA(1152, 65537),
- rtc::KeyParams::ECDSA(rtc::EC_NIST_P256))));
-INSTANTIATE_TEST_CASE_P(
- SSLStreamAdapterTestsDTLS,
- SSLStreamAdapterTestDTLS,
- Combine(Values(rtc::KeyParams::RSA(1024, 65537),
- rtc::KeyParams::RSA(1152, 65537),
- rtc::KeyParams::ECDSA(rtc::EC_NIST_P256)),
- Values(rtc::KeyParams::RSA(1024, 65537),
- rtc::KeyParams::RSA(1152, 65537),
- rtc::KeyParams::ECDSA(rtc::EC_NIST_P256))));
+INSTANTIATE_TEST_CASE_P(SSLStreamAdapterTestsTLS,
+ SSLStreamAdapterTestTLS,
+ Combine(Values(rtc::KT_RSA, rtc::KT_ECDSA),
+ Values(rtc::KT_RSA, rtc::KT_ECDSA)));
+INSTANTIATE_TEST_CASE_P(SSLStreamAdapterTestsDTLS,
+ SSLStreamAdapterTestDTLS,
+ Combine(Values(rtc::KT_RSA, rtc::KT_ECDSA),
+ Values(rtc::KT_RSA, rtc::KT_ECDSA)));
