Repository: pion/webrtc
Pull request #2110 (2022-02-02 10:36:26)

Use ICE role to set DTLS role in answer
When creating answer, check ICE role while determining DTLS role.
ORTC thread on how roles are set
https://github.com/w3c/ortc/issues/167#issuecomment-69409953

When remote is ice-lite and there is no answering role set,
the answer uses the default which is DTLS client. So 'setup'
is set as 'active' and answer sent.

But, when DTLS transport is started, it checks the ICE role
and sets itself as DTLS server (because remote is lite
and hence local agent becomes controlling ICE agent).

So, both sides end up being DTLS server adnd nobody sends
client hello.


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/2110?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#2110](https://codecov.io/gh/pion/webrtc/pull/2110?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (b03b51c) into [master](https://codecov.io/gh/pion/webrtc/commit/664f6df0d3cdee8563b05a67fe3bc23bca19824d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (664f6df) will **decrease** coverage by `6.90%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/2110/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/webrtc/pull/2110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master    #2110      +/-   ##
==========================================
- Coverage   77.19%   70.28%   -6.91%     
==========================================
  Files          84       61      -23     
  Lines        8773     3224    -5549     
==========================================
- Hits         6772     2266    -4506     
+ Misses       1592      843     -749     
+ Partials      409      115     -294     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `70.28% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/2110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uc3RhbnRzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-81.82%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwY29kZWMuZ28=) | `13.33% <0.00%> (-80.00%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-75.00%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `9.09% <0.00%> (-63.64%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlbXV4Lmdv) | `0.00% <0.00%> (-54.55%)` | :arrow_down: |
| [atomicbool.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YXRvbWljYm9vbC5nbw==) | `57.14% <0.00%> (-42.86%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-34.49%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `24.13% <0.00%> (-34.49%)` | :arrow_down: |
| ... and [36 more](https://codecov.io/gh/pion/webrtc/pull/2110/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/2110?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/webrtc/pull/2110?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [664f6df...b03b51c](https://codecov.io/gh/pion/webrtc/pull/2110?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

Sean-Der: @boks1971 Is my change ok? I just removed a little bit of indirection. This is the original diff

```patch
diff --git a/peerconnection.go b/peerconnection.go
index b822dba..c26d01b 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -786,8 +786,9 @@ func (pc *PeerConnection) createICETransport() *ICETransport {
 // CreateAnswer starts the PeerConnection and generates the localDescription
 func (pc *PeerConnection) CreateAnswer(options *AnswerOptions) (SessionDescription, error) {
        useIdentity := pc.idpLoginURL != nil
+       remoteDesc := pc.RemoteDescription()
        switch {
-       case pc.RemoteDescription() == nil:
+       case remoteDesc == nil:
                return SessionDescription{}, &rtcerr.InvalidStateError{Err: ErrNoRemoteDescription}
        case useIdentity:
                return SessionDescription{}, errIdentityProviderNotImplemented
@@ -799,7 +800,27 @@ func (pc *PeerConnection) CreateAnswer(options *AnswerOptions) (SessionDescripti

        connectionRole := connectionRoleFromDtlsRole(pc.api.settingEngine.answeringDTLSRole)
        if connectionRole == sdp.ConnectionRole(0) {
-               connectionRole = connectionRoleFromDtlsRole(defaultDtlsRoleAnswer)
+               remoteIsLite := false
+               for _, a := range remoteDesc.parsed.Attributes {
+                       if strings.TrimSpace(a.Key) == sdp.AttrKeyICELite {
+                               remoteIsLite = true
+                               break
+                       }
+               }
+
+               iceRole := ICERoleControlled
+               // If one of the agents is lite and the other one is not, the lite agent must be the controlling agent.
+               // If both or neither agents are lite the offering agent is controlling.
+               // RFC 8445 S6.1.1
+               if remoteIsLite && !pc.api.settingEngine.candidates.ICELite {
+                       iceRole = ICERoleControlling
+               }
+
+               if iceRole == ICERoleControlling {
+                       connectionRole = connectionRoleFromDtlsRole(DTLSRoleServer)
+               } else {
+                       connectionRole = connectionRoleFromDtlsRole(defaultDtlsRoleAnswer)
+               }
        }
        pc.mu.Lock()
        defer pc.mu.Unlock()
@@ -1144,6 +1165,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
        for _, a := range desc.parsed.Attributes {
                if strings.TrimSpace(a.Key) == sdp.AttrKeyICELite {
                        remoteIsLite = true
+                       break
                }
        }
```
Sean-Der: @boks1971 Were you able to reproduce this one Pion <-> Pion by chance? A test would be nice if possible, I can write one right now!
boks1971: > @boks1971 Is my change ok? I just removed a little bit of indirection. This is the original diff
> 
> ```diff
> diff --git a/peerconnection.go b/peerconnection.go
> index b822dba..c26d01b 100644
> --- a/peerconnection.go
> +++ b/peerconnection.go
> @@ -786,8 +786,9 @@ func (pc *PeerConnection) createICETransport() *ICETransport {
>  // CreateAnswer starts the PeerConnection and generates the localDescription
>  func (pc *PeerConnection) CreateAnswer(options *AnswerOptions) (SessionDescription, error) {
>         useIdentity := pc.idpLoginURL != nil
> +       remoteDesc := pc.RemoteDescription()
>         switch {
> -       case pc.RemoteDescription() == nil:
> +       case remoteDesc == nil:
>                 return SessionDescription{}, &rtcerr.InvalidStateError{Err: ErrNoRemoteDescription}
>         case useIdentity:
>                 return SessionDescription{}, errIdentityProviderNotImplemented
> @@ -799,7 +800,27 @@ func (pc *PeerConnection) CreateAnswer(options *AnswerOptions) (SessionDescripti
> 
>         connectionRole := connectionRoleFromDtlsRole(pc.api.settingEngine.answeringDTLSRole)
>         if connectionRole == sdp.ConnectionRole(0) {
> -               connectionRole = connectionRoleFromDtlsRole(defaultDtlsRoleAnswer)
> +               remoteIsLite := false
> +               for _, a := range remoteDesc.parsed.Attributes {
> +                       if strings.TrimSpace(a.Key) == sdp.AttrKeyICELite {
> +                               remoteIsLite = true
> +                               break
> +                       }
> +               }
> +
> +               iceRole := ICERoleControlled
> +               // If one of the agents is lite and the other one is not, the lite agent must be the controlling agent.
> +               // If both or neither agents are lite the offering agent is controlling.
> +               // RFC 8445 S6.1.1
> +               if remoteIsLite && !pc.api.settingEngine.candidates.ICELite {
> +                       iceRole = ICERoleControlling
> +               }
> +
> +               if iceRole == ICERoleControlling {
> +                       connectionRole = connectionRoleFromDtlsRole(DTLSRoleServer)
> +               } else {
> +                       connectionRole = connectionRoleFromDtlsRole(defaultDtlsRoleAnswer)
> +               }
>         }
>         pc.mu.Lock()
>         defer pc.mu.Unlock()
> @@ -1144,6 +1165,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
>         for _, a := range desc.parsed.Attributes {
>                 if strings.TrimSpace(a.Key) == sdp.AttrKeyICELite {
>                         remoteIsLite = true
> +                       break
>                 }
>         }
> ```

Looks great @Sean-Der . Left one comment about removing the else.
boks1971: > @boks1971 Were you able to reproduce this one Pion <-> Pion by chance? A test would be nice if possible, I can write one right now!

Yes @Sean-Der . LiveKit server-sdk-go (https://github.com/livekit/server-sdk-go) uses Pion to connect to livekit-server (https://github.com/livekit/livekit-server) which is also Pion. We enabled `ice-lite` in livekit-server a couple of weeks back and found this now.

Patch:
diff --git a/peerconnection.go b/peerconnection.go
index b822dba3..1794e8d4 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -786,8 +786,9 @@ func (pc *PeerConnection) createICETransport() *ICETransport {
 // CreateAnswer starts the PeerConnection and generates the localDescription
 func (pc *PeerConnection) CreateAnswer(options *AnswerOptions) (SessionDescription, error) {
 	useIdentity := pc.idpLoginURL != nil
+	remoteDesc := pc.RemoteDescription()
 	switch {
-	case pc.RemoteDescription() == nil:
+	case remoteDesc == nil:
 		return SessionDescription{}, &rtcerr.InvalidStateError{Err: ErrNoRemoteDescription}
 	case useIdentity:
 		return SessionDescription{}, errIdentityProviderNotImplemented
@@ -800,6 +801,13 @@ func (pc *PeerConnection) CreateAnswer(options *AnswerOptions) (SessionDescripti
 	connectionRole := connectionRoleFromDtlsRole(pc.api.settingEngine.answeringDTLSRole)
 	if connectionRole == sdp.ConnectionRole(0) {
 		connectionRole = connectionRoleFromDtlsRole(defaultDtlsRoleAnswer)
+
+		// If one of the agents is lite and the other one is not, the lite agent must be the controlling agent.
+		// If both or neither agents are lite the offering agent is controlling.
+		// RFC 8445 S6.1.1
+		if isIceLiteSet(remoteDesc.parsed) && !pc.api.settingEngine.candidates.ICELite {
+			connectionRole = connectionRoleFromDtlsRole(DTLSRoleServer)
+		}
 	}
 	pc.mu.Lock()
 	defer pc.mu.Unlock()
@@ -1140,12 +1148,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 		return nil
 	}
 
-	remoteIsLite := false
-	for _, a := range desc.parsed.Attributes {
-		if strings.TrimSpace(a.Key) == sdp.AttrKeyICELite {
-			remoteIsLite = true
-		}
-	}
+	remoteIsLite := isIceLiteSet(desc.parsed)
 
 	fingerprint, fingerprintHash, err := extractFingerprint(desc.parsed)
 	if err != nil {
diff --git a/sdp.go b/sdp.go
index 81dbcf0a..c181bfe6 100644
--- a/sdp.go
+++ b/sdp.go
@@ -732,3 +732,13 @@ func updateSDPOrigin(origin *sdp.Origin, d *sdp.SessionDescription) {
 		d.Origin.SessionVersion = atomic.AddUint64(&origin.SessionVersion, 1)
 	}
 }
+
+func isIceLiteSet(desc *sdp.SessionDescription) bool {
+	for _, a := range desc.Attributes {
+		if strings.TrimSpace(a.Key) == sdp.AttrKeyICELite {
+			return true
+		}
+	}
+
+	return false
+}


Test patch:
diff --git a/peerconnection_go_test.go b/peerconnection_go_test.go
index 22b71275..f5832a3e 100644
--- a/peerconnection_go_test.go
+++ b/peerconnection_go_test.go
@@ -563,80 +563,52 @@ func TestOneAttrKeyConnectionSetupPerMediaDescriptionInSDP(t *testing.T) {
 	assert.NoError(t, pc.Close())
 }
 
-func TestPeerConnection_OfferingLite(t *testing.T) {
+func TestPeerConnection_IceLite(t *testing.T) {
 	report := test.CheckRoutines(t)
 	defer report()
 
 	lim := test.TimeOut(time.Second * 10)
 	defer lim.Stop()
 
-	s := SettingEngine{}
-	s.SetLite(true)
-	offerPC, err := NewAPI(WithSettingEngine(s)).NewPeerConnection(Configuration{})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	answerPC, err := NewAPI().NewPeerConnection(Configuration{})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if err = signalPair(offerPC, answerPC); err != nil {
-		t.Fatal(err)
-	}
-
-	iceComplete := make(chan interface{})
-	answerPC.OnICEConnectionStateChange(func(iceState ICEConnectionState) {
-		if iceState == ICEConnectionStateConnected {
-			select {
-			case <-iceComplete:
-			default:
-				close(iceComplete)
-			}
+	connectTwoAgents := func(offerIsLite, answerisLite bool) {
+		offerSettingEngine := SettingEngine{}
+		offerSettingEngine.SetLite(offerIsLite)
+		offerPC, err := NewAPI(WithSettingEngine(offerSettingEngine)).NewPeerConnection(Configuration{})
+		if err != nil {
+			t.Fatal(err)
 		}
-	})
 
-	<-iceComplete
-	closePairNow(t, offerPC, answerPC)
-}
-
-func TestPeerConnection_AnsweringLite(t *testing.T) {
-	report := test.CheckRoutines(t)
-	defer report()
+		answerSettingEngine := SettingEngine{}
+		answerSettingEngine.SetLite(answerisLite)
+		answerPC, err := NewAPI(WithSettingEngine(answerSettingEngine)).NewPeerConnection(Configuration{})
+		if err != nil {
+			t.Fatal(err)
+		}
 
-	lim := test.TimeOut(time.Second * 10)
-	defer lim.Stop()
+		if err = signalPair(offerPC, answerPC); err != nil {
+			t.Fatal(err)
+		}
 
-	offerPC, err := NewAPI().NewPeerConnection(Configuration{})
-	if err != nil {
-		t.Fatal(err)
-	}
+		dataChannelOpen := make(chan interface{})
+		answerPC.OnDataChannel(func(_ *DataChannel) {
+			close(dataChannelOpen)
+		})
 
-	s := SettingEngine{}
-	s.SetLite(true)
-	answerPC, err := NewAPI(WithSettingEngine(s)).NewPeerConnection(Configuration{})
-	if err != nil {
-		t.Fatal(err)
+		<-dataChannelOpen
+		closePairNow(t, offerPC, answerPC)
 	}
 
-	if err = signalPair(offerPC, answerPC); err != nil {
-		t.Fatal(err)
-	}
+	t.Run("Offerer", func(t *testing.T) {
+		connectTwoAgents(true, false)
+	})
 
-	iceComplete := make(chan interface{})
-	answerPC.OnICEConnectionStateChange(func(iceState ICEConnectionState) {
-		if iceState == ICEConnectionStateConnected {
-			select {
-			case <-iceComplete:
-			default:
-				close(iceComplete)
-			}
-		}
+	t.Run("Answerer", func(t *testing.T) {
+		connectTwoAgents(false, true)
 	})
 
-	<-iceComplete
-	closePairNow(t, offerPC, answerPC)
+	t.Run("Both", func(t *testing.T) {
+		connectTwoAgents(true, true)
+	})
 }
 
 func TestOnICEGatheringStateChange(t *testing.T) {