FreeBSD support

[mrbaseman]

I have started this work some time ago. At the current stage I have managed to configure, compile and run openfortivpn on FreeBSD 11.
The last remaining issue is that FreeBSD doesn't have pppd anymore and uses a user-space tool called ppp. It doesn't take many options from the command line but rather from STDIN or from a config file. To my understanding of the man page it should be possible to put all necessary parameters into /etc/ppp/ppp.conf and openfortivpn should work with this. If this works, the only missing bit is to provide a working sample configuration and perhaps an update of the man page.
Many command line options of openfortivpn concerning how pppd is called are currently not supported on BSD. At a later stage one can modify the way ppp is called and pipe setup commands into the client before sending it to the background. I just haven't figured out to do this correctly in combination with forkpty, but we can keep this as a separate task, once ppp has successfully opened a connection by feeding it with appropriate parameters from a config file.

[DimitriPapadopoulos]

You check for pppd/ppp at build-time. It is possible the build machine lacks either of them. They are technically required on the machine running openfortivpn only. How do you handle that?

[DimitriPapadopoulos]

<termios.h> is POSIX and should be available on all supported platforms. While the #ifdef does't really hurt, perhaps we should remove it for simplicity.

[DimitriPapadopoulos]

Out of curiosity, what is <libutil.h> for?

[mrbaseman]

since we didn't have to include termios.h explicitly on other platforms than FreeBSD so far, I have included it in the #ifdef section, honestly without further investigations if this header should always be there.

The reason for both headers is the synopsis of forkpty

[DimitriPapadopoulos]

Header <termios.h> is included implicitly on Linux, it just needs to be included explicitly on all platforms. Build should fail without <termios.h>. I suggest removing the #ifdef from the source file and having autoconf fail when <termios.h> is missing.

[DimitriPapadopoulos]

Build should fail when any of the required headers is missing (<stdio.h>, <unistd.h>, etc.). Can this be enforced in autoconf, the same way autoconf fails when a required function is missing?

[DimitriPapadopoulos]

Indeed it looks like FreeBSD is not compliant with recent versions of POSIX which specify fokrpty() is defined in <termios.h>.

I checked FreeBSD's <termios.h> does not define forkpty(), <libutil.h> does.

[mrbaseman]

I just see Travis build failed due to the missing runtime dependency of pppd. Maybe such dependencies should be implemented as configure options --with-pppd=/usr/sbin/pppd and --with-ppp=... etc.? Any hints from an autoconf-Guru how to do this better are welcome ;-)

[mrbaseman]

@DimitriPapadopoulos our comments just crossed... Travis has given me the same feedback and as a quick fix I have changed the code back to default behavior when /usr/sbin/ppp is not available.
But you are right, auto-detection at build-time is probably not what we want here. I'll check for better solutions, see my comment above

[DimitriPapadopoulos]

I think it would be OK to postpone detection and switch between /usr/sbin/pppd and /usr/sbin/ppp at run-time.

I don't see a security issue here because /usr/sbin is not supposed to contain rogue ppp or pppd executables.

[DimitriPapadopoulos]

Perhaps build-time is better though. No need to push cross-platform code in an executable specific to a single platform. Options --with-pppd/--with-ppp are they way to go as far as I can see:
3. Adding Options

[DimitriPapadopoulos]

While pppd_run() can switch between pppd and ppp pppd_terminate() is specific to pppd as far as I can see. Am I missing something?

[mrbaseman]

• run-time detection of pppd vs. ppp was quite easy to implement because basically it was already there.
• I'll check the build-time options tomorrow
• you are right, I forgot to handle this in pppd_terminate(). When I started to work on the BSD support, the pppd specific error messages weren't yet there. Probably the name of the program was already present in the error messages but just together with an exit code. I have to check the ppp man page again, for possible exit codes and what they mean
• About required headers, there is also AC_CHECK_HEADER, but if we want to make several ones required, we can also change the AC_CHECK_HEADERS line. However, not all of them are present on all platforms, so we need a second line with optional headers, just like we do for the functions. @DimitriPapadopoulos, in your opionion, which headers are required ones which should always be there?

[mrbaseman]

I would assume that all includes that are not enclosed in an ifdef statement are required ones...

[DimitriPapadopoulos]

Exactly, all headers not protected by #ifdef HAVE_... could be added to the list.

However do we really need to check for headers that are documented in the C standard (<stdio.h>, <stdlib.h>, ...) or have been in POSIX for years (<unistd.h>, <sys/socket.h>, ...)? That means more maintenance for no clear benefit. I would only check for headers/functions not available on some of the platforms we support (usually because they're a "recent" addition to the Single Unix Specification).

[DimitriPapadopoulos]

Return values of ppp are not documented.

The FreeBSD source code shows that main() returns or exit() is called inconsistently with numerical values or EX_START/EX_NORMAL/EX_REDIAL/EX_RECONNECT/EX_...:

• exit(EX_START);
• exit(1);
• return c;
• return EX_NORMAL;

Return values of ppp are probably less informative than those of pppd.

[mrbaseman]

the distinction between required and optional headers was quite straight forward.
The output of grep '#' src/* produces about 350 lines which are quite easy to read.
I think the list of required headers is much easier to maintain than the list of required functions and I would expect that the configure script complains about missing headers rather than smoothly running through and producing a Makefile which doesn't allow to compile on the given platform.

[DimitriPapadopoulos]

You're right, the list of header files is rather short.

The list of functions is much more difficult to get right.

[mrbaseman]

concerning ppp's exit codes: To a large extend sysexits are used and for them err() could be used. I'll have a look and update pppd_terminate() accordingly.
PS: strerror() is recommended for portable code instead of err()

[mrbaseman]

@DimitriPapadopoulos I think I have addressed all your comments now. Thanks for the input.
The mechanism for detection is now:

• at configure time the existence of /proc/net/route is checked, if it is not available, netstat is searched at well-known locations.
• Also, the existence of ppp is checked, and the path is stored in a preprocessor variable PPP_PATH. If pppd is found, it's location overwrites PPP_PATH.
• For cross-compiling there are configure-options that override the detected values. --with-pppd and --with-ppp override PPP_PATH and additionally set HAVE_USR_SBIN_PPP or HAVE_USR_SBIN_PPPD respectively. Depending on which one is set, it's assumed that pppd shall be used or ppp respectively.
• --enable-proc allows to override HAVE_PROC_NET_ROUTE if the first test resulted in a wrong result when cross-compiling.
• At run-time the existence of the file at PPPD_PATH specified at configure-time is double-checked

[gbon121]

Hi Martin.
I don't have a FreeBSD installation to test on, but from the online manpage for ppp, it seems that

• trusted non-root users can invoke ppp, if an appropriate allow users option is added to the vpn-client stanza of /etc/ppp/ppp.conf, or they belong to the network group;
• environment variables can be used to pass information to the ppp process (e.g. remote login credentials);
• if present, the /etc/ppp/ppp.linkup script is executed when the ppp link comes up, and very likely it inherits the environment of the ppp process. Update: it's not a script, just a piece of configuration that is read later.

If the above conditions are met, then perhaps an easy way to handle additional routes and nameservers is to set environment variables in the (unprivileged) openfortivpn process, and use native commands in ppp.linkup, along the lines of #270 Nope.

Update: Section PERMISSIONS in ppp(8) clearly states that external commands ("shell" or "bg") are executed with the user id that invoked ppp.

[mrbaseman]

I didn't have time to work on the BSD port in the last two weeks. I'm stuck with the problem that logging in and establishing the tunnel seems to work but the read_thread always reads 0 bytes. My assumption is that the FD_SET didn't work as expected.

With the configuration my current approach is to have a static config file that already contains the important parameters (/etc/ppp/ppp.conf seems to be the usual place). Once this works, the next step would be to pass parameters in a more elegant way to ppp, maybe using a diagnostic port and pppctl.

Maybe the problem with the file descriptor is that the device line in the ppp config is still wrong and ppp opens a separate tcp tunnel in parallel to the existing one. Probably I should pass the file descriptor of the tunnel here. This doesn't work with a static config file, of course, but that's the current area I'm looking into. Maybe a first workaround could be a symlink in /dev with a known name which can be added to the config file... (there is no /proc on FreeBSD and even if I mount it, there are no fd subdirectories in the entries, so the symlink approach probably won't work)
Anyhow, I still have the feeling that I'm quite close to a working openfortivpn on FreeBSD now.

[DimitriPapadopoulos]

Perhaps you should change pppd, Linux-specific (part of) PPP implementation, to ppp in user-visible text, including --pppd-... options.

[mrbaseman]

@DimitriPapadopoulos good point. Having just one letter more or less in the debug output isn't worth the hassle with extra string buffers... but I wouldn't touch the command line options now. --pppd-no-peerdns for instance is probably going to disappear anyway (#330) in the long term, and we have noticed that changing the command line options in a smooth way isn't that easy

[DimitriPapadopoulos]

You're right, in any case let's stabilize FreeBSD support and the DNS stuff first.

That said it's very easy to change pppd into ppp because there are no functional changes, just need to support both option styles for some time. By the way, do pppd options on Linux match ppp options on FreeBSD?

[mrbaseman]

ppp on FreeBSD only supports a few options on the command line. Most of the connection details have to be supplied either in a config file /etc/ppp/ppp.conf with which I was experimenting a bit, or on stdin but I'm not sure how this works together with the pthreads that we are running at this stage.
I was also trying to get pppd onto a current FreeBSD, just to see that everything else is working now, but I didn't succeed in that either. Probably I have to focus on debugging the communication of ppp with the remote side - first of all ensure it's talking on the right file descriptor and also figuring out all the config parameters needed to make it talk to a pppd-like service on the other end of the line.
I must admit that I didn't have much time to work on this in the past weeks.

[mrbaseman]

I have just compiled and tested my bsd_support branch on MacOS X.
There it works smoothly, too. I've just fetched the latest cleanup commits by @nferch, too

[DimitriPapadopoulos]

@mrbaseman Excellent work Martin!

[adrienverge]

I confirm that compiling your branch (commit e66e1d3) on Fedora 28 works fine.

👍

[DimitriPapadopoulos]

Martin, note that systemd is not required. Does the build fail if systemd is missing on Linux?

[mrbaseman]

Yes. I have tested the branch again on a few platforms: Ubuntu 16.04, CentOS 7.5, SLES 12.3, and SL 6.10. On the latter I have found that configure fails due to the absence of libsystemd.

Maybe it's worth cherry-picking 06d3241 into a separate PR? It's a fix to the autoconf system, but it's unrelated to FreeBSD and it fixes an issue which is already present on the current master. I'm not sure if "absence of libsystemd on linux" is the correct condition. Maybe also a particular version of autoconf is needed to reproduce this. (in this case it was autoconf (GNU Autoconf) 2.63)

[DimitriPapadopoulos]

Indeed a separate PR would be better. Please pull it directly.

I seem to recall I tried on a Linux machine without systemd and that it worked for me: build didn't fail, simply HAVE_SYSTEMD wasn't defined. I may be wrong though, so your patch is welcome.

[mrbaseman]

shall we merge this now?

[adrienverge]

OK for me, I trust you and Dimitri's review!

[DimitriPapadopoulos]

Will you squash the commits?

Other than that I suggest you merge. I'll test the master branch myself for a few days. Then perhaps we can publish a new version in a few weeks :-)

[DimitriPapadopoulos]

Ah, just one general remark. It looks the build system now checks whether some files that are needed at run time on the production machine are available at build time on the build machine (/usr/sbin/ppp, /usr/sbin/pppd, /usr/sbin/netstat, /usr/bin/netstat, /proc/net/route). There are overrides for some of them. But shouldn't all possibilities be hard-wired in the program at build time and then tested at run time?

[mrbaseman]

There are overrides for almost all of them. If /proc/net/route is not available on the build system, it can be enabled via --enable-proc, the location of netstat can be controlled with --with-netstat, and the ppp program can be adjusted by --with-pppd or --with-ppp respectively.
Ah, there is no --disable-proc for cross-compiling for FreeBSD on Linux.... Ok, I'll check that.
With "tested at run time" you mean we should check if the program that we call really exists and is executable? We did for pppd in the past and it's done for ppp also, but it wasn't done so far for netstat and for route for instance, but I can add such tests, too.
So, the agenda is:

• implement a --disable-proc configure option
• add checks like if (access(pppd_path, F_OK) != 0) for the other external binaries
• repeat testing
• squash and merge :-)

[DimitriPapadopoulos]

Yes, adding the option/checks you suggest could fix some edge cases (production machine very different from build machine).

I would even suggest getting rid of AC_CHECK_FILE in configure.ac. Finding the proper executables initially looked like a run time task to me, not a task that can be automated at build time based on characteristics of the build machine that may differ from the production machine. On the other hand testing multiple paths to find an executable file, instead of a single path, may be seen as a security issue (larger attack surface). So I would eventually stick to a single path for each executable on Linux, FreeBSD and macOS, and let users override the path at build time if needed. Which is almost what you do: I was just arguing that it's perhaps better to hard-wire the default executable path for each platform instead of relying on AC_CHECK_FILE. The override remains useful of course and should be added for all hard-wired paths.

I don't know if this makes sense. What do you think? Either way the current situation is OK with me if you'd rather stay with AC_CHECK_FILE. I have no strong opinion on this issue.

[mrbaseman]

I would like to stay with the configure mechanism. We don't have a reliable mechanism to detect a specific platform. At compile time we used to have some #ifdef __APPLE__ but large parts of this code are re-used on BSD, and there we don't have this macro definition. With every new platform the decisions would become more complex, and apart from that it's not at runtime anyway. Also, the configure mechanism might work out of the box on other platforms that are similar to the ones that we already support.

I have re-worked the override options now, because they were not working in both directions. I have added comments in configure.ac and to the commit message, where I have explained the relevant details about autoconf. Basically, the order in which the statements are put into the file is not necessarily the order in which they are executed, at least that's my interpretation of what I have observed in various tests of the override options.

I have also added a few runtime checks for the compiled in paths. I have also hard-coded the absolute path to the route command on OSX and BSD, while being at it. I believe we can't do such a check on the special file /proc/net/route but there is already a check to handle failure of the open() command.

[mrbaseman]

I have re-tested with the current state on Ubuntu 16.04, CentOS 7, SL 6, SLES 12, FreeBSD 11, OSX 10.11. Building works fine on all of them, as client I could test Ubuntu, OSX, and FreeBSD. I'd merge it now. I it turns out that it needs further adjustment, then we can improve the code in separate pull requests.

[DimitriPapadopoulos]

Agreed.

[adrienverge]

👍