Display commit signatures

[pylbrecht]

These changes are continuing the work of #3141.
I took over its changes and rebased them on latest main.

This work is required for #4747, as it lays the foundation for writing CLI tests for jj sign.

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added tests to cover my changes

[bnjmnt4n]

Is there anything blocking this PR? It seems to be in a functional state.

[pylbrecht]

Is there anything blocking this PR? It seems to be in a functional state.

Well, since I simply materialized the changes from @julienvincent and @necauqua, there are still two things missing:

• printing hints about dropped commit signatures 9eaea83
• docs 739ddf2

Regarding printing hints about dropped signatures, I reached out for help (#4712 (comment), #4712 (comment)).

Maybe we could merge without the hints (and create a new issue to track that missing piece to be implemented later)? Then I would focus on getting the docs ready.

[bnjmnt4n]

Based on the PR title, I would presume that this PR is focused mainly on being able to display commit signatures. So it seems fine to me if printing hints about dropped commit signatures is shifted to a different issue/PR, since it isn't directly related to displaying commit signatures.

[pylbrecht]

Sounds good to me.
I will get the PR ready and if nobody objects until then, we can merge without the hints.

[pylbrecht]

I pushed a slightly modified version of 739ddf2. However, I believe it was planned to add a builtin_log_compact_with_sig, which looks like this for good signatures (fancy checkmarks):

    Rebased 1 descendant commits
    Working copy now at: rlvkpnrz b162855d (empty) (no description set)
    Parent commit      : qpvuntsm [✓︎] 5aab9df2 (empty) init
    Commit was signed: qpvuntsm [✓︎] 5aab9df2 (empty) init

https://github.com/jj-vcs/jj/pull/3142/files#diff-edf6d251f71a7d4a3b3b8cf73345ea16e6dc7691ed1b98a1811d5709cbe011d2R47-R50)

@necauqua, originally this was your work, I think. Was it planned to add builtin_log_compact_with_sig?
(I cross-read a bunch of issues / PRs in the past, but I am too tired right now to recollect the facts coming out of all the discussions.)

[pylbrecht]

@julienvincent, I added you as co-author to the commits.
However the email from your Github profile does not seem to be the one you used for Google's CLA, which makes the cla/google job fail on CI.

Would you like to provide the email address you used to sign the CLA, so I can add you as a co-author?

[julienvincent]

Thanks for attribution - I just went ahead and signed the CLA for the email address you used.

I reran the check which is now passing 👍🏻

[yuja]

What I did not know is that .key() and .display() both require the slow (but cached) verification. Now the implementation feels a little repetitive, but maybe it has to be?

Yeah, there might be a better way to organize the verification API, but I have no idea. @necauqua ?

I think that's good as a start, btw.

[necauqua]

@necauqua ?

With gpg you have to verify to get any info, not sure about ssh, but probably same

The only thing you (kind of obviously) get for free is checking if there's a signature at all

With that in mind maybe it should be something like:

# cheap
commit.is_signed() -> Boolean
# does the verification, slow
commit.signature() -> Option<CryptographicSignature>
sig.status() -> Good|Bad|Unknown(|Invalid?)
sig.display()
sig.key()

I vaguely recall that in the code the verification result is the thing that contains key/display/status, so we just reflect that structure

[pylbrecht]

Thanks for your feedback, @yuja and @necauqua.

I implemented it as suggested, with the exception of using commit.signed() instead of commit.is_signed().
There were no other is_...() template functions, so I felt .signed() would be more consistent with what we already have (e.g. divergent(), immutable(), etc.).

If you disagree, I am happy to change it back to .is_signed(). No strong feelings whatsoever!

[yuja]

commit.signed() instead of commit.is_signed()

Isn't if(commit.signed(), ..) equivalent to if(commit.signature(), ..)? (Option type can be casted to boolean in templater.) About the naming, I agree.

I'll take a look maybe tomorrow, thanks.

[necauqua]

equivalent

The point is that if there is a signature then the .signature() has to do full verification to get the signature object, I did see that we're duplicating the meaning a bit with this separate boolean.

Technically we could probably make the Option => Boolean coersion avoid computing the content of the option somehow, that would be a cool optimisation.

OTOH having a separate boolean thing that's guaranteed to be cheap without magic may be nicer.
But I love the lazy option coersion idea.

also yeah agree with naming, forgot about it :)

[pylbrecht]

The point is that if there is a signature then the .signature() has to do full verification to get the signature object, I did see that we're duplicating the meaning a bit with this separate boolean.

Exactly this.

Technically we could probably make the Option => Boolean coersion avoid computing the content of the option somehow, that would be a cool optimisation.

That's what I have been thinking about for the past hour. But now my head hurts and I still have no idea how to implement that.
It would certainly be the way I would like it to be in terms of ergonomics.

[yuja]

equivalent

The point is that if there is a signature then the .signature() has to do full verification to get the signature object, I did see that we're duplicating the meaning a bit with this separate boolean.

Oh, I thought .signature() would return a lazy wrapper CryptographicSignature(Commit), and .signature().<whatever>() would invoke the underlying verify() method.

Maybe I misread #4853 (comment) that we were discussing about the library API.

[pylbrecht]

Oh, I thought .signature() would return a lazy wrapper CryptographicSignature(Commit), and .signature().<whatever>() would invoke the underlying verify() method.

That's how I implemented it at first, but that resulted in the repitition I mentioned in #4853 (comment) (i.e. signature().verify(), signature().key(), and signature().display() all having to call Commit::verification()).

If it's fine that every signature().<whatever>() method calls Commit::verification(), I am happy to go back to the lazy wrapper CryptographicSignature(Commit).

[yuja]

Oh, I thought .signature() would return a lazy wrapper CryptographicSignature(Commit), and .signature().<whatever>() would invoke the underlying verify() method.

That's how I implemented it at first, but that resulted in the repitition I mentioned in #4853 (comment) (i.e. signature().verify(), signature().key(), and signature().display() all having to call Commit::verification()).

Yes, and I think that's okay as a start. (Regarding the method name, .status() might be better because they all do the verification under the hood.)

We could cache the result in that object (by OnceCell), but it wouldn't help because templater doesn't provide syntax to call multiple methods on the same object. Since the verification result is cached anyway by the backend, we wouldn't have to pay extra effort in templater part.

[pylbrecht]

Oh, I thought .signature() would return a lazy wrapper CryptographicSignature(Commit), and .signature().<whatever>() would invoke the underlying verify() method.

That's how I implemented it at first, but that resulted in the repitition I mentioned in #4853 (comment) (i.e. signature().verify(), signature().key(), and signature().display() all having to call Commit::verification()).

Yes, and I think that's okay as a start. (Regarding the method name, .status() might be better because they all do the verification under the hood.)

Great, then I will "rollback" to that implementation (probably later today).

We could cache the result in that object (by OnceCell), but it wouldn't help because templater doesn't provide syntax to call multiple methods on the same object. Since the verification result is cached anyway by the backend, we wouldn't have to pay extra effort in templater part.

Thanks for elaborating on this!
I also thought about caching the verification within CryptographicSignature, but wasn't sure whether the templater wouldn't just create a new instance every time we call commit.signature().

[necauqua]

I didn't look at the current code much, so maybe I was adding a bit to the confusion because of that, sorry 🙃

Yeah I wanted to mention (but never did) that I do recall the backend doing caching of it's own, so it's ok to call verify repeatedly and does, in fact, give purpose to that caching lol

[pylbrecht]

I am not sure if we want to steamroller every error into "invalid", or if we want something more fine-grained.
SignError defines two variants:

/// An error type for the signing/verifying operations
#[derive(Debug, Error)]
pub enum SignError {
    /// The verification failed because the signature *format* was invalid.
    #[error("Invalid signature")]
    InvalidSignatureFormat,
    /// A generic error from the backend impl.
    #[error("Signing error")]
    Backend(#[source] Box<dyn std::error::Error + Send + Sync>),
}

Maybe we only want to convert InvalidSignatureFormat to "invalid" and bubble up the other?

[necauqua]

I'm pretty sure that was exactly the point of separating out the explicitly invalid variant
Or maybe not 🙃

[yuja]

If it rarely occurs, I would just propagate all errors. The way it was implemented would be just because we didn't have a mechanism to propagate error from template functions.

[pylbrecht]

If it rarely occurs, I would just propagate all errors. The way it was implemented would be just because we didn't have a mechanism to propagate error from template functions.

That would mean removing "invalid" entirely, correct?

[yuja]

That would mean removing "invalid" entirely, correct?

Correct. A template property error will be rendered inline as <Error: ..>. If "invalid" is non-error, it might be too verbose. So the decision would depend on whether the error is unusual or not.

[yuja]

Thanks!

[yuja]

nit: maybe better to propagate error?

[yuja]

If this is supposed to be set by Signer, I don't think Verification::new() should have backend argument.

I personally don't mind if it's set by the signing backend, and the field is pub backend: String.

[pylbrecht]

Ah yeah, I recall that I was a bit unsure about this part.

If the Signer should set backend, wouldn't we need some sort of setter method then?
Or how would we set it from "outside" Verification?

@necauqua, @julienvincent, maybe you remember your intentions? (I think the original changes came from you?)

[necauqua]

Afair no specific reason, Yujas suggestion is totally ok

[pylbrecht]

The Signer does set backend here. However, removing backend from Verification::new() makes some tests in lib/tests/test_signing.rs fail:

Local changes I applied to get the test results

diff --git a/lib/src/gpg_signing.rs b/lib/src/gpg_signing.rs
index 7b64c2ed11..a878fc4621 100644
--- a/lib/src/gpg_signing.rs
+++ b/lib/src/gpg_signing.rs
@@ -70,7 +70,7 @@
                 .next()
                 .and_then(|bs| str::from_utf8(bs).ok())
                 .map(|value| value.trim().to_owned());
-            Some(Verification::new(status, key, display, Some("gpg".into())))
+            Some(Verification::new(status, key, display))
         })
         .ok_or(SignError::InvalidSignatureFormat)
 }
@@ -223,12 +223,7 @@
     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()),
-                Some("gpg".into())
-            )
+            Verification::new(SigStatus::Bad, Some("123".into()), Some("456".into()),)
         );
     }
 
@@ -236,12 +231,7 @@
     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,
-                Some("gpg".into())
-            )
+            Verification::new(SigStatus::Unknown, Some("123".into()), None,)
         );
     }
 
@@ -249,12 +239,7 @@
     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()),
-                Some("gpg".into())
-            )
+            Verification::new(SigStatus::Good, Some("123".into()), Some("456".into()),)
         );
     }
 
@@ -262,22 +247,12 @@
     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()),
-                Some("gpg".into())
-            )
+            Verification::new(SigStatus::Good, Some("123".into()), Some("456".into()),)
         );
 
         assert_eq!(
             parse_gpg_verify_output(b"[GNUPG:] EXPKEYSIG 123 456", false).unwrap(),
-            Verification::new(
-                SigStatus::Bad,
-                Some("123".into()),
-                Some("456".into()),
-                Some("gpg".into())
-            )
+            Verification::new(SigStatus::Bad, Some("123".into()), Some("456".into()),)
         );
     }
 }
diff --git a/lib/src/signing.rs b/lib/src/signing.rs
index 8f88639489..168554da2a 100644
--- a/lib/src/signing.rs
+++ b/lib/src/signing.rs
@@ -73,17 +73,12 @@
     }
 
     /// Create a new verification
-    pub fn new(
-        status: SigStatus,
-        key: Option<String>,
-        display: Option<String>,
-        backend: Option<String>,
-    ) -> Self {
+    pub fn new(status: SigStatus, key: Option<String>, display: Option<String>) -> Self {
         Self {
             status,
             key,
             display,
-            backend,
+            backend: None,
         }
     }
 
diff --git a/lib/src/ssh_signing.rs b/lib/src/ssh_signing.rs
index 88da6d5f4d..e19f489fbf 100644
--- a/lib/src/ssh_signing.rs
+++ b/lib/src/ssh_signing.rs
@@ -251,12 +251,7 @@
                     Ok(_) => SigStatus::Good,
                     Err(_) => SigStatus::Bad,
                 };
-                Ok(Verification::new(
-                    status,
-                    None,
-                    Some(principal),
-                    Some(self.name().into()),
-                ))
+                Ok(Verification::new(status, None, Some(principal)))
             }
             _ => {
                 command
@@ -274,14 +269,8 @@
                         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 71e5f3eb25..59b703f482 100644
--- a/lib/src/test_signing_backend.rs
+++ b/lib/src/test_signing_backend.rs
@@ -60,19 +60,9 @@
 
         let sig = self.sign(data, key.as_deref())?;
         if sig == signature {
-            Ok(Verification::new(
-                SigStatus::Good,
-                key,
-                Some("test".into()),
-                Some(self.name().into()),
-            ))
+            Ok(Verification::new(SigStatus::Good, key, Some("test".into())))
         } else {
-            Ok(Verification::new(
-                SigStatus::Bad,
-                key,
-                Some("test".into()),
-                Some(self.name().into()),
-            ))
+            Ok(Verification::new(SigStatus::Bad, key, Some("test".into())))
         }
     }
 }
diff --git a/lib/tests/test_signing.rs b/lib/tests/test_signing.rs
index 302bdab832..bf140a0ba7 100644
--- a/lib/tests/test_signing.rs
+++ b/lib/tests/test_signing.rs
@@ -49,7 +49,6 @@
         SigStatus::Good,
         Some("impeccable".to_owned()),
         Some("test".into()),
-        Some("test".into()),
     ))
 }

running 6 tests
test test_signing::manual::local_backend ... FAILED
test test_signing::forced::git_backend ... FAILED
test test_signing::configured::git_backend ... FAILED
test test_signing::manual::git_backend ... FAILED
test test_signing::keep_on_rewrite::git_backend ... FAILED
test test_signing::manual_drop_on_rewrite::git_backend ... ok

failures:

---- test_signing::manual::local_backend stdout ----
thread 'test_signing::manual::local_backend' panicked at lib/tests/test_signing.rs:80:5:
assertion `left == right` failed
  left: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: Some("test") })
 right: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: None })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test_signing::forced::git_backend stdout ----
thread 'test_signing::forced::git_backend' panicked at lib/tests/test_signing.rs:162:5:
assertion `left == right` failed
  left: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: Some("test") })
 right: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: None })

---- test_signing::configured::git_backend stdout ----
thread 'test_signing::configured::git_backend' panicked at lib/tests/test_signing.rs:181:5:
assertion `left == right` failed
  left: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: Some("test") })
 right: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: None })

---- test_signing::manual::git_backend stdout ----
thread 'test_signing::manual::git_backend' panicked at lib/tests/test_signing.rs:80:5:
assertion `left == right` failed
  left: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: Some("test") })
 right: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: None })

---- test_signing::keep_on_rewrite::git_backend stdout ----
thread 'test_signing::keep_on_rewrite::git_backend' panicked at lib/tests/test_signing.rs:109:5:
assertion `left == right` failed
  left: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: Some("test") })
 right: Some(Verification { status: Good, key: Some("impeccable"), display: Some("test"), backend: None })


failures:
    test_signing::configured::git_backend
    test_signing::forced::git_backend
    test_signing::keep_on_rewrite::git_backend
    test_signing::manual::git_backend
    test_signing::manual::local_backend

test result: FAILED. 1 passed; 5 failed; 0 ignored; 0 measured; 396 filtered out; finished in 0.20s

error: test failed, to rerun pass `-p jj-lib --test runner`

We can no longer construct a Verification instance to assert against (see good_signature()).

Since nobody raised any major concerns with it, I will just go for @yuja's suggestion of making Verification::backend public since that is easy to do.

We could convert the failing test_signing tests to snapshot tests, which would let us assert without having to construct a Verification instance (like we do in test_gpg). I am happy to go that extra mile, if you confirm that it's worth it! 😁

[necauqua]

eh, you could fix good_signature with the field being public, or you could convert the tests if you want, not a huge difference in terms of worth idk

[yuja]

That would mean removing "invalid" entirely, correct?

Correct. A template property error will be rendered inline as <Error: ..>. If "invalid" is non-error, it might be too verbose. So the decision would depend on whether the error is unusual or not.

[necauqua]

(actually looking through the code top-to-bottom finally)

imo you should mention that just checking for signature presence via the boolean coersion (if(commit.signature(), "commit has a signature", "commit is unsigned")) is very fast and cheap, as opposed to the verification part

Also, [..] verifying the signature. Though [..] -> [..] verifying the signature (for example, shelling out to gpg or ssh-keygen). Though [..], just giving an example of why it is slow.

fyi creating a single commit with a gpg signature made through yubikey takes ~200ms for me, a bit annoying :)

fyi² it is (by design) possible to implement another gpg backend (for example) that'll be faster by using some rust gpg implementation or something instead of the lazy way of just calling the gpg binary.

[yuja]

reminder: you'll need to relocate the changelog entry after the release

[yuja]

nit: squash Some("test".into()) from the next commit