Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-31603: [C++] Wrap Parquet encryption keys in SecureString#46017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
5f73a1f
Wrap encryption keys in SecureString
EnricoMi 0456be4
Rename non-trivial SecureString methods
EnricoMi 689bc8d
Add mutable as_span method, add as_view
EnricoMi 42d7319
More tests
EnricoMi 01ab190
Change SecureClear(std::string&) to SecureClear(std::string*)
EnricoMi f3f4087
Decrypt key directly intoSecureString
EnricoMi f90f196
Remove const from KeyWithMasterId members
EnricoMi 0e74be5
Replace call-by-ref with call-by-value when copied
EnricoMi 29bc5e9
Use const for constants and construct SecureString from consts in tests
EnricoMi 75c1f0a
Inline IntegerKeyIdRetriever::GetKey(std::string) implementation
EnricoMi d3670d4
Add comment to `no_key`
EnricoMi adbbfc8
Revert `InternalFileDecryptor::RetrieveColumnKeyIfEmpty`
EnricoMi fe79da0
Remove `noexcept`
EnricoMi 6f5ef6c
Merge remote-tracking branch 'origin/main' into secure-string
EnricoMi db70c19
More secure cleared assertions on construction and assignment
EnricoMi d6f9ea9
Improve SecureString assignment tests
EnricoMi 510349c
More context on SecureClear code
EnricoMi 92a7980
Add SecureString implementation to arrow/util/
EnricoMi ec3c7c6
Merge branch 'main' into secure-string
EnricoMi d69f354
Merge branch 'secure-string-util' into secure-string
EnricoMi 219d207
Move to arrow::util::SecureString
EnricoMi 20d67b3
Fix import for memset_s, improve for loops in tests
EnricoMi 15f94c6
Address code review comments
EnricoMi 2a2ae81
Merge branch 'secure-string-util' into secure-string
EnricoMi 973b233
Test secure SecureString deconstruction
EnricoMi 9c88744
Test correctness of AssertSecurelyCleared
EnricoMi f3562f8
Rename SecureString argument to other
EnricoMi 9ee3e2c
Move std::move into secure_move, assert string ptr
EnricoMi 77e4e20
Add comments, fix linting
EnricoMi 1f42383
Improve assertions
EnricoMi 8d9c4f9
Use testing::AssertionResult rather than capturing assertions through…
EnricoMi 4297f0d
Expect string buffers larger than requested size
EnricoMi 064dfe7
Handle string buffers larger than init size
EnricoMi d4faa4f
Don't access deallocated memory in ASAN / Valgrind mode
EnricoMi 267626c
Fix SecureClear for non-local strings, stabalize mem assertions
EnricoMi 6995d36
Avoid assigning short string to long string in test
EnricoMi 03b1fef
Fix memory issues in tests
EnricoMi e7470cd
Improve comments
EnricoMi 8639235
Apply code review comments
EnricoMi 7f827ed
Merge remote-tracking branch 'origin/main' into secure-string-util
EnricoMi 0ec848c
Move SecureClear(std::string*) up in source file as well
EnricoMi 4336801
Merge branch 'secure-string-util' into secure-string
EnricoMi e11d223
Add back std::string methods as deprecated
EnricoMi ee284be
Merge remote-tracking branch 'origin/main' into secure-string
EnricoMi efd7cb9
Merge branch 'main' into secure-string
EnricoMi fb41244
Fix merge
EnricoMi fa9abed
Use const in favour of inline strings
EnricoMi 290449a
Remove deprecations to move users to more secure methods
EnricoMi 184afcf
Move no_key_ into encryption.cc
EnricoMi edd97b8
Rename no_key_
EnricoMi 0a31f50
Revert "Remove deprecations to move users to more secure methods"
EnricoMi 1987cda
Merge branch 'main' into secure-string
pitrou 04b15e2
Update deprecation notices
pitrou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 4 additions & 3 deletions
7 cpp/examples/parquet/low_level_api/encryption_reader_writer.cc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 4 additions & 3 deletions
7 cpp/examples/parquet/low_level_api/encryption_reader_writer_all_crypto_options.cc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -26,31 +26,35 @@ | ||
| #include "arrow/util/utf8.h" | ||
| #include "parquet/encryption/encryption_internal.h" | ||
| namespace parquet { | ||
| using ::arrow::util::SecureString; | ||
| // integer key retriever | ||
| void IntegerKeyIdRetriever::PutKey(uint32_t key_id, const std::string& key) { | ||
| key_map_.insert({key_id, key}); | ||
| } | ||
| namespace parquet { | ||
| std::string IntegerKeyIdRetriever::GetKey(const std::string& key_metadata) { | ||
| uint32_t key_id; | ||
| memcpy(reinterpret_cast<uint8_t*>(&key_id), key_metadata.c_str(), 4); | ||
| // any empty SecureString key is interpreted as if no key is given | ||
| // this instance is used when a SecureString reference is returned | ||
| static SecureString kNoKey = SecureString(); | ||
| return key_map_.at(key_id); | ||
| // integer key retriever | ||
| void IntegerKeyIdRetriever::PutKey(uint32_t key_id, SecureString key) { | ||
| key_map_.insert({key_id, std::move(key)}); | ||
| } | ||
| // string key retriever | ||
| void StringKeyIdRetriever::PutKey(const std::string& key_id, const std::string& key) { | ||
| key_map_.insert({key_id, key}); | ||
| void StringKeyIdRetriever::PutKey(std::string key_id, SecureString key) { | ||
| key_map_.insert({std::move(key_id), std::move(key)}); | ||
| } | ||
| std::string StringKeyIdRetriever::GetKey(const std::string& key_id) { | ||
| SecureString StringKeyIdRetriever::GetKeyById(const std::string& key_id) { | ||
| return key_map_.at(key_id); | ||
| } | ||
| ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key( | ||
| std::string column_key) { | ||
| return key(SecureString(std::move(column_key))); | ||
| } | ||
| ColumnEncryptionProperties::Builder* ColumnEncryptionProperties::Builder::key( | ||
| SecureString column_key) { | ||
| if (column_key.empty()) return this; | ||
| DCHECK(key_.empty()); | ||
| @@ -92,6 +96,11 @@ FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::column_key | ||
| FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::footer_key( | ||
| std::string footer_key) { | ||
| return this->footer_key(SecureString(std::move(footer_key))); | ||
| } | ||
| FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::footer_key( | ||
| SecureString footer_key) { | ||
| if (footer_key.empty()) { | ||
| return this; | ||
| } | ||
| @@ -129,10 +138,10 @@ FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::aad_prefix | ||
| } | ||
| ColumnDecryptionProperties::Builder* ColumnDecryptionProperties::Builder::key( | ||
| std::string key) { | ||
| SecureString key) { | ||
| if (key.empty()) return this; | ||
| DCHECK(!key.empty()); | ||
| DCHECK(key_.empty()); | ||
| key_ = std::move(key); | ||
| return this; | ||
| } | ||
| @@ -182,74 +191,69 @@ FileEncryptionProperties::Builder::disable_aad_prefix_storage() { | ||
| ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted, | ||
| std::string column_path, | ||
| std::string key, | ||
| std::string key_metadata) { | ||
| DCHECK(!column_path.empty()); | ||
| column_path_ = std::move(column_path); | ||
| SecureString key, | ||
| std::string key_metadata) | ||
| : column_path_(std::move(column_path)), | ||
EnricoMi marked this conversation as resolved.
Outdated
Uh oh!There was an error while loading. Please reload this page. | ||
| encrypted_(encrypted), | ||
| encrypted_with_footer_key_(encrypted && key.empty()), | ||
| key_(std::move(key)), | ||
| key_metadata_(std::move(key_metadata)) { | ||
| DCHECK(!column_path_.empty()); | ||
| if (!encrypted) { | ||
| DCHECK(key.empty() && key_metadata.empty()); | ||
| DCHECK(key_.empty() && key_metadata_.empty()); | ||
| } | ||
| if (!key.empty()) { | ||
| DCHECK(key.length() == 16 || key.length() == 24 || key.length() == 32); | ||
| if (!key_.empty()) { | ||
| DCHECK(key_.length() == 16 || key_.length() == 24 || key_.length() == 32); | ||
| } | ||
| encrypted_with_footer_key_ = (encrypted && key.empty()); | ||
| if (encrypted_with_footer_key_) { | ||
| DCHECK(key_metadata.empty()); | ||
| DCHECK(key_metadata_.empty()); | ||
| } | ||
| encrypted_ = encrypted; | ||
| key_metadata_ = std::move(key_metadata); | ||
| key_ = std::move(key); | ||
| } | ||
| ColumnDecryptionProperties::ColumnDecryptionProperties(std::string column_path, | ||
| std::string key) { | ||
| DCHECK(!column_path.empty()); | ||
| column_path_ = std::move(column_path); | ||
| SecureString key) | ||
| : column_path_(std::move(column_path)), key_(std::move(key)) { | ||
| DCHECK(!column_path_.empty()); | ||
| if (!key.empty()) { | ||
| DCHECK(key.length() == 16 || key.length() == 24 || key.length() == 32); | ||
| if (!key_.empty()) { | ||
| DCHECK(key_.length() == 16 || key_.length() == 24 || key_.length() == 32); | ||
| } | ||
| key_ = std::move(key); | ||
| } | ||
| std::string FileDecryptionProperties::column_key(const std::string& column_path) const { | ||
| const SecureString& FileDecryptionProperties::column_key( | ||
| const std::string& column_path) const { | ||
| if (column_decryption_properties_.find(column_path) != | ||
| column_decryption_properties_.end()) { | ||
| auto column_prop = column_decryption_properties_.at(column_path); | ||
| if (column_prop != nullptr) { | ||
| return column_prop->key(); | ||
| } | ||
| } | ||
| return {}; | ||
| return kNoKey; | ||
| } | ||
| FileDecryptionProperties::FileDecryptionProperties( | ||
| std::string footer_key, std::shared_ptr<DecryptionKeyRetriever> key_retriever, | ||
| SecureString footer_key, std::shared_ptr<DecryptionKeyRetriever> key_retriever, | ||
| bool check_plaintext_footer_integrity, std::string aad_prefix, | ||
| std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier, | ||
| ColumnPathToDecryptionPropertiesMap column_decryption_properties, | ||
| bool plaintext_files_allowed) { | ||
| DCHECK(!footer_key.empty() || nullptr != key_retriever || | ||
| 0 != column_decryption_properties.size()); | ||
| if (!footer_key.empty()) { | ||
| DCHECK(footer_key.length() == 16 || footer_key.length() == 24 || | ||
| footer_key.length() == 32); | ||
| bool plaintext_files_allowed) | ||
| : footer_key_(std::move(footer_key)), | ||
| aad_prefix_(std::move(aad_prefix)), | ||
| aad_prefix_verifier_(std::move(aad_prefix_verifier)), | ||
| column_decryption_properties_(std::move(column_decryption_properties)), | ||
| key_retriever_(std::move(key_retriever)), | ||
| check_plaintext_footer_integrity_(check_plaintext_footer_integrity), | ||
| plaintext_files_allowed_(plaintext_files_allowed) { | ||
| DCHECK(!footer_key_.empty() || nullptr != key_retriever_ || | ||
| 0 != column_decryption_properties_.size()); | ||
| if (!footer_key_.empty()) { | ||
| DCHECK(footer_key_.length() == 16 || footer_key_.length() == 24 || | ||
| footer_key_.length() == 32); | ||
| } | ||
| if (footer_key.empty() && check_plaintext_footer_integrity) { | ||
| DCHECK(nullptr != key_retriever); | ||
| if (footer_key_.empty() && check_plaintext_footer_integrity) { | ||
| DCHECK(nullptr != key_retriever_); | ||
| } | ||
| aad_prefix_verifier_ = std::move(aad_prefix_verifier); | ||
| footer_key_ = std::move(footer_key); | ||
| check_plaintext_footer_integrity_ = check_plaintext_footer_integrity; | ||
| key_retriever_ = std::move(key_retriever); | ||
| aad_prefix_ = std::move(aad_prefix); | ||
| column_decryption_properties_ = std::move(column_decryption_properties); | ||
| plaintext_files_allowed_ = plaintext_files_allowed; | ||
| } | ||
| FileEncryptionProperties::Builder* FileEncryptionProperties::Builder::footer_key_id( | ||
| @@ -282,7 +286,7 @@ FileEncryptionProperties::column_encryption_properties(const std::string& column | ||
| } | ||
| FileEncryptionProperties::FileEncryptionProperties( | ||
| ParquetCipher::type cipher, std::string footer_key, std::string footer_key_metadata, | ||
| ParquetCipher::type cipher, SecureString footer_key, std::string footer_key_metadata, | ||
| bool encrypted_footer, std::string aad_prefix, bool store_aad_prefix_in_file, | ||
| ColumnPathToEncryptionPropertiesMap encrypted_columns) | ||
| : footer_key_(std::move(footer_key)), | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you've corrected a bug; the intention with these setter methods seems to be one-time setting