Reading deployment secrets from environment variables #827
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors deployment secret management from file-based to environment variable-based configuration, specifically for the encryption key used in cryptographic operations. The change improves scalability and aligns with containerized deployment best practices by reading secrets from environment variables (with .env file fallback for local development) instead of separate secret files.
Key Changes:
- Introduced environment variable expansion in YAML configuration loading via
os.ExpandEnv - Refactored encryption service initialization to read keys from config instead of files
- Updated build scripts to generate
.envfiles with auto-generated secrets for local development - Simplified test setups by removing file-based crypto key dependencies
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/resources/deployment.yaml | Updated crypto configuration to use plaintext key for integration tests |
| start.sh | Added .env file loading function to read environment variables on startup (with path bug) |
| start.ps1 | Added PowerShell .env file loading function for Windows support |
| build.sh | Replaced crypto file generation with .env file generation containing auto-generated secrets |
| build.ps1 | PowerShell version of build script with .env file generation |
| backend/tests/resources/testKey | Removed deprecated test crypto key file |
| backend/internal/system/crypto/encrypt/service_test.go | Refactored tests to use config-based encryption service initialization |
| backend/internal/system/crypto/encrypt/service.go | Refactored to read encryption key from config instead of file, removed file reading logic |
| backend/internal/system/config/config_test.go | Updated test to verify new crypto config structure |
| backend/internal/system/config/config.go | Added environment variable expansion and new crypto config structure |
| backend/internal/notification/*.go | Simplified test setup by removing crypto file dependencies |
| backend/cmd/server/repository/resources/conf/default.json | Added crypto config section with empty default key |
| backend/cmd/server/repository/conf/deployment.yaml | Added crypto config with environment variable placeholder |
| backend/cmd/server/main.go | Added encryption service initialization with nil check |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
==========================================
- Coverage 86.99% 86.96% -0.03%
==========================================
Files 310 310
Lines 24305 24303 -2
Branches 606 606
==========================================
- Hits 21143 21136 -7
- Misses 1967 1971 +4
- Partials 1195 1196 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
backend/cmd/server/main.go:150
- The order of operations in
main()is incorrect. The.envfile is being loaded AFTER the config file has already been loaded and processed byLoadConfig(). This means that whenos.ExpandEnv()is called inLoadConfig()(line 211), the environment variables from.envfile won't be available yet.
The .env file should be loaded BEFORE calling LoadConfig() to ensure that environment variables like THUNDER_CRYPTO_ENC_KEY are available when the deployment.yaml is processed.
Suggested fix:
func initThunderConfigurations(logger *log.Logger, thunderHome string) *config.Config {
// Load the environment variables FIRST
envFilePath := path.Join(thunderHome, "repository/conf/.env")
if err := config.LoadEnv(envFilePath); err != nil {
logger.Fatal("Failed to load environment variables", log.Error(err))
}
// Then load the configurations
configFilePath := path.Join(thunderHome, "repository/conf/deployment.yaml")
defaultConfigPath := path.Join(thunderHome, "repository/resources/conf/default.json")
cfg, err := config.LoadConfig(configFilePath, defaultConfigPath)
if err != nil {
logger.Fatal("Failed to load configurations", log.Error(err))
}
// Initialize runtime configurations
if err := config.InitializeThunderRuntime(thunderHome, cfg); err != nil {
logger.Fatal("Failed to initialize thunder runtime", log.Error(err))
}
return cfg
} // Load the environment variables.
envFilePath := path.Join(thunderHome, "repository/conf/.env")
if err := config.LoadEnv(envFilePath); err != nil {
logger.Fatal("Failed to load environment variables", log.Error(err))
}
// Initialize runtime configurations.
if err := config.InitializeThunderRuntime(thunderHome, cfg); err != nil {
logger.Fatal("Failed to initialize thunder runtime", log.Error(err))
}
| // LoadEnv loads environment variables from a .env file. | ||
| func LoadEnv(path string) error { | ||
| path = filepath.Clean(path) | ||
| content, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, line := range strings.Split(string(content), "\n") { | ||
| // Skip empty lines and comments | ||
| if line == "" || strings.HasPrefix(line, "#") { | ||
| continue | ||
| } | ||
| parts := strings.SplitN(line, "=", 2) | ||
| if len(parts) != 2 { | ||
| continue | ||
| } | ||
| key := strings.TrimSpace(parts[0]) | ||
| value := strings.TrimSpace(parts[1]) | ||
|
|
||
| // Set the environment variable only if it's not already set | ||
| if _, exists := os.LookupEnv(key); !exists { | ||
| if err := os.Setenv(key, value); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The .env file parsing is vulnerable to variable substitution injection. If an attacker can modify the .env file, they could inject values that reference other environment variables using the syntax that os.ExpandEnv would process when the deployment.yaml is loaded. While the .env file itself should be protected, consider documenting that:
- The
.envfile should have restricted permissions (0600) - The
.envfile should not be committed to version control - In production, use proper secret management instead of
.envfiles
Additionally, consider validating that keys don't contain suspicious patterns before setting them.
hwupathum
force-pushed
the
aes-key2
branch
2 times, most recently
from
e366e29 to
ebe2999
Compare
| # Uses the value of THUNDER_CRYPTO_ENC_KEY from the environment. | ||
| # This may come from the local .env file or from a Kubernetes secret when deployed. | ||
| key: "${THUNDER_CRYPTO_ENC_KEY}" | ||
|
|
There was a problem hiding this comment.
The Helm chart configuration uses "${THUNDER_CRYPTO_ENC_KEY}" which will be expanded by the Go application at runtime via os.ExpandEnv(). However, the Helm chart's deployment template (templates/thunder-deployment.yaml) doesn't configure any environment variables or secrets for the pod.
To make this work in Kubernetes deployments, you need to either:
- Add documentation on how users should create and mount a Kubernetes Secret containing
THUNDER_CRYPTO_ENC_KEY - Include a Secret template in the Helm chart and wire it up to the deployment
- Add an
envorenvFromsection in the deployment template to source the encryption key from a Secret
Example addition to templates/thunder-deployment.yaml:
env:
- name: THUNDER_CRYPTO_ENC_KEY
valueFrom:
secretKeyRef:
name: thunder-secrets
key: encryption-key| # Crypto secret configuration for Kubernetes deployments. | |
| # If enabled, the deployment template should mount the secret as an environment variable. | |
| cryptoSecret: | |
| enabled: false | |
| # Name of the Kubernetes Secret containing the encryption key. | |
| secretName: thunder-secrets | |
| # Key in the secret that contains the encryption key value. | |
| secretKey: encryption-key | |
| # If enabled, the deployment template should set the THUNDER_CRYPTO_ENC_KEY environment variable from this secret. |
| logger.Error("failed to decode encryption key from config", log.Error(err)) | ||
| return nil, err |
There was a problem hiding this comment.
[nitpick] The error at line 75 logs "failed to decode encryption key from config" which could expose that an invalid hex string was provided. While the actual key value isn't logged (which is good), consider whether this log message could be used for information disclosure in a security context.
The current implementation is acceptable since:
- The actual key value is not logged
- The error message is generic
- This is only logged during initialization, not during runtime operations
However, ensure debug mode doesn't log the config value elsewhere in the codebase.
| crypto: | ||
| encrypt: | ||
| # Encryption key for cryptography | ||
| key: "<ENCRYPTION_KEY>" |
There was a problem hiding this comment.
The OpenChoreo Helm chart adds a crypto.encrypt.key configuration in values.yaml, but the corresponding template file (templates/thunder-component.yaml) is not being updated in this PR to use this value. This means the encryption key configuration won't be applied when deploying with the OpenChoreo Helm chart.
Either:
- Include the template file changes in this PR to wire up the encryption key, or
- Document in the PR that OpenChoreo Helm chart support will be added in a follow-up PR
| function ensure_environment_secrets() { | ||
| local env_dir=$1 | ||
| local secret_file_name=".env" | ||
|
|
||
| echo "=== Ensuring crypto key file exists in the distribution ===" | ||
| # Generate secrets file if it doesn't exist in the secrets directory | ||
| local local_secrets_file="$SECRETS_DIR/$secret_file_name" | ||
| if [[ ! -f "$local_secrets_file" ]]; then | ||
| mkdir -p "$SECRETS_DIR" | ||
| echo "Generating secrets in $SECRETS_DIR..." | ||
|
|
||
| # Check Whether the key file exists | ||
| if [ -f "$KEY_FILE" ]; then | ||
| echo "Default crypto key file already present in $KEY_FILE. Skipping generation." | ||
| else | ||
| echo "Default crypto key file not found. Generating new key at $KEY_FILE..." | ||
|
|
||
| # Generate key using openssl | ||
| # 32 bytes = 64 hex characters | ||
| local NEW_KEY | ||
| NEW_KEY=$(openssl rand -hex 32 2> /dev/null) | ||
|
|
||
| if [[ $? -ne 0 || -z "$NEW_KEY" ]]; then | ||
| echo "ERROR: Failed to generate crypto key with 'openssl rand -hex 32'." | ||
| echo "Please ensure OpenSSL is installed and in your PATH (required for certs anyway)." | ||
| # Can add more secrets as needed | ||
| local aes_key | ||
| if ! aes_key=$(head -c 32 /dev/urandom | xxd -p -c 32 2>/dev/null); then | ||
| echo "ERROR: Failed to generate AES key. /dev/urandom or xxd not available." | ||
| exit 1 | ||
| fi | ||
| echo "THUNDER_CRYPTO_ENC_KEY=$aes_key" > "$local_secrets_file" | ||
|
|
||
| # Ensure the target directory exists | ||
| mkdir -p "$KEY_DIR" | ||
|
|
||
| # Write the key to the new file. | ||
| echo -n "$NEW_KEY" > "$KEY_FILE" | ||
|
|
||
| echo "Successfully generated and added new crypto key to $KEY_FILE." | ||
| echo "Secrets generated successfully in $SECRETS_DIR." | ||
| else | ||
| echo "Secrets already exist in $SECRETS_DIR." | ||
| fi | ||
|
|
||
| # Copy the generated secrets to the specified directory | ||
| local secrets_file="$env_dir/$secret_file_name" | ||
|
|
||
| # Check if the crypto_file key is defined | ||
| if grep -qE "^\s*crypto_file\s*:" "$DEPLOYMENT_FILE"; then | ||
|
|
||
| # If it is defined check if it matches the default path. | ||
| local expected_line_pattern="^\s*crypto_file\s*:\s*\"$KEY_PATH_IN_YAML\"" | ||
|
|
||
| if grep -qE "$expected_line_pattern" "$DEPLOYMENT_FILE"; then | ||
| # The line exists and matches the default then proceed | ||
| echo "Crypto key file is already configured in $DEPLOYMENT_FILE." | ||
| else | ||
| # The line exists, but has a different value. This is a user misconfiguration. | ||
| echo "ERROR: 'crypto_file' is defined in $DEPLOYMENT_FILE but does not match the default path." >&2 | ||
| echo "This script expects to manage the default key: '$KEY_PATH_IN_YAML'" >&2 | ||
| echo "If you have a custom key, this build script cannot verify it." >&2 | ||
| echo "Please remove the 'crypto_file' line to allow this script to set the default," >&2 | ||
| echo "or ensure it points to the correct default path." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ ! -f "$secrets_file" ]]; then | ||
| mkdir -p "$env_dir" | ||
| echo "Copying secrets to $env_dir..." | ||
| cp "$local_secrets_file" "$secrets_file" | ||
| echo "Secrets copied successfully to $env_dir." | ||
| else | ||
| # The crypto_file line is missing it needs to be added. | ||
| echo "Crypto key file is not configured in $DEPLOYMENT_FILE. Inserting entry..." | ||
|
|
||
| local KEY_FILE_LINE_TO_INSERT=" crypto_file: \"$KEY_PATH_IN_YAML\"" | ||
| local ANCHOR_LINE_PATTERN="key_file: \"repository/resources/security/server.key\"" | ||
| local TEMP_FILE="$DEPLOYMENT_FILE.tmp" | ||
|
|
||
| # Using 'awk' for a safer and more readable insertion | ||
| awk -v anchor="$ANCHOR_LINE_PATTERN" -v new_line="$KEY_FILE_LINE_TO_INSERT" ' | ||
| { | ||
| print $0 # Print the current line | ||
| if ($0 ~ anchor) { | ||
| print new_line # Print the new line after the anchor | ||
| } | ||
| } | ||
| ' "$DEPLOYMENT_FILE" > "$TEMP_FILE" | ||
|
|
||
| # Check if the new line was added. | ||
| if ! grep -q "crypto_file:" "$TEMP_FILE"; then | ||
| echo "ERROR: Could not insert crypto_file line into $DEPLOYMENT_FILE." >&2 | ||
| echo "Anchor line (or pattern '$ANCHOR_LINE_PATTERN') not found." >&2 | ||
| rm "$TEMP_FILE" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Move the new file into place | ||
| mv "$TEMP_FILE" "$DEPLOYMENT_FILE" | ||
|
|
||
| echo "Successfully updated $DEPLOYMENT_FILE to use the default key file." | ||
| echo "Secrets already exist in $env_dir." | ||
| fi | ||
|
|
||
| echo "================================================================" | ||
| } |
There was a problem hiding this comment.
The .env file containing the encryption key is created without explicitly setting restrictive file permissions. While the user's umask may provide some protection, it's a security best practice to explicitly set restrictive permissions (e.g., 0600) on files containing secrets.
Consider adding after line 673:
chmod 600 "$local_secrets_file"And after line 686:
chmod 600 "$secrets_file"This ensures the encryption key file is only readable by the owner, regardless of the system's umask settings.
| function Ensure-Environment-Secrets { | ||
| param( | ||
| [string]$conf_dir | ||
| [string]$env_dir | ||
| ) | ||
|
|
||
| $secret_file_name = ".env" | ||
|
|
||
| # Generate secrets file if it doesn't exist in the secrets directory | ||
| $local_secrets_file = Join-Path $SECRETS_DIR $secret_file_name | ||
|
|
||
| if (-not (Test-Path $local_secrets_file)) { | ||
| New-Item -Path $SECRETS_DIR -ItemType Directory -Force | Out-Null | ||
| Write-Host "Generating secrets in $SECRETS_DIR..." | ||
|
|
||
| $DEPLOYMENT_FILE = Join-Path $conf_dir "deployment.yaml" | ||
| # Resolve the .. path segment to get a clean key directory path | ||
| $KEY_DIR_Temp = Join-Path $conf_dir ".." "resources/security" | ||
| $KEY_DIR = (Resolve-Path -Path $KEY_DIR_Temp).Path | ||
| $KEY_FILE = Join-Path $KEY_DIR "crypto.key" | ||
| $KEY_PATH_IN_YAML = "repository/resources/security/crypto.key" | ||
| # Generate AES key using random bytes | ||
| $bytes = New-Object byte[] 32 | ||
| $rng = [System.Security.Cryptography.RandomNumberGenerator]::Create() | ||
| $rng.GetBytes($bytes) | ||
| $aes_key = [System.BitConverter]::ToString($bytes).Replace('-', '').ToLower() | ||
| $rng.Dispose() | ||
|
|
||
| Write-Host "================================================================" | ||
| Write-Host "Ensuring crypto key file exists..." | ||
| # Write secret to file | ||
| "THUNDER_CRYPTO_ENC_KEY=$aes_key" | Set-Content -Path $local_secrets_file -Encoding ascii | ||
|
|
||
| # 1. Check if the key file exists | ||
| if (Test-Path $KEY_FILE) { | ||
| Write-Host "Default crypto key file already present in $KEY_FILE. Skipping generation." | ||
| Write-Host "Secrets generated successfully in $SECRETS_DIR." | ||
| } | ||
| else { | ||
| Write-Host "Default crypto key file not found. Generating new key at $KEY_FILE..." | ||
| $NEW_KEY = $null | ||
|
|
||
| # Try generating key using OpenSSL first | ||
| $openssl = Get-Command openssl -ErrorAction SilentlyContinue | ||
| if ($openssl) { | ||
| try { | ||
| Write-Host " - Using OpenSSL to generate key..." | ||
| # openssl rand -hex 32 returns a 64-char string. | ||
| $NEW_KEY = (openssl rand -hex 32 | Out-String).Trim() | ||
|
|
||
| if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrEmpty($NEW_KEY) -or $NEW_KEY.Length -ne 64) { | ||
| throw "OpenSSL rand command failed or returned empty/incorrect length." | ||
| } | ||
| } | ||
| catch { | ||
| Write-Host " - OpenSSL failed: $_. Falling back to POSIX tools/DOTNET." | ||
| $NEW_KEY = $null | ||
| } | ||
| } | ||
| else { | ||
| Write-Host " - OpenSSL not found. Falling back to POSIX tools/DOTNET." | ||
| } | ||
|
|
||
| # Try POSIX tools as first fallback option | ||
| if ([string]::IsNullOrEmpty($NEW_KEY)) { | ||
| $bash = Get-Command bash -ErrorAction SilentlyContinue | ||
| if ($bash -and (Test-Path /dev/urandom)) { | ||
| try { | ||
| Write-Host " - Using POSIX tools (/dev/urandom) to generate key..." | ||
| # Command: head -c 32 /dev/urandom | xxd -p -c 256 | ||
| # Generates 32 random bytes, converts to a single line of hex (64 chars) | ||
| # The ToLower() ensures consistency with the openssl/dotnet output. | ||
| $POS_KEY_RAW = (& bash -c 'head -c 32 /dev/urandom | xxd -p -c 256' | Out-String).Trim() | ||
| $NEW_KEY = $POS_KEY_RAW.ToLower() | ||
|
|
||
| if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrEmpty($NEW_KEY) -or $NEW_KEY.Length -ne 64) { | ||
| throw "POSIX key generation command failed or returned invalid length." | ||
| } | ||
| } | ||
| catch { | ||
| Write-Host " - POSIX tool failed: $_. Falling back to .NET cryptography." | ||
| $NEW_KEY = $null | ||
| } | ||
| } | ||
| else { | ||
| Write-Host " - POSIX tools not found or not suitable. Falling back to .NET cryptography." | ||
| } | ||
| } | ||
|
|
||
| # try .NET cryptography as final fallback | ||
| if ([string]::IsNullOrEmpty($NEW_KEY)) { | ||
| try { | ||
| Write-Host " - Using .NET cryptography to generate key..." | ||
| $bytes = New-Object byte[] 32 | ||
| # Note: System.Security.Cryptography.RandomNumberGenerator is available in both .NET Framework and .NET (Core) | ||
| $rng = [System.Security.Cryptography.RandomNumberGenerator]::Create() | ||
| $rng.GetBytes($bytes) | ||
| $rng.Dispose() | ||
| # Convert bytes to lowercase hex string (64 chars) | ||
| $NEW_KEY = ([System.BitConverter]::ToString($bytes) -replace '-', '').ToLower() | ||
| } | ||
| catch { | ||
| throw "Failed to generate crypto key using .NET: $_" | ||
| } | ||
| } | ||
| # --- END: .NET cryptography fallback --- | ||
|
|
||
| # Ensure the target directory exists | ||
| New-Item -Path $KEY_DIR -ItemType Directory -Force | Out-Null | ||
|
|
||
| # Write the key to the new file (NoNewline matches 'echo -n') | ||
| Set-Content -Path $KEY_FILE -Value $NEW_KEY -NoNewline -Encoding Ascii | ||
|
|
||
| Write-Host "Successfully generated and added new crypto key to $KEY_FILE." | ||
| Write-Host "Secrets already exist in $SECRETS_DIR." | ||
| } | ||
|
|
||
| # 2. Check and update deployment.yaml | ||
| if (-not (Test-Path $DEPLOYMENT_FILE)) { | ||
| throw "ERROR: $DEPLOYMENT_FILE not found. Cannot configure crypto key." | ||
| } | ||
|
|
||
| $configLines = Get-Content $DEPLOYMENT_FILE | ||
| $cryptoLine = $configLines | Select-String -Pattern "^\s*crypto_file\s*:" -ErrorAction SilentlyContinue | ||
| # Copy the generated secrets to the specified directory | ||
| $secrets_file = Join-Path $env_dir $secret_file_name | ||
|
|
||
|
|
||
| $ANCHOR_LINE_PATTERN = '^\s*key_file\s*:\s*".*server\.key"' | ||
| $KEY_FILE_LINE_TO_INSERT = ' crypto_file: "{0}"' -f $KEY_PATH_IN_YAML | ||
|
|
||
| if ($cryptoLine) { | ||
| # Config exists, check if it's correct | ||
| $expected_line_pattern = '^\s*crypto_file\s*:\s*"{0}"' -f $KEY_PATH_IN_YAML | ||
| if ($cryptoLine -match $expected_line_pattern) { | ||
| Write-Host "Crypto key file is already configured in $DEPLOYMENT_FILE." | ||
| } | ||
| else { | ||
| throw "ERROR: 'crypto_file' is defined in $DEPLOYMENT_FILE but does not match the default path '$KEY_PATH_IN_YAML'. Please fix or remove the line." | ||
| } | ||
| if (-not (Test-Path $secrets_file)) { | ||
| New-Item -Path $env_dir -ItemType Directory -Force | Out-Null | ||
| Write-Host "Copying secrets to $env_dir..." | ||
| Copy-Item -Path $local_secrets_file -Destination $secrets_file -Force | ||
| Write-Host "Secrets copied successfully to $env_dir." | ||
| } | ||
| else { | ||
| # Config is missing, add it | ||
| Write-Host "Crypto key file is not configured in $DEPLOYMENT_FILE. Inserting entry..." | ||
|
|
||
| $newConfigLines = @() | ||
| $lineInserted = $false | ||
|
|
||
| foreach ($line in $configLines) { | ||
| $newConfigLines += $line | ||
| if ($line -match $ANCHOR_LINE_PATTERN) { | ||
| $newConfigLines += $KEY_FILE_LINE_TO_INSERT | ||
| $lineInserted = $true | ||
| } | ||
| } | ||
|
|
||
| if (-not $lineInserted) { | ||
| throw "ERROR: Could not insert crypto_file line into $DEPLOYMENT_FILE. Anchor line (pattern '$ANCHOR_LINE_PATTERN') not found." | ||
| } | ||
|
|
||
| # Save the file (YAML should use UTF-8) | ||
| Set-Content -Path $DEPLOYMENT_FILE -Value $newConfigLines -Encoding UTF8 | ||
| Write-Host "Successfully updated $DEPLOYMENT_FILE to use the default key file." | ||
| Write-Host "Secrets already exist in $env_dir." | ||
| } | ||
| Write-Host "================================================================" | ||
| } |
There was a problem hiding this comment.
Similar to the bash script, the PowerShell script creates the .env file without explicitly setting restrictive file permissions. In Windows, consider using ACLs to restrict access to the file.
After line 1069, consider adding:
$acl = Get-Acl $local_secrets_file
$acl.SetAccessRuleProtection($true, $false)
$accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule($env:USERNAME, "FullControl", "Allow")
$acl.SetAccessRule($accessRule)
Set-Acl $local_secrets_file $aclThis ensures only the current user can access the encryption key file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
backend/tests/resources/testKey:1
- [nitpick] The test key file
backend/tests/resources/testKeyhas been deleted (shown by the diff removing the only line). However, multiple test files in the notification package were referencing this file path and have been updated to use hardcoded keys instead.
Verify that:
- There are no other tests or code that might still reference this deleted file
- The hardcoded test keys in the updated test files are appropriate for testing purposes and don't contain actual production keys
The change appears intentional and correct based on the test file updates, but it's worth double-checking for any missed references.
| } | ||
|
|
||
| func TestNondefaultKeySize(t *testing.T) { | ||
| func (suite *EncryptionTestSuite) TestNon32() { |
There was a problem hiding this comment.
The test name TestNon32 is unclear and doesn't follow naming conventions. The test is validating that key sizes of 16 and 24 bytes (128-bit and 192-bit AES keys) work correctly, but the name doesn't convey this meaning.
Consider renaming to TestEncryptDecryptWithNon256BitKeys or TestEncryptDecryptWith128And192BitKeys to make the purpose clear.
| func (suite *EncryptionTestSuite) TestNon32() { | |
| func (suite *EncryptionTestSuite) TestEncryptDecryptWith128And192BitKeys() { |
| | `configuration.security.certFile` | Server certificate file path | `repository/resources/security/server.cert` | | ||
| | `configuration.security.keyFile` | Server key file path | `repository/resources/security/server.key` | | ||
| | `configuration.security.cryptoFile` | Crypto key file path | `repository/resources/security/crypto.key` | | ||
| | `configuration.crypto.encrypt.key` | Encryption key for cryptography | See values.yaml | |
There was a problem hiding this comment.
The README documentation entry for configuration.crypto.encrypt.key (line 170) simply says "See values.yaml" which is not very helpful for users. The documentation should explain:
- That this is a 64-character hexadecimal string representing a 256-bit AES key
- How to generate a secure key (e.g., using
openssl rand -hex 32) - That in Kubernetes deployments, this should be stored in a Secret and referenced via environment variable
- The relationship between the values.yaml setting and the THUNDER_CRYPTO_ENC_KEY environment variable
Consider adding a more detailed explanation or a link to a security configuration guide.
| | `configuration.crypto.encrypt.key` | Encryption key for cryptography | See values.yaml | | |
| | `configuration.crypto.encrypt.key` | **Required.** 64-character hexadecimal string (256-bit AES key) used for encryption. <br>Generate securely with `openssl rand -hex 32`. <br>**Kubernetes:** Store in a Secret and reference via the `THUNDER_CRYPTO_ENC_KEY` environment variable. <br>If both are set, the environment variable takes precedence over the value in `values.yaml`. | _None (must be set)_ | |
| // resetSingleton resets the singleton state for testing purposes | ||
| func resetSingleton() { | ||
| instance = nil | ||
| once = sync.Once{} | ||
| } |
There was a problem hiding this comment.
[nitpick] The resetSingleton function (lines 337-340) modifies package-level variables instance and once which are not exported. This function is only used in tests, but it's defined in the test file and accesses unexported package variables. While this works in Go when the test is in the same package, it creates a tight coupling between the test and implementation.
Consider whether this reset functionality should be:
- Exported as a public function for testing purposes (e.g.,
ResetForTesting()) - Or kept as-is with a comment explaining it's a test-only helper that requires same-package access
The current implementation is functional but could benefit from documentation clarifying its purpose and scope.
| // Load the encryption service. | ||
| encrypt.GetEncryptionService() |
There was a problem hiding this comment.
[nitpick] The call to encrypt.GetEncryptionService() on line 74 will panic if the encryption key is not properly configured (as per the implementation in service.go line 57). While this is intentional to prevent the server from starting with invalid configuration, the panic is not caught or logged before propagating up, which could result in an unclear error message.
Consider adding error handling here to provide a more user-friendly error message before the panic occurs, or document that this is intentional fail-fast behavior for critical configuration.
| func (suite *ConfigTestSuite) TestLoadEnv_FileNotFound() { | ||
| // Test with non-existent file | ||
| err := LoadEnv("/non/existent/path/.env") | ||
| assert.Error(suite.T(), err) | ||
| } |
There was a problem hiding this comment.
The test TestLoadEnv_FileNotFound (lines 741-745) expects an error when the .env file doesn't exist, but in the actual application code in main.go (line 131-132), a missing .env file causes a Fatal error, terminating the application. This inconsistency between test expectations and production behavior could lead to confusion.
If the .env file is intended to be optional (as a fallback when environment variables aren't set), the production code should handle missing files gracefully, not fatally. The test should align with the intended production behavior.
| } | ||
| key := strings.TrimSpace(parts[0]) | ||
| value := strings.TrimSpace(parts[1]) | ||
| // Remove surrounding quotes if present |
There was a problem hiding this comment.
The LoadEnv function only sets environment variables if they don't already exist (line 292), which is good for security. However, there's a potential security issue: values with spaces are trimmed on line 282 (value := strings.TrimSpace(parts[1])), which could unintentionally modify secrets that legitimately have leading/trailing whitespace.
Consider only trimming spaces before quote removal, not after. The trimming should handle the case where there are spaces around quotes (e.g., KEY = "value"), but preserve intentional whitespace within the value itself.
| // Remove surrounding quotes if present | |
| // Remove surrounding quotes if present, but do not trim after removing quotes |
|
In the implementation, can we support reading the configurations from the environment variable as a generic feature rather than implementing it to support We can follow the pattern we have used in the #478 to indicate that the value should be read from the environment variable. Can you sync with @rajithacharith
Regarding supporting this (.env), I don't think won't be used in production deployment. So do we need to provide this support. |
2 participants
Purpose
Currently deployment secrets like encryption key is read via a separate secret file [1] which has following drawbacks.
[1] #723
Approach
.env)..envfile and load environment variables.envfile in development environment and packing the file to the distribution.Related Issues
Related PRs
Checklist
Security checks