Skip to content

Commit

Permalink
sign: Add templater methods to show signature info
Browse files Browse the repository at this point in the history
Disclaimer: this is the work of @necauqua and @julienvincent (see
#3141). I simply materialized the changes by rebasing them on latest
`main` and making the necessary adjustments to pass CI.

---

I had to fix an issue in `TestSignatureBackend::sign()`.

The following test was failing:
```
---- test_signature_templates::test_signature_templates stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot: signature_templates
Source: cli/tests/test_signature_templates.rs:28
────────────────────────────────────────────────────────────────────────────────
Expression: stdout
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    0     0 │ @  Commit ID: 05ac066d05701071af20e77506a0f2195194cbc9
    1     1 │ │  Change ID: qpvuntsmwlqtpsluzzsnyyzlmlwvmlnu
    2     2 │ │  Author: Test User <[email protected]> (2001-02-03 08:05:07)
    3     3 │ │  Committer: Test User <[email protected]> (2001-02-03 08:05:07)
    4       │-│  Signature: Good test signature
          4 │+│  Signature: Bad test signature
    5     5 │ │
    6     6 │ │      (no description set)
    7     7 │ │
    8     8 │ ◆  Commit ID: 0000000000000000000000000000000000000000
────────────┴───────────────────────────────────────────────────────────────────
```

Print debugging revealed that the signature was bad, because of a
missing trailing `\n` in `TestSignatureBackend::sign()`.

```diff
diff --git a/lib/src/test_signing_backend.rs b/lib/src/test_signing_backend.rs
index d47fef1086..0ba249e358 100644
--- a/lib/src/test_signing_backend.rs
+++ b/lib/src/test_signing_backend.rs
@@ -59,6 +59,8 @@
         let key = (!key.is_empty()).then_some(std::str::from_utf8(key).unwrap().to_owned());

         let sig = self.sign(data, key.as_deref())?;
+        dbg!(&std::str::from_utf8(&signature).unwrap());
+        dbg!(&std::str::from_utf8(&sig).unwrap());
         if sig == signature {
             Ok(Verification::new(
                 SigStatus::Good,
```

```
[lib/src/test_signing_backend.rs:62:9] &std::str::from_utf8(&signature).unwrap() = \"--- JJ-TEST-SIGNATURE ---\\nKEY: \\n5300977ff3ecda4555bd86d383b070afac7b7459c07f762af918943975394a8261d244629e430c8554258904f16dd9c18d737f8969f2e7d849246db0d93cc004\\n\"
[lib/src/test_signing_backend.rs:63:9] &std::str::from_utf8(&sig).unwrap() = \"--- JJ-TEST-SIGNATURE ---\\nKEY: \\n5300977ff3ecda4555bd86d383b070afac7b7459c07f762af918943975394a8261d244629e430c8554258904f16dd9c18d737f8969f2e7d849246db0d93cc004\"
```

Thankfully, @yuja pointed out that libgit2 appends a trailing newline
(see bfb7613).

Co-authored-by: necauqua <[email protected]>
Co-authored-by: julienvincent <[email protected]>
  • Loading branch information
2 people authored and pylbrecht committed Jan 1, 2025
1 parent ea06d56 commit e502a68
Show file tree
Hide file tree
Showing 12 changed files with 292 additions and 30 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
123 changes: 123 additions & 0 deletions cli/src/commit_templater.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -243,6 +246,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,
)
}
}
}
}
Expand Down Expand Up @@ -319,6 +335,12 @@ impl<'repo> CommitTemplateLanguage<'repo> {
) -> CommitTemplatePropertyKind<'repo> {
CommitTemplatePropertyKind::TreeDiff(Box::new(property))
}

fn wrap_cryptographic_signature_opt(
property: impl TemplateProperty<Output = Option<CryptographicSignature>> + 'repo,
) -> CommitTemplatePropertyKind<'repo> {
CommitTemplatePropertyKind::CryptographicSignatureOpt(Box::new(property))
}
}

pub enum CommitTemplatePropertyKind<'repo> {
Expand All @@ -332,6 +354,9 @@ pub enum CommitTemplatePropertyKind<'repo> {
CommitOrChangeId(Box<dyn TemplateProperty<Output = CommitOrChangeId> + 'repo>),
ShortestIdPrefix(Box<dyn TemplateProperty<Output = ShortestIdPrefix> + 'repo>),
TreeDiff(Box<dyn TemplateProperty<Output = TreeDiff> + 'repo>),
CryptographicSignatureOpt(
Box<dyn TemplateProperty<Output = Option<CryptographicSignature>> + 'repo>,
),
}

impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
Expand All @@ -347,6 +372,9 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
CommitTemplatePropertyKind::CommitOrChangeId(_) => "CommitOrChangeId",
CommitTemplatePropertyKind::ShortestIdPrefix(_) => "ShortestIdPrefix",
CommitTemplatePropertyKind::TreeDiff(_) => "TreeDiff",
CommitTemplatePropertyKind::CryptographicSignatureOpt(_) => {
"Option<CryptographicSignature>"
}
}
}

Expand All @@ -372,6 +400,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())))
}
}
}

Expand Down Expand Up @@ -408,6 +439,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
Some(property.into_template())
}
CommitTemplatePropertyKind::TreeDiff(_) => None,
CommitTemplatePropertyKind::CryptographicSignatureOpt(_) => None,
}
}

Expand All @@ -426,6 +458,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
(CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None,
(CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None,
(CommitTemplatePropertyKind::TreeDiff(_), _) => None,
(CommitTemplatePropertyKind::CryptographicSignatureOpt(_), _) => None,
}
}

Expand All @@ -447,6 +480,7 @@ impl<'repo> IntoTemplateProperty<'repo> for CommitTemplatePropertyKind<'repo> {
(CommitTemplatePropertyKind::CommitOrChangeId(_), _) => None,
(CommitTemplatePropertyKind::ShortestIdPrefix(_), _) => None,
(CommitTemplatePropertyKind::TreeDiff(_), _) => None,
(CommitTemplatePropertyKind::CryptographicSignatureOpt(_), _) => None,
}
}
}
Expand All @@ -463,6 +497,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> {
Expand All @@ -475,6 +511,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(),
}
}

Expand All @@ -486,6 +523,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(),
}
}

Expand All @@ -497,6 +535,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);
Expand All @@ -511,6 +550,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,
);
}
}

Expand Down Expand Up @@ -621,6 +664,14 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm
Ok(L::wrap_boolean(out_property))
},
);
map.insert(
"signature",
|_language, _diagnostics, _build_ctx, self_property, function| {
function.expect_no_arguments()?;
let out_property = self_property.map(CryptographicSignature::new);
Ok(L::wrap_cryptographic_signature_opt(out_property))
},
);
map.insert(
"working_copies",
|language, _diagnostics, _build_ctx, self_property, function| {
Expand Down Expand Up @@ -1671,3 +1722,75 @@ 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 {
commit: Commit,
}

impl CryptographicSignature {
fn new(commit: Commit) -> Option<Self> {
commit.is_signed().then_some(Self { commit })
}

fn verify(&self) -> SignResult<Verification> {
self.commit
.verification()
.transpose()
.expect("must have signature")
}

fn status(&self) -> SignResult<SigStatus> {
self.verify().map(|verification| verification.status)
}

fn key(&self) -> Option<String> {
self.verify().map_or(None, |verification| verification.key)
}

fn display(&self) -> Option<String> {
self.verify()
.map_or(None, |verification| verification.display)
}
}

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::<CryptographicSignature>::new();
map.insert(
"status",
|_language, _diagnostics, _build_ctx, self_property, function| {
function.expect_no_arguments()?;
let out_property = self_property.map(|sig| {
let status = match sig.status() {
Ok(SigStatus::Good) => "good",
Ok(SigStatus::Bad) => "bad",
Ok(SigStatus::Unknown) => "unknown",
Err(_) => "invalid",
};
status.to_string()
});
Ok(L::wrap_string(out_property))
},
);
map.insert(
"key",
|_language, _diagnostics, _build_ctx, self_property, function| {
function.expect_no_arguments()?;
let out_property = self_property.map(|sig| 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 = self_property.map(|sig| sig.display().unwrap_or_default());
Ok(L::wrap_string(out_property))
},
);
map
}
1 change: 1 addition & 0 deletions cli/tests/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
39 changes: 39 additions & 0 deletions cli/tests/test_signature_templates.rs
Original file line number Diff line number Diff line change
@@ -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(signature,
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");
}
11 changes: 11 additions & 0 deletions docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,17 @@ 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 in your templates, you can use
`commit.signature()` (see [Commit type](./templates.md#commit-type)). The
returned [CryptographicSignature
Type](./templates.md#cryptographicsignature-type) provides methods to retrieve
signature details.

## Git settings

### Default remotes for `jj git fetch` and `jj git push`
Expand Down
15 changes: 15 additions & 0 deletions docs/templates.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ This type cannot be printed. The following methods are defined.
* `parents() -> List<Commit>`
* `author() -> Signature`
* `committer() -> Signature`
* `signature() -> Option<CryptographicSignature>`
* `mine() -> Boolean`: Commits where the author's email matches the email of the current
user.
* `working_copies() -> String`: For multi-workspace repository, indicate
Expand Down Expand Up @@ -129,6 +130,20 @@ 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.

!!! warning

Calling any of `.status()`, `.key()`, or `.display()` is slow, as it incurs
the performance cost of verifying the signature. Though consecutive calls
will be faster, because the backend caches the verification result.

### Email type

The following methods are defined.
Expand Down
Loading

0 comments on commit e502a68

Please sign in to comment.