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

PKCE support #10871

Merged
merged 3 commits into from Sep 28, 2016
Merged

PKCE support #10871

merged 3 commits into from Sep 28, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Sep 10, 2016

This adds support for https://tools.ietf.org/html/rfc7636, which improves the security of code challenges with public clients (clients for which a client_secret of "" is valid)

When requesting an authorization code, a "code challenge" (generated by hashing a privately held "code verifier") can be sent.

When exchanging the code for an access token, the unhashed code verifier must be provided along with the authorization code in order to successfully obtain an access token. This means that someone who intercepted the authorization code would not be able to obtain an access token, despite no client_secret existing.

Builds on #10819

@liggitt liggitt changed the title WIP - PKCE support PKCE support Sep 10, 2016
@liggitt
Copy link
Contributor Author

liggitt commented Sep 10, 2016

[test]

@liggitt
Copy link
Contributor Author

liggitt commented Sep 12, 2016

flake on #10487, [test]

@liggitt liggitt added this to the 1.4.0 milestone Sep 12, 2016
@enj
Copy link
Contributor

enj commented Sep 12, 2016

This line in osincli probably needs to be updated to support an empty secret.

// https://tools.ietf.org/html/rfc7636#section-4.1
if matched := pkceMatcher.MatchString(ret.CodeVerifier); !matched {
w.SetError(E_INVALID_REQUEST, "code_verifier invalid (rfc7636)")
w.InternalError = errors.New("invalid format")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why? I just care about differentiating between a bad format and a failed comparison with code_challenge (below)

Copy link
Contributor

Choose a reason for hiding this comment

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

The internal errors show up in the server log. It would be nice if the error was code_verifier invalid format so you don't have to figure out what is invalid (I ended up putting debug print statements when working on the browser CLI flow in places where it wasn't possible to set the internal error so that way I could differentiate between the different E_INVALID_REQUEST). Similarly, below the error could be code_verifier failed comparison with code_challenge.

@enj
Copy link
Contributor

enj commented Sep 12, 2016

Other than the comment about the InternalError, the code LGTM. I will work on the osincli PR - should I made it is so the client always uses PKCE or only when the client is public?

@liggitt
Copy link
Contributor Author

liggitt commented Sep 12, 2016

I would probably just make these changes to osincli:

  1. allow an empty client_secret
  2. add CodeChallenge, CodeChallengeMethod, CodeVerifier fields to the client config
  3. set CodeChallenge/CodeChallengeMethod on code requests, if specified
  4. set CodeVerifier on token requests, if specified
  5. add a helper func that generates a verifier, challenge, and challenge method

@liggitt
Copy link
Contributor Author

liggitt commented Sep 13, 2016

opened https://github.com/RangelReale/osin/pull/134, will pick up in the next bump

@liggitt liggitt added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2016
@liggitt liggitt removed this from the 1.4.0 milestone Sep 15, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2016
@liggitt liggitt added needs-api-review and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 28, 2016
@liggitt
Copy link
Contributor Author

liggitt commented Sep 28, 2016

@openshift/api-review for the two new fields in OAuthAuthorizeToken

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to dc4d900

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9402/)

@smarterclayton
Copy link
Contributor

API approved

@liggitt
Copy link
Contributor Author

liggitt commented Sep 28, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 28, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9402/) (Image: devenv-rhel7_5093)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to dc4d900

@openshift-bot openshift-bot merged commit bfdf642 into openshift:master Sep 28, 2016
@liggitt liggitt deleted the pkce branch September 28, 2016 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants