[DAPS-1575] - feature (core) add cipher engine #1682
Conversation
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
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…scanning 1216 daps feature add improved ci scanning
Update nlhoman json version
* Update devel from staging
* fix(install_foxx): remove ssl_args argument it is not a valid flag.
* feature: Implemented logging functions
* refactor: remove all scripts related to dependencies from DataFed repo * feature: update DataFedDependencies submodule and move ground truth of requirements.txt to DataFedDependencies.
…to lib/error_codes.js (#1679)
Contributor
Reviewer's GuideThis PR introduces a new symmetric cipher engine in the common library, leveraging OpenSSL for AES-256-CBC encryption/decryption and Base64 encoding/decoding, integrates the necessary OpenSSL dependencies into the build system, extends utility functions to support reading binary keys, and adds comprehensive unit tests. Class diagram for the new CipherEngine classclassDiagram
class SDMS::CipherEngine {
+static int BASE64_ENCODED_BLOCK_SIZE
+static int BASE64_INPUT_BLOCK_SIZE
+static int NULL_TERMINATOR_SIZE
+static int IV_LENGTH
+static int KEY_LENGTH
+static int MAX_MSG_LENGTH
+static int ENCODED_IV_LENGTH
+static int ENCODED_MSG_LENGTH
+CipherEngine(const unsigned char* inputKey)
+CipherEngine()
+static void generateEncryptionKey(unsigned char token_key[KEY_LENGTH])
+static void generateIV(unsigned char iv[IV_LENGTH])
+CipherBytes encryptAlgorithm(unsigned char* iv, const std::string& msg, LogContext log_context)
+CipherString encodeBytes(CipherBytes unencoded_bytes, LogContext log_context)
+CipherString encrypt(unsigned char *iv, const std::string& msg, LogContext log_context)
+CipherString encrypt(const std::string& msg, LogContext log_context)
+std::string decrypt(const CipherString& encrypted_string, LogContext log_context)
+static bool tokenNeedsUpdate(const libjson::Value::Object &obj)
-std::unique_ptr<char[]> encode64(const unsigned char* input, const int length, LogContext log_context) const
-std::unique_ptr<unsigned char[]> decode64(const char* input, const int length, LogContext log_context) const
-unsigned char key[KEY_LENGTH]
-static void handleErrors(void)
}
class SDMS::CipherEngine::CipherBytes {
+unsigned char encrypted_msg[ENCODED_MSG_LENGTH]
+unsigned char iv[IV_LENGTH]
+int encrypted_msg_len
}
class SDMS::CipherEngine::CipherString {
+std::unique_ptr<char[]> encrypted_msg
+std::unique_ptr<char[]> iv
+int encrypted_msg_len
}
SDMS::CipherEngine "1" *-- "1" SDMS::CipherEngine::CipherBytes
SDMS::CipherEngine "1" *-- "1" SDMS::CipherEngine::CipherString
Class diagram for the new readFile utility functionclassDiagram
class Util {
+void readFile(const std::string &fileName, const int arraySize, unsigned char* array)
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `common/source/Util.cpp:55-58` </location>
<code_context>
return result;
}
+void readFile(const std::string &fileName,const int arraySize, unsigned char* array)
+{
+ //Converting Key for encryption funct
+ unsigned char keyChar[arraySize] = {};
+ //Grabbing key
+ std::ifstream keyFile(fileName, std::ios::binary);
+ if(!keyFile.is_open())
+ {
+ throw TraceException(__FILE__, __LINE__, 0, std::string("Cannot find token key file: ") + fileName);
+ return;
+ }
+
+ keyFile.read(reinterpret_cast<char*>(keyChar),arraySize);
+
+ std::copy(keyChar, keyChar + arraySize, array);
+}
size_t curlResponseWriteCB(char *ptr, size_t size, size_t nmemb,
</code_context>
<issue_to_address>
**suggestion:** Consider handling partial reads in readFile for robustness.
Since keyFile.read may read fewer than arraySize bytes if the file is too short, consider checking keyFile.gcount() and handling incomplete reads appropriately, such as by throwing an exception or zeroing the unused buffer portion.
```suggestion
keyFile.read(reinterpret_cast<char*>(keyChar), arraySize);
std::streamsize bytesRead = keyFile.gcount();
if (bytesRead < arraySize) {
// Zero the unused portion of the buffer
std::memset(keyChar + bytesRead, 0, arraySize - bytesRead);
throw TraceException(__FILE__, __LINE__, 0,
std::string("Key file '") + fileName + "' is too short: expected " +
std::to_string(arraySize) + " bytes, got " + std::to_string(bytesRead));
// Optionally, you could return or handle differently, but throwing is robust
}
std::copy(keyChar, keyChar + arraySize, array);
}
```
</issue_to_address>
### Comment 2
<location> `common/source/CipherEngine.cpp:161-163` </location>
<code_context>
+ * the encrypted output.
+ * EVP_EncryptUpdate can be called multiple times if necessary
+ */
+ if(1 != EVP_EncryptUpdate(ctx, bytes_result.encrypted_msg, &len, msg_unsigned.data(), msg_unsigned.size()))
+ handleErrors();
+ bytes_result.encrypted_msg_len = len;
+
+ /*
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential buffer overflow risk in encrypted_msg assignment.
Check that ENCODED_MSG_LENGTH accounts for possible padding added during encryption, or switch to dynamic allocation to prevent overflow.
</issue_to_address>
### Comment 3
<location> `common/source/CipherEngine.cpp:233` </location>
<code_context>
+ handleErrors();
+ }
+ int plaintext_len = 0;
+ unsigned char plaintext[encoded_encrypted_string.encrypted_msg_len + EVP_MAX_BLOCK_LENGTH] = {};
+ const int ciphertext_len = encoded_encrypted_string.encrypted_msg_len;
+ int len = 0;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider using dynamic allocation for plaintext buffer in decrypt.
Stack allocation may cause issues with large encrypted_msg_len values. Prefer std::vector or std::unique_ptr for safer, flexible buffer management.
Suggested implementation:
```cpp
int plaintext_len = 0;
std::vector<unsigned char> plaintext(encoded_encrypted_string.encrypted_msg_len + EVP_MAX_BLOCK_LENGTH, 0);
const int ciphertext_len = encoded_encrypted_string.encrypted_msg_len;
int len = 0;
```
```cpp
if(1 != EVP_DecryptUpdate(ctx, plaintext.data(), &len, ciphertext.get(), ciphertext_len))
{
handleErrors();
}
plaintext_len = len;
```
</issue_to_address>
### Comment 4
<location> `common/source/CipherEngine.cpp:214` </location>
<code_context>
+ EVP_CIPHER_CTX *ctx = nullptr;
+
+ //converts the cipherstring back to a unsigned char
+ std::unique_ptr<unsigned char[]> ciphertext = decode64(encoded_encrypted_string.encrypted_msg.get(), static_cast<int>(strlen(encoded_encrypted_string.encrypted_msg.get())), log_context);
+ std::unique_ptr<unsigned char[]> iv = decode64(encoded_encrypted_string.iv.get(), static_cast<int>(strlen(encoded_encrypted_string.iv.get())),log_context);
+ /* Create and initialise the context */
</code_context>
<issue_to_address>
**issue (bug_risk):** Using strlen on encrypted_msg may be unsafe for binary data.
If encrypted_msg has null bytes, strlen will give an incorrect length. Use the actual length of the encoded string instead of relying on null-termination.
</issue_to_address>
### Comment 5
<location> `common/tests/unit/test_CipherEngine.cpp:23-32` </location>
<code_context>
+BOOST_AUTO_TEST_CASE(testing_KeyGeneration)
</code_context>
<issue_to_address>
**suggestion (testing):** Missing negative and error condition tests for key generation.
Add tests that simulate key generation failure, such as mocking RAND_bytes to return 0, to verify error handling in CipherEngine::generateEncryptionKey.
Suggested implementation:
```cpp
BOOST_AUTO_TEST_CASE(testing_KeyGeneration)
{
unsigned char token_key[SDMS::CipherEngine::KEY_LENGTH];
unsigned char keyArray[SDMS::CipherEngine::KEY_LENGTH];
unsigned char finalArray[SDMS::CipherEngine::KEY_LENGTH];
CipherEngine::generateEncryptionKey(token_key);
std::string fname = "datafed-token-key.txt";
std::ofstream outf(fname, std::ios::binary);
outf.write(reinterpret_cast<const char*>(token_key), SDMS::CipherEngine::KEY_LENGTH);
outf.close();
}
// Negative test: Simulate RAND_bytes failure
BOOST_AUTO_TEST_CASE(testing_KeyGenerationFailure)
{
unsigned char token_key[SDMS::CipherEngine::KEY_LENGTH];
// Save original RAND_bytes
int (*orig_rand_bytes)(unsigned char *, int) = RAND_bytes;
// Mock RAND_bytes to always fail
RAND_bytes = [](unsigned char *buf, int num) -> int {
(void)buf; (void)num;
return 0;
};
// Expect generateEncryptionKey to throw or handle error
BOOST_CHECK_THROW(CipherEngine::generateEncryptionKey(token_key), std::runtime_error);
// Restore original RAND_bytes
RAND_bytes = orig_rand_bytes;
}
```
- If `CipherEngine::generateEncryptionKey` does not throw a `std::runtime_error` on failure, adjust the `BOOST_CHECK_THROW` to match the actual error handling (e.g., check return value or error code).
- If you use a mocking framework (e.g., GMock), use its facilities to mock `RAND_bytes` instead of direct assignment.
- Ensure that the test suite is linked against OpenSSL and that `RAND_bytes` is accessible for mocking.
</issue_to_address>
### Comment 6
<location> `common/tests/unit/test_CipherEngine.cpp:67-76` </location>
<code_context>
+BOOST_AUTO_TEST_CASE(test_EncryptionDecryption)
</code_context>
<issue_to_address>
**suggestion (testing):** No tests for invalid key or corrupted ciphertext/IV.
Add tests for decryption with invalid keys, corrupted ciphertext, and IV to verify proper error handling.
</issue_to_address>
### Comment 7
<location> `common/tests/unit/test_CipherEngine.cpp:92-89` </location>
<code_context>
+BOOST_AUTO_TEST_CASE(test_EncryptionDecryption_KeyGen)
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not cover empty or very large message input.
Please add tests for empty strings and messages at or above MAX_MSG_LENGTH to ensure proper handling and expected exceptions.
Suggested implementation:
```cpp
BOOST_AUTO_TEST_CASE(test_EncryptionDecryption_KeyGen)
{
LogContext log_context;
unsigned char key[SDMS::CipherEngine::KEY_LENGTH];
CipherEngine::generateEncryptionKey(key);
//Instance only used for encrypting
CipherEngine encryptCipher(key);
//Sets struct CipherString: which contains cipherText, cipherIV, cipherPaddedLen
//Mimicing a globus token
// Test 1: Empty string
{
std::string empty_msg = "";
CipherString encoded_encrypted_packet = encryptCipher.encrypt(empty_msg, log_context);
CipherEngine decryptCipher(key);
std::string decrypted_msg = decryptCipher.decrypt(encoded_encrypted_packet, log_context);
BOOST_CHECK(decrypted_msg == empty_msg);
}
// Test 2: Message at MAX_MSG_LENGTH
{
std::string max_msg(SDMS::CipherEngine::MAX_MSG_LENGTH, 'A');
CipherString encoded_encrypted_packet = encryptCipher.encrypt(max_msg, log_context);
CipherEngine decryptCipher(key);
std::string decrypted_msg = decryptCipher.decrypt(encoded_encrypted_packet, log_context);
BOOST_CHECK(decrypted_msg == max_msg);
}
// Test 3: Message above MAX_MSG_LENGTH
{
std::string over_max_msg(SDMS::CipherEngine::MAX_MSG_LENGTH + 1, 'B');
bool exception_thrown = false;
try {
CipherString encoded_encrypted_packet = encryptCipher.encrypt(over_max_msg, log_context);
} catch (const std::exception& e) {
exception_thrown = true;
}
BOOST_CHECK(exception_thrown);
}
```
- Ensure that `SDMS::CipherEngine::MAX_MSG_LENGTH` is defined and accessible in this test file.
- If the encrypt function does not throw an exception for oversized input, you may need to update the CipherEngine implementation to enforce this limit and throw an exception.
- If a specific exception type is used, replace `std::exception` with the correct type in the catch block.
</issue_to_address>
### Comment 8
<location> `common/tests/unit/test_CipherEngine.cpp:117-89` </location>
<code_context>
+BOOST_AUTO_TEST_CASE(test_EncryptionDecryption_IVGen)
</code_context>
<issue_to_address>
**suggestion (testing):** No test for invalid IV length or content.
Add a test with an invalid IV length or content to verify that the functions correctly handle errors and raise exceptions.
Suggested implementation:
```cpp
BOOST_AUTO_TEST_CASE(test_EncryptionDecryption_IVGen)
{
LogContext log_context;
unsigned char key[SDMS::CipherEngine::KEY_LENGTH];
readFile("datafed-token-key.txt", SDMS::CipherEngine::KEY_LENGTH, key);
//Instance only used for encrypting
CipherEngine encryptCipher(key);
//Sets struct CipherString: which contains cipherText, cipherIV, cipherPaddedLen
//Mimicing a globus token
}
// Test for invalid IV length or content
BOOST_AUTO_TEST_CASE(test_EncryptionDecryption_InvalidIV)
{
LogContext log_context;
unsigned char key[SDMS::CipherEngine::KEY_LENGTH];
readFile("datafed-token-key.txt", SDMS::CipherEngine::KEY_LENGTH, key);
CipherEngine decryptCipher(key);
// Prepare a valid encrypted packet (simulate or use a dummy)
std::string valid_encrypted_packet = "dummy_encrypted_data";
// Prepare an invalid IV (wrong length)
std::string invalid_iv(SDMS::CipherEngine::IV_LENGTH - 2, 'A'); // IV too short
// Simulate a packet with invalid IV (how your decrypt function expects input)
// If decrypt expects a struct or specific format, adjust accordingly
try {
// This assumes decryptCipher.decrypt takes the IV as part of the packet or as a parameter
// You may need to adjust this call to match your actual API
std::string result = decryptCipher.decrypt(valid_encrypted_packet, log_context, invalid_iv);
BOOST_FAIL("Expected exception for invalid IV length, but none was thrown.");
} catch (const std::exception& e) {
BOOST_CHECK_MESSAGE(true, "Exception thrown as expected for invalid IV length.");
} catch (...) {
BOOST_FAIL("Unexpected exception type thrown for invalid IV length.");
}
}
```
- If your `decrypt` function does not take the IV as a separate parameter, you will need to adjust the test to construct the encrypted packet with the invalid IV in the correct format.
- If your code uses a custom exception type for invalid IV, you may want to catch that specific exception instead of `std::exception`.
- Ensure that the dummy encrypted packet and invalid IV are constructed in a way that matches your actual decryption API.
</issue_to_address>
### Comment 9
<location> `common/tests/unit/test_CipherEngine.cpp:147-89` </location>
<code_context>
+BOOST_AUTO_TEST_CASE(test_EncryptionDecryptionJSONValue)
</code_context>
<issue_to_address>
**suggestion (testing):** No test for missing or malformed JSON fields.
Please add tests for cases where required JSON fields are missing or malformed to verify error handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+55
to
+58
| keyFile.read(reinterpret_cast<char*>(keyChar),arraySize); | ||
|
|
||
| std::copy(keyChar, keyChar + arraySize, array); | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: Consider handling partial reads in readFile for robustness.
Since keyFile.read may read fewer than arraySize bytes if the file is too short, consider checking keyFile.gcount() and handling incomplete reads appropriately, such as by throwing an exception or zeroing the unused buffer portion.
Suggested change
| keyFile.read(reinterpret_cast<char*>(keyChar),arraySize); | |
| std::copy(keyChar, keyChar + arraySize, array); | |
| } | |
| keyFile.read(reinterpret_cast<char*>(keyChar), arraySize); | |
| std::streamsize bytesRead = keyFile.gcount(); | |
| if (bytesRead < arraySize) { | |
| // Zero the unused portion of the buffer | |
| std::memset(keyChar + bytesRead, 0, arraySize - bytesRead); | |
| throw TraceException(__FILE__, __LINE__, 0, | |
| std::string("Key file '") + fileName + "' is too short: expected " + | |
| std::to_string(arraySize) + " bytes, got " + std::to_string(bytesRead)); | |
| // Optionally, you could return or handle differently, but throwing is robust | |
| } | |
| std::copy(keyChar, keyChar + arraySize, array); | |
| } |
JoshuaSBrown
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ticket
Description
How Has This Been Tested?
Artifacts (if appropriate):
Tasks
Summary by Sourcery
Introduce a symmetric encryption feature via CipherEngine in the common module, including file-based key loading, OpenSSL integration, and comprehensive unit tests.
New Features:
Enhancements:
Build:
Tests: