Skip to content

Require code_verifier if code_challenge provided #453

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

Closed
Kehrlann opened this issue Oct 5, 2021 · 2 comments
Closed

Require code_verifier if code_challenge provided #453

Kehrlann opened this issue Oct 5, 2021 · 2 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@Kehrlann
Copy link
Contributor

Kehrlann commented Oct 5, 2021

Current "requireProofKey" setting for clients is slightly unclear in the javadoc, it says:

Set to {@code true} if the client is required to provide a proof key challenge and verifier when performing the Authorization Code Grant flow.

Current Behavior

When requireProofKey = true:

  • in the authorization code request
    • a code_challenge is REQUIRED
  • in the token request
    • if the client is authenticated (e.g. through client_secret), then do not check the code_verifier (so the name "requireProofKey" is a bit misleading, it's not required here)
    • if the client is not authenticated (e.g. no client_secret was present), then check the code_verifier

When requireProofKey = false

  • in the authorization code request
    • a code_challenge is not required but MAY be present
  • in the token request
    • if the client is authenticated (e.g. through client_secret), then do not check the code_verifier
    • if the client is not authenticated (e.g. no client_secret was present), then check the code_verifier, it must be present and valid wrt/ code_challenge. If no code_challenge was supplied, it fails (but it fails by validating PKCE, it maybe should fail earlier?)

Proposed behavior

In every case

  • if a code_challenge is supplied in the authorization code request
    • a valid code_verifier MUST be present in the token request (differs from current behavior)

When requireProofKey = true:

  • in the authorization code request
    • a code_challenge is REQUIRED (same as current behavior)
  • in the token request
    • a valid code_verifier MUST be present (see above, differs from the current behavior)
    • a client_secret or any other authentication, MAY be present, and, in that case, MUST be validated (same as current)

When requireProofKey = false

  • in the authorization code request
    • a code_challenge is not required but MAY be present (same as current)
  • in the token request
    • if a code_challenge was sent in the authorization code request
      • a valid code_verifier MUST be present (see above, this differs from the current behavior)
      • UNSURE: should we also check the authentication? I believe if a client_secret or other authentication is provided, it should be validated, but if no authentication was sent, the request is still valid.
    • if a code_challenge was not sent, a valid authentication is REQUIRED (same as current)

Alternatives

An alternate path could be to change "allowPublicClients" which only does verification on the token requests, but I personally prefer requireProofKey.

@jgrandja
Copy link
Collaborator

Thanks for catching this @Kehrlann. I agree...

if a code_challenge is supplied in the authorization code request
a valid code_verifier MUST be present in the token request (differs from current behavior)

Would you be able to submit a fix for this?

@jgrandja jgrandja added this to the 0.2.1 milestone Oct 21, 2021
@jgrandja jgrandja changed the title Clarify "clientSettings#isRequireProofKey" Validate code_verifier if code_challenge provided Oct 21, 2021
@jgrandja jgrandja changed the title Validate code_verifier if code_challenge provided Require code_verifier if code_challenge provided Oct 21, 2021
@jgrandja jgrandja added type: bug A general bug and removed type: enhancement A general enhancement labels Oct 21, 2021
@Kehrlann
Copy link
Contributor Author

@jgrandja will do!

Kehrlann added a commit to Kehrlann/spring-authorization-server that referenced this issue Oct 22, 2021
doba16 pushed a commit to doba16/spring-authorization-server that referenced this issue Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants