From 6ef6d0499ab0db5155a48fc70b9419f9c3788ac6 Mon Sep 17 00:00:00 2001 From: Andrew Moore Date: Tue, 1 Mar 2022 23:44:05 -0500 Subject: [PATCH] feat(oidc): add pkce support (#2924) Implements Proof Key for Code Exchange for OpenID Connect Authorization Code Flow. By default this is enabled for the public client type and requires the S256 challenge method. Closes #2921 --- config.template.yml | 4 +++ docs/configuration/identity-providers/oidc.md | 34 +++++++++++++++++++ internal/configuration/config.template.yml | 4 +++ .../schema/identity_providers.go | 4 +++ internal/configuration/validator/const.go | 3 ++ .../validator/identity_providers.go | 8 +++++ .../validator/identity_providers_test.go | 18 ++++++++++ internal/handlers/handler_oidc_wellknown.go | 5 +++ internal/oidc/provider.go | 17 ++++++---- internal/oidc/types.go | 11 +++--- 10 files changed, 96 insertions(+), 12 deletions(-) diff --git a/config.template.yml b/config.template.yml index 44820d343..5149041ad 100644 --- a/config.template.yml +++ b/config.template.yml @@ -684,6 +684,10 @@ notifier: ## security reasons. # minimum_parameter_entropy: 8 + ## SECURITY NOTICE: It's not recommended changing this option, and highly discouraged to have it set to 'never' + ## for security reasons. + # enforce_pkce: public_clients_only + ## Clients is a list of known clients and their configuration. # clients: # - diff --git a/docs/configuration/identity-providers/oidc.md b/docs/configuration/identity-providers/oidc.md index 0f832fd5c..f29df5665 100644 --- a/docs/configuration/identity-providers/oidc.md +++ b/docs/configuration/identity-providers/oidc.md @@ -34,6 +34,7 @@ identity_providers: id_token_lifespan: 1h refresh_token_lifespan: 90m enable_client_debug_messages: false + enforce_pkce: public_clients_only clients: - id: myapp description: My Application @@ -184,6 +185,39 @@ This controls the minimum length of the `nonce` and `state` parameters. make certain scenarios less secure. It is highly encouraged that if your OpenID Connect RP does not send these parameters or sends parameters with a lower length than the default that they implement a change rather than changing this value. +### enforce_pkce +
+type: string +{: .label .label-config .label-purple } +default: public_clients_only +{: .label .label-config .label-blue } +required: no +{: .label .label-config .label-green } +
+ +[Proof Key for Code Exchange](https://datatracker.ietf.org/doc/html/rfc7636) enforcement policy: if specified, must be either `never`, `public_clients_only` or `always`. + +If set to `public_clients_only` (default), PKCE will be required for public clients using the Authorization Code flow. + +When set to `always`, PKCE will be required for all clients using the Authorization Code flow. + +***Security Notice:*** Changing this value to `never` is generally discouraged, reducing it from the default can theoretically +make certain client-side applications (mobile applications, SPA) vulnerable to CSRF and authorization code interception attacks. + +### enable_pkce_plain_challenge +
+type: boolean +{: .label .label-config .label-purple } +default: false +{: .label .label-config .label-blue } +required: no +{: .label .label-config .label-green } +
+ +Allows PKCE `plain` challenges when set to `true`. + +***Security Notice:*** Changing this value is generally discouraged. Applications should use the `S256` PKCE challenge method instead. + ### clients A list of clients to configure. The options for each client are described below. diff --git a/internal/configuration/config.template.yml b/internal/configuration/config.template.yml index 44820d343..5149041ad 100644 --- a/internal/configuration/config.template.yml +++ b/internal/configuration/config.template.yml @@ -684,6 +684,10 @@ notifier: ## security reasons. # minimum_parameter_entropy: 8 + ## SECURITY NOTICE: It's not recommended changing this option, and highly discouraged to have it set to 'never' + ## for security reasons. + # enforce_pkce: public_clients_only + ## Clients is a list of known clients and their configuration. # clients: # - diff --git a/internal/configuration/schema/identity_providers.go b/internal/configuration/schema/identity_providers.go index a52cc3695..4ceccabb9 100644 --- a/internal/configuration/schema/identity_providers.go +++ b/internal/configuration/schema/identity_providers.go @@ -21,6 +21,9 @@ type OpenIDConnectConfiguration struct { EnableClientDebugMessages bool `koanf:"enable_client_debug_messages"` MinimumParameterEntropy int `koanf:"minimum_parameter_entropy"` + EnforcePKCE string `koanf:"enforce_pkce"` + EnablePKCEPlainChallenge bool `koanf:"enable_pkce_plain_challenge"` + Clients []OpenIDConnectClientConfiguration `koanf:"clients"` } @@ -49,6 +52,7 @@ var DefaultOpenIDConnectConfiguration = OpenIDConnectConfiguration{ AuthorizeCodeLifespan: time.Minute, IDTokenLifespan: time.Hour, RefreshTokenLifespan: time.Minute * 90, + EnforcePKCE: "public_clients_only", } // DefaultOpenIDConnectClientConfiguration contains defaults for OIDC Clients. diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index 2754d2363..812bbab5a 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -121,6 +121,9 @@ const ( "more clients configured" errFmtOIDCNoPrivateKey = "identity_providers: oidc: option 'issuer_private_key' is required" + errFmtOIDCEnforcePKCEInvalidValue = "identity_providers: oidc: option 'enforce_pkce' must be 'never', " + + "'public_clients_only' or 'always', but it is configured as '%s'" + errFmtOIDCClientsDuplicateID = "identity_providers: oidc: one or more clients have the same id but all client" + "id's must be unique" errFmtOIDCClientsWithEmptyID = "identity_providers: oidc: one or more clients have been configured with " + diff --git a/internal/configuration/validator/identity_providers.go b/internal/configuration/validator/identity_providers.go index 525aef418..7f5fc8086 100644 --- a/internal/configuration/validator/identity_providers.go +++ b/internal/configuration/validator/identity_providers.go @@ -41,6 +41,14 @@ func validateOIDC(config *schema.OpenIDConnectConfiguration, validator *schema.S validator.PushWarning(fmt.Errorf(errFmtOIDCServerInsecureParameterEntropy, config.MinimumParameterEntropy)) } + if config.EnforcePKCE == "" { + config.EnforcePKCE = schema.DefaultOpenIDConnectConfiguration.EnforcePKCE + } + + if config.EnforcePKCE != "never" && config.EnforcePKCE != "public_clients_only" && config.EnforcePKCE != "always" { + validator.Push(fmt.Errorf(errFmtOIDCEnforcePKCEInvalidValue, config.EnforcePKCE)) + } + validateOIDCClients(config, validator) if len(config.Clients) == 0 { diff --git a/internal/configuration/validator/identity_providers_test.go b/internal/configuration/validator/identity_providers_test.go index 00dcb7bfc..83a2af348 100644 --- a/internal/configuration/validator/identity_providers_test.go +++ b/internal/configuration/validator/identity_providers_test.go @@ -29,6 +29,24 @@ func TestShouldRaiseErrorWhenInvalidOIDCServerConfiguration(t *testing.T) { assert.EqualError(t, validator.Errors()[1], errFmtOIDCNoClientsConfigured) } +func TestShouldRaiseErrorWhenOIDCPKCEEnforceValueInvalid(t *testing.T) { + validator := schema.NewStructValidator() + config := &schema.IdentityProvidersConfiguration{ + OIDC: &schema.OpenIDConnectConfiguration{ + HMACSecret: "rLABDrx87et5KvRHVUgTm3pezWWd8LMN", + IssuerPrivateKey: "key-material", + EnforcePKCE: "invalid", + }, + } + + ValidateIdentityProviders(config, validator) + + require.Len(t, validator.Errors(), 2) + + assert.EqualError(t, validator.Errors()[0], "identity_providers: oidc: option 'enforce_pkce' must be 'never', 'public_clients_only' or 'always', but it is configured as 'invalid'") + assert.EqualError(t, validator.Errors()[1], errFmtOIDCNoClientsConfigured) +} + func TestShouldRaiseErrorWhenOIDCServerIssuerPrivateKeyPathInvalid(t *testing.T) { validator := schema.NewStructValidator() config := &schema.IdentityProvidersConfiguration{ diff --git a/internal/handlers/handler_oidc_wellknown.go b/internal/handlers/handler_oidc_wellknown.go index 3582a45b8..6afe1f59b 100644 --- a/internal/handlers/handler_oidc_wellknown.go +++ b/internal/handlers/handler_oidc_wellknown.go @@ -74,6 +74,7 @@ func oidcWellKnown(ctx *middlewares.AutheliaCtx) { oidc.ClaimPreferredUsername, oidc.ClaimDisplayName, }, + CodeChallengeMethodsSupported: []string{"S256"}, RequestURIParameterSupported: false, BackChannelLogoutSupported: false, @@ -82,6 +83,10 @@ func oidcWellKnown(ctx *middlewares.AutheliaCtx) { FrontChannelLogoutSessionSupported: false, } + if ctx.Configuration.IdentityProviders.OIDC.EnablePKCEPlainChallenge { + wellKnown.CodeChallengeMethodsSupported = append(wellKnown.CodeChallengeMethodsSupported, "plain") + } + ctx.SetContentType("application/json") if err := json.NewEncoder(ctx).Encode(wellKnown); err != nil { diff --git a/internal/oidc/provider.go b/internal/oidc/provider.go index 09503c036..b8b3bc2d8 100644 --- a/internal/oidc/provider.go +++ b/internal/oidc/provider.go @@ -23,12 +23,15 @@ func NewOpenIDConnectProvider(configuration *schema.OpenIDConnectConfiguration) provider.Store = NewOpenIDConnectStore(configuration) composeConfiguration := &compose.Config{ - AccessTokenLifespan: configuration.AccessTokenLifespan, - AuthorizeCodeLifespan: configuration.AuthorizeCodeLifespan, - IDTokenLifespan: configuration.IDTokenLifespan, - RefreshTokenLifespan: configuration.RefreshTokenLifespan, - SendDebugMessagesToClients: configuration.EnableClientDebugMessages, - MinParameterEntropy: configuration.MinimumParameterEntropy, + AccessTokenLifespan: configuration.AccessTokenLifespan, + AuthorizeCodeLifespan: configuration.AuthorizeCodeLifespan, + IDTokenLifespan: configuration.IDTokenLifespan, + RefreshTokenLifespan: configuration.RefreshTokenLifespan, + SendDebugMessagesToClients: configuration.EnableClientDebugMessages, + MinParameterEntropy: configuration.MinimumParameterEntropy, + EnforcePKCE: configuration.EnforcePKCE == "always", + EnforcePKCEForPublicClients: configuration.EnforcePKCE != "never", + EnablePKCEPlainChallengeMethod: configuration.EnablePKCEPlainChallenge, } keyManager, err := NewKeyManagerWithConfiguration(configuration) @@ -82,7 +85,7 @@ func NewOpenIDConnectProvider(configuration *schema.OpenIDConnectConfiguration) compose.OAuth2TokenIntrospectionFactory, compose.OAuth2TokenRevocationFactory, - // compose.OAuth2PKCEFactory,. + compose.OAuth2PKCEFactory, ) provider.herodot = herodot.NewJSONWriter(nil) diff --git a/internal/oidc/types.go b/internal/oidc/types.go index 140309ca1..c52b9ac3e 100644 --- a/internal/oidc/types.go +++ b/internal/oidc/types.go @@ -86,11 +86,12 @@ type WellKnownConfiguration struct { Algorithms []string `json:"id_token_signing_alg_values_supported"` UserinfoAlgorithms []string `json:"userinfo_signing_alg_values_supported"` - SubjectTypesSupported []string `json:"subject_types_supported"` - ResponseTypesSupported []string `json:"response_types_supported"` - ResponseModesSupported []string `json:"response_modes_supported"` - ScopesSupported []string `json:"scopes_supported"` - ClaimsSupported []string `json:"claims_supported"` + SubjectTypesSupported []string `json:"subject_types_supported"` + ResponseTypesSupported []string `json:"response_types_supported"` + ResponseModesSupported []string `json:"response_modes_supported"` + ScopesSupported []string `json:"scopes_supported"` + ClaimsSupported []string `json:"claims_supported"` + CodeChallengeMethodsSupported []string `json:"code_challenge_methods_supported"` RequestURIParameterSupported bool `json:"request_uri_parameter_supported"` BackChannelLogoutSupported bool `json:"backchannel_logout_supported"`