From de2a27caab19aa3669066cf80e0f48ae53477eed Mon Sep 17 00:00:00 2001 From: Puck Meerburg Date: Sat, 28 Sep 2024 16:54:39 +0200 Subject: [PATCH 1/6] fixup! Add a test for builtin:fetchurl cert verification --- tests/nixos/fetchurl.nix | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/nixos/fetchurl.nix b/tests/nixos/fetchurl.nix index 476f779bcc3..f873bf4b56f 100644 --- a/tests/nixos/fetchurl.nix +++ b/tests/nixos/fetchurl.nix @@ -67,6 +67,9 @@ in out = machine.succeed("curl https://good/index.html") assert out == "hello world\n" + out = machine.succeed("cat ${badCert}/cert.pem > /tmp/cafile.pem; curl --cacert /tmp/cafile.pem https://bad/index.html") + assert out == "foobar\n" + # Fetching from a server with a trusted cert should work. machine.succeed("nix build --no-substitute --expr 'import { url = \"https://good/index.html\"; hash = \"sha256-qUiQTy8PR5uPgZdpSzAYSw0u0cHNKh7A+4XSmaGSpEc=\"; }'") @@ -74,5 +77,8 @@ in err = machine.fail("nix build --no-substitute --expr 'import { url = \"https://bad/index.html\"; hash = \"sha256-rsBwZF/lPuOzdjBZN2E08FjMM3JHyXit0Xi2zN+wAZ8=\"; }' 2>&1") print(err) assert "SSL certificate problem: self-signed certificate" in err + + # Fetching from a server with a trusted cert should work via environment variable override. + machine.succeed("NIX_SSL_CERT_FILE=/tmp/cafile.pem nix build --no-substitute --expr 'import { url = \"https://bad/index.html\"; hash = \"sha256-rsBwZF/lPuOzdjBZN2E08FjMM3JHyXit0Xi2zN+wAZ8=\"; }'") ''; } From 9b818f14ddc167266f76b5874924a740d29720cb Mon Sep 17 00:00:00 2001 From: Puck Meerburg Date: Sat, 28 Sep 2024 16:54:39 +0200 Subject: [PATCH 2/6] fix passing CA files into builtins:fetchurl sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch has been manually adapted from https://github.com/lix-project/lix/commit/14dc84ed03f1b7e5a41bb6fdce00916faab32b60 Tested with: $ NIX_SSL_CERT_FILE=$(nix-build '' -A cacert)/etc/ssl/certs/ca-bundle.crt nix-build --store $(mktemp -d) -E 'import { url = https://google.com; }' warning: found empty hash, assuming 'sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=' this derivation will be built: /nix/store/4qljhy0jj2b0abjzpsbyarpia1bqylwc-google.com.drv building '/nix/store/4qljhy0jj2b0abjzpsbyarpia1bqylwc-google.com.drv'... error: … writing file '/nix/store/0zynn4n8yx59bczy1mgh1lq2rnprvvrc-google.com' error: unable to download 'https://google.com': Problem with the SSL CA cert (path? access rights?) (77) error: builder for '/nix/store/4qljhy0jj2b0abjzpsbyarpia1bqylwc-google.com.drv' failed with exit code 1 Now returns: nix-env % NIX_SSL_CERT_FILE=$(nix-build '' -A cacert)/etc/ssl/certs/ca-bundle.crt nix-build --store $(mktemp -d) -E 'import { url = https://google.com; }' this derivation will be built: /nix/store/4qljhy0jj2b0abjzpsbyarpia1bqylwc-google.com.drv building '/nix/store/4qljhy0jj2b0abjzpsbyarpia1bqylwc-google.com.drv'... error: hash mismatch in fixed-output derivation '/nix/store/4qljhy0jj2b0abjzpsbyarpia1bqylwc-google.com.drv': specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= got: sha256-5xXEhGtnRdopaUTqaz2M1o2NE7ovhU0SjcSOPwntqwY= (cherry picked from commit 1fbdf409524bb350b8614f3d95067cb9ba3c57f2) --- src/libstore/build/local-derivation-goal.cc | 21 ++++++++++++++------- src/libstore/builtins.hh | 4 +++- src/libstore/builtins/fetchurl.cc | 8 +++++++- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 2eec3ca2b67..2184f0b4b69 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1732,13 +1732,20 @@ void LocalDerivationGoal::runChild() bool setUser = true; - /* Make the contents of netrc available to builtin:fetchurl - (which may run under a different uid and/or in a sandbox). */ + /* Make the contents of netrc and the CA certificate bundle + available to builtin:fetchurl (which may run under a + different uid and/or in a sandbox). */ std::string netrcData; - try { - if (drv->isBuiltin() && drv->builder == "builtin:fetchurl") - netrcData = readFile(settings.netrcFile); - } catch (SystemError &) { } + std::string caFileData; + if (drv->isBuiltin() && drv->builder == "builtin:fetchurl") { + try { + netrcData = readFile(settings.netrcFile); + } catch (SystemError &) { } + + try { + caFileData = readFile(settings.caFile); + } catch (SystemError &) { } + } #if __linux__ if (useChroot) { @@ -2166,7 +2173,7 @@ void LocalDerivationGoal::runChild() e.second = rewriteStrings(e.second, inputRewrites); if (drv->builder == "builtin:fetchurl") - builtinFetchurl(drv2, netrcData); + builtinFetchurl(drv2, netrcData, caFileData); else if (drv->builder == "builtin:buildenv") builtinBuildenv(drv2); else if (drv->builder == "builtin:unpack-channel") diff --git a/src/libstore/builtins.hh b/src/libstore/builtins.hh index d201fb3ac5b..3073179a5ec 100644 --- a/src/libstore/builtins.hh +++ b/src/libstore/builtins.hh @@ -6,7 +6,9 @@ namespace nix { // TODO: make pluggable. -void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData); +void builtinFetchurl(const BasicDerivation & drv, + const std::string & netrcData, + const std::string & caFileData); void builtinUnpackChannel(const BasicDerivation & drv); } diff --git a/src/libstore/builtins/fetchurl.cc b/src/libstore/builtins/fetchurl.cc index 3f4a3076e97..5624faa67e6 100644 --- a/src/libstore/builtins/fetchurl.cc +++ b/src/libstore/builtins/fetchurl.cc @@ -6,7 +6,10 @@ namespace nix { -void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData) +void builtinFetchurl( + const BasicDerivation & drv, + const std::string & netrcData, + const std::string & caFileData) { /* Make the host's netrc data available. Too bad curl requires this to be stored in a file. It would be nice if we could just @@ -16,6 +19,9 @@ void builtinFetchurl(const BasicDerivation & drv, const std::string & netrcData) writeFile(settings.netrcFile, netrcData, 0600); } + settings.caFile = "ca-certificates.crt"; + writeFile(settings.caFile, caFileData, 0600); + auto out = get(drv.outputs, "out"); if (!out) throw Error("'builtin:fetchurl' requires an 'out' output"); From 8d763e7ab9b580d21859985bb88b6b4439b449bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Sat, 28 Sep 2024 17:06:10 +0200 Subject: [PATCH 3/6] tests/nixos/fetchurl: drop unused variables (cherry picked from commit de9946cbfd4858133462c8cc6b7838edb3be2451) --- tests/nixos/fetchurl.nix | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/nixos/fetchurl.nix b/tests/nixos/fetchurl.nix index f873bf4b56f..243c0cacc6e 100644 --- a/tests/nixos/fetchurl.nix +++ b/tests/nixos/fetchurl.nix @@ -1,7 +1,7 @@ # Test whether builtin:fetchurl properly performs TLS certificate # checks on HTTPS servers. -{ lib, config, pkgs, ... }: +{ pkgs, ... }: let @@ -25,7 +25,7 @@ in name = "nss-preload"; nodes = { - machine = { lib, pkgs, ... }: { + machine = { pkgs, ... }: { services.nginx = { enable = true; @@ -60,7 +60,7 @@ in }; }; - testScript = { nodes, ... }: '' + testScript = '' machine.wait_for_unit("nginx") machine.wait_for_open_port(443) From 1cc79f134396a1ceaa745ddd5503f81ec0bbbaf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 12 Apr 2024 15:57:53 +0200 Subject: [PATCH 4/6] Fix the access of symlinks to host files in the sandbox https://github.com/NixOS/nix/pull/10456 fixed the addition of symlink store paths to the sandbox, but also made it so that the hardcoded sandbox paths (like `/etc/hosts`) were now bind-mounted without following the possible symlinks. This made these files unreadable if there were symlinks (because the sandbox would now contain a symlink to an unreachable file rather than the underlying file). In particular, this broke FOD derivations on NixOS as `/etc/hosts` is a symlink there. Fix that by canonicalizing all these hardcoded sandbox paths before adding them to the sandbox. (cherry picked from commit acbb1523c1dc28043d6dab729db696485938f969) --- src/libstore/build/local-derivation-goal.cc | 13 +++-- tests/functional/linux-sandbox.sh | 14 +++++- tests/functional/symlink-derivation.nix | 55 +++++++++++++++------ 3 files changed, 62 insertions(+), 20 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 2184f0b4b69..b8228bc11cd 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -1847,11 +1847,18 @@ void LocalDerivationGoal::runChild() if (pathExists(path)) ss.push_back(path); - if (settings.caFile != "") - pathsInChroot.try_emplace("/etc/ssl/certs/ca-certificates.crt", settings.caFile, true); + if (settings.caFile != "" && pathExists(settings.caFile)) { + Path caFile = settings.caFile; + pathsInChroot.try_emplace("/etc/ssl/certs/ca-certificates.crt", canonPath(caFile, true), true); + } } - for (auto & i : ss) pathsInChroot.emplace(i, i); + for (auto & i : ss) { + // For backwards-compatibiliy, resolve all the symlinks in the + // chroot paths + auto canonicalPath = canonPath(i, true); + pathsInChroot.emplace(i, canonicalPath); + } /* Bind-mount all the directories from the "host" filesystem that we want in the chroot diff --git a/tests/functional/linux-sandbox.sh b/tests/functional/linux-sandbox.sh index 04209277bbe..880d56fca13 100644 --- a/tests/functional/linux-sandbox.sh +++ b/tests/functional/linux-sandbox.sh @@ -60,7 +60,11 @@ testCert () { nocert=$TEST_ROOT/no-cert-file.pem cert=$TEST_ROOT/some-cert-file.pem +symlinkcert=$TEST_ROOT/symlink-cert-file.pem +symlinkDir=$TEST_ROOT/symlink-dir echo -n "CERT_CONTENT" > $cert +ln -s $cert $symlinkcert +ln -s $TEST_ROOT $symlinkDir # No cert in sandbox when not a fixed-output derivation testCert missing normal "$cert" @@ -74,5 +78,13 @@ testCert missing fixed-output "$nocert" # Cert in sandbox when ssl-cert-file is set to an existing file testCert present fixed-output "$cert" +# Cert in sandbox when ssl-cert-file is set to a symlink to an existing file +testCert present fixed-output "$symlinkcert" + # Symlinks should be added in the sandbox directly and not followed -nix-sandbox-build symlink-derivation.nix +nix-sandbox-build symlink-derivation.nix -A depends_on_symlink +nix-sandbox-build symlink-derivation.nix -A test_sandbox_paths \ + --option extra-sandbox-paths "/file=$cert" \ + --option extra-sandbox-paths "/dir=$TEST_ROOT" \ + --option extra-sandbox-paths "/symlinkDir=$symlinkDir" \ + --option extra-sandbox-paths "/symlink=$symlinkcert" diff --git a/tests/functional/symlink-derivation.nix b/tests/functional/symlink-derivation.nix index 17ba3742403..e9a74cdcef2 100644 --- a/tests/functional/symlink-derivation.nix +++ b/tests/functional/symlink-derivation.nix @@ -15,22 +15,45 @@ let ''; }; in -mkDerivation { - name = "depends-on-symlink"; - buildCommand = '' - ( - set -x +{ + depends_on_symlink = mkDerivation { + name = "depends-on-symlink"; + buildCommand = '' + ( + set -x + + # `foo_symlink` should be a symlink pointing to `foo_in_store` + [[ -L ${foo_symlink} ]] + [[ $(readlink ${foo_symlink}) == ${foo_in_store} ]] + + # `symlink_to_not_in_store` should be a symlink pointing to `./.`, which + # is not available in the sandbox + [[ -L ${symlink_to_not_in_store} ]] + [[ $(readlink ${symlink_to_not_in_store}) == ${builtins.toString ./.} ]] + (! ls ${symlink_to_not_in_store}/) + + # Native paths + ) + echo "Success!" > $out + ''; + }; + + test_sandbox_paths = mkDerivation { + # Depends on the caller to set a bunch of `--sandbox-path` arguments + name = "test-sandbox-paths"; + buildCommand = '' + ( + set -x + [[ -f /file ]] + [[ -d /dir ]] - # `foo_symlink` should be a symlink pointing to `foo_in_store` - [[ -L ${foo_symlink} ]] - [[ $(readlink ${foo_symlink}) == ${foo_in_store} ]] + # /symlink and /symlinkDir should be available as raw symlinks + # (pointing to files outside of the sandbox) + [[ -L /symlink ]] && [[ ! -e $(readlink /symlink) ]] + [[ -L /symlinkDir ]] && [[ ! -e $(readlink /symlinkDir) ]] + ) - # `symlink_to_not_in_store` should be a symlink pointing to `./.`, which - # is not available in the sandbox - [[ -L ${symlink_to_not_in_store} ]] - [[ $(readlink ${symlink_to_not_in_store}) == ${builtins.toString ./.} ]] - (! ls ${symlink_to_not_in_store}/) - ) - echo "Success!" > $out - ''; + touch $out + ''; + }; } From e8e62c95dd0447a4921e7c9517191dad5df22f35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Fri, 12 Apr 2024 16:10:22 +0200 Subject: [PATCH 5/6] Test the inclusion of transitive symlinks in the sandbox (cherry picked from commit cef677ddbcad420220474935b660c147718a3a7c) --- tests/functional/linux-sandbox.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/functional/linux-sandbox.sh b/tests/functional/linux-sandbox.sh index 880d56fca13..e553791d90e 100644 --- a/tests/functional/linux-sandbox.sh +++ b/tests/functional/linux-sandbox.sh @@ -61,9 +61,11 @@ testCert () { nocert=$TEST_ROOT/no-cert-file.pem cert=$TEST_ROOT/some-cert-file.pem symlinkcert=$TEST_ROOT/symlink-cert-file.pem +transitivesymlinkcert=$TEST_ROOT/transitive-symlink-cert-file.pem symlinkDir=$TEST_ROOT/symlink-dir echo -n "CERT_CONTENT" > $cert ln -s $cert $symlinkcert +ln -s $symlinkcert $transitivesymlinkcert ln -s $TEST_ROOT $symlinkDir # No cert in sandbox when not a fixed-output derivation @@ -78,8 +80,9 @@ testCert missing fixed-output "$nocert" # Cert in sandbox when ssl-cert-file is set to an existing file testCert present fixed-output "$cert" -# Cert in sandbox when ssl-cert-file is set to a symlink to an existing file +# Cert in sandbox when ssl-cert-file is set to a (potentially transitive) symlink to an existing file testCert present fixed-output "$symlinkcert" +testCert present fixed-output "$transitivesymlinkcert" # Symlinks should be added in the sandbox directly and not followed nix-sandbox-build symlink-derivation.nix -A depends_on_symlink From c31abadb2597227291512bae2932a2f533e31fe9 Mon Sep 17 00:00:00 2001 From: Tom Bereknyei Date: Fri, 2 Aug 2024 20:02:57 -0400 Subject: [PATCH 6/6] feat: better warning for common SSL errors (cherry picked from commit 3e5bf903413f420c1f997e4b55140761172b8434) --- src/libstore/filetransfer.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index dcbec4acd84..98c14e64bf7 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -50,6 +50,8 @@ struct curlFileTransfer : public FileTransfer bool done = false; // whether either the success or failure function has been called Callback callback; CURL * req = 0; + // buffer to accompany the `req` above + char errbuf[CURL_ERROR_SIZE]; bool active = false; // whether the handle has been added to the multi object std::string statusMsg; @@ -352,6 +354,9 @@ struct curlFileTransfer : public FileTransfer if (writtenToSink) curl_easy_setopt(req, CURLOPT_RESUME_FROM_LARGE, writtenToSink); + curl_easy_setopt(req, CURLOPT_ERRORBUFFER, errbuf); + errbuf[0] = 0; + result.data.clear(); result.bodySize = 0; } @@ -465,8 +470,8 @@ struct curlFileTransfer : public FileTransfer code == CURLE_OK ? "" : fmt(" (curl error: %s)", curl_easy_strerror(code))) : FileTransferError(err, std::move(response), - "unable to %s '%s': %s (%d)", - request.verb(), request.uri, curl_easy_strerror(code), code); + "unable to %s '%s': %s (%d) %s", + request.verb(), request.uri, curl_easy_strerror(code), code, errbuf); /* If this is a transient error, then maybe retry the download after a while. If we're writing to a