Repository: aiortc/aiortc

Issue:
aiortc rejecting offer with setup=passive against RFC guidance
Repro steps
1. Have an SDP that has `a=setup:passive` in it
2. Create `RTCSessionDescription` with this sdp and `type="offer"`
3. Call `pc.setRemoteDescription` with above session description

Expected
1. This succeeds. See [RFC spec](https://datatracker.ietf.org/doc/html/rfc8842#section-5.3-6) that states receiver should be prepared for both `active` and `passive` as well.

> Even though an offerer is required to insert an "SDP" setup attribute with an "actpass" attribute value in initial offers ([Section 5.2](https://datatracker.ietf.org/doc/html/rfc8842#sec-oa-offer)) and subsequent offers ([Section 5.5](https://datatracker.ietf.org/doc/html/rfc8842#sec-oa-mod)), the **_answerer MUST be able to receive initial and subsequent offers with other attribute values_**, in order to be backward compatible with older implementations that might insert other attribute values in initial and subsequent offers.

Actual
1. An exception is raised with message "DTLS setup attribute must be 'actpass' for an offer" from [__validate_description method](https://github.com/aiortc/aiortc/blob/e9c13eab915ddc27f365356ed9ded0585b9a1bb7/src/aiortc/rtcpeerconnection.py#L1277)

PS: I see this got discussed a bit in https://github.com/aiortc/aiortc/issues/267


Discussion:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/aiortc/aiortc/pull/1090?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiortc) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 99.98%. Comparing base [(`d420ec8`)](https://app.codecov.io/gh/aiortc/aiortc/commit/d420ec8552d3934868f969b14e6da43da062cef2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiortc) to head [(`0b82548`)](https://app.codecov.io/gh/aiortc/aiortc/pull/1090?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiortc).
> Report is 4 commits behind head on main.


<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##              main    #1090      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files           30       30              
  Lines         5846     5868      +22     
===========================================
+ Hits          5846     5867      +21     
- Misses           0        1       +1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/aiortc/aiortc/pull/1090?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiortc).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiortc).

jlaine: 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 `actpass`.. but allow other values for *backwards* compatibility?

https://datatracker.ietf.org/doc/html/rfc8842#name-generating-the-initial-sdp-
fippo: 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 ;-)
jlaine: Rebasing on top of `main` will fix the error building wheels on macOs / x86_64.
fippo: > Rebasing on top of `main` will fix the error building wheels on macOs / x86_64.

That fixed one job but another started failing 😂 

Patch:
diff --git a/src/aiortc/rtcpeerconnection.py b/src/aiortc/rtcpeerconnection.py
index 17e06cb..70b5ee9 100644
--- a/src/aiortc/rtcpeerconnection.py
+++ b/src/aiortc/rtcpeerconnection.py
@@ -928,6 +928,8 @@ class RTCPeerConnection(AsyncIOEventEmitter):
                     iceTransport._role_set = True
 
                 # set DTLS role
+                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"
@@ -1273,8 +1275,6 @@ class RTCPeerConnection(AsyncIOEventEmitter):
                 raise ValueError("ICE username fragment or password is missing")
 
             # check DTLS role is allowed
-            if description.type == "offer" and media.dtls.role != "auto":
-                raise ValueError("DTLS setup attribute must be 'actpass' for an offer")
             if description.type in ["answer", "pranswer"] and media.dtls.role not in [
                 "client",
                 "server",


Test patch:
diff --git a/tests/test_rtcpeerconnection.py b/tests/test_rtcpeerconnection.py
index ba51b4a..3f5a2d3 100644
--- a/tests/test_rtcpeerconnection.py
+++ b/tests/test_rtcpeerconnection.py
@@ -4685,30 +4685,6 @@ a=rtpmap:0 PCMU/8000
         await pc1.close()
         await pc2.close()
 
-    @asynctest
-    async def test_setRemoteDescription_with_invalid_dtls_setup_for_offer(self):
-        pc1 = RTCPeerConnection()
-        pc2 = RTCPeerConnection()
-
-        # apply offer
-        pc1.addTrack(AudioStreamTrack())
-        offer = await pc1.createOffer()
-        await pc1.setLocalDescription(offer)
-        mangled = RTCSessionDescription(
-            sdp=pc1.localDescription.sdp.replace("a=setup:actpass", "a=setup:active"),
-            type=pc1.localDescription.type,
-        )
-        with self.assertRaises(ValueError) as cm:
-            await pc2.setRemoteDescription(mangled)
-        self.assertEqual(
-            str(cm.exception),
-            "DTLS setup attribute must be 'actpass' for an offer",
-        )
-
-        # close
-        await pc1.close()
-        await pc2.close()
-
     @asynctest
     async def test_setRemoteDescription_with_invalid_dtls_setup_for_answer(self):
         pc1 = RTCPeerConnection()
@@ -4976,3 +4952,193 @@ a=rtpmap:0 PCMU/8000
                 "closed",
             ],
         )
+
+    @asynctest
+    async def test_dtls_role_offer_actpass(self):
+        pc1 = RTCPeerConnection()
+        pc2 = RTCPeerConnection()
+
+        pc1_states = track_states(pc1)
+        pc2_states = track_states(pc2)
+
+        self.assertEqual(pc1.iceConnectionState, "new")
+        self.assertEqual(pc1.iceGatheringState, "new")
+        self.assertIsNone(pc1.localDescription)
+        self.assertIsNone(pc1.remoteDescription)
+
+        self.assertEqual(pc2.iceConnectionState, "new")
+        self.assertEqual(pc2.iceGatheringState, "new")
+        self.assertIsNone(pc2.localDescription)
+        self.assertIsNone(pc2.remoteDescription)
+
+        # create offer
+        pc1.createDataChannel("chat", protocol="")
+        offer = await pc1.createOffer()
+        self.assertEqual(offer.type, "offer")
+
+        await pc1.setLocalDescription(offer)
+        self.assertEqual(pc1.iceConnectionState, "new")
+        self.assertEqual(pc1.iceGatheringState, "complete")
+
+        # set remote description
+        await pc2.setRemoteDescription(pc1.localDescription)
+
+        # create answer
+        answer = await pc2.createAnswer()
+        self.assertHasDtls(answer, "active")
+
+        await pc2.setLocalDescription(answer)
+        await self.assertIceChecking(pc2)
+
+        # handle answer
+        await pc1.setRemoteDescription(pc2.localDescription)
+        self.assertEqual(pc1.remoteDescription, pc2.localDescription)
+
+        # check outcome
+        await self.assertIceCompleted(pc1, pc2)
+
+        self.assertEqual(pc1.sctp.transport._role, "server")
+        self.assertEqual(pc2.sctp.transport._role, "client")
+        # close
+        await pc1.close()
+        await pc2.close()
+        self.assertClosed(pc1)
+        self.assertClosed(pc2)
+
+        # check state changes
+        self.assertEqual(
+            pc1_states["connectionState"], ["new", "connecting", "connected", "closed"]
+        )
+        self.assertEqual(
+            pc2_states["connectionState"], ["new", "connecting", "connected", "closed"]
+        )
+
+    @asynctest
+    async def test_dtls_role_offer_passive(self):
+        pc1 = RTCPeerConnection()
+        pc2 = RTCPeerConnection()
+
+        pc1_states = track_states(pc1)
+        pc2_states = track_states(pc2)
+
+        self.assertEqual(pc1.iceConnectionState, "new")
+        self.assertEqual(pc1.iceGatheringState, "new")
+        self.assertIsNone(pc1.localDescription)
+        self.assertIsNone(pc1.remoteDescription)
+
+        self.assertEqual(pc2.iceConnectionState, "new")
+        self.assertEqual(pc2.iceGatheringState, "new")
+        self.assertIsNone(pc2.localDescription)
+        self.assertIsNone(pc2.remoteDescription)
+
+        # create offer
+        pc1.createDataChannel("chat", protocol="")
+        offer = await pc1.createOffer()
+        self.assertEqual(offer.type, "offer")
+
+        await pc1.setLocalDescription(offer)
+        self.assertEqual(pc1.iceConnectionState, "new")
+        self.assertEqual(pc1.iceGatheringState, "complete")
+
+        # handle offer with replaced DTLS role
+        await pc2.setRemoteDescription(
+            RTCSessionDescription(
+                type="offer", sdp=pc1.localDescription.sdp.replace("actpass", "passive")
+            )
+        )
+
+        # create answer
+        answer = await pc2.createAnswer()
+        self.assertHasDtls(answer, "active")
+
+        await pc2.setLocalDescription(answer)
+        await self.assertIceChecking(pc2)
+
+        # handle answer
+        await pc1.setRemoteDescription(pc2.localDescription)
+        self.assertEqual(pc1.remoteDescription, pc2.localDescription)
+
+        # check outcome
+        await self.assertIceCompleted(pc1, pc2)
+
+        # pc1 is explicity passive so server.
+        self.assertEqual(pc1.sctp.transport._role, "server")
+        self.assertEqual(pc2.sctp.transport._role, "client")
+        # close
+        await pc1.close()
+        await pc2.close()
+        self.assertClosed(pc1)
+        self.assertClosed(pc2)
+
+        # check state changes
+        self.assertEqual(
+            pc1_states["connectionState"], ["new", "connecting", "connected", "closed"]
+        )
+        self.assertEqual(
+            pc2_states["connectionState"], ["new", "connecting", "connected", "closed"]
+        )
+
+    @asynctest
+    async def test_dtls_role_offer_active(self):
+        pc1 = RTCPeerConnection()
+        pc2 = RTCPeerConnection()
+
+        pc1_states = track_states(pc1)
+        pc2_states = track_states(pc2)
+
+        self.assertEqual(pc1.iceConnectionState, "new")
+        self.assertEqual(pc1.iceGatheringState, "new")
+        self.assertIsNone(pc1.localDescription)
+        self.assertIsNone(pc1.remoteDescription)
+
+        self.assertEqual(pc2.iceConnectionState, "new")
+        self.assertEqual(pc2.iceGatheringState, "new")
+        self.assertIsNone(pc2.localDescription)
+        self.assertIsNone(pc2.remoteDescription)
+
+        # create offer
+        pc1.createDataChannel("chat", protocol="")
+        offer = await pc1.createOffer()
+        self.assertEqual(offer.type, "offer")
+
+        await pc1.setLocalDescription(offer)
+        self.assertEqual(pc1.iceConnectionState, "new")
+        self.assertEqual(pc1.iceGatheringState, "complete")
+
+        # handle offer with replaced DTLS role
+        await pc2.setRemoteDescription(
+            RTCSessionDescription(
+                type="offer", sdp=pc1.localDescription.sdp.replace("actpass", "active")
+            )
+        )
+
+        # create answer
+        answer = await pc2.createAnswer()
+        self.assertHasDtls(answer, "passive")
+
+        await pc2.setLocalDescription(answer)
+        await self.assertIceChecking(pc2)
+
+        # handle answer
+        await pc1.setRemoteDescription(pc2.localDescription)
+        self.assertEqual(pc1.remoteDescription, pc2.localDescription)
+
+        # check outcome
+        await self.assertIceCompleted(pc1, pc2)
+
+        # pc1 is explicity active so client.
+        self.assertEqual(pc1.sctp.transport._role, "client")
+        self.assertEqual(pc2.sctp.transport._role, "server")
+        # close
+        await pc1.close()
+        await pc2.close()
+        self.assertClosed(pc1)
+        self.assertClosed(pc2)
+
+        # check state changes
+        self.assertEqual(
+            pc1_states["connectionState"], ["new", "connecting", "connected", "closed"]
+        )
+        self.assertEqual(
+            pc2_states["connectionState"], ["new", "connecting", "connected", "closed"]
+        )