[Abandoned-Replaced] Machinery for loading of dynamic libraries. #36

argmarco-tkd · 2025-08-07T23:20:30Z

Rationale for this change

In this PR, I'm creating the machinery (utils classes) for

Loading an implementation of the DataBatchProtectionAgent from a shared library (e.g. *.so file)
Wrapping that implementation in a Decorator which will ensure the shared library handle will be properly disposed of.

These changes are described in this doc

What changes are included in this PR?

Mostly new files + their respective tests.
A relatively important, but hopefully not controversial change is the addition of the "CloseDynamicLibrary" function to Arrow's io_util.cc - the same file which already contained functionality for dealing with dynamic libraries.

Are these changes tested?

Yes - unit tests have been created.

Additional Notes

The interface for DataBatchProtectionAgentInterface is still being worked on. I have created a temporary interface (parquet/encryption/external/DBPAInterface.h), which hopefully is close-enough to the final version. Once the interface is finalized, we will incorporate it here, and send the final PR.
This PR is of the changes between two branches: dev_dll_work (feature branch) and dev-miniapp
Github is being special and telling me that these files have diffs, when they are identical. Please ignore: Dockerfile.miniApp, build-clean.sh, build.sh, build-tests.sh
Please ignore the changes to internal_file_encryptor.cc - this diff will dissappear in the final (i.e. non-draft) PR.

…cryptor*

…yptor related stuff. Begin moving things around.

…. It has its own .so file now.

…s is done to facilitate testing

…ry_wrapper now pass.

…ader stuff - needed for refactoring

…of DBPALibraryWrapper

…f LoadableEncryptorInterface

…brary stuff with arrow-provided libraries.

github-actions · 2025-08-07T23:21:27Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

avalerio-tkd

Thanks for the changes. The PR quite large. I gave it an initial look and we can discuss further offline for my comments or to dig dipper.

avalerio-tkd · 2025-08-08T01:16:46Z

cpp/src/parquet/encryption/CMakeLists.txt

+)
+
+# DBPATestAgent configuration
+target_link_libraries(DBPATestAgent PUBLIC


Are these "test" targets the unittests for the loading module or a temporary testing module we're adding as an intermediate step?

This is a test target for unit testing. The library build here will never be 'released' (this is the reason that the library does not have an 'install' target)

avalerio-tkd · 2025-08-08T01:17:25Z

cpp/src/parquet/encryption/crypto_factory.cc

    const KmsConnectionConfig& kms_connection_config,
    const DecryptionConfiguration& decryption_config, const std::string& file_path,
    const std::shared_ptr<::arrow::fs::FileSystem>& file_system) {
+  std::cout << "Getting file decryption properties!!" << std::endl;


Add TODO note to remove printouts or move to logging.debug. Same elsewhere.

file should not be here, but will do where pertinent.

avalerio-tkd · 2025-08-08T01:20:22Z

cpp/src/parquet/encryption/encryption_internal.cc


 int32_t ExternalDecryptorImpl::Decrypt(span<const uint8_t> ciphertext, span<const uint8_t> key,
                                       span<const uint8_t> aad, span<uint8_t> plaintext) {
+  std::cout << "ExternalDecryptorImpl::Decrypt called" << std::endl;


Now that I'm thinking: Are these your comments or some printouts you inherited when rebasing your client. If these are not yours, may be you'd like to rebase again so the diff are only your changes?

yeah. There was a bit of github trickery that went into this PR. There are a lot more diffs than I would have expected - not because of file changes, but because of my branching setup + added to this: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-comparing-branches-in-pull-requests

avalerio-tkd · 2025-08-08T01:21:07Z

cpp/src/parquet/encryption/encryption_internal.cc

  std::cout << "Here I would call the external decryption service. Hold for params." << std::endl;
 }

+


Nit: extra whitespaces. Same elsewhere.

avalerio-tkd · 2025-08-08T01:22:27Z

cpp/src/parquet/encryption/external/dbpa_interface.h

@@ -0,0 +1,47 @@
+// Licensed to the Apache Software Foundation (ASF) under one


I'd remove the license note for now and replace with a TODO to find the right License note later. We can also put something in the backlog regarding this if you'd like.

Good call. Will do. Both.

avalerio-tkd · 2025-08-08T01:22:37Z

cpp/src/parquet/encryption/external/dbpa_library_wrapper.cc

@@ -0,0 +1,95 @@
+// Licensed to the Apache Software Foundation (ASF) under one


Same here for the license.

avalerio-tkd · 2025-08-08T01:26:54Z

cpp/src/parquet/encryption/external/loadable_encryptor.h

+
+// Interface for loadable encryptors that can be dynamically loaded from shared libraries
+// This extends the base EncryptorInterface with initialization capabilities
+class PARQUET_EXPORT LoadableEncryptorInterface : public parquet::encryption::EncryptorInterface {


Is this the EncryptorInterface from the mini-app initial refactor, or is this a new one? Should this be added when the refactor is done instead of here?

this should not be here.

avalerio-tkd · 2025-08-08T01:31:34Z

cpp/src/parquet/encryption/internal_file_encryptor.cc

-        encryption_properties->app_context(), false);
-    } else {
+      //TODO: move this elsewhere.
+      bool use_dll_encryptor = false;


This is a bit hardcoded at the moment it seems. It's ok if it is, let's just add a short comment on what should look like when integrated with dev_phase2 branch.

argmarco-tkd · 2025-08-08T03:03:09Z

Thanks for the comments, @avalerio-tkd. I'm closing this PR, as it contains more files than it should and the PR is a bit messy, I'll admit. (I'm blaming the tooling this time). Will address your comments and send out a new, cleaner PR.

This is the new PR: #37

argmarco-tkd and others added 29 commits July 8, 2025 10:47

Added DLLEncryptor. Currently unused.

216bd5e

creating empty DLL Encryptor. Some wiring.

5059384

DLLEncryptor succesfully loaded (via the Loader) from the mini-app.

a35c9ac

Refactored and separated the DLLEncryptorLoader from DLLEncryptor impl

a61d2a9

Refactoring. Began the renaming of DLLEncryptorLoader into LoadableEn…

9717f7e

…cryptor*

Extracted LoadableEncryptorInterface into its own file.

e99e3e9

adding a few utils to Dockerfile.miniApp

323e608

adding parallelism to ./ninja to the helper build scripts

30a2b35

created the parquet/encryption/[external] folder to put external encr…

958f8a9

…yptor related stuff. Begin moving things around.

First pass at implementing DBPALibraryWrapper - tests not added yet.

6243e77

cleaning up the impl of LoadFromLibrary()

d391d71

Defined DBPATestAgent as a simple agent for testing loading machinery…

edff151

…. It has its own .so file now.

moving DBPALibraryWrapper into its own .h and .cc files. No tests yet.

6c8f265

Adding the option to pass a function to close the shared library. Thi…

a62b405

…s is done to facilitate testing

Reviewed and improved the testing suite. All the tests for dbpa_libra…

0248ef6

…ry_wrapper now pass.

Reviewed and improved the testing suite. All the tests for dbpa_libra…

1db72b6

…ry_wrapper now pass.

ensuring that WIN32 stuff throws an exception (unimplemented)

3bf7406

commented out the references to loadble_encryptor_utils and to DLL lo…

3b09ef3

…ader stuff - needed for refactoring

Added a default value (function) for the handle_closing_fn parameter …

4d52fc7

…of DBPALibraryWrapper

Modified LoadableEncryptorUtils to work with DBPAInterface, instead o…

e91a805

…f LoadableEncryptorInterface

Adding tests for loadable_encryptor_utils

7d5b847

Added CloseDynamicLibrary() to arrow/util/io_util.cc

300ddb3

Replaced the "custom" usage of dlopen, dlsym, etc to handle Shared Li…

bdafe6c

…brary stuff with arrow-provided libraries.

better casting

c33df5c

Added better heuristics to find the *.so file during tests

be838ef

Removed references to DLLEncryptor (sources, CMake configs)

08b817f

working

99a0d64

closing decryption loop in miniapp. super hacky code, pls ignore

7beec95

Removing unnecessary prints

4747aef

argmarco-tkd requested a review from sofia-tekdatum August 7, 2025 23:20

argmarco-tkd requested review from avalerio-tkd and cristekdatum August 7, 2025 23:20

argmarco-tkd changed the title ~~[Draft Review] Machinery for loading of dynamic libraries.~~ [Draft] Machinery for loading of dynamic libraries. Aug 7, 2025

github-actions bot added the awaiting review label Aug 7, 2025

avalerio-tkd reviewed Aug 8, 2025

View reviewed changes

argmarco-tkd closed this Aug 8, 2025

argmarco-tkd added a commit that referenced this pull request Aug 8, 2025

Some cleanup after feedback in draft PR: #36

d2c48f8

argmarco-tkd mentioned this pull request Aug 8, 2025

Machinery for loading external Agent (encryptor) out of a shared library #37

Merged

argmarco-tkd changed the title ~~[Draft] Machinery for loading of dynamic libraries.~~ [Abandoned-Replaced] Machinery for loading of dynamic libraries. Sep 2, 2025

		std::cout << "Here I would call the external decryption service. Hold for params." << std::endl;
		}

		@@ -0,0 +1,47 @@
		// Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,95 @@
		// Licensed to the Apache Software Foundation (ASF) under one

[Abandoned-Replaced] Machinery for loading of dynamic libraries. #36

[Abandoned-Replaced] Machinery for loading of dynamic libraries. #36

Uh oh!

Conversation

argmarco-tkd commented Aug 7, 2025

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Additional Notes

Uh oh!

github-actions bot commented Aug 7, 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!

argmarco-tkd Aug 8, 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!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

argmarco-tkd Aug 8, 2025 •

edited

Loading

argmarco-tkd commented Aug 8, 2025 •

edited

Loading