-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
RelyingPartyRegistrations should not fail when SPSSODescriptor elements are present #12664
Comments
Hi, @stnor, thanks for the detailed report. Looks like there are a couple of things here.
I've added #12667 to address the second one. As for the first one, yes, I think that makes sense to add and am happy to use this ticket to address that.
It is in our Wiki: https://github.com/spring-projects/spring-security/wiki/SAML-2.0-Migration-Guide - note that the main page didn't have it in the bullet list, so I've added it there to make it easier to see. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
I think I might be asking for the wrong thing here after looking at #10551 ? @jzheaux To be clear, I want to participate in the Federation mentioned above as an SP and let the user select what IdP to use for authenticating with my service. Shouldn't I have just one RelyingParty, with the SAME callback url for all IdP:s. I can only publish one URL in the federation, and it can't obviously be IdP specific then. Is this possible with 'new' SAML at present? If so, how to go about it? |
@stnor, yes this is possible. The key is to reuse the same relying party configuration for each relying party registration instance. Note that by default, the callback URLs will contain a unique registration id so that Spring Security can lookup the right IdP configuration. If you want to have the exact same callback URL for all IdPs, then you will need to specify how to perform the lookup. |
Are you able to provide a PR for improving |
I was unsure in which branch you wanted the PR for... The PR is for main, but I am running 5.8. |
It's not really about wanting to have one url in a federated setup. I can only publish one SP entry to the federation metadata catalog, so that's the ONLY way of doing it :) I think it's a shame that the project doesn't provide a more backwards compatible way of migrating from the old project, like a alternative resolver with the same URL for all, but I will take a look at this and see if I can solve it on my own. Another issue is that is the federation I am in, many of the entityIds are URLs, which you can't have in the non-query string part of the url. The old SAML extension used |
Adding what I've got so far. Managed to get both /saml/login?idp=foo and /saml/SSO to work I think. Using OpenSAML 4.
|
Is there support for discovery?
Today I point the login button to /saml/login which, if logged in, gets the user signed on without Idp selection. Otherwise to idp selection (in a SPA). Using saml2 I'm getting a 404 unless I specify an RP using my |
I ended up with this to fix the 404:
and
In what cases does the .loginPage do anything? I can't figure out when it makes a difference. @jzheaux Any feedback appreciated. Also please take a look at the PR. |
@stnor I appreciate you digging in here. The more use cases the better. That said, I want to make sure that this ticket focuses on one change, and we can add other tickets for other enhancements and bug fixes. For now, let me try and address each question one at a time, and I'll clarify if it sounds to me like it needs another ticket.
Spring Security uses the merge forward strategy. Since this is a bug, it should be on the earliest affected branch, which would be
Understood. I could have worded my comment better. I understand that it's not cosmetic in this case.
Progress is ongoing, and I think that it may be helpful to discuss this further on another ticket. I've reopened #10243 and you can see if that appears helpful to what you are trying to do. Also, you might be interested in the SAML migration sample to assist with migrating extension projects. That samples uses Finally, in many cases, a static URL is sufficient since the
While the registration id defaults to the entity id in RelyingPartyRegistrations.fromMetadata(...).registrationId("id").build(); Indeed, it's expected that those using the default URL strategy will need to. Currently, the JavaDoc states: * Note that by default the registrationId is set to be the given metadata location,
* but this will most often not be sufficient. To complete the configuration, most
* applications will also need to provide a registrationId, like so:
*
* <pre>
* Iterable<RelyingPartyRegistration> registrations = RelyingPartyRegistrations
* .collectionFromMetadataLocation(location).iterator();
* RelyingPartyRegistration one = registrations.next().registrationId("one").build();
* RelyingPartyRegistration two = registrations.next().registrationId("two").build();
* return new InMemoryRelyingPartyRegistrationRepository(one, two);
* </pre> Given your feedback here, it seems to me that the JavaDoc and reference should be clearer on why this is necessary. I've opened #12764 to address that.
I think the easiest way for me to comment on your project is to have a GitHub sample. I'd be happy to provide a PR that simplifies what you've got so far, including providing feedback on the 404, etc. From this point on in the ticket, I'd recommend that we keep our discussion to the issue in the description. Please feel free to open additional tickets to discuss additional topics. |
Thanks @jzheaux! |
I cannot tell you how much I appreciate this. See https://github.com/stnor/fed-saml-example/issues/1 |
@jzheaux is there any plan to release this fix as part of the 5.8.x version? What would be the way to get it into the next milestone? (https://github.com/spring-projects/spring-security/milestone/285) |
@lasselindqvist, that was added in |
I've created #13054 now to clarify that. |
I am trying to migrate from the old SAML extension project to the new. on Spring 5.8.x (not boot).
It would be good if i could use
RelyingPartyRegistrations.collectionFromMetadataLocation()
could skip "SP" entries instead of throwing exceptions.Right now I am getting
org.springframework.security.saml2.Saml2Exception: Metadata response is missing the necessary IDPSSODescriptor element
Ideally there should be a flag to skip entities without IDPSSODescriptor. In this federation, there are
SPSSODescriptor
:s mixed in the same metadata as the IdP:s in this case.See
https://fed.skolfederation.se/prod/md/skolfederation-3_1.xml
(A Federation for school owners (IdP) ca 200+ and e-learning resources (SPs) in Sweden).Since the classes are package private and final, it is hard to work around the issue at present.
The only possible workaround seems to be to copy classes..
Also, how does one parse and store the other metadata, that was read by the old implementation, such as "organisation.name" when
RelyingPartyRegistration
is final and there are no hooks in the code afaik. Couldn't it be an interface instead? Or expose the XMLObject?I have a dropdown list to select the IdP by OrgName in my implementation today, that's using the old project.
I'm unable to find a migration guide, and the docs are pretty sparse.
Thanks.
The text was updated successfully, but these errors were encountered: