Daniel Lenski [Thu, 16 Sep 2021 19:51:43 +0000 (12:51 -0700)]
Provide the vpnc-script with our PID (as $VPNPID)
This will enable a vpnc-script to more easily identify which VPN connection
is calling it, in the case of multiple concurrent or "stacked" VPN
connections.
Because OpenConnect (and vpnc) invoke the vpnc-script via an intermediate
shell process, the vpnc-script would otherwise need to determine its
GRANDparent PID, which is an error-prone, and not easily portable, process.
See https://gitlab.com/openconnect/vpnc-scripts/-/issues/28 and
https://gitlab.com/openconnect/vpnc-scripts/-/merge_requests/36 for issues
with the current approaches.
Daniel Lenski [Wed, 11 Aug 2021 17:12:40 +0000 (17:12 +0000)]
Only remove ERR_GET_FUNC for OpenSSL v3.0 and newer
This function is removed in OpenSSL 3.0 beta 2, per
https://github.com/openssl/openssl/blob/openssl-3.0.0-beta2/CHANGES.md:
> The ERR_GET_FUNC() function was removed. With the loss of
> meaningful function codes, this function can only cause
> problems for calling applications.
It appears that this function may not have had any useful purpose for a long
time (see
https://gitlab.com/openconnect/openconnect/-/merge_requests/262#note_648720006),
but in the absence of clear documentation or testing, we should limit its
remove to OpenSSL 3.0+ to be on the safe side.
Dimitri Papadopoulos [Sun, 1 Aug 2021 21:04:42 +0000 (23:04 +0200)]
Build with OpenSSL 3.0 beta 2 Release Candidate
From the OpenSSL 3.0 Migration guide:
The function code part of an OpenSSL error code is no longer relevant
This code is now always set to zero. Related functions are deprecated.
In our case, removing calls to ERR_GET_FUNC() will not change anything:
The reason code PKCS12_R_MAC_VERIFY_FAILURE is raised in two OpenSSL functions:
* PKCS12_newpass() in p12_npas.c,
* PKCS12_parse() in p12_kiss.c.
In out code, we check the reason code is PKCS12_R_MAC_VERIFY_FAILURE after
calling PKCS12_parse(), so the incriminated function is necessarily
PKCS12_parse(). Verifying the function code is PKCS12_F_PKCS12_PARSE is
redundant.
EVP_F_EVP_DECRYPTFINAL_EX / EVP_R_BAD_DECRYPT
The reason code EVP_R_BAD_DECRYPT is raised in a single OpenSSL function:
* EVP_DecryptFinal_ex() in evp_enc.c
Therefore verifying the function code is EVP_F_EVP_DECRYPTFINAL_EX is
useless, EVP_F_EVP_DECRYPTFINAL_EX is the only possible value.
Mark auth-swtpm test as XFAIL on Fedora/OpenSSL and Fedora/OpenSSL/clang
Apparently, verifying that either 'tsstartup' or 'tpm2_startup' is available
is *not* sufficient to make auth-swtpm tests work again. See error log at
https://gitlab.com/openconnect/openconnect/-/issues/287#note_641338923
[Originally by DP. DL added Fedora/OpenSSL/clang as well]
Signed-off-by: Dimitri Papadopoulos <3350651-DimitriPapadopoulos@users.noreply.gitlab.com> Signed-off-by: Daniel Lenski <dlenski@gmail.com>
Daniel Lenski [Mon, 2 Aug 2021 20:41:29 +0000 (13:41 -0700)]
Verify that TPMv2 startup tools are present in order to enable auth-swtpm tests
Autoconf source now verifies that either 'tpm2_startup' or 'tsstartup' is
found before enabling this test.
See discussion at https://gitlab.com/openconnect/openconnect/-/issues/287#note_640185660.
I also added tpm2-tools (package providing 'tpm2_startup') to the Fedora
build image, in https://gitlab.com/openconnect/build-images/-/commit/35ee4ffb88ba319014c321dc8999e48fce81f130.
Daniel Lenski [Mon, 2 Aug 2021 16:58:04 +0000 (09:58 -0700)]
Mark sync/no-HDLC PPP tests as XFAIL for all CI images
See https://gitlab.com/openconnect/openconnect/-/issues/287#note_641198529 for discussions.
Across all CI images, non-HDLC PPP tests are consistently failing (this is
described as "synchronous" framing in the '90s-era terminology of pppd, and
is supported by 'pppd sync').
FAIL: ppp-over-tls
==================
Testing PPP ...
[...]
Starting PPP peer (sync/no-HDLC, IPv4+IPv6, DNS, extraneous VJ and CCP)... started in 0 seconds
2021/07/31 20:54:18 socat[10622] E waitpid(): child 10625 exited with status 1
Connecting to it with openconnect --protocol=nullppp... failed (after 0 seconds)
[...]
===== START pppd log =====
Couldn't set tty to PPP discipline: Invalid argument
The 'pppd sync' support has always appeared to be a fairly marginal part of
pppd capabilities, brittle and not well-tested, and I've run into other
problems with it before (see eaabbb09 for example).
This is frustrating because non-HDLC/pre-framed PPP is the version that is
(and should be!) used in all modern implementations of PPP, including F5 and
Fortinet's implementations.
This patch splits the sync/no-HDLC PPP test into a separate script
(ppp-over-tls-sync), and marks it as XFAIL for all CI runs, so that we can
continue to test it by default when running locally, and to fail on the
other PPP tests (which use async mode aka “HDLC-like” framing).
Daniel Lenski [Sat, 31 Jul 2021 14:42:12 +0000 (07:42 -0700)]
Use sysctl to un-disable IPv6 for all CI runs where PPP tests are enabled
See https://gitlab.com/openconnect/openconnect/-/issues/287#note_640115686,
and https://gitlab.com/openconnect/vpnc-scripts/-/issues/12#note_547951023
for where this issue was originally discovered (specifically on the Ubuntu
18.04 CI runs).
David Woodhouse [Wed, 28 Jul 2021 15:52:26 +0000 (16:52 +0100)]
Make all cert rules order-only
For some reason, perhaps a make update or perhaps just higher precision
timestamps causing some files to actually appear as older than others,
the CI has taken to rebuilding all the certs. Don't do that.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
The comments on TPM2TSS_GENKEY and CREATE_TPM2_KEY say the former can
only create keys, while the latter can import them too, but we used them
the other way around. This causes the auth-hwtpm test to fail on
machines just with tpm2-tss-engine installed.
While I haven't found official documentation for the TAP_IOCTL_GET_VERSION
control code, clearly the DeviceIoControl() parameters were incorrect,
see other online examples:
https://github.com/juhovh/tapcfg/blob/3d5ef74/src/lib/tapcfg_windows.c#L140-L146
https://github.com/OpenVPN/openvpn/blob/34b4254/src/openvpn/tun.c#L6030-L6032
David Woodhouse [Tue, 29 Jun 2021 12:20:34 +0000 (13:20 +0100)]
vhost: Avoid TX queue when writing directly is faster
Using vhost makes high-volume transfers go nice and fast, especially
we are using 100% of a CPU in the single-threaded OpenConnect process
and just offloading the kernel←→user copies for the tun packets to
the vhost thread instead of having to do them from our single thread
too.
However, for a lightly used link with *occasional* packets, which is
fairly much the definition of a VPN being used for VoIP, it adds a lot
of unwanted latency. If our userspace thread is otherwise going to be
*idle*, and fall back into select() to wait for something else to do,
then we might as well just write the packet *directly* to the tun
device.
So... when the queue is stopped and would need to be kicked, and if
there are only a *few* (heuristic: half max_qlen) packets on the
queue to be sent, just send them directly.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 16 Jun 2021 23:05:14 +0000 (00:05 +0100)]
Initial vhost-net support
We spend a lot of CPU time copying packets between kernel and userspace.
Eventually we want to implement a completely in-kernel data path. It
isn't even that hard, now that most of the functionality we need from
the kernel is there and it's mostly just a case of fitting it together.
In the meantime, though, there are a few things we can do even on today's
released kernels. For a start, we can use vhost-net to avoid having to
do the read()/write() on the tun device in our mainloop.
Ultimately, it ends up being done by a kernel thread instead; it doesn't
really go away. But that should at least give us a performance win which
would compare with a decent threading model, while allowing OpenConnect
to remain naïvely single-threaded and lock-free.
We have to carefully pick a configuration for vhost-net which actually
works, since it's fairly hosed for IFF_TUN support:
https://lore.kernel.org/netdev/2433592d2b26deec33336dd3e83acfd273b0cf30.camel@infradead.org/T/
But by limiting the sndbuf (which disables XDP, sadly) and by requesting
a virtio header that we don't actually want, we *can* make it work even
with today's production kernels.
Thanks to Eugenio Pérez Martín >eperezma@redhat.com> for his blog at
https://www.redhat.com/en/blog/virtqueues-and-virtio-ring-how-data-travels
and for lots more help and guidance as I floundered around trying to make
this work.
Although this gives a 10% improvement on the bandwidth we can manage in
my testing (up to 2.75Gb/s with other tricks, on a c5.8xlarge Skylake VM)
it also introduces a small amount of extra latency, so disable it by
default unless the user has bumped the queue length to 16 or more, which
presumably means they choose bandwidth over latency.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 1 Jul 2021 16:03:13 +0000 (17:03 +0100)]
Clear epoll_fd after forking to background self
Otherwise we remove the events from the epoll_fd before we exit in
the parent process.
This would be a bit awful if it were something we require all users of
libopenconnect to know about, but it isn't. We make everything O_CLOEXEC
and we don't expect users to be calling openconnect_vpninfo_free() from
another process after forking, like the background code does. We only
do it there so that we can check for memory leaks (I think).
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 1 Jul 2021 15:30:27 +0000 (16:30 +0100)]
Fix epoll support for connection pause/restart
We need to actually remove the file descriptors from the epoll set.
Otherwise we get -EEXIST when adding them again (in the case of the
cmd_fd as we re-enter the main loop).
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Dimitri Papadopoulos [Tue, 29 Jun 2021 09:50:16 +0000 (11:50 +0200)]
Reorganize #include
- Reorder header files as suggested here:
https://stackoverflow.com/questions/2762568/c-c-include-header-file-order
https://softwareengineering.stackexchange.com/questions/325549/c-header-file-order
- Remove duplicates
- Remove unused headers files
- Change "config.h" to <config.h>
- Include <winsock2.h> before openconnect.h, which is not entirely self-contained.
Daniel Lenski [Fri, 25 Jun 2021 16:42:27 +0000 (09:42 -0700)]
With --user, enter username in all forms, not just the first
Until now, the -u/--user=USERNAME option has caused the OpenConnect CLI to
automatically fill the username only in the *first* form where there is a
match field. This patch causes it to fill the username repeatedly
(including when a form is repeated due to an authentication failure).
As described by @DimitriPapadopoulos in #267:
> In many cases, I expect the authentication failure to be caused by an
> incorrect password, not an incorrect username
>
> Having to [re]enter the username, when it has already been specified from
> the command line or worse in a config file, is an annoyance. I suggest
> openconnect [re]prompts for the username only when it has been entered
> interactively from the start.
projects / users / dwmw2 / openconnect.git / log