spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.55k stars 5.79k forks source link

Allow Custom AuthorizationGrantType in OAuth2AuthorizationRequest #15111

Closed stetsche closed 2 weeks ago

stetsche commented 1 month ago

Context

We are currently in the process of upgrading the library https://github.com/oxctl/spring-security-lti13 from Spring 5 to 6. This library integrates with Spring Security to handle authentication following the LTI 1.3 standard ( https://www.imsglobal.org/spec/lti/v1p3 ) and the underlying security framework ( https://www.imsglobal.org/spec/security/v1p1 ). It powers several learning tools and frameworks. LTI 1.3 is the de facto standard for integrations with learning tools into learing platforms and is widely adopted in the edtech sector.

One of the key features of this library is the authentication of users within a learning platform using a third-party initiated login with an implicit grant flow, as described in https://www.imsglobal.org/spec/security/v1p1/#openid_connect_launch_flow. This approach mitigates the security risks associated with the implicit grant flow, making it a secure method for authentication.

However, with the release of Spring Security 6, support for the implicit grant flow has been removed. We have attempted to define implicit as a custom AuthorizationGrantType structure and configure Spring Security to allow a custom type. Unfortunately, this approach failed as a custom AuthorizationGrantType cannot be specified for OAuth2AuthorizationRequest.

We are wondering if OAuth2AuthorizationRequest could be more customizable by allowing custom AuthorizationGrantTypes, perhaps through the builder.

Alternatively, we would welcome ideas for other approaches that would enable the implementation of an implicit flow on top of Spring Security. Your insights and suggestions would be greatly appreciated, even if it's "we don't want to be extended in this way" as then we can better evaluate potential solution options.

Current Behavior

There is no way to customize AuthorizationGrantType forOAuth2AuthorizationRequest. Both the constructor and the Builder's constructor are private and there are no setters.

Expected Behavior

There is a way to customize AuthorizationGrantType for OAuth2AuthorizationRequest. For example OAuth2AuthorizationRequest could expose a method like OAuth2AuthorizationRequest.ofType(new AuthorizationGrantType("implicit")).build().

sjohnr commented 1 month ago

@stetsche thanks for reaching out and providing an overview of your situation.

One of the key features of this library is the authentication of users within a learning platform using a third-party initiated login with an implicit grant flow, as described in https://www.imsglobal.org/spec/security/v1p1/#openid_connect_launch_flow. This approach mitigates the security risks associated with the implicit grant flow, making it a secure method for authentication.

This is a tough one as I don't feel like there are many good answers for it. The fact that the implicit grant should not be used provides, in fairly strong terms, the reasoning behind removing support in the framework for the grant type. We don't intend or wish to make things more difficult for users, but when the security of a particular flow contains known weaknesses, removing it from the framework helps users make better choices. The right choice, given the information at hand, is to build on the authorization code grant.

I can't speak for the LTI 1.3 standard, but the fact that it builds on the implicit flow is concerning, even in the case where it is attempting to improve security. For that reason, I personally don't recommend using it. However, I'm sure there are points and counterpoints to consider that I am not well versed in so I can't make a strong recommendation one way or another.

We are wondering if OAuth2AuthorizationRequest could be more customizable by allowing custom AuthorizationGrantTypes, perhaps through the builder.

Regarding allowing the customization of grant type in OAuth2AuthorizationRequest, this class is strongly tied to the OAuth2AuthorizationRequestRedirectFilter and its functionality. Allowing custom grant types there would potentially require customization in numerous other areas where support for the implicit grant was removed, which could make it much harder to maintain that filter for those using it for its intended purpose at this point (the authorization code grant). While it feels like it would be reasonable to simply customize the grant type, I feel it could be bigger than that.

I will leave this issue open for a while longer in case there's additional feedback that could shed light on some area of the discussion I'm missing. If there's no additional feedback, I will likely close this ticket after some time.

buckett commented 1 month ago

I can't speak for the LTI 1.3 standard, but the fact that it builds on the implicit flow is concerning, even in the case where it is attempting to improve security. For that reason, I personally don't recommend using it. However, I'm sure there are points and counterpoints to consider that I am not well versed in so I can't make a strong recommendation one way or another.

The LTI 1.3 standard is developed by 1EdTech (previously known as IMS) and is widely supported in the education space for integrating tools into the learning platforms (eg Canvas, Moodle, Sakai, Blackboard). It is a closed membership group (fee paying) and I don't know if there is a plan to change away from using the implicit grant (even with the extra protections).

We are developing tools to integrate into learning platforms and so if we want the tools to actually work we basically have to support the LTI 1.3 standard, as we don't control the platforms we integrate with. I can see that adding back the implicit grant doesn't encourage people to move to different flows and isn't a "good" solution.

Regarding allowing the customization of grant type in OAuth2AuthorizationRequest, this class is strongly tied to the OAuth2AuthorizationRequestRedirectFilter and its functionality. Allowing custom grant types there would potentially require customization in numerous other areas where support for the implicit grant was removed, which could make it much harder to maintain that filter for those using it for its intended purpose at this point (the authorization code grant). While it feels like it would be reasonable to simply customize the grant type, I feel it could be bigger than that.

We did end up taking a copy of OAuth2AuthorizationRequestRedirectFilter and customising it because we needed additional behaviour and this has worked reasonably well.

The existing way we've extended Spring Security had worked well as it meant that all the additional features of Spring Security "just worked" and configuration worked through the existing properties which meant it feels familiar to people who have experience of Spring Security before.

One "hack" is to just pretend that we're doing a different grant type (in the config) and then actually do an implicit grant in our code, but this is just confusing and liable create problems/confusion in the future.

Thanks for Spring Security.

sjohnr commented 1 month ago

We did end up taking a copy of OAuth2AuthorizationRequestRedirectFilter and customising it because we needed additional behaviour and this has worked reasonably well.

The existing way we've extended Spring Security had worked well as it meant that all the additional features of Spring Security "just worked" and configuration worked through the existing properties which meant it feels familiar to people who have experience of Spring Security before.

I think that makes sense given the context you provided. I'm sure it's not ideal and often copying code out of the framework creates a maintenance burden, but it seems a reasonable trade-off given the situation. It feels like the framework is suggesting the right thing and you are opting out of that suggestion due to your requirements, which you are more than welcome to do.

Thanks for Spring Security.

You're quite welcome! Thanks for being part of our community!

stetsche commented 4 weeks ago

The customization of a copy of OAuth2AuthorizationRequestRedirectFilter was made a while back to address other behaviour, I think it was custom error handling, but I am not certain. Trying to apply this approach for allowing extensiblity of to OAuth2AuthorizationRequest would probably not work that well though, becasue it would cascade into other parts of Spring Security and we would end up duplicating a lot of code rather then doing customizations. I think the point that @buckett made was that the need of customization of OAuth2AuthorizationRequestRedirectFilter when customizing AuthorizationGrantType for OAuth2AuthorizationRequest would not be a big issue for people that want to go down that road. I can't tell though, if there would actually be a number of other places that would need to be changed, but it does not appear to be the case to me. I also want to add that it has always been a pleasure to work with Spring Security and it's great to see you are hearing us out on this issue.

sjohnr commented 4 weeks ago

Thanks for the feedback @stetsche.

Trying to apply this approach for allowing extensiblity of to OAuth2AuthorizationRequest would probably not work that well though, becasue it would cascade into other parts of Spring Security and we would end up duplicating a lot of code rather then doing customizations.

I can't tell though, if there would actually be a number of other places that would need to be changed, but it does not appear to be the case to me.

I don't want to be pedantic but it feels like only one of these should be the case. Admittedly, I have not done deep research to figure out how much change would actually be necessary, but I do know which parts of the code base were modified in order to remove implicit grant entirely. While it's not a lot of places, it's certainly more than just the OAuth2AuthorizationRequest. I also would point out that I'm not suggesting copy/paste as a general strategy for customizing Spring Security as I believe this issue affects only OAuth2AuthorizationRequestRedirectFilter. If there are other areas where an enhancement is needed to allow customization, we can take each on a case-by-case basis as the list should be small.

In any case, I don't feel it makes sense to allow arbitrary grant types in OAuth2AuthorizationRequest and we certainly don't want to restore implicit support directly, so I'm struggling to see a path forward for this specific issue. If you have any other ideas, I'd love to hear them.

I also want to add that it has always been a pleasure to work with Spring Security and it's great to see you are hearing us out on this issue.

Great to hear! Likewise, it's a pleasure working with folks like you in the community.

sjohnr commented 2 weeks ago

Hey folks, thanks for the conversation on this issue! I'm sure it's disappointing not to have a path forward for this issue, but as I mentioned above, I think it's best for the framework at the moment to go ahead and close this issue without resolution.

While the authorization code grant has very specific support in the framework, we generally want to support custom grant types so I feel you should be relatively successful getting going again outside of this issue. If you have other issues supporting your use case, please reach out (either with a comment here or a new issue) so we can keep track of it and see what we can do to improve.