From 622bf42ed4f1a54347d70928e9aef9b3f0336647 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sat, 8 Apr 2023 16:02:34 +1000 Subject: [PATCH] fix(configuration): secret permission errors panic (#5141) This fixes an issue where attempting to load secrets the process does not have read permissions for would cause panics as well as the bit size check of the OpenID Connect 1.0 private key can potentially panic on malformed private keys. This was caused by us returning values on errors instead of nil's. Fixes #5138 Signed-off-by: James Elliott --- internal/configuration/const.go | 4 +- internal/configuration/koanf_callbacks.go | 25 +++++++--- .../configuration/koanf_callbacks_test.go | 6 +-- internal/configuration/provider_test.go | 27 +++++------ internal/configuration/validator/const.go | 11 +++-- .../validator/identity_providers.go | 4 +- .../validator/identity_providers_test.go | 29 ++++++++++++ internal/utils/const.go | 13 +++++- internal/utils/errs.go | 46 ++++++++++--------- 9 files changed, 113 insertions(+), 52 deletions(-) diff --git a/internal/configuration/const.go b/internal/configuration/const.go index 7b6321784..cc9b5a6fb 100644 --- a/internal/configuration/const.go +++ b/internal/configuration/const.go @@ -28,7 +28,9 @@ var ( const ( errFmtSecretAlreadyDefined = "secrets: error loading secret into key '%s': it's already defined in other " + "configuration sources" - errFmtSecretIOIssue = "secrets: error loading secret path %s into key '%s': %v" + errFmtSecretOSError = "secrets: error loading secret path %s into key '%s': %w" + errFmtSecretOSPermission = "secrets: error loading secret path %s into key '%s': file permission error occurred: %w" + errFmtSecretOSNotExist = "secrets: error loading secret path %s into key '%s': file does not exist error occurred: %w" errFmtGenerateConfiguration = "error occurred generating configuration: %+v" errFmtDecodeHookCouldNotParse = "could not decode '%s' to a %s%s: %w" diff --git a/internal/configuration/koanf_callbacks.go b/internal/configuration/koanf_callbacks.go index d62984eb1..b7218a639 100644 --- a/internal/configuration/koanf_callbacks.go +++ b/internal/configuration/koanf_callbacks.go @@ -2,6 +2,7 @@ package configuration import ( "fmt" + "os" "strings" "github.com/spf13/pflag" @@ -40,13 +41,25 @@ func koanfEnvironmentSecretsCallback(keyMap map[string]string, validator *schema return "", nil } - v, err := loadSecret(value) - if err != nil { - validator.Push(fmt.Errorf(errFmtSecretIOIssue, value, k, err)) - return k, "" - } + switch v, err := loadSecret(value); err { + case nil: + return k, v + default: + switch { + case os.IsNotExist(err): + validator.Push(fmt.Errorf(errFmtSecretOSNotExist, value, k, err)) - return k, v + return "", nil + case os.IsPermission(err): + validator.Push(fmt.Errorf(errFmtSecretOSPermission, value, k, err)) + + return "", nil + default: + validator.Push(fmt.Errorf(errFmtSecretOSError, value, k, err)) + + return "", nil + } + } } } diff --git a/internal/configuration/koanf_callbacks_test.go b/internal/configuration/koanf_callbacks_test.go index 561038efd..039f18637 100644 --- a/internal/configuration/koanf_callbacks_test.go +++ b/internal/configuration/koanf_callbacks_test.go @@ -117,10 +117,10 @@ func TestKoanfSecretCallbackShouldErrorOnFSError(t *testing.T) { callback := koanfEnvironmentSecretsCallback(keyMap, val) key, value := callback("AUTHELIA_THEME", secret) - assert.Equal(t, "theme", key) - assert.Equal(t, "", value) + assert.Equal(t, "", key) + assert.Equal(t, nil, value) require.Len(t, val.Errors(), 1) assert.Len(t, val.Warnings(), 0) - assert.EqualError(t, val.Errors()[0], fmt.Sprintf(errFmtSecretIOIssue, secret, "theme", fmt.Sprintf("open %s: permission denied", secret))) + assert.EqualError(t, val.Errors()[0], fmt.Sprintf("secrets: error loading secret path %s into key 'theme': file permission error occurred: open %s: permission denied", secret, secret)) } diff --git a/internal/configuration/provider_test.go b/internal/configuration/provider_test.go index f60d68651..f91b9629b 100644 --- a/internal/configuration/provider_test.go +++ b/internal/configuration/provider_test.go @@ -22,7 +22,7 @@ func TestShouldErrorSecretNotExist(t *testing.T) { testSetEnv(t, "JWT_SECRET_FILE", filepath.Join(dir, "jwt")) testSetEnv(t, "DUO_API_SECRET_KEY_FILE", filepath.Join(dir, "duo")) testSetEnv(t, "SESSION_SECRET_FILE", filepath.Join(dir, "session")) - testSetEnv(t, "AUTHENTICATION_BACKEND_LDAP_PASSWORD_FILE", filepath.Join(dir, "authentication")) + testSetEnv(t, "AUTHENTICATION_BACKEND_LDAP_PASSWORD_FILE", dir) testSetEnv(t, "NOTIFIER_SMTP_PASSWORD_FILE", filepath.Join(dir, "notifier")) testSetEnv(t, "SESSION_REDIS_PASSWORD_FILE", filepath.Join(dir, "redis")) testSetEnv(t, "SESSION_REDIS_HIGH_AVAILABILITY_SENTINEL_PASSWORD_FILE", filepath.Join(dir, "redis-sentinel")) @@ -44,20 +44,21 @@ func TestShouldErrorSecretNotExist(t *testing.T) { sort.Sort(utils.ErrSliceSortAlphabetical(errs)) errFmt := utils.GetExpectedErrTxt("filenotfound") + errFmtDir := utils.GetExpectedErrTxt("isdir") // ignore the errors before this as they are checked by the validator. - assert.EqualError(t, errs[0], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "authentication"), "authentication_backend.ldap.password", fmt.Sprintf(errFmt, filepath.Join(dir, "authentication")))) - assert.EqualError(t, errs[1], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "duo"), "duo_api.secret_key", fmt.Sprintf(errFmt, filepath.Join(dir, "duo")))) - assert.EqualError(t, errs[2], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "jwt"), "jwt_secret", fmt.Sprintf(errFmt, filepath.Join(dir, "jwt")))) - assert.EqualError(t, errs[3], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "mysql"), "storage.mysql.password", fmt.Sprintf(errFmt, filepath.Join(dir, "mysql")))) - assert.EqualError(t, errs[4], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "notifier"), "notifier.smtp.password", fmt.Sprintf(errFmt, filepath.Join(dir, "notifier")))) - assert.EqualError(t, errs[5], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "oidc-hmac"), "identity_providers.oidc.hmac_secret", fmt.Sprintf(errFmt, filepath.Join(dir, "oidc-hmac")))) - assert.EqualError(t, errs[6], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "oidc-key"), "identity_providers.oidc.issuer_private_key", fmt.Sprintf(errFmt, filepath.Join(dir, "oidc-key")))) - assert.EqualError(t, errs[7], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "postgres"), "storage.postgres.password", fmt.Sprintf(errFmt, filepath.Join(dir, "postgres")))) - assert.EqualError(t, errs[8], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "redis"), "session.redis.password", fmt.Sprintf(errFmt, filepath.Join(dir, "redis")))) - assert.EqualError(t, errs[9], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "redis-sentinel"), "session.redis.high_availability.sentinel_password", fmt.Sprintf(errFmt, filepath.Join(dir, "redis-sentinel")))) - assert.EqualError(t, errs[10], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "session"), "session.secret", fmt.Sprintf(errFmt, filepath.Join(dir, "session")))) - assert.EqualError(t, errs[11], fmt.Sprintf(errFmtSecretIOIssue, filepath.Join(dir, "tls"), "server.tls.key", fmt.Sprintf(errFmt, filepath.Join(dir, "tls")))) + assert.EqualError(t, errs[0], fmt.Sprintf("secrets: error loading secret path %s into key 'authentication_backend.ldap.password': %s", dir, fmt.Sprintf(errFmtDir, dir))) + assert.EqualError(t, errs[1], fmt.Sprintf("secrets: error loading secret path %s into key 'duo_api.secret_key': file does not exist error occurred: %s", filepath.Join(dir, "duo"), fmt.Sprintf(errFmt, filepath.Join(dir, "duo")))) + assert.EqualError(t, errs[2], fmt.Sprintf("secrets: error loading secret path %s into key 'jwt_secret': file does not exist error occurred: %s", filepath.Join(dir, "jwt"), fmt.Sprintf(errFmt, filepath.Join(dir, "jwt")))) + assert.EqualError(t, errs[3], fmt.Sprintf("secrets: error loading secret path %s into key 'storage.mysql.password': file does not exist error occurred: %s", filepath.Join(dir, "mysql"), fmt.Sprintf(errFmt, filepath.Join(dir, "mysql")))) + assert.EqualError(t, errs[4], fmt.Sprintf("secrets: error loading secret path %s into key 'notifier.smtp.password': file does not exist error occurred: %s", filepath.Join(dir, "notifier"), fmt.Sprintf(errFmt, filepath.Join(dir, "notifier")))) + assert.EqualError(t, errs[5], fmt.Sprintf("secrets: error loading secret path %s into key 'identity_providers.oidc.hmac_secret': file does not exist error occurred: %s", filepath.Join(dir, "oidc-hmac"), fmt.Sprintf(errFmt, filepath.Join(dir, "oidc-hmac")))) + assert.EqualError(t, errs[6], fmt.Sprintf("secrets: error loading secret path %s into key 'identity_providers.oidc.issuer_private_key': file does not exist error occurred: %s", filepath.Join(dir, "oidc-key"), fmt.Sprintf(errFmt, filepath.Join(dir, "oidc-key")))) + assert.EqualError(t, errs[7], fmt.Sprintf("secrets: error loading secret path %s into key 'storage.postgres.password': file does not exist error occurred: %s", filepath.Join(dir, "postgres"), fmt.Sprintf(errFmt, filepath.Join(dir, "postgres")))) + assert.EqualError(t, errs[8], fmt.Sprintf("secrets: error loading secret path %s into key 'session.redis.password': file does not exist error occurred: %s", filepath.Join(dir, "redis"), fmt.Sprintf(errFmt, filepath.Join(dir, "redis")))) + assert.EqualError(t, errs[9], fmt.Sprintf("secrets: error loading secret path %s into key 'session.redis.high_availability.sentinel_password': file does not exist error occurred: %s", filepath.Join(dir, "redis-sentinel"), fmt.Sprintf(errFmt, filepath.Join(dir, "redis-sentinel")))) + assert.EqualError(t, errs[10], fmt.Sprintf("secrets: error loading secret path %s into key 'session.secret': file does not exist error occurred: %s", filepath.Join(dir, "session"), fmt.Sprintf(errFmt, filepath.Join(dir, "session")))) + assert.EqualError(t, errs[11], fmt.Sprintf("secrets: error loading secret path %s into key 'server.tls.key': file does not exist error occurred: %s", filepath.Join(dir, "tls"), fmt.Sprintf(errFmt, filepath.Join(dir, "tls")))) } func TestLoadShouldReturnErrWithoutValidator(t *testing.T) { diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index 0a91a138f..44ac2622b 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -142,11 +142,12 @@ const ( const ( errFmtOIDCNoClientsConfigured = "identity_providers: oidc: option 'clients' must have one or " + "more clients configured" - errFmtOIDCNoPrivateKey = "identity_providers: oidc: option 'issuer_private_key' is required" - errFmtOIDCInvalidPrivateKeyBitSize = "identity_providers: oidc: option 'issuer_private_key' must be an RSA private key with %d bits or more but it only has %d bits" - errFmtOIDCCertificateMismatch = "identity_providers: oidc: option 'issuer_private_key' does not appear to be the private key the certificate provided by option 'issuer_certificate_chain'" - errFmtOIDCCertificateChain = "identity_providers: oidc: option 'issuer_certificate_chain' produced an error during validation of the chain: %w" - errFmtOIDCEnforcePKCEInvalidValue = "identity_providers: oidc: option 'enforce_pkce' must be 'never', " + + errFmtOIDCNoPrivateKey = "identity_providers: oidc: option 'issuer_private_key' is required" + errFmtOIDCInvalidPrivateKeyBitSize = "identity_providers: oidc: option 'issuer_private_key' must be an RSA private key with %d bits or more but it only has %d bits" + errFmtOIDCInvalidPrivateKeyMalformedMissingPublicKey = "identity_providers: oidc: option 'issuer_private_key' must be a valid RSA private key but the provided data is missing the public key bits" + errFmtOIDCCertificateMismatch = "identity_providers: oidc: option 'issuer_private_key' does not appear to be the private key the certificate provided by option 'issuer_certificate_chain'" + errFmtOIDCCertificateChain = "identity_providers: oidc: option 'issuer_certificate_chain' produced an error during validation of the chain: %w" + errFmtOIDCEnforcePKCEInvalidValue = "identity_providers: oidc: option 'enforce_pkce' must be 'never', " + "'public_clients_only' or 'always', but it is configured as '%s'" errFmtOIDCCORSInvalidOrigin = "identity_providers: oidc: cors: option 'allowed_origins' contains an invalid value '%s' as it has a %s: origins must only be scheme, hostname, and an optional port" diff --git a/internal/configuration/validator/identity_providers.go b/internal/configuration/validator/identity_providers.go index 95f7c2707..b9ea7e9b9 100644 --- a/internal/configuration/validator/identity_providers.go +++ b/internal/configuration/validator/identity_providers.go @@ -37,7 +37,9 @@ func validateOIDC(config *schema.OpenIDConnectConfiguration, val *schema.StructV } } - if config.IssuerPrivateKey.Size()*8 < 2048 { + if config.IssuerPrivateKey.PublicKey.N == nil { + val.Push(fmt.Errorf(errFmtOIDCInvalidPrivateKeyMalformedMissingPublicKey)) + } else if config.IssuerPrivateKey.Size()*8 < 2048 { val.Push(fmt.Errorf(errFmtOIDCInvalidPrivateKeyBitSize, 2048, config.IssuerPrivateKey.Size()*8)) } } diff --git a/internal/configuration/validator/identity_providers_test.go b/internal/configuration/validator/identity_providers_test.go index d77844046..8957c61f8 100644 --- a/internal/configuration/validator/identity_providers_test.go +++ b/internal/configuration/validator/identity_providers_test.go @@ -525,6 +525,35 @@ func TestShouldRaiseErrorOnKeySizeTooSmall(t *testing.T) { assert.EqualError(t, validator.Errors()[0], "identity_providers: oidc: option 'issuer_private_key' must be an RSA private key with 2048 bits or more but it only has 1024 bits") } +func TestShouldRaiseErrorOnKeyInvalidPublicKey(t *testing.T) { + validator := schema.NewStructValidator() + config := &schema.IdentityProvidersConfiguration{ + OIDC: &schema.OpenIDConnectConfiguration{ + HMACSecret: "rLABDrx87et5KvRHVUgTm3pezWWd8LMN", + IssuerPrivateKey: MustParseRSAPrivateKey(testKey3), + Clients: []schema.OpenIDConnectClientConfiguration{ + { + ID: "good_id", + Secret: MustDecodeSecret(goodOpenIDConnectClientSecret), + Policy: "two_factor", + RedirectURIs: []string{ + "https://google.com/callback", + }, + }, + }, + }, + } + + config.OIDC.IssuerPrivateKey.PublicKey.N = nil + + ValidateIdentityProviders(config, validator) + + assert.Len(t, validator.Warnings(), 0) + require.Len(t, validator.Errors(), 1) + + assert.EqualError(t, validator.Errors()[0], "identity_providers: oidc: option 'issuer_private_key' must be a valid RSA private key but the provided data is missing the public key bits") +} + func TestShouldRaiseErrorWhenOIDCClientConfiguredWithBadResponseModes(t *testing.T) { validator := schema.NewStructValidator() config := &schema.IdentityProvidersConfiguration{ diff --git a/internal/utils/const.go b/internal/utils/const.go index 3ce72025f..eb53c2d93 100644 --- a/internal/utils/const.go +++ b/internal/utils/const.go @@ -122,6 +122,15 @@ var htmlEscaper = strings.NewReplacer( var ErrTimeoutReached = errors.New("timeout reached") const ( - windows = "windows" - errFmtLinuxNotFound = "open %s: no such file or directory" + windows = "windows" + errFmtLinuxNotFound = "%s %%s: no such file or directory" + errFmtWindowsNotFound = "%s %%s: The system cannot find the %s specified." + + strStat = "stat" + strOpen = "open" + strFile = "file" + strPath = "path" + strIsDir = "isdir" + strPathNotFound = "pathnotfound" + strFileNotFound = "filenotfound" ) diff --git a/internal/utils/errs.go b/internal/utils/errs.go index 7dc0f828e..a96b9e1ea 100644 --- a/internal/utils/errs.go +++ b/internal/utils/errs.go @@ -1,6 +1,9 @@ package utils -import "runtime" +import ( + "fmt" + "runtime" +) // ErrSliceSortAlphabetical is a helper type that can be used with sort.Sort to sort a slice of errors in alphabetical // order. Usage is simple just do sort.Sort(ErrSliceSortAlphabetical([]error{})). @@ -12,28 +15,29 @@ func (s ErrSliceSortAlphabetical) Less(i, j int) bool { return s[i].Error() < s[ func (s ErrSliceSortAlphabetical) Swap(i, j int) { s[i], s[j] = s[j], s[i] } -// GetExpectedErrTxt returns error text for expected errs. +// GetExpectedErrTxt returns error text for expected errs. THIS IS A TEST UTILITY FUNCTION. func GetExpectedErrTxt(err string) string { - switch err { - case "pathnotfound": - switch runtime.GOOS { - case windows: - return "open %s: The system cannot find the path specified." - default: - return errFmtLinuxNotFound - } - case "filenotfound": - switch runtime.GOOS { - case windows: - return "open %s: The system cannot find the file specified." - default: - return errFmtLinuxNotFound - } - case "yamlisdir": - switch runtime.GOOS { - case windows: + switch runtime.GOOS { + case windows: + switch err { + case strPathNotFound: + return fmt.Sprintf(errFmtWindowsNotFound, strOpen, strPath) + case strStat + strPathNotFound: + return fmt.Sprintf(errFmtWindowsNotFound, strStat, strPath) + case strFileNotFound: + return fmt.Sprintf(errFmtWindowsNotFound, strOpen, strFile) + case strStat + strFileNotFound: + return fmt.Sprintf(errFmtWindowsNotFound, strStat, strFile) + case strIsDir: return "read %s: The handle is invalid." - default: + } + default: + switch err { + case strPathNotFound, strFileNotFound: + return fmt.Sprintf(errFmtLinuxNotFound, strOpen) + case strStat + strPathNotFound, strStat + strFileNotFound: + return fmt.Sprintf(errFmtLinuxNotFound, strStat) + case strIsDir: return "read %s: is a directory" } }