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

nginx: support basic auth in location blocks #101192

Merged
merged 4 commits into from Nov 2, 2020

Conversation

@grahamc
Copy link
Member

@grahamc grahamc commented Oct 20, 2020

Motivation for this change

Extend the nginx support for basic auth to the location blocks.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@grahamc grahamc changed the title nixpkgs: support basic auth in location blocks nginx: support basic auth in location blocks Oct 20, 2020
Copy link
Member

@aanderse aanderse left a comment

Usually I'm a pretty big grump for passwords in the nix store, but if you're doing basic auth you probably aren't using it for real protection anyways... and I'm certainly guilty of having passwords in httpd configs under /etc that are world readable (on Debian, at least).

I guess I should whip something together for the httpd module so I can stay keeping up with the Joneses...

@aanderse aanderse requested a review from Mic92 Oct 20, 2020
default = null;
description = ''
Basic Auth password file for a vhost.
Can be created via: <command>htpasswd -c &lt;filename&gt; &lt;username&gt;</command>
Copy link
Member

@ajs124 ajs124 Oct 20, 2020

Maybe it should be mentioned that the hashes htpasswd produces by default and all the hashes nginx actually understands aren't really considered good for storing passwords anymore.

Copy link
Member Author

@grahamc grahamc Oct 20, 2020

Possibly so, what would you like the wording to be? Note: these options and their text mirror the vhost-options.nix content.

Copy link
Member

@ajs124 ajs124 Oct 20, 2020

Right. I mean, in an ideal world I would like this to say "use htpasswd -B to create bcrypt hashed passwords", but nginx doesn't support those.

So maybe just something along the lines of: "Warning: The generate file contains the users' passwords in a non-cryptographically-securely hashed way"

@@ -243,6 +243,7 @@ in
nfs4 = handleTest ./nfs { version = 4; };
nghttpx = handleTest ./nghttpx.nix {};
nginx = handleTest ./nginx.nix {};
nginx-auth= handleTest ./nginx-auth.nix {};
Copy link
Member

@ajs124 ajs124 Oct 20, 2020

missing space

description = ''
Basic Auth password file for a vhost.
Can be created via: <command>htpasswd -c &lt;filename&gt; &lt;username&gt;</command>
'';
Copy link
Member

@Mic92 Mic92 Oct 26, 2020

Suggested change
'';
WARNING: The generate file contains the users' passwords in a non-cryptographically-securely hashed way
'';

Mic92
Mic92 approved these changes Oct 26, 2020
@grahamc grahamc force-pushed the nixpkgs-location-basic-auth branch from 82774da to 3361a03 Nov 2, 2020
@grahamc
Copy link
Member Author

@grahamc grahamc commented Nov 2, 2020

I added that warning and squashed the fixup. Thanks! I'll merge once the PR is green.

@grahamc grahamc merged commit 75a2bc9 into NixOS:master Nov 2, 2020
19 of 20 checks passed
@grahamc grahamc deleted the nixpkgs-location-basic-auth branch Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants