Add CRC32 integrity check for cluster bus messages#4201
Open
enjoy-binbin wants to merge 3 commits into
Open
Conversation
The cluster bus only checks the magic string and length, so a corrupted gossip message (e.g. a bit flip from NIC/switch faults) is still accepted. The receiver then updates cluster metadata from the bad data: a wrong configEpoch (the rule is max(new, old), so a larger bogus value can never be corrected and scrambles slot ownership), a phantom node created from a corrupted nodename, or an inconsistent slot bitmap. Add a uint32_t crc field to the message header holding a CRC32 of the whole message, computed by the sender and verified by the receiver. When the field is 0 the sender has CRC disabled, so the receiver skips the check; old nodes never set the field, giving the same effect and keeping mixed/old peers compatible. When the CRC check fails, the receiver discards the packet. It should be noted that if the receiver consistently discards packets, the node may enter the PFAIL/FAIL state. The check is gated by the new cluster-crc-enabled configuration, the default is no. If disabled, no CRC check will be performed. The CRC seed is derived from requirepass, so only nodes with the same password verify, which also isolates distinct clusters. New CLUSTER INFO field: cluster_stats_messages_crc_mismatch. New DEBUG cluster-crc-flip-bit / cluster-crc-flip-time commands. Signed-off-by: Binbin <binloveplay1314@qq.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesCluster bus messages now support optional CRC32 checksums. The protocol layout, checksum implementation, build wiring, configuration, send/receive validation, mismatch statistics, and DEBUG fault-injection controls are updated. Cluster CRC32 support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant clusterSendMessage
participant clusterMsgFinalizeCRC
participant crc32
participant clusterIsValidPacket
participant clusterState
clusterSendMessage->>clusterMsgFinalizeCRC: finalize full-header message
clusterMsgFinalizeCRC->>crc32: compute seeded CRC32
crc32-->>clusterMsgFinalizeCRC: return checksum
clusterMsgFinalizeCRC-->>clusterSendMessage: send message with CRC
clusterIsValidPacket->>crc32: verify received CRC32
crc32-->>clusterIsValidPacket: return checksum
clusterIsValidPacket->>clusterState: increment mismatch counter on failure
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cluster_legacy.c`:
- Around line 3941-3945: Update the CRC validation path in clusterMsgVerifyCRC,
before any access or modification of hdr->crc, to verify that the received
packet length is large enough to contain the CRC field at its defined offset.
Reject undersized or unrecognized packets without performing CRC verification,
while preserving normal verification for sufficiently large packets.
In `@src/debug.c`:
- Around line 655-657: Update the cluster-crc-flip-bit branch in the debug
command handler to parse its argument with getLongFromObjectOrReply() instead of
atoi, propagating a reply on invalid or out-of-range input and assigning the
validated result to server.debug_cluster_crc_flip_bit. Match the existing
CLUSTER-CRC-FLIP-TIME implementation’s parsing behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79ac1c9f-fba1-48a6-aa26-238a1381a09a
📒 Files selected for processing (10)
cmake/Modules/SourceFiles.cmakesrc/Makefilesrc/cluster.csrc/cluster_legacy.csrc/cluster_legacy.hsrc/config.csrc/crc32.csrc/debug.csrc/server.csrc/server.h
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4201 +/- ##
============================================
- Coverage 76.81% 76.80% -0.01%
============================================
Files 162 163 +1
Lines 81455 81527 +72
============================================
+ Hits 62570 62618 +48
- Misses 18885 18909 +24
🚀 New features to boost your workflow:
|
Signed-off-by: Binbin <binloveplay1314@qq.com>
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| static uint32_t clusterCrcSeed(void) { | ||
| if (server.requirepass == NULL || sdslen(server.requirepass) == 0) return 0; | ||
|
|
||
| return crc32(0, (const unsigned char *)server.requirepass, sdslen(server.requirepass)); |
There was a problem hiding this comment.
server.requirepass is not a stable cluster-wide secret. It is a per-node, modifiable ACL setting (src/config.c:3428-3429, src/server.h:2344-2346), and the cluster bus itself just connects and sends PING/MEET without any password negotiation (src/cluster_legacy.c:4711-4733, src/cluster_legacy.c:6308-6323). Using it as the CRC seed means a normal rolling CONFIG SET requirepass changes the seed on one node immediately, so once cluster-crc-enabled is on that node starts rejecting full-header packets from peers that have not been updated yet and can drive the cluster into PFAIL/FAIL. The seed needs to come from a dedicated cluster-wide secret/capability, or the CRC should stay unkeyed.
This reverts commit 6e8fa3a. See isClusterMsgSignatureAndLengthValid. Signed-off-by: Binbin <binloveplay1314@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The cluster bus only checks the magic string and length, so
a corrupted gossip message (e.g. a bit flip from NIC/switch
faults) is still accepted. The receiver then updates cluster
metadata from the bad data: a wrong configEpoch (the rule is
max(new, old), so a larger bogus value can never be corrected
and scrambles slot ownership), a phantom node created from a
corrupted nodename, or an inconsistent slot bitmap.
Add a uint32_t crc field to the message header holding a CRC32
of the whole message, computed by the sender and verified by
the receiver. When the field is 0 the sender has CRC disabled,
so the receiver skips the check; old nodes never set the field,
giving the same effect and keeping mixed/old peers compatible.
When the CRC check fails, the receiver discards the packet. It
should be noted that if the receiver consistently discards packets,
the node may enter the PFAIL/FAIL state.
The check is gated by the new cluster-crc-enabled configuration,
the default is no. If disabled, no CRC check will be performed.
The CRC seed is derived from requirepass, so only nodes with the
same password verify, which also isolates distinct clusters.
New CLUSTER INFO field: cluster_stats_messages_crc_mismatch.
New DEBUG cluster-crc-flip-bit / cluster-crc-flip-time commands.