diff --git a/CHANGELOG.md b/CHANGELOG.md index 6498ec0c82..e2e94226ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). below it, similar to a "scissor line" in git. When editing multiple commits, only ignore until the next `JJ: describe` line. +* Add templater support for rendering commit signatures. + ### Fixed bugs * The `$NO_COLOR` environment variable must now be non-empty to be respected. diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index c0746a01bf..db7a901e75 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -49,6 +49,9 @@ use jj_lib::revset::RevsetDiagnostics; use jj_lib::revset::RevsetModifier; use jj_lib::revset::RevsetParseContext; use jj_lib::revset::UserRevsetExpression; +use jj_lib::signing::SigStatus; +use jj_lib::signing::SignResult; +use jj_lib::signing::Verification; use jj_lib::store::Store; use once_cell::unsync::OnceCell; @@ -74,6 +77,7 @@ use crate::templater::PlainTextFormattedProperty; use crate::templater::SizeHint; use crate::templater::Template; use crate::templater::TemplateFormatter; +use crate::templater::TemplateFunction; use crate::templater::TemplateProperty; use crate::templater::TemplatePropertyError; use crate::templater::TemplatePropertyExt as _; @@ -243,6 +247,19 @@ impl<'repo> TemplateLanguage<'repo> for CommitTemplateLanguage<'repo> { let build = template_parser::lookup_method(type_name, table, function)?; build(self, diagnostics, build_ctx, property, function) } + CommitTemplatePropertyKind::CryptographicSignatureOpt(property) => { + let type_name = "CryptographicSignature"; + let table = &self.build_fn_table.cryptographic_signature_methods; + let build = template_parser::lookup_method(type_name, table, function)?; + let inner_property = property.try_unwrap(type_name); + build( + self, + diagnostics, + build_ctx, + Box::new(inner_property), + function, + ) + } } } } @@ -319,6 +336,13 @@ impl<'repo> CommitTemplateLanguage<'repo> { ) -> CommitTemplatePropertyKind<'repo> { CommitTemplatePropertyKind::TreeDiff(Box::new(property)) } + + fn wrap_cryptographic_signature_opt( + &self, + property: impl TemplateProperty> + 'repo, + ) -> CommitTemplatePropertyKind<'repo> { + CommitTemplatePropertyKind::CryptographicSignatureOpt(Box::new(property)) + } } pub enum CommitTemplatePropertyKind<'repo> { @@ -332,6 +356,9 @@ pub enum CommitTemplatePropertyKind<'repo> { CommitOrChangeId(Box + 'repo>), ShortestIdPrefix(Box + 'repo>), TreeDiff(Box + 'repo>), + CryptographicSignatureOpt( + Box> + 'repo>, + ), } impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> { @@ -347,6 +374,9 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> { CommitTemplatePropertyKind::CommitOrChangeId(_) => "CommitOrChangeId", CommitTemplatePropertyKind::ShortestIdPrefix(_) => "ShortestIdPrefix", CommitTemplatePropertyKind::TreeDiff(_) => "TreeDiff", + CommitTemplatePropertyKind::CryptographicSignatureOpt(_) => { + "Option" + } } } @@ -372,6 +402,9 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> { // TODO: boolean cast could be implemented, but explicit // diff.empty() method might be better. CommitTemplatePropertyKind::TreeDiff(_) => None, + CommitTemplatePropertyKind::CryptographicSignatureOpt(property) => { + Some(Box::new(property.map(|sig| sig.is_some()))) + } } } @@ -408,6 +441,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> { Some(property.into_template()) } CommitTemplatePropertyKind::TreeDiff(_) => None, + CommitTemplatePropertyKind::CryptographicSignatureOpt(_) => None, } } @@ -426,6 +460,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> { (CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None, (CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None, (CommitTemplatePropertyKind::TreeDiff(_), _) => None, + (CommitTemplatePropertyKind::CryptographicSignatureOpt(_), _) => None, } } @@ -447,6 +482,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> { (CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None, (CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None, (CommitTemplatePropertyKind::TreeDiff(_), _) => None, + (CommitTemplatePropertyKind::CryptographicSignatureOpt(_), _) => None, } } } @@ -463,6 +499,8 @@ pub struct CommitTemplateBuildFnTable<'repo> { pub commit_or_change_id_methods: CommitTemplateBuildMethodFnMap<'repo, CommitOrChangeId>, pub shortest_id_prefix_methods: CommitTemplateBuildMethodFnMap<'repo, ShortestIdPrefix>, pub tree_diff_methods: CommitTemplateBuildMethodFnMap<'repo, TreeDiff>, + pub cryptographic_signature_methods: + CommitTemplateBuildMethodFnMap<'repo, CryptographicSignature>, } impl<'repo> CommitTemplateBuildFnTable<'repo> { @@ -475,6 +513,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> { commit_or_change_id_methods: builtin_commit_or_change_id_methods(), shortest_id_prefix_methods: builtin_shortest_id_prefix_methods(), tree_diff_methods: builtin_tree_diff_methods(), + cryptographic_signature_methods: builtin_cryptographic_signature_methods(), } } @@ -486,6 +525,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> { commit_or_change_id_methods: HashMap::new(), shortest_id_prefix_methods: HashMap::new(), tree_diff_methods: HashMap::new(), + cryptographic_signature_methods: HashMap::new(), } } @@ -497,6 +537,7 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> { commit_or_change_id_methods, shortest_id_prefix_methods, tree_diff_methods, + cryptographic_signature_methods, } = extension; self.core.merge(core); @@ -511,6 +552,10 @@ impl<'repo> CommitTemplateBuildFnTable<'repo> { shortest_id_prefix_methods, ); merge_fn_map(&mut self.tree_diff_methods, tree_diff_methods); + merge_fn_map( + &mut self.cryptographic_signature_methods, + cryptographic_signature_methods, + ); } } @@ -621,6 +666,31 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm Ok(L::wrap_boolean(out_property)) }, ); + map.insert( + "is_signed", + |_language, _diagnostics, _build_ctx, self_property, function| { + function.expect_no_arguments()?; + let out_property = self_property.map(|commit| commit.is_signed()); + Ok(L::wrap_boolean(out_property)) + }, + ); + map.insert( + "signature", + |language, _diagnostics, _build_ctx, self_property, function| { + function.expect_no_arguments()?; + Ok( + language.wrap_cryptographic_signature_opt(TemplateFunction::new( + self_property, + |commit| { + Ok(commit + .verification() + .transpose() + .map(|verification| CryptographicSignature(verification))) + }, + )), + ) + }, + ); map.insert( "working_copies", |language, _diagnostics, _build_ctx, self_property, function| { @@ -1671,3 +1741,67 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T // TODO: add files() or map() to support custom summary-like formatting? map } + +#[derive(Debug)] +pub struct CryptographicSignature(SignResult); + +impl CryptographicSignature { + fn status(&self) -> String { + self.0 + .as_ref() + .map_or("invalid".into(), |verification| match verification.status { + SigStatus::Good => "good".into(), + SigStatus::Bad => "bad".into(), + SigStatus::Unknown => "unknown".into(), + }) + } + + fn key(&self) -> Option { + match &self.0 { + Ok(verification) => verification.key.clone(), + Err(_) => None, + } + } + + fn display(&self) -> Option { + match &self.0 { + Ok(verification) => verification.display.clone(), + Err(_) => None, + } + } +} + +pub fn builtin_cryptographic_signature_methods<'repo>( +) -> CommitTemplateBuildMethodFnMap<'repo, CryptographicSignature> { + type L<'repo> = CommitTemplateLanguage<'repo>; + // Not using maplit::hashmap!{} or custom declarative macro here because + // code completion inside macro is quite restricted. + let mut map = CommitTemplateBuildMethodFnMap::::new(); + map.insert( + "status", + |_language, _diagnostics, _build_ctx, self_property, function| { + function.expect_no_arguments()?; + let out_property = TemplateFunction::new(self_property, |sig| Ok(sig.status())); + Ok(L::wrap_string(out_property)) + }, + ); + map.insert( + "key", + |_language, _diagnostics, _build_ctx, self_property, function| { + function.expect_no_arguments()?; + let out_property = + TemplateFunction::new(self_property, |sig| Ok(sig.key().unwrap_or_default())); + Ok(L::wrap_string(out_property)) + }, + ); + map.insert( + "display", + |_language, _diagnostics, _build_ctx, self_property, function| { + function.expect_no_arguments()?; + let out_property = + TemplateFunction::new(self_property, |sig| Ok(sig.display().unwrap_or_default())); + Ok(L::wrap_string(out_property)) + }, + ); + map +} diff --git a/cli/tests/runner.rs b/cli/tests/runner.rs index a8312038a7..55a10bf59e 100644 --- a/cli/tests/runner.rs +++ b/cli/tests/runner.rs @@ -63,6 +63,7 @@ mod test_revset_output; mod test_root; mod test_shell_completion; mod test_show_command; +mod test_signature_templates; mod test_simplify_parents_command; mod test_sparse_command; mod test_split_command; diff --git a/cli/tests/test_signature_templates.rs b/cli/tests/test_signature_templates.rs new file mode 100644 index 0000000000..a7bb26620e --- /dev/null +++ b/cli/tests/test_signature_templates.rs @@ -0,0 +1,39 @@ +// Copyright 2022 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::common::TestEnvironment; + +#[test] +fn test_signature_templates() { + let test_env = TestEnvironment::default(); + + test_env.add_config(r#"signing.sign-all = true"#); + test_env.add_config(r#"signing.backend = "test""#); + + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + let template = r#"if(is_signed, + signature.status() ++ " " ++ signature.display(), + "no" + ) ++ " signature""#; + + let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T", template]); + insta::assert_snapshot!(stdout, @r" + @ good test signature + ◆ no signature"); + + let stdout = test_env.jj_cmd_success(&repo_path, &["show", "-T", template]); + insta::assert_snapshot!(stdout, @"good test signature"); +} diff --git a/docs/config.md b/docs/config.md index 9d6463f842..9289f8f380 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1027,6 +1027,28 @@ as follows: backends.ssh.allowed-signers = "/path/to/allowed-signers" ``` +## Commit Signature Verification + +By default signature verification and display is **disabled** as it incurs a +performance cost when rendering medium to large change logs. + +If you want to display commit signatures, you can use the +provided template functions, which are part of +[CommitType](./templates.md#commit-type): +* [`commit.signed()`](./templates.md#commit.signed) +* [`commit.signature()`](./templates.md#commit.signature) + +!!! warning + + [`commit.signature()`](./templates.md#commit.signature) comes with the + performance cost of verifying the signature. + If you only need to check if the commit is signed, use + [`commit.signed()`](./templates.md#commit.signed). + + +See also [CryptographicSignature Type](./templates.md#cryptographicsignature-type) +for available template methods. + ## Git settings ### Default remotes for `jj git fetch` and `jj git push` diff --git a/docs/templates.md b/docs/templates.md index 67691ce7c1..fd7aab0e6a 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -93,6 +93,9 @@ This type cannot be printed. The following methods are defined. * `parents() -> List` * `author() -> Signature` * `committer() -> Signature` +* `signature() -> Option`: + Incurs performance cost of verifying the signature. +* `signed() -> Boolean`: True if the commit is signed. * `mine() -> Boolean`: Commits where the author's email matches the email of the current user. * `working_copies() -> String`: For multi-workspace repository, indicate @@ -129,6 +132,14 @@ The following methods are defined. * `.short([len: Integer]) -> String` * `.shortest([min_len: Integer]) -> ShortestIdPrefix`: Shortest unique prefix. +### CryptographicSignature type + +The following methods are defined. + +* `.status() -> String`: The signature's status ("good", "bad", "unknown", "invalid"). +* `.key() -> String`: The signature's key id representation (for GPG, this is the key fingerprint). +* `.display() -> String`: The signature's display string (for GPG this is the formatted primary user ID. + ### Email type The following methods are defined. diff --git a/lib/src/gpg_signing.rs b/lib/src/gpg_signing.rs index 6ba6dfb2f0..7b64c2ed11 100644 --- a/lib/src/gpg_signing.rs +++ b/lib/src/gpg_signing.rs @@ -70,7 +70,7 @@ fn parse_gpg_verify_output( .next() .and_then(|bs| str::from_utf8(bs).ok()) .map(|value| value.trim().to_owned()); - Some(Verification::new(status, key, display)) + Some(Verification::new(status, key, display, Some("gpg".into()))) }) .ok_or(SignError::InvalidSignatureFormat) } @@ -223,7 +223,12 @@ mod tests { fn gpg_verify_bad_signature() { assert_eq!( parse_gpg_verify_output(b"[GNUPG:] BADSIG 123 456", true).unwrap(), - Verification::new(SigStatus::Bad, Some("123".into()), Some("456".into())) + Verification::new( + SigStatus::Bad, + Some("123".into()), + Some("456".into()), + Some("gpg".into()) + ) ); } @@ -231,7 +236,12 @@ mod tests { fn gpg_verify_unknown_signature() { assert_eq!( parse_gpg_verify_output(b"[GNUPG:] NO_PUBKEY 123", true).unwrap(), - Verification::new(SigStatus::Unknown, Some("123".into()), None) + Verification::new( + SigStatus::Unknown, + Some("123".into()), + None, + Some("gpg".into()) + ) ); } @@ -239,7 +249,12 @@ mod tests { fn gpg_verify_good_signature() { assert_eq!( parse_gpg_verify_output(b"[GNUPG:] GOODSIG 123 456", true).unwrap(), - Verification::new(SigStatus::Good, Some("123".into()), Some("456".into())) + Verification::new( + SigStatus::Good, + Some("123".into()), + Some("456".into()), + Some("gpg".into()) + ) ); } @@ -247,12 +262,22 @@ mod tests { fn gpg_verify_expired_signature() { assert_eq!( parse_gpg_verify_output(b"[GNUPG:] EXPKEYSIG 123 456", true).unwrap(), - Verification::new(SigStatus::Good, Some("123".into()), Some("456".into())) + Verification::new( + SigStatus::Good, + Some("123".into()), + Some("456".into()), + Some("gpg".into()) + ) ); assert_eq!( parse_gpg_verify_output(b"[GNUPG:] EXPKEYSIG 123 456", false).unwrap(), - Verification::new(SigStatus::Bad, Some("123".into()), Some("456".into())) + Verification::new( + SigStatus::Bad, + Some("123".into()), + Some("456".into()), + Some("gpg".into()) + ) ); } } diff --git a/lib/src/signing.rs b/lib/src/signing.rs index a8e46d083b..05af6fa8fc 100644 --- a/lib/src/signing.rs +++ b/lib/src/signing.rs @@ -53,6 +53,11 @@ pub struct Verification { /// A display string, if available. For GPG, this will be formatted primary /// user ID. pub display: Option, + /// The name of the backend that provided this verification. + /// Is `None` when no backend was found that could read the signature. + /// + /// Always set by the signer. + backend: Option, } impl Verification { @@ -63,17 +68,29 @@ impl Verification { status: SigStatus::Unknown, key: None, display: None, + backend: None, } } /// Create a new verification - pub fn new(status: SigStatus, key: Option, display: Option) -> Self { + pub fn new( + status: SigStatus, + key: Option, + display: Option, + backend: Option, + ) -> Self { Self { status, key, display, + backend, } } + + /// The name of the backend that provided this verification. + pub fn backend(&self) -> Option<&str> { + self.backend.as_deref() + } } /// The backend for signing and verifying cryptographic signatures. @@ -237,7 +254,10 @@ impl Signer { .find_map(|backend| match backend.verify(data, signature) { Ok(check) if check.status == SigStatus::Unknown => None, Err(SignError::InvalidSignatureFormat) => None, - e => Some(e), + e => Some(e.map(|mut v| { + v.backend = Some(backend.name().to_owned()); + v + })), }) .transpose()?; diff --git a/lib/src/ssh_signing.rs b/lib/src/ssh_signing.rs index e19f489fbf..88da6d5f4d 100644 --- a/lib/src/ssh_signing.rs +++ b/lib/src/ssh_signing.rs @@ -251,7 +251,12 @@ impl SigningBackend for SshBackend { Ok(_) => SigStatus::Good, Err(_) => SigStatus::Bad, }; - Ok(Verification::new(status, None, Some(principal))) + Ok(Verification::new( + status, + None, + Some(principal), + Some(self.name().into()), + )) } _ => { command @@ -269,8 +274,14 @@ impl SigningBackend for SshBackend { SigStatus::Unknown, None, Some("Signature OK. Unknown principal".into()), + Some(self.name().into()), + )), + Err(_) => Ok(Verification::new( + SigStatus::Bad, + None, + None, + Some(self.name().into()), )), - Err(_) => Ok(Verification::new(SigStatus::Bad, None, None)), } } } diff --git a/lib/src/test_signing_backend.rs b/lib/src/test_signing_backend.rs index e04adf0ec7..7caee38ddf 100644 --- a/lib/src/test_signing_backend.rs +++ b/lib/src/test_signing_backend.rs @@ -46,7 +46,7 @@ impl SigningBackend for TestSigningBackend { let hash: String = blake2b_hash(&body).encode_hex(); - Ok(format!("{PREFIX}{key}\n{hash}").into_bytes()) + Ok(format!("{PREFIX}{key}\n{hash}\n").into_bytes()) } fn verify(&self, data: &[u8], signature: &[u8]) -> SignResult { @@ -60,17 +60,19 @@ impl SigningBackend for TestSigningBackend { let sig = self.sign(data, key.as_deref())?; if sig == signature { - Ok(Verification { - status: SigStatus::Good, + Ok(Verification::new( + SigStatus::Good, key, - display: Some(self.name().into()), - }) + Some(self.name().into()), + Some(self.name().into()), + )) } else { - Ok(Verification { - status: SigStatus::Bad, + Ok(Verification::new( + SigStatus::Bad, key, - display: Some(self.name().into()), - }) + Some(self.name().into()), + Some(self.name().into()), + )) } } } diff --git a/lib/tests/test_gpg.rs b/lib/tests/test_gpg.rs index ddac993bcf..87db755cfa 100644 --- a/lib/tests/test_gpg.rs +++ b/lib/tests/test_gpg.rs @@ -134,7 +134,7 @@ fn gpg_signing_roundtrip_explicit_key() { let data = b"hello world"; let signature = backend.sign(data, Some("Someone Else")).unwrap(); - assert_debug_snapshot!(backend.verify(data, &signature).unwrap(), @r###" + assert_debug_snapshot!(backend.verify(data, &signature).unwrap(), @r#" Verification { status: Good, key: Some( @@ -143,8 +143,11 @@ fn gpg_signing_roundtrip_explicit_key() { display: Some( "Someone Else (jj test signing key) ", ), + backend: Some( + "gpg", + ), } - "###); + "#); assert_debug_snapshot!(backend.verify(b"so so bad", &signature).unwrap(), @r###" Verification { status: Bad, @@ -154,6 +157,9 @@ fn gpg_signing_roundtrip_explicit_key() { display: Some( "Someone Else (jj test signing key) ", ), + backend: Some( + "gpg", + ), } "###); } @@ -172,24 +178,30 @@ fn unknown_key() { e+U6bvqw3pOBoI53Th35drQ0qPI+jAE= =kwsk -----END PGP SIGNATURE-----"; - assert_debug_snapshot!(backend.verify(b"hello world", signature).unwrap(), @r###" + assert_debug_snapshot!(backend.verify(b"hello world", signature).unwrap(), @r#" Verification { status: Unknown, key: Some( "071FE3E324DD7333", ), display: None, + backend: Some( + "gpg", + ), } - "###); - assert_debug_snapshot!(backend.verify(b"so bad", signature).unwrap(), @r###" + "#); + assert_debug_snapshot!(backend.verify(b"so bad", signature).unwrap(), @r#" Verification { status: Unknown, key: Some( "071FE3E324DD7333", ), display: None, + backend: Some( + "gpg", + ), } - "###); + "#); } #[test] diff --git a/lib/tests/test_signing.rs b/lib/tests/test_signing.rs index 4989f7ff41..302bdab832 100644 --- a/lib/tests/test_signing.rs +++ b/lib/tests/test_signing.rs @@ -45,11 +45,12 @@ fn someone_else() -> Signature { } fn good_verification() -> Option { - Some(Verification { - status: SigStatus::Good, - key: Some("impeccable".to_owned()), - display: Some("test".into()), - }) + Some(Verification::new( + SigStatus::Good, + Some("impeccable".to_owned()), + Some("test".into()), + Some("test".into()), + )) } #[test_case(TestRepoBackend::Local ; "local backend")] diff --git a/lib/tests/test_ssh_signing.rs b/lib/tests/test_ssh_signing.rs index 83d75dba05..c082947afc 100644 --- a/lib/tests/test_ssh_signing.rs +++ b/lib/tests/test_ssh_signing.rs @@ -37,14 +37,14 @@ y2yxhhHnagH52avUqw5hAAAAAAECAwQF static PUBLIC_KEY: &str = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIGj+J6N6SO+4P8dOZqfR1oiay2yxhhHnagH52avUqw5h"; -struct SshEnvironment { +pub struct SshEnvironment { _keys: tempfile::TempDir, - private_key_path: PathBuf, - allowed_signers: Option, + pub private_key_path: PathBuf, + pub allowed_signers: Option, } impl SshEnvironment { - fn new() -> Result { + pub fn new() -> Result { let keys_dir = tempfile::Builder::new() .prefix("jj-test-signing-keys-") .tempdir()