Fix SDP media transport defaulting to RTP/AVP regardless of SRTP - #22
Open
geejay101 wants to merge 1 commit into
Open
Fix SDP media transport defaulting to RTP/AVP regardless of SRTP#22geejay101 wants to merge 1 commit into
geejay101 wants to merge 1 commit into
Conversation
SDPSession's SDP-building code (updateSDP, updateRtpSDP, and the two early/default-fallback media construction sites) always defaulted a media's transport to the literal "RTP/AVP", even when SRTP (m_secure) was in effect and an a=crypto attribute was being attached. Per RFC 4568, crypto attributes require the RTP/SAVP profile, not RTP/AVP -- offering crypto under AVP is non-conformant, and at least one common softphone/PBX peer rejects it outright with 488/INCOMPATIBLE_DESTINATION. Default the transport to RTP/SAVP when m_secure is set, at every media construction site, instead of hardcoding RTP/AVP unconditionally. Also suppress the a=encryption:optional line once transport is already RTP/SAVP, since it's redundant there. modules/ysipchan.cpp: an outgoing call to a sips: destination now defaults m_secure to true for that leg (sips() is resolved from the parsed destination URI immediately beforehand), and clears any transport/ transport_* parameters that may have already been set on the message from an unrelated leg's own media negotiation, so the corrected default above actually gets a chance to apply instead of being silently pre-empted. An incoming call's own leg now defaults m_secure to true unconditionally, so SRTP offered by a caller is always recognized/preserved regardless of the global [general] secure= setting; this remains fully overridable per call via the existing secure= message parameter, since YateSIPConnection:: callAccept already narrows m_secure from an explicit "secure" parameter on the call.execute message. libs/ysdp/session.cpp + yatesdp.h: SDPSession::updateRtpSDP is a static member function and has no access to m_secure, so it gains an explicit "secure" parameter instead, passed as m_secure from both call sites.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SDP-building paths in SDPSession (updateSDP, updateRtpSDP, and the two early/fallback media construction spots) always set a media's transport to RTP/AVP. That happened even when SRTP was on (secure=enable) and we were attaching an a=crypto attribute. That's wrong per RFC 4568: crypto belongs under the RTP/SAVP profile, not RTP/AVP. Offering crypto under AVP isn't conformant, and some softphone/PBX peers reject it outright with 488 INCOMPATIBLE_DESTINATION.
The fix is to default transport to RTP/SAVP whenever secure is set, at each place we build a media descriptor, rather than hardcoding RTP/AVP. While we're there, we drop the a=encryption:optional line once transport is already RTP/SAVP, since it's redundant at that point.
Outgoing calls to a sips: destination now default m_secure to true for that leg. (sips() is resolved from the parsed destination URI just before this.) We also clear any transport/transport_* params that a previous leg's media negotiation may have left on the message, otherwise they'd pre-empt the corrected default and it'd never apply.
updateRtpSDP is a static member and can't see m_secure, so it takes an explicit secure parameter instead. Both call sites pass m_secure through.