Several improvements for gp-okta.py

[coldcoff]

Hello @arthepsy,

please have a look at this pull request.

• implement all of @aclindsa's changes (aclindsa:master) [ I had foked from him, initially ] So this PR fully includes Add option for second round of Okta authentication #7 and is a superset of it.

• rework @aclindsa's new 'another_dance' -> the real concept is to first dance with the portal (like vpn.company.com), afterwards dance with a specific gateway (like vpn-uk.company.com), that also explains why there are two separate urls (this will answer your question Add option for second round of Okta authentication #7 (comment) and following in Add option for second round of Okta authentication #7), see also https://github.com/dlenski/openconnect/blob/master/PAN_GlobalProtect_protocol_doc.md
  (Note that in @aclindsa's setup the portal and the gateway seem to run on the same machine, which is only a special case of my general implementation, where really different machines in different regions of the worls may be involved).

• Implement possibility to use an own client client certificate for https communication, fixes prelogin.response: "Valid client certificate is required" #17.

• Implement possibility to check the server certificates for vpn_url and okta_url, fixes Rebase, Client-Certificates, Double/Split authn with Okta aclindsa/pan-globalprotect-okta#1 (comment). The configured certificates are also passed to the final openconnect call, potentially together with new learnt certificates from the paloalto_getconfig call (see also @nicklan's fork).

• some more, small bugfixes, enhancements, improvements (see individual commit messages). Based on feedback from our userbase of some dozen people here.

• New Feature: Allow Yubikey webauthn authentication, if configured as 2FA in Okta. Just insert your YubiKey and press the blinking button, when asked to do so. Needs fido2 packages to work (pip install fido2).

I had asked @aclindsa to merge in his fork (as my work was based on his fork and he alreday has a PR against your repository open). But he asked me to bring the changes to your attention as now 15 of the 17 commits are from me, and as I might be able to answer most of your questions / comments better than him. Both of us don't want to maintain a permanent fork and we would like to bring our enhancements "back home" to you in your original repository (aclindsa#1 (comment)). Also this might be one more step towards @dlenski's wish for one-script-to-rule-them-all (see: dlenski/openconnect#122 (comment)).

Please test, have a close look and feel free to ask every question or comment that comes to your mind reg. our changes. I'll try to address everything until it hopefully gets merged ;-)

Thank you and best regards

@coldcoff

[coldcoff]

Any news? Can this be please be merged? We have it here in production for months without problems.

[nootanghimire]

Just confirming I needed this fork in order to make it working! Having this merged would be awesome! (cc @arthepsy)

Thanks heaps for making it!

[cardonator]

can you add push to this list? It was missed in #14.

[coldcoff]

done, please test.

[cardonator]

Small request: for some reason my GP server works with the prelogin_cookie but not the userauthcookie. Can the token to use be an option that is set in the config?

[coldcoff]

Or some preference setting? Could you please send a patch that is working on your side, then I can test here as well.

[cardonator]

sure I will polish it up a bit tomorrow and send it. sorry to hijack your PR but this is the smoothest experience for using GP out of the few options there are :)

[cardonator]

I was wrong, for some reason it wasn't working yesterday but it is working fine now.

The next thing is that my server requires a HIP report, so I need to be able to pass --csd-wrapper=hipreport.sh to openconnect at the end.

[cardonator]

(FYI I woke up and realized I can add that to the openconnect command in the config file... maybe something to consider adding to the README)

[arthepsy]

I merged the pull request, but this completely broke our company's VPN in two ways, e.g.:

[INFO] Discarding 'vpn_url', as concrete gateway is given and another_dance = 0
..
[DEBUG] https://GW/ssl-vpn/prelogin.esp

Issue is, our company's GW are named, not full URLs (same as for example gateway "Manual ..", which is not a valid URL).

redirect form: unexpected url found ('https', u'gw.DOMAIN.TLD') != (u'https', u'vpn.DOMAIN.TLD')

Errors out, because gateway URL is different than VPN url.

Will investigate how to fix those issues.

[arthepsy]

@coldcoff could You please explain, is this really required and is correct? Seems wrong to me:

        cmd = cmd.format(username, cookie_type,
               'https://{0}'.format(gateway) if conf.get('another_dance', '').lower() in ['1', 'true'] else conf.get('vpn_url'))

i.e, really, openconnect should connect to gateway_url (I renamed it) if another_dance = 1? Maybe it's meant to check for non-empty gateway_url not another_dance?

It fails for my company's VPN, but then again, I think I don't have a setup, where I can test this, because authenticating directly to our gateway, gives GlobalProtect gateway does not exist.

[arthepsy]

Fixed 1st issue (retaining this pull request features) in 2a1fb94, by using gateway_url. Also, while there, fixed gateway list retrieval.

[arthepsy]

Fixed 2nd issue in b5dffd6, by only warning about unexpected URL.

[cardonator]

@arthepsy when you have these fixes in a PR let me know. I couldn't use the "dance" in this on my VPN, either. I had to hard code one of the gateways.

Also, does the dance randomly pick the second gateway? The official client does something to try to figure out what the best option is.

[arthepsy]

@cardonator these fixes are in master right now. You can test them out. Right now I'll only add features, e.g., removing "bug.*" and adding openconnect_fmt, so regardless of openconnect version, anything can be fixed in config. This will fix compatibility with openconnect 8.05+. Will also work on auto-detecting it. Anyhow, go ahead and test right now.

What do You mean "hard code one of the gateways"?

If with dance You mean another_dance, in this pull request, I'm not sure, as I can't test it. For me, it worked before and now works with another_dance=0. My company's VPN dance is like this: portal -> okta -> gateway (acts as portal).

Official clients works detects which gateway to use by "priorities" (by getting config from portal) defined by administrator. They actually are in SAML responses.

[cardonator]

Okay, thanks for the info. I have to specify one of the gateways returned by the portal in order to connect prior to your changes. Right now, there is an issue on line 685 because in Python 3 gateways.keys() returns a dict_keys object which doesn't support pop.

Traceback (most recent call last):
  File "./gp-okta.py", line 841, in <module>
    sys.exit(main())
  File "./gp-okta.py", line 782, in main
    gateway_url = choose_gateway_url(conf, gateways)
  File "./gp-okta.py", line 686, in choose_gateway_url
    gateway_host = gateways.keys().pop()
AttributeError: 'dict_keys' object has no attribute 'pop'

If I fix that issue by casting it to a list, it does select one of the gateways but the dance still fails for some reason with

POST https://gateway/global-protect/getconfig.esp
Got HTTP response: HTTP/1.1 512 Custom error
Unexpected 512 result from server

It tries to do the Okta dance again after that and then fails on putting in the login credentials:

Enter login credentials
Username: fgets (stdin): Resource temporarily unavailable

If I add the second dance config, I get this instead:

[ERROR] okta redirect form (2): unexpected url found ('https', 'portalhost') != ('https', 'gateway')

If I add a value to the gateway config parameter of one of the gateways that is returned from the portal, then I am able to connect without issues.

[arthepsy]

Yes, I haven't tested compatibility for Python2/Python3 with latest commits. Will do that after I implement few features (working on them now).

So, do I understand correctly that except Python3 issue, when You change gateway = name in config, all is fine? If so, it's same for my company and is expected. I use only vpn_url and gateway, no other options. Or I misunderstood something?

[cardonator]

No that's exactly right. MY portal is weird because it does have a set of gateways that imply they need to do the okta dance. But if the behavior you understand is having the gateway specified, that use case does work for me as long as I have the push functionality (that you have gratefully merged in :) )

[arthepsy]

Actually, gateway is required for piping to openconnect command, e.g., it does something like this: printf "<cookie>\n<gateway>" | openconnect .... You can manually check what's required, by running openconnect command and pasting in line-by-line. Then You'll see where and why it could fail.

P.S. I'll commit new features and fixes in few minutes.

[cardonator]

Okay, thanks. I'll move this conversation to a new issue if I have more comments.

[arthepsy]

@cardonator #21.

[coldcoff]

@coldcoff could You please explain, is this really required and is correct? Seems wrong to me:

        cmd = cmd.format(username, cookie_type,
               'https://{0}'.format(gateway) if conf.get('another_dance', '').lower() in ['1', 'true'] else conf.get('vpn_url'))

i.e, really, openconnect should connect to gateway_url (I renamed it) if another_dance = 1? Maybe it's meant to check for non-empty gateway_url not another_dance?

It fails for my company's VPN, but then again, I think I don't have a setup, where I can test this, because authenticating directly to our gateway, gives GlobalProtect gateway does not exist.

This is basically line 929? The reasoning is/was:
If we dance twice already in python then the final openconnect destination to connect to should be the gateway. not the portal. We did all in gp-okta.py and can direct openconnect directly to the gateway, including the credentials needed.

Note that a portal does not need to offer any gateway capabilities, although often both functionalities run on the very same machine, under different paths.