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

Basic wireguard IPv6 (NAT) support #1455

Merged
merged 4 commits into from
Mar 10, 2022

Conversation

DerDanilo
Copy link
Contributor

@DerDanilo DerDanilo commented Feb 11, 2022

Following my comment (#1394 (comment)) I tried to integrate IPv6 for pivpn.


This PR adds basic IPv6 support for Wireguard using IPv6 NAT. IPv6 NAT is more or less ugly but works fine, since requests are desired be coming from the public IPv6 of the VPN server and not the client IPv6 (in most cases).

  • Since the IPv6 interface can differ from the IPv4 interface it's configurable. Usually it's the same.
  • I am not sure what has to be done to have all tests run successfully since IPv6 has : and :: which might cause some trouble.
  • I have no experience with CLI GUI stuff, hence I hope someone can adjust the code if it's not working

WG IPv6 basically works on multiple installations I am maintaining. Now some people need to test and help finalize it to make this work with pivpn.

Edit: I fixed lint errors but don't know what is wrong with the setup tests since the setup seems to finish without errors but wireguard doesn't seem to be able to start. Can someone please check on that?

Thanks!

@Anuskuss
Copy link

Tried it out, log said:

ip6tables v1.8.2 (legacy): Empty interface is likely to be undesired
Try `ip6tables -h' or 'ip6tables --help' for more information.

I can't get it to connect. Also, shouldn't there be an IPv6 subnet in the Address field in the configs like this?

Anyway, I'm not even sure how to connect to my Pi at all. Should I use my router's public IPv6 or my Pi's? The Endpoint field will not accept an IP address without a port, so how would that even work with the Pi's IPv6 address?
I can ping my router, but the Pi will not respond. I've enabled everything, forwarded the port, and even put my Pi in the DMZ (it's called "Exposed Host" in my router), but the Pi will still not respond to a ping.
At the end of the day, I decided to just stop trying to make this work. Thank God I have Dual Stack so I don't have to make this work. Also, I don't want any help from you, I simply don't care anymore. I really just wanted to let you know about that warning I got. Wish you best of luck.

@DerDanilo
Copy link
Contributor Author

Tried it out, log said:

ip6tables v1.8.2 (legacy): Empty interface is likely to be undesired
Try `ip6tables -h' or 'ip6tables --help' for more information.

I can't get it to connect. Also, shouldn't there be an IPv6 subnet in the Address field in the configs like this?

Thanks for that info!
Reading your post-error comments was fun! IPv6 can be annoying sometimes but it's actually not much different than IPv4.

@coolapso
Copy link
Member

@DerDanilo not sure if you are local testing before hand, but just want to make sure you please do try some local testing before pushing to the MR, Travis CI has usage limits and even tho they are quite kind in raising them if needed i'd rather avoid it.

Once all tests are passing and you feel code is ready to go let me know.
I will try to find some time during the week to assist, review and test.

@DerDanilo
Copy link
Contributor Author

DerDanilo commented Feb 13, 2022

@4s3ti I just launched a clean test machine today to do more testing even though I thought I tested everything already. Found the bug with the missing IPv6dev reference, which is fixed now.
Did some more testing but didn't figure out why wireguard won't start in the travis test machines while it's starting just fine on a bullseye vm (amd64). Maybe some issue with IPv6 in the test env after all?
I do local tests (which worked fine) and then tried to fix an error that I could not figure out, hence needed to push to get the travis results. (If there is an easier way to do the travis tests locally - without having to build an entire dev env - I am open for that).

The annoying thing with local tests is that the install script will always install the master upstream repo (into pivpnFilesDir="/usr/local/src/pivpn"), which can be changed by hand of course, but the branch then is wrong, since I am not pushing against the fork master obviously. Maybe this can be improved for future testing, allowing providing such option on the CLI via options or maybe just env vars. 'custom_git_repo_url' and 'custom_git_branch' (to checkout after clone).
These options then should also be referenced in the tests to have those test properly. I don't have much experience with Travis and hence don't know how that behaves.

Looking forward to have this fixed. Any help is appreciated.


Other question: Can we add an option to disable any script interaction with the firewall at all? I understand that this should be handled by the script for folks that don't work on it otherwise. But it is a valid option to have the script not fiddle with any firewall if the user wishes so. Could be an option in the unattended file and should possible be an additional question in the whiptail dialog.

@coolapso
Copy link
Member

coolapso commented Feb 26, 2022

I am really struggling to find time, barely can find time to anything else other than keep an eye and manage the project. :(

@DerDanilo there are ways to test the pipelines locally .. but to be honest .. the easy way is really to just push and look at the build :p if credits go over .. no worries i'll just request more

What i usually do is to spin up a cloud instance and try to replicate the build environment and see what happens.

I usually use hetzner as my cloud provider for this kind of things, and i've noticed it gets quite close to the build environment, however, but i believe any clean debian system might work.

Other question: Can we add an option to disable any script interaction with the firewall at all? I understand that this should be handled by the script for folks that don't work on it otherwise. But it is a valid option to have the script not fiddle with any firewall if the user wishes so. Could be an option in the unattended file and should possible be an additional question in the whiptail dialog.

I am okay with that, considering that we also provide a warning mentioning that we won't provide any (of the few we do) support past that point @orazioedoardo any opinion about this?

@orazioedoardo
Copy link
Member

@4s3ti I'm ok too with an unattended option to not touch the firewall. @DerDanilo shouldn't you check whether the Pi's network actually supports IPv6?

Anyway, I'm not even sure how to connect to my Pi at all. Should I use my router's public IPv6 or my Pi's? The Endpoint field will not accept an IP address without a port, so how would that even work with the Pi's IPv6 address?

@Anuskuss You should put the public IPv6 address of the Pi in the Endpoint field of the .conf, and tell the router to allow port 51820 to pass through its firewall (this is not port forwarding, there is no nat involved).

@DerDanilo
Copy link
Contributor Author

DerDanilo commented Feb 26, 2022

What i usually do is to spin up a cloud instance and try to replicate the build environment and see what happens.

I actually struggle finding time to. It was a lucky evening where I pushed the IPv6 changes. Does the project contain instructions on how to launch the build env? Could you please push some that devs can use?
If I find time I can also try to launch older version which still might be used. But to be honest. I wouldn't even bother testing with anything older that 20.04 and (maybe) Buster, rather bullseye already. If people start using the project today they'll most probably be on newer systems anyways.

There is also the issue with iptables being replace by nftables. We could also add support for that. I didn't have to work with that and I think that the whole firewall options should be another PR anyways. (don't touch my FW, update FW rules when running script again and nftables support)

@DerDanilo shouldn't you check whether the Pi's network actually supports IPv6?

Why? Most people don't fiddle with IPv6 on their system, hence link local addresses are enabled by default. If people do they should know what they are doing. I think that IPv6 should be enabled by default, people refusing not using IPv6 are part of the problem why it's not as far as it could be today. There are few internet connections without IPv6 (outgoing at least), hence statistically this should work most of the time.

We could maybe put in an option that allows to disable IPv6 at all, ignoring all IPv6 configuration. But do we really want to do that? < Quote Comments from above >

@Anuskuss You should put the public IPv6 address of the Pi in the Endpoint field of the .conf, and tell the router to allow port 51820 to pass through its firewall (this is not port forwarding, there is no nat involved).

For the sake of dynamic IPv6 addresses (yes some providers even rotate IPv6 subnets) and that most pis have the 'privacy/private' IPv6 option enabled I skipped that. It is also not required if one wishes to connect to a DNS name which is the best option here. Those can contain dynamic IPs without any config change requirements. With dual stack a DNS name is anyways the best option, if one wishes to use that. So people either put their IP there or put a DNS name.

We are opening the required port with the script already. I think that it should be clear that any firewall in front of the server needs editing to make this work.

Or did I misunderstand what you are referring to?


From travis log:

Drop-In: /etc/systemd/system/[email protected]

             └─�]8;;file:https://pivpn.test/etc/systemd/system/[email protected]/override.conf�override.conf�]8;;�

This looks like there is a double reference. Could also just be a display issue.

Why don't we run tests with Debian itself? I think that he two latest Ubuntu and Debian releases should be tested.

@orazioedoardo
Copy link
Member

There is also the issue with iptables being replace by nftables. We could also add support for that.

I'm planning to do this.

Why? Most people don't fiddle with IPv6 on their system, hence link local addresses are enabled by default.

I mean actual IPv6 internet connectivity, a global unicast address and possibly a reachability test.

I think that it should be clear that any firewall in front of the server needs editing to make this work.

That comment was a suggestion for the user above, clearly you can't change the router firewall from the Pi, unless UPnP is enabled (I think automatic port forwarding was requested in the past).

It is also not required if one wishes to connect to a DNS name which is the best option here. Those can contain dynamic IPs without any config change requirements. With dual stack a DNS name is anyways the best option, if one wishes to use that. So people either put their IP there or put a DNS name.

Though currently the whiptail prompt has options for the IPv4 address or a domain name. In case IPv6 is supported, there are some options:

  1. Show both IPv4 and IPv6 address, and create two configurations if IP is selected instead of the DNS name.
  2. Only provide the option for DNS same and say why it is the best option.

For the sake of dynamic IPv6 addresses (yes some providers even rotate IPv6 subnets) and that most pis have the 'privacy/private' IPv6 option enabled I skipped that.

Ok, speaking of this, I think the script should ask to set a static IPv6 address too.

@DerDanilo
Copy link
Contributor Author

There is also the issue with iptables being replace by nftables. We could also add support for that.

I'm planning to do this.

Would you mind including an option to not touch any firewall rules at all in the same PR?
Should at least be in the unattended file but maybe also in the whiptail commands.
Something like "it looks like you're using iptables/ufw/nftables, should pivpn configure your firewall for wireguard?"

Why? Most people don't fiddle with IPv6 on their system, hence link local addresses are enabled by default.

I mean actual IPv6 internet connectivity, a global unicast address and possibly a reachability test.

Maybe we can just do a ping6 google.com or something like this. If this works out IPv6 should be fine. What do you think?
Any suggestion how to implement that?
We should maybe also provide an option that disables any IP checks in case of problems there.

I think that it should be clear that any firewall in front of the server needs editing to make this work.

That comment was a suggestion for the user above, clearly you can't change the router firewall from the Pi, unless UPnP is enabled (I think automatic port forwarding was requested in the past).

UPnP is not reliable enough I think. But yes, it could also be an option to add support for that. Please don't make this the default.

I suggest you also include this feature in your firewall PR (nftables).

It is also not required if one wishes to connect to a DNS name which is the best option here. Those can contain dynamic IPs without any config change requirements. With dual stack a DNS name is anyways the best option, if one wishes to use that. So people either put their IP there or put a DNS name.

Though currently the whiptail prompt has options for the IPv4 address or a domain name. In case IPv6 is supported, there are some options:

1. Show both IPv4 and IPv6 address, and create two configurations if IP is selected instead of the DNS name.

2. Only provide the option for DNS same and say why it is the best option.

Why put a static IP into the config? All configs have to be updated if any IP was changed.
Are you suggesting to create two different config files per user if the user chooses to use both IP stacks instead of a DNS name?
I suggest to simply say that a user may use a single IP as Endpoint or a DNS based endpoint to support dual stack.

From some reddit post:

Wireguard for Android/iOS will prefer IPv4 if the endpoint hostname you specified successfully resolves AAAA and A, to help with roaming between potentially IPv4-only networks. You can enforce IPv6 as transport by using a AAAA-only endpoint. wg-quick will prefer IPv6 if possible.

AFAIK pivpn doesn't yet support to properly update existing profiles. I am also missing an option to override the "Allowed IPs" var per user. Yes users may simply add the nets their routing through the VPN but we have some cases where changing the list on a per user basis is very useful and avoids editing the config afterwards.

For the sake of dynamic IPv6 addresses (yes some providers even rotate IPv6 subnets) and that most pis have the 'privacy/private' IPv6 option enabled I skipped that.

Ok, speaking of this, I think the script should ask to set a static IPv6 address too.

Are you referring to the part where the script will change the servers IP address? In general I think that this should be removed because the user should at least understand how to configure an IP address when going for a VPN.
As said; IPv6 on common internet providers are also dynamic for privacy reasons. Hence that should not be the default at least.

@orazioedoardo
Copy link
Member

Would you mind including an option to not touch any firewall rules at all in the same PR?

Ok.

Maybe we can just do a ping6 google.com or something like this. If this works out IPv6 should be fine. What do you think?

Should be enough according to my limited knowledge of IPv6.

We should maybe also provide an option that disables any IP checks in case of problems there.

Which problems could arise by checking for connectivity?

UPnP is not reliable enough I think. But yes, it could also be an option to add support for that. Please don't make this the default.

Definitely not default. UPnP is often seen as a security risk so I'm not sure if we want to support such feature, although it would simplify the work for the user.

Are you suggesting to create two different config files per user if the user chooses to use both IP stacks instead of a DNS name?

Yes, but see below too.

AFAIK pivpn doesn't yet support to properly update existing profiles.

Yes, this is quite implicitly requested.

I suggest to simply say that a user may use a single IP as Endpoint or a DNS based endpoint to support dual stack.

Which one? IPv4, IPv6, or ask the user?

Are you referring to the part where the script will change the servers IP address? In general I think that this should be removed because the user should at least understand how to configure an IP address when going for a VPN.

I'm referring to the part of the script that sets the static LAN IP. In the same way, I think it should set a static IPv6 but warn that some ISPs change the prefix from time to time (even if is not recommended by RIPE: https://www.ripe.net/publications/docs/ripe-690). If we are removing tasks that the script has a chance to perform, then the script itself is less useful to the user.

@DerDanilo
Copy link
Contributor Author

We should maybe also provide an option that disables any IP checks in case of problems there.

Which problems could arise by checking for connectivity?

External firewall rules, simply not required because the admin doesn't want any checks. DNS names being blocked. etc.

Are you suggesting to create two different config files per user if the user chooses to use both IP stacks instead of a DNS name?

Yes, but see below too.

AFAIK pivpn doesn't yet support to properly update existing profiles.

Yes, this is quite implicitly requested.

I suggest to simply say that a user may use a single IP as Endpoint or a DNS based endpoint to support dual stack.

Which one? IPv4, IPv6, or ask the user?

Let the user choose, so ask. Suggest IPv4 if in doubt because of the mentioned mobile restrictions. There are also many ISPs not supporting IPv6. (Mine doesn't and there is no way to get IPv6 because it's handled by the city itself. There is no other ISP with decent internet speed here, so I'm stuck and bring IPv6 via WG to my devices. - IPv6 in WG IPv4 tunnel)

Are you referring to the part where the script will change the servers IP address? In general I think that this should be removed because the user should at least understand how to configure an IP address when going for a VPN.

I'm referring to the part of the script that sets the static LAN IP. In the same way, I think it should set a static IPv6 but warn that some ISPs change the prefix from time to time (even if is not recommended by RIPE: https://www.ripe.net/publications/docs/ripe-690). If we are removing tasks that the script has a chance to perform, then the script itself is less useful to the user.

If you want to spend time for that okay fine with me. Just please don't let this PR stale forever because nobody finds time to do all of the suggested. We should sort the suggested and split parts into different PRs. This way we've IPv6 support which will be improved with other PRs.

Thanks!

@orazioedoardo
Copy link
Member

External firewall rules, simply not required because the admin doesn't want any checks. DNS names being blocked. etc.

@4s3ti can you add an IPv6 address to install.pivpn.io?

Let the user choose, so ask. Suggest IPv4 if in doubt because of the mentioned mobile restrictions.

ACK

If you want to spend time for that okay fine with me. Just please don't let this PR stale forever because nobody finds time to do all of the suggested. We should sort the suggested and split parts into different PRs.

Separate pull requests. I will test this as soon as i have time IRL, and find out why the checks fail.

@DerDanilo
Copy link
Contributor Author

@4s3ti can you add an IPv6 address to install.pivpn.io?

I would advise against using such subdomain for that. Rather create a specific domain for that purpose.
I can provide simple PHP scripts to provide the public IP (v4 or v6) of the device connecting to the subdomain.
I suggest: ipv4.pivpn.io and ipv6.pivpn.io for that.
This way we'd also have the public IP and can asure that this will work for the installation script. Doesn't really require anything. Just PHP on the server.

If you want to spend time for that okay fine with me. Just please don't let this PR stale forever because nobody finds time to do all of the suggested. We should sort the suggested and split parts into different PRs.

Separate pull requests. I will test this as soon as i have time IRL, and find out why the checks fail.

I am happy to further help improve pivpn but I rather go for smaller packages adding improvements than one or two huge PRs.

@4s3ti Can we maybe discuss out of public Github regarding the dev env and testing? Maybe a discord group or something that also allows voice. Maybe you can create one and provide the info in the repo README ("for devs only, no support!" with manual server approval from your side).

@coolapso
Copy link
Member

coolapso commented Mar 1, 2022

@4s3ti can you add an IPv6 address to install.pivpn.io?

Not sure ... need to investigate that.

@4s3ti Can we maybe discuss out of public Github regarding the dev env and testing? Maybe a discord group or something that also allows voice. Maybe you can create one and provide the info in the repo README ("for devs only, no support!" with manual server approval from your side).

We have matrix where we can chat .. also can make a dev only room on matrix .. would that suffice?

@DerDanilo
Copy link
Contributor Author

We have matrix where we can chat .. also can make a dev only room on matrix .. would that suffice?

Where can I sign up?

@4s3ti can you add an IPv6 address to install.pivpn.io?

Not sure ... need to investigate that.

Hetzner cloud for a few bugs is available with both.

@coolapso
Copy link
Member

coolapso commented Mar 1, 2022

Hetzner cloud for a few bugs is available with both.

I know .. but I have everything running on Google Kubernetes service, so configuring IPv6 might require some tricks .. need to investigate that

As for matrix .. the easiest place is here: https://element.io/ the only reason why i am not opening up a discord is because it becomes way to much stuff to manage. the room is on the README ... and during the weekend can spin up a devs room or something. will think the best approach, in the meantime you can always PM me there

@orazioedoardo
Copy link
Member

orazioedoardo commented Mar 1, 2022

I would advise against using such subdomain for that. Rather create a specific domain for that purpose.

The rationale behind install.pivpn.io is that the user has likely visited the website so it is implicitly trusted and available when the script is being run.

@DerDanilo
Copy link
Contributor Author

lets discuss further in matrix. To much discussion here.

@DerDanilo
Copy link
Contributor Author

I would advise against using such subdomain for that. Rather create a specific domain for that purpose.

The rationale behind install.pivpn.io is that the user has likely visited the website so it is implicitly trusted and available when the script is being run.

We might switch to that URL in the future. For the time being I implemented just testing at google.com as this is most probably also allowed and google.com supports IPv6 of course.

@DerDanilo
Copy link
Contributor Author

DerDanilo commented Mar 8, 2022

After discussing with @4s3ti in matrix we decided to disable IPv6 tests in travis since travis doesn't really support IPv6 properly. It's only available in the LXC container which are only running on ARM which brings other problems to the table.

I implemented the following:

  • A switch to disable IPv6 config
  • IPv6 is enabled by default IF
    --> IPv6 simple ping test, that will automatically disable ipv6 if not working (using google.com for now, may be switched to install.pivpn.io in future, no IPv6 address at the moment)
  • allow disabling IPv6 via unattended file switch (mainly for testing exclusion, since ipv6 should be used if available)

We need someone to test this now without and with unattended file. Disabling IPv6 via unattended file works already otherwise travis tests would fail.

Update:
Did local tests, all working well after fixing the IPv6 ping test syntax. Even a reconfigure seems to work just fine.
If someone wants to confirm we can merge this into testing.

I sqashed most commits into a few since it was kinda messy.

@DerDanilo DerDanilo mentioned this pull request Mar 9, 2022
@DerDanilo DerDanilo changed the base branch from master to test March 10, 2022 09:54
@coolapso
Copy link
Member

LGTM.

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

4 participants