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

Move routing and name resolution changes into an external script? #678

Open
DimitriPapadopoulos opened this issue Apr 28, 2020 · 17 comments

Comments

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Apr 28, 2020

I'm wondering whether moving the code that changes routing and name resolution parameters to an external script would help write and maintain openfortivpn. Typically most of ipv4.h/ipv4.c would move to that script. Additionally an option should be provided to run an alternative script.

Cons:

  • Spawning an external script instead of handling that internally in the C code is bad - or so I thought.
  • Providing an option to run an external script to be run with administrative rights is a security hole. This risk needs to be discussed and mitigated. See for example Wrapper for ip on the OpenVPN community site.

Pros:

  • A script language is probably more suited to changing routing and name resolution parameters in a portable way.
  • Relevant code would probably shrink at 10% of the initial C code and would be easier to maintain.
  • Would help implement and maintain alternative methods of handling the name resolution subsystems (NetworkManager, systemd-resolved, resolvconf) as envisioned in Improve DNS handling? #600.
  • Linux packagers have a chance to easily adapt the name resolution stuff to their specific distribution.
  • We already have a PR that suggests we add systemd-resolved support as en external script: Add support for systemd-resolved #615.
  • It could actually help run openfortivpn without sudo privileges (or at least drop such privileges after spawning pppd). See Revisit running openfortivpn as root? #650.
  • IPv6 support would be easier.
  • OpenVPN have ended up doing the same, see OpenVPN - ArchWiki.
@adrienverge
Copy link
Owner

Hey Dimitri, thanks for sharing the idea!
I have taken too much distance to give valuable feedback, all I can tell is that running external shell scripts from a sudo program has always scared me. However you seem to say that it would allow running openfortivpn without root privileges, which I see as a very good move.
You and Martin have proven to master the code of this repo, so I'll trust the choice you make!

@DimitriPapadopoulos
Copy link
Collaborator Author

Indeed external helper programs with SUID set may help limit the part of the code that runs as root. I think that's what FortiClient does. Of course external helper programs don't have to be scripts. Like you, I feel uneasy about running SUID scripts (they actually require a C wrapper). However they're really much easier to write, maintain and adapt to each Linux distribution.

It would also help with macOS: the DNS part is currently broken because changes to /etc/resolv.conf are not taken into account by recent macOS releases. Instead we have to call a specific Objective-C API - or run a command from a script.

The real obstacle is spawning pppd with option noauth. Do you know why that option requires root privileges and why it is required? Is there no way around it?

@adrienverge
Copy link
Owner

I'm sorry, I have no idea...

@zez3
Copy link

zez3 commented May 1, 2020

The real obstacle is spawning pppd with option noauth. Do you know why that option requires root privileges and why it is required? Is there no way around it?

See:
https://www.tldp.org/HOWTO/pdf/PPP-HOWTO.pdf#%5B%7B%22num%22%3A517%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2Cnull%2C798%2C0%5D

An alternative (and better method) to this set up is to use the sudo program. This offers superior security andwill allow you to set things up so that any (authorised) user can activate/deactivate the link using the scripts.Using sudo will allow an authorised user to activate/deactivate the PPP link cleanly and securely.
"
So the user running the VPN client will need to add itself in this new(PPP | openfortivpn or whatever the name will be) group. This will/should happen ideally during the the initial installation phase.

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented May 1, 2020

@zez3 Indeed using sudo sounds interesting, yet I'm not certain what you have in mind exactly. Could you please help me get a clearer view? Please note that what is discussed here is not how to set up sudo so that you don't have to enter a password when you run sudo openfortivpn - I already do that on my workstation. Instead we discuss how to get rid of the need to run openfortivpn with sudo, at least in some scenarios.

  1. Routing and name resolution changes can be handled by openfortivpn (in which case you need sudo openfortivpn anyway), handed over to pppd, or left to the caller (for example in the case of NetworkManager-fortivpnssl). Obviously the two latter scenarios are the ones of interest here.
  2. Spawning pppd with option nooauth requires to be root (being in group pid, PPP or whatever it's called on various Linux distributions is not sufficient). At least that's my understanding right now and I cannot find a way round it. How can I arrange for pppdto be spawned as root if openfortivpn is started without sudo?

@zez3
Copy link

zez3 commented May 3, 2020

Yassou Dimitri,

what I've meant is that openfortivpn should do all the necessary steps during initial install and check at every new connection that pppd has still the right permissions:
chmod u+s /usr/sbin/pppd

http://www.iitk.ac.in/LDP/HOWTO/PPP-HOWTO/x1569.html
But I guess it's already doing that.

openfortivpn should not need to do those routing changes directly. That should be handled by pppd
or caller like you said. Or should it?, see bellow...
That unless someone writes it's own pppd implementation. So then it hit me!
and I found myself reading this master thesis:
https://pdfs.semanticscholar.org/fd1b/d979b31c87e5e55a1582b02482f012596051.pdf
"But the main cause of the poor throughput performance seems to relate to the encoding and decoding of HDLC-like frames which is required to communicate with pppd. As long as this has to be performed, a pppd-based solution can simply not compete with a client which is not relying on HDLC-like framing.We will therefore conclude that a pppd-based solution is not viable when seeking to achieve high VPN throughput rates.
"
where the speed culprit seems to be exactly pppd
#428
This is how it looks on my mac:
image

I will post more in the appropriate issue

This still does not solve the noauth root need. For an pppd connection to work without noauth we probably need the tunnel server to authenticate itself by writing an password in the /etc/ppp/pap-secrets or chap-secrets
Again at install time and check at every new vpn connect attempt.

I've just tried this:
/etc/ppp/pap-secrets

# OUTBOUND connections

# Here you should add your userid password to connect to your providers via
# PAP. The * means that the password is to be used for ANY host you connect
# to. Thus you do not have to worry about the foreign machine name. Just
# replace password with your password.
# If you have different providers with different passwords then you better
# remove the following line.

# * password
* * "" *

and when I am calling my peer I no longer get the
"The remote system is required to authenticate itself but I couldn't find any suitable secret (password) for it to use to do so."
We should not spawn /usr/sbin/pppd noauth
/usr/sbin/pppd: using the noauth option requires root privilege

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented May 3, 2020

Actually openfortivpn should not always let ppp modify routing and name resolution parameters for multiple reasons:

  • ppp cannot handle part of the name resolution stuff: search domains, split DNS, more than 2 DNS servers, etc.
  • ppp is itself "broken" on some platforms such as macOS in the sense that it is unable to interact with the current name resolution subsystem.

The alternatives options are:

  • Let the caller handle routing and name resolution. This is already partly covered by --no-routes and --no-dns but we should find a more efficient way to pass parameters sent by the VPN appliance back to the caller: a simple file in /tmp or more elaborate IPC using D-BUS for example?
  • Have openfortivpn handle routing and name resolution. It looks like supporting systemd-resolved is part of the solution.

See #600. This part of the discussion should be moved there.

In any case I understand you have come to the same conclusion that noauth is required. See #650. This other part of the discussion should be moved there.

@DimitriPapadopoulos
Copy link
Collaborator Author

@zez3 Please do post your findings on macOS in #428. It might be worth delving into pppd_read() and pppd_write(): lots of things happen in there, including calling HDLC routines - but not only.

@mrbaseman
Copy link
Collaborator

First of all sorry, that I didn't comment here for a longer time.

We are currently discussing scenarios that involve IPv6 at work. I stumbled across this proposal. Actually, it might help a lot for the IPv6 support. The external tools (route, ip, netstat, ...) already support ipv6. We "simply" have to extract the ipv6 routes from the config that the fortigate pushes and find a way to use the tools in a portable way. On Mac OS X and FreeBSD we already use them, but ignore IPv6 so far. Parsing text output might be even more complicated for IPv6 addresses than for IPv4 ones. So an external library might help in this context, too.

As I have learned today, in a high level picture it doesn't make a difference which IP protocol is used.in which place. If our name resolution and the socket connections support IPv6 we should be able to establish a tunnel to an IPv6 gateway. If we can handle IPv6 addresses and IPv6 routes we should be able to enable IPv6 traffic through the tunnel. Using tools that already support IPv6 seems to be an easier task than rewriting the whole IP-C-code for IPv6 .

@dwmw2
Copy link

dwmw2 commented Apr 27, 2021

I'd recommend using vpnc-script. It handles IP and DNS configuration on a whole bunch of operating systems including Mac OS, Solaris, *BSD, various Linux setups. Just set the appropriate environment variables and spawn it, and it should do the right thing.

@DimitriPapadopoulos
Copy link
Collaborator Author

@dwmw2 Thank you for the hint about vpnc-script. Does the script really work with most Linux distributions? It does seem worth integrating it - but I really don't have much free time these days...

@dwmw2
Copy link

dwmw2 commented Apr 27, 2021

Most Linux distributions ship it already; we moved it out of the vpnc package itself so it could be a dependency for openconnect too. Even NetworkManager has a tool which 'collects' the environment variables and feeds the information back to NM for when the actual VPN client is running without root privs etc.

@pschichtel
Copy link

Continuing from PR #986

@DimitriPapadopoulos I've read a bunch of the related issue discussion yesterday and those left me asking the same question. if there was a way for ppp ip-{up,down}.d scripts to fetch information about the tunnel (e.g. DNS and IP routes pushed by the server, could be as simple as having a file), then I don't think there would be any benefit to a solution in openfortivpn directly. I'm doing routing and DNS like this for a while now because I don't agree with a few things the server pushes. It would be need if I could filter over what the server tries to push instead of replicating the list.

currently I do this:

#!/bin/bash

routes=(...)
dns_servers=(...)
search_domains=(...)

resolvectl dns "$1" "${dns_servers[@]}"
resolvectl domain "$1" "${search_domains[@]}"

for route in "${routes[@]}"
do
    ip route add "$route" dev "$1"
done

What I'm wondering:
Is it even possible to provide information about the tunnel (routes, DNS) before ppp calls the ip-up script? Or would that information be communicated over the tunnel itself?

If the information is available before ppp's ip-up, then why bother to implement your own up/down hooks? The script could just fetch the information from e.g. a json file and e.g. filter it with jq (#960).

If the information is not available before ppp's ip-up, then I guess it is necessary to at least provide some hook for the point in time when ppp is up and details about the tunnel are known.

I'm not familiar with the protocol, can anyone clarify?

@DimitriPapadopoulos
Copy link
Collaborator Author

That's exactly what vpnc-scripts does: get information DNS and IP routes pushed by the server from the calling program. There are multiple ways to achieve that:

  • a file,
  • IPC,
  • environment variables as vpnc-script expects.

If I were to choose, I would choose the latter - to avoid re-inventing the wheel.

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Jul 20, 2022

Additionally, the routing and DNS suggestions sent by the VPN server are parsed and known by openfortivpn, before even establishing the PPP tunnel.

Most of the routing and DNS changes are performed directly by openfortivpn in src/ipv4.c.

It should be conceptually "straightforward" to change the code in there into calls to vpnc-scripts.

@DimitriPapadopoulos
Copy link
Collaborator Author

DimitriPapadopoulos commented Jul 20, 2022

It is true that pppd is able to change DNS parameters on its own. This can lead to surprising bugs when both openfortivpn and pppd attempt to change these parameters. This is why we usually recommend --set-dns=1 and --pppd-use-peerdns=0 to make sure only openfortivpn changes DNS parameters - not pppd. The only exception is when pppd clearly handles DNS parameters better than openfortivpn, for example on recent macOS systems.

Moving to vpnc-scripts should remove any need to have pppd handle DNS parameters., as the script will hopefully handle DNS parameters better than pppd, even on macOS.

@DimitriPapadopoulos
Copy link
Collaborator Author

Ideally, we would also get rid of pppd altogether, and replace it with PPP code embedded in openfortivpn associated to a tun device, as suggested in #650 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants