MultiSamlStrategy and InResponseTo are Incompatible by Default

[gluxon]

For every HTTP request, MultiSamlStrategy creates a brand new SAML object. This in turn creates a new InMemoryCacheProvider.

Meaning by default, the InResponseTo setting will always cause the middleware to fail with "InResponseTo is not valid". Cache keys will never persist beyond the life time of a single HTTP request unless a custom cacheProvider is supplied.

I don't think the usage of two officially supported parts of the library should silently break. There are a 2 ways to fix this (that I see):

1. Add an error if a custom cacheProvider isn't supplied by the user when using the MultiSamlStrategy. Forcing users to reconfigure this library with an external cache store would workaround this issue.
2. Have MultiSamlStrategy hold onto its ownInMemoryCacheProvider objects by default that it passes to new SAML instances. To do this securely, it would need to persist caches for each identity provider.

I lean towards 2, but unfortunately the current API design doesn't require each identity provider to be uniquely keyed. The endpoint SAML option gets close, but that can change with binding methods.

Edit: To be clear, I'm just hoping to open a discussion on how to best resolve this. I'm not demanding it to be fixed or trying to force a solution. I may be able to submit a pull request once we agree on something.

[gluxon]

Actually, I forgot that the optional idpIssuer parameter existed. That would satisfy the requirement for uniquely identifying IdPs.

[stavros-wb]

@gluxon nice issue description. Would it be a problem to implement 2 without partitioning the cache per provider?

[gluxon]

I'm not sure how big of a problem it would be in practice, but it would allow a malicious identity provider to respond to an authentication request made to a different identity provider (assuming SAML response signing is off). If response signing is enforced, then not partitioning the cache should be fine.

@stavros-wb I just realized that you wrote MultiSamlStrategy. Thanks for that! It's been very useful to me.

[stavros-wb]

another option would be to hash the provider's data. given that the values don't change, it could be a poor man's id @markstos wdyt?

[stavros-wb]

@gluxon can you try with the latest master?

[gluxon]

@stavros-wb Sorry for the late response, but this works! Thank you!

[crossan007]

It seems like 3.2.0 no longer defaults to InMemoryCacheProvider for MultiSamlStrategy (possibly as of 224f25f).

Any ideas if this was an intentional change?

I am still able to explicitly define a InMemoryCacheProvider in my initialization options as cacheProvider

i.e.

import { CacheProvider } from "passport-saml/lib/node-saml/inmemory-cache-provider";

...
const SAMLCacheProvider = new CacheProvider({
  keyExpirationPeriodMs: 5000
});
...

passport.use(new MultiSamlStrategy({
  getSamlOptions: getSamlOptions,
  maxAssertionAgeMs: 5000,
  acceptedClockSkewMs: 5000,
  validateInResponseTo: true,
  wantAssertionsSigned: true,
  audience: appVars.saml.SPEntityId,
  signatureAlgorithm: "sha256",
  digestAlgorithm: "sha256",
  cacheProvider: SAMLCacheProvider
},
...

[cjbarth]

What is the last version that worked as you expected @crossan007 ?

[crossan007]

I'm not actually sure.

I'm working on a new project and attempting to use the library.

I was searching through issues when I found the addition of the default cache provider for multi saml, and then dug through the commit history to locate where the default cache provider was removed when I realized what was added doesn't seem to work

[cjbarth]

If my memory serves me correctly, this was removed because we want consumers to be explicit about which cache provider they use. Feel free to specify an InMemoryCacheProvider yourself during construction. I see that this change was made for the 3.0.0 release. It is possible that some documentation is out of date. Please feel free to create a PR to fix any documentation.