diff --git a/roles/sys-svc-webserver-core/README.md b/roles/sys-svc-webserver-core/README.md index 9e8c9c54d8..d78181cb93 100644 --- a/roles/sys-svc-webserver-core/README.md +++ b/roles/sys-svc-webserver-core/README.md @@ -40,6 +40,7 @@ Solid `1:1` edges are fixed relationships; dashed `0..1` edges are conditional ( - **General logs**: `journalctl -f -u NGINX` - **Filter by host**: `journalctl -u NGINX -f | grep "{{ inventory_hostname }}"` - **Enable detailed format**: set `MODE_DEBUG: true` and reload NGINX. +- **Redacted request path**: both access log formats write the path without query string, Referer or basic-auth user. Path segments after a password-reset, e-mail confirmation, e-mail login or account activation route (e.g. `/password/reset/`, `/confirm-email/`) are written as `[redacted]`; extend the `$infinito_access_log_path` map in [nginx.conf.j2](./templates/nginx.conf.j2) for further token routes. ## Credits diff --git a/roles/sys-svc-webserver-core/templates/nginx.conf.j2 b/roles/sys-svc-webserver-core/templates/nginx.conf.j2 index 97b3e84dbf..d14b87b3f8 100644 --- a/roles/sys-svc-webserver-core/templates/nginx.conf.j2 +++ b/roles/sys-svc-webserver-core/templates/nginx.conf.j2 @@ -26,10 +26,13 @@ http server_names_hash_bucket_size 128; # size of each bucket for server_name lookups (in bytes) server_names_hash_max_size 512; # maximum total buckets for the server_name hash table {% include 'roles/sys-svc-proxy/templates/headers/buffers.conf.j2' %} + map $request_uri $infinito_access_log_path { + "~*^(?P[^?]*?/(?:password/reset|password-reset|reset-password|confirm-email|lostpassword/reset/form|email-login|activate-account)/)" "${infinito_access_log_token_route}[redacted]"; + "~^(?P[^?]*)" "${infinito_access_log_plain_path}"; + } {% if MODE_DEBUG | bool %} log_format debug '$host - $remote_addr [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"Referer: $http_referer" ' + '"$request_method $infinito_access_log_path $server_protocol" $status $body_bytes_sent ' '"User-Agent: $http_user_agent" ' '"ReqTime: $request_time" "UpstreamTime: $upstream_response_time" ' '"ReqLength: $request_length" "BytesSent: $bytes_sent" ' @@ -37,6 +40,11 @@ http '"X-Forwarded-For: $http_x_forwarded_for" ' '"Scheme: $scheme" "Protocol: $server_protocol" "ServerName: $server_name"'; access_log /dev/stdout debug; +{% else %} + log_format redacted '$remote_addr - - [$time_local] ' + '"$request_method $infinito_access_log_path $server_protocol" $status $body_bytes_sent ' + '"-" "$http_user_agent"'; + access_log /dev/stdout redacted; {% endif %} error_log /dev/stderr info; sendfile on; diff --git a/tests/unit/python/roles/sys-svc-webserver-core/__init__.py b/tests/unit/python/roles/sys-svc-webserver-core/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unit/python/roles/sys-svc-webserver-core/templates/__init__.py b/tests/unit/python/roles/sys-svc-webserver-core/templates/__init__.py new file mode 100644 index 0000000000..087c384b57 --- /dev/null +++ b/tests/unit/python/roles/sys-svc-webserver-core/templates/__init__.py @@ -0,0 +1,5 @@ +from pathlib import Path + +PROJECT_ROOT: Path = Path(__file__).resolve().parents[6] + +__all__ = ["PROJECT_ROOT"] diff --git a/tests/unit/python/roles/sys-svc-webserver-core/templates/test_nginx_access_log.py b/tests/unit/python/roles/sys-svc-webserver-core/templates/test_nginx_access_log.py new file mode 100644 index 0000000000..db0e3fbd5e --- /dev/null +++ b/tests/unit/python/roles/sys-svc-webserver-core/templates/test_nginx_access_log.py @@ -0,0 +1,194 @@ +"""The front proxy's access log keeps no tokens from URLs.""" + +from __future__ import annotations + +import re +import unittest + +from jinja2 import Environment, FileSystemLoader, StrictUndefined, select_autoescape + +from . import PROJECT_ROOT + +TEMPLATE = "roles/sys-svc-webserver-core/templates/nginx.conf.j2" + +LEAKING_VARIABLE = re.compile( + r"\$\{?(request|request_uri|args|query_string|is_args|arg_\w+|http_referer|remote_user)\b" +) + +REDACTED_PATH = "$infinito_access_log_path" + +MODES = {"debug": True, "production": False} + + +def render(debug: bool) -> str: + """Return the rendered nginx.conf. + + Args: + debug: the value of ``MODE_DEBUG``. + """ + env = Environment( + loader=FileSystemLoader(str(PROJECT_ROOT)), + undefined=StrictUndefined, + autoescape=select_autoescape(), + ) + env.filters["bool"] = bool + return env.get_template(TEMPLATE).render( + MODE_DEBUG=debug, + DEPLOYMENT_MODE="compose", + WEBSERVER_PROXY_WORKER_PROCESSES=1, + WEBSERVER_PROXY_WORKER_CONNECTIONS=256, + WEBSERVER_PROXY_WORKER_RLIMIT_NOFILE=512, + lookup=lambda _kind, key: ( + ["/etc/nginx/conf.d/http/"] + if key.endswith("http_includes") + else f"/var/lib/nginx/{key}" + ), + ) + + +def directives(rendered: str, name: str) -> list[str]: + """Return the arguments of every ``name`` directive, joined across lines. + + Args: + rendered: the rendered nginx.conf. + name: the directive, e.g. ``log_format``. + """ + return re.findall(rf"^\s*{name}\s+([^;]*);", rendered, re.MULTILINE) + + +def log_formats(rendered: str) -> dict[str, str]: + """Return every declared log format body by its name. + + Args: + rendered: the rendered nginx.conf. + """ + return { + body.split(maxsplit=1)[0]: body.split(maxsplit=1)[1] + for body in directives(rendered, "log_format") + } + + +def logged_path(rendered: str, request_uri: str) -> str: + """Return the path the access log writes, resolved as nginx's map does. + + Args: + rendered: the rendered nginx.conf. + request_uri: the request URI as the client sent it. + """ + block = re.search( + r"map \$request_uri \$infinito_access_log_path \{(.*?)\n\s*\}", + rendered, + re.DOTALL, + ) + if block is None: + raise AssertionError("nginx.conf declares no map for $infinito_access_log_path") + for source, value in re.findall( + r'^\s*"([^"]+)"\s+"([^"]*)";', block.group(1), re.MULTILINE + ): + caseless = source.startswith("~*") + match = re.search( + source[2:] if caseless else source[1:], + request_uri, + re.IGNORECASE if caseless else 0, + ) + if match: + return re.sub( + r"\$\{(\w+)\}|\$(\w+)", + lambda var, m=match: m.group(var.group(1) or var.group(2)), + value, + ) + return "" + + +class TestAccessLogFormat(unittest.TestCase): + def test_every_mode_writes_its_access_log_in_a_format_declared_here(self): + for mode, debug in MODES.items(): + with self.subTest(mode=mode): + rendered = render(debug) + logs = [args.split() for args in directives(rendered, "access_log")] + self.assertTrue(logs) + for log in logs: + self.assertEqual(len(log), 2, log) + self.assertIn(log[1], log_formats(rendered)) + + def test_no_format_logs_the_query_string_the_referer_or_the_raw_request(self): + for mode, debug in MODES.items(): + for name, body in log_formats(render(debug)).items(): + with self.subTest(mode=mode, format=name): + self.assertIsNone(LEAKING_VARIABLE.search(body), body) + + def test_every_format_logs_method_redacted_path_and_status(self): + for mode, debug in MODES.items(): + for name, body in log_formats(render(debug)).items(): + with self.subTest(mode=mode, format=name): + self.assertIn( + f'"$request_method {REDACTED_PATH} $server_protocol"', body + ) + self.assertIn("$status", body) + + def test_the_debug_format_keeps_host_timings_and_upstream(self): + body = log_formats(render(True))["debug"] + for variable in ("$host", "$request_time", "$upstream_response_time"): + with self.subTest(variable=variable): + self.assertIn(variable, body) + + +class TestLoggedPath(unittest.TestCase): + rendered: str + + @classmethod + def setUpClass(cls) -> None: + cls.rendered = render(False) + + def test_the_query_string_is_dropped(self): + for request_uri, expected in ( + ("/", "/"), + ("/index.php?access_token=s3cr3t", "/index.php"), + ("/api/v1/items?page=2&token=s3cr3t", "/api/v1/items"), + ( + "/realms/x/login-actions/action-token?key=s3cr3t", + "/realms/x/login-actions/action-token", + ), + ): + with self.subTest(request_uri=request_uri): + self.assertEqual(logged_path(self.rendered, request_uri), expected) + + def test_token_routes_are_redacted(self): + for request_uri, expected in ( + ("/password/reset/s3cr3t?email=a%40b.c", "/password/reset/[redacted]"), + ("/i/confirm-email/s3cr3t/s3cr3t2", "/i/confirm-email/[redacted]"), + ("/accounts/confirm-email/s3cr3t/", "/accounts/confirm-email/[redacted]"), + ( + "/index.php/lostpassword/reset/form/s3cr3t/alice", + "/index.php/lostpassword/reset/form/[redacted]", + ), + ("/u/password-reset/s3cr3t", "/u/password-reset/[redacted]"), + ("/reset-password/s3cr3t", "/reset-password/[redacted]"), + ("/session/email-login/s3cr3t", "/session/email-login/[redacted]"), + ("/u/activate-account/s3cr3t", "/u/activate-account/[redacted]"), + ("/Password/Reset/s3cr3t", "/Password/Reset/[redacted]"), + ): + with self.subTest(request_uri=request_uri): + logged = logged_path(self.rendered, request_uri) + self.assertEqual(logged, expected) + self.assertNotIn("s3cr3t", logged) + + def test_lookalike_paths_are_logged_unchanged(self): + for request_uri in ( + "/password/reset", + "/xpassword/reset/abc", + "/settings/password", + "/confirm-email", + ): + with self.subTest(request_uri=request_uri): + self.assertEqual(logged_path(self.rendered, request_uri), request_uri) + + def test_both_modes_share_the_same_map(self): + self.assertEqual( + logged_path(render(True), "/password/reset/s3cr3t?x=1"), + logged_path(self.rendered, "/password/reset/s3cr3t?x=1"), + ) + + +if __name__ == "__main__": + unittest.main()