diff --git a/backend/internal/appconfig/service.go b/backend/internal/appconfig/service.go index 7412b7ae..99126a87 100644 --- a/backend/internal/appconfig/service.go +++ b/backend/internal/appconfig/service.go @@ -217,31 +217,28 @@ func (s *AppConfigService) loadDbConfigFromEnv() (*AppConfigModel, error) { return nil, fmt.Errorf("app configuration field %s is missing its environment variable name", field.Name) } - // Set the value if it's set + // Sensitive values can also be loaded from a file, using the variable with the "_FILE" suffix (files have precedence over the env var) + if field.Tag.Get("sensitive") == "true" { + value, ok, err := common.LoadStringEnvVarFromFile(envVarName) + if err != nil { + return nil, err + } + if ok { + rv.Field(i).SetString(value) + continue + } + } + + // Set the value from the environment variable if it's set value, ok := os.LookupEnv(envVarName) if ok { rv.Field(i).SetString(value) - continue - } - - // If it's sensitive, we also allow reading from file - if field.Tag.Get("sensitive") == "true" { - fileName := os.Getenv(envVarName + "_FILE") - if fileName != "" { - // #nosec G703 - Value is provided by admin - b, err := os.ReadFile(fileName) - if err != nil { - return nil, fmt.Errorf("failed to read secret '%s' from file '%s': %w", envVarName, fileName, err) - } - - rv.Field(i).SetString(string(b)) - continue - } } } // Validate the resolved configuration before exposing values to the rest of the application - if err := validateEnvConfig(dest); err != nil { + err := validateEnvConfig(dest) + if err != nil { return nil, err } @@ -252,12 +249,13 @@ func (s *AppConfigService) loadDbConfigFromEnv() (*AppConfigModel, error) { func validateEnvConfig(config *AppConfigModel) error { // Map the resolved model to the canonical update DTO so both configuration paths share validation rules var input dto.AppConfigUpdateDto - if err := dto.MapStruct(config, &input); err != nil { + err := dto.MapStruct(config, &input) + if err != nil { return fmt.Errorf("failed to prepare environment app configuration for validation: %w", err) } // Collect every invalid environment variable - err := input.Validate() + err = input.Validate() if err != nil { validationErrors, ok := errors.AsType[validator.ValidationErrors](err) if !ok { diff --git a/backend/internal/appconfig/service_test.go b/backend/internal/appconfig/service_test.go index d53863b0..0cc6838c 100644 --- a/backend/internal/appconfig/service_test.go +++ b/backend/internal/appconfig/service_test.go @@ -2,6 +2,8 @@ package appconfig import ( "encoding/json" + "os" + "path/filepath" "testing" "time" @@ -458,3 +460,69 @@ func TestService_CIMDURLAllowlist(t *testing.T) { assert.Empty(t, svc.GetCIMDURLAllowlist()) }) } + +func TestService_LoadDbConfigFromEnv(t *testing.T) { + // writeSecretFile writes content to a file in a temporary directory that is removed when the test ends, returning its path + writeSecretFile := func(t *testing.T, content string) string { + t.Helper() + + fileName := filepath.Join(t.TempDir(), "secret.txt") + err := os.WriteFile(fileName, []byte(content), 0600) + require.NoError(t, err) + + return fileName + } + + t.Run("loads a sensitive value from a file", func(t *testing.T) { + t.Setenv("LDAP_BIND_PASSWORD_FILE", writeSecretFile(t, "my-ldap-password")) + + svc := &AppConfigService{} + cfg, err := svc.loadDbConfigFromEnv() + require.NoError(t, err) + + assert.Equal(t, AppConfigValue("my-ldap-password"), cfg.LdapBindPassword) + }) + + t.Run("trims whitespace from values loaded from a file", func(t *testing.T) { + // Files containing secrets normally end with a trailing newline, which must not be part of the value + t.Setenv("LDAP_BIND_PASSWORD_FILE", writeSecretFile(t, "my-ldap-password\n")) + t.Setenv("SMTP_PASSWORD_FILE", writeSecretFile(t, " my-smtp-password \r\n")) + + svc := &AppConfigService{} + cfg, err := svc.loadDbConfigFromEnv() + require.NoError(t, err) + + assert.Equal(t, AppConfigValue("my-ldap-password"), cfg.LdapBindPassword) + assert.Equal(t, AppConfigValue("my-smtp-password"), cfg.SmtpPassword) + }) + + t.Run("the file takes precedence over the environment variable", func(t *testing.T) { + t.Setenv("LDAP_BIND_PASSWORD", "from-env") + t.Setenv("LDAP_BIND_PASSWORD_FILE", writeSecretFile(t, "from-file\n")) + + svc := &AppConfigService{} + cfg, err := svc.loadDbConfigFromEnv() + require.NoError(t, err) + + assert.Equal(t, AppConfigValue("from-file"), cfg.LdapBindPassword) + }) + + t.Run("values that are not sensitive are not loaded from a file", func(t *testing.T) { + t.Setenv("LDAP_BIND_DN_FILE", writeSecretFile(t, "cn=from-file")) + + svc := &AppConfigService{} + cfg, err := svc.loadDbConfigFromEnv() + require.NoError(t, err) + + assert.Empty(t, cfg.LdapBindDn) + }) + + t.Run("returns an error when the file cannot be read", func(t *testing.T) { + t.Setenv("LDAP_BIND_PASSWORD_FILE", filepath.Join(t.TempDir(), "does-not-exist.txt")) + + svc := &AppConfigService{} + _, err := svc.loadDbConfigFromEnv() + require.Error(t, err) + assert.ErrorContains(t, err, "LDAP_BIND_PASSWORD_FILE") + }) +} diff --git a/backend/internal/common/env_config.go b/backend/internal/common/env_config.go index ce51557b..0814b243 100644 --- a/backend/internal/common/env_config.go +++ b/backend/internal/common/env_config.go @@ -390,36 +390,30 @@ func resolveFileBasedEnvVariable(field reflect.Value, fieldType reflect.StructFi return nil } - // Only process fields with the "env" tag - envTag := fieldType.Tag.Get("env") - if envTag == "" { + // Only process fields with the "env" tag, ignoring any option that follows the name of the variable + envVarName, _, _ := strings.Cut(fieldType.Tag.Get("env"), ",") + if envVarName == "" { return nil } - envVarName := envTag - if commaIndex := len(envTag); commaIndex > 0 { - envVarName = envTag[:commaIndex] - } - - // If the file environment variable is not set, skip - envVarFileName := envVarName + "_FILE" - envVarFileValue := os.Getenv(envVarFileName) - if envVarFileValue == "" { - return nil - } - - // #nosec G703 - Path is passed by the admin - fileContent, err := os.ReadFile(envVarFileValue) - if err != nil { - return fmt.Errorf("failed to read file for env var %s: %w", envVarFileName, err) - } - + // Load the value from the file referenced by the "_FILE" variable, keeping the value parsed from the environment if that variable is not set if isString { - field.SetString(strings.TrimSpace(string(fileContent))) - } else { - field.SetBytes(fileContent) + value, ok, err := LoadStringEnvVarFromFile(envVarName) + if err != nil || !ok { + return err + } + + field.SetString(value) + return nil } + value, ok, err := LoadEnvVarFromFile(envVarName) + if err != nil || !ok { + return err + } + + field.SetBytes(value) + return nil } diff --git a/backend/internal/common/env_file.go b/backend/internal/common/env_file.go new file mode 100644 index 00000000..fa98d964 --- /dev/null +++ b/backend/internal/common/env_file.go @@ -0,0 +1,41 @@ +package common + +import ( + "fmt" + "os" + "strings" +) + +// FileEnvVarSuffix is the suffix of the environment variable that contains the path to the file with the value of another environment variable +// For example, the value of "DB_CONNECTION_STRING" can be loaded from the file whose path is in "DB_CONNECTION_STRING_FILE" +const FileEnvVarSuffix = "_FILE" + +// LoadEnvVarFromFile loads the value of the environment variable envVarName from the file referenced by the corresponding "_FILE" environment variable +// The second return value is false when the "_FILE" variable is not set, in which case the caller should use the value of the environment variable itself +// The content of the file is returned as-is: use LoadStringEnvVarFromFile for values that are used as strings +func LoadEnvVarFromFile(envVarName string) ([]byte, bool, error) { + fileEnvVarName := envVarName + FileEnvVarSuffix + fileName := os.Getenv(fileEnvVarName) + if fileName == "" { + return nil, false, nil + } + + // #nosec G703 - Path is passed by the admin + fileContent, err := os.ReadFile(fileName) + if err != nil { + return nil, false, fmt.Errorf("failed to read file '%s' for env var %s: %w", fileName, fileEnvVarName, err) + } + + return fileContent, true, nil +} + +// LoadStringEnvVarFromFile is like LoadEnvVarFromFile, but returns the content of the file as a string, with leading and trailing whitespace removed +// Trimming is required because tools that write secrets to a file, including shell redirections and most editors, normally append a trailing newline +func LoadStringEnvVarFromFile(envVarName string) (string, bool, error) { + fileContent, ok, err := LoadEnvVarFromFile(envVarName) + if err != nil || !ok { + return "", false, err + } + + return strings.TrimSpace(string(fileContent)), true, nil +} diff --git a/backend/internal/common/env_file_test.go b/backend/internal/common/env_file_test.go new file mode 100644 index 00000000..11da719a --- /dev/null +++ b/backend/internal/common/env_file_test.go @@ -0,0 +1,93 @@ +package common + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadEnvVarFromFile(t *testing.T) { + t.Run("returns false when the file env var is not set", func(t *testing.T) { + value, ok, err := LoadEnvVarFromFile("TEST_SECRET") + + require.NoError(t, err) + assert.False(t, ok) + assert.Empty(t, value) + }) + + t.Run("returns the content of the file as-is", func(t *testing.T) { + fileName := writeTempFile(t, "secret.txt", " my-secret\n") + t.Setenv("TEST_SECRET_FILE", fileName) + + value, ok, err := LoadEnvVarFromFile("TEST_SECRET") + + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, []byte(" my-secret\n"), value) + }) + + t.Run("returns an error when the file cannot be read", func(t *testing.T) { + t.Setenv("TEST_SECRET_FILE", filepath.Join(t.TempDir(), "does-not-exist.txt")) + + _, ok, err := LoadEnvVarFromFile("TEST_SECRET") + + require.Error(t, err) + require.ErrorContains(t, err, "TEST_SECRET_FILE") + assert.False(t, ok) + }) +} + +func TestLoadStringEnvVarFromFile(t *testing.T) { + t.Run("returns false when the file env var is not set", func(t *testing.T) { + value, ok, err := LoadStringEnvVarFromFile("TEST_SECRET") + + require.NoError(t, err) + assert.False(t, ok) + assert.Empty(t, value) + }) + + t.Run("trims leading and trailing whitespace", func(t *testing.T) { + fileName := writeTempFile(t, "secret.txt", "\tmy-secret \r\n") + t.Setenv("TEST_SECRET_FILE", fileName) + + value, ok, err := LoadStringEnvVarFromFile("TEST_SECRET") + + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "my-secret", value) + }) + + t.Run("returns an empty value when the file is empty", func(t *testing.T) { + fileName := writeTempFile(t, "empty.txt", "\n") + t.Setenv("TEST_SECRET_FILE", fileName) + + value, ok, err := LoadStringEnvVarFromFile("TEST_SECRET") + + require.NoError(t, err) + assert.True(t, ok) + assert.Empty(t, value) + }) + + t.Run("returns an error when the file cannot be read", func(t *testing.T) { + t.Setenv("TEST_SECRET_FILE", filepath.Join(t.TempDir(), "does-not-exist.txt")) + + _, ok, err := LoadStringEnvVarFromFile("TEST_SECRET") + + require.Error(t, err) + assert.False(t, ok) + }) +} + +// writeTempFile writes content to a file in a temporary directory that is removed when the test ends, returning its path +func writeTempFile(t *testing.T, name string, content string) string { + t.Helper() + + fileName := filepath.Join(t.TempDir(), name) + err := os.WriteFile(fileName, []byte(content), 0600) + require.NoError(t, err) + + return fileName +}