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

[EVAKA-HOTFIX] Support Single Logout w/o 3rd party cookies #738

Merged
merged 20 commits into from
May 12, 2021

Conversation

mikkopiu
Copy link
Contributor

@mikkopiu mikkopiu commented Apr 22, 2021

Summary of SLO issue / vulnerability

  1. User logs into eVaka via IdP
  2. User logs into service B via same IdP SSO session
  3. User starts Single Logout from service B and IdP initiates SAML LogoutRequests via the User's browser
  4. User's browser blocks 3rd party cookies, i.e. eVaka's logout cookie/token
  5. eVaka incorrectly responds with SAML success message and User thinks they've logged out of eVaka but haven't <- the major security issue
  6. Another entity/person with access to the same computer can use User's eVaka session

Summary

  • apigw: support SLO without cookies, drop logout cookie
    • Other systems like Shibboleth get around the 3rd party cookie issue with Single Logout by not utilizing cookies at all for this but instead use the SAML nameID (and sessionIndex) properties presented in all SAML LogoutRequest messages
    • When logouts are always only done through SAML there's no need for the logout cookie itself but the idea is actually useful as an effective "secondary index" for Redis:
      • By storing a nameID + sessionIndex keyed item pointing to the session ID, we effectively create a second index that can be used with just the SAML LogoutRequest's nameID and sessionIndex properties
  • apigw: clarify logout start error logging
    • No SAML error will ever be present when starting the logout flow from the SP (eVaka itself) -> don't waste time attempting to parse SAML errors and clarify the error description
  • apigw: split SAML config creation from Strategy creation
    • Allows separate instation of the SAML class (from passport-saml) used for parsing SAML messages for SLO
    • NOTE: This would be entirely unnecessary if passport-saml either supported hooking into passport.authenticate() before it automatically redirects on SAMLRequest or if it had separate methods for first parsing the request and then doing everything else (redirects etc.)
  • apigw: don't use deprecated body-parser directly
    • Express still actually uses the same body-parser package/methods behind the scenes but as the library user we should use the official wrapper methods express.json() and express.urlencoded()
    • Options stay as-is as the implementations stay as-is
  • apigw: Extend GatewayTester to allow further cookie manipulation and non-dummy authentication
  • apigw: add test certificates for unit testing
  • apigw: add dependencies and features to test tooling for testing the new SAML functionality & mocking required parts of app setup and authentication
  • Add .editorconfig for apigw/ (matches frontend/.editorconfig)
  • apigw: enable @typescript/no-floating-promises rule for ESLint (can't forget to await a Promise, already enabled in frontend projects)
Testing instructions

On master version (e.g. staging):

  1. Go to https://staging.espoonvarhaiskasvatus.fi/ -> login with Suomi.fi (any user)
  2. Go to https://staging.espoositoavustukset.fi/nuoriso/ in the same browser session -> login with Suomi.fi -> will use the same IdP session
  3. Open dev tools for request logs
  4. Attempt to log out in https://staging.espoositoavustukset.fi/nuoriso/
  5. -> Suomi.fi will trigger Single Logout (SLO) for https://staging.espoonvarhaiskasvatus.fi/api/application/auth/saml/logout/callback (with a SAMLRequest query parameter)
  6. -> eVaka will respond with 302 with a SAMLResponse indicating success
  7. -> Suomi.fi shows "Sinut on jo kirjattu ulos seuraavista palveluista Espoon kaupungin varhaiskasvatus Kirjauduttu ulos"
  8. Go to eVaka tab and attempt to log out
  9. -> You will get an (ugly) JSON error response page with {"message":"Internal server error"} + an alert

On dev (once this branch is deployed):

  1. Repeat process
  2. -> you will be successfully logged out without any errors
  • Manually tested in staging against Suomi.fi
  • Manually tested in staging against Azure AD
  • Manually tested in staging against Keycloak
  • Manually tested in staging with mobile employee authentication

@mikkopiu mikkopiu force-pushed the fix/sfi-double-logout branch 6 times, most recently from 8d97975 to 39017b2 Compare April 28, 2021 10:27
@mikkopiu mikkopiu force-pushed the fix/sfi-double-logout branch 2 times, most recently from 83d6d6b to 5fe222c Compare April 30, 2021 09:50
@mikkopiu mikkopiu force-pushed the fix/sfi-double-logout branch 5 times, most recently from 3f36d94 to f20406e Compare May 10, 2021 14:26
req,
res,
config.sessionType,
profile?.nameID && createLogoutToken(profile.nameID, profile.sessionIndex)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing this with undefined should effectively simulate the situation in master when running the regression tests: if nothing is parsed from the SAML message, only cookies are used (and the tests don't currently care about SameSite in this context, so evaka.eugw.session will be available in the "IdP-initiated logout" test case).

@mikkopiu mikkopiu marked this pull request as ready for review May 10, 2021 14:35
- Express still actually uses the same body-parser package/methods behind the scenes but as the library user we should use the official wrapper methods `express.json()` and `express.urlencoded()`
- Options stay as-is as the implementations stay as-is
- Allows separate instation of the `SAML` class (from `passport-saml`) used for parsing/verifying SAML messages
- Will later be used to parse logout tokens from SAML LogoutRequest messages where cookies aren't available
- No SAML error will ever be present when starting the logout flow from the SP (eVaka itself) -> don't waste time attempting to parse SAML errors and clarify the error description
- Many browsers are starting to disable 3rd party cookies by default (even SameSite None) which breaks Single Logout with passport-saml that currently only supports logging out with a cookie
    - Browser details: https://webkit.org/blog/10218/full-third-party-cookie-blocking-and-more/ and https://blog.google/products/chrome/more-intuitive-privacy-and-security-controls-chrome/
    - passport-saml issues: node-saml/passport-saml#419 and node-saml/passport-saml#221
    - This is actually a real security issue as passport-saml will give a successful looking response from `passport.authenticate()` in the logout callback which results in IdPs showing "successfully logged out" messages to the user -- even though the user is still fully logged in!
    - This also causes an usability issue when the user tries to initate SLO from another application, fails to end the eVaka session, attempts a logout from eVaka and gets an ugly JSON error message as a response when APIGW attempts to make a LogoutRequest to the IdP that already ended the session -> will be fixed separately
- Other systems like Shibboleth get around the 3rd party cookie issue with Single Logout by not utilizing cookies at all for this but instead use the SAML nameID (and sessionIndex) properties presented in all SAML LogoutRequest messages
    - Source: https://wiki.shibboleth.net/confluence/display/DEV/IdP+SameSite+Testing#IdPSameSiteTesting-SameSiteandSingleLogout
- When logouts are always only done through SAML there's no need for the logout cookie itself but the idea is actually useful as an effective "secondary index" for Redis:
    - By storing a nameID + sessionIndex keyed item pointing to the session ID, we effectively create a second index that can be used with just the SAML LogoutRequest's nameID and sessionIndex properties
- Not relevant for e.g. mobile logouts where the request will never have a SAMLRequest query parameter or body -> make it optional
- Add debug logging for easier understanding of login/logout flow events
- Just for easier debugging and makes key collisions slightly less likely
- Instead of re-generating the token from session details (if no SAMLRequest Profile was available), use the logoutToken's value directly
- Trying to avoid having too much SAML-specific stuff in the session implementation
- Also unify code style for generating session and logout token Redis keys
- Instead of having anything specifically SAML-related in the session module, take the logout token in as just a string and move the parsing and nameID+sessionIndex logic into more SAML-specific modules
- This also allows dropping unnecessary arguments from all other routes than logout callback
    - But requires the login callback route to have logic for generating the token initially before saving it
- Disallow floating Promises to prevent unexpected order of operations in async functions where an async function is called but not awaited
- Already enabled in frontend projects
- Enables temporarily removing and re-adding the same cookies
    - Useful for simulating missing cookies e.g. in 3rd party SAML request cases
- Augment "tough-cookie" types to add missing `sameSiteContext` property for `setCookie` options object
    - Missing from `@types/tough-cookie@4.0.0` but implemented in library
    - Extremely relevant for 3rd party cookie testing
- Also return the actual Cookie instance from getCookie (all current usage just checks for truthyness/not-undefined which is still valid) -> enables new setCookie usage
- Match frontends config
- For test use only, named accordingly
- To allow testing cases where using the actual full Strategy instead of a dummy strategy is needed, allow overriding the `sfiMock` config with an argument that defaults to the config to not break current usage
- Allows using the tester with non-dummy passport strategies
…-mock

- Unfortunately redis-mock currently has an open issue (yeahoffline/redis-mock#135) and an un-merged fix for it (yeahoffline/redis-mock#178) that means it doesn't support multiple arguments (i.e. spread) for the `.del()` method -> to allow using the mock library in tests add support to AsyncRedisClient for using an array of keys instead of argument-per-key and always provide the keys to the underlying redis client as an array instead of using the spread operator
- As passport-saml and SAML authentication as a whole is very deeply integrated into the application, tests for it need to unfortunately set up its own instances of almost everything for the app in order to override important configs
    - Session store must use Redis as the Single Logout token system relies on it being available (instead of the default MemoryStore for local development) -> need to override client setup to use a in-memory mock-Redis
    - SAML configuration must use "real" certificates but obviously nothing from a real environment
    - SAML configuration needs additional adjustments to simplify testing
- As simulating real 3rd party cookie situations would be really complex, deleting and re-adding cookies is used to replicate the situation
    - Also implement a reference test case where cookies are available
- Add necessary XML + crypto dependencies for creating and parsing SAML messages
    - Although passport-saml has the SAML class (with missing official types...), it doesn't really provide useful methods/abstractions -- they go too far into request handling instead of just parsing and generating messages -- so decided to impelement these manually
- Based on the wonderful examples from node-saml/passport-saml#419
apigw/src/internal/app.ts Outdated Show resolved Hide resolved
- Axios eats the stack traces for failed requests -> explicitly expect specific response status codes outside of axios by always validating status to `true`
@mikkopiu mikkopiu requested a review from Gekkio May 11, 2021 13:58
- Globally mock redis with redis-mock in all Jest tests for APIGW
    - Allows removal of test-specific logic from apps regarding Redis clients
- Merge Suomi.fi SAML configs into one dynamic config that is once again controlled with `config.sfiMock`
    - No reason to duplicate when fallbacks and "if test then" logic can be integrated into the configs/constants
    - Still return an empty config object when Suomi.fi mocking is enabled (i.e. local dev, other tests than SAML tests) to prevent fallout from missing required props
- Utilize dynamic imports and brute force to temporarily disable Suomi.fi mocking for SAML tests ONLY
    - Not pretty but allows use of the actual enduser app instance (instead of faking it) and configs
    - Slightly less brittle against changes in the implementation
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