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 port forwarding with PIA using --port-forwarding #245

Merged
merged 11 commits into from
Jan 20, 2024

Conversation

BenLand100
Copy link
Contributor

Disclaimer: I don't know rust, at all! [Seriously, check this code before doing anything with it!]

This PR removes --protonvpn-port-forwarding and makes a more generic framework triggered by --port-forwarding for API driven port forwarding that also works for PIA. In PIA this is done with an API instead of natpmpc, but I implemented the functionality in basically the same way as natpmpc.

This PR also has several limitations noted by FIXMEs relating to two points:

  • Launching curl commands within the network namespace required the PIA certificate. I didn't know how to package this with the rust build framework, so hardcoded a local path on my machine for testing. Presumably this is a quick fix for someone knows rust.
  • I didn't see an easy way to know the CN of the VPN vopono connected to. In openvpn this could be parsed from the config files (somehow), but wireguard was a bit more opaque. This method does work for both protocols. I've hardcoded the CN for the PIA Netherlands servers for testing.
  • Ideally I'd like to specify a callback script to be invoked periodically, allowing the end user to do whatever needs to be done to spin up services on that port. If you're following so far, you won't be surprised that I hardcoded a path to a script here.

Hopefully someone can finish this off, or perhaps one day I may get more motivated to learn rust...

Removes --protonvpn-port-forwarding and makes a more generic framework
for API driven port forwarding that also works for PIA.

This commit has several limitations noted by FIXMEs
Copy link
Owner

@jamesmcm jamesmcm left a comment

Choose a reason for hiding this comment

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

Thanks, overall it looks really good! Nice use of the trait especially.

I think the main thing is to add parsing of https://serverlist.piaservers.net/vpninfo/servers/v4 for the CN addresses, and to add the writing out of the cert file to the provider config directory (e.g. ~/.config/vopono/pia during the PIA sync process (if we don't download them already) so then we can use those relative paths. Note it's already embedded here https://github.com/jamesmcm/vopono/blob/master/vopono_core/src/config/providers/pia/wireguard.rs#L94

vopono_core/src/network/piapf.rs Outdated Show resolved Hide resolved
vopono_core/src/network/piapf.rs Outdated Show resolved Hide resolved
vopono_core/src/network/piapf.rs Outdated Show resolved Hide resolved

let vpn_hostname = match protocol {
Protocol::OpenVpn => "nl-amsterdam.privacy.network".to_string(), // FIXME: Parse this from the OpenVPN conf?
Protocol::Wireguard => "nl-amsterdam.privacy.network".to_string(), // FIXME: [Insert clever idea to get wireguard endpoint hostname here]
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For wireguard it was already parsing a v6 list which had the necessary info. Added it to the Config struct there.

Openvpn seems to come from PIA as a zip, and the files are named differently than the IDs in the v4/v6 lists, so created a similar map for these filenames by parsing out the endpoint. This will only work when the config files actually contain hostnames... the IP-only variants for now just won't support port forwarding. Perhaps one could parse the IPs then look them up in v4/v6...

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, I think it's okay for now!

}

// Spawn thread to repeat above every 15 minutes
fn thread_loop(params: ThreadParams, recv: Receiver<bool>) {
Copy link
Owner

Choose a reason for hiding this comment

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

We might be able to put this and the channel logic in a trait to share it a bit better with natpmpc but this isn't critical (and could be tricky with the parts shared across threads).

vopono_core/src/network/piapf.rs Outdated Show resolved Hide resolved
@BenLand100
Copy link
Contributor Author

This has been a fun exercise playing with rust :) I'll add in a conf option for a callback script this afternoon sometime. Let me know if there are any other pre-merge changes to see to.

This accepts an argument which is a program to execute in the network
namespace which will receive the port being forwarded each time that
port is refreshed. Use this to update services to utilize the forwarded
port.
/// Path or alias to executable script or binary to be called with the port as an argumnet
/// when the port forwarding is refreshed (PIA only)
#[clap(long = "port-forwarding-callback")]
pub port_forwarding_callback: Option<String>,
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have an example use of this? It'd be nice to add to the docs 🙂

@jamesmcm
Copy link
Owner

Thanks overall it looks great, btw you should be able to run rustfmt with cargo fmt and clippy with cargo clippy locally to run those checks directly (and cargo fmt will reformat it directly).

I think the only thing left is to update the docs (README/USERGUIDE) on the usage too.

jamesmcm and others added 4 commits January 20, 2024 14:20
Adds support for new API for Wireguard device management so we can
generate and add Wireguard keys directly when syncing
Re-add Mullvad Wireguard device management
Copy link
Owner

@jamesmcm jamesmcm left a comment

Choose a reason for hiding this comment

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

Thanks, I added the new args to the docs

@jamesmcm jamesmcm merged commit 5c7e591 into jamesmcm:master Jan 20, 2024
2 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

2 participants