JNI+mm: Generate certificate if non-default key type is specified.

webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm

Index: webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm
diff --git a/webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm b/webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm
index 0bb85a29b6d937fb8d5f2fc8be77cb401bed079d..5beae99ed990a9ada498b6d069612842034902d3 100644
--- a/webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm
+++ b/webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm
@@ -15,6 +15,7 @@
 #import "RTCIceServer+Private.h"
 #import "WebRTC/RTCLogging.h"
 
+#include "webrtc/base/rtccertificategenerator.h"
 #include "webrtc/base/sslidentity.h"
 
 @implementation RTCConfiguration
@@ -74,39 +75,43 @@
 
 #pragma mark - Private
 
-- (webrtc::PeerConnectionInterface::RTCConfiguration)nativeConfiguration {
-  webrtc::PeerConnectionInterface::RTCConfiguration nativeConfig;
+- (webrtc::PeerConnectionInterface::RTCConfiguration*)nativeConfiguration {

[tkchin_webrtc, 2016/05/13 17:48:05]

RTCConfiguration *)

[hbos, 2016/05/16 12:49:05]

Done.

+  std::unique_ptr<webrtc::PeerConnectionInterface::RTCConfiguration>

[tkchin_webrtc, 2016/05/13 17:48:05]

Are we returning a pointer to avoid memory copy? Or to surface an error with
certificate generation? Why don't we want a default fallback?

This will cause RTCPeerConnectionFactory to potentially return nil on init,
which isn't great because it's like saying your constructor failed.
We generally reserve returning nil on init for fairly exceptional circumstances
- when is certificate generation expected to fail?

It may be better for RTCConfiguration to store a copy of the C++ object on init,
so RTCConfiguration is the object that returns nil on init instead, which makes
more sense, and will be caught by assertions in RTCPeerConnectionFactory.

[tkchin_webrtc, 2016/05/13 18:07:38]

and by factory I meant peerconnection's private initWithFactory:configuration...
method :)
which I guess isn't so bad because it's private and within our control.

[hbos, 2016/05/16 12:49:05]

To say that we failed to generate the configuration asked for (surface
certificate failure).
As for attempting to generate a different certificate as a fallback, if you
choose ECDSA over RSA for security reasons it may(?) be bad if you implicitly
end up with an RSA certificate.
Under very rare circumstances. I don't know how to make it fail or if this ever
happens in practice. Our unittests EXPECT generate certificate to return a
certificate and I haven't ever seen the unittest failing to produce one. But the
lower-level crypto libraries could return null and that is propagated by the
interface instead of crashing.
Do you mean to move the GenerateCertificate code from createNativeConfiguration
to RTCConfiguration's init? That would make _keyType pointless, and it should
then be removed or be made const? If we don't move the code we have the option
to modify the default value of _keyType to support other certificates.

In both cases we end up with an init that could conceivably return nil.
Alternatively we could add an assertion that GenerateCertificate does not fail
and change the code back so that we do not propagate the error.

What do you think I should do? Currently I did not change this and still have
createNativeConfiguration.

+      nativeConfig(new webrtc::PeerConnectionInterface::RTCConfiguration());
 
   for (RTCIceServer *iceServer in _iceServers) {
-    nativeConfig.servers.push_back(iceServer.nativeServer);
+    nativeConfig->servers.push_back(iceServer.nativeServer);
   }
-  nativeConfig.type =
+  nativeConfig->type =
       [[self class] nativeTransportsTypeForTransportPolicy:_iceTransportPolicy];
-  nativeConfig.bundle_policy =
+  nativeConfig->bundle_policy =
       [[self class] nativeBundlePolicyForPolicy:_bundlePolicy];
-  nativeConfig.rtcp_mux_policy =
+  nativeConfig->rtcp_mux_policy =
       [[self class] nativeRtcpMuxPolicyForPolicy:_rtcpMuxPolicy];
-  nativeConfig.tcp_candidate_policy =
+  nativeConfig->tcp_candidate_policy =
       [[self class] nativeTcpCandidatePolicyForPolicy:_tcpCandidatePolicy];
-  nativeConfig.continual_gathering_policy = [[self class]
+  nativeConfig->continual_gathering_policy = [[self class]
       nativeContinualGatheringPolicyForPolicy:_continualGatheringPolicy];
-  nativeConfig.audio_jitter_buffer_max_packets = _audioJitterBufferMaxPackets;
-  nativeConfig.ice_connection_receiving_timeout =
+  nativeConfig->audio_jitter_buffer_max_packets = _audioJitterBufferMaxPackets;
+  nativeConfig->ice_connection_receiving_timeout =
       _iceConnectionReceivingTimeout;
-  nativeConfig.ice_backup_candidate_pair_ping_interval =
+  nativeConfig->ice_backup_candidate_pair_ping_interval =
       _iceBackupCandidatePairPingInterval;
-  if (_keyType == RTCEncryptionKeyTypeECDSA) {
-    std::unique_ptr<rtc::SSLIdentity> identity(
-        rtc::SSLIdentity::Generate(webrtc::kIdentityName, rtc::KT_ECDSA));
-    if (identity) {
-      nativeConfig.certificates.push_back(
-          rtc::RTCCertificate::Create(std::move(identity)));
-    } else {
-      RTCLogWarning(@"Failed to generate ECDSA identity. RSA will be used.");
+  rtc::KeyType keyType =
+      [[self class] nativeEncryptionKeyTypeForKeyType:_keyType];
+  // Generate non-default certificate.
+  if (keyType != rtc::KT_DEFAULT) {
+    rtc::scoped_refptr<rtc::RTCCertificate> certificate =
+        rtc::RTCCertificateGenerator::GenerateCertificate(
+            rtc::KeyParams(keyType), rtc::Optional<uint64_t>());
+    if (!certificate) {
+      RTCLogWarning(@"Failed to generate certificate.");

[tkchin_webrtc, 2016/05/13 17:48:05]

this is an error because you will fail to create things like factory
RTCLogError

[tkchin_webrtc, 2016/05/13 18:07:38]

I meant peerconnection via [factory peerconnectionWithConfiguration..]

[hbos, 2016/05/16 12:49:05]

RTCLogError - Done.

+      return nullptr;
     }
+    nativeConfig->certificates.push_back(certificate);
   }
 
-  return nativeConfig;
+  return nativeConfig.release();
 }
 
 + (webrtc::PeerConnectionInterface::IceTransportsType)
@@ -224,6 +229,16 @@
   }
 }
 
++ (rtc::KeyType)nativeEncryptionKeyTypeForKeyType:
+    (RTCEncryptionKeyType)keyType {
+  switch (keyType) {
+    case RTCEncryptionKeyTypeRSA:
+      return rtc::KT_RSA;
+    case RTCEncryptionKeyTypeECDSA:
+      return rtc::KT_ECDSA;
+  }
+}
+
 + (RTCTcpCandidatePolicy)tcpCandidatePolicyForNativePolicy:
     (webrtc::PeerConnectionInterface::TcpCandidatePolicy)nativePolicy {
   switch (nativePolicy) {