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

IDP SAML2: added 'as:NoSession' #644

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tbenr
Copy link

@tbenr tbenr commented Jun 23, 2017

Allow AuthenticationSource to decide to avoid SessionIndex generation and association to current IDP session (avoid SLO).

I don't know it "as:" prefix is correct. If the feature is considered interesting, let's suggest the correct one.

Allow AuthenticationSource to decide to avoid SessionIndex generation and association to current IDP session (avoid SLO)
@tbenr tbenr changed the title added 'as:NoSession' IDP SAML2: added 'as:NoSession' Jun 23, 2017
@jaimeperez
Copy link
Member

Hi @tbenr!

I'm a bit confused by this. What's the use case you have in mind?

@tbenr
Copy link
Author

tbenr commented Aug 4, 2017

Hi again :-)

i have some requirements where, for some Authn Requests (heaving particular AuthnContextClassRefs) i dont have to generate a session on IdP side (so no SessionIndex released to SP). But for other AuthnContextRefs the session is still required. So I still need to have a session with SP tracking (for SLO).

So i developed an authentication module (the same module mentioned here #643 :-) ) which decide how to authenticate the user based on RequestedAuthnContext and also if simplesamlphp has to release a SessionIndex and track SP for SLO.

@jaimeperez
Copy link
Member

Ok, now you definitely got my attention 😄

Why is all this needed? I mean, you are basically letting an SP control whether users will be able to log out of it or not, from what I understand in your description. What's the purpose?

Also, what's the trouble with SessionIndex?

In any case, I feel this is very ad-hoc, so I'm a bit hesitant on how to approach this...

@tbenr
Copy link
Author

tbenr commented Aug 4, 2017

Well.. the "pratical" reason is this: I have to be compliant to SPID regulatory requiring IdP to NOT release SessionIndex when authenticating in SpidL2 or higher (let's say two factor auth). It also requires to not generating a session on IdP side. Actually, on IdP side you have a session only associated to SpidL1 (single factor auth), and eventually other authentication SpidL2 or higher MUST NOT interact with the previously established SpidL1 SSO session.

"Theoretically", IMHO these requirements are reasonable and have their underlying philosophy, even if awkward to implement.

I still need to verify\test interaction among SpidL1 SSO session and SpidL2> "temporary auth session". In my current implementation, using the feature in this PR, i can avoid SP tracking and SessionIndex creation for SpidL2>, but it still create a real session IdP side, that could potentially lead to a successful SSO with a subsequent SpidL1. But at the and I think this can be manage within the Authentication Module.

@jaimeperez
Copy link
Member

I somehow expected this being related to SPID. Lots of weird requirements coming from them lately 😞

Do you have a link where I can read about this policy? It's fine if it's only in Italian, I can probably make some sense out of it...

In any case, I definitely think this belongs in the authentication source. We have something similar in Feide, where we use the current SimpleSAMLphp codebase plus a custom auth source. There, we evaluate if a user with an existing session needs to be authenticated with two-factor again (in the reauthenticate() method) and add a LoginCompletedHandler that updates the session and runs the ReturnCallback set by the IdP.

This same mechanism can be used for your use case, making sure that if SpidL2 is required, the user is reauthenticated properly and SSO doesn't happen.

Regarding the SessionIndex, that's going to be more problematic, as there's nothing you can do about it yourself and it would require changes to the codebase. That's why I was asking for the specs, to try to understand if they do require that for real, and why. In any case, I don't really know how we should implement that properly, and I guess I need to think about it a little bit...

@tbenr
Copy link
Author

tbenr commented Aug 4, 2017

Thanks for the useful info, I'll go in deep with LoginCompletedHandler approach and how can help me.

I agree with you about SPID requirements oddity...

Anyway here is some useful links:

@jaimeperez
Copy link
Member

Thanks for the pointers!

I don't know Italian, but could it be that this is specifying a SHOULD instead of a MUST?

Tale elemento non dovrà essere presente nel caso di
asserzioni emesse a seguito di richieste di autenticazione per i livelli SPID 2 e SPID 3.

If it can be taken as a SHOULD, then you would be fine to ignore that requirement. In the end, I understand they intended this to prevent service providers from starting single logout because no session has been kept, due to the step-up level of authentication used. However, in that case, I don't see a reason why this should be enforced (as in, MUST), as that can be implicit for SPs joining SPID, and particular implementations not following that requirement might ignore logout messages anyway (like in this case).

Maybe we should find some authoritative voice that can resolve our doubts before we try to implement this?

@tbenr
Copy link
Author

tbenr commented Aug 4, 2017

Unfortunately in italian that means MUST (btw I'm actually italian :-D). I'm actually quite into SPID stuff (I already implemented an IDP with other solutions) and I agree with you that in this case there is no real benefit, apart of annoying implementers :) There are many little pitfalls that force implementers to drift from a standard de-facto SAML2 using opensource\vendor software to a customized solution. I heard such oddity could be blunted in the future but they are still officially in place.

@codecov
Copy link

codecov bot commented Nov 22, 2019

Codecov Report

Merging #644 (73c8f6a) into master (03bdf01) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #644      +/-   ##
============================================
- Coverage     40.39%   40.37%   -0.02%     
- Complexity     3454     3459       +5     
============================================
  Files           142      142              
  Lines         10401    10406       +5     
============================================
  Hits           4201     4201              
- Misses         6200     6205       +5     

@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 1c686ab to eb20457 Compare August 17, 2020 20:43
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 08ebb9c to 64fca25 Compare July 2, 2021 14:12
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 29f7b69 to 1a911ce Compare May 12, 2023 16:07
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from c7c8357 to fdbe001 Compare June 12, 2023 14:28
@tvdijen tvdijen force-pushed the master branch 8 times, most recently from 3b5f5ba to 96357ee Compare July 19, 2023 12:37
@tvdijen tvdijen force-pushed the master branch 5 times, most recently from 7587851 to d523b31 Compare August 2, 2023 11:58
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 8c90121 to d534e3b Compare September 5, 2023 08:09
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from bc1c5c8 to d0a5974 Compare October 17, 2023 21:17
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from ccb9b02 to 120a100 Compare December 1, 2023 14:34
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 6004a77 to 58bf8db Compare May 4, 2024 23:45
@tvdijen tvdijen force-pushed the master branch 2 times, most recently from 5c9fb2c to 0970efc Compare May 27, 2024 12:27
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

Successfully merging this pull request may close these issues.

None yet

3 participants