-
Notifications
You must be signed in to change notification settings - Fork 0
Wiring up encryption metadata to the DBPA remote and local agents. #167
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
Changes from 5 commits
9aded37
58fc43d
53926fc
02023d2
3d793f0
dc9a956
f9f1740
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,7 @@ namespace dbps::external { | |
| class DBPS_EXPORT LocalEncryptionResult : public EncryptionResult { | ||
| public: | ||
| // Constructor for successful encryption | ||
| LocalEncryptionResult(std::vector<uint8_t> ciphertext); | ||
| LocalEncryptionResult(std::vector<uint8_t> ciphertext, const std::map<std::string, std::string>& encryption_metadata = {}); | ||
|
|
||
| // Constructor for failed encryption | ||
| LocalEncryptionResult(const std::string& error_stage, const std::string& error_message); | ||
|
|
@@ -50,6 +50,7 @@ class DBPS_EXPORT LocalEncryptionResult : public EncryptionResult { | |
| span<const uint8_t> ciphertext() const override; | ||
| std::size_t size() const override; | ||
| bool success() const override; | ||
| const std::optional<std::map<std::string, std::string>> encryption_metadata() const override; | ||
| const std::string& error_message() const override; | ||
| const std::map<std::string, std::string>& error_fields() const override; | ||
|
|
||
|
|
@@ -58,6 +59,7 @@ class DBPS_EXPORT LocalEncryptionResult : public EncryptionResult { | |
| private: | ||
| std::vector<uint8_t> ciphertext_; | ||
| bool success_; | ||
| std::map<std::string, std::string> encryption_metadata_; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if it should be in this header file or in another, but can you add a comment with an example of what may be contained in here?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comments on the dbpa_interface.h file. |
||
| std::string error_message_; | ||
| std::map<std::string, std::string> error_fields_; | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,10 @@ | |
|
|
||
| using namespace dbps::external; | ||
|
|
||
| namespace { | ||
| const std::map<std::string, std::string> VALID_ENCRYPTION_METADATA = {{"dbps_agent_version", "v0.01"}}; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "Valid" hints that there is a counterpart, "invalid". Consider a different name. (maybe just a plain
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| } | ||
|
|
||
| // Test fixture for LocalDataBatchProtectionAgent tests | ||
| class LocalDataBatchProtectionAgentTest : public ::testing::Test { | ||
| protected: | ||
|
|
@@ -81,7 +85,7 @@ TEST_F(LocalDataBatchProtectionAgentTest, SuccessfulDecryption) { | |
| std::string app_context = R"({"user_id": "test_user"})"; | ||
|
|
||
| EXPECT_NO_THROW(agent.init("test_column", connection_config, app_context, "test_key", | ||
| Type::UNDEFINED, std::nullopt, CompressionCodec::UNCOMPRESSED, std::nullopt)); | ||
| Type::UNDEFINED, std::nullopt, CompressionCodec::UNCOMPRESSED, VALID_ENCRYPTION_METADATA)); | ||
|
|
||
| std::vector<uint8_t> test_data = {1, 2, 3, 4}; | ||
| std::map<std::string, std::string> encoding_attributes = {{"page_encoding", "PLAIN"}, {"page_type", "DICTIONARY_PAGE"}}; | ||
|
|
@@ -92,6 +96,56 @@ TEST_F(LocalDataBatchProtectionAgentTest, SuccessfulDecryption) { | |
| EXPECT_GT(result->size(), 0); | ||
| } | ||
|
|
||
| // Test roundtrip encryption/decryption | ||
| TEST_F(LocalDataBatchProtectionAgentTest, RoundTripEncryptDecrypt) { | ||
| LocalDataBatchProtectionAgent encrypt_agent; | ||
|
|
||
| std::map<std::string, std::string> connection_config; | ||
| std::string app_context = R"({"user_id": "test_user"})"; | ||
|
|
||
| EXPECT_NO_THROW(encrypt_agent.init("test_column", connection_config, app_context, "test_key", | ||
| Type::UNDEFINED, std::nullopt, CompressionCodec::UNCOMPRESSED, std::nullopt)); | ||
|
|
||
| // Original data to encrypt | ||
| std::vector<uint8_t> original_data = {1, 2, 3, 4, 5}; | ||
| std::map<std::string, std::string> encoding_attributes = {{"page_encoding", "PLAIN"}, {"page_type", "DICTIONARY_PAGE"}}; | ||
|
|
||
| // Encrypt the data | ||
| auto encrypt_result = encrypt_agent.Encrypt(original_data, encoding_attributes); | ||
|
|
||
| ASSERT_NE(encrypt_result, nullptr); | ||
| ASSERT_TRUE(encrypt_result->success()); | ||
| EXPECT_GT(encrypt_result->size(), 0); | ||
|
|
||
| // Verify encryption_metadata is present in the result | ||
| auto encryption_metadata = encrypt_result->encryption_metadata(); | ||
| ASSERT_TRUE(encryption_metadata.has_value()); | ||
| ASSERT_EQ(1, encryption_metadata->size()); | ||
| ASSERT_TRUE(encryption_metadata->at("dbps_agent_version").length() > 0); // Non-empty string | ||
|
|
||
| // Get the ciphertext | ||
| auto ciphertext_span = encrypt_result->ciphertext(); | ||
| std::vector<uint8_t> ciphertext(ciphertext_span.begin(), ciphertext_span.end()); | ||
|
|
||
| // Create a new agent for decryption with the encryption_metadata from the encryption result | ||
| LocalDataBatchProtectionAgent decrypt_agent; | ||
| EXPECT_NO_THROW(decrypt_agent.init("test_column", connection_config, app_context, "test_key", | ||
| Type::UNDEFINED, std::nullopt, CompressionCodec::UNCOMPRESSED, encryption_metadata)); | ||
|
|
||
| // Decrypt the ciphertext | ||
| auto decrypt_result = decrypt_agent.Decrypt(ciphertext, encoding_attributes); | ||
|
|
||
| ASSERT_NE(decrypt_result, nullptr); | ||
| ASSERT_TRUE(decrypt_result->success()); | ||
|
|
||
| // Verify the decrypted data matches the original | ||
| auto plaintext_span = decrypt_result->plaintext(); | ||
| std::vector<uint8_t> decrypted_data(plaintext_span.begin(), plaintext_span.end()); | ||
|
|
||
| ASSERT_EQ(original_data.size(), decrypted_data.size()); | ||
| EXPECT_EQ(original_data, decrypted_data); | ||
| } | ||
|
|
||
| // Test encryption without initialization | ||
| TEST_F(LocalDataBatchProtectionAgentTest, EncryptWithoutInit) { | ||
| LocalDataBatchProtectionAgent agent; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,21 @@ bool RemoteEncryptionResult::success() const { | |
| return response_ && response_->Success(); | ||
| } | ||
|
|
||
| const std::optional<std::map<std::string, std::string>> RemoteEncryptionResult::encryption_metadata() const { | ||
| if (!cached_encryption_metadata_.has_value() && response_ && response_->Success()) { | ||
| const auto& response_attrs = response_->GetResponseAttributes(); | ||
| // For the RemoteEncryptionResult, encryption_metadata_ is forwarded from the API response. | ||
| // The attribute in the API response is also named encryption_metadata. | ||
| // If the API reponse is empty, the cached value is set to NULL for compatibility with the class interface. | ||
| if (!response_attrs.encryption_metadata_.empty()) { | ||
| cached_encryption_metadata_ = response_attrs.encryption_metadata_; | ||
| } else { | ||
| cached_encryption_metadata_ = std::nullopt; | ||
| } | ||
|
Comment on lines
+52
to
+60
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's document what's going on here - it's a bit hard to grasp
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. You can let me know if the comment is sufficient and I can expand more if needed.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a clearer, thanks - but the whole concept of 'cached metadata' is a bit confusing. maybe let's add a short note on that? (in particular, why is it 'cached' ?)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you! it's much clearer now. |
||
| } | ||
| return cached_encryption_metadata_; | ||
| } | ||
|
|
||
| const std::string& RemoteEncryptionResult::error_message() const { | ||
| if (cached_error_message_.empty() && response_) { | ||
| if (!response_->Success()) { | ||
|
|
@@ -318,7 +333,9 @@ std::unique_ptr<DecryptionResult> RemoteDataBatchProtectionAgent::Decrypt(span<c | |
| compression_type_, | ||
| column_key_id_, | ||
| user_id_, | ||
| app_context_ | ||
| app_context_, | ||
| // For the Decrypt use case, column_encryption_metadata_ will be set on the constructor, however adding a default empty map for type safety. | ||
| column_encryption_metadata_.value_or(std::map<std::string, std::string>{}) | ||
| ); | ||
|
|
||
| // Validate that response fields match request fields | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ class DBPS_EXPORT RemoteEncryptionResult : public EncryptionResult { | |
| span<const uint8_t> ciphertext() const override; | ||
| std::size_t size() const override; | ||
| bool success() const override; | ||
| const std::optional<std::map<std::string, std::string>> encryption_metadata() const override; | ||
| const std::string& error_message() const override; | ||
| const std::map<std::string, std::string>& error_fields() const override; | ||
|
|
||
|
|
@@ -57,11 +58,12 @@ class DBPS_EXPORT RemoteEncryptionResult : public EncryptionResult { | |
| private: | ||
| std::unique_ptr<EncryptApiResponse> response_; | ||
|
|
||
| // Cache error message/fields to avoid repeated parsing of API response. | ||
| // Cache error message/fields/encryption_metadata to avoid repeated parsing of API response. | ||
| // Defined as mutable because it has lazy evaluation (updated only once on the getter methods) | ||
| // The caching of this is possible because the API response `response_` doesn't change after construction. | ||
| mutable std::string cached_error_message_; | ||
| mutable std::map<std::string, std::string> cached_error_fields_; | ||
| mutable std::optional<std::map<std::string, std::string>> cached_encryption_metadata_; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's add a note for the future, where we state that this is cacheable only because the metadata is identical across requests (but that may change)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, this is the RemoteEncryptionResult, not the RemoteDPBA. A new instance is returned on each call to RemoteDBPA.Encrypt(). This is cached separately on each RemoteEncryptionResult instance, to avoid parsing the Json with every call to But let me know if something is unclear, and we can discuss. |
||
| }; | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.