Skip to content

Commit 403ba70

Browse files
EnricoMiEnricoMi
authored
GH-47338: [C++][Python] Remove deprecated string-based Parquet encryption methods (#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 #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: #47338 Authored-by: Enrico Minack <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
1 parent 512dcd7 commit 403ba70

17 files changed

+58
-119
lines changed

‎cpp/src/parquet/encryption/encryption.cc‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,10 @@ void StringKeyIdRetriever::PutKey(std::string key_id, SecureString key) {
4444
key_map_.insert({std::move(key_id), std::move(key)});
4545
}
4646

47-
SecureString StringKeyIdRetriever::GetKeyById(const std::string& key_id) {
47+
SecureString StringKeyIdRetriever::GetKey(const std::string& key_id) {
4848
return key_map_.at(key_id);
4949
}
5050

51-
ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key(
52-
std::string column_key) {
53-
return key(SecureString(std::move(column_key)));
54-
}
55-
5651
ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key(
5752
SecureString column_key) {
5853
if (column_key.empty()) return this;
@@ -94,11 +89,6 @@ FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::column_key
9489
return this;
9590
}
9691

97-
FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::footer_key(
98-
std::string footer_key) {
99-
return this->footer_key(SecureString(std::move(footer_key)));
100-
}
101-
10292
FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::footer_key(
10393
SecureString footer_key) {
10494
if (footer_key.empty()) {

‎cpp/src/parquet/encryption/encryption.h‎

Lines changed: 4 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -49,22 +49,7 @@ using ColumnPathToEncryptionPropertiesMap =
4949
class PARQUET_EXPORT DecryptionKeyRetriever {
5050
public:
5151
/// \brief Retrieve a key.
52-
/// \deprecated Deprecated since 22.0.0.
53-
/// Implement GetKeyById(const std::string&) instead.
54-
ARROW_DEPRECATED(
55-
"Deprecated in 22.0.0. "
56-
"Implement GetKeyById(const std::string&) instead.")
57-
virtual std::string GetKey(const std::string& key_id) {
58-
throw ParquetException("Not implemented");
59-
}
60-
61-
/// \brief Retrieve a key by its id.
62-
virtual ::arrow::util::SecureString GetKeyById(const std::string& key_id) {
63-
ARROW_SUPPRESS_DEPRECATION_WARNING
64-
auto key = ::arrow::util::SecureString(GetKey(key_id));
65-
ARROW_UNSUPPRESS_DEPRECATION_WARNING
66-
return key;
67-
}
52+
virtual ::arrow::util::SecureString GetKey(const std::string& key_id) = 0;
6853

6954
virtual ~DecryptionKeyRetriever() {}
7055
};
@@ -74,18 +59,16 @@ class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
7459
public:
7560
void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
7661

77-
::arrow::util::SecureString GetKeyById(const std::string& key_id_string) override {
62+
::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
7863
// key_id_string is string but for IntegerKeyIdRetriever it encodes
7964
// a native-endian 32 bit unsigned integer key_id
8065
uint32_t key_id;
8166
assert(key_id_string.size() == sizeof(key_id));
8267
memcpy(&key_id, key_id_string.data(), sizeof(key_id));
8368

84-
return GetKeyById(key_id);
69+
return key_map_.at(key_id);
8570
}
8671

87-
::arrow::util::SecureString GetKeyById(uint32_t key_id) { return key_map_.at(key_id); }
88-
8972
private:
9073
std::map<uint32_t, ::arrow::util::SecureString> key_map_;
9174
};
@@ -94,7 +77,7 @@ class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
9477
class PARQUET_EXPORT StringKeyIdRetriever : public DecryptionKeyRetriever {
9578
public:
9679
void PutKey(std::string key_id, ::arrow::util::SecureString key);
97-
::arrow::util::SecureString GetKeyById(const std::string& key_id) override;
80+
::arrow::util::SecureString GetKey(const std::string& key_id) override;
9881

9982
private:
10083
std::map<std::string, ::arrow::util::SecureString> key_map_;
@@ -143,11 +126,6 @@ class PARQUET_EXPORT ColumnEncryptionProperties {
143126
/// be encrypted with the footer key.
144127
/// keyBytes Key length must be either 16, 24 or 32 bytes.
145128
/// Caller is responsible for wiping out the input key array.
146-
/// \deprecated "Deprecated in 22.0.0. Use key(arrow::util::SecureString) instead."
147-
ARROW_DEPRECATED("Deprecated in 22.0.0. Use key(arrow::util::SecureString) instead.")
148-
Builder* key(std::string column_key);
149-
150-
/// \copydoc key(std::string)
151129
Builder* key(::arrow::util::SecureString column_key);
152130

153131
/// Set a key retrieval metadata.
@@ -259,14 +237,6 @@ class PARQUET_EXPORT FileDecryptionProperties {
259237
/// will be wiped out (array values set to 0).
260238
/// Caller is responsible for wiping out the input key array.
261239
/// param footerKey Key length must be either 16, 24 or 32 bytes.
262-
/// \deprecated Deprecated since 22.0.0.
263-
/// Use footer_key(arrow::util::SecureString) instead.
264-
ARROW_DEPRECATED(
265-
"Deprecated in 22.0.0. "
266-
"Use footer_key(arrow::util::SecureString) instead.")
267-
Builder* footer_key(std::string footer_key);
268-
269-
/// \copydoc footer_key(std::string footer_key)
270240
Builder* footer_key(::arrow::util::SecureString footer_key);
271241

272242
/// Set explicit column keys (decryption properties).
@@ -376,14 +346,6 @@ class PARQUET_EXPORT FileEncryptionProperties {
376346
public:
377347
class PARQUET_EXPORT Builder {
378348
public:
379-
/// \deprecated Deprecated since 22.0.0. Use Builder(arrow::util::SecureString)
380-
/// instead.
381-
ARROW_DEPRECATED(
382-
"Deprecated in 22.0.0. "
383-
"Use Builder(arrow::util::SecureString) instead")
384-
explicit Builder(std::string footer_key)
385-
: Builder(::arrow::util::SecureString(std::move(footer_key))) {}
386-
387349
explicit Builder(::arrow::util::SecureString footer_key)
388350
: parquet_cipher_(kDefaultEncryptionAlgorithm),
389351
encrypted_footer_(kDefaultEncryptedFooter),

‎cpp/src/parquet/encryption/file_key_unwrapper.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ FileKeyUnwrapper::FileKeyUnwrapper(
6969
kms_connection_config.key_access_token(), cache_entry_lifetime_seconds_);
7070
}
7171

72-
SecureString FileKeyUnwrapper::GetKeyById(const std::string& key_metadata_bytes) {
72+
SecureString FileKeyUnwrapper::GetKey(const std::string& key_metadata_bytes) {
7373
// key_metadata is expected to be in UTF8 encoding
7474
::arrow::util::InitializeUTF8();
7575
if (!::arrow::util::ValidateUTF8(
@@ -110,15 +110,15 @@ KeyWithMasterId FileKeyUnwrapper::GetDataEncryptionKey(const KeyMaterial& key_ma
110110

111111
SecureString data_key;
112112
if (!double_wrapping) {
113-
data_key = kms_client->UnWrapKey(encoded_wrapped_dek, master_key_id);
113+
data_key = kms_client->UnwrapKey(encoded_wrapped_dek, master_key_id);
114114
} else {
115115
// Get Key Encryption Key
116116
const std::string& encoded_kek_id = key_material.kek_id();
117117
const std::string& encoded_wrapped_kek = key_material.wrapped_kek();
118118

119119
const SecureString kek_bytes = kek_per_kek_id_->GetOrInsert(
120120
encoded_kek_id, [kms_client, encoded_wrapped_kek, master_key_id]() {
121-
return kms_client->UnWrapKey(encoded_wrapped_kek, master_key_id);
121+
return kms_client->UnwrapKey(encoded_wrapped_kek, master_key_id);
122122
});
123123

124124
// Decrypt the data key

‎cpp/src/parquet/encryption/file_key_unwrapper.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ class PARQUET_EXPORT FileKeyUnwrapper : public DecryptionKeyRetriever {
6565
std::shared_ptr<FileKeyMaterialStore> key_material_store);
6666

6767
/// Get the data key from key metadata
68-
::arrow::util::SecureString GetKeyById(const std::string& key_metadata_bytes) override;
68+
::arrow::util::SecureString GetKey(const std::string& key_metadata_bytes) override;
6969

7070
/// Get the data key along with the master key id from key material
7171
KeyWithMasterId GetDataEncryptionKey(const KeyMaterial& key_material);

‎cpp/src/parquet/encryption/internal_file_decryptor.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ const SecureString& InternalFileDecryptor::GetFooterKey() {
7878
if (properties_->key_retriever() == nullptr)
7979
throw ParquetException("No footer key or key retriever");
8080
try {
81-
footer_key_ = properties_->key_retriever()->GetKeyById(footer_key_metadata_);
81+
footer_key_ = properties_->key_retriever()->GetKey(footer_key_metadata_);
8282
} catch (KeyAccessDeniedException& e) {
8383
std::stringstream ss;
8484
ss << "Footer key: access denied " << e.what() << "\n";
@@ -117,7 +117,7 @@ SecureString InternalFileDecryptor::GetColumnKey(const std::string& column_path,
117117
if (column_key.empty() && !column_key_metadata.empty() &&
118118
properties_->key_retriever() != nullptr) {
119119
try {
120-
column_key = properties_->key_retriever()->GetKeyById(column_key_metadata);
120+
column_key = properties_->key_retriever()->GetKey(column_key_metadata);
121121
} catch (KeyAccessDeniedException& e) {
122122
std::stringstream ss;
123123
ss << "HiddenColumnException, path=" + column_path + " " << e.what() << "\n";

‎cpp/src/parquet/encryption/key_wrapping_test.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,10 @@ class KeyWrappingTest : public ::testing::Test {
8686
FileKeyUnwrapper unwrapper(&key_toolkit, kms_connection_config_,
8787
cache_entry_lifetime_seconds, readable_file_path,
8888
file_system);
89-
SecureString footer_key = unwrapper.GetKeyById(key_metadata_json_footer);
89+
SecureString footer_key = unwrapper.GetKey(key_metadata_json_footer);
9090
ASSERT_EQ(footer_key, kFooterEncryptionKey);
9191

92-
SecureString column_key = unwrapper.GetKeyById(key_metadata_json_column);
92+
SecureString column_key = unwrapper.GetKey(key_metadata_json_column);
9393
ASSERT_EQ(column_key, kColumnEncryptionKey1);
9494
}
9595

‎cpp/src/parquet/encryption/kms_client.h‎

Lines changed: 3 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -84,42 +84,12 @@ class PARQUET_EXPORT KmsClient {
8484
///
8585
/// Encrypts it with the master key, encodes the result
8686
/// and potentially adds a KMS-specific metadata.
87-
///
88-
/// \deprecated Deprecated since 22.0.0. Implement
89-
/// WrapKey(const SecureString&, const std::string&) instead.
90-
ARROW_DEPRECATED(
91-
"Deprecated in 22.0.0. "
92-
"Implement WrapKey(const SecureString&, const std::string&) instead.")
93-
virtual std::string WrapKey(const std::string& key_bytes,
94-
const std::string& master_key_identifier) {
95-
throw ParquetException("Not implemented");
96-
}
97-
98-
/// \copydoc WrapKey(const std::string&, const std::string&)
9987
virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
100-
const std::string& master_key_identifier) {
101-
ARROW_SUPPRESS_DEPRECATION_WARNING
102-
auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
103-
ARROW_UNSUPPRESS_DEPRECATION_WARNING
104-
return key;
105-
}
88+
const std::string& master_key_identifier) = 0;
10689

10790
/// \brief Decrypts (unwraps) a key with the master key.
108-
/// \deprecated Deprecated since 22.0.0. Implement UnWrapKey instead.
109-
ARROW_DEPRECATED("Deprecated in 22.0.0. Implement UnWrapKey instead.")
110-
virtual std::string UnwrapKey(const std::string& wrapped_key,
111-
const std::string& master_key_identifier) {
112-
throw ParquetException("Not implemented");
113-
}
114-
115-
/// \copydoc UnwrapKey(const std::string&, const std::string&)
116-
virtual ::arrow::util::SecureString UnWrapKey(
117-
const std::string& wrapped_key, const std::string& master_key_identifier) {
118-
ARROW_SUPPRESS_DEPRECATION_WARNING
119-
auto key = ::arrow::util::SecureString(UnwrapKey(wrapped_key, master_key_identifier));
120-
ARROW_UNSUPPRESS_DEPRECATION_WARNING
121-
return key;
122-
}
91+
virtual ::arrow::util::SecureString UnwrapKey(
92+
const std::string& wrapped_key, const std::string& master_key_identifier) = 0;
12393

12494
virtual ~KmsClient() {}
12595
};

‎cpp/src/parquet/encryption/local_wrap_kms_client.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ std::string LocalWrapKmsClient::WrapKey(const SecureString& key_bytes,
8484
return LocalKeyWrap::CreateSerialized(encrypted_encoded_key);
8585
}
8686

87-
SecureString LocalWrapKmsClient::UnWrapKey(const std::string& wrapped_key,
87+
SecureString LocalWrapKmsClient::UnwrapKey(const std::string& wrapped_key,
8888
const std::string& master_key_identifier) {
8989
LocalKeyWrap key_wrap = LocalKeyWrap::Parse(wrapped_key);
9090
const std::string& master_key_version = key_wrap.master_key_version();

‎cpp/src/parquet/encryption/local_wrap_kms_client.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ class PARQUET_EXPORT LocalWrapKmsClient : public KmsClient {
3838
std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
3939
const std::string& master_key_identifier) override;
4040

41-
::arrow::util::SecureString UnWrapKey(
41+
::arrow::util::SecureString UnwrapKey(
4242
const std::string& wrapped_key, const std::string& master_key_identifier) override;
4343

4444
protected:

‎cpp/src/parquet/encryption/properties_test.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,9 +224,9 @@ TEST(TestDecryptionProperties, UseKeyRetriever) {
224224
std::shared_ptr<parquet::FileDecryptionProperties> props = builder.build();
225225

226226
auto out_key_retriever = props->key_retriever();
227-
ASSERT_EQ(kFooterEncryptionKey, out_key_retriever->GetKeyById("kf"));
228-
ASSERT_EQ(kColumnEncryptionKey1, out_key_retriever->GetKeyById("kc1"));
229-
ASSERT_EQ(kColumnEncryptionKey2, out_key_retriever->GetKeyById("kc2"));
227+
ASSERT_EQ(kFooterEncryptionKey, out_key_retriever->GetKey("kf"));
228+
ASSERT_EQ(kColumnEncryptionKey1, out_key_retriever->GetKey("kc1"));
229+
ASSERT_EQ(kColumnEncryptionKey2, out_key_retriever->GetKey("kc2"));
230230
}
231231

232232
TEST(TestDecryptionProperties, SupplyAadPrefix) {

0 commit comments

Comments
 (0)