GH-47338: [C++][Python] Remove deprecated string-based Parquet encryption methods by EnricoMi · Pull Request #47339 · apache/arrow

EnricoMi · 2025-08-15T05:55:14Z

Rationale for this change

Passing encryption keys to Parquet via strings is insecure. Users are encouraged to use the SecureString based methods introduced in #46017 instead. To enforce this migration, deprecated methods are removed.

What changes are included in this PR?

Remove deprecated string-based methods.

Are these changes tested?

Existing Parquet encryption tests.

Are there any user-facing changes?

C++ user code that passes encryption keys to Parquet is affected and has to be adjusted. Python user code is not affected.

This PR includes breaking changes to public APIs.

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

github-actions · 2025-08-15T05:55:36Z

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

EnricoMi · 2025-08-15T05:59:12Z

@pitrou Here is the follow-up on migrating Parquet encryption to SecureStrings. The first commit has been approved as part of #46017: #46017 (comment)

There is no change to the Python API and only minimal glue-code for Python KMS client to work with the new C++ KMS client API. Rolling secure strings out to PyArrow is out of scope of this PR.

pitrou · 2025-08-19T10:10:58Z

+cdef extern from "Python.h":
+ # To let us get a PyObject* and avoid Cython auto-ref-counting
+ PyObject* PyBytes_FromStringAndSizeNative" PyBytes_FromStringAndSize"(
+ char *v, Py_ssize_t len) except NULL


I'm a bit surprised that this declaration is required. Cython already provides a declaration for this function and it would be surprised if it didn't have the right ref-counting behavior.
(also, by the way, if we require Cython 3.1 we will gain implicit std::string_view conversion to/from bytes objects)

Importing PyBytes_FromStringAndSize via from cpython.bytes cimport PyBytes_FromStringAndSize works.
I have copied this from io.pxi, maybe it is not needed there as well / any more:
arrow/python/pyarrow/io.pxi
Lines 43 to 46 in cdc1459
cdef extern from"Python.h":
# To let us get a PyObject* and avoid Cython auto-ref-counting
PyObject* PyBytes_FromStringAndSizeNative" PyBytes_FromStringAndSize"(
char*v, Py_ssize_t len) exceptNULL

pitrou · 2025-08-19T10:12:28Z

+ cdef:
+ cpp_string_view view = key.as_view()
+ key_bytes = PyObject_to_object(
+ PyBytes_FromStringAndSizeNative(view.data(), view.size()))


Of course this means the bytes object payload won't be cleared securely like the SecureString is.
We could try to tackle this as a followup issue/PR. bytes objects being immutable, this might be a bit delicate...

Ok, even the cryptography package doesn't do anything about it, so we may just have to live with it:
https://cryptography.io/en/latest/limitations/#secure-memory-wiping
However, cryptography does not clear memory by default, as there is no way to clear immutable structures such as bytes. As a result, cryptography, like almost all software in Python is potentially vulnerable to this attack. The CERT secure coding guidelines assesses this issue as “Severity: medium, Likelihood: unlikely, Remediation Cost: expensive to repair” and we do not consider this a high risk for most users.

Yes, I deliberately left this piece of work for a separate PR.
Happy to see this as "Likelihood: unlikely, Remediation Cost: expensive to repair" as well.

pitrou · 2025-08-19T10:16:13Z

+
+ cdef:
+ c_string cstr = tobytes(key)
+ shared_ptr[CSecureString] css = shared_ptr[CSecureString](new CSecureString(move(cstr)))


It'd be nice to find if modern Cython allows us to make use of make_shared. At least it has a declaration for it:
https://github.com/cython/cython/blob/b862800bda6cc5f540d3b7961350cd2b6ad00dbd/Cython/Includes/libcpp/memory.pxd#L106-L107

Works, this is much nicer, thanks!

pitrou · 2025-08-19T10:18:13Z

 cdef void _cb_unwrap_key(
 handler, const c_string& wrapped_key,
- const c_string& master_key_identifier, c_string* out) except *:
+ const c_string& master_key_identifier, shared_ptr[CSecureString]* out) except *:


I wonder why this is expecting a shared_ptr output, is it because of Cython's limited support for move-only types?

I made it work with CSecureString* out and CSecureString& out. What do you prefer?

pitrou · 2025-08-19T10:19:23Z

 key = handler.unwrap_key(wk_str, mkid_str)
- out[0] = tobytes(key)
+
+ cdef:


I wonder if a cdef: block is really required, especially as this is already a cdef function. Cython nowadays is more flexible than it used to be.

Cython doesn't like the definition of a C_string:
Error compiling Cython file: ------------------------------------------------------------ ... cdef void _cb_unwrap_key( handler, const c_string& wrapped_key, const c_string& master_key_identifier, CSecureString& out) except *: c_string cstr ^ ------------------------------------------------------------ pyarrow/_parquet_encryption.pyx:323:13: Syntax error in simple statement list
Without the cdef of c_string cstr, it fails with
Error compiling Cython file: ------------------------------------------------------------ ... const c_string& master_key_identifier, CSecureString& out) except *: mkid_str = frombytes(master_key_identifier) wk_str = frombytes(wrapped_key) key = handler.unwrap_key(wk_str, mkid_str) cstr = tobytes(key) out = CSecureString(move(cstr)) ^ ------------------------------------------------------------ pyarrow/_parquet_encryption.pyx:327:23: no suitable method found

You are right, cdefs can be removed by using proper casting / c-type annotation: 79cd06e

pitrou · 2025-08-19T14:09:01Z

Apart from the comments above, these changes look ok.

EnricoMi · 2025-08-22T07:58:29Z

All comments addressed. Diff is much cleaner now.

pitrou · 2025-08-26T12:39:41Z

Hmm, can you rebase/merge from main to fix most of the macOS CI builds?

pitrou

LGTM for the most part, just one request

EnricoMi · 2025-09-04T07:13:00Z

Comments addressed

…ng]*`

pitrou · 2025-09-04T12:30:18Z

I rebased on main to get up-to-date CI results.

pitrou · 2025-09-04T13:12:52Z

Thanks a lot again @EnricoMi :)

conbench-apache-arrow · 2025-09-04T20:04:13Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 403ba70.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

…encryption methods (apache#47339) ### Rationale for this change Passing encryption keys to Parquet via strings is insecure. Users are encouraged to use the `SecureString` based methods introduced in apache#46017 instead. To enforce this migration, deprecated methods are removed. ### What changes are included in this PR? Remove deprecated string-based methods. ### Are these changes tested? Existing Parquet encryption tests. ### Are there any user-facing changes? C++ user code that passes encryption keys to Parquet is affected and has to be adjusted. Python user code is not affected. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#47338 Authored-by: Enrico Minack <github@enrico.minack.dev> Signed-off-by: Antoine Pitrou <antoine@python.org>

EnricoMi requested review from AlenkaF, raulcd, rok and wgtmac as code owners August 15, 2025 05:55

github-actionsBot added Component: Parquet Component: C++ Component: Python awaiting review Awaiting review labels Aug 15, 2025

pitrou reviewed Aug 19, 2025
View reviewed changes

github-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 19, 2025

pitrou reviewed Aug 19, 2025
View reviewed changes

pitrou requested changes Aug 26, 2025
View reviewed changes

Comment threadpython/pyarrow/_parquet_encryption.pyx Outdated

EnricoMi force-pushed the secure-string-python branch from 79cd06e to fb5a9deCompare August 27, 2025 11:38

pitrou approved these changes Sep 4, 2025
View reviewed changes

EnricoMi added 6 commits September 4, 2025 14:09

Remove deprecations to move users to more secure methods
7992390

Make Python KmsClient work with SecureString-based CKmsClient interface
13b8996

Return CSecureString via CSecureString* not `shared_ptr[CSecureStri…
a586588
…ng]*`

Return CSecureString via CSecureString& not CSecureString*
40bead5

Use PyBytes_FromStringAndSize through cimport
a4a1176

Remove cdef from callback implementations
f01a81b

Return CSecureString* instead of CSecureString&
b125031

pitrou force-pushed the secure-string-python branch from fb5a9de to b125031Compare September 4, 2025 12:15

pitrou merged commit 403ba70 into apache:mainSep 4, 2025
34 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Sep 4, 2025

pitrou mentioned this pull request Sep 4, 2025
[C++] Remove deprecated string-based Parquet encryption methods #47338
Closed

	cdef extern from"Python.h":
	# To let us get a PyObject* and avoid Cython auto-ref-counting
	PyObject* PyBytes_FromStringAndSizeNative" PyBytes_FromStringAndSize"(
	char*v, Py_ssize_t len) exceptNULL

Uh oh!

Conversation

EnricoMi commented Aug 15, 2025• edited by github-actions BotLoading 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-actionsBot commented Aug 15, 2025

Uh oh!

EnricoMi commented Aug 15, 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!

EnricoMiAug 22, 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!

EnricoMiAug 22, 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!

pitrou commented Aug 19, 2025

Uh oh!

EnricoMi commented Aug 22, 2025

Uh oh!

pitrou commented Aug 26, 2025

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EnricoMi commented Sep 4, 2025

Uh oh!

pitrou commented Sep 4, 2025

Uh oh!

Uh oh!

pitrou commented Sep 4, 2025

Uh oh!

conbench-apache-arrowBot commented Sep 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EnricoMi commented Aug 15, 2025•
edited by github-actions Bot
Loading

EnricoMiAug 22, 2025•
edited
Loading

EnricoMiAug 22, 2025•
edited
Loading