spring-projects / spring-authorization-server

Spring Authorization Server
https://spring.io/projects/spring-authorization-server
Apache License 2.0
4.84k stars 1.28k forks source link

Authorize endpoint should take Spring Security filter chain access rules into account #1415

Closed AndreasKasparek closed 3 months ago

AndreasKasparek commented 12 months ago

Describe the bug OAuth2AuthorizationEndpointFilter is added to the security filter chain by the OAuth2AuthorizationEndpointConfigurer before the AbstractPreAuthenticatedProcessingFilter. The endpoint filter contains a simple authentication check:

if (!authenticationResult.isAuthenticated()) {
        // If the Principal (Resource Owner) is not authenticated then
        // pass through the chain with the expectation that the authentication process
        // will commence via AuthenticationEntryPoint
        filterChain.doFilter(request, response);
        return;
}

This check is called after the OAuth2AuthorizationCodeRequestAuthenticationProvider had a chance to authenticate the incoming authorization code request. This authentication provider mainly validates the correctness of the incoming request (necessary parameters, client exists, consent needed...) and does a very simple check of the currently authenticated user (principal). It does not actually authenticate the principal but relies on other code to have already done that:

private static boolean isPrincipalAuthenticated(Authentication principal) {
        return principal != null &&
        !AnonymousAuthenticationToken.class.isAssignableFrom(principal.getClass()) &&
        principal.isAuthenticated();
}

If the principal is not authenticated, the authentication provider returns the current unauthenticated authorization code request, which in turn leads the endpoint filter to stop its work and instead continue the filter chain. However, if the principal is marked as authenticated (and not anonymous), the authentication provider will issue a valid authorization code and the endpoint filter will redirect the request back to the OAuth2 client, thus aborting the processing of the current filter chain.

Due to the fact that the OAuth2AuthorizationEndpointFilter is registered before the AuthorizationFilter, the Spring Security request-based access rules configured on the security filter chain via http.authorizeHttpRequests(authorize -> ...) are ignored if the principal is marked as authenticated. If the server wants to enforce additional access restrictions on the OAuth2 authorize endpoint apart from simple authentication (like username+password) by specifying, for example, hasAuthority(...) or hasRole(...), then these are only taken into account if the principal is not regarded as authenticated.

This prevents the implementation of multi-step authentication schemes (like additional 2FA) or mandatory user interactions during the sign-in process (like requiring password changes for expiring credentials, confirming terms and conditions, etc.).

To Reproduce Create a SecurityFilterChain with a setup similar to this:

AuthenticationSuccessHandler successHandler = ...;
AuthorizationServerSettings authorizationServerSettings = ...;
OAuth2AuthorizationServerConfigurer authorizationServerConfigurer = new OAuth2AuthorizationServerConfigurer();
http.securityMatcher(authorizationServerConfigurer.getEndpointsMatcher())
                .authorizeHttpRequests(authorize ->
                        authorize.requestMatchers(authorizationServerSettings.getAuthorizationEndpoint()).hasAuthority("all_steps_completed")
                                .anyRequest().authenticated())
        .formLogin(formLogin -> formLogin.successHandler(successHandler))
        .apply(authorizationServerConfigurer);

The normal username+password form login will create an authenticated auth token. In the successHandler assume that the user has to provide an additional second factor or needs to perform further steps (like update the password if it wasn't changed for some while). The success handler will therefore redirect to additional UI pages (protected by a separate security filter chain, not shown here) where the user has to undertake these additional steps. On the last of these steps the corresponding success handler will assign the "all_steps_completed" authority to the principal and then redirect back to the saved request (which points to the OAuth2 authorize endpoint covered by this filter chain).

This process works as long as the user follows the happy path. However, if on any of the additional pages before the principal was assigned the "all_steps_completed" authority the user changes the URL in the browser manually back to oauth2/authorize, the OAuth2AuthorizationCodeRequestAuthenticationProvider will successfully create an authorize code and redirect back to the original calling OAuth2 client even so the current principal does not satisfy the access restriction defined for this endpoint in the above SecurityFilterChain config yet.

Expected behavior https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1 states:

The authorization server validates the request to ensure that all required parameters are present and valid. If the request is valid, the authorization server authenticates the resource owner and obtains an authorization decision (by asking the resource owner or by establishing approval via other means).

The OAuth2 spec makes no assumption on how to authorize the authorize code request. The authorization server may not only require consent from the resource owner but restrict access further. The logical implementation for additional access restrictions would be the usage of Spring Security's http request authorization scheme configured via SecurityFilterChain (see https://docs.spring.io/spring-security/reference/servlet/authorization/authorize-http-requests.html).

For this to work, the endpoint filter that issues the authorization code and redirects back to the OAuth2 client would have to run after the AuthorizationFilter so that Spring Security's access control takes full effect. The advantage of this would also be that the OAuth2AuthorizationCodeRequestAuthenticationProvider does not have to explicitly check the state of the current principal anymore because that would have been already handled by the authorization filter. And the endpoint filter would not need an extra shortcut for unauthenticated users.

My assumption is that the only reason why the OAuth2AuthorizationEndpointFilter is added so early in the security filter chain is so that it can first evaluate the correctness of the incoming request and report any OAuth2 errors back to the clients (like e.g. INVALID_GRANT) before it actually checks the authentication/authorization of the current principal. That way it can abort the authorize code request immediately and not start the user interaction if the request does not satisfy the basic format requirements.

However, currently the OAuth2AuthorizationEndpointConfigurer already registers other filters like the OAuth2TokenEndpointFilter after the AuthorizationFilter. That means for these endpoints the principal is verified before the request is validated for correctness.

My proposal would be to run the request conversion into an OAuth2AuthorizationCodeRequestAuthenticationToken and the OAuth2AuthorizationCodeRequestAuthenticationValidator in a separate endpoint filter before the AuthorizationFilter to allow fail-fast if the request is malformed. But then run the actual authorization code generation and the redirection login in another endpoint filter after the authorization filter to make full use of the Spring Security features. The OAuth2AuthorizationCodeRequestAuthenticationProvider is not authenticating the principal but only the code request. It should therefore let Spring Security handle the user principal.

AndreasKasparek commented 12 months ago

Created after initial (now deleted) discussion on #534

jgrandja commented 11 months ago

Following comment from @sjohnr copied from gh-534


@AndreasKasparek

Is this intentional? Why is the endpoint filter not added after the AuthorizationFilter in the chain?

At the current time, yes it is intentional. The goal of this project is to adhere to the spec, which does not mention additional authorization checks. Two-factor authentication would actually best be implemented as a first-class Spring Security feature, and not in this project, because Spring Security is focused on authentication while this project is not.

How do you propose to implement something like 2FA and further checks correctly with the given implementation?

As I mentioned in my comment, the work necessary to deliver a how-to guide for two-factor authentication is not complete. It is possible additional enhancements to Spring Authorization Server's core will be required before it can be delivered, and I have not yet had time to go deeper than that. There are likely other approaches that one could take, and it's fun to get creative and try to find them, but as I mentioned it is not currently the priority at the moment.

I will say that it's definitely something I will continue to think about, as I am committed to building a high-quality guide around this topic in the future.

Having said that, I would ask you both (@wdkeyser02 @AndreasKasparek) not to use this issue as a discussion forum for your own exploration. Please take that discussion elsewhere, such as Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it).

jgrandja commented 11 months ago

@AndreasKasparek

My assumption is that the only reason why the OAuth2AuthorizationEndpointFilter is added so early in the security filter chain is so that it can first evaluate the correctness of the incoming request and report any OAuth2 errors back to the clients (like e.g. INVALID_GRANT) before it actually checks the authentication/authorization of the current principal. That way it can abort the authorize code request immediately and not start the user interaction if the request does not satisfy the basic format requirements.

Yes, your assumption is correct. As per 4.1.1 Authorization Request:

The authorization server validates the request to ensure that all required parameters are present and valid. If the request is valid, the authorization server authenticates the resource owner and obtains an authorization decision (by asking the resource owner or by establishing approval via other means).

The Authorization Request parameters must be validated first before checking if the user is authenticated. This processing sequence helps with the user experience. If it was implemented in the reverse, for example, check if the user is authenticated first and then validate the Authorization Request parameters, then the user would receive the error message after they have successfully authenticated, resulting in a degraded user experience.

This is the main reason why OAuth2AuthorizationEndpointFilter sits before AuthorizationFilter. If it was positioned after AuthorizationFilter, then the Authorization Request parameters could not be validated first and would not be spec compliant.

the Spring Security request-based access rules configured on the security filter chain via http.authorizeHttpRequests(authorize -> ...) are ignored if the principal is marked as authenticated. If the server wants to enforce additional access restrictions on the OAuth2 authorize endpoint apart from simple authentication (like username+password) by specifying, for example, hasAuthority(...) or hasRole(...)

I haven't seen a use case or a request from the community for allowing the ability to enforce additional access restrictions on the OAuth2 authorize endpoint, for example, hasAuthority(...) or hasRole(...). The spec does not specify it either. If you look at the 4.1 Authorization Code Grant flow :

(B) The authorization server authenticates the resource owner (via the user-agent) and establishes whether the resource owner grants or denies the client's access request. (C) Assuming the resource owner grants access, the authorization server redirects the user-agent back to the client using the redirection URI provided earlier (in the request or during client registration). The redirection URI includes an authorization code and any local state provided by the client earlier.

This is where the "Authorization Decisioning" happens, it's between the Resource Owner and the Client requesting access (via the AS) and whether the Resource Owner is granting or denying the access request to the client, which is typically scope-based but could also be claims and/or general authorities. This has no relation to authorization rules specified for a protected endpoint. These are 2 distinct areas for authorization decisioning, with the latter not being referred to anywhere in the spec.

This prevents the implementation of multi-step authentication schemes (like additional 2FA) or mandatory user interactions during the sign-in process (like requiring password changes for expiring credentials, confirming terms and conditions, etc.)

Based on your sample configuration, you are mixing the Authentication sub-system with the OAuth2 Authorization Server sub-system, because you are configuring formLogin() in the authorizationServerSecurityFilterChain. This is not recommended and should be avoided. Each of these sub-systems perform 2 different functions and should be separated in their own SecurityFilterChain. This has been well documented in the reference - see the Getting Started guide. You will notice that the defaultSecurityFilterChain is where the Authentication sub-system is configured. This is the Spring Security filter chain and is the main concern that is handled by Spring Security. And to be clear, user authentication is NOT a concern for Spring Authorization Server and therefore should not be configured in it's SecurityFilterChain. The Spring Security SecurityFilterChain is where you would configure the primary authentication mechanism and 2nd, 3rd, etc. factors for authentication. After the "complete" authentication is performed by Spring Security, the saved request (the oauth2 authorization request) would then be replayed and processed by the Spring Authorization SecurityFilterChain. The only requirement here is that the current request is authenticated, as per spec. There are no other requirements as far as the type of user that should be authenticated and/or the roles/authorities it should be associated to.

The authorization server may not only require consent from the resource owner but restrict access further.

Where in the spec does it provide details on potentially restricting further outside of the consent flow?

The logical implementation for additional access restrictions would be the usage of Spring Security's http request authorization scheme configured via SecurityFilterChain (see https://docs.spring.io/spring-security/reference/servlet/authorization/authorize-http-requests.html).

As mentioned previously, authorization is performed between the Resource Owner and the client being granted or denied the access request. Authorization rules defined on protected endpoints is a separate area for authorization and has no relation to the OAuth2 consent flow.

I hope my explanations make sense here?

The current implementation behaves as expected and complies with the spec so no changes are required at this point. Before I close this issue, I'll wait for your response.

AndreasKasparek commented 11 months ago

Hi @jgrandja, Thanks for your feedback. I understand your flow of thought and agree that the OAuth2 spec mainly speaks about the resource owner's authorization of the client request, but that does not necessary mean that the authorization server itself is not allowed to impose additional restrictions. What if the authorization server implements multiple authentication schemes for different use-cases based on a central user database, OAuth2 being just one of them, and it wants to restrict the OAuth2 code grant to only a subset of the existing users (say only users with the developer role should be able use OAuth2 for authentication)?

As someone using the Spring ecosystem, my expectation is that all the different Spring modules/projects work seamlessly together and I can mix and match them to my liking. I would not expect that the usage of one project breaks the behavior of another, but that is currently the case here. I need to use Spring Security's security filter chain mechanism to configure the Spring Authorization Server endpoints but at the same time I am not able to use the other features of Spring Security on that same chain to its full extend (namely endpoint http security). And the worst part is that this is not even clearly visible (I have not seen it documented, there is no fail-fast in code, etc.): I can configure access restrictions for the oauth2 endpoints but the config may just be silently ignored. It would at least be nice if this would produce an error somehow or at least be mentioned prominently in documentation.

You mention that form login must not be configured on the same security filter chain as the OAuth2 authorization server sub-system. My example was a simplification. In our code we are actually following the guidelines and have two separate security filter chains for that. But that doesn't solve the problem because in the end you have to transfer the principal's authentication information (e.g. AuthenticationToken in the SecurityContext) from one chain to the other. I assume that normally happens by spanning the user's web session across the filter chains:

  1. The OAuth client sends the code grant request via the user's browser to oauth/authorize endpoint. The browser sends along all cookies for the authorization server's domain, including the session cookie to support SSO. The corresponding security filter chain includes a SecurityContextHolderFilter that loads the user's authentication from the web session. If this is the first request, the authentication is anonymous and the later running AuthorizationFilter on that chain will probably reject that request because the access configuration for the authorize endpoint should not be permitAll. The exception handling will create and store a SavedRequest in the web session and redirects the user to the login entry point (login UI page).
  2. The user is redirected to the login page(s) configured on another security filter chain. Here we can implement whatever we want to authenticate the user and do additional steps. This can include several interactions across multiple UI pages/endpoints to e.g. ask for username, password, 2FA, etc. This chain updates the authentication in the security context accordingly and stores it in the session. At the end of the sign-in process the success handler will load the SavedRequest and redirect back to its URL.
  3. The browser is redirected to the original oauth/authorize endpoint, where basically step 1 is repeated, only this time the security context contains an authentication that passes all checks. Unfortunately the checks here do not follow the regular Spring Security checks of this filter chain but come down to a simple isAuthenticated and not anonymous check in the OAuth2AuthorizationCodeRequestAuthenticationProvider.

If at anytime step 2 has already produced an AuthenticationToken where isAuthenticated evaluates to true (which e.g. would already be the case when using AbstractUserDetailsAuthenticationProvider for the username+password check), the user could just manually change the browser's location back to the oauth/authorize endpoint and thus skip any further access checks (relying e.g. on hasRole or hasAuthority) on the form login filter chain and the authorization server filter chain.

At the moment it seems the only way to prevent this is to ensure that the form login filter chain never sets the authenticated flag to true as long as the sign-in process is not fully completed (personally I have never understood why the AnonymousAuthenticationToken sets this to true). That makes sense if one only wants to cover actual authentication of a user, but if the sign-in steps should include additional mandatory interactions (like asking for confirmation of a privacy policy or additional user consent or whatever) that are semantically unrelated to authentication (meaning: we already know who the user is and have verified their credentials) before being allowed to proceed, then this does not work. We are currently using roles and authorities to control these further process steps, but as these are disregarded by the authorization server filter chain, we would have to translate these also into the isAuthenticated state, which feels wrong.

TL;DR: It is possible to implement workarounds for the current behavior of OAuth2AuthorizationEndpointFilter (and related classes) but it increases complexity of the code significantly and deviates from the common usage of the Spring Security module, and therefore bears the risk of not being understood correctly by developers. As this is a security critical component, having to deal with such undocumented deviations and unexpected behavior in the combination of different Spring projects is very unfortunate.

jgrandja commented 11 months ago

Thanks for the extra details @AndreasKasparek.

It is possible to implement workarounds for the current behavior of OAuth2AuthorizationEndpointFilter (and related classes) but it increases complexity of the code significantly and deviates from the common usage of the Spring Security module

Can you explain the details of the workaround you used?

Our goal is to make Spring Authorization Server easy to configure and use so the fact that it increases the complexity on your end is not ideal. Let's figure out a solution that will simplify on your end.

What if we introduced OAuth2AuthorizationCodeRequestAuthenticationProvider.setAuthenticationTrustResolver(AuthenticationTrustResolver) and within the provider we replaced isPrincipalAuthenticated(Authentication) with authenticationTrustResolver.isFullyAuthenticated(Authentication). This would provide you a hook to evaluate the current Authentication and it's current set of authorities and/or any other attributes associated with it. Could this be a solution that could work for your use case?

AndreasKasparek commented 11 months ago

Can you explain the details of the workaround you used?

As the current authentication provide is only checking the isAuthenticated flag, the workaround would be to only set this flag to true when we deem the sign-up to be completed and all necessary user interactions finished. So basically when the last step in the UI filter chain is done and before the success handler is ready to redirect back to the SavedRequest, we would need to update the Authentication in the SecurityContext and set the flag to true. As mentioned before, I would regard this as a workaround because it may change the semantics of this flag to cover more than it normally should.

What if we introduced OAuth2AuthorizationCodeRequestAuthenticationProvider.setAuthenticationTrustResolver(AuthenticationTrustResolver) and within the provider we replaced isPrincipalAuthenticated(Authentication) with authenticationTrustResolver.isFullyAuthenticated(Authentication). [...] Could this be a solution that could work for your use case?

Yes, I think so. It's still not perfect because the access decision would still be distributed across different places (instead of having everything neatly in the http security config for the chains) but it would at least allow us more control and not having to juggle the isAuthenticated flag too much. If we create this trust resolver within the same configuration class as the respective security chain, we could at least have the code in one place to make it more visible.

Thank you for considering this! Much appreciated.

jgrandja commented 11 months ago

Ok good to know that AuthenticationTrustResolver could be a potential solution. I'll also look at what the impact would be if we moved the OAuth2AuthorizationEndpointFilter behind AuthorizationFilter.

Please give me some time on this. Either way, this enhancement will have to wait for the 1.3 release cycle. We're releasing 1.2 later in Nov and then I'll circle back to this.

KeithLeoSmithIdeam commented 11 months ago

i am trying to solve the same or related issue, i would like the authenticated Principle to be resolved before the or in the OAuth2AuthorizationEndpointFilter so that i can use basic auth to get an auth code instead of having to always redirect to the login page first to get an auth code

@jgrandja Do you have a workable solution / work around?

gaborbsd commented 6 months ago

My workaround is a plain old HttpFilter that protects the authorization endpoint and redirects to the MFA page when the user is already authenticated but MFA is not yet completed (the user does not have the MFA authority yet). It works but it is not nice. I agree with @AndreasKasparek in that the best would be if Spring Authorization Server took into account the hasAuthority() rules, it would be consistent with what one usually expects based on how the Authorization Server is configured.

jsantana3c commented 5 months ago

hey guys, was this issue released? also, do you have any guidance on how to implement it? because I've been struggling too trying to use session management with a concurrency control of 1.

jgrandja commented 3 months ago

@AndreasKasparek I've finally circled back to this.

Regarding your original comment:

My proposal would be to run the request conversion into an OAuth2AuthorizationCodeRequestAuthenticationToken and the OAuth2AuthorizationCodeRequestAuthenticationValidator in a separate endpoint filter before the AuthorizationFilter to allow fail-fast if the request is malformed. But then run the actual authorization code generation and the redirection login in another endpoint filter after the authorization filter to make full use of the Spring Security features. The OAuth2AuthorizationCodeRequestAuthenticationProvider is not authenticating the principal but only the code request. It should therefore let Spring Security handle the user principal.

I tried this out and there were too many side effects. Splitting the processing of the authorization_code grant into 2 separate flows did not seem natural to me. This required 2x Filter's and 2x AuthenticationProvider's. One of the side effects required OAuth2AuthorizationCodeRequestAuthenticationProvider.authenticationValidator to be moved to the "validating" Filter, however, it would still be required in the OAuth2AuthorizationCodeRequestAuthenticationProvider because it's possible that validation was not performed so we would need to validate before proceeding. It also doesn't seem natural that the OAuth2AuthorizationCodeRequestAuthenticationProvider should just process the request without any validation because it was pre-validated in the (before) "validating" Filter. Especially, not seeing the redirect_uri validation in OAuth2AuthorizationCodeRequestAuthenticationProvider would be very confusing and users would wonder where is the validation happening? It just doesn't feel right and the "business rule" validation (client_id, redirect_uri, scope) MUST happen in OAuth2AuthorizationCodeRequestAuthenticationProvider before processing can proceed.

There were a couple of other implementation details that did not sit well with me as I prototyped it out. Either way, it was a good exercise and it further validated that the current implementation is where it should be.

Regarding your comment:

If at anytime step 2 has already produced an AuthenticationToken where isAuthenticated evaluates to true (which e.g. would already be the case when using AbstractUserDetailsAuthenticationProvider for the username+password check), the user could just manually change the browser's location back to the oauth/authorize endpoint and thus skip any further access checks (relying e.g. on hasRole or hasAuthority) on the form login filter chain and the authorization server filter chain.

Yes you are right. The user could bypass further access checks after logging in and manually entering /oauth/authorize. I did propose OAuth2AuthorizationCodeRequestAuthenticationProvider.setAuthenticationTrustResolver(AuthenticationTrustResolver) that could help solve this issue but I don't think that will work for all cases, based on your following comment:

At the moment it seems the only way to prevent this is to ensure that the form login filter chain never sets the authenticated flag to true as long as the sign-in process is not fully completed (personally I have never understood why the AnonymousAuthenticationToken sets this to true). That makes sense if one only wants to cover actual authentication of a user, but if the sign-in steps should include additional mandatory interactions (like asking for confirmation of a privacy policy or additional user consent or whatever) that are semantically unrelated to authentication (meaning: we already know who the user is and have verified their credentials) before being allowed to proceed, then this does not work.

Excellent point and a valid step in an MFA flow. So evaluating an Authentication and associated authorities will not be enough to fulfill your requirements.

I'm not sure why I didn't think of this other solution last we spoke but here it is.

I believe the main goal you are trying to achieve is that MFA MUST be fully complete before the authorization_code grant can proceed. This may involve checking user authorities via the Authentication or other attributes of the Authentication that are non-authority related. There are many other use cases that can be applicable as well, for example, checking a custom request parameter or header that provides a value indicating where the user is in the authn/MFA process.

The solution is quite simple actually. You can register a custom AuthenticationConverter in OAuth2AuthorizationEndpointFilter that provides a fail-fast mechanism. For example, the custom implementation would look very similar to OAuth2AuthorizationCodeRequestAuthenticationConverter but it would evaluate the Authentication and check for all_steps_completed and also if the privacy policy was confirmed. Since it provides access to HttpServletRequest, you have the ability to check custom parameters and headers.

I believe the custom AuthenticationConverter hook will give you all the flexibility you need and not just limit you to Authentication.authorities checks.

What are your thoughts?

AndreasKasparek commented 3 months ago

Hi @jgrandja, Sorry for my short answer here but I am currently busy with other stuff and not fully in this topic right now to provide any valuable and detailed feedback.

I think your proposal with the AuthenticationConverter is a possible workaround. Another workaround that we are currently using is to just register an additional AuthenticationProvider for that endpoint filter which runs before the default OAuth2AuthorizationCodeRequestAuthenticationProvider and clears the security context in case the current Principal is not in the right state. I guess there are a couple other ways how one could implement this right now.

The main issue I still have with all of this (and why I regard it only as a workaround) is that I still cannot see any of that in our security configuration. When I look at the SecurityFilterChain for the OAuth2 authorization server, it says (simplified snippet):

http
  [...]
     .authorizeHttpRequests(authorize -> authorize
          .requestMatchers("/oauth/jwk").permitAll()
          .requestMatchers("/oauth/authorize").access(new WebExpressionAuthorizationManager("isAuthenticated() and hasAuthority('sign_in_completed')"))
          .anyRequest().authenticated())
   [...]

and that configuration for the authorize endpoint is a just ignored. I guess I could even configure a denyAll on the authorize endpoint here and it would still not prevent anything because this code might never be evaluated. And in my opinion that is a security risk. It could easily slip through code reviews if the reviewer is not familiar with the finer details that we have discussed here and just assumes that the presence of proper access control in the security chain is enough to secure the endpoint. And I couldn't blame them because access to most of the other endpoints in that SpringBoot application is correctly evaluated by Spring Security but not this one. Therefore I would regard this as a gap in the security framework, and that's something you don't want to have.

Maybe I am just too picky with it, but I am working in financial services where auditors and penetration testers really look at that stuff. Nonetheless thank you for all your time and coming back to this topic.

jgrandja commented 3 months ago

@AndreasKasparek There is no need to apply authorizeHttpRequests() on the OAuth2 endpoints in your security configuration:

http
  [...]
     .authorizeHttpRequests(authorize -> authorize
          .requestMatchers("/oauth/jwk").permitAll()
          .requestMatchers("/oauth/authorize").access(new WebExpressionAuthorizationManager("isAuthenticated() and hasAuthority('sign_in_completed')"))
          .anyRequest().authenticated())
   [...]

Each of the OAuth2 endpoints are placed in the correct order within the SecurityFilterChain. For example, NimbusJwkSetEndpointFilter is placed before AbstractPreAuthenticatedProcessingFilter, so your authorization rule .requestMatchers("/oauth/jwk").permitAll() is redundant and never evaluated anyway. It would also be ignored if you changed it to denyAll.

We have never advertised the use of authorizeHttpRequests() on the OAuth2 endpoints in any of our samples or reference documentation so I'm not sure what has lead you down this path.

I guess I could even configure a denyAll on the authorize endpoint here and it would still not prevent anything because this code might never be evaluated. And in my opinion that is a security risk. It could easily slip through code reviews if the reviewer is not familiar with the finer details that we have discussed here and just assumes that the presence of proper access control in the security chain is enough to secure the endpoint.

As per the bold text, that is exactly the point I'm relaying here is that the OAuth2 endpoints are strategically positioned within the SecurityFilterChain and strict access control is applied. Again, there is no need to apply authorizeHttpRequests() rules and you can simply assume the OAuth2 endpoints are protected.

Therefore I would regard this as a gap in the security framework, and that's something you don't want to have.

FWIW, I am fully confident with the default security applied by OAuth2AuthorizationServerConfigurer and all other OAuth2 Configurer's. But if you see a gap in the default security applied please report it here. To-date, there has only been 1 CVE so our track record is pretty good so far.

I think your proposal with the AuthenticationConverter is a possible workaround. Another workaround that we are currently using is to just register an additional AuthenticationProvider for that endpoint filter which runs before the default OAuth2AuthorizationCodeRequestAuthenticationProvider and clears the security context in case the current Principal is not in the right state. I guess there are a couple other ways how one could implement this right now.

I'm glad you found another way to implement your requirements. And agreed, there are many ways to do one thing. I think Spring Authorization Server and Spring Security provide great flexibility with the many hooks that you can choose from.

I'm leaning towards closing this issue since there are a couple of options for you to run with. And we're not planning on moving the order of the OAuth2AuthorizationEndpointFilter based on the reasons explained in my previous comment.

I'll wait for your response before I close this.

AndreasKasparek commented 3 months ago

Not sure where we got that impression. We migrated from the old OAuth2 server module, maybe it was different then? Thanks for the hint.

I guess we can close this ticket if you think everything is working as intended. Thank you very much again for all your time spent on discussing and investigating this.

jgrandja commented 3 months ago

@AndreasKasparek

We migrated from the old OAuth2 server module, maybe it was different then?

Yup. That is most likely where the authorizeHttpRequests() rules came from. The legacy OAuth2 endpoints were implemented as @Controller's, whereas Spring Authorization Server implements the OAuth2 endpoints as Filter's and security is automatically applied within the SecurityFilterChain without the need for further protection.

Thank you very much again for all your time spent on discussing and investigating this.

No worries at all. I'm just glad we came up with a couple of options for you to move forward with. Good luck!