-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Direnv support #322650
Direnv support #322650
Conversation
Originally posted by @Frontear in #322512 (comment): Not a fan of a Having it default included means its tracked, which means per-user secrets have a chance to end up accidentally in a commit. Sure this is a user error and isn't necessarily nixpkgs responsibility, but I honestly think I'd prefer it being left out and left to the user to create and maintain. Originally posted by @infinisil in #322512 (comment): @Frontear It's very standard to commit the
Originally posted by @Frontear in #322512 (comment): I've always believed envrc shouldnt be commited due to it holding environment secrets, unless im conflating the role of .env with .envrc. In any case thats fair that everyone needing to manually make one is tedious. Originally posted by @peterhoeg in #322512 (comment): I'm using this pattern all over in for f in .env .env.local .env.secrets; do
dotenv_if_exists $f
done And those files are then obviously gitignored. |
Wasn't a fan at first but I support the notion of convenience for the nixpkgs community at large. Losing my ability to set secrets in one way (there are alternatives) is a small price to pay for overall convenience. Of course there can be an argument of opt-in semantics and automatic consent but I was never much of an extreme vocalist for such, nor do I personally care too much, so that argument isn't mine to make. 👍 for implementation! |
@@ -0,0 +1 @@ | |||
use nix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use nix | |
use nix | |
watch_file .envrc.private | |
if [[ -f .envrc.private ]]; then | |
source_env .envrc.private | |
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in .envrc.private people than can do things like:
export GITHUB_TOKEN=$(rbw get gitlab.com-token)
or whatever they like to also add to their nixpkgs development workflow.
We would than add .envrc.private to .gitignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit more flexible than restricting the choice to dotenv, where one could only add hardcode static secrets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main problem with this seems to be that it doesn't require the user to confirm it if the .envrc.private
file changed, see direnv/direnv#348. Especially in Nixpkgs this considerably increases the attack surface, since we run so many update scripts, which could populate .envrc.private
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from changing shell.nix to add a shellHook or changing any code in nixpkgs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess at least the shell.nix
is being code owned by the security team, so if somebody does something sketchy they'd be notified. Can't do the same here. Ping @NixOS/security for input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add .envrc.private
than as well to the code owner list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway. Not the hill, I want to die on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add .envrc.private than as well to the code owner list?
Unless I'm missing something, code owners only work when you commit the file to GitHub, which defeats the point of it :P
Anyways, I don't want to say No to this, just that I'm personally not sure about the security impact of it. So I'd say let's go with a more minimal solution without .private
for now, which we can then still change in a backwards-compatible way later if necessary.
Description of changes
Split off from #322512, since there's a bunch of discussion just about direnv.
This creates a
.envrc
, so that if you have https://direnv.net/ installed, theshell.nix
is loaded automatically.Ping @Frontear @mweinelt @peterhoeg
Things done
Add a 👍 reaction to pull requests you find important.