Saml external browser #1217

filippor · 2024-04-29T00:57:35Z

add parameter --ext-browser-saml[=]

start listening on listen port (default)
print an address to manually open in a browser
( the gateway server will send browser a javascrip to call localhost:8020/?id=<auth-id> )
receive the http request and extract the auth-d
use auth-id to autenticate on address /remote/saml/auth_id?id=

there is also parameter --auth-id= if retrieved externally

NEED HELP to review printed messages and update man pages

the ext-browser-saml can be configured configuration file but in this case the port is required
es

host = vpn.sample.com
port = 10443
ext-browser-saml = 8020

start listening on port 8020 or parameter print a address to open in browser when id received proceed with the authemntication

LorenzISR · 2024-04-29T05:40:56Z

as i've mentioned in #867, imo this is not a good idea. external programms such as https://github.com/gm-vm/openfortivpn-webview are a cleaner approach to handling this -- maby look into packaging it for your distro instead?

expose auth-id option

filippor · 2024-04-29T13:35:44Z

LorenzISR

Can you be more explicit this pr use a different method than openfortivpn-webview

LorenzISR · 2024-04-29T14:10:11Z

i know that, i've read your code.

you are adding a bunch of code for functionality that openfortipvn-webview can already do, but better. if it gets packaged for distros that is actually a much nicer user experience than having to manually run an external browser. that is why i've said that you should look into packaging it instead.

filippor · 2024-04-29T14:46:21Z

The advantage is that this solution could use the default browser that keep the session and if it is valid doesn't need to reautenticate. And i think it is easy to integrate in external gui or NetworkManager
and can be executed with
xdg-open https://<gateway>:<port>/remote/saml/start?redirect=1 &sudo openfortivpn <gateway>:<port> --ext-browser-saml

INADDR_ANY does not always work

exzombie · 2024-04-30T07:10:00Z

I hope to be able to test this in the coming weeks.

I'm a regular user of code from an unfinished MR in NetworkManager-fortisslvpn which uses the same approach as openfortipvn-webview, with the benefits of NetworkManager integration. While it's a life saver in my situation, I have to say that the approach taken in this PR would be preferable. The VPN is not very stable, and I am often on a flaky uplink, necessitating many reconnections. Typing the password and dealing with 2FA can be quite annoying.

We can debate whether this functionality belongs into openfortivpn itself instead of an external tool. I think having it in openfortivpn has the benefit of making it available to all users. For comparison, having openfortivpn-webview available doesn't do much for those of us who need NetworkManager, and while the Gnome version of the NM user interface now has an unfinished reimplementation of what openfortivpn-webview offers, other interfaces (like KDE Plasma) don't have it. In other words, everybody would need to integrate a webview, which is not trivial. On the other hand, if openfortivpn can handle this by itself, adding SAML to NM interfaces is one new checkbox away.

The way I see it, this is just another way for openfortivpn to get the cookie, complementing the stdin method. Authentication is delegated to an external tool (i.e., the default browser), same as always.

robdevops · 2024-04-30T08:23:56Z

Working well on Ubuntu. I do strongly prefer this method because it is a much simpler command that does not require additional packages, nor the awkward pipe into sudo, nor re-authentication because it uses my existing browser session.

This is how convoluted my launch command needs to be with openfortivpn-webview:
$ sudo whoami # cache sudo credentials because you can't (easily) type your password into a pipe
$ ~/openfortivpn-webview-1.2.0.AppImage --trusted-cert='sha256/+NbF2hzIunRbOynladSfCD9gj9HEKYHc4Y7h9dlGqQg=' vpn.example.com:10443 | sudo ./openfortivpn --cookie-on-stdin --trusted-cert f8d6c5da1cc8ba755b3b29e569d49f082f608fd1c42981dce18ee1f5d946a908 vpn.example.com:10443

Vs this branch:
$ sudo ./openfortivpn --ext-browser-saml vpn.example.com:10443 --trusted-cert f8d6c5da1cc8ba755b3b29e569d49f082f608fd1c42981dce18ee1f5d946a908

It is better in every way: quicker to install, easier to use, and saves a good 40 secs by using existing session.

refactor

robdevops · 2024-05-01T01:52:29Z

Hi @LorenzISR

a bunch of code for functionality that openfortipvn-webview can already do, but better. if it gets packaged for distros that is actually a much nicer user experience

Expanding on my previous comment, the user experience is superior with this PR for the following reasons:

Even if openfortipvn-webview were packaged, in order to continue supporting non-GUI systems, it can never be pulled in as a dependency of the openfortipvn package, so on any system with a working browser, the openfortipvn-webview method will always be more complex to install.
The openfortipvn-webview launch command is convoluted with openfortipvn-webview, requiring shell tricks like pipes or stdin redirects, so it will always be harder to use, especially when combined with sudo and password input.
Both methods use an external browser. With this PR, it is guaranteed the user will have browser package options that have an active maintainer, as long as they are on a supported OS.
This PR supports hardware auth methods like YubiKey, FIDO/U2F, smart cards, and Passkeys. It is a more universal solution.
The reuse of existing browser session cookies can significantly reduce the number of user interactions and average time to connect.

I think the last two items in particular are high value for user experience. I respectfully request you reconsider use cases where this PR adds real value.

LorenzISR · 2024-05-02T08:33:16Z

pipes are not "tricks", they are normal features of a shell, and they have always been. you will need sudo anyways, regardless if you use webview or not. i am not using sudo but doas instead but using pipes works perfectly fine for me. openfortivpn-webview can safe the credentials (i think?), you'll just have to click the button for your SSO provider to safe them. i don't understand why openfortivpn-webview would not work with hardware auth methods?

that said, i am not a maintainer but just a random person on the internet who's worked on this problem before and this is just my opinion.

robdevops · 2024-05-03T02:32:56Z

pipes are not "tricks", they are normal features of a shell, and they have always been. you will need sudo anyways, regardless if you use webview or not. i am not using sudo but doas instead but using pipes works perfectly fine for me.

I'll clarify. If sudo is at the beginning of the command, it can cleanly prompt for keyboard input before openfortivpn is executed. In the openfortivpn-webview case where sudo is after a pipe, you have sudo writing to stdout, openfortivpn-webview also writing to stdout, sudo reading from stdin, and finally openfortivpn reading from stdin. The output of sudo and openfortivpn-webview gets mixed together and the sudo password prompt is buried above line feeds from openfortivpn-webview, with openfortivpn-webview printing to the part of the screen where the users password input indicator is supposed to be printed. It's clunky. There are ways to make this smoother, but my point is it's an inferior user experience out of the box. If further steps are required to make this smoother, then the existence of those steps themselves make for a more complicated user experience.

i don't understand why openfortivpn-webview would not work with hardware auth methods?

I thought I read on another discussion that it didn't. Apologies if I am wrong.

openfortivpn-webview can safe the credentials (i think?), you'll just have to click the button for your SSO provider to safe them. i don't understand why openfortivpn-webview would not work with hardware auth methods?

openfortivpn-webview does cache, but you may be overlooking that SAML is commonly used for single sign-on, where web apps can use the VPN initiated cookie or vice versa. That is, the browser may already be signed in before the VPN is ever used, and likewise, signing into the VPN may also sign into various web apps. Using openfortivpn-webview segregates the cache of the VPN from the cache of web apps, breaking the single sign-on model by resulting in more consecutive sign on interactions.

LorenzISR · 2024-05-03T06:01:11Z

sudo does not read the password from stdin.

i'd argue that re-using the sign-in from the browser might even be a security risk for some and not desired; having to sign in once for your VPN is fine imo and this is also how it works on windows.

gm-vm · 2024-05-03T07:35:00Z

This seems to do what it is described here.

That's neat, I don't remember having this option in my official client (and I no longer have access to the VPN to check it now), but this definitely removes the need of using a custom made browser.

@LorenzISR this seems to be a feature of the gateway that lets clients use any browser for the authentication. It is different from launching the browser from within openfortivpn.

robdevops · 2024-05-03T12:39:20Z

@gm-vm that's correct. I use this option in the official mac client.

filippor · 2024-05-03T16:46:06Z

If needed I can split this PR
1 for adding the authentication method with ID and another to listen for id on port.
But I think the changes are not so big.

herpus-derpus · 2024-05-17T10:01:34Z

I am of the opinion this PR would vastly improve all linux user experience for everyone I work with.

My default browser session requires me to SAML auth with a second factor each day. Currently I have to do this twice each morning since openfortivpn-webview requires it separately.

If the openfortivpn client could make use of my already authenticated session in my default browser, this would significantly improve the SSO experience for myself and all other linux desktop users where I work. They would be over the moon. I don't see a decrease in security for us, since we need to SAML auth daily anyway as policy means our session expire each day.

herpus-derpus · 2024-05-17T10:02:56Z

Presumably I could build openfortivpn from this filippor:saml-external-take2 branch ?
Is that easy ?

filippor · 2024-05-17T12:00:11Z

Presumably I could build openfortivpn from this filippor:saml-external-take2 branch ? Is that easy ?
It is not complicated to compile the instruction is in the readme on this repository

or to retrieve the token you could use this project https://github.com/filippor/XdgOpenSaml/tree/main it also use the external browser a binary that not require javacompiled for linux-x86-64 is here

dsl400 · 2024-05-20T18:14:58Z

Without knowing this even existed and driven only by the need of a specific feature I written a solution that kinda solves the same problem, there are few extra features like specifying routes, dns entries and domain prefixes in the config file to override the configuration coming from the server.
In my case the server uses microsoft services with 2FA. While I do undertand the need for a SAML sign in method on systems without gui I am not really sure that openfortivpn-webview is a good idea and here are some reasons:

bloat
security
overcomplicated

I really would not push a "home made browser" over a mainstream browser.

and ....

that said, i am not a maintainer but just a random person on the internet who's worked on this problem before and this is just my opinion.

itss0076 · 2024-07-17T11:01:26Z

Rather than trying to put the browser spawning and web server listening bits into the (run as root) openfortivpn process, why not use the privilege-separated interface already provided for openfortvpn-webview?

Make a new process (run unprivileged) that does the SAML dance with the VPN appliance, by shelling out to the user's browser and listening for a response on port 8020, and then outputs the "SVPNCOOKIE=..." string for piping into "sudo openfortivpn --cookie-on-stdin ...".

filippor · 2024-07-17T19:51:07Z

The implementation as external program already exist at this repo https://github.com/filippor/XdgOpenSaml.
The idea of this pr is to integrate all the process in this software.

Rainer-Keller · 2024-10-02T14:31:59Z

src/config.c

@@ -87,6 +87,8 @@ const struct vpn_config invalid_cfg = {
 	.user_agent = NULL,
 	.hostcheck = NULL,
 	.check_virtual_desktop = NULL,
+	.auth_id = NULL,
+	.listen_port = 0


The variable names are very generic. Maybe, being more specific about what the listen_port is actually listen for might improve readability.

Rainer-Keller · 2024-10-02T14:43:05Z

src/http.c

-	if (username[0] == '\0' && tunnel->config->password[0] == '\0') {
+	if (tunnel->config->auth_id != NULL) {
+		char url[256];
+		snprintf(url,sizeof(url), "/remote/saml/auth_id?id=%s",


Size check is missing. In case the auth ID gets too long, it will be cut off without notice.

Rainer-Keller · 2024-10-02T14:47:38Z

src/idlistener.c

+
+static void print_url(struct vpn_config *cfg) {
+	char url[512];
+	snprintf(url, sizeof(url), "https://%s:%d/remote/saml/start?redirect=1",


Size check missing. For long server names, the string might be cut off.

Rainer-Keller · 2024-10-02T14:47:38Z

src/idlistener.c

+
+static void print_url(struct vpn_config *cfg) {
+	char url[512];
+	snprintf(url, sizeof(url), "https://%s:%d/remote/saml/start?redirect=1",


Size check missing. For long server names, the string might be cut off.

Rainer-Keller · 2024-10-02T14:55:10Z

src/idlistener.c

+			cfg->gateway_host, cfg->gateway_port);
+
+	if (cfg->realm[0] != '\0') {
+		strncat(url, "&realm=", sizeof(url) - 1);


To my understanding the 3rd argument is how may bytes from the src argument it should copy, but not the total size of both strings combined.

Also it has to be checked if there is actual space in url to fit the additional string. According th the man page, behavior is undefined.

Rainer-Keller · 2024-10-02T15:24:49Z

src/main.c

+				if (optarg != NULL) {
+					port = strtol(optarg, NULL, 0);
+					if (port < 1 || port > 65535) {
+						log_error("Specify a valid listen port or omit for parameter ext-browser-saml\n");


Suggestion:

Specify a valid listen port for parameter ext-browser-saml or omit it to use the default.

Rainer-Keller · 2024-10-02T15:26:56Z

src/main.c

+			}
+			if (strcmp(long_options[option_index].name, "auth-id") == 0) {
+				free(cli_cfg.auth_id);
+				cli_cfg.auth_id = strdup(optarg);


I find it handy to have at least a zero length check to catch shell mistakes like empty variables when calling as early as possible.

Rainer-Keller · 2024-10-02T15:31:24Z

src/main.c

+	    if(cfg.auth_id){
+	    	log_warn("auth-id will be ignored conflict with --ext-browser-saml");
+	    }
+		log_debug("Will listen on port \"%d\" for authentication id \n", cfg.listen_port);


There's about the same message in idlistener.c:listen_for_id

Rainer-Keller · 2024-10-02T15:42:43Z

src/main.c

+		free(cfg.auth_id);
+		cfg.auth_id = listen_for_id(&cfg);
+	}
+
 	if (geteuid() != 0) {
 		log_error("This process was not spawned with root privileges, which are required.\n");
 		ret = EXIT_FAILURE;


(Sorry, for putting it here, but I didn't find out how to comment on files that there not changed.)

This MR is missing changes to README.md and doc/openfortivpn.1.in

Rainer-Keller · 2024-10-02T15:45:15Z

src/idlistener.c

+	read(newsockfd, buffer, MAX_REQUEST_SIZE - 1);
+	log_debug("Received HTTP request:\n%s\n", buffer);
+
+	char *id = parse_request(buffer);


I'd like to have a check if the id string has zero length, just in case something went wrong. I won't make sense to continue in that case.

Rainer-Keller · 2024-10-02T15:57:45Z

src/idlistener.c

+	if (cfg->realm[0] != '\0') {
+		strncat(url, "&realm=", sizeof(url) - 1);
+		char *dt = url + strlen(url);
+		url_encode(dt, cfg->realm);


In the documentation of url_encode it is stated that the buffer size must be at least 3x the size of cfg->realm +1. I can't find that reflected in the code here.

filippor added 3 commits April 29, 2024 01:51

add parameter and auth by id

bb197aa

Implement --ext-browser-saml

391f70b

start listening on port 8020 or parameter print a address to open in browser when id received proceed with the authemntication

add response message

4a48dd4

filippor mentioned this pull request Apr 29, 2024

Support login SAML #867

Open

listen only from localhost

4cdc964

expose auth-id option

filippor marked this pull request as ready for review April 29, 2024 10:16

remove static url_encode

f2382c6

filippor added 2 commits April 29, 2024 17:09

restore listen on all port

9edb330

INADDR_ANY does not always work

more resilient request parser and fix compile warnings

3823c38

filippor added 2 commits April 30, 2024 12:26

release port after connection

65fe582

configuration file add ext-browser-saml parameter

d3ff5a6

refactor

set option before bind

cd37145

gm-vm mentioned this pull request Jun 5, 2024

WebKitGTK version? gm-vm/openfortivpn-webview#31

Closed

filippor added 2 commits August 20, 2024 11:11

Merge branch 'master' into saml-external-take2

2691469

fix merge

25fec71

exzombie mentioned this pull request Oct 2, 2024

Support SAML authentication #1241

Open

Rainer-Keller reviewed Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Saml external browser #1217

Saml external browser #1217

filippor commented Apr 29, 2024 •

edited

Loading

LorenzISR commented Apr 29, 2024

filippor commented Apr 29, 2024

LorenzISR commented Apr 29, 2024

filippor commented Apr 29, 2024 •

edited

Loading

exzombie commented Apr 30, 2024

robdevops commented Apr 30, 2024 •

edited

Loading

robdevops commented May 1, 2024

LorenzISR commented May 2, 2024

robdevops commented May 3, 2024

LorenzISR commented May 3, 2024

gm-vm commented May 3, 2024

robdevops commented May 3, 2024

filippor commented May 3, 2024

herpus-derpus commented May 17, 2024

herpus-derpus commented May 17, 2024

filippor commented May 17, 2024 •

edited

Loading

dsl400 commented May 20, 2024 •

edited

Loading

itss0076 commented Jul 17, 2024

filippor commented Jul 17, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Rainer-Keller Oct 2, 2024

Saml external browser #1217

Are you sure you want to change the base?

Saml external browser #1217

Conversation

filippor commented Apr 29, 2024 • edited Loading

LorenzISR commented Apr 29, 2024

filippor commented Apr 29, 2024

LorenzISR commented Apr 29, 2024

filippor commented Apr 29, 2024 • edited Loading

exzombie commented Apr 30, 2024

robdevops commented Apr 30, 2024 • edited Loading

robdevops commented May 1, 2024

LorenzISR commented May 2, 2024

robdevops commented May 3, 2024

LorenzISR commented May 3, 2024

gm-vm commented May 3, 2024

robdevops commented May 3, 2024

filippor commented May 3, 2024

herpus-derpus commented May 17, 2024

herpus-derpus commented May 17, 2024

filippor commented May 17, 2024 • edited Loading

dsl400 commented May 20, 2024 • edited Loading

itss0076 commented Jul 17, 2024

filippor commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filippor commented Apr 29, 2024 •

edited

Loading

filippor commented Apr 29, 2024 •

edited

Loading

robdevops commented Apr 30, 2024 •

edited

Loading

filippor commented May 17, 2024 •

edited

Loading

dsl400 commented May 20, 2024 •

edited

Loading