Skip to content

Commit e50c2a6

Browse files
jzheauxjzheaux
committed
Rework Login Validation Logic
This commit simplifies the validation code, favoring the construction of a list of error codes over using an accumulating Saml2ResponseValidationResult. This will simplify future analysis by making the code more readable. Signed-off-by: Josh Cummings <3627351+jzheaux@users.noreply.github.com>
1 parent 356131b commit e50c2a6

3 files changed

Lines changed: 135 additions & 26 deletions

File tree

‎saml2/saml2-service-provider/src/main/java/org/springframework/security/saml2/provider/service/authentication/BaseOpenSamlAuthenticationProvider.java‎

Lines changed: 37 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -326,62 +326,75 @@ private void process(Saml2AuthenticationToken token, Response response) {
326326
boolean responseSigned = response.isSigned();
327327

328328
ResponseToken responseToken = new ResponseToken(response, token);
329-
Saml2ResponseValidatorResult result = this.responseSignatureValidator.convert(responseToken);
329+
Collection<Saml2Error> responseSignatureErrors = this.responseSignatureValidator.convert(responseToken)
330+
.getErrors();
331+
if (!responseSignatureErrors.isEmpty()) {
332+
reportErrors(response, responseSignatureErrors);
333+
return;
334+
}
335+
336+
Collection<Saml2Error> errors = new ArrayList<>();
330337
if (responseSigned) {
331338
this.responseElementsDecrypter.accept(responseToken);
332339
}
333340
else if (!response.getEncryptedAssertions().isEmpty()) {
334-
result = result.concat(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE,
341+
errors.add(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE,
335342
"Did not decrypt response [" + response.getID() + "] since it is not signed"));
336343
}
337344
if (!this.validateResponseAfterAssertions) {
338-
result = result.concat(this.responseValidator.convert(responseToken));
345+
errors.addAll(this.responseValidator.convert(responseToken).getErrors());
339346
}
340347
boolean allAssertionsSigned = true;
341348
for (Assertion assertion : response.getAssertions()) {
342349
AssertionToken assertionToken = new AssertionToken(assertion, token);
343-
result = result.concat(this.assertionSignatureValidator.convert(assertionToken));
350+
Collection<Saml2Error> assertionSignatureErrors = this.assertionSignatureValidator.convert(assertionToken)
351+
.getErrors();
352+
errors.addAll(assertionSignatureErrors);
344353
allAssertionsSigned = allAssertionsSigned && assertion.isSigned();
354+
if (!assertionSignatureErrors.isEmpty()) {
355+
continue;
356+
}
345357
if (responseSigned || assertion.isSigned()) {
346358
this.assertionElementsDecrypter.accept(new AssertionToken(assertion, token));
347359
}
348-
result = result.concat(this.assertionValidator.convert(assertionToken));
360+
errors.addAll(this.assertionValidator.convert(assertionToken).getErrors());
349361
}
350362
if (!responseSigned && !allAssertionsSigned) {
351363
String description = "Either the response or one of the assertions is unsigned. "
352364
+ "Please either sign the response or all of the assertions.";
353-
result = result.concat(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE, description));
365+
errors.add(new Saml2Error(Saml2ErrorCodes.INVALID_SIGNATURE, description));
354366
}
355367
if (this.validateResponseAfterAssertions) {
356-
result = result.concat(this.responseValidator.convert(responseToken));
368+
errors.addAll(this.responseValidator.convert(responseToken).getErrors());
357369
}
358370
else {
359371
Assertion firstAssertion = CollectionUtils.firstElement(response.getAssertions());
360372
if (firstAssertion != null && !hasName(firstAssertion)) {
361-
Saml2Error error = new Saml2Error(Saml2ErrorCodes.SUBJECT_NOT_FOUND,
362-
"Assertion [" + firstAssertion.getID() + "] is missing a subject");
363-
result = result.concat(error);
373+
errors.add(new Saml2Error(Saml2ErrorCodes.SUBJECT_NOT_FOUND,
374+
"Assertion [" + firstAssertion.getID() + "] is missing a subject"));
364375
}
365376
}
366377

367-
if (result.hasErrors()) {
368-
Collection<Saml2Error> errors = result.getErrors();
369-
if (this.logger.isTraceEnabled()) {
370-
this.logger.trace("Found " + errors.size() + " validation errors in SAML response [" + response.getID()
371-
+ "]: " + errors);
372-
}
373-
else if (this.logger.isDebugEnabled()) {
374-
this.logger
375-
.debug("Found " + errors.size() + " validation errors in SAML response [" + response.getID() + "]");
376-
}
377-
Saml2Error first = errors.iterator().next();
378-
throw createAuthenticationException(first.getErrorCode(), first.getDescription(), null);
379-
}
380-
else {
378+
reportErrors(response, errors);
379+
}
380+
381+
private void reportErrors(Response response, Collection<Saml2Error> errors) {
382+
if (errors.isEmpty()) {
381383
if (this.logger.isDebugEnabled()) {
382384
this.logger.debug("Successfully processed SAML Response [" + response.getID() + "]");
383385
}
386+
return;
387+
}
388+
if (this.logger.isTraceEnabled()) {
389+
this.logger.debug("Found " + errors.size() + " validation errors in SAML response [" + response.getID()
390+
+ "]: " + errors);
391+
}
392+
else if (this.logger.isDebugEnabled()) {
393+
this.logger
394+
.debug("Found " + errors.size() + " validation errors in SAML response [" + response.getID() + "]");
384395
}
396+
Saml2Error first = errors.iterator().next();
397+
throw createAuthenticationException(first.getErrorCode(), first.getDescription(), null);
385398
}
386399

387400
private Converter<ResponseToken, Saml2ResponseValidatorResult> createDefaultResponseSignatureValidator() {

‎saml2/saml2-service-provider/src/opensaml4Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProviderTests.java‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333

3434
import com.fasterxml.jackson.databind.ObjectMapper;
3535
import org.junit.jupiter.api.Test;
36+
import org.mockito.ArgumentCaptor;
3637
import org.opensaml.core.xml.XMLObject;
3738
import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport;
3839
import org.opensaml.core.xml.schema.XSDateTime;
@@ -87,6 +88,7 @@
8788
import static org.mockito.Mockito.atLeastOnce;
8889
import static org.mockito.Mockito.mock;
8990
import static org.mockito.Mockito.verify;
91+
import static org.mockito.Mockito.verifyNoInteractions;
9092

9193
/**
9294
* Tests for {@link OpenSaml4AuthenticationProvider}
@@ -173,6 +175,53 @@ public void authenticateWhenInvalidSignatureOnAssertionThenThrowAuthenticationEx
173175
.satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE));
174176
}
175177

178+
@Test
179+
public void authenticateWhenResponseSignatureInvalidThenSkipsResponseAndAssertionValidation() {
180+
Response response = response();
181+
response.setDestination(DESTINATION + "invalid");
182+
Assertion assertion = assertion();
183+
response.getAssertions().add(signed(assertion));
184+
// Sign the response with a credential the verifier does not trust
185+
TestOpenSamlObjects.signed(response, TestSaml2X509Credentials.relyingPartyDecryptingCredential(),
186+
RELYING_PARTY_ENTITY_ID);
187+
OpenSaml4AuthenticationProvider provider = new OpenSaml4AuthenticationProvider();
188+
Converter<ResponseToken, Saml2ResponseValidatorResult> responseValidator = mock(Converter.class);
189+
Converter<OpenSaml4AuthenticationProvider.AssertionToken, Saml2ResponseValidatorResult> assertionValidator = mock(
190+
Converter.class);
191+
provider.setResponseValidator(responseValidator);
192+
provider.setAssertionValidator(assertionValidator);
193+
Saml2AuthenticationToken token = token(response, verifying(registration()));
194+
assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token))
195+
.satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE));
196+
verifyNoInteractions(responseValidator, assertionValidator);
197+
}
198+
199+
@Test
200+
public void authenticateWhenAssertionSignatureInvalidThenSkipsThatAssertionValidationOnly() {
201+
Response response = response();
202+
Assertion bad = assertion();
203+
bad.setID("bad-assertion");
204+
TestOpenSamlObjects.signed(bad, TestSaml2X509Credentials.relyingPartyDecryptingCredential(),
205+
RELYING_PARTY_ENTITY_ID);
206+
response.getAssertions().add(bad);
207+
Assertion good = assertion();
208+
good.setID("good-assertion");
209+
response.getAssertions().add(signed(good));
210+
OpenSaml4AuthenticationProvider provider = new OpenSaml4AuthenticationProvider();
211+
Converter<OpenSaml4AuthenticationProvider.AssertionToken, Saml2ResponseValidatorResult> assertionValidator = mock(
212+
Converter.class);
213+
given(assertionValidator.convert(any(OpenSaml4AuthenticationProvider.AssertionToken.class)))
214+
.willReturn(Saml2ResponseValidatorResult.success());
215+
provider.setAssertionValidator(assertionValidator);
216+
Saml2AuthenticationToken token = token(signed(response), verifying(registration()));
217+
assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token))
218+
.satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE));
219+
ArgumentCaptor<OpenSaml4AuthenticationProvider.AssertionToken> captor = ArgumentCaptor
220+
.forClass(OpenSaml4AuthenticationProvider.AssertionToken.class);
221+
verify(assertionValidator).convert(captor.capture());
222+
assertThat(captor.getValue().getAssertion().getID()).isEqualTo("good-assertion");
223+
}
224+
176225
@Test
177226
public void authenticateWhenOpenSAMLValidationErrorThenThrowAuthenticationException() {
178227
Response response = response();
@@ -502,7 +551,7 @@ public void authenticateWhenDecryptionKeysAreWrongThenThrowAuthenticationExcepti
502551
EncryptedAssertion encryptedAssertion = TestOpenSamlObjects.encrypted(assertion(),
503552
TestSaml2X509Credentials.assertingPartyEncryptingCredential());
504553
response.getEncryptedAssertions().add(encryptedAssertion);
505-
Saml2AuthenticationToken token = token(signed(response), registration()
554+
Saml2AuthenticationToken token = token(signed(response), verifying(registration())
506555
.decryptionX509Credentials((c) -> c.add(TestSaml2X509Credentials.assertingPartyPrivateCredential())));
507556
assertThatExceptionOfType(Saml2AuthenticationException.class)
508557
.isThrownBy(() -> this.provider.authenticate(token))

‎saml2/saml2-service-provider/src/opensaml5Test/java/org/springframework/security/saml2/provider/service/authentication/OpenSaml5AuthenticationProviderTests.java‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434

3535
import com.fasterxml.jackson.databind.ObjectMapper;
3636
import org.junit.jupiter.api.Test;
37+
import org.mockito.ArgumentCaptor;
3738
import org.opensaml.core.xml.XMLObject;
3839
import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport;
3940
import org.opensaml.core.xml.schema.XSDateTime;
@@ -183,6 +184,52 @@ public void authenticateWhenInvalidSignatureOnAssertionThenThrowAuthenticationEx
183184
.satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE));
184185
}
185186

187+
@Test
188+
public void authenticateWhenResponseSignatureInvalidThenSkipsResponseAndAssertionValidation() {
189+
Response response = response();
190+
response.setDestination(DESTINATION + "invalid");
191+
Assertion assertion = assertion();
192+
response.getAssertions().add(signed(assertion));
193+
TestOpenSamlObjects.signed(response, TestSaml2X509Credentials.relyingPartyDecryptingCredential(),
194+
RELYING_PARTY_ENTITY_ID);
195+
OpenSaml5AuthenticationProvider provider = new OpenSaml5AuthenticationProvider();
196+
Converter<ResponseToken, Saml2ResponseValidatorResult> responseValidator = mock(Converter.class);
197+
Converter<OpenSaml5AuthenticationProvider.AssertionToken, Saml2ResponseValidatorResult> assertionValidator = mock(
198+
Converter.class);
199+
provider.setResponseValidator(responseValidator);
200+
provider.setAssertionValidator(assertionValidator);
201+
Saml2AuthenticationToken token = token(response, verifying(registration()));
202+
assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token))
203+
.satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE));
204+
verifyNoInteractions(responseValidator, assertionValidator);
205+
}
206+
207+
@Test
208+
public void authenticateWhenAssertionSignatureInvalidThenSkipsThatAssertionValidationOnly() {
209+
Response response = response();
210+
Assertion bad = assertion();
211+
bad.setID("bad-assertion");
212+
TestOpenSamlObjects.signed(bad, TestSaml2X509Credentials.relyingPartyDecryptingCredential(),
213+
RELYING_PARTY_ENTITY_ID);
214+
response.getAssertions().add(bad);
215+
Assertion good = assertion();
216+
good.setID("good-assertion");
217+
response.getAssertions().add(signed(good));
218+
OpenSaml5AuthenticationProvider provider = new OpenSaml5AuthenticationProvider();
219+
Converter<OpenSaml5AuthenticationProvider.AssertionToken, Saml2ResponseValidatorResult> assertionValidator = mock(
220+
Converter.class);
221+
given(assertionValidator.convert(any(OpenSaml5AuthenticationProvider.AssertionToken.class)))
222+
.willReturn(Saml2ResponseValidatorResult.success());
223+
provider.setAssertionValidator(assertionValidator);
224+
Saml2AuthenticationToken token = token(signed(response), verifying(registration()));
225+
assertThatExceptionOfType(Saml2AuthenticationException.class).isThrownBy(() -> provider.authenticate(token))
226+
.satisfies(errorOf(Saml2ErrorCodes.INVALID_SIGNATURE));
227+
ArgumentCaptor<OpenSaml5AuthenticationProvider.AssertionToken> captor = ArgumentCaptor
228+
.forClass(OpenSaml5AuthenticationProvider.AssertionToken.class);
229+
verify(assertionValidator).convert(captor.capture());
230+
assertThat(captor.getValue().getAssertion().getID()).isEqualTo("good-assertion");
231+
}
232+
186233
@Test
187234
public void authenticateWhenOpenSAMLValidationErrorThenThrowAuthenticationException() {
188235
Response response = response();
@@ -512,7 +559,7 @@ public void authenticateWhenDecryptionKeysAreWrongThenThrowAuthenticationExcepti
512559
EncryptedAssertion encryptedAssertion = TestOpenSamlObjects.encrypted(assertion(),
513560
TestSaml2X509Credentials.assertingPartyEncryptingCredential());
514561
response.getEncryptedAssertions().add(encryptedAssertion);
515-
Saml2AuthenticationToken token = token(signed(response), registration()
562+
Saml2AuthenticationToken token = token(signed(response), verifying(registration())
516563
.decryptionX509Credentials((c) -> c.add(TestSaml2X509Credentials.assertingPartyPrivateCredential())));
517564
assertThatExceptionOfType(Saml2AuthenticationException.class)
518565
.isThrownBy(() -> this.provider.authenticate(token))

0 commit comments

Comments
 (0)