Encrypt instance secrets at rest - #515
Conversation
|
PR Title: Encrypt instance secrets at rest Commit: 本次变更为 octobus 的实例密钥（instances.secret_json）引入静态加密：新增 internal/store/secret_crypto.go，使用 AES-256-GCM 加解密，密文以 "octobus-secret-v1:" 前缀标记、nonce 前置、base64(RawStd) 编码。密钥来源优先级为 OCTOBUS_SECRET_ENCRYPTION_KEY 环境变量（base64 编码 32 字节）→ .secret-key 文件（缺失时自动生成随机密钥并以 0600 写入）→ :memory: 随机密钥。接线方面：Open 时通过 loadSecretKey 加载密钥存入 Store.secretKey；Migrate 中调用 encryptLegacySecrets 将既有明文 secret 事务性迁移为密文；UpsertInstance 写库前加密，GetInstance/ListInstances/ListEnabledInstancesByService 读库后经 scanInstanceWithKey 解密。新增两个测试覆盖"加密落盘且重开后仍可读"与"遗留明文迁移"。整体加密实现正确、迁移具备事务性且幂等，但密钥管理路径存在可靠性/数据完整性风险：密钥文件缺失时静默生成新密钥会使既有密文无法解密（Open 阶段无任何报错）；密钥文件写入非原子，崩溃可留下截断密钥文件导致后续无法启动；默认密钥与数据库同目录明文存放削弱静态加密效果。 |
![monkeyscan[bot]](https://avatars.githubusercontent.com/u/231811742?s=60&v=4){kind=small}
| return randomSecretKey() | ||
| } | ||
|
|
||
| keyPath := dbPath + ".secret-key" |
There was a problem hiding this comment.
默认密钥与数据库同目录明文存放，削弱静态加密保护效果
默认（未设置 OCTOBUS_SECRET_ENCRYPTION_KEY）时，AES-256-GCM 密钥以明文写入与数据库同目录的 .secret-key 文件（仅 0600）。能读取数据库文件的一方（备份导出、目录拷贝、同一 OS 用户下的其他进程/恶意软件）通常也能读取该密钥，导致“静态加密”退化为仅防随意查看的混淆，无法满足常见的威胁模型（数据库文件脱离密钥环境泄露时仍不可读）。
Problem code:
Changed code at internal/store/secret_crypto.go:33
Recommendation:
在生产环境中将环境变量/密钥管理服务作为加载密钥的强制路径，并明确密钥文件自动生成仅用于开发或本机使用；若保留密钥文件方案，建议与数据库分目录存放并收紧目录权限，同时在文档中明确其威胁模型与局限。
|
PR Title: Encrypt instance secrets at rest Commit: 本次变更围绕密钥文件的生命周期管理做了一次收敛修复，主要针对两个历史问题：(1) 密钥文件非原子写入，崩溃/并发下可能留下截断密钥文件或读到部分内容；(2) 密钥文件缺失时静默生成新密钥，导致已有加密数据永久不可解密。 实现方式：
测试新增了密钥文件缺失且存在加密数据时 Open 报错的用例。 总体评估：修复方向正确，主要故障路径（密钥文件丢失、崩溃截断、并发首开）均得到妥善处理。剩余风险主要在于 validateSecretKey 仅校验单条记录（LIMIT 1），在库内混合多种密钥加密记录时校验结果不确定且可能漏检；另有少量边缘场景（如密钥文件只读/加固导致强制 chmod 失败）但实际触发条件受限。未发现由本次变更引入的高危正确性缺陷。 |
| 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 的目标是「打开即校验，避免密钥与库内密文不匹配时静默解密失败」。但实现使用 SELECT secret_json FROM instances WHERE substr(secret_json,1,?)=? LIMIT 1 只取一条带前缀密文做解密验证：1) 当库内混合了由不同密钥加密的历史记录时（密钥文件曾被替换/切换过 OCTOBUS_SECRET_ENCRYPTION_KEY 后又切回，旧行未用新密钥重加密），若被选中的第一行恰巧能用当前密钥解密，校验通过、Open 成功，但其余旧密钥行在后续 GetInstance/List 读取时仍会以 gcm 校验失败告终——即本变更试图消除的静默失败路径依然存在；2) 查询没有 ORDER BY，命中哪一行由 SQLite 内部顺序决定，因此同一数据库的校验结果是不确定的：某些行序下能正常打开（漏检），换一种数据分布/插入顺序则直接启动失败（单行损坏也会拖垮整个 Open）。这是对历史 finding「密钥不匹配导致密文不可解密」的修复不完整。
Problem code:
Changed code at internal/store/secret_crypto.go:123-136
Recommendation:
改为遍历所有带前缀的实例密文逐一尝试解密（按 id 取全部匹配行），任一行解密失败即返回明确错误并在错误中附带 instance id，既避免漏检也避免仅凭单行做全局判定。示例：SELECT id, secret_json FROM instances WHERE substr(secret_json,1,?)=? 循环 Scan 后对每行调用 decryptSecret，失败时返回 fmt.Errorf("secret encryption key does not decrypt stored instance secret for instance %q: %w", id, err)。
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()
|
Follow-up fixes after CI/code review:
Verification: |
|
PR Title: Encrypt instance secrets at rest Commit: 本次变更在 internal/store/secret_crypto.go 的 loadSecretKey 中新增环境变量 OCTOBUS_SECRET_ENCRYPTION_KEY_FILE，允许覆盖默认的 .secret-key 路径，把实例密钥放到外部独立受管位置；README 相应补充了密钥存储说明。新增逻辑本身简洁（os.Getenv 后为空则回退默认路径），且当库中存在加密 secret 而密钥文件缺失时会显式报错，属正确的 fail-stop 行为。审查发现两个由该新入口暴露的问题：(1) readSecretKeyFile 对外部密钥文件无条件执行 os.Chmod(0600)，对只读挂载/K8s Secret 卷/Vault 模板等典型外部受管文件会因 EROFS/EPERM 直接启动失败，使该特性的主要部署形态不可用；(2) 当 KEY_FILE 显式配置但文件缺失且库中尚无加密 secret 时，代码会静默生成新随机密钥并写入该路径，与 README 语义不一致，可能掩盖配置错误并在后续导致已加密数据不可解密。两处均建议将外部密钥文件的权限处理/缺失行为改为显式且可诊断。 |
| return randomSecretKey() | ||
| } | ||
|
|
||
| keyPath := os.Getenv(secretKeyFileEnv) |
There was a problem hiding this comment.
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:
Changed code at internal/store/secret_crypto.go:38
Recommendation:
对已存在且可读的外部密钥文件，应把 chmod 降级为尽力而为（失败仅记录而不阻断），权限收紧只对 daemon 自己创建的文件负责（writeSecretKeyAtomically 已保证 0600）；或在文档中明确要求密钥文件必须可被进程 chmod 为 0600，并在失败时给出清晰可操作的错误提示。
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
@@ -87,7 +87,8 @@ func readSecretKeyFile(path string) ([]byte, error) {
if len(key) != secretKeyBytes {
return nil, fmt.Errorf("%w: %q", errInvalidSecretKeyLength, path)
}
- if err := os.Chmod(path, 0o600); err != nil {
- return nil, err
- }
+ // 只读挂载/KMS 注入等外部密钥文件可能无法 chmod；文件已可读且长度合法即应接受。
+ // 本进程创建的密钥文件已由 writeSecretKeyAtomically 置为 0600。
+ _ = os.Chmod(path, 0o600)
return key, nil
}
| 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:
Changed code at internal/store/secret_crypto.go:41
Recommendation:
当 secretKeyFileEnv 被显式设置且其指向的文件不存在时，应直接返回清晰错误（提示检查路径/挂载/密钥文件是否已就绪），而不是自动生成并写入新密钥；仅在未配置 KEY_FILE、走默认 .secret-key 路径时保留现有的自动创建引导逻辑。
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 {1 participant
问题
实例 Secret 直接写入 SQLite 的
secret_json字段，仅依赖数据库文件权限保护。影响
数据库备份、WAL、快照、磁盘镜像或容器挂载目录泄露时，API Key、密码和其他凭证可以被直接恢复。
修复内容
0600的密钥文件；生产环境可通过OCTOBUS_SECRET_ENCRYPTION_KEY提供 base64 编码的 32 字节密钥。SecretJSON接口不变。验证
go test ./internal/store ./internal/supervisor通过。