close
The Wayback Machine - https://web.archive.org/web/20220516211428/https://github.com/pi-hole/docs/pull/409
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

Explain to noobs that you need to port forward vpn, and make a disclaimer on iptables-legacy #409

Merged
merged 5 commits into from Dec 1, 2020

Conversation

MrRedness
Copy link
Contributor

@MrRedness MrRedness commented Oct 26, 2020

By submitting this pull request, I confirm the following:
please fill any appropriate checkboxes, e.g: [X]

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

Please make sure you Sign Off all commits. Pi-hole enforces the DCO.


What does this PR aim to accomplish?:
A detailed description, screenshots (if necessary), as well as links to any relevant GitHub issues
Better explain that you need to port forward your vpn. Also, another thing I didn't include in this PR but would be nice to include is a disclaimer that in newer linux dists you need to use iptables-legacy instead of iptables (iptables by itself uses nf tables spits out error messages left and right). When I was setting up openvpn it gave me an error about the openvpn-iptables.service. I finally figured out you have to edit the service file and replace iptables with iptables-legacy. Alternatively, (what I ended up doing afterwards) is just setup the firewall using the firewall guide you included, but replacing iptables with iptables-legacy (or in my case sudo iptables-legacy, as I didn't have root). It would be nice to include a disclaimer of sorts (or alternative commands using iptables-legacy), but I'll also be contacting the creator of the "road warrior" script to find a solution.

How does this PR accomplish the above?:
A detailed description (such as a changelog) and screenshots (if necessary) of the implemented fix
Added a sentence to both installation.md and firewall.md explaining that you need to portforward your openvpn port.

What documentation changes (if any) are needed to support this PR?:
A detailed list of any necessary changes


  • You must follow the template instructions. Failure to do so will result in your pull request being closed.
  • Please respect that Pi-hole is developed by volunteers, who can only reply in their spare time.

MrRedness added 2 commits Oct 26, 2020
A noob like me didn't realize you have to portforward. It would help if it was written somewhere.
@netlify
Copy link

@netlify netlify bot commented Oct 26, 2020

Deploy preview for pihole-docs ready!

Built with commit 2c26a47

https://deploy-preview-409--pihole-docs.netlify.app

@dschaper
Copy link
Member

@dschaper dschaper commented Oct 26, 2020

Finding: **/*.md !**/node_modules/**
Linting: 57 file(s)
Summary: 2 error(s)
docs/guides/vpn/installation.md:67 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"]
docs/guides/vpn/installation.md:69 MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Above] [Context: "### Install Pi-hole"]

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

Well it seems I'm also a noob at coding but I think you get the point why it would be nice to explain how to port forward and how to use iptables-legacy. Also, for the raspberry pi users this seems like a great script that would save some time, and it also supports wireguard.

@dschaper
Copy link
Member

@dschaper dschaper commented Oct 28, 2020

I don't know if pivpn has been fixed, but in the recent past it broke Pi-hole.

I don't have a pro/con for this particular Pull Request, I just linked to the reasons the required tests failed, the formatting needs to be cleaned up and then the tests will pass.

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

I'll try to fix up my formatting

@dschaper
Copy link
Member

@dschaper dschaper commented Oct 28, 2020

Great!

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

Yay I figured out how to add a line between my text and a header! 😂

@dschaper
Copy link
Member

@dschaper dschaper commented Oct 28, 2020

Don't sell yourself short, you figured out how to read the error message, understand what it meant, understand how to fix it and then actually fix it.

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

Thank you. So do you think this could be added to the docs?

@dschaper
Copy link
Member

@dschaper dschaper commented Oct 28, 2020

I don't like adding instructions to forward ports, that usually is a security problem but I don't have much input for this. @RamSet might have some input though.

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

Oh, ok. Just maybe make a disclaimer somewhere that it may be necessary since it is not mentioned anywhere and it took me about 20 minutes to figure it out (wait, it is necessary right? I assume so unless you find someway to route the vpn through another tunnel). Oh, and when I was trying to fix the error in the code it showed an X next to firewall.md so i thought that was the error and I found that the first line on that page started with ** but did not end with it, making it unbolded. At first I thought it was my issue but it is present in the official doc as well. https://docs.pi-hole.net/guides/vpn/firewall/ Not sure if that's intentional or not.

@RamSet
Copy link
Sponsor Member

@RamSet RamSet commented Oct 28, 2020

I agree with the disclaimer.

The original guide had IPTABLES instructions for the default port (or any port for that matter) but did not instruct the user that port forwarding needs to be set-up.
It is however, a necessary step for the VPN to work, obviously.

I think we all assumed it is self understood that port forwarding is needed (experience) but indeed this would puzzle someone totally new to this.

The changes above are a welcome addition to the guide.

RamSet
RamSet approved these changes Oct 28, 2020
Copy link
Member

@RamSet RamSet left a comment

Makes sense for a new user.

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

Thank you. What is your opinion on the other two changes.

  1. Either adding a disclaimer for newer dists to use iptables-legacy, or use the iptables-convert tool to convert your old rules into nftb rules that can be used on the new iptables and display them on the same page. You would ask users to do iptables -v to check their version, and if it says iptables (nf_tables) they would need to use one of the two options listed above.
    (And maybe clarify you may need to use root, but that is something really basic and probably doesn't need to be mentioned)
  2. Is the ** on the first line of firewall.md done on purpose, or were you trying to bold it?

@RamSet
Copy link
Sponsor Member

@RamSet RamSet commented Oct 28, 2020

Thank you. What is your opinion on the other two changes.

  1. Either adding a disclaimer for newer dists to use iptables-legacy, or use the iptables-convert tool to convert your old rules into nftb rules that can be used on the new iptables and display them on the same page. You would ask users to do iptables -v to check their version, and if it says iptables (nf_tables) they would need to use one of the two options listed above.

This takes it out of the realm if Pi-hole. IPTABLES, especially in the “hands” of an unexperienced user is a deeeeep rabbit hole.
A fresh system, takes default clean IPTABLES rules, no need for conversion or adaptation.

(And maybe clarify you may need to use root, but that is something really basic and probably doesn't need to be mentioned)

I suppose... it will pop an error if sudo is not used

  1. Is the ** on the first line of firewall.md done on purpose, or were you trying to bold it?

Bold 😊

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

@RamSet
Copy link
Sponsor Member

@RamSet RamSet commented Oct 28, 2020

The RoadWarrior is something we recommend using as it does everything for the user, including setting (as up to date as the install script is) IPTABLES rules.

I agree, our guide needs review as most of the instructions do not apply (since the new OpenVpn was released half a year ago -or so-). I just didn’t have the time to review the outdated information but I set up a personal note to do that ASAP.

I will review the guide and remove/edit irrelevant/outdated information.

@MrRedness
Copy link
Contributor Author

@MrRedness MrRedness commented Oct 28, 2020

DL6ER
DL6ER approved these changes Dec 1, 2020
@DL6ER
Copy link
Member

@DL6ER DL6ER commented Dec 1, 2020

I'll merge the changes in here - regardless of a possibly happening edit of the guide or not (it would be in a separate PR anyway). Note that this Pi-hole recommendation is to move away from OpenVPN towards using WireGuard (see #376) because it is a lot easier to set up and understand + gives much higher throughput and an overall better performance, especially on lower-end hardware. In addition, it also saves data volume (considering always-connected mobile devices) so it seems to be preferential in virtually all aspects.

@DL6ER DL6ER merged commit 0ba610b into pi-hole:master Dec 1, 2020
6 checks passed
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

5 participants