Skip to content
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

SAML2 Provider SubjectConfirmation validation failure #7514

Closed
blucas opened this issue Oct 7, 2019 · 6 comments · Fixed by #7577
Closed

SAML2 Provider SubjectConfirmation validation failure #7514

blucas opened this issue Oct 7, 2019 · 6 comments · Fixed by #7577
Assignees
Labels
in: saml2 An issue in SAML2 modules type: bug A general bug
Milestone

Comments

@blucas
Copy link
Contributor

blucas commented Oct 7, 2019

Summary

When validating the assertion, if the IdP has provided a SubjectConfirmation which matches the Bearer method, the validation will fail. This is due to the fact that the OpenSamlAuthenticationProvider does not set the necessary parameter

SAML2AssertionValidationParameters.SC_VALID_ADDRESSES

This parameter is used to obtain valid address and compare it to what has been provided in the assertion. But as this parameter is not set this code block fails.

Actual Behavior

SubjectConfirmation validation fails.

Expected Behavior

SubjectConfirmation validation should succeed.

Configuration

Version

spring-security-5.2.0
spring-boot-2.2.0-RC1

Sample

@fhanik FYI.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 7, 2019
@fhanik fhanik added in: saml2 An issue in SAML2 modules type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 7, 2019
@fhanik fhanik self-assigned this Oct 7, 2019
@fhanik fhanik added this to the 5.2.1 milestone Oct 7, 2019
@fhanik
Copy link
Contributor

fhanik commented Oct 9, 2019

Thank you for the report. Give me some time to review the different use cases. I would like to get a feel for how common actual address validation is in the majority of scenarios.

Using IP addresses in an environment where IP addresses are becoming very fluid (virtualized environments, cloud, container based) feels a bit like it doesn't apply as much. However, we shouldn't be failing, so what are our options? Do we provide an option to validate or disable that check?

@blucas
Copy link
Contributor Author

blucas commented Oct 9, 2019

I agree that address validation isn't necessarily useful, but if you view the source code for AbstractSubjectConfirmationValidator (super class of BearerSubjectConfirmationValidator) you'll see that it is responsible for validating addresses. Its implementation uses InetAddress.getAllByName(...) which seems to imply that it would support name resolution lookups to compare to the SP's whitelist.

The AbstractSubjectConfirmationValidator.validateAddress method will return ValidationResult.INDETERMINATE if the whitelist isn't populated by the SP and the IdP included an address in the message, but unfortunately the validator is specifically requiring that the validateAddress method returns a VALID

result = validateAddress(confirmation, assertion, context);
if (result != ValidationResult.VALID) {
    return result;
}

From there, OpenSamlAuthenticationProvider will throw a Saml2ErrorCodes.INVALID_ASSERTION as the validator returned ValidationResult.INDETERMINATE instead of ValidationResult.VALID.

In summary, I think the provider should support validation of the address (which may be a hostname or IP) as well as the ability to disable (or ignore) specific ValidationResult.INDETERMINATE results.

#7517 Provides a solution to validate the address

@fhanik
Copy link
Contributor

fhanik commented Oct 9, 2019

I totally agree that it should not fail. Let me digest whether we actually want to perform the check, or disable it.

@blucas
Copy link
Contributor Author

blucas commented Oct 24, 2019

@fhanik - Have you had a chance to look into this yet?

@fhanik
Copy link
Contributor

fhanik commented Oct 24, 2019

Hi @blucas Thanks for the ping. It's on my list for Monday. Right now I'm favoring bypassing the check in its entirety. With more and more systems going to a fluid cloud where addresses become irrelevant that check seems superfluous. The system already does the Destination check.

@blucas
Copy link
Contributor Author

blucas commented Oct 28, 2019

@fhanik - Thanks for the update. Let me know if there is anything I can do to help.

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: bug A general bug
Projects
None yet
3 participants