Proposed fix for missing WWW-Authenticate header #1877
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| @@ -344,7 +355,9 @@ public void init(HttpSecurity httpSecurity) throws Exception { | |||
| ExceptionHandlingConfigurer<HttpSecurity> exceptionHandling = httpSecurity | |||
| .getConfigurer(ExceptionHandlingConfigurer.class); | |||
| if (exceptionHandling != null) { | |||
| exceptionHandling.defaultAuthenticationEntryPointFor(new HttpStatusEntryPoint(HttpStatus.UNAUTHORIZED), | |||
| var entryPoint = new BasicAuthenticationEntryPoint(); | |||
There was a problem hiding this comment.
Please revert the changes in this class and instead add the fix here:
There was a problem hiding this comment.
Please revert the changes in this class and instead add the fix here:
Ah, I see now how this is handled there. I will change this, but as a side note, I'm a bit confused about what the point of adding the HttpStatusEntryPoint in the OAuth2AuthorizationServerConfigurer is in that case. The Spring Boot autoconfiguration seems to override this anyway by registering a LoginUrlEntryPoint; but if the autoconfiguration isn't used and this HttpStatusEntryPoint is registered, it seems to have some odd consequences:
- Per the implementation of org.springframework.security.config.annotation.web.configurers.ExceptionHandlingConfigurer#createDefaultEntryPoint when there's only one mapping, this becomes the default entrypoint for all URLs, not just the ones it was actually registered for - which doesn't seem like it would be the intention here.
- But in fact AFAICT this is irrelevant because I think all of the auth server filters catch AuthenticationExceptions and handle them explicitly rather than allowing them to propagate out to the ExceptionTranslationFilter
So on the one hand I'm puzzled about whether this bit of configuration ever really does anything useful; and on the other I'm concerned that if it does have a purpose it might not really be correct anyway because:
- It will, by default, apply to more than just the URLs it's configured for (effectively any URL handled by this filter chain) and
- It's still not clear to me that it's ever really acceptable to return a 401 without a www-authenticate header according to the HTTP spec: https://datatracker.ietf.org/doc/html/rfc7235#section-3.1
There was a problem hiding this comment.
what the point of adding the
HttpStatusEntryPointin theOAuth2AuthorizationServerConfigureris in that case
By default, client authentication is required for the Token endpoint, Token Introspection endpoint, Token Revocation endpoint and Device Authorization endpoint. Therefore, this is added as a sensible default in case it passes through the OAuth2ClientAuthenticationFilter, which won't happen with the default configuration, but may happen if client authentication is customized and misconfigured. Consider this sensible default as a defense-in-depth measure.
There was a problem hiding this comment.
Right, understood. As currently being discussed on the original issue comments, I'm unconvinced that this is valid without a WWW-Authenticate header, and thus I'd argue that this change should probably stay, in addition to updating the OAuth2ClientAuthenticationFilter
jgrandja
added
type: enhancement
jgrandja
added
type: bug
|
The Spring team has recently migrated to the Developer Certificate of Origin (DCO) for our contribution process. See Submitting Pull Requests for additional details on the new process. Please format the commits in this PR as the DCO check did not pass. |
symposion
force-pushed
the
fix-468
branch
2 times, most recently
from
552a4a0 to
83943b2
Compare
|
@jgrandja This is a bit more radical than I suspect you were thinking. It's not complete but I wanted to get some feedback on the approach. Just modifying OAuth2ClientAuthenticationFilter isn't totally straightforward. The problem is that in the case of missing authentication, we end up throwing an AccessDenied exception from AuthorizationManager, which doesn't really join up with any of the other parts of the Spring Auth Server implementation at the moment. Furthermore, I've noticed that there are several places with somewhat duplicated code handling authentication errors. With this in mind, I've taken some inspiration from the implementation in the OAuth2 resource server, which centralises the handling of both authentication failure and missing authentication by sharing an AuthenticationEntryPoint between the ExceptionTranslationFilter and the BearerTokenAuthenticationFilter to achieve consistency in the way it formats 401 responses across the board. I think this is cleaner than just catching AccessDenied in the OAuth2ClientAuthenticationFilter and completely bypassing all the normal spring security mechanisms for handling this flow. In the case of Spring Auth Server, there's a certain amount of stuff that doesn't really fit in an AuthenticationEntryPoint + there was already an existing class that seemed suitable for re-use : OAuth2ErrorAuthenticationFailureHandler . I've made this a bit more generic and expanded the usage to basically everywhere that handles authentication failures, including a new AuthenticationEntryPoint that defers to it. I think this approach fits better with the way this is handled in other places by Spring security - such as the standard web security, or the resource server implementation; and also results in a bit less code duplication. If you don't completely hate this, then I need to think about whether this handler should actually be a shared object so that the realm can be configured centrally somehow. |
|
@symposion There are too many touch points in the latest updates and I'm very confident that we can isolate the required changes to
For the "no client authentication included" use case, we can add a check here: if (authenticationRequest == null) {
// TODO No client authentication included
// Reuse and enhance the default authenticationFailureHandler to return 401 WWW-Authenticate: Basic
}Makes sense? |
Yes, I can do that. The big downside I see with doing that - and why I didn't propose that - is that we're effectively bypassing all of the normal authorization behaviour of Spring Security. Spring Authorization Server currently uses these mechanisms to define the authentication requirements for its various endpoints. If I make the change you're proposing, anyone who is expecting to be able to customise this in the normal way using At the very least, if we're going to take this approach I think we're going to need some way to disable it so people can choose to have the filter chain proceed as normal down to the AuthorizationFilter if the authentication is null - which would be the normal flow in Spring Security. |
The token endpoint is not intended to be compatible with |
Understood. I will re-implement the simpler way. |
|
@jgrandja This is as minimally invasive as I think I can get it. My residual doubt here is that I'm now applying the WWW-Authenticate header to all INVALID_CLIENT errors, which isn't quite as limited as we discussed on the original ticket. If we want to limit this to only cases where no credentials were provided or client_secret_basic was used, then I think either:
(2) feels pretty ugly to me; but (1) is a bigger change that will touch more classes. Personally I'm not sure that there's much harm in just leaving it as it is, given that Basic is the only Authorization header auth scheme that the default server supports, so I don't think we'd ever be spec-invalid doing this. But I know you had more reservations about this originally. Let me know what your preferred approach is here. |
|
Thanks @symposion. The latest changes are pretty close but I do see the dilemma in the failure handler and always sending back On a side note, I'm hoping to get this into a patch release but with the addition of |
There was a problem hiding this comment.
Thanks for your patience @symposion.
Please see review feedback.
| @@ -90,6 +90,8 @@ public final class OAuth2ClientAuthenticationFilter extends OncePerRequestFilter | |||
|
|
|||
| private final AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource = new WebAuthenticationDetailsSource(); | |||
|
|
|||
| private final String realmName; | |||
There was a problem hiding this comment.
Please remove realmName as the framework would not make much use of it. Furthermore, the implicit "realm" where the client credentials are authenticated against is the single instance of RegisteredClientRepository.
The "realm" parameter is mainly used for user-based authentication.
There was a problem hiding this comment.
Sorry, I'm finally back to this. I don't think that it's valid to omit "realm" it's a mandatory parameter for Basic auth: https://datatracker.ietf.org/doc/html/rfc7617#section-2
| } | ||
| else { | ||
| this.authenticationFailureHandler.onAuthenticationFailure(request, response, | ||
| new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_CLIENT)); |
There was a problem hiding this comment.
Define private static final String MISSING_CLIENT_AUTH_ERROR_CODE = "missing_client_auth" and use that as the error code instead of INVALID_CLIENT. Then the authenticationFailureHandler can evaluate that specific error code and default to 401 WWW-Authenticate: Basic.
| OAuth2Error error = ((OAuth2AuthenticationException) exception).getError(); | ||
| ServletServerHttpResponse httpResponse = new ServletServerHttpResponse(response); | ||
| if (OAuth2ErrorCodes.INVALID_CLIENT.equals(error.getErrorCode())) { | ||
| httpResponse.setStatusCode(HttpStatus.UNAUTHORIZED); | ||
| httpResponse.getHeaders().set("WWW-Authenticate", "Basic realm=\"" + this.realmName + "\""); |
There was a problem hiding this comment.
Only return WWW-Authenticate: Basic when error code is MISSING_CLIENT_AUTH_ERROR_CODE OR when the HttpServletRequest contains the header Authorization Basic.
|
@symposion Just touching base to see if you had a chance to update based on my review comments. |
Hi, yes sorry I've been away for a bit but I'll try and get back to this this week. |
|
@jgrandja I've reworked my commit again now.
|
Although
Just to give you a little bit of background on our process...we prefer to hide implementations as it gives us flexibility to change code without breaking existing applications. Furthermore, the more we expose APIs, the more we need to support and the less flexibility we have. So in a nutshell, everything is hidden and we only expose if it's absolutely necessary and makes sense from a usability standpoint. |
I'll do this as you ask because I need to be finished with this but I think we have a different approach to Internet Standards. IMO if there's a clear stipulation in a standard that you are implementing, you follow it, whether or not it's convenient/relevant to your particular product. That's how we have predictable interoperability without having to negotiate individually with every different integration. I can pretty much guarantee that somewhere out there are a bunch of people parsing
Ok, understood. On the other side of the fence, I can tell you that we have experienced significant frustration as a result of this policy. There are many elements of Spring Security, and especially the Auth server, that we've needed to customise for various reasons, and we frequently find ourselves having to copy-paste large chunks of code because of final classes with private members that effectively prevent us from making minor changes to behaviour. I understand that you're trying to avoid committing to maintaining too broad a public API so that you don't break your users' code with future updates, but on the flip side, if this results in people having to resort widely to copy-paste re-implementation, this is really only paying lip-service to true maintainability. It's significantly more painful having to try and maintain what is effectively a fork of parts of the library's code in the face of future updates than it is to deal with clear breaking changes to an API. I get that it's less work for you as a maintainer because it's no longer technically your problem, but that's achieved by making your library less helpful to its users. IMO the real solution here is not increased reliance on private internal interfaces, but attempting to ensure that the contracts/divisions of responsibilities between your components are defined with sufficient clarity and rigour that you don't need to make breaking changes to them in the first place. In this instance, on one side we're saying that this error code is a private internal implementation detail, but on the other, the OAuth2ClientAuthenticationFilter has a public extension point that allows you to override the error handling. In order to produce a spec-compliant result, it is unavoidable that any user-supplied implementation of the error handling will have to replicate the remapping from this internal, private error code to the spec-valid "invalid_client". Nor can a user-supplied error handler delegate to the base implementation, because it's a private member of a final class, so really the only credible way that a user can tweak the implementation of the error handler is to start with a copy-pasta of the original method and redefine the string constant in their own code. If you later decide to change the way this is done, you will break that user's implementation; technically it'll be their problem because they're relying on a private implementation detail, but what choice did they actually have? The reality is that the error handling interface here is just badly designed because it leaks information that you've decided should be private, and you cannot define a useful, compliant implementation of it without relying on that private information. The irony of all of this is that the old implementation of this behaviour in the pre-5.8 version (which I originally tried to replicate) didn't suffer nearly as badly from this problem. Instead of trying to make this an internal implementation detail of the OAuth2ClientAuthenticationFilter, it deferred to a well-defined, standard piece of Spring Security behaviour - an AuthenticationEntryPoint - which is specifically designed to be the point at which users can customize the behaviour of a Spring Security application's response to an authentication failure. As I pointed out that time, the resource server implementation - which has some pretty strong parallels to the auth server in many areas - takes exactly this approach and has ended up with an implementation that's a lot easier for users to usefully customize without committing to a particularly broader public API for them to maintain. Anyway, we've both spent a lot of time discussing this now and I think it's pretty clear what approach you want to take, so I'm just going to do that and try and bring this ticket to a close; maybe in the future you might consider revisiting this to make it more friendly to users that want to customize some elements of the behaviour. |
Current implementation does not include the WWW-Authenticate header when returning a 401 for missing/invalid credentials when attempting to access the token endpoints. Fixes-468 Signed-off-by: Lucian Holland <lucian@patientsknowbest.com>
|
@symposion Apologies for the delayed response. I've been working on a couple of priority tasks for the upcoming
I'll give this some thought and I'll get back to you.
This is not ideal and we need to address this. We certainly do not want you to be copy-pasting large chunks of code. Please let me know exactly what you are copy-pasting and I'll see if there is an alternative customization you can do and if not we'll see how we can help you reduce the amount of copied code.
Thanks for sharing but this clearly is your opinion and you have that right. However, I don't agree that most people are copy-pasting code to make their use cases work. We have provided many extension points in the framework and we are totally flexible on opening up more extension points when the needs arise from our use base. In some instances, users are copying-pasting code because they don't see the extension point that is available to them until we engage in a discussion. Either way, I would prefer to discuss concrete cases of copy-pasting code rather than broad (and opinionated) statements. To iterate, we are flexible on opening more extension points in order to reduce code duplication on your end. But you need to give me specific cases so we can figure it out and move you forward.
The idea I proposed for
I'll give this some further thought as well. At this point, no need to provide any further updates. I appreciate all the feedback and the time you have spent on this. I'll circle back to this after I finish my current priorities and let you know my thoughts on the items mentioned above. |
|
@symposion Regarding your comment
I put together a branch that makes use of I think this will address your concerns. |
This looks great, thank you! It's nicely extensible for a variety of possible customizations. I think the introduction of a specific exception class to capture the authentication failure really improves this. In regards to the original issue I raised, I think we might still have to customize slightly because as I read the code, in the case that no authentication information is provided at all it's still not going to return a WWW-Authenticate header, because it will fall back to the DefaultAuthenticationEntryPoint. But it would now be trivial for us to provide an overridden EntryPoint to detect that situation and return the header to keep these types of Http Clients happy. Thanks for all your help with this. As regards the other, generalised comments, I agree that specifics are much more helpful. As it happens I've hit a concrete problem that exemplifies this with the time handling issue I raised elsewhere. When I get a moment I'll document that more thoroughly on that ticket. |
jgrandja
added
status: duplicate
|
@symposion I'm really glad that we were able to come up with a solution that meets your needs. I know it took a while but we made it happen in the end.
I enhanced the solution further and now you can easily customize this. Please see this test for how to customize. gh-1983 is now merged so I'll close this as a duplicate. Thanks again for all the time you spent on this. |
|
@symposion Unfortunately, I had to revert the enhancement applied in gh-1983, as well as, the fix applied in gh-468. Given the There were a few Spring Boot specific smoke tests that were failing as a result of these changes. I will admit that I rushed trying to get these changes in before the release, especially given all the time we spent on it, but lesson learned is that it's never good to rush. I'm sorry it didn't work out for this release but I will prioritize it for the next release. |
Current implementation does not include the WWW-Authenticate header when returning a 401 for missing/invalid credentials when attempting to access the token endpoints. This PR would change to use the standard BasicAuthenticationEntryPoint in order to populate this header correctly.
I will add testing if the fix approach is considered acceptable
Related gh-468