10000 Use custom_config variable to verify custom config is specified rather than only looking in commandline. by vulpyne · Pull Request #135 · jamesmcm/vopono · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use custom_config variable to verify custom config is specified rather than only looking in commandline. #135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 5, 2022
Merged

Use custom_config variable to verify custom config is specified rather than only looking in commandline. #135

merged 1 commit into from
Feb 5, 2022

Conversation

vulpyne
Copy link
Contributor
@vulpyne vulpyne commented Feb 5, 2022

Note: I made this change without really understanding the rest of the code or testing with non-custom providers. It compiles appears to work in this case. It seems like the simple/obvious solution but I'm not 100% sure it's correct. Please review before merging.

Closes #134

@jamesmcm
Copy link
Owner
jamesmcm commented Feb 5, 2022

Thanks, this also made me realise we might want to add the open ports and port forwarding to the config file too.

@jamesmcm jamesmcm merged commit a4b436b into jamesmcm:master Feb 5, 2022
@vulpyne
Copy link
Contributor Author
vulpyne commented Feb 5, 2022

@jamesmcm

Thanks, this also made me realise we might want to add the open ports and port forwarding to the config file too.

Glad to help! The port stuff is not something I'd need at the moment personally, but I'm sure it would be useful.

I'm not sure if I should just create issues for this instead but a couple other things I noticed:

  1. vopono requires DNS to be set in the Wireguard config even though Wireguard itself doesn't.
  2. vopono seems hardcoded to just route all traffic over the VPN and if the Wireguard config is set up with AllowedIPs to only route some stuff it won't work correctly. This is also not documented as far as I can see.

Unfortunately, I don't currently have the time to dig into fixing those issues myself. The second one doesn't really affect me negatively once I realized what was going on.

@jamesmcm
Copy link
Owner
jamesmcm commented Feb 5, 2022 •
edited
Loading

Thanks, the first one should be manageable - we can just set it to 8.8.8.8 (or user provided --dns ) if missing for example, like with OpenVPN (though there it also checks for the server response).

The second is trickier as it's harder to know what it should do, especially since the killswitch is intended to block all other traffic from getting out of the network namespace.

@vulpyne
Copy link
Contributor Author
vulpyne commented Feb 5, 2022

The first one isn't too much of a problem, since the fix is obvious (even with no changes to vopono). The handling you mentioned is definitely more ergonomic though!

For the AllowedIPs thing I think just adding a little documentation and showing a warning if the WireGuard config isn't set to handle all traffic would be all that's really needed. The biggest issue with it is that the behavior is unexpected and the reason it doesn't work in that case may not be obvious (especially for a non-technical user.)

jamesmcm added a commit that referenced this pull request Feb 12, 2022
@jamesmcm
Switch to Clap v3
982b2bb 
StructOpt is removed as a dependency

Also make DNS setting in Wireguard config optional
And added note regarding AllowedIPs in Wireguard config needing to
allow all IPs as discussed in PR #135
@jamesmcm jamesmcm mentioned this pull request Feb 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom must be specified on commandline even when custom_config is in configuration
2 participants
@vulpyne @jamesmcm
0