generate a random salt and store it in object service

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<u8> {
 }
 
 impl<SVC: Service> CloudServer<SVC> {
-    pub(in crate::server) fn new(service: SVC, encryption_secret: Vec<u8>) -> Result<Self> {
-        let cryptor = Cryptor::new(b"Taskchampion", &encryption_secret.into())?;
+    pub(in crate::server) fn new(mut service: SVC, encryption_secret: Vec<u8>) -> Result<Self> {
+        let salt = Self::get_salt(&mut service)?;
+        let cryptor = Cryptor::new(salt, &encryption_secret.into())?;
         Ok(Self {
             service,
             cryptor,
@@ -93,6 +94,17 @@ impl<SVC: Service> CloudServer<SVC> {
         })
     }
 
+    /// Get the salt value stored in the service, creating a new random one if necessary.
+    fn get_salt(service: &mut SVC) -> Result<Vec<u8>> {
+        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<u8> {
         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<Vec<u8>, (u64, Vec<u8>)>);
 
     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<MockService> {
-        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::<MockService>::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::<MockService>::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();

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<Vec<u8>> {
+        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<aead::LessSafeKey> {
-        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<Sealed> for Vec<u8> {
 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<u8>) {
+        fn defaults() -> (Uuid, Vec<u8>, Vec<u8>) {
+            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(),
             )
         }

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<u8>) -> Result<SyncServer> {
+        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))