Skip to content

Commit

Permalink
openid: Adds a validator used to validate OIDC parameters (#266)
Browse files Browse the repository at this point in the history
The validator, for now, validates the prompt parameter of OIDC requests.
  • Loading branch information
arekkas authored May 1, 2018
1 parent 2bf9b6c commit 91c9d19
Show file tree
Hide file tree
Showing 13 changed files with 275 additions and 12 deletions.
67 changes: 60 additions & 7 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions compose/compose_openid.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ func OpenIDConnectExplicitFactory(config *Config, storage interface{}, strategy
IDTokenHandleHelper: &openid.IDTokenHandleHelper{
IDTokenStrategy: strategy.(openid.OpenIDConnectTokenStrategy),
},
OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(config.AllowedPromptValues),
}
}

Expand Down Expand Up @@ -63,6 +64,7 @@ func OpenIDConnectImplicitFactory(config *Config, storage interface{}, strategy
IDTokenHandleHelper: &openid.IDTokenHandleHelper{
IDTokenStrategy: strategy.(openid.OpenIDConnectTokenStrategy),
},
OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(config.AllowedPromptValues),
}
}

Expand All @@ -88,5 +90,6 @@ func OpenIDConnectHybridFactory(config *Config, storage interface{}, strategy in
IDTokenHandleHelper: &openid.IDTokenHandleHelper{
IDTokenStrategy: strategy.(openid.OpenIDConnectTokenStrategy),
},
OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(config.AllowedPromptValues),
}
}
3 changes: 3 additions & 0 deletions compose/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ type Config struct {

// EnablePKCEPlainChallengeMethod sets whether or not to allow the plain challenge method (S256 should be used whenever possible, plain is really discouraged). Defaults to false.
EnablePKCEPlainChallengeMethod bool

// AllowedPromptValues sets which OpenID Connect prompt values the server supports. Defaults to []string{"login", "none", "consent", "select_account"}.
AllowedPromptValues []string
}

// GetScopeStrategy returns the scope strategy to be used. Defaults to glob scope strategy.
Expand Down
7 changes: 6 additions & 1 deletion handler/openid/flow_explicit_auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import (

type OpenIDConnectExplicitHandler struct {
// OpenIDConnectRequestStorage is the storage for open id connect sessions.
OpenIDConnectRequestStorage OpenIDConnectRequestStorage
OpenIDConnectRequestStorage OpenIDConnectRequestStorage
OpenIDConnectRequestValidator *OpenIDConnectRequestValidator

*IDTokenHandleHelper
}
Expand All @@ -56,6 +57,10 @@ func (c *OpenIDConnectExplicitHandler) HandleAuthorizeEndpointRequest(ctx contex
return errors.WithStack(fosite.ErrMisconfiguration.WithDebug("Authorization code has not been issued yet"))
}

if err := c.OpenIDConnectRequestValidator.ValidatePrompt(ar); err != nil {
return err
}

if err := c.OpenIDConnectRequestStorage.CreateOpenIDConnectSession(ctx, resp.GetCode(), ar.Sanitize(oidcParameters)); err != nil {
return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
}
Expand Down
1 change: 1 addition & 0 deletions handler/openid/flow_explicit_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func TestExplicit_HandleAuthorizeEndpointRequest(t *testing.T) {
IDTokenHandleHelper: &IDTokenHandleHelper{
IDTokenStrategy: j,
},
OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(nil),
}
for k, c := range []struct {
description string
Expand Down
5 changes: 5 additions & 0 deletions handler/openid/flow_hybrid.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type OpenIDConnectHybridHandler struct {
AuthorizeExplicitGrantHandler *oauth2.AuthorizeExplicitGrantHandler
IDTokenHandleHelper *IDTokenHandleHelper
ScopeStrategy fosite.ScopeStrategy
OpenIDConnectRequestValidator *OpenIDConnectRequestValidator

Enigma *jwt.RS256JWTStrategy
}
Expand All @@ -59,6 +60,10 @@ func (c *OpenIDConnectHybridHandler) HandleAuthorizeEndpointRequest(ctx context.
return errors.WithStack(fosite.ErrInvalidGrant.WithDebug("The client is not allowed to use the id_token response type"))
}

if err := c.OpenIDConnectRequestValidator.ValidatePrompt(ar); err != nil {
return err
}

sess, ok := ar.GetSession().(Session)
if !ok {
return errors.WithStack(ErrInvalidSession)
Expand Down
3 changes: 2 additions & 1 deletion handler/openid/flow_hybrid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ func TestHybrid_HandleAuthorizeEndpointRequest(t *testing.T) {
IDTokenHandleHelper: &IDTokenHandleHelper{
IDTokenStrategy: idStrategy,
},
ScopeStrategy: fosite.HierarchicScopeStrategy,
ScopeStrategy: fosite.HierarchicScopeStrategy,
OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(nil),
}
for k, c := range []struct {
description string
Expand Down
7 changes: 6 additions & 1 deletion handler/openid/flow_implicit.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ import (
type OpenIDConnectImplicitHandler struct {
AuthorizeImplicitGrantTypeHandler *oauth2.AuthorizeImplicitGrantTypeHandler
*IDTokenHandleHelper
ScopeStrategy fosite.ScopeStrategy
ScopeStrategy fosite.ScopeStrategy
OpenIDConnectRequestValidator *OpenIDConnectRequestValidator

RS256JWTStrategy *jwt.RS256JWTStrategy
}
Expand All @@ -53,6 +54,10 @@ func (c *OpenIDConnectImplicitHandler) HandleAuthorizeEndpointRequest(ctx contex
return errors.WithStack(fosite.ErrInvalidGrant.WithDebug("The client is not allowed to use implicit grant type"))
}

if err := c.OpenIDConnectRequestValidator.ValidatePrompt(ar); err != nil {
return err
}

if ar.GetResponseTypes().Exact("id_token") && !ar.GetClient().GetResponseTypes().Has("id_token") {
return errors.WithStack(fosite.ErrInvalidGrant.WithDebug("The client is not allowed to use response type id_token"))
} else if ar.GetResponseTypes().Matches("token", "id_token") && !ar.GetClient().GetResponseTypes().Has("token", "id_token") {
Expand Down
3 changes: 2 additions & 1 deletion handler/openid/flow_implicit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ func TestImplicit_HandleAuthorizeEndpointRequest(t *testing.T) {
IDTokenHandleHelper: &IDTokenHandleHelper{
IDTokenStrategy: idStrategy,
},
ScopeStrategy: fosite.HierarchicScopeStrategy,
ScopeStrategy: fosite.HierarchicScopeStrategy,
OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(nil),
}
for k, c := range []struct {
description string
Expand Down
98 changes: 98 additions & 0 deletions handler/openid/validator.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Copyright © 2017-2018 Aeneas Rekkas <[email protected]>
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* @author Aeneas Rekkas <[email protected]>
* @Copyright 2017-2018 Aeneas Rekkas <[email protected]>
* @license Apache-2.0
*
*/

package openid

import (
"fmt"

"github.com/ory/fosite"
"github.com/ory/go-convenience/stringslice"
"github.com/ory/go-convenience/stringsx"
"github.com/pkg/errors"
)

type OpenIDConnectRequestValidator struct {
AllowedPrompt []string
}

func NewOpenIDConnectRequestValidator(prompt []string) *OpenIDConnectRequestValidator {
if len(prompt) == 0 {
prompt = []string{"login", "none", "consent", "select_account"}
}

return &OpenIDConnectRequestValidator{
AllowedPrompt: prompt,
}
}

func (v *OpenIDConnectRequestValidator) ValidatePrompt(req fosite.AuthorizeRequester) error {
// prompt is case sensitive!
prompt := stringsx.Splitx(req.GetRequestForm().Get("prompt"), " ")

if req.GetClient().IsPublic() {
// Threat: Malicious Client Obtains Existing Authorization by Fraud
// https://tools.ietf.org/html/rfc6819#section-4.2.3
//
// Authorization servers should not automatically process repeat
// authorizations to public clients unless the client is validated
// using a pre-registered redirect URI

// Client Impersonation
// https://tools.ietf.org/html/rfc8252#section-8.6#
//
// As stated in Section 10.2 of OAuth 2.0 [RFC6749], the authorization
// server SHOULD NOT process authorization requests automatically
// without user consent or interaction, except when the identity of the
// client can be assured. This includes the case where the user has
// previously approved an authorization request for a given client id --
// unless the identity of the client can be proven, the request SHOULD
// be processed as if no previous request had been approved.

// To make sure that we are not vulnerable to this type of attack, we will always require consent for public
// clients.

// If prompt is none - meaning that no consent should be requested, we must terminate with an error.
if stringslice.Has(prompt, "none") {
return errors.WithStack(fosite.ErrConsentRequired.WithDebug("OAuth 2.0 Client is marked public and requires end-user consent, but prompt=none was requested"))
}
}

if !isWhitelisted(prompt, v.AllowedPrompt) {
return errors.WithStack(fosite.ErrInvalidRequest.WithDebug(fmt.Sprintf(`Used unknown value "%s" for prompt parameter`, prompt)))
}

if stringslice.Has(prompt, "none") && len(prompt) > 1 {
// If this parameter contains none with any other value, an error is returned.
return errors.WithStack(fosite.ErrInvalidRequest.WithDebug("Parameter prompt was set to none, but contains other values as well which is not allowed"))
}

return nil
}

func isWhitelisted(items []string, whiteList []string) bool {
for _, item := range items {
if !stringslice.Has(whiteList, item) {
return false
}
}
return true
}
Loading

0 comments on commit 91c9d19

Please sign in to comment.