Create the SrtpTransportInterface.

Create the SrtpTransportInterface.

Create the SrtpTransportInterface, a subclass of RtpTransportInterface, which
allows the user to set the send and receive keys. The functionalities are
implemented inside the RtpTransportAdapters on top of BaseChannel.

BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2714813004
Cr-Commit-Position: refs/heads/master@{#17023}
Committed: https://chromium.googlesource.com/external/webrtc/+/d3501adf17fda4ad270faee89b6fe4f4a496b147

[Zhi Huang, 2017-02-24 00:57:12]

Patchset #1 (id:1) has been deleted

[Zhi Huang, 2017-02-24 00:57:25]

Patchset #1 (id:20001) has been deleted

[Taylor Brandstetter, 2017-02-24 18:57:56]

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

[Taylor Brandstetter, 2017-02-24 18:57:58]

Did you plan to also add an "srtptransport_unittests"?

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
File webrtc/api/ortc/srtptransportinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
webrtc/api/ortc/srtptransportinterface.h:20: // The subclass of the RtpTransport
which uses the SRTP. The Keying information
nit: Don't need "the" before "SRTP"

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
webrtc/api/ortc/srtptransportinterface.h:21: // is explicitly passed in from the
application.
May also want to note that this is similar in role to Edge's RTCSrtpSdesTranport
(https://msdn.microsoft.com/en-us/library/mt502527(v=vs.85).aspx). And describe
how it could be used to implement SDES. Namely, after applying an answer, the
negotiated keying information in the offer and answer would be set.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
webrtc/api/ortc/srtptransportinterface.h:26: // Set the SRTP keying material for
sending stream from the applciation.
nit: "application" misspelled. 

It may also be better to use the phrases "outgoing RTP streams" and "incoming
RTP streams" instead of "sending stream" and "receiving stream".

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
File webrtc/ortc/ortcfactory_integrationtest.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:268:
TEST_F(OrtcFactoryIntegrationTest, OneWayAudioRtpSenderAndReceiverWithSrtp) {
Since these are high level tests, I'd suggest making all but one of them use
SRTP. So, maybe have "BasicTwoWayAudioVideoWithoutSrtp", for example.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:291:
EXPECT_TRUE((srtp_transports.first->SetSrtpSendKey(kTestCryptoParams1)).ok());
Since the individual tests don't care about what keys are used, this could go
into the helper function. "CreateSrtpTransportPairAndSetKeys".

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:293:
(srtp_transports.first->SetSrtpReceiveKey(kTestCryptoParams1)).ok());
Should set different send and receive keys

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:619: 
Some other end-to-end SRTP tests that would be nice to have:

* SrtpTransport on one side, normal RtpTransport on the other side. Make sure
they don't render any frames after waiting for some period (1 second?).

* Same thing, but two SrtpTransports that have mismatching keys.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.cc:187: "Re-set the SRTP send key is currently
unsupported.");
nit: "Re-setting", or maybe "Setting the SRTP send key twice" to avoid confusion
with the word "reset".

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.h:27: // Implementation of RtpTransportInterface
to be used with RtpSenderAdapter,
This comment should be updated. It should also mention that this class is used
to implement both a secure and insecure RTP transport.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransportc...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransportc...
webrtc/ortc/rtptransportcontrolleradapter.cc:582: if (inner_transport ==
inner_audio_transport_) {
What if both "inner_audio_transport_" and "inner_video_transport_" are unset,
because no RtpSender or RtpReceiver is created yet? 

I'd suggest doing the same thing as is done with RtcpParameters, and cache the
keys in SrtpTransport. Then in CreateVoiceChannel (for example), do this sort of
thing:

If (rtp_transport->is_srtp()) {
  if (!rtp_transport->keys_set()) {
    LOG_AND_RETURN_ERROR(...)
  }
  local_audio_description_.set_cryptos(rtp_transport->get_recv_key());
  remote_audio_description_.set_cryptos(rtp_transport->get_send_key());
}

Then, since changing the key isn't supported, this is the only place where the
keys would need to be set, and RtpTransportControllerAdapter::SetSrtpSendKey
wouldn't be needed.

[Zhi Huang, 2017-02-26 19:34:22]

Patchset #2 (id:60001) has been deleted

[Zhi Huang, 2017-02-27 05:10:33]

Patchset #4 (id:120001) has been deleted

[Zhi Huang, 2017-02-27 05:11:39]

Patchset #3 (id:100001) has been deleted

[Zhi Huang, 2017-02-27 05:12:26]

Patchset #2 (id:80001) has been deleted

[Zhi Huang, 2017-02-27 05:16:00]

Please take a look. Thanks.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
File webrtc/api/ortc/srtptransportinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
webrtc/api/ortc/srtptransportinterface.h:20: // The subclass of the RtpTransport
which uses the SRTP. The Keying information
On 2017/02/24 18:57:57, Taylor Brandstetter wrote:
> nit: Don't need "the" before "SRTP"

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
webrtc/api/ortc/srtptransportinterface.h:21: // is explicitly passed in from the
application.
On 2017/02/24 18:57:57, Taylor Brandstetter wrote:
> May also want to note that this is similar in role to Edge's
RTCSrtpSdesTranport
> (https://msdn.microsoft.com/en-us/library/mt502527(v=vs.85).aspx). And
describe
> how it could be used to implement SDES. Namely, after applying an answer, the
> negotiated keying information in the offer and answer would be set.

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/api/ortc/srtptrans...
webrtc/api/ortc/srtptransportinterface.h:26: // Set the SRTP keying material for
sending stream from the applciation.
On 2017/02/24 18:57:57, Taylor Brandstetter wrote:
> nit: "application" misspelled. 
> 
> It may also be better to use the phrases "outgoing RTP streams" and "incoming
> RTP streams" instead of "sending stream" and "receiving stream".

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
File webrtc/ortc/ortcfactory_integrationtest.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:268:
TEST_F(OrtcFactoryIntegrationTest, OneWayAudioRtpSenderAndReceiverWithSrtp) {
On 2017/02/24 18:57:57, Taylor Brandstetter wrote:
> Since these are high level tests, I'd suggest making all but one of them use
> SRTP. So, maybe have "BasicTwoWayAudioVideoWithoutSrtp", for example.

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:291:
EXPECT_TRUE((srtp_transports.first->SetSrtpSendKey(kTestCryptoParams1)).ok());
On 2017/02/24 18:57:58, Taylor Brandstetter wrote:
> Since the individual tests don't care about what keys are used, this could go
> into the helper function. "CreateSrtpTransportPairAndSetKeys".

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:293:
(srtp_transports.first->SetSrtpReceiveKey(kTestCryptoParams1)).ok());
On 2017/02/24 18:57:58, Taylor Brandstetter wrote:
> Should set different send and receive keys

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:619: 
On 2017/02/24 18:57:57, Taylor Brandstetter wrote:
> Some other end-to-end SRTP tests that would be nice to have:
> 
> * SrtpTransport on one side, normal RtpTransport on the other side. Make sure
> they don't render any frames after waiting for some period (1 second?).
> 
> * Same thing, but two SrtpTransports that have mismatching keys.

I'm not sure about the expected behavior when on side uses SRTP and the other
side uses RTP. When RTP send packet to SRTP, the packet wouldn't be decrypted.
While if SRTP sends a packet to RTP, is it expected to tell that this is an
encrypted packet and the receiver should be created on SRTP transport? I think
this is something that should be determined by the upper layer application when
parsing SDP or doing other forms of signaling. 

I'll expect that RTP side can rendered some frames while SRTP side cannot.
Please correct me if I missed something.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.cc:187: "Re-set the SRTP send key is currently
unsupported.");
On 2017/02/24 18:57:58, Taylor Brandstetter wrote:
> nit: "Re-setting", or maybe "Setting the SRTP send key twice" to avoid
confusion
> with the word "reset".

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransporta...
webrtc/ortc/rtptransportadapter.h:27: // Implementation of RtpTransportInterface
to be used with RtpSenderAdapter,
On 2017/02/24 18:57:58, Taylor Brandstetter wrote:
> This comment should be updated. It should also mention that this class is used
> to implement both a secure and insecure RTP transport.

Done.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransportc...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransportc...
webrtc/ortc/rtptransportcontrolleradapter.cc:582: if (inner_transport ==
inner_audio_transport_) {
On 2017/02/24 18:57:58, Taylor Brandstetter wrote:
> What if both "inner_audio_transport_" and "inner_video_transport_" are unset,
> because no RtpSender or RtpReceiver is created yet? 
> 
> I'd suggest doing the same thing as is done with RtcpParameters, and cache the
> keys in SrtpTransport. Then in CreateVoiceChannel (for example), do this sort
of
> thing:
> 
> If (rtp_transport->is_srtp()) {
>   if (!rtp_transport->keys_set()) {
>     LOG_AND_RETURN_ERROR(...)
>   }
>   local_audio_description_.set_cryptos(rtp_transport->get_recv_key());
>   remote_audio_description_.set_cryptos(rtp_transport->get_send_key());
> }
> 
> Then, since changing the key isn't supported, this is the only place where the
> keys would need to be set, and RtpTransportControllerAdapter::SetSrtpSendKey
> wouldn't be needed.

So this means that the sender cannot be created before setting the keys.
What I was thinking is that the senders can be created before setting the keys
but the keys should be set before sending or receiving anything.

It seems that your suggestion would make it cleaner.

[Taylor Brandstetter, 2017-02-27 21:48:51]

FYI, the big ORTC CL has landed, so it should be possible to merge with it now.
Hopefully it won't be too messy; if it is, I can help

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
File webrtc/ortc/ortcfactory_integrationtest.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/ortcfactory_i...
webrtc/ortc/ortcfactory_integrationtest.cc:619: 
On 2017/02/27 05:16:00, Zhi Huang wrote:
> On 2017/02/24 18:57:57, Taylor Brandstetter wrote:
> > Some other end-to-end SRTP tests that would be nice to have:
> > 
> > * SrtpTransport on one side, normal RtpTransport on the other side. Make
sure
> > they don't render any frames after waiting for some period (1 second?).
> > 
> > * Same thing, but two SrtpTransports that have mismatching keys.
> 
> I'm not sure about the expected behavior when on side uses SRTP and the other
> side uses RTP. When RTP send packet to SRTP, the packet wouldn't be decrypted.
> While if SRTP sends a packet to RTP, is it expected to tell that this is an
> encrypted packet and the receiver should be created on SRTP transport? I think
> this is something that should be determined by the upper layer application
when
> parsing SDP or doing other forms of signaling. 
> 
> I'll expect that RTP side can rendered some frames while SRTP side cannot.
> Please correct me if I missed something.
> 

I'd expect the RTP side would try to process the frames but get garbage and
fail. Can you see if this works as expected? It would just be nice to have some
end-to-end test that ensures that media is actually encrypted by an
SrtpTransport.

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransportc...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/40001/webrtc/ortc/rtptransportc...
webrtc/ortc/rtptransportcontrolleradapter.cc:582: if (inner_transport ==
inner_audio_transport_) {
On 2017/02/27 05:16:00, Zhi Huang wrote:
> On 2017/02/24 18:57:58, Taylor Brandstetter wrote:
> > What if both "inner_audio_transport_" and "inner_video_transport_" are
unset,
> > because no RtpSender or RtpReceiver is created yet? 
> > 
> > I'd suggest doing the same thing as is done with RtcpParameters, and cache
the
> > keys in SrtpTransport. Then in CreateVoiceChannel (for example), do this
sort
> of
> > thing:
> > 
> > If (rtp_transport->is_srtp()) {
> >   if (!rtp_transport->keys_set()) {
> >     LOG_AND_RETURN_ERROR(...)
> >   }
> >   local_audio_description_.set_cryptos(rtp_transport->get_recv_key());
> >   remote_audio_description_.set_cryptos(rtp_transport->get_send_key());
> > }
> > 
> > Then, since changing the key isn't supported, this is the only place where
the
> > keys would need to be set, and RtpTransportControllerAdapter::SetSrtpSendKey
> > wouldn't be needed.
> 
> So this means that the sender cannot be created before setting the keys.
> What I was thinking is that the senders can be created before setting the keys
> but the keys should be set before sending or receiving anything.
> 
> It seems that your suggestion would make it cleaner.

It doesn't *have* to work that way, but it seems simpler. And there shouldn't be
the need to create a sender before setting keys, since it wouldn't be able to
send anything anyway.

We could fix this limitation in the future, and for now just document it in the
header file, like other limitations.

[Zhi Huang, 2017-03-01 01:55:35]

Patchset #3 (id:160001) has been deleted

[Zhi Huang, 2017-03-01 02:09:53]

Patchset #3 (id:180001) has been deleted

[Zhi Huang, 2017-03-01 02:17:12]

Please take another look. Thanks.

[Zhi Huang, 2017-03-01 02:17:13]

Please take another look. Thanks.

[Taylor Brandstetter, 2017-03-01 19:18:09]

Good job with this CL; I think all I have are nits and minor comments about
tests

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
File webrtc/api/ortc/srtptransportinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:23: // After applying the answer, the
negotiated keying information from the offer
I'd say "If using SDP and SDES (RFC4568) for signaling, then after an applying
an answer..."

Just to make it clear that any signaling protocol can be used on top of this
API.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:33: // Set the SRTP keying material for
outgoing stream from the application.
nit: "streams" or "stream(s)" instead of "stream", since there may both an audio
and video stream using the same SRTP session. Also, saying "from the
application" may not be necessary.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:34: virtual RTCError
SetSrtpSendKey(const cricket::CryptoParams& params) = 0;
We should document the current limitations of the implementation. Which, as I
understand it, are:

* Send and receive keys must use the same crypto suite
* Can't change keys after initially set.
* Must set the keys before creating a sender/receiver using the transport.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
File webrtc/ortc/ortcfactory_integrationtest.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:29: const int kReceivingTimeout =
1000;  // 1 second.
nit: This isn't really a "timeout", since the test waits for this time
unconditionally. Maybe "kReceivingDuration" would be better?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:170: return {std::move(transport1),
std::move(transport2)};
Is it possible to return "std::move(srtp_transports)" instead of making a new
pair?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:217: // configurations.
Could you add a comment saying: "If |expect_success| is true, waits for
kDefaultTimeout for kDefaultNumFrames frames to be received by all RtpReceivers.
If |expect_success| is false, simply waits for kReceivingTimeout, and stores the
number of received frames in |received_audio_frame1_| etc."

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:218: void
BasicsTwoWayRtpSendersAndReceiversTest(UdpTransportPair udp_transports,
nit: "Basic" instead of "Basics"

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:218: void
BasicsTwoWayRtpSendersAndReceiversTest(UdpTransportPair udp_transports,
It doesn't look like the udp_transports parameter is used.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:224: rendered_video_frame2_ = 0;
nit: "frames", plural?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:460: true /*expect_success*/);
nit: Our style (at least, from what I've learned from Peter) is to do:

bool expect_success = true;
BasicTwoWayRtpSendersAndReceiversTest(std::move(udp_transports),
                                      std::move(rtp_transports),
                                      expect_success);

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:472:
TEST_F(OrtcFactoryIntegrationTest, FullTwoWayAudioVideoRtpSendersAndReceivers) {
Could we change this test to use SRTP?

It would also be useful to have at least one test that uses different keys for
audio and video.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:592:
TEST_F(OrtcFactoryIntegrationTest,
Could these new tests be added above the "FullTwoWay" test? That way the tests
get gradually more complex as you move further in the file.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:602: // Tests that the packet cannot
be decoded if the keys are mismatched.
nit: "Tests that packets..."

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:610: // No frame is expected to be
decoded.
nit: "No frames are..."

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:637: // The SRTP side is not expected
to decode any audio and video frames.
nit: "audio or video"

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:639: // possible that the encrypted
audio frames can be accidentally decoded.
Could you add: "which is why received_audio_frames1_ is not validated", and a
TODO to fix this?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.cc:189: }
I think it would make sense to check that the crypto suites match, so that an
UNSUPPORTED_OPERATION can be returned here, before trying to apply the
descriptions to BaseChannel.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:74: // receiver should be created.
nit: "can be created" instead of "should be"?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:76: bool key_set() { return have_send_key_ &&
have_receive_key_; }
nit: "keys_set"?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:105: bool have_receive_key_;
nit: have_send_key_ and have_receive_key_ can default to false here

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:930: RTCError
RtpTransportControllerAdapter::MaybeSetCryptosForAudio(
nit: Instead of two separate methods, there could be one method that takes a
pointer to two MediaContentDescriptions

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
File webrtc/ortc/srtptransport_unittests.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:1: /*
nit: File should be named "_unittest.cc" (singular), to match existing naming.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:69: TEST_F(SrtpTransportTest,
SetSrtpSendAndReceiveKeyForSender) {
Could you split this into two tests; one for setting the keys once (success
case) and one for setting them twice (error case)?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:92: EXPECT_FALSE(sender_result.ok());
nit: Verify that the error is "UNSUPPORTED_PARAMETER"?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:96: EXPECT_FALSE(sender_result.ok());
nit: Add comment saying something along the lines of "Ensure that after the keys
*are* set, a sender can be created, despite the previous errors."

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:121: cricket::MEDIA_TYPE_VIDEO));
This is smart! I should have done this with some of the other tests I wrote.

[Zhi Huang, 2017-03-02 23:33:59]

Patchset #4 (id:220001) has been deleted

[Zhi Huang, 2017-03-02 23:35:40]

Please take a look. Thanks.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
File webrtc/api/ortc/srtptransportinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:23: // After applying the answer, the
negotiated keying information from the offer
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> I'd say "If using SDP and SDES (RFC4568) for signaling, then after an applying
> an answer..."
> 
> Just to make it clear that any signaling protocol can be used on top of this
> API.

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:33: // Set the SRTP keying material for
outgoing stream from the application.
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> nit: "streams" or "stream(s)" instead of "stream", since there may both an
audio
> and video stream using the same SRTP session. Also, saying "from the
> application" may not be necessary.

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:34: virtual RTCError
SetSrtpSendKey(const cricket::CryptoParams& params) = 0;
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> We should document the current limitations of the implementation. Which, as I
> understand it, are:
> 
> * Send and receive keys must use the same crypto suite
> * Can't change keys after initially set.
> * Must set the keys before creating a sender/receiver using the transport.

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
File webrtc/ortc/ortcfactory_integrationtest.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:29: const int kReceivingTimeout =
1000;  // 1 second.
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> nit: This isn't really a "timeout", since the test waits for this time
> unconditionally. Maybe "kReceivingDuration" would be better?

Yes, this would be better.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:170: return {std::move(transport1),
std::move(transport2)};
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> Is it possible to return "std::move(srtp_transports)" instead of making a new
> pair?

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:217: // configurations.
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> Could you add a comment saying: "If |expect_success| is true, waits for
> kDefaultTimeout for kDefaultNumFrames frames to be received by all
RtpReceivers.
> If |expect_success| is false, simply waits for kReceivingTimeout, and stores
the
> number of received frames in |received_audio_frame1_| etc."

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:218: void
BasicsTwoWayRtpSendersAndReceiversTest(UdpTransportPair udp_transports,
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> nit: "Basic" instead of "Basics"

Oh, I'm so careless.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:224: rendered_video_frame2_ = 0;
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> nit: "frames", plural?

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:460: true /*expect_success*/);
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> nit: Our style (at least, from what I've learned from Peter) is to do:
> 
> bool expect_success = true;
> BasicTwoWayRtpSendersAndReceiversTest(std::move(udp_transports),
>                                       std::move(rtp_transports),
>                                       expect_success);

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:472:
TEST_F(OrtcFactoryIntegrationTest, FullTwoWayAudioVideoRtpSendersAndReceivers) {
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> Could we change this test to use SRTP?
> 
> It would also be useful to have at least one test that uses different keys for
> audio and video.

How about setting different keys in this test since it is expected to be the
most complex one?

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:592:
TEST_F(OrtcFactoryIntegrationTest,
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> Could these new tests be added above the "FullTwoWay" test? That way the tests
> get gradually more complex as you move further in the file.

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:602: // Tests that the packet cannot
be decoded if the keys are mismatched.
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> nit: "Tests that packets..."

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:610: // No frame is expected to be
decoded.
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> nit: "No frames are..."

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:637: // The SRTP side is not expected
to decode any audio and video frames.
On 2017/03/01 19:18:07, Taylor Brandstetter wrote:
> nit: "audio or video"

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:639: // possible that the encrypted
audio frames can be accidentally decoded.
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> Could you add: "which is why received_audio_frames1_ is not validated", and a
> TODO to fix this?

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.cc:189: }
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> I think it would make sense to check that the crypto suites match, so that an
> UNSUPPORTED_OPERATION can be returned here, before trying to apply the
> descriptions to BaseChannel.

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:74: // receiver should be created.
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> nit: "can be created" instead of "should be"?

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:76: bool key_set() { return have_send_key_ &&
have_receive_key_; }
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> nit: "keys_set"?

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:105: bool have_receive_key_;
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> nit: have_send_key_ and have_receive_key_ can default to false here

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportcontrolleradapter.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportcontrolleradapter.cc:930: RTCError
RtpTransportControllerAdapter::MaybeSetCryptosForAudio(
On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> nit: Instead of two separate methods, there could be one method that takes a
> pointer to two MediaContentDescriptions

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
File webrtc/ortc/srtptransport_unittests.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:1: /*
On 2017/03/01 19:18:09, Taylor Brandstetter wrote:
> nit: File should be named "_unittest.cc" (singular), to match existing naming.

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:69: TEST_F(SrtpTransportTest,
SetSrtpSendAndReceiveKeyForSender) {
On 2017/03/01 19:18:09, Taylor Brandstetter wrote:
> Could you split this into two tests; one for setting the keys once (success
> case) and one for setting them twice (error case)?

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:92: EXPECT_FALSE(sender_result.ok());
On 2017/03/01 19:18:09, Taylor Brandstetter wrote:
> nit: Verify that the error is "UNSUPPORTED_PARAMETER"?

Done.

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittests.cc:96: EXPECT_FALSE(sender_result.ok());
On 2017/03/01 19:18:09, Taylor Brandstetter wrote:
> nit: Add comment saying something along the lines of "Ensure that after the
keys
> *are* set, a sender can be created, despite the previous errors."

Done.

[Taylor Brandstetter, 2017-03-02 23:51:10]

lgtm with a couple remaining nits

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
File webrtc/ortc/ortcfactory_integrationtest.cc (right):

https://codereview.webrtc.org/2714813004/diff/200001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:472:
TEST_F(OrtcFactoryIntegrationTest, FullTwoWayAudioVideoRtpSendersAndReceivers) {
On 2017/03/02 23:35:39, Zhi Huang wrote:
> On 2017/03/01 19:18:08, Taylor Brandstetter wrote:
> > Could we change this test to use SRTP?
> > 
> > It would also be useful to have at least one test that uses different keys
for
> > audio and video.
> 
> How about setting different keys in this test since it is expected to be the
> most complex one?

Sounds good.

https://codereview.webrtc.org/2714813004/diff/240001/webrtc/ortc/ortcfactory_...
File webrtc/ortc/ortcfactory_integrationtest.cc (right):

https://codereview.webrtc.org/2714813004/diff/240001/webrtc/ortc/ortcfactory_...
webrtc/ortc/ortcfactory_integrationtest.cc:537:
TEST_F(OrtcFactoryIntegrationTest, FullTwoWayAudioVideoRtpSendersAndReceivers) {
nit: For consistency with the names of the above tests, put "Srtp" in the name?

https://codereview.webrtc.org/2714813004/diff/240001/webrtc/ortc/srtptranspor...
File webrtc/ortc/srtptransport_unittest.cc (right):

https://codereview.webrtc.org/2714813004/diff/240001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittest.cc:25: static const char kTestKeyParams3[] =
nit: Could these constants have names that make it more clear that they use
different crypto suites? For example, "kTestSha1KeyParams1" and
"kTestGcmKeyParams".

https://codereview.webrtc.org/2714813004/diff/240001/webrtc/ortc/srtptranspor...
webrtc/ortc/srtptransport_unittest.cc:90:
EXPECT_EQ(srtp_transport_->SetSrtpSendKey(kTestCryptoParams2).type(),
nit: The EXPECT_EQ macro should have the expected value first, then the actual
value

[Taylor Brandstetter, 2017-03-02 23:51:26]

Description was changed from

==========
Create the SrtpTransportInterface.

Create the SrtpTransportInterface, a subclass of RtpTransportInterface, which
allows the user to set the send nad receive keys. The functionalities are
implemented inside the RtpTransportAdapters on top of BaseChannel.

BUG=webrtc:7013
==========

to

==========
Create the SrtpTransportInterface.

Create the SrtpTransportInterface, a subclass of RtpTransportInterface, which
allows the user to set the send and receive keys. The functionalities are
implemented inside the RtpTransportAdapters on top of BaseChannel.

BUG=webrtc:7013
==========

[Zhi Huang, 2017-03-03 00:33:44]

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

[commit-bot: I haz the power, 2017-03-03 00:33:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-03 00:38:45]

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

[commit-bot: I haz the power, 2017-03-03 00:38:46]

Dry run: Try jobs failed on following builders:
  android_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23466)

[Zhi Huang, 2017-03-03 01:36:43]

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

[Zhi Huang, 2017-03-03 01:36:46]

The CQ bit was unchecked by zhihuang@webrtc.org

[Zhi Huang, 2017-03-03 01:43:02]

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

[commit-bot: I haz the power, 2017-03-03 01:43:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-03 02:08:10]

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

[commit-bot: I haz the power, 2017-03-03 02:08:11]

Dry run: This issue passed the CQ dry run.

[pthatcher1, 2017-03-03 06:28:37]

pthatcher@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2017-03-03 06:28:38]

Mainly nits.

But I do think the use of rtc::optional would be good.

Nice CL.

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/ortcfact...
File webrtc/api/ortc/ortcfactoryinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/ortcfact...
webrtc/api/ortc/ortcfactoryinterface.h:130: // Creates a SrtpTransport which is
an RTP transport that uses SRTP.
an SrtpTransport

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/srtptran...
File webrtc/api/ortc/srtptransportinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:34: // There are some limitations of
current implementation:
the current implementation

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:42: // Set the SRTP keying material for
incoming streams.
Isn't it for incoming streams and RTCP?  Might as well just say for receiving
RTP and RTCP.

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:105: bool have_receive_key_ = false;
Why not use rtc::Optional<cricket::CryptoParams> for the keys?  That's a little
cleaner than having the separate bools.

[Zhi Huang, 2017-03-03 18:57:07]

Patchset #7 (id:300001) has been deleted

[Zhi Huang, 2017-03-03 18:57:18]

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

[commit-bot: I haz the power, 2017-03-03 18:57:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-03 19:24:01]

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

[commit-bot: I haz the power, 2017-03-03 19:24:02]

Dry run: This issue passed the CQ dry run.

[Zhi Huang, 2017-03-03 22:34:47]

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/ortcfact...
File webrtc/api/ortc/ortcfactoryinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/ortcfact...
webrtc/api/ortc/ortcfactoryinterface.h:130: // Creates a SrtpTransport which is
an RTP transport that uses SRTP.
On 2017/03/03 06:28:37, pthatcher1 wrote:
> an SrtpTransport

Done.

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/srtptran...
File webrtc/api/ortc/srtptransportinterface.h (right):

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:34: // There are some limitations of
current implementation:
On 2017/03/03 06:28:38, pthatcher1 wrote:
> the current implementation

I made a lot of mistakes like this recently. Maybe someone can teach me how to
use "the". :)

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/api/ortc/srtptran...
webrtc/api/ortc/srtptransportinterface.h:42: // Set the SRTP keying material for
incoming streams.
On 2017/03/03 06:28:38, pthatcher1 wrote:
> Isn't it for incoming streams and RTCP?  Might as well just say for receiving
> RTP and RTCP.  

Done.

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/ortc/rtptransport...
File webrtc/ortc/rtptransportadapter.h (right):

https://codereview.webrtc.org/2714813004/diff/280001/webrtc/ortc/rtptransport...
webrtc/ortc/rtptransportadapter.h:105: bool have_receive_key_ = false;
On 2017/03/03 06:28:38, pthatcher1 wrote:
> Why not use rtc::Optional<cricket::CryptoParams> for the keys?  That's a
little
> cleaner than having the separate bools.

Yes, this is a good point!

[Zhi Huang, 2017-03-03 22:36:46]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2017-03-03 22:36:47]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2714813004/#ps320001 (title:
"Use rtc::Optional for SRTP send and receive keys.")

[commit-bot: I haz the power, 2017-03-03 22:36:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-03 22:39:07]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1488580606665590,
"parent_rev": "716e726ef0b322e8317b749613691da043bfc61c", "commit_rev":
"d3501adf17fda4ad270faee89b6fe4f4a496b147"}

[commit-bot: I haz the power, 2017-03-03 22:39:11]

Description was changed from

==========
Create the SrtpTransportInterface.

Create the SrtpTransportInterface, a subclass of RtpTransportInterface, which
allows the user to set the send and receive keys. The functionalities are
implemented inside the RtpTransportAdapters on top of BaseChannel.

BUG=webrtc:7013
==========

to

==========
Create the SrtpTransportInterface.

Create the SrtpTransportInterface, a subclass of RtpTransportInterface, which
allows the user to set the send and receive keys. The functionalities are
implemented inside the RtpTransportAdapters on top of BaseChannel.

BUG=webrtc:7013

Review-Url: https://codereview.webrtc.org/2714813004
Cr-Commit-Position: refs/heads/master@{#17023}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d3501adf17fda4ad270faee89...
==========

[commit-bot: I haz the power, 2017-03-03 22:39:12]

Committed patchset #7 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/d3501adf17fda4ad270faee89...