search

tngan / samlify

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

Expose request/response ID #50

Closed sebakerckhof closed 7 years ago

sebakerckhof commented 7 years ago

On single page applications (e.g. if we want this: https://github.com/tngan/express-saml2/issues/46 ) it is common to do the single sign on in a pop-up window, so the user doesn't leave the SPA and the page doesn't refresh when the user is logged in.

The way this works is usually pretty hacky, but afraid this is the common way of doing this: 1) Create a unique ID on the client 2) Open a pop-up window requesting the login page, passing the unique ID (e.g. as a query variable) + have an interval checking if the pop-up is still open. 3) The login page stores the unique ID together with the SAML request ID 4) Normal SAML-authentication flow to the IDP and back. 5) When the assertion consumer URL receives a login response. It matches the SAML response ID with the request ID and unique ID (saved in step 3) and adds the login response data. 6) Close popup 7) Client notifies the pop-up is closed 8) Client asks the server to log in with the unique ID generated in step 1 9) Server logs in the client and returns user info to the client

To make this flow possible we need sendLoginRequest and parseLoginResponse to return respectively the request ID and response ID.

Unless @tngan you know a better way to make the popup flow work.

sebakerckhof commented 7 years ago

@tngan Is this still something that can go in v2 ?

tngan commented 7 years ago

@sebakerckhof Not really, i prefer to hold it first, i am now working on the vue.js application, I will release some parts by the end of next week. During the meantime, would you like to take a look about the inbound saml in Okta if you have time ? :)

I think we can start discussing it after the new example application is done.

sebakerckhof commented 7 years ago

I could take a look. However, why I think this might be important to get in v2 is that it's a potentially API breaking change for a quite common use case. So that would mean if it's not in 2.0 it would have to wait until the next major release...

tngan commented 7 years ago

@sebakerckhof Yes, it makes sense, I will take a look into it very soon and keep you posted.

sebakerckhof commented 7 years ago

I guess we could do 2 things here: 1) Extract and return the ID generated by the createLoginResponse/createLoginRequest functions OR 2) we could let the user pass an ID in those functions (instead of the generateID functions of the entity. What do you think would be best?

sebakerckhof commented 7 years ago

@tngan I finally have some time again to work on this. I'd like to tackle this issue, do you have a preference (or alternative) regarding the options listed above?

tngan commented 7 years ago

I think passed-in function is good enough and right now the default ID is generated independent with time (uuid, v4).

sebakerckhof commented 7 years ago

Can you explain how the use case described in the first post would bd able to achieve this way?

sebakerckhof commented 7 years ago

Because this has nothing to do with security of the ID or which algorithm it uses. It's just that the callee needs to know the ID in order to link them to be able to support the popup-login use case.

tngan commented 7 years ago

The current design of the application driven by vue.js is to separate the front and backend. The workflow of client-side SSO is somehow like:

  1. User clicks the button in SP, acutally it might not be a popup window, it can be a dialog embedded in SP, forwarding an ajax request with login request to the IdP, put the IdP login panel directly in SP as well.
  2. Once IdP verifies and response a json that allows SP to wait for the state change.
  3. At the same time, IdP sends POST/ signed reponse back to SP
  4. SP parses and verifies the reponse back to IdP and change the state in IdP.
  5. Once the response is parsed and verified successfully, SP will get notified and grant the user to access, the question is how SP get notified, in fact, SP frontend can keep tracking with a session ID (given by IdP after the login verification) if any user is being authenticated in that session by firing request regularly.

Just an idea, feel free to discuss.

sebakerckhof commented 7 years ago

About step 1: It has to be a popup, since a lot of SAML login pages would have the 'x-frame-options:SAMEORIGIN' HTTP header, and rightfully so (with oAuth2 that's even a requirement https://tools.ietf.org/html/draft-ietf-oauth-v2-23#section-10.13 ) which would not even allow them to be shown in a dialog.

About step 2: you lost me there. Where is this json coming from?

tngan commented 7 years ago

Step 1. Nice, I have no doubt to use a popup right now, it seems to be a common practice after I have done some researches.

Step 2. It's just a response from the IdP side.

After I rethink the process, the following includes some modifications based on your initial proposed solution.

sebakerckhof commented 7 years ago

Okay, That flow seems good, but when you say SP called an API to get a pre-session ID (psID). Is that something that's widely supported by Idp's? It's the first time I hear of such api.

tngan commented 7 years ago

I don't think so, it's just a workaround. Generate an ID from the IdP is better than allowing user to generate their own ID, I think that ID should have a specific format instead of an arbitrary pattern, and it's much easier to do tracking, because IdP cannot just wait for the SP to send a self-generated ID and always accept a random ID from an unknown SP before SAML protocol is actually carried out.

sebakerckhof commented 7 years ago

Okay, I'm confused. We are talking about SP-initiated login, right? And the ID is this one: https://github.com/tngan/express-saml2/blob/2.0.0-beta/src/binding-redirect.ts#L72 right? If you say user, do you mean the developer using our library or somebody who wants to log in? If I understand correctly, this would mean every IDP you want to integrate with has to implement this?

tngan commented 7 years ago

I wonder you are talking about the unique ID in step 1, isn't it ? The generateID function is used to customize the SAML request ID format, and that's it. That unique ID in step 1 is not related to the SAML workflow, it's just a workaround to handle SSO in SPA since all SAML workflow will be done inside the popup window, and we have to find a way to acknowledge SP the session is ready once the popup window is closed, that why that unique ID exists.

I am talking about the method to generate that unique ID, not the SAML's one.

Each SAML's response has an attribute called InResponseTo, specifying which request this response is associated with.

sebakerckhof commented 7 years ago

No, I'm talking about the ID in step 3 (in the opening post here).

So there are 2 ID's generated: A) the one in step 1 (of the opening post) is generated on the client-side of the SP, this is, like you state, just to check if the popup closes if the login has succeeded. This is never send to the IDP and is not part of the SAML flow.

B) the one in step 3 (of the opening post), this is the one send from the SP to the IDP. It is generated on the backend of the SP and is matched with the one from step 1. To do this matching, we need to know what this ID is. Currently this is not possible with the library and that's what I want to change.

It is this one: https://github.com/tngan/express-saml2/blob/2.0.0-beta/src/binding-redirect.ts#L72 When the IDP posts to the assertion consumer url, this ID is set as 'response id' in the saml response from the IDP to the SP. If the client-side later asks (when the popup is closed) if the login has succeeded, we can answer.

From what I understand you wan to ask this ID from step 3 to the IDP using an API no IDP supports?

tngan commented 7 years ago

I get your point, thanks for clarification, some follow-up questions:

For the matching part

To expose the generated ID and response ID, it's not that hard to do that and not related to the generateID itself, just add the ID into callback, and you are right, it will not be backward compatible since the return context of sendLoginRequest will get changed in redirect binding (the return context of callback in POST binding is now an object as well), I will include it in before releasing v2.

For the generation of that unique ID

sebakerckhof commented 7 years ago

Is the unique ID generated from front to back, say when open the popup, passed it in and pair up with the exposed request ID ?

Yes, so the popup would open the login route of the SP (let's say /spinitsso-redirect, like in the current examples) and the unique id would be passed with it, for example as a query variable

That unique ID should be kept tracked and have expired time, otherwise, I can always use a random unique ID and I know the format and send request to backend and try to find whether there is a match of somebody.

Yes, 3 things are important for handling these unique ids securely:

  1. They should have enough entropy, to avoid brute force attacks
  2. The unique id should be removed after the client asks for the login result (in step 8, so after the popup closes). So they can not be re-used by somebody else. This makes a brute-force attack window very small (only between step 5 and 8), which is like one second. Together with property 1 (enough entropy) this makes brute force attacks not feasible.
  3. A unique id should have a lifetime, so that if the user never gets to step 8 (e.g. his computer crashes in between), those unique ids are cleaned up.

However, all of these things are up to the user of this library. This library should not deal with this, since in real life scenarios, you want to save the id's to a database and not in-memory. Because if you want to scale to multiple servers, you're not sure the assertion consumer url will be called from the same server that created the login request.

However, the examples you create should make sure to pass all checks above, so that users of this library know about these details.

vbakke commented 7 years ago

Just a side comment: The IDs cannot start with a digit!

At least https://www.samltool.com/validate_xml.php invalidates the ID if it does, of a few forum posts mention this (e.g. https://stackoverflow.com/questions/5076675/saml-authentication-request-protocol-id)

sebakerckhof commented 7 years ago

Good catch, we'll probably want to change this when we tackle this issue.

sebakerckhof commented 7 years ago

@tngan Just to have a better starting point for further discussion I created this branch: https://github.com/tngan/express-saml2/commits/feat/expose-ids

With a very quick & dirty commit to show what would be needed for logins. Absolutely open to discuss this, but I'm ready to implement SAML SP in my SPA and I'm going to do something quick & dirty until a proper fix lands. Good news is we have a large customer base (mainly universities) using a large variety of IDP implementations I can now test with.

tngan commented 7 years ago

@sebakerckhof Please checkout 5945df4. That id could be already exposed in request/response in both login and logout with redirect or post binding.

tngan commented 7 years ago

I really want to separate the example revamp from the release task list, and move the example folder as a standalone project, it should not be the blocker of the module itself.

Or we can create an organization for this module as well and release example there.

In fact, v1 should be deprecated very soon.

vbakke commented 7 years ago

I totally see your point of not wanting unfinished examples to hold back a release. But then, I chose express-saml2 over other options, based on the fact that it also contained examples.

It might be worth considering.

From a technical point of view, the examples are just bothersome for the the development. But from a psychological point of view for new users, working examples are invaluable. :)

tngan commented 7 years ago

@vbakke Thanks for your support and feedback, working examples and clear documentation are very important in order to help people getting started. I think we are not going to remove those examples, but I would like to move them into other repositories instead.

tngan commented 7 years ago

@sebakerckhof I will close this thread since the commit is already pushed, open a new issue if you have questions related to the usage of expose id.

sebakerckhof commented 7 years ago

Nope, that commit seems to be exactly what was needed. thanks.