Replace CKR_GENERAL_ERROR with CKR_ENCRYPTED_DATA_INVALID or CKR_ENCRYPTED_DATA_LEN_RANGE upon decryption failure #690
Conversation
…YPTED_DATA_LEN_RANGE upon decryption failure softhsm#689
|
I see the CI/CD workflow involved downloading of some files but the downloading failed. Any idea how to unblock? |
|
Please rebase so Github actions are included. |
|
Rebased |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes in the SoftHSM library involve significant enhancements to cryptographic key management and generation capabilities. The modifications expand the library's functionality by adding new methods for symmetric and asymmetric key wrapping, unwrapping, and deriving keys using various cryptographic mechanisms. The implementation now supports a broader range of key types including AES, DES, RSA, DSA, EC, and GOST, with improved mechanisms for key generation, parameter validation, and mechanism permission checks. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SoftHSM
participant KeyManager
Client->>SoftHSM: Generate Key
SoftHSM->>KeyManager: Validate Mechanism
KeyManager-->>SoftHSM: Mechanism Permitted
SoftHSM->>SoftHSM: Generate Key Pair
SoftHSM-->>Client: Return Key Handles
Poem
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/SoftHSM.cpp (2)
3312-3320: Improve error handling for decryption failuresThe code now returns
CKR_ENCRYPTED_DATA_INVALIDinstead of a genericCKR_GENERAL_ERRORwhen decryption fails. This is a more specific and appropriate error code for decryption failures.Consider adding debug logging to help diagnose decryption failures:
if (!cipher->decryptUpdate(encryptedData,data)) { session->resetOp(); + DEBUG_MSG("Decryption update failed"); return CKR_ENCRYPTED_DATA_INVALID; }
3381-3389: Improve error handling for asymmetric decryption failuresSimilar to the symmetric case, the code now returns
CKR_ENCRYPTED_DATA_INVALIDinstead ofCKR_GENERAL_ERRORfor asymmetric decryption failures.Consider adding debug logging here as well:
if (!asymCrypto->decrypt(privateKey,encryptedData,data,mechanism)) { session->resetOp(); + DEBUG_MSG("Asymmetric decryption failed"); return CKR_ENCRYPTED_DATA_INVALID; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(5 hunks)
🔇 Additional comments (3)
src/lib/SoftHSM.cpp (3)
3478-3493: Improve error handling for multi-part decryption failuresThe code now returns more specific error codes for multi-part decryption failures:
CKR_ENCRYPTED_DATA_INVALIDfor decryption failuresCKR_ENCRYPTED_DATA_LEN_RANGEfor output buffer size mismatchesThis is a good improvement in error handling specificity.
Line range hint
3581-3592: Improve error handling for final decryption failuresSimilar improvements in error handling for the final decryption operation:
CKR_ENCRYPTED_DATA_INVALIDfor decryption failuresCKR_ENCRYPTED_DATA_LEN_RANGEfor output buffer size mismatchesThis is consistent with the other error handling improvements.
Line range hint
1-5000: Verify error code consistency across the codebaseThe changes introduce more specific error codes for decryption failures. We should verify that these error codes are used consistently across the entire codebase.
Consider documenting the error handling strategy for cryptographic operations in a central location to ensure consistency across the codebase. This should include:
- When to use specific error codes vs generic ones
- What debug information should be logged for each type of error
- Guidelines for error recovery and cleanup
✅ Verification successful
Error codes are used consistently for decryption failures
The codebase demonstrates consistent and well-defined usage of error codes:
- CKR_ENCRYPTED_DATA_LEN_RANGE is correctly used for all size/length related issues
- CKR_ENCRYPTED_DATA_INVALID is properly used for actual decryption failures
- Error handling includes appropriate cleanup and logging
- Test suite validates error code usage for both single and multi-part operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent use of CKR_ENCRYPTED_DATA_INVALID and CKR_ENCRYPTED_DATA_LEN_RANGE rg "CKR_ENCRYPTED_DATA_(INVALID|LEN_RANGE)" -A 2 -B 2Length of output: 5333
See #689
Summary by CodeRabbit
New Features
Improvements