adamreeve commented on code in PR #51350:
URL: https://github.com/apache/arrow/pull/51350#discussion_r4022003610
##########
cpp/src/parquet/encryption/key_management_test.cc:
##########
@@ -441,4 +509,100 @@ TEST_F(TestEncryptionKeyManagement,
ReadParquetMRExternalKeyMaterialFile) {
}
}
+TEST_F(TestEncryptionKeyManagement, ReadKmsUrlFromFile) {
+ this->SetupCryptoFactory(true);
+
+ constexpr bool internal_key_material = true;
+ constexpr bool double_wrapping = true;
+ constexpr int encryption_no = 0;
+
+ std::string file_name = "kms-config-test-file.parquet.encrypted";
+ std::string file_path = temp_dir_->path().ToString() + file_name;
+
+ auto encryption_config =
+ GetEncryptionConfiguration(double_wrapping, internal_key_material,
encryption_no);
+
+ KmsConnectionConfig write_config;
+ write_config.kms_instance_id = "123";
+ write_config.kms_instance_url = "https://example.com/kms";;
+
+ auto file_encryption_properties =
+ crypto_factory_.GetFileEncryptionProperties(write_config,
encryption_config);
+ encryptor_.EncryptFile(file_path, file_encryption_properties);
+
+ for (const auto& enable_kms_url_read : {false, true}) {
+ // Create a fresh crypto factory and client factory for each read
+ // to avoid re-using cached clients.
+ CryptoFactory read_crypto_factory;
+ auto kms_client_factory =
+ std::make_shared<TestOnlyInMemoryKmsClientFactory>(true, key_list_);
+ read_crypto_factory.RegisterKmsClientFactory(kms_client_factory);
+
+ auto decryption_config = DecryptionConfiguration();
+ decryption_config.read_kms_url = enable_kms_url_read;
+
+ KmsConnectionConfig read_config;
+
+ auto file_decryption_properties =
+ read_crypto_factory.GetFileDecryptionProperties(read_config,
decryption_config);
+
+ decryptor_.DecryptFile(file_path, file_decryption_properties);
+
+ ASSERT_EQ(kms_client_factory->CreationRequests().size(), 1);
+ const auto& request = kms_client_factory->CreationRequests()[0];
+ EXPECT_EQ(request.kms_instance_id, "123");
+ if (enable_kms_url_read) {
+ EXPECT_EQ(request.kms_instance_url, "https://example.com/kms";);
+ } else {
+ EXPECT_EQ(request.kms_instance_url, "DEFAULT");
+ }
+ }
+}
+
+TEST_F(TestEncryptionKeyManagement, ReadKmsUrlFromFileDuringKeyRotation) {
+ // Use an empty config for rotation
+ const KmsConnectionConfig rotation_config;
+ const auto requests = RotateKeysWithKmsConfig(rotation_config,
/*read_kms_url=*/true);
+
+ ASSERT_EQ(requests.size(), 2);
+ // The first KMS creation request is for wrapping new keys.
+ // This uses the empty config provided.
+ EXPECT_EQ(requests[0].kms_instance_id, "");
+ EXPECT_EQ(requests[0].kms_instance_url, "");
Review Comment:
That is probably the intended behaviour, but it doesn't actually work this
way, and the tests do pass. [SetDefaultIfEmpty is called on the
`kms_connection_config_`
member](https://github.com/apache/arrow/blob/40067d77eb4114baf77522c61b08bac9231a4422/cpp/src/parquet/encryption/file_key_wrapper.cc#L38),
but then [the original, unmodified config is passed to
`GetKmsClient`](https://github.com/apache/arrow/blob/40067d77eb4114baf77522c61b08bac9231a4422/cpp/src/parquet/encryption/file_key_wrapper.cc#L43).
I think this is probably wrong, but fixing this requires a bunch of
follow-up changes, so should be a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
