alamb commented on code in PR #9203:
URL: https://github.com/apache/arrow-rs/pull/9203#discussion_r2795167114
##########
parquet/src/encryption/ciphers.rs:
##########
@@ -204,4 +219,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());
Review Comment:
It isn't clear to me that CHACHA is a valid Parquet encryption:
https://github.com/apache/parquet-format/blob/master/Encryption.md :
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -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"))]
Review Comment:
Why is this needed? The whole test is gated by
```rust
#[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?
##########
parquet/src/encryption/ciphers.rs:
##########
@@ -40,10 +40,18 @@ pub(crate) struct RingGcmBlockDecryptor {
}
impl RingGcmBlockDecryptor {
+ #[allow(dead_code)]
Review Comment:
why do we need to allow dead code? Maybe we should remove it instead?
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -505,6 +510,12 @@ impl DecryptionPropertiesBuilder {
Ok(self)
}
+ /// The AEAD decryption algorithm to be used.
+ pub fn with_algorithm(mut self, algorithm: &'static Algorithm) -> Self {
Review Comment:
this effectively makes `Algorithm` a part of the public API I think -- this
might be ok, but it also means we would not be able to change the underlying
crypto library without breaking the API 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
