David Woodhouse [Mon, 8 Oct 2018 20:13:30 +0000 (21:13 +0100)]
Add openconnect_set_key_password()
For auto-provisioning via NetworkManager it's actually quite useful to be
able to set vpninfo->cert_password and have that special case handled,
instead of having to inject the password into the user's keyring somehow.
It's either that or revise the FSID hack...
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Mon, 8 Oct 2018 18:03:45 +0000 (11:03 -0700)]
Fix GlobalProtect authgroup handling
When connecting to a GlobalProtect server via the portal interface, then
`vpninfo->authgroup` needs to be set to the URL of one of the allowed
gateways.
The problem here is that if the user actually wanted to select the _first_
gateway in the dropdown list, it was already pre-selected, and thus clicking
"continue"/"login" on the form wouldn't trigger `OC_FORM_RESULT_NEWGROUP`.
This would prevent `vpninfo->authgroup` from getting set correctly, and the
gateway redirect would be skipped entirely. Thus it was effectively
impossible to select the first option in the gateway dropdown.
Daniel Lenski [Mon, 8 Oct 2018 17:39:54 +0000 (10:39 -0700)]
Fix issue causing front-ends/GUIs to be insensitive to changes in the Juniper realm dropdown
This has been a persistent, puzzling issue
(http://lists.infradead.org/pipermail/openconnect-devel/2018-July/004926.html,
http://lists.infradead.org/pipermail/openconnect-devel/2017-November/004558.html,
etc.). When connecting to a Juniper VPN from a front-end (e.g.
NM-OpenConnect, OpenConnect-GUI for Windows, OpenConnect for Android),
changing the selected realm/`authgroup` in the drop-down box causes the form
to immediately reload *without* saving the change.
This turned out to be a rather subtle issue…
The meaning and usage of `vpninfo->authgroup` differs across the different
protocols, which made this hard to isolate.
* With AnyConnect, changing the authgroup value in the form is supposed to
trigger an immediate reload of the form, since the form contents can
differ from one authgroup to another. Hence a `process_auth_form`
callback should immediately return `OC_FORM_RESULT_NEWGROUP` when the form
value changes.
* With Juniper, the authgroup dropdown don't *actually* need to trigger a reloading
of the form, since the form won't change if the authgroup field changes.
(At least not on any Juniper VPN I have access to.) However, it doesn't
hurt anything either, and setting the dropdown as `form->authgroup_opt`
allows CLI users to specify the desired setting with `--authgroup`, which
is very convenient.
* With GlobalProtect, the authgroup has been repurposed to represent the desired
*gateway* to connect to, in the cases where the user is connecting via the
*portal* interface. The authgroup selection always appears in a form by
itself, currently. This similarly allows CLI users to pick the desired
gateway with `--authgroup`.
Long story short, the problem here was that `form->authgroup_selection`
needed to be set to a specific index (within `form->authgroup_opt->choices[]`)
of the currently selected value, in order
for the GUI to show the right value as selected. If this wasn't set, then
every time the selection was changed (causing the form handler to return
`OC_FORM_RESULT_NEWGROUP`), the selected index would revert to `0` on the
next iteration of the form.
For AnyConnect, the `form->authgroup_selection` was already set correctly;
for Juniper and GlobalProtect, it wasn't. It seems to me that the most
robust fix here is to ensure that `process_auth_form` itself always sets
`form->authgroup_selection` to the index of the value matching
`vpninfo->authgroup` _before_ handing the form off `process_auth_form_cb`.
Tested that this change makes Juniper realm dropdowns work correctly in the
NM-OpenConnect and Android front-ends.
David Woodhouse [Mon, 1 Oct 2018 11:01:00 +0000 (12:01 +0100)]
Shift PC/SC context out of generic vpninfo
TPM2 support wants its own BOOL definition which conflicts with the
"standard" Windows one from libpcsc. Let's just isolate things so that
we only need to include PC/SC header files from within yubikey.c.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Sat, 8 Sep 2018 05:22:40 +0000 (22:22 -0700)]
GlobalProtect: query and parse prelogin.esp and use it to build auth forms, including preliminary SAML support
Until recently, I've believed the prelogin.esp to be useless, because the
initial GlobalProtect login form always contains the same two fields:
username and password.
However, the prelogin response is also important for signalling when SAML
login is required. When the VPN uses SAML login, the official GP clients
redirect the user to a web-based authentication flow (e.g. Okta,
https://github.com/dlenski/openconnect/issues/116).
That auth flow eventually sends the official client back to the GP VPN,
armed with a special cookie field, `portal-userauthcookie` or
`prelogin-cookie`, that needs to be submitted in place of the password
(already supported by openconnect as of 8b2bc5f22dda).
This preliminary SAML support simply includes the SAML method and URL in the
form banner, and fails with an error message if the cookie field name was
not specified (since it cannot be autodetected).
Daniel Lenski [Sat, 8 Sep 2018 05:01:58 +0000 (22:01 -0700)]
simplify gpst_xml_or_error handling and config parsing
* replace GPST-specific xmlnode_get_text() with global xmlnode_get_val(), and make it free existing values (simplifies config loading)
* don't deal with numeric error codes in gpst_xml_or_error, only parsing
* make gpst_xml_or_error take 2 calbacks: one for XML, one for challenge (whether in XML or JavaScript form)
* allow callbacks to take cb_data
* use all this to simplify form handling
David Woodhouse [Thu, 27 Sep 2018 11:25:17 +0000 (13:25 +0200)]
Use OpenSSL TPM2 engine
We should look at whether we want to do this natively (probably not; we
should rely on the OpenSSL STORE mechanism instead), and we should
definitely look at how we're going to do it for GnuTLS. But this is a
start...
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Wed, 5 Sep 2018 19:44:25 +0000 (12:44 -0700)]
command-line client should fill in any password field with value supplied via --passwd-on-stdin
I previously proposed adding form field hints to suggest which fields should
be populated with username/password values. David Woodhouse was hesitant to
accept this and we settled on matching the form field names by the first
four characters ("user", "pass") as a temporary compromise:
There's at least one specific case where this interferes with the
usage of the command-line client: some GlobalProtect users need to
specify an "alternative secret field" instead of the default "passwd"
field (using `--usergroup :field_name`).
Because this field's name normally doesn't start with "pass", openconnect
won't accept it via `--passwd-on-stdin`:
script_to_do_fancy_GlobalProtect_SAML_login |
openconnect --protocol=gp -u user --passwd-on-stdin --usergroup portal:portal_cookie_field_name globalprotect.company.com
As far as I can tell, there's not actually any good reason why openconnect
should *only* fill in a password-type field with the supplied password
if its name starts with "pass", so we should get rid of that check.
Daniel Lenski [Wed, 5 Sep 2018 04:58:08 +0000 (21:58 -0700)]
GlobalProtect: always set clientos=Windows in relevant requests, regardless of actual OS
I've tried, whenever possible not to lie to the server about the client
configuration. However, there are now multiple cases where a GlobalProtect
server responds with a misleading error when the `ssl-vpn/login.esp` request doesn't
contain the exact, magic value of `clientos=Windows`.
* https://github.com/dlenski/openconnect/issues/86
("Assign private IP address failed" unless `clientos=Windows` in `ssl-vpn/login.esp` request)
* https://github.com/dlenski/openconnect/issues/116
("Invalid username or password" unless `clientos=Windows` in `ssl-vpn/login.esp` request)
These cases are very difficult to debug because they seem to be suggesting totally unrelated
errors.
For reliability, this patch makes openconnect always specify `clientos=Windows` in relevant
requests, regardless of the actual `vpninfo->platname`. (The `vpninfo->platname` value can
still be sent in the "free-form" `os-version` field, as far as I can tell.)
Kyle Johnson [Sat, 1 Sep 2018 20:20:52 +0000 (15:20 -0500)]
Toggle TAP status to force Windows to re-run NLA.
Windows runs Network Location Awareness detection during the connection process
and sometimes fails because the connection script hasn't had an opportunity to
set up the connection enough for detection to work properly. Toggling the media
status forces re-detection which then occurs successfully.
Signed-off-by: Kyle Johnson <KyleJ61782@gmail.com>
David Woodhouse [Thu, 9 Aug 2018 10:27:08 +0000 (11:27 +0100)]
Don't treat Juniper 'realm' field as authgroup
The only point in the authgroup thing for the UI is to allow OpenConnect
to re-request the set of query fields for the new group. But the Juniper
protocol doesn't do that, and it's causing problems.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 9 Aug 2018 09:56:21 +0000 (10:56 +0100)]
Provide unique hostname to CSD script
We see failures when DNS gives us a different IP address and the CSD
trojan ends up talking to a different host to the one we're actually
trying to authenticate to. Since we use pinned certificate hashes, it
shouldn't matter that we lose the hostname and can't validate the cert
against it.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Sun, 5 Aug 2018 23:05:06 +0000 (16:05 -0700)]
fix segfault in Java library test application
calling the Java method setCSDWrapper(String wrapper, String TMPDIR, String
PATH) with null TMPDIR and PATH would cause a segfault because
openconnect_set_csd_wrapper(vpninfo, name, value) requires a non-NULL value
Daniel Lenski [Sat, 4 Aug 2018 03:01:26 +0000 (20:01 -0700)]
include computer name in the GP cookie
The GlobalProtect "cookie" is an overstuffed monstrosity, due to the
requirement to retain a few random, non-secret values in order to logout
successfully (see gpst_bye):
Until now, I've avoided including the computer field in this cookie, on the assumption that it
can reproduced at any time using vpninfo->localname. However, it appears that this value can't always
be reproduced correctly when running under NetworkManager:
Daniel Lenski [Thu, 2 Aug 2018 19:10:45 +0000 (12:10 -0700)]
Remove first oNCP negotiation request (only second is necessary)
The current oNCP (Juniper) protocol support issues two separate
oNCP negotiation requests.
1) POST /dana/js?prot=1&svc=1 HTTP/1.1
<ignore response body>
<teardown and restart TLS connection>
2) POST /dana/js?prot=1&svc=4 HTTP/1.1
<continue using open TLS connection for oNCP tunnel>
The first of these two requests appears to be totally unnecessary, based on
testing with two different Juniper gateways, one of which returns
"NCP-Version: 2" and one which returns "NCP-Version: 3" in response to the
oNCP negotiation requests.
Removing the first request saves an additional TLS negotiation (2-3
roundtrips with TLS 1.0) and allows the connection to start faster.
Daniel Lenski [Thu, 2 Aug 2018 19:10:45 +0000 (12:10 -0700)]
Reduce unnecessary connection-rebuilding for Juniper
The current oNCP (Juniper) protocol support sets "Connection: close" in all
HTTP requests. This is not ideal because it requires many TLS handshakes
and round-trips, making the connection very slow to start when the latency
of the connection to the gateway is high, especially if the number of
authentication forms and redirects is large.
Simply removing the "Connection: close" header causes the oNCP connection
to fail; the server doesn't interpret the first packet sent over the oNCP
tunnel correctly (the vestigial authentication packet).
However, it appears that the "Connection: close" header *only* needs to be
specified for this final HTTP request, and not for any of the prior ones.
The presence of this header seems to signal to the gateway that it should
stop treating this as an HTTP connection, and start treating it as an
oNCP tunnel.
Tested on two different Juniper gateways, one which returns
"NCP-Version: 2" and one which returns "NCP-Version: 3" in response to
the oNCP negotiation requests.
Daniel Lenski [Thu, 2 Aug 2018 01:13:59 +0000 (18:13 -0700)]
Clarify protocol description in connection message
- Include both the TCP- and UDP-based protocols' compression details
- The UDP-based protocol really can't be connected by the time this
prints, since the mainloop hasn't had enough time to receive the
connection confirmation packets; show it as "in progress"
Before (with default verbosity):
Connected as 10.0.0.3 + dead::be:ef, using SSL + deflate
Established DTLS connection (using GnuTLS). Ciphersuite (DTLS1.2)-(RSA)-(AES-128-GCM).
After:
Connected as 10.0.0.3 + dead::be:ef, using SSL + Deflate, with DTLS + LZS in progress
Established DTLS connection (using GnuTLS). Ciphersuite (DTLS1.2)-(RSA)-(AES-128-GCM).
Daniel Lenski [Wed, 1 Aug 2018 02:35:59 +0000 (19:35 -0700)]
Align naming and commenting of mechanism for receiving oversize packets across protocols
We've now implemented mechanisms to tolerate larger-than-expected packets for:
- Uncompressed CSTP packets ("Fixed regression with CSTP MTU handling"
patch in July 2016)
- Uncompressed oNCP packets ("Do not drop vpn connection if packet arrived
is larger than MTU" patch in May 2017)
- Uncompressed GPST packets (in original merge from March 2018; this is a
virtual necessity for GlobalProtect because it has no functional
mechanism for negotiating the MTU)
- Uncompressed ESP packets ("check for oversize ESP packets, with 256
bytes of headroom above calculated" in March 2018; GlobalProtect requires
this for the aforementioned reason)
- Compressed CSTP packets (preceding patch in this series)
Since this is a requiring issue across protocols, it's useful to align the
naming, commenting, and packet sizing-tolerance across the source files.
1) Use receive_mtu everywhere as the name for the maximum tolerated size of an
incoming packet.
2) Insert similar comments explaining its purpose everywhere it's used.
3) Use receive_mtu = MAX(16384, vpninfo->ip_info.mtu) for all TLS-based
tunnels, because 16384 is the maximum TLS record size.
4) Use receive_mtu = MAX(2048, vpninfo->vpninfo->ip_info.mtu + 256) for
all UDP-based tunnels, because the MTU of IP datagrams on the public
internet is effectively ~1500.
Daniel Lenski [Wed, 1 Aug 2018 02:32:26 +0000 (19:32 -0700)]
Tolerate packets that are larger than negotiated MTU after decompression
In July 2016, the "Fixed regression with CSTP MTU handling" patch
(http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/90e1555494dbc1cf462552679f9aa3d30451d123)
allowed openconnect to gracefully handle uncompressed CSTP packets larger
than the negotiated MTU.
This patch extends that approach to tolerate compressed packets which are
larger than the negotiated MTU after decompression.
Daniel Lenski [Tue, 27 Mar 2018 15:39:58 +0000 (08:39 -0700)]
check for oversize ESP packets, with 256 bytes of headroom above calculated MTU
Because GlobalProtect VPNs have no practical mechanism for negotiating the
MTU whatsoever, it is entirely based on a client-side guess. Therefore,
extra headroom is needed to reliably receive packets via both the HTTPS and
the ESP tunnels.
A similar patch by Nikolay Martynov <mar.kolya@gmail.com> was originally
applied for the Juniper HTTPS tunnel:
http://lists.infradead.org/pipermail/openconnect-devel/2017-May/004320.html
And similar functionality was incorporated into the GlobalProtect HTTPS
tunnel as well (gpst.c).
This patch adds the extra headroom for the ESP tunnel (used by both Juniper
and GlobalProtect VPNs) as well, after unexpectedly-large ESP packets were
observed "in the wild":
https://github.com/dlenski/openconnect/issues/96
Signed-off-by: Dan Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Thu, 31 May 2018 15:25:15 +0000 (18:25 +0300)]
fix typo in Juniper TOTP form name
There's a thread from November 2017 in which Kevin Cernekee and Andy
Wang discuss a Juniper VPN where TOTP fails to work:
http://lists.infradead.org/pipermail/openconnect-devel/2017-November/004569.html
… and in that thread both "ftmTotpToken" and "frmTotpToken" are
mentioned. It appears that the reason for the misbehavior discussed in
the November 2017 is because of the (overlooked) typo in the code.
I'm not sure how the "ftm" version got committed in the first
place. ¯\_(ツ)_/¯. I did a bit of archeological digging, and I can't
find the original patch (from November 29, 2016) anywhere on the
mailing list, but it's obviously in the Git history:
http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/1ff34cb9689fbaf57decac537df1e32e799bb9c7
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
projects / users / dwmw2 / openconnect.git / log