w3c / webrtc-pc

WebRTC 1.0 API
https://w3c.github.io/webrtc-pc/
Other
437 stars 115 forks source link

When ICE restarts results in connection to a new endpoint #1406

Closed aboba closed 7 years ago

aboba commented 7 years ago

From fippo in Issue https://github.com/w3c/webrtc-pc/issues/1357:

In the case of the remote fingerprint changing, when does the JS layer learn about the new DTLS transport object? The first chance is after SRD and I would also expect it to be available in onselectedcandidatepairchange.

I would expect the ice transport not to change during an ice restart. This is also consistent with the ability to query past candidates in getStats. This gets somewhat awkward in the case of the remote fingerprint changing :-/

aboba commented 7 years ago

@fippo It makes sense that the changes would become visible after SRD. However, exactly what happens is not well specified at present. For example, does the RTCDtlsTransport remain the same and make an (odd) transition back to "new"?

Or does the old RTCDtlsTransport.state transition to "closed", along with a new RTCDtlsTransport created in the "new" state? If so, does the onstatechange EventHandler for the old RTCDtlsTransport get moved to the new RTCDtlsTransport automagically, or does the application somehow need to figure out that the RTCDtlsTransport has changed and set the EventHandler itself?

aboba commented 7 years ago

Related Issue https://github.com/w3c/webrtc-pc/issues/1178

fippo commented 7 years ago

For example, does the RTCDtlsTransport remain the same and make an (odd) transition back to "new"?

I think there should be a new DTLS transport object since the internal keying material on a DTLS transport is immutable once set. Also packets on the old dtls transport still need to be decrypted ( spec pointer from this old comment here Ideally at least, getting this use-case 100% seamless might be too ambitious.

@taylor-b maybe you have a chance to look at this bug again while figuring out how your lib handles this?

But creating a new object makes things worse for RTPSender and RTPReceiver:

If so, does the onstatechange EventHandler for the old RTCDtlsTransport get moved to the new RTCDtlsTransport automagically, or does the application somehow need to figure out that the RTCDtlsTransport has changed and set the EventHandler itself?

I think the application should remove and add a statechange listener on every setRemoteDescription. No magic please.

taylor-b commented 7 years ago

@taylor-b maybe you have a chance to look at this bug again while figuring out how your lib handles this?

I don't think much has changed. We don't do anything to honor the "each DTLS association must use a new ICE session" rule, and we definitely don't handle corner cases with "pranswer"s well. But I consider those bugs.

But creating a new object makes things worse for RTPSender and RTPReceiver

If creating a new object makes things more complex without any tangible benefit, then maybe it's not worth it, even if it's the correct object model.

ekr commented 7 years ago

To recap my comments on the call: I don't think it makes sense to treat ICE restarts as overwriting the contents of the existing ICE transport as opposed to creating a new ICE transport. During a restart, you actually conceptually have two ICE contexts (different ufrag/password, candidate pairs, etc.) so it seems very misleading to have the old ICE transport somehow become the new one especially when you are doing both concurrently. Rather, there should be two ICE transports.

aboba commented 7 years ago

For reference, in ORTC an ICE restart can be achieved either by constructing a new RTCIceTransport, or by calling start() again with new values of gatherer and remoteParameters, in which case "local candidates are flushed, remote candidates are flushed, candidate pairs are flushed and state transitions to new." The RTCDtlsTransport constructor takes an RTCIceTransport as an argument, and there is no method to switch an RTCDtlsTransport to another RTCIceTransport. Therefore, constructing a new RTCIceTransport implies construction of a new RTCDtlsTransport as well, whereas calling start() again with a new RTCIceGatherer and remoteParameters does not.

taylor-b commented 7 years ago

I think we should avoid getting seduced by the names of things, and saying "'ICE transport' sounds like 'ICE context', so there should be two of them." We can define "ICE transport" to mean whatever we want. It can be "a transport that the ICE agent provides for one component of a media session", or it can be "an ICE checklist and associated candidates for an individual ICE 'generation'". Which one we choose should be decided by practical reasons. So what are the practical implications of each choice?

One object across restarts

I can't think of any problems an application could have with this. Is there something I overlooked?

Two objects across restarts

So, unless I'm missing something, it seems like having two objects would be introducing additional complexity for no practical benefit. I could be convinced, but I'd like to hear what the benefits actually are.

aboba commented 7 years ago

@taylor-b I believe that two objects would imply that a new DtlsTransport is constructed with newDtlsTransport.transport being the new IceTransport. Meanwhile, dtlsTransport.transport represents the old IceTransport. Until the newDtlsTransport comes into existence (after the answer is applied?) it's not clear to me how any information about the new Dtls/IceTransports can be accessed.

taylor-b commented 7 years ago

@aboba So even if you keep using the same DTLS association, you get a new RTCDtlsTransport? I guess it's an option, but it just shifts the problem up a level (instead of "when does dtlsTransport.[ice]transport change?" it's "when does rtpSender.[dtls]transport change?"), and it's also stranger conceptually.

aboba commented 7 years ago

From https://www.w3.org/2017/06/28-webrtc-minutes

For DTLS, we agreed that we would create a new object when a new DTLS association is created (a=tls-id change, for instance)

taylor-b commented 7 years ago

The resolution in the interim was "needs more discussion on github". I explained my thoughts about this (in the big Jun 29 comment), we still need @ekr to weigh in.

aboba commented 7 years ago

Discussion on the MMUSIC list as to whether an ICE restart necessitates a new DTLS connection: https://www.ietf.org/mail-archive/web/mmusic/current/msg18121.html

stefhak commented 7 years ago

At the September VI we decided to go with the "one object" model (https://github.com/w3c/webrtc-pc/issues/1406#issuecomment-311916547).