treewide: support NIX_SSL_CERT_FILE as an impureEnvVar#303307
treewide: support NIX_SSL_CERT_FILE as an impureEnvVar#303307lukegb merged 2 commits intoNixOS:stagingfrom
Conversation
timbertson
requested review from
Mic92,
ajs124,
infinisil,
kalbasit,
lukegb,
mweinelt and
zowoq
as code owners
github-actions
bot
added
8.has: documentation
|
@roberth would you mind taking another look at this now that I've rebased? |
ofborg
bot
added
10.rebuild-darwin-stdenv
wegank
added
the
2.status: merge conflict
|
Suggestions applied and rebased, thanks for the review @3noch |
ofborg
bot
removed
the
2.status: merge conflict
This envvar is also added to lib.proxyImpureEnvVars since it's typically required for https proxies. This change also updates fetchgit and go module fetching to use this envvar. NIX_GIT_SSL_CAINFO is still supported for backwards compatibility in fetchgit.
timbertson
force-pushed
the
https_proxy
branch
from
70ddd29 to
7eb5c09
Compare
|
Rebased again. I'd love to get this PR merged if anyone knows how to get any attention to it. It's been reviewed by a few people since I raised it nearly a year ago (as #271161), but I don't know who has authority to actually merge it. It is a bulk-rebuild, but it's also pretty small and should be a no-op in the typical case of "not using an HTTPS proxy". |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
lukegb
left a comment
lukegb
left a comment
There was a problem hiding this comment.
This change seems reasonable - there's some potential for slightly wonky things happening inside a nix-shell, but I have no serious objection and I can't think of any particular circumstances in which that would make much of a difference.
| @@ -1,7 +1,7 @@ | |||
| export NIX_SSL_CERT_FILE=@out@/etc/ssl/certs/ca-bundle.crt | |||
| export NIX_SSL_CERT_FILE="${NIX_SSL_CERT_FILE:-@out@/etc/ssl/certs/ca-bundle.crt}" | |||
There was a problem hiding this comment.
Would it make sense to check that the path in the env var is readable first before prioritizing its value over the built one?
I believe this change requires it to now be included in extra-sandbox-paths, which is unlikely to be the case in existing deployments using NIX_SSL_CERT_FILE prior to this change - and thus will suddenly point to something that doesn't exist as far as the builder is concerned.
It's worth noting that in recent versions, nix appears to bind-mount the file at NIX_SSL_CERT_FILE to /etc/ssl/certs/ca-certificates.crt, which means the builder may indeed have access to it but not at the path specified in the variable. There also may be interactions with remote builders to consider too?
There was a problem hiding this comment.
Would it make sense to check that the path in the env var is readable first before prioritizing its value over the built one?
Maybe? It wouldn't hurt, but I'm also surprised that you'd need to.
I believe this change requires it to now be included in extra-sandbox-paths, which is unlikely to be the case in existing deployments using NIX_SSL_CERT_FILE prior to this change
For build-daemon use cases, I've needed to work hard to set NIX_SSL_CERT_FILE. Either add it to the builder's env intentionally, or use the inpure-env config. So I assumed it wouldn't be accidentally set.
Maybe this is more common in a nixos deployment? It might be good to understand your use case and what happens after these changes, to confirm whether the right fix is "ignore NIX_SSL_CERT_FILE when it's not readable", or "don't do that thing you're doing". Maybe raise an issue and link it here for better visibility?
|
I just ran down the whole rabbit hole here, after being confused why most packages built fine behind my corp internet, until finally I ran into ssl cert errors when I used fetchurl with an overridden src and fake hash. Suddenly, no Putting this comment here before creating a full issue because it will be some time before I collect enough documentation to open a proper issue. |
8 participants
(this is a reopened version of #271161 for technical reasons)
Description of changes
tl;dr this PR makes
NIX_SSL_CERT_FILEthe preferred way to control CA certificates throughout nix, by allowing it to be set as in impureEnvVar, including it in proxyImpureEnvVars, and ensuring fetchers support it.Nix (the executable) as well as much of its packaged software respects
NIX_SSL_CERT_FILEas the preferred way to specify a custom set of root certificates. Within derivations, this is typically done by the setup hook ofcacert.However, there's no way to inject this variable from outside (with impure env vars used in fetchers). This is required for most use cases of an an https proxy, as the proxy's own certificate will not likely be trusted by nix's builtin cacert package.
Functionality for injecting a custom trust store has been added to
fetchgit, but because the setuphook will always overrideNIX_SSL_CERT_FILE, a different envvar had to be used (NIX_GIT_SSL_CAINFO). I have an open PR to add this same customisation to the go module fetcher.However, it'd be preferable if we didn't invent a new envvar, and instead made
NIX_SSL_CERT_FILEthe single way to control this setting for both fetchers and nix itself.For this to work, we need two small changes:
cacertshould only set this variable if it's not already set, so that any impure version is not overwritten"NIX_SSL_CERT_FILE"toproxyImpureEnvVars(fetcher.nix). It's not strictly a proxy-only variable, but:Various fetchers current (and updated) behaviour is oulined below:
NIX_GIT_SSL_CAINFOas the impure overrideable version. This is still supported, but the standardNIX_SSL_CERT_FILEnow works and is preferred.--insecureto curl for a fixed-output derivation, so I guess it doesn't need modificationGIT_SSL_CAINFOto the value ofNIX_SSL_CERT_FILEusing whatever's provided by the user / setup hookLooking through other fetchers, it seems most of them either have their own schemes or delegate to fetchurl. So this isn't a wide-reaching code change, but I think it's important to support
NIX_SSL_CERT_FILEas an impureEnvVar, so that we don't force fetchers to invent their own envvars for this purpose.Things done
Since this is a mass-rebuild, I tested locally by only applying these changes to the version of
cacertused by fetchers. With these modifications I tested fetching over https (using custom certs) with:nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)