Wiring DBPAgent into ExternalEncryptorImpl/ExternalDecryptor impl by argmarco-tkd · Pull Request #68 · protegrity/arrow

argmarco-tkd · 2025-08-20T06:24:55Z

Rationale for this change

In this work, we're wiring the shared library (aka DLL) -loaded DBPAgent instance into both ExternalEncryptorImpl and ExternalDecryptorImpl (EEI and EDI, respectively). We're using the miniApp branch for this.

As part of this change, the previous implementations of EEI and EDI are being over-written.

As a reminder - this particular PR will never be merged into dev_phase2 nor into main - an equivalent change will be made once the refactoring work happening in dev_phase2 is complete.

What changes are included in this PR?

Addition of dbpa_utils.h and dbpa_utils.cc as utils to help translate concepts between the Arrow and DBPA namespace.
Heavily modified ExternalEncryptorImpl and ExternalDecryptorImpl (inside encryption_internal.cc and encryption_internal.h).
- In particular, we modified
  - Constructors
  - Implementation of the ::Make method
  - Implementations of ::Encrypt() ::Decrypt() CiphertextLength() and PlaintextLength()
  - Eliminated the underlying usage of AESEncryptor/AESDecryptor
- We did not modify
  - The EncryptorInterface and DecryptorInterface definitions (again, this is the mini-App. We left them as-is).

Are these changes tested?

Mostly.
- The encryption workflow works (verified using the miniApp).
- Existing unit tests pass
The Decryption workflow is untested, but that's because a larger change in the Python->Cython->C++ change is required (coming in a later PR).

In theory we won't use this encryptor for footers or anything else. Not sure what the datatype would be if any? In that case, should we just throw a NotImplementedExc or something like that?

@sofia-tekdatum thoughts? Do you know if this method is required if we only encrypt column chunks, not metadata?

This is part of the signature verification done when a parquet file is going to be decrypted.

So we are going to need to implement it, I'll have more details when I finish one of the tasks in this sprint related to that.

This is remnant of the mini-app code. From what I understand, this (SignedFooterEncrypt()) won't be part of the final solution. The method is used for footer verification inside parquet/metadata.cc, via direct instantiation of AesEncryptor). Created a task to discuss, as this affects the definition of EncryptorInterface but is not exactly relevant to this PR.

https://github.com/protegrity/DataBatchProtectionService/issues/74

Perfect. Thanks for the tracking task!

avalerio-tkd · 2025-08-20T14:44:26Z

+  //TODO
+  // This is not production code. We know that the one DPBA Agent we have uses XOR encryption.
+  // Therefore it's safe to assume that the ciphertext length is the same as the plaintext length.
+  // This is not true for all DPBA Agents.


avalerio-tkd · 2025-08-20T14:46:08Z

-  std::cout << "Created ExternalDecryptorImpl" << std::endl;
+ExternalDecryptorImpl::ExternalDecryptorImpl(
+  std::unique_ptr<DataBatchProtectionAgentInterface> agent_instance)
+    : agent_instance_(std::move(agent_instance)) {


So the ExternalEncrypt/Decrypt will share an instance of the DBPA ? Can we add a comment on that if it's the case?

They do not and will not share an instance (at least that's the current plan). Each (EEI, EDI) will make a call to load an independent instance from the DLL.

This does not stop us from having each agent instance share state (via class variables or similar).

Added an FAQ entry (based on earlier discussion on the topic) here

happy to discuss.

Thanks for the FAQ. We can chat offline. Not a blocker for this PR.

avalerio-tkd · 2025-08-20T14:48:48Z


-    static std::unique_ptr<ExternalDecryptorImpl> Make(ParquetCipher::type alg_id, int32_t key_len,
-                                                       bool metadata);
+    static std::unique_ptr<ExternalDecryptorImpl> Make(


I have a mental note that we needed to talk about the params of Make, but now I think is ok, right? Let us know if we need to talk further. If not, all good.

I think we're good (i.e. no discussion needed :) ). The parameters here will change once we have the final implementation - a lot of the current params are just vestigial from the miniapp code.

sofia-tekdatum · 2025-08-20T15:13:22Z

-    static std::unique_ptr<ExternalDecryptorImpl> Make(ParquetCipher::type alg_id, int32_t key_len,
-                                                       bool metadata);
+    static std::unique_ptr<ExternalDecryptorImpl> Make(
+      ParquetCipher::type alg_id, int32_t key_len, std::string column_name, Type::type data_type, 


Ok, so if I understand the pattern, you're intending for the Arrow code to call this Make, and then within the Make we call the constructor of the DBPAgentInterface, right?

Just checking because I'd rather not leak any of those classes out where the encryptors are created.

The objective here is for any ExternalEncryptorImpls (final name TBD) to be instantiated the same - regardless of what they do underneath. This should (hopefully) be the same instantiation mechanism as for other encryptors/decryptors. Once the instantiation mechanism is invoked, EEI will internally do its thing.

For this PR (given that the changed to EEI and EDI are mostly throaway) I wanted to minimize the amount of changes and piggy-backed as much as possible onto the current ::Make builder method.

Whether the instantiation happens via ::Make() or via a constructor is somewhat irrelevant (but I believe Arrow currently prefers the utilization of the ::Make() builder method).

sofia-tekdatum · 2025-08-20T15:14:34Z

-                                                       bool metadata);
+    static std::unique_ptr<ExternalDecryptorImpl> Make(
+      ParquetCipher::type alg_id, int32_t key_len, std::string column_name, Type::type data_type, 
+      Compression::type compression_type, Encoding::type encoding, std::string ext_column_key,


Following the discussion we had over this doc:
https://docs.google.com/document/d/1goKWGR19UsVyXXQrxUPUI-1GBSxN6Yc3qEp2gykVYrg/edit?tab=t.0#heading=h.rdqcrfsd11cy

Encoding is not something you're expecting from Arrow?

Different type of encoding :)

In that document, the 'encoding' (under the "Format and Encoding" section) refers to a network-transport concept, which is only pertinent to the DBPSClient <-> DBPServer communication.

The Encoding here (in the ::Make) defines how data is physically encoded in the Parquet files.

@avalerio-tkd - please verify.

I think Marco is right, but also not sure how this Encoding::type encoding propagates on Make. Let me check offline with Sofi and will confirm.

sofia-tekdatum · 2025-08-20T15:16:34Z

+  std::cout << "  compression_type: " << CompressName(compression_type) << std::endl;
+  std::cout << "  encoding: " << encoding << std::endl;
+  std::cout << "  ext_column_key: " << ext_column_key << std::endl;
+  std::cout << "  user_id: " << user_id << std::endl;


Correct, but this being miniapp code, I think it's around there.

sofia-tekdatum · 2025-08-20T15:21:48Z

+  std::cout << "  encrypted_footer buffer size: " << encrypted_footer.size() << " bytes" << std::endl;
+
+  std::cout << "[DEBUG] Calling agent_instance_->Encrypt for footer..." << std::endl;
+  std::unique_ptr<EncryptionResult> result = agent_instance_->Encrypt(footer);


This is part of the signature verification done when a parquet file is going to be decrypted.

So we are going to need to implement it, I'll have more details when I finish one of the tasks in this sprint related to that.

avalerio-tkd · 2025-08-20T17:16:36Z

@argmarco-tkd let me confirm with Sofi about the Encoding param in Make. Otherwise it LGTM.

avalerio-tkd

LGTM+. Will sync with Sofi about the Encoding and we can revisit if needed.

github-actions Bot added Component: C++ Component: Parquet awaiting review labels Aug 20, 2025

argmarco-tkd force-pushed the dll_work_eei_impl branch 2 times, most recently from 9b99922 to 5786d51 Compare August 20, 2025 06:42

argmarco-tkd changed the title ~~Dll work eei impl~~ Wiring DBPAgent into ExternalEncryptorImpl/ExternalDecryptor impl Aug 20, 2025

Modified ExternalEncryptorImpl to make use of the DBPAgentImpl

e3a1a02

argmarco-tkd force-pushed the dll_work_eei_impl branch from 5786d51 to e3a1a02 Compare August 20, 2025 07:04

argmarco-tkd marked this pull request as ready for review August 20, 2025 07:12

argmarco-tkd assigned sofia-tekdatum, avalerio-tkd and cristekdatum Aug 20, 2025

avalerio-tkd requested review from avalerio-tkd, cristekdatum and sofia-tekdatum August 20, 2025 13:27

avalerio-tkd unassigned sofia-tekdatum, avalerio-tkd and cristekdatum Aug 20, 2025

avalerio-tkd reviewed Aug 20, 2025

View reviewed changes

github-actions Bot added awaiting committer review and removed awaiting review labels Aug 20, 2025

sofia-tekdatum reviewed Aug 20, 2025

View reviewed changes

avalerio-tkd mentioned this pull request Aug 20, 2025

Encryption features > Discussion on SignedFooterEncrypt #69

Closed

avalerio-tkd approved these changes Aug 20, 2025

View reviewed changes

argmarco-tkd merged commit e3a1a02 into dev-miniapp Aug 20, 2025
37 of 104 checks passed

argmarco-tkd mentioned this pull request Sep 4, 2025

Modifying ExternalDBPAEncryptor/DecryptorAdapter to obtain agent instance from shared lib (DLL) #119

Merged

               namespace parquet::encryption {
+              // Forward declarations for utility functions
+              std::string HackTypeToString(Type::type t);

+                if (!result->success()) {
+                  std::cout << "[ERROR] Encryption failed: " << result->error_message() << std::endl;
+                  throw ParquetException(result->error_message());

Uh oh!

Conversation

argmarco-tkd commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Related

Uh oh!

github-actions Bot commented Aug 20, 2025

Uh oh!

avalerio-tkd commented Aug 20, 2025

Uh oh!

avalerio-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

Uh oh!

avalerio-tkd Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

avalerio-tkd commented Aug 20, 2025

Uh oh!

avalerio-tkd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

argmarco-tkd commented Aug 20, 2025 •

edited

Loading

avalerio-tkd Aug 20, 2025 •

edited

Loading