Add production KEK providers#1113
Conversation
|
Stage 9B verification completed on signed head
@codex review |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughStage 9BとしてAWS KMS、GCP KMS、Vault Transit、環境変数のKEKプロバイダとソース選択を追加し、起動時のKEKロード状態を暗号化ミューテータのゲートへ接続しました。関連する依存関係、テスト、設計文書も更新されています。 ChangesKEKプロバイダとソース解決
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant KEKSourceLoader
participant KEKProvider
participant EncryptionAdmin
Startup->>KEKSourceLoader: file、URI、環境変数からKEKをロード
KEKSourceLoader->>KEKProvider: provider wrapperを生成
KEKProvider-->>KEKSourceLoader: ロード済みwrapper
KEKSourceLoader-->>Startup: kekConfiguredを返す
Startup->>EncryptionAdmin: loaded KEK状態でmutator gateを設定
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
✅ Action performedReview finished.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Stage 9B of the data-at-rest encryption design, adding support for AWS KMS, GCP KMS, Vault Transit, and a test/CI-only environment-backed KEK provider. It updates the startup wiring and mutator gates to use the loaded KEK wrapper state instead of relying solely on the legacy file flag. Feedback is provided regarding a performance improvement in the GCP KMS wrapper, suggesting that the Castagnoli CRC32 table be defined as a package-level global variable to avoid recreating it on every checksum calculation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Latest signed head |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/encryption/kek/vault_test.go (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueサブテスト（
t.Run）を使用してテーブル駆動テストの可読性を向上させるコーディングガイドラインに従い、テーブル駆動テストの反復処理を
t.Runでラップすることをお勧めします。♻️ 提案するリファクタリング
for _, target := range []string{"", "transit", "transit//key", "../key", "transit/../key"} { - _, _, err := parseVaultTarget(target) - require.ErrorIsf(t, err, ErrInvalidKEKURI, "target=%q", target) + t.Run(target, func(t *testing.T) { + _, _, err := parseVaultTarget(target) + require.ErrorIsf(t, err, ErrInvalidKEKURI, "target=%q", target) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/encryption/kek/vault_test.go` around lines 52 - 56, Wrap each invalid target iteration in the table-driven test around parseVaultTarget with t.Run, using the target value to identify the subtest. Keep the existing ErrInvalidKEKURI assertion and target-specific failure context unchanged.Source: Coding guidelines
internal/encryption/kek/source_test.go (1)
48-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueサブテスト（
t.Run）を使用してテーブル駆動テストの可読性を向上させるコーディングガイドラインに従い、テーブル駆動テストの各ケースを
t.Runでラップすることで、テスト失敗時の出力が明確になり、個別のケースを独立して実行できるようになります。♻️ 提案するリファクタリング
for _, uri := range []string{ "unknown://key", "aws-kms://not-an-arn", "gcp-kms://projects/incomplete", "vault-transit://transit", } { - _, err := newURIWrapper(context.Background(), uri) - require.ErrorIsf(t, err, ErrInvalidKEKURI, "uri=%q", uri) + t.Run(uri, func(t *testing.T) { + _, err := newURIWrapper(context.Background(), uri) + require.ErrorIsf(t, err, ErrInvalidKEKURI, "uri=%q", uri) + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/encryption/kek/source_test.go` around lines 48 - 56, Wrap each URI iteration in the table-driven test with t.Run, using the URI as the subtest name, and move the newURIWrapper and ErrInvalidKEKURI assertion into that subtest so failures identify the specific invalid URI and cases can run independently.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/encryption/kek/aws_kms.go`:
- Around line 33-36:
更新処理のARN検証で、parsed.Resourceが「key/」または「alias/」だけの場合もErrInvalidKEKURIを返すよう、各接頭辞の後に空でない識別子が存在することを確認してください。既存のAWS
KMS ARN検証条件は維持し、aws_kms_test.goの検証ケースに空のkey/およびalias/リソースを追加してください。
---
Nitpick comments:
In `@internal/encryption/kek/source_test.go`:
- Around line 48-56: Wrap each URI iteration in the table-driven test with
t.Run, using the URI as the subtest name, and move the newURIWrapper and
ErrInvalidKEKURI assertion into that subtest so failures identify the specific
invalid URI and cases can run independently.
In `@internal/encryption/kek/vault_test.go`:
- Around line 52-56: Wrap each invalid target iteration in the table-driven test
around parseVaultTarget with t.Run, using the target value to identify the
subtest. Keep the existing ErrInvalidKEKURI assertion and target-specific
failure context unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cb05f6a-f289-4958-92a5-ab6484aef3a4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
docs/design/2026_04_29_partial_data_at_rest_encryption.mddocs/design/2026_07_18_implemented_9b_kek_providers.mdgo.modinternal/encryption/errors.gointernal/encryption/kek/aws_kms.gointernal/encryption/kek/aws_kms_test.gointernal/encryption/kek/env.gointernal/encryption/kek/env_test.gointernal/encryption/kek/gcp_kms.gointernal/encryption/kek/gcp_kms_test.gointernal/encryption/kek/kek.gointernal/encryption/kek/provider.gointernal/encryption/kek/source.gointernal/encryption/kek/source_test.gointernal/encryption/kek/vault.gointernal/encryption/kek/vault_test.gointernal/encryption/startup.gomain.gomain_encryption_admin.gomain_encryption_kek_source_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68bcf1500b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Signed head 6d59ccc addresses all current review findings. KEK providers now receive a real startup wrap and unwrap preflight before encryption mutators are exposed; AWS accepts only immutable non-empty key ARNs; Vault validates a positive version and non-empty ciphertext payload; and the two table-driven test loops now use named subtests. Full go test ./..., race tests for the encryption packages and root package, targeted golangci-lint, the full commit lint hook, and git diff --check passed. Startup, mutator-gate, Wrap, and Unwrap callers were audited; no persisted or wire format changed. @codex review @coderabbitai review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Signed head |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
b3acb83 to
39142ba
Compare
78f24a9 to
011aeae
Compare
|
Rebased the four Stage 9B commits onto the sanitized #1058 head Evidence:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 011aeaebad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Latest signed head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 011aeaebad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Signed head |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
39142ba to
04bfde5
Compare
0eb8c70 to
bda3501
Compare
|
Rebased the five signed Stage 9B commits onto current #1058 head |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
04bfde5 to
e34b4de
Compare
1 participant
Summary
Safety and compatibility
Caller audit
KEKConfigurednow derives from the loaded wrapper and reaches both startup refusal and the EncryptionAdmin mutator gateVerification
go test ./... -count=1 -timeout=20mgo test -race ./internal/encryption/kek . -run ... -count=1 -timeout=15mgolangci-lint --allow-parallel-runners --config=.golangci.yaml run ./... --timeout=10m(0 issues)git diff --checkRisk
Remote provider availability and credentials are now startup dependencies when those providers are selected. Each remote operation has a 30-second deadline and startup fails closed when existing wrapped DEKs cannot be unwrapped.
Author: bootjp
Summary by CodeRabbit
新機能
改善