RelyingPartyRegistrations should not fail when SPSSODescriptor elements are present

[stnor]

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.

[jzheaux]

Hi, @stnor, thanks for the detailed report.

Looks like there are a couple of things here.

• Address SPSSODescriptor error handling
• Add to SAML Metadata documentation for getting the underlying XMLObject (already supported, undocumented, though)

I've added https://github.com/spring-projects/spring-security/issues/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.

I'm unable to find a migration guide

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.

[stnor]

Great, thanks for the prompt response.

I am looking at the migration guide right now, and I am unable to tell if the URL:s are the same form that page. Granted, I can look at the code, but ideally the URL:s should be documented if they are changed.

I am using this configuration at the moment. I can't tell if they're considered "default" or not.

// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/login/**"), samlEntryPoint()),
// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/login/**"), samlLogoutFilter()),
// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/logout/**"), samlEntryPoint()),
// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/SingleLogout/**"), samlLogoutProcessingFilter()),
// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/metadata/**"), samlMetadataDisplayFilter()),
// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/SSO/**"), samlWebSSOProcessingFilter(authenticationManager)),
// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/SSOHoK/**"), samlWebSSOHoKProcessingFilter(authenticationManager)),
// new DefaultSecurityFilterChain(new AntPathRequestMatcher("/saml/discovery/**"), samlIdpDiscovery())

[stnor]

Ah, I found this: https://github.com/spring-projects/spring-security-samples/tree/main/servlet/spring-boot/java/saml2/custom-urls

Will be very useful for migration.

[stnor]

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?

[jzheaux]

@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.

[jzheaux]

Are you able to provide a PR for improving OpenSamlAssertingPartyDetailsConverter as you described?

[stnor]

I was unsure in which branch you wanted the PR for... The PR is for main, but I am running 5.8.

[stnor]

If you want to have the exact same callback URL for all IdPs, then you will need to specify how to perform the lookup.

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 login?id={url-encoded id} whereas the new uses /authenticate/{id} (which doesn't work - StrictHttpFirewall barfs).

[stnor]

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.

    @Bean
    public SecurityFilterChain filterChain(HttpSecurity http,
                                           RefreshableRelyingPartyRegistrationRepository rpRepo,
                                           NompSamlResponseAuthenticationConverter authConverter) throws Exception {
        var relyingPartyRegistrationResolver = new QueryStringRelyingPartyResolver(rpRepo);
        var authenticationRequestResolver = new OpenSaml4AuthenticationRequestResolver(relyingPartyRegistrationResolver);
        authenticationRequestResolver.setRequestMatcher(new AntPathRequestMatcher("/saml/login"));
        var authenticationProvider = new OpenSaml4AuthenticationProvider();
        authenticationProvider.setResponseAuthenticationConverter(authConverter);
        http.saml2Login(samlLogin ->
                samlLogin
                        .authenticationManager(new ProviderManager(authenticationProvider))
                        .authenticationRequestResolver(authenticationRequestResolver)
                        .authenticationConverter(new Saml2AuthenticationTokenConverter(relyingPartyRegistrationResolver))
                        .loginProcessingUrl("/saml/SSO"));
        http.saml2Logout(samlLogout ->
                samlLogout
                        .logoutUrl("/saml/SingleLogout")
                        .relyingPartyRegistrationRepository(rpRepo));

@Component
public class RefreshableRelyingPartyRegistrationRepository
        implements RelyingPartyRegistrationRepository, Iterable<RelyingPartyRegistration> {

    private final static Logger LOGGER = LoggerFactory.getLogger(RefreshableRelyingPartyRegistrationRepository.class);
    private static final char[] SAML_JKS_PASSWORD = "pass".toCharArray();
    public static final String SAML_JKS_PATH = "classpath:saml/saml.jks";
    public static final String SAML_JKS_ALIAS = "keyalias";

    private final Map<String, RelyingPartyRegistration> relyingPartyRegistrations = new ConcurrentHashMap<>();

    private static final ResourceLoader resourceLoader = new DefaultResourceLoader();
    private final Saml2X509Credential signingCredentials;

    public RefreshableRelyingPartyRegistrationRepository() {
        this.signingCredentials = createSigningCredentials();
        refreshMetadata();
    }

    @Override
    public RelyingPartyRegistration findByRegistrationId(String registrationId) {
        return this.relyingPartyRegistrations.get(registrationId);
    }

    @Override
    public Iterator<RelyingPartyRegistration> iterator() {
        return this.relyingPartyRegistrations.values().iterator();
    }

    @Scheduled(fixedDelay = 30, timeUnit = TimeUnit.MINUTES)
    public void refreshMetadata() {
        fetchMetadata();
    }

    void fetchMetadata() {
        LOGGER.info("Fetching metadata from Skolfederation");

        //All IdP:s
        RelyingPartyRegistrations
                .collectionFromMetadataLocation("https://fed.skolfederation.se/prod/md/skolfederation-3_1.xml")
                .forEach(builder -> {
                    builder.entityId("https://nomp.se");
                    builder.assertionConsumerServiceLocation("https://nomp.se/saml/SSO");
                    builder.signingX509Credentials(credentials -> credentials.add(this.signingCredentials));
                    builder.assertingPartyDetails(apdBuilder -> {
                        apdBuilder.wantAuthnRequestsSigned(true);
                    });
                    RelyingPartyRegistration idp = builder.build();
                    this.relyingPartyRegistrations.put(idp.getRegistrationId(), idp);
                });
    }

    private Saml2X509Credential createSigningCredentials() {
        try {
            KeyStore ks = KeyStore.getInstance("JKS");
            ks.load(resourceLoader.getResource(SAML_JKS_PATH).getInputStream(), SAML_JKS_PASSWORD);
            var cert = (X509Certificate) ks.getCertificate(SAML_JKS_ALIAS);
            var key = (PrivateKey) ks.getKey(SAML_JKS_ALIAS, SAML_JKS_PASSWORD);
            return new Saml2X509Credential(key, cert, Saml2X509Credential.Saml2X509CredentialType.SIGNING);
        } catch (Exception e){
           throw new RuntimeException(e);
        }
    }

}

public class QueryStringRelyingPartyResolver implements RelyingPartyRegistrationResolver {

    private final RelyingPartyRegistrationResolver delegate;

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

    @Override
    public RelyingPartyRegistration resolve(HttpServletRequest request, String relyingPartyRegistrationId) {
        relyingPartyRegistrationId = relyingPartyRegistrationId == null ? request.getParameter("idp") : relyingPartyRegistrationId;
        return this.delegate.resolve(request, relyingPartyRegistrationId);
    }
}

[stnor]

Is there support for discovery? I have the following in my metadata today:

<md:Extensions>
<idpdisco:DiscoveryResponse xmlns:idpdisco="urn:oasis:names:tc:SAML:profiles:SSO:idp-discovery-protocol" Binding="urn:oasis:names:tc:SAML:profiles:SSO:idp-discovery-protocol" Location="https://nomp.se/saml/login?disco=true" index="0"/>
</md:Extensions>

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 ?idp=-QueryStringRelyingPartyResolver

[stnor]

I ended up with this to fix the 404:

@Controller
public class Saml2LoginRedirectController {
    public final static String LOGIN_REDIRECT = "/saml/login";

    @RequestMapping(LOGIN_REDIRECT)
    public RedirectView redirectToLogin() {
        return new RedirectView("/start/login/saml");
    }
}

and

        http.saml2Login(samlLogin ->
                samlLogin
                        .loginPage(LOGIN_REDIRECT)

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.

[jzheaux]

@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.

The PR is for main, but I am running 5.8.

Spring Security uses the merge forward strategy. Since this is a bug, it should be on the earliest affected branch, which would be 5.7.x.

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 :)

Understood. I could have worded my comment better. I understand that it's not cosmetic in this case.

I think it's a shame that the project doesn't provide a more backwards compatible way of migrating from the old project

Progress is ongoing, and I think that it may be helpful to discuss this further on another ticket. I've reopened https://github.com/spring-projects/spring-security/issues/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 /saml/SSO and the other Extension URIs. I've added https://github.com/spring-projects/spring-security-samples/issues/122 to change it to use the login endpoint you indicated.

Finally, in many cases, a static URL is sufficient since the registrationId is looked up from the stored AuthnRequest that initiated login, so if you intend to only support SP-initiated login, you may find that changing the value to /saml/SSO will "just work".

many of the entityIds are URLs ... StrictHttpFirewall barfs

While the registration id defaults to the entity id in RelyingPartyRegistrations, they are not the same thing. You can change the registration id to something else if you wish:

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&lt;RelyingPartyRegistration&gt; 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 https://github.com/spring-projects/spring-security/issues/12764 to address that.

QueryStringRelyingPartyResolver, etc.

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.

[stnor]

Thanks @jzheaux! So, other than the incorrect branch, you're happy with the PR? If so, I'll close the PR and provide another for 5.7.x.

[stnor]

Apologies. I am not able to fix the previous PR. It's beyond my git/committer skills :(

Closing the old: https://github.com/spring-projects/spring-security/pull/12742

New PR is: https://github.com/spring-projects/spring-security/pull/12770

[stnor]

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.

I cannot tell you how much I appreciate this. See https://github.com/stnor/fed-saml-example/issues/1

[abinesh-s]

@stnor , request you to take a look at below one and suggest how that can be handled

https://github.com/spring-projects/spring-security/issues/12812

[lasselindqvist]

@jzheaux is there any plan to release this fix as part of the 5.8.x version? The fix cmmit (https://github.com/stnor/spring-security/commit/ccb292ca6ab59dba7dc4b885147ccf46ae1a567b) is not in 5.8.1 at least.

What would be the way to get it into the next milestone? (https://github.com/spring-projects/spring-security/milestone/285)

[jzheaux]

@lasselindqvist, that was added in 5.8.x as 6c7703789a06b44730f5659e3c4eb0b0bdd9b2b5, so that will go out in the 5.8.3 release.

[jzheaux]

I've created https://github.com/spring-projects/spring-security/issues/13054 now to clarify that.