testing a new SSL+ESP VPN

[Daniel Lenski]

Thanks for the detailed reply, David! This gives me a pretty good idea
of how to proceed.

On Fri, Oct 14, 2016 at 4:03 PM, David Woodhouse <dwmw2 at infradead.org> wrote:
> I note you also added --globalprotect. Let's not do that; it was a
> mistake with Juniper. Let's just use --protocol.

Agreed, I was going to remove it.

> It would also be quite nice to have it autodetect the protocol when
> just given a URL. Just something to bear in mind...

I thought about this as well. I think I would do this by adding a
probe() function to vpn_proto. Do you agree with that approach?

Ideally, probe() should be robust enough to detect compatibility even
when the client credentials and/or SSL certificate is wrong or
missing.

    struct vpn_proto {
        const char *name;
        /* Check whether the server is compatible with this protocol */
        int (*probe)(struct openconnect_info *vpninfo);
        ...
    }

>> The cookie has the following format, which preserves everything that's
>> returned by the login request, and is subsequently needed to get the
>> configuration, connect to the SSL tunnel, and log the session out:
>>
>>     USER=Username;AUTH=deadbeefdeadbeef;PORTAL=GatewayName;DOMAIN=domain
>
> OK. Fairly much like Juniper in theory then, although we never did
> establish that we need anything but the DSID cookie there.

Yeah, the GP protocol is just silly and arbitrary in terms of which
parameters are required:

- getconfig.esp requires the username, the authcookie, and the portal
(all returned by login.esp) to succeed
- The SSL tunnel requires the username and the authcookie and NOT the
portal... but it will be rejected until you've posted to getconfig.esp
anyway.
- logout.esp requires the username, the authcookie, the portal, AND
the domain (???) to succeed

>> I tried to reuse existing coding style and interfaces from
>> openconnect, but there are a few things that don't work very well:
>>
>> 1. In order to log the user out, a separate HTTPS request has to be
>> issued. However, openconnect_open_https() and do_https_request() don't
>> work once got_cancel_cmd is set. Currently I have to trick openconnect
>> by temporarily setting got_cancel_cmd=0 during the logout request. Do
>> you have any suggestions on how to do this more cleanly?
>
> Hm. The other protocols are *also* supposed to issue a separate call to
> logout. That's cstp_bye() and ... hm, is that not implemented for
> Juniper? I thought it had been.
>
> For Cisco it does seem to work but perhaps only because we use the
> existing connection and just write the BYE packet to it. We never poll
> and wait for input, thus we never check the got_cancel_cmd flag.

That's right. cstp_bye() sends an "in-band" logout message. It sends a
"bye" packet to the (still open) HTTPS tunnel.

Juniper doesn't have a logout at all, as you notice.

GlobalProtect has to start a new connection.

This took me a couple hours to debug... pretty much the *only*
frustrating aspect of the code that I encountered. :-)

> You have to establish a new connection though, right? I think it's
> probably OK to set vpninfo->got_cancel_cmd=0 in the mainloop right
> before calling proto->vpn_close_session. If we get cancelled *again*
> then we'll give up on that too.

Aha, that's what I was thinking as well! got_cancel_cmd should be
un-set as soon as it is handled (by breaking out of the mainloop).

The existing code *already does this* for got_pause_cmd
(https://github.com/dlenski/openconnect/blob/globalprotect/mainloop.c#L262)
but I wasn't sure if there was some subtle reason why this shouldn't
be done similarly for got_cancel_cmd.

> Do try really hard to find a way you can make it write-and-forget like
> the Cisco one though :)

Do you mean I should try to find a way to do an in-band logout message
that doesn't require a separate request? I actually don't think it
exists.

>> 2. As I understand it, the two-phase design of openconnect is intended
>> to do all the messy XML processing in the unprivileged phase, so that
>> only the simplest parts of the SSL/DTLS/ESP tunnel setup need to be
>> done by root. However, this doesn't work so well with GlobalProtect,
>> because all of the routing information comes in an XML file which
>> isn't received until after the login and getconfig requests. So I am
>> doing some XML processing during the connection process, in
>> gpst_parse_config_xml().
>
> No, that's OK. It's not *so* much about avoiding the text processing.
> The two-phase thing is mostly about having access to the user's
> certificates, and being able to interact with the user for password
> input, etc.
>
> What we don't want is for there to be HTTP redirects to a different
> host in the second "connect" phase — because that *might* mean we have
> to ask the user if we want to accept this new server's invalid
> certificate.

Ah, I see. The current implementation simply ignores the <gw-address>
returned by getconfig.esp, and assumes that the tunnel host is always
the same as the authentication gateway. Sounds like I should consider
this to be a security feature, in keeping with the description in
openconnect.h:

    /* The elements above this line come from server-provided CSTP headers,
     * so they should be handled with caution.  gateway_addr is generated
     * locally from getnameinfo(). */
    char *gateway_addr;

It's ironic that you bring up this issue with multiple hosts and an
incorrect certificate on the second host, actually. There's actually
an outer layer of XML configuration that the Windows client fetches
and uses to determine which gateways are available based on a
configurable per-user profile.  Similar to the way that AnyConnect
sometimes advertises multiple gateways, I think. The outer layer
configuration returns a list of tunnel gateway hosts along with their
(alleged) certificates. However, if the certificate doesn't match the
tunnel gateway, the Windows client rejects it even though it comes
from the supposedly-trusted outer gateway, and won't connect, but
without explaining what's wrong.

This misconfiguration prevented the Windows client from working
properly for months and probably wasted 100-200 hours among a few of
my coworkers and me, before it got escalated to the vendor… it's
basically what made me decide that openconnect would handle this VPN a
*LOT* better. >:-(

Long story short is that there's an outer wrapper of
easily-misconfigured stupidity that openconnect has no reason to
touch, as far as I can tell.

> And it's also quite important that the connection works with just the
> cookie(s) and *not* having access to the user's certificate. Does it?

Yes. The GET-tunnel request will succeed with a valid authentication
cookie, but with no client cert.

> But where you've got fixes to the existing code (e.g. the excessive
> url-encoding, the 'const' to buf_append_urlencoded()), I could merge
> those now if you want to submit them as separate patches with a Signed-
> off-by: tag.

Sounds good. I'll submit those shortly, as well as a fix for
got_cancel_cmd if that works as expected.

> I'm planning to do an OpenConnect 7.08 release soon — it *was* waiting
> for the MTU fixes in https://github.com/openssl/openssl/pull/1666 but
> now it's also waiting for related fixes in
> https://gitlab.com/gnutls/gnutls/issues/140 as well.
>
> It'd be good to merge GP support after 7.08 perhaps.

Will do. That'll give me some time to clean these patches up more logically.

Thanks,
Dan