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

Support multiple callback urls #316

Closed
wants to merge 1 commit into from
Closed

Support multiple callback urls #316

wants to merge 1 commit into from

Conversation

dertieran
Copy link

@dertieran dertieran commented Aug 16, 2023

Description

Closes #315

This PR adds support for passing multiple callback urls to generate multiple AssertionConsumerService properties.

Right now this is done by supporting passing an string[] instead of just a string for the callbackUrl.

Not sure if this is the cleanest solution or if this should be done through another option, something more like additionalCallbackUrls.

Checklist:

  • Issue Addressed
  • Link to SAML spec
  • Tests included?
  • Documentation updated?

@markstos
Copy link
Contributor

Could you update the documentation as well?

@srd90
Copy link

srd90 commented Aug 17, 2023

This comment is more like proactive warning for anyone who spots this feature from changelog and start to utilize it.

node-saml (and passprot-saml) supports at the moment implementation of SAML 2.0 Web SSO profile. Web SSO profile defines 2 bindings for ACS endpoint (HTTP POST and HTTP Artifact, see SAML 2.0 Profiles spec lines 421-425 and 483-502).

Furthermore node-saml (and passport-saml) supports at the moment only HTTP POST binding for ACS.

"Normally" one would use multiple AssertionConsumerService elements to list endpoints for different bindings (SAML 2.0 Web SSO profile is a bit unclear about this i.e. it can also be interpreted so that different domains are allowed. See SAML 2.0 Profile spec lines 629-633).

Since node-saml (and passport-saml) support only one binding (HTTP POST) for ACS the only use case for this PR's feature would be to introduce endpoints with different domain names (and new testcase provided by this PR indicates that author of PR is going to use this to list ACS behing different domain names).

So far so good (spec seems to allow that), but...

Consider long and hard before starting to list ACS for different domain names IF you must support also single logout.

At the moment node-saml (and passport-saml) implementation allows listing only one SingleLogoutService endpoint (and binding is fixed to HTTP POST) to metadata generated by node-saml's metadata generator. Spec allows multiple SingleLogoutService endpoints (see SAML 2.0 metadata spec lines 652-654 and SAML 2.0 profile spec lines 1311-1316) into SP metadata (for supported SLO callback bindings). When SP has triggered SLO and when IdP finally forwards user back to SP after SLO propagation has ended IdP must select proper callback address according to rules at SAML 2.0 Profiles spec lines 1282-1289. So IdP may select any of the listed endpoints. If one has defined multiple SLS endpoints to SP metadata with different domains there is no way to know that IdP forwards user agent into service that is at the same domain name as user was when user clicked logout button.

From IdP initiated SLO point of view IdP would select some of the SP's single logout service endpoints (see SAML 2.0 profile spec lines 1245-1252) to propagate SLO. From SAML 2.0 point of view it doesn't matter whether domain is different - think session cookies - because SAML 2.0 processing rules define that SP must be able to terminate session based on nameid and optional sessionindex that is provided in LogoutRequest message (without using any additional information e.g. from cookies, see SAML 2.0 Core spec SLO processing rules, lines 2589-2616) but at least one SP implementation - passport-saml - doesn't work properly see node-saml/passport-saml#419.

@srd90
Copy link

srd90 commented Aug 17, 2023

One more note (in addition to previous ones):
IF you must implement your SW so that one application is accessed via multiple domain names AND you must support also SLO you might want to introduce different SP metadata for different domains so that you could be sure that same domain name is used for SLO as is used for ACS. You might come up with the solution that you use exactly same node-saml (or passport-saml) instance at BE side but introduce manually different metadatas. If you end up doing this consider issuer verification AND situation when node-saml (and passport-saml) some day catch up with SAML 2.0 specification and introduce recipient verification (see node-saml/passport-saml#509 ticket is at passport-saml side due historical reasons, see also SAML 2.0 Profiles spec lines 574-577 and SAML 2.0 security considerations lines 754-756).

@cjbarth
Copy link
Collaborator

cjbarth commented Aug 18, 2023

introduce different SP metadata for different domains so that you could be sure that same domain name is used for SLO as is used for ACS

This is the correct solution. You simply pass difference configuration data to the routines that generate the metadata based on the domain the request for metadata is coming from and everything works like it should. This is the method I've used in the past.

@cjbarth
Copy link
Collaborator

cjbarth commented Aug 18, 2023

at least one SP implementation - passport-saml - doesn't work properly

@srd90 , I can't express enough how much I appreciate your involvement with this project. I've spent a little time reviewing our previous discussions on this matter, specifically your comment and I see that, while node-saml and passport-saml don't come out-of-the-box with support for SLO, they do support it. It is only broken if it is implemented incorrectly. In fact, due to the flexibility of these libraries, there is no way to support it natively OOTB. Do I understand all that correct?

If I'm misunderstanding things, please help me to see where my comprehension fails me. Otherwise, I think it is important that we make sure we're stating clearly, not that SLO is broken, but that it isn't supported OOTB, just like the README says. If the README is wrong in this sensitive area, we certainly want to correct it.

@dertieran
Copy link
Author

dertieran commented Aug 18, 2023

Not sure if it is better to discuss this here or in the issue.

@srd90 is right my use case would be that I have one endpoint to get the metadata of my SP which supports a domain wide login to different services, each on their own subdomain.
So e.g. https://example.com/saml/metadata to get the metadata and the user is able to log in from https://a.example.com, https://b.example.com and so on.

And most of the users configure their IdP manually anyway, so this normally isn't an issue.
But I had one user just took the metadata and added it to their IdP and tried to log in from https://b.example.com.

(I already had implemented, what you also suggested, that calling the metadata endpoint from a subdomain would adjust the callbackUrl to match that domain.)

But I thought it would be nice if the user could just take the metadata file and upload it to their IdP.

So e.g. in Azure AD you could just take the metadata.xml from the test and upload it.

image

I can see that this isn't the main usecase for node-saml and I wasn't really aware of the implications.
Maybe it would even be better to have this in a separate option (like additionalCallbackUrls) instead of overlaoding the callbackUrl.

But I can also see that this might be a niche usecase and if it complicates other things I would be also fine with dropping this PR.

@cjbarth
Copy link
Collaborator

cjbarth commented Aug 18, 2023

But I thought it would be nice if the user could just take the metadata file and upload it to their IdP.

They can, and that is what I've done. Just have the request for metadata generate the metadata different depending on the domain they hit it from. Or, just make the metadata request path something like https://example.com/saml/metadata/a and https://example.com/saml/metadata/b, etc.

@cjbarth
Copy link
Collaborator

cjbarth commented Sep 30, 2023

Is this still an open issue based on the feedback provided @dertieran

@dertieran
Copy link
Author

@cjbarth sorry for the delayed response.
Using different endpoints/domains for the metadata is what I already implemented.
But the user then has multiple metadata files and would need to create multiple "applications" in e.g. Azure AD.
With the single metadata file they could just take it and upload it, as shown in the screenshot.

But if this isn't completely spec compliant or produces other issues I'm also fine to close this PR/Issue.
Because theoretically this is just relevant for the metadata for me, and I could just generate them without this library.

@cjbarth
Copy link
Collaborator

cjbarth commented Oct 8, 2023

@dertieran Are you saying that all those domains would point to the same session store, the same application? i.e. if a SLO request were to hit any of the domains, it would behave just as if it hit any of the other domains?

@dertieran
Copy link
Author

Yes, at least for me, these are the same servers, but I don't think they would have to be.

Essentially I have a service where the user has one account (and one SSO configuration), but can use different services (depending on the subdomain).
So a user might need to konfigure all subdomains/services in one "application" in the IdP so it will have the same configuration.

But again, feel free to close this if this seems to be a niche use case.
And the important part (logging in the user) works as expected, I just wanted to give the user a better metadata.xml 😃

@cjbarth
Copy link
Collaborator

cjbarth commented Oct 16, 2023

This is probably why subdomains for shared services are no longer the industry standard for sites sharing a common security context. I might suggest that you have one login domain and allow that token to be used for the other domains if you can't combine the sites. In reality, the biggest reason to use different subdomains is to separate security contexts. What your after is the opposite of that, so I'd say it is niche, so I'll close this.

e.g. browse to https://maps.google.com/ and note that you are redirected to https://www.google.com/maps

@cjbarth cjbarth closed this Oct 16, 2023
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.

[ENHANCE] Support multiple AssertionConsumerService properties in metadata
4 participants