fix: address 7 review issues — AEAD test, stream cross-test, child_level KDF, parallel decrypt, DataMap parsing, bounds check, test rename

- Add AEAD tamper detection integration test (Issue 1)
- Fix stream encrypt/decrypt cross-implementation test (Issue 2)
- Pass child_level through KDF for domain separation in shrink_data_map (Issue 3)
- Parallelize decrypt_sorted_set with rayon par_iter (Issue 4)
- Simplify DataMap::from_bytes to delegate to bincode (Issue 5)
- Add bounds check in get_n_1_n_2 for out-of-range chunk_index (Issue 6)
- Rename test_key_derivation_sizes to test_key_derivation_sizes_and_roundtrip (Issue 7)

Author: grumbach

src/data_map.rs

@@ -33,27 +33,10 @@ impl DataMap {
         bincode::serialize(self)
     }
 
-    /// Deserialize DataMap from bytes (v1 versioned format)
+    /// Deserialize DataMap from bytes (v1 versioned format).
+    /// Delegates to the `Deserialize` impl which handles version checking.
     pub fn from_bytes(bytes: &[u8]) -> Result<Self, bincode::Error> {
-        #[derive(Deserialize)]
-        struct VersionedDataMap {
-            version: u8,
-            chunk_identifiers: Vec<ChunkInfo>,
-            child: Option<usize>,
-        }
-
-        let versioned = bincode::deserialize::<VersionedDataMap>(bytes)?;
-        if versioned.version == 1 {
-            Ok(DataMap {
-                chunk_identifiers: versioned.chunk_identifiers,
-                child: versioned.child,
-            })
-        } else {
-            Err(Box::new(bincode::ErrorKind::Custom(format!(
-                "unsupported DataMap version: {ver}",
-                ver = versioned.version
-            ))))
-        }
+        bincode::deserialize(bytes)
     }
 }
 

src/decrypt.rs

@@ -8,6 +8,7 @@
 
 use crate::{cipher, utils::get_pad_key_and_nonce, utils::xor, EncryptedChunk, Error, Result};
 use bytes::Bytes;
+use rayon::prelude::*;
 use std::io::Cursor;
 use xor_name::XorName;
 
@@ -17,12 +18,19 @@ pub fn decrypt_sorted_set(
     encrypted_chunks: &[&EncryptedChunk],
     child_level: usize,
 ) -> Result<Bytes> {
-    let mut all_bytes = Vec::new();
-
-    // Process chunks sequentially to maintain proper boundaries
-    for (chunk_index, chunk) in encrypted_chunks.iter().enumerate() {
-        let decrypted = decrypt_chunk(chunk_index, &chunk.content, &src_hashes, child_level)?;
-        all_bytes.extend_from_slice(&decrypted);
+    // Decrypt chunks in parallel, then concatenate in order
+    let decrypted_chunks: Vec<Bytes> = encrypted_chunks
+        .par_iter()
+        .enumerate()
+        .map(|(chunk_index, chunk)| {
+            decrypt_chunk(chunk_index, &chunk.content, &src_hashes, child_level)
+        })
+        .collect::<Result<Vec<_>>>()?;
+
+    let total_len = decrypted_chunks.iter().map(|c| c.len()).sum();
+    let mut all_bytes = Vec::with_capacity(total_len);
+    for chunk in &decrypted_chunks {
+        all_bytes.extend_from_slice(chunk);
     }
 
     Ok(Bytes::from(all_bytes))

src/lib.rs

@@ -153,6 +153,14 @@ pub const COMPRESSION_QUALITY: i32 = 6;
 /// Encrypts a set of bytes and returns the encrypted data together with
 /// the data map that is derived from the input data.
 pub fn encrypt(bytes: Bytes) -> Result<(DataMap, Vec<EncryptedChunk>)> {
+    encrypt_with_child_level(bytes, 0)
+}
+
+/// Internal encryption that accepts a child_level for KDF domain separation.
+fn encrypt_with_child_level(
+    bytes: Bytes,
+    child_level: usize,
+) -> Result<(DataMap, Vec<EncryptedChunk>)> {
     let file_size = bytes.len();
     if file_size < MIN_ENCRYPTABLE_BYTES {
         return Err(Error::Generic(format!(
@@ -186,7 +194,7 @@ pub fn encrypt(bytes: Bytes) -> Result<(DataMap, Vec<EncryptedChunk>)> {
         }
 
         // For chunks 2 onwards, we can encrypt immediately since we have the previous two hashes
-        let pki = get_pad_key_and_nonce(chunk_index, &src_hashes, 0)?;
+        let pki = get_pad_key_and_nonce(chunk_index, &src_hashes, child_level)?;
         let encrypted_content = encrypt::encrypt_chunk(chunk_data, pki)?;
         let dst_hash = hash::content_hash(&encrypted_content);
 
@@ -204,7 +212,7 @@ pub fn encrypt(bytes: Bytes) -> Result<(DataMap, Vec<EncryptedChunk>)> {
 
     // Now process the first two chunks using the complete set of source hashes
     for (chunk_index, chunk_data, src_hash, src_size) in first_chunks {
-        let pki = get_pad_key_and_nonce(chunk_index, &src_hashes, 0)?;
+        let pki = get_pad_key_and_nonce(chunk_index, &src_hashes, child_level)?;
         let encrypted_content = encrypt::encrypt_chunk(chunk_data, pki)?;
         let dst_hash = hash::content_hash(&encrypted_content);
 
@@ -252,8 +260,7 @@ pub fn encrypt(bytes: Bytes) -> Result<(DataMap, Vec<EncryptedChunk>)> {
 /// * `Result<Bytes>` - The decrypted data or an error if chunks are missing/corrupted
 pub(crate) fn decrypt_full_set(data_map: &DataMap, chunks: &[EncryptedChunk]) -> Result<Bytes> {
     let src_hashes = extract_hashes(data_map);
-    // encrypt() always uses child_level=0 for key derivation, so decrypt must match
-    let child_level = 0;
+    let child_level = data_map.child().unwrap_or(0);
 
     // Create a mapping of chunk hashes to chunks for efficient lookup
     let chunk_map: HashMap<XorName, &EncryptedChunk> = chunks
@@ -327,8 +334,12 @@ pub(crate) fn decrypt_range(
     let mut all_bytes = Vec::new();
     for (idx, chunk) in sorted_chunks.iter().enumerate() {
         let chunk_idx = start_chunk + idx;
-        // encrypt() always uses child_level=0 for key derivation, so decrypt must match
-        let decrypted = decrypt_chunk(chunk_idx, &chunk.content, &src_hashes, 0)?;
+        let decrypted = decrypt_chunk(
+            chunk_idx,
+            &chunk.content,
+            &src_hashes,
+            data_map.child().unwrap_or(0),
+        )?;
         all_bytes.extend_from_slice(&decrypted);
     }
 
@@ -362,22 +373,23 @@ where
     let mut all_chunks = Vec::new();
 
     while data_map.len() > 3 {
-        let child_level = data_map.child().unwrap_or(0);
+        let next_child_level = data_map.child().map_or(1, |c| c + 1);
         let bytes = data_map
             .to_bytes()
             .map(Bytes::from)
             .map_err(|e| Error::Generic(format!("Failed to serialize data map: {e}")))?;
 
-        let (mut new_data_map, encrypted_chunks) = encrypt(bytes)?;
+        let (mut new_data_map, encrypted_chunks) =
+            encrypt_with_child_level(bytes, next_child_level)?;
 
         // Store and collect chunks
         for chunk in &encrypted_chunks {
             store_chunk(hash::content_hash(&chunk.content), chunk.content.clone())?;
         }
         all_chunks.extend(encrypted_chunks);
 
-        // Update data map for next iteration
-        new_data_map = DataMap::with_child(new_data_map.infos().to_vec(), child_level + 1);
+        // Tag the DataMap with the child_level used during encryption
+        new_data_map = DataMap::with_child(new_data_map.infos().to_vec(), next_child_level);
         data_map = new_data_map;
     }
     Ok((data_map, all_chunks))

src/utils.rs

@@ -51,6 +51,11 @@ pub(crate) fn get_n_1_n_2(
             "total_num_chunks must be at least 3, got {total_num_chunks}"
         )));
     }
+    if chunk_index >= total_num_chunks {
+        return Err(crate::Error::Generic(format!(
+            "chunk_index {chunk_index} out of bounds for total_num_chunks {total_num_chunks}"
+        )));
+    }
     match chunk_index {
         0 => Ok((total_num_chunks - 1, total_num_chunks - 2)),
         1 => Ok((0, total_num_chunks - 1)),
@@ -337,6 +342,16 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_get_n_1_n_2_out_of_bounds() {
+        let result = get_n_1_n_2(5, 3);
+        assert!(result.is_err(), "chunk_index >= total should fail");
+        let result = get_n_1_n_2(3, 3);
+        assert!(result.is_err(), "chunk_index == total should fail");
+        let result = get_n_1_n_2(2, 3);
+        assert!(result.is_ok(), "chunk_index < total should succeed");
+    }
+
     #[test]
     fn test_kdf_output_sizes() {
         let src = XorName([0x01; 32]);

tests/integration_tests.rs

@@ -967,26 +967,14 @@ fn test_stream_encrypt_file_decrypt_storage_cross() -> Result<()> {
 
 #[test]
 fn test_file_encrypt_stream_decrypt_cross() -> Result<()> {
+    // Use standard encrypt() then streaming decrypt — cross-compatibility test
     let data_size = 150_000;
     let original_data = random_bytes(data_size);
 
-    let mut stream = stream_encrypt(
-        data_size,
-        original_data.chunks(8192).map(|c| Bytes::from(c.to_vec())),
-    )?;
-
-    let mut storage = HashMap::new();
-    for chunk_result in stream.chunks() {
-        let (hash, content) = chunk_result?;
-        let _ = storage.insert(hash, content.to_vec());
-    }
-
-    let data_map = stream
-        .datamap()
-        .ok_or_else(|| Error::Generic("No DataMap".to_string()))?
-        .clone();
-
+    let (data_map, encrypted_chunks) = encrypt(original_data.clone())?;
+    let storage = build_chunk_storage(&encrypted_chunks);
     let fetcher = make_parallel_fetcher(&storage);
+
     let decrypt_stream = streaming_decrypt(&data_map, fetcher)?;
     let decrypted = decrypt_stream.range_full()?;
 
@@ -1297,6 +1285,40 @@ fn test_aead_tag_protects_integrity() -> Result<()> {
     Ok(())
 }
 
+// --- AEAD pipeline integrity test ---
+
+#[test]
+fn test_aead_rejects_tampered_ciphertext_through_pipeline() -> Result<()> {
+    use self_encryption::decrypt_chunk;
+
+    let original_data = random_bytes(10_000);
+    let (data_map, encrypted_chunks) = encrypt(original_data)?;
+
+    // Extract the src_hashes from the data map (the valid ones used during encryption)
+    let src_hashes: Vec<XorName> = data_map.infos().iter().map(|info| info.src_hash).collect();
+
+    // Take the first chunk's encrypted content and tamper with it
+    let first_chunk = encrypted_chunks
+        .first()
+        .ok_or_else(|| Error::Generic("no chunks".to_string()))?;
+    let mut tampered_content = first_chunk.content.to_vec();
+    let mid = tampered_content.len() / 2;
+    if let Some(byte) = tampered_content.get_mut(mid) {
+        *byte ^= 0xFF;
+    }
+    let tampered_bytes = Bytes::from(tampered_content);
+
+    // Call decrypt_chunk directly with valid src_hashes but tampered ciphertext
+    // This proves the AEAD layer (not hash lookup) is the integrity gate
+    let result = decrypt_chunk(0, &tampered_bytes, &src_hashes, 0);
+    assert!(
+        result.is_err(),
+        "decrypt_chunk should fail on tampered ciphertext due to AEAD tag verification"
+    );
+
+    Ok(())
+}
+
 // --- Task 17: Verify BLAKE3 is used for hashing ---
 
 #[test]
@@ -1327,19 +1349,10 @@ fn test_encrypted_chunk_dst_hash_is_blake3() -> Result<()> {
     Ok(())
 }
 
-// --- Task 18: Verify key derivation sizes ---
+// --- Task 18: Verify key derivation sizes and roundtrip ---
 
 #[test]
-fn test_key_derivation_sizes() -> Result<()> {
-    // Key derivation internals are tested indirectly through encrypt/decrypt roundtrips,
-    // but we can verify the constant sizes are correct for ChaCha20-Poly1305.
-    use self_encryption::hash::content_hash;
-
-    let h1 = content_hash(b"hash1");
-    let h2 = content_hash(b"hash2");
-    let h3 = content_hash(b"hash3");
-    let hashes = vec![h1, h2, h3];
-
+fn test_key_derivation_sizes_and_roundtrip() -> Result<()> {
     // XorName is 32 bytes
     assert_eq!(std::mem::size_of::<XorName>(), 32);
 
@@ -1356,10 +1369,5 @@ fn test_key_derivation_sizes() -> Result<()> {
         assert_ne!(info.dst_hash, XorName::default());
     }
 
-    // Verify hashes are 32 bytes (BLAKE3 output = XorName size)
-    for hash in &hashes {
-        assert_eq!(hash.0.len(), 32);
-    }
-
     Ok(())
 }