algesten / str0m

A Sans I/O WebRTC implementation in Rust.
MIT License
317 stars 49 forks source link

SDP negotiation now considers ICE Lite when determining ICE role #519

Closed OxleyS closed 4 months ago

OxleyS commented 4 months ago

Resolves https://github.com/algesten/str0m/issues/513. However, it does not make Lite-to-Lite connections fully functional - details below.

RFC 5245 says:

However, behavior prior to this PR would always consider the initial offerer to be controlling, irrespective of ICE Lite. This discrepancy caused role confusion and thus connection issues when the initial offerer was ICE Lite, regardless of that offerer being str0m itself or another client.

The RFC's logic can be implemented quite directly in our SDP negotiation code. The only bit of complication is in the Lite-to-Lite case, where we initially set ourselves as controlled when offering, but have to switch roles once we learn the remote peer is also ICE Lite via its SDP answer.

With this, Full-to-Full and Full-to-Lite should be fully functional in both offering directions. Lite-to-Lite will at least have the correct role set now, but there are other deficiencies in the ICE implementation that keep this case non-functional, namely the clash between the code both requiring connectivity checks and being unable to perform them. I haven't had time to dig deep into these issues yet, so I can't point out specific code. Since this case is exceedingly rare, though, resolving issues for the much more common Full-to-Lite case is more immediately valuable.

algesten commented 4 months ago

Thanks for looking into this!

resolving issues for the much more common Full-to-Lite case is more immediately valuable.

For my understanding: the issues with Full-to-Lite in the current code is that str0m would assume controlling role also as Lite, if it is the first offerer?

algesten commented 4 months ago

Lite-to-Lite will at least have the correct role set now, but there are other deficiencies in the ICE implementation that keep this case non-functional, namely the clash between the code both requiring connectivity checks and being unable to perform them.

This is happening because str0m in ice-lite mode will not make connectivity checks. In the Lite-to-Lite scenario, it sounds like the offerer must not only become controlling, but also take the Full role to start connectivity checks.

algesten commented 4 months ago

Answering myself:

In the Lite-to-Lite scenario, it sounds like the offerer must not only become controlling, but also take the Full role to start connectivity checks.

The RFC says:

      *  The agent adds the selected pair for each component to the
         valid list.  As described in [Section 11.1](https://datatracker.ietf.org/doc/html/rfc5245#section-11.1), this will permit
         media to begin flowing.  However, it is possible (and in fact
         likely) that both agents have chosen different pairs.

      *  To reconcile this, the controlling agent MUST send an updated
         offer as described in [Section 9.1.3](https://datatracker.ietf.org/doc/html/rfc5245#section-9.1.3), which will include the
         remote-candidates attribute.

Ergo. Each side should just pick a candidate and assume that will work without connectivity checks, then make an additional SDP dance to reconcile any problems.

That sounds like a lot of work for a fringe edge case. I suggest we state str0m does not support Lite-to-Lite and throw an Error to reject the OFFER/ANSWER when we detect this situation. We document the deviation making a new section here: https://github.com/algesten/str0m/blob/main/docs/ice.md#ice-lite-stays-ice-lite

Thoughts?

OxleyS commented 4 months ago

For my understanding: the issues with Full-to-Lite in the current code is that str0m would assume controlling role also as Lite, if it is the first offerer?

Yes, in this scenario you would end up with both sides claiming to be the controlling role. But also going the other way, if a Lite client makes an initial offer to Full str0m, with the current code they would both claim to be the controlled role.

That sounds like a lot of work for a fringe edge case. I suggest we state str0m does not support Lite-to-Lite and throw an Error to reject the OFFER/ANSWER when we detect this situation.

I definitely agree. Even if we did implement support for it, I can see it being very easy to accidentally break without anybody noticing. As long as we explicitly check for and reject this case as you suggest, it seems reasonable to not support it. Shall I modify the PR to do so?

algesten commented 4 months ago

I definitely agree. Even if we did implement support for it, I can see it being very easy to accidentally break without anybody noticing. As long as we explicitly check for and reject this case as you suggest, it seems reasonable to not support it. Shall I modify the PR to do so?

Yes please!

OxleyS commented 4 months ago

Oops, I forgot to update the doc. I'll do that now.