feat: implement hybrid encryption for message hash with 500-byte limit #646
Conversation
- Replace RSA-only encryption with hybrid AES-256 + RSA-2048 approach - Increase maximum payload capacity from 245 bytes to 500 bytes - Add MESSAGE_HASH_MAX_SIZE config (default: 500 bytes) - Implement payload size validation before encryption - Add comprehensive test coverage for hybrid encryption - Add tests for 500-byte limit enforcement - Maintain backward compatibility with decryption This change enables larger order recipient data while maintaining security through hybrid encryption that combines AES-256 for data encryption and RSA-2048 for key encryption.
📝 WalkthroughWalkthroughAdds a configurable MESSAGE_HASH_MAX_SIZE and implements hybrid encryption (AES-256-GCM payload + RSA-2048 key-wrap) with a 4-byte length-prefixed envelope; updates EncryptOrderRecipient/GetOrderRecipientFromMessageHash to use hybrid format with legacy fallback; adds comprehensive tests for hybrid and legacy cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Encryptor
participant JSON_Marshal as "JSON Marshal"
participant AES as "AES-256-GCM"
participant RSA as "RSA-2048 OAEP"
participant Envelope
Client->>Encryptor: provide data + RSA public key + maxSize
Encryptor->>JSON_Marshal: marshal data
JSON_Marshal-->>Encryptor: JSON bytes
Encryptor->>Encryptor: check size ≤ maxSize
Encryptor->>AES: generate AES-256 key, encrypt JSON (GCM)
AES-->>Encryptor: ciphertext + nonce
Encryptor->>RSA: encrypt AES key with RSA OAEP
RSA-->>Encryptor: encrypted AES key
Encryptor->>Envelope: assemble [4-byte len][enc AES key][ciphertext]
Envelope-->>Client: hybrid encrypted envelope
sequenceDiagram
participant Client
participant Decryptor
participant EnvelopeParser as "Envelope Parser"
participant RSA as "RSA-2048 OAEP"
participant AES as "AES-256-GCM"
participant JSON_Unmarshal as "JSON Unmarshal"
Client->>Decryptor: send envelope + RSA private key
Decryptor->>EnvelopeParser: read 4-byte length, extract enc AES key and ciphertext
EnvelopeParser-->>Decryptor: segments (encKey, ciphertext)
Decryptor->>RSA: decrypt encKey -> AES key
RSA-->>Decryptor: AES key
Decryptor->>AES: decrypt ciphertext (GCM) using AES key + nonce
AES-->>Decryptor: plaintext JSON bytes
Decryptor->>JSON_Unmarshal: unmarshal to object
JSON_Unmarshal-->>Client: original data structure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)utils/crypto/crypto_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting 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: 1
🤖 Fix all issues with AI agents
In @utils/crypto/crypto_test.go:
- Around line 135-175: The test currently compares jsonSize to a hardcoded 500;
update the test to use the actual configured limit by referencing
cryptoConf.MessageHashMaxSize (or set cryptoConf.MessageHashMaxSize explicitly
in the test) before the size check so the conditional and expectations match
EncryptOrderRecipient's behavior; ensure you import or access the same crypto
configuration used by EncryptOrderRecipient and use that value in the if
jsonSize <= ... check and any log/assert messages so
GetOrderRecipientFromMessageHash/EncryptOrderRecipient behavior is tested
consistently.
🧹 Nitpick comments (4)
utils/crypto/crypto.go (3)
315-318: Minor: Potential integer overflow when castinguint32tointon 32-bit systems.On 32-bit systems, if
keyLenexceedsmath.MaxInt32, the cast tointwould produce a negative value, potentially bypassing the length check. In practice, RSA-2048 output is 256 bytes, making this unlikely, but defensive coding would add an upper bound check.🛡️ Optional defensive check
// Extract encrypted key keyLen := binary.BigEndian.Uint32(encrypted[0:4]) + if keyLen > 1024 { // RSA-4096 max is 512 bytes; 1024 is generous upper bound + return nil, fmt.Errorf("invalid key length: %d", keyLen) + } if len(encrypted) < int(4+keyLen) { return nil, fmt.Errorf("invalid encrypted data length") }
402-402: Inconsistent config access pattern.Line 386 uses the cached
cryptoConfvariable, but line 402 callsconfig.CryptoConfig()directly. While functionally equivalent, this inconsistency could lead to confusion and potential issues if caching behavior matters.♻️ Suggested fix for consistency
// Decrypt with the private key of the aggregator - message, err := decryptHybridJSON(messageCipher, config.CryptoConfig().AggregatorPrivateKey) + message, err := decryptHybridJSON(messageCipher, cryptoConf.AggregatorPrivateKey)
407-416: Consider avoiding double JSON serialization.The current approach marshals the
interface{}back to JSON bytes, then unmarshals into the typed struct. This works but is inefficient. A cleaner approach would be to havedecryptHybridJSONaccept a destination pointer or return raw bytes.♻️ Alternative approach (if performance matters)
// Option: Return raw bytes from decryptHybridJSON and unmarshal directly func decryptHybridJSONBytes(encrypted []byte, privateKeyPEM string) ([]byte, error) { // ... same decryption logic ... return plaintext, nil // Return bytes instead of unmarshaling } // Then in GetOrderRecipientFromMessageHash: plaintext, err := decryptHybridJSONBytes(messageCipher, cryptoConf.AggregatorPrivateKey) if err != nil { return nil, fmt.Errorf("failed to decrypt message hash: %w", err) } var recipient *types.PaymentOrderRecipient if err := json.Unmarshal(plaintext, &recipient); err != nil { return nil, err }utils/crypto/crypto_test.go (1)
47-73: Good coverage for basic flow, but metadata assertion is missing.The test verifies
AccountIdentifier,AccountName,Institution, andMemo, but doesn't assert thatMetadataround-trips correctly despite being set in the test order.🧪 Add metadata assertion
assert.Equal(t, order.Institution, recipient.Institution) assert.Equal(t, order.Memo, recipient.Memo) + assert.Equal(t, "TEST123", recipient.Metadata["reference"]) + assert.Equal(t, "Test transaction", recipient.Metadata["note"]) })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/crypto.goutils/crypto/crypto.goutils/crypto/crypto_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
utils/crypto/crypto_test.go (1)
utils/crypto/crypto.go (2)
EncryptOrderRecipient(363-392)GetOrderRecipientFromMessageHash(395-418)
utils/crypto/crypto.go (1)
config/crypto.go (1)
CryptoConfig(20-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (6)
utils/crypto/crypto.go (2)
256-306: LGTM! Hybrid encryption implementation is well-designed.The
encryptHybridJSONfunction correctly implements AES-256-GCM with RSA-2048 key wrapping:
- Size validation occurs after JSON marshaling (correct order)
- Cryptographically secure random generation for both AES key and nonce
- Proper envelope format with length-prefixed RSA-encrypted key
385-386: LGTM! Clean integration with the new hybrid encryption.The
EncryptOrderRecipientfunction correctly delegates toencryptHybridJSONwith the configured public key and max size limit.utils/crypto/crypto_test.go (3)
247-270: Excellent tampering detection test.This test correctly validates that AES-GCM's authenticated encryption detects modifications to the ciphertext. The bit-flip approach is a good way to verify integrity protection.
290-315: Good nonce randomness verification.Testing that consecutive encryptions produce different outputs and different nonces confirms the cryptographic non-determinism is working correctly.
46-315: Remove comment - backward compatibility is not a PR requirement.The PR objectives, as shown in the commit message, specify only "implement hybrid encryption for message hash with 500-byte limit" with no mention of backward compatibility. The implementation correctly focuses on the hybrid encryption format. The test suite appropriately covers the new functionality without needing backward compatibility tests.
config/crypto.go (1)
16-16: Configuration is correct; runtime validation already handles edge cases.The
MessageHashMaxSizeconfiguration follows the established pattern in this file, and the default of 500 bytes is reasonable. The code already validates this at runtime inencryptHybridJSON(): any payload exceeding the limit returns an error rather than silently proceeding. This behavior is tested and intentional—no additional validation is needed at config time.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @utils/test/test.env:
- Around line 21-24: Summary: Committed RSA private keys in test.env are a
security risk and must be removed; replace with a test-safe mechanism. Fix:
delete the hard-coded AGGREGATOR_PRIVATE_KEY and AGGREGATOR_PUBLIC_KEY from the
committed test.env and instead (a) update tests that reference
AGGREGATOR_PRIVATE_KEY / AGGREGATOR_PUBLIC_KEY to obtain keys from environment
variables or a test fixture that generates ephemeral RSA key pairs at setup
time, (b) add a committed test.env.example (or README note) showing the variable
names but not real keys, and (c) ensure the real test.env or any file containing
private keys is gitignored or excluded from commits; also add a brief comment
near AGGREGATOR_PRIVATE_KEY/AGGREGATOR_PUBLIC_KEY in tests explaining they must
never be hard-coded and keep any CI provisioning logic to inject real keys
securely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/test/test.env
🧰 Additional context used
🪛 Gitleaks (8.30.0)
utils/test/test.env
[high] 24-24: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
utils/test/test.env (2)
19-19: HD wallet mnemonic is committed to version control.Similar to the RSA keys concern, this mnemonic phrase could derive real wallet addresses. Ensure this is a well-known test mnemonic that holds no real value, or consider generating it dynamically during tests.
28-28: This SendGrid API key is a dummy placeholder, not a real credential.The key
SG.RSmzXPz3QpGfIabVecxxNAis only 24 characters long, while authentic SendGrid API keys are always exactly 69 characters. Located in a test environment file alongside other obvious test credentials (test DB password, test mnemonic, test RSA keys), this is a valid placeholder key for testing and does not represent a security risk.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @utils/crypto/crypto_test.go:
- Around line 154-179: The test computes payload size using a local ad-hoc
struct which can drift from the real struct used by EncryptOrderRecipient and
then conditionally skips assertions if size > 500; instead, marshal the actual
value passed into EncryptOrderRecipient (use json.Marshal(order) rather than the
local struct) so the measured size matches the implementation, and replace the
conditional with a hard assertion (e.g., require.LessOrEqual(t, jsonSize, 500))
so the test fails fast if the payload exceeds 500 bytes before calling
EncryptOrderRecipient and GetOrderRecipientFromMessageHash.
🧹 Nitpick comments (1)
utils/crypto/crypto_test.go (1)
52-78: Consider verifying Metadata content after decryption.The test verifies
AccountIdentifier,AccountName,Institution, andMemo, but doesn't verify that theMetadatamap is correctly preserved after the round-trip. Adding an assertion likeassert.Equal(t, order.Metadata, recipient.Metadata)would strengthen the test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/crypto/crypto_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (10)
utils/crypto/crypto_test.go (10)
3-11: LGTM!Imports are appropriate for the test suite requirements.
46-50: LGTM!The conditional skip for missing RSA keys is a sensible approach for CI tolerance, and the message clearly indicates what's needed.
80-105: LGTM!Good coverage for the provider edge relationship scenario.
107-138: LGTM!Good test for hybrid encryption with larger payloads. The metadata length comparison at line 135 could be strengthened to a full equality check, but this is minor.
182-220: LGTM with same struct caveat.Good test structure—the
assert.Greaterat line 212 validates the test precondition. The error message check at line 217 properly verifies the failure reason. The same local struct concern applies here but is less critical since the guard assertion catches setup issues.
222-242: LGTM!Good edge case coverage for minimal metadata.
244-250: LGTM!Good negative test for invalid input handling.
252-275: LGTM!Good integrity test verifying that tampered ciphertext is detected. The bit manipulation approach correctly tests the authentication properties of the hybrid encryption scheme.
277-293: LGTM!Good edge case coverage for empty field handling.
295-320: LGTM!Excellent test for cryptographic randomness. Verifying that identical inputs produce different ciphertexts is critical for encryption security.
…rid and legacy formats
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@utils/crypto/crypto_test.go`:
- Around line 46-51: The skip message in TestGetOrderRecipientFromMessageHash is
misleading; update the t.Skip call to use a clear, accurate message (e.g.,
referencing TestGetOrderRecipientFromMessageHash or missing
AGGREGATOR_PUBLIC_KEY/PRIVATE_KEY) so developers know why this specific test was
skipped; locate the skip in TestGetOrderRecipientFromMessageHash and replace the
text passed to t.Skip with the corrected message.
♻️ Duplicate comments (1)
utils/crypto/crypto_test.go (1)
140-180: Previous feedback on this segment still applies.The concerns raised in prior reviews remain unaddressed:
- The hardcoded
500should referencecryptoConf.MessageHashMaxSizefor consistency with the implementation- The local struct definition may drift from the actual struct used in
EncryptOrderRecipient- The conditional
if jsonSize <= 500makes the test unreliable - if the payload happens to exceed 500 bytes, the test passes without verifying anythingConsider using
require.LessOrEqual(t, jsonSize, cryptoConf.MessageHashMaxSize)to fail fast if the test setup is wrong, ensuring the test actually exercises the intended path.
🧹 Nitpick comments (2)
utils/crypto/crypto_test.go (2)
182-220: Consider using the configured max size constant.This test correctly asserts that the payload exceeds 500 bytes before testing. However, the hardcoded
500values should referencecryptoConf.MessageHashMaxSizefor consistency with the implementation. If the configuration changes, this test would need manual updates.Suggested improvement
t.Logf("Payload size: %d bytes (exceeds 500 byte limit)", jsonSize) - assert.Greater(t, jsonSize, 500, "payload should exceed 500 bytes for this test") + assert.Greater(t, jsonSize, cryptoConf.MessageHashMaxSize, "payload should exceed max size for this test") // This should fail due to size limit _, err := EncryptOrderRecipient(order) assert.Error(t, err, "encryption should fail when payload exceeds 500 bytes")
386-419: Consider explicit error handling in test setup.Lines 397 and 407 ignore errors from encryption functions. While the test would eventually fail on the assertions, explicit error handling with
require.NoErrorwould provide clearer failure messages if the setup itself fails.Suggested improvement
// Create legacy encrypted message - legacyEncrypted, _ := PublicKeyEncryptJSON(legacyData, publicKey) + legacyEncrypted, err := PublicKeyEncryptJSON(legacyData, publicKey) + require.NoError(t, err, "legacy encryption setup should succeed") legacyHash := base64.StdEncoding.EncodeToString(legacyEncrypted) // Create new hybrid encrypted message newOrder := &ent.PaymentOrder{ AccountIdentifier: "HYBRID456", AccountName: "Hybrid User", Institution: "Hybrid Bank", Memo: "Hybrid format", } - hybridHash, _ := EncryptOrderRecipient(newOrder) + hybridHash, err := EncryptOrderRecipient(newOrder) + require.NoError(t, err, "hybrid encryption setup should succeed")Note: This requires adding
"github.com/stretchr/testify/require"to imports.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/crypto/crypto_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
utils/crypto/crypto_test.go (1)
utils/crypto/crypto.go (3)
EncryptOrderRecipient(359-388)GetOrderRecipientFromMessageHash(422-445)PublicKeyEncryptJSON(156-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (6)
utils/crypto/crypto_test.go (6)
3-11: LGTM!The added imports are all utilized in the new test functions -
base64for message tampering tests,jsonfor payload size calculations,stringsfor generating test data, andentfor thePaymentOrdertype.
52-78: LGTM!Solid round-trip test covering basic encryption and decryption with verification of all key fields.
80-105: LGTM!Good coverage for the provider edge relationship, ensuring the ProviderID is correctly encrypted and recovered.
107-138: LGTM!Good test for larger payloads with extensive metadata. The assertions verify complete round-trip integrity including metadata length.
222-320: LGTM!Comprehensive coverage of edge cases:
- Small metadata handling
- Invalid base64 rejection
- Tampered message detection (important for authenticated encryption)
- Empty field handling
- Nonce randomness verification (critical for cryptographic security)
323-361: LGTM!Excellent backward compatibility test design. The legacy RSA format test correctly simulates the old integration pattern (without
Noncefield) and verifies the fallback decryption mechanism works properly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
looks good |
Description
This PR implements hybrid encryption (AES-256 + RSA-2048) for the message hash functionality to support larger payloads while maintaining security.
Key Changes:
Benefits:
References
closes #647
This change addresses the limitation of RSA-2048 encryption which could only handle payloads up to 245 bytes.
Testing
Test Coverage:
Manual Testing:
All tests pass successfully:
```bash
cd utils/crypto && go test -v -run TestGetOrderRecipientFromMessageHash
```
Environment:
Checklist
By submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.