Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nix behind SSL proxy #11638

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

zoranbosnjak
Copy link

Motivation

I have struggled with nix behind SSL proxy for a long time and this simple change solved the problem for me. I would appreciate if it gets merged to upstream.

There are several issues describing similar problem, for example:

I am not sure if this change fixes the mentioned problems in general. But it should resolve problem when using nix under ubuntu in a single user mode.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@@ -1847,7 +1850,7 @@ void LocalDerivationGoal::runChild()
/* N.B. it is realistic that these paths might not exist. It
happens when testing Nix building fixed-output derivations
within a pure derivation. */
for (auto & path : { "/etc/resolv.conf", "/etc/services", "/etc/hosts" })
for (auto & path : { "/etc/resolv.conf", "/etc/services", "/etc/hosts", "/etc/ssl/certs/ca-certificates.crt" })
Copy link
Member

Choose a reason for hiding this comment

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

This should be unnecessary because we already do

                if (settings.caFile != "" && pathExists(settings.caFile)) {
                    Path caFile = settings.caFile;
                    pathsInChroot.try_emplace("/etc/ssl/certs/ca-certificates.crt", canonPath(caFile, true), true);
                }

just below this point.

@@ -1192,6 +1192,9 @@ void LocalDerivationGoal::initEnv()

/* Trigger colored output in various tools. */
env["TERM"] = "xterm-256color";

/* Git needs access to system SSL certificats */
env["GIT_SSL_CAINFO"] = "/etc/ssl/certs/ca-certificates.crt";
Copy link
Member

Choose a reason for hiding this comment

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

This adds GIT_SSL_CAINFO to the environment of all builds. Presumably it should only be added in derivations that have access to the network (like fetchurl). This can be checked using derivationType->isSandboxed().

However, it's probably more appropriate to set this variable somewhere in Nixpkgs (such as in a setup hook of the git package, or in the fetchGit function itself). Generally we don't want to set a bunch of package-specific environment variables in Nix itself.

@zoranbosnjak
Copy link
Author

I have checked again with several revisions. It looks like the problem is already resolved in master branch and so this patch is not even necessary any more. For the reference, the patch was necessary in revision df9a71f. Closing.

@zoranbosnjak zoranbosnjak reopened this Oct 8, 2024
@zoranbosnjak
Copy link
Author

After some retesting, it turns out that this fix is actuall necessary. Regarding the review comments:

  • GIT_SSL_CAINFO is only added when not sandboxed
  • adding /etc/ssl/certs/ca-certificates.crt again is indeed not necessary

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

Successfully merging this pull request may close these issues.

2 participants