diff --git a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/BaseOpenSamlAuthenticationProvider.java b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/BaseOpenSamlAuthenticationProvider.java index 5c24acf2c7..8532c62fe4 100644 --- a/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/BaseOpenSamlAuthenticationProvider.java +++ b/saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/BaseOpenSamlAuthenticationProvider.java @@ -326,62 +326,75 @@ class BaseOpenSamlAuthenticationProvider implements AuthenticationProvider { boolean responseSigned = response.isSigned(); ResponseToken responseToken = new ResponseToken(response, token); - Saml2ResponseValidatorResult result = this.responseSignatureValidator.convert(responseToken); + Collection responseSignatureErrors = this.responseSignatureValidator.convert(responseToken) + .getErrors(); + if (!responseSignatureErrors.isEmpty()) { + reportErrors(response, responseSignatureErrors); + return; + } + + Collection errors = new ArrayList<>(); if (responseSigned) { this.responseElementsDecrypter.accept(responseToken); } else if (!response.getEncryptedAssertions().isEmpty()) { - result = result.concat(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE, + errors.add(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE, "Did not decrypt response [" + response.getID() + "] since it is not signed")); } if (!this.validateResponseAfterAssertions) { - result = result.concat(this.responseValidator.convert(responseToken)); + errors.addAll(this.responseValidator.convert(responseToken).getErrors()); } boolean allAssertionsSigned = true; for (Assertion assertion : response.getAssertions()) { AssertionToken assertionToken = new AssertionToken(assertion, token); - result = result.concat(this.assertionSignatureValidator.convert(assertionToken)); + Collection assertionSignatureErrors = this.assertionSignatureValidator.convert(assertionToken) + .getErrors(); + errors.addAll(assertionSignatureErrors); allAssertionsSigned = allAssertionsSigned && assertion.isSigned(); + if (!assertionSignatureErrors.isEmpty()) { + continue; + } if (responseSigned || assertion.isSigned()) { this.assertionElementsDecrypter.accept(new AssertionToken(assertion, token)); } - result = result.concat(this.assertionValidator.convert(assertionToken)); + errors.addAll(this.assertionValidator.convert(assertionToken).getErrors()); } if (!responseSigned && !allAssertionsSigned) { String description = "Either the response or one of the assertions is unsigned. " + "Please either sign the response or all of the assertions."; - result = result.concat(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE, description)); + errors.add(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE, description)); } if (this.validateResponseAfterAssertions) { - result = result.concat(this.responseValidator.convert(responseToken)); + errors.addAll(this.responseValidator.convert(responseToken).getErrors()); } else { Assertion firstAssertion = CollectionUtils.firstElement(response.getAssertions()); if (firstAssertion != null && !hasName(firstAssertion)) { - Saml2Error error = new Saml2Error(Saml2ErrorCodes.SUBJECT_NOT_FOUND, - "Assertion [" + firstAssertion.getID() + "] is missing a subject"); - result = result.concat(error); + errors.add(new Saml2Error(Saml2ErrorCodes.SUBJECT_NOT_FOUND, + "Assertion [" + firstAssertion.getID() + "] is missing a subject")); } } - if (result.hasErrors()) { - Collection errors = result.getErrors(); - if (this.logger.isTraceEnabled()) { - this.logger.trace("Found " + errors.size() + " validation errors in SAML response [" + response.getID() - + "]: " + errors); - } - else if (this.logger.isDebugEnabled()) { - this.logger - .debug("Found " + errors.size() + " validation errors in SAML response [" + response.getID() + "]"); - } - Saml2Error first = errors.iterator().next(); - throw createAuthenticationException(first.getErrorCode(), first.getDescription(), null); - } - else { + reportErrors(response, errors); + } + + private void reportErrors(Response response, Collection errors) { + if (errors.isEmpty()) { if (this.logger.isDebugEnabled()) { this.logger.debug("Successfully processed SAML Response [" + response.getID() + "]"); } + return; } + if (this.logger.isTraceEnabled()) { + this.logger.debug("Found " + errors.size() + " validation errors in SAML response [" + response.getID() + + "]: " + errors); + } + else if (this.logger.isDebugEnabled()) { + this.logger + .debug("Found " + errors.size() + " validation errors in SAML response [" + response.getID() + "]"); + } + Saml2Error first = errors.iterator().next(); + throw createAuthenticationException(first.getErrorCode(), first.getDescription(), null); } private Converter createDefaultResponseSignatureValidator() { diff --git a/saml2/saml2-service-provider/src/opensaml4Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProviderTests.java b/saml2/saml2-service-provider/src/opensaml4Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProviderTests.java index d9a4fd5e25..56af63f186 100644 --- a/saml2/saml2-service-provider/src/opensaml4Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProviderTests.java +++ b/saml2/saml2-service-provider/src/opensaml4Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProviderTests.java @@ -33,6 +33,7 @@ import javax.xml.namespace.QName; import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.opensaml.core.xml.XMLObject; import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; import org.opensaml.core.xml.schema.XSDateTime; @@ -87,6 +88,7 @@ import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; /** * Tests for {@link OpenSaml4AuthenticationProvider} @@ -173,6 +175,53 @@ public class OpenSaml4AuthenticationProviderTests { .satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE)); } + @Test + public void authenticateWhenResponseSignatureInvalidThenSkipsResponseAndAssertionValidation() { + Response response = response(); + response.setDestination(DESTINATION + "invalid"); + Assertion assertion = assertion(); + response.getAssertions().add(signed(assertion)); + // Sign the response with a credential the verifier does not trust + TestOpenSamlObjects.signed(response, TestSaml2X509Credentials.relyingPartyDecryptingCredential(), + RELYING_PARTY_ENTITY_ID); + OpenSaml4AuthenticationProvider provider = new OpenSaml4AuthenticationProvider(); + Converter responseValidator = mock(Converter.class); + Converter assertionValidator = mock( + Converter.class); + provider.setResponseValidator(responseValidator); + provider.setAssertionValidator(assertionValidator); + Saml2AuthenticationToken token = token(response, verifying(registration())); + assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token)) + .satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE)); + verifyNoInteractions(responseValidator, assertionValidator); + } + + @Test + public void authenticateWhenAssertionSignatureInvalidThenSkipsThatAssertionValidationOnly() { + Response response = response(); + Assertion bad = assertion(); + bad.setID("bad-assertion"); + TestOpenSamlObjects.signed(bad, TestSaml2X509Credentials.relyingPartyDecryptingCredential(), + RELYING_PARTY_ENTITY_ID); + response.getAssertions().add(bad); + Assertion good = assertion(); + good.setID("good-assertion"); + response.getAssertions().add(signed(good)); + OpenSaml4AuthenticationProvider provider = new OpenSaml4AuthenticationProvider(); + Converter assertionValidator = mock( + Converter.class); + given(assertionValidator.convert(any(OpenSaml4AuthenticationProvider.AssertionToken.class))) + .willReturn(Saml2ResponseValidatorResult.success()); + provider.setAssertionValidator(assertionValidator); + Saml2AuthenticationToken token = token(signed(response), verifying(registration())); + assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token)) + .satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE)); + ArgumentCaptor captor = ArgumentCaptor + .forClass(OpenSaml4AuthenticationProvider.AssertionToken.class); + verify(assertionValidator).convert(captor.capture()); + assertThat(captor.getValue().getAssertion().getID()).isEqualTo("good-assertion"); + } + @Test public void authenticateWhenOpenSAMLValidationErrorThenThrowAuthenticationException() { Response response = response(); @@ -502,7 +551,7 @@ public class OpenSaml4AuthenticationProviderTests { EncryptedAssertion encryptedAssertion = TestOpenSamlObjects.encrypted(assertion(), TestSaml2X509Credentials.assertingPartyEncryptingCredential()); response.getEncryptedAssertions().add(encryptedAssertion); - Saml2AuthenticationToken token = token(signed(response), registration() + Saml2AuthenticationToken token = token(signed(response), verifying(registration()) .decryptionX509Credentials((c) -> c.add(TestSaml2X509Credentials.assertingPartyPrivateCredential()))); assertThatExceptionOfType(Saml2AuthenticationException.class) .isThrownBy(() -> this.provider.authenticate(token)) diff --git a/saml2/saml2-service-provider/src/opensaml5Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml5AuthenticationProviderTests.java b/saml2/saml2-service-provider/src/opensaml5Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml5AuthenticationProviderTests.java index 309f04bb16..df42a6f381 100644 --- a/saml2/saml2-service-provider/src/opensaml5Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml5AuthenticationProviderTests.java +++ b/saml2/saml2-service-provider/src/opensaml5Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml5AuthenticationProviderTests.java @@ -34,6 +34,7 @@ import javax.xml.namespace.QName; import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.opensaml.core.xml.XMLObject; import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; import org.opensaml.core.xml.schema.XSDateTime; @@ -183,6 +184,52 @@ public class OpenSaml5AuthenticationProviderTests { .satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE)); } + @Test + public void authenticateWhenResponseSignatureInvalidThenSkipsResponseAndAssertionValidation() { + Response response = response(); + response.setDestination(DESTINATION + "invalid"); + Assertion assertion = assertion(); + response.getAssertions().add(signed(assertion)); + TestOpenSamlObjects.signed(response, TestSaml2X509Credentials.relyingPartyDecryptingCredential(), + RELYING_PARTY_ENTITY_ID); + OpenSaml5AuthenticationProvider provider = new OpenSaml5AuthenticationProvider(); + Converter responseValidator = mock(Converter.class); + Converter assertionValidator = mock( + Converter.class); + provider.setResponseValidator(responseValidator); + provider.setAssertionValidator(assertionValidator); + Saml2AuthenticationToken token = token(response, verifying(registration())); + assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token)) + .satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE)); + verifyNoInteractions(responseValidator, assertionValidator); + } + + @Test + public void authenticateWhenAssertionSignatureInvalidThenSkipsThatAssertionValidationOnly() { + Response response = response(); + Assertion bad = assertion(); + bad.setID("bad-assertion"); + TestOpenSamlObjects.signed(bad, TestSaml2X509Credentials.relyingPartyDecryptingCredential(), + RELYING_PARTY_ENTITY_ID); + response.getAssertions().add(bad); + Assertion good = assertion(); + good.setID("good-assertion"); + response.getAssertions().add(signed(good)); + OpenSaml5AuthenticationProvider provider = new OpenSaml5AuthenticationProvider(); + Converter assertionValidator = mock( + Converter.class); + given(assertionValidator.convert(any(OpenSaml5AuthenticationProvider.AssertionToken.class))) + .willReturn(Saml2ResponseValidatorResult.success()); + provider.setAssertionValidator(assertionValidator); + Saml2AuthenticationToken token = token(signed(response), verifying(registration())); + assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token)) + .satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE)); + ArgumentCaptor captor = ArgumentCaptor + .forClass(OpenSaml5AuthenticationProvider.AssertionToken.class); + verify(assertionValidator).convert(captor.capture()); + assertThat(captor.getValue().getAssertion().getID()).isEqualTo("good-assertion"); + } + @Test public void authenticateWhenOpenSAMLValidationErrorThenThrowAuthenticationException() { Response response = response(); @@ -512,7 +559,7 @@ public class OpenSaml5AuthenticationProviderTests { EncryptedAssertion encryptedAssertion = TestOpenSamlObjects.encrypted(assertion(), TestSaml2X509Credentials.assertingPartyEncryptingCredential()); response.getEncryptedAssertions().add(encryptedAssertion); - Saml2AuthenticationToken token = token(signed(response), registration() + Saml2AuthenticationToken token = token(signed(response), verifying(registration()) .decryptionX509Credentials((c) -> c.add(TestSaml2X509Credentials.assertingPartyPrivateCredential()))); assertThatExceptionOfType(Saml2AuthenticationException.class) .isThrownBy(() -> this.provider.authenticate(token))