From 864af59adc1ca905bcdcf1f4396c4a43d6882dec Mon Sep 17 00:00:00 2001 From: Lasse Lindqvist Date: Thu, 15 Jun 2023 12:26:59 +0300 Subject: [PATCH 1/2] Choose SAML party based on entity ID rather than always using first Update `Saml2RelyingPartyRegistrationConfiguration` so that `RelyingPartyRegistrations` uses `collectionFromMetadataLocation` rather than `fromMetadataLocation` and searches candidates for a matching entity ID. Prior to this commit, it was possible for the wrong provider to be used if multiple candidates existed in the returned metadata. See gh-35902 --- ...RelyingPartyRegistrationConfiguration.java | 24 +++++- ...ml2RelyingPartyAutoConfigurationTests.java | 37 ++++++++ .../saml/idp-metadata-with-multiple-providers | 86 +++++++++++++++++++ 3 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 spring-boot-project/spring-boot-autoconfigure/src/test/resources/saml/idp-metadata-with-multiple-providers diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java index 2053bc1cb5..886ad9a5ac 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java @@ -88,9 +88,14 @@ class Saml2RelyingPartyRegistrationConfiguration { private RelyingPartyRegistration asRegistration(String id, Registration properties) { AssertingPartyProperties assertingParty = new AssertingPartyProperties(properties, id); boolean usingMetadata = StringUtils.hasText(assertingParty.getMetadataUri()); - Builder builder = (usingMetadata) - ? RelyingPartyRegistrations.fromMetadataLocation(assertingParty.getMetadataUri()).registrationId(id) - : RelyingPartyRegistration.withRegistrationId(id); + Builder builder = (usingMetadata) ? RelyingPartyRegistrations + .collectionFromMetadataLocation(properties.getAssertingparty().getMetadataUri()) + .stream() + .filter(b -> entityIdsMatch(properties, b)) + .findFirst() + .orElseThrow(() -> new IllegalStateException( + "No relying party with entity-id " + properties.getEntityId() + " found.")) + .registrationId(id) : RelyingPartyRegistration.withRegistrationId(id); builder.assertionConsumerServiceLocation(properties.getAcs().getLocation()); builder.assertionConsumerServiceBinding(properties.getAcs().getBinding()); builder.assertingPartyDetails(mapAssertingParty(properties, id, usingMetadata)); @@ -119,6 +124,19 @@ class Saml2RelyingPartyRegistrationConfiguration { return registration; } + /** + * Tests if the builder would have the correct entity-id. If no entity-id is given in + * properties, any builder passes the test. + * @param properties the properties + * @param b the builder + * @return true if the builder passes the test + */ + private boolean entityIdsMatch(Registration properties, Builder b) { + RelyingPartyRegistration rpr = b.build(); + return properties.getAssertingparty().getEntityId() == null + || properties.getAssertingparty().getEntityId().equals(rpr.getAssertingPartyDetails().getEntityId()); + } + private Consumer mapAssertingParty(Registration registration, String id, boolean usingMetadata) { return (details) -> { diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java index 32ad6d0d23..37d407d3ed 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java @@ -402,6 +402,43 @@ class Saml2RelyingPartyAutoConfigurationTests { this.contextRunner.withPropertyValues(getPropertyValues(false)) .run((context) -> assertThat(hasFilter(context, Saml2LogoutRequestFilter.class)).isTrue()); } + + @Test + void autoconfigurationShouldWorkWithMultipleProvidersWithNoEntityId() throws Exception { + try (MockWebServer server = new MockWebServer()) { + server.start(); + String metadataUrl = server.url("").toString(); + setupMockResponse(server, new ClassPathResource("saml/idp-metadata-with-multiple-providers")); + this.contextRunner.withPropertyValues(PREFIX + ".foo.assertingparty.metadata-uri=" + metadataUrl) + .run((context) -> { + assertThat(context).hasSingleBean(RelyingPartyRegistrationRepository.class); + assertThat(server.getRequestCount()).isOne(); + RelyingPartyRegistrationRepository repository = context.getBean(RelyingPartyRegistrationRepository.class); + RelyingPartyRegistration registration = repository.findByRegistrationId("foo"); + assertThat(registration.getAssertingPartyDetails().getEntityId()) + .isEqualTo("https://idp.example.com/idp/shibboleth"); + }); + } + } + + @Test + void autoconfigurationShouldWorkWithMultipleProviders() throws Exception { + try (MockWebServer server = new MockWebServer()) { + server.start(); + String metadataUrl = server.url("").toString(); + setupMockResponse(server, new ClassPathResource("saml/idp-metadata-with-multiple-providers")); + this.contextRunner.withPropertyValues(PREFIX + ".foo.assertingparty.metadata-uri=" + metadataUrl, + PREFIX + ".foo.assertingparty.entity-id=https://idp2.example.com/idp/shibboleth") + .run((context) -> { + assertThat(context).hasSingleBean(RelyingPartyRegistrationRepository.class); + assertThat(server.getRequestCount()).isOne(); + RelyingPartyRegistrationRepository repository = context.getBean(RelyingPartyRegistrationRepository.class); + RelyingPartyRegistration registration = repository.findByRegistrationId("foo"); + assertThat(registration.getAssertingPartyDetails().getEntityId()) + .isEqualTo("https://idp2.example.com/idp/shibboleth"); + }); + } + } private String[] getPropertyValuesWithoutSigningCredentials(boolean signRequests, boolean useDeprecated) { String assertingParty = useDeprecated ? "identityprovider" : "assertingparty"; diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/resources/saml/idp-metadata-with-multiple-providers b/spring-boot-project/spring-boot-autoconfigure/src/test/resources/saml/idp-metadata-with-multiple-providers new file mode 100644 index 0000000000..af40448589 --- /dev/null +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/resources/saml/idp-metadata-with-multiple-providers @@ -0,0 +1,86 @@ + + + + + + + + MIIDZjCCAk6gAwIBAgIVAL9O+PA7SXtlwZZY8MVSE9On1cVWMA0GCSqGSIb3DQEB + BQUAMCkxJzAlBgNVBAMTHmlkZW0tcHVwYWdlbnQuZG16LWludC51bmltby5pdDAe + Fw0xMzA3MjQwMDQ0MTRaFw0zMzA3MjQwMDQ0MTRaMCkxJzAlBgNVBAMTHmlkZW0t + cHVwYWdlbnQuZG16LWludC51bmltby5pdDCCASIwDQYJKoZIhvcNAMIIDQADggEP + ADCCAQoCggEBAIAcp/VyzZGXUF99kwj4NvL/Rwv4YvBgLWzpCuoxqHZ/hmBwJtqS + v0y9METBPFbgsF3hCISnxbcmNVxf/D0MoeKtw1YPbsUmow/bFe+r72hZ+IVAcejN + iDJ7t5oTjsRN1t1SqvVVk6Ryk5AZhpFW+W9pE9N6c7kJ16Rp2/mbtax9OCzxpece + byi1eiLfIBmkcRawL/vCc2v6VLI18i6HsNVO3l2yGosKCbuSoGDx2fCdAOk/rgdz + cWOvFsIZSKuD+FVbSS/J9GVs7yotsS4PRl4iX9UMnfDnOMfO7bcBgbXtDl4SCU1v + dJrRw7IL/pLz34Rv9a8nYitrzrxtLOp3nYUCAwEAAaOBhDCBgTBgBgMIIDEEWTBX + gh5pZGVtLXB1cGFnZW50LmRtei1pbnQudW5pbW8uaXSGNWh0dHBzOi8vaWRlbS1w + dXBhZ2VudC5kbXotaW50LnVuaW1vLml0L2lkcC9zaGliYm9sZXRoMB0GA1UdDgQW + BBT8PANzz+adGnTRe8ldcyxAwe4VnzANBgkqhkiG9w0BAQUFAAOCAQEAOEnO8Clu + 9z/Lf/8XOOsTdxJbV29DIF3G8KoQsB3dBsLwPZVEAQIP6ceS32Xaxrl6FMTDDNkL + qUvvInUisw0+I5zZwYHybJQCletUWTnz58SC4C9G7FpuXHFZnOGtRcgGD1NOX4UU + duus/4nVcGSLhDjszZ70Xtj0gw2Sn46oQPHTJ81QZ3Y9ih+Aj1c9OtUSBwtWZFkU + yooAKoR8li68Yb21zN2N65AqV+ndL98M8xUYMKLONuAXStDeoVCipH6PJ09Z5U2p + V5p4IQRV6QBsNw9CISJFuHzkVYTH5ZxzN80Ru46vh4y2M0Nu8GQ9I085KoZkrf5e + Cq53OZt9ISjHEw== + + + + + + + + mailto:technical.contact@example.com + + + + + + + + + MIIDZjCCAk6gAwIBAgIVAL9O+PA7SXtlwZZY8MVSE9On1cVWMA0GCSqGSIb3DQEB + BQUAMCkxJzAlBgNVBAMTHmlkZW0tcHVwYWdlbnQuZG16LWludC51bmltby5pdDAe + Fw0xMzA3MjQwMDQ0MTRaFw0zMzA3MjQwMDQ0MTRaMCkxJzAlBgNVBAMTHmlkZW0t + cHVwYWdlbnQuZG16LWludC51bmltby5pdDCCASIwDQYJKoZIhvcNAMIIDQADggEP + ADCCAQoCggEBAIAcp/VyzZGXUF99kwj4NvL/Rwv4YvBgLWzpCuoxqHZ/hmBwJtqS + v0y9METBPFbgsF3hCISnxbcmNVxf/D0MoeKtw1YPbsUmow/bFe+r72hZ+IVAcejN + iDJ7t5oTjsRN1t1SqvVVk6Ryk5AZhpFW+W9pE9N6c7kJ16Rp2/mbtax9OCzxpece + byi1eiLfIBmkcRawL/vCc2v6VLI18i6HsNVO3l2yGosKCbuSoGDx2fCdAOk/rgdz + cWOvFsIZSKuD+FVbSS/J9GVs7yotsS4PRl4iX9UMnfDnOMfO7bcBgbXtDl4SCU1v + dJrRw7IL/pLz34Rv9a8nYitrzrxtLOp3nYUCAwEAAaOBhDCBgTBgBgMIIDEEWTBX + gh5pZGVtLXB1cGFnZW50LmRtei1pbnQudW5pbW8uaXSGNWh0dHBzOi8vaWRlbS1w + dXBhZ2VudC5kbXotaW50LnVuaW1vLml0L2lkcC9zaGliYm9sZXRoMB0GA1UdDgQW + BBT8PANzz+adGnTRe8ldcyxAwe4VnzANBgkqhkiG9w0BAQUFAAOCAQEAOEnO8Clu + 9z/Lf/8XOOsTdxJbV29DIF3G8KoQsB3dBsLwPZVEAQIP6ceS32Xaxrl6FMTDDNkL + qUvvInUisw0+I5zZwYHybJQCletUWTnz58SC4C9G7FpuXHFZnOGtRcgGD1NOX4UU + duus/4nVcGSLhDjszZ70Xtj0gw2Sn46oQPHTJ81QZ3Y9ih+Aj1c9OtUSBwtWZFkU + yooAKoR8li68Yb21zN2N65AqV+ndL98M8xUYMKLONuAXStDeoVCipH6PJ09Z5U2p + V5p4IQRV6QBsNw9CISJFuHzkVYTH5ZxzN80Ru46vh4y2M0Nu8GQ9I085KoZkrf5e + Cq53OZt9ISjHEw== + + + + + + + + mailto:technical.contact2@example.com + + + \ No newline at end of file From b6990940b12bd431376f866959ac86e2b76d9395 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Sun, 2 Jul 2023 15:28:05 +0100 Subject: [PATCH 2/2] Polish 'Choose SAML party based on entity ID rather than always using first' See gh-35902 --- ...RelyingPartyRegistrationConfiguration.java | 40 +++++++------- ...ml2RelyingPartyAutoConfigurationTests.java | 54 +++++++++---------- 2 files changed, 47 insertions(+), 47 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java index 886ad9a5ac..7e9ca5120b 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyRegistrationConfiguration.java @@ -20,6 +20,7 @@ import java.io.InputStream; import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; import java.security.interfaces.RSAPrivateKey; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.function.Consumer; @@ -63,6 +64,7 @@ import org.springframework.util.StringUtils; * @author Madhura Bhave * @author Phillip Webb * @author Moritz Halbritter + * @author Lasse Lindqvist */ @Configuration(proxyBeanMethods = false) @Conditional(RegistrationConfiguredCondition.class) @@ -88,14 +90,8 @@ class Saml2RelyingPartyRegistrationConfiguration { private RelyingPartyRegistration asRegistration(String id, Registration properties) { AssertingPartyProperties assertingParty = new AssertingPartyProperties(properties, id); boolean usingMetadata = StringUtils.hasText(assertingParty.getMetadataUri()); - Builder builder = (usingMetadata) ? RelyingPartyRegistrations - .collectionFromMetadataLocation(properties.getAssertingparty().getMetadataUri()) - .stream() - .filter(b -> entityIdsMatch(properties, b)) - .findFirst() - .orElseThrow(() -> new IllegalStateException( - "No relying party with entity-id " + properties.getEntityId() + " found.")) - .registrationId(id) : RelyingPartyRegistration.withRegistrationId(id); + Builder builder = (!usingMetadata) ? RelyingPartyRegistration.withRegistrationId(id) + : createBuilderUsingMetadata(id, assertingParty).registrationId(id); builder.assertionConsumerServiceLocation(properties.getAcs().getLocation()); builder.assertionConsumerServiceBinding(properties.getAcs().getBinding()); builder.assertingPartyDetails(mapAssertingParty(properties, id, usingMetadata)); @@ -124,17 +120,23 @@ class Saml2RelyingPartyRegistrationConfiguration { return registration; } - /** - * Tests if the builder would have the correct entity-id. If no entity-id is given in - * properties, any builder passes the test. - * @param properties the properties - * @param b the builder - * @return true if the builder passes the test - */ - private boolean entityIdsMatch(Registration properties, Builder b) { - RelyingPartyRegistration rpr = b.build(); - return properties.getAssertingparty().getEntityId() == null - || properties.getAssertingparty().getEntityId().equals(rpr.getAssertingPartyDetails().getEntityId()); + private RelyingPartyRegistration.Builder createBuilderUsingMetadata(String id, + AssertingPartyProperties properties) { + String requiredEntityId = properties.getEntityId(); + Collection candidates = RelyingPartyRegistrations + .collectionFromMetadataLocation(properties.getMetadataUri()); + for (RelyingPartyRegistration.Builder candidate : candidates) { + if (requiredEntityId == null || requiredEntityId.equals(getEntityId(candidate))) { + return candidate; + } + } + throw new IllegalStateException("No relying party with Entity ID '" + requiredEntityId + "' found"); + } + + private Object getEntityId(RelyingPartyRegistration.Builder candidate) { + String[] result = new String[1]; + candidate.assertingPartyDetails((builder) -> result[0] = builder.build().getEntityId()); + return result[0]; } private Consumer mapAssertingParty(Registration registration, String id, diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java index 37d407d3ed..1df102b853 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/security/saml2/Saml2RelyingPartyAutoConfigurationTests.java @@ -16,6 +16,7 @@ package org.springframework.boot.autoconfigure.security.saml2; +import java.io.IOException; import java.io.InputStream; import java.util.List; @@ -55,6 +56,7 @@ import static org.mockito.Mockito.mock; * * @author Madhura Bhave * @author Moritz Halbritter + * @author Lasse Lindqvist */ class Saml2RelyingPartyAutoConfigurationTests { @@ -402,41 +404,37 @@ class Saml2RelyingPartyAutoConfigurationTests { this.contextRunner.withPropertyValues(getPropertyValues(false)) .run((context) -> assertThat(hasFilter(context, Saml2LogoutRequestFilter.class)).isTrue()); } - + @Test - void autoconfigurationShouldWorkWithMultipleProvidersWithNoEntityId() throws Exception { - try (MockWebServer server = new MockWebServer()) { - server.start(); - String metadataUrl = server.url("").toString(); - setupMockResponse(server, new ClassPathResource("saml/idp-metadata-with-multiple-providers")); - this.contextRunner.withPropertyValues(PREFIX + ".foo.assertingparty.metadata-uri=" + metadataUrl) - .run((context) -> { - assertThat(context).hasSingleBean(RelyingPartyRegistrationRepository.class); - assertThat(server.getRequestCount()).isOne(); - RelyingPartyRegistrationRepository repository = context.getBean(RelyingPartyRegistrationRepository.class); - RelyingPartyRegistration registration = repository.findByRegistrationId("foo"); - assertThat(registration.getAssertingPartyDetails().getEntityId()) - .isEqualTo("https://idp.example.com/idp/shibboleth"); - }); - } + void autoconfigurationWhenMultipleProvidersAndNoSpecifiedEntityId() throws Exception { + testMultipleProviders(null, "https://idp.example.com/idp/shibboleth"); } - + @Test - void autoconfigurationShouldWorkWithMultipleProviders() throws Exception { + void autoconfigurationWhenMultipleProvidersAndSpecifiedEntityId() throws Exception { + testMultipleProviders("https://idp.example.com/idp/shibboleth", "https://idp.example.com/idp/shibboleth"); + testMultipleProviders("https://idp2.example.com/idp/shibboleth", "https://idp2.example.com/idp/shibboleth"); + } + + private void testMultipleProviders(String specifiedEntityId, String expected) throws IOException, Exception { try (MockWebServer server = new MockWebServer()) { server.start(); String metadataUrl = server.url("").toString(); setupMockResponse(server, new ClassPathResource("saml/idp-metadata-with-multiple-providers")); - this.contextRunner.withPropertyValues(PREFIX + ".foo.assertingparty.metadata-uri=" + metadataUrl, - PREFIX + ".foo.assertingparty.entity-id=https://idp2.example.com/idp/shibboleth") - .run((context) -> { - assertThat(context).hasSingleBean(RelyingPartyRegistrationRepository.class); - assertThat(server.getRequestCount()).isOne(); - RelyingPartyRegistrationRepository repository = context.getBean(RelyingPartyRegistrationRepository.class); - RelyingPartyRegistration registration = repository.findByRegistrationId("foo"); - assertThat(registration.getAssertingPartyDetails().getEntityId()) - .isEqualTo("https://idp2.example.com/idp/shibboleth"); - }); + WebApplicationContextRunner contextRunner = this.contextRunner + .withPropertyValues(PREFIX + ".foo.assertingparty.metadata-uri=" + metadataUrl); + if (specifiedEntityId != null) { + contextRunner = contextRunner + .withPropertyValues(PREFIX + ".foo.assertingparty.entity-id=" + specifiedEntityId); + } + contextRunner.run((context) -> { + assertThat(context).hasSingleBean(RelyingPartyRegistrationRepository.class); + assertThat(server.getRequestCount()).isOne(); + RelyingPartyRegistrationRepository repository = context + .getBean(RelyingPartyRegistrationRepository.class); + RelyingPartyRegistration registration = repository.findByRegistrationId("foo"); + assertThat(registration.getAssertingPartyDetails().getEntityId()).isEqualTo(expected); + }); } }