Add support for validation of InResponseTo attribute when validating SAML2 responses #10849

fast-reflexes · 2022-02-19T10:58:27Z

… a SAML2 response given that this attribute is set and that the request can be found.

#9174

What do you think so far?

Left to do that I know of:

Perhaps add some test
Go through submission guidelines again

pivotal-cla · 2022-02-19T10:58:30Z

@fast-reflexes Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla · 2022-02-19T11:01:00Z

@fast-reflexes Thank you for signing the Contributor License Agreement!

jzheaux

Thanks, @fast-reflexes! I've left some feedback inline.

Additionally, will you please format your commit message like this:

Commit Title

Closes gh-9174

...-service-provider/src/main/java/org/springframework/security/saml2/core/Saml2ErrorCodes.java

...ework/security/saml2/provider/service/authentication/AbstractSaml2AuthenticationRequest.java

...framework/security/saml2/provider/service/authentication/OpenSamlAuthenticationProvider.java

...ramework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProvider.java

fast-reflexes · 2022-02-26T11:14:57Z

@jzheaux I updated the branch and also included eviction of stale stored requests.

Updated behaviour:

If valid request exists and InResponseTo is set in response, it is validated
If no valid request exists and InResponseTo is set in response, it is rejected
If valid stored request exists and no InResponseTo in response, it is rejected
A stored request that is more than 30 seconds old is considered invalid

Questions:

Do we need more tests in OpenSamlAuthenticationRequestResolverTests?
What is the purpose of including the response argument in HttpSessionSaml2AuthenticationRequestRepository? (removeAuthenticationRequest(HttpServletRequest request, HttpServletResponse response))
Any comments on the use of Long or long for issuingInstantEpochSeconds?
I see some different practices; sometimes Instant.now() is used, other times a Clock is stored in the class and Instant.now(clock) is used, yet other times, clock.millis() is used.. any preference?
Do we need to think about log out requests in connection to the changes in this PR?

jzheaux

Thanks, @fast-reflexes! I've left some feedback inline.

In addition to that feedback, will you do me the favor of making your commit title just a tad shorter? ~50 characters is ideal. This helps when scanning through Git logs.

...ramework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProvider.java

...curity/saml2/provider/service/web/authentication/OpenSaml4AuthenticationRequestResolver.java

...ework/security/saml2/provider/service/authentication/AbstractSaml2AuthenticationRequest.java

fast-reflexes · 2022-03-05T12:04:17Z

I updated based on your feedback.

What I mean is that converting the request to a string and then decoding it is less straightforward than storing the domain representation of a request (be it locked down to the OpenSAML implementation or some thin wrapper around it). Transforming it into a string is a preparation for the actual outgoing request and therefore something that I think should be done right before sending the actual request. It also makes testing more cumbersome when the request is stored as a string.
I really think we should have a flag / property for whether applications want to use validation of InResponseTo or not. Of course they can add their own validators and so on but it's more cumbersome than just opting in or out of such a feature via a Spring property.

These are my remarks as a programmer and a Spring user :) In here, I'm nothing but someone who wants to get more involved and learn to contribute so I respect your decisions to 100% and without debate, just letting you know what I think :)

Let me know what you think of the current state of the code :)

jzheaux

Thanks, @fast-reflexes! Glad this is taking shape. I've left some feedback inline.

...-service-provider/src/main/java/org/springframework/security/saml2/core/Saml2ErrorCodes.java

...ramework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProvider.java

...ork/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProviderTests.java

...ramework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProvider.java

...ork/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProviderTests.java

jzheaux · 2022-03-07T23:28:13Z

What I mean is that converting the request to a string and then decoding it is less straightforward than storing the domain representation of a request (be it locked down to the OpenSAML implementation or some thin wrapper around it).

I see your point, but not how to apply it. AuthnRequest is not Serializable, so how would you expect the AuthnRequest to be able to be stored in the session?

I really think we should have a flag / property for whether applications want to use validation of InResponseTo or not.

We can always add this in a separate PR down the road. I think boolean properties should be added quite slowly and with more usage data than we have now. You are welcome to add a ticket and then we can see if the community votes for it.

fast-reflexes · 2022-03-10T08:58:30Z

I see your point, but not how to apply it. AuthnRequest is not Serializable, so how would you expect the AuthnRequest to be able to be stored in the session?

Aha.. so THAT'S the problem ... hmm .. ok, then I'll rest my case for now!

Factored out repeatedly used code for signing a request.

fast-reflexes · 2022-03-11T06:06:45Z

...ramework/security/saml2/provider/service/authentication/OpenSaml4AuthenticationProvider.java

+			context.put(SAML2AssertionValidationParameters.SC_VALID_IN_RESPONSE_TO, requestId);
+		}
+		else {
+			context.put(SAML2AssertionValidationParameters.SC_VALID_IN_RESPONSE_TO, "");


Here we set the empty string so that we know that the validation will fail (since the very prerequisite for even doing the validation is that the some of the assertion subject confirmation has a non-empty InResponseTo). The reason for failing is either that there was no stored request, it could not be deserialized or it lacked id. You think this solution is ok?

Could trigger a false positive if we have multiple confirmations and one has the (invalid) empty string as InResponseTo whereas a later one has a proper one ...

Perhaps better to stay with the override anyways?

Won't be more code than is added by this solution

Easier to understand what's going on

Will cover all cases since we can control directly whether to consider valid or not

I think you can just leave it unset. The OpenSAML code works as follows:

If no InResponseTo in subject confirmation -> valid

Or, if no SC_VALID_IN_RESPONSE_TO set -> invalid

Or, if SC_VALID_IN_RESPONSE_TO mismatches InResponseTo -> invalid

Could trigger a false positive

If any assertions are wrong, then we should fail. We don't accept a response that is in response to multiple requests.

override anyways ... Easier to understand what's going on

Ideally, we don't write code that we don't have to as this lowers maintenance. I don't see why it's easier to understand what is going on. The code that you have written is virtually the same as OpenSAML's BearerSubjectConfirmationValidator#validateInResponseTo.

Yeah of course, can be unset, I don't know what I was thinking... and what I meant by

Could trigger a false positive if we have multiple confirmations and one has the (invalid) empty string as
InResponseTo whereas a later one has a proper one ...

was that if an assertion has a subject confirmation with the empty string as InResponseTo and SC_VALID_IN_RESPONSE_TO was set to the empty string due to problems with deserialization, then the subject confirmation would falsely be valid no matter the value of the actual request id. I was talking about multiple confirmations (not assertions) because apparently it seems it is enough with one confirmation passing the test on the subject (see SAML20AssertionValidator::validateSubjectConfirmation). I now realized that the false positive does not require two confirmations even, just one with the empty string. However, since we can leave it unset, it should not be a problem.

I've updated the code, let me know what you think!

Whenever an InResponseTo is present in the SAML2 response and / or any of its assertions, it will be validated against the stored SAML2 request. If the request is missing or the ID of the request does not match the InResponseTo, validation fails. If there is no InResponseTo, no validation of it is done (as opposed to checking whether there is a saved request or not and then failing based on that). Closes spring-projectsgh-9174

jzheaux · 2022-03-15T20:28:52Z

Thanks, @fast-reflexes! I've merged your commits into 5.7.x and into main. I also added a polish commit to make the code more consistent with the rest of Spring Security.

fast-reflexes · 2022-03-16T17:14:03Z

Great, thanks @jzheaux ! I looked into your changes to learn the style... what do you mean by

Refactored methods to have only immutable method parameters

Just curious :)

fast-reflexes mentioned this pull request Feb 19, 2022

Add InResponseTo validation support #9174

Closed

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 19, 2022

eleftherias assigned jzheaux Feb 21, 2022

eleftherias added in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 21, 2022

jzheaux requested changes Feb 22, 2022

View reviewed changes

fast-reflexes force-pushed the gh-9174 branch from c3456d0 to 5dc183d Compare February 26, 2022 11:09

fast-reflexes changed the title ~~-Add support for validation of InResponseTo attribute when validating…~~ Add support for validation of InResponseTo attribute when validating SAML2 responses Feb 26, 2022

jzheaux requested changes Feb 28, 2022

View reviewed changes

fast-reflexes force-pushed the gh-9174 branch from 4584492 to 299dc74 Compare March 5, 2022 11:58

jzheaux requested changes Mar 7, 2022

View reviewed changes

Refactored OpenSaml4AuthenticationProviderTests

1c0302e

Factored out repeatedly used code for signing a request.

fast-reflexes force-pushed the gh-9174 branch 2 times, most recently from a172bcb to 2f1d161 Compare March 11, 2022 05:54

fast-reflexes commented Mar 11, 2022

View reviewed changes

fast-reflexes force-pushed the gh-9174 branch from 2f1d161 to ceec116 Compare March 12, 2022 09:43

jzheaux closed this Mar 15, 2022

jzheaux added this to the 5.7.0-M3 milestone Mar 15, 2022

fast-reflexes deleted the gh-9174 branch March 16, 2022 17:15

Add support for validation of InResponseTo attribute when validating SAML2 responses #10849

Add support for validation of InResponseTo attribute when validating SAML2 responses #10849

Uh oh!

Conversation

fast-reflexes commented Feb 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pivotal-cla commented Feb 19, 2022

Uh oh!

pivotal-cla commented Feb 19, 2022

Uh oh!

jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fast-reflexes commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fast-reflexes commented Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jzheaux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jzheaux commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fast-reflexes commented Mar 10, 2022

Uh oh!

fast-reflexes Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fast-reflexes Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

fast-reflexes Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

jzheaux Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fast-reflexes Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzheaux commented Mar 15, 2022

Uh oh!

fast-reflexes commented Mar 16, 2022

Uh oh!

Uh oh!

fast-reflexes commented Feb 19, 2022 •

edited

Loading

fast-reflexes commented Feb 26, 2022 •

edited

Loading

fast-reflexes commented Mar 5, 2022 •

edited

Loading

jzheaux commented Mar 7, 2022 •

edited

Loading

fast-reflexes Mar 11, 2022 •

edited

Loading

jzheaux Mar 11, 2022 •

edited

Loading

fast-reflexes Mar 12, 2022 •

edited

Loading