Created the DtlsSrtpTransport.

Created the DtlsSrtpTransport.

The DtlsSrtpTransport takes DTLS responsibilities from BaseChannel.
DtlsSrtpTransport is responsible for exporting keys from DtlsTransport
and setting up the wrapped SrtpTransport.

BUG=webrtc:7013

[Zhi Huang, 2017-09-10 22:36:56]

Patchset #1 (id:1) has been deleted

[Zhi Huang, 2017-09-10 22:55:47]

Patchset #1 (id:20001) has been deleted

[Zhi Huang, 2017-09-10 22:55:52]

Patchset #1 (id:40001) has been deleted

[Zhi Huang, 2017-09-10 22:56:26]

Patchset #1 (id:60001) has been deleted

[Zhi Huang, 2017-09-10 22:58:00]

zhihuang@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Zhi Huang, 2017-09-10 22:58:01]

PTAL. Thanks.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/peerconnectioni...
File webrtc/pc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/peerconnectioni...
webrtc/pc/peerconnectioninterface_unittest.cc:2268: //
CreateAnswerAsLocalDescription();
This would be removed once https://codereview.webrtc.org/3011133002/ is landed.

[pthatcher, 2017-09-12 00:24:43]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2017-09-12 00:24:44]

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:944: if (!srtp_transport_) {
This could use an early return.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:946: // time.
If SrtpTransport is really just for SDES, I think we should call it
sdes_transport_, and call this method EnableSdes_n().

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:967: void BaseChannel::EnableDtlsSrtpTransport_n() {
This could just be called EnableDtlsSrtp_n().

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:968: if (!dtls_srtp_transport_) {
This could use an early return.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1078: // TODO(jbauch): Only update if encrypted extension
ids have changed.
Shouldn't this comment be moved down into the "else" clause?

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1094: SafeSetError("Failed to setup SRTP transport.",
error_desc);
You could just say "Failed to setup SRTP".

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
File webrtc/pc/dtlssrtptransport.cc (right):

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.cc:67: LOG(LS_INFO) << "Installing keys from
DTLS-SRTP on " << content_name_ << " "
I don't think it's worth passing in the content_name just for this comment.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
File webrtc/pc/dtlssrtptransport.h (right):

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:23: // This class exports the keying materials
from underneath DtlsTransport and
"from underneath DtlsTransport" => "from the DtlsTransport underneath"

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:28: const std::string& content_name);
Why is the content name needed?

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:30: bool SetupDtlsSrtp(bool rtcp);
Note: this is the kind of place removing support for non-muxed RTCP would be
nice.  RCS doesn't need DTLS support.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:34: cricket::DtlsTransportInternal*
dtls_transport);
The getters have two methods.  I think the setters should have two methods
(SetRtpDtlsTransport and SetRtcpDtlsTransport).

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:40: const std::vector<int>& extension_ids);
Instead of passing in a source, I think it would be better to pass in either
send or receiver extension IDs separately (either separate args or separate
methods).

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:99: bool active_ = false;
Can you leave a comment explaining when it becomes active?

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:102: // Not owned by this class.
Who has the responsibility of tracking the lifetime?  The caller of
SetDtlsTransport?  If so, that needs to be documented.

[Zhi Huang, 2017-09-27 00:44:25]

Patchset #2 (id:100001) has been deleted

[Zhi Huang, 2017-09-27 00:45:00]

Patchset #2 (id:120001) has been deleted

[Zhi Huang, 2017-09-27 00:46:33]

PTAL. Thanks.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:944: if (!srtp_transport_) {
On 2017/09/12 00:24:44, pthatcher wrote:
> This could use an early return.

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:946: // time.
On 2017/09/12 00:24:43, pthatcher wrote:
> If SrtpTransport is really just for SDES, I think we should call it
> sdes_transport_, and call this method EnableSdes_n().

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:967: void BaseChannel::EnableDtlsSrtpTransport_n() {
On 2017/09/12 00:24:43, pthatcher wrote:
> This could just be called EnableDtlsSrtp_n().

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:968: if (!dtls_srtp_transport_) {
On 2017/09/12 00:24:43, pthatcher wrote:
> This could use an early return.

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1078: // TODO(jbauch): Only update if encrypted extension
ids have changed.
On 2017/09/12 00:24:44, pthatcher wrote:
> Shouldn't this comment be moved down into the "else" clause?

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1094: SafeSetError("Failed to setup SRTP transport.",
error_desc);
On 2017/09/12 00:24:44, pthatcher wrote:
> You could just say "Failed to setup SRTP".

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
File webrtc/pc/dtlssrtptransport.cc (right):

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.cc:67: LOG(LS_INFO) << "Installing keys from
DTLS-SRTP on " << content_name_ << " "
On 2017/09/12 00:24:44, pthatcher wrote:
> I don't think it's worth passing in the content_name just for this comment.

Ok. I'll remove it then.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
File webrtc/pc/dtlssrtptransport.h (right):

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:23: // This class exports the keying materials
from underneath DtlsTransport and
On 2017/09/12 00:24:44, pthatcher wrote:
> "from underneath DtlsTransport" => "from the DtlsTransport underneath"

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:28: const std::string& content_name);
On 2017/09/12 00:24:44, pthatcher wrote:
> Why is the content name needed?

Removed.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:30: bool SetupDtlsSrtp(bool rtcp);
On 2017/09/12 00:24:44, pthatcher wrote:
> Note: this is the kind of place removing support for non-muxed RTCP would be
> nice.  RCS doesn't need DTLS support.

Acknowledged.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:34: cricket::DtlsTransportInternal*
dtls_transport);
On 2017/09/12 00:24:44, pthatcher wrote:
> The getters have two methods.  I think the setters should have two methods
> (SetRtpDtlsTransport and SetRtcpDtlsTransport).

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:40: const std::vector<int>& extension_ids);
On 2017/09/12 00:24:44, pthatcher wrote:
> Instead of passing in a source, I think it would be better to pass in either
> send or receiver extension IDs separately (either separate args or separate
> methods).

Agreed. The BaseChannel should take care of the ContentSource. I think we should
change the SrtpTransport too.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:99: bool active_ = false;
On 2017/09/12 00:24:44, pthatcher wrote:
> Can you leave a comment explaining when it becomes active?

Done.

https://codereview.webrtc.org/3012953002/diff/80001/webrtc/pc/dtlssrtptranspo...
webrtc/pc/dtlssrtptransport.h:102: // Not owned by this class.
On 2017/09/12 00:24:44, pthatcher wrote:
> Who has the responsibility of tracking the lifetime?  The caller of
> SetDtlsTransport?  If so, that needs to be documented.

The transport controller should own these.

[Taylor Brandstetter, 2017-09-27 23:54:48]

In retrospect, I think it would have been a lot easier to do this after (or in
combination with) the CL that moves responsibility to TransportController. Then
a lot of the "enabling"/"upgrading"/"switching" kind of code wouldn't be
necessary.

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.cc
File pc/channel.cc (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.cc#newcode995
pc/channel.cc:995: // certficate is ready and the |rtp_dtls_transport_| is
active. The
certificate

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.h
File pc/channel.h (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/channel.h#newcode384
pc/channel.h:384: // Create an SrtpTransport and wrap it in an
DtlsSrptTransport.
DtlsSrtpTransport

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc
File pc/dtlssrtptransport.cc (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc#...
pc/dtlssrtptransport.cc:142: void
DtlsSrtpTransport::SetSendEncryptedHeaderExtensionIds(
In a previous code review I mentioned that it may be useful to make an
RtpTransportAdapter helper class, similar to AsyncSocketAdapter. Now that we
have two classes that do this forwarding, it may now be a good time. Could you
at least add a TODO, if you agree?

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc#...
pc/dtlssrtptransport.cc:144: RTC_DCHECK(srtp_transport_);
nit: I don't know if adding these DCHECKS everywhere adds much; the only real
place I'd say it's necessary is the constructor, which doesn't have one.

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.cc#...
pc/dtlssrtptransport.cc:222: if (srtp_transport_) {
When will it be null?

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.h
File pc/dtlssrtptransport.h (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.h#n...
pc/dtlssrtptransport.h:59: void SetRtcpMuxEnabled(bool enable) override;
nit: I'd remove the newlines between all the override methods.

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport.h#n...
pc/dtlssrtptransport.h:108: bool active_ = false;
Is this ever different than srtp_transport_->IsActive()?

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport_uni...
File pc/dtlssrtptransport_unittest.cc (right):

https://codereview.webrtc.org/3012953002/diff/140001/pc/dtlssrtptransport_uni...
pc/dtlssrtptransport_unittest.cc:99: }
Should also test sending some packets; this doesn't actually verify that the
keying material was extracted correctly, just that both sides *think* they're
working.