LemmyNet / lemmy

🐀 A link aggregator and forum for the fediverse
https://join-lemmy.org
GNU Affero General Public License v3.0
13.3k stars 882 forks source link

PKCE support for SSO #5189

Open avdb13 opened 2 weeks ago

avdb13 commented 2 weeks ago

Description

In continuation of https://github.com/LemmyNet/lemmy/pull/4881.

Implements PKCE support, in order to mitigate against the threat of authorization code interception attacks.

Background reading: https://www.oauth.com/oauth2-servers/pkce

Nutomic commented 1 week ago

@privacyguard Could you have a look at this?

avdb13 commented 1 week ago

Frontend PR: https://github.com/LemmyNet/lemmy-ui/pull/2806 Trying to figure out how to test it now, hints are appreciated.

privacyguard commented 1 week ago

@Nutomic overall the server changes to support PKCE are: 1- The stored provider with all the CRUD methods should have have a boolean: use_pkce 2- The PublicOAuthProvider should contain the field in order to tell the frontend that this provider requires the code_verifier to be generated. 2- The oauth authentication route should accept the code_verifier as a parameter when use_pkce is enabled, validate it, then pass it as a parameter to the issue token request.

We gave our feedback on the functionality aspect. We'll let the rust experts give their feedback on the code.

We also gave feedback on the frontend PR.

privacyguard commented 1 week ago

@avdb13 If you check the comments on the SSO PR, there are a couple of comments detailing the steps needed to test locally. If you're using Privacy Portal to test, don't forget to enable PKCE in the OAUTH app settings there too.

https://github.com/LemmyNet/lemmy/pull/4881#issuecomment-2215943112

avdb13 commented 3 days ago

Converting it back to draft for now, I tested it against gitlab but without success as their OIDC implementation seems to be too unreliable. Testing it with django-oauth-toolkit tonight.

privacyguard commented 2 days ago

@avdb13 We tested the PRs locally, there are a couple of issues that need to be fixed: 1- The way code_challenge is being base64url encoded is incorrect. Check out the PKCE section of this tutorial it includes functions you can use to generate the code_verifier and code_challenge. The code_verifier can also be simplified to base64url encoding. 2- The schema.rs file seems to have been edited manually? When we run the migrations use_pkce should be the last property. Changing it back to the last property would require updating some structs. 3- Also, as mentioned by @dessalines, the migration file needs to be manually renamed to update the timestamp to something more recent such as 2024-11-12-090606_oauth_pkce

avdb13 commented 2 days ago

Testing it seems to assert correctness yet I keep getting "Invalid login credentials" for some reason. This was with account linking disabled.

Correction: I was incorrectly base64 encoding by not removing all trailing equal signs on the front-end side.

avdb13 commented 1 day ago

CI doesn't pass because of lemmy-js-client. If you generate the types again it should work. Iforgot to run cargo-fmt but otherwise it's complete.

Thanks for the tutorial @privacyguard , I decided to use the same code with a few small changes.

avdb13 commented 1 day ago

CI doesn't pass because of lemmy-js-client. If you generate the types again it should work.

Thanks for the tutorial @privacyguard , I decided to use the same code with a few small changes.

I have been thinking about opening another PR in order to support the nonce claim for OIDC providers, after this PR hopefully gets merged.