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] support custom signing algorithms and PKCE for social login #1507

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ToxicMushroom
Copy link

This pr would allow users to configure different oauth2 id_token signing algorithms
And prevent against worst consequences of accidental client-secret leakage using PKCE.

Please let me know if I should do the configuration stuff differently or if things (beans) are in the wrong place :)
Or if the defaults should stay pkce disabled and RS256 signing.

I currently tested these changes locally using komga [bootRun] dev,localdb,noclaim,oauth2 and my kanidm instance configured as custom oauth2 client/provider.
I also tested with the github example, seems to also support ES256 and pkce.

@gotson
Copy link
Owner

gotson commented May 27, 2024

Hi, can you provide more context ? I have no idea what the problem is in the first place.

@ToxicMushroom
Copy link
Author

ToxicMushroom commented May 27, 2024

Hi, can you provide more context ? I have no idea what the problem is in the first place.

I cannot use my IDM tool kanidm because of the spring security library only doing RS256 token signing out of the box.
Spring also does not come with PKCE by default and disabling PKCE is also considered less secure as per https://kanidm.github.io/kanidm/master/frequently_asked_questions.html#why-is-disabling-pkce-considered-insecure.

These things can sadly not by enabled via the spring oauth provider config (with no interest from spring upstream to add it), thus the code changes here to allow for it.

Sorry for the lack of context, I had provided this in the discord and didn't think further about it after being told to create an issue/pr.

@gotson
Copy link
Owner

gotson commented May 28, 2024

Thanks. This should ideally be baked in Spring Boot, as mentioned here. I have not seen this raised in the Spring Boot repo though.

I am reluctant to merge this PR, as the setting would apply to all registered OAuth2 providers, and not just the one needed.

I think a better approach would be to raise this in the Spring Boot repo and see if they can add support for it in the auto configuration.

@ToxicMushroom
Copy link
Author

It appears that pkce can now be done via a bean, idk if this is better or provides access to client config, will have to test that out: spring-projects/spring-security#15236. I'm linking this here so I don't lose track of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants