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.

Sign up for GitHub

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

Jump to bottom

feat(oauth): Add RFC7636 aka PKCE support. #244

Closed
wants to merge 3 commits into from
Closed

feat(oauth): Add RFC7636 aka PKCE support. #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53e628d
feat(oauth): Add RFC7636 aka PKCE support.
Jan 10, 2018
2a908a1
fix(compilation): Missing implementation from interface.
Jan 10, 2018
d724a36
fix(coverage): Add more tests on pkce package
Jan 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions access_request_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"net/http"
"strings"

"github.com/ory/fosite/pkce"

"net/url"

"github.com/pkg/errors"
Expand Down Expand Up @@ -89,6 +91,30 @@ func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session
}
accessRequest.Client = client

// PKCE Handling
Copy link
Member

Choose a reason for hiding this comment

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

I really think this should be a custom handler and not hardcoded here

Copy link
Author

Choose a reason for hiding this comment

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

Ok it's more like an OAuth2 extension. I'm not really aware of project code organization yet.

Copy link
Author

Choose a reason for hiding this comment

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

Most of the code are (re)generated mocks ^^

Copy link
Author

Choose a reason for hiding this comment

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

About PKCE settings (RequiredForPublicClient), maybe via a decorator (sort of middleware) in the authorize and token endpoints.

This could be helpful to add / process parameters before handling core OAuth operations.

Copy link
Member

Choose a reason for hiding this comment

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

The idea of this framework is to have handlers such as this one for the auth endpoint which implement parts of the oauth2 spec. What you would do (probably) is some checks at the /auth endpoint and then some checks or updates to the response at the /token endpoint

if accessRequest.GetGrantTypes().Has("authorization_code") {
codeVerifier := r.PostForm.Get("code_verifier")
if len(codeVerifier) > 0 {
// Validate code challenge syntax
// https://tools.ietf.org/html/rfc7636#section-4.2
if !pkce.IsValid(codeVerifier) {
return accessRequest, errors.WithStack(ErrInvalidCodeChallenge.WithDebug("Invalid code verifier format"))
}

// Validate code challenge method value
verifier := pkce.GetVerifier(session.GetCodeChallengeMethod())
if verifier == nil {
// https://tools.ietf.org/html/rfc7636#section-4.4.1
return accessRequest, errors.WithStack(ErrCodeChallengeMethodNotSupported)
}

// Verify challenge
if !verifier.Compare(codeVerifier, session.GetCodeChallenge()) {
return accessRequest, errors.WithStack(ErrInvalidCodeChallenge.WithDebug("Challenge comparison failed"))
}
}
}

var found bool = false
for _, loader := range f.TokenEndpointHandlers {
if err := loader.HandleTokenEndpointRequest(ctx, accessRequest); err == nil {
Expand Down
112 changes: 111 additions & 1 deletion access_request_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func TestNewAccessRequest(t *testing.T) {
expectErr error
expect *AccessRequest
handlers TokenEndpointHandlers
session Session
}{
{
header: http.Header{},
Expand Down Expand Up @@ -188,6 +189,107 @@ func TestNewAccessRequest(t *testing.T) {
},
},
},
/* invalid code verifier syntax */
{
header: http.Header{
"Authorization": {basicAuth("foo", "bar")},
},
method: "POST",
form: url.Values{
"grant_type": {"authorization_code"},
"code_verifier": {"123456"},
},
mock: func() {
store.EXPECT().GetClient(gomock.Any(), gomock.Eq("foo")).Return(client, nil)
client.EXPECT().IsPublic().Return(false)
client.EXPECT().GetHashedSecret().Return([]byte("foo"))
hasher.EXPECT().Compare(gomock.Eq([]byte("foo")), gomock.Eq([]byte("bar"))).Return(nil)
},
expectErr: ErrInvalidCodeChallenge,
handlers: TokenEndpointHandlers{handler},
},
/* plain code verifier should pass */
{
session: &DefaultSession{
CodeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM",
CodeChallengeMethod: "plain",
},
header: http.Header{
"Authorization": {basicAuth("foo", "bar")},
},
method: "POST",
form: url.Values{
"grant_type": {"authorization_code"},
"code_verifier": {"E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"},
},
mock: func() {
store.EXPECT().GetClient(gomock.Any(), gomock.Eq("foo")).Return(client, nil)
client.EXPECT().IsPublic().Return(false)
client.EXPECT().GetHashedSecret().Return([]byte("foo"))
hasher.EXPECT().Compare(gomock.Eq([]byte("foo")), gomock.Eq([]byte("bar"))).Return(nil)
handler.EXPECT().HandleTokenEndpointRequest(gomock.Any(), gomock.Any()).Return(nil)
},
handlers: TokenEndpointHandlers{handler},
expect: &AccessRequest{
GrantTypes: Arguments{"authorization_code"},
Request: Request{
Client: client,
},
},
},
/* plain code verifier should not pass */
{
session: &DefaultSession{
CodeChallenge: "E9Melhoa2OwvFrEMTJuCHaoeK1t8URWbuKLMstw-cB",
CodeChallengeMethod: "plain",
},
header: http.Header{
"Authorization": {basicAuth("foo", "bar")},
},
method: "POST",
form: url.Values{
"grant_type": {"authorization_code"},
"code_verifier": {"E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"},
},
mock: func() {
store.EXPECT().GetClient(gomock.Any(), gomock.Eq("foo")).Return(client, nil)
client.EXPECT().IsPublic().Return(false)
client.EXPECT().GetHashedSecret().Return([]byte("foo"))
hasher.EXPECT().Compare(gomock.Eq([]byte("foo")), gomock.Eq([]byte("bar"))).Return(nil)
handler.EXPECT().HandleTokenEndpointRequest(gomock.Any(), gomock.Any()).Return(nil)
},
expectErr: ErrInvalidCodeChallenge,
handlers: TokenEndpointHandlers{handler},
},
/* s256 verifier should pass */
{
session: &DefaultSession{
CodeChallenge: "7MerufSWZM57TmdFh_wY7UcYYjkf6bK30uM9fYEJIJ4",
CodeChallengeMethod: "s256",
},
header: http.Header{
"Authorization": {basicAuth("foo", "bar")},
},
method: "POST",
form: url.Values{
"grant_type": {"authorization_code"},
"code_verifier": {"pokiUom2YM4JRdVNyS79yhNT8qNhxo6yW6FNc39yi787tSrLrb6XXCSTNfeD"},
},
mock: func() {
store.EXPECT().GetClient(gomock.Any(), gomock.Eq("foo")).Return(client, nil)
client.EXPECT().IsPublic().Return(false)
client.EXPECT().GetHashedSecret().Return([]byte("foo"))
hasher.EXPECT().Compare(gomock.Eq([]byte("foo")), gomock.Eq([]byte("bar"))).Return(nil)
//handler.EXPECT().HandleTokenEndpointRequest(gomock.Any(), gomock.Any()).Return(nil)
},
handlers: TokenEndpointHandlers{handler},
expect: &AccessRequest{
GrantTypes: Arguments{"authorization_code"},
Request: Request{
Client: client,
},
},
},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
r := &http.Request{
Expand All @@ -199,7 +301,15 @@ func TestNewAccessRequest(t *testing.T) {
c.mock()
ctx := NewContext()
fosite.TokenEndpointHandlers = c.handlers
ar, err := fosite.NewAccessRequest(ctx, r, new(DefaultSession))

var session Session
if c.session != nil {
session = c.session
} else {
session = new(DefaultSession)
}

ar, err := fosite.NewAccessRequest(ctx, r, session)

if c.expectErr != nil {
assert.EqualError(t, err, c.expectErr.Error())
Expand Down
13 changes: 13 additions & 0 deletions authorize_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ type AuthorizeRequest struct {
State string `json:"state" gorethink:"state"`
HandledResponseTypes Arguments `json:"handledResponseTypes" gorethink:"handledResponseTypes"`

// Optional code_challenge as described in rfc7636
CodeChallenge string `json:"code_challenge" gorethink:"code_challenge"`
Copy link
Member

Choose a reason for hiding this comment

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

I really think this can be covered by Request.Form

Copy link
Author

Choose a reason for hiding this comment

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

I consider that CodeChallenge is like State that's why in coded it here.

Copy link
Author

Choose a reason for hiding this comment

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

My first problem was the Session usage, due to the fact it's an interface and shared in openid and oauth2 handler (I fix missing implementation on first commit). I had to store code_challenge to compare it on the token retrieval step, maybe Request.Form is a better place if it is shareable like session.

Copy link
Member

Choose a reason for hiding this comment

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

So the Request.Form thing will contain all request parameters from the /auth endpoint, it will thus also contain code_challenge if that parameter is given to the /auth endpoint. You won't have to deal with any extra sessions or stuff there.

// Optional code_challenge_method as described in rfc7636
CodeChallengeMethod string `json:"code_challenge_method" gorethink:"code_challenge_method"`

Request
}

Expand Down Expand Up @@ -62,6 +67,14 @@ func (d *AuthorizeRequest) GetState() string {
return d.State
}

func (d *AuthorizeRequest) GetCodeChallenge() string {
return d.CodeChallenge
}

func (d *AuthorizeRequest) GetCodeChallengeMethod() string {
return d.CodeChallengeMethod
}

func (d *AuthorizeRequest) GetRedirectURI() *url.URL {
return d.RedirectURI
}
Expand Down
43 changes: 39 additions & 4 deletions authorize_request_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,12 @@
package fosite

import (
"net/http"
"strings"

"context"

"fmt"
"net/http"
"strings"

"github.com/ory/fosite/pkce"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -80,5 +79,41 @@ func (c *Fosite) NewAuthorizeRequest(ctx context.Context, r *http.Request) (Auth

// Remove empty items from arrays
request.SetRequestedScopes(removeEmpty(strings.Split(r.Form.Get("scope"), " ")))

// Proof Key for Code Exchange by OAuth Public Clients
// https://tools.ietf.org/html/rfc7636
//
// PKCE is only for code response type
if request.ResponseTypes.Has("code") {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here, this shouldn't be a hardcoded thing but instead an add-in via handlers

Copy link
Author

Choose a reason for hiding this comment

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

Yep I'm agree. Applying "Do it, do it right, to do better" philosophy ^^

if codeChallenge := r.Form.Get("code_challenge"); len(codeChallenge) == 0 {
// Force code_challenge only for code response_type
if c.RequirePKCEForPublicClients {
// https://tools.ietf.org/html/rfc7636#section-4.4.1
return request, errors.WithStack(ErrCodeChallengeRequired)
}
} else {
codeChallengeMethod := r.Form.Get("code_challenge_method")
// https://tools.ietf.org/html/rfc7636#section-4.3, default to plain if not specified
if len(codeChallengeMethod) == 0 {
codeChallengeMethod = pkce.Plain
}

// Validate code challenge method value
if pkce.GetVerifier(codeChallengeMethod) == nil {
// https://tools.ietf.org/html/rfc7636#section-4.4.1
return request, errors.WithStack(ErrCodeChallengeMethodNotSupported)
}

// Validate code challenge syntax
// https://tools.ietf.org/html/rfc7636#section-4.2
if !pkce.IsValid(codeChallenge) {
return request, errors.WithStack(ErrInvalidCodeChallenge)
}

// Assign to request
request.CodeChallenge = codeChallenge
}
}

return request, nil
}
84 changes: 81 additions & 3 deletions authorize_request_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,82 @@ func TestNewAuthorizeRequest(t *testing.T) {
store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}}, nil)
},
},
/* missing code_challenge with required for public client */
{
desc: "missing code challenge, required for public client",
conf: &Fosite{Store: store, RequirePKCEForPublicClients: true},
query: url.Values{
"redirect_uri": {"https://foo.bar/cb"},
"client_id": {"1234"},
"response_type": {"code"},
"state": {"strong-state"},
},
expectedError: ErrCodeChallengeRequired,
mock: func() {
store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}}, nil)
},
},
/* invalid code verifier */
{
desc: "invalid challenge code verifier",
conf: &Fosite{Store: store, RequirePKCEForPublicClients: true},
query: url.Values{
"redirect_uri": {"https://foo.bar/cb"},
"client_id": {"1234"},
"response_type": {"code"},
"state": {"strong-state"},
"code_challenge": {"123456"},
"code_challenge_method": {"foo"},
},
expectedError: ErrCodeChallengeMethodNotSupported,
mock: func() {
store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}}, nil)
},
},
/* invalid challenge code syntax */
{
desc: "invalid challenge code syntax",
conf: &Fosite{Store: store, RequirePKCEForPublicClients: true},
query: url.Values{
"redirect_uri": {"https://foo.bar/cb"},
"client_id": {"1234"},
"response_type": {"code"},
"state": {"strong-state"},
"code_challenge": {"123456"},
},
expectedError: ErrInvalidCodeChallenge,
mock: func() {
store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}}, nil)
},
},
/* PKCE success case */
{
desc: "PKCE success case",
conf: &Fosite{Store: store, RequirePKCEForPublicClients: true},
query: url.Values{
"redirect_uri": {"https://foo.bar/cb"},
"client_id": {"1234"},
"response_type": {"code"},
"state": {"strong-state"},
"scope": {"foo bar"},
"code_challenge": {"E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"},
"code_challenge_method": {"s256"},
},
mock: func() {
store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}}, nil)
},
expect: &AuthorizeRequest{
RedirectURI: redir,
ResponseTypes: []string{"code"},
State: "strong-state",
CodeChallenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM",
CodeChallengeMethod: "s256",
Request: Request{
Client: &DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}},
Scopes: []string{"foo", "bar"},
},
},
},
/* success case */
{
desc: "should pass",
Expand All @@ -164,9 +240,11 @@ func TestNewAuthorizeRequest(t *testing.T) {
store.EXPECT().GetClient(gomock.Any(), "1234").Return(&DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}}, nil)
},
expect: &AuthorizeRequest{
RedirectURI: redir,
ResponseTypes: []string{"code", "token"},
State: "strong-state",
RedirectURI: redir,
ResponseTypes: []string{"code", "token"},
State: "strong-state",
CodeChallenge: "",
CodeChallengeMethod: "",
Request: Request{
Client: &DefaultClient{RedirectURIs: []string{"https://foo.bar/cb"}},
Scopes: []string{"foo", "bar"},
Expand Down
Loading