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

Remove .envrc #325793

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Remove .envrc #325793

merged 2 commits into from
Jul 26, 2024

Conversation

toastal
Copy link
Contributor

@toastal toastal commented Jul 9, 2024

Description of changes

f0160ba#commitcomment-143982098

I use this file to work with different environment variables while working with derivations. With this file being tracked, now I am more prone to accidentally commit as well as needing to stash my changes just to get the latest code from git pull --rebase upstream master. I strongly feel this file is for personal use & just like you shouldn’t commit your personal editor’s config to the repository, leave this file alone so users can customize it--the ones that want to use direnv can echo 'use nix' >> .envrc && direnv allow which does not get in anyone’s way. With the red consent banner pop-up & forced default settings, tracking this file provides a degraded user experience for this project, not an improved one.

#322512
#322650

This threads are full of objections & similar use cases along with hacky solutions about .env.* ignored files. Just because one user says they are fine losing their workflow, does not assume everyone else is. I don’t follow all Nixpkgs threads to join the original discussion since there are thousands, so I don’t notice til things are merged & discussion is over.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@toastal toastal requested review from infinisil and a team as code owners July 9, 2024 09:30
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jul 9, 2024
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Same as NixOS/nixfmt#178.

@toastal
Copy link
Contributor Author

toastal commented Jul 9, 2024 via email

@Sigmanificient
Copy link
Member

Sigmanificient commented Jul 9, 2024

Shouldn't the .direnv entry stay in .gitignore? for people who use direnv, it would avoid tracking it's cache

@infinisil
Copy link
Member

I use this file to work with different environment variables while working with derivations

In a previous discussion I gave a bunch of workarounds: #322650 (comment). Do those not work for you? I'd appreciate more info on your use case.

And if they really don't work, I'd prefer a solution of sourcing a different additional file like .envrc.private (the security implications seem negligible). Though even better would be to allow this in direnv itself, because it's a common feature request.

@piegamesde
Copy link
Member

FWIW, Lix has implemented a similar workaround: https://git.lix.systems/lix-project/lix/src/branch/main/.envrc

However, I'd argue that this should primarily be fixed upstream, i.e. in direnv itself.

@toastal
Copy link
Contributor Author

toastal commented Jul 10, 2024

When I am packaging up & testing the execution of derivation, I have used environment variables for the executable’s settings, to turn on & make sure certain debugging flags still work, for setting different prelude/stdlib/other lib locations, etc. since you need to test that execution still works. Until I have packaged it correctly, having a persistent state for these variables is useful as it might take a few days hacking on a toolchain I am unfamiliar with. Doing all testing inside the sandbox is often slow, & it is still worth testing the binary works outside the sandbox too. In other places, I have used .envrc to echo critical version numbers to remember the state of this specific project & as others have mentioned, I too have used it for certain API keys. Point being, this is a tool used for the personal environment--much in the same way I would be miffed if someone’s text editor settings were committed & used overriding my own editor somehow to a repository, it feels as if this tool now becomes blocked from my personal usage since I have tracked changes adding a lot of friction & noise with the VCS. …And to what benefit? So users don’t have to run nix-shell or have to set up their own, untracked .envrc that they do just once. This doesn’t seem like a value add.

RE: lix’s .envrc.local & .envrc.private (where security implications are in the upstream thread). A different, also-unresolved [upstream Direnv thread](https://github.com/direnv/direnv/issues/556) is full of mulled-over suggestions, all of which point to leaving .envrc` out of the repo.

Initially direnv was designed for my personal usage so I didn't plan that projects would start adding .envrc into their repositories. But I do recognize the need of splitting the private and public .envrc.

When you see projects in say the PHP space using Dotenv + .env, .env is ignored by the VCS & there is a corresponding .env.sample or .env.example that can be copied, modified, or left untouched, as main .env file both can contain sensitive keys, but is also used for setting your own debug flags/log level--stuff for the individual. This is a simpler solution that does not involve sourcing now multiple files, or branching ifs, or awaiting a non-resolved upstream issue to be resolved; the simple solution is to just not commit file or code & let folks create it themselves or have an example. However, if you had an .envrc.example or .envrc.recommended with just use nix you would see that it’s pretty ridiculous to have lying around a basically empty file with no actual environment variables versus a line in the README that just says “Tip: if you want to use Direnv to automatically execute the Nix development shell run: echo "use nix" >> .envrc && direnv allow”--or, you know, leave that out of the docs as users that know Direnv know how to add this already & others may not want to use it or have a different tool & may not appreciate the implicit endorsement of the tool they aren’t using.

This is not also not addressing the scary, big, red, we-will-execute-code-on-your-machine banner you get when cloning the repository for the first time for those with Direnv installed (which I appreciate they ask for consent, +1). Especially if just wanting to read the code or make a documentation fix which does not require the Nix shell, this is a bit of a shock & a distraction. The current behavior might be the experience you would want at some Big.Corp where everyone will be required to consent to the environment, & everyone with repo access will be expected to be developing on the code, but nixpkgs, while big, isn’t Big.Corp where users of many backgrounds/experiences/use cases are paying a visit & I would argue Nixpkgs doesn’t need to suggest or endorse extra tooling unless required (even if it tooling I like & use daily).

@infinisil
Copy link
Member

Thanks for the explanation of your use case, indeed I don't think any workaround is fit for this, so again I suggest you add support for a .envrc.local, I'd be okay with that.

And to what benefit? So users don’t have to run nix-shell or have to set up their own, untracked .envrc that they do just once. This doesn’t seem like a value add.

The value comes from users automatically being informed about the shell existing. Without that, most users wouldn't even be aware of it. It also comes with editor integration for a very smooth auto-formatting experience.

And note that even just in public GitHub repos, over 14k of them commit their .envrc, so it's not uncommon ;)

@toastal
Copy link
Contributor Author

toastal commented Jul 12, 2024

Not uncommon is not the same as not what the developer intended, as quoted. Folks do stuff wrong all the time (it’s been a decade where I am still trying to get folks to stop using icon fonts, & look at any project with config not following XDG, curl | sh, committing .{idea,vscode}, using Docker instead of Nix, etc.) …what folks need are big projects setting good examples since the small projects copy from the big ones. I have been using Direnv in the way as the developer intends as a personal tool & these 14k projects on Microsoft Github have taken that autonomy away instead giving me a warning banner which is a reminder that I won’t be able to easily modify .envrc for my needs. And often I just get/clone out a project to read some source code locally where my tooling is better than some slow code forge UX that doesn’t work without authentication.

@emilazy
Copy link
Member

emilazy commented Jul 14, 2024

For what it’s worth, I think a fairly serious concern that isn’t related to any of these more subjective issues of preference is that if you run direnv allow in the Nixpkgs tree and then check out someone’s PR branch in the same directory, they instantly gain arbitrary code execution as your user, bypassing all protections like the build sandbox. The @NixOS/security ping for changes to the relevant files only helps if it’s noticed before someone checks the branch out.

That’s an issue with direnv in general – and I do still use and like it – but Nixpkgs is an especially central target given that it is so popular and that maintainers are constantly dealing with a large number of essentially untrusted pull requests. (You could fairly argue that anyone reviewing Nixpkgs PRs should take greater precautions in general as a result of this, but in practice I suspect almost none do.)

@emilazy
Copy link
Member

emilazy commented Jul 14, 2024

One potential solution would be to provide the relevant shell environment as an export from Nixpkgs and ensure that the .envrc does not import any Nix code from inside the tree (i.e., no shell.nix). That would require keeping nixfmt-rfc-style versions in sync across the release branches, but any attempt to hijack the facility through a PR would trigger another direnv permissions prompt.

@toonn
Copy link
Contributor

toonn commented Jul 14, 2024

I use worktrees a lot to work on several things at once, review PRs and such. Often these worktrees are short-lived. Since direnv records consent per path this basically means I get pestered with the warning no matter how many times I (dis)allow.

I really hope an alternative way of informing people how to enable useful tool integrations is found. Preferably one that doesn't force one particular solution among many alternatives, instructions that achieve equivalent behavior using Lorri for example.

@piegamesde
Copy link
Member

That’s an issue with direnv in general

I'd argue that this is only an issue with direnv-nix, because direnv generally checks the file for modifications and asks you for re-approval. Only when sourcing stuff from elsewhere (as is done in direnv-nix by default) causes the attack vector you describe.

Since I already advocated for upstream fixes to problems earlier, the mitigation would be to allow to track external files as part of the direnv source for modifications. As prior art, we already have the watch_file builtin which at least reloads the shell on modification, we'd need something similar but with the approval baked in. This would still require upholding the invariant that no source code outside of these files gets sources, but which sounds like an acceptable tradeoff to me.

@infinisil
Copy link
Member

I just noticed somebody accidentally pushing their .envrc changes in a PR (got notified as a code owner), which can be seen as an argument in favor of this PR.

toastal added 2 commits July 23, 2024 17:06
This reverts commit f0160ba.

This is an anti-feature for this project.
@emilazy
Copy link
Member

emilazy commented Jul 23, 2024

I'd argue that this is only an issue with direnv-nix, because direnv generally checks the file for modifications and asks you for re-approval. Only when sourcing stuff from elsewhere (as is done in direnv-nix by default) causes the attack vector you describe.

Since I already advocated for upstream fixes to problems earlier, the mitigation would be to allow to track external files as part of the direnv source for modifications. As prior art, we already have the watch_file builtin which at least reloads the shell on modification, we'd need something similar but with the approval baked in. This would still require upholding the invariant that no source code outside of these files gets sources, but which sounds like an acceptable tradeoff to me.

Yes. However I don’t think it would be feasible to prompt people for reapproval every time any file involved in the building of a Haskell project like nixfmt-rfc-style changes, regardless.

I lean more and more towards going with this PR and only shipping an .envrc.recommended after learning about situations like @toonn’s. However, even in that case, my security concerns in #325793 (comment) apply to anyone who takes that recommendation. Therefore, regardless of the outcome of this PR, I strongly recommend doing the following:

  1. Ship a nixpkgs-dev-shell package in Nixpkgs with the shell environment currently in shell.nix; backport it to 24.05 and keep it (and nixfmt-rfc-style) in sync across release branches.

  2. Remove shell.nix so that people aren’t tempted to run it from a potentially‐malicious branch. Don’t export it in flake.nix’s devShells for the same reason. Document nix-shell '<nixpkgs>' -A nixpkgs-dev-shell instead. (Perhaps it could even install a convenience binary to start the shell, so that people can put it in their environment.systemPackages if they want.)

  3. Make .envrc{,.recommended} do use flake nixpkgs#nixpkgs-dev-shell / use nix '<nixpkgs>' -A nixpkgs-dev-shell. This will use the system Nixpkgs, which is implicitly trusted, and avoid any risk from having a malicious branch checked out. As a bonus, it’ll still be possible to use the development environment even when working on staging branches that rebuild the world and would make direnv terribly annoying at present.

@piegamesde
Copy link
Member

However I don’t think it would be feasible to prompt people for reapproval every time any file involved in the building of a Haskell project like nixfmt-rfc-style changes, regardless.

I see. To prevent that, we'd have to not only pin the package but the entire Nixpkgs instead, so that only pin bumps re-prompt. Not sure if that's the current plan though

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Alright let's just go ahead with this PR for now. There's some good arguments for this, arguments which only make sense at the scale of Nixpkgs, and it's not hard to just do echo 'use nix' > .envrc for those that want it, even though it would be nice not needing to do that.

@infinisil infinisil dismissed mweinelt’s stale review July 25, 2024 22:28

Good arguments were brought up

@infinisil infinisil mentioned this pull request Jul 25, 2024
13 tasks
@infinisil infinisil merged commit 236fdd5 into NixOS:master Jul 26, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: policy discussion 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants