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

enable PKCE by default #807

Closed
isdnfan opened this issue Mar 6, 2024 · 2 comments · Fixed by #956
Closed

enable PKCE by default #807

isdnfan opened this issue Mar 6, 2024 · 2 comments · Fixed by #956

Comments

@isdnfan
Copy link

isdnfan commented Mar 6, 2024

with #740 PKCE was disabled by default. According to different sources PKCE is more secure and recommended for all sorts of clients:

https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-16#section-2.1.1

Clients MUST prevent injection (replay) of authorization codes into the authorization response by attackers. Public clients MUST use PKCE [RFC7636] to this end. For confidential clients, the use of PKCE [RFC7636] is RECOMMENDED.

in general IdP MUST advertise PKCE support and user_oidc should dynamically adopt rather hard-coding one or another variant:

https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-16#section-2.1.1

Authorization servers MUST provide a way to detect their support for PKCE. To this end, they MUST either (a) publish the element code_challenge_methods_supported in their AS metadata ([RFC8414]) containing the supported PKCE challenge methods (which can be used by the client to detect PKCE support) or (b) provide a deployment-specific way to ensure or determine PKCE support by the AS.

Please reverse use_pkce setting and enable PKCE by default:

'user_oidc' => [
    'use_pkce' => true,
],

in order

  • to keep backward compatibility
  • use RECOMMENDED config by default
  • people could still disable it if required by broken IdP
@edward-ly
Copy link
Contributor

I see a few possible solutions here:

  1. Add 'user_oidc' => [ 'use_pkce' => true, ], to the default config.php, in which case there is nothing to be done on the user_oidc side.
  2. Default to true if use_pkce is not specified.
  3. Default to the value of has_internet_connection if use_pkce is not specified.

If possible, (1) would probably be the best solution, but if not, I would go with (3) as the next best option as PKCE is not a strict requirement for private IdPs. I love to hear what everyone thinks before making a decision, though.

@edward-ly edward-ly added question Further information is requested discussion priority: normal and removed enhancement New feature or request labels Oct 3, 2024
@isdnfan
Copy link
Author

isdnfan commented Oct 5, 2024

@edward-ly: my intention was to change the default in case use_pkce is not defined (case 2 in the list).

Please allow some comments on your list:
Option 1: If the setting is defined this is an admin task to provide/adjust right values.
Option 1 relies on config.php option which is not really different from now (may or may not exist after upgrades).
Option 3 would introduce some "hidden" assumption which is not obvious and IMHO not a good practice.

as described in the issue I would appreciate user_oidc doesn't hardcode PKCE support but rather consume it from IdP. and in case there is some broken IdP the admin should take care of it rather adding a burden to everybody else by setting a bad default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants