Skip to content

Added pkce on client side for authorization grant flow. Test cases added#743

Closed
amans330 wants to merge 5 commits intooauthlib:masterfrom
amans330:pkce
Closed

Added pkce on client side for authorization grant flow. Test cases added#743
amans330 wants to merge 5 commits intooauthlib:masterfrom
amans330:pkce

Conversation

@amans330
Copy link
Copy Markdown
Contributor

Hi, Please review my pull request for PKCE implementation on the client-side. Do let me know if any changes are required.

@amans330
Copy link
Copy Markdown
Contributor Author

@auvipy I noticed a build fail. Though it doesn't seem to be caused by my code change. Anything I need to do about this?

@JonathanHuot JonathanHuot added Feature OAuth2-Client This impact the client part of OAuth2. labels Jan 11, 2021
@amans330
Copy link
Copy Markdown
Contributor Author

amans330 commented Mar 2, 2021

@thedrow will finish it by eod today. Just starting a new job today!

@amans330
Copy link
Copy Markdown
Contributor Author

amans330 commented Mar 3, 2021

Hi @JonathanHuot I made the changes based on the above comments. Please review.

@amans330 amans330 requested a review from JonathanHuot March 10, 2021 20:57
Merge branch 'master' into pkce
@amans330
Copy link
Copy Markdown
Contributor Author

amans330 commented May 5, 2021

@JonathanHuot This PR is awaiting review from you.

Copy link
Copy Markdown
Member

@JonathanHuot JonathanHuot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR so far, I have added minor comments inline.

Also, I think it is worth to put additional python functions to compute code_challenge_value and code_verifier. The oauthlib framework still needs to be open to any custom values, but at the same time, by default, it should prepare the ground for the implementor. For example, see the RFC:

   The client first creates a code verifier, "code_verifier", for each
   OAuth 2.0 [RFC6749] Authorization Request, in the following manner:

   code_verifier = high-entropy cryptographic random STRING using the
   unreserved characters [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~"
   from Section 2.3 of [RFC3986], with a minimum length of 43 characters
   and a maximum length of 128 characters.

   ABNF for "code_verifier" is as follows.

   code-verifier = 43*128unreserved
   unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
   ALPHA = %x41-5A / %x61-7A
   DIGIT = %x30-39

   NOTE: The code verifier SHOULD have enough entropy to make it
   impractical to guess the value.  It is RECOMMENDED that the output of
   a suitable random number generator be used to create a 32-octet
   sequence.  The octet sequence is then base64url-encoded to produce a
   43-octet URL safe string to use as the code verifier.

and

   The client then creates a code challenge derived from the code
   verifier by using one of the following transformations on the code
   verifier:

   plain
      code_challenge = code_verifier

   S256
      code_challenge = BASE64URL-ENCODE(SHA256(ASCII(code_verifier)))

   If the client is capable of using "S256", it MUST use "S256", as
   "S256" is Mandatory To Implement (MTI) on the server.  Clients are
   permitted to use "plain" only if they cannot support "S256" for some
   technical reason and know via out-of-band configuration that the
   server supports "plain".

By doing this in oauthlib side, it will prevent implementor to do mistake during the implementation and will be very easy to implement it right.
Thoughts?

to the client. The parameter SHOULD be used for preventing
cross-site request forgery as described in `Section 10.12`_.

:param code_challenge: OPTIONAL. A challenge derived from the code verifier that is sent in the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "OPTIONAL" of code_challenge is a bit misleading compared to the same for code_challenge_method.
If we are handling a client with PKCE enforced, the code_challenge is REQUIRED, and method is always OPTIONAL, but also ignored in case of client is not using PKCE.

What do you think ?

authorization request as described in
`Section 4.1.1`_, and their values MUST be identical. *

:param code_verifier: A cryptographically random string that is used to correlate the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, I think because these are options only for Authorization Server supporting PKCE and with clients with PKCE enabled (or enforced), we should somehow, find a way to specify it is a PKCE parameter only.

@rigzba21
Copy link
Copy Markdown
Contributor

Hi, @JonathanHuot, I'd love to see this capability/enhancement in oauthlib! I've rebased my fork's feature branch with this PR's commits, and will hopefully have a WIP/draft PR to submit soon. Thanks!

@rigzba21 rigzba21 mentioned this pull request Oct 29, 2021
@auvipy auvipy closed this Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature OAuth2-Client This impact the client part of OAuth2.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants