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

Privacy issues with SessionIndex implementation #41

Open
pmeulen opened this issue Jun 13, 2014 · 5 comments
Open

Privacy issues with SessionIndex implementation #41

pmeulen opened this issue Jun 13, 2014 · 5 comments
Assignees

Comments

@pmeulen
Copy link
Member

pmeulen commented Jun 13, 2014

An IdP may set a SessionIndex attribute on the AuthnStatement element in an Assertion. From the spec:
"SessionIndex [Optional]
Specifies the index of a particular session between the principal identified by the subject and the authenticating authority."

The current implementation passes the SessionIndex from the authenticating IdP through the proxy to the SP: https://github.com/OpenConext/OpenConext-engineblock/blob/master/library/EngineBlock/Corto/ProxyServer.php#L586

This is a privacy issue because this allows different SP to correlate users, defeating persistent and transient NameID mechanisms.

The SessionIndex can be used for specifying a particular session in a logout. SAML Logout is not currently supported by engine.

Implementation options:

  • Do no pass SessionIndex. It is optional. This might cause a problem for an SP that expect this value.
  • Generate a new random SessionIndex per SP. Keep this session in the session of the user in engineblock.
@pmeulen pmeulen added the bug label Jun 13, 2014
@thijskh
Copy link
Member

thijskh commented Sep 18, 2024

We occasionally see an issue with SPs failing when they do not get a SessionIndex from us (even though logout is not supported, they require it a priori unconditionally). This happens when the IdP does not send a SessionIndex.

We could tackle this and the above issue by always generating a random SessionIndex whether or not it was present before. It makes sense because SessionIndex from the IdP alone is not relevant or useful since we will always override the corresponding NameID and we do not support any form of logout.

@tvdijen
Copy link
Contributor

tvdijen commented Sep 18, 2024

We've also seen cases with SPs failing when the SessionIndex is missing.
The SAML core-specs (page 27) recommend to use the Assertion ID for it.

@baszoetekouw
Copy link
Member

That sounds like a nice solution then: simply add (or override) the value we send to the SP by setting it to the Assertion ID.

@baszoetekouw baszoetekouw removed their assignment Dec 11, 2024
@baszoetekouw baszoetekouw moved this to Backlog in PHP development Jan 8, 2025
@baszoetekouw baszoetekouw moved this from Backlog to New in PHP development Jan 22, 2025
@baszoetekouw baszoetekouw moved this from New to Backlog in PHP development Jan 22, 2025
@baszoetekouw baszoetekouw removed the bug label Jan 22, 2025
@baszoetekouw baszoetekouw moved this from Backlog to New in PHP development Jan 22, 2025
@baszoetekouw baszoetekouw moved this from New to Backlog in PHP development Jan 22, 2025
@baszoetekouw
Copy link
Member

Dus concreet: EB moet in alle SAMLResponses in the AuthnStatement het attribuut SessionIndex opnemen met als waarde dezelfde waarde als de ID van de bovenliggende Assertion.

Het wordt dus:

<samlp:Response>
  <saml:Assertion ID="CORTO123[...]abc">
    <saml:AuthnStatement SessionIndex="CORTO123[...]abc">

ongeacht de SessionIndex die EB van de upstream IdP ontvangt.

@tvdijen
Copy link
Contributor

tvdijen commented Jan 22, 2025

For future reference: the SP that had this issue was using the OIOSAML library, thinking that it was a standard SAML2-library, but after digging a bit more it appeared to be some Danish profile that lays additional requirements on top of SAML2 (of which one was a MUST for SessionIndex)

@pablothedude pablothedude moved this from Backlog to In Progress in PHP development Jan 28, 2025
@pablothedude pablothedude self-assigned this Jan 28, 2025
pablothedude added a commit that referenced this issue Jan 28, 2025
This was a privacy issue because this allows different SP to correlate
users, defeating persistent and transient NameID mechanisms.
#41
pablothedude added a commit that referenced this issue Jan 28, 2025
This was a privacy issue because this allows different SP to correlate
users, defeating persistent and transient NameID mechanisms.
#41
@pablothedude pablothedude moved this from In Progress to Delivered in PHP development Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Delivered
Development

No branches or pull requests

6 participants