- Updates from review comments.

Author: avalerio-tkd

src/common/dbpa_interface.h

@@ -64,6 +64,8 @@ class DBPS_EXPORT EncryptionResult {
     virtual bool success() const = 0;
 
     // Encryption metadata (valid when success() == true)
+    // Map of string key-value pairs containing any extra parameters used during encryption that are needed for decryption,
+    // for example, the encryption_algorithm_version used.
     virtual const std::optional<std::map<std::string, std::string>> encryption_metadata() const = 0;
 
     // Error details (valid when success() == false).
@@ -135,7 +137,12 @@ class DBPS_EXPORT DataBatchProtectionAgentInterface {
         span<const uint8_t> ciphertext,
         std::map<std::string, std::string> encoding_attributes) = 0;
 
-    virtual const std::optional<std::map<std::string, std::string>> EncryptionMetadata() const {
+    /* Returns the encryption metadata provided during the class init() call.
+     * The encryption metadata is a map of string key-value pairs and is defined only for Decrypt usage.
+     * This metadata map is the one returned by the EncryptionResult.encryption_metadata() during the Encrypt call and indicates
+     *   any extra parameters used during encryption that are needed for decryption, for example, the encryption_algorithm_version used.
+     */
+        virtual const std::optional<std::map<std::string, std::string>> EncryptionMetadata() const {
         return column_encryption_metadata_;
     }
 

src/common/dbpa_local.cpp

@@ -198,7 +198,7 @@ std::unique_ptr<EncryptionResult> LocalDataBatchProtectionAgent::Encrypt(
         column_key_id_,
         user_id_,
         app_context_,
-        {}
+        {}  // encryption_metadata, which is empty for the Encryption call.
     );
 
     // Convert plaintext span to vector for the sequencer

src/common/dbpa_local_test.cpp

@@ -23,7 +23,7 @@
 using namespace dbps::external;
 
 namespace {
-    const std::map<std::string, std::string> VALID_ENCRYPTION_METADATA = {{"dbps_version", "v0.01"}};
+    const std::map<std::string, std::string> VALID_ENCRYPTION_METADATA = {{"dbps_agent_version", "v0.01"}};
 }
 
 // Test fixture for LocalDataBatchProtectionAgent tests
@@ -121,7 +121,7 @@ TEST_F(LocalDataBatchProtectionAgentTest, RoundTripEncryptDecrypt) {
     auto encryption_metadata = encrypt_result->encryption_metadata();
     ASSERT_TRUE(encryption_metadata.has_value());
     ASSERT_EQ(1, encryption_metadata->size());
-    ASSERT_TRUE(encryption_metadata->at("dbps_version").length() > 0); // Non-empty string
+    ASSERT_TRUE(encryption_metadata->at("dbps_agent_version").length() > 0); // Non-empty string
 
     // Get the ciphertext
     auto ciphertext_span = encrypt_result->ciphertext();

src/common/dbpa_remote.cpp

@@ -50,6 +50,9 @@ bool RemoteEncryptionResult::success() const {
 const std::optional<std::map<std::string, std::string>> RemoteEncryptionResult::encryption_metadata() const {
     if (!cached_encryption_metadata_.has_value() && response_ && response_->Success()) {
         const auto& response_attrs = response_->GetResponseAttributes();
+        // For the RemoteEncryptionResult, encryption_metadata_ is forwarded from the API response.
+        // The attribute in the API response is also named encryption_metadata.
+        // If the API reponse is empty, the cached value is set to NULL for compatibility with the class interface.
         if (!response_attrs.encryption_metadata_.empty()) {
             cached_encryption_metadata_ = response_attrs.encryption_metadata_;
         } else {

src/common/json_request.cpp

@@ -69,6 +69,7 @@ std::optional<int> SafeParseToInt(const std::string& str) {
 static std::optional<crow::json::rvalue> SafeLoadJsonBody(const std::string& json_string) {
     auto json_body = crow::json::load(json_string);
     if (!json_body || json_body.t() == crow::json::type::Null) {
+        CROW_LOG_ERROR << "Invalid JSON body: " << json_string;
         return std::nullopt;
     }
     return json_body;

src/common/json_request_test.cpp

@@ -142,7 +142,7 @@ const std::string VALID_DECRYPT_JSON = R"({
     },
     "application_context": "{\"user_id\": \"user456\"}",
     "encryption_metadata": {
-        "dbps_version": "v0.01"
+        "dbps_agent_version": "v0.01"
     },
     "debug": {
         "reference_id": "ref789",
@@ -358,7 +358,7 @@ TEST(JsonRequest, DecryptJsonRequestValidParse) {
 
     // Verify encryption_metadata is parsed correctly
     ASSERT_EQ(1, request.encryption_metadata_.size());
-    ASSERT_EQ("v0.01", request.encryption_metadata_.at("dbps_version"));
+    ASSERT_EQ("v0.01", request.encryption_metadata_.at("dbps_agent_version"));
 
     ASSERT_TRUE(request.IsValid());
     ASSERT_EQ("", request.GetValidationError());
@@ -600,7 +600,7 @@ const std::string VALID_ENCRYPT_RESPONSE_JSON = R"({
         "pretty_printed_value": "ENCRYPTED_test@example.com"
     },
     "encryption_metadata": {
-        "dbps_version": "v0.01"
+        "dbps_agent_version": "v0.01"
     }
 })";
 
@@ -640,7 +640,7 @@ TEST(JsonRequest, EncryptJsonResponseValidParse) {
 
     // Verify encryption_metadata is parsed correctly
     ASSERT_EQ(1, response.encryption_metadata_.size());
-    ASSERT_EQ("v0.01", response.encryption_metadata_.at("dbps_version"));
+    ASSERT_EQ("v0.01", response.encryption_metadata_.at("dbps_agent_version"));
 
     ASSERT_TRUE(response.IsValid());
     ASSERT_EQ("", response.GetValidationError());
@@ -749,7 +749,7 @@ TEST(JsonRequest, EncryptJsonResponseToJson) {
     response.reference_id_ = "ref456";
     response.encrypted_compression_ = CompressionCodec::GZIP;
     response.encrypted_value_ = StringToBinary("ENCRYPTED_data");
-    response.encryption_metadata_["dbps_version"] = "v0.01";
+    response.encryption_metadata_["dbps_agent_version"] = "v0.01";
 
     ASSERT_TRUE(response.IsValid());
 
@@ -763,7 +763,7 @@ TEST(JsonRequest, EncryptJsonResponseToJson) {
 
     // Verify encryption_metadata is serialized correctly
     ASSERT_TRUE(json_string.find("encryption_metadata") != std::string::npos);
-    ASSERT_TRUE(json_string.find("dbps_version") != std::string::npos);
+    ASSERT_TRUE(json_string.find("dbps_agent_version") != std::string::npos);
     ASSERT_TRUE(json_string.find("v0.01") != std::string::npos);
 }
 

src/scripts/dbpa_remote_testapp.cpp

@@ -37,7 +37,7 @@ template <typename T>
 using span = tcb::span<T>;
 
 namespace {
-    const std::map<std::string, std::string> VALID_ENCRYPTION_METADATA = {{"dbps_version", "v0.01_unittest"}};
+    const std::map<std::string, std::string> VALID_ENCRYPTION_METADATA = {{"dbps_agent_version", "v0.01_unittest"}};
     const std::string SEQUENCER_ENCRYPTION_METADATA_VERSION = "v0.01";
 }
 
@@ -203,20 +203,20 @@ class DBPARemoteTestApp {
 
                 // Verify encryption metadata
                 auto encryption_metadata = encrypt_result->encryption_metadata();
-                if (!encryption_metadata || encryption_metadata->find("dbps_version") == encryption_metadata->end()) {
+                if (!encryption_metadata || encryption_metadata->find("dbps_agent_version") == encryption_metadata->end()) {
                     std::cout << "  ERROR: Encryption metadata verification failed" << std::endl;
                     all_succeeded = false;
                     continue;
                 }
-                if (encryption_metadata->at("dbps_version") != SEQUENCER_ENCRYPTION_METADATA_VERSION) {
+                if (encryption_metadata->at("dbps_agent_version") != SEQUENCER_ENCRYPTION_METADATA_VERSION) {
                     std::cout << "  ERROR: Encryption metadata version mismatch" << std::endl;
                     std::cout << "    Expected: " << SEQUENCER_ENCRYPTION_METADATA_VERSION << std::endl;
-                    std::cout << "    Got: " << encryption_metadata->at("dbps_version") << std::endl;
+                    std::cout << "    Got: " << encryption_metadata->at("dbps_agent_version") << std::endl;
                     all_succeeded = false;
                     continue;
                 }
                 std::cout << "  OK: Encryption metadata verified" << std::endl;
-                std::cout << "    dbps_version: " << encryption_metadata->at("dbps_version") << std::endl;
+                std::cout << "    dbps_agent_version: " << encryption_metadata->at("dbps_agent_version") << std::endl;
 
                 std::cout << "  OK: Encrypted (" << encrypt_result->size() << " bytes)" << std::endl;
                 std::cout << "  OK: Ciphertext size: " << encrypt_result->size() << " bytes" << std::endl;

src/server/encryption_sequencer.cpp

@@ -29,7 +29,7 @@ using namespace dbps::external;
 using namespace dbps::enum_utils;
 
 namespace {
-    constexpr const char* DBPS_VERSION_KEY = "dbps_version";
+    constexpr const char* DBPS_VERSION_KEY = "dbps_agent_version";
     constexpr const char* DBPS_VERSION_VALUE = "v0.01";
 }
 
@@ -394,7 +394,7 @@ bool DataBatchEncryptionSequencer::ConvertAndDecrypt(const std::vector<uint8_t>&
 
     // TODO: Make server version check a Sequencer error and stop processing.
     //
-    // Check encryption_metadata for dbps_version
+    // Check encryption_metadata for dbps_agent_version
     // 
     // The DBPS server version check during Decrypt is to future-proof against changes on the Encryption process.
     // The Encryption process could change due to updates on the payload decoding, updates on fallback encryption methods, or other changes,
@@ -406,10 +406,10 @@ bool DataBatchEncryptionSequencer::ConvertAndDecrypt(const std::vector<uint8_t>&
         error_message_ = "encryption_metadata must contain key '" + std::string(DBPS_VERSION_KEY) + "'";
         return false;
     } else if (it->second.find(DBPS_VERSION_VALUE) != 0) {
-        std::cerr << "ERROR: EncryptionSequencer - encryption_metadata['" << DBPS_VERSION_KEY << "'] must start with '" 
+        std::cerr << "ERROR: EncryptionSequencer - encryption_metadata['" << DBPS_VERSION_KEY << "'] must match '" 
                   << DBPS_VERSION_VALUE << "', but got '" << it->second << "'" << std::endl;
         error_stage_ = "decrypt_version_check";
-        error_message_ = "encryption_metadata['" + std::string(DBPS_VERSION_KEY) + "'] must start with '" + std::string(DBPS_VERSION_VALUE) + "'";
+        error_message_ = "encryption_metadata['" + std::string(DBPS_VERSION_KEY) + "'] must match '" + std::string(DBPS_VERSION_VALUE) + "'";
         return false;
     }
 

src/server/encryption_sequencer_test.cpp

@@ -252,7 +252,31 @@ TEST(EncryptionSequencer, InputValidation) {
         EXPECT_FALSE(result) << "Empty key_id test should have failed";
         EXPECT_EQ(sequencer.error_stage_, "validation") << "Wrong error stage for empty key_id";
     }
-    
+
+    // Test 4: Missing encryption_metadata
+    {
+        DataBatchEncryptionSequencer sequencer(
+            "test_column", Type::BYTE_ARRAY, std::nullopt, CompressionCodec::UNCOMPRESSED, Format::PLAIN, {{"page_type", "DICTIONARY_PAGE"}}, CompressionCodec::UNCOMPRESSED, "test_key", "test_user", "{}",
+            {}  // encryption_metadata, setting it to empty map.
+        );
+        bool result = sequencer.ConvertAndDecrypt(HELLO_WORLD_DATA);
+        EXPECT_FALSE(result) << "Missing encryption_metadata test should have failed";
+        EXPECT_EQ(sequencer.error_stage_, "decrypt_version_check") << "Wrong error stage for missing encryption_metadata";
+        EXPECT_TRUE(sequencer.error_message_.find("encryption_metadata must contain key") != std::string::npos) << "Wrong error message for missing encryption_metadata";
+    }
+
+    // Test 5: Incorrect encryption_metadata version
+    {
+        DataBatchEncryptionSequencer sequencer(
+            "test_column", Type::BYTE_ARRAY, std::nullopt, CompressionCodec::UNCOMPRESSED, Format::PLAIN, {{"page_type", "DICTIONARY_PAGE"}}, CompressionCodec::UNCOMPRESSED, "test_key", "test_user", "{}",
+            {{"dbps_agent_version", "v0.09"}}  // encryption_metadata, setting it to incorrect version.
+        );
+        bool result = sequencer.ConvertAndDecrypt(HELLO_WORLD_DATA);
+        EXPECT_FALSE(result) << "Incorrect encryption_metadata version test should have failed";
+        EXPECT_EQ(sequencer.error_stage_, "decrypt_version_check") << "Wrong error stage for incorrect encryption_metadata version";
+        EXPECT_TRUE(sequencer.error_message_.find("must match") != std::string::npos) << "Wrong error message for incorrect encryption_metadata version";
+    }
+
 }
 
 // Test round-trip encryption/decryption
@@ -267,8 +291,8 @@ TEST(EncryptionSequencer, RoundTripEncryption) {
         bool encrypt_result = sequencer.ConvertAndEncrypt(HELLO_WORLD_DATA);
         ASSERT_TRUE(encrypt_result) << "Round trip encryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_;
 
-        // Decrypt the encrypted result - need encryption_metadata with dbps_version
-        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_version").length() > 0);
+        // Decrypt the encrypted result - need encryption_metadata with dbps_agent_version
+        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_agent_version").length() > 0);
         bool decrypt_result = sequencer.ConvertAndDecrypt(sequencer.encrypted_result_);
         ASSERT_TRUE(decrypt_result) << "Round trip decryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_;
 
@@ -286,8 +310,8 @@ TEST(EncryptionSequencer, RoundTripEncryption) {
         bool encrypt_result = sequencer.ConvertAndEncrypt(BINARY_DATA);  // Binary data: 0x00, 0x01, 0x02, 0x03, 0x04, 0x05
         ASSERT_TRUE(encrypt_result) << "Binary round trip encryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_;
 
-        // Decrypt the encrypted result - need encryption_metadata with dbps_version
-        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_version").length() > 0);
+        // Decrypt the encrypted result - need encryption_metadata with dbps_agent_version
+        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_agent_version").length() > 0);
         bool decrypt_result = sequencer.ConvertAndDecrypt(sequencer.encrypted_result_);
         ASSERT_TRUE(decrypt_result) << "Binary round trip decryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_;
 
@@ -307,8 +331,8 @@ TEST(EncryptionSequencer, RoundTripEncryption) {
         bool encrypt_result = sequencer.ConvertAndEncrypt(SINGLE_CHAR_DATA);
         ASSERT_TRUE(encrypt_result) << "Single char round trip encryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_;
 
-        // Decrypt the encrypted result - need encryption_metadata with dbps_version
-        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_version").length() > 0);
+        // Decrypt the encrypted result - need encryption_metadata with dbps_agent_version
+        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_agent_version").length() > 0);
         bool decrypt_result = sequencer.ConvertAndDecrypt(sequencer.encrypted_result_);
         ASSERT_TRUE(decrypt_result) << "Single char round trip decryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_;
 
@@ -335,9 +359,9 @@ TEST(EncryptionSequencer, RoundTripEncryption) {
         // Key-aware XOR encryption should produce different results for different keys
         EXPECT_NE(sequencer1.encrypted_result_, sequencer2.encrypted_result_);
 
-        // But both should decrypt back to the same original - need encryption_metadata with dbps_version
-        ASSERT_TRUE(sequencer1.encryption_metadata_.size() > 0 && sequencer1.encryption_metadata_.at("dbps_version").length() > 0);
-        ASSERT_TRUE(sequencer2.encryption_metadata_.size() > 0 && sequencer2.encryption_metadata_.at("dbps_version").length() > 0);
+        // But both should decrypt back to the same original - need encryption_metadata with dbps_agent_version
+        ASSERT_TRUE(sequencer1.encryption_metadata_.size() > 0 && sequencer1.encryption_metadata_.at("dbps_agent_version").length() > 0);
+        ASSERT_TRUE(sequencer2.encryption_metadata_.size() > 0 && sequencer2.encryption_metadata_.at("dbps_agent_version").length() > 0);
         bool decrypt1 = sequencer1.ConvertAndDecrypt(sequencer1.encrypted_result_);
         bool decrypt2 = sequencer2.ConvertAndDecrypt(sequencer2.encrypted_result_);
 
@@ -376,8 +400,8 @@ TEST(EncryptionSequencer, ResultStorage) {
         bool encrypt_result = sequencer.ConvertAndEncrypt(HELLO_WORLD_DATA);
         ASSERT_TRUE(encrypt_result) << "Result storage decryption test failed during encryption";
 
-        // Then decrypt it - need encryption_metadata with dbps_version
-        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_version").length() > 0);
+        // Then decrypt it - need encryption_metadata with dbps_agent_version
+        ASSERT_TRUE(sequencer.encryption_metadata_.size() > 0 && sequencer.encryption_metadata_.at("dbps_agent_version").length() > 0);
         bool decrypt_result = sequencer.ConvertAndDecrypt(sequencer.encrypted_result_);
         ASSERT_TRUE(decrypt_result) << "Result storage decryption test failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_;