Encrypt instance secrets at rest #515
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,240 @@ | ||
| package store | ||
|
|
||
| import ( | ||
| "context" | ||
| "crypto/aes" | ||
| "crypto/cipher" | ||
| "crypto/rand" | ||
| "database/sql" | ||
| "encoding/base64" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "time" | ||
| ) | ||
|
|
||
| const ( | ||
| encryptedSecretPrefix = "octobus-secret-v1:" | ||
| secretKeyEnv = "OCTOBUS_SECRET_ENCRYPTION_KEY" | ||
| secretKeyFileEnv = "OCTOBUS_SECRET_ENCRYPTION_KEY_FILE" | ||
| secretKeyBytes = 32 | ||
| ) | ||
|
|
||
| func loadSecretKey(dbPath string, hasEncryptedSecrets func() (bool, error)) ([]byte, error) { | ||
| if encoded := os.Getenv(secretKeyEnv); encoded != "" { | ||
| key, err := base64.StdEncoding.DecodeString(encoded) | ||
| if err != nil || len(key) != secretKeyBytes { | ||
| return nil, fmt.Errorf("%s must be base64-encoded %d-byte key", secretKeyEnv, secretKeyBytes) | ||
| } | ||
| return key, nil | ||
| } | ||
| if dbPath == ":memory:" { | ||
| return randomSecretKey() | ||
| } | ||
|
|
||
| keyPath := os.Getenv(secretKeyFileEnv) | ||
| if keyPath == "" { | ||
| keyPath = dbPath + ".secret-key" | ||
| } | ||
|
There was a problem hiding this comment. KEY_FILE 显式配置但目标文件缺失且库中尚无加密 secret 时，会静默生成新密钥并写入该路径，掩盖配置错误并可能导致后续数据不可解密当 OCTOBUS_SECRET_ENCRYPTION_KEY_FILE 被显式配置但目标文件不存在、且数据库尚无任何加密 secret（或 hasEncryptedSecrets 返回 false）时，loadSecretKey 会落入 randomSecretKey + writeSecretKeyAtomically 分支，在该路径自动生成并写入一个全新的随机密钥（必要时还会用 MkdirAll 创建父目录）。这与 README 的语义不一致：README 将 KEY_FILE 描述为操作者“提供”的密钥文件，并称只有“两者都未配置”时 OctoBus 才自动创建 .secret-key。在显式配置外部密钥的生产部署中，文件缺失通常意味着路径拼写错误、Secret 尚未挂载或编排顺序问题；此时静默生成密钥会掩盖该错误——应用将使用本地随机密钥加密后续写入的 secrets，一旦操作者随后修复路径或挂载真正的受管密钥并重启，这些已加密数据会因密钥不一致而无法解密（本地随机密钥已被覆盖/丢弃），同时该随机密钥也被写入了操作者本不打算用作密钥存储的位置。 Problem code: Recommendation: Suggested diff: diff --git a/internal/store/secret_crypto.go b/internal/store/secret_crypto.go
--- a/internal/store/secret_crypto.go
+++ b/internal/store/secret_crypto.go
@@ -42,6 +42,9 @@ func loadSecretKey(dbPath string, hasEncryptedSecrets func() (bool, error)) ([]b
key, err := readSecretKeyFile(keyPath)
if err == nil {
return key, nil
}
if !errors.Is(err, os.ErrNotExist) {
return nil, err
}
+ if os.Getenv(secretKeyFileEnv) != "" {
+ return nil, fmt.Errorf("secret key file %q from %s does not exist", keyPath, secretKeyFileEnv)
+ }
if hasEncryptedSecrets != nil { |
||
| key, err := readSecretKeyFile(keyPath) | ||
| if err == nil { | ||
| return key, nil | ||
| } | ||
| if !errors.Is(err, os.ErrNotExist) { | ||
| return nil, err | ||
| } | ||
| if hasEncryptedSecrets != nil { | ||
| encrypted, checkErr := hasEncryptedSecrets() | ||
| if checkErr != nil { | ||
| return nil, checkErr | ||
| } | ||
| if encrypted { | ||
| return nil, fmt.Errorf("secret key file %q is missing while encrypted instance secrets exist", keyPath) | ||
| } | ||
| } | ||
| key, err = randomSecretKey() | ||
|
monkeyscan[bot] marked this conversation as resolved.
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := writeSecretKeyAtomically(keyPath, key); err == nil { | ||
| return key, nil | ||
|
monkeyscan[bot] marked this conversation as resolved.
|
||
| } else if !errors.Is(err, os.ErrExist) { | ||
| return nil, err | ||
| } | ||
| for attempt := 0; attempt < 5; attempt++ { | ||
| key, err = readSecretKeyFile(keyPath) | ||
| if err == nil { | ||
| return key, nil | ||
| } | ||
| if !errors.Is(err, errInvalidSecretKeyLength) { | ||
| return nil, err | ||
| } | ||
| time.Sleep(10 * time.Millisecond) | ||
| } | ||
| return nil, fmt.Errorf("read secret key file %q: %w", keyPath, errInvalidSecretKeyLength) | ||
| } | ||
|
|
||
| var errInvalidSecretKeyLength = errors.New("secret key has invalid length") | ||
|
|
||
| func readSecretKeyFile(path string) ([]byte, error) { | ||
| key, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(key) != secretKeyBytes { | ||
| return nil, fmt.Errorf("%w: %q", errInvalidSecretKeyLength, path) | ||
| } | ||
| if err := os.Chmod(path, 0o600); err != nil { | ||
| return nil, err | ||
| } | ||
| return key, nil | ||
| } | ||
|
|
||
| func writeSecretKeyAtomically(path string, key []byte) error { | ||
| if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { | ||
| return err | ||
| } | ||
| tmp, err := os.CreateTemp(filepath.Dir(path), ".secret-key-") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| tmpPath := tmp.Name() | ||
| defer os.Remove(tmpPath) | ||
| if err := tmp.Chmod(0o600); err != nil { | ||
| _ = tmp.Close() | ||
| return err | ||
| } | ||
| if _, err := tmp.Write(key); err != nil { | ||
| _ = tmp.Close() | ||
| return err | ||
| } | ||
| if err := tmp.Sync(); err != nil { | ||
| _ = tmp.Close() | ||
| return err | ||
| } | ||
| if err := tmp.Close(); err != nil { | ||
| return err | ||
| } | ||
| if err := os.Link(tmpPath, path); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (s *Store) validateSecretKey(ctx context.Context) error { | ||
| var encoded string | ||
| err := s.db.QueryRowContext(ctx, `SELECT secret_json FROM instances WHERE substr(secret_json, 1, ?) = ? LIMIT 1`, len(encryptedSecretPrefix), encryptedSecretPrefix).Scan(&encoded) | ||
| if errors.Is(err, sql.ErrNoRows) || strings.Contains(errString(err), "no such table") { | ||
| return nil | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err := decryptSecret(s.secretKey, encoded); err != nil { | ||
| return fmt.Errorf("secret encryption key does not decrypt stored instance secrets: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
There was a problem hiding this comment. validateSecretKey 仅校验单条加密记录，密钥不匹配/混合密钥时校验结果不确定且可能漏检本次变更新增 validateSecretKey 的目标是「打开即校验，避免密钥与库内密文不匹配时静默解密失败」。但实现使用 Problem code: Recommendation: Suggested diff: - err := s.db.QueryRowContext(ctx, `SELECT secret_json FROM instances WHERE substr(secret_json, 1, ?) = ? LIMIT 1`, len(encryptedSecretPrefix), encryptedSecretPrefix).Scan(&encoded)
- if errors.Is(err, sql.ErrNoRows) || strings.Contains(errString(err), "no such table") {
- return nil
- }
- if err != nil {
- return err
- }
- if _, err := decryptSecret(s.secretKey, encoded); err != nil {
- return fmt.Errorf("secret encryption key does not decrypt stored instance secrets: %w", err)
- }
- return nil
+ rows, err := s.db.QueryContext(ctx, `SELECT id, secret_json FROM instances WHERE substr(secret_json, 1, ?) = ?`, len(encryptedSecretPrefix), encryptedSecretPrefix)
+ if err != nil {
+ if strings.Contains(errString(err), "no such table") {
+ return nil
+ }
+ return err
+ }
+ defer rows.Close()
+ for rows.Next() {
+ var id, encoded string
+ if err := rows.Scan(&id, &encoded); err != nil {
+ return err
+ }
+ if _, err := decryptSecret(s.secretKey, encoded); err != nil {
+ return fmt.Errorf("secret encryption key does not decrypt stored instance secret for instance %q: %w", id, err)
+ }
+ }
+ return rows.Err() |
||
|
|
||
| func errString(err error) string { | ||
| if err == nil { | ||
| return "" | ||
| } | ||
| return err.Error() | ||
| } | ||
|
|
||
| func randomSecretKey() ([]byte, error) { | ||
| key := make([]byte, secretKeyBytes) | ||
| if _, err := io.ReadFull(rand.Reader, key); err != nil { | ||
| return nil, err | ||
| } | ||
| return key, nil | ||
| } | ||
|
|
||
| func encryptSecret(key, plaintext []byte) (string, error) { | ||
| block, err := aes.NewCipher(key) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| gcm, err := cipher.NewGCM(block) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| nonce := make([]byte, gcm.NonceSize()) | ||
| if _, err := io.ReadFull(rand.Reader, nonce); err != nil { | ||
| return "", err | ||
| } | ||
| ciphertext := gcm.Seal(nonce, nonce, plaintext, nil) | ||
| return encryptedSecretPrefix + base64.RawStdEncoding.EncodeToString(ciphertext), nil | ||
| } | ||
|
|
||
| func decryptSecret(key []byte, encoded string) ([]byte, error) { | ||
| if len(encoded) < len(encryptedSecretPrefix) || encoded[:len(encryptedSecretPrefix)] != encryptedSecretPrefix { | ||
| return []byte(encoded), nil | ||
| } | ||
| payload, err := base64.RawStdEncoding.DecodeString(encoded[len(encryptedSecretPrefix):]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode encrypted instance secret: %w", err) | ||
| } | ||
| block, err := aes.NewCipher(key) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| gcm, err := cipher.NewGCM(block) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(payload) < gcm.NonceSize() { | ||
| return nil, errors.New("encrypted instance secret is truncated") | ||
| } | ||
| return gcm.Open(nil, payload[:gcm.NonceSize()], payload[gcm.NonceSize():], nil) | ||
| } | ||
|
|
||
| func (s *Store) encryptLegacySecrets(ctx context.Context) error { | ||
| rows, err := s.db.QueryContext(ctx, `SELECT id, secret_json FROM instances WHERE secret_json <> '' AND substr(secret_json, 1, ?) <> ?`, len(encryptedSecretPrefix), encryptedSecretPrefix) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| type legacySecret struct { | ||
| id string | ||
| data string | ||
| } | ||
| var legacy []legacySecret | ||
| for rows.Next() { | ||
| var item legacySecret | ||
| if err := rows.Scan(&item.id, &item.data); err != nil { | ||
| _ = rows.Close() | ||
| return err | ||
| } | ||
| legacy = append(legacy, item) | ||
| } | ||
| if err := rows.Err(); err != nil { | ||
| _ = rows.Close() | ||
| return err | ||
| } | ||
| if err := rows.Close(); err != nil { | ||
| return err | ||
| } | ||
| if len(legacy) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| tx, err := s.db.BeginTx(ctx, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer tx.Rollback() | ||
| for _, item := range legacy { | ||
| encrypted, err := encryptSecret(s.secretKey, []byte(item.data)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err := tx.ExecContext(ctx, `UPDATE instances SET secret_json = ? WHERE id = ?`, encrypted, item.id); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return tx.Commit() | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| package store | ||
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "octobus/internal/domain" | ||
| ) | ||
|
|
||
| func TestInstanceSecretsAreEncryptedAtRestAndReadableAcrossReopen(t *testing.T) { | ||
| dbPath := t.TempDir() + "/octobus.db" | ||
| ctx := context.Background() | ||
| st, err := Open(dbPath) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := st.UpsertService(ctx, domain.Service{ID: "secret-service", Name: "Secret Service"}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| plain := []byte(`{"apiToken":"do-not-store-plaintext"}`) | ||
| if err := st.UpsertInstance(ctx, domain.Instance{ID: "secret-instance", ServiceID: "secret-service", Name: "Instance", SecretJSON: plain}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| var stored string | ||
| if err := st.DB().QueryRowContext(ctx, `SELECT secret_json FROM instances WHERE id = ?`, "secret-instance").Scan(&stored); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if strings.Contains(stored, "do-not-store-plaintext") || !strings.HasPrefix(stored, encryptedSecretPrefix) { | ||
| t.Fatalf("secret at rest is not encrypted: %q", stored) | ||
| } | ||
| if err := st.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| st, err = Open(dbPath) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer st.Close() | ||
| got, err := st.GetInstance(ctx, "secret-instance") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if string(got.SecretJSON) != string(plain) { | ||
| t.Fatalf("decrypted secret = %s", got.SecretJSON) | ||
| } | ||
| } | ||
|
|
||
| func TestOpenFailsWhenSecretKeyIsMissingForEncryptedData(t *testing.T) { | ||
| t.Setenv(secretKeyEnv, "") | ||
| dbPath := t.TempDir() + "/octobus.db" | ||
| ctx := context.Background() | ||
| st, err := Open(dbPath) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := st.UpsertService(ctx, domain.Service{ID: "secret-service", Name: "Secret Service"}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := st.UpsertInstance(ctx, domain.Instance{ID: "secret-instance", ServiceID: "secret-service", Name: "Instance", SecretJSON: []byte(`{"token":"value"}`)}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := st.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := os.Remove(dbPath + ".secret-key"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if _, err := Open(dbPath); err == nil || !strings.Contains(err.Error(), "encrypted instance secrets exist") { | ||
| t.Fatalf("missing secret key error = %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestLegacyInstanceSecretsAreEncryptedDuringMigration(t *testing.T) { | ||
| dbPath := t.TempDir() + "/octobus.db" | ||
| ctx := context.Background() | ||
| st, err := Open(dbPath) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := st.UpsertService(ctx, domain.Service{ID: "legacy-service", Name: "Legacy Service"}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := st.UpsertInstance(ctx, domain.Instance{ID: "legacy-instance", ServiceID: "legacy-service", Name: "Instance", SecretJSON: []byte(`{}`)}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if _, err := st.DB().ExecContext(ctx, `UPDATE instances SET secret_json = ? WHERE id = ?`, `{"legacy":"secret"}`, "legacy-instance"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := st.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| st, err = Open(dbPath) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer st.Close() | ||
| var stored string | ||
| if err := st.DB().QueryRowContext(ctx, `SELECT secret_json FROM instances WHERE id = ?`, "legacy-instance").Scan(&stored); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if strings.Contains(stored, "legacy") || !strings.HasPrefix(stored, encryptedSecretPrefix) { | ||
| t.Fatalf("legacy secret was not migrated: %q", stored) | ||
| } | ||
| got, err := st.GetInstance(ctx, "legacy-instance") | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if string(got.SecretJSON) != `{"legacy":"secret"}` { | ||
| t.Fatalf("migrated secret = %s", got.SecretJSON) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OCTOBUS_SECRET_ENCRYPTION_KEY_FILE 指向只读/非属主的外部密钥文件时，readSecretKeyFile 的 os.Chmod 会使启动失败
本次新增的 OCTOBUS_SECRET_ENCRYPTION_KEY_FILE（第 38-41 行）让密钥路径可以指向外部受管位置（README 也建议 KMS/Vault/Secret 管理的生产部署）。但读取逻辑 readSecretKeyFile 在成功读取并校验 32 字节长度后，无条件执行 os.Chmod(path, 0o600)；任何 chmod 失败都会作为非 os.ErrNotExist 错误被 loadSecretKey 原样返回，导致启动失败。外部密钥文件的典型部署形态——K8s Secret 卷/投影、Vault Agent 模板文件、只读挂载、root 属主而进程以非 root 运行——会因 EROFS/EPERM 使 chmod 失败，即便文件存在、可读且恰好 32 字节。也就是说，该特性在最常见的安全部署方式下不可用。改动前 chmod 只作用于 daemon 自己创建、位于可写数据目录的默认 .secret-key，几乎不会失败；本次变更新增了指向外部文件的入口，把这一既有行为暴露为新的故障路径（启动即报错，特性形同虚设）。
Problem code:
Recommendation:
对已存在且可读的外部密钥文件，应把 chmod 降级为尽力而为（失败仅记录而不阻断），权限收紧只对 daemon 自己创建的文件负责（writeSecretKeyAtomically 已保证 0600）；或在文档中明确要求密钥文件必须可被进程 chmod 为 0600，并在失败时给出清晰可操作的错误提示。
Suggested diff: