Implementation of the rest of the gpg signing support #2728

necauqua · 2023-12-21T06:25:13Z

So I've been using this stack locally for a while and I'm very happy with it, but I couldn't find it in me (at least for a little while at the moment) to actually finish it, and the only thing that has to be done here is just more tests 🤷

And well also I just noticed the changelog checkbox, changelog too
And also SSH impl, I was focused on GPG, but an SSH impl should be trivial at this point (cc @chriskrycho)

I'm just pushing this just so that it's not only on my machine, and maybe for someone to take it and make the final push to have it in main (maybe me after a few months) - every commit is independent and can be cherry-picked (hm, except I think in the templater one I touch some stuff in the signer, could conflict a little, eh)

Also the GPG impl commit has those weird tests that need a gpg binary in path which I'm 95% certain will fail on windows/mac, and maybe on linux too 🙃

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

Now it is actually possible to set GPG as the main backend and have jj "preserving" signatures on rewrites. Just no way to make signatures yet

Cannot make it accept multiple revisions easily, that'd have to be done with a rebaser

Add it to default templates as well, it only shows anything when there's a signature.

necauqua · 2023-12-21T06:28:28Z

huh codespell actually caught something, cool

necauqua · 2023-12-21T06:41:45Z

Huuuuuh, gpg is not only present on the windows worker, but even works (in a sense that jj successfully calls the command, so a build off this PR should work on windows) - only thing breaking seems to be how I import the test keys - idk how to fix, and I've no windows machine

And it did work on mac too

yuja

(just random comments for the first two. I haven't reviewed the other patches.)

yuja · 2023-12-22T13:40:10Z

lib/src/gpg_signing.rs

+
+impl SigningBackend for GpgBackend {
+    fn name(&self) -> &str {
+        "GPG"


nit: maybe better to stick to lowercase "gpg"? I expect config key would be lowercase (with dash or underscore.)

Agreed. Resolved in #3007

yuja · 2023-12-22T13:41:18Z

lib/src/gpg_signing.rs

+        // in the output from --status-fd=1
+        // Assume signature is invalid if none of the above was found
+        output
+            .split(|&b| b == b'\n')


Perhaps, this text parsing part can be split to function, and added test for that.

yuja · 2023-12-22T13:44:36Z

lib/src/gpg_signing.rs

+                signature_file.path().as_os_str(), /* the only reason we have those .as_refs
+                                                    * transmuting to &OsStr everywhere.. */
+                "-".as_ref(),
+            ],


Maybe run() could be split to two functions? If the first half returned a Command object, we don't have to build a slice of a certain type.

yuja · 2023-12-22T13:45:55Z

lib/tests/test_gpg.rs

+        .prefix("jj-test-")
+        .tempdir()
+        .unwrap()
+        .into_path();


Maybe you know, but this temporary directory isn't removed at exit.

The tempfile crate implements the Drop trait which cleans up the file. This should be fine!

It's static object, so Drop won't be called afaik.

Ah, makes sense

yuja · 2023-12-22T13:51:23Z

cli/src/commands/sign.rs

+    force: bool,
+    /// Drop the signature, explicitly "un-signing" the commit.
+    #[arg(long, short = 'D', conflicts_with = "force")]
+    drop: bool,


nit: might be better to make sign --drop a separate command. jj tends to have create/delete/set/get/list verbs as subcommands.

julienvincent · 2024-02-06T22:09:00Z

I started work on adding an ssh backend - initial wip implementation based on the GpgBackend can be found here: julienvincent@eb623e6

This commit was created and signed by that commit! I love that haha.

julienvincent · 2024-02-09T10:13:18Z

A comment I have after using this - I don't think commands like jj log and jj show should show or verify the signature by default.

Even if someone has signing enabled they might not have the necessary public keys configured for verification on their local machine.
It has a not-insignificant performance impact when viewing the log - especially on larger repos.

Git only performs signature validation when the --show-signature flag is passed - I think jj should expose the same or similar behaviour.

I'll make this change on my branch unless someone objects.

necauqua · 2024-02-09T21:59:19Z

might not have the necessary public keys configured for verification on their local machine

Well then it'll show ?s, which is "unknown", all part of the plan - it specifically does show you that you need to import some keys if you enabled signing

performance impact

I thought about it, but it didn't seem that bad, and it's only there if you enable signing, again

I think jj should expose the same or similar

We could remove it from the default template and add docs that show how to get it back if someone wants them (also check out builtin_log_detailed)
I had no issues, but I've never used repos that would have thousands of signatures either - and I really like the green checkmarks it gives me :)

Also okay performance issue makes sense for log, but what's wrong with show? It's just a single commit then

necauqua · 2024-02-19T22:45:04Z

Superseded by #3007 and it's followups

necauqua added 2 commits December 21, 2023 07:13

necauqua changed the title ~~sign: Implement and test GPG signing backend~~ Implementation of the rest of the gpg signing support Dec 21, 2023

necauqua added 2 commits December 21, 2023 08:27

sign: Print hints about dropped commit signatures

Verified

This commit was signed with the committer’s verified signature.

necauqua Anton Bulakh

GPG key ID: 071FE3E324DD7333

Verified
Learn about vigilant mode

cd6a6d1

necauqua force-pushed the necauqua/signing branch from ff42130 to 682eb69 Compare December 21, 2023 06:28

necauqua mentioned this pull request Dec 21, 2023

sign: add basic signing support to backend #1738

Closed

4 tasks

sign: Add the global flags mentioned in configs

Verified

This commit was signed with the committer’s verified signature.

necauqua Anton Bulakh

GPG key ID: 071FE3E324DD7333

Verified
Learn about vigilant mode

77b21fd

necauqua force-pushed the necauqua/signing branch from 682eb69 to 77b21fd Compare December 21, 2023 07:17

yuja reviewed Dec 22, 2023

View reviewed changes

martinvonz mentioned this pull request Feb 5, 2024

support for signed commits #58

Closed

0xdeafbeef linked an issue Feb 6, 2024 that may be closed by this pull request

support for signed commits #58

Closed

julienvincent mentioned this pull request Feb 9, 2024

Add ssh signing backend implementation #2998

Closed

4 tasks

julienvincent mentioned this pull request Feb 9, 2024

Commit signing backend implementation #3007

Merged

4 tasks

necauqua closed this Feb 19, 2024

necauqua deleted the necauqua/signing branch February 19, 2024 22:45

This was referenced Feb 25, 2024

Signature verification #3141

Closed

Add jj sign command #3142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of the rest of the gpg signing support #2728

Implementation of the rest of the gpg signing support #2728

necauqua commented Dec 21, 2023 •

edited by rdamazio

Loading

necauqua commented Dec 21, 2023

necauqua commented Dec 21, 2023 •

edited

Loading

yuja left a comment

yuja Dec 22, 2023

julienvincent Feb 9, 2024

yuja Dec 22, 2023

yuja Dec 22, 2023

yuja Dec 22, 2023

julienvincent Feb 9, 2024

yuja Feb 9, 2024

julienvincent Feb 9, 2024

yuja Dec 22, 2023

julienvincent commented Feb 6, 2024

julienvincent commented Feb 9, 2024 •

edited

Loading

necauqua commented Feb 9, 2024 •

edited

Loading

necauqua commented Feb 19, 2024

Implementation of the rest of the gpg signing support #2728

Implementation of the rest of the gpg signing support #2728

Conversation

necauqua commented Dec 21, 2023 • edited by rdamazio Loading

Checklist

necauqua commented Dec 21, 2023

necauqua commented Dec 21, 2023 • edited Loading

yuja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienvincent commented Feb 6, 2024

julienvincent commented Feb 9, 2024 • edited Loading

necauqua commented Feb 9, 2024 • edited Loading

necauqua commented Feb 19, 2024

necauqua commented Dec 21, 2023 •

edited by rdamazio

Loading

necauqua commented Dec 21, 2023 •

edited

Loading

julienvincent commented Feb 9, 2024 •

edited

Loading

necauqua commented Feb 9, 2024 •

edited

Loading