Support SAML authentication #1241

Rainer-Keller · 2024-09-15T06:32:19Z

This pull request is based on #1219, but has everything unrelated to SAML removed and all the review issues fixed.

These changes were taken from the original author and all changes unrelated to SAML were removed.

dsl400 · 2024-09-15T11:57:05Z

WARN: Bad key in configuration file: "route".
this will remove the option to configure routes locally and bypass the configuration coming from the server?

Rainer-Keller · 2024-09-15T13:07:05Z

WARN: Bad key in configuration file: "route". this will remove the option to configure routes locally and bypass the configuration coming from the server?

I've extracted only the SAML feature from your branch for easier review. The configuration options for dns and route overwrite I did not adopt. These options may go in as a separate pull request.

dsl400 · 2024-09-15T13:26:47Z

I will wait for this to be merged first

Rainer-Keller · 2024-09-15T13:33:47Z

I will wait for this to be merged first

Let's see how it goes...

exzombie · 2024-10-02T12:23:19Z

How does this compare to #1217?

Rainer-Keller · 2024-10-02T14:28:52Z

How does this compare to #1217?

At a first glance it looks like both MRs are doing about the same.

dsl400 · 2024-10-02T14:49:59Z

Not sure if something changed since I posted this comment

exzombie · 2024-10-02T15:23:40Z

Not sure if something changed since I posted this comment

It hasn't AFAICT, but all I get from your comment is that it's an independently developed alternative, and that you have implemented some additional features. Now, @Rainer-Keller has put some effort into breaking this up into manageable pieces, so I thought that there was some reason why this alternative implementation was deemed better.

Hopefully, in the near future, I'll have some time to try out one or the other implementation. I'm not convinced having two to test is better than one 😅

Rainer-Keller · 2024-10-02T15:52:16Z

@exzombie Yours looks fine as well. It has the extra option to pass the auth ID from the command line. But not sure of what use that is.
I was not aware that there is a 3rd implementation. The first one I found was the one from @dsl400 .
I left some comments on your MR ;)

If would be good to get some feedback from a maintainer which of these changes has the highest chance to get in. Otherwise we'll have double effort.
Once we know which one it is, I'll make some suggestions for error/debug message rephrasing.

src/http_server.c

exzombie · 2024-10-02T17:01:20Z

@exzombie Yours looks fine as well. It has the extra option to pass the auth ID from the command line. But not sure of what use that is.
I was not aware that there is a 3rd implementation. The first one I found was the one from @dsl400 .
I left some comments on your MR ;)

There seems to be some confusion, that MR is not mine, it is from @filippor. And are you sure there's a third one?

Anyway, thanks for reviewing that MR; I haven't taken the time yet, I'm still waiting for some prerequisites to fall into place. I hope we'll be able to move the SAML story forward, and that FortiClient updates won't break EMS, but that's a different ball of hair ...

mrbaseman · 2024-10-02T21:09:52Z

Actually, this pull request looks quite clean to me. I have been hesitant to dependencies on too many external tools, and also workflows involving sudo and pipes. Although I haven't recapitulated all discussions in the previous pull requests, and honestly I didn't do a detailed code review of them, I would still vote for this one. The reason for me is, that @Rainer-Keller has dropped several unrelated changes, and thereby has made the change much more readable and understandable. There are also a couple of potentially security related fixes included in this PR, compared to the original one.
Since I didn't have the time to take care of this project recently, I would like to hear the opinion of the other maintainers, too (I did follow the github discussions, but currently I'm not that much involved in the code development anymore)

Rainer-Keller · 2024-10-03T05:06:05Z

@exzombie
Right, seems I treated the PR you sent as if was yours.

I'm still waiting for some prerequisites to fall into place

Any details on what prerequisites that are?
Microsoft has dropped support for external 2nd factor (not sure if that is the right term). So right now, SAML is the only option. I'm using my custom built version at the moment.

And are you sure there's a third one?

There are solutions out there that seems to use a special built browser tool to extract the SAML id and forward this to openfortivpn.

@mrbaseman

I have been hesitant to dependencies on too many external tools, and also workflows involving sudo and pipes

Pipes have been used in the original MR from @dsl400 , but these were unrelated to SAML and I have removed them.

This would be my summary of the two PRs

#1241

Better error messages
Issues already resolved
More checks on the SAML id
Intuitive names of variables, functions and commandline options
User documentation

#1217

(a little) Less lines code
Realm support
HTML output for the response
Option to pass the SAML id on the commandline

If we all agree it is my PR that it the preferred candidate, then I can integrate the features from #1217 here as well.

exzombie · 2024-10-03T08:19:00Z

@Rainer-Keller

Any details on what prerequisites that are?

Our internal stuff, mostly, and Fortinet dragging their feet with supporting Ubuntu 24.04. So, nothing you need to bother yourself with.

There are solutions out there that seems to use a special built browser tool to extract the SAML id and forward this to openfortivpn.

Ah, ok. Just making sure I haven't missed another attempt at what I consider a proper SAML implementation.

filippor · 2024-10-04T17:36:05Z

Hello I create the pull request #1217 when I found the way to implement it using external browser. I'm not a c developer. Thanks for the code review. If it is needed I will fix the issue. I have no problems if this one will be merged. But I will wait for a word from a maintainer to complete my PR. It is working as is for my needs

exzombie

Hey! Thanks again for taking the time to push this feature forward. I'm happy with the simplicity of the approach, the "HTTP server" is really lightweight. That said, dealing with strings in C is one of the things I personally hate quite a bit, and the approach taken in the HTTP server does not convince me that the code is safe. I made some suggestions, hope you find them helpful.

src/config.c

src/http_server.c

src/http.c

src/http_server.c

exzombie · 2024-10-09T09:01:44Z

src/http_server.c

+        return -1;
+    }
+
+    strncpy(id, &request[id_start], id_length);


I'm confused here. id_length is determined by looking for a space. Then, you use this length with strncpy, which will not insert a null terminator at id[id_length-1]. And yet, the data in id is expected to be null terminated, because it's used with strlen in http.c. If this code works, I guess it's because id happens to be initialized to zeros. But we can't rely on that.

I suggest using memcpy and adding a null at the end explicitly.

This section was taken from the original author. I've replaced it with a completely different approach.

src/main.c

src/http_server.c

README.md

src/http_server.c

In the initial implementation there were still some issues that needed to be fixed. One main improvement is that the http server thread does not run the tunnel thread. Instead the http server is first shut down and then the tunnel is started as usual.

Fix compiler warnings about comparing signed with unsigned integers.

Rainer-Keller · 2024-11-23T11:01:23Z

Is there a way to get to know if/when the PR can go forward? I am fine with everything, but would be nice to know at least.

dsl400 · 2024-12-08T15:19:52Z

while trying to use this inside openwrt I found that is actually very fiddly to route traffic from lan to 127.0.0.1
can we use INADDR_ANY instead of INADDR_LOOPBACK ?

Rainer-Keller · 2024-12-09T17:08:47Z

while trying to use this inside openwrt I found that is actually very fiddly to route traffic from lan to 127.0.0.1 can we use INADDR_ANY instead of INADDR_LOOPBACK ?

I had this in an earlier version of this pull request, but then realized INADDR_ANY does open the port to be connectable by everyone in your local network. In case you are on a public WiFi, you would need to protect this port from strangers trying to get into your connection setup. Hence I changed this to be available only on the local machine.

When I am using Fortinet, it redirects directly to localhost.
Can you explain what happens in your case? I think I didn't get the use case why it would be required to route traffic between LAN and localhost.

dsl400 · 2024-12-09T17:32:57Z

I am trying to run the vpn client in my router as a service because the windows client does not allow me to configure split tunneling and the vpn connection is sometimes very slow.
While trying to do this I had issues exposing port 8020 to my local network
I think the best option here is to make the interface selection available via configuration and set INADDR_LOOPBACK as default

Rainer-Keller · 2024-12-10T09:24:37Z

I think the best option here is to make the interface selection available via configuration and set INADDR_LOOPBACK as default

Having this as a configuration option would be possible.

Let me propose another option. Maybe you have already tried this, but if not it would work without any changes:
ssh has port forwarding capabilities. If setting up the VPN is a manual action anyways, you can provide an extra option to ssh to make it work.

ssh -L 8020:localhost:8020 me@router

This will make ssh listen on your local machine on port 8020 (first 8020:), any connection to this port will be tunneled by ssh to the machine your are logged in to, and on the remote machine it will be connecting to localhost:8020.
When issuing this ssh command, your local browser will be connecting to the openfortivpn client on the remote machine.

dsl400 · 2024-12-10T11:01:45Z

Thanks for the idea. Exposing port 8020 on my pc has to be done anyway to finish the authentication.

Rainer-Keller · 2024-12-11T08:56:55Z

Does this mean it works for you, or is the config option still needed?

In my opinion the config option will add more code just for this single case, while the ssh port forwarding seems to be the cleaner solution anyways.

dsl400 · 2024-12-11T10:13:25Z

The option could be a good thing. SSH port tunnel involves addin ssh keys in the router for all pc-s in the network

exzombie · 2024-12-11T12:13:23Z

But can you make Fortinet redirect your browser to the router's IP? I thought the loopback address was hardcoded.

Rainer-Keller · 2024-12-11T16:16:14Z

The option could be a good thing. SSH port tunnel involves addin ssh keys in the router for all pc-s in the network

I have the code ready on Rainer-Keller@5a3e680.
This pull request has already received multiple reviews. Not sure if I should add the code to this request and restart the reviews or if I should wait until it is merged and then create a follow up pull request.

Listen for SAML login requests

994fabe

These changes were taken from the original author and all changes unrelated to SAML were removed.

Rainer-Keller force-pushed the master branch from 5ac02fb to 4db8b39 Compare September 15, 2024 07:49

Rainer-Keller force-pushed the master branch from 4db8b39 to 0e9f255 Compare September 15, 2024 13:26

Rainer-Keller force-pushed the master branch 2 times, most recently from 412e692 to 191099c Compare September 21, 2024 05:06

Rainer-Keller commented Oct 2, 2024

View reviewed changes

src/http_server.c Outdated Show resolved Hide resolved

Rainer-Keller force-pushed the master branch from 191099c to 6b961e6 Compare October 4, 2024 05:53

Rainer-Keller force-pushed the master branch from 6b961e6 to da010ae Compare October 6, 2024 05:36

exzombie reviewed Oct 9, 2024

View reviewed changes

exzombie reviewed Oct 10, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Rainer-Keller force-pushed the master branch 3 times, most recently from 8791318 to a91b087 Compare October 10, 2024 12:47

Rainer-Keller commented Oct 10, 2024

View reviewed changes

src/http_server.c Outdated Show resolved Hide resolved

Rainer-Keller force-pushed the master branch 2 times, most recently from 2727d74 to 67769e6 Compare October 10, 2024 15:02

Rainer-Keller force-pushed the master branch from 67769e6 to 400c787 Compare October 11, 2024 04:59

exzombie reviewed Oct 16, 2024

View reviewed changes

src/http_server.c Outdated Show resolved Hide resolved

Rainer-Keller force-pushed the master branch from 400c787 to 51e8d8e Compare October 16, 2024 14:43

Rainer-Keller added 3 commits October 16, 2024 16:51

Improve SAML authentication

8c45fad

In the initial implementation there were still some issues that needed to be fixed. One main improvement is that the http server thread does not run the tunnel thread. Instead the http server is first shut down and then the tunnel is started as usual.

Print SAML authentication URL for user convenience

80a9df7

Fix some compiler warnings

ecc7fa9

Fix compiler warnings about comparing signed with unsigned integers.

Rainer-Keller force-pushed the master branch from 51e8d8e to ecc7fa9 Compare October 16, 2024 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SAML authentication #1241

Support SAML authentication #1241

Rainer-Keller commented Sep 15, 2024

dsl400 commented Sep 15, 2024 •

edited

Loading

Rainer-Keller commented Sep 15, 2024

dsl400 commented Sep 15, 2024

Rainer-Keller commented Sep 15, 2024

exzombie commented Oct 2, 2024

Rainer-Keller commented Oct 2, 2024

dsl400 commented Oct 2, 2024

exzombie commented Oct 2, 2024

Rainer-Keller commented Oct 2, 2024

exzombie commented Oct 2, 2024

mrbaseman commented Oct 2, 2024

Rainer-Keller commented Oct 3, 2024 •

edited

Loading

exzombie commented Oct 3, 2024 •

edited

Loading

filippor commented Oct 4, 2024

exzombie left a comment

exzombie Oct 9, 2024

Rainer-Keller Oct 10, 2024

Rainer-Keller commented Nov 23, 2024

dsl400 commented Dec 8, 2024

Rainer-Keller commented Dec 9, 2024 •

edited

Loading

dsl400 commented Dec 9, 2024

Rainer-Keller commented Dec 10, 2024 •

edited

Loading

dsl400 commented Dec 10, 2024

Rainer-Keller commented Dec 11, 2024

dsl400 commented Dec 11, 2024

exzombie commented Dec 11, 2024

Rainer-Keller commented Dec 11, 2024 •

edited

Loading

Support SAML authentication #1241

Are you sure you want to change the base?

Support SAML authentication #1241

Conversation

Rainer-Keller commented Sep 15, 2024

dsl400 commented Sep 15, 2024 • edited Loading

Rainer-Keller commented Sep 15, 2024

dsl400 commented Sep 15, 2024

Rainer-Keller commented Sep 15, 2024

exzombie commented Oct 2, 2024

Rainer-Keller commented Oct 2, 2024

dsl400 commented Oct 2, 2024

exzombie commented Oct 2, 2024

Rainer-Keller commented Oct 2, 2024

exzombie commented Oct 2, 2024

mrbaseman commented Oct 2, 2024

Rainer-Keller commented Oct 3, 2024 • edited Loading

This would be my summary of the two PRs

exzombie commented Oct 3, 2024 • edited Loading

filippor commented Oct 4, 2024

exzombie left a comment

Choose a reason for hiding this comment

exzombie Oct 9, 2024

Choose a reason for hiding this comment

Rainer-Keller Oct 10, 2024

Choose a reason for hiding this comment

Rainer-Keller commented Nov 23, 2024

dsl400 commented Dec 8, 2024

Rainer-Keller commented Dec 9, 2024 • edited Loading

dsl400 commented Dec 9, 2024

Rainer-Keller commented Dec 10, 2024 • edited Loading

dsl400 commented Dec 10, 2024

Rainer-Keller commented Dec 11, 2024

dsl400 commented Dec 11, 2024

exzombie commented Dec 11, 2024

Rainer-Keller commented Dec 11, 2024 • edited Loading

dsl400 commented Sep 15, 2024 •

edited

Loading

Rainer-Keller commented Oct 3, 2024 •

edited

Loading

exzombie commented Oct 3, 2024 •

edited

Loading

Rainer-Keller commented Dec 9, 2024 •

edited

Loading

Rainer-Keller commented Dec 10, 2024 •

edited

Loading

Rainer-Keller commented Dec 11, 2024 •

edited

Loading