From d4ecabd83d30369b720aea121487faae2085b21c Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 27 Dec 2023 15:04:26 -0500 Subject: [PATCH 1/2] rename crypto.rs to encryption.rs --- .../taskchampion/src/server/cloud/server.rs | 2 +- .../taskchampion/src/server/crypto.rs | 406 ------------------ taskchampion/taskchampion/src/server/mod.rs | 2 +- .../taskchampion/src/server/sync/mod.rs | 2 +- 4 files changed, 3 insertions(+), 409 deletions(-) delete mode 100644 taskchampion/taskchampion/src/server/crypto.rs diff --git a/taskchampion/taskchampion/src/server/cloud/server.rs b/taskchampion/taskchampion/src/server/cloud/server.rs index 898095c99..d06c18958 100644 --- a/taskchampion/taskchampion/src/server/cloud/server.rs +++ b/taskchampion/taskchampion/src/server/cloud/server.rs @@ -1,6 +1,6 @@ use super::service::{ObjectInfo, Service}; use crate::errors::{Error, Result}; -use crate::server::crypto::{Cryptor, Sealed, Unsealed}; +use crate::server::encryption::{Cryptor, Sealed, Unsealed}; use crate::server::{ AddVersionResult, GetVersionResult, HistorySegment, Server, Snapshot, SnapshotUrgency, VersionId, diff --git a/taskchampion/taskchampion/src/server/crypto.rs b/taskchampion/taskchampion/src/server/crypto.rs deleted file mode 100644 index 5bf1b0ed9..000000000 --- a/taskchampion/taskchampion/src/server/crypto.rs +++ /dev/null @@ -1,406 +0,0 @@ -/// 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 uuid::Uuid; - -const PBKDF2_ITERATIONS: u32 = 100000; -const ENVELOPE_VERSION: u8 = 1; -const AAD_LEN: usize = 17; -const TASK_APP_ID: u8 = 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 -/// context. -#[derive(Clone)] -pub(super) struct Cryptor { - key: aead::LessSafeKey, - rng: rand::SystemRandom, -} - -impl Cryptor { - pub(super) fn new(salt: impl AsRef<[u8]>, secret: &Secret) -> Result { - Ok(Cryptor { - key: Self::derive_key(salt, secret)?, - rng: rand::SystemRandom::new(), - }) - } - - /// 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, - std::num::NonZeroU32::new(PBKDF2_ITERATIONS).unwrap(), - salt.as_ref(), - secret.as_ref(), - &mut key_bytes, - ); - - let unbound_key = aead::UnboundKey::new(&aead::CHACHA20_POLY1305, &key_bytes) - .map_err(|_| anyhow::anyhow!("error while creating AEAD key"))?; - Ok(aead::LessSafeKey::new(unbound_key)) - } - - /// Encrypt the given payload. - pub(super) fn seal(&self, payload: Unsealed) -> Result { - 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 = aead::Nonce::assume_unique_for_key(nonce_buf); - - let aad = self.make_aad(version_id); - - 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) -> Result { - 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 = aead::Nonce::assume_unique_for_key(nonce); - let aad = self.make_aad(version_id); - - 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(), - }) - } - - fn make_aad(&self, version_id: Uuid) -> aead::Aad<[u8; AAD_LEN]> { - let mut aad = [0u8; AAD_LEN]; - aad[0] = TASK_APP_ID; - aad[1..].copy_from_slice(version_id.as_bytes()); - aead::Aad::from(aad) - } -} - -/// Secret represents a secret key as used for encryption and decryption. -pub(super) struct Secret(pub(super) Vec); - -impl From> for Secret { - fn from(bytes: Vec) -> Self { - Self(bytes) - } -} - -impl AsRef<[u8]> for Secret { - fn as_ref(&self) -> &[u8] { - &self.0 - } -} - -/// Envelope for the data stored on the server, containing the information -/// required to decrypt. -#[derive(Debug, PartialEq, Eq)] -struct Envelope<'a> { - nonce: &'a [u8], - payload: &'a [u8], -} - -impl<'a> Envelope<'a> { - fn from_bytes(buf: &'a [u8]) -> Result> { - if buf.len() <= 1 + aead::NONCE_LEN { - return Err(Error::Server(String::from("envelope is too small"))); - } - - let version = buf[0]; - if version != ENVELOPE_VERSION { - return Err(Error::Server(format!( - "unrecognized encryption envelope version {}", - version - ))); - } - - Ok(Envelope { - nonce: &buf[1..1 + aead::NONCE_LEN], - payload: &buf[1 + aead::NONCE_LEN..], - }) - } - - fn to_bytes(&self) -> Vec { - let mut buf = Vec::with_capacity(1 + self.nonce.len() + self.payload.len()); - - buf.push(ENVELOPE_VERSION); - buf.extend_from_slice(self.nonce); - buf.extend_from_slice(self.payload); - buf - } -} - -/// A unsealed payload with an attached version_id. The version_id is used to -/// validate the context of the payload on unsealing. -pub(super) struct Unsealed { - pub(super) version_id: Uuid, - pub(super) payload: Vec, -} - -impl From for Vec { - fn from(val: Unsealed) -> Self { - val.payload - } -} - -/// An encrypted payload -pub(super) struct Sealed { - pub(super) version_id: Uuid, - pub(super) payload: Vec, -} - -impl AsRef<[u8]> for Sealed { - fn as_ref(&self) -> &[u8] { - self.payload.as_ref() - } -} - -impl From for Vec { - fn from(val: Sealed) -> Self { - val.payload - } -} - -#[cfg(test)] -mod test { - use super::*; - use pretty_assertions::assert_eq; - - #[test] - fn envelope_round_trip() { - let env = Envelope { - nonce: &[2; 12], - payload: b"HELLO", - }; - - let bytes = env.to_bytes(); - let env2 = Envelope::from_bytes(&bytes).unwrap(); - assert_eq!(env, env2); - } - - #[test] - fn envelope_bad_version() { - let env = Envelope { - nonce: &[2; 12], - payload: b"HELLO", - }; - - let mut bytes = env.to_bytes(); - bytes[0] = 99; - assert!(Envelope::from_bytes(&bytes).is_err()); - } - - #[test] - fn envelope_too_short() { - let env = Envelope { - nonce: &[2; 12], - payload: b"HELLO", - }; - - let bytes = env.to_bytes(); - let bytes = &bytes[..10]; - assert!(Envelope::from_bytes(bytes).is_err()); - } - - #[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 cryptor = Cryptor::new(Uuid::new_v4(), &secret).unwrap(); - - let unsealed = Unsealed { - version_id, - payload: payload.clone(), - }; - let sealed = cryptor.seal(unsealed).unwrap(); - let unsealed = cryptor.unseal(sealed).unwrap(); - - assert_eq!(unsealed.payload, payload); - 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 salt = Uuid::new_v4(); - - let secret = Secret(b"SEKRIT".to_vec()); - let cryptor = Cryptor::new(salt, &secret).unwrap(); - - let unsealed = Unsealed { - version_id, - payload, - }; - let sealed = cryptor.seal(unsealed).unwrap(); - - let secret = Secret(b"DIFFERENT_SECRET".to_vec()); - let cryptor = Cryptor::new(salt, &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 salt = Uuid::new_v4(); - - let secret = Secret(b"SEKRIT".to_vec()); - let cryptor = Cryptor::new(salt, &secret).unwrap(); - - let unsealed = Unsealed { - version_id, - payload, - }; - let mut sealed = cryptor.seal(unsealed).unwrap(); - sealed.version_id = Uuid::new_v4(); // change the version_id - assert!(cryptor.unseal(sealed).is_err()); - } - - #[test] - fn round_trip_bad_salt() { - let version_id = Uuid::new_v4(); - let payload = b"HISTORY REPEATS ITSELF".to_vec(); - let salt = Uuid::new_v4(); - - let secret = Secret(b"SEKRIT".to_vec()); - let cryptor = Cryptor::new(salt, &secret).unwrap(); - - let unsealed = Unsealed { - version_id, - payload, - }; - let sealed = cryptor.seal(unsealed).unwrap(); - - let salt = Uuid::new_v4(); - let cryptor = Cryptor::new(salt, &secret).unwrap(); - assert!(cryptor.unseal(sealed).is_err()); - } - - mod externally_valid { - // validate data generated by generate-test-data.py. The intent is to - // validate that this format matches the specification by implementing - // the specification in a second language - use super::*; - use pretty_assertions::assert_eq; - - /// The values in generate-test-data.py - fn defaults() -> (Uuid, Uuid, Vec) { - ( - Uuid::parse_str("b0517957-f912-4d49-8330-f612e73030c4").unwrap(), - Uuid::parse_str("0666d464-418a-4a08-ad53-6f15c78270cd").unwrap(), - b"b4a4e6b7b811eda1dc1a2693ded".to_vec(), - ) - } - - #[test] - fn good() { - let (version_id, salt, encryption_secret) = defaults(); - let sealed = Sealed { - version_id, - payload: include_bytes!("test-good.data").to_vec(), - }; - - let cryptor = Cryptor::new(salt, &Secret(encryption_secret)).unwrap(); - let unsealed = cryptor.unseal(sealed).unwrap(); - - assert_eq!(unsealed.payload, b"SUCCESS"); - assert_eq!(unsealed.version_id, version_id); - } - - #[test] - fn bad_version_id() { - let (version_id, salt, encryption_secret) = defaults(); - let sealed = Sealed { - version_id, - payload: include_bytes!("test-bad-version-id.data").to_vec(), - }; - - let cryptor = Cryptor::new(salt, &Secret(encryption_secret)).unwrap(); - assert!(cryptor.unseal(sealed).is_err()); - } - - #[test] - fn bad_salt() { - let (version_id, salt, encryption_secret) = defaults(); - let sealed = Sealed { - version_id, - payload: include_bytes!("test-bad-client-id.data").to_vec(), - }; - - let cryptor = Cryptor::new(salt, &Secret(encryption_secret)).unwrap(); - assert!(cryptor.unseal(sealed).is_err()); - } - - #[test] - fn bad_secret() { - let (version_id, salt, encryption_secret) = defaults(); - let sealed = Sealed { - version_id, - payload: include_bytes!("test-bad-secret.data").to_vec(), - }; - - let cryptor = Cryptor::new(salt, &Secret(encryption_secret)).unwrap(); - assert!(cryptor.unseal(sealed).is_err()); - } - - #[test] - fn bad_version() { - let (version_id, salt, encryption_secret) = defaults(); - let sealed = Sealed { - version_id, - payload: include_bytes!("test-bad-version.data").to_vec(), - }; - - let cryptor = Cryptor::new(salt, &Secret(encryption_secret)).unwrap(); - assert!(cryptor.unseal(sealed).is_err()); - } - - #[test] - fn bad_app_id() { - let (version_id, salt, encryption_secret) = defaults(); - let sealed = Sealed { - version_id, - payload: include_bytes!("test-bad-app-id.data").to_vec(), - }; - - let cryptor = Cryptor::new(salt, &Secret(encryption_secret)).unwrap(); - assert!(cryptor.unseal(sealed).is_err()); - } - } -} diff --git a/taskchampion/taskchampion/src/server/mod.rs b/taskchampion/taskchampion/src/server/mod.rs index 5b6cd544d..caccdee02 100644 --- a/taskchampion/taskchampion/src/server/mod.rs +++ b/taskchampion/taskchampion/src/server/mod.rs @@ -17,7 +17,7 @@ mod op; mod types; #[cfg(feature = "encryption")] -mod crypto; +mod encryption; #[cfg(feature = "server-sync")] mod sync; diff --git a/taskchampion/taskchampion/src/server/sync/mod.rs b/taskchampion/taskchampion/src/server/sync/mod.rs index b38b2ce2e..c42db7499 100644 --- a/taskchampion/taskchampion/src/server/sync/mod.rs +++ b/taskchampion/taskchampion/src/server/sync/mod.rs @@ -6,7 +6,7 @@ use crate::server::{ use std::time::Duration; use uuid::Uuid; -use super::crypto::{Cryptor, Sealed, Secret, Unsealed}; +use super::encryption::{Cryptor, Sealed, Secret, Unsealed}; pub struct SyncServer { origin: String, From ec5332c441169df9c8bcb0e33b95a80cbe86f420 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 27 Dec 2023 15:50:11 -0500 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: ryneeverett --- doc/man/task-sync.5.in | 10 +++++----- taskchampion/docs/src/encryption.md | 2 +- taskchampion/docs/src/object-store.md | 2 +- taskchampion/taskchampion/src/server/cloud/gcp.rs | 9 +++------ taskchampion/taskchampion/src/server/cloud/server.rs | 2 +- 5 files changed, 11 insertions(+), 14 deletions(-) diff --git a/doc/man/task-sync.5.in b/doc/man/task-sync.5.in index 0b54ea531..c8461ff76 100644 --- a/doc/man/task-sync.5.in +++ b/doc/man/task-sync.5.in @@ -45,7 +45,7 @@ periodically, such as via Taskwarrior provides several options for synchronizing your tasks: - To a server specifically designed to handle Taskwarrior data. - - To a cloud service such as GCP or AWS. + + To a cloud storage provider. Currently only GCP is supported. - To a local, on-disk file. .SS Sync Server @@ -74,12 +74,12 @@ Configure Taskwarrior with these details: To synchronize your tasks to GCP, use the GCP Console to create a new project, and within that project a new Cloud Storage bucket. The default settings for -the bucket are adequete. +the bucket are adequate. -Authenticate to the project with +Authenticate to the project with: - gcloud config set project $PROJECT_NAME - gcloud auth application-default login + $ gcloud config set project $PROJECT_NAME + $ gcloud auth application-default login Then configure Taskwarrior with: diff --git a/taskchampion/docs/src/encryption.md b/taskchampion/docs/src/encryption.md index d05900dae..5d18bddef 100644 --- a/taskchampion/docs/src/encryption.md +++ b/taskchampion/docs/src/encryption.md @@ -8,7 +8,7 @@ Encryption is not used for local (on-disk) sync, but is used for all cases where ## 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 value depends on the implemenation of the protocol, as described in subsequent chapters. +The salt value depends on the implementation of the protocol, as described in subsequent chapters. ## Encryption diff --git a/taskchampion/docs/src/object-store.md b/taskchampion/docs/src/object-store.md index 03ff723a0..6620d6c8f 100644 --- a/taskchampion/docs/src/object-store.md +++ b/taskchampion/docs/src/object-store.md @@ -4,5 +4,5 @@ TaskChampion also supports use of a generic key-value store to synchronize repli In this case, the salt used in key derivation is the SHA256 hash of the string "TaskChampion". -The details of the mapping from this protocol to keys an values are private to the implementation. +The details of the mapping from this protocol to keys and values are private to the implementation. Other applications should not access the key-value store directly. diff --git a/taskchampion/taskchampion/src/server/cloud/gcp.rs b/taskchampion/taskchampion/src/server/cloud/gcp.rs index 316592f72..79d326c4b 100644 --- a/taskchampion/taskchampion/src/server/cloud/gcp.rs +++ b/taskchampion/taskchampion/src/server/cloud/gcp.rs @@ -113,8 +113,7 @@ impl Service for GcpService { if existing_value.is_some() { return Ok(false); } - // Generation 0 indicates that the upload should succeed only if the object does not - // exist. + // Generation 0 indicates that the object does not yet exist. 0 } else { get_res?.generation @@ -288,7 +287,6 @@ mod tests { return; }; - // Clean up. svc.del(&pfx("testy")).unwrap(); } @@ -306,8 +304,8 @@ mod tests { // And another object that should not be included in the list. svc.put(&pfx("xxx"), b"data").unwrap(); - let got_names: Vec<_> = svc.list(&pfx("pp-")).collect::>().unwrap(); - let mut got_names: Vec<_> = got_names.into_iter().map(|oi| oi.name).collect(); + let got_objects: Vec<_> = svc.list(&pfx("pp-")).collect::>().unwrap(); + let mut got_names: Vec<_> = got_objects.into_iter().map(|oi| oi.name).collect(); got_names.sort(); assert_eq!(got_names, names); @@ -359,7 +357,6 @@ mod tests { return; }; - // Create the existing file, with two generations. svc.put(&pfx("testy"), b"foo1").unwrap(); assert!(!svc .compare_and_swap(&pfx("testy"), None, b"bar".to_vec()) diff --git a/taskchampion/taskchampion/src/server/cloud/server.rs b/taskchampion/taskchampion/src/server/cloud/server.rs index d06c18958..11407cd0e 100644 --- a/taskchampion/taskchampion/src/server/cloud/server.rs +++ b/taskchampion/taskchampion/src/server/cloud/server.rs @@ -172,7 +172,7 @@ impl CloudServer { .collect::>>() } - /// Determine the snapshot urgency. This is done probabalistically, so that approximately one + /// Determine the snapshot urgency. This is done probabilistically, so that approximately one /// in 25 sync's results in a new snapshot on a client that is not trying to avoid snapshots, /// and one in 100 on a client that is trying to avoid snapshots. fn snapshot_urgency(&self) -> Result {