Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2997983002:
Completed the functionalities of SrtpTransport. (Closed)
DescriptionCompleted the functionalities of SrtpTransport.
The SrtpTransport takes the SRTP responsibilities from the BaseChannel
and SrtpFilter. SrtpTransport is now responsible for setting the crypto
keys, protecting and unprotecting the packets. SrtpTransport doesn't know
if the keys are from SDES or DTLS handshake.
BaseChannel is now only responsible setting the offer/answer for SDES
or extracting the key from DtlsTransport and configuring the
SrtpTransport.
SrtpFilter is used by BaseChannel as a helper for SDES negotiation.
BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2997983002
Cr-Commit-Position: refs/heads/master@{#19636}
Committed: https://chromium.googlesource.com/external/webrtc/+/e683c6871fef24d3ff64f085d6bc0e965f17fcf7
Patch Set 1 : Added unit tests for SrtpTransport. #
Total comments: 24
Patch Set 2 : Resolve comments. #Patch Set 3 : Create the SrtpTransport by default and reset it to nullptr if the insecure RTP protocol is used. #
Total comments: 47
Patch Set 4 : Resolve the comments. #Patch Set 5 : Remove the UpdateRtpParams method. #
Total comments: 14
Patch Set 6 : Review comments. #Patch Set 7 : Fix the chromimum issue. #
Messages
Total messages: 52 (38 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== SrtpTransport. ========== to ========== Completed the functionalities of SrtpTransport. The SrtpTransport takes the SRTP responsibilities from the BaseChannel and SrtpFilter. SrtpTransport is now responsible for setting the crypto keys, protecting and unprotecting the packets. SrtpTransport doesn't know if the keys are from SDES or DTLS handshake. BaseChannel is now only responsible setting the offer/answer for SDES or extracting the key from DtlsTransport and configuring the SrtpTransport. SrtpFilter is used by BaseChannel as a helper for SDES negotiation. ==========
Patchset #1 (id:20001) has been deleted
zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org
Hi Taylor, PTAL. I'm not confident about the external authentication part in this CL. Please pay extra attention there. Thanks!
This is just what I had in mind; looks good. Though I think things can be simplified further if we remove support for falling back from SRTP to plain RTP (or in other words, creating a plain RtpTransport that's wrapped in an SrtpTransport once SDES is negotiated). See comments for more details. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/p2p/base/fakepacke... File webrtc/p2p/base/fakepackettransport.h (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/p2p/base/fakepacke... webrtc/p2p/base/fakepackettransport.h:91: } Can this be private and called internally? Instead of SendPacketInternal calling SignalReadPacket, it could call a private "HandleReceivedPacket" method which both sets last_received_packet_ and calls SignalReadPacket. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:328: srtp_filter_.ResetParams(); nit: Don't need to actually reset srtp_filter_ here. If DTLS is being used, srtp_filter_ is unused, right? https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:691: updated_options = options; Since the options are updated inside of srtp_transport_->SendPacket, don't need to make a copy here. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:931: srtp_filter_.EnableDtlsSrtp(); Why does srtp_filter_ even need to know if DTLS-SRTP is used? If DTLS-SRTP is used, srtp_filter_ isn't really used, right? https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:1074: LOG(LS_INFO) << "Wrapping RtpTransport in SrtpTransport."; I don't think upgrading from plain RTP to SRTP is something we need to support. RFC4568 says: When the offerer receives the answer, the offerer MUST verify that one of the initially offered crypto suites and its accompanying tag were accepted and echoed in the answer. So I'd say we should drop support for this, for both simplicity and standards compliance. Probably safest to do so in a separate CL. This means that we'd only create the RTP transport object once the first description is set, and the "proto" string (e.g., "RTP", "RTP/SAVPF" or "UDP/TLS/RTP/SAVPF") is known. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:1150: LOG(LS_INFO) << "No crypto keys are provided for SDES."; This looks the same as the code above; could it be moved out of the switch statement? // If setting an SDES answer succeeded, apply the negotiated parameters // to the SRTP transport. if ((action == CA_PRANSWER || action == CA_ANSWER) && !dlts && ret) { // ... } https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:1433: rtp_abs_sendtime_extn_id_ = rtp_abs_sendtime_extn_id; If this were always called after the SRTP transport is created, it wouldn't need to be cached in both places. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtpfilter.h File webrtc/pc/srtpfilter.h (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtpfilter.h#ne... webrtc/pc/srtpfilter.h:46: // TODO(zhihuang): Find a better name for this class, like "SdesNegotiator". That name sounds fine to me. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtpfilter.h#ne... webrtc/pc/srtpfilter.h:85: void EnableDtlsSrtp(); Like I mentioned in another comment, I don't think this class should care if DTLS-SRTP is used. The meaning of "IsActive" can be changed from "SRTP is active" to "SDES is active". https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport.h File webrtc/pc/srtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport.h... webrtc/pc/srtptransport.h:35: SrtpTransport(std::unique_ptr<RtpTransportInternal> transport, If we remove support for upgrading from plain RTP to SRTP, then I think we can remove the RtpTransportInternal argument from the constructor. The fact that SrtpTransport wraps an RtpTransport internally can be an implementation detail, and it can construct the inner RtpTransport itself. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport.h... webrtc/pc/srtptransport.h:69: bool IsActive() const; Some comments for this method and the other new methods would be helpful. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport_u... File webrtc/pc/srtptransport_unittest.cc (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport_u... webrtc/pc/srtptransport_unittest.cc:362: bool enable_external_auth = false; These tests aren't using external auth, like the tests they're replacing are. To make sure we don't lose test coverage, they should test it. Some background about what "external auth" is: as you probably know, in chromium, packets are actually delivered to the OS in a separate process, which means the timestamps webrtc puts in them (in the "abs-send-time" extension) may be inaccurate. More accurate send timestamps improve the performance of the bandwidth estimator, so we want to fill in this timestamp right before the packet is sent, in the browser process. But that also means the HMAC needs to be recalculated, since it's computed from the full contents of the packet including header extensions. This rewriting happens in "cricket::ApplyPacketOptions", called here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/p2p/socket... The packet options passed along everywhere include "PacketTimeUpdateParams", which includes just enough information to do this rewriting of the relevant bits. Though it doesn't work for GCM ciphers. So, how does it relate to these tests? In these tests, there's nothing that actually does this rewriting. So we'd need to add it; FakeRtpTransport could do it I think. Or we could just do what the old tests do, and do "TestRtpAuthParams" instead of actually trying to decode the packet. But testing it for real would be preferable.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
PTAL. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/p2p/base/fakepacke... File webrtc/p2p/base/fakepackettransport.h (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/p2p/base/fakepacke... webrtc/p2p/base/fakepackettransport.h:91: } On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > Can this be private and called internally? Instead of SendPacketInternal calling > SignalReadPacket, it could call a private "HandleReceivedPacket" method which > both sets last_received_packet_ and calls SignalReadPacket. We can just store the last sent packet before trigger the signal and have a getter for it. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:328: srtp_filter_.ResetParams(); On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > nit: Don't need to actually reset srtp_filter_ here. If DTLS is being used, > srtp_filter_ is unused, right? Right. Previously, srtp_filter_.IsActive() can also imply the DTLS keys are successfully installed. So I think the method |secure()| should be renamed to |secure_sdes()| as well. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:691: updated_options = options; On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > Since the options are updated inside of srtp_transport_->SendPacket, don't need > to make a copy here. Done. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:931: srtp_filter_.EnableDtlsSrtp(); On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > Why does srtp_filter_ even need to know if DTLS-SRTP is used? If DTLS-SRTP is > used, srtp_filter_ isn't really used, right? Done. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:1074: LOG(LS_INFO) << "Wrapping RtpTransport in SrtpTransport."; On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > I don't think upgrading from plain RTP to SRTP is something we need to support. > RFC4568 says: > > When the offerer receives the answer, the offerer MUST verify that > one of the initially offered crypto suites and its accompanying tag > were accepted and echoed in the answer. > > So I'd say we should drop support for this, for both simplicity and standards > compliance. Probably safest to do so in a separate CL. > > This means that we'd only create the RTP transport object once the first > description is set, and the "proto" string (e.g., "RTP", "RTP/SAVPF" or > "UDP/TLS/RTP/SAVPF") is known. I would like to talk about this a little bit more. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:1150: LOG(LS_INFO) << "No crypto keys are provided for SDES."; On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > This looks the same as the code above; could it be moved out of the switch > statement? > > // If setting an SDES answer succeeded, apply the negotiated parameters > // to the SRTP transport. > if ((action == CA_PRANSWER || action == CA_ANSWER) && !dlts && ret) { > // ... > } Yes, that would be cleaner. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/channel.cc#newc... webrtc/pc/channel.cc:1433: rtp_abs_sendtime_extn_id_ = rtp_abs_sendtime_extn_id; On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > If this were always called after the SRTP transport is created, it wouldn't need > to be cached in both places. This is called after SetTransportParameters which create the srtptransport. I'll add a DCHECK here then. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtpfilter.h File webrtc/pc/srtpfilter.h (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtpfilter.h#ne... webrtc/pc/srtpfilter.h:46: // TODO(zhihuang): Find a better name for this class, like "SdesNegotiator". On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > That name sounds fine to me. I think it would be better to do that in a separate CL since renaming the header would affect some internal projects. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtpfilter.h#ne... webrtc/pc/srtpfilter.h:85: void EnableDtlsSrtp(); On 2017/08/23 22:13:29, Taylor Brandstetter wrote: > Like I mentioned in another comment, I don't think this class should care if > DTLS-SRTP is used. The meaning of "IsActive" can be changed from "SRTP is > active" to "SDES is active". Done. Removed. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport.h File webrtc/pc/srtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport.h... webrtc/pc/srtptransport.h:35: SrtpTransport(std::unique_ptr<RtpTransportInternal> transport, On 2017/08/23 22:13:30, Taylor Brandstetter wrote: > If we remove support for upgrading from plain RTP to SRTP, then I think we can > remove the RtpTransportInternal argument from the constructor. The fact that > SrtpTransport wraps an RtpTransport internally can be an implementation detail, > and it can construct the inner RtpTransport itself. Acknowledged. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport.h... webrtc/pc/srtptransport.h:69: bool IsActive() const; On 2017/08/23 22:13:30, Taylor Brandstetter wrote: > Some comments for this method and the other new methods would be helpful. Done. https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport_u... File webrtc/pc/srtptransport_unittest.cc (right): https://codereview.webrtc.org/2997983002/diff/40001/webrtc/pc/srtptransport_u... webrtc/pc/srtptransport_unittest.cc:362: bool enable_external_auth = false; On 2017/08/23 22:13:30, Taylor Brandstetter wrote: > These tests aren't using external auth, like the tests they're replacing are. To > make sure we don't lose test coverage, they should test it. > > Some background about what "external auth" is: as you probably know, in > chromium, packets are actually delivered to the OS in a separate process, which > means the timestamps webrtc puts in them (in the "abs-send-time" extension) may > be inaccurate. More accurate send timestamps improve the performance of the > bandwidth estimator, so we want to fill in this timestamp right before the > packet is sent, in the browser process. But that also means the HMAC needs to be > recalculated, since it's computed from the full contents of the packet including > header extensions. > > This rewriting happens in "cricket::ApplyPacketOptions", called here: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/p2p/socket... > > The packet options passed along everywhere include "PacketTimeUpdateParams", > which includes just enough information to do this rewriting of the relevant > bits. Though it doesn't work for GCM ciphers. > > So, how does it relate to these tests? In these tests, there's nothing that > actually does this rewriting. So we'd need to add it; FakeRtpTransport could do > it I think. Or we could just do what the old tests do, and do > "TestRtpAuthParams" instead of actually trying to decode the packet. But testing > it for real would be preferable. Thanks for the detailed explanation! Done.
Patchset #3 (id:120001) has been deleted
I've some updates on this. The idea of patch3 is creating an SrtpTransport by default since this is the most common case and using the SrtpTransport as a plain RtpTransport if the insecure protocol is specified in the SDP.
Sorry for leading you wrong, but I think it would be cleaner for now to do what you were doing before, and wrap the plain RtpTransport in an SrtpTransport once SDES or DTLS negotiation finishes. Once we have a "DtlsSrtpTransport" and have shifted responsibility to TransportController, I think most our difficulties will be solved. How I'm imagining it will work is: 1. When new m= section appears, WebRtcSession calls TransportController::CreateRtpTransport, passing in the ContentInfo*. 2. TransportController defers to JsepTransport, which creates the stack of transport objects needed depending on the ContentInfo. 3. RtpTransportInternal interface passed to BaseChannel, which just has "SendRtpPacket" and "SendRtcpPacket" interfaces. 4. Later, when the answer is applied, TransportController::PushdownTransportDescription is called. Again, JsepTransport handles the details. I'll try to work on a prototype of moving things to TransportController if I have time, to see if there are any unanticipated roadblocks. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:122: return protocol == "RTP/AVPF" || protocol == "RTP/AVP"; Looking elsewhere, it looks like we typically use "cryptos.empty()" to tell if SDES is being used, and "IsDtlsActive" to tell if DTLS is being used. Also, we actually currently accept the "RTP/AVP" string in chromium for DTLS (I checked using the SDP munging sample). So, sorry for leading you wrong by suggesting using the protocol string, but I think we should aim for minimizing unintended side effects of this CL, and do something similar to CheckSrtpConfig_n https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:760: RTC_DCHECK(!srtp_required_); This should be an error rather than a DCHECK since it's possible to hit, right? https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:761: srtp_transport_ = nullptr; This unsets the srtp_transport_ pointer, but rtp_transport_ is still really an SrtpTransport, correct? So how does this work? Are there any tests that exercise using plain RTP? I guess there are the ORTC tests, so I wonder if they pass (and how). EDIT: Nevermind, I see now. See later comment. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:1075: LOG(LS_WARNING) << "Setting SRTP without an SrtpTransport."; It looks like this warning message would be logged for every offer/answer if not using SRTP. Maybe it should only be logged if "cryptos" is nonempty. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:1118: LOG(LS_INFO) << "No crypto keys are provided for SDES."; Is it even possible to hit this point? Won't "srtp_transport_" already be null, from PushdownRemoteDescription? https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h File webrtc/pc/channel.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:103: // This function returns true if we are using SRTP. nit: Should update this comment. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc File webrtc/pc/srtpfilter.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc#... webrtc/pc/srtpfilter.cc:19: #include "webrtc/pc/srtptransport.h" What part of srtptransport.h does this use? https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc#... webrtc/pc/srtpfilter.cc:124: CryptoParams new_send_params = Can't these still be const refs? https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc#... webrtc/pc/srtpfilter.cc:242: std::vector<unsigned char>* key, Could this be an rtc::Buffer like before, instead of a vector<unsigned char>? May make the copy simpler. Another option would be to not even do the parsing in this class, but do it in SrtpTransport, implementing SetSrtpSendKey and SetSrtpReceiveKey from SrtpTransportInterface, which take CryptoParams as input. That could be saved for a later CL though if it makes things more complex. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.h File webrtc/pc/srtpfilter.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.h#n... webrtc/pc/srtpfilter.h:107: bool ParseSendParams(const CryptoParams& send_params); nit: This does more than parse, since it modifies the class's state. So I'd call it "Apply" like before, or something similar. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.cc File webrtc/pc/srtptransport.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.cc:113: return rtp_transport_->SendPacket(rtcp, packet, options, flags); Oh, I see how this works now. If plain RTP is used, BaseChannel will call "rtp_transport_->SendPacket", which will actually go into "SrtpTransport::SendPacket", which will call "rtp_transport_->SendPacket". We do this kind of thing elsewhere but I don't think it's a good pattern, and it also defeats some of the purpose of "srtp_required", which was to give us a high degree of confidence that, if true, it will be impossible to send unencrypted packets. But with this code, it's possible if a bug is introduced and SrtpTransport::SendPacket ends up called while IsActive is false. So I'd suggest going back to something like you had before. We can work on cleaning up this class once TransportController becomes responsible for it and it gets more decoupled from BaseChannel. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.cc:165: send_session_ = nullptr; Any reason to reset the send session but not the receive session? It looks like SrtpFilter wasn't resetting either before. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport_... File webrtc/pc/srtptransport_unittest.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport_... webrtc/pc/srtptransport_unittest.cc:428: It looks like all the equivalent tests from srtpfilter_unittest.cc are here except for "TestSetParamsKeyTooShort"; might as well add that.
pthatcher@google.com changed reviewers: + pthatcher@google.com
https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:692: if (secure()) { I think making the shorter path the early return would be nicer: if (!srtp_active()) { return rtp_transport_->...; } ... return srtp_transport->...; https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:697: return srtp_transport_->SendPacket(rtcp, packet, options, flags); I don't think it should be the responsibility of the BaseChannel to set the "I'm really SRTP" flag. That should be the job of the SrtpTransport. I think SrtpTransport should have a SendPacket which doesn't take the flags and then it calls DtlsTransport with the flags. In fact, it would be even better to have separate SendRtpPacket and SendRtpPacket so that you wouldn't need to rtcp bool either. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:701: return rtp_transport_->SendPacket(rtcp, packet, options, PF_NORMAL); And similarly here, it would make sense to call SendRtpPacket or SendRtcpPacket. And not pass in the flag, because it's not doing anything anyway. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:955: } If this is only used for changing the header extensions IDs, why not have SetEncryptedHeaderExtensionIds do the update? https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h File webrtc/pc/channel.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:104: bool secure_sdes() const { return srtp_filter_.IsActive(); } While we're changing the method name, can we make it something more understandable, like "sdes_active" (and the other "dtls_active")? https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:110: bool secure() const { return secure_sdes() || secure_dtls(); } Would "srtp_active" be a good name here, to along with the others? https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:407: SrtpFilter srtp_filter_; It feels like this really should be renamed to SdesX sdes_x_. What should the x be? Hmmm... maybe SdesSession or SdesNegotiation. But it's not a filter any more, and it's SDES-specific, not just any SRTP. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.h File webrtc/pc/srtpfilter.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.h#n... webrtc/pc/srtpfilter.h:46: // TODO(zhihuang): Find a better name for this class, like "SdesNegotiator". If you don't rename the class in this CL, at least rename the member variable (from srtp_filter_). I'd prefer them not matching if at least one is a good name. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.cc File webrtc/pc/srtptransport.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.cc:113: return rtp_transport_->SendPacket(rtcp, packet, options, flags); On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > Oh, I see how this works now. If plain RTP is used, BaseChannel will call > "rtp_transport_->SendPacket", which will actually go into > "SrtpTransport::SendPacket", which will call "rtp_transport_->SendPacket". We do > this kind of thing elsewhere but I don't think it's a good pattern, and it also > defeats some of the purpose of "srtp_required", which was to give us a high > degree of confidence that, if true, it will be impossible to send unencrypted > packets. But with this code, it's possible if a bug is introduced and > SrtpTransport::SendPacket ends up called while IsActive is false. > > So I'd suggest going back to something like you had before. We can work on > cleaning up this class once TransportController becomes responsible for it and > it gets more decoupled from BaseChannel. Yeah, I would prefer that the SrtpTransport doesn't send anything if it's not active and the user of the SrtpTransport has to ensure it is active. And the user can bypass the SrtpTransport if it doesn't want crypto. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.cc:149: SignalPacketReceived(rtcp, packet, packet_time); Same here: if it's not active, drop the packet. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.h File webrtc/pc/srtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.h:118: // keys. Why do we have this split between SetRtpParams and UpdateRtpParams? Why not just have one?
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
PTAL. Thanks! https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:122: return protocol == "RTP/AVPF" || protocol == "RTP/AVP"; On 2017/08/26 02:40:38, Taylor Brandstetter wrote: > Looking elsewhere, it looks like we typically use "cryptos.empty()" to tell if > SDES is being used, and "IsDtlsActive" to tell if DTLS is being used. Also, we > actually currently accept the "RTP/AVP" string in chromium for DTLS (I checked > using the SDP munging sample). So, sorry for leading you wrong by suggesting > using the protocol string, but I think we should aim for minimizing unintended > side effects of this CL, and do something similar to CheckSrtpConfig_n Removed. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:692: if (secure()) { On 2017/08/28 21:42:56, pthatcher wrote: > I think making the shorter path the early return would be nicer: > > if (!srtp_active()) { > return rtp_transport_->...; > } > > ... > return srtp_transport->...; Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:697: return srtp_transport_->SendPacket(rtcp, packet, options, flags); On 2017/08/28 21:42:56, pthatcher wrote: > I don't think it should be the responsibility of the BaseChannel to set the "I'm > really SRTP" flag. That should be the job of the SrtpTransport. I think > SrtpTransport should have a SendPacket which doesn't take the flags and then it > calls DtlsTransport with the flags. > > In fact, it would be even better to have separate SendRtpPacket and > SendRtpPacket so that you wouldn't need to rtcp bool either. SrtpTransport doesn't interact with DtlsTransport directly and it relies on the wrapped RtpTransport to manage the states (ready_to_send_ etc.) The RtpTransport needs to have the capability to set the flags explicitly so we still need SendPacket(bool rtcp, rtc::CopyOnWriteBuffer* packet, const rtc::PacketOptions& options, int flag); in RtpTransportInternal and the SrtpTransport still needs to override this method. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:701: return rtp_transport_->SendPacket(rtcp, packet, options, PF_NORMAL); On 2017/08/28 21:42:56, pthatcher wrote: > And similarly here, it would make sense to call SendRtpPacket or SendRtcpPacket. > And not pass in the flag, because it's not doing anything anyway. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:760: RTC_DCHECK(!srtp_required_); On 2017/08/26 02:40:38, Taylor Brandstetter wrote: > This should be an error rather than a DCHECK since it's possible to hit, right? I'll remove these for now. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:761: srtp_transport_ = nullptr; On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > This unsets the srtp_transport_ pointer, but rtp_transport_ is still really an > SrtpTransport, correct? So how does this work? Are there any tests that exercise > using plain RTP? I guess there are the ORTC tests, so I wonder if they pass (and > how). > > EDIT: Nevermind, I see now. See later comment. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:955: } On 2017/08/28 21:42:56, pthatcher wrote: > If this is only used for changing the header extensions IDs, why not have > SetEncryptedHeaderExtensionIds do the update? SetEncryptedHeaderExtensionIds just stores the updated ids and we need to call UpdateRtpParams for new ids to take effect. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:1075: LOG(LS_WARNING) << "Setting SRTP without an SrtpTransport."; On 2017/08/26 02:40:38, Taylor Brandstetter wrote: > It looks like this warning message would be logged for every offer/answer if not > using SRTP. Maybe it should only be logged if "cryptos" is nonempty. Acknowledged. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:1118: LOG(LS_INFO) << "No crypto keys are provided for SDES."; On 2017/08/26 02:40:38, Taylor Brandstetter wrote: > Is it even possible to hit this point? Won't "srtp_transport_" already be null, > from PushdownRemoteDescription? If we still support upgrading rtp_transport to srtp_transport for now, then I think this could be hit if the crypto is empty in answer. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h File webrtc/pc/channel.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:103: // This function returns true if we are using SRTP. On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > nit: Should update this comment. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:104: bool secure_sdes() const { return srtp_filter_.IsActive(); } On 2017/08/28 21:42:56, pthatcher wrote: > While we're changing the method name, can we make it something more > understandable, like "sdes_active" (and the other "dtls_active")? Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:110: bool secure() const { return secure_sdes() || secure_dtls(); } On 2017/08/28 21:42:56, pthatcher wrote: > Would "srtp_active" be a good name here, to along with the others? It sounds good to me. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/channel.h#newc... webrtc/pc/channel.h:407: SrtpFilter srtp_filter_; On 2017/08/28 21:42:56, pthatcher wrote: > It feels like this really should be renamed to SdesX sdes_x_. What should the x > be? Hmmm... maybe SdesSession or SdesNegotiation. But it's not a filter any > more, and it's SDES-specific, not just any SRTP. What about sdes_negotiator_? I plan to rename the class and files in a separate CL since it would affect internal projects. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc File webrtc/pc/srtpfilter.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc#... webrtc/pc/srtpfilter.cc:19: #include "webrtc/pc/srtptransport.h" On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > What part of srtptransport.h does this use? Removed. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc#... webrtc/pc/srtpfilter.cc:124: CryptoParams new_send_params = On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > Can't these still be const refs? Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.cc#... webrtc/pc/srtpfilter.cc:242: std::vector<unsigned char>* key, On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > Could this be an rtc::Buffer like before, instead of a vector<unsigned char>? > May make the copy simpler. > > Another option would be to not even do the parsing in this class, but do it in > SrtpTransport, implementing SetSrtpSendKey and SetSrtpReceiveKey from > SrtpTransportInterface, which take CryptoParams as input. That could be saved > for a later CL though if it makes things more complex. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.h File webrtc/pc/srtpfilter.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.h#n... webrtc/pc/srtpfilter.h:46: // TODO(zhihuang): Find a better name for this class, like "SdesNegotiator". On 2017/08/28 21:42:56, pthatcher wrote: > If you don't rename the class in this CL, at least rename the member variable > (from srtp_filter_). I'd prefer them not matching if at least one is a good > name. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtpfilter.h#n... webrtc/pc/srtpfilter.h:107: bool ParseSendParams(const CryptoParams& send_params); On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > nit: This does more than parse, since it modifies the class's state. So I'd call > it "Apply" like before, or something similar. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.cc File webrtc/pc/srtptransport.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.cc:113: return rtp_transport_->SendPacket(rtcp, packet, options, flags); On 2017/08/28 21:42:56, pthatcher wrote: > On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > > Oh, I see how this works now. If plain RTP is used, BaseChannel will call > > "rtp_transport_->SendPacket", which will actually go into > > "SrtpTransport::SendPacket", which will call "rtp_transport_->SendPacket". We > do > > this kind of thing elsewhere but I don't think it's a good pattern, and it > also > > defeats some of the purpose of "srtp_required", which was to give us a high > > degree of confidence that, if true, it will be impossible to send unencrypted > > packets. But with this code, it's possible if a bug is introduced and > > SrtpTransport::SendPacket ends up called while IsActive is false. > > > > So I'd suggest going back to something like you had before. We can work on > > cleaning up this class once TransportController becomes responsible for it and > > it gets more decoupled from BaseChannel. > > Yeah, I would prefer that the SrtpTransport doesn't send anything if it's not > active and the user of the SrtpTransport has to ensure it is active. And the > user can bypass the SrtpTransport if it doesn't want crypto. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.cc:149: SignalPacketReceived(rtcp, packet, packet_time); On 2017/08/28 21:42:56, pthatcher wrote: > Same here: if it's not active, drop the packet. Done. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.cc:165: send_session_ = nullptr; On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > Any reason to reset the send session but not the receive session? It looks like > SrtpFilter wasn't resetting either before. They are used to determine the status of the srtptransport. If both of are non-null, the transport is active. The logic here is that if we failed to set the key, then reset the session and the transport become inactive. Maybe it would be cleaner just calling ResetParams. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.h File webrtc/pc/srtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.h:118: // keys. On 2017/08/28 21:42:56, pthatcher wrote: > Why do we have this split between SetRtpParams and UpdateRtpParams? Why not > just have one? SetRtpParams calls SrtpSession::SetSend/Recv; UpdateRtpParams calls: SrtpSession::UpdateSend/Recv. We can use the same method but it would require some extra refactoring in SrtpSession. https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport_... File webrtc/pc/srtptransport_unittest.cc (right): https://codereview.webrtc.org/2997983002/diff/140001/webrtc/pc/srtptransport_... webrtc/pc/srtptransport_unittest.cc:428: On 2017/08/26 02:40:39, Taylor Brandstetter wrote: > It looks like all the equivalent tests from srtpfilter_unittest.cc are here > except for "TestSetParamsKeyTooShort"; might as well add that. Oh, I missed that. Done.
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/21962)
I removed the UpdateRtpParams method in this patch to make it slightly easier to understand. The current implementation of SetEncryptedHeaderExtensionIds just caches the new ids and it won't take effect until SetRtpParams/UpdateParams are called. It's not very straightforward to change the SetEncryptedHeaderExtensionIds to call SetRtpParams/UpdateRtpParams inside SRTP transport because we need to set the keys at the same time. One possible solution would be caching the keys set last time and update the header extension ids with the cached keys. But I'm not sure if it is a good practice to cached the keys as a member variables in the class. I've reached out to some experts to get some advice.
Looks good, just a few minor comments left https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:321: // When using DTLS-SRTP, we must reset the SrtpFilter every time the transport nit: "we must reset the SrtpTransport every time the DtlsTransport changes..." https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:591: // Reset the srtp filter if it's not the CONNECTED state. For the CONNECTED nit: "Reset the SrtpTransport" https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:1076: if (!srtp_transport_ && !dtls && !cryptos.empty()) { nit: May be helpful to add a comment here, saying something like "If SRTP was not required, but we're setting a description that uses SDES, we need to upgrade to an SrtpTransport." https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/rtptransport.h File webrtc/pc/rtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/rtptransport.h... webrtc/pc/rtptransport.h:68: int flags) override; Since BaseChannel now goes through Send[Rtp|Rtcp]Packet, can SendPacket be made private? I guess SendRtpPacket and SendRtcpPacket would need the "flags" argument then, for SrtpTransport to pass through to RtpTransport, which you wanted to avoid. But that seems preferable to having two ways to send packets in the public interface. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport.h File webrtc/pc/srtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.h:121: // Set the header extension ids that should be encrypted for the given source. Can you add a comment saying that this doesn't immediately update the SRTP session with the new IDs, and you need to call SetRtpParams for that to happen? https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport_... File webrtc/pc/srtptransport_unittest.cc (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport_... webrtc/pc/srtptransport_unittest.cc:39: class FakeRtpTransport : public RtpTransport { I'm realizing now: do we even need a "FakeRtpTransport" class, or could we just create real RtpTransports with FakePacketTransports? https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport_... webrtc/pc/srtptransport_unittest.cc:74: peer_->SignalPacketReceived(rtcp, packet, rtc::PacketTime()); It looks like this will cause the packet to be received twice: once from SendPacket and once from SignalPacketReceived being called directly. I tested this in a debugger and it looks like the only reason the first "SendPacket" doesn't work is that the payload type is unhandled (need to call AddHandledPayloadType).
Patchset #6 (id:240001) has been deleted
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_msvc_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_msvc_rel/builds/427) win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/12075) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/28625)
Patchset #6 (id:260001) has been deleted
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21021)
PTAL. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc File webrtc/pc/channel.cc (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:321: // When using DTLS-SRTP, we must reset the SrtpFilter every time the transport On 2017/08/30 21:24:26, Taylor Brandstetter wrote: > nit: "we must reset the SrtpTransport every time the DtlsTransport changes..." Done. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:591: // Reset the srtp filter if it's not the CONNECTED state. For the CONNECTED On 2017/08/30 21:24:26, Taylor Brandstetter wrote: > nit: "Reset the SrtpTransport" Done. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/channel.cc#new... webrtc/pc/channel.cc:1076: if (!srtp_transport_ && !dtls && !cryptos.empty()) { On 2017/08/30 21:24:26, Taylor Brandstetter wrote: > nit: May be helpful to add a comment here, saying something like "If SRTP was > not required, but we're setting a description that uses SDES, we need to upgrade > to an SrtpTransport." Done. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/rtptransport.h File webrtc/pc/rtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/rtptransport.h... webrtc/pc/rtptransport.h:68: int flags) override; On 2017/08/30 21:24:26, Taylor Brandstetter wrote: > Since BaseChannel now goes through Send[Rtp|Rtcp]Packet, can SendPacket be made > private? > > I guess SendRtpPacket and SendRtcpPacket would need the "flags" argument then, > for SrtpTransport to pass through to RtpTransport, which you wanted to avoid. > But that seems preferable to having two ways to send packets in the public > interface. OK. Then we don't even need SendPacket to be part of RtpTransportInternal which would simplify the code. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport.h File webrtc/pc/srtptransport.h (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport.... webrtc/pc/srtptransport.h:121: // Set the header extension ids that should be encrypted for the given source. On 2017/08/30 21:24:26, Taylor Brandstetter wrote: > Can you add a comment saying that this doesn't immediately update the SRTP > session with the new IDs, and you need to call SetRtpParams for that to happen? Done. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport_... File webrtc/pc/srtptransport_unittest.cc (right): https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport_... webrtc/pc/srtptransport_unittest.cc:39: class FakeRtpTransport : public RtpTransport { On 2017/08/30 21:24:26, Taylor Brandstetter wrote: > I'm realizing now: do we even need a "FakeRtpTransport" class, or could we just > create real RtpTransports with FakePacketTransports? We don't actually need this fake class. Removing it would make it cleaner. Done. https://codereview.webrtc.org/2997983002/diff/220001/webrtc/pc/srtptransport_... webrtc/pc/srtptransport_unittest.cc:74: peer_->SignalPacketReceived(rtcp, packet, rtc::PacketTime()); On 2017/08/30 21:24:26, Taylor Brandstetter wrote: > It looks like this will cause the packet to be received twice: once from > SendPacket and once from SignalPacketReceived being called directly. I tested > this in a debugger and it looks like the only reason the first "SendPacket" > doesn't work is that the payload type is unhandled (need to call > AddHandledPayloadType). Done.
lgtm
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21066)
Description was changed from ========== Completed the functionalities of SrtpTransport. The SrtpTransport takes the SRTP responsibilities from the BaseChannel and SrtpFilter. SrtpTransport is now responsible for setting the crypto keys, protecting and unprotecting the packets. SrtpTransport doesn't know if the keys are from SDES or DTLS handshake. BaseChannel is now only responsible setting the offer/answer for SDES or extracting the key from DtlsTransport and configuring the SrtpTransport. SrtpFilter is used by BaseChannel as a helper for SDES negotiation. ========== to ========== Completed the functionalities of SrtpTransport. The SrtpTransport takes the SRTP responsibilities from the BaseChannel and SrtpFilter. SrtpTransport is now responsible for setting the crypto keys, protecting and unprotecting the packets. SrtpTransport doesn't know if the keys are from SDES or DTLS handshake. BaseChannel is now only responsible setting the offer/answer for SDES or extracting the key from DtlsTransport and configuring the SrtpTransport. SrtpFilter is used by BaseChannel as a helper for SDES negotiation. BUG=webrtc:7013 ==========
Patchset #7 (id:300001) has been deleted
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhihuang@webrtc.org
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/2997983002/#ps320001 (title: "Fix the chromimum issue.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1504220257522760,
"parent_rev": "4c14009e83bd147b440a785d7fb492da3355576c", "commit_rev":
"e683c6871fef24d3ff64f085d6bc0e965f17fcf7"}
Message was sent while issue was closed.
Description was changed from ========== Completed the functionalities of SrtpTransport. The SrtpTransport takes the SRTP responsibilities from the BaseChannel and SrtpFilter. SrtpTransport is now responsible for setting the crypto keys, protecting and unprotecting the packets. SrtpTransport doesn't know if the keys are from SDES or DTLS handshake. BaseChannel is now only responsible setting the offer/answer for SDES or extracting the key from DtlsTransport and configuring the SrtpTransport. SrtpFilter is used by BaseChannel as a helper for SDES negotiation. BUG=webrtc:7013 ========== to ========== Completed the functionalities of SrtpTransport. The SrtpTransport takes the SRTP responsibilities from the BaseChannel and SrtpFilter. SrtpTransport is now responsible for setting the crypto keys, protecting and unprotecting the packets. SrtpTransport doesn't know if the keys are from SDES or DTLS handshake. BaseChannel is now only responsible setting the offer/answer for SDES or extracting the key from DtlsTransport and configuring the SrtpTransport. SrtpFilter is used by BaseChannel as a helper for SDES negotiation. BUG=webrtc:7013 Review-Url: https://codereview.webrtc.org/2997983002 Cr-Commit-Position: refs/heads/master@{#19636} Committed: https://chromium.googlesource.com/external/webrtc/+/e683c6871fef24d3ff64f085d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/e683c6871fef24d3ff64f085d...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:320001) has been created in https://codereview.webrtc.org/3012333003/ by sprang@webrtc.org. The reason for reverting is: This seems to be causing some video freezes. See https://bugs.chromium.org/p/webrtc/issues/detail?id=8251. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
