GH-31603: [C++] Wrap Parquet encryption keys in SecureString #46017

EnricoMi · 2025-04-03T09:50:21Z

Rationale for this change

Cryptographic keys must be kept private. Using the new arrow::util::SecureString is vital for storing secrets securely.

What changes are included in this PR?

Uses the arrow::util::SecureString introduced in #46626 for cryptographic keys throughout Parquet encryption.

Are these changes tested?

Unit tests.

Are there any user-facing changes?

APIs that hand over secrets to Arrow require the secret to be encapsulated in a SecureString.

This PR includes breaking changes to public APIs.

TODO:

provide instructions for migration

Supersedes #12890.

GitHub Issue: [C++][Parquet] WipeOutDecryptionKeys doesn't securely wipe out keys #31603

github-actions · 2025-04-03T09:50:50Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

github-actions · 2025-04-03T09:52:19Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

github-actions · 2025-04-03T10:00:11Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

pitrou · 2025-04-03T10:11:18Z

@EnricoMi Do you want a preliminary review on this?

pitrou · 2025-04-03T10:23:12Z

For the record, OpenSSL allows allocating memory from a secure heap, but that requires calling process-wide initialization functions. I've asked about this.

EnricoMi · 2025-04-03T13:25:03Z

@EnricoMi Do you want a preliminary review on this?

Yes, please! This is in a complete PoC state (Linux build and test is green).

Is this the right direction before I roll this out to Python?
Can I break the API or should I deprecate exiting methods and provide both, std::string and secure string methods?
Do you have some hints how I can fix the link issues for Windows? I presume I need to add the secure_string.cc file somewhere so Windows links to the object file.
Shouldn't it be sufficient to only use OPENSSL_cleanse and count on OpenSSL providing OS-dependent implementations?

EnricoMi · 2025-04-03T13:29:20Z

I agree, that OPENSSL_secure_malloc seems problematic. What if the application crashes (or receives kill -9) and CRYPTO_secure_malloc_done cannot be called. Are secrets then not protected? If secrets are protected without CRYPTO_secure_malloc_done, and multiple calls to OPENSSL_secure_malloc are idempotent, then this would be a good way to go.

pitrou · 2025-04-03T14:11:16Z

Is this the right direction before I roll this out to Python?

Yes!

Can I break the API or should I deprecate exiting methods and provide both, std::string and secure string methods?

I would rather deprecate the existing methods.

Do you have some hints how I can fix the link issues for Windows?

You probably want to add a PARQUET_EXPORT annotation to the SecureString class?

Shouldn't it be sufficient to only use OPENSSL_cleanse and count on OpenSSL providing OS-dependent implementations?

Well, currently the OpenSSL function is very crude :)
https://github.com/openssl/openssl/blob/c66e00398c9feabc02ff6e678089a3dc95f985d2/crypto/mem_clr.c#L18-L25

If secrets are protected without CRYPTO_secure_malloc_done, and multiple calls to OPENSSL_secure_malloc are idempotent, then this would be a good way to go.

What I'm worried about is different libraries in the same process all trying to use the CRYPTO_secure_malloc APIs, or perhaps even one library calling CRYPTO_secure_malloc_done while another library is still using those API calls.

cpp/src/parquet/encryption/secure_string.h

pitrou · 2025-04-03T15:37:13Z

cpp/src/parquet/encryption/secure_string_test.cc

For more stringent testing, I think we also want to examine the string area before it is cleared (or moved, etc.).

Something like (untested):

std::string_view StringArea(const std::string& string) { return std::string_view(string.data(), string.capacity()); } void AssertSecurelyCleared(std::string_view area) { // the entire area is filled with zeros std::string zeros(area.size(), '\0'); ASSERT_EQ(area, std::string_view(zeros)); } void AssertSecurelyCleared(const std::string& string) { AssertSecurelyCleared(StringArea(string)); } TEST(TestSecureString, SecureClearCheck) { // short string { std::string tiny("abc"); auto old_area = StringArea(tiny); SecureString::SecureClear(tiny); AssertSecurelyCleared(tiny); AssertSecurelyCleared(old_area); } // etc.

But tiny.data() and old_area.data() point to the same address, what is the point testing old_area? Just in case pointers change? If they would change, then old_area would point to a freed memory area, right?

Sorry @EnricoMi , I realize I forgot to reply to this comment:

Just in case pointers change? If they would change, then old_area would point to a freed memory area, right?

Right, but the freed memory area was not necessarily cleaned up before being freed (unless the allocator was built in debug mode perhaps). That's why it's important to check that the old area doesn't contain the sensitive secrets anymore.

pitrou · 2025-04-03T15:49:15Z

cpp/src/parquet/encryption/secure_string_test.cc

Probably also want a test that a SecureString was cleared on destruction ?

How would I do that, the memory where SecureString.as_span points to will be freed on deconstruction. How can I access that memory securely?

The standardese "true" way would be to template SecureString on an allocator so that you could intercept destruction of the bytes it stores. That's entirely too much here; we could just add a debug assertion inside ~SecureString (maybe only enabled by an env var or #if defined(ARROW_ASSERT_SECURE_STRING_ZEROED))

Oh, alternatively: since every STL I'm aware of implements std::string with an inline short string case, we could use:

// Most std::string impls store their contents inline, so we can check // the object representation for a crib to ensure that destruction does // wipe all sensitive bytes. constexpr auto kCrib = "01CRIB01"; alignas(SecureString) char object_repr[sizeof(SecureString)]; std::string_view object_repr_view{object_repr, sizeof(object_repr)}; SecureString* cribbed = new (&object_repr) SecureString(std::string(kCrib)); bool crib_found_before = object_repr_view.find(kCrib) != object_repr_view.npos; cribbed->~SecureString(); // NOTE: do not allow this call to be skipped! bool crib_found_after = object_repr_view.find(kCrib) != object_repr_view.npos; if (crib_found_before) { ASSERT_FALSE(crib_found_after); }

I think that's well-defined: https://godbolt.org/z/31cY59EM3

How would I do that, the memory where SecureString.as_span points to will be freed on deconstruction. How can I access that memory securely?

Depends what you call "securely", but in most cases (unless running under Valgrind or Address Sanitizer), you can still access a deallocated area. We should just try to do that here.

(this is morally the same as with the old area comment, by the way)

cpp/src/parquet/encryption/encryption.h

pitrou · 2025-04-03T16:05:12Z

cpp/src/parquet/encryption/encryption.h

Is empty_string_ useful at all? It looks rather dubious.

It is, method FileDecryptionProperties::column_key returns an empty string as the encryption key when the key id is not found. It should rather throw a meaningful key not found exception. I may fix that while I am touching this code anyway.

This looks like a holdover from the first draft of encryption; IIRC column_key() returned references to strings at that time (so return ""; would fail to compile)

Still needed, improved name.

cpp/src/parquet/encryption/encryption.h

bkietz · 2025-04-04T16:57:56Z

cpp/src/parquet/encryption/encryption.h

This looks like a holdover from the first draft of encryption; IIRC column_key() returned references to strings at that time (so return ""; would fail to compile)

cpp/src/parquet/encryption/file_key_wrapper.cc

cpp/src/parquet/encryption/internal_file_decryptor.cc

bkietz · 2025-04-04T17:06:10Z

cpp/src/parquet/encryption/read_configurations_test.cc

const seems warranted here

Suggested change

SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

const SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

Doesn't that hold for all k... members in this class (TestDecryptionConfiguration) and TestEncryptionConfiguration as well? Is it worth it given this are tests?

bkietz · 2025-04-04T17:10:47Z

cpp/src/parquet/encryption/secure_string.cc

It seems questionable to me that the ultimate fallback should not be a simple loop

Suggested change

__asm__ __volatile__("" ::"r"(data) : "memory");

for (size_t i = 0; i < size; ++i) {

data[i] = 0;

}

I don't know how cross platform this __asm__ block is.

Also, since this code is borrowed from another project it needs to be mentioned in LICENSE.txt, for example

arrow/LICENSE.txt

Lines 1541 to 1543 in 7df396e

A function gettimeofday in utilities.cc is based on

http://www.google.com/codesearch/p?hl=en#dR3YEbitojA/COPYING&q=GetSystemTimeAsFileTime%20license:bsd

What about

volatile char *p = data; while (size--) *p++ = 0;

https://cplusplus.com/articles/ETqpX9L8/

Given https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h is CC0 Public Domain Dedication, and our code deviates a lot, you could say it is as much inspired by that implementation than it is by all those variations available on stackoverflow.

I don't think this qualifies as a copy or derived work.

If it's in the public domain, then I imagine an inline comment referencing that should be sufficient. @pitrou do you agree? We do also account for some public domain artifacts in LICENSE.txt

We could add a comment, yes.

@pitrou @bkietz

I have added more comments and links to prior art. I have also wrapped the __asm__ to enable it only for GCC and Clang: 510349c

We have two options for the fallback branch:

the current memset implementation

the while loop below

Both benefit from the __asm__ __volatile__ call.

// Volatile pointer to the data is an attempt to avoid // that the compiler optimizes away the while loop. // https://cplusplus.com/articles/ETqpX9L8/ // http://stackoverflow.com/a/13299459/965097 // https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/ volatile char *p = data; while (size--) *p++ = 0;

I think the while loop is preferable; it's no more lines of code and is probably more obvious to readers.

Currently the memset_v block doesn't exclude the inline asm block

arrow/cpp/src/arrow/util/secure_string.cc

Lines 123 to 127 in 219d207

// https://github.com/openssl/openssl/blob/3423c30db3aa044f46e1f0270e2ecd899415bf5f/crypto/mem_clr.c#L22

static const volatile auto memset_v = &memset;

memset_v(data, 0, size);

# if defined(__GNUC__) || defined(__clang__)

I'd recommend rewriting to

#if (inline_asm_conditions) // inline asm #else // while loop #endif

the compiler could ordinarily be relied on to omit the redundant write, but since we're handling volatile memory we'll probably generate code which zeroes twice

I'd prefer the memset_v as it is more performant. Either memset or loop will require verbose comment to explain the meaning of the volatile, so I think both are similar "obvious" to readers.

I don't understand your comment on the inline asm block. The asm block is a NOOP, it does not zero any memory. It is merely there to tell the compiler that it cannot optimize away any code that modified data before this line. So it is not either asm or loop, it is either (memset or loop) and asm.

cpp/src/parquet/encryption/secure_string.h

bkietz · 2025-04-04T17:17:37Z

cpp/src/parquet/encryption/secure_string_test.cc

The standardese "true" way would be to template SecureString on an allocator so that you could intercept destruction of the bytes it stores. That's entirely too much here; we could just add a debug assertion inside ~SecureString (maybe only enabled by an env var or #if defined(ARROW_ASSERT_SECURE_STRING_ZEROED))

bkietz · 2025-04-07T14:53:02Z

cpp/src/parquet/encryption/secure_string.h

Would you please move this to arrow/util/? I think it will be useful outside parquet

I have extracted the SecureString implementation into a separate PR: #46626. That targets arrow/util.

This PR will focus in migrating parquet encryption to that SecureString implementation.

EnricoMi · 2025-04-11T11:05:01Z

cpp/src/parquet/encryption/encryption.cc

This looked suspicious, all other setter methods check if the member is unset.
Here, the check is always true as it checks the input.

This is a breaking change if user code calls this setter twice with non-empty keys.

I think you've corrected a bug; the intention with these setter methods seems to be one-time setting

cpp/src/parquet/encryption/encryption.cc

EnricoMi · 2025-04-11T11:14:38Z

cpp/src/parquet/encryption/encryption.cc

we are still returning a reference to a SecureString. I tried to return some std::optional, to replace empty SecureString notation for "no key given" with std::nullopt, but this opened a can of worms. Not going down this rabbit whole in this PR.

At least the naming is improved.

Another use for a generic default reference: #46303 (comment)

EnricoMi · 2025-07-07T19:52:12Z

@pitrou I have removed the deprecated methods to "encourage" users to migrate to the new methods: 290449a

But this will require migrating the Python API as well:
https://github.com/apache/arrow/actions/runs/16126071702/job/45503157743?pr=46017#step:6:3956

I would like to move touching the Python API to a separate PR, which would contain the revert 290449a.

pitrou · 2025-07-08T11:52:08Z

But this will require migrating the Python API as well: https://github.com/apache/arrow/actions/runs/16126071702/job/45503157743?pr=46017#step:6:3956

I would like to move touching the Python API to a separate PR, which would contain the revert 290449a.

Ok, that sounds good to me.

EnricoMi · 2025-07-08T13:05:32Z

But this will require migrating the Python API as well: https://github.com/apache/arrow/actions/runs/16126071702/job/45503157743?pr=46017#step:6:3956
I would like to move touching the Python API to a separate PR, which would contain the revert 290449a.

Ok, that sounds good to me.

Alright, once you approve this PR, I will revert 290449a and we are ready to merge.

pitrou

LGTM except two minor comments

pitrou · 2025-07-09T11:27:01Z

cpp/src/parquet/encryption/encryption.h


+  // any empty SecureString key is interpreted as if no key is given
+  // this instance is used if a SecureString reference is returned
+  const ::arrow::util::SecureString no_key_ = ::arrow::util::SecureString();


Does this need to be an instance variable? It could just be a private constant inside encryption.cc

Done in 184afcf.

pitrou · 2025-07-09T11:27:39Z

cpp/src/parquet/encryption/encryption.h

+  void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
+
+  ::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
+    // key_id_string is string but for IntegerKeyIdRetriever it encodes


I'm not sure it's worth inlining this method, is it? Especially as it will usually be called through the vtable, anyway.

@bkietz suggested the implementation given the signature is somewhat surprising: #46017 (comment)

Fair enough. I do find the implementation weird as well, and I'm not sure this class is actually useful, but hey :)

It is indeed surprising...

EnricoMi · 2025-07-09T12:52:15Z

Maybe @bkietz and @wgtmac want to make a final pass over the PR.

cpp/src/parquet/encryption/encryption.cc

wgtmac

I just took a quick pass on it. Thanks!

wgtmac · 2025-07-09T15:54:26Z

cpp/src/parquet/encryption/encryption.h

+  void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
+
+  ::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
+    // key_id_string is string but for IntegerKeyIdRetriever it encodes


It is indeed surprising...

pitrou · 2025-07-10T11:36:45Z

@EnricoMi We have two +1s after a meticulous review pass, so perhaps you can go ahead and revert the compatibility-breaking changes so that we get a clean CI and merge.

This reverts commit 290449a.

EnricoMi · 2025-07-10T18:25:52Z

✔️ All green.

cpp/src/parquet/encryption/encryption.h

pitrou · 2025-07-15T15:13:47Z

@github-actions crossbow submit -g cpp

github-actions · 2025-07-15T15:16:22Z

Revision: 04b15e2

Submitted crossbow builds: ursacomputing/crossbow @ actions-12f2531100

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-42-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Apr 3, 2025

EnricoMi mentioned this pull request Apr 3, 2025

EXP: SecureZero helper to securely clear memory #12890

Closed

pitrou reviewed Apr 3, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 3, 2025

EnricoMi force-pushed the secure-string branch 2 times, most recently from 9848539 to b303afa Compare April 4, 2025 16:29

bkietz requested changes Apr 4, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 4, 2025

bkietz mentioned this pull request Apr 7, 2025

GH-40343: [C++] Move S3FileSystem to the registry #41559

Merged

bkietz requested changes Apr 7, 2025

View reviewed changes

EnricoMi force-pushed the secure-string branch from b303afa to 4590921 Compare April 11, 2025 10:52

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 11, 2025

EnricoMi commented Apr 11, 2025

View reviewed changes

cpp/src/parquet/encryption/encryption.cc Outdated Show resolved Hide resolved

EnricoMi commented Apr 11, 2025

View reviewed changes

EnricoMi force-pushed the secure-string branch from 4590921 to 88d4bb0 Compare April 11, 2025 11:25

Wrap encryption keys in SecureString

5f73a1f

EnricoMi added 2 commits July 7, 2025 21:28

Use const in favour of inline strings

fa9abed

Remove deprecations to move users to more secure methods

290449a

EnricoMi force-pushed the secure-string branch from ab85c65 to 290449a Compare July 7, 2025 19:28

pitrou approved these changes Jul 9, 2025

View reviewed changes

Move no_key_ into encryption.cc

184afcf

EnricoMi requested a review from bkietz July 9, 2025 12:51

pitrou reviewed Jul 9, 2025

View reviewed changes

cpp/src/parquet/encryption/encryption.cc Outdated Show resolved Hide resolved

Rename no_key_

edd97b8

wgtmac approved these changes Jul 10, 2025

View reviewed changes

Revert "Remove deprecations to move users to more secure methods"

0a31f50

This reverts commit 290449a.

pitrou reviewed Jul 15, 2025

View reviewed changes

cpp/src/parquet/encryption/encryption.h Outdated Show resolved Hide resolved

pitrou added 2 commits July 15, 2025 17:08

Merge branch 'main' into secure-string

1987cda

Update deprecation notices

04b15e2

pitrou merged commit 6e5d143 into apache:main Jul 15, 2025
36 of 37 checks passed

pitrou removed the awaiting change review Awaiting change review label Jul 15, 2025

pitrou mentioned this pull request Jul 15, 2025

[C++][Parquet] WipeOutDecryptionKeys doesn't securely wipe out keys #31603

Closed

thisisnic mentioned this pull request Aug 7, 2025

[R] r-binary-packages nightly failures #47277

Closed

This was referenced Aug 15, 2025

[C++] Remove deprecated string-based Parquet encryption methods #47338

Closed

GH-47338: [C++][Python] Remove deprecated string-based Parquet encryption methods #47339

Merged

	SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;
	const SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

-  __asm__ __volatile__("" ::"r"(data) : "memory");
+  for (size_t i = 0; i < size; ++i) {
+    data[i] = 0;
+  }

	A function gettimeofday in utilities.cc is based on

	http://www.google.com/codesearch/p?hl=en#dR3YEbitojA/COPYING&q=GetSystemTimeAsFileTime%20license:bsd

	// https://github.com/openssl/openssl/blob/3423c30db3aa044f46e1f0270e2ecd899415bf5f/crypto/mem_clr.c#L22
	static const volatile auto memset_v = &memset;
	memset_v(data, 0, size);

	# if defined(__GNUC__) \|\| defined(__clang__)

GH-31603: [C++] Wrap Parquet encryption keys in SecureString #46017

GH-31603: [C++] Wrap Parquet encryption keys in SecureString #46017

Uh oh!

Conversation

EnricoMi commented Apr 3, 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?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

github-actions bot commented Apr 3, 2025

Uh oh!

pitrou commented Apr 3, 2025

Uh oh!

pitrou commented Apr 3, 2025

Uh oh!

EnricoMi commented Apr 3, 2025

Uh oh!

EnricoMi commented Apr 3, 2025

Uh oh!

pitrou commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

bkietz Apr 4, 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!

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!

Uh oh!

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!

EnricoMi Apr 11, 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!

EnricoMi commented Apr 3, 2025 •

edited

Loading

pitrou commented Apr 3, 2025 •

edited

Loading

bkietz Apr 4, 2025 •

edited

Loading

EnricoMi Apr 11, 2025 •

edited

Loading

bkietz Apr 7, 2025 •

edited

Loading

EnricoMi May 28, 2025 •

edited

Loading

EnricoMi Apr 11, 2025 •

edited

Loading