From 0677a2057f3c788e32e46ac4e9516a59eb90c726 Mon Sep 17 00:00:00 2001 From: Anton Bulakh Date: Sun, 12 Nov 2023 03:40:23 +0200 Subject: [PATCH] sign: Implement generic commit signing on the backend --- cli/examples/custom-backend/main.rs | 10 ++- lib/src/backend.rs | 15 +++- lib/src/git_backend.rs | 115 +++++++++++++++++++++++----- lib/src/local_backend.rs | 30 +++++--- lib/src/store.rs | 2 +- lib/testutils/src/test_backend.rs | 16 +++- 6 files changed, 153 insertions(+), 35 deletions(-) diff --git a/cli/examples/custom-backend/main.rs b/cli/examples/custom-backend/main.rs index bd55d5f31c..3827f59c2c 100644 --- a/cli/examples/custom-backend/main.rs +++ b/cli/examples/custom-backend/main.rs @@ -21,7 +21,7 @@ use jj_cli::cli_util::{CliRunner, CommandError, CommandHelper}; use jj_cli::ui::Ui; use jj_lib::backend::{ Backend, BackendInitError, BackendLoadError, BackendResult, ChangeId, Commit, CommitId, - Conflict, ConflictId, FileId, SymlinkId, Tree, TreeId, + Conflict, ConflictId, FileId, SigningFn, SymlinkId, Tree, TreeId, }; use jj_lib::git_backend::GitBackend; use jj_lib::repo::StoreFactories; @@ -160,7 +160,11 @@ impl Backend for JitBackend { self.inner.read_commit(id).await } - fn write_commit(&self, contents: Commit) -> BackendResult<(CommitId, Commit)> { - self.inner.write_commit(contents) + fn write_commit( + &self, + contents: Commit, + sign_with: Option, + ) -> BackendResult<(CommitId, Commit)> { + self.inner.write_commit(contents, sign_with) } } diff --git a/lib/src/backend.rs b/lib/src/backend.rs index 0755deb128..5bf3aadaff 100644 --- a/lib/src/backend.rs +++ b/lib/src/backend.rs @@ -463,6 +463,8 @@ pub fn make_root_commit(root_change_id: ChangeId, empty_tree_id: TreeId) -> Comm } } +pub type SigningFn = Box BackendResult>>; + #[async_trait] pub trait Backend: Send + Sync + Debug { fn as_any(&self) -> &dyn Any; @@ -518,5 +520,16 @@ pub trait Backend: Send + Sync + Debug { /// committer name to an authenticated user's name, or the backend's /// timestamps may have less precision than the millisecond precision in /// `Commit`. - fn write_commit(&self, contents: Commit) -> BackendResult<(CommitId, Commit)>; + /// + /// The `sign_with` parameter could contain a function to cryptographically + /// sign some binary representation of the commit. + /// If the backend supports it, it could call it and store the result in + /// an implementation specific fashion, and both `read_commit` and the + /// return of `write_commit` should read it back as the `secure_sig` + /// field. + fn write_commit( + &self, + contents: Commit, + sign_with: Option, + ) -> BackendResult<(CommitId, Commit)>; } diff --git a/lib/src/git_backend.rs b/lib/src/git_backend.rs index f782633794..e941e87f8c 100644 --- a/lib/src/git_backend.rs +++ b/lib/src/git_backend.rs @@ -22,9 +22,9 @@ use std::sync::{Arc, Mutex, MutexGuard}; use std::{fs, str}; use async_trait::async_trait; -use gix::objs::CommitRefIter; +use gix::objs::{CommitRefIter, WriteTo}; +use gix::refs::transaction::{Change, LogChange, PreviousValue, RefEdit, RefLog}; use gix::refs::Target; -use gix::refs::transaction::{RefEdit, Change, LogChange, RefLog, PreviousValue}; use itertools::Itertools; use prost::Message; use smallvec::SmallVec; @@ -33,8 +33,8 @@ use thiserror::Error; use crate::backend::{ make_root_commit, Backend, BackendError, BackendInitError, BackendLoadError, BackendResult, ChangeId, Commit, CommitId, Conflict, ConflictId, ConflictTerm, FileId, MergedTreeId, - MillisSinceEpoch, ObjectId, SecureSig, Signature, SymlinkId, Timestamp, Tree, TreeId, - TreeValue, + MillisSinceEpoch, ObjectId, SecureSig, Signature, SigningFn, SymlinkId, Timestamp, Tree, + TreeId, TreeValue, }; use crate::file_util::{IoResultExt as _, PathError}; use crate::lock::FileLock; @@ -946,7 +946,13 @@ impl Backend for GitBackend { Ok(commit) } - fn write_commit(&self, mut contents: Commit) -> BackendResult<(CommitId, Commit)> { + fn write_commit( + &self, + mut contents: Commit, + mut sign_with: Option, + ) -> BackendResult<(CommitId, Commit)> { + assert!(contents.secure_sig.is_none(), "commit.secure_sig was set"); + let locked_repo = self.lock_git_repo(); let git_tree_id = match &contents.root_tree { MergedTreeId::Legacy(tree_id) => validate_git_object_id(tree_id)?, @@ -963,7 +969,7 @@ impl Backend for GitBackend { "Cannot write a commit with no parents".into(), )); } - let mut parents = vec![]; + let mut parents = SmallVec::new(); for parent_id in &contents.parents { if *parent_id == self.root_commit_id { // Git doesn't have a root commit, so if the parent is the root commit, we don't @@ -981,7 +987,6 @@ impl Backend for GitBackend { parents.push(validate_git_object_id(parent_id)?); } } - let parents: SmallVec<[gix::ObjectId; 1]> = parents.into(); let extras = serialize_extras(&contents); // If two writers write commits of the same id with different metadata, they @@ -992,7 +997,7 @@ impl Backend for GitBackend { // repository is rsync-ed. let (table, table_lock) = self.read_extra_metadata_table_locked()?; let (git_id, commit) = loop { - let commit = gix::objs::Commit { + let mut commit = gix::objs::Commit { message: message.to_owned().into(), tree: git_tree_id, author: author.into(), @@ -1002,7 +1007,17 @@ impl Backend for GitBackend { extra_headers: Default::default(), }; - // todo sign commits here + if let Some(sign) = &mut sign_with { + // we don't use gix pool, but at least use their heuristic + let mut data = Vec::with_capacity(512); + commit.write_to(&mut data).unwrap(); + + let sig = sign(&data)?; + commit + .extra_headers + .push(("gpgsig".into(), sig.clone().into())); + contents.secure_sig = Some(SecureSig { data, sig }); + } let git_id = locked_repo @@ -1153,11 +1168,13 @@ fn bytes_vec_from_json(value: &serde_json::Value) -> Vec { mod tests { use assert_matches::assert_matches; use git2::Oid; + use hex::ToHex; use pollster::FutureExt; use test_case::test_case; use super::*; use crate::backend::{FileId, MillisSinceEpoch}; + use crate::content_hash::blake2b_hash; #[test_case(false; "legacy tree format")] #[test_case(true; "tree-level conflict format")] @@ -1477,13 +1494,13 @@ mod tests { // No parents commit.parents = vec![]; assert_matches!( - backend.write_commit(commit.clone()), + backend.write_commit(commit.clone(), None), Err(BackendError::Other(err)) if err.to_string().contains("no parents") ); // Only root commit as parent commit.parents = vec![backend.root_commit_id().clone()]; - let first_id = backend.write_commit(commit.clone()).unwrap().0; + let first_id = backend.write_commit(commit.clone(), None).unwrap().0; let first_commit = backend.read_commit(&first_id).block_on().unwrap(); assert_eq!(first_commit, commit); let first_git_commit = git_repo.find_commit(git_id(&first_id)).unwrap(); @@ -1491,7 +1508,7 @@ mod tests { // Only non-root commit as parent commit.parents = vec![first_id.clone()]; - let second_id = backend.write_commit(commit.clone()).unwrap().0; + let second_id = backend.write_commit(commit.clone(), None).unwrap().0; let second_commit = backend.read_commit(&second_id).block_on().unwrap(); assert_eq!(second_commit, commit); let second_git_commit = git_repo.find_commit(git_id(&second_id)).unwrap(); @@ -1502,7 +1519,7 @@ mod tests { // Merge commit commit.parents = vec![first_id.clone(), second_id.clone()]; - let merge_id = backend.write_commit(commit.clone()).unwrap().0; + let merge_id = backend.write_commit(commit.clone(), None).unwrap().0; let merge_commit = backend.read_commit(&merge_id).block_on().unwrap(); assert_eq!(merge_commit, commit); let merge_git_commit = git_repo.find_commit(git_id(&merge_id)).unwrap(); @@ -1514,7 +1531,7 @@ mod tests { // Merge commit with root as one parent commit.parents = vec![first_id, backend.root_commit_id().clone()]; assert_matches!( - backend.write_commit(commit), + backend.write_commit(commit, None), Err(BackendError::Other(err)) if err.to_string().contains("root commit") ); } @@ -1554,7 +1571,7 @@ mod tests { // When writing a tree-level conflict, the root tree on the git side has the // individual trees as subtrees. - let read_commit_id = backend.write_commit(commit.clone()).unwrap().0; + let read_commit_id = backend.write_commit(commit.clone(), None).unwrap().0; let read_commit = backend.read_commit(&read_commit_id).block_on().unwrap(); assert_eq!(read_commit, commit); let git_commit = git_repo @@ -1598,7 +1615,7 @@ mod tests { // When writing a single tree using the new format, it's represented by a // regular git tree. commit.root_tree = MergedTreeId::resolved(create_tree(5)); - let read_commit_id = backend.write_commit(commit.clone()).unwrap().0; + let read_commit_id = backend.write_commit(commit.clone(), None).unwrap().0; let read_commit = backend.read_commit(&read_commit_id).block_on().unwrap(); assert_eq!(read_commit, commit); let git_commit = git_repo @@ -1633,7 +1650,7 @@ mod tests { committer: signature, secure_sig: None, }; - let commit_id = backend.write_commit(commit).unwrap().0; + let commit_id = backend.write_commit(commit, None).unwrap().0; let git_refs = backend .open_git_repo() .unwrap() @@ -1663,11 +1680,11 @@ mod tests { // second after the epoch, so the timestamp adjustment can remove 1 // second and it will still be nonnegative commit1.committer.timestamp.timestamp = MillisSinceEpoch(1000); - let (commit_id1, mut commit2) = backend.write_commit(commit1).unwrap(); + let (commit_id1, mut commit2) = backend.write_commit(commit1, None).unwrap(); commit2.predecessors.push(commit_id1.clone()); // `write_commit` should prevent the ids from being the same by changing the // committer timestamp of the commit it actually writes. - let (commit_id2, mut actual_commit2) = backend.write_commit(commit2.clone()).unwrap(); + let (commit_id2, mut actual_commit2) = backend.write_commit(commit2.clone(), None).unwrap(); // The returned matches the ID assert_eq!( backend.read_commit(&commit_id2).block_on().unwrap(), @@ -1685,6 +1702,66 @@ mod tests { assert_eq!(actual_commit2, commit2); } + #[test] + fn write_signed_commit() { + let settings = user_settings(); + let temp_dir = testutils::new_temp_dir(); + let backend = GitBackend::init_internal(&settings, temp_dir.path()).unwrap(); + + let commit = Commit { + parents: vec![backend.root_commit_id().clone()], + predecessors: vec![], + root_tree: MergedTreeId::Legacy(backend.empty_tree_id().clone()), + change_id: ChangeId::new(vec![]), + description: "initial".to_string(), + author: create_signature(), + committer: create_signature(), + secure_sig: None, + }; + + let signer = Box::new(|data: &_| { + let hash: String = blake2b_hash(data).encode_hex(); + Ok(format!("test sig\n\n\nhash={hash}").into_bytes()) + }); + + let (id, commit) = backend.write_commit(commit, Some(signer)).unwrap(); + + let git_repo = backend.git_repo(); + let obj = git_repo.find_object(id.as_bytes()).unwrap(); + insta::assert_snapshot!(std::str::from_utf8(&obj.data).unwrap(), @r###" + tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 + author Someone 0 +0000 + committer Someone 0 +0000 + gpgsig test sig + + + hash=9ad9526c3b2103c41a229f2f3c82d107a0ecd902f476a855f0e1dd5f7bef1430663de12749b73e293a877113895a8a2a0f29da4bbc5a5f9a19c3523fb0e53518 + + initial + "###); + + let returned_sig = commit.secure_sig.expect("failed to return the signature"); + + let commit = backend.read_commit(&id).block_on().unwrap(); + + let sig = commit.secure_sig.expect("failed to read the signature"); + assert_eq!(&sig, &returned_sig); + + insta::assert_snapshot!(std::str::from_utf8(&sig.sig).unwrap(), @r###" + test sig + + + hash=9ad9526c3b2103c41a229f2f3c82d107a0ecd902f476a855f0e1dd5f7bef1430663de12749b73e293a877113895a8a2a0f29da4bbc5a5f9a19c3523fb0e53518 + "###); + insta::assert_snapshot!(std::str::from_utf8(&sig.data).unwrap(), @r###" + tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904 + author Someone 0 +0000 + committer Someone 0 +0000 + + initial + "###); + } + fn git_id(commit_id: &CommitId) -> Oid { Oid::from_bytes(commit_id.as_bytes()).unwrap() } diff --git a/lib/src/local_backend.rs b/lib/src/local_backend.rs index 0be59169b2..8f01e84c9c 100644 --- a/lib/src/local_backend.rs +++ b/lib/src/local_backend.rs @@ -29,7 +29,7 @@ use tempfile::NamedTempFile; use crate::backend::{ make_root_commit, Backend, BackendError, BackendResult, ChangeId, Commit, CommitId, Conflict, ConflictId, ConflictTerm, FileId, MergedTreeId, MillisSinceEpoch, ObjectId, SecureSig, - Signature, SymlinkId, Timestamp, Tree, TreeId, TreeValue, + Signature, SigningFn, SymlinkId, Timestamp, Tree, TreeId, TreeValue, }; use crate::content_hash::blake2b_hash; use crate::file_util::persist_content_addressed_temp_file; @@ -264,7 +264,13 @@ impl Backend for LocalBackend { Ok(commit_from_proto(proto)) } - fn write_commit(&self, commit: Commit) -> BackendResult<(CommitId, Commit)> { + fn write_commit( + &self, + mut commit: Commit, + sign_with: Option, + ) -> BackendResult<(CommitId, Commit)> { + assert!(commit.secure_sig.is_none(), "commit.secure_sig was set"); + if commit.parents.is_empty() { return Err(BackendError::Other( "Cannot write a commit with no parents".into(), @@ -272,7 +278,14 @@ impl Backend for LocalBackend { } let temp_file = NamedTempFile::new_in(&self.path).map_err(to_other_err)?; - let proto = commit_to_proto(&commit); + let mut proto = commit_to_proto(&commit); + if let Some(mut sign) = sign_with { + let data = proto.encode_to_vec(); + let sig = sign(&data)?; + proto.secure_sig = Some(sig.clone()); + commit.secure_sig = Some(SecureSig { data, sig }); + } + temp_file .as_file() .write_all(&proto.encode_to_vec()) @@ -307,7 +320,6 @@ pub fn commit_to_proto(commit: &Commit) -> crate::protos::local_store::Commit { proto.description = commit.description.clone(); proto.author = Some(signature_to_proto(&commit.author)); proto.committer = Some(signature_to_proto(&commit.committer)); - proto.secure_sig = commit.secure_sig.as_ref().map(|s| s.sig.clone()); proto } @@ -500,31 +512,31 @@ mod tests { // No parents commit.parents = vec![]; assert_matches!( - backend.write_commit(commit.clone()), + backend.write_commit(commit.clone(), None), Err(BackendError::Other(err)) if err.to_string().contains("no parents") ); // Only root commit as parent commit.parents = vec![backend.root_commit_id().clone()]; - let first_id = backend.write_commit(commit.clone()).unwrap().0; + let first_id = backend.write_commit(commit.clone(), None).unwrap().0; let first_commit = backend.read_commit(&first_id).block_on().unwrap(); assert_eq!(first_commit, commit); // Only non-root commit as parent commit.parents = vec![first_id.clone()]; - let second_id = backend.write_commit(commit.clone()).unwrap().0; + let second_id = backend.write_commit(commit.clone(), None).unwrap().0; let second_commit = backend.read_commit(&second_id).block_on().unwrap(); assert_eq!(second_commit, commit); // Merge commit commit.parents = vec![first_id.clone(), second_id.clone()]; - let merge_id = backend.write_commit(commit.clone()).unwrap().0; + let merge_id = backend.write_commit(commit.clone(), None).unwrap().0; let merge_commit = backend.read_commit(&merge_id).block_on().unwrap(); assert_eq!(merge_commit, commit); // Merge commit with root as one parent commit.parents = vec![first_id, backend.root_commit_id().clone()]; - let root_merge_id = backend.write_commit(commit.clone()).unwrap().0; + let root_merge_id = backend.write_commit(commit.clone(), None).unwrap().0; let root_merge_commit = backend.read_commit(&root_merge_id).block_on().unwrap(); assert_eq!(root_merge_commit, commit); } diff --git a/lib/src/store.rs b/lib/src/store.rs index 832c2c381b..69d1f73dd6 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -126,7 +126,7 @@ impl Store { pub fn write_commit(self: &Arc, commit: backend::Commit) -> BackendResult { assert!(!commit.parents.is_empty()); - let (commit_id, commit) = self.backend.write_commit(commit)?; + let (commit_id, commit) = self.backend.write_commit(commit, None)?; let data = Arc::new(commit); { let mut write_locked_cache = self.commit_cache.write().unwrap(); diff --git a/lib/testutils/src/test_backend.rs b/lib/testutils/src/test_backend.rs index f0d263fc14..f4e1c62a1e 100644 --- a/lib/testutils/src/test_backend.rs +++ b/lib/testutils/src/test_backend.rs @@ -22,7 +22,7 @@ use std::sync::{Arc, Mutex, MutexGuard, OnceLock}; use async_trait::async_trait; use jj_lib::backend::{ make_root_commit, Backend, BackendError, BackendResult, ChangeId, Commit, CommitId, Conflict, - ConflictId, FileId, ObjectId, SymlinkId, Tree, TreeId, + ConflictId, FileId, ObjectId, SecureSig, SigningFn, SymlinkId, Tree, TreeId, }; use jj_lib::repo_path::RepoPath; @@ -273,7 +273,19 @@ impl Backend for TestBackend { } } - fn write_commit(&self, contents: Commit) -> BackendResult<(CommitId, Commit)> { + fn write_commit( + &self, + mut contents: Commit, + mut sign_with: Option, + ) -> BackendResult<(CommitId, Commit)> { + assert!(contents.secure_sig.is_none(), "commit.secure_sig was set"); + + if let Some(sign) = &mut sign_with { + let data = format!("{contents:?}").into_bytes(); + let sig = sign(&data)?; + contents.secure_sig = Some(SecureSig { data, sig }); + } + let id = CommitId::new(get_hash(&contents)); self.locked_data() .commits