SEC-37: HttpSessionContextIntegrationFilter concurrent request handling

[spring-projects-issues]

Ben Alex(Migrated from SEC-37) said:

See http://forum.springframework.org/viewtopic.php?t=7104

Basically if more than one request comes into a web container for the same browser, both requests will be for the same jsessionid. If the requests finish in a different order from which they are received, any authentication action that occurs in the faster-to-complete request will be overwritten by the slower-to-complete request.

The fix is to only ever write to HttpSession if the SecurityContextHolder.getContext() has actually changed during the request. As such, in the above case the slower-to-complete request will not have changed its SecurityContextHolder and therefore would not have overwritten the faster-to-complete request’s modification of SecurityContextHolder and thus persistence in HttpSession.

I have no problem with this change, it makes sense, we just need to ensure test coverage carefully checks its correct operation.

[spring-projects-issues]

Ben Alex said:

Decided to implement it via a hashCode() method on SecurityContextImpl. Using an object reference as suggested in the thread resulted in the object being referenced being modified by reference, so using a hashCode int was safer.

[spring-projects-issues]

Bryan Loofbourrow said:

I discovered this Closed defect by surprise, looking through ACEGI source, when I wanted to find out why my changed Authentication was not being written to the session. I understand the general idea that you want to only write the security context if it has changed; I even commend it. But the use of hash code to accomplish the purpose doesn’t seem right, at least if it’s the only mechanism:

1) Hash codes, by definition, are a compressed value; that is, multiple states of an object can map to the same hash. I don’t see how, in general, hash codes can be used to judge whether an object has changed — they’re just identifying when two things are definitely not the same.

2) If you say, well, if you don’t override hash code, then it’s just the pointer value (is it? I don’t know), so it works anyway, I have two concerns:
a) there might be target systems (e.g. 64 bit) for which there is some small probability of that not being true
b) If an acegi user implements hashCode for his/her Authentication, then that assurance goes away, and you’re relying on the hashcode always reflecting changes. Mostly, it will, if properly implemented. But occasionally, it won’t, regardless of the quality of the implementation. This seems like an invitation to elusive bugs. Also, there is no warning, in the Javadoc for Authentication or SecurityContext, that one should consider this issue when overriding hashCode().

Any thoughts? Thanks.

[spring-projects-issues]

Ben Alex said:

From a HSCIF perspective, we’re only interested if the SecurityContext instance is different from the existing SecurityContext instance. This is because a different instance would indicate a modification that should be persisted to HttpSession (assuming there are not properties on HSCIF indicating we should not persist, as would be the case in situations such as BASIC authentication).

It is true that we expect Authentication to provide a hashCode() that is not based on memory address but rather a hash of the relevant instance fields. It is also true that the existing Authentication might have a hashCode() equal to x and even after modifying it, the new hashCode() is still equal to x. However, that seems extraordinarily unlikely for a correct implementation of hashCode() that delegates to the hashCode() method of each instance field as part of its computation.

As an aside, the most common usage scenario is the SecurityContext contains an AnonymousAuthenticationToken, which is varied to another Authentication instance when somebody logs in. Custom instances can consider extending AbstractAuthenticationToken if they’d like a hashCode() method that includes most common authentication instance fields.

You’ll be pleased to know we have a unit test that actually goes off and creates dozens of threads and does various modification logic. In the years we’ve been using that test, they’ve never failed due to different objects returning the same hash code.

The real issue is what is the alternative? As noted in my comment of 21 October 2005, we cannot simply use Object.equals(Object) because we’d need to clone the SecurityContext in order to undertake a proper comparison. That would impose considerable cloning costs at runtime (if we cloned via the typical byte array copying method) or complicate existing implementations (if we required them to provide an acceptable clone method instead).

Hope this gives some background. Can I ask whether you encountered an actual issue with your custom Authentication object not being persisted by HSCIF and were unable to correct it by using an existing Authentication instance as a guide to implementation (or extending AbstractAuthenticationToken)?

[spring-projects-issues]

Bryan Loofbourrow said:

I encountered an actual issue because I had modified my custom Authentication object, and had not overridden hashCode, so of course my change to the object was undetectable. I implemented a proper hashCode() and the problem went away. The only remaining behavioral problem was, I concede, prospective — the remote possibility of a situation in which the Authentication would not be persisted. I admit that I do not routinely override hashCode/equals on objects for which I have no expectation of comparing or mapping. Perhaps that is an unusual and inappropriate practice on my part. But if it’s not, the behavior does come as a surprise to a coder, and it might be worthwhile to allude to the issue in Javadocs.

As for alternatives, one can imagine making the whole issue explicit with setChanged/isChanged method on the SecurityContext and/or Authentication. That would at least give some opportunity to explain in the Javadocs, and give an opportunity for custom objects to explicitly call out changes. It would also break back compatibility with these interfaces, though, so I can understand why you’d not be eager to do it. The next best thing would be to simply put an isChanged method on SecurityContextImpl. Have the default implementation do what it does now, and allow me to override with whatever I want to use (which would be the explicit transient changed flag I describe above). Right now this decision is made in the middle of a long method, and I am loathe to copy the whole method in an overridden version, just to change this one thing.

That approach of splitting out this decision to a separate method in the Impl would not break back compatibility or change current behavior, it would just open up a way for application programmers to control this decision more explicitly.

I take some reassurance from the tests that you describe, but perhaps not enough for my situation: high-volume systems with high availability requirements. The interesting question for me is, how often might I expect to see such a failure in one of my installations?" If the answer is “when the sun burns out,” or even “once every 10 years,” then I’m fine with it. But even “once every 6 months” would present me with an unacceptable reliability problem. Bottom line at this point is that I don’t have the data to make the calculation, but of course I’d like to take an approach in which, having identified the issue, I’ve taken steps to protect myself from it, rather than waiting and hoping it won’t bite me, or putting a lot of effort into trying to guess how vulnerable I might be, and hoping I got it right.

[spring-projects-issues]

Bryan Loofbourrow said:

To respond and elaborate further, while I agree the clone business is untenable, I am quite tempted by something like:

if (context != null) {
if (context == previousContext) {
if (context.isChanged(previousContextHashCode))
// update session
}
} else if (!context.equals(previousContext)) {
// update session
}
} else if (previousContext != null) {
// remove session
}

I think this is getting toward the true nature of things: if it’s a different object, the interesting question is whether the new object is equal to the old object (or you could just write it anyway, if it’s not important to detect equality in this case). If it’s the same object, then rely on the implementation to tell you that it has changed in place, with the hashcode-comparison as a backstop implementation if the application developer hasn’t chosen to override with some more deterministic indicator of in-place changes.