Skip to content

Allow RelyingPartyRegistration to be Deduced from Contents of SAML Assertion instead of Path #10243

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

Closed
Tracked by #12840
vince-recupito opened this issue Sep 8, 2021 · 7 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Milestone

Comments

@vince-recupito
Copy link

SAML Service Provider should use the contents of a SAML assertion to deduce the IDP instead of the current Path-based defaults when receiving an assertion. This will enable all IDPs to use a shared, common Service Provider configuration.

Currently, the default implementation provides each IDP a separate AssertionConsumerService url to call the Service Provider ("{baseUrl}/login/saml2/sso/{registrationId}"). As an example, with two IDPs registered, we could have the two urls:
{baseUrl}/login/saml2/sso/idp1
{baseUrl}/login/saml2/sso/idp2
Each IDP gets it's own Service Provider metadata that is specific to their IDP.

Instead, an optional implementation should be to have a static AssertionConsumerService url that all IDPs use, and use the entityId within the SAML assertion to choose the correct IDP to validate against. With the same two IDPs above, we would only have:
{baseUrl}/login/saml2/sso

To enable this approach we could have:
An "AssertionRelyingPartyRegistrationResolver" (in addition to the current, DefaultRelyingPartyRegistrationResolver) that would use the contents of the assertion (entityId) to resolve the correct relying party instead of the path-based route

Some work has already been done on this front to enable different RelyingPartyRegistrationResolvers with the following completed issues:
#8887
#8768

Why is this needed?

  • This is the biggest pain point I see from migrating from Spring-Saml-Extension as that library had static service provider metadata for all IDPs. This style should be replicated to the new library to allow ease of migration.
  • It is not feasible in production to have different service provider endpoints per IDP at scale. Our company has currently over 100 different IDPs integrating with our service provider using the old library. Having to distribute individual metadata files to each IDP is more work than having a single service provider metadata with common endpoints that all IDPs can use.
  • Similarly, SAML federations expect a single Service Provider that can be published to all members of the federation. As an example, we belong to https://incommon.org/federation/. If we have to expose different Service Provider metadata for each IDP, it would be impossible to use these federations.
  • This is opinion, but I don't think it is in the spirit of SAML to expose different Service Provider configurations per IDP.

I'm new to this library so there might be more to this ticket than what I wrote. In general, we should be able to distribute the same Service Provider metadata (with the same endpoints) to all IDPs. The most obvious violation of this I saw was the AssertionConsumerService url, but perhaps there are more endpoints/values in the metadata that need to be changed in order to support this (Audience Restriction, Recipient Url, etc)?

Also wanted to say thank you for keeping SAML alive in Spring Security Core as the old project is dead!

@vince-recupito vince-recupito added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Sep 8, 2021
@jzheaux jzheaux added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 14, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Sep 14, 2021

Thanks for the detailed write-up, @vince-recupito. You are correct that RelyingPartyRegistrationResolver is intended to allow for this kind of customization.

I think the idea for an issuer-based resolver has merit -- there is something similar for OAuth 2.0 Resource Servers in JwtIssuerAuthenticationManagerResolver -- but I'm not yet sure if it's a fit for the framework.

I think there are three questions to consider:

How would you look up a RelyingPartyRegistration from an issuer value?

How would you approach the metadata endpoint (Saml2MetadataFilter) since there is no issuer in that request?

Finally, how would you handle when the registration id is already known (for example in the case of logout when the registration id is attached to the currently-authenticated user)?

@vince-recupito
Copy link
Author

  1. How would you look up a RelyingPartyRegistration from an issuer value?

Digging around in the old SAML library under the following files, it looks like entityId is extracted from the SAML response immediately. Later on in the execution path when the signature gets validated, the entityId appears to be used to locate the proper IDP with its keys. It makes sense to replicate this logic (use entityId from response).

https://github.com/spring-projects/spring-security-saml/blob/08a236e1c65c4fb559282a7f16115e6f122d77b0/core/src/main/java/org/springframework/security/saml/SAMLProcessingFilter.java#L85

https://github.com/spring-projects/spring-security-saml/blob/08a236e1c65c4fb559282a7f16115e6f122d77b0/core/src/main/java/org/springframework/security/saml/processor/SAMLProcessorImpl.java#L111

It should be fairly easy to extend RelyingPartyRegistrationRepository with a findByEntityId method to be used to locate the proper relyingParty from its entityId (We have that field saved already). Then it is up to the new RelyingPartyResolver to extract the entityId from the metadata and pass it along to the repository to locate the specific RelyingParty.

  1. How would you approach the metadata endpoint (Saml2MetadataFilter) since there is no issuer in that request?

Our end-goal would be to setup one service provider linked to many IDPs and distribute one set of service provider metadata. In this case we would have a single metadata url with no registration details (/saml2/service-provider-metadata). Furthermore, the metadata itself and the service provider would have no registrationId specific references / endpoints. This was the behavior of the old library.

The new library is a bit different from the old library in that you are forced to setup multiple service providers if there is more than one IDP. In this case I wouldn't know which metadata to expose. Perhaps we can give users of the library the option to expose their metadata on /saml2/service-provider-metadata if and only if all registered service providers have the same entityId, decryptionKeys, assertionConsumerService urls, etc (there is effectively only one service provider). If there is more than one effective service provider, we obviously couldn't allow this option as we wouldn't know which SP metadata to expose.

Finally, how would you handle when the registration id is already known (for example in the case of logout when the registration id is attached to the currently-authenticated user)?

In the case of service provider initiated flows where it is optional to specify the relying party to use (/saml2/authenticate/{registrationId}), it makes sense to leave that as is.

Our company doesn't using single-logout and I can't speak to that feature personally as I am not too familiar with it.


As a bit of a note: I think a lot of the problem comes from having to register IDP / Service Provider data at the same time as a single RelyingParty. From a modeling perspective, I would have treated the creation of SPs / IDPs independently in the library. So you could register one or more service providers with registration Ids, register one or more IDPs with registration Ids, and then a third mechanism to link certain IDPs to certain SPs. That way SP metadata / endpoints aren't tied to particular IDPs. I'm guessing there were good reasons to go that route though.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 14, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Sep 15, 2021

Thanks again, @vince-recupito.

I believe you can achieve what you want for logins with the following arrangement:

public class OpenSamlResponseRelyingPartyRegistrationResolver implements RelyingPartyRegistrationResolver {
    private final RelyingPartyRegistrationResolver delegate;

    public OpenSamlResponseRelyingPartyRegistrationResolver(RelyingPartyRegistrationRepository registrations) {
        this.delegate = new DefaultRelyingPartyRegistrationResolver(registrations);
    }

    public RelyingPartyRegistration resolve(HttpServletRequest request, String registrationId) {
        String samlResponse = request.getParameter("SAMLResponse");
        if (samlResponse == null) {
            return null;
        }
        String entityId = retrieveEntityId(samlResponse);
        if (entityId == null) {
            return null;
        }
        return this.delegate.resolve(request, entityId);
    }

    private String retreiveEntityId(String samlResponse) {
        // ...
    }
}

Then, if the EntityID is the same as the registrationId, it could be constructed like so:

@Bean 
Saml2AuthenticationTokenConverter saml2Login(RelyingPartyRegistrationRepository registrations) {
    RelyingPartyRegistrationResolver resolver = 
            new OpenSamlResponseRelyingPartyRegistrationResolver(registrations);
    return new Saml2AuthenticationTokenConverter(resolver);
}

or if you have a custom findByEntityId, you can do:

@Bean 
Saml2AuthenticationTokenConverter saml2Login(YourRelyingPartyRegistrationRepository registrations) {
    RelyingPartyRegistrationResolver resolver = 
            new OpenSamlResponseRelyingPartyRegistrationResolver(registrations::findByEntityId);
    return new Saml2AuthenticationTokenConverter(resolver);
}

Would this address what you are wanting to achieve?

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 15, 2021
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Sep 22, 2021
@spring-projects-issues
Copy link

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@spring-projects-issues spring-projects-issues removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Sep 29, 2021
@vladonemo
Copy link

vladonemo commented Jul 25, 2022

I'd like to reopen this issue as I'm facing the same while migrating from the SAML extension. The suggestion in #10243 (comment) would work, however, the implementation of the retrieveEntityId should be done in this project, rather than on the client side.

If implemented on the client side, this is needed:

    private RelyingPartyRegistration retrieveEntityId(String saml2Response)
    {
        byte[] b = this.samlDecode(saml2Response);
        String samlResponse = new String(b, StandardCharsets.UTF_8);
        Response response = parse(samlResponse);
        return relyingPartyRegistrationRepository.findByRegistrationId(
                response.getIssuer().getValue());
    }

The trouble is, that the samlDecode is being called by the Saml2AuthenticationTokenConverter shortly after the RelyingPartyRegistrationResolver is invoked. Hence the response is decoded twice. Moreover, the parse method is called later on in the OpenSaml4AuthenticationProvider. This call is somewhat expensive to be called twice, as it unmarshalls the XML.
Neither of the 2 classes (the Saml2AuthenticationTokenConverter nor OpenSaml4AuthenticationProvider) are extensible enough to avoid this duplicated calls.

@jzheaux
Copy link
Contributor

jzheaux commented Feb 22, 2023

Good point, @vladonemo, perhaps there is something else that can be done here.

Another idea is to implement AuthenticationConverter, like so:

public class OpenSamlIssuerAuthenticationTokenConverter implements AuthenticationConverter {
    private final Saml2AuthenticationTokenConverter delegate;
    private final RelyingPartyRegistrationResolver registrations;

    public OpenSamlAuthenticationTokenConverter(RelyingPartyRegistrationResolver registrations) {
        this.delegate = new Saml2AuthenticationTokenConverter(registrations);
        this.registrations = registrations;
    }

    @Override 
    public Authentication convert(HttpServletRequest request) {
        Response response = parseSamlResponse(request);
        if (response == null) {
            return null;
        }
        Saml2AuthenticationToken token = this.delegate.convert(request);
        if (token != null) {
            return new OpenSamlAuthenticationToken(
                    token.getRelyingPartyRegistration(), response, token.getAuthenticationRequest());
        }
        String registrationId = response.getIssuer();
        RelyingPartyRegistration registration = this.registrations.resolve(request, registrationid);
        if (registration == null) {
            return null;
        }
        return new OpenSamlAuthenticationToken(registration, response, authenticationRequest);
    }
}

Hypothetically, OpenSaml4AuthenticationProvider could be enhanced to lookup the Response object in the event that the Authentication is an instance of OpenSamlAuthenticationToken so that the response is not parsed twice.

jzheaux added a commit to jzheaux/spring-security that referenced this issue Mar 17, 2023
@jzheaux jzheaux added this to the 6.1.0-RC1 milestone May 12, 2023
@jzheaux jzheaux changed the title Allow Relying Parties to be Deduced from Contents of SAML Assertion instead of Path Allow RelyingPartyRegistration to be Deduced from Contents of SAML Assertion instead of Path Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants