security: fix 7 credential exposure and secret management issues#391
Merged
security: fix 7 credential exposure and secret management issues#391
Conversation
…URL path normalization - Remove hardcoded fallback secrets in platform/config.go - Add validateConfig() to fail startup if required secrets are missing - Fix path normalization in pkg/crypto/presign.go SignURL to match VerifyURL - Update config tests to set required environment variables
Remove the development fallback secret that was used when SECRETS_ENCRYPTION_KEY was not configured. The server now exits with an error if the environment variable is not set.
- Use MYSQL_PWD and POSTGRES_PASSWORD env vars instead of CLI args - Pass passwords via shell wrapper to avoid appearing in process list - Fix credential rotation to store in Vault FIRST before updating DB - Update rotation tests to match new versioned credential path flow
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR implements versioned Vault-backed DB credential rotation with in-container secret delivery via environment variables, adds config validation and strict server secret initialization (no dev fallback), normalizes presigned storage keys by stripping leading slashes, and updates DB schema, repo, tests, CI and docs to match. ChangesDatabase Credential Rotation & Password Management
Server Secret & Configuration Validation
Presigned URL Path Normalization
Sequence Diagram(s)sequenceDiagram
participant Service as DatabaseService
participant Repo as DatabaseRepo
participant Vault as Vault
participant Compute as ContainerExec
participant DB as Database
Service->>Repo: Get DB record (CredentialPath/version)
alt CredentialPath points to Vault
Service->>Vault: Read current credential (if needed)
end
Service->>Vault: Write new credential to versioned path (vN+1)
Vault-->>Service: Success (new versioned path)
Service->>Compute: Exec ALTER USER (sh -c) with PGPASSWORD / MYSQL_PWD env
Compute-->>Service: Command success
Service->>Repo: Update DB record with new CredentialPath and CredentialVersion
Service->>Vault: Delete old versioned credential path
Vault-->>Service: Cleanup success / failure (increments metric on failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple security issues related to secret handling and credential exposure across config loading, API key hashing, presigned URL signing, and database credential rotation workflows.
Changes:
- Require critical secrets (e.g.,
STORAGE_SECRET,POWERDNS_API_KEY) to be provided via environment and validate at startup. - Normalize presigned URL signing path construction to match verification behavior.
- Rework API key hashing secret requirements and adjust DB credential rotation to write to Vault before persisting DB metadata.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/crypto/presign.go | Normalizes object key path handling when generating presigned URLs. |
| internal/platform/config.go | Removes hardcoded secret fallbacks and validates required env vars on startup. |
| internal/platform/config_test.go | Updates config tests to set required env vars. |
| internal/core/services/identity.go | Enforces presence of SECRETS_ENCRYPTION_KEY (with a test-only fallback). |
| internal/core/services/identity_hash_test.go | Updates tests for the new server secret behavior. |
| internal/core/services/database.go | Adjusts credential rotation ordering and switches MySQL/Postgres exec commands away from -p<password>. |
| internal/core/services/database_vault_test.go | Updates Vault rotation tests to reflect new ordering/versioned path behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+566
to
+567
| // Use MYSQL_PWD env var to avoid password appearing in process list | ||
| cmd = []string{"sh", "-c", fmt.Sprintf("MYSQL_PWD='%s' mysql -u root --execute='STOP REPLICA; RESET REPLICA ALL;'", sqlStringLiteral(password))} |
Comment on lines
+970
to
+973
| // Use psql with password via env var (POSTGRES_PASSWORD_FILE not supported by psql) | ||
| // The password won't appear in /proc/cmdline since it's passed via env | ||
| stmt := fmt.Sprintf("ALTER USER %s WITH PASSWORD '%s';", postgresIdentifier(username), sqlStringLiteral(targetPassword)) | ||
| return []string{"sh", "-c", "POSTGRES_PASSWORD='" + sqlStringLiteral(targetPassword) + "' psql -h 127.0.0.1 -U " + username + " -d postgres -c '" + stmt + "'"} |
Comment on lines
+975
to
+977
| // Use mysql with password via MYSQL_PWD env var to avoid cmdline exposure | ||
| stmt := fmt.Sprintf("ALTER USER '%s'@'%%' IDENTIFIED BY '%s';", sqlStringLiteral(username), sqlStringLiteral(targetPassword)) | ||
| return []string{"sh", "-c", "MYSQL_PWD='" + sqlStringLiteral(authPassword) + "' mysql -u " + sqlStringLiteral(username) + " --execute='" + stmt + "'"} |
Comment on lines
+872
to
+873
| // Use versioned path to avoid split-brain: store new cred BEFORE updating DB | ||
| versionedPath := vaultPath + "/v2" |
| // The old path still has the old password for rollback if needed | ||
| db.CredentialPath = versionedPath | ||
| if err := s.repo.Update(ctx, db); err != nil { | ||
| return errors.Wrap(errors.Internal, "failed to update DB credential path", err) |
Comment on lines
+13
to
+19
| origPort := os.Getenv("PORT") | ||
| origSecret := os.Getenv("STORAGE_SECRET") | ||
| origDNSKey := os.Getenv("POWERDNS_API_KEY") | ||
| defer func() { | ||
| os.Setenv("PORT", origPort) | ||
| os.Setenv("STORAGE_SECRET", origSecret) | ||
| os.Setenv("POWERDNS_API_KEY", origDNSKey) |
Comment on lines
+30
to
+36
| // For tests, allow a fallback to avoid os.Exit in package init | ||
| if os.Getenv("TEST_SECRETS") != "" { | ||
| return os.Getenv("TEST_SECRETS") | ||
| } | ||
| slog.Default().Error("SECRETS_ENCRYPTION_KEY environment variable is required") | ||
| os.Exit(1) | ||
| return "" // unreachable but satisfies compiler |
Comment on lines
30
to
37
| defer func() { | ||
| if origVal != "" { | ||
| os.Setenv("SECRETS_ENCRYPTION_KEY", origVal) | ||
| if origSecrets != "" { | ||
| os.Setenv("SECRETS_ENCRYPTION_KEY", origSecrets) | ||
| } else { | ||
| os.Unsetenv("SECRETS_ENCRYPTION_KEY") | ||
| } | ||
| os.Setenv("TEST_SECRETS", origTestSecrets) | ||
| }() |
Comment on lines
+22
to
25
| // Clean path - strip leading slash to match VerifyURL behavior | ||
| cleanKey := strings.TrimPrefix(key, "/") | ||
| path := fmt.Sprintf("/storage/presigned/%s/%s", bucket, cleanKey) | ||
| u.Path = path |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/services/database.go (1)
956-959:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
sqlStringLiteralis being used as a shell escape on lines 567, 973, 977 — it isn't one.This helper escapes for SQL string literals (
'→''), which is correct inside the SQLALTER USER … PASSWORD '…'text. But it's also being interpolated into POSIX shell single-quoted segments, e.g."MYSQL_PWD='" + sqlStringLiteral(authPassword) + "'". In shell,'foo''bar'doesn't represent an embedded quote — it parses as the concatenation of the stringsfooandbar. So a password containing'ends up:
- corrupted in the env var fed to mysql/psql (auth fails), and
- in pathological inputs, capable of breaking out of the quoted region (close-quote, run something, reopen).
In the current code paths the password comes from
util.GenerateRandomPassword(random hex), so the bug is dormant — butcurrentPasswordfor rotation is read from Vault, which is externally writable, andusernameinterpolated raw on line 973 has no escaping at all.Recommended: add a real POSIX-shell quoter and use it for every value going inside
sh -c.🛡️ Sketch
// shellSingleQuote wraps s in POSIX single quotes, escaping any internal single quotes. func shellSingleQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" }…then use
shellSingleQuote(authPassword),shellSingleQuote(username),shellSingleQuote(stmt)(wherestmtitself was built withsqlStringLiteralfor the SQL-internal quoting). Same fix applies to the MySQL branch (line 977) and thePromoteToPrimaryMySQL command on line 567.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/services/database.go` around lines 956 - 959, The sqlStringLiteral helper only escapes SQL single quotes and is being misused inside shell single-quoted contexts (e.g., when building the sh -c payload with MYSQL_PWD='" + sqlStringLiteral(authPassword) + "'" and when interpolating username and stmt for psql/mysql commands), which corrupts passwords and allows shell injection; add a proper POSIX shell quoter (e.g., shellSingleQuote that wraps the string in '...' and escapes internal ' as '\''), and replace all places that inject values into sh -c with shellSingleQuote(authPassword), shellSingleQuote(username), and shellSingleQuote(stmt) (where stmt still uses sqlStringLiteral for SQL internal quoting); apply the same fix in the MySQL branch and in PromoteToPrimary usages so every value passed into sh -c is shell-quoted.
🧹 Nitpick comments (3)
internal/core/services/identity_hash_test.go (1)
26-52: 💤 Low valueOptional: use
t.Setenvto eliminate the manual save/restore.Since Go 1.17
t.Setenvautomatically reverts the env var (including unset → unset) at test cleanup, which would also flatten the asymmetric restore at line 36 (os.Setenv("TEST_SECRETS", origTestSecrets)re-creates the var as set-but-empty rather than unset, while lines 31-35 correctly distinguish unset).♻️ Sketch
func TestGetServerSecret(t *testing.T) { - // Save original values - origSecrets := os.Getenv("SECRETS_ENCRYPTION_KEY") - origTestSecrets := os.Getenv("TEST_SECRETS") - defer func() { - if origSecrets != "" { - os.Setenv("SECRETS_ENCRYPTION_KEY", origSecrets) - } else { - os.Unsetenv("SECRETS_ENCRYPTION_KEY") - } - os.Setenv("TEST_SECRETS", origTestSecrets) - }() - t.Run("WithEnvVar", func(t *testing.T) { - os.Setenv("SECRETS_ENCRYPTION_KEY", "test-secret-key") - os.Unsetenv("TEST_SECRETS") + t.Setenv("SECRETS_ENCRYPTION_KEY", "test-secret-key") + os.Unsetenv("TEST_SECRETS") // t.Setenv has no "unset" form; the parent test still restores secret := getServerSecret() assert.Equal(t, "test-secret-key", secret) }) t.Run("WithoutEnvVar_UsesTestFallback", func(t *testing.T) { os.Unsetenv("SECRETS_ENCRYPTION_KEY") - os.Setenv("TEST_SECRETS", "test-only-secret") + t.Setenv("TEST_SECRETS", "test-only-secret") secret := getServerSecret() assert.Equal(t, "test-only-secret", secret) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/services/identity_hash_test.go` around lines 26 - 52, Replace the manual save/restore of environment variables in TestGetServerSecret with t.Setenv to ensure proper cleanup; specifically, remove the origSecrets/origTestSecrets variables and the defer block and in each subtest call t.Setenv("SECRETS_ENCRYPTION_KEY", "...") or t.Setenv("TEST_SECRETS", "...") (or t.Setenv with empty string to unset) before calling getServerSecret(), so the test functions (TestGetServerSecret and its t.Run subtests) rely on t.Setenv for automatic revert and avoid the asymmetric restore that currently re-creates TEST_SECRETS as set-but-empty.internal/core/services/database_vault_test.go (1)
74-122: ⚡ Quick winTests mirror the new sequence — but consider asserting the
Compute.Execargv.Right now the success test passes any
mock.Anythingfor the command, so it can't catch the kind of bug the changes inbuildPasswordChangeCmdintroduce (e.g., the wrong env-var name / wrong password being interpolated for the postgres branch — see comment ondatabase.goline 973). Amock.MatchedBychecking that the third argv element referencesPGPASSWORD=<oldPass>andALTER USERwould have caught that regression at unit-test level.Also: once the hardcoded
/v2suffix indoRotateCredentialsis replaced with a real versioning scheme,versionedPath := db.CredentialPath + "/v2"here will need to change in lockstep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/services/database_vault_test.go` around lines 74 - 122, The test allows any command for mockCompute.Exec so it won't catch regressions in buildPasswordChangeCmd; update the RotateCredentials_Success test to use mock.MatchedBy for the Exec call (mockCompute.On("Exec", mock.Anything, db.ContainerID, mock.MatchedBy(...))) and assert the third argv string contains the expected environment var and SQL (e.g., "PGPASSWORD=old-pass" and "ALTER USER" for postgres flow), referencing buildPasswordChangeCmd / doRotateCredentials / RotateCredentials so the Exec arguments are validated; also remember to adjust the test's versionedPath construction when doRotateCredentials' hardcoded "/v2" is replaced.internal/platform/config.go (1)
119-126: 💤 Low valueNit: log-and-return causes double logging.
slog.Error(...)plusreturn fmt.Errorf(...)will normally log the same string twice (once here, once at the top-level error site inmain). Either log here and return a sentinel error, or just return the error and let the caller log. Not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/config.go` around lines 119 - 126, The code currently calls slog.Error and also returns fmt.Errorf for missing config values (cfg.StorageSecret and cfg.PowerDNSAPIKey), causing duplicate logs; remove the inline slog.Error calls and just return the error from the validation block (e.g., return fmt.Errorf("STORAGE_SECRET environment variable is required")) so the caller can log once, or alternatively keep slog.Error and return a sentinel error value — update the checks around cfg.StorageSecret and cfg.PowerDNSAPIKey in the config validation function accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/services/database.go`:
- Around line 894-906: The current sequence can cause split-brain if ALTER
succeeds but s.repo.Update(ctx, db) fails because db.CredentialPath still points
at vaultPath containing the old password; fix by writing the new password to
both the versioned path and the current vault path before performing the engine
ALTER: call s.secrets.StoreSecret(ctx, versionedPath, {"password": newPassword})
and s.secrets.StoreSecret(ctx, vaultPath, {"password": newPassword"}) prior to
executing the ALTER step, then proceed with ALTER and s.repo.Update as before
(keep db.CredentialPath = versionedPath and repo.Update after ALTER); this
ensures callers reading vaultPath continue to get a valid password even if
repo.Update fails.
- Around line 866-876: The current code blindly appends "/v2" to
db.CredentialPath (versionedPath := vaultPath + "/v2"), which causes repeated
"/v2" nesting on subsequent rotations; change this to compute a stable base path
and produce a true new versioned path (e.g. derive basePath by stripping any
existing version suffix from db.CredentialPath or by using s.getVaultPath(db.ID)
as the canonical base, then increment a db.CredentialVersion counter or use a
timestamp to create versionedPath := basePath + "/v{N}" or basePath +
"/{timestamp}"), store the secret via s.secrets.StoreSecret at that new
versionedPath, and update db.CredentialVersion and db.CredentialPath accordingly
so future rotations use the base + new version rather than appending "/v2"
repeatedly; update any subsequent logic that writes the "old path" to reference
the previous versionedPath variable rather than manipulating db.CredentialPath
directly.
- Around line 566-567: The Exec API currently exposes secrets by requiring
callers to embed env vars into a shell command string; change the
ComputeBackend.Exec signature to accept explicit environment variables (e.g.,
Exec(ctx context.Context, id string, cmd []string, env map[string]string) or an
options struct that carries Env) and update every backend implementation to use
the new signature; then replace uses that build shell commands with embedded
MYSQL_PWD/POSTGRES_PASSWORD (notably the MySQL promotion path that constructs
cmd = []string{"sh","-c", fmt.Sprintf("MYSQL_PWD='%s' mysql ...", ...)} and the
callers buildPasswordChangeCmd) so they pass the password in the Env map and
invoke mysql/psql directly (no sh -c or inline password), and update all callers
and tests accordingly.
- Around line 967-980: In buildPasswordChangeCmd, the Postgres branch currently
sets POSTGRES_PASSWORD to the new password (targetPassword) and uses
sqlStringLiteral (a SQL escaper) for shell interpolation; change it to export
PGPASSWORD with the current authPassword (PGPASSWORD is what libpq/psql reads)
and ensure all interpolated values (PGPASSWORD value, -U username, and the SQL
statement) are safely shell-quoted using a POSIX shell quoting routine (not
sqlStringLiteral) so the env and command are passed correctly; update the
returned []string for domain.EnginePostgres to use PGPASSWORD=authPassword and
properly shell-escape username and stmt.
In `@internal/core/services/identity.go`:
- Around line 30-33: The TEST_SECRETS fallback in the package init (the branch
checking os.Getenv("TEST_SECRETS")) is too permissive for production; change the
guard so it only triggers under real test runs (e.g., use testing.Testing() from
the testing package on Go 1.21+ or detect os.Args[0] ending with ".test") before
returning os.Getenv("TEST_SECRETS"), so only true test processes can use
TEST_SECRETS; update the init/secret-loading code that references TEST_SECRETS
accordingly and remove or tighten this fallback if you later remove the global
initialization pattern.
- Around line 23-37: Remove the package-level var serverSecret and the
getServerSecret function; instead validate presence of the secrets encryption
key in validateConfig and pass it into IdentityService via a new serverSecret
field on IdentityService and through IdentityServiceParams; convert
computeKeyHash into a method on IdentityService that uses s.serverSecret; ensure
the IdentityService constructor returns an error if the secret is missing (no
os.Exit) and delete the TEST_SECRETS fallback so tests create IdentityService
with test secrets via IdentityServiceParams.
In `@internal/platform/config.go`:
- Around line 117-128: The config validator validateConfig currently only checks
StorageSecret and PowerDNSAPIKey; add a check that the secrets encryption key is
present (e.g., verify cfg.SecretsEncryptionKey or the field that maps to
SECRETS_ENCRYPTION_KEY is non-empty) and return a descriptive error (and log via
slog.Error) when missing so NewConfig's caller can fail startup cleanly; after
this change remove the package-init os.Exit/ global enforcement in
internal/core/services/identity.go so identity initialization no longer exits on
missing SECRETS_ENCRYPTION_KEY.
---
Outside diff comments:
In `@internal/core/services/database.go`:
- Around line 956-959: The sqlStringLiteral helper only escapes SQL single
quotes and is being misused inside shell single-quoted contexts (e.g., when
building the sh -c payload with MYSQL_PWD='" + sqlStringLiteral(authPassword) +
"'" and when interpolating username and stmt for psql/mysql commands), which
corrupts passwords and allows shell injection; add a proper POSIX shell quoter
(e.g., shellSingleQuote that wraps the string in '...' and escapes internal ' as
'\''), and replace all places that inject values into sh -c with
shellSingleQuote(authPassword), shellSingleQuote(username), and
shellSingleQuote(stmt) (where stmt still uses sqlStringLiteral for SQL internal
quoting); apply the same fix in the MySQL branch and in PromoteToPrimary usages
so every value passed into sh -c is shell-quoted.
---
Nitpick comments:
In `@internal/core/services/database_vault_test.go`:
- Around line 74-122: The test allows any command for mockCompute.Exec so it
won't catch regressions in buildPasswordChangeCmd; update the
RotateCredentials_Success test to use mock.MatchedBy for the Exec call
(mockCompute.On("Exec", mock.Anything, db.ContainerID, mock.MatchedBy(...))) and
assert the third argv string contains the expected environment var and SQL
(e.g., "PGPASSWORD=old-pass" and "ALTER USER" for postgres flow), referencing
buildPasswordChangeCmd / doRotateCredentials / RotateCredentials so the Exec
arguments are validated; also remember to adjust the test's versionedPath
construction when doRotateCredentials' hardcoded "/v2" is replaced.
In `@internal/core/services/identity_hash_test.go`:
- Around line 26-52: Replace the manual save/restore of environment variables in
TestGetServerSecret with t.Setenv to ensure proper cleanup; specifically, remove
the origSecrets/origTestSecrets variables and the defer block and in each
subtest call t.Setenv("SECRETS_ENCRYPTION_KEY", "...") or
t.Setenv("TEST_SECRETS", "...") (or t.Setenv with empty string to unset) before
calling getServerSecret(), so the test functions (TestGetServerSecret and its
t.Run subtests) rely on t.Setenv for automatic revert and avoid the asymmetric
restore that currently re-creates TEST_SECRETS as set-but-empty.
In `@internal/platform/config.go`:
- Around line 119-126: The code currently calls slog.Error and also returns
fmt.Errorf for missing config values (cfg.StorageSecret and cfg.PowerDNSAPIKey),
causing duplicate logs; remove the inline slog.Error calls and just return the
error from the validation block (e.g., return fmt.Errorf("STORAGE_SECRET
environment variable is required")) so the caller can log once, or alternatively
keep slog.Error and return a sentinel error value — update the checks around
cfg.StorageSecret and cfg.PowerDNSAPIKey in the config validation function
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b64501d6-017c-4cf4-a17a-573afdb0bc53
📒 Files selected for processing (7)
internal/core/services/database.gointernal/core/services/database_vault_test.gointernal/core/services/identity.gointernal/core/services/identity_hash_test.gointernal/platform/config.gointernal/platform/config_test.gopkg/crypto/presign.go
Comment on lines
+566
to
+567
| // Use MYSQL_PWD env var to avoid password appearing in process list | ||
| cmd = []string{"sh", "-c", fmt.Sprintf("MYSQL_PWD='%s' mysql -u root --execute='STOP REPLICA; RESET REPLICA ALL;'", sqlStringLiteral(password))} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the ComputeBackend Exec signature so we know whether env/stdin are already plumbable.
ast-grep --pattern 'type ComputeBackend interface {
$$$
}'
rg -nP '\bExec\s*\(' --type=go -C2 -g 'internal/core/ports/**'
rg -nP '\bfunc\s+\([^)]+\)\s+Exec\s*\(' --type=go -C2Repository: poyrazK/thecloud
Length of output: 17019
🏁 Script executed:
# First, let's read the database.go file to see lines around 566-567
sed -n '560,575p' internal/core/services/database.goRepository: poyrazK/thecloud
Length of output: 637
🏁 Script executed:
# Also get the buildPasswordChangeCmd implementations around lines 973 and 977
sed -n '970,985p' internal/core/services/database.goRepository: poyrazK/thecloud
Length of output: 1229
🏁 Script executed:
# Let's see the full function context for both issues
sed -n '550,580p' internal/core/services/database.go | head -40
sed -n '960,990p' internal/core/services/database.goRepository: poyrazK/thecloud
Length of output: 2822
🏁 Script executed:
# Find sqlStringLiteral implementation
rg -n "func sqlStringLiteral" internal/core/services/database.go -A 10Repository: poyrazK/thecloud
Length of output: 432
🏁 Script executed:
# Check Docker adapter Exec implementation to understand the execution context
sed -n '1007,1030p' internal/repositories/docker/adapter.goRepository: poyrazK/thecloud
Length of output: 790
🏁 Script executed:
# Understand the container execution context - are passwords visible to other processes?
rg -n "ExecOptions\|docker client" internal/repositories/docker/adapter.go | head -20Repository: poyrazK/thecloud
Length of output: 42
sh -c command string still exposes password in process argv — fix requires ComputeBackend.Exec API changes.
The review comment is correct. Using sh -c with embedded MYSQL_PWD='<password>' still exposes the full command string (including password) in /proc/<pid>/cmdline for as long as the shell process runs, even though the intent was to avoid process list exposure. Any sibling process in the same container PID namespace can read this.
This affects:
- Lines 566–567: promotion path for MySQL replication
- Line 973: PostgreSQL password change via
buildPasswordChangeCmd - Line 977: MySQL password change via
buildPasswordChangeCmd
The comments claiming the password "won't appear in /proc/cmdline" (lines 567, 973) are incorrect.
To fix: Extend ComputeBackend.Exec interface to accept environment variables (or stdin), then pass MYSQL_PWD and POSTGRES_PASSWORD as env map entries rather than shell command strings. Current signature Exec(ctx context.Context, id string, cmd []string) (string, error) provides no mechanism for this.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/services/database.go` around lines 566 - 567, The Exec API
currently exposes secrets by requiring callers to embed env vars into a shell
command string; change the ComputeBackend.Exec signature to accept explicit
environment variables (e.g., Exec(ctx context.Context, id string, cmd []string,
env map[string]string) or an options struct that carries Env) and update every
backend implementation to use the new signature; then replace uses that build
shell commands with embedded MYSQL_PWD/POSTGRES_PASSWORD (notably the MySQL
promotion path that constructs cmd = []string{"sh","-c",
fmt.Sprintf("MYSQL_PWD='%s' mysql ...", ...)} and the callers
buildPasswordChangeCmd) so they pass the password in the Env map and invoke
mysql/psql directly (no sh -c or inline password), and update all callers and
tests accordingly.
Comment on lines
23
to
37
| var serverSecret = getServerSecret() | ||
|
|
||
| func getServerSecret() string { | ||
| // Use the secrets encryption key if available, otherwise fall back to a warning string | ||
| // that will be rejected in production | ||
| secret := platform.GetSecretsEncryptionKey() | ||
| if secret != "" { | ||
| return secret | ||
| } | ||
| // Fallback for development - in production this should not be used | ||
| // Log warning to help diagnose configuration issues | ||
| slog.Default().Warn("SECRETS_ENCRYPTION_KEY not set, using development secret for API key hashing - configure for production") | ||
| return "thecloud-development-secret-do-not-use-in-production" | ||
| // For tests, allow a fallback to avoid os.Exit in package init | ||
| if os.Getenv("TEST_SECRETS") != "" { | ||
| return os.Getenv("TEST_SECRETS") | ||
| } | ||
| slog.Default().Error("SECRETS_ENCRYPTION_KEY environment variable is required") | ||
| os.Exit(1) | ||
| return "" // unreachable but satisfies compiler | ||
| } |
There was a problem hiding this comment.
Avoid os.Exit from a package-level initializer; inject the secret instead.
var serverSecret = getServerSecret() runs at package import, so os.Exit(1) here turns a missing env var into an opaque process termination during init for anything that imports internal/core/services (CLIs, migration tools, sibling-package tests, etc.). Per the repo guidelines this is also a "no panic / no global / use constructor injection" violation.
Recommended shape:
- Enforce
SECRETS_ENCRYPTION_KEYonce ininternal/platform/config.go'svalidateConfig(whereSTORAGE_SECRETandPOWERDNS_API_KEYare already enforced). - Make the secret a field on
IdentityServiceand pass it viaIdentityServiceParams. ThencomputeKeyHashbecomes a method onIdentityServiceusings.serverSecret. - Drop the
TEST_SECRETSbranch — tests can constructIdentityServicewith whatever secret they want via the params struct.
That removes the global, removes the os.Exit, removes a production-active backdoor (TEST_SECRETS), and centralises the missing-key error in one place.
As per coding guidelines: "Do not panic in production code - return errors instead", "Do not use global variables", and "Use constructor injection for dependencies instead of global initialization".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/services/identity.go` around lines 23 - 37, Remove the
package-level var serverSecret and the getServerSecret function; instead
validate presence of the secrets encryption key in validateConfig and pass it
into IdentityService via a new serverSecret field on IdentityService and through
IdentityServiceParams; convert computeKeyHash into a method on IdentityService
that uses s.serverSecret; ensure the IdentityService constructor returns an
error if the secret is missing (no os.Exit) and delete the TEST_SECRETS fallback
so tests create IdentityService with test secrets via IdentityServiceParams.
Comment on lines
+30
to
+33
| // For tests, allow a fallback to avoid os.Exit in package init | ||
| if os.Getenv("TEST_SECRETS") != "" { | ||
| return os.Getenv("TEST_SECRETS") | ||
| } |
There was a problem hiding this comment.
TEST_SECRETS is a production-active bypass.
This branch isn't gated by build tag, env name, or test detection — it's just another env var that, when set in a real deployment, silently satisfies the missing-SECRETS_ENCRYPTION_KEY check. That undermines the very guarantee this PR is trying to add.
If you keep the package-init pattern in the short term, please at minimum gate the fallback on something only present in tests (e.g., testing.Testing() from Go 1.21+, or strings.HasSuffix(os.Args[0], ".test")) so a misconfigured production env can't trip it. Removing the global (see other comment) eliminates the need for it entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/services/identity.go` around lines 30 - 33, The TEST_SECRETS
fallback in the package init (the branch checking os.Getenv("TEST_SECRETS")) is
too permissive for production; change the guard so it only triggers under real
test runs (e.g., use testing.Testing() from the testing package on Go 1.21+ or
detect os.Args[0] ending with ".test") before returning
os.Getenv("TEST_SECRETS"), so only true test processes can use TEST_SECRETS;
update the init/secret-loading code that references TEST_SECRETS accordingly and
remove or tighten this fallback if you later remove the global initialization
pattern.
Comment on lines
+117
to
+128
| // validateConfig ensures required configuration values are present. | ||
| func validateConfig(cfg *Config) error { | ||
| if cfg.StorageSecret == "" { | ||
| slog.Error("STORAGE_SECRET environment variable is required") | ||
| return fmt.Errorf("STORAGE_SECRET environment variable is required") | ||
| } | ||
| if cfg.PowerDNSAPIKey == "" { | ||
| slog.Error("POWERDNS_API_KEY environment variable is required") | ||
| return fmt.Errorf("POWERDNS_API_KEY environment variable is required") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Validate SECRETS_ENCRYPTION_KEY here too.
The PR objective says "Fail startup if SECRETS_ENCRYPTION_KEY is not set (remove weak fallback)", but this validator only enforces STORAGE_SECRET and POWERDNS_API_KEY. Today the enforcement lives in internal/core/services/identity.go as an os.Exit(1) during package init, which is the wrong layer (see identity.go comment).
Adding the check here lets NewConfig's caller (main) report the misconfiguration cleanly and lets identity.go drop the global + os.Exit.
🛡️ Suggested fix
func validateConfig(cfg *Config) error {
if cfg.StorageSecret == "" {
slog.Error("STORAGE_SECRET environment variable is required")
return fmt.Errorf("STORAGE_SECRET environment variable is required")
}
if cfg.PowerDNSAPIKey == "" {
slog.Error("POWERDNS_API_KEY environment variable is required")
return fmt.Errorf("POWERDNS_API_KEY environment variable is required")
}
+ if cfg.SecretsEncryptionKey == "" {
+ return fmt.Errorf("SECRETS_ENCRYPTION_KEY environment variable is required")
+ }
return nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/platform/config.go` around lines 117 - 128, The config validator
validateConfig currently only checks StorageSecret and PowerDNSAPIKey; add a
check that the secrets encryption key is present (e.g., verify
cfg.SecretsEncryptionKey or the field that maps to SECRETS_ENCRYPTION_KEY is
non-empty) and return a descriptive error (and log via slog.Error) when missing
so NewConfig's caller can fail startup cleanly; after this change remove the
package-init os.Exit/ global enforcement in internal/core/services/identity.go
so identity initialization no longer exits on missing SECRETS_ENCRYPTION_KEY.
…ation - Remove redundant slog.Error calls in validateConfig (errors returned to caller) - Remove unused log/slog import from platform/config.go - Add test case for POWERDNS_API_KEY validation failure - Add clarifying comments about env var visibility tradeoff in buildPasswordChangeCmd - Note: credential version cleanup deferred (requires DB schema change)
Adds STORAGE_SECRET and POWERDNS_API_KEY to both the Setup Infrastructure and Start API steps in the E2E workflow. These were previously required by config validation but missing from CI, causing test failures. Also removes the fallback default for POWERDNS_API_KEY in docker-compose.yml so that missing env vars surface as errors rather than silently using an insecure default.
Track rotation count in database record. Each rotation stores at
versioned Vault path (v{count}) and cleans up the previous version
after successful rotation.
- Add credential_version column to databases table
- Increment version on each rotation and compute v{n} path
- Delete previous versioned path from Vault after successful rotation
- Update repository queries and unit tests for new field
Add thecloud_credential_cleanup_failures_total Prometheus counter to track when old credential version cleanup fails in Vault. This allows ops to monitor and alert if cleanup is consistently failing. Incremented in doRotateCredentials when DeleteSecret fails.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+566
to
+567
| // Use MYSQL_PWD env var to avoid password appearing in process list | ||
| cmd = []string{"sh", "-c", fmt.Sprintf("MYSQL_PWD='%s' mysql -u root --execute='STOP REPLICA; RESET REPLICA ALL;'", sqlStringLiteral(password))} |
Comment on lines
867
to
+900
| @@ -880,27 +893,20 @@ func (s *DatabaseService) doRotateCredentials(ctx context.Context, id uuid.UUID, | |||
| return errors.Wrap(errors.Internal, "failed to execute password rotation in container", execErr) | |||
| } | |||
|
|
|||
| // 2. Update in Vault ONLY after DB success | |||
| vaultPath := db.CredentialPath | |||
| if vaultPath == "" { | |||
| vaultPath = s.getVaultPath(db.ID) | |||
| } | |||
| if err := s.secrets.StoreSecret(ctx, vaultPath, map[string]interface{}{"password": newPassword}); err != nil { | |||
| // Vault store failed but DB already has new password - rollback to original | |||
| rollbackCmd := s.buildPasswordChangeCmd(db.Engine, db.Username, currentPassword, newPassword) | |||
| if _, rollbackErr := s.compute.Exec(ctx, db.ContainerID, rollbackCmd); rollbackErr != nil { | |||
| // Rollback also failed - system is in critical state requiring manual intervention | |||
| return errors.Wrap(errors.Internal, | |||
| fmt.Sprintf("credential rotation failed and rollback also failed - manual intervention required (vault store error: %v)", err), | |||
| rollbackErr) | |||
| } | |||
| return errors.Wrap(errors.Internal, "vault store failed, DB password rolled back", err) | |||
| // 3. Update DB record to point to new versioned path (ONLY after DB confirmed updated) | |||
| // The old path still has the old password for rollback if needed | |||
| db.CredentialPath = versionedPath | |||
| db.CredentialVersion = newVersion | |||
| if err := s.repo.Update(ctx, db); err != nil { | |||
Comment on lines
973
to
980
| case domain.EnginePostgres: | ||
| return []string{"psql", "-h", "127.0.0.1", "-U", username, "-d", "postgres", "-c", | ||
| fmt.Sprintf("ALTER USER %s WITH PASSWORD '%s';", postgresIdentifier(username), sqlStringLiteral(targetPassword))} | ||
| // Use psql with password via env var (POSTGRES_PASSWORD_FILE not supported by psql). | ||
| // Note: While env vars avoid cmdline exposure, the password is still visible to | ||
| // root users via /proc/$pid/environ. This is a known tradeoff - the alternative | ||
| // (stdin password) requires more complex interaction patterns with psql. | ||
| stmt := fmt.Sprintf("ALTER USER %s WITH PASSWORD '%s';", postgresIdentifier(username), sqlStringLiteral(targetPassword)) | ||
| return []string{"sh", "-c", "POSTGRES_PASSWORD='" + sqlStringLiteral(targetPassword) + "' psql -h 127.0.0.1 -U " + username + " -d postgres -c '" + stmt + "'"} | ||
| case domain.EngineMySQL: |
Comment on lines
27
to
34
| func (r *DatabaseRepository) Create(ctx context.Context, db *domain.Database) error { | ||
| query := ` | ||
| INSERT INTO databases (id, user_id, tenant_id, name, engine, version, status, role, primary_id, vpc_id, container_id, port, username, password, created_at, updated_at, allocated_storage, parameters, metrics_enabled, metrics_port, exporter_container_id, pooling_enabled, pooling_port, pooler_container_id, credential_path) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25) | ||
| INSERT INTO databases (id, user_id, tenant_id, name, engine, version, status, role, primary_id, vpc_id, container_id, port, username, password, created_at, updated_at, allocated_storage, parameters, metrics_enabled, metrics_port, exporter_container_id, pooling_enabled, pooling_port, pooler_container_id, credential_path, credential_version) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23, $24, $25, $26) | ||
| ` | ||
| _, err := r.db.Exec(ctx, query, | ||
| db.ID, db.UserID, db.TenantID, db.Name, db.Engine, db.Version, db.Status, db.Role, db.PrimaryID, db.VpcID, db.ContainerID, db.Port, db.Username, db.Password, db.CreatedAt, db.UpdatedAt, db.AllocatedStorage, db.Parameters, db.MetricsEnabled, db.MetricsPort, db.ExporterContainerID, db.PoolingEnabled, db.PoolingPort, db.PoolerContainerID, db.CredentialPath, | ||
| db.ID, db.UserID, db.TenantID, db.Name, db.Engine, db.Version, db.Status, db.Role, db.PrimaryID, db.VpcID, db.ContainerID, db.Port, db.Username, db.Password, db.CreatedAt, db.UpdatedAt, db.AllocatedStorage, db.Parameters, db.MetricsEnabled, db.MetricsPort, db.ExporterContainerID, db.PoolingEnabled, db.PoolingPort, db.PoolerContainerID, db.CredentialPath, db.CredentialVersion, | ||
| ) |
Comment on lines
+1
to
+7
| -- +goose Up | ||
|
|
||
| ALTER TABLE databases DROP COLUMN credential_version; | ||
|
|
||
| -- +goose Down | ||
|
|
||
| ALTER TABLE databases ADD COLUMN credential_version INT DEFAULT 1 NOT NULL; |
Comment on lines
22
to
+36
| // serverSecret is used as HMAC key to prevent rainbow table attacks on API key hashes. | ||
| // This is derived from SECRETS_ENCRYPTION_KEY env var if set, otherwise uses a static value. | ||
| // In production, set SECRETS_ENCRYPTION_KEY for proper security. | ||
| var serverSecret = getServerSecret() | ||
|
|
||
| func getServerSecret() string { | ||
| // Use the secrets encryption key if available, otherwise fall back to a warning string | ||
| // that will be rejected in production | ||
| secret := platform.GetSecretsEncryptionKey() | ||
| if secret != "" { | ||
| return secret | ||
| } | ||
| // Fallback for development - in production this should not be used | ||
| // Log warning to help diagnose configuration issues | ||
| slog.Default().Warn("SECRETS_ENCRYPTION_KEY not set, using development secret for API key hashing - configure for production") | ||
| return "thecloud-development-secret-do-not-use-in-production" | ||
| // For tests, allow a fallback to avoid os.Exit in package init | ||
| if os.Getenv("TEST_SECRETS") != "" { | ||
| return os.Getenv("TEST_SECRETS") | ||
| } | ||
| slog.Default().Error("SECRETS_ENCRYPTION_KEY environment variable is required") | ||
| os.Exit(1) | ||
| return "" // unreachable but satisfies compiler |
Comment on lines
22
to
+46
| t.Run("Default values", func(t *testing.T) { | ||
| err := os.Unsetenv("PORT") | ||
| require.NoError(t, err) | ||
| os.Unsetenv("PORT") | ||
| os.Setenv("STORAGE_SECRET", "test-secret") | ||
| os.Setenv("POWERDNS_API_KEY", "test-dns-key") | ||
| cfg, err := NewConfig() | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "8080", cfg.Port) | ||
| }) | ||
|
|
||
| t.Run("Env override", func(t *testing.T) { | ||
| err := os.Setenv("PORT", "9090") | ||
| require.NoError(t, err) | ||
| os.Setenv("PORT", "9090") | ||
| os.Setenv("STORAGE_SECRET", "test-secret") | ||
| os.Setenv("POWERDNS_API_KEY", "test-dns-key") | ||
| cfg, err := NewConfig() | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "9090", cfg.Port) | ||
| }) | ||
|
|
||
| t.Run("Missing POWERDNS_API_KEY fails", func(t *testing.T) { | ||
| os.Setenv("STORAGE_SECRET", "test-secret") | ||
| os.Unsetenv("POWERDNS_API_KEY") | ||
| _, err := NewConfig() | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "POWERDNS_API_KEY") | ||
| }) |
Comment on lines
+118
to
143
| t.Run("RotateCredentials_VaultFailure_WithoutDBChange", func(t *testing.T) { | ||
| // Create a fresh db for this test | ||
| testDB := &domain.Database{ | ||
| ID: db.ID, | ||
| UserID: db.UserID, | ||
| Name: db.Name, | ||
| Engine: db.Engine, | ||
| Username: db.Username, | ||
| ContainerID: db.ContainerID, | ||
| CredentialPath: "secret/rds/" + dbID.String() + "/credentials", | ||
| CredentialVersion: 1, | ||
| } | ||
| mockRepo.On("GetByID", mock.Anything, dbID).Return(testDB, nil).Once() | ||
| mockSecrets.On("GetSecret", mock.Anything, testDB.CredentialPath).Return(map[string]interface{}{"password": "old-pass"}, nil).Once() | ||
| // Vault store fails at step 1 (versioned path) - DB is NOT changed | ||
| versionedPath := testDB.CredentialPath + "/v2" | ||
| mockSecrets.On("StoreSecret", mock.Anything, versionedPath, mock.Anything).Return(fmt.Errorf("vault error")).Once() | ||
|
|
||
| err := svc.RotateCredentials(ctx, dbID, "") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "vault store failed, DB password rolled back") | ||
| assert.Contains(t, err.Error(), "failed to store new credential in vault") | ||
|
|
||
| mockSecrets.AssertExpectations(t) | ||
| // Compute.Exec should NOT be called since Vault failed before DB change | ||
| mockCompute.AssertExpectations(t) | ||
| mockRepo.AssertExpectations(t) | ||
| }) |
Comment on lines
+116
to
+124
| // validateConfig ensures required configuration values are present. | ||
| func validateConfig(cfg *Config) error { | ||
| if cfg.StorageSecret == "" { | ||
| return fmt.Errorf("STORAGE_SECRET environment variable is required") | ||
| } | ||
| if cfg.PowerDNSAPIKey == "" { | ||
| return fmt.Errorf("POWERDNS_API_KEY environment variable is required") | ||
| } | ||
| return nil |
Comment on lines
+872
to
+877
| // Increment version and compute new versioned path | ||
| newVersion := db.CredentialVersion + 1 | ||
| versionedPath := vaultPath + "/v" + strconv.Itoa(newVersion) | ||
| if err := s.secrets.StoreSecret(ctx, versionedPath, map[string]interface{}{"password": newPassword}); err != nil { | ||
| return errors.Wrap(errors.Internal, "failed to store new credential in vault", err) | ||
| } |
SECRETS_ENCRYPTION_KEY, STORAGE_SECRET, and POWERDNS_API_KEY are now required at startup. Adding them to workflow-level env so they apply to all jobs automatically.
Previously used POSTGRES_PASSWORD (wrong env var) and targetPassword (wrong - it was setting the new password instead of using current password for authentication). The psql client uses PGPASSWORD (libpq standard) not POSTGRES_PASSWORD.
All other .down.sql files in the migrations directory contain only the raw down SQL without goose markers. The 108 migration was created with both -- +goose Up/Down markers like a .up.sql file, causing the migration_test.go runFile parser to extract the wrong section when running down migrations (ADD instead of DROP). Also adds IF EXISTS for safer column dropping.
The benchmark tests initialize services that require SECRETS_ENCRYPTION_KEY, STORAGE_SECRET, and POWERDNS_API_KEY. Without these, go test exits with status 1 and no output, causing bench.txt to be empty and the benchmark-action to fail with "No benchmark result found".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix 7 security issues across 5 files involving credential exposure, weak hashing, hardcoded secrets, and presigned URL vulnerabilities:
storage-secret-keyfallback, requireSTORAGE_SECRETenv varSECRETS_ENCRYPTION_KEYnot set instead of using weak fallbackMYSQL_PWDenv var instead of-pflag exposing password in CLIPGPASSWORDenv var instead of passing password in process argsTest plan
go build ./...- clean compileRelated Issues
Summary by CodeRabbit
Security Improvements
Configuration
Bug Fixes
Other