Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a whitelist of IP addresses that can use the default 'admin' password to login into sites. #1227

Merged
merged 3 commits into from Apr 9, 2016

Conversation

@mworrell
Copy link
Member

mworrell commented Mar 30, 2016

Often the admin password for development sites is "admin".
It happens quite often that these sites are exposed to the Internet, which risks total access to the server.

This merge request adds a IP address whitelist for IP addresses that are allowed to use the password "admin" for logging into a site using the "admin" account.

There is a new Zotonic configuration in the zotonic.config:

%%% IP whitelist, used for accessing sites with a default "admin" password
   %% {ip_whitelist, "127.0.0.0/8,10.0.0.0/8,192.168.0.0/16,172.16.0.0/12,::1,fd00::/8"},

Per default the local loopback interface and local network addresses are whitelisted.

…word to login into sites.
@ddeboer ddeboer added this to the 0.16 milestone Mar 30, 2016
@ddeboer

This comment has been minimized.

Copy link
Member

ddeboer commented Mar 30, 2016

I’m in favour of this PR. It is important, however, that we document it well. Can you push some documentation in this PR?

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Mar 30, 2016

I was looking where to document it and am a bit uncertain about the best chapter/section.

@mworrell mworrell self-assigned this Mar 30, 2016
@arjan

This comment has been minimized.

Copy link
Member

arjan commented Mar 30, 2016

Documentation indeed, plus a better name for the config option; ip_whitelist is too generic, since this is only a protection for the admin

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Mar 30, 2016

@arjan Later I would like to re-use the same whitelist for other functionalities. These are basically trusted IP addresses.

@ddeboer

This comment has been minimized.

Copy link
Member

ddeboer commented Mar 30, 2016

I was looking where to document it and am a bit uncertain about the best chapter/section.

In the reference, we have http://zotonic.com/docs/0.13/ref/configuration/site-configuration.html and http://zotonic.com/docs/0.13/ref/configuration/zotonic-configuration.html.

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Mar 30, 2016

@ddeboer yes, needs documentation in both, as this can be overruled per site.

@arjan

This comment has been minimized.

Copy link
Member

arjan commented Mar 30, 2016

I would not use one whitelist for multiple functionalities; there might be different uses for each whitelist.
(e.g. API access, admin access, et cetera);
The whitelist-checking code could be generic of course, but the actual config value should rather not.

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Mar 30, 2016

I am not sure about the whitelists, it is good to have the ability to differentiate between whitelists, but not good to be forced to maintain multiple lists for simple use cases. Maybe we can differentiate later, if we need to add a whitelist and use the ip_whitelist as the default fallback?

@ddeboer

This comment has been minimized.

Copy link
Member

ddeboer commented Apr 5, 2016

@arjan What about adding this as is for now? We can always introduce different whitelist later.

@CyBeRoni

This comment has been minimized.

Copy link
Contributor

CyBeRoni commented Apr 6, 2016

Can this be extended to the zotonic status page? (Remove ability to log in entirely in that case.)

@CyBeRoni

This comment has been minimized.

Copy link
Contributor

CyBeRoni commented Apr 6, 2016

By the way I think you meant fe80::/10 instead of fd00::/8. The former is link-local addressing, the latter is for ULAs.

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Apr 7, 2016

The fe80::/10 should indeed be added, if we also add 169.254.0.0/16. Which makes sense in ad-hoc networks, which are local by default.

Regarding the ULA, it is basically the same as the ipv4 private addresses, isn't it?
https://en.wikipedia.org/wiki/IPv6_address#Unique_local_addresses

fc00::/7 — Unique local addresses (ULAs) are intended for local communication. They are routable only within a set of cooperating sites.[21] The block is split into two halves, the upper half (fd00::/8) is used for "probabilistically unique" addresses in which a 40-bit pseudorandom number is used to obtain a /48 allocation. This means that there is only a small chance that two sites that wish to merge or communicate with each other will have conflicting addresses. No allocation method for the lower half of the block (fc00::/8) is currently defined. These addresses are comparable to IPv4 private addresses (10.0.0.0/8, 172.16.0.0/12 and 192.168.0.0/16)

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Apr 7, 2016

For the zotonic_status page, here we are indeed coming to the point @arjan made to have multiple whitelists...

I am thinking what the best solution is:

  1. Multiple independent whitelists
  2. Different whitelists, with the ip_whitelist as an ultimate fallback
  3. One list, with tagging per block what is allowed/disallowed

I am for option 2.

As we are now only using this during log on we don't need to be efficient with matching ip addresses. Only if we are going to whitelist IP addresses for requests we need efficient matching (for which there are some solutions).

@CyBeRoni

This comment has been minimized.

Copy link
Contributor

CyBeRoni commented Apr 7, 2016

Regarding the ULA, it is basically the same as the ipv4 private addresses, isn't it?

There's not really such a thing in IPv6. They're non-routable so I suppose it's mostly safe to include them.

ULAs are meant for non-internet-connected networks that also will never be connected to the internet.

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Apr 8, 2016

I propose to merge this and add a separate issue to add an ip-whitelist for the zotonic_status site.

All ok?

@arjan

This comment has been minimized.

Copy link
Member

arjan commented Apr 9, 2016

👍
Separate whitelists with a single fallback whitelist is a good solution.

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Apr 9, 2016

Good, then we merge this and refactor into a more generic solution.

Sent from my iPhone

On 9 apr. 2016, at 13:31, Arjan Scherpenisse notifications@github.com wrote:

Separate whitelists with a single fallback whitelist is a good solution.


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub

@ddeboer ddeboer merged commit 3784b1e into 0.x Apr 9, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ddeboer ddeboer deleted the whitelist-default-admin-password branch Apr 9, 2016
@ddeboer

This comment has been minimized.

Copy link
Member

ddeboer commented Apr 9, 2016

Thanks, @mworrell!

@mworrell

This comment has been minimized.

Copy link
Member Author

mworrell commented Apr 10, 2016

Also cherry-picked into the master.

Sent from my iPhone

On 9 apr. 2016, at 15:20, David de Boer notifications@github.com wrote:

Merged #1227.


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.