Daniel Lenski [Mon, 12 Apr 2021 03:23:11 +0000 (20:23 -0700)]
Set Fortinet DPD interval from server's config
The name of the tag implies that its purpose is DTLS-specific (<dtls-config heartbeat-interval="X">)
and that it requires the use of the DTLS heartbeat extension (https://tools.ietf.org/html/rfc6520).
However, PPP already contains natural analogues for keepalive (Discard-Request) and DPD (Echo-Request
and Echo-Reply), which appear to work just fine with Fortinet servers. We may as well use the server's
heartbeat/keepalive interval for a more generic, transport-agnostic DPD mechanism, since OpenConnect
and PPP already support it.
The official Fortinet client software for Windows/Mac appears to be very deficient at dead peer
detection, so this may represent a substantial improvement in functionality for some users.
David Woodhouse [Sun, 11 Apr 2021 17:02:39 +0000 (18:02 +0100)]
Fix --disable-ipv6 option
Commit 7e2129f8a9 ("ensure that openconnect_disable_{dtls,ipv6} do
nothing if vpninfo has ever been connected") added a check to disallow
setting this... if it wasn't already set, OR the connection had
previously been established. Which seems wrong; I think that was
supposed to have been *AND*.
Not that I object very much to this not working, a whole quarter of a
century after RFC1883 was published. But I wanted to use it for testing
PPP negotiation, and it wasn't working.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 9 Apr 2021 12:09:11 +0000 (13:09 +0100)]
Add 'proto' integer value to struct vpn_proto
Despite making no mention of this in the commit message, commit 423449253 ("rename (resp_buf, form_buf) → (req_buf, resp_buf) in f5.c
and fortinet.c") added a 'flavor' field to a bunch of functions in
auth-html.c and passed it through from the protocol code to indicate
some protocol-specific quirks for form parsing.
Until then, we had mostly structured the code so that there weren't many
cases where 'core' code needed to behave differently depending on the
protocol. There was only one case, in fact, to handle the stupid Pulse
ESP address family bug. That one *used* to check which protocol was in
use by comparing proto->udp_send_probes with &oncp_esp_send_probes.
So, add an integer 'proto' field to struct vpn_proto so it can be
checked without a strcmp. Remove the now-redundant 'flavor' field from
all those functions in auth-html.c since it's now in the vpninfo, and
make the ESP code check that for NC|PULSE too instead of the trick of
checking the proto->udp_send_probes function. Which was kind of icky.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Thu, 8 Apr 2021 21:32:32 +0000 (14:32 -0700)]
Multi-protocol support documentation
Mention F5 and Fortinet on the index page, explain the multi-protocol design
philosophy a bit more, and add a page with a concise list of the supported
protocols.
Daniel Lenski [Thu, 8 Apr 2021 21:35:08 +0000 (14:35 -0700)]
Expand F5 and Fortinet documentation
Explain currently-supported authentication modes, and request feedback on additional modes.
Explain Fortinet's misfeature/design flaw, which prevents it from automatically reconnecting after a dropped or roamed connection, without a new authentication. Request feedback if anyone has acceess to a Fortinet VPN that *doesn't* have this flaw.
Daniel Lenski [Mon, 5 Apr 2021 15:37:00 +0000 (08:37 -0700)]
Use oc_text_buf for internal_get_url()
Commit c8ca180f ("factor out internal_get_url function") consolidated
two pieces of code which reconstruct the URL from its constituent parts
into a single internal_get_url() function.
Both the original versions used oc_text_buf() and built up the URL
string in a simple and readable fashion. The new internal_get_url()
function used asprintf with "%s%s%s" and ternary operators ?"/":"" in an
apparent attempt to win an obfuscated C contest.
Unsurprisingly, the first attempt was buggy; this was subsequently fixed
in commit f78f0eccc ("bugfix internal_get_url") but the horridness
remained.
In addition, the original two versions had slightly different behaviour —
the one in auth-juniper.c would *always* append a trailing / at the end
of the URL string even if vpninfo->urlpath was NULL, while the one in
AnyConnect's auth.c would only do so if it was non-NULL (including when
it's set to "").
As well as switching back to using oc_text_buf to build up the string so
that we can read the code without our eyes bleeding, fix it to always
add the trailing / even when vpninfo->urlpath is NULL.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Fri, 2 Apr 2021 07:57:14 +0000 (00:57 -0700)]
add test path including frmSelectRoles
frmSelectRoles is a "form" that sort of acts like a realm dropdown except that…
(1) It comes AFTER the credentials have been submitted (¯\_(ツ)_/¯)
(2) It doesn't actually contain any form fields. It only contains links to
a small number of role choices. It then redirects to 'GET login.cgi'.
Inane, but apparently used in the real world. See examples at:
https://gitlab.gnome.org/GNOME/NetworkManager-openconnect/-/issues/30#note_988366
David Woodhouse [Sat, 27 Mar 2021 16:17:57 +0000 (16:17 +0000)]
Add Wintun support
Refactor the device discovery a little bit to accommodate it. We need
to scan the registry as we did for TAP-Windows anyway, in order to
find the TUNIDX that vpnc-script-win.js will use to configure routes.
So run through the scan as before, but *if* nothing is found and the
user specified an ifname, attempt to create a Wintun device with that
name. Then scan again and presumably we'll find it this time.
This should hopefully make it relatively harmless to existing setups
using TAP-Windows.
There's lots of memcpy here as Wintun *really* wants us to use its pool.
More thought required for that one, as it can't even handle stripping
packet headers so even if we decrypt directly into its buffer that
wouldn't be enough.
The configuration is also fairly horrid, as vpnc-script-win.js seems
to depend on the Legacy IP address being set on the interface before
it will set up the routes. Which of course assumes that there *is*
Legacy IP at all, and it isn't an IPv6-only tunnel.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Mon, 29 Mar 2021 23:59:08 +0000 (16:59 -0700)]
bugfix !165 Juniper forms handling
I missed a small piece of my original "fix for forms with 'id' but no 'name'" (this one:
https://gitlab.com/openconnect/openconnect/-/commit/c8a6ee94180a09339eafefe0a2d889e7aa508e56#5503a3e92468be6db7c76e16b82bb5f3e9b14131_260_259)
in rebasing the ppp_core branch to merge cleanly with the changes from !171.
Caused this crash: https://gitlab.com/openconnect/openconnect/-/merge_requests/169#note_540411103
TODO: add Flask-based tests of Juniper auth to verify Juniper form handling.
David Woodhouse [Mon, 29 Mar 2021 16:44:21 +0000 (17:44 +0100)]
Fix installer deps
Just including .openconnect.exe.d from the main Makefile means it always
has to be up to date. Which means we always have to *build* openconnect,
even if it was just invoked for 'make clean'. That's bogus.
So shift the DLL dependency tracking out into its own Makefile, to be
invoked recursively. Much as I hate recursive make in the general case,
this is one of the few cases where it makes sense.
The automatic dependencies weren't working very well anyway, as it was
happy to 'discover' openconnect.exe and libopenconnect-5.dll in the MinGW
sysroot instead of using the locally built ones, *and* libtool messes up
the filenames so you can't just depend on .libs/openconnect.exe anyway.
Fix up $(DISTCLEANFILES) to be a bit more complete while we're at it.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 29 Mar 2021 16:09:32 +0000 (17:09 +0100)]
Fix handling of downloaded files
We want the MinGW package builds to have the downloads provided in advance
as part of the source RPM, instead of requiring network access and fetching
them once for each of the win32 and win64 builds.
So let make find the file in its VPATH and then use the full pathname that
it puts into $< for the recipe. And use that recipe to validate the hash
of the file too.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 29 Mar 2021 10:17:21 +0000 (11:17 +0100)]
Turn off -Wdeclaration-after-statement and allow C99
I think this far into the 21st century we can allow declarations to be
in the middle of code blocks. Even MSVC supports that, even if it doesn't
do other parts of C99 like named struct initialisers.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Mon, 29 Mar 2021 03:41:59 +0000 (20:41 -0700)]
flask-based tests: give up on CentOS7
For reasons that are not clear, OpenConnect fails to handshake a TLS connection
to Python 3.6 and Flask (from CentOS 7 EPEL), and I don't want to install
a 7-year-old distribution on a VM just to figure out why.
See https://gitlab.com/openconnect/openconnect/-/jobs/1135245618#L520 for
verification of the issue, and 9df05119a30dc3405ab56edbcab2576f46cac799
for a possibly-related issue with even older CentOS 6.
Daniel Lenski [Sun, 14 Mar 2021 18:38:38 +0000 (11:38 -0700)]
don't require F5 forms other than first one to have any particular name/ID
This allow-list of known forms is a relic of the HTML-form-parsing code
which was originally written for the Juniper protocol, and requires a bunch
of special-casing. I don't think it's particularly relevant for F5 forms.
Daniel Lenski [Mon, 22 Feb 2021 02:44:22 +0000 (18:44 -0800)]
make F5 and Fortinet tests go through config-pulling (up to the point of tunnel connection), rather than stopping after authentication
The fake servers don't actually implement the tunnel, but they do implement
the pre-tunnel configuration endpoints, and then simulate a "cookie rejected"
response upon tunnel connection.
OpenConnect distinguishes “cookie rejected” errors via an exit status of 2,
whereas any other failure results in an exit status of 1.
Daniel Lenski [Sun, 21 Feb 2021 23:02:44 +0000 (15:02 -0800)]
add auth-f5 tests
This tests OpenConnect's ability to authenticate with a (fake) F5
server, using username+password (the only option that OpenConnect
currently supports).
The fake F5 authentication server requires python3 and Flask.
Daniel Lenski [Sun, 21 Feb 2021 23:02:44 +0000 (15:02 -0800)]
add auth-fortinet tests
This tests OpenConnect's ability to authenticate with a (fake) Fortinet
server, using all of the options that OpenConnect currently supports
(username+password, username+password+token, non-default realm).
The fake Fortinet authentication server requires Python 3.6 and Flask.
Daniel Lenski [Fri, 19 Feb 2021 04:14:00 +0000 (20:14 -0800)]
Fortinet: parse <split-dns> domains and DNS servers from config
Chimped config containing these settings from
https://github.com/adrienverge/openfortivpn/issues/824#issuecomment-764641406.
This doesn't actually *do* anything with the settings yet.
See https://github.com/dlenski/openconnect/issues/151 and
https://gitlab.com/openconnect/openconnect/-/merge_requests/132 for
discussion about split-DNS.
Daniel Lenski [Thu, 18 Feb 2021 03:27:10 +0000 (19:27 -0800)]
attempt to implement Fortinet challenge-based 2FA (ping #225)
2FA involves reading a bunch of values from the initial response and
parroting them back, along with changing the 'credential' field to 'code'.
This is based on Openfortivpn's implementation
(https://github.com/adrienverge/openfortivpn/blob/master/src/http.c#L711-L754),
and has been tested for basic correctness against a toy HTTPS server that
gives responses with the right format.
Still to do:
- Another one-time password mode: https://github.com/adrienverge/openfortivpn/blob/master/src/http.c#L672-L679, https://github.com/adrienverge/openfortivpn/commit/61dd4fcbbfe1dd89b95081e7d5f5c3e933d7897a
- There's a mobile-app-based "push" mode: https://github.com/adrienverge/openfortivpn/commit/2f16d964560d9b00acbf7bfc7131a0ac40c688f2
Daniel Lenski [Mon, 8 Feb 2021 01:49:10 +0000 (17:49 -0800)]
implement fortinet_obtain_cookie
Fortinet uses HTML-based login forms, similar to Juniper's, resulting in an SVPNCOOKIE.
Believe it or not, they're even more badly designed than Juniper's forms:
1. Inconsistent use of HTTP redirects ('Location' header) vs. Javascript-only redirects
2. Significant <input> fields that aren't actually nested within the <form> tag.
3. Misleading HTTP 405 status ("Method Not Allowed") after incorrect credentials are entered.
4. No apparent way to determine the allowed values for the realm field.
(Though various articles floating around suggest it comes from the URL-path entry point,
from which I assume it's triggered by JavaScript.)
Daniel Lenski [Thu, 4 Feb 2021 19:31:20 +0000 (11:31 -0800)]
Fortinet: server rejects asyncmap and header compression options
This appears to be a "feature" of all Fortinet servers, not just the one I have
access to. Openfortivpn calls pppd with the 'noaccomp nopfcomp default-asyncmap'
options: https://github.com/adrienverge/openfortivpn/blob/ba44ce1/src/tunnel.c#L233-L245
We should avoid offering these options to save an unnecessary round-trip in the
LCP stage of PPP configuration.
Don't blame me. I didn't design this.
For that matter, we don't need to include the asyncmap option with *any*
encapsulation that doesn't use HDLC.
Daniel Lenski [Wed, 13 May 2020 21:29:41 +0000 (14:29 -0700)]
add test-fortinet-login.py
Often easier to prototype HTTPS-based authentication flows in Python, since
they're so fiddly and arbitary. So I copied `test-f5-login.py` to
`test-fortinet-login.py`. Currently only handles basic
username-and-password auth, no 2FA:
positional arguments:
endpoint Fortinet server (or complete URL, e.g.
https://forti.vpn.com/remote/login)
extra Extra field to pass to include in the login query
string (e.g. "foo=bar")
optional arguments:
-h, --help show this help message and exit
-v, --verbose
--no-verify Ignore invalid server certificate
Login credentials:
-u USERNAME, --username USERNAME
Username (will prompt if unspecified)
-p PASSWORD, --password PASSWORD
Password (will prompt if unspecified)
-r REALM, --realm REALM
Realm (empty if unspecified)
-c CERT, --cert CERT PEM file containing client certificate (and optionally
private key)
--key KEY PEM file containing client private key (if not
included in same file as certificate)
```
Daniel Lenski [Mon, 8 Feb 2021 20:43:07 +0000 (12:43 -0800)]
F5: implement f5_obtain_cookie
Like Fortinet, F5's authentication interface is very Javascript-heavy.
Unlike with Fortinet, I didn't even both to try to parse and "follow" HTML forms.
For now, we just create a static form (with username and password fields),
ask the user to fill it out, submit it, and attempt to detect successful login
by a length-0 response including F5_ST and MRHSession cookies.
The F5_ST cookie appears to be a reliable(?) indicator of successful login.
It's a "session timeout" cookie (https://support.f5.com/csp/article/K15387),
which looks like '1z1z1z1612808487z604800'. The 4th field appears to be the
Unix epoch timestamp of session activation, and the 5th appears to be the
duration in seconds until it expires.
Daniel Lenski [Mon, 29 Mar 2021 02:55:18 +0000 (19:55 -0700)]
ppp-over-tls tests: fix PPP-over-IPv6 tests on Ubuntu
For reasons that are unclear, but probably also unimportant, IPv6 is disabled by default on this CI
image (verified in https://gitlab.com/openconnect/openconnect/-/jobs/1135199323#L335), and this will
cause PPP tests using IPv6 to fail.
Explicitly enabling IPv6 with sysctl resolves this.
Daniel Lenski [Fri, 26 Feb 2021 02:39:45 +0000 (18:39 -0800)]
ppp-over-tls tests: give up on CentOS 6
We should be able to --enable-ppp-tests on CentOS 6, but they simply aren't working.
For reasons that are not clear, OpenConnect fails to handshake a TLS
connection to socat 1.7.2 (from CentOS 6 EPEL), and I don't want to install
an 11-year-old distribution on a VM just to figure out why.
Some CI is still failing because pppd can't successfully execute the
/etc/ppp/* scripts after configuring the interfaces. Let's just move these
out of the way so that pppd won't try to execute them.
Side rant: pppd is the most appallingly bad program in terms of separation
of concerns.
Daniel Lenski [Mon, 8 Feb 2021 19:38:19 +0000 (11:38 -0800)]
factor out internal_split_cookies from auth-juniper.c
This is useful for other protocols that use HTTP cookies for authentication, and may
need a way to handoff >1 cookie from the authentication phase to the connection phase.
Improve it slightly by allowing it to set a "default" HTTP cookie if the
authcookie string doesn't contain '=', since most protocols really only NEED
one cookie for the connection phase to work.
This allows shortcuts (like `openconnect --protocol=nc -C 'foobar'` → 'Cookie: DSID=foobar'
or `openconnect --protocol=fortinet -C 'ABCD123456=='` → 'Cookie: SVPNCOOKIE=ABCD123456==') without limiting
the ability to store multiple cookies if/when useful.
Daniel Lenski [Wed, 24 Feb 2021 05:03:47 +0000 (21:03 -0800)]
ppp-over-tls tests: try to keep CentOS 6 CI working, and improve flaky startup of pppd
Even with EPEL, CentOS has an old version of socat which doesn't support the
'rawer' option, so let's use the older 'raw,echo=0' combination to keep it
limping along.
More carefully try to verify that socat and pppd start up and connect to each other:
- Wait for socat to create PTY in 1-second increments, and keep going until PTY
actually exists (up to 15 seconds).
- Wait for ppp to connect to PTY in 1-second increments, and keep going until pppd
creates a "UUCP-style lockfile" for the PTY.
- Log how long it takes for the above process to complete (socat and pppd combined
startup) in the test output.
projects / users / dwmw2 / openconnect.git / log