Repository: pion/webrtc (go)

Issue:
RFC8843 : Accept offer with `a=group:BUNDLE` line even if it has conflicting ice-ufrag values
### Your environment.
  - Pion Version: v3.2.14
  - PBX: Asterisk v18 (WebRTC mode)

### What did you do?

Pion receives an SDP as offer which is then provided to SetRemoteDescription :

```
v=0
o=- 852496663 852496663 IN IP4 ...........
s=Asterisk
c=IN IP4 ................
t=0 0
a=group:BUNDLE audio-0 video-1 video-2
m=audio 11514 UDP/TLS/RTP/SAVPF 107 101
a=connection:new
a=setup:actpass
a=fingerprint:SHA-256 3E:E0:8A:06:36:56:73:9E:72:08:61:5D:13:38:C0:42:9A:25:3E:34:E7:03:7F:6E:F5:9F:B9:2C:7D:C7:02:C5
a=ice-ufrag:6992ba23181e294c0bd380af7ac57964
a=ice-pwd:7d1090fb4b82e27374cf42b8363b631d
a=candidate:H9387b5c9 1 UDP 2130706431 .............. 11514 typ host
a=candidate:Hd30485b5 1 UDP 2130706431 .............. 11514 typ host
a=rtpmap:107 opus/48000/2
a=fmtp:107 useinbandfec=1
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-16
a=ptime:20
a=maxptime:20
a=sendrecv
a=rtcp-mux
a=ssrc:2081773396 cname:43c36c43-de21-4f8b-8ab7-4580bac88350
a=mid:audio-0
m=video 16464 UDP/TLS/RTP/SAVPF 100
a=connection:new
a=setup:actpass
a=fingerprint:SHA-256 43:DC:6D:4D:8B:7E:FE:47:3C:F3:18:6B:B7:D8:B0:0A:1F:20:55:18:60:AF:2F:46:E9:1F:A6:FE:F5:E0:22:4B
a=ice-ufrag:287c549566e5ca694b2aaf053d484b35
a=ice-pwd:548946fd5d6c7cb068bbcfb656593bb8
a=candidate:H9387b5c9 1 UDP 2130706431 ................... 16464 typ host
a=candidate:Hd30485b5 1 UDP 2130706431 ................... 16464 typ host
a=rtpmap:100 VP8/90000
a=sendrecv
a=rtcp-mux
a=ssrc:181453472 cname:efb36874-07c8-46aa-93cf-82f2237ae446
a=mid:video-1
m=video 16690 UDP/TLS/RTP/SAVPF 100
a=connection:new
a=setup:actpass
a=fingerprint:SHA-256 6D:37:49:4E:43:C8:D0:1F:49:50:17:36:F6:1B:2D:62:2F:CC:6F:51:B3:8F:D3:28:AF:02:37:0D:08:EA:83:53
a=ice-ufrag:194467e75266fb7d26b5d08f2521c42c
a=ice-pwd:037f3c931c992e497733cb7401d07efe
a=candidate:H9387b5c9 1 UDP 2130706431 ......................... 16690 typ host
a=candidate:Hd30485b5 1 UDP 2130706431 ......................... 16690 typ host
a=rtpmap:100 VP8/90000
a=recvonly
a=rtcp-mux
a=ssrc:27453603 cname:69161142-12b4-4468-b0d2-51c7229292e2
a=mid:video-2
```

### What did you expect?

I expect Pion to follow the [RFC8843](https://datatracker.ietf.org/doc/html/rfc8843) and accept that offer even if it has conflicting ice-ufrag values because it has the 
`a=group:BUNDLE` line. Then choose which stream's transport it wants to use (typically the first one but the RFC allows to choose any one). And respond with an answer with bundling enabled for the chosen transport.



### What happened?

Error : SetRemoteDescription called with multiple conflicting ice-ufrag values

Pull request:
Only collect single fingerprints and ICE credentials
#### Description
The way currently DTLS fingerprints and ICE credentials are picked is causing interop issues as described in #2621 

Peers which don't use Bundle can use different fingerprints and credentials in each media section. Even though is not (yet) supported by Pion, receiving an SDP offer from such a peer is valid.

Additionally if Bundle is being used the group attribute determines which media section is the master bundle section, which establishes the transport. Currently Pion always just uses the first credentials/fingerprint it can find in the SDP, which results in not spec compliant behavior.

This PR attempts to fix the above issues and make Pion more spec compliant and interoperable.

#### Reference issue
Fixes #2621


Patch:
diff --git a/sdp.go b/sdp.go
index 3dd79b50..10a24edc 100644
--- a/sdp.go
+++ b/sdp.go
@@ -719,96 +719,156 @@ func getPeerDirection(media *sdp.MediaDescription) RTPTransceiverDirection {
 	return RTPTransceiverDirectionUnknown
 }
 
-func extractFingerprint(desc *sdp.SessionDescription) (string, string, error) {
-	fingerprints := []string{}
+func extractBundleID(desc *sdp.SessionDescription) string {
+	groupAttribute, _ := desc.Attribute(sdp.AttrKeyGroup)
 
-	if fingerprint, haveFingerprint := desc.Attribute("fingerprint"); haveFingerprint {
-		fingerprints = append(fingerprints, fingerprint)
+	isBundled := strings.Contains(groupAttribute, "BUNDLE")
+
+	if !isBundled {
+		return ""
 	}
 
-	for _, m := range desc.MediaDescriptions {
-		if fingerprint, haveFingerprint := m.Attribute("fingerprint"); haveFingerprint {
-			fingerprints = append(fingerprints, fingerprint)
-		}
+	bundleIDs := strings.Split(groupAttribute, " ")
+
+	if len(bundleIDs) < 2 {
+		return ""
 	}
 
-	if len(fingerprints) < 1 {
-		return "", "", ErrSessionDescriptionNoFingerprint
+	return bundleIDs[1]
+}
+
+func extractFingerprint(desc *sdp.SessionDescription) (string, string, error) { //nolint: gocognit
+	fingerprint := ""
+
+	// Fingerprint on session level has highest priority
+	if sessionFingerprint, haveFingerprint := desc.Attribute("fingerprint"); haveFingerprint {
+		fingerprint = sessionFingerprint
 	}
 
-	for _, m := range fingerprints {
-		if m != fingerprints[0] {
-			return "", "", ErrSessionDescriptionConflictingFingerprints
+	if fingerprint == "" {
+		bundleID := extractBundleID(desc)
+		if bundleID != "" {
+			// Locate the fingerprint of the bundled media section
+			for _, m := range desc.MediaDescriptions {
+				if mid, haveMid := m.Attribute("mid"); haveMid {
+					if mid == bundleID && fingerprint == "" {
+						if mediaFingerprint, haveFingerprint := m.Attribute("fingerprint"); haveFingerprint {
+							fingerprint = mediaFingerprint
+						}
+					}
+				}
+			}
+		} else {
+			// Take the fingerprint from the first media section which has one.
+			// Note: According to Bundle spec each media section would have it's own transport
+			//       with it's own cert and fingerprint each, so we would need to return a list.
+			for _, m := range desc.MediaDescriptions {
+				mediaFingerprint, haveFingerprint := m.Attribute("fingerprint")
+				if haveFingerprint && fingerprint == "" {
+					fingerprint = mediaFingerprint
+				}
+			}
 		}
 	}
 
-	parts := strings.Split(fingerprints[0], " ")
+	if fingerprint == "" {
+		return "", "", ErrSessionDescriptionNoFingerprint
+	}
+
+	parts := strings.Split(fingerprint, " ")
 	if len(parts) != 2 {
 		return "", "", ErrSessionDescriptionInvalidFingerprint
 	}
 	return parts[1], parts[0], nil
 }
 
-func extractICEDetails(desc *sdp.SessionDescription, log logging.LeveledLogger) (string, string, []ICECandidate, error) { // nolint:gocognit
+func extractICEDetailsFromMedia(media *sdp.MediaDescription, log logging.LeveledLogger) (string, string, []ICECandidate, error) {
+	remoteUfrag := ""
+	remotePwd := ""
 	candidates := []ICECandidate{}
-	remotePwds := []string{}
-	remoteUfrags := []string{}
 
+	if ufrag, haveUfrag := media.Attribute("ice-ufrag"); haveUfrag {
+		remoteUfrag = ufrag
+	}
+	if pwd, havePwd := media.Attribute("ice-pwd"); havePwd {
+		remotePwd = pwd
+	}
+	for _, a := range media.Attributes {
+		if a.IsICECandidate() {
+			c, err := ice.UnmarshalCandidate(a.Value)
+			if err != nil {
+				if errors.Is(err, ice.ErrUnknownCandidateTyp) || errors.Is(err, ice.ErrDetermineNetworkType) {
+					log.Warnf("Discarding remote candidate: %s", err)
+					continue
+				}
+				return "", "", nil, err
+			}
+
+			candidate, err := newICECandidateFromICE(c)
+			if err != nil {
+				return "", "", nil, err
+			}
+
+			candidates = append(candidates, candidate)
+		}
+	}
+
+	return remoteUfrag, remotePwd, candidates, nil
+}
+
+func extractICEDetails(desc *sdp.SessionDescription, log logging.LeveledLogger) (string, string, []ICECandidate, error) { // nolint:gocognit
+	remoteCandidates := []ICECandidate{}
+	remotePwd := ""
+	remoteUfrag := ""
+
+	// Ufrag and Pw are allow at session level and thus have highest prio
 	if ufrag, haveUfrag := desc.Attribute("ice-ufrag"); haveUfrag {
-		remoteUfrags = append(remoteUfrags, ufrag)
+		remoteUfrag = ufrag
 	}
 	if pwd, havePwd := desc.Attribute("ice-pwd"); havePwd {
-		remotePwds = append(remotePwds, pwd)
+		remotePwd = pwd
 	}
 
-	for _, m := range desc.MediaDescriptions {
-		if ufrag, haveUfrag := m.Attribute("ice-ufrag"); haveUfrag {
-			remoteUfrags = append(remoteUfrags, ufrag)
-		}
-		if pwd, havePwd := m.Attribute("ice-pwd"); havePwd {
-			remotePwds = append(remotePwds, pwd)
-		}
+	bundleID := extractBundleID(desc)
+	missing := true
 
-		for _, a := range m.Attributes {
-			if a.IsICECandidate() {
-				c, err := ice.UnmarshalCandidate(a.Value)
+	for _, m := range desc.MediaDescriptions {
+		mid := getMidValue(m)
+		// If bundled, only take ICE detail from bundle master section
+		if bundleID != "" {
+			if mid == bundleID {
+				ufrag, pwd, candidates, err := extractICEDetailsFromMedia(m, log)
 				if err != nil {
-					if errors.Is(err, ice.ErrUnknownCandidateTyp) || errors.Is(err, ice.ErrDetermineNetworkType) {
-						log.Warnf("Discarding remote candidate: %s", err)
-						continue
-					}
 					return "", "", nil, err
 				}
-
-				candidate, err := newICECandidateFromICE(c)
-				if err != nil {
-					return "", "", nil, err
+				if remoteUfrag == "" && ufrag != "" {
+					remoteUfrag = ufrag
+					remotePwd = pwd
 				}
-
-				candidates = append(candidates, candidate)
+				remoteCandidates = candidates
+			}
+		} else if missing {
+			// For not-bundled, take ICE details from the first media section
+			ufrag, pwd, candidates, err := extractICEDetailsFromMedia(m, log)
+			if err != nil {
+				return "", "", nil, err
+			}
+			if remoteUfrag == "" && ufrag != "" {
+				remoteUfrag = ufrag
+				remotePwd = pwd
 			}
+			remoteCandidates = candidates
+			missing = false
 		}
 	}
 
-	if len(remoteUfrags) == 0 {
+	if remoteUfrag == "" {
 		return "", "", nil, ErrSessionDescriptionMissingIceUfrag
-	} else if len(remotePwds) == 0 {
+	} else if remotePwd == "" {
 		return "", "", nil, ErrSessionDescriptionMissingIcePwd
 	}
 
-	for _, m := range remoteUfrags {
-		if m != remoteUfrags[0] {
-			return "", "", nil, ErrSessionDescriptionConflictingIceUfrag
-		}
-	}
-
-	for _, m := range remotePwds {
-		if m != remotePwds[0] {
-			return "", "", nil, ErrSessionDescriptionConflictingIcePwd
-		}
-	}
-
-	return remoteUfrags[0], remotePwds[0], candidates, nil
+	return remoteUfrag, remotePwd, remoteCandidates, nil
 }
 
 func haveApplicationMediaSection(desc *sdp.SessionDescription) bool {


Test patch:
diff --git a/mediaengine_test.go b/mediaengine_test.go
index d598c03c..fa3cc2d8 100644
--- a/mediaengine_test.go
+++ b/mediaengine_test.go
@@ -645,7 +645,7 @@ v=0
 o=- 8448668841136641781 4 IN IP4 127.0.0.1
 s=-
 t=0 0
-a=group:BUNDLE 0 1 2
+a=group:BUNDLE 1
 a=extmap-allow-mixed
 a=msid-semantic: WMS 4beea6b0-cf95-449c-a1ec-78e16b247426
 m=video 9 UDP/TLS/RTP/SAVPF 96 127
diff --git a/peerconnection_go_test.go b/peerconnection_go_test.go
index d7e88da4..3de9acf5 100644
--- a/peerconnection_go_test.go
+++ b/peerconnection_go_test.go
@@ -1398,6 +1398,7 @@ func TestTransceiverCreatedByRemoteSdpHasSameCodecOrderAsRemote(t *testing.T) {
 o=- 4596489990601351948 2 IN IP4 127.0.0.1
 s=-
 t=0 0
+a=group:BUNDLE 0 1
 m=video 60323 UDP/TLS/RTP/SAVPF 98 94 106
 a=ice-ufrag:1/MvHwjAyVf27aLu
 a=ice-pwd:3dBU7cFOBl120v33cynDvN1E
@@ -1458,6 +1459,7 @@ a=fmtp:125 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=42e01
 o=- 4596489990601351948 2 IN IP4 127.0.0.1
 s=-
 t=0 0
+a=group:BUNDLE 0 1
 m=video 60323 UDP/TLS/RTP/SAVPF 98 106
 a=ice-ufrag:1/MvHwjAyVf27aLu
 a=ice-pwd:3dBU7cFOBl120v33cynDvN1E
@@ -1548,7 +1550,7 @@ o=- 4596489990601351948 2 IN IP4 127.0.0.1
 s=-
 t=0 0
 a=fingerprint:sha-256 F7:BF:B4:42:5B:44:C0:B9:49:70:6D:26:D7:3E:E6:08:B1:5B:25:2E:32:88:50:B6:3C:BE:4E:18:A7:2C:85:7C
-a=group:BUNDLE 0 1
+a=group:BUNDLE 0
 a=msid-semantic:WMS *
 m=video 9 UDP/TLS/RTP/SAVPF 97
 c=IN IP4 0.0.0.0
diff --git a/peerconnection_media_test.go b/peerconnection_media_test.go
index 74863148..a9450096 100644
--- a/peerconnection_media_test.go
+++ b/peerconnection_media_test.go
@@ -1072,6 +1072,9 @@ func TestPeerConnection_Simulcast_Probe(t *testing.T) {
 			for scanner.Scan() {
 				if strings.HasPrefix(scanner.Text(), "m=video") {
 					shouldDiscard = !shouldDiscard
+				} else if strings.HasPrefix(scanner.Text(), "a=group:BUNDLE") {
+					filtered += "a=group:BUNDLE 1 2\r\n"
+					continue
 				}
 
 				if !shouldDiscard {
diff --git a/peerconnection_test.go b/peerconnection_test.go
index 12811ebc..a87ba1fb 100644
--- a/peerconnection_test.go
+++ b/peerconnection_test.go
@@ -282,6 +282,7 @@ const minimalOffer = `v=0
 o=- 4596489990601351948 2 IN IP4 127.0.0.1
 s=-
 t=0 0
+a=group:BUNDLE data
 a=msid-semantic: WMS
 m=application 47299 DTLS/SCTP 5000
 c=IN IP4 192.168.20.129
diff --git a/sdp_test.go b/sdp_test.go
index e3b9bf9b..46a3b8de 100644
--- a/sdp_test.go
+++ b/sdp_test.go
@@ -59,22 +59,69 @@ func TestExtractFingerprint(t *testing.T) {
 		assert.Equal(t, ErrSessionDescriptionInvalidFingerprint, err)
 	})
 
-	t.Run("Conflicting Fingerprint", func(t *testing.T) {
+	t.Run("Session fingerprint wins over media", func(t *testing.T) {
 		s := &sdp.SessionDescription{
-			Attributes: []sdp.Attribute{{Key: "fingerprint", Value: "foo"}},
+			Attributes: []sdp.Attribute{{Key: "fingerprint", Value: "foo bar"}},
 			MediaDescriptions: []*sdp.MediaDescription{
-				{Attributes: []sdp.Attribute{{Key: "fingerprint", Value: "foo blah"}}},
+				{Attributes: []sdp.Attribute{{Key: "fingerprint", Value: "zoo boo"}}},
 			},
 		}
 
-		_, _, err := extractFingerprint(s)
-		assert.Equal(t, ErrSessionDescriptionConflictingFingerprints, err)
+		fingerprint, hash, err := extractFingerprint(s)
+		assert.NoError(t, err)
+		assert.Equal(t, fingerprint, "bar")
+		assert.Equal(t, hash, "foo")
+	})
+
+	t.Run("Fingerprint from master bundle section", func(t *testing.T) {
+		s := &sdp.SessionDescription{
+			Attributes: []sdp.Attribute{
+				{Key: "group", Value: "BUNDLE 1 0"},
+			},
+			MediaDescriptions: []*sdp.MediaDescription{
+				{Attributes: []sdp.Attribute{
+					{Key: "mid", Value: "0"},
+					{Key: "fingerprint", Value: "zoo boo"},
+				}},
+				{Attributes: []sdp.Attribute{
+					{Key: "mid", Value: "1"},
+					{Key: "fingerprint", Value: "bar foo"},
+				}},
+			},
+		}
+
+		fingerprint, hash, err := extractFingerprint(s)
+		assert.NoError(t, err)
+		assert.Equal(t, fingerprint, "foo")
+		assert.Equal(t, hash, "bar")
+	})
+
+	t.Run("Fingerprint from first media section", func(t *testing.T) {
+		s := &sdp.SessionDescription{
+			MediaDescriptions: []*sdp.MediaDescription{
+				{Attributes: []sdp.Attribute{
+					{Key: "mid", Value: "0"},
+					{Key: "fingerprint", Value: "zoo boo"},
+				}},
+				{Attributes: []sdp.Attribute{
+					{Key: "mid", Value: "1"},
+					{Key: "fingerprint", Value: "bar foo"},
+				}},
+			},
+		}
+
+		fingerprint, hash, err := extractFingerprint(s)
+		assert.NoError(t, err)
+		assert.Equal(t, fingerprint, "boo")
+		assert.Equal(t, hash, "zoo")
 	})
 }
 
 func TestExtractICEDetails(t *testing.T) {
-	const defaultUfrag = "defaultPwd"
-	const defaultPwd = "defaultUfrag"
+	const defaultUfrag = "defaultUfrag"
+	const defaultPwd = "defaultPwd"
+	const invalidUfrag = "invalidUfrag"
+	const invalidPwd = "invalidPwd"
 
 	t.Run("Missing ice-pwd", func(t *testing.T) {
 		s := &sdp.SessionDescription{
@@ -131,28 +178,91 @@ func TestExtractICEDetails(t *testing.T) {
 		assert.NoError(t, err)
 	})
 
-	t.Run("Conflict ufrag", func(t *testing.T) {
+	t.Run("ice details at session preferred over media", func(t *testing.T) {
 		s := &sdp.SessionDescription{
-			Attributes: []sdp.Attribute{{Key: "ice-ufrag", Value: "invalidUfrag"}},
+			Attributes: []sdp.Attribute{
+				{Key: "ice-ufrag", Value: defaultUfrag},
+				{Key: "ice-pwd", Value: defaultPwd},
+			},
 			MediaDescriptions: []*sdp.MediaDescription{
-				{Attributes: []sdp.Attribute{{Key: "ice-ufrag", Value: defaultUfrag}, {Key: "ice-pwd", Value: defaultPwd}}},
+				{
+					Attributes: []sdp.Attribute{
+						{Key: "ice-ufrag", Value: invalidUfrag},
+						{Key: "ice-pwd", Value: invalidPwd},
+					},
+				},
 			},
 		}
 
-		_, _, _, err := extractICEDetails(s, nil)
-		assert.Equal(t, err, ErrSessionDescriptionConflictingIceUfrag)
+		ufrag, pwd, _, err := extractICEDetails(s, nil)
+		assert.Equal(t, ufrag, defaultUfrag)
+		assert.Equal(t, pwd, defaultPwd)
+		assert.NoError(t, err)
 	})
 
-	t.Run("Conflict pwd", func(t *testing.T) {
+	t.Run("ice details from bundle media section", func(t *testing.T) {
 		s := &sdp.SessionDescription{
-			Attributes: []sdp.Attribute{{Key: "ice-pwd", Value: "invalidPwd"}},
+			Attributes: []sdp.Attribute{
+				{Key: "group", Value: "BUNDLE 5 2"},
+			},
+			MediaDescriptions: []*sdp.MediaDescription{
+				{
+					Attributes: []sdp.Attribute{
+						{Key: "mid", Value: "2"},
+						{Key: "ice-ufrag", Value: invalidUfrag},
+						{Key: "ice-pwd", Value: invalidPwd},
+					},
+				},
+				{
+					Attributes: []sdp.Attribute{
+						{Key: "mid", Value: "5"},
+						{Key: "ice-ufrag", Value: defaultUfrag},
+						{Key: "ice-pwd", Value: defaultPwd},
+					},
+				},
+			},
+		}
+
+		ufrag, pwd, _, err := extractICEDetails(s, nil)
+		assert.Equal(t, ufrag, defaultUfrag)
+		assert.Equal(t, pwd, defaultPwd)
+		assert.NoError(t, err)
+	})
+
+	t.Run("ice details from first media section", func(t *testing.T) {
+		s := &sdp.SessionDescription{
+			MediaDescriptions: []*sdp.MediaDescription{
+				{
+					Attributes: []sdp.Attribute{
+						{Key: "ice-ufrag", Value: defaultUfrag},
+						{Key: "ice-pwd", Value: defaultPwd},
+					},
+				},
+				{
+					Attributes: []sdp.Attribute{
+						{Key: "ice-ufrag", Value: invalidUfrag},
+						{Key: "ice-pwd", Value: invalidPwd},
+					},
+				},
+			},
+		}
+
+		ufrag, pwd, _, err := extractICEDetails(s, nil)
+		assert.Equal(t, ufrag, defaultUfrag)
+		assert.Equal(t, pwd, defaultPwd)
+		assert.NoError(t, err)
+	})
+
+	t.Run("Missing pwd at session level", func(t *testing.T) {
+		s := &sdp.SessionDescription{
+			Attributes: []sdp.Attribute{{Key: "ice-ufrag", Value: "invalidUfrag"}},
 			MediaDescriptions: []*sdp.MediaDescription{
 				{Attributes: []sdp.Attribute{{Key: "ice-ufrag", Value: defaultUfrag}, {Key: "ice-pwd", Value: defaultPwd}}},
 			},
 		}
 
 		_, _, _, err := extractICEDetails(s, nil)
-		assert.Equal(t, err, ErrSessionDescriptionConflictingIcePwd)
+		assert.Equal(t, err, ErrSessionDescriptionMissingIcePwd)
 	})
 }