Open mdaniel opened 2 years ago
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.
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 other-person@example.com
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
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
other-person@example.com
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.
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 <cole.mickens@gmail.com> 1646208948 -0800
committer Martin von Zweigbergk <martinvonz@gmail.com> 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
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:
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.
git2-ext has two goals
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.
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:
It is simple. Never worry forgetting to append -S
for the commit before pushing.
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.
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).
Also of interest, signing with ssh keys: https://calebhearth.com/sign-git-with-ssh
@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.
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.
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.
@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.
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
Based on my understanding, there are a few ways signing is used in git today:
-S
flag when creating an object such as a commit or tag.-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).commit.gpgsign = true
in your gitconfig, which automatically enables 1 and 2.git tag -v <tag>
to check that the tag has a valid signature.--verify-signatures
flag on a merge or pull to validate that all commits in the merge/pull have valid signatures, otherwise aborting.Git has 3 supported signing formats:
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 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:
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
With that in mind, I see this as a sensible approach for the first rollout:
[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
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.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!
Update from @epage on signing commits with libgit2: https://github.com/arxanas/git-branchless/issues/465#issuecomment-1372379421
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.
I want to extend a little on @rslabbert's report and just outline some UX design that I was thinking about:
-S[<keyid>]/--sign=[<keyid>]/--no-sign
flags, as well as --resign
:
--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.--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.--no-sign
flag was set.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.--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
)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.
-r
defaulting to @
.-L
flag as an "are you sure" impression - again, in interactive shell we can just ask the user "are you sure".--resign
, if any of the commits are signed with a different keyid, require that flag, and eveything described above applies.signature
object to the templater, which would contain all the information git log --show-signature
would show.
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:
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.
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, 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 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?
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.
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.
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).
@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 Good to know - I'd be very happy to build from source from that branch but I am actually using the ssh backend currently!
@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.
@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!
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)
Yea I've just been using the built-in so far. It's pretty good for what it is, but I'd ideally like to be working in something like https://github.com/sindrets/diffview.nvim.
@necauqua
but adding ssh or anything else is simple
Ok I had a peek and have started working on adding an SshBackend :) It seems pretty straight forward.
Well, it's a bit outdated, I should at least rebase it.
If you get a chance to do this that would be awesome. I gave it a try but there are a few conflicts I would need a lot more context to be able to resolve. Focusing on just adding the SshBackend for now.
I've gained a need for x509 support, and fortunately it seems to be a very easy addition to the existing GPG support. Even git treats them as nearly identical. Assuming it's fine to extend the GPG implementation with x509, there's the question of configuration.
I'm thinking either signing.backends.gpg.format
or signing.format
. I'm leaning towards the former because it would be less likely to impose overloading per-repo configurations. Thoughts?
I mean sure the backends are nearly identical, but that's an implementation detail, why instead of exposing it as "x509" you are inventing separate configs?.
You could generalize the gpg signing backend to work for both, and then expose two instances of it with slightly different configs or generics or whatever as the gpg
and the x509
backends.
Even the very git code you linked does basically just that
(they call the struct gpg_format
for historical reasons similar to how the commit header for the signature is called gpgsig
, even though one of the backends is ssh and it's generic enough to have any form of cryptohtaphic signing, you probably knew that)
I'm not sure I see the benefit here. I'm proposing adding a format
config value somewhere, either generic or gpg specific, and then when that's set to x509
, the gpg backend behaves slightly differently. Why would we want to expose it as a different backend? What's the benefit of hiding that it's almost identical to gpg?
Hm, I've read up on x509, and it's just a signature with a pubkey that was signed by a CA, so it should be basically identical to gpg indeed.
In my head the x509 was a separate signing method, and also I've had git as an example where it is a separate "backend".
I still stand by not inventing additional configs and following what git does here - and well, semantically it is a different backend after all, but I guess I'm closer to neutral now so if somebody else agrees then I'm not opposed - not like I'll be using it anyway 🤷
It's hard to put in words, but it intuitively feels more correct to expose it as a separate backend to me - and anyway the difference between setting a different signing.backend
and setting this extra format config is that the latter would take two lines instead of one :joy:
gpg
and gpgsm
might have similar command-line interface, but they appear to be different in protocol and key storage. So it's probably better to add separate backends?
We could add separate "format"/"backend" abstractions (e.g. the gnupg backend supports pgp and x509 formats), but I don't think it will simplify the user-facing API.
I hadn't realized that the basic singing support was release a couple of releases ago :sweat_smile:.
I gave it a go today and it's great. One issue I had was, since I manage my .jjconfig
with nix home-manager, it was easy to get into a difficult to recover from state where I can't switch back to a revision with a healthy config.
> jj new main
Internal error: Unexpected error from backend
Caused by:
...
It'd be really nice if this error was less fatal. Perhaps printing something like this
> jj new main
Warning: Failed to sign revision "rwvpnvwm"
Help: consider explicitly singing with `jj sign -r rwv`
Caused by:
...
Once #3142 gets merged, I'll probably look at opening a PR for this. It looks like it could be fun first jj
contribution.
It'd be really nice if this error was less fatal. Perhaps printing something like this
> jj new main Warning: Failed to sign revision "rwvpnvwm" Help: consider explicitly singing with `jj sign -r rwv` Caused by: ...
Wouldn't it be very likely that the suggested command would also fail then? Oh, I see, not in your case where the jj new main
command took you to a state where you config is working. I wonder if that setup is too uncommon to optimize for. Maybe we should instead hint something like this:
> jj new main
Internal error: Unexpected error from backend
Caused by:
...
Hint: You can temporarily disable signing with `--config-toml signing.sign-all=false`
Just a thought.
If this is ready, or at least if it's shipping partially completed, would it be possible to update the docs accordingly? This page still seems to imply that there is no support whatsoever for signed commits: https://martinvonz.github.io/jj/latest/git-compatibility/#supported-features
@dacid44 I think https://martinvonz.github.io/jj/prerelease/git-compatibility/#supported-features seems OK, and that will become the "latest" version in a few days.
It looks like the docs here show the ability to specify an ssh key path with ~/...
, but when I tried it, it only seems to work with a full absolute path.
@dacid44 that was added in https://github.com/martinvonz/jj/commit/e803bed845228c49e5a5b10e1bb8f0af89b79b44, which has not been released.
Expected Behavior
jj close -S
would sign the commit enabling compatibility with workflows which require verified commitsActual 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 runningSpecifications