linux/stdenv: Fixups to accomodate boostrap-tools changes and ssl in fetchurl by wkennington · Pull Request #8082 · NixOS/nixpkgs

wkennington · 2015-05-31T02:38:56Z

This is the second patchset for #8081. This adds support for the new boostrap-tools changes and also adds ssl verification support into fetchurl.

wkennington · 2015-05-31T03:08:29Z

@copumpkin @shlevy Let me know what needs to be done to add ssl verification support to the darwin stdenv. It looks like there is already a working curl with https in the bootstrap tools. Would it make sense to add cacert to the darwin bootstrap-tools and then enable ssl verification throughout?

wkennington · 2015-05-31T03:36:10Z

I added support to the darwin stdenv in both of these PR's, let me know if it works fine.

vcunat · 2015-05-31T07:53:36Z

pkgs/build-support/fetchurl/default.nix

Was there a discussion about (dis)advantages of using insecure connection by default for fixed-output derivations? For most use cases I don't think there's any added security, and I suppose there's some (small) performance loss.

Well, the main reason I want this implemented is for updated packages so
you can reasonably trust hashes you get out of failed fetchurls.

On Sun, May 31, 2015, 00:53 Vladimír Čunát notifications@github.com wrote:

In pkgs/build-support/fetchurl/default.nix
#8082 (comment):

@@ -1,4 +1,4 @@
-{ stdenv, curl }: # Note that curl' may benull', in case of the native stdenv.
+{ stdenv, curl, ca_cert_file ? null, insecure ? false }: # Note that curl' may benull', in case of the native stdenv.

Was there a discussion about (dis)advantages of using insecure connection
by default for fixed-output derivations? For most use cases I don't think
there's any added security, and I suppose there's some (small) performance
loss.

—
Reply to this email directly or view it on GitHub
https://github.com/NixOS/nixpkgs/pull/8082/files#r31387880.

The performance penalty should be minute compared to the costs of
everything else in the TLS stack.

On Sun, May 31, 2015, 00:54 William Kennington william@wkennington.com
wrote:

Well, the main reason I want this implemented is for updated packages so
you can reasonably trust hashes you get out of failed fetchurls.

On Sun, May 31, 2015, 00:53 Vladimír Čunát notifications@github.com
wrote:

In pkgs/build-support/fetchurl/default.nix
#8082 (comment):

@@ -1,4 +1,4 @@
-{ stdenv, curl }: # Note that curl' may benull', in case of the native stdenv.
+{ stdenv, curl, ca_cert_file ? null, insecure ? false }: # Note that curl' may benull', in case of the native stdenv.

Was there a discussion about (dis)advantages of using insecure connection
by default for fixed-output derivations? For most use cases I don't think
there's any added security, and I suppose there's some (small) performance
loss.

—
Reply to this email directly or view it on GitHub
https://github.com/NixOS/nixpkgs/pull/8082/files#r31387880.

I see. I suspect the main part of performance loss is due to extra latency of https handshake, but according to docs it seems that would happen even with --insecure with https, so it's more about whether to prefer https URLs over http ones.

I already thought before: it would be nice if fetching was done over plain http/ftp/... by default, but failed-hash ones would be re-checked securely afterward (ideally by PGP, or at least https re-fetch).

Actually, thinking about it again, most traffic is supposed to go from binary caches instead of fetching sources, so having https by default for source fetches sounds good. The actual checking of certs (--insecure) will probably have completely negligible overhead, given that you need to build the package after you fetch the sources.

BTW, adding that PGP scheme would be nice, maybe as opt-in via an impure env. variable for nixpkgs devs...

Oh it's not the performance I'm concerned about, I was wondering if it even
took into account proper CA validation when connecting over https. I dont
think it has a valid set of CA pem's in its configuration.

On Sun, May 31, 2015, 01:22 Vladimír Čunát notifications@github.com wrote:

In pkgs/build-support/fetchurl/default.nix
#8082 (comment):

@@ -1,4 +1,4 @@
-{ stdenv, curl }: # Note that curl' may benull', in case of the native stdenv.
+{ stdenv, curl, ca_cert_file ? null, insecure ? false }: # Note that curl' may benull', in case of the native stdenv.

Binary cache handling is done by nix itself, not nixpkgs. There, if all
fetches (from one cache) are done in a single connection, the overhead
should be pretty minimal. It might be considered to only fetch *.narinfo
files securely (tiny), but the performance difference doesn't seem worth
the bother (encoding long streams by symmetric ciphers).

—
Reply to this email directly or view it on GitHub
https://github.com/NixOS/nixpkgs/pull/8082/files#r31388082.

I think this is the relevant place: NixOS/nix#491.

Anyway does this look good otherwise?

On Sun, May 31, 2015, 01:32 Vladimír Čunát notifications@github.com wrote:

In pkgs/build-support/fetchurl/default.nix
#8082 (comment):

@@ -1,4 +1,4 @@
-{ stdenv, curl }: # Note that curl' may benull', in case of the native stdenv.
+{ stdenv, curl, ca_cert_file ? null, insecure ? false }: # Note that curl' may benull', in case of the native stdenv.

I think this is the relevant place: NixOS/nix#491
NixOS/nix#491.

—
Reply to this email directly or view it on GitHub
https://github.com/NixOS/nixpkgs/pull/8082/files#r31388163.

What about dropping the insecure parameter and consider let insecure = ca_cert_file != null;? Is there any sense in a different setting?

With passing the flag, the cert file should be unused; without the file the verification should always fail without --insecure, I think.

I want to make sure all implementations explicitly understand that a lack
of ca implies insecure but I wouldn't be against changing this.

On Sun, May 31, 2015, 01:53 Vladimír Čunát notifications@github.com wrote:

In pkgs/build-support/fetchurl/default.nix
#8082 (comment):

@@ -1,4 +1,4 @@
-{ stdenv, curl }: # Note that curl' may benull', in case of the native stdenv.
+{ stdenv, curl, ca_cert_file ? null, insecure ? false }: # Note that curl' may benull', in case of the native stdenv.

What about dropping the insecure parameter and consider let insecure =
ca_cert_file != null;? Is there any sense in a different setting?

—
Reply to this email directly or view it on GitHub
https://github.com/NixOS/nixpkgs/pull/8082/files#r31388309.

domenkozar · 2015-05-31T09:09:15Z

+1, having tls support in fetching is important as we don't know how users get the hash and they might use an incorrect sha. If we decide to use insecure fetchurl, we should fix it's message not to report the hash, but to rather instruct to use nix-prefetch-url.

If you are a direct user of fetchurl, you are encouraged to implement support for adding a default set of certificates for ssl verification. This can be done by providing the nss cacert ca-bundle.crt.

…-tools has it

…able

edolstra · 2015-07-07T13:28:41Z

I don't really see the point of this. I mean, the hash is verified by Nix, so what's wrong with using --insecure?

wkennington · 2015-07-07T17:42:42Z

For end users yes this is true, but if developers use the workflow of
changing the package version / invalidating the hash + use nix-build to
tell them the new hash they won't be using ssl since they are relying on
fetchurl. As someone who is guilty of almost always doing this I think it
should at least have ssl verification behind it.

On Tue, Jul 7, 2015, 06:28 Eelco Dolstra notifications@github.com wrote:

I don't really see the point of this. I mean, the hash is verified by Nix,
so what's wrong with using --insecure?

—
Reply to this email directly or view it on GitHub
#8082 (comment).

edolstra · 2015-07-07T18:00:24Z

Yeah, as @domenkozar said, it's better to use nix-prefetch-url for that.

wkennington · 2015-07-07T18:04:15Z

Sure, but it isn't always obvious what that URL is if you aren't directly
calling it (fetchFromGitHub and family) or the calls in Ruby gems. On top
of that it is too easy for other developers to follow the same practice. I
don't think the right answer is to not include this because users shouldn't
do it.

On Tue, Jul 7, 2015, 11:00 Eelco Dolstra notifications@github.com wrote:

Yeah, as @domenkozar https://github.com/domenkozar said, it's better to
use nix-prefetch-url for that.

—
Reply to this email directly or view it on GitHub
#8082 (comment).

edolstra · 2015-07-07T18:06:51Z

I always use the following hacky shell function around nix-prefetch-url:

prefetch-url-from() {
    local url=$(nix-instantiate --eval-only --xml --strict "${2:-.}" -A "$1".urls ${system:+--argstr system $system} | grep 'value=".*"' | sed 's^.*value="\(.*\)".*^\1^' | head)
    if test -n "$url"; then
        echo "fetching $url"
        nix-prefetch-url "$url"
    fi
}

To use:

$ cd nixpkgs
$ prefetch-from-url hello.src

will download the source of hello and print the hash on stdout.

infinisil · 2019-09-30T21:43:18Z

Would still love to have this!

vcunat added the mass-rebuild label May 31, 2015

vcunat reviewed May 31, 2015
View reviewed changes

domenkozar added the 1.severity: security Issues which raise a security issue, or PRs that fix one label May 31, 2015

wkennington added 8 commits May 31, 2015 14:20

linux/stdenv: Changes to accomodate new bootstrap-tools

f77907d

fetchurl: Add ssl verification support and require by default

8b81646

If you are a direct user of fetchurl, you are encouraged to implement support for adding a default set of certificates for ssl verification. This can be done by providing the nss cacert ca-bundle.crt.

linux/stdenv: Bootstrap can provide the ca-bundle as the new boostrap…

c5bdb50

…-tools has it

cacert: Allow this to be referenced before the real fetchurl is avail…

517c161

…able

fetchurl: Use cacert for the default ca-bundle.crt

6f24a7a

stdenv/darwin: Add ca-bundle support

3e87789

stdenv/native: Use insecure curl since we don't have certs

427527f

glibc: Don't force rpath

93ec6e3

wkennington force-pushed the master.bootstrap2 branch 7 times, most recently from 8a61525 to 95e2d55 Compare June 1, 2015 17:49

bootstrapTools: Update to newly built versions

00f3d8e

wkennington force-pushed the master.bootstrap2 branch from 95e2d55 to 00f3d8e Compare June 1, 2015 18:11

domenkozar added this to the 15.06 milestone Jun 2, 2015

domenkozar modified the milestones: 16.03, 15.09 Sep 9, 2015

wkennington closed this Dec 17, 2015

wkennington deleted the master.bootstrap2 branch December 17, 2015 19:33

Uh oh!

Conversation

wkennington commented May 31, 2015

Uh oh!

wkennington commented May 31, 2015

Uh oh!

wkennington commented May 31, 2015

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!

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!

domenkozar commented May 31, 2015

Uh oh!

edolstra commented Jul 7, 2015

Uh oh!

wkennington commented Jul 7, 2015

Uh oh!

edolstra commented Jul 7, 2015

Uh oh!

wkennington commented Jul 7, 2015

Uh oh!

edolstra commented Jul 7, 2015

Uh oh!

infinisil commented Sep 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants