feat!: replace dead key-rotation machinery with keyring decrypt helper (LAB-683) - #67
Conversation
…r (LAB-683) Deletes the rotation dead code the LAB-275 audit flagged as a trust bug and implements the cachekit-core half of the keyring decision (protocol/decisions/key-rotation.md, merged via protocol#34): - Remove ZeroKnowledgeEncryptor::rotate_key() NotImplemented stub and the EncryptionError::NotImplemented variant - Delete src/encryption/key_rotation.rs whole (KeyRotationState, RotationAwareHeader) and the EncryptionHeader alias + re-exports - Add encryption::keyring::Keyring: one current master key + at most 3 decrypt-only keys; sequential decrypt current-first with identical AAD per attempt; per-entry HKDF-derived encryption-key fingerprints for fingerprint-based selection; cap and forward-only self-collision rejected at construction; all key material zeroized on drop BREAKING CHANGE: public items EncryptionHeader, KeyRotationState, RotationAwareHeader, and ZeroKnowledgeEncryptor::rotate_key() are removed (EncryptionError::NotImplemented variant included). Use Keyring for master-key rotation.
This comment has been minimized.
This comment has been minimized.
WalkthroughThe encryption module replaces rotation-aware state with a decrypt-only keyring. The keyring validates master keys, derives tenant keys, supports ordered decryption fallback, updates error mappings, and changes public exports and architecture documentation. ChangesMaster-key decrypt keyring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Keyring
participant ZeroKnowledgeEncryptor
participant AESGCM
Caller->>Keyring: decrypt ciphertext for tenant with AAD
Keyring->>ZeroKnowledgeEncryptor: derive per-entry tenant key
Keyring->>AESGCM: try current key
AESGCM-->>Keyring: authentication failure
Keyring->>AESGCM: try ordered decrypt-only key
AESGCM-->>Keyring: plaintext or terminal error
Keyring-->>Caller: return decryption result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/encryption/mod.rs`:
- Around line 15-20: Update Keyring::new to accept only master keys with exactly
32 bytes, rejecting both shorter and longer inputs; update
InvalidMasterKeyLength to represent the fixed 32-byte requirement. Apply the
public API change consistently at src/encryption/mod.rs lines 15-20 and
src/lib.rs lines 83-84, preserving both re-export paths.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0cbc0994-1e12-4eec-ae44-a8c30b5d7565
📒 Files selected for processing (7)
README.mdsrc/encryption/core.rssrc/encryption/key_rotation.rssrc/encryption/keyring.rssrc/encryption/mod.rssrc/ffi/error.rssrc/lib.rs
💤 Files with no reviewable changes (1)
- src/encryption/key_rotation.rs
…l findings) Panel findings applied (LAB-683 crypto/protocol review gate): - Key-derivation failure and decrypt_at out-of-range index no longer fold into DecryptionFailed (same FFI code as AuthenticationFailed): new KeyDerivation(#[from] KeyDerivationError) and KeyringIndexOutOfRange variants map to input/config errors so fail-open SDK policies cannot mistake a bad tenant_id or a binding indexing bug for a cache miss (LAB-241 signal-ambiguity class) - TenantKeys: manual Debug impl redacting key material (CWE-215); prints tenant_id + encryption fingerprint only - Cut unused public surface before the SDK children copy the API: decrypt_only_count() deleted, entry_count() now private (bindings use encryption_fingerprints().len()) - decrypt()/decrypt_at() # Errors docs now list every terminal variant
This comment has been minimized.
This comment has been minimized.
Expert-panel review record (crypto/protocol gate, LAB-683)Panel ran at critical-stakes (foundational crypto library, all three SDKs consume this API next): bug-hunter-supreme, security-specialist, code-craftsman, catchphrase-agent, all reviewing the full diff against the normative spec ( Verdict: SHIP (after fixes in 74f85d2). No CRIT findings. Spec conformance verified clean by two agents independently: cap-3 reject-never-truncate, forward-only self-collision rejection, fingerprint over the HKDF-derived per-tenant key (byte-verified against Findings applied (commit 74f85d2)
Findings rejectedNone — every surviving finding was applied. All gates re-run green after fixes: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/encryption/keyring.rs`:
- Around line 171-174: Update the decrypt_at error documentation to list
EncryptionError::InvalidCiphertext as a separate terminal error for malformed
ciphertext, while limiting EncryptionError::AuthenticationFailed to
authentication failures; preserve the existing behavior in decrypt_aes_gcm and
the unchanged error propagation in decrypt_at.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3fe5788b-def6-4f68-9f12-e505a34e9856
📒 Files selected for processing (4)
src/encryption/core.rssrc/encryption/key_derivation.rssrc/encryption/keyring.rssrc/ffi/error.rs
decrypt_at passes EncryptionError::InvalidCiphertext through unchanged from the underlying AES-GCM decrypt; documenting it as a parenthetical under AuthenticationFailed misstated which variant callers receive.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@coderabbitai review |
|
Summary
This PR replaces the previously unimplemented key-rotation machinery with a functional multi-key decrypt keyring that supports master-key rotation through a grace window. This is a breaking change (
feat!).What Changed
Removed dead code
key_rotation.rsmodule, includingKeyRotationState,RotationAwareHeader, and theEncryptionHeadertype alias.rotate_key()stub method onZeroKnowledgeEncryptor, which had always returned aNotImplementederror.NotImplementederror variant and related header roundtrip/algorithm tests.Added new keyring functionality
keyring.rsmodule with aKeyringtype that holds one current master key (used for both encryption and decryption) plus an ordered list of up toMAX_DECRYPT_ONLY_KEYS(3) decrypt-only keys retained during rotation.decrypt_at()for fingerprint-based key selection andencryption_fingerprints()for matching against per-tenant derived-key fingerprints.Enforced construction invariants
Three new error variants validate keyring configuration:
InvalidMasterKeyLength— every key must be at least 16 bytes.KeyringCapExceeded— rejects (never truncates) more than 3 decrypt-only keys.CurrentKeyInDecryptOnlyList— enforces the forward-only rotation rule (a key that once encrypted is never re-promoted).Supporting updates
lib.rsandmod.rsto exposeKeyringandMAX_DECRYPT_ONLY_KEYS.README.mdarchitecture diagram and file listing to reflect the keyring-based rotation model.Why
The prior key-rotation code was a non-functional stub. This change delivers a working rotation strategy where rotation state is treated as configuration rather than a state machine — the ciphertext format and AAD carry no key identity, so nothing on the wire changes. Values encrypted under a retiring key remain readable as long as that key stays in the decrypt-only list, enabling zero-downtime master-key rotation with a grace window.
Breaking Changes
KeyRotationState,RotationAwareHeader,EncryptionHeader, andZeroKnowledgeEncryptor::rotate_key().EncryptionError::NotImplementedvariant.Summary
This PR refines the keyring decryption helper by introducing distinct, semantically-precise error types that prevent configuration errors and caller bugs from being misinterpreted as cache misses under fail-open policies.
Key Changes
New distinct error variants
EncryptionError::KeyringIndexOutOfRange { index, count }to explicitly signal an out-of-range keyring entry index (a caller bug), replacing the previous genericDecryptionFailedstring error.EncryptionError::KeyDerivation(wrappingKeyDerivationError) to surface key-derivation failures (e.g. an invalid/emptytenant_id) as a configuration error rather than a decryption failure.Prevention of fail-open misclassification
The core motivation is safety: previously, an out-of-range index or a bad
tenant_idwould surface asDecryptionFailed, which a fail-open SDK could mistake for a legitimate cache miss. Now these terminal errors are kept distinct fromAuthenticationFailed/DecryptionFailed, so a misconfiguration cannot masquerade as a miss.FFI error mapping
KeyringIndexOutOfRangemaps toCachekitError::InvalidInput.KeyDerivationdelegates to the underlyingKeyDerivationErrorconversion.API surface reduction
Keyring::entry_count()private (bindings can derive the count fromencryption_fingerprints().len()).decrypt_only_count()method.Security hardening for logging
Debugimpl onTenantKeyswith a manual one that redacts key material (CWE-215), printing only thetenant_idand encryption-key fingerprint.Tests
KeyringIndexOutOfRangevariant.test_bad_tenant_id_is_config_error_not_missverifying an emptytenant_idyieldsKeyDerivationrather thanAuthenticationFailed.test_entry_countstest.Breaking Change
Marked
feat!— this removes the publicentry_count()anddecrypt_only_count()methods and changes the error variants returned by keyring decryption operations.Based on the code changes provided, here's a description for this pull request:
Description
This PR updates the documentation for the
decrypt_atmethod in theKeyringimplementation, clarifying the distinction between two different error conditions that can occur during decryption.What Changed
The doc comment for
decrypt_atwas revised to separate two previously conflated error cases:AuthenticationFailedwas documented as covering both authentication failures and structural ciphertext errors (likeInvalidCiphertext) from the underlying AES-GCM decrypt.AuthenticationFailed— when the entry's key does not authenticate the ciphertext.InvalidCiphertext— for malformed ciphertext.Purpose
This change improves the accuracy of the API documentation by clearly distinguishing between an authentication failure (wrong key) and a structural/malformed ciphertext error, making the contract for callers of
decrypt_atclearer and more precise.Note on Context
The PR title (
feat!: replace dead key-rotation machinery with keyring decrypt helper (LAB-683)) suggests a broader set of changes involving replacement of key-rotation machinery. However, the single diff provided only contains a documentation comment update todecrypt_at, so this description is limited to the code change shown.Summary by CodeRabbit
New Features
Documentation