search

bluenviron / mediamtx

Ready-to-use SRT / WebRTC / RTSP / RTMP / LL-HLS media server and media proxy that allows to read, publish, proxy, record and playback video and audio streams.
MIT License
11.9k stars 1.5k forks source link

WHIP/WHEP: DELETE not properly handled #3177

Closed neilyoung closed 5 months ago

neilyoung commented 6 months ago

Which version are you using?

v1.6.0

Which operating system are you using?

Describe the issue

Description

Describe how to replicate the issue

First of all: BIG, BIG THANKS for supporting WHIP/WHEP :) That's really a big improvement for this project. Didn't check that for a couple of months, but when did you add that?

However, I think I'm having a little issue with the current realisation. According the the standard a TEAR DOWN of a WHIP session (and most likely also a WHEP session, but not explicitly checked) has to be done with a HTTP DELETE to the resource published (or subscribed) so far.

I might do a mistake but it seems, this request is not properly handled at the moment. At least I'm catching a 404.

DELETE /whip/0822db29-fe7b-49e9-b8c5-8601c716e274 HTTP/1.1
Host: jetson:8889
Connection: keep-alive
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36
Accept: */*
dnt: 1
Origin: null
Accept-Encoding: gzip, deflate
Accept-Language: de-US,de;q=0.9,en-US;q=0.8,en;q=0.7,es-AR;q=0.6,es;q=0.5,de-DE;q=0.4

HTTP/1.1 404 Not Found
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Content-Type: text/plain
Server: mediamtx
Date: Fri, 29 Mar 2024 14:44:09 GMT
Content-Length: 18

404 page not found

EDIT: Updated the Wireshark trace. WHIP client is OBS Studio 30.1.0, MacOS

Just a minor issue, but should be corrected, I guess.

Did you attach the server logs?

no

Did you attach a network dump?

yes, inline

neilyoung commented 6 months ago

OK, I got some thumbs up, but no ack.

Anyway, MediaMTX definitely behaves incorrectly here. According to the RFC https://datatracker.ietf.org/doc/html/draft-murillo-whep-02 (here for WHEP, but that's no diff to WHIP) ...

To explicitly terminate a session, the WHEP Player MUST perform an HTTP DELETE request to the resource URL returned in the Location header field of the initial HTTP POST. Upon receiving the HTTP DELETE request, the WHEP resource will be removed and the resources freed on the Media Server, terminating the ICE and DTLS sessions.

That means to me - and the sample below this statement proves it-, that the returned "location" header MUST contain a FQDN, not only a part of a path. I think OBS - as strong reference - does see it the same way, because it does this to terminate a session initiated via WHIP (in my case to http://jetson:8889/mystream/whip):

1) It gets a 201 CREATED from MediaMTX, containing just a part of the FQDN required: Location: whip/d470351d-1380-437b-9de9-6ad6aca49bc4 2) It then issues a HTTP DELETE to this partly URL, which fails with 404

MediaMTX response to WHIP 

HTTP/1.1 201 Created
Accept-Patch: application/trickle-ice-sdpfrag
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: ETag, ID, Accept-Patch, Link, Location
Content-Type: application/sdp
Etag: *
Id: d4befba0-c478-4187-8a27-75efe24e99b6
Location: whip/d470351d-1380-437b-9de9-6ad6aca49bc4
Server: mediamtx
Date: Wed, 03 Apr 2024 11:16:23 GMT
Content-Length: 1301

OBS Delete request and MediaMTX response

DELETE /whip/d470351d-1380-437b-9de9-6ad6aca49bc4 HTTP/1.1
Host: jetson:8889
Accept: */*
User-Agent: Mozilla/5.0 (OBS-Studio/30.1.0; Mac OS X; en-US)

HTTP/1.1 404 Not Found
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Content-Type: text/plain
Server: mediamtx
Date: Wed, 03 Apr 2024 11:18:35 GMT
Content-Length: 18

404 page not found

In fact, if the DELETE request is directed to a string concatenation of the stream name plus the returned "location", it works.

Like:

curl http://jetson:8889/mystream/whip/d470351d-1380-437b-9de9-6ad6aca49bc4 -X DELETE

But as said: This is not correct and IMHO no client supports that.

neilyoung commented 6 months ago

An - admittedly clumsy - attempt to solve this. What do you think? Worth a PR?

diff --git a/internal/servers/webrtc/http_server.go b/internal/servers/webrtc/http_server.go
index 95bddf2b..1b6614d5 100644
--- a/internal/servers/webrtc/http_server.go
+++ b/internal/servers/webrtc/http_server.go
@@ -203,7 +203,11 @@ func (s *httpServer) onWHIPPost(ctx *gin.Context, path string, publish bool) {
        ctx.Writer.Header().Set("ID", res.sx.uuid.String())
        ctx.Writer.Header().Set("Accept-Patch", "application/trickle-ice-sdpfrag")
        ctx.Writer.Header()["Link"] = webrtc.LinkHeaderMarshal(servers)
-       ctx.Writer.Header().Set("Location", sessionLocation(publish, res.sx.secret))
+       scheme := ctx.Request.URL.Scheme
+       if scheme == "" {
+               scheme = "http"
+       }
+       ctx.Writer.Header().Set("Location", scheme + "://" + ctx.Request.Host + ctx.Request.URL.Path + "/" + res.sx.secret.String())
        ctx.Writer.WriteHeader(http.StatusCreated)
        ctx.Writer.Write(res.answer)
 }

EDIT: Obsolete, see https://github.com/bluenviron/mediamtx/pull/3195

neilyoung commented 6 months ago

Created a PR with not much hope https://github.com/bluenviron/mediamtx/pull/3195

aler9 commented 5 months ago

Hello,

The WHIP/WHEP specification doesn't explicitly state that an absolute URL is required inside the location header, although in all examples there's an absolute URL, thus this behavior is recommended.

The OBS implementation is incorrect, since relative paths that don't start with a slash, inside the Location header, should be appended to the path of the request, discarding anything after the last slash, per RFC3986:

http://a/b/c/d;p?q + g;x = http://a/b/c/g;x

Therefore

http://jetson:8889/mystream/whip + whip/d470351d-1380-437b-9de9-6ad6aca49bc4 = http://jetson:8889/mystream/whip/d470351d-1380-437b-9de9-6ad6aca49bc4

Therefore this issue is not about specifications, but is about switching to a common practice in order to broaden compatibility.

The server prefers to send relative paths in place of absolute URLs in order to support reverse proxies and gateways, therefore the effects of switching to absolute URLs must be investigated further.

neilyoung commented 5 months ago

Thanks for the follow up. In fact, it's not only OBS. I have at least three OS projects which do exactly the same: Taking the location header "as is" and do a DELETE on this (or PATCH)

aler9 commented 5 months ago

Then i think that the best course of action is using full paths (prefixed with a slash) inside the location header:

Location: /mystream/whip/d470351d-1380-437b-9de9-6ad6aca49bc4

The problem of this approach is that we will lose compatibility with reverse proxies that serve MediaMTX in a subfolder. For instance, i usually use a gateway to proxy requests, originally addressed to /api/v1/live/video/mystream/whip, to MediaMTX in /mystream/whip, stripping the first 4 elements of the path. I guess the reverse proxy / gateway will also have to edit the Location header in order to change the path there too.

neilyoung commented 5 months ago

But there should be a way to detect, if there is a reverse proxy, e.g. by header info, shouldn't that be possible?

neilyoung commented 5 months ago

But let me check your proposal quickly...

neilyoung commented 5 months ago

I think that's going to work. Let me adjust my PR accordingly. I have tested it with OBS and one of the mentioned OS apps and it worked for me. Even behind NGINX as reverse proxy

neilyoung commented 5 months ago

Updated PR https://github.com/neilyoung/mediamtx/commit/2cec9d747cfcf3370c7d7909420e06ab91e638b7

neilyoung commented 5 months ago

That works for WHIP and WHEP

I found another little issue while testing this, maybe you have an idea. It's not 100% related, but because I ran into this, I will try to explain it here.

My server has a UFW firewall which is configured like so (just the IPv4 rules):

22/tcp                     ALLOW       Anywhere                  
8554/tcp                   ALLOW       Anywhere                  
8889/tcp                   ALLOW       Anywhere                  
8189/udp                   ALLOW       Anywhere                  
443/tcp                    ALLOW       Anywhere

With this setup I'm able to use all of the 3rdparty apps, but not OBS. OBS cannot establish the RTP connection as long as the firewall is up. Would you know, what else I would have to open for OBS?

As anticipated (from the OBS log):


14:40:27.289: [obs-webrtc] [whip_output: 'adv_stream'] PeerConnection state is now: Connecting
14:40:27.776: ==== Streaming Start ===============================================
14:41:07.375: [obs-webrtc] [whip_output: 'adv_stream'] PeerConnection state is now: Failed
neilyoung commented 5 months ago

Maybe worth another issue, but we are in a discussion here. OBS is not sending any PATCH request, which would point to that they where doing trickle-ice (even though the SDP contains that claim). They send my PC's local network address. That doesn't harm for WHIP, I guess, because they are just supposed to SEND to MediaMTX.

a=candidate:1 1 UDP 2130706431 fd00::876:d18e:36d:acaf 59791 typ host
a=candidate:2 1 UDP 2122317567 192.168.188.53 59791 typ host

I stumbled over the candidates MediaMTX is sending the the ANSWER. There is the local IP address of the AWS instance with port 8189, which will surely not be reachable. Then there are srflx candidates with public IP addresses on higher ports (>59000), which are OK. And relay candidates, also in that high port range, also public.

So I opened the UDP port range, which my STUN/TURN server is supposed to use (COTURN), and it worked. Maybe you add that info to the documentation somewhere: If one configures STUN/TURN then a possibly used server firewall has to be open for the potential port range, this server provides.

neilyoung commented 5 months ago

As another idea: Why don't just add a configuration for the public IP/port of the server? This would remove the requirement for STUN/TURN at all (like Mediasoup does it). This has two advantages: The public IP doesn't have to be retrieved by STUN server-side and there is no need for opening a potentially wide range of UDP ports.

Other than that - and unrelated again - I would suggest to publish the Link header for WHEP responses only. There is no need for publishing possibly volatile long term TURN credentials to everybody in the world.

It would also be great, if you would provide - maybe a callable interface - for creating time limited credentials instead of a fix TURN credential provisioning.

Sorry for the long post(s)

EDIT: Line 205 ff in http_server.go

    if !publish {
        ctx.Writer.Header()["Link"] = webrtc.LinkHeaderMarshal(servers)
    }
aler9 commented 5 months ago

As another idea: Why don't just add a configuration for the public IP/port of the server? This would remove the requirement for STUN/TURN at all (like Mediasoup does it). This has two advantages: The public IP doesn't have to be retrieved by STUN server-side and there is no need for opening a potentially wide range of UDP ports.

This is already implemented, enabled by default and can be tuned in this way:

# Address of a local UDP listener that will receive connections.
# Use a blank string to disable.
webrtcLocalUDPAddress: :8189
# Address of a local TCP listener that will receive connections.
# This is disabled by default since TCP is less efficient than UDP and
# introduces a progressive delay when network is congested.
webrtcLocalTCPAddress: ''

You also need to remove all STUN/TURN servers in order to use it.

Maybe worth another issue, but we are in a discussion here. OBS is not sending any PATCH request, which would point to that they where doing trickle-ice (even though the SDP contains that claim). They send my PC's local network address. That doesn't harm for WHIP, I guess, because they are just supposed to SEND to MediaMTX.

Remove all STUN/TURN servers and set the IP manually, as mentioned in the README:

webrtcAdditionalHosts: [my.ip.add.ress]

Other than that - and unrelated again - I would suggest to publish the Link header for WHEP responses only. There is no need for publishing possibly volatile long term TURN credentials to everybody in the world.

TURN credentials are not broadcasted publicly but are sent in WHIP and WHEP responses only. Turning them off for WHIP responses would have the effect of preventing people from publishing.

It would also be great, if you would provide - maybe a callable interface - for creating time limited credentials instead of a fix TURN credential provisioning.

This is already implemented, take a look at the part about "secret-based authentication" in the README.

neilyoung commented 5 months ago

This is already implemented, enabled by default and can be tuned in this way:

I think I don't understand. In which way is webrtcLocalTCPAddress the configuration point for the public IP of the server, given the comment provided?

webrtcAdditionalHosts

Sorry, what purpose has this setting?

TURN credentials are not broadcasted publicly but are sent in WHIP and WHEP responses only.

Well, that's what I meant by "broadcast"

Turning them off for WHIP responses would have the effect of preventing people from publishing.

Nope. In which way? It makes no sense to return TURN servers or even STUN servers in the 201 response header. This information is meant to support the client to gather its candidates. But for what purpose should a WHIP (I don't talk about WHEP) client send any candidate at all? There is no media traffic from the server back to the WHIP client, which would make this information necessary.

I have causally removed the provisioning of TURN/STUN information from the Link header in WHIP case and could not find any problem over various types of connections.

This is already implemented, take a look at the part about "secret-based authentication" in the README.

OK, this wasn't obvious to me. I'll give it a try.

BTW: GH Actions sent me a problem https://github.com/bluenviron/mediamtx/actions/runs/8615767557. The problem is: I was doing the changes completely in the web editor of github. How am I supposed to run the formatter on this after the change?

aler9 commented 5 months ago

You can configure a fixed public IP and a fixed port, either TCP or UDP, by using together webrtcLocalUDPAddress, webrtcLocalTCPAddress and webrtcAdditionalHosts, this is explained in the README:

https://github.com/bluenviron/mediamtx?tab=readme-ov-file#connectivity-issues

Nope. In which way? It makes no sense to return TURN servers or even STUN servers in the 201 response header.

Now i understand the observation, i checked the code and i think you're right! i will remove the Link header from POST responses ASAP.

neilyoung commented 5 months ago

Thank you very much for your patience with me and have a nice weekend :)

BTW: It could be helpful to have the TURN/STUN servers in the WHEP response, even though that would need to perform an ICE restart on the client side after having received the ANSWER. Anyway, this is the only relevant direction, which could make use of it

EDIT: Or to send it in the preflight response, which is allowed by the RFC

aler9 commented 5 months ago

I checked again all the places in which STUN/TURN servers are returned to users which are

Although it's apparently useless to send TURN servers in POST responses, this is dictated by the specification (https://datatracker.ietf.org/doc/draft-ietf-wish-whip/) and also helpful to perform ICE restarts, as you mentioned.

Therefore, the way in which servers are sent to users is right and the server doesn't seem to need any improvement.

TURN servers cannot be sent in preflight responses since the latter are unauthenticated.

neilyoung commented 5 months ago

Although it's apparently useless to send TURN servers in POST responses, this is dictated by the specification

I think the spec says MAY...

TURN servers cannot be sent in preflight responses since the latter are unauthenticated.

TURN credentials are part of the response, and if it is sent in OPTIONS it could be used for the creation of the ICE candidates to be sent in the initial offer. When it comes to the offer it is too late.

in POST responses, after a successful authentication:

I'm not using auth at all at the moment and it is not sent in OPTIONS then. I don't see a reason for having to be authenticated just for that. If the TURN credentials are life-time limited there is no danger of abuse.

Regarding AUTH: Is it correct, you are supporting Basic Auth? I'm 10% sure, this is not part of the spec, since this requires token authentication.

neilyoung commented 5 months ago

Remove all STUN/TURN servers and set the IP manually, as mentioned in the README:

webrtcAdditionalHosts: [my.ip.add.ress]

Confirmed. That works fine. Thanks for pointing me to that. IMHO, completely obsoletes STUN/TURN. However, there is still the IP of the used network propagated, which is misleading in my case, because AWS local. Can this be prevented?

neilyoung commented 5 months ago

This is already implemented, take a look at the part about "secret-based authentication" in the README.

OK, this wasn't obvious to me. I'll give it a try.

That works too. The lifetime of 24h is a bit too generous in my eyes, but ok. That's fine.

Also thanks for pointing me on that :) You know, I'm a fan of your solution, so please don't get me wrong.

aler9 commented 5 months ago

Can this be prevented?

yes, there is webrtcIPsFromInterfaces: no, i think you better take a look at the configuration file and scroll through all available options.

Since both OBS and the Eyevinn WHIP demo are both working without changing anything, can you write the name of a WHIP client that is currently not liking relative Locations?

neilyoung commented 5 months ago

Since both OBS and the Eyevinn WHIP demo are both working without changing anything, can you write the name of a WHIP client that is currently not liking relative Locations?

OBS 30.1.2 on Mac

neilyoung commented 5 months ago

i think you better take a look at the configuration file and scroll through all available options.

Yes, you are right. Sorry for bother.

neilyoung commented 5 months ago

Since both OBS and the Eyevinn WHIP demo are both working without changing anything, can you write the name of a WHIP client that is currently not liking relative Locations?

OBS 30.1.2 on Mac

DELETE is also and still not working with the latest commit a18bebfa58616ffdc5fed8f83f284585d1744ab8. I'm really sorry

aler9 commented 5 months ago

Hello, you're right and DELETE requests sent by both OBS and Eyevinn/whip were not working. This is definitively fixed by #3240. The reason why i forgot to check is that there are too many topics mentioned in this issue, therefore i just checked for connectivity and CORS.

Next time feel free to create an issue for each single issue (for instance, separate ones for connectivity, CORS and DELETE), in this way each topic can be handled separately.

neilyoung commented 5 months ago

Hehe, sorry for the mess. Thanks for the fix. Will checkout later

github-actions[bot] commented 5 months ago

This issue is mentioned in release v1.7.0 🚀 Check out the entire changelog by clicking here

neilyoung commented 5 months ago

Working. Thanks