diff --git a/downstairs/src/dump.rs b/downstairs/src/dump.rs index 8392b8c0d..3d9a89d71 100644 --- a/downstairs/src/dump.rs +++ b/downstairs/src/dump.rs @@ -699,8 +699,10 @@ fn show_extent_block( for (dir_index, r) in dvec.iter().enumerate() { print!("{:^24} ", dir_index); - max_nonce_depth = - std::cmp::max(max_nonce_depth, r.encryption_contexts(0).len()); + max_nonce_depth = std::cmp::max( + max_nonce_depth, + r.encryption_contexts(0).is_some() as usize, + ); } if !only_show_differences { print!(" {:<5}", "DIFF"); @@ -722,17 +724,14 @@ fn show_extent_block( let mut all_same_len = true; let mut nonces = Vec::with_capacity(dir_count); for r in dvec.iter() { - let ctxs = r.encryption_contexts(0); + // TODO this can only be 0 or 1 + let ctxs = + r.encryption_contexts(0).into_iter().collect::>(); print!( "{:^24} ", if depth < ctxs.len() { - if let Some(ec) = ctxs[depth] { - nonces.push(&ec.nonce); - hex::encode(ec.nonce) - } else { - all_same_len = false; - "".to_string() - } + nonces.push(ctxs[depth].nonce); + hex::encode(ctxs[depth].nonce) } else { all_same_len = false; "".to_string() @@ -756,8 +755,10 @@ fn show_extent_block( for (dir_index, r) in dvec.iter().enumerate() { print!("{:^32} ", dir_index); - max_tag_depth = - std::cmp::max(max_tag_depth, r.encryption_contexts(0).len()); + max_tag_depth = std::cmp::max( + max_tag_depth, + r.encryption_contexts(0).is_some() as usize, + ); } if !only_show_differences { print!(" {:<5}", "DIFF"); @@ -779,17 +780,13 @@ fn show_extent_block( let mut all_same_len = true; let mut tags = Vec::with_capacity(dir_count); for r in dvec.iter() { - let ctxs = r.encryption_contexts(0); + let ctxs = + r.encryption_contexts(0).into_iter().collect::>(); print!( "{:^32} ", if depth < ctxs.len() { - if let Some(ec) = ctxs[depth] { - tags.push(&ec.tag); - hex::encode(ec.tag) - } else { - all_same_len = false; - "".to_string() - } + tags.push(ctxs[depth].tag); + hex::encode(ctxs[depth].tag) } else { all_same_len = false; "".to_string() @@ -820,7 +817,8 @@ fn show_extent_block( for (dir_index, r) in dvec.iter().enumerate() { print!("{:^16} ", dir_index); - max_hash_depth = std::cmp::max(max_hash_depth, r.hashes(0).len()); + max_hash_depth = + std::cmp::max(max_hash_depth, r.hashes(0).is_some() as usize); } if !only_show_differences { print!(" {:<5}", "DIFF"); @@ -842,7 +840,7 @@ fn show_extent_block( let mut all_same_len = true; let mut hashes = Vec::with_capacity(dir_count); for r in dvec.iter() { - let block_hashes = r.hashes(0); + let block_hashes = r.hashes(0).into_iter().collect::>(); print!( "{:^16} ", if depth < block_hashes.len() { diff --git a/downstairs/src/extent_inner_raw.rs b/downstairs/src/extent_inner_raw.rs index aca83da7c..a2bad6695 100644 --- a/downstairs/src/extent_inner_raw.rs +++ b/downstairs/src/extent_inner_raw.rs @@ -14,6 +14,7 @@ use crate::{ ExtentWrite, JobId, RegionDefinition, }; use crucible_common::ExtentId; +use crucible_protocol::ReadBlockContext; use itertools::Itertools; use serde::{Deserialize, Serialize}; @@ -322,7 +323,15 @@ impl ExtentInner for RawInner { let blocks = self.get_block_contexts_inner( req.offset.0, num_blocks, - |ctx, _block| ctx.map(|c| c.block_context), + |ctx, _block| match ctx { + None => ReadBlockContext::Empty, + Some(c) => match c.block_context.encryption_context { + Some(ctx) => ReadBlockContext::Encrypted { ctx }, + None => ReadBlockContext::Unencrypted { + hash: c.block_context.hash, + }, + }, + }, )?; cdt::extent__read__get__contexts__done!(|| { (job_id.0, self.extent_number.0, num_blocks) @@ -1604,6 +1613,7 @@ mod test { }], }; inner.write(JobId(10), &write, false, IOV_MAX_TEST)?; + let prev_hash = hash; // The context should be in place, though we haven't flushed yet @@ -1629,8 +1639,11 @@ mod test { }; let resp = inner.read(JobId(21), read, IOV_MAX_TEST)?; - // We should not get back our data, because block 0 was written. - assert_ne!(resp.blocks, vec![Some(block_context)]); + // We should get back our old data, because block 0 was written. + assert_eq!( + resp.blocks, + vec![ReadBlockContext::Unencrypted { hash: prev_hash }] + ); assert_ne!(resp.data, BytesMut::from(data.as_ref())); } @@ -1656,7 +1669,10 @@ mod test { let resp = inner.read(JobId(31), read, IOV_MAX_TEST)?; // We should get back our data! Block 1 was never written. - assert_eq!(resp.blocks, vec![Some(block_context)]); + assert_eq!( + resp.blocks, + vec![ReadBlockContext::Unencrypted { hash }] + ); assert_eq!(resp.data, BytesMut::from(data.as_ref())); } @@ -1883,7 +1899,10 @@ mod test { let resp = inner.read(JobId(31), read, IOV_MAX_TEST)?; // We should get back our data! Block 1 was never written. - assert_eq!(resp.blocks, vec![Some(block_context)]); + assert_eq!( + resp.blocks, + vec![ReadBlockContext::Unencrypted { hash }] + ); assert_eq!(resp.data, BytesMut::from(data.as_ref())); } diff --git a/downstairs/src/extent_inner_sqlite.rs b/downstairs/src/extent_inner_sqlite.rs index b112bda03..5c19ec80e 100644 --- a/downstairs/src/extent_inner_sqlite.rs +++ b/downstairs/src/extent_inner_sqlite.rs @@ -9,7 +9,7 @@ use crate::{ ExtentWrite, JobId, RegionDefinition, }; use crucible_common::{crucible_bail, ExtentId}; -use crucible_protocol::EncryptionContext; +use crucible_protocol::{EncryptionContext, ReadBlockContext}; use anyhow::{bail, Result}; use itertools::Itertools; @@ -301,10 +301,18 @@ impl SqliteMoreInner { cdt::extent__read__get__contexts__done!(|| { (job_id.0, self.extent_number.0, num_blocks) }); - // Convert from DownstairsBlockContext -> BlockContext + // Convert from DownstairsBlockContext -> ReadBlockContext let blocks = block_contexts .into_iter() - .map(|bs| bs.map(|b| b.block_context)) + .map(|bs| match bs { + None => ReadBlockContext::Empty, + Some(b) => match b.block_context.encryption_context { + Some(ctx) => ReadBlockContext::Encrypted { ctx }, + None => ReadBlockContext::Unencrypted { + hash: b.block_context.hash, + }, + }, + }) .collect(); // To avoid a `memset`, we're reading directly into uninitialized @@ -1542,6 +1550,7 @@ mod test { // We haven't flushed, but this should leave our context in place. inner.fully_rehash_and_clean_all_stale_contexts(false)?; + let prev_hash = hash; // Therefore, we expect that write_unwritten to the first block won't // do anything. @@ -1565,8 +1574,11 @@ mod test { }; let resp = inner.read(JobId(21), read, IOV_MAX_TEST)?; - // We should not get back our data, because block 0 was written. - assert_ne!(resp.blocks, vec![Some(block_context)]); + // We should get back our old data, because block 0 was written. + assert_eq!( + resp.blocks, + vec![ReadBlockContext::Unencrypted { hash: prev_hash }] + ); assert_ne!(resp.data, BytesMut::from(data.as_ref())); } @@ -1592,7 +1604,10 @@ mod test { let resp = inner.read(JobId(31), read, IOV_MAX_TEST)?; // We should get back our data! Block 1 was never written. - assert_eq!(resp.blocks, vec![Some(block_context)]); + assert_eq!( + resp.blocks, + vec![ReadBlockContext::Unencrypted { hash }] + ); assert_eq!(resp.data, BytesMut::from(data.as_ref())); } @@ -1653,7 +1668,10 @@ mod test { let resp = inner.read(JobId(31), read, IOV_MAX_TEST)?; // We should get back our data! Block 1 was never written. - assert_eq!(resp.blocks, vec![Some(block_context)]); + assert_eq!( + resp.blocks, + vec![ReadBlockContext::Unencrypted { hash }] + ); assert_eq!(resp.data, BytesMut::from(data.as_ref())); } @@ -1694,8 +1712,10 @@ mod test { data: BytesMut::with_capacity(512 * 2), }; let resp = inner.read(JobId(31), read(), IOV_MAX_TEST)?; - let expected_blocks: Vec<_> = - block_contexts.iter().map(|b| Some(*b)).collect(); + let expected_blocks: Vec<_> = block_contexts + .iter() + .map(|b| ReadBlockContext::Unencrypted { hash: b.hash }) + .collect(); assert_eq!(resp.blocks, expected_blocks); assert_eq!(resp.data, BytesMut::from(data.as_ref())); diff --git a/downstairs/src/lib.rs b/downstairs/src/lib.rs index 020cbfb4e..686c7595d 100644 --- a/downstairs/src/lib.rs +++ b/downstairs/src/lib.rs @@ -20,7 +20,8 @@ use crucible_common::{ }; use crucible_protocol::{ BlockContext, CrucibleDecoder, JobId, Message, MessageWriter, - ReconciliationId, SnapshotDetails, CRUCIBLE_MESSAGE_VERSION, + ReadBlockContext, ReconciliationId, SnapshotDetails, + CRUCIBLE_MESSAGE_VERSION, }; use repair_client::Client; @@ -204,7 +205,7 @@ impl IntoIterator for RegionReadRequest { /// Do not derive `Clone` on this type; it will be expensive and tempting to /// call by accident! pub(crate) struct RegionReadResponse { - blocks: Vec>, + blocks: Vec, data: BytesMut, uninit: BytesMut, } @@ -281,24 +282,27 @@ impl RegionReadResponse { assert!(was_empty || prev_ptr == self.data.as_ptr()); } - /// Returns hashes for the given response - /// - /// This is expensive and should only be used for debugging - pub fn hashes(&self, i: usize) -> Vec { - self.blocks[i].iter().map(|ctx| ctx.hash).collect() + /// Returns the hash for the given response (if unencrypted) + pub fn hashes(&self, i: usize) -> Option { + match self.blocks[i] { + ReadBlockContext::Empty | ReadBlockContext::Encrypted { .. } => { + None + } + ReadBlockContext::Unencrypted { hash } => Some(hash), + } } /// Returns encryption contexts for the given response - /// - /// This is expensive and should only be used for debugging pub fn encryption_contexts( &self, i: usize, - ) -> Vec> { - self.blocks[i] - .iter() - .map(|ctx| ctx.encryption_context.as_ref()) - .collect() + ) -> Option { + match self.blocks[i] { + ReadBlockContext::Empty | ReadBlockContext::Unencrypted { .. } => { + None + } + ReadBlockContext::Encrypted { ctx } => Some(ctx), + } } } @@ -307,7 +311,7 @@ impl RegionReadResponse { /// Do not derive `Clone` on this type; it will be expensive and tempting to /// call by accident! pub(crate) struct ExtentReadResponse { - blocks: Vec>, + blocks: Vec, /// At this point, the buffer must be fully initialized data: BytesMut, } @@ -5444,9 +5448,11 @@ mod test { assert_eq!(response.blocks.len(), 1); - let hashes = response.hashes(0); - assert_eq!(hashes.len(), 1); - assert_eq!(integrity_hash(&[&response.data[..]]), hashes[0],); + let hash = response.hashes(0); + assert_eq!( + integrity_hash(&[&response.data[..]]), + hash.unwrap() + ); read_data.extend_from_slice(&response.data[..]); } diff --git a/downstairs/src/region.rs b/downstairs/src/region.rs index eb120f7a1..5c06a8e1f 100644 --- a/downstairs/src/region.rs +++ b/downstairs/src/region.rs @@ -2384,7 +2384,8 @@ pub(crate) mod test { .unwrap(); assert_eq!(responses.blocks.len(), 1); - assert_eq!(responses.hashes(0).len(), 1); + assert!(responses.hashes(0).is_none()); + assert!(responses.encryption_contexts(0).is_some()); assert_eq!(responses.data[..], [9u8; 512][..]); } @@ -2429,19 +2430,15 @@ pub(crate) mod test { // Same block, now try to write something else to it. let data = Bytes::from(vec![1u8; 512]); + let ctx = crucible_protocol::EncryptionContext { + nonce: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12], + tag: [4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19], + }; let write = ExtentWrite { offset, data, block_contexts: vec![BlockContext { - encryption_context: Some( - crucible_protocol::EncryptionContext { - nonce: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12], - tag: [ - 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, - 18, 19, - ], - }, - ), + encryption_context: Some(ctx), hash: 9163319254371683066, // hash for all 1s }], }; @@ -2468,8 +2465,9 @@ pub(crate) mod test { // We should still have one response. assert_eq!(responses.blocks.len(), 1); - // Hash should be just 1 - assert_eq!(responses.hashes(0).len(), 1); + // We don't return the hash, because we've got encryption + assert!(responses.hashes(0).is_none()); + assert_eq!(responses.encryption_contexts(0), Some(ctx)); // Data should match first write assert_eq!(responses.data[..], [9u8; 512][..]); } @@ -2525,19 +2523,15 @@ pub(crate) mod test { // Create a new write IO with different data. let data = Bytes::from(vec![1u8; 512]); + let ctx = crucible_protocol::EncryptionContext { + nonce: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12], + tag: [4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19], + }; let write = ExtentWrite { offset, data, block_contexts: vec![BlockContext { - encryption_context: Some( - crucible_protocol::EncryptionContext { - nonce: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12], - tag: [ - 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, - 18, 19, - ], - }, - ), + encryption_context: Some(ctx), hash: 9163319254371683066, // hash for all 1s }], }; @@ -2568,8 +2562,9 @@ pub(crate) mod test { // We should still have one response. assert_eq!(responses.blocks.len(), 1); - // Hash should be just 1 - assert_eq!(responses.hashes(0).len(), 1); + // We don't return the hash, because we have encryption + assert!(responses.hashes(0).is_none()); + assert_eq!(responses.encryption_contexts(0), Some(ctx)); // Data should match first write assert_eq!(responses.data[..], [9u8; 512][..]); } @@ -3244,12 +3239,18 @@ pub(crate) mod test { let resp = ext.read(JobId(i as u64), req).unwrap(); // Now that we've checked that, flatten out for an easier eq - let actual_ctxts: Vec<_> = - resp.blocks.iter().map(|b| b.unwrap()).collect(); + let actual_ctxts = resp.blocks.clone(); // What we expect is the hashes for the last write we did - let expected_ctxts: Vec<_> = - last_writes.0[i].write.block_contexts.clone(); + let expected_ctxts: Vec<_> = last_writes.0[i] + .write + .block_contexts + .iter() + .map(|b| match b.encryption_context { + Some(ctx) => ReadBlockContext::Encrypted { ctx }, + None => ReadBlockContext::Unencrypted { hash: b.hash }, + }) + .collect(); // Check that they're right. assert_eq!(expected_ctxts, actual_ctxts); @@ -3318,12 +3319,21 @@ pub(crate) mod test { let out = ext.read(JobId(0), req).unwrap(); // Now that we've checked that, flatten out for an easier eq - let actual_ctxts: Vec<_> = - out.blocks.iter().map(|b| b.unwrap()).collect(); + let actual_ctxts = out.blocks.clone(); + + // What we expect is the hashes for the last write we did + let expected_ctxts: Vec<_> = last_write + .block_contexts + .iter() + .map(|b| match b.encryption_context { + Some(ctx) => ReadBlockContext::Encrypted { ctx }, + None => ReadBlockContext::Unencrypted { hash: b.hash }, + }) + .collect(); // What we expect is the hashes for the last write we did // Check that they're right. - assert_eq!(last_write.block_contexts, actual_ctxts); + assert_eq!(expected_ctxts, actual_ctxts); } fn test_bad_hash_bad(backend: Backend) { @@ -3390,7 +3400,8 @@ pub(crate) mod test { .unwrap(); assert_eq!(responses.blocks.len(), 1); - assert_eq!(responses.hashes(0).len(), 0); + assert!(responses.hashes(0).is_none()); + assert!(responses.encryption_contexts(0).is_none()); assert_eq!(responses.data[..], [0u8; 512][..]); } diff --git a/protocol/src/lib.rs b/protocol/src/lib.rs index a50a90938..d493a6318 100644 --- a/protocol/src/lib.rs +++ b/protocol/src/lib.rs @@ -162,6 +162,9 @@ pub struct SnapshotDetails { #[repr(u32)] #[derive(IntoPrimitive)] pub enum MessageVersion { + /// Use `ReadBlockContext` instead of `Option` + V11 = 11, + /// Reorganize messages to be start + length, instead of random-access V10 = 10, @@ -204,7 +207,7 @@ pub enum MessageVersion { } impl MessageVersion { pub const fn current() -> Self { - Self::V10 + Self::V11 } } @@ -213,7 +216,7 @@ impl MessageVersion { * This, along with the MessageVersion enum above should be updated whenever * changes are made to the Message enum below. */ -pub const CRUCIBLE_MESSAGE_VERSION: u32 = 10; +pub const CRUCIBLE_MESSAGE_VERSION: u32 = 11; /* * If you add or change the Message enum, you must also increment the @@ -570,12 +573,19 @@ pub struct WriteHeader { pub contexts: Vec, } +#[derive(Debug, PartialEq, Copy, Clone, Serialize, Deserialize)] +pub enum ReadBlockContext { + Empty, + Encrypted { ctx: EncryptionContext }, + Unencrypted { hash: u64 }, +} + #[derive(Debug, PartialEq, Clone, Serialize, Deserialize)] pub struct ReadResponseHeader { pub upstairs_id: Uuid, pub session_id: Uuid, pub job_id: JobId, - pub blocks: Result>, CrucibleError>, + pub blocks: Result, CrucibleError>, } impl Message { @@ -1306,4 +1316,28 @@ mod tests { ] ); } + + #[test] + fn read_block_context_size() { + let m = ReadBlockContext::Encrypted { + ctx: EncryptionContext { + nonce: [1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2], + tag: [ + 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, + 25, + ], + }, + }; + let encoded = bincode::serialize(&m).unwrap(); + assert_eq!(encoded.len(), 32); + assert_eq!( + &encoded, + &[ + 1, 0, 0, 0, // enum tag + 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, // nonce + 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, + 25, // tag + ] + ); + } } diff --git a/upstairs/src/buffer.rs b/upstairs/src/buffer.rs index d8d0da003..f501f06b8 100644 --- a/upstairs/src/buffer.rs +++ b/upstairs/src/buffer.rs @@ -1,6 +1,7 @@ // Copyright 2023 Oxide Computer Company use crate::RawReadResponse; use bytes::{Bytes, BytesMut}; +use crucible_protocol::ReadBlockContext; use itertools::Itertools; /* @@ -150,7 +151,7 @@ impl Buffer { .blocks .iter() .enumerate() - .group_by(|(_i, b)| b.is_none()) + .group_by(|(_i, b)| matches!(b, ReadBlockContext::Empty)) { if empty { continue; @@ -374,7 +375,6 @@ impl UninitializedBuffer { #[cfg(test)] mod test { use super::*; - use crate::BlockContext; use rand::RngCore; #[test] @@ -496,12 +496,9 @@ mod test { let blocks = (0..10) .map(|i| { if f(i) { - Some(BlockContext { - hash: 123, - encryption_context: None, - }) + ReadBlockContext::Unencrypted { hash: 123 } } else { - None + ReadBlockContext::Empty } }) .collect(); @@ -568,12 +565,7 @@ mod test { rng.fill_bytes(&mut data); let blocks = (0..10) - .map(|_| { - Some(BlockContext { - hash: 123, - encryption_context: None, - }) - }) + .map(|_| ReadBlockContext::Unencrypted { hash: 123 }) .collect(); let prev_data_ptr = data.as_ptr(); diff --git a/upstairs/src/client.rs b/upstairs/src/client.rs index 595c40df3..ea8ae191c 100644 --- a/upstairs/src/client.rs +++ b/upstairs/src/client.rs @@ -10,7 +10,7 @@ use crucible_common::{ deadline_secs, verbose_timeout, x509::TLSContext, ExtentId, }; use crucible_protocol::{ - BlockContext, MessageWriter, ReconciliationId, CRUCIBLE_MESSAGE_VERSION, + MessageWriter, ReconciliationId, CRUCIBLE_MESSAGE_VERSION, }; use std::{ @@ -2923,7 +2923,7 @@ fn update_net_done_probes(m: &Message, cid: ClientId) { /// The return value of this will be stored with the job, and compared /// between each read. pub(crate) fn validate_encrypted_read_response( - block_context: Option, + block_context: Option, data: &mut [u8], encryption_context: &EncryptionContext, log: &Logger, @@ -2937,7 +2937,7 @@ pub(crate) fn validate_encrypted_read_response( // check that this read response contains block contexts that contain // a matching encryption context. - let Some(context) = block_context else { + let Some(ctx) = block_context else { // No block context(s) in the response! // // Either this is a read of an unwritten block, or an attacker @@ -2956,49 +2956,25 @@ pub(crate) fn validate_encrypted_read_response( } }; - let Some(block_encryption_ctx) = &context.encryption_context else { - // this block context is missing an encryption context! - return Err(CrucibleError::DecryptionError); - }; - - // We'll start with decryption, ignoring the hash; we're using authenticated - // encryption, so the check is just as strong as the hash check. + // We're using authenticated encryption, so if it decrypts correctly, we can + // be confident that it wasn't corrupted. Corruption either on-disk + // (unlikely due to ZFS) or in-transit (unlikely-ish due to TCP checksums) + // will both result in decryption failure; we can't tell these cases apart. // - // Note: decrypt_in_place does not overwrite the buffer if - // it fails, otherwise we would need to copy here. There's a - // unit test to validate this behaviour. + // Note: decrypt_in_place does not overwrite the buffer if it fails, + // otherwise we would need to copy here. There's a unit test to validate + // this behaviour. use aes_gcm_siv::{Nonce, Tag}; let decryption_result = encryption_context.decrypt_in_place( data, - Nonce::from_slice(&block_encryption_ctx.nonce[..]), - Tag::from_slice(&block_encryption_ctx.tag[..]), + Nonce::from_slice(&ctx.nonce[..]), + Tag::from_slice(&ctx.tag[..]), ); if decryption_result.is_ok() { - Ok(Validation::Encrypted(*block_encryption_ctx)) + Ok(Validation::Encrypted(ctx)) } else { - // Validate integrity hash before decryption - let computed_hash = integrity_hash(&[ - &block_encryption_ctx.nonce[..], - &block_encryption_ctx.tag[..], - data, - ]); - - if computed_hash == context.hash { - // No encryption context combination decrypted this block, but - // one valid hash was found. This can occur if the decryption - // key doesn't match the key that the data was encrypted with. - error!(log, "Decryption failed with correct hash"); - Err(CrucibleError::DecryptionError) - } else { - error!( - log, - "No match for integrity hash\n\ - Expected: 0x{:x} != Computed: 0x{:x}", - context.hash, - computed_hash - ); - Err(CrucibleError::HashMismatch) - } + error!(log, "Decryption failed!"); + Err(CrucibleError::DecryptionError) } } @@ -3008,20 +2984,20 @@ pub(crate) fn validate_encrypted_read_response( /// block is all 0 /// - Err otherwise pub(crate) fn validate_unencrypted_read_response( - block_context: Option, + block_hash: Option, data: &mut [u8], log: &Logger, ) -> Result { - if let Some(context) = block_context { + if let Some(hash) = block_hash { // check integrity hashes - make sure it is correct let computed_hash = integrity_hash(&[data]); - if computed_hash == context.hash { + if computed_hash == hash { Ok(Validation::Unencrypted(computed_hash)) } else { // No integrity hash was correct for this response error!(log, "No match computed hash:0x{:x}", computed_hash,); - error!(log, "No match hash:0x{:x}", context.hash); + error!(log, "No match hash:0x{:x}", hash); error!(log, "Data from hash:"); for (i, d) in data[..6].iter().enumerate() { error!(log, "[{i}]:{d}"); diff --git a/upstairs/src/deferred.rs b/upstairs/src/deferred.rs index 680a8f85d..7ab7ac71d 100644 --- a/upstairs/src/deferred.rs +++ b/upstairs/src/deferred.rs @@ -7,7 +7,8 @@ use crate::{ BlockRes, ClientId, ImpactedBlocks, Message, RawWrite, Validation, }; use bytes::BytesMut; -use crucible_common::{integrity_hash, RegionDefinition}; +use crucible_common::{integrity_hash, CrucibleError, RegionDefinition}; +use crucible_protocol::ReadBlockContext; use futures::{ future::{ready, Either, Ready}, stream::FuturesOrdered, @@ -233,18 +234,48 @@ impl DeferredRead { let block_size = data.len() / rs.len(); for (i, r) in rs.iter_mut().enumerate() { let v = if let Some(ctx) = &self.cfg.encryption_context { - validate_encrypted_read_response( - *r, - &mut data[i * block_size..][..block_size], - ctx, - &self.log, - ) + match r { + ReadBlockContext::Empty => Ok(None), + ReadBlockContext::Encrypted { ctx } => Ok(Some(*ctx)), + ReadBlockContext::Unencrypted { .. } => { + error!( + self.log, + "expected encrypted but got unencrypted \ + block context" + ); + Err(CrucibleError::MissingBlockContext) + } + } + .and_then(|r| { + validate_encrypted_read_response( + r, + &mut data[i * block_size..][..block_size], + ctx, + &self.log, + ) + }) } else { - validate_unencrypted_read_response( - *r, - &mut data[i * block_size..][..block_size], - &self.log, - ) + match r { + ReadBlockContext::Empty => Ok(None), + ReadBlockContext::Unencrypted { hash } => { + Ok(Some(*hash)) + } + ReadBlockContext::Encrypted { .. } => { + error!( + self.log, + "expected unencrypted but got encrypted \ + block context" + ); + Err(CrucibleError::MissingBlockContext) + } + } + .and_then(|r| { + validate_unencrypted_read_response( + r, + &mut data[i * block_size..][..block_size], + &self.log, + ) + }) }; match v { Ok(hash) => hashes.push(hash), diff --git a/upstairs/src/downstairs.rs b/upstairs/src/downstairs.rs index 47532d37e..c26f7c197 100644 --- a/upstairs/src/downstairs.rs +++ b/upstairs/src/downstairs.rs @@ -4514,7 +4514,7 @@ pub(crate) mod test { use bytes::BytesMut; use crucible_common::{BlockOffset, ExtentId}; - use crucible_protocol::{BlockContext, Message}; + use crucible_protocol::{Message, ReadBlockContext}; use ringbuffer::RingBuffer; use std::{ @@ -4528,10 +4528,9 @@ pub(crate) mod test { pub fn build_read_response(data: &[u8]) -> RawReadResponse { RawReadResponse { data: BytesMut::from(data), - blocks: vec![Some(BlockContext { + blocks: vec![ReadBlockContext::Unencrypted { hash: crucible_common::integrity_hash(&[data]), - encryption_context: None, - })], + }], } } diff --git a/upstairs/src/dummy_downstairs_tests.rs b/upstairs/src/dummy_downstairs_tests.rs index 1ce6791f3..8e4a0954c 100644 --- a/upstairs/src/dummy_downstairs_tests.rs +++ b/upstairs/src/dummy_downstairs_tests.rs @@ -10,7 +10,6 @@ use std::time::Duration; use crate::client::CLIENT_TIMEOUT_SECS; use crate::guest::Guest; use crate::up_main; -use crate::BlockContext; use crate::BlockIO; use crate::Buffer; use crate::CrucibleError; @@ -27,6 +26,7 @@ use crucible_protocol::CrucibleDecoder; use crucible_protocol::CrucibleEncoder; use crucible_protocol::JobId; use crucible_protocol::Message; +use crucible_protocol::ReadBlockContext; use crucible_protocol::ReadResponseHeader; use crucible_protocol::WriteHeader; @@ -609,15 +609,12 @@ impl TestHarness { } } -fn make_blank_read_response() -> (Option, BytesMut) { +fn make_blank_read_response() -> (ReadBlockContext, BytesMut) { let data = vec![0u8; 512]; let hash = crucible_common::integrity_hash(&[&data]); ( - Some(BlockContext { - hash, - encryption_context: None, - }), + ReadBlockContext::Unencrypted { hash }, BytesMut::from(&data[..]), ) } diff --git a/upstairs/src/lib.rs b/upstairs/src/lib.rs index 1b958b02b..f9702f526 100644 --- a/upstairs/src/lib.rs +++ b/upstairs/src/lib.rs @@ -668,7 +668,7 @@ impl RegionDefinitionStatus { #[derive(Debug, Default)] pub(crate) struct RawReadResponse { /// Per-block metadata - pub blocks: Vec>, + pub blocks: Vec, /// Raw data pub data: bytes::BytesMut, } diff --git a/upstairs/src/test.rs b/upstairs/src/test.rs index b6f6b0d58..2faa9cf73 100644 --- a/upstairs/src/test.rs +++ b/upstairs/src/test.rs @@ -377,29 +377,21 @@ pub(crate) mod up_test { assert_ne!(original_data, data); - let read_response_hash = integrity_hash(&[&nonce, &tag, &data[..]]); - - // Create the read response - let block_context = BlockContext { - hash: read_response_hash, - encryption_context: Some(crucible_protocol::EncryptionContext { - nonce: nonce.into(), - tag: tag.into(), - }), + // Create the read response context + let ctx = crucible_protocol::EncryptionContext { + nonce: nonce.into(), + tag: tag.into(), }; // Validate it let successful_hash = validate_encrypted_read_response( - Some(block_context), + Some(ctx), &mut data, &Arc::new(context), &csl(), )?; - assert_eq!( - successful_hash, - Validation::Encrypted(block_context.encryption_context.unwrap()) - ); + assert_eq!(successful_hash, Validation::Encrypted(ctx)); // `validate_encrypted_read_response` will mutate the read // response's data value, make sure it decrypted @@ -463,15 +455,9 @@ pub(crate) mod up_test { let read_response_hash = integrity_hash(&[&data[..]]); let original_data = data.clone(); - // Create the read response - let block_context = BlockContext { - hash: read_response_hash, - encryption_context: None, - }; - // Validate it let successful_hash = validate_unencrypted_read_response( - Some(block_context), + Some(read_response_hash), &mut data, &csl(), )?; diff --git a/upstairs/src/upstairs.rs b/upstairs/src/upstairs.rs index 8df80d21a..f3c3a6f35 100644 --- a/upstairs/src/upstairs.rs +++ b/upstairs/src/upstairs.rs @@ -2094,11 +2094,11 @@ pub(crate) mod test { client::ClientStopReason, downstairs::test::set_all_active, test::{make_encrypted_upstairs, make_upstairs}, - Block, BlockContext, BlockOp, BlockOpWaiter, DsState, JobId, + Block, BlockOp, BlockOpWaiter, DsState, JobId, }; use bytes::BytesMut; use crucible_common::integrity_hash; - use crucible_protocol::ReadResponseHeader; + use crucible_protocol::{ReadBlockContext, ReadResponseHeader}; use futures::FutureExt; // Test function to create just enough of an Upstairs for our needs. @@ -3650,20 +3650,19 @@ pub(crate) mod test { // fake read response from downstairs that will successfully decrypt let mut data = Vec::from([1u8; 512]); - let (nonce, tag, hash) = up + let (nonce, tag, _hash) = up .cfg .encryption_context .as_ref() .unwrap() .encrypt_in_place(&mut data); - let blocks = Ok(vec![Some(BlockContext { - encryption_context: Some(crucible_protocol::EncryptionContext { + let blocks = Ok(vec![ReadBlockContext::Encrypted { + ctx: crucible_protocol::EncryptionContext { nonce: nonce.into(), tag: tag.into(), - }), - hash, - })]); + }, + }]); let data = BytesMut::from(&data[..]); // Because this read is small, it happens right away @@ -3699,7 +3698,7 @@ pub(crate) mod test { let mut data = Vec::from([1u8; 512]); - let (nonce, tag, hash) = up + let (nonce, tag, _hash) = up .cfg .encryption_context .as_ref() @@ -3714,12 +3713,9 @@ pub(crate) mod test { let mut responses = vec![]; let mut buf = BytesMut::new(); for _ in 0..blocks { - responses.push(Some(BlockContext { - encryption_context: Some( - crucible_protocol::EncryptionContext { nonce, tag }, - ), - hash, - })); + responses.push(ReadBlockContext::Encrypted { + ctx: crucible_protocol::EncryptionContext { nonce, tag }, + }); buf.extend(&data); } @@ -3779,21 +3775,14 @@ pub(crate) mod test { tag[3] = 0xFF; } - // compute integrity hash after alteration above! It should still - // validate - let hash = integrity_hash(&[&nonce, &tag, &data]); - // Build up the long read response, which should be long enough to // trigger the deferred read path. let mut responses = vec![]; let mut buf = BytesMut::new(); for _ in 0..blocks { - responses.push(Some(BlockContext { - encryption_context: Some( - crucible_protocol::EncryptionContext { nonce, tag }, - ), - hash, - })); + responses.push(ReadBlockContext::Encrypted { + ctx: crucible_protocol::EncryptionContext { nonce, tag }, + }); buf.extend(&data[..]); } @@ -3863,17 +3852,9 @@ pub(crate) mod test { tag[3] = 0xFF; } - // compute integrity hash after alteration above! It should still - // validate - let hash = integrity_hash(&[&nonce, &tag, &data]); - - let responses = Ok(vec![Some(BlockContext { - encryption_context: Some(crucible_protocol::EncryptionContext { - nonce, - tag, - }), - hash, - })]); + let responses = Ok(vec![ReadBlockContext::Encrypted { + ctx: crucible_protocol::EncryptionContext { nonce, tag }, + }]); // Prepare to receive the message with an invalid tag let thread = std::thread::spawn(move || { @@ -3917,10 +3898,9 @@ pub(crate) mod test { // fake read response from downstairs that will fail integrity hash // check - let responses = Ok(vec![Some(BlockContext { - encryption_context: None, + let responses = Ok(vec![ReadBlockContext::Unencrypted { hash: 10000, // junk hash - })]); + }]); // Prepare to handle the response with a junk hash let thread = std::thread::spawn(move || { @@ -3964,10 +3944,7 @@ pub(crate) mod test { let data = BytesMut::from([1u8; 512].as_slice()); let hash = integrity_hash(&[&data]); - let r1 = Ok(vec![Some(BlockContext { - encryption_context: None, - hash, - })]); + let r1 = Ok(vec![ReadBlockContext::Unencrypted { hash }]); up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(1), action: ClientAction::Response(Message::ReadResponse { @@ -3988,10 +3965,7 @@ pub(crate) mod test { // between multiple ReadResponse let data = BytesMut::from([2u8; 512].as_slice()); let hash = integrity_hash(&[&data]); - let r2 = Ok(vec![Some(BlockContext { - encryption_context: None, - hash, - })]); + let r2 = Ok(vec![ReadBlockContext::Unencrypted { hash }]); let thread = std::thread::spawn(move || { up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(2), @@ -4033,10 +4007,7 @@ pub(crate) mod test { for client_id in [ClientId::new(0), ClientId::new(1)] { let data = BytesMut::from([1u8; 512].as_slice()); let hash = integrity_hash(&[&data]); - let r = Ok(vec![Some(BlockContext { - encryption_context: None, - hash, - })]); + let r = Ok(vec![ReadBlockContext::Unencrypted { hash }]); up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id, action: ClientAction::Response(Message::ReadResponse { @@ -4058,10 +4029,7 @@ pub(crate) mod test { // between multiple ReadResponse let data = BytesMut::from([2u8; 512].as_slice()); let hash = integrity_hash(&[&data]); - let r = Ok(vec![Some(BlockContext { - encryption_context: None, - hash, - })]); + let r = Ok(vec![ReadBlockContext::Unencrypted { hash }]); let thread = std::thread::spawn(move || { up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(2), @@ -4102,10 +4070,7 @@ pub(crate) mod test { let data = BytesMut::from([1u8; 512].as_slice()); let hash = integrity_hash(&[&data]); - let r1 = Ok(vec![Some(BlockContext { - encryption_context: None, - hash, - })]); + let r1 = Ok(vec![ReadBlockContext::Unencrypted { hash }]); up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(1), action: ClientAction::Response(Message::ReadResponse { @@ -4123,10 +4088,7 @@ pub(crate) mod test { // the first block matches. let data = BytesMut::from([1u8; 512 * 2].as_slice()); let hash = integrity_hash(&[&data[0..512]]); - let response = Some(BlockContext { - encryption_context: None, - hash, - }); + let response = ReadBlockContext::Unencrypted { hash }; let r2 = Ok(vec![response, response]); let thread = std::thread::spawn(move || { up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { @@ -4169,7 +4131,7 @@ pub(crate) mod test { // The first read has no block contexts, because it was unwritten let data = BytesMut::from([0u8; 512].as_slice()); - let r1 = Ok(vec![None]); + let r1 = Ok(vec![ReadBlockContext::Empty]); up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(1), action: ClientAction::Response(Message::ReadResponse { @@ -4185,10 +4147,7 @@ pub(crate) mod test { // Send back a second response with actual block contexts (oh no!) let hash = integrity_hash(&[&data]); - let r2 = Ok(vec![Some(BlockContext { - encryption_context: None, - hash, - })]); + let r2 = Ok(vec![ReadBlockContext::Unencrypted { hash }]); let thread = std::thread::spawn(move || { up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(2), @@ -4230,10 +4189,7 @@ pub(crate) mod test { // The first read has no block contexts, because it was unwritten let data = BytesMut::from([0u8; 512].as_slice()); let hash = integrity_hash(&[&data]); - let r1 = Ok(vec![Some(BlockContext { - encryption_context: None, - hash, - })]); + let r1 = Ok(vec![ReadBlockContext::Unencrypted { hash }]); up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(1), action: ClientAction::Response(Message::ReadResponse { @@ -4248,7 +4204,7 @@ pub(crate) mod test { })); // Send back a second response with no actual data (oh no!) - let r2 = Ok(vec![None]); + let r2 = Ok(vec![ReadBlockContext::Empty]); let thread = std::thread::spawn(move || { up.apply(UpstairsAction::Downstairs(DownstairsAction::Client { client_id: ClientId::new(2),