[BREAKING CHANGE] drop use of tindercrypt, and use ring directly

.changelogs/2021-10-16-issue299.md

@@ -0,0 +1 @@
+- The encryption format used for synchronization has changed incompatibly

Cargo.lock

@@ -2253,12 +2253,6 @@ dependencies = [
  "tempfile",
 ]
 
-[[package]]
-name = "protobuf"
-version = "2.24.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "db50e77ae196458ccd3dc58a31ea1a90b0698ab1b7928d89f644c25d72070267"
-
 [[package]]
 name = "pulldown-cmark"
 version = "0.7.2"
@@ -2967,6 +2961,7 @@ dependencies = [
  "log",
  "pretty_assertions",
  "proptest",
+ "ring",
  "rstest",
  "rusqlite",
  "serde",
@@ -2975,7 +2970,6 @@ dependencies = [
  "strum_macros 0.21.1",
  "tempfile",
  "thiserror",
- "tindercrypt",
  "ureq",
  "uuid",
 ]
@@ -3184,18 +3178,6 @@ dependencies = [
  "syn",
 ]
 
-[[package]]
-name = "tindercrypt"
-version = "0.2.2"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f34fd5cc8db265f27abf29e8a3cec5cc643ae9f3f9ae39f08a77dc4add375b6d"
-dependencies = [
- "protobuf",
- "rand 0.7.3",
- "ring",
- "thiserror",
-]
-
 [[package]]
 name = "tinyvec"
 version = "1.2.0"

docs/src/sync-protocol.md

@@ -42,7 +42,32 @@ The fourth invariant prevents the server from discarding versions newer than the
 
 ### Encryption
 
-TBD (#299)
+The client configuration includes an encryption secret of arbitrary length and a clientId to identify itself.
+This section describes how that information is used to encrypt and decrypt data sent to the server (versions and snapshots).
+
+#### Key Derivation
+
+The client derives the 32-byte encryption key from the configured encryption secret using PBKDF2 with HMAC-SHA256 and 100,000 iterations.
+The salt is the SHA256 hash of the 16-byte form of the client key.
+
+#### Encryption
+
+The client uses [AEAD](https://commondatastorage.googleapis.com/chromium-boringssl-docs/aead.h.html), with algorithm AES_256_GCM.
+Each encrypted payload has an associated version ID.
+The 16-byte form of this UUID is used as the associated data (AAD) with the AEAD algorithm.
+The client should generate a random nonce, noting that AEAD is _not secure_ if a nonce is used repeatedly for the same key.
+
+Although the AEAD specification distinguishes ciphertext and tags, for purposes of this specification they are considered concatenated into a single bytestring as in BoringSSL's `EVP_AEAD_CTX_seal`.
+
+#### Representation
+
+The final byte-stream is comprised of the following structure, with integers represented in network-endian format.
+
+* `version` (32-bit int) - format version (always 1)
+* `nonce` (12 bytes) - encryption nonce
+* `ciphertext` (remaining bytes) - ciphertext from sealing operation
+
+Future versions may have a completely different format.
 
 ### Version
 

taskchampion/Cargo.toml

@@ -19,12 +19,12 @@ anyhow = "1.0"
 thiserror = "1.0"
 ureq = "^2.1.0"
 log = "^0.4.14"
-tindercrypt = { version = "^0.2.2", default-features = false }
 rusqlite = { version = "0.25", features = ["bundled"] }
 strum = "0.21"
 strum_macros = "0.21"
 flate2 = "1"
 byteorder = "1.0"
+ring = "0.16"
 
 [dev-dependencies]
 proptest = "^1.0.0"

taskchampion/src/server/config.rs

@@ -33,7 +33,7 @@ impl ServerConfig {
                 origin,
                 client_key,
                 encryption_secret,
-            } => Box::new(RemoteServer::new(origin, client_key, encryption_secret)),
+            } => Box::new(RemoteServer::new(origin, client_key, encryption_secret)?),
         })
     }
 }

taskchampion/src/server/remote/crypto.rs

@@ -1,9 +1,107 @@
-use crate::server::HistorySegment;
+/// This module implements the encryption specified in the sync-protocol
+/// document.
 use byteorder::{NetworkEndian, ReadBytesExt, WriteBytesExt};
+use ring::{aead, digest, pbkdf2, rand, rand::SecureRandom};
 use std::io::{Cursor, Read};
-use tindercrypt::cryptors::RingCryptor;
 use uuid::Uuid;
 
+const PBKDF2_ITERATIONS: u32 = 100000;
+const ENVELOPE_VERSION: u32 = 1;
+
+/// An Cryptor stores a secret and allows sealing and unsealing.  It derives a key from the secret,
+/// which takes a nontrivial amount of time, so it should be created once and re-used for the given
+/// client_key.
+pub(super) struct Cryptor {
+    key: aead::LessSafeKey,
+    rng: rand::SystemRandom,
+}
+
+impl Cryptor {
+    pub(super) fn new(client_key: Uuid, secret: &Secret) -> anyhow::Result<Self> {
+        Ok(Cryptor {
+            key: Self::derive_key(client_key, secret)?,
+            rng: rand::SystemRandom::new(),
+        })
+    }
+
+    /// Derive a key as specified for version 1.  Note that this may take 10s of ms.
+    fn derive_key(client_key: Uuid, secret: &Secret) -> anyhow::Result<aead::LessSafeKey> {
+        let salt = digest::digest(&digest::SHA256, client_key.as_bytes());
+
+        let mut key_bytes = vec![0u8; ring::aead::AES_256_GCM.key_len()];
+        pbkdf2::derive(
+            pbkdf2::PBKDF2_HMAC_SHA256,
+            std::num::NonZeroU32::new(PBKDF2_ITERATIONS).unwrap(),
+            salt.as_ref(),
+            secret.as_ref(),
+            &mut key_bytes,
+        );
+
+        let unbound_key = ring::aead::UnboundKey::new(&ring::aead::AES_256_GCM, &key_bytes)
+            .map_err(|_| anyhow::anyhow!("error while creating AEAD key"))?;
+        Ok(ring::aead::LessSafeKey::new(unbound_key))
+    }
+
+    /// Encrypt the given payload.
+    pub(super) fn seal(&self, payload: Unsealed) -> anyhow::Result<Sealed> {
+        let Unsealed {
+            version_id,
+            mut payload,
+        } = payload;
+
+        let mut nonce_buf = [0u8; aead::NONCE_LEN];
+        self.rng
+            .fill(&mut nonce_buf)
+            .map_err(|_| anyhow::anyhow!("error generating random nonce"))?;
+        let nonce = ring::aead::Nonce::assume_unique_for_key(nonce_buf);
+
+        let aad = ring::aead::Aad::from(version_id.as_bytes());
+
+        let tag = self
+            .key
+            .seal_in_place_separate_tag(nonce, aad, &mut payload)
+            .map_err(|_| anyhow::anyhow!("error while sealing"))?;
+        payload.extend_from_slice(tag.as_ref());
+
+        let env = Envelope {
+            nonce: &nonce_buf,
+            payload: payload.as_ref(),
+        };
+
+        Ok(Sealed {
+            version_id,
+            payload: env.to_bytes(),
+        })
+    }
+
+    /// Decrypt the given payload, verifying it was created for the given version_id
+    pub(super) fn unseal(&self, payload: Sealed) -> anyhow::Result<Unsealed> {
+        let Sealed {
+            version_id,
+            payload,
+        } = payload;
+
+        let env = Envelope::from_bytes(&payload)?;
+
+        let mut nonce = [0u8; aead::NONCE_LEN];
+        nonce.copy_from_slice(env.nonce);
+        let nonce = ring::aead::Nonce::assume_unique_for_key(nonce);
+        let aad = ring::aead::Aad::from(version_id.as_bytes());
+
+        let mut payload = env.payload.to_vec();
+        let plaintext = self
+            .key
+            .open_in_place(nonce, aad, payload.as_mut())
+            .map_err(|_| anyhow::anyhow!("error while creating AEAD key"))?;
+
+        Ok(Unsealed {
+            version_id,
+            payload: plaintext.to_vec(),
+        })
+    }
+}
+
+/// Secret represents a secret key as used for encryption and decryption.
 pub(super) struct Secret(pub(super) Vec<u8>);
 
 impl From<Vec<u8>> for Secret {
@@ -18,24 +116,17 @@ impl AsRef<[u8]> for Secret {
     }
 }
 
-const PBKDF2_SALT_SIZE: usize = 32;
-const NONCE_SIZE: usize = 12;
-const ENVELOPE_VERSION: u32 = 1;
-
 /// Envelope for the data stored on the server, containing the information
 /// required to decrypt.
 #[derive(Debug, PartialEq, Eq)]
-struct Envelope {
+struct Envelope<'a> {
-    kdf_iterations: u32,
+    nonce: &'a [u8],
-    kdf_salt: [u8; PBKDF2_SALT_SIZE],
+    payload: &'a [u8],
-    nonce: [u8; NONCE_SIZE],
 }
 
-impl Envelope {
+impl<'a> Envelope<'a> {
-    const SIZE: usize = 4 + 4 + PBKDF2_SALT_SIZE + NONCE_SIZE;
+    fn from_bytes(buf: &'a [u8]) -> anyhow::Result<Envelope<'a>> {
-
+        if buf.len() <= 4 + aead::NONCE_LEN {
-    fn from_bytes(buf: &[u8]) -> anyhow::Result<Self> {
-        if buf.len() < 4 {
             anyhow::bail!("envelope is too small");
         }
 
@@ -44,84 +135,61 @@ impl Envelope {
         if version != 1 {
             anyhow::bail!("unrecognized envelope version {}", version);
         }
-        if buf.len() != Envelope::SIZE {
-            anyhow::bail!("envelope size {} is not {}", buf.len(), Envelope::SIZE);
-        }
 
-        let kdf_iterations = rdr.read_u32::<NetworkEndian>().unwrap();
+        Ok(Envelope {
-
+            nonce: &buf[4..4 + aead::NONCE_LEN],
-        let mut env = Envelope {
+            payload: &buf[4 + aead::NONCE_LEN..],
-            kdf_iterations,
+        })
-            kdf_salt: [0; PBKDF2_SALT_SIZE],
-            nonce: [0; NONCE_SIZE],
-        };
-        env.kdf_salt.clone_from_slice(&buf[8..8 + PBKDF2_SALT_SIZE]);
-        env.nonce
-            .clone_from_slice(&buf[8 + PBKDF2_SALT_SIZE..8 + PBKDF2_SALT_SIZE + NONCE_SIZE]);
-        Ok(env) // TODO: test
     }
 
     fn to_bytes(&self) -> Vec<u8> {
-        let mut buf = Vec::with_capacity(Envelope::SIZE);
+        let mut buf = Vec::with_capacity(4 + self.nonce.len() + self.payload.len());
 
         buf.write_u32::<NetworkEndian>(ENVELOPE_VERSION).unwrap();
-        buf.write_u32::<NetworkEndian>(self.kdf_iterations).unwrap();
+        buf.extend_from_slice(self.nonce);
-        buf.extend_from_slice(&self.kdf_salt);
+        buf.extend_from_slice(self.payload);
-        buf.extend_from_slice(&self.nonce);
+        buf
-        buf // TODO: test
     }
 }
 
-/// A cleartext payload with an attached version_id.  The version_id is used to
+/// A unsealed payload with an attached version_id.  The version_id is used to
-/// validate the context of the payload.
+/// validate the context of the payload on unsealing.
-pub(super) struct Cleartext {
+pub(super) struct Unsealed {
     pub(super) version_id: Uuid,
-    pub(super) payload: HistorySegment,
+    pub(super) payload: Vec<u8>,
 }
 
-impl Cleartext {
+/// An encrypted payload
-    /// Seal the payload into its ciphertext
+pub(super) struct Sealed {
-    pub(super) fn seal(self, secret: &Secret) -> anyhow::Result<Ciphertext> {
+    pub(super) version_id: Uuid,
-        let cryptor = RingCryptor::new().with_aad(self.version_id.as_bytes());
+    pub(super) payload: Vec<u8>,
-        let ciphertext = cryptor.seal_with_passphrase(secret.as_ref(), &self.payload)?;
-        Ok(Ciphertext(ciphertext))
-    }
 }
 
-/// An ecrypted payload
+impl Sealed {
-pub(super) struct Ciphertext(pub(super) Vec<u8>);
-
-impl Ciphertext {
     pub(super) fn from_resp(
         resp: ureq::Response,
+        version_id: Uuid,
         content_type: &str,
-    ) -> Result<Ciphertext, anyhow::Error> {
+    ) -> Result<Sealed, anyhow::Error> {
         if resp.header("Content-Type") == Some(content_type) {
             let mut reader = resp.into_reader();
-            let mut bytes = vec![];
+            let mut payload = vec![];
-            reader.read_to_end(&mut bytes)?;
+            reader.read_to_end(&mut payload)?;
-            Ok(Self(bytes))
+            Ok(Self {
+                version_id,
+                payload,
+            })
         } else {
             Err(anyhow::anyhow!(
                 "Response did not have expected content-type"
             ))
         }
     }
-
-    pub(super) fn open(self, secret: &Secret, version_id: Uuid) -> anyhow::Result<Cleartext> {
-        let cryptor = RingCryptor::new().with_aad(version_id.as_bytes());
-        let plaintext = cryptor.open(secret.as_ref(), &self.0)?;
-
-        Ok(Cleartext {
-            version_id,
-            payload: plaintext,
-        })
-    }
 }
 
-impl AsRef<[u8]> for Ciphertext {
+impl AsRef<[u8]> for Sealed {
     fn as_ref(&self) -> &[u8] {
-        self.0.as_ref()
+        self.payload.as_ref()
     }
 }
 
@@ -133,9 +201,8 @@ mod test {
     #[test]
     fn envelope_round_trip() {
         let env = Envelope {
-            kdf_iterations: 100,
+            nonce: &[2; 12],
-            kdf_salt: [1; 32],
+            payload: b"HELLO",
-            nonce: [2; 12],
         };
 
         let bytes = env.to_bytes();
@@ -147,48 +214,76 @@ mod test {
     fn round_trip() {
         let version_id = Uuid::new_v4();
         let payload = b"HISTORY REPEATS ITSELF".to_vec();
-        let secret = Secret(b"SEKRIT".to_vec());
 
-        let cleartext = Cleartext {
+        let secret = Secret(b"SEKRIT".to_vec());
+        let cryptor = Cryptor::new(Uuid::new_v4(), &secret).unwrap();
+
+        let unsealed = Unsealed {
             version_id,
             payload: payload.clone(),
         };
-        let ciphertext = cleartext.seal(&secret).unwrap();
+        let sealed = cryptor.seal(unsealed).unwrap();
-        let cleartext = ciphertext.open(&secret, version_id).unwrap();
+        let unsealed = cryptor.unseal(sealed).unwrap();
 
-        assert_eq!(cleartext.payload, payload);
+        assert_eq!(unsealed.payload, payload);
-        assert_eq!(cleartext.version_id, version_id);
+        assert_eq!(unsealed.version_id, version_id);
     }
 
     #[test]
     fn round_trip_bad_key() {
         let version_id = Uuid::new_v4();
         let payload = b"HISTORY REPEATS ITSELF".to_vec();
-        let secret = Secret(b"SEKRIT".to_vec());
+        let client_key = Uuid::new_v4();
 
-        let cleartext = Cleartext {
+        let secret = Secret(b"SEKRIT".to_vec());
+        let cryptor = Cryptor::new(client_key, &secret).unwrap();
+
+        let unsealed = Unsealed {
             version_id,
             payload: payload.clone(),
         };
-        let ciphertext = cleartext.seal(&secret).unwrap();
+        let sealed = cryptor.seal(unsealed).unwrap();
 
-        let secret = Secret(b"BADSEKRIT".to_vec());
+        let secret = Secret(b"DIFFERENT_SECRET".to_vec());
-        assert!(ciphertext.open(&secret, version_id).is_err());
+        let cryptor = Cryptor::new(client_key, &secret).unwrap();
+        assert!(cryptor.unseal(sealed).is_err());
     }
 
     #[test]
     fn round_trip_bad_version() {
         let version_id = Uuid::new_v4();
         let payload = b"HISTORY REPEATS ITSELF".to_vec();
-        let secret = Secret(b"SEKRIT".to_vec());
+        let client_key = Uuid::new_v4();
 
-        let cleartext = Cleartext {
+        let secret = Secret(b"SEKRIT".to_vec());
+        let cryptor = Cryptor::new(client_key, &secret).unwrap();
+
+        let unsealed = Unsealed {
             version_id,
             payload: payload.clone(),
         };
-        let ciphertext = cleartext.seal(&secret).unwrap();
+        let mut sealed = cryptor.seal(unsealed).unwrap();
+        sealed.version_id = Uuid::new_v4(); // change the version_id
+        assert!(cryptor.unseal(sealed).is_err());
+    }
 
-        let bad_version_id = Uuid::new_v4();
+    #[test]
-        assert!(ciphertext.open(&secret, bad_version_id).is_err());
+    fn round_trip_bad_client_key() {
+        let version_id = Uuid::new_v4();
+        let payload = b"HISTORY REPEATS ITSELF".to_vec();
+        let client_key = Uuid::new_v4();
+
+        let secret = Secret(b"SEKRIT".to_vec());
+        let cryptor = Cryptor::new(client_key, &secret).unwrap();
+
+        let unsealed = Unsealed {
+            version_id,
+            payload: payload.clone(),
+        };
+        let sealed = cryptor.seal(unsealed).unwrap();
+
+        let client_key = Uuid::new_v4();
+        let cryptor = Cryptor::new(client_key, &secret).unwrap();
+        assert!(cryptor.unseal(sealed).is_err());
     }
 }

taskchampion/src/server/remote/mod.rs

@@ -6,12 +6,12 @@ use std::time::Duration;
 use uuid::Uuid;
 
 mod crypto;
-use crypto::{Ciphertext, Cleartext, Secret};
+use crypto::{Cryptor, Sealed, Secret, Unsealed};
 
 pub struct RemoteServer {
     origin: String,
     client_key: Uuid,
-    encryption_secret: Secret,
+    cryptor: Cryptor,
     agent: ureq::Agent,
 }
 
@@ -28,16 +28,20 @@ impl RemoteServer {
     /// without a trailing slash, such as `https://tcsync.example.com`.  Pass a client_key to
     /// identify this client to the server.  Multiple replicas synchronizing the same task history
     /// should use the same client_key.
-    pub fn new(origin: String, client_key: Uuid, encryption_secret: Vec<u8>) -> RemoteServer {
+    pub fn new(
-        RemoteServer {
+        origin: String,
+        client_key: Uuid,
+        encryption_secret: Vec<u8>,
+    ) -> anyhow::Result<RemoteServer> {
+        Ok(RemoteServer {
             origin,
             client_key,
-            encryption_secret: encryption_secret.into(),
+            cryptor: Cryptor::new(client_key, &Secret(encryption_secret.to_vec()))?,
             agent: ureq::AgentBuilder::new()
                 .timeout_connect(Duration::from_secs(10))
                 .timeout_read(Duration::from_secs(60))
                 .build(),
-        }
+        })
     }
 }
 
@@ -73,17 +77,17 @@ impl Server for RemoteServer {
             "{}/v1/client/add-version/{}",
             self.origin, parent_version_id
         );
-        let cleartext = Cleartext {
+        let unsealed = Unsealed {
             version_id: parent_version_id,
             payload: history_segment,
         };
-        let ciphertext = cleartext.seal(&self.encryption_secret)?;
+        let sealed = self.cryptor.seal(unsealed)?;
         match self
             .agent
             .post(&url)
             .set("Content-Type", HISTORY_SEGMENT_CONTENT_TYPE)
             .set("X-Client-Key", &self.client_key.to_string())
-            .send_bytes(ciphertext.as_ref())
+            .send_bytes(sealed.as_ref())
         {
             Ok(resp) => {
                 let version_id = get_uuid_header(&resp, "X-Version-Id")?;
@@ -120,10 +124,9 @@ impl Server for RemoteServer {
             Ok(resp) => {
                 let parent_version_id = get_uuid_header(&resp, "X-Parent-Version-Id")?;
                 let version_id = get_uuid_header(&resp, "X-Version-Id")?;
-                let ciphertext = Ciphertext::from_resp(resp, HISTORY_SEGMENT_CONTENT_TYPE)?;
+                let sealed =
-                let history_segment = ciphertext
+                    Sealed::from_resp(resp, parent_version_id, HISTORY_SEGMENT_CONTENT_TYPE)?;
-                    .open(&self.encryption_secret, parent_version_id)?
+                let history_segment = self.cryptor.unseal(sealed)?.payload;
-                    .payload;
                 Ok(GetVersionResult::Version {
                     version_id,
                     parent_version_id,
@@ -139,17 +142,17 @@ impl Server for RemoteServer {
 
     fn add_snapshot(&mut self, version_id: VersionId, snapshot: Snapshot) -> anyhow::Result<()> {
         let url = format!("{}/v1/client/add-snapshot/{}", self.origin, version_id);
-        let cleartext = Cleartext {
+        let unsealed = Unsealed {
             version_id,
             payload: snapshot,
         };
-        let ciphertext = cleartext.seal(&self.encryption_secret)?;
+        let sealed = self.cryptor.seal(unsealed)?;
         Ok(self
             .agent
             .post(&url)
             .set("Content-Type", SNAPSHOT_CONTENT_TYPE)
             .set("X-Client-Key", &self.client_key.to_string())
-            .send_bytes(ciphertext.as_ref())
+            .send_bytes(sealed.as_ref())
             .map(|_| ())?)
     }
 }