Skip to content

Enable OAuth PKCE Extension - #31129

Merged
oneiros merged 5 commits into
mastodon:mainfrom
ThisIsMissEm:feat/enable-oauth-pkce
Jul 26, 2024
Merged

oneiros merged 5 commits into
mastodon:mainfrom
ThisIsMissEm:feat/enable-oauth-pkce

Conversation

@ThisIsMissEm

@ThisIsMissEm ThisIsMissEm commented Jul 24, 2024 •

Copy link
Copy Markdown
Contributor

This pull request partially supersedes #30329, in that I've separated out the PKCE changes from the other changes.

As per OAuth 2.0 Security Best Current Practices, we MUST support PKCE.

Whilst PKCE is recommended for all applications to use, forcing PKCE usage is a breaking change, so for now we do not force PKCE usage, only support it. PKCE is an important security mechanism for preventing OAuth Authorization Code Injection Attacks. Note: OAuth 2.1 requires the usage of PKCE.

In accordance with best practices, we only support S256, and explicitly do not support plain for PKCE code_challenge_method. Unfortunately, Doorkeeper does not support a setting for which code_challenge_methods are supported, hence adding a OauthPreAuthorizationExtension module and changing the doorkeeper.errors.messages.invalid_code_challenge_method value.

I've also attempted to add some better tests to ensure certain elements are definitely present on the page, such that we're not just relying on the response.body containing a given string, but actually that the element exists.

Fixes: #21913

Comment thread spec/requests/well_known/oauth_metadata_spec.rb
Comment thread config/application.rb
Comment thread app/lib/oauth_pre_authorization_extension.rb Outdated

@oneiros oneiros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have one remark about spec types, but otherwise this is already looking very good. Thanks for your help with this!

Comment thread app/lib/oauth_pre_authorization_extension.rb Outdated
Comment thread config/application.rb
Comment thread spec/controllers/oauth/authorizations_controller_spec.rb
@ClearlyClaire

Copy link
Copy Markdown
Contributor

First, to make sure I understand exactly the context, as I'm not so familiar with Doorkeepr and OAuth:

  • PKCE is an extension of OAuth made so that the OAuth provider can ensure that the application that received the code is truly the one that requested it, before giving it the authentication token
  • this is primarily meant for public clients, which Mastodon doesn't really support at the moment (which is a whole other issue), but can still be useful for confidential clients in some cases, and has no real drawback, and therefore is a MUST in the latest OAuth 2.0 best practices
  • Doorkeeper does support it natively, provided the database has the correct columns and the parameters are passed properly
  • there is some discussion regarding PKCE and refresh tokens (which we don't currently support either) but my understanding is that for the most part, those are orthogonal and refresh tokens do not typically involve a potentially unsecure channel, unlike the initial authentication

Please correct me if I'm wrong on any on this.

(Now I'll look at the code itself and especially where you tagged me)

@ThisIsMissEm

ThisIsMissEm commented Jul 25, 2024 •

Copy link
Copy Markdown
Contributor Author

@ClearlyClaire

this is primarily meant for public clients, which Mastodon doesn't really support at the moment (which is a whole other issue), but can still be useful for confidential clients in some cases, and has no real drawback, and therefore is a MUST in the latest OAuth 2.0 best practices

It was originally for public clients, but is now a MUST for all clients due to authorization code injection attacks.

there is some discussion regarding PKCE and refresh tokens (which we don't currently support either) but my understanding is that for the most part, those are orthogonal and refresh tokens do not typically involve a potentially unsecure channel, unlike the initial authentication

tl;dr; let's disregard refresh tokens for now, and focus on PKCE. (#27948 has been made a draft to indicate it should not be in 4.3)

Doorkeeper's refresh token model is fundamentally broken though, because it doesn't keep trust chains (basically if you revoke the access token the refresh token can remain valid, and if you do a access token -> refresh token -> access token -> refresh token then there's something where the revocation of the original access token doesn't result in the second token set being revoked or something. (I'm still trying to get my head fully around this.

From Auth0:

tl;dr; is that you should actually have a separate oauth_refresh_tokens table, instead of just a column in the oauth_access_tokens table, as you need to be able to follow the chain of tokens.

this is primarily meant for public clients, which Mastodon doesn't really support at the moment (which is a whole other issue),

Yeah, this is something I want to bring support for, but it's "complicated" — I need to fully review the implications of this, but work is started in #30329

@ThisIsMissEm

ThisIsMissEm commented Jul 25, 2024 •

Copy link
Copy Markdown
Contributor Author

@ClearlyClaire @oneiros also, on "public clients", I'm not sure if it actually makes long-term sense to continue support for POST /api/v1/apps once we have implemented OAuth Client ID Metadata Documents, since these are superior to DCR for federated many clients to many servers setups.

(Note: Client ID Metadata Documents are currently an I-D and have been put in front of the IETF OAuth working group at IETF 120 this week, where overall they were well received.

There's also further considerations around public clients vs confidential, which requires things like prohibiting certain grant flows for certain clients. There's also the question of whether jwks_uri is supported, which enables confidential clients potentially (I still need to fully understand this part of the OAuth specs)

@ThisIsMissEm

Copy link
Copy Markdown
Contributor Author

Added an issue about Client ID Metadata Documents here: #31151

@oneiros
oneiros added this pull request to the merge queue Jul 26, 2024
Merged via the queue into mastodon:main with commit 693d9b0 Jul 26, 2024
cheeaun added a commit to cheeaun/phanpy that referenced this pull request Aug 27, 2024
justinwritescode pushed a commit to justinwritescode/mastodon that referenced this pull request Sep 15, 2024
@ThisIsMissEm
ThisIsMissEm deleted the feat/enable-oauth-pkce branch October 1, 2024 19:00
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.

Support OAuth PKCE to allow third-party single-page apps to utilize best practice when authorizing with an instance

3 participants