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

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@lrvick
Copy link

lrvick commented Sep 29, 2018

No description provided.

@srhb

This comment has been minimized.

Copy link

srhb commented on 1f31e80 Sep 29, 2018

Thanks! A bit of an arbitrary place to throw comments at you, but here goes.

I don't think this proposal will fly if it conflates anything regarding nix expression authorship with binaries, binary hashes or the cache. I'll try to elucidate why:

I think it should stick to ensuring there's a way to verify the authorship of nix expressions. If this is not the case, you end up in a hairy mess of trying to define what a "package" is (this is not simple in nixpkgs, we don't really have that concept, we just pretend to.)

Nix should continue functioning completely if there is no cache. Caches are not fundamental to Nix but simply (very nice) conveniences, and the security model is imo flawed if it ever assumes the existence of a cache. People need to be able to say "yes, this nix expression was signed by trusted people, therefore it is safe for me to build it" regardless of whether substitution takes place or not.

Trusting a substituter/cache to actually do substitution benevolently also seems completely orthogonal to trusting the original nix expressions, to me. (That's what we have signed NARs for. I don't know if that's considered sufficient, since there's no chain from the original, yet-to-be-created signed expressions, but still, it's not the same problem.)

Regarding reproducibility: While the situation is clearly better than I thought from looking at Debians data, we do not have full reproducibility yet, and on top of that we cannot expect contributors to build eg. Chromium. "Packages" (for want of a better word) are generally not built before they land in the repo, but after, and, reiterating, the substitution mechanism is not fundamental but a convenience.

Lastly: Given your knowledge, it seems the right thing to tackle is how to trust the nix expressions that people evaluate. Solving that problem appears to me to be hard, but I'm sure you know your stuff. The added problem of trusting the substituter to do the right thing after "trusted nix expressions" is solved seems small and self-contained.

Thanks again for writing this up. :)

This comment has been minimized.

Copy link
Owner Author

lrvick replied Sep 29, 2018

My thinking was that security concious users should have a configurable way to only trust a cached artifact is if there is a known signed hash for a binary.

I assumed it may be easier to introduce this change at the same time as signing to kill 2 birds with one stone if we are going to go through the work of improving package integrity processes.

This fits with the overall theme of package integrity as it is my assumption most users will install artifacts from cache vs building from source most of the time.

A contributor/maintainer having to build/hash locally can be avoided. You could also have an autoscale group of beefy CI systems that independantly build a nixpkg description and report back their hashes as comments in the PR. Should probably not merge nixpkgs that can't build anyway so 2 birds.

Then the maintainer can then note that if the CI servers agree on a hash, it can be added to the nix expression. If they don't agree, then merging can still proceed, but the cache will not be able to be trusted for that build by users running a more secure configuration.

I can break out that scheme into a standalone RFC if you think that makes more sense, but I do think it is a critical component to bring package integrity to the typical user, who won't be locally compiling most packages from source.

This comment has been minimized.

Copy link

Ekleog replied Sep 29, 2018

@lrvick I agree with @srhb that the trusting the build system and trusting the build descriptions do not need to be conflated, and the work necessary towards the two seems orthogonal to me.

The idea of having groups of beefy CI systems that independently build a nixpkgs description has already been raised, and there's work towards that being done (eg. with cachix, or the overall effort towards build reproducibility). I also think there are already ideas better than the one you put forward here: the last one I remember of being having more or less each CI build and sign the built packages with their keys, and then the user can decide to attribute more or less trust to each CI system. Individual users could also contribute by compiling things locally, and paranoid users may want to opt-in to always rebuilding a random X% of downloaded packages and checking compatibility with what the cache returned (making it likely any attack by the CIs would be caught locally by someone).

All this being secure because the signature includes the store path. So this means that by just signing like a binary cache, you sign the association between a store path (ie. a nix expression) and a binary. And these signatures can be totally independent from the .nix, which would make for even more security than the scheme you're proposing, while requiring fewer modifications to the workflow nixpkgs (though more to the ecosystem around it, like nix, hydra & co.).

tl;dr: yes, I think this should at least be split off into a separate RFC, if not completely left for later consideration :)

This comment has been minimized.

Copy link
Owner Author

lrvick replied Sep 29, 2018

@Ekleog I would very much like to see the RFC for that then because it is really the other half to this proposal that will be required before NixOS binary installs can be trusted.

I'll drop those bits from my RFC then.

@dpflug

This comment has been minimized.

Copy link

dpflug commented on rfcs/0034-package-integrity.md in 1f31e80 Sep 29, 2018

"Build a nixpkg and add the hash of the binary to it", to match verb tense of NixOS#1

@dpflug

This comment has been minimized.

Copy link

dpflug commented on rfcs/0034-package-integrity.md in 1f31e80 Sep 29, 2018

"adding" is duplicated

@dpflug

This comment has been minimized.

Copy link

dpflug commented on rfcs/0034-package-integrity.md in 1f31e80 Sep 29, 2018

AUR may be a stretch to include. It's always been said that users are taking their security into their own hands when using it.

This comment has been minimized.

Copy link
Owner Author

lrvick replied Sep 29, 2018

I mention AUR below to clarify this may still be something desired

@dpflug

This comment has been minimized.

Copy link

dpflug commented on rfcs/0034-package-integrity.md in 1f31e80 Sep 29, 2018

Phrasing is awkward. Any successful attack on GitHub that allows MITM or code modification would work.

lrvick added some commits Sep 29, 2018

@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

This comment has been minimized.

@grahamc

grahamc Sep 29, 2018

Member

eh?

This comment has been minimized.

@tilpner

tilpner Sep 29, 2018

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

This comment has been minimized.

@lrvick

lrvick Sep 29, 2018

Author

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.

This comment has been minimized.

@grahamc

grahamc Oct 1, 2018

Member

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.

This comment has been minimized.

@lrvick

lrvick Oct 1, 2018

Author

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

oxij commented Sep 30, 2018

@Ekleog

This comment has been minimized.

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

This comment has been minimized.

Copy link

oxij commented Sep 30, 2018

@lrvick

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

oxij commented Oct 1, 2018

@lrvick

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

Profpatsch commented Oct 1, 2018

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

Ekleog commented Oct 12, 2018

@copumpkin Currently (ie. with git's built-in signatures), the semantics of a signature are defined by the project using them:

  • It technically signs the whole repository as well as its whole history
  • In practice, no one will take you as responsible for anything but the diff introduced by the commit (so just checking the name -> pname + version, in your example)

In the world as git-wotr sees it, there are two kinds of signature:

  • Commit-state signature, that sign the whole state of the repository at a given point in time (will be required for saying “from now on all commits must be signed”: it's initialization of trust, as we couldn't reasonably check and sign all commits since the beginning of nixpkgs)
  • Commit-diff signature, that explicitly signs only the diff between a commit and its parent. (and this makes me think there's a potential ambiguity for merge commits, opened git-wotr/spec#14 to track it)

These signatures are both explicitly defined in the git-wotr spec and explicitly encoded in the signed object, so that there could be no ambiguity as to what someone actually signed.

So I think that, when we get to the point where committers have generated keys and understood how to use them by trying git's built-in signing mechanism, git-wotr clients will be ready to make signatures whose meaning is unambiguous. :)

@copumpkin

This comment has been minimized.

Copy link
Member

copumpkin commented Oct 12, 2018

So in the world where commit-diff signatures are the norm (seems like the only remotely feasible option here), what do those signatures mean to a user?

Joe User installs Nix, gets the latest nixpkgs channel deployed to their box, and types nix-shell -p vim or nix-env -iA nixpkgs.vim. How do we figure out whose diff commits are relevant to that invocation to make Joe feel comfortable with installing that package? We could use some Nix-fu to figure out that vim comes from pkgs/applications/editors/vim/default.nix and run effectively a git blame on that file to figure out who's responsible for it. Or do we want to try to be smarter, since pkgs/applications/editors/vim/default.nix just calls out to pkgs/applications/editors/vim/common.nix, which calls mkDerivation and a bajillion other transitive dependencies down to our bootstrap?

@Ekleog

This comment has been minimized.

Copy link
Member

Ekleog commented Oct 12, 2018

What a signature means

So each signature, individually, means that the reviewer (the person who put the signature) has checked that the commit doesn't introduce a backdoor or a vulnerability, basically. (there is a bit more subtility in git-wotr with - reviews to indicate mild disagreement, but let's ignore that)

How a user verifies nixkgs

The user then configures its nixpkgs to only use commits where they are confident the whole state of the repository is safe. For most users, that will be just giving the list of nixpkgs committers to git-wotr, and requiring one positive review per commit.

Then, git-wotr takes the history, finds a commit that's state-signed, and checks that all commits since then fit the user's security policy (so for the standard user, that it has been reviewed by at least one nixpkgs committer, but more complex policies should be possible to implement -- that part is still under discussion). If all commits don't match, then the update is cancelled.

(Note: I'm ignoring features like compromise recovery and handling of particularly severe vulnerabilities in this explanation for the sake of simplicity. And yes, there are optimizations to apply, so as not to re-check everything all the time.)

Summary

Overall, the idea is: the user only ever sees a repository when its security policy is matched by all commits. Because otherwise, indeed as you mention it's just ticking a box and not making significant security improvements. Which is the reason why all nixpkgs committers should have public keys :) (oh, and git-wotr allows signing after the fact, so a committer forgetting to sign a commit wouldn't be an irrecoverable issue)

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Oct 12, 2018

I have a feeling that mandating destruction of PR history is not a good policy in the most unflexible version, so maybe there should be an explicit support for slightly more flexible merge commit signing.

@Ekleog

This comment has been minimized.

Copy link
Member

Ekleog commented Oct 12, 2018

Hmm? Why would there be destruction of PR history? I'd like to invite you to comment on git-wotr/spec#14 about merge commit signing, though :)

@wiktor-k

This comment has been minimized.

Copy link

wiktor-k commented Oct 12, 2018

@Ekleog,

It's true that we haven't really discussed how we would assign trust to committers. Up to now, the assumption was “somewhere, there is a trusted list of committers”, and we went from there to think about a scheme to sign git commits efficiently (feel free to come participate to the discussion, I think we're almost getting to an agreement).

I've read the spec, it seems to me like a signed review protocol, did I get this right?

There is a project for doing code reviews purely in git, you may find this interesting: https://github.com/google/git-appraise#metadata

However, the question of “how do we distribute this list of committers so that it is trusted” hasn't been raised yet… and intuitively, it will be out-of-scope of the spec linked above. I completely agree with you that the archlinux model sounds good: the committer list should be signed by 3 of 5 master keys, so there is no SPOF.

Arch Linux uses special package for that but I think the core of this approach is the following:

  1. there is a special keyring directory for package maintainers keys (in Arch it's /etc/pacman.d/gnupg)
  2. there is one ultimately trusted key, created (gpg --gen-key) while bootstrapping the installation (in Arch it has UID Pacman Keyring Master Key)
  3. that installation key then signs (gpg --lsign) 5 master keys and sets their trust to marginal

Everything else is derived from that. Note that mere presence of a key in that special keyring does not allow it to sign packages, it must be signed by at least 3 master keys. That has a nice property (not used in Arch though) that you could verify packages with gpg --auto-key-retrieve - if they have appropriate sigs it will work, if that auto-retrieved key doesn't have sigs, it won't work. Authorization is by signing keys, you can set expiration for that too, and if you want to remove a committer you just revoke sig. With the caveat that you should refresh that special keyring.

However, I'm not sure about signing each key individually: to me, signing a key means asserting the identity of someone. Here, we would want to assert their being the people who were granted commit access.

Yes, I don't like it too. I've been thinking for this for some time, even considering signatures with critical notations. But ultimately the approach with special keyring directory and master keys has - I think - the best security/convenience ratio.

I think the most important point we want to ensure is proof of continuity: the person who owns the key at any point in time must be the same person as the person who owned the key when they were promoted contributor.

I don't follow, if they use the same key they surely must be the same person...?

For this, I'd think the easiest would be to have these 3-of-5 keys sign the contributor list, in a format that can be eg. a list of fingerprints of the keys of all contributors, one per line (a format that's likely to be similar to the one consumed by git-wotr, according to current discussions).

Keys of all contributors reminds me of this I-D: https://tools.ietf.org/html/draft-mccain-keylist-01 (just a random trivia you may find useful). Although keyservers would already contain a list of keys that your master keys have signed: https://pgp.cs.uu.nl/stats/6598f235f23fb2ae.html (this is an example, not a master key).

My current view of the plan for the future:

That sounds like a very solid plan 👍

@copumpkin,

What am I attesting to when I sign my commits? In a system like pijul or darcs, it makes a bit more sense that I'm signing my patch, but in git, a commit represents a snapshot of the entire tree.

I wouldn't be worried about being held responsible for other people's changes. I'd view signing parent commit and tree as an additional safety net - that your changes make sense only in context of work of others.

Note that in general signing a commit doesn't mean much, for example git has signed pushes that are a correct mechanism for indicating "yes, I want master to point to that commit" (mere act of signing an object doesn't mean you want it there).

(Not to mention the legal angle of DCOs, CLAs and Signed-off-bys).

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Oct 12, 2018

@oxij

This comment has been minimized.

Copy link

oxij commented Oct 12, 2018

@wiktor-k We are aware of git-appraise, but it doesn't sign anything. Also see git-wotr/spec#4 (comment) as it is highly related.

@wiktor-k

This comment has been minimized.

Copy link

wiktor-k commented Oct 12, 2018

Got it @oxij, thanks for the explanation.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Oct 12, 2018

@Ekleog

This comment has been minimized.

Copy link
Member

Ekleog commented Oct 13, 2018

@wiktor-k

Yes, I don't like it too. I've been thinking for this for some time, even considering signatures with critical notations. But ultimately the approach with special keyring directory and master keys has - I think - the best security/convenience ratio.

The problem with this is it lacks the metadata required to set policies like mentioned in git-wotr/spec#9 (comment) . I believe we can get the same user-friendly behaviour with appropriate tooling and a simple list of fingerprints of contributors, while keeping more flexibility for those who would rather have a different review scheme, all while using the same review signatures :)

@7c6f434c

Hmm? Why would there be destruction of PR history?

Because if you want all commits to be signed, it is easiest to squash just in case you miss some weird problem related to the order of the commits.

git-wotr's current draft already includes a way to say “this commit's signature is valid iff. used along with these other commits” (initially included to handle the “what if a backdoor is actually introduced, how to tag all commits between it and its removal as untrusted”). So it'd simply be a matter of signing all the PR's commits to be valid iff. used along with the merge commit.

@Ekleog Ekleog referenced this pull request Oct 13, 2018

Open

Signature Format #4

@Mic92 Mic92 referenced this pull request Oct 29, 2018

Open

Reviewed channel/revision #93

@Ekleog Ekleog referenced this pull request Dec 2, 2018

Closed

Introduce concept of NixOS support states #23590

2 of 7 tasks complete
@shlevy

This comment has been minimized.

Copy link
Member

shlevy commented Jan 17, 2019

@lrvick Are you ready for us to open nominations for a shepherd team as per the new RFC process?

@Mic92

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

globin commented Jan 24, 2019

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

@grahamc

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

nixos-discourse commented Jan 24, 2019

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

This comment has been minimized.

Copy link
Member

7c6f434c commented Jan 25, 2019

@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

This comment has been minimized.

Copy link

oxij commented Jan 25, 2019

@globin

This comment has been minimized.

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

This comment has been minimized.

Copy link

nixos-discourse commented Feb 8, 2019

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

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