Add option to enable ECDSA key for Java API.

Add option to enable ECDSA key for Java API.

Committed: https://crrev.com/97579a4e122ad42592b5d7da9475e128da63a948
Cr-Commit-Position: refs/heads/master@{#9835}

[AlexG, 2015-08-25 22:09:27]

glaznev@webrtc.org changed reviewers:
+ hbos@webrtc.org, jiayl@webrtc.org

[AlexG, 2015-08-25 22:09:28]

PTAL

[AlexG, 2015-08-25 22:36:13]

The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-25 22:36:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312293003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312293003/1

[jiayl2, 2015-08-25 23:06:02]

https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
talk/app/webrtc/java/jni/peerconnection_jni.cc:150: g_rtc_certificate =
rtc::RTCCertificate::Create(ssl_identity.Pass());
If the app runs for days or months, the certificate may expire, assuming this
method is called on per app launch?

Also, there is the privacy side that some app may not want to reuse the
certificate across PeerConnection.

I think it'll be better to create a new certificate in createPeerConnection.

[AlexG, 2015-08-25 23:15:26]

https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
talk/app/webrtc/java/jni/peerconnection_jni.cc:150: g_rtc_certificate =
rtc::RTCCertificate::Create(ssl_identity.Pass());
On 2015/08/25 23:06:02, jiayl2 wrote:
> If the app runs for days or months, the certificate may expire, assuming this
> method is called on per app launch?
> 
> Also, there is the privacy side that some app may not want to reuse the
> certificate across PeerConnection.
> 
> I think it'll be better to create a new certificate in createPeerConnection.

ok. Should I call both rtc::SSLIdentity::Generate() and
rtc::RTCCertificate::Create() in createPeerConnection or SSLIdentity can be
generated in JNI_OnLoad()?

[jiayl2, 2015-08-25 23:16:35]

On 2015/08/25 23:15:26, AlexG wrote:
>
https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
> File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):
> 
>
https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
> talk/app/webrtc/java/jni/peerconnection_jni.cc:150: g_rtc_certificate =
> rtc::RTCCertificate::Create(ssl_identity.Pass());
> On 2015/08/25 23:06:02, jiayl2 wrote:
> > If the app runs for days or months, the certificate may expire, assuming
this
> > method is called on per app launch?
> > 
> > Also, there is the privacy side that some app may not want to reuse the
> > certificate across PeerConnection.
> > 
> > I think it'll be better to create a new certificate in createPeerConnection.
> 
> ok. Should I call both rtc::SSLIdentity::Generate() and
> rtc::RTCCertificate::Create() in createPeerConnection or SSLIdentity can be
> generated in JNI_OnLoad()?

Both in createPeerConnection.

[AlexG, 2015-08-25 23:56:02]

PTAL

https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/1/talk/app/webrtc/java/jni/peer...
talk/app/webrtc/java/jni/peerconnection_jni.cc:150: g_rtc_certificate =
rtc::RTCCertificate::Create(ssl_identity.Pass());
On 2015/08/25 23:15:26, AlexG wrote:
> On 2015/08/25 23:06:02, jiayl2 wrote:
> > If the app runs for days or months, the certificate may expire, assuming
this
> > method is called on per app launch?
> > 
> > Also, there is the privacy side that some app may not want to reuse the
> > certificate across PeerConnection.
> > 
> > I think it'll be better to create a new certificate in createPeerConnection.
> 
> ok. Should I call both rtc::SSLIdentity::Generate() and
> rtc::RTCCertificate::Create() in createPeerConnection or SSLIdentity can be
> generated in JNI_OnLoad()?

Done.

[jiayl2, 2015-08-26 00:12:23]

https://codereview.webrtc.org/1312293003/diff/20001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/20001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1346:
rtc::SSLIdentity::Generate(std::string(), rtc::KT_ECDSA));
Reuse string
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

[AlexG, 2015-08-26 00:37:04]

PTAL

https://codereview.webrtc.org/1312293003/diff/20001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/20001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1346:
rtc::SSLIdentity::Generate(std::string(), rtc::KT_ECDSA));
On 2015/08/26 00:12:23, jiayl2 wrote:
> Reuse string
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

Done.

[hbos, 2015-08-26 09:50:04]

https://codereview.webrtc.org/1312293003/diff/40001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1348: // Create ECDSA
certificate.
This should probably not be placed here without having to explicitly enable it.
This would otherwise enable it for everything using this Java API which we do
not want to do.

I suggest adding a #ifdef JNI_CREATEPEERCONNECTION_USE_ECDSA (or if you can
think of a better name) around this code until we have added proper support to
control the certificates from the Java layers. You can build with this flag if
you want to do testing in the mean time.

Alternatively you could have a boolean flag or key type enum on the
RTCConfiguration that you would set from the application layer, but that could
only be temporary since the Java layer need to resemble the WebRTC RFC.

https://codereview.webrtc.org/1312293003/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1356: LOG(LS_WARNING) << "Failed
to generate SSLIdentity.";
While failing to generate is (exceptionally?) rare, it can happen. When this
happens this will not abort. Is that on purpose?

The peer connection subsequently created will use the default key type
(currently RSA, EC some time in the future, but you should not expect it to use
a specific type if you do not tell it to do so).

If it is on purpose, please add a comment clarifying that the peer connection
will created with default key type (and perhaps update the log message as well).
Otherwise, crash or find a way to abort safely.

[AlexG, 2015-08-31 19:07:23]

PTAL

https://codereview.webrtc.org/1312293003/diff/40001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1348: // Create ECDSA
certificate.
On 2015/08/26 09:50:04, hbos wrote:
> This should probably not be placed here without having to explicitly enable
it.
> This would otherwise enable it for everything using this Java API which we do
> not want to do.
> 
> I suggest adding a #ifdef JNI_CREATEPEERCONNECTION_USE_ECDSA (or if you can
> think of a better name) around this code until we have added proper support to
> control the certificates from the Java layers. You can build with this flag if
> you want to do testing in the mean time.
> 
> Alternatively you could have a boolean flag or key type enum on the
> RTCConfiguration that you would set from the application layer, but that could
> only be temporary since the Java layer need to resemble the WebRTC RFC.

Done. Added Java enum to configure certificate type

https://codereview.webrtc.org/1312293003/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1356: LOG(LS_WARNING) << "Failed
to generate SSLIdentity.";
On 2015/08/26 09:50:04, hbos wrote:
> While failing to generate is (exceptionally?) rare, it can happen. When this
> happens this will not abort. Is that on purpose?
> 
> The peer connection subsequently created will use the default key type
> (currently RSA, EC some time in the future, but you should not expect it to
use
> a specific type if you do not tell it to do so).
> 
> If it is on purpose, please add a comment clarifying that the peer connection
> will created with default key type (and perhaps update the log message as
well).
> Otherwise, crash or find a way to abort safely.

Done. Yes, this is on purpose. I added comment and updated log message.

[AlexG, 2015-08-31 19:31:06]

The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-31 19:31:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312293003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312293003/80001

[jiayl2, 2015-08-31 19:47:42]

https://codereview.webrtc.org/1312293003/diff/80001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:130: static const char
kIdentityName[] = "WebRTC";
a minor issue, but can you move it to a header file shared with
dtlsidentitystore?

[AlexG, 2015-08-31 20:14:13]

https://codereview.webrtc.org/1312293003/diff/80001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1312293003/diff/80001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:130: static const char
kIdentityName[] = "WebRTC";
On 2015/08/31 19:47:42, jiayl2 wrote:
> a minor issue, but can you move it to a header file shared with
> dtlsidentitystore?

Done.

[jiayl2, 2015-08-31 20:39:06]

lgtm

[AlexG, 2015-08-31 21:32:03]

The CQ bit was checked by glaznev@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-31 21:32:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312293003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312293003/120001

[commit-bot: I haz the power, 2015-08-31 23:32:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-31 23:32:40]

Dry run: Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hbos, 2015-09-01 08:59:24]

lgtm.

Please add a "TODO(hbos,torbjorng):" above "enum KeyType..." in
sslidentity.h:110 saying something like "Don't change KT_DEFAULT without first
updating PeerConnectionFactory_nativeCreatePeerConnection's certificate
generation code."

[AlexG, 2015-09-01 17:26:51]

The CQ bit was checked by glaznev@webrtc.org

[AlexG, 2015-09-01 17:26:52]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org,
jiayl@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1312293003/#ps130001 (title:
"Add TODO item")

[commit-bot: I haz the power, 2015-09-01 17:27:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312293003/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312293003/130001

[commit-bot: I haz the power, 2015-09-01 18:31:32]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-09-01 18:31:40]

Patchset 8 (id:??) landed as
https://crrev.com/97579a4e122ad42592b5d7da9475e128da63a948
Cr-Commit-Position: refs/heads/master@{#9835}