Navigation Menu

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 0034] Expression Integrity #34

Closed
wants to merge 3 commits into from
Closed

Conversation

lrvick
Copy link

@lrvick lrvick commented Sep 29, 2018

No description provided.

@lrvick lrvick changed the title [RFC 0035] Expression Integrity [RFC 0034] Expression Integrity Sep 29, 2018

### Notes

* Local building is required for integrity as no trusted cache system exists
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the misunderstanding lies in "cache system I can trust"/"trust-less cache system" vs "cache system that requires trust"

Copy link
Author

@lrvick lrvick Sep 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the current artifact hosting system is compromised, it is game over and clients will blindly trust malicious builds.

It is only when you have signing by each maintainer -or- a highly transparent reproducible build system that clients will have a way to notice if an artifact server is compromised. Much the same way debian users can trust any random http/ftp mirror today, because the client will verify the hash and the maintainer signature on that package. Any system that trusts a single individual or computer is not compromised, is bound to be abused.

This RFC if implemented would only protect users building from source as only the nix expressions themselves would have signatures by both the creator and reviewer.

Copy link
Member

@grahamc grahamc Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe you misunderstand what our Hydra is? It doesn't host artifacts, it is a build server. Its results are uploaded to our binary cache. Nothing a maintainer builds goes to the binary cache. Maintainers cannot upload to the binary cache. There is no way for a maintainer to sign the NAR in the end, because the maintainer has no part in its creation. Other distributions do this because they put a great deal of trust in the maintainer to not backdoor the artifacts. We don't, because, well, we don't.

Copy link
Author

@lrvick lrvick Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that it blindly pulls code from github without verifying it, then blindly builds/signs artifacts. This is honestly awful security. Instead of an individual maintainer/reviewer pairs that can at worst only backdoor their own package, you have a central system that can backdoor any package at any time, and so can those with remote access to that build system.

Because hydra signs anything without code review or verification of signatures from code reviewers it means you need absolute trust in Github, in Hydra, the network that connects them, every person with shell access on the Hydra machines, and every one with push access. This is imo highly irresponsible and honestly puts every individual with that much solitary power in a non obvious position of personal risk.

As one practical example: If I had a remote shell exec 0-day I would use it gain remote access to hydra to inject a small hard to detect change just before compilation in major bitcoin wallet packages that compromises the random number generator allowing me to factor keys. I would then wait for a big enough wallet balance years later and take it all. I could also blackmail a hydra maintainer to do it for me as the potential payout is worth it. I could also phish the github credentials of one person with commit access, turn off notifications, and rewrite history quickly just after the next change to that package. Hydra can't hope to deliver trusted outputs until it can verify its inputs and independant copies of it can be run with different maintainers that all get the same results (reproducible builds). We can't even begin to solve that problem until we have a trusted signed tree of package definitions without a SPOF.

@ElvishJerricco
Copy link

ElvishJerricco commented Sep 29, 2018

This proposal seems to expect signatures to come from contributors who open PRs. Don't we actually want signatures to come from reviewers with commit access who actually merge the PR to master / staging? Nixpkgs has a lot of contributors, so the random PR author is by no means someone we should ask everyone to trust. Trust should be placed solely in those that are trusted with commit access to the repo.

Furthermore, I think this proposal needs to spell out how changes get merged, mechanically. My opinion is that reviewers should have to checkout the rev locally on their machines, merge it to master, sign the merge commit, and push. It's a bit of overhead, but if the point of this is to remove our reliance on GitHub's trustworthiness, then we can't trust the signing key they use with the web UI merge button.

@lrvick
Copy link
Author

lrvick commented Sep 30, 2018

This proposal seems to expect signatures to come from contributors who open PRs. Don't we actually want signatures to come from reviewers with commit access who actually merge the PR to master / staging? Nixpkgs has a lot of contributors, so the random PR author is by no means someone we should ask everyone to trust. Trust should be placed solely in those that are trusted with commit access to the repo.

Any first time contributor should have careful scrutiny and have their signing key documented. From there we will be able to know future updates are by the same person, and not an account takeover. This helps build trust and prove authorship. If you don't do this it makes social engineering attacks easier since anyone can commit as anyone else.

See: https://github.com/lrvick/security-token-docs/blob/master/Use_Cases/Commit_Signing.md

Furthermore, I think this proposal needs to spell out how changes get merged, mechanically. My opinion is that reviewers should have to checkout the rev locally on their machines, merge it to master, sign the merge commit, and push. It's a bit of overhead, but if the point of this is to remove our reliance on GitHub's trustworthiness, then we can't trust the signing key they use with the web UI merge button.

If you use the patch-id/git-notes combo as I mentioned then it will not matter at all if you use the big green merge button as the git-notes signatures will persist and remain valid even if github or any bot does the actual merge.

If you chose -not- to do the patch-id/git-notes route then a local git merge would indeed be required.

Native git merge commit signing has some drawbacks:

  • You can't squash commits without invalidating the signature
  • You can't however has no way to have multiple reviewers sign
  • You -must- merge locally on the command line
  • You can't have more than one signature on security critical components
  • You can't add new signatures later

Signing the patch-id in the git notes interface as metadata addresses all these issues. I favor this approach but there was some debate on IRC so I left it as an option for discussion.

This feature is shipping shortly in the hashbang "git signatures" project but you can just as easily do it by hand or with your own tooling via the one liner I supplied.

@Ekleog
Copy link
Member

Ekleog commented Sep 30, 2018

@lrvick

Any first time contributor should have careful scrutiny and have their signing key documented. From there we will be able to know future updates are by the same person, and not an account takeover. This helps build trust and prove authorship. If you don't do this it makes social engineering attacks easier since anyone can commit as anyone else.

I think there is a quid pro quo on the word “contributor”.

In NixOS (I hope I don't speak only for myself), I think we have two kinds of people:

  • Contributors, basically anyone who writes code that is aimed at ending up in nixpkgs: PR authors, committers, etc.
  • Maintainers, also called committers, who have the commit bit set and are allowed to commit directly to nixpkgs.

Requiring all committers to sign their work would be unreasonable, as (gut feeling) a big number of them are, at least at first, only doing drive-by PR sending, and increasing the hurdles on that one would make contribution harder. In addition, we wouldn't gain much from it: it'd be like Linux requiring that everyone submitting a patch signs it, anyway half the contributions are first-time contributions so there would be no possible scrutiny.

On the other hand, requiring committers to have a key and sign any PR they merge is reasonable, and does help with the following threat models (which are currently unmitigated):

  • A compromised or malicious GitHub employee adds a commit with a backdoor to the repository: this threat model is completely eliminated
  • A compromised or malicious nixpkgs committer adds a commit with a backdoor to the repository: this threat model makes the identity of the culprit obvious as soon as the backdoor is discovered, which should incentivize against it

The only use case from your link not covered by having only maintainers signing the merge commit or similar is the part “reputation hijacking where someone might commit malicious code to a repo with a naive maintainer hoping they will blindly trust your name without careful inspection”. Honestly, I think that in nixpkgs anyone who reached this point will anyways be given the commit bit quite soon.

Now, even committers sometimes make PRs for others to check, but usually what I've seen is mostly large code changes that touch everywhere and are sent as “RFC”s. In these cases, usurpation of identity would most likely end up being known by the usurped person, because it doesn't go fast and there are cross-medium talks about them anyway.

So overall I do think there would be a (small) security benefit by requesting all contributors to also sign their commits, but that it is not actually a good trade-off: too much convenience lost for too little security. It's like saying the only secure computer is the one that's unplugged from every network (including the electric grid)… that's likely true, but it's not a good trade-off :)


Now, about patch-id signing, there is a question I wonder about: is it actually secure? See https://www.reddit.com/r/rust/comments/6b9tsl/merging_patches_and_pijul/dhkzgou/ for an example why signing only patches can still lead to vulnerabilities depending on the order in which they're applied / merged. (thread is about pijul but this example would merge silently on git to)

@lrvick
Copy link
Author

lrvick commented Sep 30, 2018

I would argue people that want to do drive by commits should do so in the Nix User Repository which is full #yolo mode.

From there someone willing to follow signing rules can cherry pick things from NUR over to the regular repo which has strict guidelines. Even so, not requiring signing these days is to me like not requiring public keys for ssh. Generating a key is really easy and worth anyone taking the 10 mins to setup to prevent impersonation. I find most people in this space are happy to learn if asked.

As far as merging patches out of order: in git-signatures we are solving this by having the tool -also- sign the current master HEAD the patch is intended to merge on top of. As long as this patch merges as the most recent change on top of the signed ref for the given changed files then order is verifiable.

@Ekleog
Copy link
Member

Ekleog commented Sep 30, 2018

This… sounds like too much overhead to actually work. IMO security is highly important (heck, it's $dayjob), but when it comes in direct opposition with convenience, it's not going to work… especially when there's no threat model it significantly improves (IOW, more is not necessarily better, people will just work around it)

I do agree it'd be a better world if people all generated keys and signed all their commits. But I'm really not seeing any net advantage to signing commits apart from merge commits: anyway people will likely mostly review using the github interface (and thus “trusting” information from there), with maybe (if we're lucky) a double-check locally before signing. And, TBH, it's not that bad: at least the scheme'd still cover the case “GitHub employees adds in a commit” and “contributor pushes a backdoor”, it would simply not cover the case “GitHub website is special-cased to display wrong code for a review”. (which sounds quite less likely to me)

Also, NUR is great, but it's not discoverable and I really don't think Archlinux's trend towards pushing everything to it is a good thing for security, as it forces one to install quite a lot of packages from AUR when using it, thus either hugely loosening security or forcing review of many more packages. IMO most things should be in nixpkgs, with experiments, things rejected from nixpkgs (eg. backports of major updates to releases) and impure stuff (eg. mozilla's current rust-overlay) in NUR.

tl;dr: I think we'd be better off reminding maintainers that they should really review the code and not just trust whoever they see it being from, and reviewing locally only, than forcing every contributor to do quite a bit more work.

@oxij
Copy link
Member

oxij commented Sep 30, 2018 via email

@Ekleog
Copy link
Member

Ekleog commented Sep 30, 2018

@oxij I think you are missing two threats models that are helped by having maintainers (not contributors, thus no problem for drive-by contributors or editing patches) signing every merge commit:

  • A GitHub employee is compromised, and introduces a backdoor in an additional commit. Signing each commit on master would prevent such a commit from ever entering hydra / people fetching the repository and building from it
  • A nixpkgs committer is compromised. Having the committer sign their patch won't stop them from adding in a backdoor, but it will make the backdoor be associated with the committer's name. This should be enough to discourage “casual attackers”, and to make motivated attackers actually known and be able to kick them out of the project

Basically, currently the trusted base is:

  • Nixpkgs committers (without possibility of associating wrongdoing to its author once detected)
  • GitHub (without possibility of associating wrongdoing to them)
  • Hydra (with possibility of automatically detecting wrongdoing for reproducible packages)

With enforced signatures on master, the trusted base would become:

  • Nixpkgs committers (with possibility of associating wrongdoing to its author once detected)
  • Hydra (with possibility of automatically detecting wrongdoing for reproducible packages)

Basically, it'd be what you're doing at SLNOS with WoT-based code review, except every nixpkgs committer would be fully-trusted for this. Sure, it's less good, but more wouldn't be possible in practice (cf. PR backlog) and it's way better than nothing.

(not replying for the part about changing the build system to increase its trust as it has been put out of scope of this RFC, which focuses on being able to trust a git checkout / checkout from channel)
(for bootstrap binaries, well, I guess it's off-topic too, and I can't find the link again, but ISTR having read that someone did a bootstrap from a few-bytes “compiler” to gcc with GuixSD -- would still require that all languages be compilable from C, granted)

Now, for the SHA-1 issue of git, which is, I think, on-topic given we're thinking of ways to secure a git repository against unauthorized modification: it's not that big a deal currently (though it sure will become a big deal with time). Currently, we only know of collision attacks on SHA-1, which means that a malicious commit could only replace a previous commit that was designed to be replaced. Hopefully a committer would notice said previous commit was “weird” and refuse signing it. (oh, and also currently a single collision is known, and git has added a static check against it, but we do agree that assuming more collisions won't be discovered is not a good idea).

My personal hope is that git will make it possible to use another hash before a pre-image attack on SHA-1 is discovered :)

@oxij
Copy link
Member

oxij commented Sep 30, 2018 via email

@lrvick
Copy link
Author

lrvick commented Sep 30, 2018

Meanwhile, splitting repository into "NUR" and "regular nixpkgs" defeats the whole point, the hurdle of getting big changes into "nixpkgs" already produced Triton and SLNOS, if it were any higher most contributions would just contribute directly to "NUR" which would then become the de facto "nixpkgs" like Arch's AUR. Would you then propose signing all commits in "NUR" and making a new "NUUR" for drive-by contributors?

It is actually my goal to get NixOS to at -least- the level of security of Arch so I can switch from Arch to NixOS personally. I have had to install maybe 3 PKGFILES from AUR in years and reviewing just those 3 and building myself was no big deal. Brand new unvetted experiments end up there by people not willing to publish their public key and go through the process of publishing/signing in the main repos. Someone else may grab something from AUR may be willing to do that work and review/sign that package so it can become available to all Arch users.

Making all contributors sign their patches is not going to work for drive-by contributors and will prevent people with commit access from editing patches. IMO "don't ask, just fix" policy of SLNOS is the best thing ever, even without commit access to nixpkgs I now regularly do edits of other people's patches here in nixpkgs. Authorship is overrated anyway.

The very fact any of 100+ people could make a last minute change to reviewed code is crazy. If you wan to make a change make a signed commit in a PR and have a reviewer sign off on the change. No single person should ever have the right to execute code on other peoples machines without review. If I as an attacker want to target someone I know uses NixOS at a major company I now only need to pressure or blackmail one NixOS maintainer in order to sneak in a subtle drive-by change that allows me to compromise my target. Honestly giving any one single person the ability to make arbitrary changes to a widely used operating system is dangerous for them personally, not just the users. Chain of signed authorship and review is vital for all changes and should have signed documentation.

If you are really paranoid, the only solution to noted trust issues is to review all the patches yourself/with people you trust and build everything yourself. We do that (with some Web-of-Trust tooling over git and inside of our nixpkgs) at SLNOS (we don't review everything, though, only packages we use, reviewing everything is unrealistic with a handful of people).

Not many people have time to do this themselves which is why my end client being able to pull all author/maintainer keys and use them to prove all changes touched the systems of two of those people, then I have strong end to end evidence there was code review and no SPOF. Without signatures the evidence that reviews happened would be asking the Github API about PRs related to given commits which puts us back at SPOF.

NixOS recently switched to 2FA for this (NixOS/nixpkgs#42761).

This is nice but nowhere near good enough. First you must take seriously the fact there could be an exploit on Github or a way to bypass 2FA. I have personally been inolved with multiple security issues with Github. Trust me, they don't always get it right. Beyond the obvious like an exploit on Github to bypass 2fa or a compromised Github employee: Github allows terrible 2FA methods like TOTP, and SMS. TOTP is both easy to Phish (no domain checking), and anyone with a $20 TV tuner and 2tb of A5/1 rainbow tables in the US can intercept all SMS 2FA codes. Porting someones number is also trivial social engineering. Many people make this their backup 2FA making it the weakest link. Working in security I have personally seen these methods used in the wild and had to deal with the aftermath. This is not sci-fi stuff. It is easy. Only U2F 2FA has good phishing protection and Github does not let you enforce that. Github only confirms that -some- form of 2FA is enabled, even a near useless one.

Put simply, it is unreasonable to trust Github to be anything more than a place to upload and distribute externally signed code.

I agree that this (and only this much) would be useful, I just don't see it being practical. You would need to distribute maintainer's pubkeys to at least hydra and, possibly, to users out-of-GitHub (and out-of-Amazon, or else it doesn't make much sense either) to mitigate potential malicious third-party infrastructure. Key distribution is hard, I don't see that scaling and working well without a bunch more centralized infrastructure with trusted hardware physically owned by NixOS people.

Key distribution is a solved problem between HPK and WKP. We need only note the public key ids of authors/maintainers we verified with TOFU then every client can pull keys directly from their public mirrors.

Arch handles this quite eleganty: https://wiki.archlinux.org/index.php/Pacman/Package_signing

My personal hope is that git will make it possible to use another hash before a pre-image attack on SHA-1 is discovered :)

For sure and Torvalds has started the gears turning towards this. Breaking SHA1 is still crazily expensive and all we can hope in security is to keep the attack cost higher than the potential gains.

@lrvick
Copy link
Author

lrvick commented Sep 30, 2018

Adding to the above, I want to be clear in that we need to support use cases where we do -not- have to trust the current SPOF Hydra deployment. Imagine I want to use NixOS in a secure environment where I can't risk the maintainers of Hydra backdooring me (or their key being stolen) I would want to run my own private deployment of Hydra that can verify upstream signing, and create a private package cache. This is not possible to do securely unless there is an authoritative list of key fingerprints I can validate against.

@lrvick
Copy link
Author

lrvick commented Sep 30, 2018

Also relevant, I just found out Guix already signs -all- commits with maintainer keys and has had an ongoing discussion on solving for verification for pull/hydra.
https://bugs.gnu.org/22883

@oxij
Copy link
Member

oxij commented Oct 1, 2018 via email

@lrvick
Copy link
Author

lrvick commented Oct 1, 2018

@oxij Every maintainer of gentoo, debian, arch, fedora, Guix, and countless other distributions has no problem individually signing their respective concepts of a package. I get that GPG is complex for casual non technical users but are you seriously implying Nix maintainers are not smart enough to type the 3 or 4 commands required to tell their Yubikey (or similar device) to generate a key?

From there it is literally just "git commit" and tap your key when it flashes. You never have to run a gpg command yourself for typical signing flows. Signing happens automatically. I have deployed this to hundreds of people and never had anyone accidentally leak keys, because they can't see the keys in the first place. CI in turn makes sure the signing rules are met, and does verifications. For the alternative signing flows I mentioned I am working to get those covered in the git signatures project so a maintainer would need only type "git signatures add" ( https://github.com/hashbang/git-signatures )

I encourage you to try a modern smartcard based commit signing workflow yourself. It is really simple to setup and hard to screw up. And yes, I am assuming smartcard based flows because exposing private keys to system memory -is- asking for trouble. If users don't have a smartcard they also don't have any reasonably useful 2FA i(U2F) and probably not someone you should want having master commit access.

See: https://github.com/lrvick/security-token-docs/blob/master/Use_Cases/GPG/Simple_Setup.md

I repeat, I'm not against accommodating a support for git signatures, or encouraging it. I'm against requiring it like we do for 2FA as it will turn the whole thing into another security theater like 2FA is.

As long as anyone can cowboy a change all the way to users either at the VCS level or hydra level without any review or way for end user package managers to notice... then it really is security theatre. Signing only has value if it is 100% and every maintainer publishe their keys, like virtually every other Linux distro.

Arch and NixOS have different packaging models. I feel that a big chunk of what is awesome about NixOS comes from the ease of making changes to a system that is build from a single repository. The difference is similar to a difference between editing process of a scientific article versus editing of a Wikipedia article. I want nixpkgs to be more like Wikipedia, I think random people should be able to edit random packages without asking permission. That's the point, I would be against any system of signature restrictions that would prevent that.

It sounds to me like you are saying that the many single points of trust I pointed out in NixOS today are a feature and not a bug. That being able to experiment and play quickly is more important than following industry standard code integrity practices providing a secure and mature operating system useful for servers, workstations, individuals, or companies. If that really is true and you speak for the NixOS team at large, then I suppose you can disregard this RFC.

@Ekleog
Copy link
Member

Ekleog commented Oct 1, 2018

(just answering a few points from the discussion above that I didn't see answered yet)

@oxij

By contrast, having hydra sign channels with the key it already uses for binary caches solves the "silent compromise of GitHub" issue essentially for free.

Can you explain in what way? Silent compromise of GitHub is someone from GitHub pushing a commit with a backdoor to nixpkgs with a spoofed commit author name. I don't see how hydra signing what it sees from GitHub would help here.

I don't think it matters that much. Sure, having that possibility is better than not, but I'd prefer to have a rapid way to stop hydra from distributing malicious things over ways to assign blame.

If you don't care about deniability then indeed this scheme won't be of any use: the whole point of it is to ensure traceability of commits. And yes, I do think compromise is much less likely when it's traceable than when, if a backdoor is found, there is no way to answer the “who did that?” question.

I agree that this (and only this much) would be useful, I just don't see it being practical. You would need to distribute maintainer's pubkeys to at least hydra and, possibly, to users out-of-GitHub (and out-of-Amazon, or else it doesn't make much sense either) to mitigate potential malicious third-party infrastructure.

A solution to this is to put the keys in the repository itself. This way, there's TOFU at play, and people who want extra security can verify a maintainer's key and check the history from a commit signed by this maintainer.

Once the repository has been initialized any subsequent fetch must be signed by keys already present, which means that compromised infrastructure can do no more than just DoS.

I repeat, I'm not against accommodating a support for git signatures, or encouraging it. I'm against requiring it like we do for 2FA as it will turn the whole thing into another security theater like 2FA is.

Then it's good, because this RFC should in the first step provide support and encourage everyone using it. And only once it'll have been actually applied by enough maintainers will it switch to the phase of actually enforcing it. But I can't wait the moment when we'll be able to enforce it.

Also, this RFC can (and should) be followed-up with an effort to remove trust from hydra, by having multiple independent CI systems build the packages concurrently, and check for reproducibility. But for this to have a meaning, the nix expressions evaluated must first be trusted. And I really don't think we can reasonably rely on having access to the cache to identify whether a given nix checkout should be trusted.

That said, your point does raise the question of tagging compromised commits as compromised. FWIW, here is what I remember from talks on #nixos-security. We need 3 features for security:

  • Trusting the nixpkgs checkout (tackled by this PR)
  • Trusting the build system (tackled by the reproducible builds effort, and not-done-yet tooling to use it)
  • Having a way to mark compromised commits as compromised
    In my opinion, this third point is really important, but requests more than just a “this commit is compromised”, as you mention. Because it shouldn't handle only willingly-inserted backdoors (what we're currently talking about), but also security vulnerabilities. And we don't want to just block all commits with known security vulnerabilities in any package, else no commit would ever be secure. So we'd need a more granular way to block compromised things, that should be handled by a third system.

@lrvick

No single person should ever have the right to execute code on other peoples machines without review.

I fully agree with this. Unfortunately, we just don't currently have the manpower to do so, as I understand it, which means it's just not possible to enforce that -- already enforcing one review would be a great first step, and with time maybe we'll be able to enforce two reviews.

[about git's SHA-1 usage] For sure and Torvalds has started the gears turning towards this.

That's off-topic, but FWIW last I checked (like one or two years ago) it was Brian M. Carlson who was working on this, and Linus Torvalds was not involved in it.

It sounds to me like you are saying that the many single points of trust I pointed out in NixOS today are a feature and not a bug. That being able to experiment and play quickly is more important than following industry standard code integrity practices providing a secure and mature operating system useful for servers, workstations, individuals, or companies. If that really is true and you speak for the NixOS team at large, then I suppose you can disregard this RFC.

(off-topic: no one speaks for the NixOS team, and that's actually an issue with RFCs, as no one is able to give a definitive “yes we'll do it” / “no we won't do it”)

I do agree with @oxij that the ability to quickly add packages is more important than following strict code integrity practices. However, that “more important” is actually just saying we need a balance between security and convenience.

However, having NixOS not sign anything is a problem to adoption. I know people who refused NixOS due to it.

So I think this balance is important, and must be kept in mind. And we shouldn't always lean towards convenience just because it has more weight, sometimes a little bit less convenience (ie. requiring proper practices from maintainers) can give a lot more security (ie. drastically reducing the attack surface).

@grahamc
Copy link
Member

grahamc commented Oct 1, 2018

has no problem individually signing their respective concepts of a package

We do sign our packages, NARs are signed by a well known key.

Also relevant, I just found out Guix already signs -all- commits with maintainer keys and has had an ongoing discussion on solving for verification for pull/hydra.
https://bugs.gnu.org/22883

This is a bug about their channel distribution which uses HTTP, which is a disaster. Not about additional verification they use over us. They use an HTTP URL, we use https://nixos.org/channels/.


I find it interesting that the Linux Kernel doesn't sign their commits?

@Profpatsch
Copy link
Member

And yes, I am assuming smartcard based flows because exposing private keys to system memory -is- asking for trouble. If users don't have a smartcard they also don't have any reasonably useful 2FA i(U2F) and probably not someone you should want having master commit access.

Sorry, but now that’s just crazy talk.

@tilpner
Copy link
Member

tilpner commented Oct 1, 2018

This is a bug about their channel distribution which uses HTTP, which is a disaster. Not about additional verification they use over us. They use an HTTP URL, we use https://nixos.org/channels/.

@grahamc AFAICT, the issue is the missing verification of integrity (and authenticity?) when Hydra takes nixpkgs and wraps it up in a channel. If Hydra could check for signed-ness of nixpkgs before updating a channel, attacks on and by GitHub would be prevented.

@grahamc
Copy link
Member

grahamc commented Oct 1, 2018

@tilpner I'm pretty sure it isn't:

Right now, when a user does a "guix pull", that pulls down the latest
repository of code from git, which is kept in a tarball. Once you
receive the latest code, this has some checks: what's the hash of each
package, etc.

Unfortunately, it's delivered over http:

this suggests to me that the tarball of code came from git, but not using git, and it is being served over http.

@zimbatm
Copy link
Member

zimbatm commented Oct 1, 2018

How would we do merges in a scenario where all the commits are being signed? The GitHub Merge button can't sign the merge commit for us.

@Ekleog
Copy link
Member

Ekleog commented Oct 1, 2018

@grahamc The point about the Linux Kernel is debatable: the one source of trust is Linus' repository, not GitHub (as is the case for us). Linus himself tag-signs the releases, which is enough to prove that every previous commit was indeed in his repository.

Now, the question is how the commits actually ended up in his repository. Currently, my understanding is he gives at least a cursory look at patches -- at least judging from rants about patches that made it past subsystem maintainers and were rejected by him for various reasons. As a consequence, I understand by it that he takes “responsibility” (moral responsibility, not legal responsibility, which is disclaimed by GPL) for all the patches that enter his repository.

Also, the kernel documentation states the following.

Some maintainers (including Linus) want to see pull requests from signed commits; that increases their confidence that the request actually came from you. Linus, in particular, will not pull from public hosting sites like GitHub in the absence of a signed tag.

So while all commits are not signed, some commits must still be signed when using public hosting sites. Also, this other page of the kernel documentation insists on the importance of signing the commits -- though when sending patches by email I can't find any mention that signing the mail would be mandatory from maintainers.

tl;dr: the big difference with the Linux kernel is that we don't have a “central source of truth” that is managed by someone who can take moral responsibility for everything that gets in, and instead we have a bazaar where everyone contributes randomly on GitHub without any coordination -- signing commits would allow to be able to trust this absence of coordination.

@Ekleog
Copy link
Member

Ekleog commented Oct 1, 2018

@zimbatm The idea is that instead of using the “merge” button, committers would instead do:

$ git fetch $NIXOS_REMOTE_NAME pull/$PR_ID/head:pr-$PR_ID
$ git pull
$ git merge -S pr-PR_ID
$ git push

(with appropriate tooling this could be simplified down / improved upon using git notes, I just put an example of what it'd look like with the basic git-only version)

This is a bit longer to type, but I'd think the gained security is worth a slight regression in reviewing efficiency.

@oxij
Copy link
Member

oxij commented Oct 1, 2018 via email

one used today that users can install from by hand as they please.

This could be an analogue of the Arch Linux User Repository (AUR) vs the
trusted/signed official repos.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually have something similar already at https://github.com/nix-community/nur

@Ekleog
Copy link
Member

Ekleog commented Oct 1, 2018

@oxij

My point is that with a proper response system as soon as somebody notices everyone will know. I.e. it will stop being silent. How do you signal that with git signatures alone? That's why I consider response system to be of higher priority than signatures.

Oh, if that is your point, then I do get it. However, I think the response system should be the same as a response system for known security vulnerabilities, because it'd kill two birds with one stone :) and thus doing it with only hydra tagging known-bad commits won't do it.

TOFU alone only defends you, not the new user.

Well, the new user is going to be protected from the point they first got the key on. Anyway, trust initialization is a hard problem. A solution can be to have, in addition to the in-git keys, a master key that signs (and revokes) all committers' keys designed only for trust initialization (so not that security-sensitive) and publish said master key's fingerprint during talks, etc. (like Qubes OS does). However, I think that'd be a good second step once TOFU will be implemented.

I also imagine that keys stored in the repo combined with verification of every commit are going to be really slow (as you would need to walk the whole history, verify each commit, checkout keys, update keychain, repeat).

I don't think it'd be that slow: you'd only need to check that the latest commit is signed (because it implicitly signs every previous commit, all committers should have a receive-hook or equivalent through aliases [receive-hooks don't exist yet] that prevent them from signing a commit on top of an unsigned commit), and then https://github.com/Ekleog/signed-git/blob/master/git-hooks/check-range-signed.sh can do the job by just looking at when the keys folder changes.

As I said, that would be nice, but the design of such a system is non-trivial. Meanwhile, one signed bit of "we might be compromised, be aware" is simple to implement and check via nix-channels and will make the response much smoother. Sure, open CVEs are important, but we are talking code integrity here, not response to security vulnerabilities.

To be honest, I think a proper response to CVEs is more important than both reaction against code injection (as you want it) and prevention against code injection (as this RFC attempts to do). Given reaction against code injection is also a subset of the problem of a proper response to CVEs, I would personally tend not to even try to get work done in the sub-case, to focus on the broader case of CVE vulnerabilities and especially tagging individual packages / modules as unsafe. But, IMO, that'd be a separate discussion :)

@zimbatm
Copy link
Member

zimbatm commented Oct 1, 2018

Since this RFC is about GPG signing commits, let's evaluate this idea thoroughly. The author cares about this particular thing so I don't think we will convince him that other security work is more important. At the end, all of these should probably happen

My main concern with the current proposal is that it doesn't give a detailed idea of how the review and merge process would look like. Would I still be allowed to review PRs using the GitHub UI? If not, how do I leave comments? Changing the maintainer workflow is a serious thing and should be considered carefully to not loose contributors. That means that we also need to be able to convince people that it's the right thing to do.

A high level view of things I imagine would have to be done:

  • package git-signature
  • document and fix the GPG smartcard interface in NixOS. The last time I tried with a yubikey I failed miserably.
  • gather everyone's GPG public key (by adding it to the maintainers-list.nix?)
  • update the contributing manual
  • spend time with contributors to help them with their setup
  • and finally maybe enforce GPG signing

I think the three first steps can happen regardless the outcome of this RFC. I would also be happy to start signing my merge commits if an extra section is added to the manual that explains how to do it. It doesn't have to be an all-or-nothing switch.

@Mic92
Copy link
Member

Mic92 commented Jan 17, 2019

I got now my yubikey. Our yubioath-desktop, that can be used for github 2fa, should be now also fixed: NixOS/nixpkgs#54215
I also started documenting the Yubikey in our nixos wiki: https://nixos.wiki/wiki/Yubikey

@Zimmi48
Copy link
Member

Zimmi48 commented Jan 18, 2019

@Mic92 Great! Although, it would be nice to make this page more comprehensive, and giving some context, for instance, by linking to whatever Yubikey basics this page does not cover.

@Mic92
Copy link
Member

Mic92 commented Jan 21, 2019

It would be great if someone could write a nix expression for a live image to generate a key: https://github.com/drduh/YubiKey-Guide#live-image
Should not be too hard considering that we already have documentation on how to setup our yubikey.

Also we have now a documented way to specify gpg-keys in the maintainers file: NixOS/nixpkgs#47663

@Mic92
Copy link
Member

Mic92 commented Jan 21, 2019

@lrvick if you are still interested in this rfc, it would be great if you could answer @shlevy question.

@Mic92
Copy link
Member

Mic92 commented Jan 22, 2019

I created a nix expression that builds an live-system image to generate keys offline: Mic92/dotfiles@6a48eee

It follow best practices of https://github.com/drduh/YubiKey-Guide#live-image

@lrvick
Copy link
Author

lrvick commented Jan 23, 2019

@shlevy Sure. I am not a NixOS maintainer and dont have as much time as I would like to be super hands on with this, but I would love to see this move forward as it is the blocker for me being able to use or recommend NixOS.

Any way I can be of help or anywhere else I can weigh in lmk.

@globin
Copy link
Member

globin commented Jan 24, 2019

Per meeting of the @NixOS/rfc-steering-committee this RFC is open for shepherding nominations.

@grahamc
Copy link
Member

grahamc commented Jan 24, 2019

I'm in favor of people choosing to use GPG for signing commits. I'm not interested in requiring people to do so at this time. I think there are other low hanging fruit of improvements like requiring PRs and reviews, which improves the wrench-factor by one whereas just GPG doesn't.

@Ekleog
Copy link
Member

Ekleog commented Jan 24, 2019

@grahamc I agree with you, and despite being in favor of this RFC I think we should postpone it until at least some tooling is ready to be tested. So let's go through the RFC process and see whether it ends up in postpone. :)

I'd like to nominate @oxij and myself as shepherds. And @grahamc, should he accept it, for his implication in the processes used by nixpkgs contributors and committers.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/rfcs-31-and-34-open-for-shepherd-nominations/1965/1

@7c6f434c
Copy link
Member

@grahamc requiring PRs hits the bottleneck, though; this RFC might be useful if (during the discussion) it becomes an approved set of recommended practices (and requests for better tooling) — getting Git to at least Monotone-type security hopefully shouldn't require continuous efforts, and having an officially recommended set of setup scripts might increase adoption.

@oxij
Copy link
Member

oxij commented Jan 25, 2019 via email

@globin
Copy link
Member

globin commented Feb 8, 2019

Per meeting of the @NixOS/rfc-steering-committee: Shepherding Team is @grahamc as leader, @edolstra and @Ekleog.

Also shepherds, please be reminded that it could make sense to have a chat on IRC or by video conference, to discuss your opinions and with the author to move this forward in an orderly and constructive way!

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/shepherds-chosen-for-rfc-34-expression-integrity/2074/1

@FRidh
Copy link
Member

FRidh commented Feb 17, 2019

Related discussion NixOS/nixpkgs#20836.

@grahamc
Copy link
Member

grahamc commented Mar 19, 2019

The shepherd team met today. I mistakenly left out @lrvick (RFC teams should be meeting with the RFC author/coauthors). We think our summary is still valid.

Our summary of this RFC and the feedback it received is as follows:

  1. The RFC describes an important threat model around trusting Contributors, GitHub, Hydra, and the binary cache. Some of these can be addressed by signing commits, but not all of them.
  2. The RFC as written is also under-specified. As it is written, this RFC cannot be applied because there is no detailed implementation plan.
  3. The RFC seems to misunderstand some of the implementation details of how Nix, Hydra, and the binary cache work.

Having signed commits does have significant benefits in attributing a malicious change to the malicious actor. We as a community would like to improve our infrastructure and source security.

Specifically, we want:

  1. Accountability of who committed what
  2. Verifiable channel updates, ensuring the source came from Nixpkgs and that our infrastructure verified it. One possible route might be signed channel updates like Hydra uses for the binary cache NARs.

We also would like to see a push to improve binary reproducibility, and we know this can help ensure Hydra is not compromised.

Moving forward, we recommend rejecting this RFC, and ask for new, smaller scoped RFCs be written where at least one author is deeply knowledgeable of Nix. Some specific RFCs we would like to see are:

  1. Incentivise signed commits
  2. Requiring signed commits
  3. Verifiable channel update integrity

Thank you for the RFC, we hope this improves NixOS's security story.

@grahamc grahamc added status: FCP in Final Comment Period and removed status: in discussion labels Mar 19, 2019
@grahamc
Copy link
Member

grahamc commented Mar 19, 2019

This is now in Final Comment Period, ending on Friday, 2019-03-29.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/any-interest-in-checkings-signatures-while-building-packages/8918/1

@L-as
Copy link
Member

L-as commented Aug 14, 2021

Relevant: #100

@Switcheffect
Copy link

No description provided.

*//#

infinisil referenced this pull request in nixpkgs-architecture/rfc-140 Jan 30, 2023
Improve detailed design section and other minor things
@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

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