From 07cb9ed128257cd3a6fdc12e5b35c4ccbb68aa3e Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 31 Jul 2024 15:46:20 -0400 Subject: [PATCH] Simplify block context for reads (#1391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Right now, the `ReadResponse` sends back a `Vec>`, i.e. an `Option` for each block. The `BlockContext` in turn contains ```rust pub struct BlockContext { pub hash: u64, pub encryption_context: Option, } ``` If `encryption_context` is populated, then sending `hash` to the Upstairs is unnecessary: we are using authenticated encryption, so we know whether the block data + context is valid based on whether it decrypted successfully. This PR removes `hash` from the `ReadResponse` message in favor of a new `enum`: ```rust #[derive(Debug, PartialEq, Copy, Clone, Serialize, Deserialize)] pub enum ReadBlockContext { Empty, Encrypted { ctx: EncryptionContext }, Unencrypted { hash: u64 }, } ``` This does not change the on-disk format or write message format, which both continue to use hashes: - When **sending** data to the Downstairs, the hash (in `BlockContext`) lets us detect corruption in transit. We can't use the encryption context here, because the Downstairs isn't allowed to have encryption keys - When recovering from a bad write, `on_disk_hash` is used to figure out which context slot is valid. `on_disk_hash` is never sent over the network¹ This PR is a step towards [RFD 490 § Metadata reduction](https://rfd.shared.oxide.computer/rfd/490#_metadata_reduction), which proposes to **not** store block hashes for encrypted data on disk. If in the future we don't store block hashes for encrypted data, we would not be able to send them over the network; this PR removes that future hurdle. However, the PR stands alone as a small optimization (39 → 32 bytes per block) that simplifies program behavior (no need to think about what happens if encryption fails but the hash matches, or vice versa). ¹ `on_disk_hash` is also _technically_ superfluous if we already have `hash` (see https://github.com/oxidecomputer/crucible/issues/1161), but this PR doesn't change it --- downstairs/src/dump.rs | 42 +++++----- downstairs/src/extent_inner_raw.rs | 29 +++++-- downstairs/src/extent_inner_sqlite.rs | 38 ++++++--- downstairs/src/lib.rs | 42 +++++----- downstairs/src/region.rs | 73 ++++++++++-------- protocol/src/lib.rs | 40 +++++++++- upstairs/src/buffer.rs | 18 ++--- upstairs/src/client.rs | 62 +++++---------- upstairs/src/deferred.rs | 55 ++++++++++--- upstairs/src/downstairs.rs | 7 +- upstairs/src/dummy_downstairs_tests.rs | 9 +-- upstairs/src/lib.rs | 2 +- upstairs/src/test.rs | 28 ++----- upstairs/src/upstairs.rs | 102 +++++++------------------ 14 files changed, 286 insertions(+), 261 deletions(-) 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),