From 32c68804e094f2c1fed5eec25a9b07ff9ebf300e Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sun, 28 May 2023 11:50:55 +1000 Subject: [PATCH] feat(oidc): disable minimum parameter entropy (#5495) This allows disabling the minimum parameter entropy checks. Signed-off-by: James Elliott --- .../openid-connect/provider.md | 2 + internal/configuration/validator/const.go | 6 +- .../validator/identity_providers.go | 11 +- .../validator/identity_providers_test.go | 107 ++++++++++++++---- 4 files changed, 101 insertions(+), 25 deletions(-) diff --git a/docs/content/en/configuration/identity-providers/openid-connect/provider.md b/docs/content/en/configuration/identity-providers/openid-connect/provider.md index 335deff45..12f2b56f1 100644 --- a/docs/content/en/configuration/identity-providers/openid-connect/provider.md +++ b/docs/content/en/configuration/identity-providers/openid-connect/provider.md @@ -305,6 +305,8 @@ make certain scenarios less secure. It is highly encouraged that if your OpenID these parameters or sends parameters with a lower length than the default that they implement a change rather than changing this value. +This restriction can also be disabled entirely when set to `-1`. + ### enforce_pkce {{< confkey type="string" default="public_clients_only" required="no" >}} diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index e6a158d37..a9e934f2f 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -147,8 +147,10 @@ const ( errFmtOIDCProviderNoPrivateKey = "identity_providers: oidc: option `issuer_private_keys` or 'issuer_private_key' is required" errFmtOIDCProviderEnforcePKCEInvalidValue = "identity_providers: oidc: option 'enforce_pkce' must be 'never', " + "'public_clients_only' or 'always', but it's configured as '%s'" - errFmtOIDCProviderInsecureParameterEntropy = "openid connect provider: SECURITY ISSUE - minimum parameter entropy is " + - "configured to an unsafe value, it should be above 8 but it's configured to %d" + errFmtOIDCProviderInsecureParameterEntropy = "identity_providers: oidc: option 'minimum_parameter_entropy' is " + + "configured to an unsafe and insecure value, it should at least be %d but it's configured to %d" + errFmtOIDCProviderInsecureDisabledParameterEntropy = "identity_providers: oidc: option 'minimum_parameter_entropy' is " + + "disabled which is considered unsafe and insecure" errFmtOIDCProviderPrivateKeysInvalid = "identity_providers: oidc: issuer_private_keys: key #%d: option 'key' must be a valid private key but the provided data is malformed as it's missing the public key bits" errFmtOIDCProviderPrivateKeysCalcThumbprint = "identity_providers: oidc: issuer_private_keys: key #%d: option 'key' failed to calculate thumbprint to configure key id value: %w" errFmtOIDCProviderPrivateKeysKeyIDLength = "identity_providers: oidc: issuer_private_keys: key #%d with key id '%s': option `key_id` must be 100 characters or less" diff --git a/internal/configuration/validator/identity_providers.go b/internal/configuration/validator/identity_providers.go index 14e505495..77a29ccc6 100644 --- a/internal/configuration/validator/identity_providers.go +++ b/internal/configuration/validator/identity_providers.go @@ -10,6 +10,8 @@ import ( "strings" "time" + "github.com/ory/fosite" + "github.com/authelia/authelia/v4/internal/configuration/schema" "github.com/authelia/authelia/v4/internal/oidc" "github.com/authelia/authelia/v4/internal/utils" @@ -31,8 +33,13 @@ func validateOIDC(config *schema.OpenIDConnect, val *schema.StructValidator) { sort.Sort(oidc.SortedSigningAlgs(config.Discovery.ResponseObjectSigningAlgs)) - if config.MinimumParameterEntropy != 0 && config.MinimumParameterEntropy < 8 { - val.PushWarning(fmt.Errorf(errFmtOIDCProviderInsecureParameterEntropy, config.MinimumParameterEntropy)) + switch { + case config.MinimumParameterEntropy == -1: + val.PushWarning(fmt.Errorf(errFmtOIDCProviderInsecureDisabledParameterEntropy)) + case config.MinimumParameterEntropy <= 0: + config.MinimumParameterEntropy = fosite.MinParameterEntropy + case config.MinimumParameterEntropy < fosite.MinParameterEntropy: + val.PushWarning(fmt.Errorf(errFmtOIDCProviderInsecureParameterEntropy, fosite.MinParameterEntropy, config.MinimumParameterEntropy)) } switch config.EnforcePKCE { diff --git a/internal/configuration/validator/identity_providers_test.go b/internal/configuration/validator/identity_providers_test.go index 7f9a68932..a282f049f 100644 --- a/internal/configuration/validator/identity_providers_test.go +++ b/internal/configuration/validator/identity_providers_test.go @@ -504,32 +504,97 @@ func TestShouldRaiseErrorOnCertificateNotValid(t *testing.T) { assert.EqualError(t, validator.Errors()[0], "identity_providers: oidc: issuer_private_keys: key #1 with key id 'bf1e10': option 'certificate_chain' does not appear to contain the public key for the private key provided by option 'key'") } -func TestValidateIdentityProvidersShouldRaiseWarningOnSecurityIssue(t *testing.T) { - validator := schema.NewStructValidator() - config := &schema.IdentityProviders{ - OIDC: &schema.OpenIDConnect{ - HMACSecret: "abc", - IssuerPrivateKey: keyRSA2048, - MinimumParameterEntropy: 1, - Clients: []schema.OpenIDConnectClient{ - { - ID: "good_id", - Secret: tOpenIDConnectPBKDF2ClientSecret, - Policy: "two_factor", - RedirectURIs: []string{ - "https://google.com/callback", - }, - }, - }, +func TestValidateIdentityProvidersOpenIDConnectMinimumParameterEntropy(t *testing.T) { + testCases := []struct { + name string + have int + expected int + warnings []string + errors []string + }{ + { + "ShouldNotOverrideCustomValue", + 20, + 20, + nil, + nil, + }, + { + "ShouldSetDefault", + 0, + 8, + nil, + nil, + }, + { + "ShouldSetDefaultNegative", + -2, + 8, + nil, + nil, + }, + { + "ShouldAllowDisabledAndWarn", + -1, + -1, + []string{"identity_providers: oidc: option 'minimum_parameter_entropy' is disabled which is considered unsafe and insecure"}, + nil, + }, + { + "ShouldWarnOnTooLow", + 2, + 2, + []string{"identity_providers: oidc: option 'minimum_parameter_entropy' is configured to an unsafe and insecure value, it should at least be 8 but it's configured to 2"}, + nil, }, } - ValidateIdentityProviders(config, validator) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + validator := schema.NewStructValidator() + config := &schema.IdentityProviders{ + OIDC: &schema.OpenIDConnect{ + HMACSecret: "abc", + IssuerPrivateKey: keyRSA2048, + MinimumParameterEntropy: tc.have, + Clients: []schema.OpenIDConnectClient{ + { + ID: "good_id", + Secret: tOpenIDConnectPBKDF2ClientSecret, + Policy: "two_factor", + RedirectURIs: []string{ + "https://google.com/callback", + }, + }, + }, + }, + } - assert.Len(t, validator.Errors(), 0) - require.Len(t, validator.Warnings(), 1) + ValidateIdentityProviders(config, validator) - assert.EqualError(t, validator.Warnings()[0], "openid connect provider: SECURITY ISSUE - minimum parameter entropy is configured to an unsafe value, it should be above 8 but it's configured to 1") + assert.Equal(t, tc.expected, config.OIDC.MinimumParameterEntropy) + + if n := len(tc.warnings); n == 0 { + assert.Len(t, validator.Warnings(), 0) + } else { + require.Len(t, validator.Warnings(), n) + + for i := 0; i < n; i++ { + assert.EqualError(t, validator.Warnings()[i], tc.warnings[i]) + } + } + + if n := len(tc.errors); n == 0 { + assert.Len(t, validator.Errors(), 0) + } else { + require.Len(t, validator.Errors(), n) + + for i := 0; i < n; i++ { + assert.EqualError(t, validator.Errors()[i], tc.errors[i]) + } + } + }) + } } func TestValidateIdentityProvidersShouldRaiseErrorsOnInvalidClientTypes(t *testing.T) {