buildSupport: add helpers for verifying code signatures by xeji · Pull Request #43233 · NixOS/nixpkgs

xeji · 2018-07-08T21:23:50Z

Motivation for this change

Many projects provide signed source tarballs or signed binaries, which are currently not checked in nixpkgs. While it's not perfect, I believe that checking signatures provides some security advantage our typical "TOFU" workflow of implicitly trusting changed sha256 hashes.

The main reason is that, as opposed to sha256 hashes, public keys are usually long term, so we can invest more time to verify them initially, and after doing this once, malicious changes are more likely to be spotted.

This PR proposes some simple helpers for checking code signatures. It also provides some examples on how to use them.

Things done

fetchpgpkey is an extension of fetchurl that fetches a PGP public key to the nix store and verifies its fingerprint with gpg. It still needs a sha256 because it is based on fetchurl. (I have experimented with an alternative implementation using builtins.fetchurl and just a fingerprint, no sha256, but that is not strictly a fixed-output derivation).
verifySignatureHook sets up signature verification and adds a preUnpackHook to automatically verify the signature srcSignature of src with signaturePublicKey when present. Verification can also be triggered manually using the shell functions defined in the hook.
Implemented 3 different examples for various use cases of the hook. These examples are for demonstartion and may or may not be merged with this PR.
- samba4: the tarball is signed, but the signature is for the uncompressed tar archive. The hook has an option for this.
- tor-browser-bundle-bin: provides signed tarballs but there's not standard unpack phase, so the verification must be triggered manually in buildCommand
- 1password: the tarball contains a signed binary and the signature file, so verification is done in checkPhase here.

Possible future work

Enhance fetchgit to verify signed revisions using git verify-commit.

cc @matthewbauer @jb55 @garbas

GrahamcOfBorg · 2018-07-08T21:54:44Z

Success on x86_64-linux (full log)

Attempted: samba4

Partial log (click to expand)

strip is /nix/store/4qvrxzxa535y8304mk195x50b6p9607d-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/pbrwprlmjdr80hw69vi94liag3jrbxkf-samba-4.7.6-dev/lib 
patching script interpreter paths in /nix/store/pbrwprlmjdr80hw69vi94liag3jrbxkf-samba-4.7.6-dev
checking for references to /tmp/nix-build-samba-4.7.6.drv-0 in /nix/store/pbrwprlmjdr80hw69vi94liag3jrbxkf-samba-4.7.6-dev...
shrinking RPATHs of ELF executables and libraries in /nix/store/2600n568lxja2xkcd8dlcr5ifw2d1rq9-samba-4.7.6-man
gzipping man pages under /nix/store/2600n568lxja2xkcd8dlcr5ifw2d1rq9-samba-4.7.6-man/share/man/
strip is /nix/store/4qvrxzxa535y8304mk195x50b6p9607d-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/2600n568lxja2xkcd8dlcr5ifw2d1rq9-samba-4.7.6-man
checking for references to /tmp/nix-build-samba-4.7.6.drv-0 in /nix/store/2600n568lxja2xkcd8dlcr5ifw2d1rq9-samba-4.7.6-man...
/nix/store/ls86kj3655g2cy8zspprbxvmlcvjnxkq-samba-4.7.6

GrahamcOfBorg · 2018-07-08T21:57:14Z

Success on x86_64-darwin (full log)

Attempted: samba4

Partial log (click to expand)

/nix/store/pqcxjnqhg4gqsbv3pm7clq54ahz08w4x-samba-4.7.6/bin/pidl: interpreter directive changed from "/usr/bin/env perl" to "/nix/store/206145946rz9hjqyvldq5ka927d4934s-perl-5.24.3/bin/perl"
/nix/store/pqcxjnqhg4gqsbv3pm7clq54ahz08w4x-samba-4.7.6/bin/smbtar: interpreter directive changed from "/bin/sh" to "/nix/store/q2wqq1k20v8kc3vckapqf5nws30brnni-bash-4.4-p23/bin/sh"
moving /nix/store/pqcxjnqhg4gqsbv3pm7clq54ahz08w4x-samba-4.7.6/sbin/* to /nix/store/pqcxjnqhg4gqsbv3pm7clq54ahz08w4x-samba-4.7.6/bin
strip is /nix/store/7ddbq63v97nk8gkbf7gcsfmby37h6gbl-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/0l9d4yzidj2wvj7iipyvalxf3iplr5qp-samba-4.7.6-dev/lib
patching script interpreter paths in /nix/store/0l9d4yzidj2wvj7iipyvalxf3iplr5qp-samba-4.7.6-dev
gzipping man pages under /nix/store/i76lbanwfwcw1gjs8cxhz1w2dhfnqn48-samba-4.7.6-man/share/man/
strip is /nix/store/7ddbq63v97nk8gkbf7gcsfmby37h6gbl-cctools-binutils-darwin/bin/strip
patching script interpreter paths in /nix/store/i76lbanwfwcw1gjs8cxhz1w2dhfnqn48-samba-4.7.6-man
/nix/store/pqcxjnqhg4gqsbv3pm7clq54ahz08w4x-samba-4.7.6

GrahamcOfBorg · 2018-07-08T21:59:18Z

Success on aarch64-linux (full log)

Attempted: samba4

Partial log (click to expand)

strip is /nix/store/0pjsgkxz0rp5baycq5sp2s72lrr5q9sg-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/517bwbca7g0ckljr92bgv9sfav6y1462-samba-4.7.6-dev/lib
patching script interpreter paths in /nix/store/517bwbca7g0ckljr92bgv9sfav6y1462-samba-4.7.6-dev
checking for references to /build in /nix/store/517bwbca7g0ckljr92bgv9sfav6y1462-samba-4.7.6-dev...
shrinking RPATHs of ELF executables and libraries in /nix/store/aqfm0zkhgakdp2h6kvdlzn3f1kcjnjcr-samba-4.7.6-man
gzipping man pages under /nix/store/aqfm0zkhgakdp2h6kvdlzn3f1kcjnjcr-samba-4.7.6-man/share/man/
strip is /nix/store/0pjsgkxz0rp5baycq5sp2s72lrr5q9sg-binutils-2.30/bin/strip
patching script interpreter paths in /nix/store/aqfm0zkhgakdp2h6kvdlzn3f1kcjnjcr-samba-4.7.6-man
checking for references to /build in /nix/store/aqfm0zkhgakdp2h6kvdlzn3f1kcjnjcr-samba-4.7.6-man...
/nix/store/ixzlxf5bxjsqr1d08nsvjvssnsish60n-samba-4.7.6

matthewbauer · 2018-07-08T22:50:52Z

pkgs/servers/samba/4.x.nix

+    fingerprint = "52FBC0B86D954B0843324CDC6F33915B6568B7EA";
+  };
+
+  nativeBuildInputs = [ verifySignatureHook ];


Does this work? We aren't downloading binaries here so I have no idea where the signature would be coming from. This should be not needed at all for source based builds.

It works by convention: The hook assumes that srcSignature contains the signature file, and signaturePublicKey the corresponding key. If they're present, srcSignature is checked against src. Of course you have to fetchurl the signature file and fetchpgpkey the key for the hook to work.

And yes, it can be used for source base builds if upstream provides a signature for the source tarball. It obviously doesn't work with fetchFromGitHub, but that would be a case for verifying signed git commits.

Ok did not realize that on tarball signatures! I wonder if we could put the source-based part of the hook in fetchurl then? Just have an optional attribute that is verified for us. This would also make it possible to support multiple-src derivations.

Sure we can do that. I only decided against modifying fetchurl for now to avoid a rebuild-everything change. It would take several extra attributes: URL and hash of the signature file, and the public key which I would still download separately with fetchpgpgkey as it may be re-used for multiple projects or sources.

Actually it shouldn't though! fetchurl is fixed output so you can make any changes you want to it.

orivej · 2018-07-08T22:53:53Z

It should be worth taking a look at how Triton did it: see https://github.com/triton/triton/commits/master?before=6a1de201948b7755177562105e49181baca2bc2e+35&path[]=pkgs&path[]=build-support&path[]=fetchurl&path[]=builder.sh starting from Mar 30, 2016. (/cc @wkennington in case you are interested or would like to help.)

matthewbauer · 2018-07-08T23:03:50Z

Okay a few thoughts on this.

I think there is a fairly good case to be made to do this for 1password. We are relying on a binary blob & that's unfortunate. I know a lot of people do use 1password & there is definitely good reason to support it in Nixpkgs because of that. However, we should be encouraging everyone to use free and open source software as much as possible! Nixpkgs just does not work very well with binary blobs.
The setup hook makes a lot more sense to me - it is much more reusable & we now don't have to check in the public key. I don't really understand the need for two layers of integrity checks but enough people have voiced support that maybe it is worth doing. At least I don't think it will hurt to do this.
I would strongly encourage us to drop the tor-browser-bundle-bin & firefox-bin. It makes no sense to support these things when we have a good source-based version of each of them. If someone wants to maintain them please put them in something like NUR or another overlay. Nix has never worked well with binary blobs and this is partly by design. This isn't exactly related to this PR but just something that I had forgotten was still around.

xeji · 2018-07-08T23:14:56Z

However, we should be encouraging everyone to use free and open source software as much as possible! Nixpkgs just does not work very well with binary blobs.

@matthewbauer fully agree. I just randomly selected these examples to test and show different use cases of the setup hook.

xeji · 2018-07-09T10:45:10Z

Marked WIP while I have a look at how Triton does it and try to put part of the features into fetchurl.

jb55 · 2018-07-30T05:06:25Z

Would it be hard to support this common use case? Here's are the instructions for verifying bitcoin signatures:

Verify that the checksum of the release file is listed in the checksums
file using the following command:

sha256sum --ignore-missing --check SHA256SUMS.asc

In the output produced by the above command, you can safely ignore any
warnings and failures, but you must ensure the output lists "OK" after
the name of the release file you downloaded. For example:

bitcoin-0.16.2-x86_64-linux-gnu.tar.gz: OK

Obtain a copy of the release signing key by running the following command:

gpg --recv-keys 01EA5486DE18A882D4C2684590C8019E36C2E964

The output of the command above should say that one key was imported,
updated, has new signatures, or remained unchanged.
Verify that the checksums file is PGP signed by the release signing key:

gpg --verify SHA256SUMS.asc

c0bw3b · 2018-10-26T18:45:37Z

Ooooh I would really like to see this happen.
I don't see it as an extra layer of integrity checking. The SHA checksum ensure integrity of the file but verifying the file signature ensure the identity of the source. There is some overlap in practical but it remains two different goals.

I'm discovering this PR just now and it seems the proposed implementation is easily extensible.
A future improvement could be to support verification of Ed25519 signatures (think minisign or OpenBSD signify). It seems it would be easy enough to write fetched25519key and then extend verifySignatureHook to check whether it's fed with a PGP or Ed25519 key.

TL;DR; I'll now read other posts

jb55 · 2018-10-26T19:01:56Z

A future improvement could be to support verification of Ed25519 signatures

gpg supports ed25519 sigs

Mic92 · 2018-12-14T11:34:06Z

pkgs/build-support/setup-hooks/verify-signature.sh

+# if UNCOMPRESS is set, uncompress DATAFILE before verification
+verifySignature() {
+  if [ -z "$3" ]; then
+    gpgv --keyring pubring.kbx "$1" "$2" || exit 1


Can you make gpg ignoring expire dates? Otherwise it will become a problem when somebody tries to build an old nixpkgs revision. We could print a warning if a gpg key is expired.

The warning should be good enough for the TOFU case.

Mic92 · 2018-12-14T11:35:29Z

pkgs/applications/misc/1password/default.nix

+  nativeBuildInputs = [ verifySignatureHook ];
+
+  signaturePublicKey = fetchpgpkey {
+    url = https://keybase.io/1password/pgp_keys.asc;


What if they update their key?

Can we fetch their key from a key-server using the current keyid instead?

Keyservers are just one of many ways of fetching OpenPGP keys, I don't think it would make sense to force downloads through them (also, we'd want to fetch using the fingerprint, not the keyid).

Having a fetchFromSks wrapper of fetchpgpkey that fetches from the sks-keyservers network might be a meaningful addition, but I don't think it's required for the first iteration

Ekleog · 2019-05-12T11:46:14Z

pkgs/build-support/fetchpgpkey/default.nix

+
+  name = "pubkey-${fingerprint}";
+
+  postFetch =


I think we could reduce issues when a key gets an additional signature (eg. for when fetching from sks-keyservers) by stripping it of all UIDs, signatures, etc. before the sha256 check.

roconnor-blockstream · 2020-02-22T03:36:39Z

Please do not make this part of fetchurl. Placing it as part of fetchurl would enable an attack where and attacker tricks a victim to load a trojan-horse application into their nix-store and then updates the hash of a fetchurl in nixpkgs to reference the trojan horse.

Now I do respect that if the attacker has access to the victim's nixpkgs, there are a lot of bad things they can do, but getting limited "access" to nixpkgs hashes is maybe easier that you might think (see the reference to r-ryantm below). However my larger point is that putting a GPG check into the derivation "guarantees" that it has been executed during the build, but putting the check in fetchurl does not, because fetchurl is simply bypassed if a file with the listed hash value ever already exists in the nix-store.

As to motivation of why we want this: We'd like to use automatic updates by bots such as r-ryantm, however an attacker can currently attack packages by hijacking domains and serving "updated" trojan-horse applications, and get nixpkgs to be (semi-)automatically updated. For those packages with signed source releases, it would let us use automatic updates for signed software more safely.

stale · 2020-08-20T04:45:28Z

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

Mic92 · 2020-08-20T09:12:31Z

I agree with @roconnor-blockstream we should make this part of our tooling rather than the build process. Once we have our own checksum we no longer need to rely on public key cryptography.

nixos-discourse · 2020-09-08T17:45:23Z

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/7

danielfullmer · 2020-11-23T20:21:14Z

@Mic92 Correct me if otherwise, but I don't think @roconnor-blockstream was saying that it shouldn't be verified via Nix in a derivation, only that the verification shouldn't be behind a fixed output derivation like fetchurl. Having a signature check take place in Nix provides assurance of build input authenticity in addition to just integrity.

There is also another alternative I haven't seen mentioned: Have another derivation that takes the source and signature as input, and outputs a symlink to the original source if and only if it passes a signature check. The output of that verification derivation could then be passed as the src to mkDerivation. This seems like more clean separation of concerns, rather than just adding signature checking as an additional setup hook in the stdenv. But, it comes at the cost of an additional derivation. I have an example use of this idea for android APKs here: nix-community/robotnix#59

xeji added 5 commits July 8, 2018 22:45

build-support: add fetchpgpkey

2738727

build-support/setup-hooks: add verifySignatureHook

317ece1

samba4: verify source tarball signature

376eeee

tor-browser-bundle-bin: verify tarball signatures

403e8a1

1password: check binary code signature during build

8f961de

xeji requested a review from Ericson2314 as a code owner July 8, 2018 21:23

xeji mentioned this pull request Jul 8, 2018

[WIP] 1password: check binary code signature during build #42580

Closed

9 tasks

matthewbauer reviewed Jul 8, 2018

View reviewed changes

xeji added the 2.status: work-in-progress label Jul 9, 2018

Mic92 reviewed Dec 14, 2018

View reviewed changes

Ekleog reviewed May 12, 2019

View reviewed changes

danieldk mentioned this pull request Jul 6, 2020

eid-mw: 4.4.16 -> 4.4.27 #92252

Merged

10 tasks

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 20, 2020

Mic92 closed this Aug 20, 2020

danielfullmer mentioned this pull request Nov 23, 2020

APK signature verification nix-community/robotnix#59

Merged

xeji deleted the p/verifysignature branch February 17, 2022 18:04

flandweber mentioned this pull request Sep 18, 2024

fetchgit: add git signature verification #330457

Open

13 tasks

Uh oh!

Conversation

xeji commented Jul 8, 2018

Motivation for this change

Things done

Possible future work

Uh oh!

GrahamcOfBorg commented Jul 8, 2018

Uh oh!

GrahamcOfBorg commented Jul 8, 2018

Uh oh!

GrahamcOfBorg commented Jul 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orivej commented Jul 8, 2018

Uh oh!

matthewbauer commented Jul 8, 2018

Uh oh!

xeji commented Jul 8, 2018

Uh oh!

xeji commented Jul 9, 2018

Uh oh!

jb55 commented Jul 30, 2018

Uh oh!

c0bw3b commented Oct 26, 2018

Uh oh!

jb55 commented Oct 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roconnor-blockstream commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Aug 20, 2020

Uh oh!

Mic92 commented Aug 20, 2020

Uh oh!

nixos-discourse commented Sep 8, 2020

Uh oh!

danielfullmer commented Nov 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

roconnor-blockstream commented Feb 22, 2020 •

edited

Loading