Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(250)

Issue 1269843005: Added DtlsCertificate, a ref counted object owning an SSLIdentity (Closed)

Created:
5 years, 4 months ago by hbos
Modified:
5 years, 3 months ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

--- Made obsolete by other CLs, see webrtc:4927 --- Added DtlsCertificate, a ref counted object owning an SSLIdentity. The use of DtlsCertificate allows the same SSLIdentity to be reused by multiple peer connections. The old CreatePeerConnection signature still exists, but a new CreatePeerConnection signature is added that takes a certificate instead of a DtlsIdentityStoreInterface. This is for the latest WebRTC standard which says that you should optionally be able to generate certificates separately and supply them to CreatePeerConnection. A scoped_refptr<DtlsCertificate> is passed through these layers (just like DtlsIdentityStoreInterface*)... PeerConnectionFactory::CreatePeerConnection -> PeerConnection::Initialize -> WebRtcSession::Initialize -> WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory (Edit: Should make the certificate(s) a part of RTCConfiguration, not a stand alone parameter of CreatePeerConnection) These functions have been updated so that there is one version taking a store and one version taking a certificate. WebRtcSessionDescriptionFactory has been cleaned up a bit. The majority of the files changed are updates from SSLIdentity* to scoped_refptr<DtlsCertificate> (Patch Set 11 and onwards). BUG=webrtc:4902

Patch Set 1 #

Patch Set 2 : Addressed asan,lsan issues #

Patch Set 3 : CreatePeerConnection with new function signature #

Patch Set 4 : WebRtcSession's SSLIdentity* -> scoped_refptr<DtlsCertificate>, ownership and memsafety reasons #

Patch Set 5 : WebRtcSessionDescriptionFactory using DtlsIdentityStore/DtlsIdentityService when generating certifi… #

Patch Set 6 : webrtcsession_unittest cleanup #

Total comments: 13

Patch Set 7 : Using supplied DTLSIdentityServiceInterface* if certificate is null #

Patch Set 8 : Merge with master #

Patch Set 9 : webrtcsession unittest added to ensure when a cert is provided it is used #

Total comments: 9

Patch Set 10 : Addressed grunell's comments EXCEPT repl SSLIdentity* /w scoped_refptr<DtlsCertificate> #

Patch Set 11 : Replacing SSLIdentity* /w scoped_refptr<DtlsCertificate> in surrounding code #

Total comments: 3

Patch Set 12 : Cleanup #

Total comments: 6

Patch Set 13 : Addressed tommi's comments (const ref all over the place) #

Patch Set 14 : Merge w master AFTER landing of 1176383004; service -> store. #

Patch Set 15 : Trying to get iOS to compile #

Total comments: 50

Patch Set 16 : Addressed most of grunell's comments #

Patch Set 17 : Merge with master #

Patch Set 18 : Updated webrtcsession_unittest: Parameterized tests #

Total comments: 1

Patch Set 19 : Merge with master #

Total comments: 14
Unified diffs Side-by-side diffs Delta from patch set Stats (+980 lines, -423 lines) Patch
A + talk/app/webrtc/dtlscertificate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +22 lines, -17 lines 1 comment Download
A + talk/app/webrtc/dtlscertificate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +16 lines, -5 lines 0 comments Download
M talk/app/webrtc/java/jni/peerconnection_jni.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -1 line 1 comment Download
M talk/app/webrtc/objc/RTCPeerConnection.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +12 lines, -3 lines 0 comments Download
M talk/app/webrtc/peerconnection.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +22 lines, -5 lines 1 comment Download
M talk/app/webrtc/peerconnection.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +44 lines, -9 lines 3 comments Download
M talk/app/webrtc/peerconnectionfactory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +11 lines, -0 lines 1 comment Download
M talk/app/webrtc/peerconnectionfactory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +45 lines, -7 lines 0 comments Download
M talk/app/webrtc/peerconnectionfactoryproxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +7 lines, -0 lines 0 comments Download
M talk/app/webrtc/peerconnectioninterface.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +15 lines, -0 lines 0 comments Download
M talk/app/webrtc/proxy.h View 1 2 3 4 5 6 3 chunks +42 lines, -0 lines 1 comment Download
M talk/app/webrtc/statscollector.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -3 lines 1 comment Download
M talk/app/webrtc/statscollector_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -2 lines 0 comments Download
M talk/app/webrtc/test/fakedtlsidentitystore.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +29 lines, -0 lines 0 comments Download
M talk/app/webrtc/webrtcsession.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +21 lines, -3 lines 1 comment Download
M talk/app/webrtc/webrtcsession.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +89 lines, -24 lines 1 comment Download
M talk/app/webrtc/webrtcsession_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 27 chunks +118 lines, -68 lines 0 comments Download
M talk/app/webrtc/webrtcsessiondescriptionfactory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +50 lines, -13 lines 0 comments Download
M talk/app/webrtc/webrtcsessiondescriptionfactory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 12 chunks +108 lines, -38 lines 1 comment Download
M talk/libjingle.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +11 lines, -2 lines 0 comments Download
M talk/libjingle_tests.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M talk/session/media/channel_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +12 lines, -8 lines 0 comments Download
M talk/session/media/mediasession_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +13 lines, -5 lines 0 comments Download
M webrtc/base/base.gyp View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/base/nssstreamadapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +6 lines, -5 lines 0 comments Download
M webrtc/base/opensslstreamadapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +4 lines, -3 lines 0 comments Download
M webrtc/base/opensslstreamadapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +8 lines, -5 lines 0 comments Download
M webrtc/base/ssladapter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +5 lines, -3 lines 0 comments Download
M webrtc/base/sslstreamadapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +6 lines, -7 lines 0 comments Download
M webrtc/base/sslstreamadapter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +43 lines, -36 lines 1 comment Download
M webrtc/base/sslstreamadapterhelper.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -3 lines 0 comments Download
M webrtc/base/sslstreamadapterhelper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -3 lines 0 comments Download
M webrtc/p2p/base/dtlstransport.h View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +14 lines, -12 lines 1 comment Download
M webrtc/p2p/base/dtlstransportchannel.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -3 lines 0 comments Download
M webrtc/p2p/base/dtlstransportchannel.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +11 lines, -11 lines 0 comments Download
M webrtc/p2p/base/dtlstransportchannel_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 9 chunks +36 lines, -26 lines 0 comments Download
M webrtc/p2p/base/fakesession.h View 1 2 3 4 5 6 7 8 9 10 11 12 12 chunks +32 lines, -26 lines 0 comments Download
M webrtc/p2p/base/p2ptransportchannel.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -2 lines 0 comments Download
M webrtc/p2p/base/rawtransportchannel.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -2 lines 0 comments Download
M webrtc/p2p/base/session.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +9 lines, -5 lines 0 comments Download
M webrtc/p2p/base/session.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +11 lines, -11 lines 0 comments Download
M webrtc/p2p/base/transport.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +10 lines, -5 lines 0 comments Download
M webrtc/p2p/base/transport.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +7 lines, -4 lines 0 comments Download
M webrtc/p2p/base/transportchannel.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +4 lines, -3 lines 0 comments Download
M webrtc/p2p/base/transportchannelimpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -6 lines 0 comments Download
M webrtc/p2p/base/transportchannelproxy.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M webrtc/p2p/base/transportchannelproxy.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -3 lines 0 comments Download
M webrtc/p2p/base/transportdescriptionfactory.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +12 lines, -5 lines 0 comments Download
M webrtc/p2p/base/transportdescriptionfactory.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +5 lines, -4 lines 0 comments Download
M webrtc/p2p/base/transportdescriptionfactory_unittest.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +23 lines, -16 lines 0 comments Download

Messages

Total messages: 28 (9 generated)
hbos
PTAL tommi and henrikg. https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/dtlscertificate.cc File talk/app/webrtc/dtlscertificate.cc (right): https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/dtlscertificate.cc#newcode38 talk/app/webrtc/dtlscertificate.cc:38: return new rtc::RefCountedObject<DtlsCertificate>(identity.release(), 0.0); In ...
5 years, 4 months ago (2015-08-04 12:50:21 UTC) #5
Henrik Grunell WebRTC
First round - more high level reviewing. Please file a bug. Link to the spec ...
5 years, 4 months ago (2015-08-05 07:41:20 UTC) #6
hbos
After talking with henrikg I realized we don't want to remove the passing around of ...
5 years, 4 months ago (2015-08-05 08:13:00 UTC) #7
hbos
PTAL henrikg, tommi. https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconnectioninterface.h File talk/app/webrtc/peerconnectioninterface.h (right): https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconnectioninterface.h#newcode553 talk/app/webrtc/peerconnectioninterface.h:553: // new function signature. On 2015/08/05 ...
5 years, 4 months ago (2015-08-06 12:11:34 UTC) #8
Henrik Grunell WebRTC
Another round. https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconnectioninterface.h File talk/app/webrtc/peerconnectioninterface.h (right): https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconnectioninterface.h#newcode553 talk/app/webrtc/peerconnectioninterface.h:553: // new function signature. On 2015/08/06 12:11:34, ...
5 years, 4 months ago (2015-08-06 14:06:52 UTC) #9
hbos
PTAL grunell. Passing scoped_refptr<DtlsCertificate> around instead of SSLIdentity* wound up affecting a lot of files ...
5 years, 4 months ago (2015-08-10 15:10:18 UTC) #13
tommi (sloooow) - chröme
https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcsessiondescriptionfactory.cc File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcsessiondescriptionfactory.cc#newcode512 talk/app/webrtc/webrtcsessiondescriptionfactory.cc:512: rtc::scoped_refptr<DtlsCertificate> certificate) { this should be passed by const ...
5 years, 4 months ago (2015-08-10 16:10:16 UTC) #15
tommi (sloooow) - chröme
https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/session.h File webrtc/p2p/base/session.h (right): https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/session.h#newcode320 webrtc/p2p/base/session.h:320: rtc::scoped_refptr<webrtc::DtlsCertificate> certificate() { also return by const ref
5 years, 4 months ago (2015-08-10 16:10:54 UTC) #16
hbos
https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcsessiondescriptionfactory.cc File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right): https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcsessiondescriptionfactory.cc#newcode512 talk/app/webrtc/webrtcsessiondescriptionfactory.cc:512: rtc::scoped_refptr<DtlsCertificate> certificate) { On 2015/08/10 16:10:16, tommi wrote: > ...
5 years, 4 months ago (2015-08-12 08:55:15 UTC) #17
Henrik Grunell WebRTC
Another round. Generally, should we add new tests? And/or should we test with "pre-generated" cert ...
5 years, 4 months ago (2015-08-12 14:46:30 UTC) #18
hbos
Addressed most of grunell's comments, but have yet to look at if more webrtcsession_unittest test ...
5 years, 4 months ago (2015-08-14 14:09:39 UTC) #20
hbos
PTAL grunell. Many unittests in webrtcsession_unittest.cc have been parameterized. Hmh, perhaps not all of the ...
5 years, 4 months ago (2015-08-18 08:56:35 UTC) #21
Henrik Grunell WebRTC
Looked at your comments and fixes for my last round. Will take another review round ...
5 years, 4 months ago (2015-08-18 14:25:11 UTC) #22
tommi (sloooow) - chröme
is there a way to break this cl up into smaller more easily reviewable changelists? ...
5 years, 4 months ago (2015-08-18 14:49:36 UTC) #23
Henrik Grunell WebRTC
On 2015/08/18 14:49:36, tommi wrote: > is there a way to break this cl up ...
5 years, 4 months ago (2015-08-18 19:34:25 UTC) #24
hbos
On 2015/08/18 19:34:25, Henrik Grunell (webrtc) wrote: > On 2015/08/18 14:49:36, tommi wrote: > > ...
5 years, 4 months ago (2015-08-19 08:43:57 UTC) #25
Henrik Grunell WebRTC
On 2015/08/19 08:43:57, hbos wrote: > On 2015/08/18 19:34:25, Henrik Grunell (webrtc) wrote: > > ...
5 years, 4 months ago (2015-08-19 09:04:27 UTC) #26
tommi (sloooow) - chröme
should we close this cl then?
5 years, 3 months ago (2015-08-31 17:19:42 UTC) #27
hbos_chromium
5 years, 3 months ago (2015-09-01 08:07:26 UTC) #28
On 2015/08/31 17:19:42, tommi wrote:
> should we close this cl then?

Yes! Not sure why I kept it open after editing it as obsolete, was going to
close it shortly after but forgot.

Powered by Google App Engine
This is Rietveld 408576698