Add server-side password protection for the web interface #197

DL6ER · 2016-11-16T10:57:04Z

Fixes #128.

Changes proposed in this pull request:

Add an effective password protection to the web interface as I always felt that the current way of doing it is to inconvenient for most of the users.

Behavior:

No password is set: No surprises, everything is always shown to everyone.
The user sets a password in setupVars.conf: Now, the user will be shown a "login" page:

Also, clicking on the links "Enable/Disable" doesn't do anything completely locking out any non-authorized user.
Once logged in, the hashed password is always carried around in all links to be sure that we only have to log in once.

The hashing of the password guarantees that the password is never visible in plain-text to someone who might look over your shoulder.

Once we agree on how we like to have it, we should definitely add an (clearly documented) option for the pihole command to make it easy to set the password. We might even ask for it during the installation.

Syntax for setupVars.conf: webpassword="ABC"

@pi-hole/dashboard

…the password hash

DL6ER · 2016-11-16T11:28:02Z

Added Logout button

…ly logged in.

PromoFaux · 2016-11-16T19:07:18Z

Thanks for this @DL6ER , I think before we accept, we're going to need to make some changes to how setupVars.conf is handled in the base pi-hole repo.

At the moment, if a user was to choose to re-install (i.e pihole -r followed by reconfigure) then any values in setupVars.conf are wiped out in order to start fresh.

Obviously, there shouldn't really be any reason for the user to have to run that, but it's a possibility, and would end with them having to re-add the password field, (and temperature flag).

Question more related to this PR (I haven't fully gone through the code yet, or tested it...) will the password stored in setupVars.conf always be in plain text, or is it hashed after the first use? Obviously if it's in plain-text then we have a security risk...

PromoFaux · 2016-11-16T19:14:28Z

Just tried it out, and in answer to my question, yes.. the password is stored in plain text... not really too happy with that!

Could you also hide these buttons when the user is not logged in?

DL6ER · 2016-11-16T19:37:05Z

I think it makes sense to hide the whole navigation bar if the user is not logged in.

Concerning password and hashing: I think it is not wise to save a hashed password using PHP. Instead, the password should be stored as a hash in the setupVars.conf file from the beginning, i.e. the pihole command should do this.

I didn't do this here intentionally, so testing would be easier. Should I open a PR to integrate the password setting feature into the pihole command? I'd obviously save only the hash there.

Concerning the possible wipe out of the password and temperature flag: Well, both of these settings are not critical by any mean, so if they get wiped out, then we'll just fall back to the defaults (no password + Celcius).

DL6ER · 2016-11-16T19:45:27Z

This is how it looks on mobile devices, not unpleasant surprises here.

PromoFaux · 2016-11-16T19:46:14Z

Should I open a PR to integrate the password setting feature into the pihole command? I'd obviously save only the hash there.

Yes please! If you can, try and make it so that it's a seperate script that is called by pihole, as with list.sh, update.sh etc.

Concerning the possible wipe out of the password and temperature flag: Well, both of these settings are not critical by any mean, so if they get wiped out, then we'll just fall back to the defaults (no password + Celcius).

Agreed.

DL6ER · 2016-11-16T20:53:58Z

I opened the PR there. Also, I changed the behavior of the script such that the saved password has to be the corresponding hash.

PromoFaux · 2016-11-16T21:12:23Z

Oh, minor niggle, can you CAPSLOCK the variable names?

DL6ER · 2016-11-16T21:20:22Z

@PromoFaux Not sure what you mean. You mean like $pwstring -> $PWSTRING?

PromoFaux · 2016-11-16T21:21:34Z

in setupVars, and then everywhere they are referenced. See the top 6 as an example :)

pi@raspberrypi:/etc/.pihole $ cat /etc/pihole/setupVars.conf
PIHOLE_INTERFACE=eth0
IPV4_ADDRESS=192.168.1.4/24
IPV6_ADDRESS=fe80::49d7:ea2e:28fd:c0b9
PIHOLE_DNS_1=8.8.8.8
PIHOLE_DNS_2=8.8.4.4
QUERY_LOGGING=true
temperatureunit=C
webpassword=9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08

i.e do TEMPERATURE_UNIT and WEB_PASSWORD

DL6ER · 2016-11-16T21:23:20Z

Okay, I got confused, since this is the web interface pull request. Also, in my setupVars.conf everything is in lower case, but might be that this has changed recently:

pi@raspberrypi:~ $ cat /etc/pihole/setupVars.conf
piholeInterface=eth0
IPv4_address=192.168.2.10/24
IPv6_address=
piholeDNS1=8.8.8.8
piholeDNS2=8.8.4.4
temperatureunit=C
webpassword=183c1b634da0078fcf5b0af84bdcbb3e817708c3f22b329be84165f4bad1ae48

PromoFaux · 2016-11-16T21:24:32Z

AH, yes sorry... they're capped in the development branch ready for next release.

Changed them as per google style guide

DL6ER · 2016-11-16T21:28:58Z

Well, this is a problem, I think. The function used in the web interface to read from the setupVars.conf file is case sensitive!
Currently, there is e.g. in data.php
$divide = parse_ini_file("/etc/pihole/setupVars.conf")['IPv6_address'] != "" && parse_ini_file("/etc/pihole/setupVars.conf")['IPv4_address'] != "";

This will fail if the capitalization of the variable names changes.

PromoFaux · 2016-11-16T21:29:47Z

Have you based off of devel on the web side of things? I thought I'd updated that...

DL6ER · 2016-11-16T21:31:09Z

Sure. It is now updated, see e.g. here: https://github.com/pi-hole/AdminLTE/blob/devel/data.php#L3

PromoFaux · 2016-11-16T21:31:31Z

Yeah, turns out I completely imagined that I'd updated that... Good catch!

AzureMarker

Can we possibly show the user on the main screen when logged in that their session won't expire on that screen? Or would that be unnecessary?

AzureMarker · 2016-11-20T21:50:26Z

js/pihole/index.js

    updateTopClientsChart();

-    updateForwardDestinations();
-
    updateTopLists();


updateTopClientsChart() and updateTopLists() are being called even if not authorized, which will cause more strain on the Pi-hole for no gain since those charts are not available when not logged in.

The API will reject the queries anyway, so there isn't much work to be done here. Anyhow, I'll check if the items exist at all before trying to get the data.

AzureMarker · 2016-11-20T21:51:59Z

header.php

@@ -255,6 +263,27 @@
                        <i class="fa fa-paypal"></i> <span>Donate</span>


We can still show the donate button when not logged in.

AzureMarker · 2016-11-20T22:02:05Z

js/pihole/footer.js

+        else
+            document.getElementById("sessiontimercounter").textContent = "-- : --";
+
+        $('.timer').text(currentTimeString);


currentTimeString is not defined

Okay, that shouldn't be there, actually...

PromoFaux · 2016-11-20T22:20:47Z

Edit: Fixed by clearing browser cache as per @Mcat12's suggestion

AzureMarker · 2016-11-20T22:53:33Z

I think that is from your browser not updating cached files.

DL6ER · 2016-11-21T09:38:21Z

@Mcat12

Can we possibly show the user on the main screen when logged in that their session won't expire on that screen? Or would that be unnecessary?

I think that is not necessary. Most people won't even know that there is some countdown in the background that might log them out at some time. If you like, we could put the timer display at some other (less hidden) place. Then everything should be more or less self-explanatory (they see that the timer is continuously reset on the main page).

I see that codacy comolained about valid code since it wants to enforce a certain code style. I'm fine with that and will change the corresponding parts.

dschaper · 2016-11-21T09:40:57Z

So the session is based on time, not on the session? If a user leaves they can still come back within the hour and the session will still be active?

DL6ER · 2016-11-21T09:43:55Z

@dschaper The session functionality of PHP has an internal timeout feature, i.e. it will invalidate a valid session on its own after a certain amount of time (defaulting to 24 minutes).

Say you visit a site and close your browser completely. If you reopen it within the said 24 minutes, then your session will be refreshed and you have another 24 minutes. If, however, you open up your browser more than 24 minutes later, then you have to login again.

If different users log in, each of them will have his own (independent) timeout.

dschaper · 2016-11-21T09:47:06Z

Okay, and then the logout button will immediately invalidate the session...

dschaper · 2016-11-21T09:48:54Z

As for Codacy, we have control of the applied rulesets, so if there are hits being registered that are no applicable, we can just disable those particular checks for these files.

Conflicts: api.php js/pihole/index.js

…orized

DL6ER · 2016-11-21T14:03:10Z

Currently, the session countdown timer is slightly hidden because I don't know if people would want to see it. Should I put it to some more prominent place clearly visible to the logged in user?

AzureMarker · 2016-11-21T15:52:16Z

It's fine where it is.

AzureMarker · 2016-11-23T03:08:10Z

js/pihole/index.js

+       && !!document.getElementById("ad-frequency"))
+    {
+        updateTopClientsChart();
+    }

    updateTopLists();


This one should only be called if authorized.

Actually, this should be moved to and replace line 155, because that one is called twice when authorized and never when not authorized.

AzureMarker · 2016-11-23T16:54:43Z

Approved, pending merge resolution

Conflicts: header.php

DL6ER · 2016-11-23T17:07:14Z

@Mcat12 Conflict resolved.

AzureMarker · 2016-11-23T17:38:15Z

Approved

DL6ER added 5 commits November 16, 2016 11:47

Add server-side password protection for the web interface

3bc6474

Use hashed once logged in to hide the plain-text password

a754c3c

Fixed a typo

c3a6b0e

Change button text to 'Login'

72e5fc5

Added "Logout" button which redirects to index.php without providing …

70f9076

…the password hash

DL6ER added 3 commits November 16, 2016 12:53

Show "Logout" only if a password is defined

7cbd9bd

Show "Logout" only if (a) password is set and (b) user has successful…

cb874e7

…ly logged in.

Undid change to footer.php

b6cf46a

Hide main navigation if user is not logged in

4dfbcff

DL6ER mentioned this pull request Nov 16, 2016

Add webpage.sh pi-hole/pi-hole#924

Merged

15 tasks

Always use only hashes for the password

06df8f0

Compute double hashes to avoid rainbow table vulnerability

6d5c901

Capitalized variable name

41fe76c

PromoFaux mentioned this pull request Nov 16, 2016

Update data.php #198

Merged

AzureMarker reviewed Nov 20, 2016

View reviewed changes

AzureMarker mentioned this pull request Nov 21, 2016

Secure pause resume plus general security fixes #188

Merged

DL6ER added 4 commits November 21, 2016 10:51

Addressed codacy issues

9e3a092

Moved Donate item outside of $auth scope on the main menu

3dea0cf

Merge branch 'devel' into auth

f0adb16

Conflicts: api.php js/pihole/index.js

Load "Top Domains", "Top Advertisers", and "Top Clients" only if auth…

4675b5a

…orized

DL6ER mentioned this pull request Nov 21, 2016

Feature request: Authentication for (parts of) web-interface #128

Closed

DL6ER mentioned this pull request Nov 22, 2016

Adding a Help Center to the Web UI #208

Merged

DL6ER added the Enhancement label Nov 22, 2016

AzureMarker reviewed Nov 23, 2016

View reviewed changes

Fixed copy-and-paste error

07376ff

DL6ER added 2 commits November 23, 2016 18:02

Merge branch 'devel' into auth

d9adccc

Conflicts: header.php

Extended password protection to php/queryads.php

21649a2

Show 'Help' only if authorized

f3fa7ff

AzureMarker merged commit 4e9f6b6 into pi-hole:devel Nov 23, 2016

DL6ER deleted the auth branch November 23, 2016 17:42

wtreur mentioned this pull request Jan 12, 2017

V2.11.1 update [WIP] pi-hole/docker-pi-hole#84

Merged

		@@ -255,6 +263,27 @@
		<i class="fa fa-paypal"></i> <span>Donate</span>

Add server-side password protection for the web interface #197

Add server-side password protection for the web interface #197

Conversation

DL6ER commented Nov 16, 2016 • edited Loading

DL6ER commented Nov 16, 2016

PromoFaux commented Nov 16, 2016

PromoFaux commented Nov 16, 2016 • edited Loading

DL6ER commented Nov 16, 2016 • edited Loading

DL6ER commented Nov 16, 2016

PromoFaux commented Nov 16, 2016

DL6ER commented Nov 16, 2016

PromoFaux commented Nov 16, 2016

DL6ER commented Nov 16, 2016 • edited Loading

PromoFaux commented Nov 16, 2016 • edited Loading

DL6ER commented Nov 16, 2016

PromoFaux commented Nov 16, 2016

DL6ER commented Nov 16, 2016

PromoFaux commented Nov 16, 2016

DL6ER commented Nov 16, 2016

PromoFaux commented Nov 16, 2016

AzureMarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PromoFaux commented Nov 20, 2016 • edited Loading

AzureMarker commented Nov 20, 2016

DL6ER commented Nov 21, 2016

dschaper commented Nov 21, 2016

DL6ER commented Nov 21, 2016 • edited Loading

dschaper commented Nov 21, 2016

dschaper commented Nov 21, 2016

DL6ER commented Nov 21, 2016

AzureMarker commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AzureMarker commented Nov 23, 2016 • edited by dschaper Loading

DL6ER commented Nov 23, 2016

AzureMarker commented Nov 23, 2016 • edited by dschaper Loading

DL6ER commented Nov 16, 2016 •

edited

Loading

PromoFaux commented Nov 16, 2016 •

edited

Loading

DL6ER commented Nov 16, 2016 •

edited

Loading

DL6ER commented Nov 16, 2016 •

edited

Loading

PromoFaux commented Nov 16, 2016 •

edited

Loading

PromoFaux commented Nov 20, 2016 •

edited

Loading

DL6ER commented Nov 21, 2016 •

edited

Loading

AzureMarker commented Nov 23, 2016 •

edited by dschaper

Loading

AzureMarker commented Nov 23, 2016 •

edited by dschaper

Loading