Option to enforce that clients use PKCE

[hickford]

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]

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

[hickford]

@nbulaj I don't have the expertise with Ruby

[westnordost]

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]

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]

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]

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

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

[nbulaj]

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

Or not 🤔

[bhuone-garbu]

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

doorkeeper/lib/doorkeeper/config.rb

Lines 116 to 120 in 7ffdeec

	# Require non-confidential apps to use PKCE (send a code_verifier) when requesting
	# an access_token using an authorization code (disabled by default)
	def force_pkce
	@config.instance_variable_set(:@force_pkce, true)
	end

[bhuone-garbu]

@nbulaj what's the solution here? 🤔

[nbulaj]

Released as 5.7.1

[Kharonus]

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