Skip to content
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

[RFC 0100] Sign commits #100

Closed
wants to merge 11 commits into from
Closed

[RFC 0100] Sign commits #100

wants to merge 11 commits into from

Conversation

L-as
Copy link
Member

@L-as L-as commented Aug 14, 2021

  • Add a mechanism by which Nix can verify trust of new Git commits
  • Speed up fetching Git repositories with Nix (since non-Git inputs can't be verified)
  • Use this in Nixpkgs, and after a transition phase where we develop the tooling,
    disallow insecure merging.

Rendered

Add mechanisms by which Nixpkgs can be trusted.
Specifically:
- Require that authorized committers (people who commit directly to Nixpkgs) sign their commits eventually
- Add a mechanism by which flake Git inputs can be verified for trust and signatures
- Speed up fetching Git repositories with Nix (since non-Git inputs can't be checked)
rfcs/0100-sign-commits.md Outdated Show resolved Hide resolved
@hmenke
Copy link
Member

hmenke commented Aug 14, 2021

I'm very interested in this and I'd like to shepherd this (if it's not a problem that I've never done this before).

@kloenk
Copy link
Member

kloenk commented Aug 14, 2021

An open question for me, for which I did not find an answer yet. How does this handle the GitHub signing key. Most PRs are merged via the GitHub web interface. IIRC this uses a special signing key for that. Is this key Added to the /.authorized-committers.nix?

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

@kloenk

Edit: Through a transitional phase we would still merge through the web interface, trusting GitHub's PGP key, but eventually, once support in GitHub's Desktop interface and CLI (cli/cli#1318) has been implemented, we will make another RFC to decide whether to exit the transitional phase and untrust GitHub's PGP key.

This is explained in https://github.com/ngi-nix/rfcs/blob/sign-commits/rfcs/0100-sign-commits.md#signing-the-commits. For Nixpkgs, merging through GitHub would not be possible, since we want to trust the individual authorized committers and not GitHub (which would not be much more secure than what we have now).

Of course, it is possible to just add their key to your /.authorized-committers file, but I personally wouldn't do it.

@ryantm
Copy link
Member

ryantm commented Aug 14, 2021

For Nixpkgs, merging through GitHub would not be possible

This requirement would severely limit my ability to contribute to the project. Right now, I mostly merge leaf package updates where the maintainer has approved from my phone. I guess it's possible to set up some GPG thing on my phone but it sounds like a lot of work and more clumsy unless you know some great app that shows GitHub diffs, comments, action check statuses and allows signed merges.

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

Edit: This is now mentioned in the RFC, and is required to exist before we move away from using the web interface.

@ryantm You are completely right. https://github.com/mobile/ exists, but it's not open source, so we can't add signing functionality to it even if we wanted to.
There are three solutions for merging from your phone I can think of:

  • We could make a script for nix-on-droid that takes the URL as parameter and does the merge for you automatically. (easiest solution)
  • We could make an app, that you can share the URL to, which then does the merge.
  • We could make a browser extension for Firefox Nightly (you can do that now), that does the merge in WASM.

@tomberek
Copy link
Contributor

I'm interested in the "sign tags only" alternative. Is there any available discussion or rationale behind accepting/rejecting that alternative?

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

@tomberek

  1. Users of Nixpkgs rarely fix their used revision to a tag.
  2. Not signing every commit prevents you from using the algorithm described.
  3. Somewhat the same thing can be achieved by the "redirection flake" design describe in future work.

@cole-h
Copy link
Member

cole-h commented Aug 14, 2021

I like this in theory, but I don't like GPG. It's a chore to renew keys, and I'd really rather not deal with it. My current key expired back in January, and I haven't renewed it (nor do I plan on doing so). If GitHub were to support e.g. signify, minisign, or some other similar tool, that would be fine. But they don't, and I don't want to touch GPG.

N.B.: I only skim-read the RFC to see if there was any mention of alternatives to GPG.

rfcs/0100-sign-commits.md Outdated Show resolved Hide resolved
@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

Edit: You can use https://github.com/withoutboats/bpb/ or https://gitlab.com/wiktor/git-gpg-shim to avoid GPG.

@cole-h

I like this in theory, but I don't like GPG. It's a chore to renew keys, and I'd really rather not deal with it. My current key expired back in January, and I haven't renewed it (nor do I plan on doing so). If GitHub were to support e.g. signify, minisign, or some other similar tool, that would be fine. But they don't, and I don't want to touch GPG.

You could use other PGP clients technically. Sequoia looks promising, and it should be possible to hook it into git's signing functionality quite easily. You'd have to implement a subset of the functionality here as needed by git.

I do however agree that GPG is a pain to use. I personally use Sequoia to manage my keys and only import them into GPG for signing commits.

@picnoir
Copy link
Member

picnoir commented Aug 14, 2021

I second @ryantm on that one. Adopting this PR would entail a massive workflow disruption for me.

I'm personally not against moving to a more "git-native" workflow instead of the forge-based we currently have. However, I feel like this should not be treated as a technical detail buried in a RFC whose main topic is signing commits. This is a massive change having some serious technical and social repercussions.

Nix and Guix have a radically different development workflow. I'm really not convinced we can adapt this signing mechanism as is for a workflow heavily dependent on a PR-based web forge.

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

@NinjaTrappeur

Edit: This has been accounted for and is now part of the RFC.

I'm not sure I quite understand, are you also talking about merging on your phone? If so, I understand that it would need some solution to replace the current web interface.
If you're worried about merging on other devices, then you'd just have a script to execute the commands specified in this PR. Everything would stay the same, except pressing the merge button.

@sternenseemann
Copy link
Member

sternenseemann commented Aug 14, 2021

Everything would stay the same, except pressing the merge button.

Which is precisely not “everything staying the same”.

Edit: To add to that: Recent additions to our infrastructure have mostly been to reduce the necessity to switch to a terminal and run git commands: Think backport action, rebase action etc.

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

Everything would stay the same, except pressing the merge button.

Which is precisely not “everything staying the same”.

Edit: Most users would likely use GitHub's Desktop interface

That is what "except" means.
You would still:

  • Use the GitHub diffs
  • Discuss it on GitHub
  • Review it on GitHub
  • Check the CI on GitHub
  • Handle labels on GitHub
  • etc.

For desktop users, you pressing the button would be changed by you running some script with the PR number as argument:

#!/bin/sh
cd /my/nixpkgs
git switch master
git pull
gh pr checkout "$1"
git switch master
git merge -m "Merged $1" --no-ff --gpg-sign -
git push origin master
gh pr close "$1"

For example as:

nixpkgsmerge 123

You wouldn't do any rebases or similar in the terminal. You'd run this one command. If you're on mobile, of course it's a bit more annoying to type in the command, but this is really all you'd do.

@Profpatsch
Copy link
Member

I would rather not sign any commits, because of the reasons that have already been listed:

  1. GPG is a pain to deal with and I don’t really trust the friction will help us get more maintainers
  2. As a maintainer I do not vouch that the code isn’t malicious, so having me sign the commit does look more like a blame game (see also: plausible denyability)
  3. Github already signs merge commits (with a github key)
  4. Our identity management is mostly done by github anyway, which is how people check who did a thing and who commented/reviewed.
  5. Mobile workflows would suffer if a maintainer’s GPG key is required

👎 from me.

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

@Profpatsch
How do you feel about just trusting GitHub's PGP key then? That is possible with this system too. Whether to trust GitHub or not is policy and not ingrained into the system.

Edit: This is now done in a transitional phase.

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

I also don't see how this affects how many maintainers there are?

@Profpatsch
Copy link
Member

How would that improve anything? Git refs are already content-addressed, so if somebody tampers with the data, the hashes will change

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

This is a general system for more than just Nixpkgs, it applies to all flakes and is usable by all flakes, as mentioned in the design section.

As for why it would improve the situation for Nixpkgs, HTTPS only secures the channel, not the data. Let's say we fully trust GitHub, but some day they're hacked. Somebody gets inside their infrastructure and modifies the data before it's encrypted with TLS. With this system in place, the modified data would not be signed by the GitHub PGP key. It would raise the barriers, since they'd have to gain access to the APIs for signing the commits with GitHub's private PGP key.

In my honest opinion, this would be a minor improvement to security (nonetheless an improvement), but even if Nixpkgs doesn't make full use of it, other projects can. In addition, if you trust GitHub's key, no part of the workflow would change at all.

@L-as
Copy link
Member Author

L-as commented Aug 14, 2021

Also, Git refs are practically not content-addressed due to SHA-1 being broken, but it will hopefully eventually be replaced by SHA-256.

@nathantypanski
Copy link

nathantypanski commented Dec 2, 2022

I share other users' concerns regarding both PGP/GPG and S/MIME signatures. Both the GPG and S/MIME formats are complex and rife with cryptographic problems emblematic of legacy cryptography engineering practices. Neither is a good choice for modern tooling.

Luckily, we now have better options. GitHub now supports SSH signing commits, documented here. I have found this much more reliable and easy to use than GPG signatures, and it can piggyback on the SSH keys found on your GitHub profile which may be fetched at https://github.com/<username>.keys.

I believe that if NixOS were to implement commit signing with SSH signatures, we could address many of the usability and file format issues surrounding signed commits with other software.

@KFearsoff
Copy link

KFearsoff commented Jun 13, 2023

I believe that if NixOS were to implement commit signing with SSH signatures, we could address many of the usability and file format issues surrounding signed commits with other software.

100% agree. Let's use SSH signing, it's easy to wrap your head around, easy to setup and more secure. PGP/GPG is a nightmare that can actually reduce trust, due to being such an old horrible piece of junk.

@L-as
Copy link
Member Author

L-as commented Nov 1, 2023

I still think about this every once in a while and in reality this is a very small part of what is necessary.

The truth is NixOS is in many ways insecure wrt. the supply chain, with many small holes.

I think fixing Git comes first though, SHA1 is a big problem.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/git-verify-in-band-commit-verification/38991/1

@ngi-nix ngi-nix closed this by deleting the head repository Mar 20, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/from-whonix-to-nix-hex-means-six/46339/1

@AkechiShiro
Copy link

AkechiShiro commented Jun 1, 2024

Hello @L-as any explanation on the deletion of the head repo ?

Also any way forward with this valid issue ?

@L-as
Copy link
Member Author

L-as commented Jun 2, 2024

@AkechiShiro

This was done part of Summer of Nix I think originally. They deleted the repo. It's not under my control .

I don't think the RFC as it is now should be discussed anyway, it's not in a good place.
But I would prefer keeping the RFC number since it's nice and round.

We fundamentally rely on GitHub, and one of my motivations was not relying on GitHub to be honest,
but I don't think this is at all realistic.

I would probably also restrict the scope to just the Nix implementation, and not Nixpkgs, since there are fewer people working on Nix.
Since Nix has concrete releases you could just have the Nix team sign the release tarballs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.