David Woodhouse [Fri, 22 Apr 2022 16:02:08 +0000 (17:02 +0100)]
Make all STRAP support conditional on HPKE
We really don't care about STRAP; we only did it for the external browser
support. The only case we've seen STRAP failing is when we *did* advertise
it and then didn't really do it, so omitting it completely seems harmless
for now.
And older GnuTLS such as on CentOS doesn't have some of the functions we
are using to obtain the Finished message or export the privkey, so just
disable it all. We *could* support the basic STRAP from GnuTLS 3.4.0 on
but there's not a lot of point.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 22 Apr 2022 15:14:17 +0000 (16:14 +0100)]
Silence static-analyser warning about redundant assignment to 'sep'
I did this for a reason. The *compiler* is clever enough not to bother
actually doing the assignment (not that it would matter anyway, since it
is hardly a fast path). But *developers*, including myself, are much less
likely to spot that it needs to be added in the 'deflate' case if we add
a new case at the end. So now in order to shut the tools up, I have to
turn a non-bug into a latent *actual* bug.
I suppose I could leave it there with a comment, or refactor it into a
loop over tuples of the form { COMPR_LZ4, "oc-lz4" }… but it probably
doesn't matter as we're unlikely to be adding more. Just suck it up.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
The assignment *was* necessary. The point was that the first time 'sep'
was used, it's a space. And *after* that, it's a comma. Using a comma
every time ends up sending headers which look like this:
X-DTLS-Accept-Encoding:,lzs
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 22 Apr 2022 14:39:14 +0000 (15:39 +0100)]
Export STRAP private key with AnyConnect cookie
For STRAP we need to reconnect using the same private key as the auth
process did. Thankfully we already have precedent for this; we can put
multiple 'cookies' into the opaque string that is passed from auth to
connection process, and use internal_split_cookies() to parse them.
So encode the privkey into an 'openconnect_strapkey' cookie which we
handle specially on ingestion.
Fix up a few places where vpninfo->cookie was handled directly, to make
it all work.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 21 Apr 2022 21:14:00 +0000 (22:14 +0100)]
Attempt to implement AnyConnect Session Token Re-use Anchor Protocol (STRAP)
In order to implement the external-browser SAML support we had to send
the X-AnyConnect-STRAP-Pubkey: header, even though we didn't really know
what that was.
That turns out to cause a regression for some user (issue #410), as the
server then rejects us we don't include a valid X-AnyConnect-STRAP-Verify:
header in our CONNECT request.
That header is supposed to contain our Finished message from the TLS
handshake, hashed and signed with our STRAP-Pubkey. Or if we rekey, it's
a signed hash of the Finished message concatenated with the (DER) public
key that we also send in a new X-AnyConnect-STRAP-Pubkey: header.
Lightly tested, as we can't actually work out how to make *our* servers
reject the connections for this offence, and Cisco's documentation is
very sparse. But it shouldn't make things worse for anyone.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Fri, 11 Feb 2022 18:13:46 +0000 (10:13 -0800)]
GP: add 'internal=no' flag to the login and configuration requests
Without these flags, one user reports consistently hitting the "Matching
client config not found" error in response to the /ssl-vpn/getconfig.esp
request. See https://gitlab.com/openconnect/openconnect/-/issues/246
I *suspect* that 'internal=no' is the implicit default if unspecified
(https://gitlab.com/openconnect/openconnect/-/issues/246#note_836128670),
but we should get more testing on other GlobalProtect VPNs to confirm that
this works fine with them. If there is variation in whether this parameter
is allowed/expected, then we need a way to automatically detect the correct
value.
Daniel Lenski [Wed, 20 Apr 2022 03:43:27 +0000 (20:43 -0700)]
Fix initial client request XML structure when announcing multicert capability
Having a separate 'announce_multicert_capability' function wasn't ideal:
1. Building the initial '<config-auth>' XML was more spread out and complex
than necessary due to an extra layer of functions.
2. When multicert auth is offered by the client, the resultant XML actually
contained *two* nested capabilities/auth-method tags:
What Cisco clients appear to send is a *single* '<capabilities>' tag
containing multiple '<auth-method>', as shown in the MITM capture in the
comments of f51ecb36bedcd370086586295978627daeabade4 ("Converse the multiple certificate authentication (multicert) protocol."),
which introduced this structural mistake.
With the non-repeated '<capabilities>' tag, we can also fix the multicert
auth tests, where the check that the client was offering multicert was
disabled in 45da3b07dfc8f808e7b0d0bf80fbf7e73b1b0721 ("fake-cisco-server.py: Disable check for `multiple-cert` support"),
probably due to confusion over the XML structure and xmltodict's handling
of it.
DWORD is unsigned, so PRIu32 would be the proper format specifier for
DWORD, not PRId32. Except DWORD is defined as 'unsigned long int', while
PRIu32 is defined as u, and "%u" is the format specifier for 'unsigned int'.
What a mess! It looks like the only viable format specifier is "%lu".
obs workflow: add xdg-utils build-dep on openconnect.dsc too
Due to <reasons> we need to duplicate the list of build deps for Debian builds.
https://gitlab.com/openconnect/openconnect/-/commit/9b5a9e004af579bd3b24e115061fb99c94504bce
added this package to .obs/debian/control, also add it to .obs/openconnect.dsc
David Woodhouse [Tue, 12 Apr 2022 09:56:26 +0000 (11:56 +0200)]
fake-cisco-server.py: Disable check for `multiple-cert` support
Now we support SSO, there are *multiple* auth-method nodes, not just one.
We should check that *one* of them contains 'multiple-cert'. Comment it
out for now.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Sun, 10 Apr 2022 21:11:39 +0000 (22:11 +0100)]
Add openconnect_set_external_browser_callback() and defaults
There is a little bit of duplication here since the *library* will spawn
the default (xdg-open) and the application (main.c) has basically the same
code, but that's because we don't want to *require* that the app register
the callback. With suitable default behaviour from the library, it will
work even with existing versions of the NM auth-dialog or other GUI tools.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 7 Apr 2022 11:30:21 +0000 (12:30 +0100)]
AnyConnect: Add support for external browser SSO
For external browser SSO we need to listen on a local port to accept the
encoded token from the browser, as it's passed to us via a redirect to
http://localhost:29786/api/sso/<base64>
This implements a simple listening loop, accepting connections and decoding
the blob we get back.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Mike Gilbert [Sun, 10 Apr 2022 16:21:57 +0000 (12:21 -0400)]
openssl: allow ALL ciphers when allow-insecure-crypto is enabled
Previously, the cipher list was set to "DEFAULT:+3DES:+RC4". However,
according to ciphers(1), the DEFAULT keyword cannot be combined with
other strings using the + characters. In other words, ":+3DES:+RC4" gets
ignored.
The user is opting into insecure behavior, so let's keep it simple and
just allow everything.
This change fixes the obsolete-server-crypto test when openconnect is
built against openssl-1.1.x.
It's just a quirk that free takes non-const: it doesn't actually modify
the value pointed to, either conceptually or in practice, it merely looks
up the memory block using the pointer and deallocates it.
Unfortunately, the compiler will emit warning when attempting to free()
a "const char *". So either switch to a pointer to non-const or cast to
(void *) when calling free() on a pointer to const.
David Woodhouse [Wed, 6 Apr 2022 15:42:09 +0000 (16:42 +0100)]
epoll: Handle EPOLLERR as 'readable'
When we use select(), an error event on a file descriptor causes it to
be reported as readable, and some of the handlers will only clean it up
if that happens. So set the 'readable' flag on EPOLLERR too.
This is part of #389.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
However, we forget to make stoken.c set the `auth_id` for the forms that it
creates for RSA SecurID token decryption and PIN entry. Let's name these:
- `_rsa_unlock`, for token decryption.
- `_rsa_pin`, for PIN entry. Also, rename the numeric PIN field to `pin`
rather than `password`; there can't be any existing users relying on
`--form-entry` to set its value, because that wouldn't work without the
`auth_id`.
Luca Boccassi [Wed, 2 Mar 2022 16:35:29 +0000 (16:35 +0000)]
Add --enable-docs option
Allow to explicitly disable building documentation. Some
platforms are too old (RHEL 7) and even if Python is available due
to build macros, building documentation fails. Allow to disable it
explicitly.
David Woodhouse [Sat, 26 Feb 2022 17:34:26 +0000 (17:34 +0000)]
Fix Windows tun setup crash
The intention was to use vpninfo->hostname, by passing it as a parameter
to the create_ifname_w() function. But that function still just uses
vpninfo->ifname unconditionally, ignoring the argument. And causing the
crash in #370.
Fix up some bogus formatting while we're at it. We don't do open braces
on the same line as the function declaration.
Fixes: 48bd28aadd ("Use hostname as Wintun ifname (if ifname not specified)") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Dimitri Papadopoulos [Sat, 20 Nov 2021 08:34:14 +0000 (09:34 +0100)]
Avoid assert statement outside of tests
This fixes DeepSource alerts:
Assert statement used outside of tests
Usage of assert statement in application logic is discouraged. assert is
removed with compiling to optimized byte code. Consider raising an
exception instead. Ideally, assert statement should be used only in tests.
Dimitri Papadopoulos [Sat, 20 Nov 2021 08:18:24 +0000 (09:18 +0100)]
Local variable redefined argument
This fixes a DeepSource alert:
Argument redefined from local
A local name is redefining an argument, which might suggest a potential
error. This is taken into account only for a handful of name binding
operations, such as for iterations, with statements, and exception
handler assignments.
projects / users / dwmw2 / openconnect.git / log