Daniel Lenski [Thu, 24 Jun 2021 06:19:12 +0000 (23:19 -0700)]
Update documentation for the --authenticate option
Mention the CONNECT_URL and RESOLVE options, and how to use them to invoke
the connection phase in the maximally-robust way (which should work for all
protocols, and all possible proxies).
David Woodhouse [Mon, 28 Jun 2021 13:44:51 +0000 (14:44 +0100)]
Reuse packets
I see malloc/free showing up at ~5% of perf traces, and it's entirely
pointless when we could be reusing packets.
This trick isn't *perfect* and there's potential for a pathological
case where all the packets on the free_queue are too small to be
reused but we never get rid of them anyway — but rounding up to 2KiB
should mean that never happens in practice, and the alignment we get
from that rounding probably doesn't hurt performance anyway.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 24 Jun 2021 15:54:00 +0000 (16:54 +0100)]
Stop polling cmd_fd while busy
We have an explicit select() call on the cmd_fd even when we're busy
shovelling packets and never hit the main select() in the mainloop.
This is *just* to ensure that we react to a cancel command quickly.
In the *common* case that we're running in openconnect(8), there's no
need for that since the *only* thing that will write to the cmd_fd is
openconnect itself, and *that* can set a flag in memory to tell us to
look.
So implement that optimisation — don't check it each time around the
mainloop unless the vpninfo->need_poll_cmd_fd flag is set. That flag
is set whenever we have written to cmd_fd and there's something to be
read. And cleared by poll_cmd_fd() when it runs and finds nothing there.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Fri, 28 May 2021 19:42:57 +0000 (12:42 -0700)]
Assume that a 'portal-*cookie' will allow us to bypass gateway SAML
For many GlobalProtect VPNs with SAML, the 'portal-userauthcookie' appears
to be *the* mechanism by which gateway authentication can be bypassed once
portal authentication is complete.
Unfortunately, there are exceptions which will require a more complex
resolution involved a re-entrant SAML flow
(https://gitlab.com/openconnect/openconnect/-/issues/147#note_587163143),
but this patch will at least not make them worse.
This can work in many cases…
- When the user's password is only usable one time (already working as of 008aefd7),
- When the portal requires SAML but the gateway doesn't (already working in 008aefd7),
- When the gateway requires SAML even though the portal doesn't (fixed here)
Additionally, this patch adds tests (tests/{fake-gp-server.py,gp-auth-and-config}) of
OpenConnect's ability to complete the following SAML flows:
- (SAML to portal after acquiring prelogin-cookie externally) → (complete gateway login
using portal-userauthcookie)
- (SAML to gateway after acquiring prelogin-cookie externally)
Daniel Lenski [Thu, 24 Jun 2021 06:16:16 +0000 (23:16 -0700)]
More complete comment about issues with proxies in connection phase
The comment for openconnect_get_connect_url (added in
https://gitlab.com/openconnect/openconnect/-/commit/ec6c0caed28ebf4f60984a49ce3122196f9c87fa)
should mention the possibility that a proxy requires the correct hostname at
the TLS layer (via Server Name Indication, SNI) as well at the HTTP layer
(via 'Host' header), in order to correctly forward it to the VPN server.
See https://gitlab.gnome.org/GNOME/NetworkManager-openconnect/-/issues/46
for a case where the 'Host' header was apparently required.
Daniel Lenski [Thu, 17 Jun 2021 20:23:18 +0000 (13:23 -0700)]
Fortinet requires us to check for an HTTP error response only over TLS
If the Fortinet PPP connection request *succeeds* over TLS, there is no HTTP
response before we start exchanging PPP packets. If it *fails*, there is an
HTTP response.
If the Fortinet PPP connection request is over DTLS, a 'svrhello' response
is expected regardless of whether it succeeded or failed. This is handled
by fortinet_dtls_catch_svrhello()
Let's only check for that HTTP response in Fortinet if we're definitely
connecting over TLS. The "proceeding to tunnel stage" test in
'fortinet-auth-config-tests' verifies the correctness of the HTTP response
parsing behavior.
Daniel Lenski [Thu, 17 Jun 2021 20:14:32 +0000 (13:14 -0700)]
PPP: Replace no_terminate_on_pause flag with terminate_on_pause flag
We know of two real-world PPP-based VPNs that require us *not* to TERMINATE
at the PPP layer if we will want to subsequently resume a connection. We
don't know of any real-world PPP-based VPNs that *do* want us to TERMINATE
at the PPP layer in this case; in fact, any server that *requires* this
would be unable to resume inadvertently dropped connections.
Replace the no_terminate_on_pause flag with a terminate_on_pause flag, in
order to reduce boilerplate and use the more plausible behavior as the
default.
Daniel Lenski [Thu, 17 Jun 2021 17:25:15 +0000 (10:25 -0700)]
Follow disable_ipv6 for Pulse and Fortinet
As with other protocols (AnyConnect, F5, GP), the behavior of 'disable_ipv6'
for these protocols is relatively "shallow": if set, it will cause
OpenConnect to ignore any IPv6 address or netmask sent by the server, but
will *not* ignore IPv6 split-{in,ex}cludes or IPv6 addresses of DNS servers.
More thorough IPv6-ignoring could be handled by the vpnc-script, or cleaned
up as part of a future change to simplify IP configuration and routing
across protocols.
(The lack of support for --disable-ipv6 in Pulse was noted in
https://gitlab.com/openconnect/openconnect/-/issues/254.)
Daniel Lenski [Thu, 20 May 2021 01:35:55 +0000 (18:35 -0700)]
Add tests of using portal-userauthcookie to continue through gateway
This test sets up fake-gp-server.py to require a one-time password on the
portal *and* on the gateway, and to set 'portal-userauthcookie' after
successful login to the gateway. For OpenConnect to successfully login
within prompting for a second password, the 'portal-userauthcookie' value
from the portal must be forwarded to the gateway login request.
Kevin Yue [Wed, 13 May 2020 06:35:42 +0000 (14:35 +0800)]
Pass the `portal-*cookie` values received in the portal config to the gateway login
These "cookies" appear to be the mechanism by which GlobalProtect clients
can login to the portal and then automatically login to gateway *even if*
the credentials used on the portal are not reusable:
1. Because the credentials used on the portal include a one-time password.
2. Because the credentials used on the portal resulted from SAML login.
(ctx->alt_secret, which leads to a SAML nonce value that can only be
used once).
The logs provided by users (see
https://gitlab.com/openconnect/openconnect/-/issues/147#note_578888250 and
https://gitlab.com/openconnect/openconnect/-/issues/147#note_580406042)
allowed me to answer one of the key unanswered questions (see
https://gitlab.com/openconnect/openconnect/-/merge_requests/109#note_341959833):
> If we do have a `portal_userauthcookie` and/or
> `portal_prelogonuserauthcookie`, should we omit the password from form
> submitted to the gateway? Or do we have to leave it in?
The answer is that it doesn't appear to matter: real servers appear to
ignore the `passwd` field if the `portal-*cookie` field is correctly set.
Signed-off-by: Kevin Yue <yuezk001@gmail.com> Signed-off-by: Daniel Lenski <dlenski@gmail.com>
David Woodhouse [Tue, 15 Jun 2021 13:33:35 +0000 (14:33 +0100)]
Add +SIGN-ALL to GnuTLS DTLS ciphersuite configs
At least for AES256-SHA et al in DTLSv1.2, we needed to explicitly add
+SIGN-RSA-SHA1. Half the ciphersuites already had +SIGN-ALL anyway, so
make them consistent.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 15 Jun 2021 12:02:49 +0000 (13:02 +0100)]
Support non-AEAD ciphersuites in DTLSv1.2 with GnuTLS
We have encountered a Cisco server in the wild which appears only to
support the legacy ciphersuites. And since we offer a set of DTLSv1.2
ciphers it doesn't fall back to accepting the DTLSv1.0 offer; we end
up with no DTLS at all.
This should fix #249.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Sat, 29 May 2021 17:39:57 +0000 (10:39 -0700)]
Mark obsolete-server-crypto test as XFAIL in Fedora/GnuTLS/* CI
The system-wide minimum crypto policy on Fedora prevents us from enabling
3DES and RC4 ciphers via GnuTLS priority strings. We have unconditionally
disabled it in OpenConnect for now in commit 7e862f2f03 but the
obsolete-server-crypto test is *still* failing, with ocserv reporting
'GnuTLS error (at worker-vpn.c:861): No supported cipher suites have
been found.'
Just mark obsolete-server-crypto test as XFAIL for these builds. It's
the most accurate description of the state of those tests: these
environments do not provide OpenConnect with the capabilities to
reliably enable obsolete/insecure crypto algorithms in a self-contained
way.
See https://bugzilla.redhat.com/show_bug.cgi?id=1960763 for ongoing
discussions about how to come up with a more reliable, testable, and
maintainable mechanism for OpenConnect to enable these algorithms without
compromising the system-wide minimum crypto policy.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Sat, 12 Jun 2021 07:50:09 +0000 (08:50 +0100)]
Unconditionally bypass system crypto policy
This makes me extremely sad, but they rolled it out with *no* way to
selectively allow the user to say "connect anyway", as we've always had
for "invalid" certificates, etc.
It's just unworkable and incomplete as currently implemented in the
distributions, so we have no choice except to bypass it and wait for
it to be fixed.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Thu, 10 Jun 2021 22:45:08 +0000 (15:45 -0700)]
Fix Fortinet realm name extraction
1. We were inadvertently capturing 6 characters following the 'realm'
parameter in the query string (e.g. '&lang='). Fix and include extra
parameters in tests to verify.
2. Add another comment about how the 'realm' field is saved in URL-escaped
form, and test to verify.
Daniel Lenski [Mon, 7 Jun 2021 22:46:23 +0000 (15:46 -0700)]
Fix PPP packets split across TLS records
This replicates the behavior of oncp_record_read(): if a (D)TLS record
ends with an incomplete PPP packet, we save the read-so-far length as
vpninfo->partial_rec_size, and continue reading at that point on the
next iteration.
This poorly-layered packetisation behavior was observed on Fortinet and
may exist for other encapsulations as well. So we cope with it for
PPP_ENCAP_F5 because it's trivial, but not yet PPP_ENCAP_F5_HDLC which
would be harder; we'll wait until we actually see it there before
delving into that complexity.
This works as expected to prevent loss of synchronization to the
encapsulation header. Here's an example showing receipt of incomplete
Fortinet/PPP packets, during a high rate of incoming packets generated with
'iperf3 -c $IP_ON_REMOTE_NETWORK -R', followed by recovery on subsequent
mainloop iterations:
Received IPv4 data packet of 1183 bytes over TLS
Fortinet packet is incomplete (837 bytes on wire but header payload_len is 1185). Waiting for more.
Packet contains 539 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Fortinet packet is incomplete (539 bytes on wire but header payload_len is 1185). Waiting for more.
No work to do; sleeping for 10000 ms...
No work to do; sleeping for 10000 ms...
No work to do; sleeping for 10000 ms...
No work to do; sleeping for 10000 ms...
Packet contains 13101 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Packet contains 11910 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Packet contains 10719 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Packet contains 9528 bytes after payload. Assuming concatenated packet.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Tue, 8 Jun 2021 18:13:39 +0000 (11:13 -0700)]
Rename oncp_rec_size → partial_rec_size
This variable is used to track the size of partially-received tunnel packets
across mainloop invocations.
It was intially used by the oNCP protocol, where tunneled packets can be
split across TLS records. We're now going to use it to deal with an
extremely similar mis-layering of packetisation for PPP-based protocols
as well.
Renaming it to partial_rec_size to emphasize its cross-protocol nature (cf.
https://gitlab.com/openconnect/openconnect/-/merge_requests/151).
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Mon, 7 Jun 2021 22:14:57 +0000 (15:14 -0700)]
Fix handling of concatenated PPP data packets
This 'continue' prevented us from ever actually processing a concatenated
packet if the first one was a *data* packet. Dan only tested this for
concatenation of config packets in the early stages of the connection. Bad
Dan.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 8 Jun 2021 19:34:45 +0000 (20:34 +0100)]
Increase SO_SNDBUF on UDP socket
In commit 3444f811ae ("Set SO_SNDBUF on DTLS socket and handle -EAGAIN
on it") in 2013 I cut the socket wmem to just 2 packets in size, to fix
bufferbloat issues when handling VoIP streams simultaneously with bulk
uploads.
In #250 we discovered that this is a problem for high-bandwidth setups
because we aren't actually keeping the send buffers full. Even though
there's only ~100µs of latency between getting -EAGAIN on the UDP
socket, and finally sending the packet successfully...
... that's 100µs when the physical network device actually has nothing to
send. And it's happening over and over again as the UDP socket buffers
fill and then we poll for them to be sufficiently empty again.
This is exacerbated by the fact that ip_info.mtu is actually zero at
this point for PPP protocols — but only slightly because 2*1500 isn't
actually much more than the minimum value we get when we ask for zero
anyway.
It also doesn't help that the kernel deliberately doesn't wake a waiter
until they can make "significant" progress and the buffers are down to
*half* the sndbuf value (see sock_def_write_space() in net/core/sock.c).
Testing with 'iperf3 -u -b 900M' on my home network with 1Gb/s network
between client and (Fortinet) server, I find I have to have to use a
value of around 20000 (doubled to 40000) in order to avoid seeing drops
on the *tun* interface because we aren't moving packets fast enough.
We need to find a decent balance between high-bandwidth and bufferbloat,
so let's try the following: First "assume" 1500 for the MTU if it isn't
actually set, and then multiply by the configured queue len which
defaults to 10. That ought to be a reasonable compromise for bandwidth
vs. bufferbloat for the general case, *and* allows users to tweak it.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Mon, 31 May 2021 22:56:30 +0000 (15:56 -0700)]
Clarify 'Certificate Validation Failure' error from Cisco servers
Cisco servers send this ambiguous error string when the CLIENT certificate
is absent or incorrect. We rewrite it to make this clearer, while
preserving the original error as a substring. See #160.
Daniel Lenski [Fri, 28 May 2021 19:50:51 +0000 (12:50 -0700)]
Print an error message if dtls_addr is NULL in dtls_setup()
This should reduce confusion and help debugging in situations like #242,
where a Cisco server apparently does not return any 'X-DTLS-*' headers upon
connection.
We were printing this message in connect_dtls_socket(), but since
dtls_setup() gets called — and fails — *first* in that case, the message
never got printed.
Daniel Lenski [Thu, 20 May 2021 22:53:25 +0000 (15:53 -0700)]
Bugfix GlobalProtect ESP magic pings over Legacy IP
GlobalProtect IPv6 support was added in
https://gitlab.com/openconnect/openconnect/-/merge_requests/188, and
specifically support for initiating an ESP connection via ICMPv6 "magic
pings" in specifically 5b98b62883216cf9306f06c6b3c9dde81bcfe789.
Getting the ICMPv6 packets to have correct checksums was quite tricky (see
commit notes) and the commit was revised several times.
Somehow we managed to remove the pre-existing code to compute the checksum
correctly in the case of ICMPv4 "magic pings", leaving behind an ICMPv4
checksum that's always zero (and thus rejected by the server, and never
correctly initiates a connection).
Tom Carroll [Mon, 17 May 2021 05:58:24 +0000 (22:58 -0700)]
Use C99 initializer instead of memset.
Strictly speaking, using memset() here violates strict aliasing rules,
and it would be entirely permissible for an assert() like this example
to *fail*:
David Woodhouse [Wed, 19 May 2021 08:32:41 +0000 (09:32 +0100)]
Fix first line length in buf_append_base64()
We need to add four (for the characters we're about to append) *after*
checking against line_len and resetting ll to zero. Otherwise the first
line thinks it starts at 4 while the others start at 0.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Tom Carroll [Tue, 18 May 2021 06:58:23 +0000 (23:58 -0700)]
Correct calculation of base64 encode buffer length.
In the previous formulation, it would first multiple then divide. It would then
promote to unsigned int. The formula would overflow for large len. For example,
needed = 2 when len == INT_MAX. In the revised formulation, it is promoted,
divided, then multiplied. The revised calculation avoids the overflow and
computes needed correctly over len in {0, 1, ..., INT_MAX}. For len == INT_MAX,
needed is correctly computed as 2863311533.
Signed-off-by: Tom Carroll <incentivedesign@gmail.com>
Tom Carroll [Mon, 17 May 2021 05:56:57 +0000 (22:56 -0700)]
gnutls.c:943:21: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare]
The gci->nr_certs field is an unsigned int. Use that as the loop variable
for freeing them too. And we don't actually need the if (gci->certs)
condition here either because ->nr_certs won't be non-zero in that case
anyway.
Signed-off-by: Tom Carroll <incentivedesign@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
projects / users / dwmw2 / openconnect.git / log