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

lighttpd: do not overwrite /etc/lighttpd/lighttpd.conf (On new installs) #5075

Merged
merged 4 commits into from
Dec 19, 2022

Conversation

gstrauss
Copy link
Contributor

@gstrauss gstrauss commented Dec 19, 2022

What does this PR aim to accomplish?:

lighttpd: do not overwrite /etc/lighttpd/lighttpd.conf (on new installations)

How does this PR accomplish the above?:

This PR requires #5065 and subsumes #5066.

This PR shows how pi-hole can avoid overwriting /etc/lighttpd/lighttpd.conf for new installations, while still maintaining lighttpd.conf for existing pi-hole installations.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

@gstrauss gstrauss force-pushed the lighttpd-simplify-2 branch 2 times, most recently from 39ba72c to 043ad56 Compare December 19, 2022 08:47
@gstrauss
Copy link
Contributor Author

On Fedora-based systems, not sure why sed rename is failing in the tests. I'll continue debugging later.

DEBUG    testinfra:base.py:288 RUN CommandResult(command=b'docker exec f1349c07597be5edc1e424f453b46258d32d8af6473eb5f9c2abbe0b5d5ba775 /bin/bash -c \'sed -i \'"\'"\'s/nameserver.*/nameserver 127.0.0.1/\'"\'"\' /etc/resolv.conf\'', exit_status=4, stdout=None, stderr=b'sed: cannot rename /etc/sedtvsuT3: Device or resource busy\n')
DEBUG    testinfra:base.py:288 RUN CommandResult(command=b'docker exec f1349c07597be5edc1e424f453b46258d32d8af6473eb5f9c2abbe0b5d5ba775 /bin/bash -c \'curl -s --head "http://127.0.0.1/admin/" | head -n 1 | grep "HTTP/1.[01] [23].." > /dev/null\'', exit_status=1, stdout=None, stderr=None)

@gstrauss gstrauss force-pushed the lighttpd-simplify-2 branch 3 times, most recently from aaec711 to 8de590f Compare December 19, 2022 09:31
@gstrauss
Copy link
Contributor Author

The sed error is fixed with some docker-foo, but there remains a test error retrieving the /admin/ page for the Fedora-based builds. Will troubleshoot more tomorrow.

@gstrauss gstrauss force-pushed the lighttpd-simplify-2 branch 11 times, most recently from 02bc95b to 5b6d6c1 Compare December 19, 2022 19:55
Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
@gstrauss
Copy link
Contributor Author

Tests were failing on Fedora due to the system lighttpd.conf issuing some warnings and the tests treating warnings trace as an error. I'll work out some adjustments...

@gstrauss gstrauss marked this pull request as draft December 19, 2022 20:34
Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
@gstrauss gstrauss marked this pull request as ready for review December 19, 2022 21:24
@gstrauss
Copy link
Contributor Author

Updated description now that #5065 has been merged. This PR subsumes #5066.

This PR shows how pi-hole can avoid overwriting /etc/lighttpd/lighttpd.conf for new installations, while still maintaining lighttpd.conf for existing pi-hole installations. As discussed in #5066, this PR also removes obsolete pihole block page error handler.

@PromoFaux: tests all pass, including on Fedora 37, newly added to the pi-hole CI pipeline.

@gstrauss
Copy link
Contributor Author

For compatibility with existing installations, the only potential breaking change is removal of the obsolete pihole block page error handler. Most users are not using the obsolete pihole block page error handler; BLOCK_IPV4 and BLOCK_IPV6 are not used in the default pi-hole DNS resolver config.

Existing users can opt-in to convert to the new, less-invasive configuration -- which does not overwrite the system lighttpd.conf -- by uninstalling pi-hole and answering No to question ${QST} Do you wish to go through each dependency for removal? (Choosing No will leave all dependencies installed) [Y/n], and then reinstalling pi-hole.

test/test_any_automated_install.py Outdated Show resolved Hide resolved
remove obsolete pihole block page error handler

x-ref:
  Remove the advanced functionality of the 404 page (Blockpage)
  pi-hole#3910
Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
(/var/www/html/pihole/ dir and contents are still removed in uninstall)
(/var/www/html/index.lighttpd.orig is still removed in uninstall)

Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
@PromoFaux PromoFaux changed the title lighttpd: do not overwrite /etc/lighttpd/lighttpd.conf lighttpd: do not overwrite /etc/lighttpd/lighttpd.conf (On new installs) Dec 19, 2022
@PromoFaux
Copy link
Member

by uninstalling pi-hole

This is probably like using a sledgehammer to crack a nut. Simpler:

@PromoFaux PromoFaux merged commit ec1d4c5 into pi-hole:development Dec 19, 2022
@gstrauss
Copy link
Contributor Author

by uninstalling pi-hole

This is probably like using a sledgehammer to crack a nut. Simpler:

* delete the pi-hole flavoured `/etc/lighttpd/lighttpd.conf`

* One of (untested!):
  
  * uninstall and reinstall `lighttpd` (will this install a default config?)
  * replace `/etc/lighttpd/lighttpd.conf` with a default config manually

Haha. Yes.
pi-hole install backs up /etc/lighttpd/lighttpd.conf to /etc/lighttpd/lighttpd.conf.orig, and the uninstall restores that.

        if [[ -f /etc/lighttpd/lighttpd.conf.orig ]]; then
            ${SUDO} mv /etc/lighttpd/lighttpd.conf.orig /etc/lighttpd/lighttpd.conf 
        fi

The uninstall also cleans up and removes /var/www/html/pihole (if empty), which includes removing index.php, so the uninstall would do a bit of cleanup in addition to restoring the original lighttpd.conf.

In any case, there are a small number of steps that can be performed manually for existing installations that wish to migrate.

However, note that the (now-removed) code in basic-install.sh which backed up lighttpd.conf would overwrite an existing backup, so running the pi-hole installer without running the uninstaller could theoretically have overwritten the original backup. In that case, reinstalling the system lighttpd package and then merging the lighttpd.conf.rpmnew or lighttpd.conf-opkg, or ... could restore the system-provided lighttpd.conf.

-        elif [[ -f "${lighttpdConfig}" ]]; then
-            # back up the original
-            mv "${lighttpdConfig}"{,.orig}
-        fi

@gstrauss
Copy link
Contributor Author

Thank you very much for your assistance and guidance here, @PromoFaux

If lighttpd-related questions come up in the future asking how to do something using lighttpd, please tag me.


// Determine block page type
if ($serverName === "pi.hole"
|| (!empty($_SERVER["VIRTUAL_HOST"]) && $serverName === $_SERVER["VIRTUAL_HOST"])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gstrauss I totally forgot to mention/ask - are we able to reproduce this in pihole-admin.conf where the following lines are?

$HTTP["host"] == "pi.hole" {
$HTTP["url"] == "/" {
url.redirect = ("" => "/admin/")
}
}

If my docs reading is correct, is it something like the following?

 $HTTP["host"] == "pi.hole"  ||  $HTTP["host"] == env.VIRTUAL_HOST { 
     $HTTP["url"] == "/" { 
         url.redirect = ("" => "/admin/") 
     } 
 } 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lighttpd.conf syntax is historically its own custom syntax (for better and worse), and is not shell syntax. If VIRTUAL_HOST is set in the shell environment before starting lighttpd, then this syntax should work:

$HTTP["url"] == "/" {
    $HTTP["host"] == "pi.hole" {
         url.redirect = ("" => "/admin/")
    }
    $HTTP["host"] == env.VIRTUAL_HOST {
         url.redirect = ("" => "/admin/")
    }
} 

but the above will fail if VIRTUAL_HOST is not set. It should at least be set to VIRTUAL_HOST="" to work
(Constructing a regex using lighttpd.conf syntax is possible but ugly and verbose
e.g. ^(" + env.VIRTUAL_HOST + "|pi.hole)$)
That also requires VIRTUAL_HOST be set in the shell environment, and not missing from the shell env.

Another option, which I know is in wide use, is to use lighttpd.conf include_shell to run a short shell script which generates the simpler lighttpd.conf syntax to do what is desired, which could generate the syntax I posted above for a list of pi-hole admin hosts ("127.0.0.1", "::1", "localhost", "pi.hole", etc)

Lastly, a few lines of lua can be written and used with lighttpd mod_magnet. Lua is a programming language and is much more expressive than lighttpd.conf syntax. Whenever someone asks "can I do this arbitrarily complex thing in lighttpd.conf?", the answer frequently includes: one option is to use lighttpd mod_magnet and a lua script.

Maybe not as simple as you had hoped, but there are solutions available. If you want to extend what is currently checked in, I would suggest include_shell and a shell script to generate the simpler lighttpd.conf directives to do exactly what you want. I think requiring VIRTUAL_HOST be set in the environment will complicate things for various existing scripts in the OS which check lighttpd -f /etc/lighttpd.conf -tt and expect it to work (without knowing that VIRTUAL_HOST needs to be set in the environment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, the "better" or advantages of lighttpd.conf syntax is realized in how lighttpd uses lighttpd.conf: lighttpd parses and generates a static configuration at startup, with limited conditionals allowed by the syntax. Processing the lighttpd configuration for each and every request at runtime is much faster than runtime config parsing done by many other web servers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intercepting and redirecting "/" might be desirable for new installations, but not for existing lighttpd configs which are used for other purposes. If you do use include_shell and create a shell script for this configuration, instead of adding it to pihole-admin.conf, you might consider a new file pihole-admin-redir.conf so that some people can more easily disable just that part.

Alternatively -- and I think the best solution for this very-specific pi-hole use -- you can construct a simple index.html that you drop into the document root if index.* is not already present, and that index.html can be a splash page and/or can do the redirection in the HTML <meta http-equiv="Refresh" content="0; url='/admin/'" />

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the information.

On reflection, I have decided that we do not need to include the functionality - But I spotted it and realised that I had overlooked it before, so wanted to find a way back in case we needed it.

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-20-and-web-v5-18-released/59959/1

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-20-1-web-v5-18-1-and-core-v5-15-released/60495/1

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/yesterdays-update-removed-the-index-html/60531/2

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/changing-default-listening-port-of-lighttpd-in-raspberry-pi-os-no-longer-honors-external-conf/62235/9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants