Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/workspace-engine/pkg/config/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,6 @@ type Config struct {
RegisterAddress string `envconfig:"REGISTER_ADDRESS" default:""`

TraceTokenSecret string `envconfig:"TRACE_TOKEN_SECRET" default:"secret"`

AES256Key string `envconfig:"AES_256_KEY" default:""`
}
104 changes: 104 additions & 0 deletions apps/workspace-engine/pkg/secrets/secrets.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package secrets

import (
"crypto/aes"
"crypto/cipher"
"crypto/rand"
"encoding/base64"
"fmt"
"strings"
"workspace-engine/pkg/config"

"github.com/charmbracelet/log"
)

const AES_256_PREFIX = "aes256:"

type Encryption interface {
Encrypt(plaintext string) (string, error)
Decrypt(ciphertext string) (string, error)
}

type AES256Encryption struct {
gcm cipher.AEAD
}

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}
}
Comment on lines +26 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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).


// Encrypt encrypts plaintext and returns base64-encoded ciphertext with aes256: prefix
func (e *AES256Encryption) Encrypt(plaintext string) (string, error) {
nonce := make([]byte, e.gcm.NonceSize())
if _, err := rand.Read(nonce); err != nil {
return "", fmt.Errorf("failed to generate nonce: %w", err)
}

ciphertext := e.gcm.Seal(nonce, nonce, []byte(plaintext), nil)
return AES_256_PREFIX + base64.StdEncoding.EncodeToString(ciphertext), nil
}

// Decrypt decrypts base64-encoded ciphertext (with aes256: prefix) and returns plaintext
func (e *AES256Encryption) Decrypt(ciphertext string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and remove the prefix here

if !strings.HasPrefix(ciphertext, AES_256_PREFIX) {
return "", fmt.Errorf("invalid ciphertext: missing %s prefix", AES_256_PREFIX)
}

encoded := strings.TrimPrefix(ciphertext, AES_256_PREFIX)
data, err := base64.StdEncoding.DecodeString(encoded)
if err != nil {
return "", fmt.Errorf("failed to decode base64: %w", err)
}

nonceSize := e.gcm.NonceSize()
if len(data) < nonceSize {
return "", fmt.Errorf("ciphertext too short")
}

nonce, encrypted := data[:nonceSize], data[nonceSize:]
plaintext, err := e.gcm.Open(nil, nonce, encrypted, nil)
if err != nil {
return "", fmt.Errorf("failed to decrypt: %w", err)
}

return string(plaintext), nil
}

type NoopEncryption struct {
}

func NewNoopEncryption() Encryption {
return &NoopEncryption{}
}

func (e *NoopEncryption) Encrypt(plaintext string) (string, error) {
return plaintext, nil
}

func (e *NoopEncryption) Decrypt(ciphertext string) (string, error) {
return ciphertext, nil
}
18 changes: 16 additions & 2 deletions apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"workspace-engine/pkg/messaging"
"workspace-engine/pkg/messaging/confluent"
"workspace-engine/pkg/oapi"
"workspace-engine/pkg/secrets"
"workspace-engine/pkg/templatefuncs"
"workspace-engine/pkg/workspace/jobagents/types"
"workspace-engine/pkg/workspace/releasemanager/verification"
Expand All @@ -34,10 +35,12 @@ var _ types.Dispatchable = &ArgoApplication{}
type ArgoApplication struct {
store *store.Store
verifications *verification.Manager
secrets secrets.Encryption
}

func NewArgoApplication(store *store.Store, verifications *verification.Manager) *ArgoApplication {
return &ArgoApplication{store: store, verifications: verifications}
secrets := secrets.NewEncryption()
return &ArgoApplication{store: store, verifications: verifications, secrets: secrets}
}

func (a *ArgoApplication) Type() string {
Expand Down Expand Up @@ -91,15 +94,26 @@ func (a *ArgoApplication) Dispatch(ctx context.Context, dispatchCtx types.Dispat
return nil
}

func (a *ArgoApplication) decryptOrPlaintext(value string) string {
decrypted, err := a.secrets.Decrypt(value)
if err != nil {
return value
}
return decrypted
}

func (a *ArgoApplication) parseJobAgentConfig(jobAgentConfig oapi.JobAgentConfig) (string, string, string, error) {
serverAddr, ok := jobAgentConfig["serverUrl"].(string)
if !ok {
return "", "", "", fmt.Errorf("serverUrl is required")
}
apiKey, ok := jobAgentConfig["apiKey"].(string)
apiKeyRaw, ok := jobAgentConfig["apiKey"].(string)
if !ok {
return "", "", "", fmt.Errorf("apiKey is required")
}

apiKey := a.decryptOrPlaintext(apiKeyRaw)

template, ok := jobAgentConfig["template"].(string)
if !ok {
return "", "", "", fmt.Errorf("template is required")
Expand Down
42 changes: 38 additions & 4 deletions apps/workspace-engine/pkg/workspace/store/job_agents.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,57 @@ package store

import (
"context"
"fmt"
"strings"
"workspace-engine/pkg/oapi"
"workspace-engine/pkg/secrets"
"workspace-engine/pkg/workspace/store/repository"

"github.com/charmbracelet/log"
)

func NewJobAgents(store *Store) *JobAgents {
secrets := secrets.NewEncryption()
return &JobAgents{
repo: store.repo,
store: store,
repo: store.repo,
store: store,
secrets: secrets,
}
}

type JobAgents struct {
repo *repository.InMemoryStore
store *Store
repo *repository.InMemoryStore
store *Store
secrets secrets.Encryption
}

func (j *JobAgents) encryptCredentials(jobAgent *oapi.JobAgent) error {
jobAgentConfig := jobAgent.Config
for k, v := range jobAgentConfig {
if k == "apiKey" {
plaintext, ok := v.(string)
if !ok {
return fmt.Errorf("apiKey is not a string: %v", v)
}
if strings.HasPrefix(plaintext, secrets.AES_256_PREFIX) {
continue
}
encrypted, err := j.secrets.Encrypt(plaintext)
if err != nil {
return err
}
jobAgentConfig[k] = encrypted
}
}
return nil
}

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)
}
Comment on lines 50 to 58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Expand Down
44 changes: 44 additions & 0 deletions apps/workspace-engine/test/e2e/engine_job_agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -671,3 +671,47 @@ func TestEngine_JobAgentNameUniqueness(t *testing.T) {
t.Fatal("agents should have different IDs")
}
}

func TestEngine_JobAgentApiKeyEncryption(t *testing.T) {
t.Setenv("CTRLPLANE_AES_256_KEY", "01234567890123456789012345678901")

jobAgentID := "job-agent-encrypted"
originalApiKey := "super-secret-api-key"

engine := integration.NewTestWorkspace(t)
ctx := context.Background()

ja := c.NewJobAgent(engine.Workspace().ID)
ja.Id = jobAgentID
ja.Name = "Encrypted Agent"
ja.WorkspaceId = engine.Workspace().ID
ja.Config = map[string]any{
"serverUrl": "https://argocd.example.com",
"apiKey": originalApiKey,
"template": "my-app-template",
}

engine.PushEvent(ctx, handler.JobAgentCreate, ja)

retrievedJa, exists := engine.Workspace().JobAgents().Get(jobAgentID)
if !exists {
t.Fatal("job agent not found")
}

storedApiKey, ok := retrievedJa.Config["apiKey"].(string)
if !ok {
t.Fatal("apiKey not found in config")
}

if storedApiKey == originalApiKey {
t.Fatal("apiKey should be encrypted, but it matches the original plaintext")
}

if retrievedJa.Config["serverUrl"] != "https://argocd.example.com" {
t.Fatalf("serverUrl should not be encrypted: got %v", retrievedJa.Config["serverUrl"])
}

if retrievedJa.Config["template"] != "my-app-template" {
t.Fatalf("template should not be encrypted: got %v", retrievedJa.Config["template"])
}
}
Loading