feat: simple encryption of creds #792
Conversation
📝 WalkthroughWalkthroughAdds a pluggable secrets package implementing AES‑256‑GCM and noop encryption, wires encryption into JobAgents to encrypt 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, aes256:]
Store->>DB: Store JobAgent (encrypted apiKey)
DB-->>Store: OK
Client->>Argo: Parse JobAgent config / perform action
Argo->>Enc: Decrypt(encrypted apiKey) or return plaintext
Enc-->>Argo: plaintext apiKey (or error -> fallback to original)
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 (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) { |
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/secrets/secrets.go`:
- Around line 26-52: NewEncryption currently silently falls back to
NoopEncryption on key misconfiguration or cipher errors; change it so that an
empty config.Global.AES256Key still returns &NoopEncryption{}, but if AES256Key
is set and either its length != 32 or aes.NewCipher / cipher.NewGCM fails, fail
fast (e.g., call log.Fatal or panic/exit) instead of returning NoopEncryption so
credentials are not stored plaintext; update NewEncryption to log the detailed
error (include the err value) and terminate when the provided key is invalid or
cipher setup fails, leaving NoopEncryption only for the intentionally unset key
case (references: NewEncryption, config.Global.AES256Key, NoopEncryption,
AES256Encryption, aes.NewCipher, cipher.NewGCM).
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/workspace/store/job_agents.go (2)
14-20: Variablesecretsshadows the imported package name.On Line 15,
secrets := secrets.NewEncryption()shadows thesecretspackage import. This compiles but hurts readability — any later reference tosecretsin this function would resolve to the local variable, not the package. Consider renaming the local variable.Proposed fix
func NewJobAgents(store *Store) *JobAgents { - secrets := secrets.NewEncryption() + enc := secrets.NewEncryption() return &JobAgents{ repo: store.repo, store: store, - secrets: secrets, + secrets: enc, } }
52-52: Use structured logging (log.Error) instead oflog.Errorf.The codebase uses
charmbracelet/logwith structured key-value pairs (e.g.,log.Error("msg", "key", val)).log.Errorfuses printf-style formatting which is inconsistent with the rest of the codebase's logging pattern.Proposed fix
- log.Errorf("error encrypting credentials, skipping job agent upsert: %v", err) + log.Error("error encrypting credentials, skipping job agent upsert", "error", err)apps/workspace-engine/pkg/secrets/secrets.go (2)
15-24: Add doc comments on exported types per coding guidelines.The
Encryptioninterface,AES256Encryptionstruct, andAES_256_PREFIXconstant are all exported but lack doc comments. As per coding guidelines, exported functions/types/methods should be documented.
91-104:NoopEncryptionshould document that it provides no actual encryption.A brief doc comment on the type and
NewNoopEncryptionwould make the security implications clear to future readers — this is a passthrough used only when no key is configured.
| func NewEncryption() Encryption { | ||
| keyStr := config.Global.AES256Key | ||
| if keyStr == "" { | ||
| log.Error("AES_256_KEY is not set, using noop encryption") | ||
| return &NoopEncryption{} | ||
| } | ||
|
|
||
| if len(keyStr) != 32 { | ||
| log.Error("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", "error", err) | ||
| return &NoopEncryption{} | ||
| } | ||
|
|
||
| gcm, err := cipher.NewGCM(block) | ||
| if err != nil { | ||
| log.Error("failed to create GCM", "error", err) | ||
| return &NoopEncryption{} | ||
| } | ||
|
|
||
| return &AES256Encryption{gcm: gcm} | ||
| } |
There was a problem hiding this comment.
Silent fallback to NoopEncryption on misconfiguration stores credentials in plaintext.
If AES_256_KEY is set but has the wrong length, or cipher setup fails, the system silently falls back to no encryption. An operator may believe encryption is active when it isn't. Consider making this a fatal error (at least when the key is set but invalid), reserving the noop fallback only for when the key is intentionally omitted.
Proposed fix (fail hard on invalid key, noop only when unset)
func NewEncryption() Encryption {
keyStr := config.Global.AES256Key
if keyStr == "" {
- log.Error("AES_256_KEY is not set, using noop encryption")
+ log.Warn("AES_256_KEY is not set, using noop encryption")
return &NoopEncryption{}
}
if len(keyStr) != 32 {
- log.Error("AES_256_KEY must be 32 bytes, using noop encryption")
- return &NoopEncryption{}
+ log.Fatal("AES_256_KEY is set but must be exactly 32 bytes")
}
key := []byte(keyStr)
block, err := aes.NewCipher(key)
if err != nil {
- log.Error("failed to create cipher", "error", err)
- return &NoopEncryption{}
+ log.Fatal("failed to create AES cipher", "error", err)
}
gcm, err := cipher.NewGCM(block)
if err != nil {
- log.Error("failed to create GCM", "error", err)
- return &NoopEncryption{}
+ log.Fatal("failed to create GCM cipher", "error", err)
}
return &AES256Encryption{gcm: gcm}
}🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/secrets/secrets.go` around lines 26 - 52,
NewEncryption currently silently falls back to NoopEncryption on key
misconfiguration or cipher errors; change it so that an empty
config.Global.AES256Key still returns &NoopEncryption{}, but if AES256Key is set
and either its length != 32 or aes.NewCipher / cipher.NewGCM fails, fail fast
(e.g., call log.Fatal or panic/exit) instead of returning NoopEncryption so
credentials are not stored plaintext; update NewEncryption to log the detailed
error (include the err value) and terminate when the provided key is invalid or
cipher setup fails, leaving NoopEncryption only for the intentionally unset key
case (references: NewEncryption, config.Global.AES256Key, NoopEncryption,
AES256Encryption, aes.NewCipher, cipher.NewGCM).
3 participants
Summary by CodeRabbit
New Features
Tests