ARROW-9960: [C++] Enable external material and rotation for encryption keys by revit13 · Pull Request #10491 · apache/arrow

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 <[email protected]>

Signed-off-by: Revital Sur <[email protected]> Co-authored-by: Maya Anderson <[email protected]>

Signed-off-by: Revital Sur <[email protected]>

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

+  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

+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

+  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

+  ::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

+    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

+  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

+    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

+
+  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

+    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

+      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

-    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

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


Typo, should be kKeyMaterialFilePrefix

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

+    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 <[email protected]> Co-authored-by: Revital Sur <[email protected]> Co-authored-by: Maya Anderson <[email protected]> Signed-off-by: Will Jones <[email protected]>

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 <[email protected]>

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

5be9002

Signed-off-by: Revital Sur <[email protected]> Co-authored-by: Maya Anderson <[email protected]>

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 <[email protected]>

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

	/// \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"

Conversation

revit13 commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 9, 2021

Uh oh!

pitrou commented May 4, 2022

Uh oh!

revit13 commented May 4, 2022

Uh oh!

ggershinsky commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented May 9, 2022

Uh oh!

andersonm-ibm commented May 9, 2022

Uh oh!

ggershinsky commented May 9, 2022

Uh oh!

wjones127 commented Jan 13, 2023

Uh oh!

wjones127 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!

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!

adamreeve commented Feb 14, 2023

Uh oh!

andersonm-ibm commented Feb 27, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

revit13 commented Jun 9, 2021 •

edited

Loading

ggershinsky commented May 6, 2022 •

edited

Loading