Conversation
📝 WalkthroughWalkthroughAdds a pluggable secrets package with AES‑256 GCM and noop implementations, encrypts JobAgent apiKey values before storage, decrypts apiKeys on use (with plaintext fallback), and adds an e2e test validating apiKey encryption. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Store as JobAgents Store
participant Enc as Encryption
participant DB as Database
participant Argo as ArgoApplication
Client->>Store: Upsert JobAgent (config includes plaintext apiKey)
Store->>Enc: Encrypt(apiKey)
Enc-->>Store: encrypted(apiKey) [base64]
Store->>DB: Store JobAgent (encrypted apiKey)
DB-->>Store: OK
Client->>Argo: Parse JobAgent config / request action
Argo->>Enc: Decrypt(encrypted apiKey) or return plaintext
Enc-->>Argo: plaintext apiKey (or error -> fallback)
Argo-->>Client: Use apiKey for agent operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
| } | ||
|
|
||
| func NewEncryption() Encryption { | ||
| keyStr := os.Getenv("CTRLPLANE_AES_256_KEY") |
There was a problem hiding this comment.
there is a globals config object
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/secrets/secrets.go`:
- Around line 23-49: In NewEncryption, the log.Error calls inside the error
branches for aes.NewCipher and cipher.NewGCM use printf-style "%w" instead of
structured key/value pairs; change the two calls in NewEncryption that read
log.Error("failed to create cipher: %w", err) and log.Error("failed to create
GCM: %w", err) to structured form like log.Error("failed to create cipher",
"error", err) and log.Error("failed to create GCM", "error", err) so they match
the codebase pattern (references: NewEncryption, AES256Encryption,
NoopEncryption).
In `@apps/workspace-engine/pkg/workspace/store/job_agents.go`:
- Around line 41-49: The Upsert method on JobAgents currently swallows
encryption errors and silently skips the upsert; change JobAgents.Upsert so it
returns an error instead of returning early: call j.encryptCredentials(jobAgent)
and if it returns an error propagate that error to the caller (do not just log
and return), perform j.repo.JobAgents.Set(jobAgent.Id, jobAgent) and
j.store.changeset.RecordUpsert(jobAgent) only on success, and update any callers
to handle the returned error (including tests) so they can retry/alert as
appropriate; reference symbols: JobAgents.Upsert, encryptCredentials,
j.repo.JobAgents.Set, j.store.changeset.RecordUpsert.
- Around line 27-39: In JobAgents.encryptCredentials, avoid the unsafe type
assertion v.(string) when reading jobAgent.Config["apiKey"]; instead use the
comma-ok idiom to check the value and its type (e.g., val, ok := v.(string)) and
handle the non-string or nil case by returning a clear error (or skipping
encryption) before calling j.secrets.Encrypt; update the logic that assigns
jobAgentConfig[k] to use the encrypted string only when the cast succeeds.
- Around line 27-39: The encryptCredentials method in JobAgents blindly
re-encrypts jobAgent.Config["apiKey"] which causes double-encryption when an
already-encrypted JobAgent is re-upserted; update encryptCredentials to first
detect and handle already-encrypted values by either attempting to decrypt via
j.secrets.Decrypt and using the decrypted value (then re-encrypt once), or by
checking/adding a stable prefix/marker (e.g., "enc:") around values produced by
j.secrets.Encrypt and skipping encryption if the marker is present; ensure you
reference and use j.secrets.Encrypt and j.secrets.Decrypt (or add the marker
logic) inside JobAgents.encryptCredentials so Upsert/Get/HandleJobAgentUpdated
flow is idempotent.
🧹 Nitpick comments (5)
apps/workspace-engine/pkg/secrets/secrets.go (1)
23-33: Silent fallback toNoopEncryptionstores credentials in plaintext.When
CTRLPLANE_AES_256_KEYis missing or invalid, the code logs an error but silently falls back to no-op, meaning API keys are stored unencrypted. In production, this could go unnoticed. Consider whetherNewEncryptionshould return an error so callers can decide the behavior, or at least emit a more prominent warning (e.g., at startup health checks).apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go (3)
41-44: Variablesecretsshadows the imported package name.
secrets := secrets.NewEncryption()reuses the package identifier as a local variable name. This works but can cause confusion if someone later tries to reference the package in this function. Same pattern exists injob_agents.goLine 13.Proposed fix
func NewArgoApplication(store *store.Store, verifications *verification.Manager) *ArgoApplication { - secrets := secrets.NewEncryption() - return &ArgoApplication{store: store, verifications: verifications, secrets: secrets} + enc := secrets.NewEncryption() + return &ArgoApplication{store: store, verifications: verifications, secrets: enc} }
97-103:decryptOrPlaintextsilently swallows decryption errors — consider logging.Falling back to plaintext on error is reasonable for migration (supporting both encrypted and unencrypted values). However, silently ignoring errors makes it hard to diagnose issues where a truly encrypted value fails to decrypt (e.g., key rotation). A debug/warn log would help operators.
Proposed fix
func (a *ArgoApplication) decryptOrPlaintext(value string) string { decrypted, err := a.secrets.Decrypt(value) if err != nil { + log.Debug("failed to decrypt value, using as plaintext", "error", err) return value } return decrypted }
35-48: Encryption instance created per-constructor rather than dependency-injected.Both
ArgoApplicationandJobAgentsindependently callsecrets.NewEncryption(), each reading the env var and constructing a cipher. This is fine for now but makes it harder to test (can't inject a mock) and creates implicit coupling to the environment. If this is intentional for simplicity, that's okay — just noting it for future consideration.apps/workspace-engine/test/e2e/engine_job_agent_test.go (1)
675-717: Test verifies encryption but not the decryption round-trip.The test confirms the stored
apiKeydiffers from plaintext and that other fields remain unencrypted — good coverage for the encrypt path. However, it doesn't verify that the encrypted value can be successfully decrypted back to the original key (which is ultimately what matters for ArgoCD dispatch).Consider adding an assertion that decrypts
storedApiKeyback tooriginalApiKey, or a separate test that exercises the full encrypt → store → retrieve → decrypt → use flow.
| func NewEncryption() Encryption { | ||
| keyStr := os.Getenv("CTRLPLANE_AES_256_KEY") | ||
| if keyStr == "" { | ||
| log.Error("CTRLPLANE_AES_256_KEY is not set, using noop encryption") | ||
| return &NoopEncryption{} | ||
| } | ||
|
|
||
| if len(keyStr) != 32 { | ||
| log.Error("CTRLPLANE_AES_256_KEY must be 32 bytes, using noop encryption") | ||
| return &NoopEncryption{} | ||
| } | ||
|
|
||
| key := []byte(keyStr) | ||
| block, err := aes.NewCipher(key) | ||
| if err != nil { | ||
| log.Error("failed to create cipher: %w", err) | ||
| return &NoopEncryption{} | ||
| } | ||
|
|
||
| gcm, err := cipher.NewGCM(block) | ||
| if err != nil { | ||
| log.Error("failed to create GCM: %w", err) | ||
| return &NoopEncryption{} | ||
| } | ||
|
|
||
| return &AES256Encryption{gcm: gcm} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file content and imports
head -50 apps/workspace-engine/pkg/secrets/secrets.goRepository: ctrlplanedev/ctrlplane
Length of output: 1026
🏁 Script executed:
# Check for imports to identify the logging library
grep -n "import\|log\." apps/workspace-engine/pkg/secrets/secrets.go | head -30Repository: ctrlplanedev/ctrlplane
Length of output: 329
🏁 Script executed:
# Search for charmbracelet/log usage patterns in the codebase
rg "charmbracelet/log" apps/workspace-engine/Repository: ctrlplanedev/ctrlplane
Length of output: 5034
🌐 Web query:
charmbracelet/log v0.4.2 Error() API documentation structured logging
💡 Result:
In github.com/charmbracelet/log v0.4.2, the Error API is a leveled, structured log call that takes a message plus optional key/value pairs:
- Package-level function:
func Error(msg interface{}, keyvals ...interface{})(pkg.go.dev) - Logger method:
func (l *Logger) Error(msg interface{}, keyvals ...interface{})(pkg.go.dev)
Structured logging shape
keyvals is a variadic list interpreted as pairs ("key1", value1, "key2", value2, ...). Example usage from the project docs:
err := fmt.Errorf("too much sugar")
log.Error("failed to bake cookies", "err", err)To carry persistent structured context, you can create a sub-logger with fields:
log.With("err", err).Errorf("unable to start %s", "oven")slog integration (and what v0.4.2 fixed)
*log.Logger also implements log/slog’s handler interface, so you can do:
handler := log.New(os.Stderr)
logger := slog.New(handler)
logger.Error("meow?")v0.4.2 specifically fixed an issue when using the JSON formatter as a slog handler with error-typed messages, ensuring errors are extracted/recognized correctly in that path. (gitlab.com)
Citations:
- 1: https://pkg.go.dev/github.com/charmbracelet/log?utm_source=openai
- 2: https://pkg.go.dev/github.com/charmbracelet/log
- 3: https://github.com/charmbracelet/log?utm_source=openai
- 4: https://github.com/charmbracelet/log?utm_source=openai
- 5: https://pkg.go.dev/github.com/charmbracelet/log?utm_source=openai
- 6: https://gitlab.com/bitfoode/grc/-/merge_requests/6?utm_source=openai
🏁 Script executed:
# Search for other log.Error calls in the codebase to see how they're used
rg "log\.Error\(" apps/workspace-engine/ -A 1 | head -60Repository: ctrlplanedev/ctrlplane
Length of output: 4182
Fix log.Error calls to use structured key-value args instead of %w format strings.
charmbracelet/log uses structured logging. The %w format verb appears literally in output instead of formatting the error. Change both error logging calls to use the structured format:
log.Error("failed to create cipher", "error", err)and:
log.Error("failed to create GCM", "error", err)This matches the pattern used throughout the codebase.
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/secrets/secrets.go` around lines 23 - 49, In
NewEncryption, the log.Error calls inside the error branches for aes.NewCipher
and cipher.NewGCM use printf-style "%w" instead of structured key/value pairs;
change the two calls in NewEncryption that read log.Error("failed to create
cipher: %w", err) and log.Error("failed to create GCM: %w", err) to structured
form like log.Error("failed to create cipher", "error", err) and
log.Error("failed to create GCM", "error", err) so they match the codebase
pattern (references: NewEncryption, AES256Encryption, NoopEncryption).
| func (j *JobAgents) encryptCredentials(jobAgent *oapi.JobAgent) error { | ||
| jobAgentConfig := jobAgent.Config | ||
| for k, v := range jobAgentConfig { | ||
| if k == "apiKey" { | ||
| encrypted, err := j.secrets.Encrypt(v.(string)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| jobAgentConfig[k] = encrypted | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Unsafe type assertion on Line 31 will panic if apiKey is not a string.
If Config["apiKey"] is nil, a number, or any non-string type, v.(string) panics. Use the comma-ok idiom.
Proposed fix
func (j *JobAgents) encryptCredentials(jobAgent *oapi.JobAgent) error {
jobAgentConfig := jobAgent.Config
for k, v := range jobAgentConfig {
if k == "apiKey" {
- encrypted, err := j.secrets.Encrypt(v.(string))
+ strVal, ok := v.(string)
+ if !ok {
+ continue
+ }
+ encrypted, err := j.secrets.Encrypt(strVal)
if err != nil {
return err
}
jobAgentConfig[k] = encrypted
}
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (j *JobAgents) encryptCredentials(jobAgent *oapi.JobAgent) error { | |
| jobAgentConfig := jobAgent.Config | |
| for k, v := range jobAgentConfig { | |
| if k == "apiKey" { | |
| encrypted, err := j.secrets.Encrypt(v.(string)) | |
| if err != nil { | |
| return err | |
| } | |
| jobAgentConfig[k] = encrypted | |
| } | |
| } | |
| return nil | |
| } | |
| func (j *JobAgents) encryptCredentials(jobAgent *oapi.JobAgent) error { | |
| jobAgentConfig := jobAgent.Config | |
| for k, v := range jobAgentConfig { | |
| if k == "apiKey" { | |
| strVal, ok := v.(string) | |
| if !ok { | |
| continue | |
| } | |
| encrypted, err := j.secrets.Encrypt(strVal) | |
| if err != nil { | |
| return err | |
| } | |
| jobAgentConfig[k] = encrypted | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/store/job_agents.go` around lines 27 -
39, In JobAgents.encryptCredentials, avoid the unsafe type assertion v.(string)
when reading jobAgent.Config["apiKey"]; instead use the comma-ok idiom to check
the value and its type (e.g., val, ok := v.(string)) and handle the non-string
or nil case by returning a clear error (or skipping encryption) before calling
j.secrets.Encrypt; update the logic that assigns jobAgentConfig[k] to use the
encrypted string only when the cast succeeds.
🧩 Analysis chain
🏁 Script executed:
# First, find the JobAgents struct and Upsert method
rg -n "func.*Upsert" apps/workspace-engine/pkg/workspace/store/ --type=go -B2 -A10Repository: ctrlplanedev/ctrlplane
Length of output: 29164
🏁 Script executed:
# Find the JobAgents type definition
rg -n "type JobAgents" apps/workspace-engine/pkg/workspace/store/ --type=go -B2 -A10Repository: ctrlplanedev/ctrlplane
Length of output: 1156
🏁 Script executed:
# Search for where encryptCredentials is called
rg -n "encryptCredentials" apps/workspace-engine/pkg/workspace/store/ --type=go -B3 -A3Repository: ctrlplanedev/ctrlplane
Length of output: 1316
🏁 Script executed:
# Find Get/Retrieve methods on JobAgents to understand retrieval pattern
rg -n "func.*Get" apps/workspace-engine/pkg/workspace/store/job_agents.go --type=go -B2 -A10Repository: ctrlplanedev/ctrlplane
Length of output: 378
🏁 Script executed:
# Search for any decryption functions
rg -n "Decrypt" apps/workspace-engine/pkg/workspace/store/ --type=goRepository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Search for patterns where JobAgents are retrieved and potentially re-upseerted
# Look for Get followed by Upsert usage patterns
rg -n "jobAgent.*Get|Get.*jobAgent" apps/workspace-engine/ --type=go -B3 -A5Repository: ctrlplanedev/ctrlplane
Length of output: 28457
🏁 Script executed:
# Check if encryptCredentials has any logic to detect already-encrypted values
cat apps/workspace-engine/pkg/workspace/store/job_agents.goRepository: ctrlplanedev/ctrlplane
Length of output: 1582
🏁 Script executed:
# Search for any usage of JobAgents.Upsert to see what scenarios call it
rg -n "\.Upsert\(" apps/workspace-engine/ --type=go -B5 | grep -i "jobagent" -A5 -B5Repository: ctrlplanedev/ctrlplane
Length of output: 50379
🏁 Script executed:
# Look for JobAgentUpdate handler and event processing
rg -n "JobAgentUpdate" apps/workspace-engine/ --type=go -B5 -A10Repository: ctrlplanedev/ctrlplane
Length of output: 16394
🏁 Script executed:
# Search for handlers that process JobAgent events
rg -n "handler\.JobAgent" apps/workspace-engine/ --type=go -B3 -A8Repository: ctrlplanedev/ctrlplane
Length of output: 36751
🏁 Script executed:
# Check if there's any test showing the update flow
cat apps/workspace-engine/test/e2e/engine_job_agent_test.go | grep -A 20 "Update job agent"Repository: ctrlplanedev/ctrlplane
Length of output: 648
🏁 Script executed:
# Look at the specific test that handles apiKey encryption
sed -n '680,710p' apps/workspace-engine/test/e2e/engine_job_agent_test.goRepository: ctrlplanedev/ctrlplane
Length of output: 892
🏁 Script executed:
# Check if there's any decrypt logic in the secrets package
rg -n "type Encryption" apps/workspace-engine/pkg/secrets/ --type=go -A10Repository: ctrlplanedev/ctrlplane
Length of output: 842
🏁 Script executed:
# Check what happens when apiKey is used after retrieval
rg -n "Config\[\"apiKey\"\]" apps/workspace-engine/ --type=go -B3 -A3Repository: ctrlplanedev/ctrlplane
Length of output: 1288
Double encryption risk confirmed: re-upsetering a JobAgent with encrypted credentials will double-encrypt the apiKey and render it unrecoverable.
The vulnerability is real. When a JobAgent is retrieved via Get(), the apiKey is returned in encrypted form without decryption. If that JobAgent is then updated and Upsert() is called again (as happens in HandleJobAgentUpdated at line 57 of pkg/events/handler/jobagents/jobagents.go), encryptCredentials will encrypt the already-encrypted value a second time, making it irrecoverable.
The encryptCredentials method has no protection against this—it blindly encrypts any apiKey field without checking if it's already encrypted. The Encryption interface includes both Encrypt and Decrypt methods, but retrieval never calls Decrypt.
Fix this by either:
- Decrypting before re-encrypting in
encryptCredentialsto ensure idempotency, or - Adding a marker/prefix (e.g.,
enc:...) to encrypted values so the function can skip already-encrypted fields.
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/store/job_agents.go` around lines 27 -
39, The encryptCredentials method in JobAgents blindly re-encrypts
jobAgent.Config["apiKey"] which causes double-encryption when an
already-encrypted JobAgent is re-upserted; update encryptCredentials to first
detect and handle already-encrypted values by either attempting to decrypt via
j.secrets.Decrypt and using the decrypted value (then re-encrypt once), or by
checking/adding a stable prefix/marker (e.g., "enc:") around values produced by
j.secrets.Encrypt and skipping encryption if the marker is present; ensure you
reference and use j.secrets.Encrypt and j.secrets.Decrypt (or add the marker
logic) inside JobAgents.encryptCredentials so Upsert/Get/HandleJobAgentUpdated
flow is idempotent.
| func (j *JobAgents) Upsert(ctx context.Context, jobAgent *oapi.JobAgent) { | ||
| if err := j.encryptCredentials(jobAgent); err != nil { | ||
| log.Errorf("error encrypting credentials, skipping job agent upsert: %v", err) | ||
| return | ||
| } | ||
|
|
||
| j.repo.JobAgents.Set(jobAgent.Id, jobAgent) | ||
| j.store.changeset.RecordUpsert(jobAgent) | ||
| } |
There was a problem hiding this comment.
Silently skipping the upsert on encryption failure may cause data loss.
If encryption fails, the entire upsert is skipped without propagating the error to the caller. The job agent creation/update is silently dropped. Depending on the event-driven architecture, this event may never be retried, leading to a permanently missing agent.
Consider returning the error so the caller can decide how to handle it (e.g., retry, alert).
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/store/job_agents.go` around lines 41 -
49, The Upsert method on JobAgents currently swallows encryption errors and
silently skips the upsert; change JobAgents.Upsert so it returns an error
instead of returning early: call j.encryptCredentials(jobAgent) and if it
returns an error propagate that error to the caller (do not just log and
return), perform j.repo.JobAgents.Set(jobAgent.Id, jobAgent) and
j.store.changeset.RecordUpsert(jobAgent) only on success, and update any callers
to handle the returned error (including tests) so they can retry/alert as
appropriate; reference symbols: JobAgents.Upsert, encryptCredentials,
j.repo.JobAgents.Set, j.store.changeset.RecordUpsert.
| } | ||
|
|
||
| ciphertext := e.gcm.Seal(nonce, nonce, []byte(plaintext), nil) | ||
| return base64.StdEncoding.EncodeToString(ciphertext), nil |
There was a problem hiding this comment.
it would be nice to prefix with aes256:<base64> just so its easier to understand whats going on
There was a problem hiding this comment.
This is a pretty command pattern
| } | ||
|
|
||
| // Decrypt decrypts base64-encoded ciphertext and returns plaintext | ||
| func (e *AES256Encryption) Decrypt(ciphertext string) (string, error) { |
|
|
3 participants
Summary by CodeRabbit
New Features
Tests