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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,236 @@ | ||
| 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" | ||
| 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 := dbPath + ".secret-key" | ||
| 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）时，AES-256-GCM 密钥以明文写入与数据库同目录的 .secret-key 文件（仅 0600）。能读取数据库文件的一方（备份导出、目录拷贝、同一 OS 用户下的其他进程/恶意软件）通常也能读取该密钥，导致“静态加密”退化为仅防随意查看的混淆，无法满足常见的威胁模型（数据库文件脱离密钥环境泄露时仍不可读）。
Problem code:
Recommendation:
在生产环境中将环境变量/密钥管理服务作为加载密钥的强制路径，并明确密钥文件自动生成仅用于开发或本机使用；若保留密钥文件方案，建议与数据库分目录存放并收紧目录权限，同时在文档中明确其威胁模型与局限。