Extend OTP support to also allow using FTM push

[fvanderveen]

In our organization, the fortinet server is configured to be able to send a push notification to the FortiToken app. I figured I could give it a shot to add this feature to this client :).

Checking the requests the official client sends for this, I found it sends ftmpush=1 instead of the code=<OTP>&code2= data in the logincheck post.

I've done my best to keep the code around this as clean as I could, but my C++ isn't that strong.
The code does what I want it to do though; it sends me a push notification if I don't pre-configure the OTP, and uses the given OTP if I run openfortivpn with the --otp=<OTP> option.

[DimitriPapadopoulos]

As I'm new to this, some context:

• Technical Tip: FortiToken Push on FortiAuthenticator - operation flow and details
• SSL VPN with FortiToken mobile push authentication
• Configuring FortiToken Mobile

[mrbaseman]

Looks good to me.

[mrbaseman]

Maybe the help output and the man page should be updated?

[fvanderveen]

Maybe the help output and the man page should be updated?

@mrbaseman @DimitriPapadopoulos I didn't really add any options. Though this comment made me think it might be useful to add an opt-out option for this. So I added a commit that adds --no-ftm-push to disable using this 2fa mechanism.

I tried figuring out if I could somehow add this to the config file as well; but I couldn't find a nice way to do so. If either of you think it's necessary to be possible through the config file as well; some help on what direction would be nice.

[mrbaseman]

the config file is loaded here in load_config in config.c and merged with the command line options at a later stage

[fvanderveen]

@mrbaseman thanks; I think I got everything now. Please have a look again :).

[fvanderveen]

I rebased this due to a merge conflict in http.c. It came from the hostcheck support that was recently merged.

Did a small smoke test for FTM push, as well as checking if --otp=<otp> and --no-ftm-push still worked as expected.

One thing I noticed, while testing after the rebase, is that our fortinet server returned a http/405 when sending both ftmpush=1 and magic=<value> in the same logincheck request.
Since it was happy to eat it when sending code=<otp>&code2=, I moved this parameter to that part of the code; as I suspect this check functionality is only used (or supported) when sending an OTP token back for 2FA.

[DimitriPapadopoulos]

Looks good to me. Thank you for taking the extra time to re-check after the last rebase. Would you agree to squashing these commits into a single commit?

Otherwise it might be worth intercepting communication between FortiClient and the Fortinet appliance to make sure there are no other parameters like magic= that are omited with ftmpush=1. I'll have a look if I can get hold of a recent FortiClient at some point and check relevant traffic.

Finally we still have this issue with Travis CI. I really don't know how to fix it without looking into the repository settings. Perhaps this branch has inherited incorrect ties to Travis CI from before #732.

[fvanderveen]

@DimitriPapadopoulos looked at what the windows forti vpn client does; and even though it receives a portal and magic parameter; neither is actually sent back for the ftmpush=1 request.

I don't want to assume that this is always going to be the case. It could be something simple as the client recognizing portal=maz-tunnel as the default, and simply not sending it back for that reason.

As I don't have access to configure this server, I cannot really test what the multi-portal functionality would do with ftmpush. (I'd like to err on the side of caution here; and just send it along. It doesn't seem to break anything here.)

I'm fine with squashing these commits, don't think there is anything too useful to keep in the separate commit messages that isn't also mentioned in this PR.

[DimitriPapadopoulos]

Great. Just squash the commits then, I think we're ready to integrate them. Thank you so much for your patience.

[fvanderveen]

@DimitriPapadopoulos ah, I thought it would be an option in github when merging the branch; maybe I'm spoiled by this option existing in gitlab :).

I've squashed the commits, and added some description into the commit message, just in case that helps someone in the future.

[mrbaseman]

@DimitriPapadopoulos Squashing the commits during merge is also possible here. Unfortunately Github is now waiting for Status to be reported back, I think from Travis ci

[DimitriPapadopoulos]

Oh sure, it's an option in GitHub too. But I think we cannot both rebase and squash - and sometimes you need both by the time you merge.

Yes, Travis CI still doesn't work on this PR. Perhaps we can test this commit locally and then ask @adrienverge to merge.

[adrienverge]

But I think we cannot both rebase and squash

I believe the "Squash and merge" button actually squashes and rebases (so it's awkwardly named).

Yes, Travis CI still doesn't work on this PR. Perhaps we can test this commit locally and then ask @adrienverge to merge.

Sure, just ping me when needed.

[DimitriPapadopoulos]

@adrienverge The otp/add-ftm-push-support branch passes tests. Can you please merge?

[adrienverge]

Sure!