-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[parquet] Allow more encryption algorithms #9203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
c017804
1d12eb4
e8da4b9
3e1b8a7
1f1f0da
92b4094
12c1888
3ec6b70
deec81e
35b482f
f55fe3d
d92014b
0fa4888
bd79c92
e95eb4d
e8999d3
928ec10
0abe16d
206c6c6
36fd995
efe04f6
d26f41b
1b3f2f7
5b8b244
96ad92d
f414b3e
4336eed
44caecc
12d768d
06c6b73
a91f2f9
ec912ab
28f3ab2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ use crate::errors::ParquetError; | |
| use crate::errors::ParquetError::General; | ||
| use crate::errors::Result; | ||
| use crate::file::metadata::HeapSize; | ||
| use ring::aead::{AES_128_GCM, Aad, LessSafeKey, NonceSequence, UnboundKey}; | ||
| use ring::aead::{AES_128_GCM, Aad, LessSafeKey, NonceSequence, UnboundKey, Algorithm}; | ||
| use ring::rand::{SecureRandom, SystemRandom}; | ||
| use std::fmt::Debug; | ||
|
|
||
|
|
@@ -41,9 +41,12 @@ pub(crate) struct RingGcmBlockDecryptor { | |
|
|
||
| impl RingGcmBlockDecryptor { | ||
| pub(crate) fn new(key_bytes: &[u8]) -> Result<Self> { | ||
| // todo support other key sizes | ||
| let key = UnboundKey::new(&AES_128_GCM, key_bytes) | ||
| .map_err(|_| General("Failed to create AES key".to_string()))?; | ||
| Self::new_with_algorithm(&AES_128_GCM, key_bytes) | ||
| } | ||
|
|
||
| pub(crate) fn new_with_algorithm(algorithm: &'static Algorithm, key_bytes: &[u8]) -> Result<Self> { | ||
| let key = UnboundKey::new(algorithm, key_bytes) | ||
| .map_err(|_| general_err!("Failed to create {:?} key", algorithm))?; | ||
|
|
||
| Ok(Self { | ||
| key: LessSafeKey::new(key), | ||
|
|
@@ -143,11 +146,13 @@ impl RingGcmBlockEncryptor { | |
| /// The nonce will advance appropriately with each block encryption and | ||
| /// return an error if it wraps around. | ||
| pub(crate) fn new(key_bytes: &[u8]) -> Result<Self> { | ||
| let rng = SystemRandom::new(); | ||
| Self::new_with_algorithm(&AES_128_GCM, key_bytes) | ||
| } | ||
|
|
||
| // todo support other key sizes | ||
| let key = UnboundKey::new(&AES_128_GCM, key_bytes) | ||
| .map_err(|e| general_err!("Error creating AES key: {}", e))?; | ||
| pub(crate) fn new_with_algorithm(algorithm: &'static Algorithm, key_bytes: &[u8]) -> Result<Self> { | ||
| let rng = SystemRandom::new(); | ||
| let key = UnboundKey::new(&algorithm, key_bytes) | ||
| .map_err(|e| general_err!("Error creating {:?} key: {}", algorithm, e))?; | ||
| let nonce = CounterNonce::new(&rng)?; | ||
|
|
||
| Ok(Self { | ||
|
|
@@ -188,6 +193,7 @@ impl BlockEncryptor for RingGcmBlockEncryptor { | |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use ring::aead::{AES_256_GCM, CHACHA20_POLY1305}; | ||
| use super::*; | ||
|
|
||
| #[test] | ||
|
|
@@ -204,4 +210,26 @@ mod tests { | |
|
|
||
| assert_eq!(plaintext, decrypted.as_slice()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_round_trip_with_algorithm() { | ||
| let key = [0u8; 32]; | ||
| let mut encryptor = RingGcmBlockEncryptor::new_with_algorithm(&AES_256_GCM, &key).unwrap(); | ||
| let decryptor = RingGcmBlockDecryptor::new_with_algorithm(&AES_256_GCM, &key).unwrap(); | ||
|
|
||
| let plaintext = b"hello, world!"; | ||
| let aad = b"some aad"; | ||
|
|
||
| let ciphertext = encryptor.encrypt(plaintext, aad).unwrap(); | ||
| let decrypted = decryptor.decrypt(&ciphertext, aad).unwrap(); | ||
|
|
||
| assert_eq!(plaintext, decrypted.as_slice()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_round_trip_with_incorrect_key_length() { | ||
| let key = [0u8; 16]; | ||
| assert!(RingGcmBlockEncryptor::new_with_algorithm(&CHACHA20_POLY1305, &key).is_err()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't clear to me that CHACHA is a valid Parquet encryption: https://github.com/apache/parquet-format/blob/master/Encryption.md : |
||
| assert!(RingGcmBlockDecryptor::new_with_algorithm(&CHACHA20_POLY1305, &key).is_err()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ use std::collections::HashMap; | |
| use std::fmt::Formatter; | ||
| use std::io::Read; | ||
| use std::sync::Arc; | ||
| use ring::aead::{Algorithm, AES_128_GCM}; | ||
|
|
||
| /// Trait for retrieving an encryption key using the key's metadata | ||
| /// | ||
|
|
@@ -352,6 +353,7 @@ pub struct FileDecryptionProperties { | |
| keys: DecryptionKeys, | ||
| aad_prefix: Option<Vec<u8>>, | ||
| footer_signature_verification: bool, | ||
| algorithm: &'static Algorithm | ||
|
hsiang-c marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| impl HeapSize for FileDecryptionProperties { | ||
|
|
@@ -448,6 +450,7 @@ pub struct DecryptionPropertiesBuilder { | |
| column_keys: HashMap<String, Vec<u8>>, | ||
| aad_prefix: Option<Vec<u8>>, | ||
| footer_signature_verification: bool, | ||
| algorithm: &'static Algorithm | ||
| } | ||
|
|
||
| impl DecryptionPropertiesBuilder { | ||
|
|
@@ -459,6 +462,7 @@ impl DecryptionPropertiesBuilder { | |
| column_keys: HashMap::default(), | ||
| aad_prefix: None, | ||
| footer_signature_verification: true, | ||
| algorithm: &AES_128_GCM | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -472,6 +476,7 @@ impl DecryptionPropertiesBuilder { | |
| keys, | ||
| aad_prefix: self.aad_prefix, | ||
| footer_signature_verification: self.footer_signature_verification, | ||
| algorithm: self.algorithm | ||
| })) | ||
| } | ||
|
|
||
|
|
@@ -505,6 +510,11 @@ impl DecryptionPropertiesBuilder { | |
| Ok(self) | ||
| } | ||
|
|
||
| pub fn with_algorithm(mut self, algorithm: &'static Algorithm) -> Self { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this effectively makes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I don't like this and I don't think it's necessary, see my comment on Changing the crypto library is something we might potentially want to do to be able to add AES_GCM_CTR support for example (#7258 (comment)). I think if we want to support new algorithms like AES_GCM_CTR in the future the user should provide an enum value from a supported set of algorithms like is done in the C++ implementation, but we can infer the key length from the provided key so this isn't necessary for this change. |
||
| self.algorithm = algorithm; | ||
| self | ||
| } | ||
|
|
||
| /// Disable verification of footer tags for files that use plaintext footers. | ||
| /// Signature verification is enabled by default. | ||
| pub fn disable_footer_signature_verification(mut self) -> Self { | ||
|
|
@@ -520,6 +530,7 @@ pub struct DecryptionPropertiesBuilderWithRetriever { | |
| key_retriever: Arc<dyn KeyRetriever>, | ||
| aad_prefix: Option<Vec<u8>>, | ||
| footer_signature_verification: bool, | ||
| algorithm: &'static Algorithm | ||
| } | ||
|
|
||
| impl DecryptionPropertiesBuilderWithRetriever { | ||
|
|
@@ -530,6 +541,7 @@ impl DecryptionPropertiesBuilderWithRetriever { | |
| key_retriever, | ||
| aad_prefix: None, | ||
| footer_signature_verification: true, | ||
| algorithm: &AES_128_GCM | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -540,6 +552,7 @@ impl DecryptionPropertiesBuilderWithRetriever { | |
| keys, | ||
| aad_prefix: self.aad_prefix, | ||
| footer_signature_verification: self.footer_signature_verification, | ||
| algorithm: self.algorithm | ||
| })) | ||
| } | ||
|
|
||
|
|
@@ -551,6 +564,11 @@ impl DecryptionPropertiesBuilderWithRetriever { | |
| self | ||
| } | ||
|
|
||
| pub fn with_algorithm(mut self, algorithm: &'static Algorithm) -> Self { | ||
| self.algorithm = algorithm; | ||
| self | ||
| } | ||
|
|
||
| /// Disable verification of footer tags for files that use plaintext footers. | ||
| /// Signature verification is enabled by default. | ||
| pub fn disable_footer_signature_verification(mut self) -> Self { | ||
|
|
@@ -596,7 +614,8 @@ impl FileDecryptor { | |
| ) -> Result<Self> { | ||
| let file_aad = [aad_prefix.as_slice(), aad_file_unique.as_slice()].concat(); | ||
| let footer_key = decryption_properties.footer_key(footer_key_metadata)?; | ||
| let footer_decryptor = RingGcmBlockDecryptor::new(&footer_key).map_err(|e| { | ||
| let algorithm = decryption_properties.algorithm; | ||
| let footer_decryptor = RingGcmBlockDecryptor::new_with_algorithm(algorithm, &footer_key).map_err(|e| { | ||
| general_err!( | ||
| "Invalid footer key. {}", | ||
| e.to_string().replace("Parquet error: ", "") | ||
|
|
@@ -641,7 +660,8 @@ impl FileDecryptor { | |
| let column_key = self | ||
| .decryption_properties | ||
| .column_key(column_name, key_metadata)?; | ||
| Ok(Arc::new(RingGcmBlockDecryptor::new(&column_key)?)) | ||
| let algorithm = self.decryption_properties.algorithm; | ||
| Ok(Arc::new(RingGcmBlockDecryptor::new_with_algorithm(algorithm, &column_key)?)) | ||
| } | ||
|
|
||
| pub(crate) fn get_column_metadata_decryptor( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2118,6 +2118,9 @@ mod tests { | |
| .set_file_decryptor(Some(decryptor)) | ||
| .build(); | ||
|
|
||
| #[cfg(feature = "encryption")] | ||
| let expected_size_with_decryptor = 3080; | ||
| #[cfg(not(feature = "encryption"))] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? The whole test is gated by #[cfg(feature = "encryption")]
fn test_memory_size_with_decryptor() {it seems like you just need to update the 3072 to 3080 to reflect the additional size? |
||
| let expected_size_with_decryptor = 3072; | ||
| assert!(expected_size_with_decryptor > base_expected_size); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.