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: re-enable ssl_session_tickets #350018

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bmillwood
Copy link
Contributor

Mozilla's recommendations no longer include this item. Per mozilla/server-side-tls#135 and
mozilla/ssl-config-generator#252, it sounds like versions of nginx prior to 1.23.2 (released 2022-10-19) didn't make it easy to properly rotate the ticket encryption keys, but since that version it's now done automatically. Since nixos-24.05 is already on nginx-1.26.2, it seems safe to remove this option.

I came across this while looking to see what was included in recommendedTlsSettings and noticing that it had fallen out of sync with its own source. I don't personally feel strongly that ssl_session_tickets is important to set one way or the other, this just seems like housekeeping to me. I believe it has performance implications (reducing the amount of memory nginx needs?) but the docs are not very helpful.

One other notable difference between the Mozilla settings and ours is Mozilla enables HSTS. I don't know if this is not included because it's new or if it's omitted intentionally, but I decided not to include it here because it seems like it could be a more significant / impactful / client-facing change. Let me know if we want it and I might do a followup PR, perhaps adding it as an option.

Things done

  • Built on platform(s)
    • x86_64-linux -- well, actually I cherry-picked onto nixos-24.05 and then built that, but I imagine that's sufficient?
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true (because it's the default on Linux and I haven't overridden it)
  • 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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 20, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 20, 2024
@emilazy
Copy link
Member

emilazy commented Oct 21, 2024

I’m a little nervous about the compromise to forward secrecy posed by session tickets, but if we’re tracking Mozilla’s recommendations here then it makes sense to match the update.

I think we should not enable HSTS out of the box since it is the kind of policy decision that is hard to revert and can cause painful availability issues. An option for it might be nice though.

@bmillwood
Copy link
Contributor Author

bmillwood commented Oct 21, 2024

I’m a little nervous about the compromise to forward secrecy posed by session tickets, but if we’re tracking Mozilla’s recommendations here then it makes sense to match the update.

Equally happy to update the comment to say that we're not following Mozilla's recommendations, I don't have a horse in that race :)

I think we should not enable HSTS out of the box since it is the kind of policy decision that is hard to revert and can cause painful availability issues. An option for it might be nice though.

Cool. I'll probably make a separate PR adding the option. (edit: #350302)

Mozilla's [recommendations][1] no longer include this item. Per
[mozilla/server-side-tls#135][2] and
[mozilla/ssl-config-generator#252][3], it sounds like versions of nginx
prior to 1.23.2 (released 2022-10-19) didn't make it easy to properly
rotate the ticket encryption keys, but since that version it's now done
automatically. Since nixos-24.05 is already on nginx-1.26.2, it seems
safe to remove this option.

[1]: https://ssl-config.mozilla.org/#server=nginx&config=intermediate
[2]: mozilla/server-side-tls#135
[3]: mozilla/ssl-config-generator#252
@bmillwood bmillwood force-pushed the update-recommended-tls-settings branch from 283ce1d to db13dd0 Compare November 4, 2024 11:15
@bmillwood
Copy link
Contributor Author

@emilazy do you want to choose something to happen here?

@janbrasna
Copy link

Feel free to feed any concerns back to us @mozilla/ssl-config-generator via issues or discussions — chances are if it's not good enough for you, it probably won't be for others, too;)

it sounds like versions of nginx prior to 1.23.2 (released 2022-10-19) didn't make it easy to properly rotate the ticket encryption keys, but since that version it's now done automatically.

Their changelog makes it sound it's safe now, but from the mailinglist changeset it looked more like just a better GC at first glance — but I will look more deeply into what's the actual change: it's the OCT '22 commits here: https://github.com/nginx/nginx/commits/release-1.23.2/ if you want to inspect yourselves if that's reasonably safe for PFS.

One other notable difference between the Mozilla settings and ours is Mozilla enables HSTS. I don't know if this is not included because it's new or if it's omitted intentionally, but I decided not to include it here

It's been around for years, but for these exact concerns downstream the HSTS is only behind a preference to help emit the correct headers and time spans, but also make it easy to consume configs without these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants