Implement handling of non-actpass setup in offers #1090
Conversation
fippo
force-pushed
the
active-passive-who-is-not-confused-by-this
branch
from
b57992b
to
0b82548
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1090 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 30 30
Lines 5868 5868
=========================================
Hits 5868 5868 ☔ View full report in Codecov by Sentry. |
fippo
force-pushed
the
active-passive-who-is-not-confused-by-this
branch
3 times, most recently
from
4c4fa9a
to
1609289
Compare
| if description.type == "offer" and media.dtls.role == "client": | ||
| dtlsTransport._set_role(role="server") | ||
| if description.type == "answer": | ||
| dtlsTransport._set_role( | ||
| role="server" if media.dtls.role == "client" else "client" |
There was a problem hiding this comment.
Btw what role do we end up with when none of these conditionals match?
There was a problem hiding this comment.
auto which I hope means "determined by who wins ICE"
There was a problem hiding this comment.
I'm not sure I understand why we are only handling media.dtls.role == "client"?
There was a problem hiding this comment.
This is the handling of offers so the existing code assumes actpass. Lets see if I can prove that by adding more tests because who does not like more tests ;-)
There was a problem hiding this comment.
As I suspected... the current code assumes role=auto in SRD. Which is not quite correct since auto means "determined by DTLS" whereas when we parse actpass we should be setting this to "client" explicitly. This is currently done in createAnswer which is a bit confusing but ok.
| @@ -1272,9 +1274,6 @@ def __validate_description( | |||
| if not media.ice.usernameFragment or not media.ice.password: | |||
| raise ValueError("ICE username fragment or password is missing") | |||
|
|
|||
| # check DTLS role is allowed | |||
| if description.type == "offer" and media.dtls.role != "auto": | |||
There was a problem hiding this comment.
Do we need to also remove raising error in __validate_description when peer's role in peer's offer is not actpass?
aiortc/src/aiortc/rtcpeerconnection.py
Line 1277 in e9c13ea
|
Hi @fippo ! Thanks for looking into this. I must admit I'm confused by the wording in RFC 8842, it stills say that offers should use https://datatracker.ietf.org/doc/html/rfc8842#name-generating-the-initial-sdp- |
|
Backward compatible in the sense "if you don't know the other side supports this keep using actpass but if you know it does (or assume)... then go ahead". For this to work we need more implementation (like aiortc) to support the new style ;-) |
|
Rebasing on top of |
fippo
force-pushed
the
active-passive-who-is-not-confused-by-this
branch
from
1609289
to
15c3610
Compare
That fixed one job but another started failing 😂 |
RFC 8842 updated RFC 5763 to allow a differt setup attribute than actpass in offers which allows for more control of the DTLS role. See https://issues.webrtc.org/issues/42223106 for additional details Fixes aiortc#1087
fippo
force-pushed
the
active-passive-who-is-not-confused-by-this
branch
from
15c3610
to
0b40293
Compare
|
Thanks so much @fippo! |
RFC 8842 updated RFC 5763 to allow a differt setup attribute than
actpass
in offers which allows for more control of the DTLS role.
See https://issues.webrtc.org/issues/42223106 for additional details
Fixes #1087