adamreeve commented on code in PR #9203:
URL: https://github.com/apache/arrow-rs/pull/9203#discussion_r2795654329
##########
parquet/src/encryption/ciphers.rs:
##########
@@ -139,15 +147,21 @@ pub(crate) struct RingGcmBlockEncryptor {
}
impl RingGcmBlockEncryptor {
- /// Create a new `RingGcmBlockEncryptor` with a given key and random nonce.
+ #[allow(dead_code)]
+ pub(crate) fn new(key_bytes: &[u8]) -> Result<Self> {
+ Self::new_with_algorithm(&AES_128_GCM, key_bytes)
+ }
+
+ /// Create a new `RingGcmBlockEncryptor` with an AEAD algorithm, a given
key and random nonce.
/// 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> {
+ pub(crate) fn new_with_algorithm(
+ algorithm: &'static Algorithm,
+ key_bytes: &[u8],
+ ) -> Result<Self> {
Review Comment:
As above, I think we should reuse the existing new method.
##########
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:
Yes I don't like this and I don't think it's necessary, see my comment on
`new_with_algorithm`. This also makes it possible for users to easily create
Parquet files that aren't compliant with the spec by using a non-standard
algorithm.
Changing the crypto library is something we might potentially want to do to
be able to add AES_GCM_CTR support for example
(https://github.com/apache/arrow-rs/issues/7258#issuecomment-2759562283).
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](https://github.com/apache/arrow/blob/e11aeeee4c032513a4ee895c22246bf5bb7eaf3f/cpp/src/parquet/encryption/encryption.h#L357).
##########
parquet/tests/encryption/encryption_async.rs:
##########
@@ -115,7 +115,7 @@ async fn test_misspecified_encryption_keys() {
// Too short footer key
check_for_error(
Review Comment:
Could you please add some higher level tests for round-tripping files with
larger key sizes? It might be beneficial to other Parquet implementations too
if you could add files that use larger keys to the
[parquet-testing](https://github.com/apache/parquet-testing/) repository.
##########
parquet/src/encryption/ciphers.rs:
##########
@@ -40,10 +40,18 @@ pub(crate) struct RingGcmBlockDecryptor {
}
impl RingGcmBlockDecryptor {
+ #[allow(dead_code)]
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)
+ }
+
+ /// Create a new `RingGcmBlockDecryptor` with an AEAD algorithm and a
given key.
+ pub(crate) fn new_with_algorithm(
+ algorithm: &'static Algorithm,
+ key_bytes: &[u8],
+ ) -> Result<Self> {
Review Comment:
I don't think there's any need to add this new constructor, we can reuse the
existing new method above and change the algorithm based on the length of the
key.
This is also a bit semantically confusing as you could create a new
"Ring**Gcm**BlockDecryptor" with a non-GCM algorithm.
--
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]
