tngan / samlify

Node.js library for SAML SSO
https://samlify.js.org
MIT License
609 stars 217 forks source link

Handling multiple IdPs with parseLoginResponse #357

Open tjunnone opened 4 years ago

tjunnone commented 4 years ago

Let's say you have several possible IdP configurations stored in a database, and a single ACS/assert route handler (e.g. you don't create a unique ACS URL per IdP, which seems to be discouraged).

serviceProvider.parseLoginResponse takes the IdP as the first parameter, seemingly creating a chicken-and-egg situation when you have lots of possible IdPs: To parse the response you need to have the IdP instance, but to dynamically identify and instantiate an IdP at runtime you need the response content.

I think the way to dynamically handle IdPs at runtime is that you'd want to have a look at the extracted "issuer" value in the ACS route handler, prior to validating & parsing the response, and construct the correct IdP instance. Would something like serviceProvider.peekLoginResponse(binding, request) make sense as an API addition? It would do the same thing as the existing function, but without any validation or signature checking. I managed to re-use the Samlify Extractor class to peek the response and get the issuer ID, but it doesn't seem to be public API and I worry it might break.

tngan commented 4 years ago

@tjunnone You can create a pool of IdPs when your service starts, by getting unique IdP list from your database.

Yes, create an ACS URL per idp which is not practically feasible, but what if you create a single ACS URL with a query parameters. You can see the implementation in

https://github.com/authenio/react-samlify/blob/4132d5677cddce25b075cc718dbc100e0ed9bf8c/middleware/index.ts#L11-L42

which is a middleware, by assigning corresponding idp and piping through the controller based on the query parameters.

tjunnone commented 4 years ago

Thank you for the response!

Consider an online service where users can upload and configure their own IdP metadata files (pretty common in SaaS apps):

I did a bit of research of what other frameworks do. In passport-saml, they provide a MultiSamlStrategy that works by allowing you to map the incoming ACS response to a SAML configuration (pick an IdP dynamically).

I think the following pattern would be even easier than adding new concepts like strategies or making the ACS URL carry additional information (being able to use the same SP metadata for everyone is a big plus in my opinion):

(adopted from https://github.com/authenio/react-samlify/blob/master/server.ts)

  // assertion consumer service endpoint (post-binding)
  app.post('/sp/acs', async (req, res) => {
    try {
      const issuer = await req.sp.getLoginResponseIssuer('post', req);
      const idp = await mapIssuerToIDP(issuer); // App specific logic of finding the right IDP
      const { extract } = await req.sp.parseLoginResponse(idp, 'post', req);
     ...
  });

basically, by introducing one method to extract the IdP issuer ID (only, to prevent accidental use of unvalidated SAML response), the correct IdP can be selected in any way the server wants at runtime, from a file, from a database, etc. Thoughts? Would you be open to a PR that does this?

tngan commented 4 years ago

@tjunnone Using issuer as key makes sense, I am open to this feature.

In OpenAM, there is something called circle of trust, it's a pool of entities (SPs and IDPs) that provides another layer of protection. It is also using issuer as key, provide a function getting essential information before parsing process would be easier to do some early stage checking.

Let's confirm the implementation spec before you start. It should be a generic function that can get any information from raw response.

Function Signature: getRawResponseProps(binding, request, fields)

I think we can reuse the extractor module, the API is not public right now, you don't need to expose the extractor module, but you can expose the fields as the last arguments.

Here is an example

https://github.com/tngan/samlify/blob/3e63a3ae03ade0e077747ae04ac5152cc469d2fb/test/extractor.ts#L75-L88

So, you could do something like

getRawRepsonseProps(binding, request, fields) {
  // implement the decode, inflate based on different bindings and get the raw response 
  // rawResponse is just a string
  return extract(rawResponse, fields);
}

// example
getRawRepsonseProps('post', req, [
  {
    key: 'issuer',
    localPath: ['Response', 'Issuer'],
    attributes: []
  }
]);

The extractor function is originally a helper function I created for internal use only. If we want to make the field configuration public, I think we may need a documentation on the extractor module.

I foresee it might introduce some code refactoring as well, but that can be done right after.

nflaig commented 4 years ago

This addition would certainly give developers more freedom on how to implement a multi idp setup. The solution I used was to have unique URLs per Idp by suffixing the URL with the idp ID. This also makes it easy to know which idp to usein case of SP initiated SSO.

@tjunnone why is having unique acs url per idp discouraged? I dont see a disadvantage in that. Also having a single SP metadata file could be nice but in my case it is possible to configure IdPs differently which changes the metadata as well.

tmonck commented 3 years ago

Just curious of the status of this feature. We are using this library and we would need this feature to get the issuer so we can look up the IDP.

tmonck commented 3 years ago

Just checking in post the new year to see if there is anything on this.

morexlt commented 1 year ago

Hey! I want to check if this was implemented. Because I have the same problem that @tmonck mention, I need to parse the SAMLReponse in order to get some information to get the idp metadata from the db