search

fedora-infra / fmn.sse

THIS REPOSITORY HAS BEEN REPLACED BY https://github.com/fedora-infra/fmn
GNU General Public License v2.0
2 stars 5 forks source link

Do not allow the creation of server resources with a GET request #55

Open jeremycline opened 7 years ago

jeremycline commented 7 years ago

Currently, when a client makes a request for a URL like /user/jcline/, the SSE server will create an exchange with the name user if it doesn't already exist, and a queue with the name jcline and the routing key user-jcline. This is problematic for two reasons. The first is it is very unexpected to create resources on a server with GET. Secondly, this opens up a simple denial of service attack on RabbitMQ. A malicious user could make millions of requests that they immediately close which will create millions of exchanges and millions of queues. I don't know at what point RabbitMQ falls over, but it will fall over by either running out of memory or hitting other internal limits on the number of queues/exchanges.

The solution is to use passive=True when declaring the exchange and queue, which merely checks to see if they exist. If they do not, it returns an error which we should turn into an HTTP 404 to indicate to the client the resource does not exist.

This means that it is the responsibility of someone else (the FMN consumer seems reasonable) to create the queues and exchanges that the SSE server uses.

pypingou commented 7 years ago

I wonder if the weakest bit here wouldn't be that well-known Too many open files which is something we should be able to test then (by default max open file / process is 4096 iirc).

That being said :+1: to the general idea of this ticket

pypingou commented 7 years ago

This however does raise the question of new user/comers.

Do we want them to come up in hubs and have nothing shown because FMN hasn't started to send them notifications? Should the hubs client try to reconnect every X seconds in that situation? (Should we then send something else than a 404 to make it easier to detect that we're in that situation instead of querying the wrong url?)

skrzepto commented 7 years ago

@pypingou We can have a default message be shown for new comers

Looks like our system wasn't able to find a queue for your notifications. This can be due to the fact that you are new to our community. After you have made some contributions you should see notifications roll in.

For users who have a queue existing but no messages

Looks like there are no notifications at this moment.
jeremycline commented 7 years ago

Do we want them to come up in hubs and have nothing shown because FMN hasn't started to send them notifications? Should the hubs client try to reconnect every X seconds in that situation? (Should we then send something else than a 404 to make it easier to detect that we're in that situation instead of querying the wrong url?)

I think HTTP 404 is all we can send send without making the SSE server much smarter and querying the user/group database to see if the URL might exist in the future. If we did that, we could respond with a HTTP 204 or something, but I'm not sure it's worth the extra overhead to distinguish between "There's no content there and there probably won't ever be content there" and "There's no content there, but it's more likely that there will be content there in the future".

The downside is if (when) there's a bug in this area it won't be quite as clear. On the other hand, adding all that extra logic to the SSE server will probably introduce other bugs.