Only collect single fingerprints/ICE credentials

The way currently DTLS fingerprints and ICE credentials
are picked is causing interop issues as described in pion#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.

Fixes pion#2621

Author: nils-ohlmeier

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

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

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 {

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

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 {