Wiring up encryption metadata to the DBPA remote and local agents. by avalerio-tkd · Pull Request #167 · protegrity/DataBatchProtectionService

avalerio-tkd · 2025-11-19T01:58:31Z

Update API server to return encryption metadata in the response.
Populate server version as encryption metadata during Encryption and check it back during Decryption.
Updated unitests.
Fixed hidden error when parsing empty Json strings.

- Update API server to return encryption metadata in the response. - Populate server version as encryption metadata during Encryption and check it back during Decryption. - Updated unitests. - Fixed hidden error when parsing empty Json strings.

sofia-tekdatum · 2025-11-19T02:26:50Z

        user_id_,
-        app_context_
+        app_context_,
+        {}


add // encryption_metadata comment

sofia-tekdatum · 2025-11-19T02:27:38Z

 private:
    std::vector<uint8_t> ciphertext_;
    bool success_;
+    std::map<std::string, std::string> encryption_metadata_;


Not sure if it should be in this header file or in another, but can you add a comment with an example of what may be contained in here?

Added comments on the dbpa_interface.h file.

sofia-tekdatum · 2025-11-19T02:32:09Z

+    auto it = encryption_metadata_.find(DBPS_VERSION_KEY);
+    if (it == encryption_metadata_.end()) {
+        std::cerr << "ERROR: EncryptionSequencer - encryption_metadata must contain key '" << DBPS_VERSION_KEY << "'" << std::endl;
+        // ++++ remove the hard error before merging ++++


Why do you want to remove this?

This was to prevent breaking the Arrow flow until we know for sure the metadata is saved on the Parquet files correctly. But this is probably not needed since that was tested already on Arrow and this is now tested on DPBA flow, so will merge with it.

sofia-tekdatum · 2025-11-19T02:34:18Z

            "test_user",       // user_id
-            "{}"               // application_context
+            "{}",               // application_context
+            {}                  // encryption_metadata


Not sure I could see a test where you verify that the sequencer stops if there's a mismatch in the expected encryption metadata?

Thanks for catching the test gap. Added.

- Updated dbpa_remote_testapp to use the new version checking.

argmarco-tkd

Thanks for this. Overall LGTM, but left a few comments around

argmarco-tkd · 2025-11-19T03:20:52Z

        user_id_,
-        app_context_
+        app_context_,
+        {}


argmarco-tkd · 2025-11-19T03:25:29Z

+        const auto& response_attrs = response_->GetResponseAttributes();
+        if (!response_attrs.encryption_metadata_.empty()) {
+            cached_encryption_metadata_ = response_attrs.encryption_metadata_;
+        } else {
+            cached_encryption_metadata_ = std::nullopt;
+        }


let's document what's going on here - it's a bit hard to grasp

Done. You can let me know if the comment is sufficient and I can expand more if needed.

it's a clearer, thanks - but the whole concept of 'cached metadata' is a bit confusing. maybe let's add a short note on that? (in particular, why is it 'cached' ?)

cached was probably a bad choice, so changed the variable names to parsed. I think that captures better the usage.

thank you! it's much clearer now.

argmarco-tkd · 2025-11-19T03:30:43Z

    // The caching of this is possible because the API response `response_` doesn't change after construction.
    mutable std::string cached_error_message_;
    mutable std::map<std::string, std::string> cached_error_fields_;
+    mutable std::optional<std::map<std::string, std::string>> cached_encryption_metadata_;


let's add a note for the future, where we state that this is cacheable only because the metadata is identical across requests (but that may change)

Actually, this is the RemoteEncryptionResult, not the RemoteDPBA. A new instance is returned on each call to RemoteDBPA.Encrypt().

This is cached separately on each RemoteEncryptionResult instance, to avoid parsing the Json with every call to encryption_metadata(). It isn't statically cached for the class.

But let me know if something is unclear, and we can discuss.

argmarco-tkd · 2025-11-19T03:38:48Z

 using namespace dbps::enum_utils;

+namespace {
+    constexpr const char* DBPS_VERSION_KEY = "dbps_version";


We should discuss this property name. Let's create a task to discuss the naming (or discuss offline when you have a chance). I.e. DBPS is technically 'server' related, but we have a local agent. Maybe dbpa_agent_version or similar is more appropriate.

Renamed it to dbpa_agent_version as suggested.

argmarco-tkd · 2025-11-19T03:45:30Z

+        std::cerr << "ERROR: EncryptionSequencer - encryption_metadata['" << DBPS_VERSION_KEY << "'] must start with '" 
+                  << DBPS_VERSION_VALUE << "', but got '" << it->second << "'" << std::endl;


The 'must start' terminology may be confusing. Can we just say "must match" ?

- Updating devel branch

avalerio-tkd

Thanks for the reviews @argmarco-tkd @sofia-tekdatum . Could you PTAL?

avalerio-tkd · 2025-11-24T00:49:03Z

        user_id_,
-        app_context_
+        app_context_,
+        {}


avalerio-tkd · 2025-11-24T01:07:11Z

 private:
    std::vector<uint8_t> ciphertext_;
    bool success_;
+    std::map<std::string, std::string> encryption_metadata_;


Added comments on the dbpa_interface.h file.

avalerio-tkd · 2025-11-24T01:12:55Z

+    auto it = encryption_metadata_.find(DBPS_VERSION_KEY);
+    if (it == encryption_metadata_.end()) {
+        std::cerr << "ERROR: EncryptionSequencer - encryption_metadata must contain key '" << DBPS_VERSION_KEY << "'" << std::endl;
+        // ++++ remove the hard error before merging ++++


This was to prevent breaking the Arrow flow until we know for sure the metadata is saved on the Parquet files correctly. But this is probably not needed since that was tested already on Arrow and this is now tested on DPBA flow, so will merge with it.

avalerio-tkd · 2025-11-24T01:51:31Z

            "test_user",       // user_id
-            "{}"               // application_context
+            "{}",               // application_context
+            {}                  // encryption_metadata


Thanks for catching the test gap. Added.

avalerio-tkd · 2025-11-24T02:28:46Z

    // The caching of this is possible because the API response `response_` doesn't change after construction.
    mutable std::string cached_error_message_;
    mutable std::map<std::string, std::string> cached_error_fields_;
+    mutable std::optional<std::map<std::string, std::string>> cached_encryption_metadata_;


Actually, this is the RemoteEncryptionResult, not the RemoteDPBA. A new instance is returned on each call to RemoteDBPA.Encrypt().

This is cached separately on each RemoteEncryptionResult instance, to avoid parsing the Json with every call to encryption_metadata(). It isn't statically cached for the class.

But let me know if something is unclear, and we can discuss.

avalerio-tkd · 2025-11-24T02:47:17Z

 using namespace dbps::enum_utils;

+namespace {
+    constexpr const char* DBPS_VERSION_KEY = "dbps_version";


Renamed it to dbpa_agent_version as suggested.

avalerio-tkd · 2025-11-24T02:50:30Z

+        std::cerr << "ERROR: EncryptionSequencer - encryption_metadata['" << DBPS_VERSION_KEY << "'] must start with '" 
+                  << DBPS_VERSION_VALUE << "', but got '" << it->second << "'" << std::endl;


argmarco-tkd

Thanks for the changes. Left a couple of comments - in particular, I think we need a bit of clarification/improved description on the 'cache' for metadata.

argmarco-tkd · 2025-11-24T17:33:20Z

 using namespace dbps::external;

+namespace {
+    const std::map<std::string, std::string> VALID_ENCRYPTION_METADATA = {{"dbps_agent_version", "v0.01"}};


nit: "Valid" hints that there is a counterpart, "invalid". Consider a different name. (maybe just a plain ENCRYPTION_METADATA or DBPS_ENCRYPTION_METADATA?)

argmarco-tkd · 2025-11-24T17:39:39Z

+        const auto& response_attrs = response_->GetResponseAttributes();
+        if (!response_attrs.encryption_metadata_.empty()) {
+            cached_encryption_metadata_ = response_attrs.encryption_metadata_;
+        } else {
+            cached_encryption_metadata_ = std::nullopt;
+        }


it's a clearer, thanks - but the whole concept of 'cached metadata' is a bit confusing. maybe let's add a short note on that? (in particular, why is it 'cached' ?)

…tionResult for clarity. - Added DBPS_ENCRYPTION_METADATA constant to dbpa_local_test.cpp for clarity.

avalerio-tkd · 2025-11-27T21:13:48Z

@argmarco-tkd @sofia-tekdatum could you PTAL? I renamed the cached variables to parsed that I think captures the intention best, but let me know if we'd like another clarification for those.

argmarco-tkd

LGTM - thanks for all the changes!

avalerio-tkd requested review from argmarco-tkd and sofia-tekdatum November 19, 2025 01:58

sofia-tekdatum reviewed Nov 19, 2025

View reviewed changes

- Small updates on version checking.

58fc43d

- Updated dbpa_remote_testapp to use the new version checking.

avalerio-tkd mentioned this pull request Nov 19, 2025

DBPS > Wire in column encryption metadata #147

Closed

- Make version checking on server a server error failure.

53926fc

argmarco-tkd reviewed Nov 19, 2025

View reviewed changes

Merge branch 'main' into av_wire_encr_meta_05

02023d2

- Updating devel branch

avalerio-tkd commented Nov 24, 2025

View reviewed changes

- Updates from review comments.

3d793f0

argmarco-tkd reviewed Nov 24, 2025

View reviewed changes

- Renamed cached to parsed in RemoteEncryptionResult and RemoteDecryp…

dc9a956

…tionResult for clarity. - Added DBPS_ENCRYPTION_METADATA constant to dbpa_local_test.cpp for clarity.

argmarco-tkd approved these changes Nov 27, 2025

View reviewed changes

- Removing TODO note as this is now implemented.

f9f1740

avalerio-tkd merged commit 9bf39b6 into main Nov 28, 2025
2 checks passed

avalerio-tkd deleted the av_wire_encr_meta_05 branch November 28, 2025 00:09

		std::cerr << "ERROR: EncryptionSequencer - encryption_metadata['" << DBPS_VERSION_KEY << "'] must start with '"
		<< DBPS_VERSION_VALUE << "', but got '" << it->second << "'" << std::endl;

Uh oh!

Conversation

avalerio-tkd commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

argmarco-tkd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avalerio-tkd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

argmarco-tkd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment