From e0711194fe148267c01659a8e80899ba8bd18707 Mon Sep 17 00:00:00 2001 From: Nils Ohlmeier Date: Wed, 13 Nov 2024 20:50:30 -0500 Subject: [PATCH] Only collect single fingerprints/ICE credentials 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. Fixes #2621 --- mediaengine_test.go | 2 +- peerconnection_go_test.go | 4 +- peerconnection_media_test.go | 3 + peerconnection_test.go | 1 + sdp.go | 168 ++++++++++++++++++++++++----------- sdp_test.go | 140 +++++++++++++++++++++++++---- 6 files changed, 247 insertions(+), 71 deletions(-) diff --git a/mediaengine_test.go b/mediaengine_test.go index d598c03cb89..fa3cc2d8994 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 d7e88da4131..3de9acf5f2a 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 74863148a53..a9450096334 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 12811ebce22..a87ba1fbb81 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.go b/sdp.go index 3dd79b50a03..10a24edc405 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 { diff --git a/sdp_test.go b/sdp_test.go index e3b9bf9bfda..46a3b8de504 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) }) }