jj icon indicating copy to clipboard operation
jj copied to clipboard

support for signed commits

Open mdaniel opened this issue 3 years ago • 44 comments
trafficstars

Expected Behavior

jj close -S would sign the commit enabling compatibility with workflows which require verified commits

Actual Behavior

There currently is no support for signing commits

Implementing this feature will require careful consideration given how much jj appears to silently create commits under the covers, since some workflows will prompt for the GPG keyphrase either on every commit action, or every so often if the gpg-agent is running

Specifications

  • Reference: https://github.com/git/git/blob/v2.35.1/commit.c#L1014

mdaniel avatar Feb 19 '22 19:02 mdaniel

I saw that Git also has a commit.gpgSign config, which makes it sign all commits. I assume that means that e.g. git rebase can get interrupted by GPG asking for a password. That will be a bigger problem with jj because it's much easier to rewrite history (e.g. jj describe <some old commit> will rewrite all descendants).

I've never worked with signed commits. How do you use them? Do you just run git commit -S right before pushing to create a PR? Would you like a warning (or error?) if you try to rewrite a commit that's been signed (unless signing is on for all commits, like with Git's commit.gpgSign)?

By the way, thanks for the request and for providing helpful links! Sorry it took a while for me to get around to even answering this.

martinvonz avatar Mar 15 '22 23:03 martinvonz

I saw that Git also has a commit.gpgSign config, which makes it sign all commits. I assume that means that e.g. git rebase can get interrupted by GPG asking for a password

Only for those poor souls running without gpg-agent; I have that commit.gpgSign = true in my global gitconfig, turning it off only for truly local repos that will never be pushed to Git(Hub|Lab)

How do you use them?

"Use" is kind of a hard word to capture here, but that's why I mentioned the workflow or policy portion. My current employer doesn't require signed commits, but it's not off the table, either. If ones threat model includes some git history shenanigans, signed commits are one defense against that

Rewriting a commit that I signed is no drama, because it gets re-signed just like a normal git-commit flow thanks to the magic of gpg-agent. Attempting to rewrite a commit that someone else signed will cause it to show up as "Unverified," meaning yes, it's technically signed but not with a keyid the actual owner of [email protected] has published into Git(Hub|Lab). I actually haven't seen what it'll do with the Author/Committer distinction, but I presume it'll say "Unverified" as before

I believe there's also some local way to have git verify the commits using the gpg --recv-key && trust-key-or-whatever flow, so this isn't only a Git(Hub|Lab) problem

Thanks for even entertaining this issue; I didn't mean to imply it's a deal breaker, but I also would bet I'm not the only one who will want it, either

mdaniel avatar Mar 16 '22 02:03 mdaniel

How do you use them?

"Use" is kind of a hard word to capture here, but that's why I mentioned the workflow or policy portion.

Sorry, I said "use" but "create" is closer to what I meant. I was wondering when you create signed commits in your workflow and what you do when you rewrite commits. Your mention that you use commit.gpgSign = true and gpg-agent answered that. I wonder (to myself - not expecting an answer from you) how users who can't use gpg-agent sign commits.

My current employer doesn't require signed commits, but it's not off the table, either. If ones threat model includes some git history shenanigans, signed commits are one defense against that

Makes sense. I suppose it may also be important for being able to attribute some security issue to a certain person, or to help clear them if the commit was not signed by them and they typically sign their commits.

Incidentally, I just set up some security scanner yesterday because Google's open-source office recommended it. Interestingly, it doesn't seem that the scanner has any opinion about signed commits (see findings), and the policy doesn't seem to mention it either. (This is not meant as an argument against adding support for signed commits.)

Attempting to rewrite a commit that someone else signed will cause it to show up as "Unverified," meaning yes, it's technically signed but not with a keyid the actual owner of [email protected] has published into Git(Hub|Lab).

Oh, so that's what "Unverified" means. I saw it on e.g. https://github.com/martinvonz/jj/commit/2a6ab8b6fceab91f35cae697c0c9f1f4de727594, which it looks like I did a "Rebase and merge" on in the GitHub UI.

Thanks for even entertaining this issue; I didn't mean to imply it's a deal breaker, but I also would bet I'm not the only one who will want it, either

Understood. I wouldn't be surprised if the nice green "Verified" chip in the GitHub UI also makes people want it.

I'm not sure when I'll get around to working on it, but it's good to keep this feature in mind either way. Since jj is written as a library with pretty thin CLI on top, a feature like this has some implications because the library code should not directly do signing. It will be probably be some kind of callback instead. That functionality doesn't exist yet. The way commits are signed also seems to require quite deep integration in the backend, so there will probably have to be a callback from the backend too, or maybe some new Signer object can be passed in, we'll see.

martinvonz avatar Mar 16 '22 03:03 martinvonz

it doesn't seem that the scanner has any opinion about signed commits

Yup, makes sense given that setting up PKI is some holy-shit, and the majority of the world doesn't care about security until they get broken into. That's why I highlighted the threat model part -- it depends on how Very Bad ™ it would be for an unauthorized commit to end up in the repo. Arguably our Homebrew friends are at the biggest risk of a supply chain attack like that, given its uber-aggressive brew() { git pull && ruby random/files.rb "$@"; } default behavior

Oh, so that's what "Unverified" means. I saw it on e.g. https://github.com/martinvonz/jj/commit/2a6ab8b6fceab91f35cae697c0c9f1f4de727594, which it looks like I did a "Rebase and merge" on in the GitHub UI.

I was curious and git show --pretty=raw 2a6ab8b6 said

author Cole Mickens <[email protected]> 1646208948 -0800
committer Martin von Zweigbergk <[email protected]> 1646239614 -0800

without mention of a broked GPG signature stanza, so I'd guess the "Unverified" means GH knows about Cole's key and found it suspicious that a commit appeared attributed to that email but was unsigned. h/t https://stackoverflow.com/a/54223057

I was curious what my merge commit looked like, and it seems (thanks to the new URL format I learned) that GH is angry with two of my keys, thus explaining the missing Unverified badge. I think it verifies commits with the gpgsig attribute, but can't do the reverse lookup for my account due to that error. I'm sure it's even worse given that I haven't had to use my gmail address on GH in years, except thanks to the CLA bot :-D

edit: nope, it seems Cole has vigilant mode enabled; it's a checkbox at the bottom of the GPG settings page

mdaniel avatar Mar 16 '22 04:03 mdaniel

cc @epage: this might be a good feature to add to git2-ext if any of us ever get around to implementing it.

Other resources:

  • Draft PR adding GPG-signed commits to gitui https://github.com/extrawurst/gitui/pull/219
  • How git-revise does GPG signing: https://github.com/mystor/git-revise/blob/7e3bf8d5031be9a938ea4c6968456d92f4b75b94/gitrevise/odb.py#L334, test: https://github.com/mystor/git-revise/blob/c06c8003870c776376e5f7bc4dd9f4dc97714b7c/tests/test_gpgsign.py#L10

arxanas avatar Mar 16 '22 05:03 arxanas

What's git2-ext? I assume it's https://github.com/gitext-rs/git2-ext, but what's the project's goal?

Thanks for the links. They will hopefully be helpful for whoever addresses this issue.

martinvonz avatar Mar 16 '22 05:03 martinvonz

git2-ext has two goals

  • Provide "good enough" implementations of essential or higher-level git2 logic, like cherry-pick, squash, hooks, authentication (not implemented yet), etc
  • The above serves as examples for people needing to write their own implementations

epage avatar Mar 16 '22 11:03 epage

By the way, a related issue is that GitHub doesn't support --ff-only merges, so you simply cannot get linear history and fully verified commits on GitHub.

GitHub seems to have a preference for integration by merging (as opposed to rebasing), so maybe that's why they haven't bothered adding support for it.

martinvonz avatar Mar 18 '22 16:03 martinvonz

Do you just run git commit -S right before pushing to create a PR

Most of commits jj made will be amended/rebased soon, so there is no need to sign them all. I think jj just need to rewrite the commits before pushing to remote, because it is a good practice to never rewrite commits published.

FYI, under git, I configured it to auto sign every commit, because:

  1. It is simple. Never worry forgetting to append -S for the commit before pushing.

  2. I prefer merge over squash/rebase, to preserve more contextual information. Months or years later, if I want to review changes introduced in a PR/branch, it would be easier. Squash may result in a very big commit, and rebase may result in broken commit which do not pass tests or even not build. TBH, I rarely need to do this. I just want to feel safe. So I am fine with squash/rebase instead of merge.

  3. I have gpg-agent configured, so passphrase is cached for a long time. And I am considering use a passwordless gpg or ssh key to sign git commits instead. Sometimes I think password is recommended just because full disk encryption is not common in the good old days.

My current workaround is to use jj git export, then in the git repo use git rebase -i --reset-author --exec to rewrite all commits made by jj, ensuring all of them are signed and fine (build successfully & lint reports no issue & pass tests).

weakish avatar Sep 15 '22 11:09 weakish

Also of interest, signing with ssh keys: https://calebhearth.com/sign-git-with-ssh

millette avatar Sep 15 '22 20:09 millette

@weakish, sorry it took me so long to reply. I was chatting with @rslabbert about this just now.

Most of commits jj made will be amended/rebased soon, so there is no need to sign them all. I think jj just need to rewrite the commits before pushing to remote, because it is a good practice to never rewrite commits published.

I think it's going to be much simpler to sign every commit. I really think the user should have gpg-agent or similar (ssh-agent when using ssh-based signing?) set up anyway, and once they do, there seems to be little reason not to sign every commit we create. If all commits are signed, there's no extra step necessary when the user decides to push the commits to a remote.

martinvonz avatar Nov 26 '22 09:11 martinvonz

I think it's going to be much simpler to sign every commit.

Hopefully this is an option and it is well explained. I think the first case of using a tool is local exploration and it should not be hampered by keys. (I had this trouble when I tried to see how Pijul was.)

I really think the user should have gpg-agent or similar

Please don't assume.

joyously avatar Nov 26 '22 17:11 joyously

Hopefully this is an option and it is well explained.

Oh, it would definitely not be mandatory :) Sorry if that was unclear.

Please don't assume.

Sure, we can definitely have a manual jj sign command for users who want to sign commits only sometimes.

martinvonz avatar Nov 26 '22 18:11 martinvonz

@rslabbert, see discussion above. If you're working on this, do you think a jj sign command would be a simpler first step than a config to sign every new commit? Feel free to do whichever you prefer.

martinvonz avatar Nov 26 '22 19:11 martinvonz

So, to fully map out the feature:

Disclaimer: I'm not a security engineer or a cryptographer. Caution is applied and preference is given to existing mechanisms/designs from git that are easy to copy

Context

Workflows

Based on my understanding, there are a few ways signing is used in git today:

  1. By passing the -S flag when creating an object such as a commit or tag.
  2. By passing the -S flag when doing a rebase, merge, squash, etc. Note that this will overwrite signing signatures on all the commits being affected (as per my understanding).
  3. By setting commit.gpgsign = true in your gitconfig, which automatically enables 1 and 2.
  4. By using git tag -v <tag> to check that the tag has a valid signature.
  5. By passing the --verify-signatures flag on a merge or pull to validate that all commits in the merge/pull have valid signatures, otherwise aborting.
  6. If you tick "verified commits" in Github, you'll see a green UI element on all signed commits matching gpg keys you've uploaded, and you can also enforce verified commits for your user/email, flagging anyone who has uploaded commits with your email who doesn't have your signature (including you or your old commits).

Formats

Git has 3 supported signing formats:

  1. openpgp, which is the default and normally what people mean when they say signing commits.
  2. x509, which isn't as common as openpgp. See smimesign as an example.
  3. ssh, the newest format, convenient because you can use the same SSH key you use for git auth, or something like gitsign from sigstore.

Design

Philosophy

Personally, I see jj as simplifying a lot of git's complexity. That means a) we shouldn't be beholden to exactly the way git does things, and b) if there is a "more correct default" we should be nudging users towards that.

Signing backends

Signing should be abstracted behind user configurable backends, supporting both multiple formats, and multiple ways to handle the same format.

For example, for git compatibility, it might make sense to support git's gpg.program functionality, where any external tool can be plugged in, and communication is done using stdin/stdout passing.

In the future, it would be good to look into:

  • gpgme provided by the gnupg project and the recommended way to wrap its functionality (though not what git does natively hence not starting with it)
  • https://sequoia-pgp.org/ for openpgp (also supports reading from an existing gpg-agent)
  • webpki for x509
  • ring for ssh
  • Gitoxide might support this natively in the future: https://github.com/Byron/gitoxide/issues/12

Config options

The jj config should be expanded to support this configurability:

# Root signing config
[signing]
enabled = true
backend = "openpgp-gnupg" # select which format-backend to use

[signing.format.openpgp]
key = "KEYID"

# Backends have their own options that can be configured
[signing.backend.openpgp-gnupg]
program = "gnupg2"

[signing.backend.openpgp-sequoia]
gpg-agent = true

Next steps

With that in mind, I see this as a sensible approach for the first rollout:

  1. Implement the config changes above.
  2. Add the signing backend machinery and an "external program" backend for openpgp. By default this should work with gnupg/gnupg2. Since this just calls the existing binary, gpg-agent integration is automatic and when a user doesn't have a gpg-agent running (or their existing session has expired) they will get the pinentry prompt to unlock their key which is consistent with git and other programs that use git.
  3. When [signing].enabled = true, automatically sign every commit written by jj including on rebase/etc. the same way (as far as I'm aware) git does when setting [commit].gpgsign = true
  4. Work on the test suite to fully test various combinations and interactions of signed being enabled and performing operations (some open questions here).

Open questions

  1. Should we detect on jj init/clone if the user has signing settings in their .gitconfig and offer to migrate it across to the jj config? This could be a good broader feature to have.
  2. Does the "signing is enabled always or not at all" approach break major workflows or does it roughly align with what people do?
  3. Is the assumption regarding how git handles signing in the face of rebasing/etc. correct?

rslabbert avatar Nov 26 '22 23:11 rslabbert

Wow, thanks for investigating and detailed reporting. Sorry it took me so long to reply. I simply forgot.

  • Add the signing backend machinery and an "external program" backend for openpgp. By default this should work with gnupg/gnupg2. Since this just calls the existing binary, gpg-agent integration is automatic and when a user doesn't have a gpg-agent running (or their existing session has expired) they will get the pinentry prompt to unlock their key which is consistent with git and other programs that use git.

One thing to keep in mind is that the library crate (jujutsu-lib) should not interact with the user. We can still implement what you suggest here, we just need to make sure that the signing is done via some callback into the CLI crate. For example, if the user starts a (hypothetical) server that uses the library crate, and that server performs operations on behalf of various users, it shouldn't ask the user running the server for a password.

Since the signatures will be added by the backend, we need to pass in the callback to it. I suspect that callback will simply take a list of bytes (the data to sign) and return another list of bytes (the signature). If we want to be able to sign some commit but not others, that seems to mean that we need to need to add an Option<SigningFunction> argument to write_commit(). We'd need to update the callers to pass that argument, propagating it from the CLI crate. Actually, I suppose we'll need to pass in some more information about the type of signing to use, since the backend may need to include e.g. commit headers saying which format was used. So SigningFunction above is probably Signer (or a better name).

  • Should we detect on jj init/clone if the user has signing settings in their .gitconfig and offer to migrate it across to the jj config? This could be a good broader feature to have.

Sounds like a nice feature :)

  • Does the "signing is enabled always or not at all" approach break major workflows or does it roughly align with what people do?

It sounded like at @joyously (and @epage?) signs only certain commits, while @mdaniel and @weakish sign all commits (in certain repos, perhaps), if I understood the messages above correctly as I skimmed them. I'm not sure how much more work it is to add support for both workflows, but do whatever you prefer if it's you who is going to implement it :) Support for one of the workflows is better than none. I think we should still keep both in mind and design for both, since it doesn't seem like it would be much harder.

(I considered making the signing callback etc. a hidden property of the backend, not affecting the write_commit() function. That would only make sense if we signed all or nothing, but I still don't like that design because we would probably still want to pass the commit information as context to the signing functions somehow. It also seems harder to reason about.)

  • Is the assumption regarding how git handles signing in the face of rebasing/etc. correct?

Yes, I think so, but that's just because I think what you describes is the behavior that makes most sense to me, not because I have any experience with it :)

Thanks again!

martinvonz avatar Dec 08 '22 06:12 martinvonz

Update from @epage on signing commits with libgit2: https://github.com/arxanas/git-branchless/issues/465#issuecomment-1372379421

arxanas avatar Jan 06 '23 02:01 arxanas

Just throwing some data points into the pot:

I wonder (to myself - not expecting an answer from you) how users who can't use gpg-agent sign commits.

This is something I have to deal with a non-zero amount of time, and it's exactly as painful as you would imagine: you need to enter the key password for every commit. In particular, rebasing some large branch means entering your password consecutively way too many times.

Does the "signing is enabled always or not at all" approach break major workflows or does it roughly align with what people do?

Anecdotal, but the people I know who sign commits at all—including myself—fully rely on commit.gpgSign = true. Those people are fairly far and few between, however.

xelxebar avatar Mar 21 '23 10:03 xelxebar

I want to extend a little on @rslabbert's report and just outline some UX design that I was thinking about:

  1. Add the global -S[<keyid>]/--sign=[<keyid>]/--no-sign flags, as well as --resign:
    • Flags are global as every command can create a commit object by snapshotting WC.
    • In Git, the long arguments are actually --gpg-sign/--no-gpg-sign, add those (hidden from help) as well for people (/scripts?) transitioning with a hint message. Git flag including the word gpg seems dated as there are ssh signatures now and possibly other non-gpg options in the future.
    • If signing requires autorebasing children - do it, but only sign children if the config is set, more on that later.
    • If the commit in question is already signed with a different keyid, require a --resign flag, otherwise fail showing the existing signature (possibly made by someone else) - also in interactive shell we can ask the user if they are sure they want to resign to avoid the "ah forgot the sudo" impression.
  2. Autosigning:
    • If the config is set, sign every commit object ever created with configured keyid, unless the --no-sign flag was set.
    • If the config is NOT set then having new WC commit (making an edit and running jj log) will NOT be signed, even if it was previously manually signed (in that case a log line saying "dropped the signature" or something would be good), unless the flags from the above were specified.
    • --resign also applies - this means that autorebasing children can fail - when you move the WC to be a commit which has descendants signed with another keyid, and invoke any jj command without a --resign flag, the working copy is created as a new child of @- (unless --ignore-working-copy ofc) but the descendants are not autorebased. That new child has the same changeid as the original commit, so they are divergent. I think that this is an expected behavior - this is consistent and solvable, an allowed state for the repo to be in, and generally people should just not (unless they really need to) directly edit/rebase some old commits behind others signatures.
  3. (Auto)rebasing:
    • Another thing is if the commit has signed descendants but we don't have autosigning with the same keyid enabled - in that case basically the same happens as in last point of autosigning, i.e. autorebase fails and we get a divergent changeid.
    • Same applies to ordinary rebases and any other edits (well those are covered by "autorebase" term) that require resigning/stripping the descendant signatures, they fail unless we explicitly do --resign - in case no autosigning is configured the descendant signature would be stripped (and probably we'd have another flag to be explicit/non-confusing, like --strip-signature)
  4. Add a jj sign [<keyid>] command that (re)adds a signature to select commit(s). Maybe even jj unsign or --strip-signature too to remove the signature should anyone ever need it.
    • It would accept a revset with -r defaulting to @.
    • If revset resolves to multiple commits, sign all of them, probably gate that behind an -L flag as an "are you sure" impression - again, in interactive shell we can just ask the user "are you sure".
    • Same thing with --resign, if any of the commits are signed with a different keyid, require that flag, and eveything described above applies.
  5. Add the signature object to the templater, which would contain all the information git log --show-signature would show.
    • imo the default log should have something like a checkmark or some other "verified" symbol similar to github.

My wording is all over the place, but reading and rereading those points I think I've covered everything I can think of about signing commits, looks okay, what are your thoughts? Point at holes I'm blind to pretty please :smile:

necauqua avatar Apr 03 '23 21:04 necauqua

That all sounds good to me. Thanks for writing it down. Will you also have time to work on that? I don't think I will have time anytime soon.

martinvonz avatar Apr 04 '23 07:04 martinvonz

I don't want to add unnecessary noise - but just jumping in here to say that I am also very interested in this feature.

I have been using signed commits for quite a while now. I do not require it but I really like the property of being able to prove without a doubt that any given change did in fact come from me. I've never needed it, but one day I might and it does not take any additional effort on my part to have this property with git.

I've just started trying to adopting jujutsu - it's concepts align so well with my way of working - I just wish I didn't have to give up commit signing for it!

julienvincent avatar Feb 05 '24 15:02 julienvincent

@julienvincent, there's pending PR #2728 to add support for it. I think it's close to ready. I think @necauqua said on Discord that he'd appreciate any help getting it across the finish line, in case you're interested in doing that.

martinvonz avatar Feb 05 '24 17:02 martinvonz

@martinvonz Ah I hadn't spotted that PR, that's awesome.

I'd be happy to help but I doubt I have enough working context on this project to do anything useful code-wise - at least not anytime soon. This is my first day trying jujutsu properly.

What kind of help is in need?

julienvincent avatar Feb 05 '24 17:02 julienvincent

I think it was about addressing code review comments and maybe about adding tests, but it's been a while since I looked at that PR.

martinvonz avatar Feb 05 '24 17:02 martinvonz

Ok. I think I'm more likely to spend time in the near future working on improving jujutsu integration in Neovim as that's more painful for me than commit signing.

But if this PR is still dangling after that then I would be willing to familiarise myself with the codebase and maybe pick it up.

julienvincent avatar Feb 05 '24 17:02 julienvincent

Ok. I think I'm more likely to spend time in the near future working on improving jujutsu integration in Neovim as that's more painful for me than commit signing.

See also https://github.com/martinvonz/jj/wiki/Vim and https://github.com/avm99963/vim-jjdescription (which I'll add to the wiki shortly).

ilyagr avatar Feb 05 '24 19:02 ilyagr

@julienvincent the PR is 100% functional (it only has the gpg backend though, but adding ssh or anything else is simple) so if you really want to, you can build jj from it and have it, like I've been doing since, feedback on usage experience appreciated.

I really like how I managed to have jj workflows work seamlessly with signing (but well, it does sign on every snapshot, so you'll need an agent to not enter the passphrase on every command, and rebases are worse)

Well, it's a bit outdated, I should at least rebase it.

Yeah, crossing the finish line only requires just a bit more tests and some minor tweaks from the review - and more review as well, since I think Yuja only reviewed the first couple of commits

And also someone with a windows machine to fix that gpg integration test setup thing too

necauqua avatar Feb 05 '24 23:02 necauqua

@necauqua Good to know - I'd be very happy to build from source from that branch but I am actually using the ssh backend currently!

julienvincent avatar Feb 06 '24 00:02 julienvincent

@ilyagr Thanks for those links - was looking for a .jjdescription plugin but hadn't found one. I found the DirDiff workflow pretty terrible though. Really want to try work on some better jujutsu-native plugin for Neovim.

julienvincent avatar Feb 06 '24 00:02 julienvincent

@ilyagr Thanks for those links - was looking for a .jjdescription plugin but hadn't found one. I found the DirDiff workflow pretty terrible though. Really want to try work on some better jujutsu-native plugin for Neovim.

Yeah, it's far from perfect. These days, I use vimtabdiff if I have to and Meld ("meld-3") otherwise. Also, some people like the builtin scm-diff-editor (I haven't used it enough to have an opinion vs Vim; I definitely prefer Meld to it)

Really want to try work on some better jujutsu-native plugin for Neovim.

That sounds wonderful, thank you!

ilyagr avatar Feb 06 '24 01:02 ilyagr