ARROW-9960: [C++] Enable external material and rotation for encryption keys #10491

revit13 · 2021-06-09T10:43:50Z

This PR adds the support for external key material and key rotation for Parquet Modular encryption.
More information on external key material and key rotation for PME can be found in https://docs.google.com/document/d/1bEu903840yb95k9q2X-BlsYKuXoygE4VnMDl9xz_zhk/edit?usp=sharing.

Thanks to @roee88 for suggesting the usage of arrow::fs::FileSystem in the implementation.

github-actions · 2021-06-09T11:27:19Z

https://issues.apache.org/jira/browse/ARROW-9960

pitrou · 2022-05-04T14:14:26Z

@revit13 @andersonm-ibm @ggershinsky What is the status of this PR? Should it be rebased?

revit13 · 2022-05-04T20:40:02Z

@pitrou yes, will rebase it. Thanks

…n keys Signed-off-by: Revital Sur <eres@il.ibm.com>

Signed-off-by: Revital Sur <eres@il.ibm.com> Co-authored-by: Maya Anderson <mayaa@il.ibm.com>

Signed-off-by: Revital Sur <eres@il.ibm.com>

ggershinsky · 2022-05-06T06:25:15Z

thanks @revit13 ! @andersonm-ibm , could you verify that the files are readable by Spark 3.2 (and vice versa)?
Also, do we need PyArrow additions to write in this mode?

pitrou · 2022-05-09T12:31:03Z

@ggershinsky @revit13 Could any validation files be added to the parquet-testing, if not already done?

andersonm-ibm · 2022-05-09T13:21:58Z

thanks @revit13 ! @andersonm-ibm , could you verify that the files are readable by Spark 3.2 (and vice versa)? Also, do we need PyArrow additions to write in this mode?

Thank you, @ggershinsky , I'll check the interop with Spark 3.2.
In addition, I've created a Jira to track the PyArrow additions needed to use this mode: ARROW-16505 .

ggershinsky · 2022-05-09T13:57:25Z

@ggershinsky @revit13 Could any validation files be added to the parquet-testing, if not already done?

cc @andersonm-ibm

wjones127 · 2023-01-13T17:31:06Z

It's been a while. @revit13 are you still interested in moving this forward? @andersonm-ibm are you still planning on helping test these changes?

If so, I can help review.

wjones127

It looks like this is nearly complete, but could use some updates since its last been touched.

Rebase the branch on latest master
Upgrade to use C++14 features (e.g. std::make_unique)
Drop unnecessary filesystem API wrappers
Improve some of the internal documentation.
Test encrypted files written by Spark 3.2

wjones127 · 2023-01-24T17:43:07Z

cpp/src/parquet/encryption/file_path.h

+  FilePath(std::string path, std::shared_ptr<::arrow::fs::FileSystem> filesystem)
+      : file_info_(std::move(path)), filesystem_(filesystem) {}
+
+  /// \brief The directory base name (component before the file base name


Suggested change

/// \brief The directory base name (component before the file base name

/// \brief The directory base name (component before the file base name)

wjones127 · 2023-01-24T17:44:45Z

cpp/src/parquet/encryption/file_path.h

+namespace parquet {
+
+/// \brief The path and filesystem where an actual file is located.
+class PARQUET_EXPORT FilePath {


Do we really need this class?

wjones127 · 2023-01-24T17:54:37Z

cpp/src/parquet/encryption/file_system_key_material_store.cc

+  std::string key_material_file_name =
+      full_prefix + parquet_file_path->base_name() +
+      std::string(FileSystemKeyMaterialStore::kKeyMaterialFileSuffix);
+  key_material_file_ = ::arrow::internal::make_unique<FilePath>(


We should probably switch this to std::make_unique

wjones127 · 2023-01-24T17:56:58Z

cpp/src/parquet/encryption/file_system_key_material_store.cc

+  ::arrow::Result<std::shared_ptr<Buffer>> buff =
+      input->ReadAt(0, input->GetSize().ValueOrDie());
+  std::string buff_str = buff.ValueOrDie()->ToString();
+  ::arrow::util::string_view sv(buff_str);


Also should upgrade string_view stuff.

wjones127 · 2023-01-24T18:06:04Z

cpp/src/parquet/encryption/file_system_key_material_store.cc

+    key.SetString(it->first.c_str(), allocator);
+    value.SetString(it->second.c_str(), allocator);


I think if you set #define RAPIDJSON_HAS_STDSTRING 1 you can just pass in the strings as is.

wjones127 · 2023-01-24T21:56:20Z

cpp/src/parquet/encryption/key_toolkit.cc

+  std::vector<::arrow::fs::FileInfo> parquet_files_in_folder;
+  ::arrow::fs::FileSelector s;
+  s.base_dir = folder_path->dir_name();
+  parquet_files_in_folder = folder_path->filesystem()->GetFileInfo(s).ValueOrDie();


Should this be recursive? Or be allowing the user to supply the vector of fileinfos?

wjones127 · 2023-01-24T21:57:08Z

cpp/src/parquet/encryption/key_toolkit.cc

+    if (parquet_file.type() != ::arrow::fs::FileType::File)
+      throw ParquetException("Expecting file type in " + folder_path->dir_name());


Why not ignore directories?

wjones127 · 2023-01-24T22:10:16Z

cpp/src/parquet/encryption/file_key_unwrapper.cc

+
+  if (key_material_store != nullptr) {
+    key_material_store_ = key_material_store;
+    checked_key_material_internal_storage_ = true;


What if instead of having this bool we just checked key_material_store != nullptr?

wjones127 · 2023-01-24T22:13:23Z

cpp/src/parquet/encryption/file_key_unwrapper.cc

+    std::string key_id_in_file = key_metadata.key_reference();
+    std::string key_material_string = key_material_store_->GetKeyMaterial(key_id_in_file);
+    if (key_material_string.empty()) {
+      throw ParquetException("Null key material for keyIDinFile: " + key_id_in_file);


Suggested change

throw ParquetException("Null key material for keyIDinFile: " + key_id_in_file);

throw ParquetException("Null key material for key with ID: " + key_id_in_file);

wjones127 · 2023-01-24T22:21:07Z

cpp/src/parquet/encryption/file_key_wrapper.cc

+      key_id_in_file =
+          KeyMaterial::kColumnKeyIdInFilePrefix + std::to_string(key_counter_);
+      key_counter_++;


Why are we appending this integer? Who else needs to know about this ID? It seems like this detail deserves more comments.

adamreeve · 2023-02-01T21:38:26Z

cpp/src/parquet/encryption/crypto_factory.cc

-    throw ParquetException("External key material store is not supported yet.");
+    if (parquet_file_path == nullptr) {
+      std::stringstream ss;
+      ss << "Output file path cannot be null"


It would be good to clarify this a bit:

Suggested change

ss << "Output file path cannot be null"

ss << "Output file path cannot be null when using external key material"

adamreeve · 2023-02-01T21:40:27Z

cpp/src/parquet/encryption/file_system_key_material_store.cc

+namespace parquet {
+namespace encryption {
+
+constexpr const char FileSystemKeyMaterialStore::kKetMaterialFilePrefix[];


Typo, should be kKeyMaterialFilePrefix

adamreeve · 2023-02-01T21:43:32Z

cpp/src/parquet/encryption/key_toolkit.cc

+    if (parquet_file.type() != ::arrow::fs::FileType::File)
+      throw ParquetException("Expecting file type in " + folder_path->dir_name());
+    std::string child = parquet_file.base_name();
+    if (filter_out_file(child)) continue;


Would it make more sense to only look for files that begin with the _KEY_MATERIAL_FOR_ prefix, rather than assuming that any file that doesn't begin with "." or "_" is a Parquet file with an associated external key material file? Otherwise this will fail for any directories that contain any other type of file.

adamreeve · 2023-02-14T21:53:01Z

It doesn't look like this pull request is going to be revived, so I've opened a new PR that addresses the review comments at #34181

…keys (#34181) This PR is a replacement for #10491 which appears to be abandoned. I've fixed the issues pointed out in review comments as well as a few more things I noticed. I've also made one significant change to the API; rather than `CryptoFactory::RotateMasterKeys` operating on a whole directory, it rotates keys for a single file, as this is much more flexible and allows users to decide what files to rotate keys for, and there didn't appear to be a good reason for this to work at the whole directory level. This does mean the API diverges slightly from the parquet-mr API though. ### Rationale for this change Use of external key material allows rotating master encryption keys without having to rewrite Parquet file data. See https://docs.google.com/document/d/1bEu903840yb95k9q2X-BlsYKuXoygE4VnMDl9xz_zhk/edit?usp=sharing for more details. ### What changes are included in this PR? Adds support for writing and reading external key material for Parquet files from C++, as well as a new `CryptoFactory::RotateMasterKeys` function that allows re-encrypting key encryption keys or data encryption keys with latest versions of master keys. ### Are these changes tested? Yes, unit tests are included. I've added an additional test that reads a file generated with parquet-mr (used by Spark) from the parquet-testing repository. This requires merging the PR at apache/parquet-testing#36 and updating the parquet-testing submodule before the new test will pass. ### Are there any user-facing changes? Yes, the existing `internal_key_material` option in `parquet::encryption::EncryptionConfiguration` will now work and use external key material. This requires using two new parameters (`file_path` and `file_system`) in `CryptoFactory::GetFileEncryptionProperties` and `CryptoFactory::GetFileDecryptionProperties`, which are needed so that we know where to write/read the external key material. Note that this means external key material won't work from Python until the new parameters are exposed in Python too. This changes the `CryptoFactory` ABI but the API is still source compatible. Is this okay or should new overloads be added that take the new parameters? The `CryptoFactory::RotateMasterKeys` function is also a new public facing API. * Closes: #25986 Lead-authored-by: Adam Reeve <adreeve@gmail.com> Co-authored-by: Revital Sur <eres@il.ibm.com> Co-authored-by: Maya Anderson <mayaa@il.ibm.com> Signed-off-by: Will Jones <willjones127@gmail.com>

andersonm-ibm · 2023-02-27T21:21:25Z

It doesn't look like this pull request is going to be revived, so I've opened a new PR that addresses the review comments at #34181

Thank you, @adamreeve and @wjones127 , for taking this up!

github-actions bot added Component: C++ Component: Parquet labels Jun 9, 2021

revit13 force-pushed the pme_key_rotation1 branch 2 times, most recently from cee4bec to 0b7209b Compare June 15, 2021 05:17

revit13 force-pushed the pme_key_rotation1 branch 2 times, most recently from 35edaea to 9964bb5 Compare June 22, 2021 11:01

revit13 force-pushed the pme_key_rotation1 branch from 5fdb6bb to eff6d6b Compare August 4, 2021 05:44

revit13 and others added 2 commits May 5, 2022 18:23

ARROW-9960: [C++] Enable external material and rotation for encryptio…

f7b1577

…n keys Signed-off-by: Revital Sur <eres@il.ibm.com>

Use ::arrow::fs::FileInfo in FilePath.

5be9002

Signed-off-by: Revital Sur <eres@il.ibm.com> Co-authored-by: Maya Anderson <mayaa@il.ibm.com>

revit13 force-pushed the pme_key_rotation1 branch from eff6d6b to 5be9002 Compare May 5, 2022 15:25

Fix after rebase.

b2d86d3

Signed-off-by: Revital Sur <eres@il.ibm.com>

asfimport mentioned this pull request Aug 26, 2022

[C++][Parquet] Enable external material and rotation for encryption keys #25986

Closed

wjones127 self-requested a review January 24, 2023 17:40

wjones127 requested changes Jan 24, 2023

View reviewed changes

adamreeve reviewed Feb 1, 2023

View reviewed changes

adamreeve mentioned this pull request Feb 14, 2023

GH-25986: [C++] Enable external material and rotation for encryption keys #34181

Merged

wjones127 closed this Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-9960: [C++] Enable external material and rotation for encryption keys #10491

ARROW-9960: [C++] Enable external material and rotation for encryption keys #10491

revit13 commented Jun 9, 2021 •

edited

github-actions bot commented Jun 9, 2021

pitrou commented May 4, 2022

revit13 commented May 4, 2022

ggershinsky commented May 6, 2022 •

edited

pitrou commented May 9, 2022

andersonm-ibm commented May 9, 2022

ggershinsky commented May 9, 2022

wjones127 commented Jan 13, 2023

wjones127 left a comment

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

wjones127 Jan 24, 2023

adamreeve Feb 1, 2023

adamreeve Feb 1, 2023

adamreeve Feb 1, 2023

adamreeve commented Feb 14, 2023

andersonm-ibm commented Feb 27, 2023

	/// \brief The directory base name (component before the file base name
	/// \brief The directory base name (component before the file base name)

		key.SetString(it->first.c_str(), allocator);
		value.SetString(it->second.c_str(), allocator);

		if (parquet_file.type() != ::arrow::fs::FileType::File)
		throw ParquetException("Expecting file type in " + folder_path->dir_name());

	throw ParquetException("Null key material for keyIDinFile: " + key_id_in_file);
	throw ParquetException("Null key material for key with ID: " + key_id_in_file);

	ss << "Output file path cannot be null"
	ss << "Output file path cannot be null when using external key material"

ARROW-9960: [C++] Enable external material and rotation for encryption keys #10491

ARROW-9960: [C++] Enable external material and rotation for encryption keys #10491

Conversation

revit13 commented Jun 9, 2021 • edited

github-actions bot commented Jun 9, 2021

pitrou commented May 4, 2022

revit13 commented May 4, 2022

ggershinsky commented May 6, 2022 • edited

pitrou commented May 9, 2022

andersonm-ibm commented May 9, 2022

ggershinsky commented May 9, 2022

wjones127 commented Jan 13, 2023

wjones127 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreeve commented Feb 14, 2023

andersonm-ibm commented Feb 27, 2023

revit13 commented Jun 9, 2021 •

edited

ggershinsky commented May 6, 2022 •

edited