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

nixos/nginx: add strictTransportSecurity options #350302

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

Conversation

bmillwood
Copy link

Let me know if this needs a changelog entry, I wasn't sure whether it was significant enough.

This is my first nontrivial nixpkgs PR, so sorry about any mess :) I didn't nixfmt these modules because it would change a lot more than what I touched.

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.

This is pretty easy for the user to set themselves, but it's a
recommended security setting so making it more discoverable / encouraged
seems like a good thing.

includeSubdomains = mkOption {
type = types.bool;
default = true;
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this should be on by default, it is a footgun unless you are really prepared. Lots of cases of random sub‐subdomains with internal test deployments or services that aren’t ready for it.

(Haven’t done a thorough review of the rest of the PR yet.)

Copy link
Author

@bmillwood bmillwood Oct 21, 2024

Choose a reason for hiding this comment

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

sure, done :)

tbh my attitude was "enableACME is so easy these days" but yeah I guess non-public uses have the potential to be trickier

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the problem is that it‘s easy if you have one single NixOS machine serving your entire domain hierarchy, and it‘s easy if you have a trillion machines that are all behind consistent HTTPS frontend stuff proxying everything to your highly‐structured web scale enterprise backends, but there‘s a wide range of setups between those where you just don‘t have complete visibility and control into everything going on under *.mycorp.com. Intranet, legacy SaaS, a random appliance you can‘t update, … there’s a big gap between “this web server on this domain can reliably do HTTPS” and “absolutely nothing under this hierarchy exists that cannot reliably do HTTPS in every single context”, and with HSTS it’s easy to screw up painfully. Of course people should set it if they can.

Will try to take a more detailed look at this PR in the next few days if nobody beats me to it, feel free to ping if I forget.

For more-or-less the same reasons that HSTS defaults false: accidentally
enabling it when you didn't intend to can be really annoying.
@bmillwood
Copy link
Author

bmillwood commented Oct 21, 2024

Something else I considered: config warnings or assertions. I didn't add any because they seem kind of obnoxious if they ever have false positives, they're mostly kind of obvious instead of something subtle that could trip people up, and trusting the user to know what they're doing seems good, at least when they don't have an easy option to say "shut up, I know what I'm doing". But here are the ones I considered:

  • complain if strictTransportSecurity.enable && !hasSSL -- but maybe there's some proxied setup where it makes sense for nginx to add the header even though SSL termination is somewhere else?
  • includeSubdomains && !enable -- here, in contrast to the above, the includeSubdomains setting is actually useless, but it might still be convenient if you're doing something like "merge these defaults into every vhost, but then disable HSTS for this one in particular"
  • preload = true when seconds or includeSubdomains don't meet Google's standards. I think this one I'm a bit more on the fence but it'll be annoying if Google's standards ever change, it doesn't really cause any harm to have the setting wrong, and people will need to go via the form submission website anyway, which will tell them what they need to know.

@bmillwood
Copy link
Author

whoever reviews this: is it OK to just tag a random apache-httpd reviewer (or reviewers) to ask if they want the same to be added there? (which also prompts the question: should we be sharing code between the two / plus maybe caddy / others as well? my guess is that code sharing usually isn't helpful and it doesn't make sense to bother doing it for those few cases where it would work, but idk the idea of having all webserver configs look ~ the same is cool)

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

Successfully merging this pull request may close these issues.

2 participants