Skip to content

nginxMainline: enable ktls support#147027

Merged
7c6f434c merged 5 commits intoNixOS:masterfrom
Izorkin:update-nginx-ktls
Dec 24, 2021
Merged

nginxMainline: enable ktls support#147027
7c6f434c merged 5 commits intoNixOS:masterfrom
Izorkin:update-nginx-ktls

Conversation

@Izorkin
Copy link
Contributor

@Izorkin Izorkin commented Nov 22, 2021

Motivation for this change

Activate TLS encryption in kernel space for nginx Mainline.
This PR is required - #146983

Example nginx configuration:

  services.nginx = {
    enable = true;
    package = pkgs.nginxMainline;
    virtualHosts."test.local" = {
      addSSL = true;
      forceSSL = false;
      enableACME = false;
      sslCertificate    = "/var/lib/cert/default.crt";
      sslCertificateKey = "/var/lib/cert/default.key";
      kTLS = true;
    };
  };

cc @ajs124 @Mic92

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/)
  • 21.11 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Nov 22, 2021
@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 22, 2021

Updated PR.

@ofborg ofborg bot requested a review from ajs124 November 22, 2021 20:02
@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 Nov 22, 2021
@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 22, 2021

Fixed nginx-variants tests.

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 23, 2021

Small fix.

@dasJ
Copy link
Member

dasJ commented Nov 24, 2021

This does currently not fail the eval when I enable ktls and I'm not using the mainline package, or am I wrong here?

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 24, 2021

An error will occur at startup with these parameters:

  services.nginx = {
    enable = true;
    package = pkgs.nginxStable;
    virtualHosts."test.local" = {
      addSSL = true;
      forceSSL = false;
      enableACME = false;
      sslCertificate    = "/var/lib/cert/default.crt";
      sslCertificateKey = "/var/lib/cert/default.key";
      kTLS = true;
    };
  };

The eval is not affected by.

@dasJ
Copy link
Member

dasJ commented Nov 24, 2021

Yeah sorry I think I was unclear with my statement there. I'd prefer if enabling ktls failed the eval instead of nginx starting because this can hint the user that something will break if they try to activate this option with their current choice of the nginx package.

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 24, 2021

I could not find a method to check the vhost.kTLS parameter.
Example error:

error: value is a string while a set was expected

       at /home/user/works/src/nixpkgs/nixos/modules/services/web-servers/nginx/default.nix:939:35:

          938|
          939|     boot.kernelModules = optional vhost.kTLS [ "tls" ];
             |                                   ^
          940|
(use '--show-trace' to show detailed location information)

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 27, 2021

@dasJ added check for nginx version + kTLS.

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 27, 2021

cc @dasJ @ajs124 @Mic92

@Izorkin
Copy link
Contributor Author

Izorkin commented Nov 30, 2021

@ofborg eval

@Izorkin
Copy link
Contributor Author

Izorkin commented Dec 11, 2021

cc @ajs124 @dasJ @Mic92

Copy link
Member

@ajs124 ajs124 left a comment

Choose a reason for hiding this comment

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

The diff LGTM, but I haven't done any real testing, so I'll not merge this right now. If anyone else does that testing, feel free to merge.

@7c6f434c
Copy link
Member

It also looks like it should only break (if wrong) the new stuff if it is enabled, not the old stuff

@7c6f434c 7c6f434c merged commit b0f154f into NixOS:master Dec 24, 2021
@Izorkin Izorkin deleted the update-nginx-ktls branch December 24, 2021 18:06
@ncfavier
Copy link
Member

ncfavier commented Jan 2, 2022

2f66ac0 broke my config (see below). hasSSL means that you have SSL enabled, which is precisely what you don't want when using rejectSSL.

{
  services.nginx.virtualHosts.default = {
    default = true;
    rejectSSL = true;
    extraConfig = "return 444;";
  };
}

@lopsided98
Copy link
Contributor

This breaks nginx on platforms without valgrind support (armv6l-linux in my case) because tengine transitively depends on valgrind. Even though tengine is not actually used by default, just evaluating it for the cfg.package != pkgs.tengine check triggers the unsupported platform error.

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: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants