Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAuth2 Resource Server Auto-Configuration can only configure a single JWS algorithm #31230

Closed
wants to merge 3 commits into from

Conversation

rs017991
Copy link
Contributor

@rs017991 rs017991 commented Jun 1, 2022

Spring Security added support for specifying multiple JWS algorithms in spring-projects/spring-security#7162

However, since the auto-configuration was not updated, it was only possible to leverage that enhancement via a custom bean.
(a bit clunky and undesirable compared to setting it in yml)

This PR fills the gap by allowing the existing property to contain a comma-delimited list of algorithms.
(if only one algorithm is defined as before, then it will behave no differently)

I'd like to get this backported to 2.7 as well as 2.6 if at all possible.
Please let me know if/when you would like me to do this.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 1, 2022
@rs017991
Copy link
Contributor Author

rs017991 commented Jun 1, 2022

It appears to be unhappy with my formatting. I'll uglify and push a follow-up commit here in a few minutes.

@rs017991
Copy link
Contributor Author

rs017991 commented Jun 1, 2022

It's all green now; I was able to appease checkstyle, but I couldn't figure out how to get the formatter to stop mangling my code so I had to disable it.

@wilkinsona
Copy link
Member

Thanks for raising this, @rs017991. It's unfortunate that we missed this when the change was made in Spring Security 5.2. To consider making the proposed changes in 2.6.x or 2.7.x, we'd have to consider this to be a bug as we do not make enhancements in maintenance releases. It could be argued that it's a bug of omission. I'll label the issue for discussion at a team meeting so that we can consider our options.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jun 6, 2022
@bclozel bclozel added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Jun 8, 2022
@bclozel bclozel added this to the 2.7.x milestone Jun 8, 2022
@bclozel bclozel added the for: merge-with-amendments Needs some changes when we merge label Jun 8, 2022
@philwebb
Copy link
Member

philwebb commented Jun 8, 2022

We're going to look at adding a jwsAlgorithms String[] property and deprecate jwsAlgorithm. We'll treat this one as a bug and attempt to fix it in 2.7.

@rs017991
Copy link
Contributor Author

rs017991 commented Jun 8, 2022

Thanks for the update @philwebb. I'll be afk for a few weeks, so feel free to rework my contribution as you see fit in my absence.

@wilkinsona wilkinsona changed the title Support Multiple JWS Algorithms OAuth2 Resource Server Auto-Configuration can only configure a single JWS algorithm Jun 9, 2022
@wilkinsona
Copy link
Member

Thanks for the pull request, @rs017991. Unfortunately, the approach that I think we need to take is sufficiently different to what's proposed here that I don't think it's worth building on top of these changes as we'd revert them almost entirely. Thanks anyway for bringing the problem to our attention. I've opened gh-31321 to track the problem.

@wilkinsona wilkinsona closed this Jun 9, 2022
@wilkinsona wilkinsona removed this from the 2.7.x milestone Jun 9, 2022
@wilkinsona wilkinsona added status: superseded An issue that has been superseded by another and removed for: merge-with-amendments Needs some changes when we merge labels Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants