triffid / pia-wg

Linux shell scripts for connecting to Private Internet Access VPN's Wireguard service
121 stars 14 forks source link

Script fails as user due to input redirection issues #5

Open IncredibleLaser opened 2 years ago

IncredibleLaser commented 2 years ago

The script fails to establish the connection to PIA due to input redirection issues.

Any lines that make use of it with sudo fail (e.g. https://github.com/triffid/pia-wg/blob/a5a68d1922aa8d7c934d5cea9c869bd6cd252313/pia-wg.sh#L487) with fopen: No such file or directory. The problem from my understanding is that the command ran is a different user than the one creating the input redirection and thus failing.

On this machine, the issue can be observed easily:

nano <(echo "test")

opens /dev/fd/63 with the content "test".

sudo nano <(echo "test")

opens the same file, but it appears empty. To run the command with sudo successfully, this one works:

sudo sh -c "nano <(echo \"test\")"

spawning a full subshell with elevated privileges, however this requires escaping any quotation marks.

I don't really have an easy solution for the problem, but this script as is doesn't work here.

Versions: sudo: 1.9.11p3 wg: v1.0.20210914 bash: 5.1.16(1)

triffid commented 2 years ago

Hmm I'll look into it

Usually I just sudo ./pia-wg.sh or similar

Perhaps I should just remove all the sudo stuff and force it into config-only mode if run as user… Might make more sense?

IncredibleLaser commented 2 years ago

Honestly, I'm not sure really. One could argue creating a config file as user makes sense, but then storing it in $XDG_CONFIG_HOME when you don't actually use it doesn't really… normally network settings and routes are root stuff so maybe just dumping the config in $PWD with a comment that it can be moved to /etc/ makes sense?

I haven't fully thought this through so take with a grain of salt

triffid commented 2 years ago

Currently, if the script is run with -c (config only) as user, it'll generate a working config and just dump it on the terminal (and save it in ~/.config/pia-wg/)

If it's run as root (without -c), it'll apply the config to the local machine

If it's run as user without -c, we encounter the issue you reported - and that's a legitimate failure in the script as offered, but what is the appropriate fix?

Perhaps it should just assume -c if run as user, rather than trying (and failing) to help folk do stuff as root?

I could absolutely fix all the sudo calls to do the do, but is that the appropriate choice? I'm thinking no at this point

IncredibleLaser commented 2 years ago

I tend to agree with your assessment. Setting routes and other network settings is a root task and as such the script should just bail if running as a user and asked to do more than drop a config.

Where I don't necessarily agree is with creating a script in ~/.config/pia-wg/. "$XDG_CONFIG_HOME defines the base directory relative to which user-specific configuration files should be stored." However, in this case, this isn't a user-specific configuration files as it is supposed to be used by a system service. It makes some sense to drop it there if you allow the script to be run as user. IMHO it should drop the generated config file in $PWD, but I'm kind of nitpicking here.

triffid commented 2 years ago

Yeah, as you can see your issue has unpacked a whole can of worms - which is good, I'm not adverse to a warranted ass-kicking ;)

At this point I'm inclined to resolve it by making "run as user" → "implicit -c" with a notification on the terminal output to this effect.

I think that would be the most sensible way to fix my previous lack of attention to this (admitted) mess.

IMHO it should drop the generated config file in $PWD

That's an interesting idea, but I'm not fully onboard with it - my personal usage case for user-mode run is just to feed the output from qrencode into my phone, and I don't particularly want wireguard configs dropped in random dirs all over my system because I'm too lazy to cd first.

Would it be acceptable to add an option flag somewhere for output location, and assume ~/.config/pia-wg/ if none is provided?

The default won't be ./ but it could be set to that if that's what you want.

IncredibleLaser commented 2 years ago

I'm in no way an authority regarding where files should be stored… I was under the impression that the current behavior is to output the config both to stdout and into ~/.config/pia-wg/. I merely objected to the latter being the default but as I said this is a minor and maybe cosmetic complaint and I wouldn't have opened an issue for this matter alone.

An option to specify an output path would be nice, but not strictly needed, in fact I'd personally say only stdout is fine (again this is personal preference) so in case you need both a config file and a qr code you could do something like pia-wg -c | tee $config_file | qrencode ..., otherwise just pipe or redirect stdout to a file.

btw I don't mean to tell you how to solve this particular issue, consider this brainstorming - it's your tool and I'm just trying to show some possibilities, but I'll be happy regardless of approach chosen for this tiny problem.