Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1453523002:
Allow remote fingerprint update during a call (Closed)
|
Created:
5 years, 1 month ago by guoweis_webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAllow remote fingerprint update during a call
Changes include the following
1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case.
2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake.
3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS (not SDES).
4. Test cases for caller or callee are transfees.
BUG=webrtc:3618
R=pthatcher@webrtc.org
Committed: https://crrev.com/9c38c2d33fa6d794704d53b18f39d5235439fe63
Cr-Commit-Position: refs/heads/master@{#10901}
Patch Set 1 #Patch Set 2 : #
Total comments: 29
Patch Set 3 : address comments. #Patch Set 4 : fix rtc_unittests error #
Total comments: 38
Patch Set 5 : #Patch Set 6 : address comments. #
Total comments: 4
Patch Set 7 : address comments. #Patch Set 8 : Remove unused code. #Patch Set 9 : Remove empty line #
Total comments: 11
Patch Set 10 : address juberti comments #Patch Set 11 : add back the handling of the EOS #Patch Set 12 : Stop the original client. #Patch Set 13 : rebase #
Messages
Total messages: 60 (36 generated)
Description was changed from ========== fix' fix fix add comments only fix fix BUG= ========== to ========== Allow remote fingerprint update during a call Changes include the follwoing 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS. 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 ==========
Description was changed from ========== Allow remote fingerprint update during a call Changes include the follwoing 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS. 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 ========== to ========== Allow remote fingerprint update during a call Changes include the following 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS. 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 ==========
Description was changed from ========== Allow remote fingerprint update during a call Changes include the following 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS. 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 ========== to ========== Allow remote fingerprint update during a call Changes include the following 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS (not SDES). 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 ==========
guoweis@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
On 2015/11/16 22:37:05, guoweis wrote: Ping
https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:662: use_alternate_key_ = use_alternate_key; Why is this (and use_alternate_key_) static? https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:987: void MutuallySetSignalingReceiver() { How about "SetSignalingReceivers" (an "s" on the end and no "Mutually")? https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1081: // This test sets up a successful call then do the call transfer to another do => does the call transfer => a call transfer https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1088: true); Isn't this the default by now? https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1105: original_peer.reset(swap_initializing_client(new_client.release())); Wouldn't this work? if (test_callee) { original_peer_.reset(initiating_client_.release()); initiating_client_.reset(new_client.release()); } else { original_peer_.reset(receiving_client_.release()); receiving_client_.reset(new_client.release()); } Then you don't need the extra functions, and I think it's more readable. In fact, does this work? I'm not sure, but it's worth checking: if (test_callee) { original_peer_ = initiating_client; initiating_client_ = new_client; } else { original_peer_ = receiving_client_; receiving_client_ = new_client; } Because that would be even better. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1245: TransferCall(true); I think there are enough "if(test_callee)" checks in the shared logic to make sense to just duplicate logic between the two tests. Maybe just put the duplicate setup code in a setup method. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... File talk/app/webrtc/test/fakedtlsidentitystore.h (right): https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:40: } KeyAndCert[] = { Should this be kKeysAndCerts? https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:112: void use_alternate_key() { key_index_ = 1; } Instead of having "use_original" and "use_alternate", why not just have "set_key_by_index", and blow up if it's out of range? https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:131: KeyAndCert[0].rsa_private_key_pem, &key); Would it make sense to use key_index_ here too (or just key() and cert(); see below)? https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:165: KeyAndCert[key_index_].rsa_private_key_pem, Would it make sense to have a getter such as key() and cert()? https://codereview.webrtc.org/1453523002/diff/20001/talk/session/media/channe... File talk/session/media/channel.cc (right): https://codereview.webrtc.org/1453523002/diff/20001/talk/session/media/channe... talk/session/media/channel.cc:478: void BaseChannel::OnDtlsSrtpSetup(TransportChannel* channel) { Since this will be fired on the worker thread, we might want to put an "_w" on the end. https://codereview.webrtc.org/1453523002/diff/20001/talk/session/media/channe... talk/session/media/channel.cc:482: was_ever_writable_ = false; That's not what "was ever writable" means. It it was ever writable, you can't go back to not ever being writable. Why not just do "srtp_filter_.reset(new SrtpFilter());" right here and then wait until it's writable before setting up the keys? In fact, it might make sense to leave it null when it's not setup, and then do srtp_filter_.release() right here and then check for null in ChannelWritable_w() to see if it should be setup again. Now that I think about it, that probably makes the most sense because then all the places where we would use srtp_filter_, we can check to see if it's null, and if it is, that's when we can drop a packet because we can't decrypt it. https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:130: } I think instead of changing the Connect method, we should make an explicit Reconnect() method. That way, it's clear that Connect is called once and Reconnect() can be called many times. Or just call channel_->Connect() below instead of using Connect(). https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:230: } I think this would be more readable as something like this: if (remote_fingerprint_value_ == remote_fingerprint_value && remote_fingerprint_algorithm_ == digest_alg) { return; } remote_fingerprint_value_ = remote_fingerprint_value.Pass(); remote_fingerprint_algorithm_ = digest_alg; if (dtls_) { SignalDtlsReset(this); set_dtls_state(DTLS_TRANSPORT_NEW); set_writable(false); Reconnect(); return true; } if (!SetupDtls()) { set_dtls_state(DTLS_TRANSPORT_FAILED); return false; } return true; https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportchannel.h (right): https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportchannel.h:84: sigslot::signal1<TransportChannel*> SignalDtlsSrtpSetup; This sounds more like it should be OnDtlsReset. It's not SRTP-specific and "Setup" makes it sound like it's done once and that's it, but this can happen many times.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
PTAL. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:662: use_alternate_key_ = use_alternate_key; On 2015/11/18 20:42:43, pthatcher1 wrote: > Why is this (and use_alternate_key_) static? Done. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:987: void MutuallySetSignalingReceiver() { On 2015/11/18 20:42:43, pthatcher1 wrote: > How about "SetSignalingReceivers" (an "s" on the end and no "Mutually")? Done. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1081: // This test sets up a successful call then do the call transfer to another On 2015/11/18 20:42:42, pthatcher1 wrote: > do => does > the call transfer => a call transfer Done. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1088: true); On 2015/11/18 20:42:42, pthatcher1 wrote: > Isn't this the default by now? It's not very clear. might be the case from https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... but it's a bit subtle. Leave it here for consistency with other cases. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1105: original_peer.reset(swap_initializing_client(new_client.release())); On 2015/11/18 20:42:43, pthatcher1 wrote: > Wouldn't this work? > > if (test_callee) { > original_peer_.reset(initiating_client_.release()); > initiating_client_.reset(new_client.release()); > } else { > original_peer_.reset(receiving_client_.release()); > receiving_client_.reset(new_client.release()); > } > > Then you don't need the extra functions, and I think it's more readable. > > In fact, does this work? I'm not sure, but it's worth checking: > > > if (test_callee) { > original_peer_ = initiating_client; > initiating_client_ = new_client; > } else { > original_peer_ = receiving_client_; > receiving_client_ = new_client; > } > > Because that would be even better. Done. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/peerconne... talk/app/webrtc/peerconnection_unittest.cc:1245: TransferCall(true); On 2015/11/18 20:42:43, pthatcher1 wrote: > I think there are enough "if(test_callee)" checks in the shared logic to make > sense to just duplicate logic between the two tests. Maybe just put the > duplicate setup code in a setup method. Done. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... File talk/app/webrtc/test/fakedtlsidentitystore.h (right): https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:40: } KeyAndCert[] = { On 2015/11/18 20:42:43, pthatcher1 wrote: > Should this be kKeysAndCerts? Done. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:112: void use_alternate_key() { key_index_ = 1; } On 2015/11/18 20:42:43, pthatcher1 wrote: > Instead of having "use_original" and "use_alternate", why not just have > "set_key_by_index", and blow up if it's out of range? I was using key index to control which key to use but I don't like the blow up behavior and want to avoid function like num_of_keys() type of function. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:131: KeyAndCert[0].rsa_private_key_pem, &key); On 2015/11/18 20:42:43, pthatcher1 wrote: > Would it make sense to use key_index_ here too (or just key() and cert(); see > below)? I didn't do that since this is a static function. https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:165: KeyAndCert[key_index_].rsa_private_key_pem, On 2015/11/18 20:42:43, pthatcher1 wrote: > Would it make sense to have a getter such as key() and cert()? From the coding standard: structs should be used for passive objects that carry data, and may have associated constants, but lack any functionality other than access/setting the data members. The accessing/setting of fields is done by directly accessing the fields rather than through method invocations. Methods should not provide behavior but should only be used to set up the data members, e.g., constructor, destructor, Initialize(), Reset(), Validate(). It seems the directly accessing the fields is better? https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:130: } On 2015/11/18 20:42:43, pthatcher1 wrote: > I think instead of changing the Connect method, we should make an explicit > Reconnect() method. That way, it's clear that Connect is called once and > Reconnect() can be called many times. Or just call channel_->Connect() below > instead of using Connect(). Done. https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:230: } On 2015/11/18 20:42:43, pthatcher1 wrote: > I think this would be more readable as something like this: > > if (remote_fingerprint_value_ == remote_fingerprint_value && > remote_fingerprint_algorithm_ == digest_alg) { > return; > } > > remote_fingerprint_value_ = remote_fingerprint_value.Pass(); > remote_fingerprint_algorithm_ = digest_alg; > > if (dtls_) { > SignalDtlsReset(this); > set_dtls_state(DTLS_TRANSPORT_NEW); > set_writable(false); > Reconnect(); > return true; > } > > if (!SetupDtls()) { > set_dtls_state(DTLS_TRANSPORT_FAILED); > return false; > } > > return true; Reconnect has to be called after SetupDtls https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportchannel.h (right): https://codereview.webrtc.org/1453523002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportchannel.h:84: sigslot::signal1<TransportChannel*> SignalDtlsSrtpSetup; On 2015/11/18 20:42:43, pthatcher1 wrote: > This sounds more like it should be OnDtlsReset. It's not SRTP-specific and > "Setup" makes it sound like it's done once and that's it, but this can happen > many times. Rename to SignalDtlsState
https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... File talk/app/webrtc/test/fakedtlsidentitystore.h (right): https://codereview.webrtc.org/1453523002/diff/20001/talk/app/webrtc/test/fake... talk/app/webrtc/test/fakedtlsidentitystore.h:165: KeyAndCert[key_index_].rsa_private_key_pem, On 2015/11/25 21:03:13, guoweis wrote: > On 2015/11/18 20:42:43, pthatcher1 wrote: > > Would it make sense to have a getter such as key() and cert()? > > From the coding standard: > structs should be used for passive objects that carry data, and may have > associated constants, but lack any functionality other than access/setting the > data members. The accessing/setting of fields is done by directly accessing the > fields rather than through method invocations. Methods should not provide > behavior but should only be used to set up the data members, e.g., constructor, > destructor, Initialize(), Reset(), Validate(). > > It seems the directly accessing the fields is better? I meant helper methods like this: const char* key() { return KeyAndCert[key_index_].rsa_private_key_pem; } const char* cert() { return KeyAndCert[key_index_].cert_pem; } I that you could then use them in here and in GenerateCertificate, but it looks like it can't be done so easily in GenerateCertificate since it's static. https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:151: bool use_alternate_key) { Why not just CreateClientWithAlternateKey and then leave off the "use_alternate_key" variable and just assume it's true? https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:987: // during the destructing time. which => on which dependency => a dependency during the destructing time => when destroyed https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1135: } These two methods aren't used any more. https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1152: rtc::scoped_ptr<PeerConnectionTestClient> receiving_client_; Usually, we leave "foo_" members private and then have a protected "Foo& foo()". https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1261: initializing_client()->IceRestart(); It looks like we are testing the scenario where ICE + DTLS are both restarted. Should we also test the case where DTLS is restarted but not ICE? https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... File talk/session/media/channel.cc (right): https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:283: dtls_srtp_setup_pending_ = true; This is kind of hard to read and understand. In the case where we are writable already, UpdateWritableState_w() below will cause SRTP to be setup. In the case where it's not, it won't be. But that's not very clear. It doesn't help that this line is far away from that line. This is already on the worker thread, so no packets will be sent while we're in this code block. I think it would be safer to: 1. Move this line down to write above UpdateWritableState_w() below. 2. Rename it to "srtp_ready_ = false". 3. srtp_filter_.ResetParams(); here as well. 4. Put a comment indicating that UpdateWritableState_w will get SRTP ready if it's writable, or that it will get it ready once it is writable. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:327: dtls_srtp_setup_pending_ = true; Same here as with set_transport_channel() https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:431: !dtls_srtp_setup_pending(); I think we need this to be something like: was_ever_writable() && (srtp_ready() || !NeedSrtp()) to work correctly with SCTP data channels. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:488: if (!ShouldSetupDtlsSrtp()) { I think it would make it more readable to rename ShouldSetupDtlsSrtp to NeedSrtp(). https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:489: dtls_srtp_setup_pending_ = false; I think a combination of (srtp_ready() || !NeedSrtp()) would make this line unecessary. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:493: // Resetting the srtp filter as long as it's not the CONNECTED signal. For CONNECTED signal => CONNECTED state https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:493: // Resetting the srtp filter as long as it's not the CONNECTED signal. For Resetting => Reset as long as => if https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:494: // CONNECTED signal, setting up DTLS-SRTP context is deferred to For CONNECTED signal => For the connected state. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:796: << (dtls_srtp_setup_pending_ ? "" : " for the first time"); Now this isn't accurate any more. Perhaps it would be easier to just use was_ever_writable_ still. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:809: TrySetupDtlsSrtp_w(); Why is this TrySetupDtlsSrtp_w() instead of SetupDtlsSrtp_w() or MaybeSetupDtlsSrtp_w()? https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... File talk/session/media/channel.h (right): https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.h:321: bool dtls_srtp_setup_pending_; It feels like this should go right under srtp_filter_. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.h:321: bool dtls_srtp_setup_pending_; And "dtls" isn't really relevant. It's more whether SRTP is ready or not. So, "srtp_setup_pending_" would be better. But that makes me think that it would be even more clear to invert it to "srtp_ready_" or "srtp_filter_ready_". And that makes me wonder: why can't we just use srtp_filter_.Active()? https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:203: // multiple times. When we have local => Once we have the local could be => can be https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:238: set_dtls_state(DTLS_TRANSPORT_NEW); Why do we set the state to new and then assert the state is new in Reconnect()? Why not just set the state to new inside of Reconnect()? And why not set_writable(false) inside of Reconnect as well? https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:551: sig = rtc::SE_CLOSE; Shouldn't we have a unit test for dtlstransportchannel that tests low-level things like this?
Patchset #5 (id:220001) has been deleted
PTAL. https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:151: bool use_alternate_key) { On 2015/11/30 20:23:10, pthatcher1 wrote: > Why not just CreateClientWithAlternateKey and then leave off the > "use_alternate_key" variable and just assume it's true? I think this will duplicate the code in this function, which I'm trying to avoid. Let me know if I miss something. https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:987: // during the destructing time. On 2015/11/30 20:23:11, pthatcher1 wrote: > which => on which > dependency => a dependency > during the destructing time => when destroyed Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1135: } On 2015/11/30 20:23:11, pthatcher1 wrote: > These two methods aren't used any more. Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1152: rtc::scoped_ptr<PeerConnectionTestClient> receiving_client_; On 2015/11/30 20:23:10, pthatcher1 wrote: > Usually, we leave "foo_" members private and then have a protected "Foo& foo()". but we'll be doing this foo() = new client(): Unless we have the swaping functions again? Since I want to keep the old client? https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1261: initializing_client()->IceRestart(); On 2015/11/30 20:23:10, pthatcher1 wrote: > It looks like we are testing the scenario where ICE + DTLS are both restarted. > Should we also test the case where DTLS is restarted but not ICE? Can we do that? Since we don't do DTLS rekeying, I'm not sure how we could only restart DTLS. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... File talk/session/media/channel.cc (right): https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:283: dtls_srtp_setup_pending_ = true; On 2015/11/30 20:23:11, pthatcher1 wrote: > This is kind of hard to read and understand. In the case where we are writable > already, UpdateWritableState_w() below will cause SRTP to be setup. In the > case where it's not, it won't be. But that's not very clear. It doesn't help > that this line is far away from that line. > > This is already on the worker thread, so no packets will be sent while we're in > this code block. I think it would be safer to: > > 1. Move this line down to write above UpdateWritableState_w() below. > 2. Rename it to "srtp_ready_ = false". > 3. srtp_filter_.ResetParams(); here as well. > 4. Put a comment indicating that UpdateWritableState_w will get SRTP ready if > it's writable, or that it will get it ready once it is writable. Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:327: dtls_srtp_setup_pending_ = true; On 2015/11/30 20:23:11, pthatcher1 wrote: > Same here as with set_transport_channel() Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:431: !dtls_srtp_setup_pending(); On 2015/11/30 20:23:11, pthatcher1 wrote: > I think we need this to be something like: > > was_ever_writable() && (srtp_ready() || !NeedSrtp()) > > to work correctly with SCTP data channels. Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:488: if (!ShouldSetupDtlsSrtp()) { On 2015/11/30 20:23:11, pthatcher1 wrote: > I think it would make it more readable to rename ShouldSetupDtlsSrtp to > NeedSrtp(). NeedSrtp doesn't capture the Dtls concept. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:493: // Resetting the srtp filter as long as it's not the CONNECTED signal. For On 2015/11/30 20:23:11, pthatcher1 wrote: > CONNECTED signal => CONNECTED state Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:494: // CONNECTED signal, setting up DTLS-SRTP context is deferred to On 2015/11/30 20:23:11, pthatcher1 wrote: > For CONNECTED signal => For the connected state. Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.cc:809: TrySetupDtlsSrtp_w(); On 2015/11/30 20:23:11, pthatcher1 wrote: > Why is this TrySetupDtlsSrtp_w() instead of SetupDtlsSrtp_w() or > MaybeSetupDtlsSrtp_w()? Done. https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... File talk/session/media/channel.h (right): https://codereview.webrtc.org/1453523002/diff/200001/talk/session/media/chann... talk/session/media/channel.h:321: bool dtls_srtp_setup_pending_; On 2015/11/30 20:23:11, pthatcher1 wrote: > It feels like this should go right under srtp_filter_. Done. https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:203: // multiple times. On 2015/11/30 20:23:11, pthatcher1 wrote: > When we have local => Once we have the local > could be => can be Done. https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:238: set_dtls_state(DTLS_TRANSPORT_NEW); On 2015/11/30 20:23:11, pthatcher1 wrote: > Why do we set the state to new and then assert the state is new in Reconnect()? > Why not just set the state to new inside of Reconnect()? > > And why not set_writable(false) inside of Reconnect as well? Done. https://codereview.webrtc.org/1453523002/diff/200001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:551: sig = rtc::SE_CLOSE; On 2015/11/30 20:23:11, pthatcher1 wrote: > Shouldn't we have a unit test for dtlstransportchannel that tests low-level > things like this? Yes, but I'd like to add it in next CL.
lgtm, with nits https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1152: rtc::scoped_ptr<PeerConnectionTestClient> receiving_client_; On 2015/12/01 22:05:12, guoweis wrote: > On 2015/11/30 20:23:10, pthatcher1 wrote: > > Usually, we leave "foo_" members private and then have a protected "Foo& > foo()". > > but we'll be doing this > > foo() = new client(): > > Unless we have the swaping functions again? Since I want to keep the old client? > Then add set_foo and use it: set_foo(new_client()). https://codereview.webrtc.org/1453523002/diff/200001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:1261: initializing_client()->IceRestart(); On 2015/12/01 22:05:12, guoweis wrote: > On 2015/11/30 20:23:10, pthatcher1 wrote: > > It looks like we are testing the scenario where ICE + DTLS are both restarted. > > > Should we also test the case where DTLS is restarted but not ICE? > > Can we do that? Since we don't do DTLS rekeying, I'm not sure how we could only > restart DTLS. Yeah, good point. In ORTC land, we disallow there being more than one DtlsTransport per IceTransport and also restarting DTLS. So, really, we should have implemented this as creating a new DtlsTransportChannel, not as restarting a DtlsTransportChannel. At least, that's how we'll have to refactor it some day. https://codereview.webrtc.org/1453523002/diff/260001/talk/session/media/chann... File talk/session/media/channel.cc (right): https://codereview.webrtc.org/1453523002/diff/260001/talk/session/media/chann... talk/session/media/channel.cc:252: // For DTLS/SRTP case, the SrtpFilter should be set up by the certificate Would be more clear as: "When using DTLS-SRTP, we must reset the SrtpFilter every time the transport changes and wait until the DTLS handshake is complete to set the newly negotiated parameters." https://codereview.webrtc.org/1453523002/diff/260001/talk/session/media/chann... talk/session/media/channel.cc:328: << "setting RTCP for DTLS/SRTP after SrtpFilter is active " setting => Setting https://codereview.webrtc.org/1453523002/diff/260001/talk/session/media/chann... talk/session/media/channel.cc:494: // Reset the srtp filter if it's not the CONNECTED state. For CONNECTED For CONNECTED state => For the CONNECTED state https://codereview.webrtc.org/1453523002/diff/260001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/260001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:203: // multiple times. have local certificate => have the local certificate
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1453523002/#ps300001 (title: "Remove unused code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453523002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453523002/300001
The CQ bit was unchecked by guoweis@webrtc.org
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.chromium.org/1453523002/#ps320001 (title: "Remove empty line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453523002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453523002/320001
The CQ bit was unchecked by guoweis@webrtc.org
The CQ bit was checked by guoweis@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453523002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453523002/320001
juberti@google.com changed reviewers: + juberti@google.com
https://codereview.webrtc.org/1453523002/diff/320001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:152: bool use_alternate_key) { Instead of using a magic bool value, it seems like it would be simplest to inject a DtlsIdentityStore (or perhaps a SslCertificate, since DtlsIdentityStore is going away). https://codereview.webrtc.org/1453523002/diff/320001/talk/session/media/chann... File talk/session/media/channel.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/talk/session/media/chann... talk/session/media/channel.cc:500: if (state != DTLS_TRANSPORT_CONNECTED) { Shouldl this only happen on DTLS_TRANSPORT_NEW? https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:547: // If the SSL stream has closed remotely, reset the |sig| to be SE_CLOSE Won't we get a SE_CLOSE anyway after the SE_READ? https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:640: channel_->Connect(); Why do we need to call Connect() here? We didn't do this before.
The CQ bit was unchecked by guoweis@webrtc.org
https://codereview.webrtc.org/1453523002/diff/320001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:152: bool use_alternate_key) { On 2015/12/02 00:29:43, juberti wrote: > Instead of using a magic bool value, it seems like it would be simplest to > inject a DtlsIdentityStore (or perhaps a SslCertificate, since DtlsIdentityStore > is going away). Will do. https://codereview.webrtc.org/1453523002/diff/320001/talk/session/media/chann... File talk/session/media/channel.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/talk/session/media/chann... talk/session/media/channel.cc:500: if (state != DTLS_TRANSPORT_CONNECTED) { On 2015/12/02 00:29:43, juberti wrote: > Shouldl this only happen on DTLS_TRANSPORT_NEW? NEW is the init state so it's never signaled. CONNECTING is the one that we first see. However, in reality, FAILED, CLOSE should also reset the state here anyway, don't we? https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:547: // If the SSL stream has closed remotely, reset the |sig| to be SE_CLOSE On 2015/12/02 00:29:43, juberti wrote: > Won't we get a SE_CLOSE anyway after the SE_READ? This crashes randomly after many repeated runs so it seems a timing issue. My plan was to land this first and then write a test case to prevent the transfer test cases become flaky. I don't have full investigation why this crashes at this point. Let me know if you want me to back this one out though. https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:640: channel_->Connect(); On 2015/12/02 00:29:44, juberti wrote: > Why do we need to call Connect() here? We didn't do this before. Indeed, the tranfered endpoint has always maintained writable for its |channel_|. However, I'm wondering if we should also handle the case when it becomes unwritable?
PTAL. https://codereview.webrtc.org/1453523002/diff/320001/talk/app/webrtc/peerconn... File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/talk/app/webrtc/peerconn... talk/app/webrtc/peerconnection_unittest.cc:152: bool use_alternate_key) { On 2015/12/02 18:44:15, guoweis wrote: > On 2015/12/02 00:29:43, juberti wrote: > > Instead of using a magic bool value, it seems like it would be simplest to > > inject a DtlsIdentityStore (or perhaps a SslCertificate, since > DtlsIdentityStore > > is going away). > > Will do. Done. https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:547: // If the SSL stream has closed remotely, reset the |sig| to be SE_CLOSE On 2015/12/02 18:44:15, guoweis wrote: > On 2015/12/02 00:29:43, juberti wrote: > > Won't we get a SE_CLOSE anyway after the SE_READ? > > This crashes randomly after many repeated runs so it seems a timing issue. My > plan was to land this first and then write a test case to prevent the transfer > test cases become flaky. I don't have full investigation why this crashes at > this point. > > Let me know if you want me to back this one out though. I'm backing this out. This still crashes but with the change of when we reset DTLS, it seems less likely now. I'll make the change later with a test case. https://codereview.webrtc.org/1453523002/diff/320001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.cc:640: channel_->Connect(); On 2015/12/02 18:44:15, guoweis wrote: > On 2015/12/02 00:29:44, juberti wrote: > > Why do we need to call Connect() here? We didn't do this before. > > Indeed, the tranfered endpoint has always maintained writable for its > |channel_|. However, I'm wondering if we should also handle the case when it > becomes unwritable? Right, we don't need this part. Removing it.
Patchset #11 (id:360001) has been deleted
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1453523002/#ps380001 (title: "add back the handling of the EOS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453523002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453523002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/10945)
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1453523002/#ps420001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453523002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453523002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by guoweis@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453523002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453523002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...)
The CQ bit was checked by guoweis@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by guoweis@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453523002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453523002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/5691)
Description was changed from ========== Allow remote fingerprint update during a call Changes include the following 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS (not SDES). 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 ========== to ========== Allow remote fingerprint update during a call Changes include the following 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS (not SDES). 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9c38c2d33fa6d794704d53b18... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:420001) manually as 9c38c2d33fa6d794704d53b18f39d5235439fe63 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Allow remote fingerprint update during a call Changes include the following 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS (not SDES). 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9c38c2d33fa6d794704d53b18... ========== to ========== Allow remote fingerprint update during a call Changes include the following 1. modify FakeDtlsIdentityStore to support alternate certificate so we could have a different fingerprint in test case. 2. dtlstransportchannel can accept a new fingerprint and trigger DTLS handshake. 3. #2 will trigger new signal on the media side to reset SRTP context. Only reset SRTP context when we are using DTLS (not SDES). 4. Test cases for caller or callee are transfees. BUG=webrtc:3618 R=pthatcher@webrtc.org Committed: https://crrev.com/9c38c2d33fa6d794704d53b18f39d5235439fe63 Cr-Commit-Position: refs/heads/master@{#10901} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/9c38c2d33fa6d794704d53b18f39d5235439fe63 Cr-Commit-Position: refs/heads/master@{#10901} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
