SEC-3108: DigestAuthenticationFilter should use SecurityContextHolder.createEmptyContext()

[spring-projects-issues]

Kristian Laakkonen (Migrated from SEC-3108) said:

When using Digest authentication and there are concurrent requests from the same session, it is possible that the Authentication object returned by SessionContextHolder.getContext().getAuthentication() does not have its authorities set. Then, e.g. HttpServletRequest.isUserInRole() returns sometimes incorrectly false even if the user actually has that role.

This happens because with Digest authentication, constructing the Authentication object is done in two steps, but the incomplete Authentication object is made visible to another threads after first step.

Consider the following scenario:

• User logs in to the system, a session is created, JSESSIONID is set, and user uses the system for some time.
• User makes request 1 to the server. This request has a header "Authorization: Digest ..."
• DigestAuthenticationFilter starts the authentication process and places an incomplete Authentication object (without authorities) into SecurityContext.
• Simultaneously with request 1, the same user makes request 2 to the server. This request does not have "Authorization: Digest ..." header, so no re-authentication is done. While processing request 2, request.isUserInRole() returns now false, because the Authentication object in SecurityContext does not have any authorities. User gets incorrectly a 403 Forbidden response.
• Request 1 continues the authentication process, and in AbstractSecurityInterceptor, a fully populated Authentication object is created and put into SecurityContext.

I could set createAuthenticatedToken = true in DigestAuthenticationFilter, but the comment on the set method says that user flags like isEnabled() or isAccountNonExpired() are not checked in that case, so this is not a solution.

[spring-projects-issues]

Rob Winch said:

Can you expand on how

DigestAuthenticationFilter provides an incomplete Authentication object (without authorities) into SecurityContext?

As far as I can tell this would only happen if the provided UserDetailsService returns a UserDetails without any granted authorities. If that is the case, then it is the responsibility of the UserDetailsService to return the correct roles for the user.

I'm also interested in why and how AbstractSecurityInterceptor is populating the Authentication object with roles. Why wouldn't you just ensure that DigestAuthenticationFilter provides the complete Authentication?

[spring-projects-issues]

Kristian Laakkonen said:

Can you expand on how

DigestAuthenticationFilter provides an incomplete Authentication object (without authorities) into SecurityContext?

As far as I can tell this would only happen if the provided UserDetailsService returns a UserDetails without any granted authorities. If that is the case, then it is the responsibility of the UserDetailsService to return the correct roles for the user.

The Authentication object is constructed from a UserDetails object in DigestAuthenticationFilter.java, line 233. If createAuthenticatedToken is false, the authorities are not copied from UserDetails, and the Authentication object is created without authorities.

I'm also interested in why and how AbstractSecurityInterceptor is populating the Authentication object with roles. Why wouldn't you just ensure that DigestAuthenticationFilter provides the complete Authentication?

AbstractSecurityInterceptor calls AuthenticationManager.authenticate() in AbstractSecurityInterceptor.java, line 353 and finally we end up in AbstractUserDetailsAuthenticationProvider.java, line 125 where a new Authentication token is created with the authorities set.

I don't know why the full authentication process is not carried out in DigestAuthenticationFilter and AuthenticationManager is not used there and instead UserDetailsService is accessed directly. Actually Spring Security Reference says that UserDetailsService is needed because ??DigestAuthenticationFilter must have direct access to the clear text password of a user??.

[spring-projects-issues]

Rob Winch said:

Thank you for the explanation. Can you set createAuthenticatedToken to true?

[spring-projects-issues]

Kristian Laakkonen said:

Setting createAuthenticatedToken to true would mean that disabled users could log in, which is not acceptable. According to documentation and code it seems that the isEnabled() flag of UserDetails is not checked in that case (I have not actually tested it).

[spring-projects-issues]

Rob Winch said:

So getting the the heart of the problem...it sounds as though you need to be able to inject a UserDetailsChecker to verify the state of the user details. Does that sound right?

One workaround is to perform this verification in the UserDetailsService itself. This would ensure that you were able to get past your issue until we can get a release out.

[spring-projects-issues]

Kristian Laakkonen said:

Being able to inject a UserDetailsChecker and then setting createAuthenticatedToken to true would probably solve the problem in my case, yes. But the original concurrency problem is still there... I think there should be at least some documentation then that createAuthenticatedToken must be true if there can be concurrent requests.

[spring-projects-issues]

Rob Winch said:

Honestly, I'm still a little confused on the concurrency issue.

I understand that in request 1 the Authentication gets set on the SecurityContextHolder without roles. However, the SecurityContext will not be set in HttpSession until the HttpResponse is committed.

Without the response being committed the client will not be able to access the JSESSIONID to make request 2 using the HttpSession from request 1. Even if the client knew the JSESSIONID, the SecurityContext would be empty until the HttpResponse was committed since the value from SecurityContextHolder is not written to HttpSession until the HttpResponse is committed.

So my question is, "What is committing the HttpResponse before AbstractSecurityInterceptor is invoked?"

[spring-projects-issues]

Kristian Laakkonen said:

In my example the user has already been authenticated and the session has been created before requests 1 and 2 so SecurityContext and JSESSIONID are already available.

Browsers may send the Digest Authorization header even after the user has already been authenticated. This causes the DigestAuthenticationFilter to do the authentication procedure again and if there are two simultaneous requests that send the header, the problem appears.

[spring-projects-issues]

Rob Winch said:

Thanks for the explanation. The reason this is causing an issue is due to the fact that:

SecurityContextHolder.getContext().setAuthentication(...);

is being used. This would not be an issue when a new session is created. However, since the session already exists it is updating the same SecurityContext instance that the other sessions are referring to. If we use

SecurityContext context = SecurityContextHolder.createEmptyContext();
context.setAuthentication(...);
SecurityContextHolder.setContext(context);

we would no longer be updating the SecurityContext other threads are referring to. This would mean the SecurityContext with no authorities would not be available until the response was committed (after the roles were then populated).

[spring-projects-issues]

Kristian Laakkonen said:

Seems good to me. Thanks for taking time to investigate this.

[spring-projects-issues]

Rob Winch said:

Thanks for the report. This is now fixed in 3.2.x, 4.0.x, and master