-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support for signed commits #58
Comments
I saw that Git also has a I've never worked with signed commits. How do you use them? Do you just run 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. |
Only for those poor souls running without
"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 I believe there's also some local way to have git verify the commits using the 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 |
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
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.)
Oh, so that's what "Unverified" means. I saw it on e.g. 2a6ab8b, which it looks like I did a "Rebase and merge" on in the GitHub UI.
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 |
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
I was curious and
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 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 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. |
Most of commits jj made will be amended/rebased soon, FYI, under git, I configured it to auto sign every commit, because:
My current workaround is to use |
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.
I think it's going to be much simpler to sign every commit. I really think the user should have |
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.)
Please don't assume. |
Oh, it would definitely not be mandatory :) Sorry if that was unclear.
Sure, we can definitely have a manual |
@rslabbert, see discussion above. If you're working on this, do you think a |
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 ContextWorkflowsBased on my understanding, there are a few ways signing is used in git today:
FormatsGit has 3 supported signing formats:
DesignPhilosophyPersonally, 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 backendsSigning 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 In the future, it would be good to look into:
Config optionsThe 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 stepsWith that in mind, I see this as a sensible approach for the first rollout:
Open questions
|
Wow, thanks for investigating and detailed reporting. Sorry it took me so long to reply. I simply forgot.
One thing to keep in mind is that the library crate ( 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
Sounds like a nice feature :)
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
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: arxanas/git-branchless#465 (comment) |
Just throwing some data points into the pot:
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.
Anecdotal, but the people I know who sign commits at all—including myself—fully rely on |
I want to extend a little on @rslabbert's report and just outline some UX design that I was thinking about:
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? |
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. |
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 |
Yeah, it's far from perfect. These days, I use
That sounds wonderful, thank you! |
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.
Ok I had a peek and have started working on adding an SshBackend :) It seems pretty straight forward.
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 |
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 Even the very git code you linked does basically just that |
I'm not sure I see the benefit here. I'm proposing adding a |
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 |
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 😅. I gave it a go today and it's great. One issue I had was, since I manage my
It'd be really nice if this error was less fatal. Perhaps printing something like this
Once #3142 gets merged, I'll probably look at opening a PR for this. It looks like it could be fun first |
Wouldn't it be very likely that the suggested command would also fail then? Oh, I see, not in your case where the
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 |
Just adding one use-case for commit signing to shed some light on potential drawbacks (if I understood the thread correctly): some people (I) have their GPG keys on a hardware token that require an indication of user presence (a physical tap on the token) every time it needs to sign something. If every change snapshot is indeed signed, this could mean two things:
It is also both a poor UX thing and a bad security practice to have the user validate a signing operation on a seemingly not-signing-related action (even though, in the case of I don't have a solution for this, other than wild and unsubstantiated thoughts like "delayed signing", not on every snapshots, but either manually or on rarer occasions. |
Well, I've been having yubikey signing for a long while and I can confirm that a snapshot takes 200ms and (auto-)rebases take 200ms times the number of commits 🤷 A separate script thing that makes a notification when it's waiting for a tap is a lifechanger because yes, literally every single time there's a second of confusion when a jj operation seemingly hangs because you need to tap it. And I still configured it to just ask for the pin once and then cache it with no taps, which is meh security too Another point (that someone in discord raised) is that snapshots can contain invalid/testing/incomplete code and if it gets pushed somehow then this "verified" bad commit has a chance to be abused somehow - in that same conversation an idea to only sign commits when pushing (and yeah that'd be a rewrite) was floated. One stop-gap solution I just thought of is that there could be a push filter revset, similar to immutable commits - and you'd configure it to only accept signed commits (with a revset function that checks for signature presence) and disable sign-all - this'd also require finally implementing |
Can this issue be closed to prevent some miscommunication? The feature has been implemented in #3007.
The comment was from April. It's October and I've just realized that the basic singing support was released a couple of releases ago 😅. The search engines put this issue as the top result, with the Opened status, so no wonder. |
I think what remains is a |
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
The text was updated successfully, but these errors were encountered: