Feat/sdk 4227/Encryption v2 #400
Conversation
WalkthroughThis pull request introduces comprehensive encryption and key management enhancements to the CleverTap iOS SDK. The changes include adding new Swift and Objective-C files for encryption operations, implementing AES encryption methods (both AES-128 and AES-GCM), and creating a robust key management system using the iOS Keychain. The modifications extend the SDK's cryptographic capabilities, providing more secure data handling mechanisms with improved error management and cross-platform compatibility. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CTEncryptor
participant AESGCMCrypt
participant CTKeychainManager
Client->>CTEncryptor: Request encryption
CTEncryptor->>CTKeychainManager: Generate/Retrieve Key
CTKeychainManager-->>CTEncryptor: Return Key
CTEncryptor->>AESGCMCrypt: Perform Cryptographic Operation
AESGCMCrypt-->>CTEncryptor: Return Encrypted/Decrypted Data
CTEncryptor-->>Client: Return Result
Possibly Related PRs
Suggested Reviewers
🪧 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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (10)
CleverTapSDK/Encryption/AESGCMCrypt.swift (3)
11-16: Enforce specialized error messages.
TheAESGCMEncryptionErrorCodeenum is helpful, but the error cases could benefit from more specific messages or descriptions to guide users in debugging issues. Consider attaching localized or descriptive messages to each error code.
30-36: Extend the CryptError usage with user-friendly messages.
Similar to the previous enum, theCryptErrorcases might offer more context if the localized description or reason is included when throwing or logging.
66-83: Preserve structured data for future uses.
Decryption logic lumps the ciphertext and tag together while returning them as a single data block in the result. Consider returning the tag separately (especially if you plan to store or verify it in a different context). This separation can help with future expansions like partial data integrity checks.CleverTapSDK/Encryption/CTAES.m (2)
6-8: Ensure consistent logging and overall style.
You are usingNSLog()here, whereas other parts of this file (and codebase) rely on internal CleverTap logging macros. For consistency and clarity, consider aligning these logs with the rest of the SDK’s logging approach.
135-139: Remove dead or commented-out code.
The commented-out lines reference the older AES128 method. If it is no longer needed, removing them helps maintain cleaner, more maintainable code.-// NSData *outputData = [AESCrypt AES128WithOperation:operation -// key:[self generateKeyPassword] -// identifier:CLTAP_ENCRYPTION_IV -// data:data];CleverTapSDK/Encryption/AESCrypt.h (1)
10-17: Add documentation and rename parameters for clarity.
The method signature suggests thatidentifieris used like an IV or similar. For improved clarity, renameidentifierto something more descriptive (e.g.,initializationVector) and add doc comments.- + (NSData *)AES128WithOperation:(CCOperation)operation - key:(NSString *)key - identifier:(NSString *)identifier - data:(NSData *)data; + + (NSData *)AES128WithOperation:(CCOperation)operation + key:(NSString *)key + iv:(NSString *)initializationVector + data:(NSData *)data; /** * Encrypts or decrypts the given data using AES-128. * @param operation kCCEncrypt or kCCDecrypt * @param key A string-based key * @param initializationVector The IV used for cipher block chaining * @param data The raw data to be encrypted or decrypted * @return The processed data, or nil on failure */CleverTapSDK/Encryption/CTEncryptor.h (2)
2-2: Fix inconsistent file name in header comment.The file name in the header comment (
EncryptionBridge.h) doesn't match the actual file name (CTEncryptor.h).-// EncryptionBridge.h +// CTEncryptor.h
19-19: Consider adding documentation for encryption mode.The
performCryptOperationmethod could benefit from documentation specifying whether it performs encryption, decryption, or both.// Encryption/Decryption +/** + * Performs encryption or decryption on the input string. + * @param string The input string to process + * @param error On input, a pointer to an error object. If an error occurs, this pointer is set to an actual error object containing the error information. + * @return The processed string, or nil if an error occurred + */ + (NSString * _Nullable)performCryptOperation:(NSString *)string error:(NSError **)error;CleverTapSDK/Encryption/CTEncryptor.m (1)
75-75: Remove redundant return statement.The final
return nilis unreachable as all code paths already return a value.- return nil;CleverTapSDK/Encryption/CTKeychainManager.swift (1)
1-4: Remove unused CryptoKit import.The CryptoKit framework is imported but not used in the implementation.
import Foundation import Security -import CryptoKit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CleverTap-iOS-SDK.podspec(1 hunks)CleverTapSDK.xcodeproj/project.pbxproj(10 hunks)CleverTapSDK/Encryption/AESCrypt.h(1 hunks)CleverTapSDK/Encryption/AESCrypt.m(1 hunks)CleverTapSDK/Encryption/AESGCMCrypt.swift(1 hunks)CleverTapSDK/Encryption/CTAES.m(2 hunks)CleverTapSDK/Encryption/CTEncryptor.h(1 hunks)CleverTapSDK/Encryption/CTEncryptor.m(1 hunks)CleverTapSDK/Encryption/CTKeychainManager.swift(1 hunks)SwiftStarter/SwiftStarter/Supporting Files/Info.plist(1 hunks)
🔇 Additional comments (11)
CleverTapSDK/Encryption/AESGCMCrypt.swift (2)
17-28: Add safety checks or validations in AESGCMCryptResult.
You might consider basic validations for theivanddataduring initialization to ensure that invalid input is caught early.
54-65: Validate IV generation or fallback logic.
When no IV is provided, you create a nonce from empty data. Double-check that the default approach meets your security standards. Generating a random IV for each encryption operation is typically recommended to strengthen security.CleverTapSDK/Encryption/CTEncryptor.h (1)
14-16: LGTM! Well-structured key management methods.The key management methods follow Cocoa conventions for error handling and have clear, descriptive names.
CleverTapSDK/Encryption/AESCrypt.m (1)
15-20: Document the compatibility constraint and potential security implications.The comment about intentional key handling needs more context. Consider documenting:
- Why this compatibility is needed
- Security implications of the current approach
- Future plans to address this technical debt
CleverTapSDK/Encryption/CTKeychainManager.swift (5)
5-9: LGTM! Well-defined error codes.The error codes are appropriately defined for key management operations.
11-14: LGTM! Good use of reverse domain notation for key alias.The implementation follows best practices for key identification.
15-24: LGTM! Well-structured key management.The method follows the principle of key reuse and has appropriate error handling.
54-71: LGTM! Secure key retrieval implementation.The method implements proper error handling and secure key retrieval.
73-85: LGTM! Proper key deletion implementation.The method correctly handles both successful deletion and "key not found" cases.
CleverTapSDK.xcodeproj/project.pbxproj (2)
1335-1344: LGTM! Good organization of encryption-related files.The creation of a dedicated Encryption group improves code organization by grouping related files together.
3227-3227: Verify Swift-ObjC interoperability setup.The build settings have been updated to support mixed Swift and Objective-C code. Please ensure:
- Swift bridging header is properly configured
@objcexposure for Swift classes that need to be accessed from Objective-C- Module imports in Objective-C files that need to use Swift code
Also applies to: 3267-3267, 3276-3276, 3306-3306
✅ Verification successful
Swift-ObjC interoperability is properly configured ✅
The codebase shows correct implementation of Swift-Objective-C interoperability:
- Swift classes and enums are properly exposed with
@objcannotations- Objective-C files correctly import the generated Swift header
- Module support is properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for bridging header configuration xcodebuild -showBuildSettings | grep SWIFT_OBJC_BRIDGING_HEADER # Look for @objc annotations in Swift files rg "@objc" --type swift # Check imports in Objective-C files rg "^#import.*-Swift\.h" --type objcLength of output: 1140
| @available(iOS 13, *) | ||
| @objcMembers | ||
| class AESGCMCrypt: NSObject { | ||
| private let keyIdentifier = "your.key.identifier" | ||
| private static let ivSize = 12 | ||
|
|
||
| private func generateOrGetKey() throws -> SymmetricKey { | ||
| if let existingKey = try? getKeyFromKeychain() { | ||
| return existingKey | ||
| } | ||
|
|
||
| let newKey = SymmetricKey(size: .bits256) | ||
| try storeKeyInKeychain(newKey) | ||
| return newKey | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Keychain concurrency or errors more explicitly.
generateOrGetKey() is a crucial function for retrieving and storing keys. In multi-threaded scenarios or repeated calls, concurrent attempts to generate the key may occur. Ensure robust handling for potential race conditions.
|
|
||
|
|
||
| private func storeKeyInKeychain(_ key: SymmetricKey) throws { | ||
| // Implement secure key storage in Keychain | ||
| } | ||
|
|
||
| private func getKeyFromKeychain() throws -> SymmetricKey? { | ||
| // Implement secure key retrieval from Keychain | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement or remove placeholder methods.
The methods for storing/retrieving keys in the Keychain are not implemented. If these remain unimplemented, the encryption logic will likely fail outside of test or mock environments.
- private func storeKeyInKeychain(_ key: SymmetricKey) throws {
- // Implement secure key storage in Keychain
- }
+ private func storeKeyInKeychain(_ key: SymmetricKey) throws {
+ // Suggested implementation example or future integration:
+ // 1. Convert SymmetricKey to raw data
+ // 2. Use Keychain Services to store
+ // 3. Handle OSStatus errors
+ }
- private func getKeyFromKeychain() throws -> SymmetricKey? {
- // Implement secure key retrieval from Keychain
- return nil
- }
+ private func getKeyFromKeychain() throws -> SymmetricKey? {
+ // 1. Obtain raw data from Keychain
+ // 2. Convert raw data to SymmetricKey
+ // 3. Return the key object or nil if not found
+ }Committable suggestion skipped: line range outside the PR's diff.
| BOOL keyGenerated = [CTEncryptor generateKeyWithError:&error]; | ||
| if (!keyGenerated) { | ||
| NSLog(@"Error generating key: %@", error); | ||
| } | ||
| NSData *outputData = [CTEncryptor performCryptOperation:data error:&error]; | ||
| if (error) { | ||
| NSLog(@"Encryption failed: %@", error.localizedDescription); |
There was a problem hiding this comment.
Pass CCOperation to CTEncryptor or handle encryption/decryption paths.
This method now ignores the operation parameter (kCCEncrypt or kCCDecrypt). Ensure that CTEncryptor has a way to differentiate encryption from decryption to avoid unexpected behavior.
NSData *outputData = nil;
NSError *error = nil;
BOOL keyGenerated = [CTEncryptor generateKeyWithError:&error];
if (!keyGenerated) {
NSLog(@"Error generating key: %@", error);
}
- outputData = [CTEncryptor performCryptOperation:data error:&error];
+ outputData = [CTEncryptor performCryptOperation:data operation:operation error:&error];
if (error) {
NSLog(@"Encryption failed: %@", error.localizedDescription);
}
return outputData;Also applies to: 148-148
| <string>6Z6-7W8-5Z6Z</string> | ||
| <key>CleverTapToken</key> | ||
| <string>aa2-032</string> | ||
| <string>b0a-616</string> |
There was a problem hiding this comment.
Security: Remove hardcoded account credentials.
Storing account credentials in source control is a security risk. Consider:
- Moving credentials to a secure configuration file that's not committed
- Using environment variables or a secure credential management system
- Providing example credentials in a separate
Info.plist.examplefile
| size_t outputAvailableSize = [data length] + kCCBlockSizeAES128; | ||
| void *output = malloc(outputAvailableSize); | ||
|
|
There was a problem hiding this comment.
Potential memory leak in output buffer handling.
The dataWithBytesNoCopy: method takes ownership of the buffer, but if the method returns nil, the buffer isn't freed. Consider using dataWithBytesNoCopy:length:freeWhenDone: for explicit memory management.
- return [NSData dataWithBytesNoCopy:output length:outputMovedSize];
+ return [NSData dataWithBytesNoCopy:output length:outputMovedSize freeWhenDone:YES];Also applies to: 50-50
| } else { | ||
| // Handle earlier iOS versions if needed | ||
| // NSData *outputData = [AESCrypt AES128WithOperation:kCCEncrypt | ||
| // key:[self generateKeyPassword] | ||
| // identifier:CLTAP_ENCRYPTION_IV | ||
| // data:data]; | ||
| } |
There was a problem hiding this comment.
Implement fallback for earlier iOS versions.
The fallback implementation for iOS versions earlier than 13 is incomplete. Either:
- Implement the fallback using the commented AES128 code
- Remove the fallback and make iOS 13 the minimum supported version
| let result = keyData.withUnsafeMutableBytes { bytes in | ||
| SecRandomCopyBytes(kSecRandomDefault, 32, bytes.baseAddress!) | ||
| } |
There was a problem hiding this comment.
Avoid force unwrapping in security-critical code.
Replace force unwrapping with safe unwrapping to prevent potential crashes.
- SecRandomCopyBytes(kSecRandomDefault, 32, bytes.baseAddress!)
+ if let baseAddress = bytes.baseAddress {
+ SecRandomCopyBytes(kSecRandomDefault, 32, baseAddress)
+ } else {
+ return errSecBadReq
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let result = keyData.withUnsafeMutableBytes { bytes in | |
| SecRandomCopyBytes(kSecRandomDefault, 32, bytes.baseAddress!) | |
| } | |
| let result = keyData.withUnsafeMutableBytes { bytes in | |
| if let baseAddress = bytes.baseAddress { | |
| SecRandomCopyBytes(kSecRandomDefault, 32, baseAddress) | |
| } else { | |
| return errSecBadReq | |
| } | |
| } |
| let query: [String: Any] = [ | ||
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrAccount as String: keyAlias, | ||
| kSecValueData as String: keyData, | ||
| kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock, | ||
| kSecAttrService as String: Bundle.main.bundleIdentifier ?? "com.default.app" | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance keychain security with stricter accessibility.
Consider using kSecAttrAccessibleWhenPasscodeSetThisDeviceOnly to ensure keys are only accessible when the device is unlocked and has a passcode set.
let query: [String: Any] = [
kSecClass as String: kSecClassGenericPassword,
kSecAttrAccount as String: keyAlias,
kSecValueData as String: keyData,
- kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock,
+ kSecAttrAccessible as String: kSecAttrAccessibleWhenPasscodeSetThisDeviceOnly,
kSecAttrService as String: Bundle.main.bundleIdentifier ?? "com.default.app"
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let query: [String: Any] = [ | |
| kSecClass as String: kSecClassGenericPassword, | |
| kSecAttrAccount as String: keyAlias, | |
| kSecValueData as String: keyData, | |
| kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlock, | |
| kSecAttrService as String: Bundle.main.bundleIdentifier ?? "com.default.app" | |
| ] | |
| let query: [String: Any] = [ | |
| kSecClass as String: kSecClassGenericPassword, | |
| kSecAttrAccount as String: keyAlias, | |
| kSecValueData as String: keyData, | |
| kSecAttrAccessible as String: kSecAttrAccessibleWhenPasscodeSetThisDeviceOnly, | |
| kSecAttrService as String: Bundle.main.bundleIdentifier ?? "com.default.app" | |
| ] |
| @@ -13,7 +13,7 @@ s.resource_bundles = {'CleverTapSDK' => ['CleverTapSDK/*.{xcprivacy}']} | |||
| s.ios.dependency 'SDWebImage', '~> 5.11' | |||
| s.ios.resource_bundle = {'CleverTapSDK' => ['CleverTapSDK/**/*.{png,xib,html}', 'CleverTapSDK/**/*.xcdatamodeld']} | |||
| s.ios.deployment_target = '9.0' | |||
| s.ios.source_files = 'CleverTapSDK/**/*.{h,m}' | |||
| s.ios.source_files = 'CleverTapSDK/**/*.{h,m,swift}' | |||
There was a problem hiding this comment.
💡 Codebase verification
Add Swift version 6.0 to podspec
The main SDK project uses Swift 6.0, and the podspec should align with this version for the newly added Swift files:
s.ios.source_files = 'CleverTapSDK/**/*.{h,m,swift}'
+s.swift_version = '6.0'🔗 Analysis chain
Verify Swift version compatibility.
The addition of Swift files requires specifying the minimum Swift version for compatibility.
Consider adding the Swift version specification:
s.ios.source_files = 'CleverTapSDK/**/*.{h,m,swift}'
+s.swift_version = '5.0'Let's verify the Swift files in the project:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all Swift files in the project
echo "Swift files in the project:"
fd -e swift . CleverTapSDK/
# Check for Swift version specifications in other files
echo -e "\nChecking for existing Swift version specifications:"
rg -i "swift_version" .
Length of output: 2715
| @@ -769,6 +770,7 @@ | |||
| 07FD65A1223BC26300A845B7 /* CTCoverViewController~iphoneland.xib */ = {isa = PBXFileReference; lastKnownFileType = file.xib; path = "CTCoverViewController~iphoneland.xib"; sourceTree = "<group>"; }; | |||
| 07FD65A3223BCB8200A845B7 /* CTCoverViewController~ipadland.xib */ = {isa = PBXFileReference; lastKnownFileType = file.xib; path = "CTCoverViewController~ipadland.xib"; sourceTree = "<group>"; }; | |||
| 0B5564552C25946C00B87284 /* CTUserInfoMigratorTest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = CTUserInfoMigratorTest.m; sourceTree = "<group>"; }; | |||
| 0B80D41A2D4A111900521AD9 /* CTEncryptor.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CTEncryptor.swift; sourceTree = "<group>"; }; | |||
There was a problem hiding this comment.
💡 Codebase verification
Swift version 6.0 is incorrectly specified
The project is configured to use Swift 6.0, which is not yet released. Please update the SWIFT_VERSION to a stable release (e.g., Swift 5.x) to ensure proper compatibility with iOS 9.0 deployment target.
🔗 Analysis chain
Verify Swift version compatibility with deployment target.
The addition of Swift code requires careful consideration of deployment target compatibility. Please ensure that Swift 6.0 is compatible with the iOS 9.0 deployment target specified in the project settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Swift version compatibility
xcodebuild -showBuildSettings | grep SWIFT
Length of output: 145
Script:
#!/bin/bash
# Search for Swift version and deployment target settings
rg "SWIFT_VERSION|IPHONEOS_DEPLOYMENT_TARGET" CleverTapSDK.xcodeproj/project.pbxproj
Length of output: 592
|
@kushCT / @akashvercetti please see comments by CodeRabbit |
|
This PR adds a swift file to the SDK project but I do not see any other changes. |
https://wizrocket.atlassian.net/browse/SDK-4206
[TAN][SDK] Encryption Algorithm Upgrade in SDK [W.I.P]
Summary by CodeRabbit
New Features
Bug Fixes
Chores