Add dns suffix information to informative message

[charlesgoyard]

Hi,
I found out that printing the dns suffixes could be a first step to enable networknanager-fortisslvpn to scrape this information, and then report it back to whatever handles resolv.conf in NetworkManager. For now this information is not used by pppd.

On the other end, I think adding a stable parsable output would be a more appropriate, proper solution.

Sample output:

VPN account password:
INFO:   Connected to gateway.
Two-factor authentication token:
INFO:   Authenticated.
INFO:   Remote gateway has allocated a VPN.
Using interface ppp0
Connect: ppp0 <--> /dev/pts/1
INFO:   Got addresses: [0.0.0.0], ns [0.0.0.0, 0.0.0.0], ns_suffix [sub.example.com example.com]
INFO:   negotiation complete
INFO:   negotiation complete
Cannot determine ethernet address for proxy ARP
local  IP address 0.0.0.0
remote IP address 0.0.0.0
INFO:   Interface ppp0 is UP.
INFO:   Setting new routes...
INFO:   Adding VPN nameservers...
INFO:   Tunnel is up and running.

[DimitriPapadopoulos]

I understand NetworkManager-fortivpnssl does not get the name servers and search domains directly from openfortivpn, instead the information is passed to pppd (hence the --pppd-use-peerdns=1 patch) and retrieved by this pppd plugin:
https://gitlab.gnome.org/GNOME/NetworkManager-fortisslvpn/-/blob/master/src/nm-fortisslvpn-pppd-plugin.c

And indeed the usepeerdns option of pppd handles only two DNS servers, no search domains:

usepeerdns
Ask the peer for up to 2 DNS server addresses. The addresses supplied by the peer (if any) are passed to the /etc/ppp/ip-up script in the environment variables DNS1 and DNS2, and the environment variable USEPEERDNS will be set to 1. In addition, pppd will create an /var/run/ppp/resolv.conf file containing one or two nameserver lines with the address(es) supplied by the peer.

Here you argue the information should be retrieved directly from openfortivpn.

Am I correct?

[charlesgoyard]

We understand the same thing about pppd, and you are correct, I'm talking about enabling a direct conversation between the networkmanager plugin and openfortivpn.

My thinking is that networkmanager can handle the resolv.conf file by itself, without the help of pppd.
So in order to lift the pppd limitation, yes, the resolver configuration could be handled by networkmanager.

Also, this could be a first step to getting the networkmanager plugin handle the "Two-factor authentication token" prompt (which is another topic).

But all of this is a bit of a stretch, it requires a lot more work to have a nice integration between networkmanager and openfortivpn.

[DimitriPapadopoulos]

It would help to document the desired interaction between openfortivpn and NetworkManager-fortivpnssl.

For example we could add openfortivpn plugins, the same way pppd supports plugins. But frankly this looks like overkill. I don't think NetworkManager needs openfortivpn to execute specific code. Rather some kind of limited interprocess communication (IPC) is required.

On Linux D-BUS has become a standard for IPC between system components. Note that openfortivpn already notifies systemd, precisely after setting routes and DNS - or not:

openfortivpn/src/tunnel.c

Line 127 in 8b0c235

sd_notify(0, "READY=1");

Perhaps it wouldn't be too complicated to get openfortivpn to notify NetrowkManager as well as systemd. Then it would be up to the calling program to manage routing and DNS (whether the calling program is NetworkManager or a script bundled with openfortivpn). Thus openfortivpn could focus on what it does best: initiate the communication with a FortiGate appliance and "spawn a pppd process and operate the communication between the gateway and this process".

[charlesgoyard]

I totally agree with what you say, an proper, simple IPC channel is the way. Please reject my PR and let's start thinking about it.

Also, I found out that there is something started in this direction with how TFA is handled (--pinentry). At the moment I could not make it work with SMS, but maybe it's just me.

[DimitriPapadopoulos]

@charlesgoyard That said printing the search domain makes sense, not only to help improving NetworkManager-fortisslvpn but also for debugging and information.

A couple comments:

• I believe MAX_DOMAIN_LENGTH should be moved from src/xml.h to src/ipv4.h - why #include "xml.h" if we don't need to parse XML?
• In the long term we probably need something more readable than these 1 + 15 + 7 + 15 + 2 + 15 + 1 + 1 + 7 + MAX_DOMAIN_LENGTH(256) + 1 comments, but that I will fix that in a separate PR (see Improve (?) comments about buffer length #644). It's the length of this line that breaks checks. Can you break the comment in two separate lines or a single line above the declaration?

[DimitriPapadopoulos]

The more intrusive change with respect to the rest of the code is moving MAX_DOMAIN_LENGTH from xml.h to ipv4.h. We now have to include ipv4.h from xml.c but it makes sense: domain length has more to do with IP than XML!

[charlesgoyard]

Thanks! Nice cleanups as a bonus.

[DimitriPapadopoulos]

Note that callbacks for setting IP routes and name resolution might not be a bad idea after all. See:

• Improve DNS handling? #600
• Add support for systemd-resolved #615
• Revisit running openfortivpn as root? #650