search

pelias / docker

Run the Pelias geocoder in docker containers, including example projects.
MIT License
315 stars 218 forks source link

Security fix #252

Closed AlexNodex closed 3 years ago

AlexNodex commented 3 years ago

By default the docker-compose.yml pierces the firewall opening 9200 to the outside world. There is no valid reason to do this and allowing it can lead to various breaches including adding and removing data which can escalate up the chain.

:wave: I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.

Here's the reason for this change :rocket:

Here's what actually got changed :clap:

Here's how others can test the changes :eyes:

orangejulius commented 3 years ago

Hi @AlexNodex,

Wow, you must have spent a bunch of time opening all these PRs. :laughing:

So, first of all, the pelias/docker repo is really meant only for development. It's not meant to have full security precautions required in production or a production-like environment. While we don't want to make things secure for no reason, the priority for this repo is ease of local development.

Second, your changes (moving the port mapping specification from 9200:9200 to 9200) will still expose Elasticsearch to the outside world on all network interfaces, it will just do so on a random high port. This is better, but not much better.

There are, in fact, many valid development workflows that are aided by connecting directly to Elasticsearch, so we probably want to maintain the 9200:9200 mapping.

What we might be able to do is expose port 9200 from the Elasticsearch container to port 9200 on the host, but only on the localhost interface, with 127.0.0.1:9200:9200. This would still be a breaking change that could surprise some people (I'm sure there are many folks expecting to be able to connect to Elasticsearch across hosts), but it would at least be a secure by default setup.

If you'd like, you can open a single pull request to make that change across all our projects, and we can work on getting that merged.

But these PRs as they are can't be merged, so I have to close this one and all the others. Thanks for starting the discussion on this topic, and hopefully we can make things better for all use-cases in a followup PR. :)