Skip to content

Commit

Permalink
Only collect single fingerprints/ICE credentials
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@Sean-Der
Nils Ohlmeier authored and Sean-Der committed Nov 14, 2024
1 parent 363e017 commit 2fd3640
Show file tree
Hide file tree
Showing 6 changed files with 247 additions and 71 deletions.
2 changes: 1 addition & 1 deletion mediaengine_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion peerconnection_go_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions peerconnection_media_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions peerconnection_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
168 changes: 114 additions & 54 deletions sdp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit 2fd3640

Please sign in to comment.