jotyGill / openpyn-nordvpn

Easily connect to and switch between, OpenVPN servers hosted by NordVPN on Linux (+patch leakes)
GNU General Public License v3.0
628 stars 114 forks source link

Drop external deps #2 (Review) #226

Closed 1951FDG closed 5 years ago

1951FDG commented 5 years ago

management.py 151, openpyn.py 538 885 939 989: we can't have f"" strings if we want python 3.5 compatibility.

Completely overlooked this, initial pull request by ranisalt included them, f"" strings were new to me and I was like, cool, let use them :) Will this be a major issue for users on Python 3.5?

jotyGill commented 5 years ago

Yeah the f"" strings error out on python<3.6 . we have gone through the pain of keeping it compatible with 3.5 . shouldn't break that now :)

1951FDG commented 5 years ago

If you want, I can make another commit, to use str.format() alternative to f"" strings

jotyGill commented 5 years ago

That would be great, if you could! -- Securely sent with Tutanota. Get your own encrypted, ad-free mailbox: https://tutanota.com

1951FDG commented 5 years ago

I'll try to incorporate these changes before the end of the month!

1951FDG commented 5 years ago

@jotyGill, all changes have been made finally, a month later :) I also ran the tests this time, and added one new test as well!

jotyGill commented 5 years ago

@1951FDG Thanks for putting all that work into it. Really appreciate it! :)

jotyGill commented 5 years ago

@ranisalt @1951FDG The idea of keeping config files in user's directory and not use root is great but seems to be problematic in this situation. The problem is "openvpn" needs to be run as root. if we want to create a systemd service, it needs to start "openpyn" as root because we can't interactively provide "sudo" password. Other issue of permissions comes into play; if you use "sudo openpyn --update" for example, the user can't update those files later. (This can be resolved by resetting permissions to 666 on all config files).

I am trying to keep the configs in ~/.local/share but have the global systemd.service file (run by root, as before). This introduces another issue (not really issue but waste of space) of 2 config locations, one for regular user one for root.

Is there any other way of resolving this?

I guess passing "_xdg_data_home" variable point to the locatoin of regular user to the openpyn.service file could do. (reliably determining the logged in user across multiple environments, distros/docker could be another thing. i know os.getlogin() is not always reliable)

When you install it using "python3 -m pip install --user" it won't be in root's path. would need a symbolic link like sudo ln -s /home/user/.local/bin/openpyn /usr/local/bin/openpyn.

Then for different OS's the global location for python3 bins could be different. the older version might be in /usr/bin/openpyn for example. might be a conflict for some.

All of these, I am now questioning is it really worth it.

ranisalt commented 5 years ago

we can't interactively provide "sudo" password

We can have a minimal shell script with suid bit, as does profile-sync-daemon to mount filesystems non-root.

the user can't update those files later

Maybe deny running as root? This also makes the entire script safer.

jotyGill commented 5 years ago

We can have a minimal shell script with suid bit

Interesting! but looks like it may not be possible run shell scripts that way anymore? https://unix.stackexchange.com/questions/364/allow-setuid-on-shell-scripts

even if we manage to run openvpn using a shell script (as root using suid) and pass the openvpn arguments to it. We need root access to run all of iptables commands. Is there a way to resolve this if so, could you please look into it. Thanks

ranisalt commented 5 years ago

For one, there's openvpn-unroot as a script to help. I don't know about managing iptables, though. Probably won't be able to drop sudo as external dependency, but avoiding it as much as possible helps.