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.

Sign up for GitHub

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

Jump to bottom

nixos/nginx: first-class PROXY protocol support #213510

Merged
merged 1 commit into from
May 27, 2023
Merged

nixos/nginx: first-class PROXY protocol support #213510

merged 1 commit into from
May 27, 2023

Conversation

RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Jan 30, 2023 •
edited
Loading

Description of changes

This PR enables you to use PROXY protocol with maximum flexibility, as shown in the test.

An example of use-case is when you want to offer your legacy IP clients a way to connect to your IPv6-only server, but you want to have proper logs, so you will put up some TLS-SNI thing above NGINX (can be ngx_stream itself.)
While preventing anyone to spoof their IP address.

See https://www.mythic-beasts.com/support/topics/proxy for a compelling introduction of why this is interesting.

Please read the commit message for a summary.

I am running this PR in production on multiple servers at the moment to smoke out any problem.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

SuperSandro2000
SuperSandro2000 reviewed Jan 30, 2023
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Don't we need some assertions around proxyProtocol? I vaguely remember that it disabled other protocols.

nixos/modules/services/web-servers/nginx/vhost-options.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/nginx/default.nix Show resolved Hide resolved
@RaitoBezarius RaitoBezarius force-pushed the nginx-proxyprotocol branch 2 times, most recently from 7c49ac9 to 28ab4a4 Compare March 5, 2023 17:23
@RaitoBezarius
Copy link
Member Author

Don't we need some assertions around proxyProtocol? I vaguely remember that it disabled other protocols.

PROXY protocol disable normal HTTP requests, the only broken scenario I see is ACME retrieval of certificates.

If you have only PROXY listeners and you ask for a ACME certificate, it will fail.
Therefore, we should have an assert error for this kind of stuff and I think that's pretty much it?

@RaitoBezarius RaitoBezarius marked this pull request as ready for review March 5, 2023 17:56
@RaitoBezarius
Copy link
Member Author

I guess I should rebase on a recent revision of master, I'm running all the NixOS tests for NGINX at the moment, once this is good, I will rebase and it will be review time :).

@RaitoBezarius RaitoBezarius marked this pull request as draft March 13, 2023 20:11
@RaitoBezarius RaitoBezarius marked this pull request as ready for review March 13, 2023 20:12
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/1968

SuperSandro2000
SuperSandro2000 reviewed Mar 19, 2023
View reviewed changes
nixos/doc/manual/release-notes/rl-2305.section.md Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/nginx/default.nix Outdated Show resolved Hide resolved
nixos/tests/nginx-proxyprotocol/generate-certs.nix Outdated Show resolved Hide resolved
nixos/tests/nginx-proxyprotocol/generate-certs.nix Outdated Show resolved Hide resolved
nixos/tests/nginx-proxyprotocol/default.nix Outdated Show resolved Hide resolved
nixos/tests/nginx-proxyprotocol/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/nginx/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/nginx/default.nix Outdated Show resolved Hide resolved
@RaitoBezarius RaitoBezarius force-pushed the nginx-proxyprotocol branch 2 times, most recently from 2e181d5 to b97f026 Compare March 19, 2023 20:45
@RaitoBezarius RaitoBezarius force-pushed the nginx-proxyprotocol branch 2 times, most recently from f68624b to 8cb42af Compare March 19, 2023 22:19
@lukegb lukegb self-assigned this Mar 20, 2023
@RaitoBezarius
Copy link
Member Author

Could I bother NGINX maintainers @ajs124 @fpletz @emilazy to take a look at this? I would love to have it as part of 23.05, but I understand the breakage potential.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2089

@RaitoBezarius RaitoBezarius added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Apr 19, 2023
@RaitoBezarius RaitoBezarius force-pushed the nginx-proxyprotocol branch 2 times, most recently from c25a944 to 4488e1d Compare April 21, 2023 16:05
@RaitoBezarius
Copy link
Member Author

Friendly ping @fpletz @ajs124 @emilazy @lukegb :).

@RaitoBezarius RaitoBezarius marked this pull request as draft May 5, 2023 15:11
@RaitoBezarius
Copy link
Member Author

We missed the deadline for a change in 23.05, I wish we can merge this post-branch off though.

@RaitoBezarius RaitoBezarius marked this pull request as ready for review May 22, 2023 20:02
@RaitoBezarius
Copy link
Member Author

We branched-off, I suggest to merge this to have plenty of time to deal with breakages @fpletz @ajs124 @emilazy @lukegb ? :)

@RaitoBezarius
nixos/nginx: first-class PROXY protocol support
69bb0f9 
PROXY protocol is a convenient way to carry information about the
originating address/port of a TCP connection across multiple layers of
proxies/NAT, etc.

Currently, it is possible to make use of it in NGINX's NixOS module, but
is painful when we want to enable it "globally".
Technically, this is achieved by reworking the defaultListen options and
the objective is to have a coherent way to specify default listeners in
the current API design.
See `mkDefaultListenVhost` and `defaultListen` for the details.

It adds a safeguard against running a NGINX with no HTTP listeners (e.g.
only PROXY listeners) while asking for ACME certificates over HTTP-01.

An interesting usecase of PROXY protocol is to enable seamless IPv4 to
IPv6 proxy with origin IPv4 address for IPv6-only NGINX servers, it is
demonstrated how to achieve this in the tests, using sniproxy.

Finally, the tests covers:

- NGINX `defaultListen` mechanisms are not broken by these changes;
- NGINX PROXY protocol listeners are working in a final usecase
  (sniproxy);
- uses snakeoil TLS certs from ACME setup with wildcard certificates;

In the future, it is desirable to spoof-attack NGINX in this scenario to
ascertain that `set_real_ip_from` and all the layers are working as
intended and preventing any user from setting their origin IP address to
any arbitrary, opening up the NixOS module to bad™ vulnerabilities.

For now, it is quite hard to achieve while being minimalistic about the
tests dependencies.
@RaitoBezarius
Copy link
Member Author

@ofborg test nginx-proxyprotocol

@RaitoBezarius RaitoBezarius merged commit d74e5f4 into NixOS:master May 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@Izorkin Izorkin Izorkin left review comments

@fpletz fpletz fpletz approved these changes

@ajs124 ajs124 Awaiting requested review from ajs124

@emilazy emilazy Awaiting requested review from emilazy

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

@lblasc lblasc Awaiting requested review from lblasc

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

@globin globin Awaiting requested review from globin

Assignees

@lukegb lukegb

Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
Nix documentation
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RaitoBezarius @nixos-discourse @fpletz @ajs124 @SuperSandro2000 @Izorkin @lukegb