See discussion here: https://gitlab.com/openconnect/openconnect/-/merge_requests/207#note_612289607
Restarting the tunnel device if the server spontaneously changes the
client's IP address(es) doesn't currently fit within OpenConnect's security
model, though we may want/need to support it in the future.
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, 25 Jun 2021 20:11:55 +0000 (13:11 -0700)]
Fix disconnect-instead-of-hello_reply
handle_hello_reply() wouldn't actually get called unless the command packet
started with "(hello_reply". Move handling of "(disconnect" to
snx_handle_command() to fix this.
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 [Mon, 21 Jun 2021 22:28:11 +0000 (15:28 -0700)]
Leave original urlpath as-is if provided, but set to "/clients/" if not
Many VPN protocols (e.g. AnyConnect, Fortinet, oNCP, Pulse) use the
URL-path to select alternative realms/domains/groups for authentication.
Leave this possibility open for CP.
Daniel Lenski [Mon, 21 Jun 2021 21:00:53 +0000 (14:00 -0700)]
Don't hide_auth_data() if built with INSECURE_DEBUGGING
Run './configure --enable-insecure-debugging' to #define this flag. This will allow
us to see unfiltered data using the usual '--dump-http-traffic' mechanism.
Daniel Lenski [Mon, 21 Jun 2021 20:45:03 +0000 (13:45 -0700)]
Remove last vestiges of {find,get,add,set}_option
The presence or absence of the 'protocol_version' option was being used to
check whether we had already run do_ccc_client_hello(). As far as I can
tell, it should be good enough to simply call it in cp_obtain_cookie.
Daniel Lenski [Mon, 21 Jun 2021 19:46:57 +0000 (12:46 -0700)]
Save slim_cookie:session_id as vpninfo->cookie (rather than constantly rebuilding it to/from cstp_options
In addition to eliminating unnecessary use of cstp_options for saving state,
this makes the code much simpler *and* it will be required for 'openconnect
--authenticate' to work correctly (necessary for front-ends like NM-oc as
well).
I also replaced cp_bye with a more OpenConnect-idiomatic version based on
{oncp,gpst,f5,fortinet}_bye:
1. Save and restore 'vpninfo->urlpath' directly (this pattern is kinda ugly,
but it's the established one within the codebase).
2. Extract session_id directly from vpninfo->cookie; this is similar to the
GP case, where we have values that we need save in the cookie because
they're required for logout to succeed… and *only* for logout.
Daniel Lenski [Mon, 21 Jun 2021 19:14:36 +0000 (12:14 -0700)]
Simply log, rather than save, server options whose values aren't clearly needed later
Working on eliminating use of cstp_options to store a lot of state that doesn't really need to be saved.
NB: The standard value for 'connect_with_certificate_url' (based on 3 servers that I've tested)
is actually "/clients/cert/", with a trailing slash, not "/clients/cert".
Daniel Lenski [Mon, 21 Jun 2021 19:11:15 +0000 (12:11 -0700)]
Replace decode/encode with unscramble/buf_append_scrambled
The buf_append_XXX() approach is more idiomatic within the OpenConnect codebase, and these versions are a bit more efficient (fewer copies and no need for in-place reversing).
Daniel Lenski [Mon, 21 Jun 2021 00:43:33 +0000 (17:43 -0700)]
Replace stack-allocated auth form with dynamically allocated auth form
See https://gitlab.com/openconnect/openconnect/-/merge_requests/207#note_606126537
Also, standardize names for 'cstp_options', and don't merge options (see
https://gitlab.com/openconnect/openconnect/-/merge_requests/207#note_606123313)
Rudimentarily tested by pointing at a known Internet-facing CheckPoint VPN server:
$ ./openconnect --protocol=cp vpn.simmons.edu
Enter user credentials:
Username (or Challenge Response):foo
Password, passcode, or PIN+tokencode (leave blank for Challenge Response):
POST https://vpn.simmons.edu/clients/
SSL negotiation with vpn.simmons.edu
Connected to HTTPS on vpn.simmons.edu with ciphersuite (TLS1.2)-(ECDHE-RSA-SECP256R1)-(AES-256-GCM)
> POST /clients/ HTTP/1.1
> Host: vpn.simmons.edu
> User-Agent: Open AnyConnect VPN Agent v8.10-526-g67da55d3-dirty
> X-Pad: 000000000000000000000000000000000000000
> Content-Type: application/x-www-form-urlencoded
> Content-Length: 217
>
> (CCCclientRequest
> :RequestHeader (
> :id (2)
> :type (UserPass)
> :session_id ()
> )
> :RequestData (
> :client_type (TRAC)
> :username (2b204b)
> :password (362e4f)
> )
> )
Got HTTP response: HTTP/1.0 200 OK
Date: Mon, 21 Jun 2021 00:57:03 GMT
Server: Check Point SVN foundation
Content-Type: text/html
X-UA-Compatible: IE=EmulateIE7
Connection: close
X-Frame-Options: SAMEORIGIN
Strict-Transport-Security: max-age=31536000; includeSubDomains
Content-Length: 374
HTTP body length: (374)
< (CCCserverResponse
< :ResponseHeader (
< :id (2)
< :type (UserPass)
< :session_id ()
< :return_code (600)
< )
< :ResponseData (
< :authn_status (done)
< :is_authenticated (false)
< :error_message (01405e5044575a3535653a5c652d594a4168262c44216522265c2b237268702b3759394320653a352c272c6c)
< :error_id (0d721d0bff0c6c0a071706130e0d63026fff041b190a171f6e)
< :error_code (101)
< )
< )
Received error during authentication: Access denied - wrong user name or password (code 101)
Failed to complete authentication
Daniel Lenski [Mon, 21 Jun 2021 00:00:45 +0000 (17:00 -0700)]
Replace ipv4tostr with inet_ntop
ipv4tostr() was a reimplementation of inet_ntoa(), with the same problem (it
used a static string buffer). We replaced all use of inet_ntoa() with
inet_ntop() in 71bf1d939f55ff206e9f149bcc1bf2f4be2d7450
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>
projects / users / dwmw2 / openconnect.git / log