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

PKCE support for SSO #5189

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

PKCE support for SSO #5189

wants to merge 2 commits into from

Conversation

avdb13
Copy link

@avdb13 avdb13 commented Nov 12, 2024

Description

In continuation of #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
Copy link
Member

Nutomic commented Nov 12, 2024

@privacyguard Could you have a look at this?

@avdb13
Copy link
Author

avdb13 commented Nov 12, 2024

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

&context,
&oauth_provider,
&data.code,
data.pkce_code_verifier.as_deref(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make sure the provider has use_pkce enabled before accepting the pkce_code_verifier parameter from the client.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the pkce_code_verifier must be validated here against the PKCE allowed characters.

code_verifier = high-entropy cryptographic random STRING using the
unreserved characters [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~"
from Section 2.3 of [RFC3986], with a minimum length of 43 characters
and a maximum length of 128 characters.

@@ -57,6 +57,8 @@ pub struct OAuthProvider {
pub auto_verify_email: bool,
/// Allows linking an OAUTH account to an existing user account by matching emails
pub account_linking_enabled: bool,
/// switch to enable or disable PKCE
pub use_pkce: bool,
Copy link
Contributor

@privacyguard privacyguard Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use_pkce should also be added to the serialized fields in the PublicOAuthProvider. This will be required by the client to be able to check whether or not code_verifier should be generated for the provider in question at login time.

@privacyguard
Copy link
Contributor

@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
Copy link
Contributor

privacyguard commented Nov 13, 2024

@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.

#4881 (comment)

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.

4 participants