feat(crypto): v2 volume encryption #3438
Conversation
|
Warning Rate limit exceeded@mantissahz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces modifications across multiple files to enhance volume management, particularly for encrypted devices and data engine types. The changes focus on integrating Longhorn V2 engine support, updating crypto-related functions to handle different data engine types, and modifying validation logic for volume creation. The primary goal is to improve volume handling by adding more context-aware operations that can distinguish between different data engine versions. Changes
Sequence DiagramsequenceDiagram
participant NodeServer
participant CryptoModule
participant Volume
NodeServer->>CryptoModule: NodeStageVolume(volume, dataEngine)
CryptoModule->>CryptoModule: VolumeMapper(volume, dataEngine)
alt DataEngine is V2
CryptoModule-->>CryptoModule: Append '-encrypted' to volume name
end
CryptoModule->>Volume: Open/Stage Volume
Volume-->>CryptoModule: Volume Staged
CryptoModule-->>NodeServer: Return Status
The sequence diagram illustrates the enhanced volume staging process, showing how the data engine type influences volume mapping and staging operations. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
mantissahz
force-pushed
the
issue7355
branch
2 times, most recently
from
76e1361 to
cc5802d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
csi/crypto/crypto.go (2)
Line range hint
103-120: Consider consolidating volume name handlingThe volume name handling logic is duplicated between
OpenVolumeandCloseVolume. Consider extracting this to a helper function.+func getEncryptVolumeName(volume, dataEngine string) string { + if dataEngine == string(longhorn.DataEngineTypeV2) { + return volume + mapperV2VolumeSuffix + } + return volume +} func OpenVolume(volume, dataEngine, devicePath, passphrase string) error { - encryptVolumeName := volume - if dataEngine == string(longhorn.DataEngineTypeV2) { - encryptVolumeName = volume + mapperV2VolumeSuffix - } + encryptVolumeName := getEncryptVolumeName(volume, dataEngine) logrus.Infof("Opening device %s with LUKS on %s", devicePath, encryptVolumeName)
128-140: Use consistent error handlingThe error handling in
CloseVolumecould be improved to match the pattern inOpenVolumewhere errors are logged with context.func CloseVolume(volume, dataEngine string) error { namespaces := []lhtypes.Namespace{lhtypes.NamespaceMnt, lhtypes.NamespaceIpc} nsexec, err := lhns.NewNamespaceExecutor(lhtypes.ProcessNone, lhtypes.HostProcDirectory, namespaces) if err != nil { return err } - encryptVolumeName := volume - if dataEngine == string(longhorn.DataEngineTypeV2) { - encryptVolumeName = volume + mapperV2VolumeSuffix - } + encryptVolumeName := getEncryptVolumeName(volume, dataEngine) logrus.Infof("Closing LUKS device %s", encryptVolumeName) _, err = nsexec.LuksClose(encryptVolumeName, lhtypes.LuksTimeout) + if err != nil { + logrus.WithError(err).Warnf("Failed to close LUKS device %s", encryptVolumeName) + } return err }webhook/resources/volume/validator.go (1)
170-171: LGTM: Clear validation for V2 volume limitationsThe validation correctly prevents the use of backing images with V2 volumes. The error message is clear and descriptive.
However, consider adding a comment explaining why backing images are not supported for V2 volumes to help future maintainers.
if types.IsDataEngineV2(volume.Spec.DataEngine) { + // Backing images are not supported for V2 volumes due to architectural limitations + // in the current implementation of the V2 data engine. if volume.Spec.BackingImage != "" { return werror.NewInvalidError("backing image is not supported for data engine v2", "") }csi/node_server.go (1)
519-519: Consider adding error recovery for crypto operationsThe crypto device operations could benefit from additional error recovery mechanisms. If
OpenVolumefails, we might want to attempt cleanup.+func (ns *NodeServer) cleanupCryptoDevice(volumeID, dataEngine string) { + if err := crypto.CloseVolume(volumeID, dataEngine); err != nil { + ns.log.WithError(err).Warnf("Failed to cleanup crypto device for volume %s", volumeID) + } +} if err := crypto.OpenVolume(volumeID, dataEngine, devicePath, passphrase); err != nil { + ns.cleanupCryptoDevice(volumeID, dataEngine) return nil, status.Error(codes.Internal, err.Error()) }Also applies to: 529-529, 534-534
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
csi/crypto/crypto.go(4 hunks)csi/node_server.go(6 hunks)webhook/resources/volume/validator.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (6)
csi/crypto/crypto.go (3)
13-13: LGTM: Clean addition of V2 supportThe import of the Longhorn API package and the addition of the V2 volume suffix constant are well-placed and follow the existing pattern.
Also applies to: 18-18
74-77: LGTM: Clear data engine type handlingThe
VolumeMapperfunction now correctly handles different data engine types by appending the-encryptedsuffix for V2 volumes. The implementation is clean and straightforward.
144-145: Verify error handling in ResizeEncryptoDeviceThe function checks if the device is open but doesn't verify if it's a V2 volume before proceeding. Consider adding validation for V2 volume support.
csi/node_server.go (3)
491-492: LGTM: Enhanced logging with data engine infoGood addition of data engine information in the log message for better debugging.
632-644: LGTM: Improved crypto device cleanup logicThe changes properly handle crypto device cleanup based on data engine type and shared access requirements.
Line range hint
825-853: Verify volume expansion support for V2 volumesThe code handles encrypted volume expansion, but we should verify if V2 volumes support this operation.
✅ Verification successful
V2 volumes support encrypted volume expansion
The implementation in
csi/node_server.gois engine-agnostic and works for both V1 and V2 volumes. The code only checks if the volume is encrypted and handles the crypto operations accordingly, using the dataEngine parameter only for constructing the appropriate mapper path.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if volume expansion is supported for V2 volumes # by looking for any restrictions or validations in the codebase. rg "DataEngineTypeV2.*expansion"Length of output: 34
Script:
#!/bin/bash # Search for V2 volume handling and expansion-related code rg "DataEngineTypeV2" -A 5 -B 5 # Search for volume expansion validations rg "NodeExpandVolume|ControllerExpandVolume" -A 10 # Search for data engine type checks during expansion rg "dataEngine.*expand|expand.*dataEngine" -A 5 -B 5Length of output: 116265
ref: longhorn/longhorn 7355 Signed-off-by: James Lu <james.lu@suse.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
csi/crypto/crypto.go (1)
Line range hint
103-117: Enhance error message clarityWhile the implementation is correct, the error message could be more informative for debugging purposes.
Consider including the data engine type in the error message:
- logrus.WithError(err).Warnf("Failed to open LUKS device %s", devicePath) + logrus.WithError(err).Warnf("Failed to open LUKS device %s (engine: %s)", devicePath, dataEngine)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
csi/crypto/crypto.go(4 hunks)csi/node_server.go(6 hunks)webhook/resources/volume/validator.go(0 hunks)
💤 Files with no reviewable changes (1)
- webhook/resources/volume/validator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- csi/node_server.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (5)
csi/crypto/crypto.go (5)
13-13: LGTM! Clean addition of imports and constantsThe new import and constant are well-structured and necessary for V2 engine support.
Also applies to: 18-18
124-129: LGTM! Well-structured helper functionClean implementation that properly encapsulates the volume name logic for different engine types.
132-132: LGTM! Clean adaptation for V2 engine supportThe changes maintain consistency with the rest of the implementation while properly supporting V2 volumes.
Also applies to: 139-141
Line range hint
74-156: Verify path handling securityWhile the implementation looks secure, let's ensure proper path handling throughout the crypto operations.
Let's check for potential path traversal vulnerabilities:
✅ Verification successful
Path handling implementation is secure
The crypto operations use proper path handling throughout:
- All paths are constructed using
path.Join- Fixed
/dev/mapperprefix with proper validation- Standard library path cleaning prevents directory traversal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for path validation and sanitization rg -A 5 "path\.(Clean|Join|Base)" csi/ # Check for direct path concatenation rg -A 5 "\/.*\+" csi/Length of output: 2744
Script:
#!/bin/bash # Check for path-related constants in crypto package rg -A 2 "const.*[Pp]ath|var.*[Pp]ath" csi/crypto/ # Check for mapper-related constants rg -A 2 "const.*mapper|var.*mapper" csi/crypto/Length of output: 101
Script:
#!/bin/bash # Get the complete crypto.go file content cat csi/crypto/crypto.go # Search for variable declarations and imports rg -p "^(var|import|const)" csi/crypto/Length of output: 7298
74-77: LGTM! Verify volume name validationThe V2 engine path construction logic is clean and consistent. The
-encryptedsuffix addition is properly handled.Let's verify that volume names are properly validated upstream to prevent path traversal:
✅ Verification successful
Volume name validation is secure against path traversal
The implementation is safe because:
- Volume names are validated using strict regex
^[a-z0-9][a-z0-9_.-]+$- Path construction uses
path.Joinwith fixed prefix/suffix- Validation occurs in webhook before reaching the mapper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for volume name validation in the codebase rg -A 5 "func.*[Vv]alidate.*[Vv]olume[Nn]ame"Length of output: 47
Script:
#!/bin/bash # Search for volume name patterns in CRD definitions and validation contexts rg -g '*.yaml' -g '*.go' -A 5 'volume.*name.*pattern|volume.*name.*validation' # Search for volume name usage in crypto context rg -g '*.go' -A 5 'volume.*name.*=|volume.*:=|volume\s+string' # Search for general name validation patterns ast-grep --pattern 'func $_(volume string) $$ { $$$ }'Length of output: 68340
Script:
#!/bin/bash # Search for ValidateName implementation rg -g '*.go' -A 10 'func ValidateName' # Search for volume name validation usage in crypto context rg -g '*.go' -B 5 -A 5 'util\.ValidateName.*volume'Length of output: 11211
when opening and closing a luks2 format device. ref: longhorn/longhorn 7355 Signed-off-by: James Lu <james.lu@suse.com>
| if dataEngine == string(longhorn.DataEngineTypeV2) { | ||
| return path.Join(mapperFilePathPrefix, volume+mapperV2VolumeSuffix) | ||
| } |
There was a problem hiding this comment.
Use if types.IsDataEngineV2(...)
| func VolumeMapper(volume string) string { | ||
| func VolumeMapper(volume, dataEngine string) string { | ||
| if dataEngine == string(longhorn.DataEngineTypeV2) { | ||
| return path.Join(mapperFilePathPrefix, volume+mapperV2VolumeSuffix) |
There was a problem hiding this comment.
Add a comment for explaining the purpose of the suffix.
| if err != nil { | ||
| logrus.WithError(err).Warnf("Failed to open LUKS device %s", devicePath) | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| func getEncryptVolumeName(volume, dataEngine string) string { | ||
| if dataEngine == string(longhorn.DataEngineTypeV2) { |
There was a problem hiding this comment.
| if dataEngine == string(longhorn.DataEngineTypeV2) { | |
| if types.IsDataEngineV2(...) |
| func VolumeMapper(volume string) string { | ||
| func VolumeMapper(volume, dataEngine string) string { | ||
| if dataEngine == string(longhorn.DataEngineTypeV2) { | ||
| return path.Join(mapperFilePathPrefix, volume+mapperV2VolumeSuffix) |
There was a problem hiding this comment.
Reuse getEncryptVolumeName()
| return path.Join(mapperFilePathPrefix, volume+mapperV2VolumeSuffix) | |
| return path.Join(mapperFilePathPrefix, getEncryptVolumeName(volume)) |
| dataEngine := string(longhorn.DataEngineTypeV1) | ||
| if volume != nil { | ||
| dataEngine = volume.DataEngine | ||
| } | ||
| sharedAccess := requiresSharedAccess(volume, nil) | ||
| cleanupCryptoDevice := !sharedAccess || (sharedAccess && volume.Migratable) | ||
| if cleanupCryptoDevice { | ||
| cryptoDevice := crypto.VolumeMapper(volumeID, dataEngine) | ||
| if isOpen, err := crypto.IsDeviceOpen(cryptoDevice); err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } else if isOpen { | ||
| log.Infof("Volume %s closing active crypto device %s", volumeID, cryptoDevice) | ||
| if err := crypto.CloseVolume(volumeID, volume.DataEngine); err != nil { |
There was a problem hiding this comment.
It doesn't handle volume == nil case
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#7355
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context