Fix: Only check verifier isn't empty when determining whether to do PKCE auth and ignore challenge

[edorsey]

I was attempting to use oryd/hydra with a Matrix Synapse as well as Grafana.

These projects implement server-side auth, but do not follow this recommendation:

Note: although PKCE so far was recommended as a mechanism to protect
native apps, this advice applies to all kinds of OAuth clients,
including web applications.

However, they do include a "challenge" parameter, which appears to be included to try to maximize compatibility with generic OAuth.

This would cause the code below this logic to attempt PKCE verification, even though PKCE is not enforced or expected to be by the client.

Figured out that Synapse uses authlib underneath, which means it probably represents quite a few apps.

This change makes this logic only consider verifier, and ignores challenge in order to address this.

Related issue(s)

I think this discussion may have actually been related to this -> ory/hydra#2671

Checklist

• I have read the contributing guidelines
  and signed the CLA.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got green light (please contact
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added necessary documentation within the code base (if
  appropriate).

Further comments

[CLAassistant]

All committers have signed the CLA.

[aeneasr]

This doesn’t look correct to me. Could you maybe first open an issue with all the information you have gathered so far? I do not believe that it is possible to perform PKCE without those two values involved.

[edorsey]

This doesn’t look correct to me. Could you maybe first open an issue with all the information you have gathered so far? I do not believe that it is possible to perform PKCE without those two values involved.

Yes, I believe you're correct, both values are needed for PKCE. This does not contradict that.

These are private clients that do not implement PKCE, but instead use client_secret_basic. It shouldn't be performing PKCE for these clients as they do not support it. However, because they are providing a parameter named challenge it is attempting PKCE and failing.

When it fails, it tells you "The PKCE code verifier must be at least 43 characters.". That's because the verifier is 0 characters long, but challenge is not an empty string.

I'll try to get around to re-producing the problem again this weekend in order to better document it, if you're still not convinced.

[aeneasr]

I don't think that is the case. PKCE requires the code_challenge and code_verifier parameters to be available (see RFC). challenge itself is not a valid parameter and will be ignored by Ory Hydra / Ory Fosite! Is it possible that you have PKCE enforced and your clients are simply not doing PKCE requests?

[edorsey]

I don't think that is the case. PKCE requires the code_challenge and code_verifier parameters to be available (see RFC). challenge itself is not a valid parameter and will be ignored by Ory Hydra / Ory Fosite! Is it possible that you have PKCE enforced and your clients are simply not doing PKCE requests?

Ah, yes, getting the variable name confused with query parameter name.

I very well might have something misconfigured, reviewing the logs during the problem with fresh eyes is probably a good idea.

I intend to get the logs together this Sunday. Feel free to close this until I’ve provided the logs though.

[aeneasr]

Ok, closing this in that case because I think the change does not make sense as is, which is also why the CI is failing :)

[edorsey]

I don't think that is the case. PKCE requires the code_challenge and code_verifier parameters to be available (see RFC). challenge itself is not a valid parameter and will be ignored by Ory Hydra / Ory Fosite! Is it possible that you have PKCE enforced and your clients are simply not doing PKCE requests?

Finally circling back. I was adding the errant code_challenge on my end. Sorry for the busy work. I appreciate the prompt response and your patience with my hasty contribution. ✌️