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

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

 /*
  *  Copyright 2015 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.
  */
 
 #import "RTCConfiguration+Private.h"
 
 #include <memory>
 
 #import "RTCIceServer+Private.h"
 #import "WebRTC/RTCLogging.h"
 
+#include "webrtc/base/rtccertificategenerator.h"
 #include "webrtc/base/sslidentity.h"
 
 @implementation RTCConfiguration
 
 @synthesize iceServers = _iceServers;
 @synthesize iceTransportPolicy = _iceTransportPolicy;
 @synthesize bundlePolicy = _bundlePolicy;
 @synthesize rtcpMuxPolicy = _rtcpMuxPolicy;
 @synthesize tcpCandidatePolicy = _tcpCandidatePolicy;
 @synthesize continualGatheringPolicy = _continualGatheringPolicy;
@@ skipping 39 matching lines @@
       [[self class] stringForTcpCandidatePolicy:_tcpCandidatePolicy],
       [[self class]
           stringForContinualGatheringPolicy:_continualGatheringPolicy],
       _audioJitterBufferMaxPackets,
       _iceConnectionReceivingTimeout,
       _iceBackupCandidatePairPingInterval];
 }
 
 #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)
     nativeTransportsTypeForTransportPolicy:(RTCIceTransportPolicy)policy {
   switch (policy) {
     case RTCIceTransportPolicyNone:
       return webrtc::PeerConnectionInterface::kNone;
     case RTCIceTransportPolicyRelay:
       return webrtc::PeerConnectionInterface::kRelay;
     case RTCIceTransportPolicyNoHost:
@@ skipping 97 matching lines @@
 + (webrtc::PeerConnectionInterface::TcpCandidatePolicy)
     nativeTcpCandidatePolicyForPolicy:(RTCTcpCandidatePolicy)policy {
   switch (policy) {
     case RTCTcpCandidatePolicyEnabled:
       return webrtc::PeerConnectionInterface::kTcpCandidatePolicyEnabled;
     case RTCTcpCandidatePolicyDisabled:
       return webrtc::PeerConnectionInterface::kTcpCandidatePolicyDisabled;
   }
 }
 
++ (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) {
     case webrtc::PeerConnectionInterface::kTcpCandidatePolicyEnabled:
       return RTCTcpCandidatePolicyEnabled;
     case webrtc::PeerConnectionInterface::kTcpCandidatePolicyDisabled:
       return RTCTcpCandidatePolicyDisabled;
   }
 }
 
@@ skipping 31 matching lines @@
     (RTCContinualGatheringPolicy)policy {
   switch (policy) {
     case RTCContinualGatheringPolicyGatherOnce:
       return @"GATHER_ONCE";
     case RTCContinualGatheringPolicyGatherContinually:
       return @"GATHER_CONTINUALLY";
   }
 }
 
 @end