media: decide the DTLS setup role once, complementary to the offer - #175
Open
Mliviu79 wants to merge 24 commits into
Open
media: decide the DTLS setup role once, complementary to the offer#175Mliviu79 wants to merge 24 commits into
Mliviu79 wants to merge 24 commits into
Conversation
…ayload type WriteRTP takes rtcpMU to update the write statistics, but the codec lookup for the packet's payload type returns an error without unlocking. The session is then wedged permanently: every later WriteRTP blocks, and Close takes the same lock, so the dialog can never be torn down either. The lookup fails for any payload type absent from the session codec list, so a peer's choice of payload number is enough to trigger it. A session offering only PCMU also reaches it locally on the first DTMF write, since the DTMF writer stamps telephone-event. Unlock on defer instead. (cherry picked from commit fd58037)
Adds CodecAudioG722 and FORMAT_TYPE_G722 so payload type 9 resolves in CodecAudioFromPayloadType and mapSupportedCodec instead of falling through to the unnamed default codec. SDP generation gets an explicit rtpmap arm because the generic default appends the encoding parameters suffix which some peers do not match. SampleRate is 8000 as RFC 3551 requires for the G.722 RTP clock, not the 16 kHz rate the codec samples at. (cherry picked from commit 0daba53)
RFC 3551 assigns G.722 a static payload type, so a peer may offer bare "m=audio <port> RTP/AVP 9" with no a=rtpmap line. CodecsFromSDPRead only had static fast paths for ulaw and alaw, so such an offer resolved zero codecs and the call was rejected when G.722 was the only codec offered. Resolve payload type 9 the same way, which also keeps the RFC 3551 clock when a peer advertises the 16 kHz sampling rate in rtpmap. (cherry picked from commit 896bee6)
The Content-Type check in dialogHandleReferNotify accepts any message/sipfrag body, since RFC 3420 Section 5 makes the version parameter optional and PBXes such as 3CX omit it. Nothing pinned that behaviour, so a stricter prefix check could silently return 400 again and stop OnNotify from ever firing. Add table tests over the accepted and rejected Content-Type forms, plus a short body case. (cherry picked from commit cf9c0d1)
Refer returned as soon as the REFER was accepted with a 202, which only means the peer took the request for processing. A transfer that was later rejected still looked like a success to the caller. Refer now waits for the terminal sipfrag NOTIFY, bounded by a 30s answer-supervision deadline, and reports a failed transfer as a ReferFailureError carrying the SIP status and reason. ReferOptions with OnNotify keeps its existing behaviour and returns on the 202, leaving the outcome to the callback. NOTIFY correlation matches the RFC 3515 Event id against the REFER CSeq so a stale NOTIFY cannot resolve a later transfer, and the sipfrag status line is now parsed strictly. (cherry picked from commit 3f140a3)
Negotiation floors the interval by Min-SE, honors the peer's refresher and falls back to an advertised default only when the peer offers none. When we are the elected refresher a loop re-INVITEs at half the interval, otherwise a watchdog hangs up if the peer stops refreshing. Both run on the dialog context so teardown cancels them. An offer below the floor is rejected with 422 and a Min-SE header, and a timer-unaware peer is answered normally. Timers stay off unless WithSessionTimers is passed. sipgo v1.4.3 has no typed Session-Expires or Min-SE accessors, so the headers are read generically and parsed here. (cherry picked from commit 2ef255d)
Adds an ICE agent (RFC 8445) that can be enabled per media session via ICEConf, together with the rtcp-mux handling ICE forces. An ICE session nominates a single candidate pair, so it binds one socket instead of two and demultiplexes DTLS, RTP and RTCP off it. Each stream is presented as a net.PacketConn, so the existing DTLS handshake, SRTP contexts and RTP paths run over ICE unchanged. The DTLS conn is now built at Finalize on the ICE path, since there is no transport before connectivity checks nominate a pair. The non ICE path keeps building it during RemoteSDP so a server conn can still buffer a ClientHello that arrives before Finalize. (cherry picked from commit a365c5a)
Covers the packet classification of RFC 5764 and RFC 5761, the single socket bind layout and the SDP attributes an ICE offer carries, plus the negative cases: a peer that sends no ICE credentials or refuses rtcp-mux is rejected. TestICEDTLSSRTPSession negotiates two sessions end to end and sends media over the nominated pair, which exercises gathering, connectivity checks, the DTLS handshake over the mux and SRTP keyed from it. TestICEMuxCloseUnblocksReaders pins teardown: a reader parked on a stream must be released by Close, and Close is reached once per stream from MediaSession.Close. (cherry picked from commit 6be542a)
The ICE path rejected a peer that did not offer rtcp-mux by reading rtcpMux, but listenICE sets that field for every ICE session to record our own intent, so the check could never fire and a peer refusing rtcp-mux would have been accepted onto a session with nowhere to put RTCP. Track what the remote answered separately. The test covering this bound the session first, which is what production does, and it fails against the old check. (cherry picked from commit 7ff9217)
generateSDPForAudio gained an iceSetup parameter when the ICE transport landed; the G.722 negotiation test predates it. (cherry picked from commit 062f61b)
RFC 5764 section 5.1.2 multiplexes the DTLS handshake onto the socket that carries media, but the DTLS stack still treats that socket as its own. Every teardown path in it ends in nextConn.Close(): a fatal alert and a handshake timeout reach it through close(false), a peer close_notify through the read loop. Under ICE that conn is one stream of iceMux, and muxConn.Close releases the whole nominated pair. An alert on the handshake therefore tore down RTP and RTCP too, silencing a call whose media path was healthy. The DTLS association is not the session. Lend the socket to the stack instead of surrendering it: dtlsTransport now returns a wrapper whose Close is a no-op, on the ICE and the non-ICE path alike. The socket keeps being closed by MediaSession.Close, which owns it. (cherry picked from commit a9a7f1d)
The emiago/dtls fork exists for one field, StopReaderAfterHandshake. Its cost is high: the module rename makes it invisible to govulncheck, so its record layer keeps building GCM and CCM nonces from rand.Read (ciphersuite/gcm.go:63) long after upstream derived them from the epoch and sequence number per RFC 9325. It also pinned x/crypto and x/net two releases back. pion/ice already requires pion/dtls, so the fork bought a second, older DTLS stack in the same binary rather than replacing one. The field is not needed. DTLS-SRTP (RFC 5764 section 5.1.2) puts the handshake on the socket that carries media, and diago uses the association for nothing but the key exchange: never Read, never Write, only Handshake, ConnectionState and SelectedSRTPProtectionProfile. So the stack has no job once the keying material is exported, and the knob only encoded that. Left running, its read loop competes with ReadRTP for the same socket and drops what it takes as a malformed record per RFC 6347 section 4.1.2.7 -- 9 of the first 20 SRTP packets in the test added here, lost silently. Retire the stack explicitly instead: detach the transport, then close the conn. Detach first, so the close_notify that Close emits is dropped rather than sent, since the peer's association is still live and its SRTP keys hang off it. Close then retires the read loop and the handshake goroutine, and cannot reach the socket because detach has taken it away. This runs as the first statement after Handshake returns, which is where the fork ran the knob. media.DTLSConfig is unchanged, as is every exported symbol of the package. Pin v3.1.5: it serialises writePackets behind a mutex, and the flight sends race the read loop's alerts in this usage. Pre-existing failures unrelated to this change, identical on the parent commit: TestDiagoTransportConfs, TestIntegrationDialogServerReinvite, TestIntegrationPlaybackURL, TestMonitorPCMReaderWriter, TestMonitorPCMStereo. (cherry picked from commit ad4f119)
iceMux was the only importer of pion/transport/v3 anywhere in the tree, and it pulled a second copy of packetio into the binary alongside the v4 copy that pion/srtp, pion/dtls, pion/ice and pion/stun already share. The two packages are the same code: aside from the copyright year, the deadline import path and `copied := min(count, len(packet))` replacing an equivalent if/else, v3.1.1 and v4.0.2 packetio are byte-identical, and the API iceMux uses -- NewBuffer, SetLimitSize, Write, Read, Close -- is unchanged. So this is a no-op at runtime and drops the duplicate. go.mod here still resolves pion/srtp v3.0.6, which is a transport/v3-era release, so the fork's own module graph keeps v3 as an indirect. That graph is not what ships: diago is always built from the runtime through a replace directive, where minimal version selection lifts srtp to v3.0.11 -- a release that is itself on transport/v4. With this change the runtime's vendor tree and go.mod carry transport/v4 alone. Verified: go build and go vet clean; media tests pass under -race -count=2. Pre-existing and unrelated: TestRTPJitterBufferOverflow fails identically with this change stashed, and packetio has no importer besides ice_mux.go. (cherry picked from commit a7cb8b5)
… dead RTP destinations Codec matching compared the whole struct, so a peer that numbered a dynamic format differently than us matched nothing: an offer of opus 111 plus telephone-event 100 plus PCMA 8 negotiated PCMA alone, and an opus only offer was rejected outright. Codecs are now matched on encoding name, clock rate and channels, and the negotiated entry keeps local framing while taking the peer's payload type as RFC 3264 6.1 asks. SDPCodecPreferLocalOrder applies to the answerer, but the role was inferred from sessionID == 0, which is true both for a UAS applying its first offer and for a UAC applying the answer to its own INVITE. The role is a property of the body, so RemoteSDPIsAnswer is set by the caller and the dialog paths set it where they apply a response or ACK body. Fork dropped ExternalIP while LocalSDP regenerates on every call, so the 200 OK answering a re-INVITE advertised the internal bind address. It is carried now, and Init drops it when a session is rebinding to a new socket, since it only ever describes the socket it was derived from. A port of 0 means the stream is rejected and is also what an unparseable port leaves behind, and a non IP addrtype yields no IP at all. Both were latched, so the call answered and sat mute. They are refused now, along with the other negotiation failures, through ErrNoCommonCodec, ErrNoCommonCrypto and ErrNoCommonMedia so callers can answer 488 instead of counting a server error. Pre-existing failures unrelated to this change: TestDiagoTransportConfs, TestIntegrationPlaybackURL, TestMonitorPCMReaderWriter, and go vet on diago.go:785. (cherry picked from commit 5986baa)
…yload number Dynamic formats are numbered independently by each side from 96-127 (RFC 3551 section 3), and our answer echoes the number the offerer used (RFC 3264 section 6.1), so a negotiated codec reaches the SDP author carrying the peer's number. The author switched on the payload type and compared it against the package constants, which describes the format by its number rather than by what it is (RFC 4855 section 3). A peer numbering opus 107 or 111 lost a=fmtp entirely: only the generic default arm ran, which emits no fmtp. A peer numbering telephone-event 96 was described as opus, so the answer advertised opus twice, at two payload types, put opus' fmtp on the DTMF payload type, and never advertised telephone-event at all. Numbering opus 101 produced the mirror image. Key the dynamic formats on the encoding name, ahead of the static arms so that a dynamic format mapped onto a static number is still described by what it is. PCMU, PCMA and G.722 stay keyed by number: the RTP/AVP registry freezes those assignments (RFC 3551 section 6) and they can not be renumbered. Output for default numbered codecs is byte for byte unchanged. (cherry picked from commit 5a85a5d)
… set Both paths resolved the packet's codec from MediaSession.Codecs, the local list. Negotiation records the peer's payload type for a dynamic format (RFC 3264 section 6.1) in filterCodecs and never writes it back to Codecs, so the local list holds our own numbering and can not describe what is on the wire. The read path therefore rejected exactly the payload type our answer advertised, and accepted the one we never offered. It returned 0, nil without recording the SSRC, so the guard re-fired on every packet and the call was a silent one way blackhole with no error surfaced. The write path failed the lookup for the number it had just stamped. Resolve both through activeCodecs, which prefers the negotiated set and falls back to the local list when there is none. The fallback matters for the offer, and for Fork, which clones Codecs but deliberately not filterCodecs. This is the rule LocalSDP already applied inline; it now has one home. Also correct negotiatedCodec's comment, which asserted the read path gated on the peer's number. It did not. (cherry picked from commit af67044)
PCMDecoder.Init and PCMEncoder.Init switched on the payload type and compared it against the package constants. Every caller in tree hands them the negotiated media.Codec, whose payload type for a dynamic format is the number the peer chose (RFC 3551 section 3, RFC 3264 section 6.1), not ours. So opus failed to init at all on a call where the peer numbered it anything but 96: "not supported codec 107" from Asterisk, "not supported codec 111" from a WebRTC peer. The codec was negotiated, advertised and carried on the wire, and the audio path could not decode a frame of it. Select on the encoding name, which is what identifies a format (RFC 4855 section 3). PCMU and PCMA reach the same arms as before: their numbers are frozen (RFC 3551 section 6) and map to exactly one name. The FORMAT_TYPE_* constants stay: they are exported. Opus only compiles under the with_opus_c tag, so its coverage lives in a tagged file. Verified with and without it. (cherry picked from commit f5bf4c5)
Every DTMF reader and writer was built with the CodecTelephoneEvent8000 constant. telephone-event is a dynamic format, so the number carrying it is whatever negotiation settled on: each side picks its own from 96-127 (RFC 3551 section 3) and an answer echoes the offerer's (RFC 3264 section 6.1). The constant only describes our own default numbering. Against a peer that numbers telephone-event anything but 101 this broke DTMF in both directions. Outgoing events went out at 101, a payload type the peer never offered, so it dropped them. Incoming events failed the reader's payload type comparison, were not recognised as DTMF, and their RFC 4733 payloads fell through into the audio decoder as if they were speech. Resolve the codec from the session by encoding name (RFC 4855 section 3). A session that negotiated no telephone-event keeps the constant, which is the behaviour it had before. (cherry picked from commit c767973)
MediaConfig.RTPPortAllocator fills the RTPPortStart/RTPPortEnd TODO on MediaConfig. It supersedes the media package globals, which express only one range per process and only a linear scan over it, and lets a caller supply a pool, tiers or a drain window without this package knowing about any of it. Nil keeps the previous OS and globals behaviour. The port is released in Close only after the sockets are down so a drain window measures from the moment the wire is actually down, and released immediately when Init fails, which never reaches Close. The offer-less re-INVITE reply no longer dereferences a nil media session: a request for an offer can not be answered with an SDP that was never built, so it is rejected the same way the bodied path already rejects it. The keep alive guard also checks body length rather than nilness, as Content-Length: 0 can reach a handler as an empty but non nil body. (cherry picked from commit b403d5b)
MediaConfig gains DTLSConfig and ICEConfig, so a caller holding one Diago can turn on DTLS-SRTP and ICE without describing them per transport. ICE had no way in at all: a media session honours ICEConf, but nothing outside the media package ever built a session carrying one, so the agent could never run. The unexported dtlsConf is gone rather than joined by a second field saying the same thing. It only ever carried Transport.MediaDTLSConf, which stays and still applies; the new pointer is opt in and wins when set, as a value config cannot say "unset". Both dialog paths now build their media config in one place, which is what let ICE go missing from one of them. ICE is still only signalled on the DTLS profile, so a transport wanting it sets MediaSRTP to SecureRTPModeDTLS. (cherry picked from commit bdb9d40)
initMediaSessionFromConf reads RTPPortAllocator and SecureRTPAlg from the per dialog media config, but the config a dialog starts from never copied either from the Diago wide one. WithMediaConfig could set them and every dialog still bound an OS chosen port with the default SRTP profile, with nothing to show for it. The existing allocator test built a config by hand and called initMediaSessionFromConf directly, so it passed either way. Cover the propagation instead. (cherry picked from commit 8f13b44)
Two comments in the session-timer parser cited sections that say something else. The compact form of Session-Expires is defined in §4, not §7.1 — §7.1 is the Session-Expires header syntax. And the leniency note attributed to §7.4 does not belong there: §7.4 is "Generating Subsequent Session Refresh Requests", not receiver behaviour for headers it cannot parse. Treating a malformed timer header as absent is a deliberate local choice, so the sentence now says that plainly rather than borrowing authority from a section that does not grant it. No behaviour change. RFC 4028 §4 for the floor and §6 for the 422 are accurate and stay as they are. (cherry picked from commit 2111822)
…he answer RemoteSDP matched m= against a list of profile spellings. A peer offering RTP/SAVPF fell to the default arm and the call died at answer: no common media: unsupported media description protocol proto=RTP/SAVPF The m= proto is not an opaque token. It is two orthogonal axes over an optional DTLS association (RFC 4566 s5.14): S = SRTP (RFC 3711), F = the AVPF feedback profile (RFC 4585), and a UDP/TLS/ prefix meaning the keys come from a DTLS association (RFC 5764). Parse the axes and the whole matrix classifies, including spellings this stack has not met before. Keying is a separate question from security, and the proto does not answer it. RFC 5764 named the DTLS profiles UDP/TLS/RTP/SAVP(F), but a peer may equally signal DTLS-SRTP under the plain RTP/SAVP(F) name and carry a=fingerprint (RFC 5763 s5, RFC 4572 s5). Reading the keying mechanism off the proto misclassified such an offer as SDES and then rejected it for carrying no a=crypto -- a bug against the peer's legal SDP. Take security from the profile and keying from the attributes. Mirror the profile in the answer. RFC 3264 s6: "the answer MUST contain the same [...] transport protocol as the offer". LocalSDP builds both the offer and the answer, and only the offer's profile is ours to choose; before this an RTP/SAVPF offer was answered UDP/TLS/RTP/SAVP and an RTP/AVP offer could be answered RTP/SAVP, each a different protocol than the one offered. RTPProfileSAVPDisable therefore narrows to the offer path, where a profile still has to be chosen; it cannot express an answer's profile because an answer's profile is not a choice. Carry SecureRTP and SRTPAlg through Fork. They are properties of the negotiated session, not of one exchange within it, and LocalSDP keys every secure branch off SecureRTP -- so a zeroed fork answered a re-INVITE on an encrypted call with plain RTP/AVP and no crypto. iceEnabled() is ICEConf != nil && SecureRTP == DTLS, so the same zero silently dropped ICE from the answer while ICEConf suggested it was carried. A peer is entitled to read that answer as a downgrade attack. Tests drive the axis matrix rather than any one peer's offer. Pre-existing and untouched: TestRTPJitterBufferOverflow fails on the unmodified tree as well. (cherry picked from commit cf69d9e)
The role we advertised and the role we played were two independent decisions, and they disagreed. LocalSDP picked a=setup from whether a remote address was known -- "we do have remote IP, so probably we are server" -- and advertised passive. RemoteSDP separately picked the handshake role from the peer's a=setup, and for an actpass offer made us the client. So against Asterisk, which offers actpass, we told the peer to initiate and then initiated ourselves. Both endpoints sent a ClientHello and the peer aborted: dtls conn handshake: handshake error: alert: Alert Fatal: UnexpectedMessage Derive both from one function. RFC 4145 section 4.1: active initiates, passive accepts, so an answer must be the complement of the offer -- active to a passive offer, passive to an active one. RFC 5763 section 5 for the case the peer defers: "setup:active allows the answer and the DTLS handshake to occur in parallel. Thus, setup:active is RECOMMENDED", a passive answer merely stalls the handshake until the answer arrives. And where we make the offer: "The endpoint that is the offerer MUST use the setup attribute value of setup:actpass". Knowing the remote address is not evidence of answering. An offerer knows exactly who it is calling, which is why the inference produced a passive answer to an actpass offer. Whether an offer was applied is what decides it, and RemoteSDP already records that. The SDPSetupRole callback and DTLSRole still govern where they are set; localDTLSSetup reads both. Neither is set anywhere in this tree, so the inference was what ran. Tests drive the RFC 4145 role table, and assert the invariant the bug broke directly: whatever a=setup we advertise, the handshake role must match it. Pre-existing and untouched: TestRTPJitterBufferOverflow fails on the unmodified tree as well. (cherry picked from commit 23174a7)
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.
Fixes #174
Stacked on #173 — it needs
ICEConf,DTLSRole,iceMux,rtcpMuxandSecureRTPfrom the ICE commit, andRemoteSDPIsAnswerandExternalIPfrom the codec/role commit. It will show those commits until #173 merges.Two commits, deliberately separable.
1. Decide the role once
localDTLSSetupbecomes the single source for the role, anddtlsActsAsClientreads the same function, so the value advertised ina=setupand the role played in the handshake cannot diverge.RemoteSDPnow records the peer'sa=setupinstead of acting on it directly.The arms are RFC 4145 section 4.1's legal-pairs table — peer
activeanswerspassive, peerpassiveanswersactive— and foractpass, RFC 5763 section 5: "setup:active allows the answer and the DTLS handshake to occur in parallel. Thus, setup:active is RECOMMENDED.""Am I answering" now comes from whether an offer was applied, not from whether the remote address is known. The
SDPSetupRoleandDTLSRoleoverrides are preserved.This commit alone fixes the Asterisk interop failure and leaves
TestIntegrationDiagoDTLSCalluntouched and passing. If you want only the interop fix, take this commit.2. The offerer advertises
actpassRFC 5763 section 5 requires it, and RFC 8842 section 5.2 restates it. This is the commit that changes what a peer sees on an initial offer, and it is the reason the test fixture changes — kept separate so it can be argued on its own.
The test change, and why it is here
TestIntegrationDiagoDTLSCallconfigures its caller withMediaDTLSConf: media.DTLSConfig{}and the comment "We do not need any Certificate verification". With no certificate,LocalSDPsizes fingerprints bylen(DTLSConf.Certificates), so that endpoint offersUDP/TLS/RTP/SAVPwith zeroa=fingerprintlines.Once the offerer advertises
actpass, the answerer takes the RECOMMENDEDactive, and by complementarity the caller becomes the DTLS server — the one role pion checks for a certificate. It fails withno certificates configured.The fixture now supplies
testdata.ClientCertificate()andServerClientAuthRequireCert. Both lines are required: a certificate alone still fails, because as server it then parses the peer's fingerprint andServerClientAuthNoCertsuppresses theCertificateRequestthat RFC 8122 section 6.2 makes mandatory.The edit lives in commit 2, the commit that makes it necessary. Commit 1 does not touch the test.
Verification
Built on #173.
go build ./...andgofmt -l .clean.ok github.com/emiago/diago 1.104sFAIL ... dtls fatal: no certificates configuredok github.com/emiago/diago 1.082sFull suite matches the base exactly —
TestDiagoTransportConfs,TestMonitorPCMReaderWriter, and the root-package timeout inTestIntegrationDialogClientReinviteMedia, all of which do the same on unmodified4c7195d. No failures added.