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

fix: only use allowed characters in error_description #526

Merged
merged 6 commits into from
Nov 4, 2020
Merged

fix: only use allowed characters in error_description #526

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
811e5e2
fix: only use allowed characters in error_description
aeneasr Nov 4, 2020
02c3af5
fix: allow all request object algs when client value is unset
aeneasr Nov 4, 2020
3f824e1
fix: use state from request object
aeneasr Nov 4, 2020
dc2cbb0
fix: prevent debug details from leaking during key lookup
aeneasr Nov 4, 2020
12c4e18
fix: kid header is not required for key lookup
aeneasr Nov 4, 2020
1078346
fix: do not accidentally leak jwks fetching errors
aeneasr Nov 4, 2020
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
4 changes: 2 additions & 2 deletions access_error_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,14 @@ func TestWriteAccessError_RFC6749(t *testing.T) {

expectDescription := c.err.Description
if c.err.Hint != "" {
expectDescription += "\n\n" + c.err.Hint
expectDescription += " " + c.err.Hint
}

if !c.debug {
assert.Equal(t, expectDescription, params.Description)
assert.Empty(t, params.Debug)
} else {
assert.Equal(t, expectDescription+"\n\n"+c.expectDebugMessage, params.Description)
assert.Equal(t, expectDescription+" "+c.expectDebugMessage, params.Description)
assert.Equal(t, c.expectDebugMessage, params.Debug)
}
})
Expand Down
4 changes: 2 additions & 2 deletions access_request_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session
accessRequest := NewAccessRequest(session)

if r.Method != "POST" {
return accessRequest, errors.WithStack(ErrInvalidRequest.WithHintf("HTTP method is \"%s\", expected \"POST\".", r.Method))
return accessRequest, errors.WithStack(ErrInvalidRequest.WithHintf("HTTP method is '%s', expected 'POST'.", r.Method))
} else if err := r.ParseMultipartForm(1 << 20); err != nil && err != http.ErrNotMultipart {
return accessRequest, errors.WithStack(ErrInvalidRequest.WithHint("Unable to parse HTTP body, make sure to send a properly formatted form request body.").WithCause(err).WithDebug(err.Error()))
} else if len(r.PostForm) == 0 {
Expand All @@ -75,7 +75,7 @@ func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session
accessRequest.SetRequestedAudience(GetAudiences(r.PostForm))
accessRequest.GrantTypes = RemoveEmpty(strings.Split(r.PostForm.Get("grant_type"), " "))
if len(accessRequest.GrantTypes) < 1 {
return accessRequest, errors.WithStack(ErrInvalidRequest.WithHint(`Request parameter "grant_type"" is missing`))
return accessRequest, errors.WithStack(ErrInvalidRequest.WithHint("Request parameter 'grant_type' is missing"))
}

client, err := f.AuthenticateClient(ctx, r, r.PostForm)
Expand Down
6 changes: 3 additions & 3 deletions audience_strategy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ func DefaultAudienceMatchingStrategy(haystack []string, needle []string) error {
for _, n := range needle {
nu, err := url.Parse(n)
if err != nil {
return errors.WithStack(ErrInvalidRequest.WithHintf(`Unable to parse requested audience "%s".`, n).WithCause(err).WithDebug(err.Error()))
return errors.WithStack(ErrInvalidRequest.WithHintf("Unable to parse requested audience '%s'.", n).WithCause(err).WithDebug(err.Error()))
aeneasr marked this conversation as resolved.
Show resolved Hide resolved
}

var found bool
for _, h := range haystack {
hu, err := url.Parse(h)
if err != nil {
return errors.WithStack(ErrInvalidRequest.WithHintf(`Unable to parse whitelisted audience "%s".`, h).WithCause(err).WithDebug(err.Error()))
return errors.WithStack(ErrInvalidRequest.WithHintf("Unable to parse whitelisted audience '%s'.", h).WithCause(err).WithDebug(err.Error()))
}

allowedPath := strings.TrimRight(hu.Path, "/")
Expand All @@ -39,7 +39,7 @@ func DefaultAudienceMatchingStrategy(haystack []string, needle []string) error {
}

if !found {
return errors.WithStack(ErrInvalidRequest.WithHintf(`Requested audience "%s" has not been whitelisted by the OAuth 2.0 Client.`, n))
return errors.WithStack(ErrInvalidRequest.WithHintf("Requested audience '%s' has not been whitelisted by the OAuth 2.0 Client.", n))
}
}

Expand Down
22 changes: 11 additions & 11 deletions authorize_error_test.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion authorize_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func MatchRedirectURIWithClientRedirectURIs(rawurl string, client Client) (*url.
}
}

return nil, errors.WithStack(ErrInvalidRequest.WithHint(`The "redirect_uri" parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls.`))
return nil, errors.WithStack(ErrInvalidRequest.WithHint("The 'redirect_uri' parameter does not match any of the OAuth 2.0 Client's pre-registered redirect urls."))
}

// Match a requested redirect URI against a pool of registered client URIs
Expand Down
55 changes: 30 additions & 25 deletions authorize_request_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,25 +47,25 @@ func (f *Fosite) authorizeRequestParametersFromOpenIDConnectRequest(request *Aut
if len(request.Form.Get("request")+request.Form.Get("request_uri")) == 0 {
return nil
} else if len(request.Form.Get("request")) > 0 && len(request.Form.Get("request_uri")) > 0 {
return errors.WithStack(ErrInvalidRequest.WithHint(`OpenID Connect parameters "request" and "request_uri" were both given, but you can use at most one.`))
return errors.WithStack(ErrInvalidRequest.WithHint("OpenID Connect parameters 'request' and 'request_uri' were both given, but you can use at most one."))
}

oidcClient, ok := request.Client.(OpenIDConnectClient)
if !ok {
if len(request.Form.Get("request_uri")) > 0 {
return errors.WithStack(ErrRequestURINotSupported.WithHint(`OpenID Connect "request_uri" context was given, but the OAuth 2.0 Client does not implement advanced OpenID Connect capabilities.`))
return errors.WithStack(ErrRequestURINotSupported.WithHint("OpenID Connect 'request_uri' context was given, but the OAuth 2.0 Client does not implement advanced OpenID Connect capabilities."))
}
return errors.WithStack(ErrRequestNotSupported.WithHint(`OpenID Connect "request" context was given, but the OAuth 2.0 Client does not implement advanced OpenID Connect capabilities.`))
return errors.WithStack(ErrRequestNotSupported.WithHint("OpenID Connect 'request' context was given, but the OAuth 2.0 Client does not implement advanced OpenID Connect capabilities."))
}

if oidcClient.GetJSONWebKeys() == nil && len(oidcClient.GetJSONWebKeysURI()) == 0 {
return errors.WithStack(ErrInvalidRequest.WithHint(`OpenID Connect "request" or "request_uri" context was given, but the OAuth 2.0 Client does not have any JSON Web Keys registered.`))
return errors.WithStack(ErrInvalidRequest.WithHint("OpenID Connect 'request' or 'request_uri' context was given, but the OAuth 2.0 Client does not have any JSON Web Keys registered."))
}

assertion := request.Form.Get("request")
if location := request.Form.Get("request_uri"); len(location) > 0 {
if !stringslice.Has(oidcClient.GetRequestURIs(), location) {
return errors.WithStack(ErrInvalidRequestURI.WithHintf("Request URI \"%s\" is not whitelisted by the OAuth 2.0 Client.", location))
return errors.WithStack(ErrInvalidRequestURI.WithHintf("Request URI '%s' is not whitelisted by the OAuth 2.0 Client.", location))
}

hc := f.HTTPClient
Expand All @@ -75,25 +75,30 @@ func (f *Fosite) authorizeRequestParametersFromOpenIDConnectRequest(request *Aut

response, err := hc.Get(location)
if err != nil {
return errors.WithStack(ErrInvalidRequestURI.WithHint(`Unable to fetch OpenID Connect request parameters from "request_uri".`).WithCause(err).WithDebug(err.Error()))
return errors.WithStack(ErrInvalidRequestURI.WithHintf("Unable to fetch OpenID Connect request parameters from 'request_uri' because: %s.", err.Error()).WithCause(err).WithDebug(err.Error()))
}
defer response.Body.Close()

if response.StatusCode != http.StatusOK {
return errors.WithStack(ErrInvalidRequestURI.WithHintf(`Unable to fetch OpenID Connect request parameters from "request_uri" because status code "%d" was expected, but got "%d".`, http.StatusOK, response.StatusCode))
return errors.WithStack(ErrInvalidRequestURI.WithHintf("Unable to fetch OpenID Connect request parameters from 'request_uri' because status code '%d' was expected, but got '%d'.", http.StatusOK, response.StatusCode))
}

body, err := ioutil.ReadAll(response.Body)
if err != nil {
return errors.WithStack(ErrInvalidRequestURI.WithHint(`Unable to fetch OpenID Connect request parameters from "request_uri" because error occurred during body parsing.`).WithCause(err).WithDebug(err.Error()))
return errors.WithStack(ErrInvalidRequestURI.WithHintf("Unable to fetch OpenID Connect request parameters from 'request_uri' because body parsing failed with: %s.", err).WithCause(err).WithDebug(err.Error()))
}

assertion = string(body)
}

token, err := jwt.ParseWithClaims(assertion, new(jwt.MapClaims), func(t *jwt.Token) (interface{}, error) {
if oidcClient.GetRequestObjectSigningAlgorithm() != fmt.Sprintf("%s", t.Header["alg"]) {
return nil, errors.WithStack(ErrInvalidRequestObject.WithHintf(`The request object uses signing algorithm %s, but the requested OAuth 2.0 Client enforces signing algorithm %s.`, t.Header["alg"], oidcClient.GetRequestObjectSigningAlgorithm()))
// request_object_signing_alg - OPTIONAL.
// JWS [JWS] alg algorithm [JWA] that MUST be used for signing Request Objects sent to the OP. All Request Objects from this Client MUST be rejected,
// if not signed with this algorithm. Request Objects are described in Section 6.1 of OpenID Connect Core 1.0 [OpenID.Core]. This algorithm MUST
// be used both when the Request Object is passed by value (using the request parameter) and when it is passed by reference (using the request_uri parameter).
// Servers SHOULD support RS256. The value none MAY be used. The default, if omitted, is that any algorithm supported by the OP and the RP MAY be used.
if oidcClient.GetRequestObjectSigningAlgorithm() != "" && oidcClient.GetRequestObjectSigningAlgorithm() != fmt.Sprintf("%s", t.Header["alg"]) {
return nil, errors.WithStack(ErrInvalidRequestObject.WithHintf("The request object uses signing algorithm '%s', but the requested OAuth 2.0 Client enforces signing algorithm '%s'.", t.Header["alg"], oidcClient.GetRequestObjectSigningAlgorithm()))
}

if t.Method == jwt.SigningMethodNone {
Expand All @@ -104,23 +109,23 @@ func (f *Fosite) authorizeRequestParametersFromOpenIDConnectRequest(request *Aut
case *jwt.SigningMethodRSA:
key, err := f.findClientPublicJWK(oidcClient, t, true)
if err != nil {
return nil, errors.WithStack(ErrInvalidRequestObject.WithHint("Unable to retrieve RSA signing key from OAuth 2.0 Client.").WithCause(err).WithDebug(err.Error()))
return nil, errors.WithStack(ErrInvalidRequestObject.WithHintf("Unable to retrieve RSA signing key from OAuth 2.0 Client because: %s.", err).WithCause(err).WithDebug(err.Error()))
aeneasr marked this conversation as resolved.
Show resolved Hide resolved
}
return key, nil
case *jwt.SigningMethodECDSA:
key, err := f.findClientPublicJWK(oidcClient, t, false)
if err != nil {
return nil, errors.WithStack(ErrInvalidRequestObject.WithHint("Unable to retrieve ECDSA signing key from OAuth 2.0 Client.").WithCause(err).WithDebug(err.Error()))
return nil, errors.WithStack(ErrInvalidRequestObject.WithHintf("Unable to retrieve ECDSA signing key from OAuth 2.0 Client because: %s.", err).WithCause(err).WithDebug(err.Error()))
}
return key, nil
case *jwt.SigningMethodRSAPSS:
key, err := f.findClientPublicJWK(oidcClient, t, true)
if err != nil {
return nil, errors.WithStack(ErrInvalidRequestObject.WithHint("Unable to retrieve RSA signing key from OAuth 2.0 Client.").WithCause(err).WithDebug(err.Error()))
return nil, errors.WithStack(ErrInvalidRequestObject.WithHintf("Unable to retrieve RSA signing key from OAuth 2.0 Client because: %s.", err).WithCause(err).WithDebug(err.Error()))
}
return key, nil
default:
return nil, errors.WithStack(ErrInvalidRequestObject.WithHintf(`This request object uses unsupported signing algorithm "%s"."`, t.Header["alg"]))
return nil, errors.WithStack(ErrInvalidRequestObject.WithHintf("This request object uses unsupported signing algorithm '%s'.", t.Header["alg"]))
}
})
if err != nil {
Expand All @@ -139,7 +144,7 @@ func (f *Fosite) authorizeRequestParametersFromOpenIDConnectRequest(request *Aut

claims, ok := token.Claims.(*jwt.MapClaims)
if !ok {
return errors.WithStack(ErrInvalidRequestObject.WithHint("Unable to type assert claims from request object.").WithDebugf(`Got claims of type %T but expected type "*jwt.MapClaims".`, token.Claims))
return errors.WithStack(ErrInvalidRequestObject.WithHint("Unable to type assert claims from request object.").WithDebugf(`Got claims of type %T but expected type '*jwt.MapClaims'.`, token.Claims))
}

for k, v := range *claims {
Expand All @@ -153,11 +158,12 @@ func (f *Fosite) authorizeRequestParametersFromOpenIDConnectRequest(request *Aut
}
}

request.State = request.Form.Get("state")
request.Form.Set("scope", strings.Join(claimScope, " "))
return nil
}

func (f *Fosite) validateAuthorizeRedirectURI(r *http.Request, request *AuthorizeRequest) error {
func (f *Fosite) validateAuthorizeRedirectURI(_ *http.Request, request *AuthorizeRequest) error {
// Fetch redirect URI from request
rawRedirURI := request.Form.Get("redirect_uri")

Expand All @@ -166,17 +172,17 @@ func (f *Fosite) validateAuthorizeRedirectURI(r *http.Request, request *Authoriz
if err != nil {
return err
} else if !IsValidRedirectURI(redirectURI) {
return errors.WithStack(ErrInvalidRequest.WithHintf(`The redirect URI "%s" contains an illegal character (for example #) or is otherwise invalid.`, redirectURI))
return errors.WithStack(ErrInvalidRequest.WithHintf("The redirect URI '%s' contains an illegal character (for example #) or is otherwise invalid.", redirectURI))
}
request.RedirectURI = redirectURI
return nil
}

func (f *Fosite) validateAuthorizeScope(r *http.Request, request *AuthorizeRequest) error {
func (f *Fosite) validateAuthorizeScope(_ *http.Request, request *AuthorizeRequest) error {
scope := RemoveEmpty(strings.Split(request.Form.Get("scope"), " "))
for _, permission := range scope {
if !f.ScopeStrategy(request.Client.GetScopes(), permission) {
return errors.WithStack(ErrInvalidScope.WithHintf(`The OAuth 2.0 Client is not allowed to request scope "%s".`, permission))
return errors.WithStack(ErrInvalidScope.WithHintf("The OAuth 2.0 Client is not allowed to request scope '%s'.", permission))
}
}
request.SetRequestedScopes(scope)
Expand All @@ -192,7 +198,7 @@ func (f *Fosite) validateResponseTypes(r *http.Request, request *AuthorizeReques
// response types is defined by their respective specifications.
responseTypes := RemoveEmpty(strings.Split(r.Form.Get("response_type"), " "))
if len(responseTypes) == 0 {
return errors.WithStack(ErrUnsupportedResponseType.WithHint(`The request is missing the "response_type"" parameter.`))
return errors.WithStack(ErrUnsupportedResponseType.WithHint("`The request is missing the 'response_type' parameter."))
}

var found bool
Expand All @@ -204,7 +210,7 @@ func (f *Fosite) validateResponseTypes(r *http.Request, request *AuthorizeReques
}

if !found {
return errors.WithStack(ErrUnsupportedResponseType.WithHintf("The client is not allowed to request response_type \"%s\".", r.Form.Get("response_type")))
return errors.WithStack(ErrUnsupportedResponseType.WithHintf("The client is not allowed to request response_type '%s'.", r.Form.Get("response_type")))
}

request.ResponseTypes = responseTypes
Expand All @@ -225,8 +231,7 @@ func (f *Fosite) NewAuthorizeRequest(ctx context.Context, r *http.Request) (Auth
request.Form = r.Form

// Save state to the request to be returned in error conditions (https://github.com/ory/hydra/issues/1642)
state := request.Form.Get("state")
request.State = state
request.State = request.Form.Get("state")

client, err := f.Store.GetClient(ctx, request.GetRequestForm().Get("client_id"))
if err != nil {
Expand Down Expand Up @@ -264,9 +269,9 @@ func (f *Fosite) NewAuthorizeRequest(ctx context.Context, r *http.Request) (Auth
//
// https://tools.ietf.org/html/rfc6819#section-4.4.1.8
// The "state" parameter should not be guessable
if len(state) < f.GetMinParameterEntropy() {
if len(request.State) < f.GetMinParameterEntropy() {
// We're assuming that using less then, by default, 8 characters for the state can not be considered "unguessable"
return request, errors.WithStack(ErrInvalidState.WithHintf(`Request parameter "state" must be at least be %d characters long to ensure sufficient entropy.`, f.GetMinParameterEntropy()))
return request, errors.WithStack(ErrInvalidState.WithHintf("Request parameter 'state' must be at least be %d characters long to ensure sufficient entropy.", f.GetMinParameterEntropy()))
}

return request, nil
Expand Down
10 changes: 8 additions & 2 deletions authorize_request_handler_oidc_request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func TestAuthorizeRequestParametersFromOpenIDConnectRequest(t *testing.T) {
}

validRequestObject := mustGenerateAssertion(t, jwt.MapClaims{"scope": "foo", "foo": "bar", "baz": "baz"}, key, "kid-foo")
validNoneRequestObject := mustGenerateNoneAssertion(t, jwt.MapClaims{"scope": "foo", "foo": "bar", "baz": "baz"})
validNoneRequestObject := mustGenerateNoneAssertion(t, jwt.MapClaims{"scope": "foo", "foo": "bar", "baz": "baz", "state": "some-state"})

var reqH http.HandlerFunc = func(rw http.ResponseWriter, r *http.Request) {
rw.Write([]byte(validRequestObject))
Expand Down Expand Up @@ -180,7 +180,13 @@ func TestAuthorizeRequestParametersFromOpenIDConnectRequest(t *testing.T) {
d: "should pass when request object uses algorithm none",
form: url.Values{"scope": {"openid"}, "request": {validNoneRequestObject}},
client: &DefaultOpenIDConnectClient{JSONWebKeysURI: reqJWK.URL, RequestObjectSigningAlgorithm: "none"},
expectForm: url.Values{"scope": {"foo openid"}, "request": {validNoneRequestObject}, "foo": {"bar"}, "baz": {"baz"}},
expectForm: url.Values{"state": {"some-state"}, "scope": {"foo openid"}, "request": {validNoneRequestObject}, "foo": {"bar"}, "baz": {"baz"}},
},
{
d: "should pass when request object uses algorithm none and the client did not explicitly allow any algorithm",
form: url.Values{"scope": {"openid"}, "request": {validNoneRequestObject}},
client: &DefaultOpenIDConnectClient{JSONWebKeysURI: reqJWK.URL},
expectForm: url.Values{"state": {"some-state"}, "scope": {"foo openid"}, "request": {validNoneRequestObject}, "foo": {"bar"}, "baz": {"baz"}},
},
} {
t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) {
Expand Down
Loading