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

Added README, token generation, error handler, routes for all required use cases #1

Merged
merged 3 commits into from
Nov 21, 2022

Conversation

PaulaSazonov
Copy link
Owner

Added README, routes now handling all required use cases, added token-generating and error handler

@srd90
Copy link

srd90 commented Nov 20, 2022

About these:

842a083#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R44
... The latter case is to terminate the authentication session within suomi.fi, if a logout is initiated by an another SP. The authentication session within suomi.fi is valid for 32 minutes, during that time you could authenticate to multiple services that use suomi.fi (Vero, Kela, etc.), and this just means that if the authentication session is terminated in one service, it is terminated in all of them.

and

842a083#diff-336525865815cfe527e7b7352690bb5ad65c119c04c0023919ca9ae9878e2af3R104

/**
* IdP notifies logout, possible cases:
* 1: SP requested logout, IdP confirms logout.
* 2: IdP request logout after receive logout request from another SP that shared a session.
*/
router.get(
'/SAML2/SLO/REDIRECT',
(req, res, next) => {
passport.authenticate('saml', () => {
next()
})(req, res, next)
},
(req, res) => req.logout(() => res.redirect('/login')),
)

This reference implementation's IdP initiated Logout handling (i.e. case "2: IdP request logout after receive logout request from another SP that shared a session. ") doesn't work properly at all circumstances and whats worse it reports success even if it fails to terminate local session (as matter of fact it reports always success regardless of success/failure of local session termination).
This means that user's local session to service which uses this reference implementation might not be terminated even though it was reported to user that all sessions started during SSO were terminated successfully.
Next person who starts to use browser (in schools, libraries, any shared computers, etc.) and writes to address bar a service which uses this reference implementation might see previous user's information if local session has not timed out. Local session's timeout might be significantly longer than IdP's SSO session TTL and local session's TTL is not bound to SSO session's TTL (even if it would be there would still be 0-32min to "reuse" previous user's local session).

Further information available from:

  1. IdP-initiated SLO node-saml/passport-saml#221 (comment)
    • on of the comments of aforementioned issue
  2. passport-saml's IdP initiated LogoutRequest handling doesn't always close sessions node-saml/passport-saml#419

There are two publicly available implementations which try to handle situation correctly but those implementations are very application specific (as of now it is impossible to implement IdP initiated SLO properly with passport-saml or with node-saml so that one implementation would work in all SW stacks / applications). Those publicly available implementations which might implement it correctly (and which might be used as example how to address IdP initated SLO problem) are:

fwiw this problem was discussed previously at your this issue report: node-saml/passport-saml#445

@PaulaSazonov
Copy link
Owner Author

Thank you @srd90 for the input! That was a very good point, I added a logout handler to terminate the local session as well.

@srd90
Copy link

srd90 commented Nov 20, 2022

I am sorry to inform you that 7372ab8 did not address or fix issue described at previous comment at all.

Please, take a good look at all the material linked to this PR's previous comment.

Hint: issue is related to the fact that 3rd party (session) cookies are not being delivered in modern browser under all circumstances (e.g. when IdP which lives another top level domain is propagating SLO requests from iframes). When there is not any session cookie available there cannot be any authenticated reg.session associated by underlying session management subsystem to be destroyed and certainly not the authenticated session which was designated by information in IdP initiated logoutrequest SAML XML message ( ... some SW stacks might have been configured to create new local session implicitly if there aren't any due lack of session cookie etc etc but that is another session).

Under such conditions (not being 100% sure that correct local session was terminated) SAML service provider must not report successfull local session termination. If SAML service provided is not 100% sure it must report with some other response code.

This was last attempt to comment this particular codebase.

@PaulaSazonov
Copy link
Owner Author

Yes, I understand your point. This codebase was to serve as a reference to myself, and it is not a full implementation (the proper cache for the sessions is not implemented at all). I will try to build on this if I have the time, but for now it is more of a hobby project.

@PaulaSazonov PaulaSazonov merged commit 4aa8542 into main Nov 21, 2022
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.

2 participants