Conversation
The encryption design's Jepsen gate is explicit that NO new workload is required: encryption is consistency-transparent, so the acceptance gate is the existing Redis and DynamoDB workloads passing against an encrypted cluster. What was missing was the ability to stand that cluster up. --encryption provisions the §5.1 KEK file on each node with owner-only permissions and starts the server with --encryption-enabled, --kekFile and --encryptionSidecarPath. The three travel together because a sidecar path alone only enables read-only capability probing, while the mutating RPCs a bootstrap needs require the flag and a KEK source too. The KEK is a fixed test value rather than a generated one: every node must unwrap the same sidecar, and a per-node random KEK would fail startup with ErrKEKMismatch — a far more confusing failure than a hardcoded test key. The flag lives in common-cli-opts, so a future workload inherits the gate without opting in, and defaults off so existing runs measure the same thing they did before. server-args is extracted from start-node! as a pure function purely so the flag set is testable without SSH. That matters more than it looks: a --encryption run that silently produced an UNENCRYPTED cluster would report PASS and be recorded as evidence for this gate, which is worse than having no gate. The first version of these tests did not pin it — dropping the flags from db.clj left them all green. Full suite: 157 tests, 0 failures. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
|
@claude review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
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 |
|
Claude finished @bootjp's task in 3m 39s —— View job Review:
|
![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: 395f7d6259
ℹ️ 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".
| (when (:encryption opts) | ||
| (provision-kek! node)) |
There was a problem hiding this comment.
When --encryption is used with a fresh Jepsen data directory, this branch only provisions a KEK before starting the nodes. The server's --encryption-enabled flag merely enables the encryption mutator RPCs, while buildEncryptionWriteWiring deliberately keeps writes cleartext until both BootstrapEncryption and EnableStorageEnvelope have applied; nothing in setup! invokes either operation. Consequently the Redis and DynamoDB acceptance runs can report PASS while all workload data was stored unencrypted. After cluster membership is established, the harness must bootstrap the DEKs, perform the storage-envelope cutover, and wait for it to apply before beginning the workload.
Useful? React with 👍 / 👎.
| [nil "--encryption" "Run against a cluster with data-at-rest encryption enabled." | ||
| :default false] |
There was a problem hiding this comment.
Because this option is added to common-cli-opts, the S3, SQS, DynamoDB-types, DynamoDB-multi-table, and Redis-zset entrypoints all accept --encryption; however, their test constructors omit :encryption when building ekdb/db (a repo-wide search shows only redis_workload.clj and dynamodb_workload.clj propagate it). Those commands therefore silently launch an unencrypted cluster despite the CLI promise. Either propagate the option through every workload using the common options or reject/remove it from unsupported entrypoints.
Useful? React with 👍 / 👎.
Two review findings, both of which made the §8.4 gate report success on a cleartext cluster — the exact failure this suite's own docstring calls "worse than no gate at all, because it would be recorded as evidence". P1 — setup! provisioned a KEK and started the nodes, and nothing else. --encryption-enabled only opens the EncryptionAdmin mutator RPCs; buildEncryptionWriteWiring deliberately keeps the store's envelope gate closed until BOTH BootstrapEncryption and EnableStorageEnvelope have applied (the gate reads cache.StorageEnvelopeActive). Every "encrypted" Redis and DynamoDB run therefore stored cleartext and could still PASS. setup! now bootstraps the DEKs and performs the §7.1 Phase-1 storage cutover after membership is established — after, because the writer batch needs a registry entry per member (§5.6 step 1a) and the cutover's capability gate requires every voter to report capable. It then polls every node until it reports storage_envelope_active and FAILS SETUP if any node does not, so the workload can never start against a cluster that is still writing cleartext. Bootstrap takes the KEK-wrapped DEK bytes as an argument, which an operator gets from their KMS and the harness has no way to produce. The new cmd/jepsen-encryption-setup generates a DEK, wraps it under the harness KEK file, and prints only the wrapped form — the plaintext never leaves the process. It is harness tooling on purpose: putting DEK generation into elastickv-admin would add operator-facing handling of raw key material, which needs its own design and review. The orchestration itself uses the shipped elastickv-admin subcommands, so the run exercises the real operator path. P2 — --encryption lives in common-cli-opts so that, as this suite already claimed, "a future workload gets the gate without opting in". Five entrypoints (s3, sqs-htfifo, dynamodb-types, dynamodb-multi-table, redis-zset-safety) accepted the flag and then dropped it when building ekdb/db, so the CLI advertised a guarantee it did not provide. All five now thread it, and one table-driven test drives every workload constructor so the next one fails here instead of shipping a silent lie. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Both findings are correct and fixed in 2d280f2. The P1 is the failure mode this suite's own namespace docstring names — "a run that reported PASS while the cluster was never actually encrypted would be worse than no gate at all, because it would be recorded as evidence" — so it is exactly the thing the PR was supposed to prevent and didn't. P1 — Bootstrap and activate encryption before running workloads. Confirmed by tracing it rather than taking it as read:
So every "encrypted" Redis and DynamoDB run stored cleartext and could still pass.
Bootstrap takes the KEK-wrapped DEK bytes as an argument — an operator gets them from their KMS, and the harness had no way to produce them. Nothing in the repo wraps a DEK from a KEK file. New A related gap this surfaced, not in scope here: there is no documented way for an operator using P2 — Do not advertise encryption to workloads that drop the option. Confirmed; the audit matches yours exactly — only Verification
One honest limitation: the unit tests cover the argv construction and the status parsing, not the SSH orchestration — |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @bootjp's task in 4m 52s —— View job Review:
|
1 participant
What §8.4 actually asks for
I went looking for an "encrypted Jepsen workload" and found the design says the opposite:
So the missing piece was never a workload — it was the ability to stand the cluster up encrypted.
--encryptiondoes that: it provisions the §5.1 KEK file on each node with owner-only permissions and starts the server with--encryption-enabled,--kekFileand--encryptionSidecarPath.Decisions worth reviewing
All three flags travel together. A sidecar path alone only enables read-only capability probing; the mutating RPCs a bootstrap needs require
--encryption-enabledand a KEK source. Two of the three yields either a refusal to start or, worse, a cluster that starts unencrypted.The KEK is a fixed test value, not generated. Every node must unwrap the same sidecar — a per-node random KEK fails startup with
ErrKEKMismatch, which is a much more confusing failure to debug than a hardcoded test key is to notice.The flag lives in
common-cli-optsso a future workload inherits the gate without opting in, and defaults off so existing runs measure exactly what they did before.The test that wasn't testing anything
My first version of these tests passed with
db.cljdropping the encryption flags entirely — i.e. it would have green-lit a--encryptionrun that produced an unencrypted cluster, reported PASS, and been recorded as evidence for this acceptance gate. That is strictly worse than having no gate.Fixed by extracting
server-argsfromstart-node!as a pure function, purely so the flag set is assertable without SSH. Dropping the flags now fails 3 tests.Test evidence
db.cljdrops the flags → 3 failures (this is the one that initially passed)9 tests: flag availability and default, propagation through both workloads named in §8.4,
ekdb/dbcarrying the option, the three-flag emission, absence when unrequested, and that enabling it disturbs no other argv entry.Behavior change / risk
Test-harness only; no production code. Default off, so every existing run is byte-identical.
Not included: wiring
--encryptionthrough the remaining workloads (S3, SQS, multi-table). §8.4 names Redis and DynamoDB as the gate, and the flag is incommon-cli-opts, so extending is a one-line change per workload if you want it.Self-review (five passes)
start-node!, since startup guards refuse a missing KEK before anything a workload could observe.https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE