search

doorkeeper-gem / doorkeeper

Doorkeeper is an OAuth 2 provider for Ruby on Rails / Grape.
https://doorkeeper.gitbook.io/guides/
MIT License
5.33k stars 1.07k forks source link

Option to enforce that clients use PKCE #1654

Open hickford opened 1 year ago

hickford commented 1 year ago

OAuth best practice is to enforce that clients use PKCE. Draft OAuth 2.1 insists authorization servers enforce the use of PKCE by public clients, and recommends enforcing it for all clients https://www.ietf.org/archive/id/draft-ietf-oauth-v2-1-08.html#name-countermeasures-2

using code_challenge and code_verifier is REQUIRED for clients, and authorization servers MUST enforce their use, unless ... The client is a confidential client. ... In this case, using and enforcing code_challenge and code_verifier is still RECOMMENDED.

Thus it would be useful to have an 'enforce client use of PKCE' option with choices: none, public, all.

nbulaj commented 1 year ago

Nice idea @hickford , thanks! Would you mind to create a PR for it?

hickford commented 1 year ago

@nbulaj I don't have the expertise with Ruby

westnordost commented 10 months ago

Perhaps it would be more straightforward to have one option to enforce OAuth 2.1 compliance. (In case there are more requirements for OAuth 2.1 in the final version)

bhuone-garbu commented 3 months ago

We definitely need to implement the PKCE flow for non-public clients. I'm pleased to see that the PR has been merged. Can we confirm if there are any plans to release a new patch for Doorkeeper?

nbulaj commented 3 months ago

Which patch you're talking about @bhuone-garbu ? #1705 was released with 5.7.0. This issue should be closed

Or you're talking about the last point of the MR?

A more stringent option that also requires PKCE for confidential clients could later be added to a bundled OAuth 2.1 option.

bhuone-garbu commented 3 months ago

@nbulaj I just downloaded the latest version 5.7.0 and the config in source code doesn't contain this force_pkce flag at all.

bhuone-garbu commented 3 months ago

I mean #1705 was only merged a month ago but the latest release 5.7.0 was 2 months ago

nbulaj commented 3 months ago

LOL yeah, changelog entry was added to the wrong place https://github.com/doorkeeper-gem/doorkeeper/pull/1705/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR15

Or not :thinking:

bhuone-garbu commented 3 months ago

LOL yeah, changelog entry was added to the wrong place https://github.com/doorkeeper-gem/doorkeeper/pull/1705/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR15

Or not 🤔

either way, I can confirm the 5.7.0 definitely doesn't contain what the main branch has

https://github.com/doorkeeper-gem/doorkeeper/blob/7ffdeec302b747d21e11334d02b2ede0959fc633/lib/doorkeeper/config.rb#L116-L120

bhuone-garbu commented 3 months ago

@nbulaj what's the solution here? 🤔

nbulaj commented 3 months ago

Released as 5.7.1

Kharonus commented 1 week ago

Just checked, that with 5.7.1 force_pkce is only effective on non-confidential apps. Why the restriction? PKCE is useful over an authentication with secret, too, and forcing it on clients for a confidential app make totally sense.

See the PKCE RFC:

PKCE is recommended even if a client is using a client secret ...