SAML/SSO/2FA support #1042
SAML/SSO/2FA support #1042
Conversation
|
Btw, thank you for your sugesstion to use webkit2gtk instead, @Newbytee. I feel stupid that I didn't do this initially 😆 |
|
Thanks for your brilliant work! I have a few issues and suggestions: Error on WaylandI installed this repo on Fedora 37 (Wayland). I get this error: (root)$ openfortivpn <myserver.org>:443 --saml
INFO: Connected to gateway.
Authorization required, but no authorization protocol specified
(openfortivpn:26675): Gtk-WARNING **: 11:37:16.388: cannot open display: :0
ERROR: Could not authenticate to gateway. Please check the password, client certificate, etc.
INFO: Closed connection to gateway.
INFO: Logged out.The reason is that root cannot open GTK windows on Wayland I think. What is the recommended way of running Better documentationIt works fine like this: Somewhere, it should be mentioned that the the cookie can be obtained via this page: https://<myserver.org>:443/remote/saml/start . One solution could be to print this URL and brief instructions. Moreover, imo, Update build dependenciesI had to install two more dependencies to build. Please mention that in README.md:
|
|
It works beautifully! Thanks! |
|
I think this is brilliant and I love where this is headed, but I'm having an issue. The SAML login does not appear to be following redirects. For example, my login process starts with https://azvpn.mydomain.com:4433/remote/saml/start, but the actual authentication happens at login.microsoftonline.com. I don't really know what I'm talking about, but I hope this helps. Below is an excerpt from the official forticlient logs: |
|
The problem is probably that the SAML login wasn't using the specified port. Should be fixed now. Could you try if it works for you? |
|
I tried to install the latest branch, but get an error on |
|
Sorry, I accidentally named to variables the same in a header file. Should work now. |
|
Now it work again for me! |
|
Thats great! Does it work for you now, @mikedehaan? |
|
I noticed an issue: // Don't allow other users to read the cookies.
chmod("/home/lorenz/.openfortivpncookies", 0600);Should probably be |
|
By “gui application” I mean that:
This may be an issue for some users. I am not against this solution, I am just trying to combine the best of both worlds. |
Only for 2FA authentications, and only if the user wants to use
Is it possible to make WebKitGTK optional during build? If openfortivpn is built without WebKitGTK, only the |
I agree that this might be a unnecessary dependency, but I don't know of any other way to avoid this on most distros. Maby creating a openfortivpn-headless package? I am currently working on implementing a compile time option to disable saml login. As @MrTomRod already said in his comment, you only need a graphical session (X/Wayland) if you want to use saml. Everything else does work without it. |
|
There's a package for openwrt available for instance, but as far as I have seen there is no X server or webkitgtk on that platform |
|
Openwrt could just compile the package without saml support. |
|
@mrbaseman @adrienverge Would a dependency on WebKitGTK be acceptable if a build-time option was available to drop SAML support and the WebKitGTK dependency? Again, note that a graphical session is required only for SAML. Otherwise |
|
@LorenzISR Can you fix tests? |
|
Also note that openfortivpn is often called from Networkmanager-fortisslvpn. I am afraid this will break Networkmanager-fortisslvpn. |
|
As far as I can tell, the biggest problem seems to be the dependency on Webkit-/GTK (which are huge packages) for people who don't need it. I don't know if most people are using openfortivpn through NetworkManager but if this is the case its probably a better idea to just let NetworkManager (or alternatives) do this if used for most people. If its disabled by default at build time, there is little sense in merging this here, right?
NetworkManager could just pass the saml flag to openfortivpn. In the end you need to decide if the extra dependency is worth it or not. I think its more easy to use openfortivpn from the command line rather than from NetworkManager but I don't know how many people are on my side. |
That's interesting:
With all that said, I vote for applying this patch. @adrienverge I realize this leaves openfortivpn in a less clean state, but then major changes are needed to address new needs in the cleanest possible way:
|
|
@theinvisible As a maintainer of openfortigui what is your opinion on adding SAML support based on WebKit/GTK to openfortivpn? |
|
New to the scene here but this feature is of interest to me. From what I gather, we don't really need WebKit/GTK here so much as need a browser to be involved. Could we instead set up a local HTTP server for the SAML process and then have the browser POST back to that local server any token relevant tokens? It would require that we depend on something that would provide HTTP which I imagine would be much lighter and we can depend on the user's local browser regardless of their OS and start that up with something like |
That's true, these huge packages + the extra complexification of compilation.
@DimitriPapadopoulos I follow you on the choice you think is the best. Inputs from @theinvisible and @er1 could be fruitful. |
There was a problem hiding this comment.
I don't believe openfortivpn can cleanly and reliably start a browser (see saml_get_cookie comments). But why is that even necessary?
Maybe I'm missing something obvious, but IMO #974 implemented all the SAML/2FA support that belongs into core openfortivpn. Network Manager & friends should now be able to implement auth on top of --cookie[-on-stdin] (issue link).
For CLI, there's openfortivpn-webview, a browser that prints the VPN cookie to stdout. This hacky one-liner is what I've been using for months:
$ openfortivpn-webview my.fortigate.com | sudo openfortivpn my.fortigate.com --cookie-on-stdinLinking WebKitGTK significantly increases footprint, even when the browser isn't used.
RSS of openfortivpn at __libc_start_main, i.e. before doing anything:
# 1.19.0
$ grep ^VmRSS /proc/21337/status
VmRSS: 1936 kB
# This PR
$ grep ^VmRSS /proc/21581/status
VmRSS: 30420 kB
Startup also feels noticeably slower, but I didn't measure that.
The footprint issues are fixable, but IMO the whole "impersonate a user and start a browser" approach is both infeasible and unnecessary.
| clearenv(); | ||
| setenv("HOME", home_dir, 1); | ||
| setenv("DISPLAY", ":0", 1); | ||
|
|
||
| // Needed for wayland | ||
| setenv("XDG_RUNTIME_DIR", xdg_runtime_dir, 1); |
There was a problem hiding this comment.
These hard-coded envvars assume the the user's homedir is under /home, their X11 session is on local display 0 and their XDG_RUNTIME_DIR is at /run/user/$UID. HOME, DISPLAY and XDG_RUNTIME_DIR might not suffice either. What about DBUS_SESSION_BUS_ADDRESS, XMODIFIERS, GTK_IM_MODULE, etc.?
| // Needed for wayland | ||
| setenv("XDG_RUNTIME_DIR", xdg_runtime_dir, 1); | ||
|
|
||
| setuid(browser_uid); |
There was a problem hiding this comment.
If setuid fails, the browser will run as root. If it succeeds, it will run as browser_uid but still have access to file descriptors that were opened as root.
|
@er1 This all started with ticket #867 which ended up with multiple implementations:
Does your proposal add something radically different? |
|
@gm-vm I believe a drawback of gm-vm/openfortivpn-webview is that installation might appear difficult for many users: |
|
@LorenzISR I believe @mvf raises thought-provoking concerns:
Indeed, what does the WebKitGTK integration offer that an external program such as gm-vm/openfortivpn-webview doesn't? Wouldn't WebKitGTK-based external software be a good solution for Linux/BSD platforms? A challenge would be to rapidly provide RPM/DEB packages bundled with Linux distributions. Or instead of re-inventing the wheel, extending OpenFortiGUI by @theinvisible to support SAML could maybe do the trick. |
It shouldn't matter, but I created the Electron version simply because the Qt one stopped working with my SAML provider for some unknown reason. I had no issues with Electron and so I ended up using it. I also remember doing the authentication from a minimal python+Gtk+WebKit browser. The main advantage of the Qt version is that it uses shared libraries for pretty much everything and so the binary is really small.
Already supported. I should update the README.
Yep, that was Atom.
The Qt version needs to be built from source, while there are portable binaries for Linux (x64) of the electron variant here: https://github.com/gm-vm/openfortivpn-webview/releases The AppImage can be executed as is, the tar.gz needs to be extracted and inside of it there will be an executable (and a bunch of files).
Linux for sure, but I haven't tried it on anything else. I know someone else has been using the Electron application with macOS. No idea about FreeBSD, but it's mostly a matter of Qt or Electron being supported there. |
No, debian stable is currently on 1.15.0 where --cookie does not exist, in looking for SAML support, this ticket came up first which led me to (wrongly) believe SAML support was not implemented yet but progress was being made toward it. I believe openfortivpn-webview will fit my use case. |
|
I am pretty much out of arguments at this point why this should be merged. I think that doing this with an external program using |
|
@gm-vm Do you have an idea how best to pack openfortivpn-webview for different distributions? While I am not the maintainer of openfortivpn and cannot speak for him, I wonder whether you would agree to merge the two projects or maybe pack the sources together, using Git subtrees or submodules for example. Ideas? |
|
@DimitriPapadopoulos I'm using I'm only building those two because other than being (almost) universal, I need to adjust few things for the other formats (I remember for example that The Qt app needs work to be packaged properly, right now the Makefile only generates an executable. My hope is that some day someone will add the support for this directly in NetworkManager. openfortivpn-webview was just a way to conveniently use the VPN without first studying how to patch NetworkManager. I'm not against including it as submodule, but is that desired? While the Qt application requires almost no maintenance, the electron app needs to be updated constantly due to the fact that it bundles electron. Luckily there's dependabot and so far I think it notified me about only two vulnerabilities that were affecting electron itself, all the other vulnerabilities only had impact on stuff used at build time. I still had to go through the changes and check they were OK. What I'm saying is that there is some maintenance burden and providing packages for popular distributions only make things worse. Not to mention the fact that there are two seemingly equivalent applications that makes everything confusing, I'd probably have to sort that out. I understand the desire of providing an easy way to use SAML with Alternatively there could even simply be a section in the wiki (maybe linked in the README, because I personally always miss the wiki of Github projects), explaining how to do the authentication with SAML providers using external applications, even standard browsers. This would also allow to share simple scripts to do the authentication headlessly where doable. |
|
Hello, Is it possible to clone your branch yet ? I am using your SAML solution to connect to work vpn. Without your solution I will be not able to connect to vpn in future. |
|
This branch still works on my system. You could compile it yourself and use it until it stops working. Otherwise, you could install the latest version of openfortivpn (Fedora: 1.17.0-5 or later / Ubuntu: 1.19.0-2 or later) and perform 2FA yourself using following these instructions. If I understood correctly, no GUI will be added to this library and you have to wait for things like openfortigui to make this more convenient. |
|
@LorenzISR: Why this PR has been closed? |
|
@Neustradamus if you still interested in this PR you may want to check build in my fork with a few small patches for my fortivpn provider. I don't plan to support this fork much but now it works for me as is. |
|
@BLaDZer: Thanks for your comment! It will be better to have in official build... I do not know why there is not an official support... |
|
I agree |
|
Dear all, It is possible to create a new PR linked to this original? Thanks in advance. |
|
since openfortivpn runs as root, it does not make any sense to run the browser as root too. this whole PR is a complete hack, as the review @mvf also shows. it's a much better idea to use an external program that fetches the cookie and pipes it into the stdin. |
Hi,
I just deleted the code that I initially wrote and the pull request was closed aswell (Not on purpose). As sugessted by @Newbytee, I've rewritten this with WebKit2Gtk.
It probably needs a compile time option aswell? I need some help with that.
Regarding your comment, @DimitriPapadopoulos "Perhaps the core openfortivpn shouldn't be changed into a GUI program instead of a command line program. It would be great if a library was available to mimic a browser from within a command line program, and that could be used to interact with HTML or JavaScript forms pages. I don't know of such a library."
I don't think adding webkit2gtk to authenticate makes it a gui application. I would also like to avoid browser stuff but unfortunately there is no other way for doing SAML at the moment (None that I know of). I already explained why this should be a feature in #867
What do you think? Btw, I don't want to sound unkind, its just that my english isn't that great.