From 135d54f5358aaf2c71bea0a1ce3cdaaadb7a5222 Mon Sep 17 00:00:00 2001 From: Yegor Timoshenko Date: Sat, 13 Oct 2018 21:45:25 +0000 Subject: [PATCH 1/6] nginx: if root is in Nix store, use path's hash as ETag Resolves #25485. Usage example: $ realpath /var/www /nix/store/wnrhnnpdj3x50j5xz38zp1qxs1ygwccw-site $ curl --head localhost HTTP/1.1 200 OK Server: nginx Date: Fri, 28 Sep 2018 06:09:25 GMT Content-Type: text/html Content-Length: 50 Last-Modified: Thu, 01 Jan 1970 00:00:01 GMT Connection: keep-alive ETag: "wnrhnnpdj3x50j5xz38zp1qxs1ygwccw" Accept-Ranges: bytes --- pkgs/servers/http/nginx/generic.nix | 4 +- pkgs/servers/http/nginx/nix-etag-1.15.4.patch | 87 +++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 pkgs/servers/http/nginx/nix-etag-1.15.4.patch diff --git a/pkgs/servers/http/nginx/generic.nix b/pkgs/servers/http/nginx/generic.nix index 02bef43c184f..fa7864886dc5 100644 --- a/pkgs/servers/http/nginx/generic.nix +++ b/pkgs/servers/http/nginx/generic.nix @@ -75,7 +75,9 @@ stdenv.mkDerivation { preConfigure = (concatMapStringsSep "\n" (mod: mod.preConfigure or "") modules); - patches = stdenv.lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [ + patches = [ + ./nix-etag-1.15.4.patch + ] ++ stdenv.lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [ (fetchpatch { url = "https://raw.githubusercontent.com/openwrt/packages/master/net/nginx/patches/102-sizeof_test_fix.patch"; sha256 = "0i2k30ac8d7inj9l6bl0684kjglam2f68z8lf3xggcc2i5wzhh8a"; diff --git a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch new file mode 100644 index 000000000000..7415b4e15d01 --- /dev/null +++ b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch @@ -0,0 +1,87 @@ +From 3ed81ef9e973247d0c95bf240b3bd2e640f2605a Mon Sep 17 00:00:00 2001 +From: Yegor Timoshenko +Date: Fri, 28 Sep 2018 03:27:04 +0000 +Subject: [PATCH] If root is in Nix store, set ETag to its path hash + +--- + src/http/ngx_http_core_module.c | 56 +++++++++++++++++++++++++++------ + 1 file changed, 47 insertions(+), 9 deletions(-) + +diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c +index c57ec00c..d996d7e3 100644 +--- a/src/http/ngx_http_core_module.c ++++ b/src/http/ngx_http_core_module.c +@@ -1583,6 +1583,7 @@ ngx_http_set_etag(ngx_http_request_t *r) + { + ngx_table_elt_t *etag; + ngx_http_core_loc_conf_t *clcf; ++ u_char *real, *ptr1, *ptr2; + + clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); + +@@ -1598,16 +1599,53 @@ ngx_http_set_etag(ngx_http_request_t *r) + etag->hash = 1; + ngx_str_set(&etag->key, "ETag"); + +- etag->value.data = ngx_pnalloc(r->pool, NGX_OFF_T_LEN + NGX_TIME_T_LEN + 3); +- if (etag->value.data == NULL) { +- etag->hash = 0; +- return NGX_ERROR; +- } ++ real = ngx_realpath(clcf->root.data, NULL); ++ ++ #define NIX_STORE_DIR "/nix/store" ++ #define NIX_STORE_LEN ngx_strlen(NIX_STORE_DIR) ++ ++ if (!ngx_strncmp(real, NIX_STORE_DIR, NIX_STORE_LEN) ++ && real[NIX_STORE_LEN] == '/' ++ && real[NIX_STORE_LEN + 1] != '\0') ++ { ++ ptr1 = real + NIX_STORE_LEN; ++ *ptr1 = '"'; ++ ++ ptr2 = (u_char *) ngx_strchr(ptr1, '-'); ++ ++ if (ptr2 == NULL) { ++ ngx_free(real); ++ etag->hash = 0; ++ return NGX_ERROR; ++ } ++ ++ *ptr2++ = '"'; ++ *ptr2 = '\0'; + +- etag->value.len = ngx_sprintf(etag->value.data, "\"%xT-%xO\"", +- r->headers_out.last_modified_time, +- r->headers_out.content_length_n) +- - etag->value.data; ++ etag->value.len = ngx_strlen(ptr1); ++ etag->value.data = ngx_pnalloc(r->pool, etag->value.len); ++ ++ if (etag->value.data == NULL) { ++ ngx_free(real); ++ etag->hash = 0; ++ return NGX_ERROR; ++ } ++ ++ ngx_memcpy(etag->value.data, ptr1, etag->value.len); ++ } else { ++ etag->value.data = ngx_pnalloc(r->pool, NGX_OFF_T_LEN + NGX_TIME_T_LEN + 3); ++ ++ if (etag->value.data == NULL) { ++ ngx_free(real); ++ etag->hash = 0; ++ return NGX_ERROR; ++ } ++ ++ etag->value.len = ngx_sprintf(etag->value.data, "\"%xT-%xO\"", ++ r->headers_out.last_modified_time, ++ r->headers_out.content_length_n) ++ - etag->value.data; ++ } + + r->headers_out.etag = etag; + +-- +2.18.0 + From f03302b6362c8291b9390fb3aa6ee8f0d5098ec4 Mon Sep 17 00:00:00 2001 From: Yegor Timoshenko Date: Tue, 16 Oct 2018 12:58:37 +0000 Subject: [PATCH 2/6] nginx: check for realpath() == NULL in ETag patch Thanks to Gabriel Ebner! --- pkgs/servers/http/nginx/nix-etag-1.15.4.patch | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch index 7415b4e15d01..a4ee95076c1f 100644 --- a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch +++ b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch @@ -1,14 +1,14 @@ -From 3ed81ef9e973247d0c95bf240b3bd2e640f2605a Mon Sep 17 00:00:00 2001 +From a0ac82e4c79f359a7001a265cdb57e35978c6c23 Mon Sep 17 00:00:00 2001 From: Yegor Timoshenko Date: Fri, 28 Sep 2018 03:27:04 +0000 Subject: [PATCH] If root is in Nix store, set ETag to its path hash --- - src/http/ngx_http_core_module.c | 56 +++++++++++++++++++++++++++------ - 1 file changed, 47 insertions(+), 9 deletions(-) + src/http/ngx_http_core_module.c | 55 +++++++++++++++++++++++++++++---- + 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c -index c57ec00c..d996d7e3 100644 +index c57ec00c..4eec7ec5 100644 --- a/src/http/ngx_http_core_module.c +++ b/src/http/ngx_http_core_module.c @@ -1583,6 +1583,7 @@ ngx_http_set_etag(ngx_http_request_t *r) @@ -19,17 +19,23 @@ index c57ec00c..d996d7e3 100644 clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); -@@ -1598,16 +1599,53 @@ ngx_http_set_etag(ngx_http_request_t *r) +@@ -1598,16 +1599,58 @@ ngx_http_set_etag(ngx_http_request_t *r) etag->hash = 1; ngx_str_set(&etag->key, "ETag"); - etag->value.data = ngx_pnalloc(r->pool, NGX_OFF_T_LEN + NGX_TIME_T_LEN + 3); - if (etag->value.data == NULL) { -- etag->hash = 0; -- return NGX_ERROR; -- } + real = ngx_realpath(clcf->root.data, NULL); + ++ if (real == NULL) { + etag->hash = 0; + return NGX_ERROR; + } + +- etag->value.len = ngx_sprintf(etag->value.data, "\"%xT-%xO\"", +- r->headers_out.last_modified_time, +- r->headers_out.content_length_n) +- - etag->value.data; + #define NIX_STORE_DIR "/nix/store" + #define NIX_STORE_LEN ngx_strlen(NIX_STORE_DIR) + @@ -50,11 +56,7 @@ index c57ec00c..d996d7e3 100644 + + *ptr2++ = '"'; + *ptr2 = '\0'; - -- etag->value.len = ngx_sprintf(etag->value.data, "\"%xT-%xO\"", -- r->headers_out.last_modified_time, -- r->headers_out.content_length_n) -- - etag->value.data; ++ + etag->value.len = ngx_strlen(ptr1); + etag->value.data = ngx_pnalloc(r->pool, etag->value.len); + @@ -83,5 +85,5 @@ index c57ec00c..d996d7e3 100644 r->headers_out.etag = etag; -- -2.18.0 +2.19.0 From 1da8eec00fbe79301fcd5ce0b091405fc475fab0 Mon Sep 17 00:00:00 2001 From: Yegor Timoshenko Date: Tue, 16 Oct 2018 13:24:34 +0000 Subject: [PATCH 3/6] nginx: handle impure symlinks in ETag patch --- pkgs/servers/http/nginx/nix-etag-1.15.4.patch | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch index a4ee95076c1f..4d8a6510cbfe 100644 --- a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch +++ b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch @@ -1,14 +1,14 @@ -From a0ac82e4c79f359a7001a265cdb57e35978c6c23 Mon Sep 17 00:00:00 2001 +From f6a978f024d01202f954483423af1b2d5d5159a6 Mon Sep 17 00:00:00 2001 From: Yegor Timoshenko Date: Fri, 28 Sep 2018 03:27:04 +0000 Subject: [PATCH] If root is in Nix store, set ETag to its path hash --- - src/http/ngx_http_core_module.c | 55 +++++++++++++++++++++++++++++---- - 1 file changed, 49 insertions(+), 6 deletions(-) + src/http/ngx_http_core_module.c | 56 +++++++++++++++++++++++++++++---- + 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c -index c57ec00c..4eec7ec5 100644 +index c57ec00c..b7992de2 100644 --- a/src/http/ngx_http_core_module.c +++ b/src/http/ngx_http_core_module.c @@ -1583,6 +1583,7 @@ ngx_http_set_etag(ngx_http_request_t *r) @@ -19,7 +19,7 @@ index c57ec00c..4eec7ec5 100644 clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); -@@ -1598,16 +1599,58 @@ ngx_http_set_etag(ngx_http_request_t *r) +@@ -1598,16 +1599,59 @@ ngx_http_set_etag(ngx_http_request_t *r) etag->hash = 1; ngx_str_set(&etag->key, "ETag"); @@ -39,7 +39,8 @@ index c57ec00c..4eec7ec5 100644 + #define NIX_STORE_DIR "/nix/store" + #define NIX_STORE_LEN ngx_strlen(NIX_STORE_DIR) + -+ if (!ngx_strncmp(real, NIX_STORE_DIR, NIX_STORE_LEN) ++ if (r->headers_out.last_modified_time == 1 ++ && !ngx_strncmp(real, NIX_STORE_DIR, NIX_STORE_LEN) + && real[NIX_STORE_LEN] == '/' + && real[NIX_STORE_LEN + 1] != '\0') + { From af5a3ce4740e78b667994ed444a72bec610b3ec8 Mon Sep 17 00:00:00 2001 From: aszlig Date: Thu, 18 Apr 2019 08:13:41 +0200 Subject: [PATCH 4/6] nginx: Fix memleak in nix-etag patch The original patch introduced a new "real" variable which gets populated (and allocated) via ngx_realpath(). It's properly freed in error conditions but it won't be freed if ngx_http_set_etag returns successfully. Adding another ngx_free() just before returning fixes that memory leak. I also fixed a small indentation issue along the way. Signed-off-by: aszlig --- pkgs/servers/http/nginx/nix-etag-1.15.4.patch | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch index 4d8a6510cbfe..1a8dcb4303ad 100644 --- a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch +++ b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch @@ -19,7 +19,7 @@ index c57ec00c..b7992de2 100644 clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module); -@@ -1598,16 +1599,59 @@ ngx_http_set_etag(ngx_http_request_t *r) +@@ -1598,16 +1599,61 @@ ngx_http_set_etag(ngx_http_request_t *r) etag->hash = 1; ngx_str_set(&etag->key, "ETag"); @@ -44,7 +44,7 @@ index c57ec00c..b7992de2 100644 + && real[NIX_STORE_LEN] == '/' + && real[NIX_STORE_LEN + 1] != '\0') + { -+ ptr1 = real + NIX_STORE_LEN; ++ ptr1 = real + NIX_STORE_LEN; + *ptr1 = '"'; + + ptr2 = (u_char *) ngx_strchr(ptr1, '-'); @@ -82,6 +82,8 @@ index c57ec00c..b7992de2 100644 + r->headers_out.content_length_n) + - etag->value.data; + } ++ ++ ngx_free(real); r->headers_out.etag = etag; From d533285224de23354f445f728049e7758d686f20 Mon Sep 17 00:00:00 2001 From: aszlig Date: Thu, 18 Apr 2019 09:35:26 +0200 Subject: [PATCH 5/6] nixos/tests/nginx: Add subtest for Nix ETag patch This is to make sure that we get different ETag values whenever we switch to a different store path but with the same file contents. I've checked this against the old behaviour without the patch and it fails as expected. Signed-off-by: aszlig --- nixos/tests/nginx.nix | 70 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 14 deletions(-) diff --git a/nixos/tests/nginx.nix b/nixos/tests/nginx.nix index a4d14986a146..d66d99821c11 100644 --- a/nixos/tests/nginx.nix +++ b/nixos/tests/nginx.nix @@ -1,18 +1,19 @@ # verifies: # 1. nginx generates config file with shared http context definitions above # generated virtual hosts config. +# 2. whether the ETag header is properly generated whenever we're serving +# files in Nix store paths -import ./make-test.nix ({ pkgs, ...} : { +import ./make-test.nix ({ pkgs, ... }: { name = "nginx"; meta = with pkgs.stdenv.lib.maintainers; { maintainers = [ mbbx6spp ]; }; - nodes = { - webserver = - { ... }: - { services.nginx.enable = true; - services.nginx.commonHttpConfig = '' + nodes = let + commonConfig = { pkgs, ... }: { + services.nginx.enable = true; + services.nginx.commonHttpConfig = '' log_format ceeformat '@cee: {"status":"$status",' '"request_time":$request_time,' '"upstream_response_time":$upstream_response_time,' @@ -24,20 +25,61 @@ import ./make-test.nix ({ pkgs, ...} : { '"request":"$request",' '"http_referer":"$http_referer",' '"upstream_addr":"$upstream_addr"}'; + ''; + services.nginx.virtualHosts."0.my.test" = { + extraConfig = '' + access_log syslog:server=unix:/dev/log,facility=user,tag=mytag,severity=info ceeformat; + location /favicon.ico { allow all; access_log off; log_not_found off; } ''; - services.nginx.virtualHosts."0.my.test" = { - extraConfig = '' - access_log syslog:server=unix:/dev/log,facility=user,tag=mytag,severity=info ceeformat; - location /favicon.ico { allow all; access_log off; log_not_found off; } - ''; - }; }; + services.nginx.virtualHosts.localhost = { + root = pkgs.runCommand "testdir" {} '' + mkdir "$out" + echo hello world > "$out/index.html" + ''; + }; + }; + in { + webserver = commonConfig; + + newwebserver = { pkgs, lib, ... }: { + imports = [ commonConfig ]; + services.nginx.virtualHosts.localhost = { + root = lib.mkForce (pkgs.runCommand "testdir2" {} '' + mkdir "$out" + echo hello world > "$out/index.html" + ''); + }; + }; }; - testScript = '' - startAll; + testScript = { nodes, ... }: let + newServerSystem = nodes.newwebserver.config.system.build.toplevel; + switch = "${newServerSystem}/bin/switch-to-configuration test"; + in '' + my $url = 'http://localhost/index.html'; + + sub checkEtag { + my $etag = $webserver->succeed( + 'curl -v '.$url.' 2>&1 | sed -n -e "s/^< [Ee][Tt][Aa][Gg]: *//p"' + ); + $etag =~ s/\r?\n$//; + my $httpCode = $webserver->succeed( + 'curl -w "%{http_code}" -X HEAD -H \'If-None-Match: '.$etag.'\' '.$url + ); + chomp $httpCode; + die "HTTP code is not 304" unless $httpCode == 304; + return $etag; + } $webserver->waitForUnit("nginx"); $webserver->waitForOpenPort("80"); + + subtest "check ETag if serving Nix store paths", sub { + my $oldEtag = checkEtag; + $webserver->succeed('${switch}'); + my $newEtag = checkEtag; + die "Old ETag $oldEtag is the same as $newEtag" if $oldEtag eq $newEtag; + }; ''; }) From 1f24685d93e96581b65d8c30a0c8eac903d82052 Mon Sep 17 00:00:00 2001 From: aszlig Date: Thu, 18 Apr 2019 10:07:55 +0200 Subject: [PATCH 6/6] nginx/etag-patch: Use Nix store dir from build env So far, the Nix store directory was hardcoded and if someone uses a different Nix store directory the patch won't work. Of course, this is pretty uncommon, but by not only substituting the store directory but also the length of it we also save a few calls to ngx_strlen(), which should save us a few cycles. Signed-off-by: aszlig --- pkgs/servers/http/nginx/generic.nix | 11 +++++++---- pkgs/servers/http/nginx/nix-etag-1.15.4.patch | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pkgs/servers/http/nginx/generic.nix b/pkgs/servers/http/nginx/generic.nix index fa7864886dc5..12b873df6a40 100644 --- a/pkgs/servers/http/nginx/generic.nix +++ b/pkgs/servers/http/nginx/generic.nix @@ -1,5 +1,5 @@ { stdenv, fetchurl, fetchpatch, openssl, zlib, pcre, libxml2, libxslt -, gd, geoip +, substituteAll, gd, geoip , withDebug ? false , withStream ? true , withMail ? false @@ -75,9 +75,12 @@ stdenv.mkDerivation { preConfigure = (concatMapStringsSep "\n" (mod: mod.preConfigure or "") modules); - patches = [ - ./nix-etag-1.15.4.patch - ] ++ stdenv.lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [ + patches = stdenv.lib.singleton (substituteAll { + src = ./nix-etag-1.15.4.patch; + preInstall = '' + export nixStoreDir="$NIX_STORE" nixStoreDirLen="''${#NIX_STORE}" + ''; + }) ++ stdenv.lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [ (fetchpatch { url = "https://raw.githubusercontent.com/openwrt/packages/master/net/nginx/patches/102-sizeof_test_fix.patch"; sha256 = "0i2k30ac8d7inj9l6bl0684kjglam2f68z8lf3xggcc2i5wzhh8a"; diff --git a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch index 1a8dcb4303ad..9dec715bf6c5 100644 --- a/pkgs/servers/http/nginx/nix-etag-1.15.4.patch +++ b/pkgs/servers/http/nginx/nix-etag-1.15.4.patch @@ -36,8 +36,8 @@ index c57ec00c..b7992de2 100644 - r->headers_out.last_modified_time, - r->headers_out.content_length_n) - - etag->value.data; -+ #define NIX_STORE_DIR "/nix/store" -+ #define NIX_STORE_LEN ngx_strlen(NIX_STORE_DIR) ++ #define NIX_STORE_DIR "@nixStoreDir@" ++ #define NIX_STORE_LEN @nixStoreDirLen@ + + if (r->headers_out.last_modified_time == 1 + && !ngx_strncmp(real, NIX_STORE_DIR, NIX_STORE_LEN)