Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response to connectivity checks prior to calling iceTransport.start()? #170

Open
aboba opened this issue Jan 9, 2015 · 24 comments
Open
Labels

Comments

@aboba
Copy link
Contributor

aboba commented Jan 9, 2015

The question has been asked whether an RTCIceGatherer object can respond to incoming ICE connectivity checks prior to calling iceTransport.start() or even constructing an RTCIceTransport.

It seems to me that the RTCIceGatherer has the information needed to respond to incoming ICE connectivity checks (the local ufrag/password), but does NOT have the information needed to initiate its own connectivity checks (the remote ufrag/password).

However, having an RTCIceGatherer respond to incoming connectivity checks before iceTransport.start() is called is tricky because it means that outgoing connectivity checks will not be triggered until iceTransport.start() is eventually called, and doing so in a timely way may require the RTCIceGatherer to provide information from the incoming checks to the RTCIceTransport once iceTransport.start() is called.

Here is some proposed text for Section 5.1:

An RTCIceGatherer MAY respond to connectivity checks before an RTCIceTransport is constructed or iceTransport.start() is called. However, connectivity checks are only initiated by the RTCIceTransport, after iceTransport.start() is called.

@aboba aboba added the 1.1 label Jan 11, 2015
@aboba
Copy link
Contributor Author

aboba commented Jan 11, 2015

Here is an update to the proposed text for Section 5.1:

An RTCIceGatherer instance is associated to an RTCIceTransport. As noted in [RFC5245] Section 7.1.2.3, an incoming connectivity check utilizes the remote ufrag and the local password, whereas an outgoing connectivity check utilizes the local ufrag and the remote password. Therefore an RTCIceGatherer possesses the information necessary to validate incoming connectivity checks and may respond to them before an RTCIceTransport is constructed or iceTransport.start() is called.

Since initiating connectivity checks requires the remote password, an RTCIceTransport can only initiate checks after iceTransport.start() is called. To enable an RTCIceTransport to initiate connectivity checks to peers that had previously sent connectivity checks, or to indicate to an RTCIceTransport that a connectivity check had previously been received (and whether it was responded to), the RTCIceGatherer must store information from incoming connectivity checks (such as the remote ufrag) along with an indication of whether it responded, and provide this to associated RTCIceTransport objects so that they can respond if the RTCIceGatherer did not, and can initiate their own connectivity checks.

@robin-raymond
Copy link
Contributor

I agree with the idea. The only issue is "may". If we do have some people support pre-transport check answers directly from the gatherer it could cause some implementations to have different behavioural patterns than others. I know it was not desirable to have different levels of API expectations. Is anyone concern about this particular "may" case and the resulting behavioural difference between an engine that does support answering and one which does not.

robin-raymond pushed a commit that referenced this issue Jan 22, 2015
#48

Update to the Statistics API, reflecting:
#85

Update on 'automatic' use of scalable video coding, as noted in:
#156

Update to the H.264 parameters, as noted in:
#158

Update to the 'Big Picture', as noted in:
#159

Added support for maxptime, as noted in:
#160

Changed 'RTCIceTransportEvent' to 'RTCIceGathererEvent' as noted in:
#161

Update to RTCRtpUnhandledEvent as noted in:
#163

Added support for RTCIceGatherer.state as noted in:
#164

Revised the text relating to RTCIceTransport.start() as noted in:
#166

Added text relating to DTLS interoperability with WebRTC 1.0, as noted in:
#167

Revised the text relating to RTCDtlsTransport.start() as noted in:
#168

Clarified handling of incoming connectivity checks by the RTCIceGatherer as noted in:
#170

Added a reference to the ICE consent specification, as noted in:
#171
@aboba
Copy link
Contributor Author

aboba commented Jan 28, 2015

At the ORTC CG meeting of January 27, 2015, there appeared to be consensus to change the "MAY" to a "MUST" so as to have consistent behavior. Text in Section 5.1 would now read as follows:

"As noted in [RFC5245] Section 7.1.2.3, an incoming connectivity check utilizes the remote ufrag and the local password, whereas an outgoing connectivity check utilizes the local ufrag and the remote password. Therefore an RTCIceGatherer possesses the information necessary to validate incoming connectivity checks and MUST respond to them before an RTCIceTransport is constructed or iceTransport.start() is called.

Since initiating connectivity checks requires the remote password, an RTCIceTransport can only initiate checks after iceTransport.start() is called. To enable an RTCIceTransport to initiate connectivity checks to peers that had previously sent connectivity checks, the RTCIceGatherer must store information from incoming connectivity checks (such as the remote ufrag), and provide this to associated RTCIceTransport objects so that they can initiate their own connectivity checks."

@aboba
Copy link
Contributor Author

aboba commented Jan 29, 2015

There is a consequence of this, which is that by responding to the connectivity check, the remote peer may believe that it has a functioning ICE transport and can then call DtlsTransport.start(). So DTLS packets can show up before there is even a DtlsTransport to even receive them.

robin-raymond pushed a commit that referenced this issue Mar 25, 2015
- Revised the text relating to RTCDtlsTransport.start(), as noted in: Issue #168
- Clarified pruning of local candidates within the RTCIceGatherer, as noted in: Issue #174
- Clarified handling of incoming connectivity checks by the RTCIceGatherer, as noted in: Issue #170
- Added Section 9.3.2.1, defining DTMF capabilities and settings, as noted in: Issue #177
- Clarified pre-requisites for insertDTMF(), based on: Issue #178
- Added Section 8.3.2 and updated Section 9.5.1 to clarify aspects of RTCP sending and receiving, based on: Issue #180
@aboba
Copy link
Contributor Author

aboba commented Apr 15, 2015

There is a problem with having an IceGatherer respond to connectivity checks. RFC 5245 Section 7.1.2.2 states:

7.1.2.2. ICE-CONTROLLED and ICE-CONTROLLING

The agent MUST include the ICE-CONTROLLED attribute in the request if
it is in the controlled role, and MUST include the ICE-CONTROLLING
attribute in the request if it is in the controlling role. The
content of either attribute MUST be the tie-breaker that was
determined in Section 5.2. These attributes are defined fully in
Section 19.1.

The problem is that an IceGatherer does not know its role, and so it cannot determine if there is a role conflict so as to correctly behave as specified in RFC 5245 Section 7.1.3.1:

7.1.3.1. Failure Cases

If the STUN transaction generates a 487 (Role Conflict) error
response, the agent checks whether it included the ICE-CONTROLLED or
ICE-CONTROLLING attribute in the Binding request. If the request
contained the ICE-CONTROLLED attribute, the agent MUST switch to the
controlling role if it has not already done so. If the request
contained the ICE-CONTROLLING attribute, the agent MUST switch to the
controlled role if it has not already done so. Once it has switched,
the agent MUST enqueue the candidate pair whose check generated the
487 into the triggered check queue. The state of that pair is set to
Waiting. When the triggered check is sent, it will contain an ICE-
CONTROLLING or ICE-CONTROLLED attribute reflecting its new role.
Note, however, that the tie-breaker value MUST NOT be reselected.

Therefore, I don't believe that an IceGatherer can respond to incoming connectivity checks.

@robin-raymond
Copy link
Contributor

Correct, the gatherer can pre-screen failed credential because it's using the local password but cannot send a successful reply because it does not know the correct role until IceTransport.start is called. If the gatherer replied earlier it might have incorrectly responded without indicating the role conflict error case.

@aboba
Copy link
Contributor Author

aboba commented Apr 16, 2015

Here is a proposed resolution:

In Section 5.1, change:

"As noted in [RFC5245] Section 7.1.2.3, an incoming connectivity check utilizes the remote ufrag and the local password, whereas an outgoing connectivity check utilizes the local ufrag and the remote password. Therefore an RTCIceGatherer possesses the information necessary to validate incoming connectivity checks and must respond to them before an RTCIceTransport is constructed or iceTransport.start() is called.

Since initiating connectivity checks requires the remote password, an RTCIceTransport can only initiate checks after iceTransport.start() is called. To enable an RTCIceTransport to initiate connectivity checks to peers that had previously sent connectivity checks, the RTCIceGatherer must store information from incoming connectivity checks (such as the remote ufrag) and provide this to associated RTCIceTransport objects so that they can initiate their own connectivity checks."

To:

"As noted in [RFC5245] Section 7.1.2.2, an incoming connectivity check contains an ICE-CONTROLLING or ICE-CONTROLLED attribute, depending on the role of the ICE agent initiating the check. Since an RTCIceGatherer object does not have a role, it cannot determine whether to respond to an incoming connectivity check with a 487 (Role Conflict) error, and therefore it must not respond to incoming connectivity checks.

To enable an RTCIceTransport to initiate connectivity checks to peers that had previously sent connectivity checks, the RTCIceGatherer must store information from incoming connectivity checks (such as the remote ufrag) and provide this to associated RTCIceTransport objects so that they can validate the incoming connectivity check in order to decide whether to respond and initiate their own connectivity checks once iceTransport.start() is called."

Also, in Section 3.3.2 add:

"As noted in [RFC5245] Section 7.1.2.3, an incoming connectivity check utilizes the remote ufrag and the local password, whereas an outgoing connectivity check utilizes the local ufrag and the remote password. Since start() provides role information as well as the local and remote ufrag/password, once start() is called, an RTCIceTransport object can validate incoming connectivity checks, determine whether an incoming check represents a role conflict and initiate connectivity checks."

@robin-raymond
Copy link
Contributor

If we did allow association a transport prior to calling start, it's possible that an incoming remote ufrag could "latch" onto the transport. But that causes a few race conditions. 1) it's still possible for multiple remote ufrags to come in where there's no local transport setup to attach and it's not always possible to know in advance how many transports would need to be pre-setup to allow latching for all remote ufrags that may arrive (so we are still left with the original problem of what to do). 2) we auto-create transports for non set-up ufrags but 3) there's a race condition where the system auto-latches an incoming ufrag to a transport while the programmer is about to call start() on that transport with a different ufrag/password received via signalling. Even an event saying "transport created/latched to remote ufrag" may not help because of the race between the event and calling start from signalling may be too close in time to prevent mistakes. 4) many scenarios (if not most) will never receive a remote ufrag incoming binding request until start is called due to firewalls anyway.

So my recommendation is to either a) ignore binding requests that come in early and allow re-transmission to solve the issue [not ideal] b) buffer incoming requests whose remote ufrag is not associated until a start is called then deliver those packets to the newly started transport to maximize setup speed.

I do not see an elegant auto-latching solution for non pre-known ufrags that solves the problem...

@aboba
Copy link
Contributor Author

aboba commented Apr 19, 2015

A revision to the proposed text:

In Section 3.3.2:

As noted in [RFC5245] Section 7.1.2.3, an incoming connectivity check utilizes the local/remote ufrag and the local password, whereas an outgoing connectivity check utilizes the local/remote ufrag and the remote password. Since start() provides role information, an RTCIceTransport object can respond to incoming connectivity checks and initiate connectivity checks.

In Section 5.1:

As noted in [RFC5245] Section 7.1.2.2, an incoming connectivity check contains an ICE-CONTROLLING or ICE-CONTROLLED attribute, depending on the role of the ICE agent initiating the check. Since an RTCIceGatherer object does not have a role, it cannot determine whether to respond to an incoming connectivity check with a 487 (Role Conflict) error; however, it can validate that incoming connectivity check utilizes the local ufrag/password, and if not, can respond with an 401 (Unauthorized) error, as described in [RFC5389] Section 10.1.2.

For incoming connectivity checks that pass validation, the RTCIceGatherer MUST buffer the incoming connectivity checks so as to be able to provide them to associated RTCIceTransport objects so that they can respond and initiate their own connectivity checks once iceTransport.start() is called.

@robin-raymond
Copy link
Contributor

I do not think that we will see incoming connectivity checks without calling start under most real-world browser to browser (or client to client) scenarios. That's because most times all incoming connectivity checks will be blocked until outgoing connectivity checks are issued too (even when TURN is present). There are a small subset of firewalls where opening the reflexive pinhole will allow incoming connectivity packets but they are the rarity and not the norm.

Having said that, if we feel handling this cause is important we could:
a) add an event from the RTCIceGatherer "on unhandled remote ufrag" which would give the received remote ufrag and the remote ICE role of the incoming connectivity check.
b) add a RTCIceTransport.earlyStart(iceGatherer, remoteUfrag, remoteIceRole) method to respond to incoming connectivity checks.

There are some consequences though of doing this:

  1. There's still a race condition between receiving the incoming check and the earlyStart method. Thus buffering [however small] is still required
  2. Responding to checks will cause incoming dtls packets to arrive (but fortunately we can go into "connected" state based upon receiving an incoming check and a follow up incoming dtls packet after the check to enable full duplex early).

The question I ask, is it worth it? Given that firewalls will most certainly block this working for client to client and this is more useful for server scenarios where firewalls are not present.

@aboba aboba removed the PR exists label Apr 29, 2015
@robin-raymond
Copy link
Contributor

The only solution to this is to associate an RTPIceGatherer to an RTCIceTransport prior to calling start which would mean incoming non-associated remote username fragment could become auto-attached to the RTCIceTransport prior to calling start.

To know this association has happened, we would need an event on the RTCIceGatherer to know about the incoming remote username fragment. We would also need an attribute RTCIceTransport.usernameFragment on the transport to indicate that attribute got filled and the RTCIceTransport.role would need to contain the correct role as determine by the incoming connectivity check (which could not be overridden by the RTCIceTransport.start() method.

This allows incoming connectivity checks to be responded to immediate if an RTCIceTransport has been attached to the RTCIceGatherer without RTCIceTransport.start() having been called.

If no RTCIceTransport is attached then we need to event about the new remoteUsernameFragment from the RTCIceGatherer as we already do and buffer the incoming connectivity check until a new RTCIceTransport has been attached to the RTCIceGatherer [to become auto-attached post connectivity check] or RTCIceTransport.start is called with the usernameFragement on a non-associated RTCIceTransport with that remote username fragment specified in the RTCIceTransport.start() method.

robin-raymond pushed a commit that referenced this issue May 7, 2015
#148

- Clarified handling of incoming connectivity checks prior to calling iceTransport.start(), as noted in:
#170

- Clarified handling of incoming DTLS packets, as noted in:
#173

- Added RTCIceGatherer as an optional argument to the RTCIceTransport constructor, as noted in:
#174

- Clarified handling of contradictory RTP/RTCP multiplexing settings, as noted in:
#185

- Clarified error handling relating to RTCIceTransport, RTCDtlsTransport and RTCIceGatherer objects in the "closed" state, as noted in:
#186

- Added component method and createAssociatedGatherer() method to the RTCIceGatherer object, as noted in:
#188

- Added close() method to the RTCIceGatherer object as noted in:
#189

- Clarified behavior of TCP candidate types, as noted in:
#190

- Clarified behavior of iceGatherer.onlocalcandidate, as noted in:
#191

- Updated terminology in Section 1.1 as noted in:
#193

- Updated RTCDtlsTransportState definitions, as noted in:
#194

- Updated RTCIceTransportState definitions, as noted in:
#197
@robin-raymond
Copy link
Contributor

@pthatcher proposed on the last CG to add a factory method like RTCIceTransport.createOrObtainExistingTransport(remoteUFrag) to solve the race that can happen when an ice transport is auto-filled at the same moment when an ice transport is being assigned a remote ufrag/password.

@aboba
Copy link
Contributor Author

aboba commented May 26, 2015

How does this work? For example:

// Example to demonstrate forking when RTP and RTCP are not multiplexed,
// so that both RTP and RTCP IceGatherer and IceTransport objects are needed. 
// Create ICE gather options
var gatherOptions = new RTCIceGatherOptions(); 
gatherOptions.gatherPolicy = RTCIceGatherPolicy.relay; 
gatherOptions.iceservers = ... ; 
// Create ICE gatherer objects
var iceRtpGatherer = new RTCIceGatherer(gatherOptions);
var iceRtcpGatherer = iceRtpGatherer.createAssociatedGatherer(); 
// Create the ICE RTP and RTCP transports
var iceRtpTransport = new RTCIceTransport(iceRtpGatherer);
var iceRtcpTransport = iceRtpTransport.createAssociatedTransport();
// Prepare to handle incoming connectivity checks
iceRtpGatherer.onunhandledcheck = function (event){iceRtpTransport.respondToIncomingChecks(iceRtpGatherer,event.remoteUfrag,event.remoteRole);}; 
iceRtcpGatherer.onunhandledcheck = function (event) {iceRtcpTransport.respondToIncomingChecks(iceRtcpGatherer,event.remoteUfrag,event.remoteRole);}; 
// Prepare to signal local candidates
iceRtpGatherer.onlocalcandidate = function (event) {mySendLocalCandidate(RTCIceComponent.RTP, event.candidate)}; 
iceRtcpGatherer.onlocalcandidate = function (event) {mySendLocalCandidate(RTCIceComponent.RTCP, event.candidate)}; 

mySendInitiate({
   "icertp": iceRtpGatherer.getLocalParameters(),
   "icertcp": iceRtcpGatherer.getLocalParameters()
 },
 function(response) {
  // Assume we are only getting a single response...
  // Create the ICE Transport Controller object and add the RTP Ice Transport to it
  var controller = new RTCIceTransportController();
  controller.addTransport(iceRtpTransport);
  // It is possible that the RTP and/or RTCP ICE transports were "early started" so do not call start if
  // the role is already set. 
  if (!iceRtpTransport.role) {iceRtpTransport.start(iceRtpGatherer, response.icertp, RTCIceRole.controlling);};
  if (!iceRtcpTransport.role){iceRtcpTransport.start(iceRtcpGatherer, response.icertcp, RTCIceRole.controlling);}; 
  // Prepare to add ICE candidates signalled by the remote peer
  mySignaller.onRemoteCandidate = function(remote) {
  if (remote.component == RTCIceComponent.RTP){
     iceRtpTransport.addRemoteCandidate(remote.candidate);
  } else {
      iceRtcpTransport.addRemoteCandidate(remote.candidate);
  };
 }

 // ... setup DTLS, RTP, SCTP, etc.
});

function mySendLocalCandidate(component, candidate){
   mySignaller.mySendLocalCandidate({
   "candidate": candidate,
   "component": component
   });
}

@robin-raymond
Copy link
Contributor

I see potential problem with RTCIceTransport.createOrObtainExistingTransport(remoteUFrag).

var iceGatherer = new RTCIceGatherer(...);
var iceTransport = new RTCIceTransport(iceGatherer,...);
var dtlsTransport = new RTCDtlsTransport(iceTransport,...)

signalWithBob.sendGathererInfo(aliceGatherer);

signalWithBob.onResponse = function(var data) {
  // NOTE: Uncertain if the previous iceTransport auto-latched from an incoming connectivity
  // check or if a new transport will be needed.
  var useIceTransport = RTCIceTransport.createOrObtainExistingTransport(data.remoteUsernameFrag);

  if (useIceTransport != iceTransport) {
     // brand new ice transport was created thus need to create new DTLS transport...

     // PROBLEM: If an ICE connectivity check arrives during this time for this remote username
     // fragment then the ICE gatherer will auto-respond and latch and incoming DTLS packets can
     // arrive immediately, i.e. BEFORE the new dtlsTransport is constructed. This is a small race
     // window but it exists. Thus the saving of buffering in the IceGatherer is at the expense of
     // having to buffer within the IceTransport instead.

     var dtlsTransport = new RTCDtlsTransport(useIceTransport);
  }

  useIceTransport.start(...);
};

@pthatcherg
Copy link

FYI, I posted the proposal for IceGatherer.getOrCreateTransport to the list, and have gotten no response so far.

I think we should proceed with it. I think the issue around receiving a DTLS packet is serious enough to block this. Worst case is that while doing call forking, the DTLS setup takes a little longer, or that we would maybe buffer a few DTLS packets in the ICE transport, neither of which is a very big deal long-term.

@robin-raymond
Copy link
Contributor

@pthatcher created slides on that for today's CG. I agree with it.

@aboba
Copy link
Contributor Author

aboba commented Jun 29, 2015

@aboba
Copy link
Contributor Author

aboba commented Jun 30, 2015

Applying Peter's proposal to the sample code, and found something that bugs me:
// These lines occur while processing the response to an offer:
var rtpTransport = iceRtpGatherer.getOrCreateTransport(response.icertp.usernameFragment);
var rtcpTransport = iceRtcpGatherer.getOrCreateTransport(response.icertcp.usernameFragment);

In the case where an RTCP transport is created, how can getOrCreateTransport know the RTP IceTransport to associate it with?

Do we really want getOrCreateAssociatedTransport ??

@robin-raymond
Copy link
Contributor

Because of issue #223 , the IceGatherer.createAssociatedGatherer() must share the same ufrag for RTCP as the base RTP gatherer. As such, the remote ufrag must also have the same behaviour so only the base RTP has a race condition where you could be auto-latched for the RTP IceTransport while you are trying to create a new one. The race condition for the RTCP can be resolved by being able to get the associated transport from the transport returned by IceGatherer.getOrCreateTransport(remoteUFrag).

The problem is that if you pre-created a bunch of RTP and RTCP transports to auto-latch at the start, you'd have to do a look in your own list of pre-created RTP ice transport to find the existing RTP transport that was returned by IceGatherer.getOrCreateTransport(remoteUFrag) to find the previously created RTCP transport if it returned an existing RTP transport so you could get the existing RTCP transport.

A better solution would be to change IceTransport.createAssociatedTransport() to IceTransport.getOrCreateAssociatedTransport() so you would not have to do the entire lookup of previous RTP ice transport just for the sake of getting an existing RTCP ice transport.

So IceTransport.createAssoicatedTransport() gets changed to IceTransport.getOrCreateAssociatedTransport() which returns the previously created one if one was previously created or a new one if one was not created. That solves that nasty little lookup problem.

The same niceness could be done for the ice gatherer by renaming IceGatherer.createAssociatedGatherer() to IceGatherer.getOrCreateAssociatedGather() so you don't have to do lookups to match your rtp and rtcp gatherers together.

Finally, the RTP gatherer / transport objects should own the RTCP gatherer / transport objects so they would not be garbage collected independent from RTP so your programmer's code would not even have to remember the related RTCP ice gatherer/transport at all (simplifying a lot of code that uses both).

For the auto-latch process on the RTCP to work, the IceGatherer.createAssociateGatherer() must be called first because IceTransport.createAssoicatedTransport() is not passed an IceGatherer to auto-latch.

@robin-raymond
Copy link
Contributor

Alt: add attribute of IceTransport.associatedTransport and IceGather.associatedGatherer instead of renaming IceTransport.createAssociatedTransport() and IceGatherer.createAssociatedGatherer()

@robin-raymond
Copy link
Contributor

RTP should own RTCP object, not the other way around. So keeping RTP transport would keep RTCP from getting garbage collected.

@aboba
Copy link
Contributor Author

aboba commented Jun 30, 2015

The behavior of rtcpIceGatherer.getOrCreateTransport() is under-specified. Here is a proposed clarification:

getOrCreateTransport
Retrieve the associated RTCIceTransport with a matching remoteUsernameFragment value, or create one from scratch. If rtcpIceGatherer.getOrCreateTransport() is called and rtcpIceGatherer.component is "RTCP", and no RTCIceTransport associated with rtcpIceGatherer matches, locate the RTCIceGatherer rtpIceGatherer from which rtcpIceGatherer was created, then attempt to locate an RTCIceTransport with matching remoteUsernameFragment associated with rtpIceGatherer. If one is found, then rtpIceGatherer.getOrCreateTransport().createAssociatedTransport() is called implicitly; if it is not found, then an "InvalidStateError" exception is thrown.

@robin-raymond robin-raymond added 1.2 and removed 1.1 labels Jul 13, 2016
@robin-raymond
Copy link
Contributor

Basic connectivity check issue is fixed but the advanced stuff was not completed (yet).

@aboba
Copy link
Contributor Author

aboba commented Jul 21, 2016

We can solve the basic connectivity check issue with buffering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants