Skip to content

Conversation

@Teeed
Copy link
Contributor

@Teeed Teeed commented Feb 14, 2020

This is my own implementation of PKCE, it is similar to #1407 but this version could be directly merged.

Issue #1114

@Teeed Teeed requested a review from sagikazarmark February 17, 2020 09:57
@mfmarche
Copy link

Hi @Teeed , I tried your PR and it was mostly functional.

It seems there is no way to get to the handleAuthCode without actually passing the basic auth (providing a valid client secret). Shouldn't the handling of the secret be deferred only in the case that PKCE flow is not used (ie. when there is no code_verifier?). Making this change allows for PKCE to commence without a secret.

@justin-slowik
Copy link
Contributor

Has there been any progress on implementing PKCE? I've been working on a branch which implements the OAuth 2 Device Authorization Grant, and I know that to make a fully featured native app flow, PKCE should be included as well. Would be beneficial not to duplicate work

}

if codeChallengeFromStorage != calculatedCodeChallenge {
s.tokenErrHelper(w, errInvalidRequest, "invalid code_verifier.", http.StatusBadRequest)
Copy link
Contributor

@deric deric Jun 16, 2020

Choose a reason for hiding this comment

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

According to RFC 7636, errInvalidGrant should be returned.

code_verifier == code_challenge.
If the values are equal, the token endpoint MUST continue processing
as normal (as defined by OAuth 2.0 [RFC6749]). If the values are not equal, an error response indicating "invalid_grant" as described in Section 5.2 of [RFC6749] MUST be returned.

@Teeed
Copy link
Contributor Author

Teeed commented Jun 16, 2020

Yeah. First of all: sorry for not responding for such a long time. Thanks for raising issues! 👍

Company which I work for (and which allowed me to push these changes to upstream) decided that this changes are no longer needed and hence development of these changes was stalled. I currently work on different projects, so I do not have time (and business reason 😕) to fix raised issues.

If anyone wants to continue this work - please do!

@heidemn-faro
Copy link
Contributor

heidemn-faro commented Jul 20, 2020

Hi @Teeed, thanks for your efforts here.
Please allow me a stupid question, since I'm not familiar with the codebase:

Does this add:

  1. PKCE for the client (e.g. a single-page app) connecting to Dex, or
  2. PKCE for Dex connecting to the upstream IdP (if this makes sense at all...)?

We would be interested in case 1.

@heidemn-faro
Copy link
Contributor

Hi @Teeed ,

If anyone wants to continue this work - please do!

Our company wants to finish your work.
Could you please sign off your commit, meaning that you accept the Dex DCO?
Otherwise, I'm not sure if we're allowed to reuse your work in our follow-up PR.

Signing off the commit should work like this:

git checkout <BRANCH>
git commit --amend --signoff
git push --force-with-lease <ORIGIN> <BRANCH>

Thanks

Signed-off-by: Tadeusz Magura-Witkowski <[email protected]>
@HEllRZA
Copy link
Contributor

HEllRZA commented Oct 26, 2020

This PR can be closed as #1784, which is based on this, has been merged. Thanks again to @Teeed for his work!

@Teeed
Copy link
Contributor Author

Teeed commented Oct 26, 2020

Closing, as these changes got merged with #1784

@Teeed Teeed closed this Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants