Skip to content

Conversation

@ThisIsMissEm
Copy link
Contributor

Summary

Whilst getting back to Mastodon OAuth work, I remembered that we didn't have a method to retrieve the configured token_endpoint_auth_methods for OAuth Authorization Server Metadata (RFC 8414), and we needed to hard-code this in mastodon.

Having a helper to determine which client authentication methods are supported is helpful for implementing RFC 8414 (there's WIP for this in #1587 / PR #1737 )

Essentially the client_credentials option maps to token_endpoint_auth_methods in the OAuth Authorization Server Metadata per the IANA registry and the Dynamic Client Registration RFC (linked below)

I do note that this current implementation doesn't really support extensibility, even though additional methods like private_key_jwt have been registered.

(The dynamic client registration spec is the thing that actually defined none, client_secret_basic, and client_secret_post as methods: https://www.rfc-editor.org/rfc/rfc7591.html#section-2 )

Other Information

I'm not sure if this documentation page is correct, which implies client_credentials option can be a proc/method: https://github.com/doorkeeper-gem/doorkeeper/wiki/Changing-how-clients-are-authenticated

If that is the case, we may need an alternative mechanism for this.

Doorkeeper.configure do
orm DOORKEEPER_ORM
client_credentials :from_digest, :from_params
client_credentials :from_basic
Copy link
Contributor Author

Choose a reason for hiding this comment

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

:from_digest was never a "supported" client authentication method, from what I can tell, so instead I've switched this to an actually supported client authentication method. (I looked through the git history for https://github.com/doorkeeper-gem/doorkeeper/blob/main/lib/doorkeeper/oauth/client/credentials.rb )

Copy link
Member

Choose a reason for hiding this comment

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

True. We have some longstanding issue for digest auth

#984

@ThisIsMissEm
Copy link
Contributor Author

ThisIsMissEm commented Apr 16, 2025

My gut feeling here is that we may need to introduce a "ClientAuthentication Registry" like we have a GrantFlow Registry, and then we evaluate those for figuring it out, deprecating client_credentials as an option in favor of client_authentication_methods which would use said registry — this would make it easier for Doorkeeper OpenID to register the additional client authentication methods that it may implement (per spec).

Currently Doorkeeper OpenID hardcodes these values too: https://github.com/doorkeeper-gem/doorkeeper-openid_connect/blob/master/app/controllers/doorkeeper/openid_connect/discovery_controller.rb#L45-L48

However, OpenID also defines the private_key_jwt and client_secret_jwt client authentication methods.

@nbulaj
Copy link
Member

nbulaj commented Apr 17, 2025

I'm not sure if this documentation page is correct, which implies client_credentials option can be a proc/method: https://github.com/doorkeeper-gem/doorkeeper/wiki/Changing-how-clients-are-authenticated

If that is the case, we may need an alternative mechanism for this.

This seems to be very old page, I checked config.rb and other files and don't see any place where we yield a block. So I believe it currently supports only collection, yes

UPD: or actually we can 🤔

image

@ThisIsMissEm
Copy link
Contributor Author

@nbulaj what do you think of this idea for being able to register different client authentication mechanisms?

That'd give an upgrade path for people using the block syntax — just write & register a custom client authentication mechanism.

Also, from what I can tell 'none' for public clients (non-confidential ones) is always an option? (This is currently handled by supporting blank? on credentials by just checking uid)

@nbulaj
Copy link
Member

nbulaj commented Apr 21, 2025

Yeah this sounds great to me @ThisIsMissEm 👍

@ThisIsMissEm
Copy link
Contributor Author

Yeah this sounds great to me @ThisIsMissEm 👍

@nbulaj good! Because it's already mostly implemented in #1772

@ThisIsMissEm
Copy link
Contributor Author

Closing this as it's better implemented by #1772

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