From: Daniel Lenski Date: Tue, 13 Apr 2021 19:29:16 +0000 (-0700) Subject: Add Fortinet DTLS support X-Git-Tag: v8.20~267 X-Git-Url: https://www.infradead.org/git/?a=commitdiff_plain;h=dc059032bd7c302c2a8a535d4e05795d0ac7bd19;p=users%2Fdwmw2%2Fopenconnect.git Add Fortinet DTLS support This is almost exactly a copy of the F5 DTLS support added in commit 0066def47e0af0c7caf29842629ff3a58da4f0c2. Key differences: - For Fortinet, vpninfo->ppp_dtls_connect_req differs from vpninfo->ppp_tls_connect_req. For F5 they are identical. - For Fortinet, the DTLS/UDP port always matches the TLS/TCP port. For F5 they can differ. Signed-off-by: Daniel Lenski --- diff --git a/fortinet.c b/fortinet.c index f9958c57..e51994de 100644 --- a/fortinet.c +++ b/fortinet.c @@ -35,6 +35,12 @@ #include "openconnect-internal.h" #include "ppp.h" +/* clthello/svrhello strings for Fortinet DTLS initialization. + * NB: C string literals implicitly add a final \0 (which is correct for these). + */ +static const char clthello[] = "GFtype\0clthello\0SVPNCOOKIE"; /* + cookie value + '\0' */ +static const char svrhello[] = "GFtype\0svrhello\0handshake"; /* + "ok"/"fail" + '\0' */ + void fortinet_common_headers(struct openconnect_info *vpninfo, struct oc_text_buf *buf) { @@ -309,9 +315,14 @@ static int parse_fortinet_xml_config(struct openconnect_info *vpninfo, char *buf domains = buf_alloc(); - if (!xmlnode_get_prop(xml_node, "dtls", &s) && atoi(s)) - vpn_progress(vpninfo, PRG_ERR, - _("WARNING: Fortinet server enables DTLS, but OpenConnect does not implement it yet.\n")); + if (vpninfo->dtls_state == DTLS_NOSECRET && + !xmlnode_get_prop(xml_node, "dtls", &s) && atoi(s)) { + udp_sockaddr(vpninfo, vpninfo->port); /* XX: DTLS always uses same port as TLS? */ + vpn_progress(vpninfo, PRG_INFO, _("DTLS is enabled on port %d\n"), vpninfo->port); + vpninfo->dtls_state = DTLS_SECRET; + /* This doesn't mean it actually will; it means that we can at least *try* */ + vpninfo->dtls12 = 1; + } for (xml_node = xml_node->children; xml_node; xml_node=xml_node->next) { if (xmlnode_is_named(xml_node, "auth-timeout") && !xmlnode_get_prop(xml_node, "val", &s)) @@ -424,8 +435,16 @@ static int fortinet_configure(struct openconnect_info *vpninfo) int ret, ipv4 = -1, ipv6 = -1; /* XXX: We should use check_address_sanity to verify that addresses haven't - changed on a reconnect, except that Fortinet doesn't appear to actually - support reconnects. */ + changed on a reconnect, except that: + + 1) We haven't yet been able to test fully on a Fortinet + server that actually allows reconnects + 2) The evidence we do have suggests that Fortinet servers which *do* allow + reconnects nevertheless *do not* allow us to redo the configuration requests + without invalidating the cookie. So reconnects *must* use only ppp_reset(), + rather than calling fortinet_configure(), to redo the PPP tunnel setup. See + https://gitlab.com/openconnect/openconnect/-/issues/235#note_552995833 + */ if (!vpninfo->cookies) { /* XX: This will happen if authentication was separate/external */ @@ -487,23 +506,6 @@ static int fortinet_configure(struct openconnect_info *vpninfo) if (ipv6 == -1) ipv6 = 0; - /* To use the DTLS tunnel instead, we should do a DTLS handshake - * to the appropriate IP:port, and then send the packet... - * - * "${BE16_LEN_OF_THIS_PACKET}GFtype\x00clthello\x00SVPNCOOKIE\x00${SVPNCOOKIE}\x00dns0\x0010.0.2.3\x00" - * - * to which the server will respond either 'ok' or 'fail'... - * - * "${BE16_LEN_OF_THIS_PACKET}GFtype\x00svrhello\x00handshake\x00ok\x00" - * - * After that, the IP-over-PPP-over-DTLS packet framing is identical to - * the IP-over-PPP-over-TLS framing. (See evidence at - * https://github.com/adrienverge/openfortivpn/issues/473#issuecomment-776456040) - * - * Starting the TLS tunnel appears to invalidate the DTLS tunnel option, and - * presumably vice versa. - */ - reqbuf = vpninfo->ppp_tls_connect_req; if (!reqbuf) reqbuf = buf_alloc(); @@ -524,9 +526,7 @@ static int fortinet_configure(struct openconnect_info *vpninfo) if (!reqbuf) reqbuf = buf_alloc(); buf_truncate(reqbuf); - const char clthello[] = "GFtype\x00clthello\x00SVPNCOOKIE\x00"; - int len = 2 + sizeof(clthello) + strlen(svpncookie->value) + 1; - buf_append_be16(reqbuf, len); + buf_append_be16(reqbuf, 2 + sizeof(clthello) + strlen(svpncookie->value) + 1); /* length */ buf_append_bytes(reqbuf, clthello, sizeof(clthello)); buf_append(reqbuf, "%s%c", svpncookie->value, 0); if ((ret = buf_error(reqbuf))) @@ -556,8 +556,15 @@ int fortinet_connect(struct openconnect_info *vpninfo) */ ret = ppp_reset(vpninfo); } - if (ret) - goto out; + if (ret) { + err: + openconnect_close_https(vpninfo, 0); + return ret; + } + + ret = ppp_tcp_should_connect(vpninfo); + if (ret <= 0) + goto err; /* XX: Openfortivpn closes and reopens the HTTPS connection here, and * also sends 'Host: sslvpn' (rather than the true hostname). Neither @@ -566,15 +573,16 @@ int fortinet_connect(struct openconnect_info *vpninfo) */ ret = openconnect_open_https(vpninfo); if (ret) - goto out; + goto err; if (vpninfo->dump_http_traffic) dump_buf(vpninfo, '>', vpninfo->ppp_tls_connect_req->data); ret = vpninfo->ssl_write(vpninfo, vpninfo->ppp_tls_connect_req->data, vpninfo->ppp_tls_connect_req->pos); - if (ret < 0) - goto out; - ret = 0; + if (ret < 0) { + openconnect_close_https(vpninfo, 0); + goto err; + } /* XX: If this connection request succeeds, no HTTP response appears. * We just start sending our encapsulated PPP configuration packets. @@ -588,15 +596,57 @@ int fortinet_connect(struct openconnect_info *vpninfo) * is PPPS_ESTABLISH so that ppp_tcp_mainloop() knows we've started. */ ppp_start_tcp_mainloop(vpninfo); - out: - if (ret) - openconnect_close_https(vpninfo, 0); - else { - monitor_fd_new(vpninfo, ssl); - monitor_read_fd(vpninfo, ssl); - monitor_except_fd(vpninfo, ssl); + /* XX: Some Fortinet servers can't cope with reconnect, which means + * there's absolutely no point in trying to opportunistically do + * DTLS after this point. Can we detect that, and disable DTLS? + * I think it's relatively harmless because the auth packet over + * DTLS will fail anyway, so we'll never make it past DTLS_CONNECTED + * to DTLS_ESTABLISHED and never give up on the existing TCP link + * but it's still a waste of time and resources trying to do it + * at all. */ + + monitor_fd_new(vpninfo, ssl); + monitor_read_fd(vpninfo, ssl); + monitor_except_fd(vpninfo, ssl); + + return 0; +} + +int fortinet_dtls_catch_svrhello(struct openconnect_info *vpninfo, struct pkt *pkt) +{ + char *const buf = (void *)pkt->data; + const int len = pkt->len; + + buf[len] = 0; + + if (load_be16(buf) != len || len < sizeof(svrhello) + 2 || + memcmp(buf + 2, svrhello, sizeof(svrhello))) { + vpn_progress(vpninfo, PRG_ERR, + _("Did not receive expected svrhello response.\n")); + dump_buf_hex(vpninfo, PRG_ERR, '<', (void *)buf, len); + disable: + dtls_close(vpninfo); + vpninfo->dtls_state = DTLS_DISABLED; + return -EINVAL; } - return ret; + + if (strncmp("ok", buf + 2 + sizeof(svrhello), + len - 2 - sizeof(svrhello))) { + vpn_progress(vpninfo, PRG_ERR, + _("svrhello status was \"%.*s\" rather than \"ok\"\n"), + (int)(len - 2 - sizeof(svrhello)), + buf + 2 + sizeof(svrhello)); + goto disable; + } + + /* XX: The 'ok' packet might get dropped, and the server won't resend + * it when we resend the GET request. What will happen in that case + * is it'll just keep sending PPP frames. If we detect a PPP frame + * we should take that as 'success' too. Bonus points for actually + * feeding it to the PPP code to process too, but dropping it *ought* + * to be OK. */ + + return 1; } int fortinet_bye(struct openconnect_info *vpninfo, const char *reason) diff --git a/library.c b/library.c index 7cbd22a2..b0655ba7 100644 --- a/library.c +++ b/library.c @@ -225,13 +225,12 @@ static const struct vpn_proto openconnect_protos[] = { .obtain_cookie = fortinet_obtain_cookie, .secure_cookie = "SVPNCOOKIE", .udp_protocol = "DTLS", -#ifdef HAVE_DTLSx /* Not yet... */ - .udp_setup = esp_setup, - .udp_mainloop = esp_mainloop, - .udp_close = esp_close, - .udp_shutdown = esp_shutdown, - .udp_send_probes = oncp_esp_send_probes, - .udp_catch_probe = oncp_esp_catch_probe, +#ifdef HAVE_DTLS + .udp_setup = dtls_setup, + .udp_mainloop = ppp_udp_mainloop, + .udp_close = dtls_close, + .udp_shutdown = dtls_shutdown, + .udp_catch_probe = fortinet_dtls_catch_svrhello, #endif }, { .name = "nullppp", diff --git a/openconnect-internal.h b/openconnect-internal.h index ac49f702..cd83e950 100644 --- a/openconnect-internal.h +++ b/openconnect-internal.h @@ -1046,6 +1046,7 @@ void fortinet_common_headers(struct openconnect_info *vpninfo, struct oc_text_bu int fortinet_obtain_cookie(struct openconnect_info *vpninfo); int fortinet_connect(struct openconnect_info *vpninfo); int fortinet_bye(struct openconnect_info *vpninfo, const char *reason); +int fortinet_dtls_catch_svrhello(struct openconnect_info *vpninfo, struct pkt *pkt); /* ppp.c */ struct oc_ppp; diff --git a/www/fortinet.xml b/www/fortinet.xml index bf751fb2..1df8dab6 100644 --- a/www/fortinet.xml +++ b/www/fortinet.xml @@ -24,6 +24,11 @@ to the command line: openconnect --protocol=fortinet fortigate.example.com

+

Since TCP over +TCP is very suboptimal, OpenConnect tries to always use PPP-over-DTLS, +and will only fall over to the PPP-over-TLS tunnel if that fails, or if +disabled via the --no-dtls argument.

+

Quirks and Issues

In terms of authentication for Fortinet VPNs, OpenConnect currently supports @@ -47,10 +52,5 @@ please send information to the mailing list so we can understand it better, and whether we can support this feature on other Fortinet VPNs.

-

OpenConnect does not yet support the UDP transport for Fortinet, and -will use PPP over TCP for connectivity, -which is suboptimal -for performance.

-