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 <james-d-elliott@users.noreply.github.com>

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"

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)
+		switch v, err := loadSecret(value); err {
-		if err != nil {
+		case nil:
-			validator.Push(fmt.Errorf(errFmtSecretIOIssue, value, k, err))
-			return k, ""
-		}
-
 			return k, v
+		default:
+			switch {
+			case os.IsNotExist(err):
+				validator.Push(fmt.Errorf(errFmtSecretOSNotExist, value, k, err))
+
+				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
+			}
+		}
 	}
 }
 

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, "", key)
-	assert.Equal(t, "", value)
+	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))
 }

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[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(errFmtSecretIOIssue, filepath.Join(dir, "duo"), "duo_api.secret_key", fmt.Sprintf(errFmt, filepath.Join(dir, "duo"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "jwt"), "jwt_secret", fmt.Sprintf(errFmt, filepath.Join(dir, "jwt"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "mysql"), "storage.mysql.password", fmt.Sprintf(errFmt, filepath.Join(dir, "mysql"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "notifier"), "notifier.smtp.password", fmt.Sprintf(errFmt, filepath.Join(dir, "notifier"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "oidc-hmac"), "identity_providers.oidc.hmac_secret", fmt.Sprintf(errFmt, filepath.Join(dir, "oidc-hmac"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "oidc-key"), "identity_providers.oidc.issuer_private_key", fmt.Sprintf(errFmt, filepath.Join(dir, "oidc-key"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "postgres"), "storage.postgres.password", fmt.Sprintf(errFmt, filepath.Join(dir, "postgres"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "redis"), "session.redis.password", fmt.Sprintf(errFmt, filepath.Join(dir, "redis"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "redis-sentinel"), "session.redis.high_availability.sentinel_password", fmt.Sprintf(errFmt, filepath.Join(dir, "redis-sentinel"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "session"), "session.secret", fmt.Sprintf(errFmt, filepath.Join(dir, "session"))))
+	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(errFmtSecretIOIssue, filepath.Join(dir, "tls"), "server.tls.key", fmt.Sprintf(errFmt, filepath.Join(dir, "tls"))))
+	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) {

internal/configuration/validator/const.go

@@ -144,6 +144,7 @@ const (
 		"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"
+	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', " +

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))
 		}
 	}

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{

internal/utils/const.go

@@ -123,5 +123,14 @@ var ErrTimeoutReached = errors.New("timeout reached")
 
 const (
 	windows               = "windows"
-	errFmtLinuxNotFound = "open %s: no such file or directory"
+	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"
 )

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 runtime.GOOS {
+	case windows:
 		switch err {
-	case "pathnotfound":
+		case strPathNotFound:
-		switch runtime.GOOS {
+			return fmt.Sprintf(errFmtWindowsNotFound, strOpen, strPath)
-		case windows:
+		case strStat + strPathNotFound:
-			return "open %s: The system cannot find the path specified."
+			return fmt.Sprintf(errFmtWindowsNotFound, strStat, strPath)
-		default:
+		case strFileNotFound:
-			return errFmtLinuxNotFound
+			return fmt.Sprintf(errFmtWindowsNotFound, strOpen, strFile)
-		}
+		case strStat + strFileNotFound:
-	case "filenotfound":
+			return fmt.Sprintf(errFmtWindowsNotFound, strStat, strFile)
-		switch runtime.GOOS {
+		case strIsDir:
-		case windows:
-			return "open %s: The system cannot find the file specified."
-		default:
-			return errFmtLinuxNotFound
-		}
-	case "yamlisdir":
-		switch runtime.GOOS {
-		case windows:
 			return "read %s: The handle is invalid."
+		}
 	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"
 		}
 	}