CBL-8891: Rekey is not atomic between database and attachments by jianminzhao · Pull Request #2544 · couchbase/couchbase-lite-core

jianminzhao · 2026-10-01T00:04:26Z

rekey() now provides fail-atomicity: after rekey() returns, completes, or is interrupted by a crash at any point, the database is guaranteed to be openable with either the pre-rekey key or the target rekey key.
Caveat: it's fail-atomic under process crash; under power loss, durability is limited by LiteCore's existing lack of fsync for blob files and renames.

DatabaseImpl::rekey() rekeys the SQLite database and the attachments BlobStore as two separate operations with no atomicity between them. A crash (or any exception) between the database rekey succeeding and the attachments swap finishing left the two permanently out of sync: the database durably on the new key, the attachments still on the old one, with no way to tell afterward that anything was wrong.

We fix it by rekeying the attachments to a staging directory before rekeying the db, and swap it to right place after the SQLite db has successfully rekeyed. If the process faulted before the swapping, the staged directory will be left on the disk and swapped next time the db is opened.

rekey() now provides fail-atomicity: after rekey() returns, completes, or is interrupted by a crash at any point, the database is guaranteed to be openable with either the pre-rekey key or the target rekey key. DatabaseImpl::rekey() rekeys the SQLite database and the attachments BlobStore as two separate operations with no atomicity between them. A crash (or any exception) between the database rekey succeeding and the attachments swap finishing left the two permanently out of sync: the database durably on the new key, the attachments still on the old one, with no way to tell afterward that anything was wrong. We fix it by rekeying the attachments to a staging directory before rekeying the db, and swap it to right place after the SQLite db has successfully rekeyed. If the process faulted before the swapping, the staged directory will be left on the disk and swapped next time the db is opened.

cbl-bot · 2026-10-01T18:23:49Z

Code Coverage Results:

Type	Percentage
branches	64.61
functions	77.52
instantiations	71.15
lines	76.04
regions	72.21

Copilot

Copilot review overview

🟡 Changes recommended

A crash or exception after SQLite rekeying can still leave the database and attachments permanently out of sync.

Review effort: Balanced
Findings: 2 · 1 · 1

Open (4)

Staged store identity does not verify database ownership · New
Crash can leave database and attachments encrypted with different keys · New
Read-only opens mutate or delete staged attachment stores · New
Missing fault-injection tests for rekey recovery states · New

What changed in this PR

Introduces staged attachment-store recovery to make database rekeying fail-atomic.

Changes:

Stages rekeyed attachments and completes pending swaps during database open.
Identifies staged stores using a target-key hash.
Preserves existing blob-store object identity during replacement.

File	Description
`LiteCore/Database/DatabaseImpl.hh`	Declares pending blob-store recovery.
`LiteCore/Database/DatabaseImpl.cc`	Implements staging, recovery, and attachment replacement.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Previously rekey() renamed the rekeyed attachments into their "Attachments_staged-<hash>" directory after the database rekey had committed. A crash between the two steps left the database on the new key with the attachments still on the old one, and nothing on disk for the next open to recover from. Now the rename happens first, so the database rekey is the single commit point: - crash before it: the database is still on the old key, the staged directory's name doesn't match, and the next open discards it; - crash after it: the database is on the new key, the name matches, and the next open installs it. The "Attachments_rekeying" scratch directory is kept so a partial copyBlobsTo never carries the staged name (it would otherwise be installed if the new key equals the current one). If the rename or the database rekey throws, both directories are removed. Update the comments and the finishPendingBlobStoreSwap() documentation to match. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

pasin · 2026-10-02T21:54:20Z

+    // a single database might ever have used.
+    static string stagedBlobDirName(const C4EncryptionKey& key) {
+        SHA256Builder digest;
+        digest << uint8_t(key.algorithm) << slice(key.bytes, kEncryptionKeySize[key.algorithm]);


Even with SHA-256, I think deriving the staged directory name from the encryption key is a security concern.

I think a fixed directory name can be used, and to check that the blobs were written with the current key, we can decrypt one blob and compare its SHA-1 to its filename?

I agree: using just a digest of the key is vulnerable to a dictionary attack.

snej

I think the attachments should be re-encrypted before rekeying the database. Something like:

Take exclusive db lock
Create a new attachments dir "Attachments_rekeyed" and copy the blobs into it encrypted with the new key
Rekey the SQLite database
Move "Attachments" to a temporary directory
Rename "Attachments_rekeyed" to "Attachments"
Release database lock
Asynchronously delete the old attachments directory

...Hm, according to the Jira ticket, those steps are how it's currently done.

So what needs to change is to check for the rekeyed attachments dir when opening the database. If it exists, try opening some attachment in it; if it succeeds continue from step 4 or 5, else delete it (there must have been a crash before step 3 finished.)

snej · 2026-10-05T17:24:30Z

+    // a single database might ever have used.
+    static string stagedBlobDirName(const C4EncryptionKey& key) {
+        SHA256Builder digest;
+        digest << uint8_t(key.algorithm) << slice(key.bytes, kEncryptionKeySize[key.algorithm]);


I agree: using just a digest of the key is vulnerable to a dictionary attack.

- Name the staged directory "Attachments_staged" instead of a key-derived hash. On open, check that a blob in it decrypts with the database's current key: if so, install it (the rekey committed); if not, discard it (the rekey was interrupted before the database rekey committed). C4BlobStore::validateEncryption() does the check, by comparing a blob's SHA-1 against the digest in its filename. - Never modify files when the database is read-only. A leftover staged directory that is not needed is ignored; one that must be installed logs a warning and throws NotWriteable. - Add the "Database Rekey Atomicity" test, which recreates the on-disk state left by a crash at each point of rekey() and checks that the database opens only with the key it is really on, with its attachments readable.

Copilot

🟡 Changes recommended

Empty attachment stores and post-commit exceptions can still leave database and blob encryption keys inconsistent.

2 open findings

Preserve staged store after rekey exceptions · New
Crash can leave database and attachments encrypted with different keys

3 resolved since last review

Staged store identity does not verify database ownership
Read-only opens mutate or delete staged attachment stores
Missing fault-injection tests for rekey recovery states

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Empty stores and recovery failures can leave stale encryption state or delete the only usable staged attachments.

2 open findings

Preserve staged store on operational swap failures · New
Synchronize blob store key after rekey reopen failure · New

2 resolved since last review

Preserve staged store after rekey exceptions
Crash can leave database and attachments encrypted with different keys

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Empty blob stores retain the old encryption key after a successful rekey.

1 open finding

Empty rekey stage leaves existing blob store using the old key · New

2 resolved since last review

🧠 Review effort: Balanced

Copilot

🔵 Needs a closer look

Failed staged-store installation can permit data loss, and encryption-transition recovery remains untested.

0 open findings

1 resolved since last review

Empty rekey stage leaves existing blob store using the old key

Previously missed (1)

In code that hasn't changed since last review

Test interrupted AES-to-none and none-to-AES rekey recovery

C/tests/c4DatabaseEncryptionTest.cc:167

The existing Database Rekey test only covers uninterrupted encryption/decryption, so it does not cover this recovery protocol for AES -> none or none -> AES. Those paths are materially different because validateEncryption interprets plaintext as encrypted data (and vice versa), especially for empty or short blobs. Add interrupted-before-commit and interrupted-after-commit reopen cases for both transitions rather than skipping the unencrypted variant.

🧠 Review effort: Balanced

jianminzhao force-pushed the cbl-8891 branch from fc92d30 to e49d09c Compare October 1, 2026 17:37

jianminzhao requested review from pasin and snej and a balanced review from Copilot October 1, 2026 20:28

Copilot started reviewing on behalf of jianminzhao October 1, 2026 20:28

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread LiteCore/Database/DatabaseImpl.cc Outdated

Comment thread LiteCore/Database/DatabaseImpl.cc Outdated

Comment thread LiteCore/Database/DatabaseImpl.cc

Comment thread LiteCore/Database/DatabaseImpl.cc

pasin reviewed Oct 2, 2026

View reviewed changes

snej reviewed Oct 5, 2026

View reviewed changes

jianminzhao added 4 commits October 6, 2026 18:16

Merge branch 'master' into cbl-8891

181ae4f

Clant Linting

94a9290

Fix test failures on Linux

e7467de

jianminzhao requested review from pasin and snej and a balanced review from Copilot October 9, 2026 01:16

Copilot started reviewing on behalf of jianminzhao October 9, 2026 01:17

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

Comment thread LiteCore/Database/DatabaseImpl.cc Outdated

addressing Copilot's comment.

094abe5

jianminzhao requested a balanced review from Copilot October 9, 2026 06:53

Copilot started reviewing on behalf of jianminzhao October 9, 2026 06:53

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

Comment thread C/c4BlobStore.cc Outdated

Comment thread LiteCore/Database/DatabaseImpl.cc Outdated

addressing Copilot's comments

380544f

jianminzhao requested a balanced review from Copilot October 9, 2026 17:41

Copilot started reviewing on behalf of jianminzhao October 9, 2026 17:42

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

Comment thread LiteCore/Database/DatabaseImpl.cc Outdated

addressing Copilot's new comments.

318317b

jianminzhao requested a balanced review from Copilot October 9, 2026 18:01

Copilot started reviewing on behalf of jianminzhao October 9, 2026 18:02

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

snej approved these changes Oct 14, 2026

View reviewed changes

jianminzhao merged commit 3377eeb into master Oct 15, 2026
10 checks passed

jianminzhao deleted the cbl-8891 branch October 15, 2026 18:47

Conversation

jianminzhao commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbl-bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pasin Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

snej Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

snej left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snej Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jianminzhao commented Oct 1, 2026 •

edited

Loading

cbl-bot commented Oct 1, 2026 •

edited

Loading

snej left a comment •

edited

Loading