Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAML2 stateless #1218

Open
juliangums opened this issue Jun 21, 2024 · 6 comments
Open

SAML2 stateless #1218

juliangums opened this issue Jun 21, 2024 · 6 comments

Comments

@juliangums
Copy link

@27pchrisl I'm unsure if I understand it correctly but I am trying to make SAML2 work with Filament Socialite https://github.com/DutchCodingCompany/filament-socialite

I am experiencing issues and I noticed that it is due to how the stateless method and attribute is implemented in the SAML2 driver. I have been through the comments of this issue #955 and I still don't quite get why it was implemented in a way that it does not set the stateless attribute to true when calling ->stateless()->redirect(). isStateless() always returns false as that is how it is defined in the AbstractProvider class. hasInvalidState() therefore also always tries to find the state in the response but because it is stateless it does not find it and therefore always returns true. Sure I can still get the user through ->stateless()->user() but for integration with the mentioned package it will check whether to use ->stateless()->user() or ->user() based on the isStateless() return. Is there no way to set this up before redirect() as other drivers do it or even simply set $stateless to true for the SAML2 driver? Other drivers do not seem to have this issue as they seem to be built in the standardised way. I don't understand why this one doesn't. Is this a design choice or technical limitation? I'm totally new to SAML2 and all this so I wouldn't know but I would like a way to make it work.

@juliangums juliangums changed the title SAML2 setup SAML2 stateless Jun 21, 2024
@27pchrisl
Copy link
Contributor

Hi @juliangums. This is a bit tricky, but fundamentally you must create state if your SAML service provider (app) is going to kick off the authentication process (SP-initiated flow). This means there's no statelessness on redirect(). If the SP initiates the flow, then it will create state, redirect the user to the IDP, pick up the state when it comes back from the IDP, and the IDP will have returned in its message to the SP that "this flow was initiated with this state".

The other type of SAML flow is IDP-initiated (much less common) where the authentication service creates a valid, signed message on its own, and redirects the user, unsolicited, to the service provider. This is a "stateless" SAML flow, because the SP hadn't created state in advance. For the service provider to accept this unsolicited inbound request, we make the callback() stateless, because this is the endpoint that's receiving the message.

So that's why we set stateless on the receiving side, not the sending side.

I haven't used filament-socialite, but I would assume that the way it works is that you hit the app (not logged in) and you're offered a choice of socialite logins (including SAML2), and if you choose to use SAML2 you're redirected to an IDP to log in and then you come back to the SP. In this case, the flow wouldn't be stateless.

Can you describe more about the issue you're seeing, as I would have thought the default behaviour should be what your app expects...

@juliangums
Copy link
Author

Thanks for your explaination/help @27pchrisl.

It seems like it's sending a stateful request to the IDP (Microsoft Entra) but thinks it is receiving a stateless response back although there is a RelayState in the response but not a state attribute. That seems to result in two issues I'm trying to explain. Let me try to break down how I think the package works:

It registers the provider redirect here:
https://github.com/DutchCodingCompany/filament-socialite/blob/main/routes/web.php#L24 which calls this controller method https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Controllers/SocialiteLoginController.php#L26
I there, it calls PanelFromUrlQuery::encrypt() where it tries to add the ID of the panel (an identifier that is needed in Filament as you can build multiple panels with separate logins) to the state.

Here is where the callback is registered: https://github.com/DutchCodingCompany/filament-socialite/blob/main/routes/web.php#L28

Issue 1 is when the PanelFromUrlQuery middleware gets called, it tries to decrypt the earlier given state/panel ID from the response going through the handle() method https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Middleware/PanelFromUrlQuery.php#L17 to the decrypt() method https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Middleware/PanelFromUrlQuery.php#L28
In there, it tries to get the state attribute from the response. The first problem is there is none. If I change the code to read RelayState instead (which exists), it seems to not be able to decrypt it. I suspect that it is also encrypted by the SAML encryption and has to be decrypted with that first, but there seem to be quite a few steps involved to even test that.

But even if I just override the call to the decrypt() method with a hardcoded panel ID (I can do that as I only have one), I'm still running into issue 2 trying to retrieve the user:

When following the former mentioned callback https://github.com/DutchCodingCompany/filament-socialite/blob/main/routes/web.php#L28 the method registered there calls the retrieveOauthUser() method to get the user https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Controllers/SocialiteLoginController.php#L134

Now the interesting part, this is where it determines if the provider is stateless or not by calling its getStateless() method https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Controllers/SocialiteLoginController.php#L53 which just returns the $stateless attribute of the provider which is always false for SAML2.

For some reason however, further down here https://github.com/DutchCodingCompany/filament-socialite/blob/main/src/Http/Controllers/SocialiteLoginController.php#L59-L61 $driver->stateless()->user() has a user instance in it while $driver->user() does not. If I override the getStateless() output to be true, it finds the user and logs in.

Any idea how to overcome these two issues?

@juliangums
Copy link
Author

juliangums commented Jun 24, 2024

@27pchrisl ok as for the state that can't be decrypted it is because it's not used for the RelayState but rather a random string you're generating here https://github.com/SocialiteProviders/Providers/blob/master/src/Saml2/Provider.php#L206
I think I have a workaround for the panel ID though.

It looks like the other issue is also down to the fact that hasInvalidState() returns false. This is because the $state is always null. Even if I remove the panel ID stuff. I made sure it is stateful and checked that this is definitely happening https://github.com/SocialiteProviders/Providers/blob/master/src/Saml2/Provider.php#L206 but then when it pulls it here
https://github.com/SocialiteProviders/Providers/blob/master/src/Saml2/Provider.php#L633 it is always null. Seems like the sessions aren't matched or something. Any idea?

@juliangums
Copy link
Author

juliangums commented Jun 25, 2024

@27pchrisl after using the workaround that is also implemented in the package soon, it works if I use ->stateless() now. But like you explained, it is actually an SP-initiated flow, and therefore should be stateful if possible. I think it should work if the sessions are matched correctly, but right now they aren't.
I figured out that the reason $state was null in hasInvalidState() is my sessions config values. I have followed what James has done here #782 (comment) and I get a value. Perhaps this could be added to the readme?

However, it still fails as the $state looks like this
eyJpdiI6IjZobjVENTB3UUNvRVBFT2RrVGZQS2c9PSIsInZhbHVlIjoiQlVMVERpbk85dGdNa1dvaGhhYU5kUT09IiwibWFjIjoiNjhiYTcyZDYxZDM0ZmEzNGE0ZjMyOGJkYjBlYzM5YjQzNGY5M2NmY2Y4NzM3ZTQwMzk2YmMyMWVkZDQ2YzJhNyIsInRhZyI6IiJ9

and the RelayState looks like this

hNIeW91nGrVkV2RegIOBhBEAUQBqsuSQTxgXaZXY

Any idea why this is happening?

@27pchrisl
Copy link
Contributor

@juliangums it looks like $state is being set to a value encrypted by Laravel's encryption system, RelayState is the state value internal to the Saml2 processor. As long as RelayState is not being modified the Saml2 provider should be able to pick up the session on the return journey.

$state isn't being generated by the Saml2 provider, and I don't think Socialite would add to the state like that - is this something filament-socialite is inserting? If so it should be able to decrypt it.

Are you still having the same failure as you started with or something different?

@juliangums
Copy link
Author

@27pchrisl
RelayState isn't modified, but it still creates a new session on return.

The state session value is actually added by the SAML2 provider here: https://github.com/SocialiteProviders/Providers/blob/master/src/Saml2/Provider.php#L206
It is set to the same value as RelayState in the line thereafter, so state and RelayState should be the same.

The state is then read again on the return here
https://github.com/SocialiteProviders/Providers/blob/master/src/Saml2/Provider.php#L633
where it doesn't match the value that has been set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants