You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pfparsons (via GitHub)" <gi...@apache.org> on 2025/10/30 23:51:19 UTC
[PR] First pass at implementing external key material features in python [arrow]
pfparsons opened a new pull request, #48009:
URL: https://github.com/apache/arrow/pull/48009
### Rationale for this change
Enables external key material and rotation for parquet files in PyArrow
### What changes are included in this PR?
### Are these changes tested?
I've modified an existing (pytest xfail) test to do a basic read write test with some verification of the external json store.
Happy to add more robust testing if this change is welcome and headed in the right direction.
### Are there any user-facing changes?
This change enables users to optionally supply a parquet file path and FileSystem to CryptoFactory methods that provide en/decryption_properties.
KeyMaterial and FileSystemKeyMaterialStore classes are defined for testing purposes - available only by importing pyarrow._parquet_encryption.<classname>
Resolves #31869
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "pfparsons (via GitHub)" <gi...@apache.org>.
pfparsons commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3487357855
Hi @raulcd would you mind taking a look at this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2496153376
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -413,6 +425,17 @@ cdef class CryptoFactory(_Weakrefable):
encryption_config : EncryptionConfiguration
Configuration of the encryption, such as which columns to encrypt
+ parquet_file_path : str or pathlib.Path, default ""
+ Path to the parquet file to be encrypted. Only required when the
+ internal_key_material attribute of EncryptionConfiguration is set
+ to False. Used to derive the path for storing key material
+ specific to this parquet file.
+
+ filesystem : FileSystem, default None
+ Used only when internal_key_material is set to False on
+ EncryptionConfiguration. If nothinng passed, will be inferred
Review Comment:
```suggestion
EncryptionConfiguration. If None, the file system will be inferred
```
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -473,9 +529,167 @@ cdef class CryptoFactory(_Weakrefable):
def remove_cache_entries_for_all_tokens(self):
self.factory.get().RemoveCacheEntriesForAllTokens()
+ def rotate_master_keys(
+ self,
+ KmsConnectionConfig kms_connection_config,
+ parquet_file_path,
+ FileSystem filesystem,
Review Comment:
Probably makes sense to default to None and infer the file system by default:
```suggestion
FileSystem filesystem=None,
```
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -445,6 +482,15 @@ cdef class CryptoFactory(_Weakrefable):
Configuration of the decryption, such as cache timeout.
Can be None.
+ parquet_file_path : str or pathlib.Path, default ""
+ Path to the parquet file to be decrypted. Only required when
+ the parquet file uses external key material. Used to derive
+ the path to the external key material file.
+
+ filesystem : FileSystem, default None
+ Used only when the parquet file uses external key material. If
+ nothinng passed, will be inferred based on parquet_file_path.
Review Comment:
```suggestion
None, the file system will be inferred based on parquet_file_path.
```
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -473,9 +529,167 @@ cdef class CryptoFactory(_Weakrefable):
def remove_cache_entries_for_all_tokens(self):
self.factory.get().RemoveCacheEntriesForAllTokens()
+ def rotate_master_keys(
+ self,
+ KmsConnectionConfig kms_connection_config,
+ parquet_file_path,
+ FileSystem filesystem,
+ double_wrapping = True,
+ cache_lifetime_seconds = 600):
+ """ Rotates master encryption keys for a Parquet file that uses
+ external key material.
+
+ Parameters
+ ----------
+ kms_connection_config : KmsConnectionConfig
+ Configuration of connection to KMS
+
+ parquet_file_path : str or pathlib.Path
+ Path to a parquet file using external key material.
+
+ filesystem : FileSystem, default None
+ Used only when the parquet file uses external key material. If
+ nothinng passed, will be inferred based on parquet_file_path.
+
+ double_wrapping : bool, default True
+ In the single wrapping mode, decrypts data keys with old master
+ keys, then encrypts them with new master keys. In the double
+ wrapping mode, decrypts KEKs (key encryption keys) with old
+ master keys, generates new KEKs and encrypts them with new master
+ keys.
+
+ cache_lifetime_seconds : int or float, default 600
+ During key rotation, KMS Client and Key Encryption Keys will be
+ cached for this duration.
+
+ Returns
+ -------
+ file_decryption_properties : FileDecryptionProperties
+ File decryption properties.
Review Comment:
This doesn't return anything
```suggestion
```
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -473,9 +529,167 @@ cdef class CryptoFactory(_Weakrefable):
def remove_cache_entries_for_all_tokens(self):
self.factory.get().RemoveCacheEntriesForAllTokens()
+ def rotate_master_keys(
+ self,
+ KmsConnectionConfig kms_connection_config,
+ parquet_file_path,
+ FileSystem filesystem,
+ double_wrapping = True,
+ cache_lifetime_seconds = 600):
+ """ Rotates master encryption keys for a Parquet file that uses
+ external key material.
+
+ Parameters
+ ----------
+ kms_connection_config : KmsConnectionConfig
+ Configuration of connection to KMS
+
+ parquet_file_path : str or pathlib.Path
+ Path to a parquet file using external key material.
+
+ filesystem : FileSystem, default None
+ Used only when the parquet file uses external key material. If
+ nothinng passed, will be inferred based on parquet_file_path.
+
+ double_wrapping : bool, default True
+ In the single wrapping mode, decrypts data keys with old master
+ keys, then encrypts them with new master keys. In the double
+ wrapping mode, decrypts KEKs (key encryption keys) with old
+ master keys, generates new KEKs and encrypts them with new master
+ keys.
+
+ cache_lifetime_seconds : int or float, default 600
+ During key rotation, KMS Client and Key Encryption Keys will be
+ cached for this duration.
+
+ Returns
+ -------
+ file_decryption_properties : FileDecryptionProperties
+ File decryption properties.
+ """
+ cdef:
+ c_string c_parquet_file_path
+ shared_ptr[CFileSystem] c_filesystem
+
+ if parquet_file_path != "":
+ filesystem, parquet_file_path = _resolve_filesystem_and_path(
+ parquet_file_path, filesystem)
+
+ c_parquet_file_path = tobytes(parquet_file_path)
+ c_filesystem = _unwrap_fs(filesystem)
+
+ status = self.factory.get().SafeRotateMasterKeys(
+ deref(kms_connection_config.unwrap().get()),
+ c_parquet_file_path,
+ c_filesystem,
+ double_wrapping,
+ cache_lifetime_seconds);
+
+ return check_status(status)
Review Comment:
`check_status` returns an int, I think this should just be:
```suggestion
check_status(status)
```
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -473,9 +529,167 @@ cdef class CryptoFactory(_Weakrefable):
def remove_cache_entries_for_all_tokens(self):
self.factory.get().RemoveCacheEntriesForAllTokens()
+ def rotate_master_keys(
+ self,
+ KmsConnectionConfig kms_connection_config,
+ parquet_file_path,
+ FileSystem filesystem,
+ double_wrapping = True,
+ cache_lifetime_seconds = 600):
+ """ Rotates master encryption keys for a Parquet file that uses
+ external key material.
+
+ Parameters
+ ----------
+ kms_connection_config : KmsConnectionConfig
+ Configuration of connection to KMS
+
+ parquet_file_path : str or pathlib.Path
+ Path to a parquet file using external key material.
+
+ filesystem : FileSystem, default None
+ Used only when the parquet file uses external key material. If
+ nothinng passed, will be inferred based on parquet_file_path.
Review Comment:
```suggestion
None, the file system will be inferred based on parquet_file_path.
```
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +533,33 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet with external key material.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
- encryption_config = pe.EncryptionConfiguration(
- footer_key=FOOTER_KEY_NAME,
- column_keys={},
- internal_key_material=False)
-
- kms_connection_config = pe.KmsConnectionConfig(
- custom_kms_conf={FOOTER_KEY_NAME: FOOTER_KEY.decode("UTF-8")}
- )
-
- def kms_factory(kms_connection_configuration):
- return InMemoryKmsClient(kms_connection_configuration)
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
- crypto_factory = pe.CryptoFactory(kms_factory)
- # Write with encryption properties
- write_encrypted_parquet(path, data_table, encryption_config,
- kms_connection_config, crypto_factory)
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+
+ try:
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(
+ path)
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1 )
+ assert all([store.get_key_material(k) is not None for k in key_ids ])
+ except Exception as e:
+ pytest.fail(f"Unable to read external key material store: {e}")
+
+ assert data_table.equals(result_table)
Review Comment:
Could you also please add a test of key rotation?
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -473,9 +529,167 @@ cdef class CryptoFactory(_Weakrefable):
def remove_cache_entries_for_all_tokens(self):
self.factory.get().RemoveCacheEntriesForAllTokens()
+ def rotate_master_keys(
+ self,
+ KmsConnectionConfig kms_connection_config,
+ parquet_file_path,
+ FileSystem filesystem,
+ double_wrapping = True,
+ cache_lifetime_seconds = 600):
+ """ Rotates master encryption keys for a Parquet file that uses
+ external key material.
+
+ Parameters
+ ----------
+ kms_connection_config : KmsConnectionConfig
+ Configuration of connection to KMS
+
+ parquet_file_path : str or pathlib.Path
+ Path to a parquet file using external key material.
+
+ filesystem : FileSystem, default None
+ Used only when the parquet file uses external key material. If
+ nothinng passed, will be inferred based on parquet_file_path.
+
+ double_wrapping : bool, default True
+ In the single wrapping mode, decrypts data keys with old master
+ keys, then encrypts them with new master keys. In the double
+ wrapping mode, decrypts KEKs (key encryption keys) with old
+ master keys, generates new KEKs and encrypts them with new master
+ keys.
Review Comment:
This comment sounds like it's assuming that the `double_wrapping` parameter matches the existing state of the file, but I think it's possible to use this method to convert from single to double wrapping or vice-versa. How about something like:
```suggestion
In the single wrapping mode, encrypts data encryption keys with
new master keys. In the double wrapping mode, generates new
KEKs (key encryption keys) and uses these to encrypt the data keys,
and encrypts the KEKs with the new master keys.
```
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -473,9 +529,167 @@ cdef class CryptoFactory(_Weakrefable):
def remove_cache_entries_for_all_tokens(self):
self.factory.get().RemoveCacheEntriesForAllTokens()
+ def rotate_master_keys(
+ self,
+ KmsConnectionConfig kms_connection_config,
+ parquet_file_path,
+ FileSystem filesystem,
+ double_wrapping = True,
+ cache_lifetime_seconds = 600):
+ """ Rotates master encryption keys for a Parquet file that uses
+ external key material.
+
+ Parameters
+ ----------
+ kms_connection_config : KmsConnectionConfig
+ Configuration of connection to KMS
+
+ parquet_file_path : str or pathlib.Path
+ Path to a parquet file using external key material.
+
+ filesystem : FileSystem, default None
+ Used only when the parquet file uses external key material. If
+ nothinng passed, will be inferred based on parquet_file_path.
+
+ double_wrapping : bool, default True
+ In the single wrapping mode, decrypts data keys with old master
+ keys, then encrypts them with new master keys. In the double
+ wrapping mode, decrypts KEKs (key encryption keys) with old
+ master keys, generates new KEKs and encrypts them with new master
+ keys.
+
+ cache_lifetime_seconds : int or float, default 600
+ During key rotation, KMS Client and Key Encryption Keys will be
+ cached for this duration.
+
+ Returns
+ -------
+ file_decryption_properties : FileDecryptionProperties
+ File decryption properties.
+ """
+ cdef:
+ c_string c_parquet_file_path
+ shared_ptr[CFileSystem] c_filesystem
+
+ if parquet_file_path != "":
+ filesystem, parquet_file_path = _resolve_filesystem_and_path(
+ parquet_file_path, filesystem)
+
+ c_parquet_file_path = tobytes(parquet_file_path)
+ c_filesystem = _unwrap_fs(filesystem)
+
+ status = self.factory.get().SafeRotateMasterKeys(
+ deref(kms_connection_config.unwrap().get()),
+ c_parquet_file_path,
+ c_filesystem,
+ double_wrapping,
+ cache_lifetime_seconds);
+
+ return check_status(status)
+
cdef inline shared_ptr[CPyCryptoFactory] unwrap(self):
return self.factory
+
+cdef class KeyMaterial(_Weakrefable):
+
+ @property
+ def is_footer_key(self):
+ return self.key_material.get().is_footer_key()
+
+ @property
+ def is_double_wrapped(self):
+ return self.key_material.get().is_double_wrapped()
+
+ @property
+ def master_key_id(self):
+ return frombytes(self.key_material.get().master_key_id())
+
+ @property
+ def wrapped_dek(self):
+ return frombytes(self.key_material.get().wrapped_dek())
+
+ @property
+ def kek_id(self):
+ return frombytes(self.key_material.get().kek_id())
+
+ @property
+ def wrapped_kek(self):
+ return frombytes(self.key_material.get().wrapped_kek())
+
+ @property
+ def kms_instance_id(self):
+ return frombytes(self.key_material.get().kms_instance_id())
+
+ @property
+ def kms_instance_url(self):
+ return frombytes(self.key_material.get().kms_instance_url())
+ @staticmethod
+ cdef inline KeyMaterial wrap(shared_ptr[CKeyMaterial] key_material):
+ wrapper = KeyMaterial()
+ wrapper.key_material = key_material
+ return wrapper
+
+ @staticmethod
+ def parse(
+ const c_string key_material_string):
+ cdef:
+ shared_ptr[CKeyMaterial] c_key_material
+ c_key_material = make_shared[CKeyMaterial](move(
+ CKeyMaterial.Parse(key_material_string)
+ ))
+ return KeyMaterial.wrap(c_key_material)
+
+cdef class FileSystemKeyMaterialStore(_Weakrefable):
+
+ def get_key_material(self, key_id):
+ cdef:
+ c_string c_key_id = tobytes(key_id)
+ c_string c_key_material_string
+
+ c_key_material_string = self.store.get().GetKeyMaterial(c_key_id)
+ if c_key_material_string.empty():
+ raise KeyError("Invalid key id")
+ return KeyMaterial.parse(c_key_material_string)
+
+ def get_key_id_set(self):
+ return self.store.get().GetKeyIDSet()
+
+ @classmethod
+ def for_file(cls, parquet_file_path,
+ FileSystem filesystem = None):
+ """Creates a FileSystemKeyMaterialStore for a parquet file that
+ was created with external key material.
+
+ Parameters
+ ----------
+ parquet_file_path : str or pathlib.Path
+ Path to a parquet file using external key material.
+
+ filesystem : FileSystem, default None
+ FileSystem where the parquet file is located. If nothinng passed,
Review Comment:
```suggestion
FileSystem where the parquet file is located. If None,
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2511731314
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
Where is that environment variable read though? It looks like it isn't actually used as far as I can tell, and the actual important environment variable is `HYPOTHESIS_PROFILE`. When that's left as the default `dev` value, the hypothesis tests are still run, but with a much lower number of examples ([profiles defined here)](https://github.com/apache/arrow/blob/bbb3d48000d98b5cec868a417734422b4c9a6976/python/pyarrow/tests/conftest.py#L35-L45).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "pfparsons (via GitHub)" <gi...@apache.org>.
pfparsons commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2511961788
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
Ok will do. I'm also happy to look into #47734 if helpful.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "pfparsons (via GitHub)" <gi...@apache.org>.
pfparsons commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2507036062
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
I did feel a bit silly writing it this way and my (shaky) rationale might be based on a misunderstanding of how/when hypothesis tests are run in ci. I see PYARROW_TEST_HYPOTHESIS is set to ON in [/dev/tasks.yml:596](https://github.com/apache/arrow/blob/main/dev/tasks/tasks.yml#L596) for a conda-python-3.11 docker test, but I couldn't find any other place where the required env var or cli flag is explicitly set. So I did this to ensure at least one test with the default values would run regardless of whether hypothesis is enabled. Maybe this is all a bit too much for the sake of permuting a couple booleans :) but I'm also interested in learning a little more about how the build works.
I'm interpreting your comment to mean the test function may be simplified to one hypothesis driven function and so I'll make this change now. Happy to follow any other suggestion you may have.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "pfparsons (via GitHub)" <gi...@apache.org>.
pfparsons commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2507069218
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -402,7 +411,9 @@ cdef class CryptoFactory(_Weakrefable):
def file_encryption_properties(self,
KmsConnectionConfig kms_connection_config,
- EncryptionConfiguration encryption_config):
+ EncryptionConfiguration encryption_config,
+ parquet_file_path="",
Review Comment:
Thanks, yes I did not like this either. I've changed the default to None for both CryptoFactory file_{en,de}cryption_properties methods.
My initial inclination to use empty string was based on following the defaults defined in [CryptoFactory.h](https://github.com/apache/arrow/blob/cd23a765442bdbaaef43d0e4b239094fb01e37ae/cpp/src/parquet/encryption/crypto_factory.h#L112) - and handled in [file_system_key_material_store::Make](https://github.com/apache/arrow/blob/cd23a765442bdbaaef43d0e4b239094fb01e37ae/cpp/src/parquet/encryption/file_system_key_material_store.cc#L45)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2506843453
##########
python/pyarrow/_parquet_encryption.pyx:
##########
@@ -402,7 +411,9 @@ cdef class CryptoFactory(_Weakrefable):
def file_encryption_properties(self,
KmsConnectionConfig kms_connection_config,
- EncryptionConfiguration encryption_config):
+ EncryptionConfiguration encryption_config,
+ parquet_file_path="",
Review Comment:
Is the empty string really a good default if this parameter is not used?, can we use `None` instead?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] Implement external key material features in Python [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve merged PR #48009:
URL: https://github.com/apache/arrow/pull/48009
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3515326537
:warning: GitHub issue #31869 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "pfparsons (via GitHub)" <gi...@apache.org>.
pfparsons commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2507071403
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -588,8 +588,7 @@ def test_external_key_material_rotation(
double_wrapping=double_wrap_initial)
# initial master key version - see MockVersioningKmsClient docstring
- kms_connection_config = pe.KmsConnectionConfig(
- key_access_token="1", custom_kms_conf={"master_key_version": "1"})
Review Comment:
Sorry, just noticed that I left the (unused) custom_kms_conf dict in the KmsConnectionConfig. This was from an earlier failed attempt to share a kms master_key_version state across instances of a mock KmsClient.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "pfparsons (via GitHub)" <gi...@apache.org>.
pfparsons commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2507036062
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
I did feel a bit silly writing it this way and my (shaky) rationale might be based on a misunderstanding of how/when hypothesis tests are run in ci. I see PYARROW_TEST_HYPOTHESIS is set to ON in [/dev/tasks.yml:596] (https://github.com/apache/arrow/blob/main/dev/tasks/tasks.yml#L596) for a conda-python-3.11 docker test, but I couldn't find any other place where the required env var or cli flag is explicitly set. So I did this to ensure at least one test with the default values would run regardless of whether hypothesis is enabled. Maybe this is all a bit too much for the sake of permuting a couple booleans :) but I'm also interested in learning a little more about how the build works.
I'm interpreting your comment to mean the test function may be simplified to one hypothesis driven function and so I'll make this change now. Happy to follow any other suggestion you may have.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] Implement external key material features in Python [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3518651328
:warning: GitHub issue #31869 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2511890047
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
OK I just tested running the tests locally and I see I get messages like `SKIPPED (hypothesis NOT enabled)` for hypothesis tests by default, unless I set `PYARROW_TEST_HYPOTHESIS=ON`. And I now see that the environment variable is handled here: https://github.com/apache/arrow/blob/bbb3d48000d98b5cec868a417734422b4c9a6976/python/pyarrow/tests/conftest.py#L89
So apologies for the confusion. I agree with Raúl that it would be best to avoid using hypothesis here and just parameterize the test.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3470744281
:warning: GitHub issue #31869 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3487339684
:warning: GitHub issue #31869 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] First pass at implementing external key material features in python [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3470736380
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2508322948
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
Hmm OK fair enough then, I had assumed that the hypothesis tests are always run. I don't see where that environment variable is actually used though. We load different profiles depending on the `HYPOTHESIS_PROFILE` variable [here](https://github.com/apache/arrow/blob/bbb3d48000d98b5cec868a417734422b4c9a6976/python/pyarrow/tests/conftest.py#L35-L45) but I can't find anywhere that disables them completely.
@raulcd do you have any idea how/if the `PYARROW_TEST_HYPOTHESIS` variable is used?
If I've interpreted this correctly and the hypothesis tests will always be run but with the `dev` profile by default, then I think it makes sense to simplify this to one hypothesis driven function. Otherwise we could just use `@pytest.mark.parametrize` here instead of hypothesis as the number of permutations is small.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2509706818
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
The hypothesis tests are only run on the extra crossbow nightly check `test-conda-python-3.11-hypothesis` which is the one that sets `PYARROW_TEST_HYPOTHESIS=ON` on the `tasks.yml` file:
https://github.com/apache/arrow/blob/c10847cde60fa03c1f5c35d70381d679b62f434e/dev/tasks/tasks.yml#L596
There has been discussions about what to do with the hypothesis tests on the past as there's not much maintenance going on over them. For example, they've been failing for more than one month now, see:
- https://github.com/apache/arrow/issues/47734
If the hypothesis permutations are not strictly necessary I would rather just add a test that runs unconditionally.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2512022176
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
That would be really helpful and would be great indeed!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] Implement external key material features in Python [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3521186594
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 5112de23222dda6fa449f0488265ee0679fd227b.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/55171824945) has more details. It also includes information about 137 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3470742120
:warning: GitHub issue #31869 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "pfparsons (via GitHub)" <gi...@apache.org>.
pfparsons commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2505968147
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +533,33 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet with external key material.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
- encryption_config = pe.EncryptionConfiguration(
- footer_key=FOOTER_KEY_NAME,
- column_keys={},
- internal_key_material=False)
-
- kms_connection_config = pe.KmsConnectionConfig(
- custom_kms_conf={FOOTER_KEY_NAME: FOOTER_KEY.decode("UTF-8")}
- )
-
- def kms_factory(kms_connection_configuration):
- return InMemoryKmsClient(kms_connection_configuration)
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
- crypto_factory = pe.CryptoFactory(kms_factory)
- # Write with encryption properties
- write_encrypted_parquet(path, data_table, encryption_config,
- kms_connection_config, crypto_factory)
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+
+ try:
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(
+ path)
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1 )
+ assert all([store.get_key_material(k) is not None for k in key_ids ])
+ except Exception as e:
+ pytest.fail(f"Unable to read external key material store: {e}")
+
+ assert data_table.equals(result_table)
Review Comment:
@adamreeve Thank you very much for the review! I've incorporated all of your changes and added a key rotation unit test. Sorry about missing the pre-commit checks. I believe I've corrected all the formatter/linter errors and have a better setup to avoid this in the future.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #48009:
URL: https://github.com/apache/arrow/pull/48009#discussion_r2506685890
##########
python/pyarrow/tests/parquet/test_encryption.py:
##########
@@ -514,31 +538,126 @@ def kms_factory(kms_connection_configuration):
# assert table.num_rows == result_table.num_rows
-@pytest.mark.xfail(reason="External key material not supported yet")
-def test_encrypted_parquet_write_external(tempdir, data_table):
- """Write an encrypted parquet, with external key
- material.
- Currently it's not implemented, so should throw
- an exception"""
+def test_encrypted_parquet_write_read_external(tempdir, data_table,
+ external_encryption_config):
+ """Write an encrypted parquet file with external key material, verify
+ it's encrypted, then read both the table and external store.
+ """
path = tempdir / PARQUET_NAME
- # Encrypt the file with the footer key
+ kms_connection_config, crypto_factory = write_encrypted_file(
+ path, data_table, FOOTER_KEY_NAME, COL_KEY_NAME, FOOTER_KEY, COL_KEY,
+ external_encryption_config)
+
+ verify_file_encrypted(path)
+
+ decryption_config = pe.DecryptionConfiguration()
+ result_table = read_encrypted_parquet(
+ path, decryption_config, kms_connection_config, crypto_factory,
+ internal_key_material=False)
+ store = pa._parquet_encryption.FileSystemKeyMaterialStore.for_file(path)
+
+ assert len(key_ids := store.get_key_id_set()) == (
+ len(external_encryption_config.column_keys[COL_KEY_NAME]) + 1)
+ assert all([store.get_key_material(k) is not None for k in key_ids])
+ assert data_table.equals(result_table)
+
+
+def test_external_key_material_rotation(
+ tempdir,
+ data_table,
+ double_wrap_initial=True,
+ double_wrap_rotated=True):
+ """Tests CryptoFactory.rotate_master_keys
+
+ Note: The CryptoFactory.rotate_master_keys() double_wrapping keword arg
+ may be either True (the default) or False regardless of whether
+ EncryptionConfig.double_wrapping was set to true (also the default) when
+ the external key material store was written. This means double wrapping may
+ be set one way initially and then applied or removed during rotation.
+ When run as a regular pytest test, this function tests the default
+ scenario - double_wrapping before and after rotation.
+
+ A separate wrapper test function below is driven by hypothesis strategies
+ to run through the permutations."""
Review Comment:
The `double_wrap_initial=True` and `double_wrap_rotated=True` permutation will also be run by the test using hypothesis below, so this could be simplified to one test function.
##########
python/pyarrow/tests/parquet/conftest.py:
##########
@@ -103,3 +103,8 @@ def s3_example_fs(s3_server):
fs.create_dir("mybucket")
yield fs, uri, path
+
+
+@pytest.fixture(scope="class")
+def reusable_tempdir(tmp_path_factory):
+ return tmp_path_factory.mktemp('encryption')
Review Comment:
This isn't specific to encryption here and could be used by other tests in the future, so maybe the directory base name should be more generic, eg. `pyarrow-parquet`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-31869: [Python][Parquet] First pass at implementing external key material features in python [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48009:
URL: https://github.com/apache/arrow/pull/48009#issuecomment-3506809498
:warning: GitHub issue #31869 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org