search

membraneframework / membrane_core

The core of the Membrane Framework, multimedia processing framework written in Elixir
https://membrane.stream
Apache License 2.0
1.3k stars 37 forks source link

Block clients from connecting to RTMPServer #890

Open PJUllrich opened 2 weeks ago

PJUllrich commented 2 weeks ago

Hey folks, I wondered whether it's possible to block new clients from connecting to a RTMPServer if they - for example - don't have a valid stream_key. This would be important to me to prevent illegal streams to my server.

What I currently do is to implement my own ClientHandler (just a copy of the ClientHandlerImpl module) which returns nil from handle_stream_published if the stream_key is invalid. This doesn't prevent the client from connecting, but it breaks the state of the ClientHandler which leads to an exception in the next callback and to the disconnection of the client.

It would be nicer though to be able to control the connection e.g. in the handle_new_client callback instead. I would like to return something like {:stop, message} to decline the connection.

Thanks for your much appreciated help in advance :)

dmorn commented 2 weeks ago

Adding some more context, if you use the RTMP.Source/SourceBin, and you curl the RTMP port (just open a TCP connection and close it basically), the element is going to crash as the handler (client handler) is going to be nil and hence won't be able to respond to the callback.

If you use the plain RTMP.Source element (because you want to handle FLV demuxing by yourself for example), everything also falls apart when you try to open 2 valid RTMP connections. The listener is going to accept it, then everything breaks.

I think right now the implementation is kind of broken. I think there are two usecases to address:

  1. One RTMP server that spawns a pipeline for each connection. As mentioned above, in case of unwelcome connections we should politely decline them (avoid crashing)
  2. One RTMP server, one pipeline, one connection at a time. In that case, the listener should not accept new connections while we have a working one.

I don't see any other usecase. While we discuss (cc @varsill), I'm going to work on 2.

varsill commented 1 week ago

Hello! @PJUllrich - Good observation. However I think that in the newest release of the rtmp_plugin there might already be some kind of a cleaner solution for that problem. There is no more handle_stream_published callback and the user needs to provide handle_new_client callback instead (this one is passed as RTMP server option). The handle_new_client callback needs to return a module that describes ClientHandler behaviour. Depending on whether you accept or reject the given app/stream_key, you could technically return different ClientHandler implementations. The ClientHandler implementation for the rejected app/stream_key could just not send data to the pipeline.

I agree with you, @dmorn that these are two valid use cases. The first one is meant to be usually solved with an external server, while the other one is mean to be solved with built_in server. With the built_in scenario, we ignore any new connection on any stream_key and app different than the one provided in URL. Because we don't demand for data from the client handler assigned to that newly connected stream_key and app, the TCP connection to the client will be closed after the client_timeout.
However, as far as I can see, the problem might arise if two clients were connection on the same app/stream_key, as it would probably make RTMP.Source fail because it would try to return setup: :complete for the second time. I think we should ignore the second connection on the same app/stream_key.