From 11be09736c1b374ecaf1cba1cb5c2ec6c7515b26 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sat, 20 Jan 2024 23:32:21 +0000 Subject: [PATCH] generate a random salt and store it in object service --- .../taskchampion/src/server/cloud/server.rs | 55 ++++++++++++++++--- .../taskchampion/src/server/encryption.rs | 22 ++++++-- .../taskchampion/src/server/sync/mod.rs | 4 +- 3 files changed, 66 insertions(+), 15 deletions(-) diff --git a/taskchampion/taskchampion/src/server/cloud/server.rs b/taskchampion/taskchampion/src/server/cloud/server.rs index ac6bfbdb1..6aaee585b 100644 --- a/taskchampion/taskchampion/src/server/cloud/server.rs +++ b/taskchampion/taskchampion/src/server/cloud/server.rs @@ -20,9 +20,9 @@ use uuid::Uuid; /// ## Encryption /// /// The encryption scheme is described in `sync-protocol.md`. The salt value used for key -/// derivation is the SHA256 hash of the string "TaskChampion". Object names are not encrypted, -/// by the nature of key/value stores. Since the content of the "latest" object can usually be -/// inferred from object names, it, too, is not encrypted. +/// derivation is stored in "salt", which is created if it does not exist. Object names are not +/// encrypted, by the nature of key/value stores. Since the content of the "latest" object can +/// usually be inferred from object names, it, too, is not encrypted. /// /// ## Object Organization /// @@ -82,8 +82,9 @@ fn version_to_bytes(v: VersionId) -> Vec { } impl CloudServer { - pub(in crate::server) fn new(service: SVC, encryption_secret: Vec) -> Result { - let cryptor = Cryptor::new(b"Taskchampion", &encryption_secret.into())?; + pub(in crate::server) fn new(mut service: SVC, encryption_secret: Vec) -> Result { + let salt = Self::get_salt(&mut service)?; + let cryptor = Cryptor::new(salt, &encryption_secret.into())?; Ok(Self { service, cryptor, @@ -93,6 +94,17 @@ impl CloudServer { }) } + /// Get the salt value stored in the service, creating a new random one if necessary. + fn get_salt(service: &mut SVC) -> Result> { + const SALT_NAME: &[u8] = b"salt"; + loop { + if let Some(salt) = service.get(SALT_NAME)? { + return Ok(salt); + } + service.compare_and_swap(SALT_NAME, None, Cryptor::gen_salt()?)?; + } + } + /// Generate an object name for the given parent and child versions. fn version_name(parent_version_id: &VersionId, child_version_id: &VersionId) -> Vec { format!( @@ -469,11 +481,20 @@ mod tests { /// A simple in-memory service for testing. All insertions via Service methods occur at time /// `INSERTION_TIME`. All versions older that 1000 are considered "old". - #[derive(Default, Clone)] + #[derive(Clone)] struct MockService(HashMap, (u64, Vec)>); const INSERTION_TIME: u64 = 9999999999; + impl MockService { + fn new() -> Self { + let mut map = HashMap::new(); + // Use a fixed salt for consistent results + map.insert(b"salt".to_vec(), (0, b"abcdefghabcdefgh".to_vec())); + Self(map) + } + } + impl Service for MockService { fn put(&mut self, name: &[u8], value: &[u8]) -> Result<()> { self.0 @@ -579,7 +600,7 @@ mod tests { Self { cryptor: self.cryptor.clone(), cleanup_probability: 0, - service: MockService::default(), + service: MockService::new(), add_version_intercept: None, } } @@ -631,7 +652,7 @@ mod tests { const SECRET: &[u8] = b"testing"; fn make_server() -> CloudServer { - let mut server = CloudServer::new(MockService::default(), SECRET.into()).unwrap(); + let mut server = CloudServer::new(MockService::new(), SECRET.into()).unwrap(); // Prevent cleanup during tests. server.cleanup_probability = 0; server @@ -744,6 +765,24 @@ mod tests { ); } + #[test] + fn get_salt_existing() { + let mut service = MockService::new(); + assert_eq!( + CloudServer::::get_salt(&mut service).unwrap(), + b"abcdefghabcdefgh".to_vec() + ); + } + + #[test] + fn get_salt_create() { + let mut service = MockService::new(); + service.del(b"salt").unwrap(); + let got_salt = CloudServer::::get_salt(&mut service).unwrap(); + let salt_obj = service.get(b"salt").unwrap().unwrap(); + assert_eq!(got_salt, salt_obj); + } + #[test] fn get_latest_empty() { let mut server = make_server(); diff --git a/taskchampion/taskchampion/src/server/encryption.rs b/taskchampion/taskchampion/src/server/encryption.rs index 5bf1b0ed9..3bdacc487 100644 --- a/taskchampion/taskchampion/src/server/encryption.rs +++ b/taskchampion/taskchampion/src/server/encryption.rs @@ -1,7 +1,7 @@ /// This module implements the encryption specified in the sync-protocol /// document. use crate::errors::{Error, Result}; -use ring::{aead, digest, pbkdf2, rand, rand::SecureRandom}; +use ring::{aead, pbkdf2, rand, rand::SecureRandom}; use uuid::Uuid; const PBKDF2_ITERATIONS: u32 = 100000; @@ -26,10 +26,17 @@ impl Cryptor { }) } + /// Generate a suitable random salt. + pub(super) fn gen_salt() -> Result> { + let rng = rand::SystemRandom::new(); + let mut salt = [0u8; 16]; + rng.fill(&mut salt) + .map_err(|_| anyhow::anyhow!("error generating random salt"))?; + Ok(salt.to_vec()) + } + /// Derive a key as specified for version 1. Note that this may take 10s of ms. fn derive_key(salt: impl AsRef<[u8]>, secret: &Secret) -> Result { - let salt = digest::digest(&digest::SHA256, salt.as_ref()); - let mut key_bytes = vec![0u8; aead::CHACHA20_POLY1305.key_len()]; pbkdf2::derive( pbkdf2::PBKDF2_HMAC_SHA256, @@ -94,7 +101,7 @@ impl Cryptor { let plaintext = self .key .open_in_place(nonce, aad, payload.as_mut()) - .map_err(|_| anyhow::anyhow!("error while creating AEAD key"))?; + .map_err(|_| anyhow::anyhow!("error while unsealing encrypted value"))?; Ok(Unsealed { version_id, @@ -198,6 +205,7 @@ impl From for Vec { mod test { use super::*; use pretty_assertions::assert_eq; + use ring::digest; #[test] fn envelope_round_trip() { @@ -320,10 +328,12 @@ mod test { use pretty_assertions::assert_eq; /// The values in generate-test-data.py - fn defaults() -> (Uuid, Uuid, Vec) { + fn defaults() -> (Uuid, Vec, Vec) { + let client_id = Uuid::parse_str("0666d464-418a-4a08-ad53-6f15c78270cd").unwrap(); + let salt = dbg!(digest::digest(&digest::SHA256, client_id.as_ref())); ( Uuid::parse_str("b0517957-f912-4d49-8330-f612e73030c4").unwrap(), - Uuid::parse_str("0666d464-418a-4a08-ad53-6f15c78270cd").unwrap(), + salt.as_ref().to_vec(), b"b4a4e6b7b811eda1dc1a2693ded".to_vec(), ) } diff --git a/taskchampion/taskchampion/src/server/sync/mod.rs b/taskchampion/taskchampion/src/server/sync/mod.rs index c42db7499..a7724fe6e 100644 --- a/taskchampion/taskchampion/src/server/sync/mod.rs +++ b/taskchampion/taskchampion/src/server/sync/mod.rs @@ -3,6 +3,7 @@ use crate::server::{ AddVersionResult, GetVersionResult, HistorySegment, Server, Snapshot, SnapshotUrgency, VersionId, }; +use ring::digest; use std::time::Duration; use uuid::Uuid; @@ -28,10 +29,11 @@ impl SyncServer { /// identify this client to the server. Multiple replicas synchronizing the same task history /// should use the same client_id. pub fn new(origin: String, client_id: Uuid, encryption_secret: Vec) -> Result { + let salt = dbg!(digest::digest(&digest::SHA256, client_id.as_ref())); Ok(SyncServer { origin, client_id, - cryptor: Cryptor::new(client_id, &Secret(encryption_secret.to_vec()))?, + cryptor: Cryptor::new(salt, &Secret(encryption_secret.to_vec()))?, agent: ureq::AgentBuilder::new() .timeout_connect(Duration::from_secs(10)) .timeout_read(Duration::from_secs(60))