You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "adamreeve (via GitHub)" <gi...@apache.org> on 2025/03/14 02:34:37 UTC
[PR] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
adamreeve opened a new pull request, #7286:
URL: https://github.com/apache/arrow-rs/pull/7286
# Which issue does this PR close?
Closes #7257.
# Rationale for this change
This allows Parquet readers to not need to directly specify the data encryption keys required to read an encrypted Parquet file, and instead be able to retrieve them dynamically based on the key metadata.
This is a prerequisite for implementing the higher level Key Management Tools API (#7256)
# What changes are included in this PR?
* Adds a new `KeyRetriever` trait.
* Updates `FileDecryptionProperties` so that it can store a `KeyRetriever` rather than explicit decryption keys.
* Adds a new `ColumnCryptoMetaData` type corresponding to the type of the same name in the Thrift format, so that this can be parsed from the column metadata and used to retrieve the key metadata to pass to the `KeyRetriever`.
* Changes how we decide if a column is encrypted. We now read this from the file metadata rather than using the `FileDecryptionProperties` (so `FileDecryptor::is_column_encrypted` has been removed).
# Are there any user-facing changes?
Yes, this adds a new way for users to create `FileDecryptionProperties`.
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#discussion_r2000144632
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,125 +160,211 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone)]
Review Comment:
I've included this suggestion, thanks Rok
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#discussion_r2076762353
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,49 +167,158 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+impl PartialEq for DecryptionKeys {
+ fn eq(&self, other: &Self) -> bool {
+ match (self, other) {
+ (DecryptionKeys::Explicit(keys), DecryptionKeys::Explicit(other_keys)) => {
+ keys.footer_key == other_keys.footer_key
+ && keys.column_keys == other_keys.column_keys
+ }
+ (DecryptionKeys::ViaRetriever(_), DecryptionKeys::ViaRetriever(_)) => true,
+ _ => false,
+ }
+ }
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone, PartialEq)]
+pub struct FileDecryptionProperties {
+ keys: DecryptionKeys,
pub(crate) aad_prefix: Option<Vec<u8>>,
}
impl FileDecryptionProperties {
- /// Returns a new FileDecryptionProperties builder
+ /// Returns a new [`FileDecryptionProperties`] builder that will use the provided key to
+ /// decrypt footer metadata.
pub fn builder(footer_key: Vec<u8>) -> DecryptionPropertiesBuilder {
DecryptionPropertiesBuilder::new(footer_key)
}
+
+ /// Returns a new [`FileDecryptionProperties`] builder that uses a [`KeyRetriever`]
+ /// to get decryption keys based on key metadata.
+ pub fn with_key_retriever(key_retriever: Arc<dyn KeyRetriever>) -> DecryptionPropertiesBuilder {
+ DecryptionPropertiesBuilder::new_with_key_retriever(key_retriever)
+ }
+
+ /// Get the encryption key for decrypting a file's footer,
+ /// and also column data if uniform encryption is used.
+ pub(crate) fn footer_key(&self, key_metadata: Option<&[u8]>) -> Result<Cow<Vec<u8>>> {
+ match &self.keys {
+ DecryptionKeys::Explicit(keys) => Ok(Cow::Borrowed(&keys.footer_key)),
+ DecryptionKeys::ViaRetriever(retriever) => {
+ let key = retriever.retrieve_key(key_metadata.unwrap_or_default())?;
+ Ok(Cow::Owned(key))
+ }
+ }
+ }
+
+ /// Get the column-specific encryption key for decrypting column data and metadata within a file
+ pub(crate) fn column_key(
+ &self,
+ column_name: &str,
+ key_metadata: Option<&[u8]>,
+ ) -> Result<Cow<Vec<u8>>> {
+ match &self.keys {
+ DecryptionKeys::Explicit(keys) => match keys.column_keys.get(column_name) {
+ None => Err(general_err!(
+ "No column decryption key set for column '{}'",
+ column_name
+ )),
+ Some(key) => Ok(Cow::Borrowed(key)),
+ },
+ DecryptionKeys::ViaRetriever(retriever) => {
+ let key = retriever.retrieve_key(key_metadata.unwrap_or_default())?;
+ Ok(Cow::Owned(key))
+ }
+ }
+ }
+}
+
+impl std::fmt::Debug for FileDecryptionProperties {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ write!(f, "FileDecryptionProperties {{ }}")
+ }
}
+/// Builder for [`FileDecryptionProperties`]
pub struct DecryptionPropertiesBuilder {
- footer_key: Vec<u8>,
+ footer_key: Option<Vec<u8>>,
+ key_retriever: Option<Arc<dyn KeyRetriever>>,
column_keys: HashMap<String, Vec<u8>>,
aad_prefix: Option<Vec<u8>>,
Review Comment:
A bit late but I made a PR to follow up on this here: https://github.com/apache/arrow-rs/pull/7477
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#discussion_r2010832726
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,49 +167,158 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+impl PartialEq for DecryptionKeys {
+ fn eq(&self, other: &Self) -> bool {
+ match (self, other) {
+ (DecryptionKeys::Explicit(keys), DecryptionKeys::Explicit(other_keys)) => {
+ keys.footer_key == other_keys.footer_key
+ && keys.column_keys == other_keys.column_keys
+ }
+ (DecryptionKeys::ViaRetriever(_), DecryptionKeys::ViaRetriever(_)) => true,
+ _ => false,
+ }
+ }
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone, PartialEq)]
+pub struct FileDecryptionProperties {
+ keys: DecryptionKeys,
pub(crate) aad_prefix: Option<Vec<u8>>,
}
impl FileDecryptionProperties {
- /// Returns a new FileDecryptionProperties builder
+ /// Returns a new [`FileDecryptionProperties`] builder that will use the provided key to
+ /// decrypt footer metadata.
pub fn builder(footer_key: Vec<u8>) -> DecryptionPropertiesBuilder {
DecryptionPropertiesBuilder::new(footer_key)
}
+
+ /// Returns a new [`FileDecryptionProperties`] builder that uses a [`KeyRetriever`]
+ /// to get decryption keys based on key metadata.
+ pub fn with_key_retriever(key_retriever: Arc<dyn KeyRetriever>) -> DecryptionPropertiesBuilder {
+ DecryptionPropertiesBuilder::new_with_key_retriever(key_retriever)
+ }
+
+ /// Get the encryption key for decrypting a file's footer,
+ /// and also column data if uniform encryption is used.
+ pub(crate) fn footer_key(&self, key_metadata: Option<&[u8]>) -> Result<Cow<Vec<u8>>> {
+ match &self.keys {
+ DecryptionKeys::Explicit(keys) => Ok(Cow::Borrowed(&keys.footer_key)),
+ DecryptionKeys::ViaRetriever(retriever) => {
+ let key = retriever.retrieve_key(key_metadata.unwrap_or_default())?;
+ Ok(Cow::Owned(key))
+ }
+ }
+ }
+
+ /// Get the column-specific encryption key for decrypting column data and metadata within a file
+ pub(crate) fn column_key(
+ &self,
+ column_name: &str,
+ key_metadata: Option<&[u8]>,
+ ) -> Result<Cow<Vec<u8>>> {
+ match &self.keys {
+ DecryptionKeys::Explicit(keys) => match keys.column_keys.get(column_name) {
+ None => Err(general_err!(
+ "No column decryption key set for column '{}'",
+ column_name
+ )),
+ Some(key) => Ok(Cow::Borrowed(key)),
+ },
+ DecryptionKeys::ViaRetriever(retriever) => {
+ let key = retriever.retrieve_key(key_metadata.unwrap_or_default())?;
+ Ok(Cow::Owned(key))
+ }
+ }
+ }
+}
+
+impl std::fmt::Debug for FileDecryptionProperties {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ write!(f, "FileDecryptionProperties {{ }}")
+ }
}
+/// Builder for [`FileDecryptionProperties`]
pub struct DecryptionPropertiesBuilder {
- footer_key: Vec<u8>,
+ footer_key: Option<Vec<u8>>,
+ key_retriever: Option<Arc<dyn KeyRetriever>>,
column_keys: HashMap<String, Vec<u8>>,
aad_prefix: Option<Vec<u8>>,
Review Comment:
Good point, I did this for `FileDecryptionProperties` but I'm not sure why I didn't do it for `DecryptionPropertiesBuilder` too, I'll follow up and tidy this up later.
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#discussion_r1997663765
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -706,30 +706,26 @@ impl<T: ChunkReader + 'static> Iterator for ReaderPageIterator<T> {
.schema_descr()
.column(self.column_idx);
- if file_decryptor.is_column_encrypted(column_name.name()) {
- let data_decryptor = file_decryptor.get_column_data_decryptor(column_name.name());
- let data_decryptor = match data_decryptor {
- Ok(data_decryptor) => data_decryptor,
- Err(err) => return Some(Err(err)),
- };
-
- let metadata_decryptor =
- file_decryptor.get_column_metadata_decryptor(column_name.name());
- let metadata_decryptor = match metadata_decryptor {
- Ok(metadata_decryptor) => metadata_decryptor,
- Err(err) => return Some(Err(err)),
- };
-
- let crypto_context = CryptoContext::new(
- rg_idx,
- self.column_idx,
- data_decryptor,
- metadata_decryptor,
- file_decryptor.file_aad().clone(),
- );
- Some(Arc::new(crypto_context))
- } else {
- None
+ let crypto_metadata = self
+ .metadata
+ .row_group(rg_idx)
+ .column(self.column_idx)
+ .crypto_metadata();
+
+ match crypto_metadata {
+ Some(crypto_metadata) => {
+ match CryptoContext::for_column(
+ file_decryptor,
+ crypto_metadata,
+ column_name.name(),
+ rg_idx,
+ self.column_idx,
+ ) {
+ Ok(context) => Some(Arc::new(context)),
+ Err(err) => return Some(Err(err)),
+ }
+ }
+ None => None,
Review Comment:
This is much nicer now.
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,125 +160,211 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone)]
Review Comment:
```suggestion
impl PartialEq for DecryptionKeys {
fn eq(&self, other: &Self) -> bool {
match (self, other) {
(DecryptionKeys::Explicit(keys), DecryptionKeys::Explicit(other_keys)) => {
keys.footer_key == other_keys.footer_key && keys.column_keys == other_keys.column_keys
}
(DecryptionKeys::ViaRetriever(_), DecryptionKeys::ViaRetriever(_)) => true,
_ => false,
}
}
}
/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
#[derive(Clone, PartialEq)]
pub struct FileDecryptionProperties {
```
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,125 +160,211 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone)]
+pub struct FileDecryptionProperties {
+ keys: DecryptionKeys,
pub(crate) aad_prefix: Option<Vec<u8>>,
}
impl FileDecryptionProperties {
- /// Returns a new FileDecryptionProperties builder
+ /// Returns a new [`FileDecryptionProperties`] builder that will use the provided key to
+ /// decrypt footer metadata.
pub fn builder(footer_key: Vec<u8>) -> DecryptionPropertiesBuilder {
DecryptionPropertiesBuilder::new(footer_key)
}
+
+ /// Returns a new [`FileDecryptionProperties`] builder that uses a [`KeyRetriever`]
+ /// to get decryption keys based on key metadata.
+ pub fn with_key_retriever(key_retriever: Arc<dyn KeyRetriever>) -> DecryptionPropertiesBuilder {
+ DecryptionPropertiesBuilder::new_with_key_retriever(key_retriever)
+ }
}
+impl std::fmt::Debug for FileDecryptionProperties {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ write!(f, "FileDecryptionProperties {{ }}")
+ }
+}
+
+impl PartialEq for FileDecryptionProperties {
+ // FileDecryptionProperties needs to implement PartialEq to allow
+ // ParquetMetaData to implement PartialEq.
+ // We cannot compare a key retriever, but this isn't derived from the metadata.
Review Comment:
I think implementing `PartialEq` is fine. Perhaps it'd be cleaner to implement it on `DecryptionKeys` and derive it on `FileDecryptionProperties`? See comment above.
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#discussion_r2000144395
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,125 +160,211 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone)]
+pub struct FileDecryptionProperties {
+ keys: DecryptionKeys,
pub(crate) aad_prefix: Option<Vec<u8>>,
}
impl FileDecryptionProperties {
- /// Returns a new FileDecryptionProperties builder
+ /// Returns a new [`FileDecryptionProperties`] builder that will use the provided key to
+ /// decrypt footer metadata.
pub fn builder(footer_key: Vec<u8>) -> DecryptionPropertiesBuilder {
DecryptionPropertiesBuilder::new(footer_key)
}
+
+ /// Returns a new [`FileDecryptionProperties`] builder that uses a [`KeyRetriever`]
+ /// to get decryption keys based on key metadata.
+ pub fn with_key_retriever(key_retriever: Arc<dyn KeyRetriever>) -> DecryptionPropertiesBuilder {
+ DecryptionPropertiesBuilder::new_with_key_retriever(key_retriever)
+ }
}
+impl std::fmt::Debug for FileDecryptionProperties {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ write!(f, "FileDecryptionProperties {{ }}")
+ }
+}
+
+impl PartialEq for FileDecryptionProperties {
+ // FileDecryptionProperties needs to implement PartialEq to allow
+ // ParquetMetaData to implement PartialEq.
+ // We cannot compare a key retriever, but this isn't derived from the metadata.
Review Comment:
Yeah that's a lot nicer, good idea thanks
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#discussion_r2010618162
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,49 +167,158 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+impl PartialEq for DecryptionKeys {
+ fn eq(&self, other: &Self) -> bool {
+ match (self, other) {
+ (DecryptionKeys::Explicit(keys), DecryptionKeys::Explicit(other_keys)) => {
+ keys.footer_key == other_keys.footer_key
+ && keys.column_keys == other_keys.column_keys
+ }
+ (DecryptionKeys::ViaRetriever(_), DecryptionKeys::ViaRetriever(_)) => true,
+ _ => false,
+ }
+ }
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone, PartialEq)]
+pub struct FileDecryptionProperties {
+ keys: DecryptionKeys,
pub(crate) aad_prefix: Option<Vec<u8>>,
}
impl FileDecryptionProperties {
- /// Returns a new FileDecryptionProperties builder
+ /// Returns a new [`FileDecryptionProperties`] builder that will use the provided key to
+ /// decrypt footer metadata.
pub fn builder(footer_key: Vec<u8>) -> DecryptionPropertiesBuilder {
DecryptionPropertiesBuilder::new(footer_key)
}
+
+ /// Returns a new [`FileDecryptionProperties`] builder that uses a [`KeyRetriever`]
+ /// to get decryption keys based on key metadata.
+ pub fn with_key_retriever(key_retriever: Arc<dyn KeyRetriever>) -> DecryptionPropertiesBuilder {
+ DecryptionPropertiesBuilder::new_with_key_retriever(key_retriever)
+ }
+
+ /// Get the encryption key for decrypting a file's footer,
+ /// and also column data if uniform encryption is used.
+ pub(crate) fn footer_key(&self, key_metadata: Option<&[u8]>) -> Result<Cow<Vec<u8>>> {
+ match &self.keys {
+ DecryptionKeys::Explicit(keys) => Ok(Cow::Borrowed(&keys.footer_key)),
+ DecryptionKeys::ViaRetriever(retriever) => {
+ let key = retriever.retrieve_key(key_metadata.unwrap_or_default())?;
+ Ok(Cow::Owned(key))
+ }
+ }
+ }
+
+ /// Get the column-specific encryption key for decrypting column data and metadata within a file
+ pub(crate) fn column_key(
+ &self,
+ column_name: &str,
+ key_metadata: Option<&[u8]>,
+ ) -> Result<Cow<Vec<u8>>> {
+ match &self.keys {
+ DecryptionKeys::Explicit(keys) => match keys.column_keys.get(column_name) {
+ None => Err(general_err!(
+ "No column decryption key set for column '{}'",
+ column_name
+ )),
+ Some(key) => Ok(Cow::Borrowed(key)),
+ },
+ DecryptionKeys::ViaRetriever(retriever) => {
+ let key = retriever.retrieve_key(key_metadata.unwrap_or_default())?;
+ Ok(Cow::Owned(key))
+ }
+ }
+ }
+}
+
+impl std::fmt::Debug for FileDecryptionProperties {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ write!(f, "FileDecryptionProperties {{ }}")
+ }
}
+/// Builder for [`FileDecryptionProperties`]
pub struct DecryptionPropertiesBuilder {
- footer_key: Vec<u8>,
+ footer_key: Option<Vec<u8>>,
+ key_retriever: Option<Arc<dyn KeyRetriever>>,
column_keys: HashMap<String, Vec<u8>>,
aad_prefix: Option<Vec<u8>>,
Review Comment:
As potentially follow on item, I think you could avoid the `unreachable!` code below by changing this to an enum. Something like
```rust
enum DecryptionPropertiesBuilder {
ExplicitKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
aad_prefix: Option<Vec<u8>>,
},
Retriever {
key_retriever: Arc<dyn KeyRetriever>
}
}
```
The compiler would force you to then explicitly spell out which APIs could be used in what mode.
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#discussion_r1994589193
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -136,125 +160,211 @@ impl CryptoContext {
}
}
-/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
-#[derive(Debug, Clone, PartialEq)]
-pub struct FileDecryptionProperties {
+#[derive(Clone, PartialEq)]
+struct ExplicitDecryptionKeys {
footer_key: Vec<u8>,
column_keys: HashMap<String, Vec<u8>>,
+}
+
+#[derive(Clone)]
+enum DecryptionKeys {
+ Explicit(ExplicitDecryptionKeys),
+ ViaRetriever(Arc<dyn KeyRetriever>),
+}
+
+/// FileDecryptionProperties hold keys and AAD data required to decrypt a Parquet file.
+#[derive(Clone)]
+pub struct FileDecryptionProperties {
+ keys: DecryptionKeys,
pub(crate) aad_prefix: Option<Vec<u8>>,
}
impl FileDecryptionProperties {
- /// Returns a new FileDecryptionProperties builder
+ /// Returns a new [`FileDecryptionProperties`] builder that will use the provided key to
+ /// decrypt footer metadata.
pub fn builder(footer_key: Vec<u8>) -> DecryptionPropertiesBuilder {
DecryptionPropertiesBuilder::new(footer_key)
}
+
+ /// Returns a new [`FileDecryptionProperties`] builder that uses a [`KeyRetriever`]
+ /// to get decryption keys based on key metadata.
+ pub fn with_key_retriever(key_retriever: Arc<dyn KeyRetriever>) -> DecryptionPropertiesBuilder {
+ DecryptionPropertiesBuilder::new_with_key_retriever(key_retriever)
+ }
}
+impl std::fmt::Debug for FileDecryptionProperties {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ write!(f, "FileDecryptionProperties {{ }}")
+ }
+}
+
+impl PartialEq for FileDecryptionProperties {
+ // FileDecryptionProperties needs to implement PartialEq to allow
+ // ParquetMetaData to implement PartialEq.
+ // We cannot compare a key retriever, but this isn't derived from the metadata.
Review Comment:
This is a bit ugly. I feel like the `FileDecryptor` doesn't really belong in `ParquetMetaData`, as it isn't part of the file metadata, but pulling it out might require passing it around in a lot more places with more use of `#[cfg(feature = "encryption")]`.
It looks like `ParquetMetaData` only needs to implement `PartialEq` for tests though, so maybe this is fine.
--
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] Allow retrieving Parquet decryption keys using the key metadata [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7286:
URL: https://github.com/apache/arrow-rs/pull/7286#issuecomment-2730554770
Thank you @adamreeve and @rok for the reviews
--
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