mirror of
https://github.com/domainaware/parsedmarc.git
synced 2026-11-07 01:09:26 +00:00
Merge commit from fork
* Stop logging credentials from configured URLs and the IPinfo token (GHSA-hjr3-rx4c-24cv) With --debug, the Splunk HEC, Elasticsearch and OpenSearch client-init lines and opensearch.set_hosts() logged configured URLs verbatim, so userinfo passwords reached the console and log_file (CWE-532). Triage found more paths of the same class: - the httpx "HTTP Request:" line, forwarded at --verbose/--debug, contains the full URL (str(httpx.URL) keeps userinfo and query), so webhook and HEC URL credentials and the IPinfo ?token= were logged on every request; - the PSL overrides, IP database and reverse DNS map downloaders logged their URL at DEBUG and str(HTTPStatusError), which embeds the full URL, at WARNING; - libpq echoes the DSN or a password fragment for some malformed PostgreSQL connection strings, logged as an output-client error. Add redact_url_credentials() (string methods only; urlsplit misparses scheme-less "user:pw@host:port") and use it at each log site; add a filter on the httpx logger that masks userinfo and the token query value; build download-failure warnings from the status code and the redacted request URL instead of the exception text; replace psycopg ProgrammingError detail from a connection_string with a fixed message. Docs now recommend separate user/password settings over URL credentials. Reported by helixnebulatech. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Mask credentials in the Loki URL in the log and LokiError text The Loki output (#926) landed on master after this fix was written. Its url keeps userinfo (copy_with(path=...) preserves it), and the --debug "Initializing Loki client" line and both LokiError messages, which the CLI logs on a failed push, included it as-is: the same CWE-532 path as the Splunk HEC line. Mask it with redact_url_credentials() at all three sites. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Withhold InvalidURL details, mask malformed URLs, and cover httpx2 and DoH nameservers Follow-up for GHSA-hjr3-rx4c-24cv (CWE-532): - httpx.InvalidURL quotes the component it rejected, so a password with an unencoded "/", "?" or "#" (RFC 3986 3.2: the authority ends there) leaked as "Invalid port: 'p4ssw0rd'", and its chained ValueError repeats it. Loki, Splunk HEC, the webhook and the three download loaders now report a fixed "details withheld" message raised from None. InvalidURL is not an httpx.HTTPError, so a malformed psl_overrides_url crashed startup; the loaders now treat it as a download failure. - redact_url_credentials() masks through the last "@" when the authority has no "@" but one follows it, so the --debug lines no longer print such passwords; the userinfo lookup is shared with the new nameserver masking. - dnspython 2.9 DoH logs its request line on the httpx2 logger, which was forwarded but not filtered; the filter now covers httpx2 and httpx2.URL. - [general] nameservers / --nameservers credentials are masked in the DNS pre-flight error, reverse DNS debug errors and per-report errors, and unusable https:// / tls:// ports are rejected before any query. - Docs recommend token/username settings for [loki] and [splunk_hec]; AGENTS.md gains rules for keeping credentials out of logs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Sean Whalen <seanthegeek@users.noreply.github.com>
This commit is contained in:
17 files changed
+1595
-73
No files matched your search
@@ -6,6 +6,7 @@ import importlib.abc
|
||||
import importlib.machinery
|
||||
import gzip
|
||||
import io
|
||||
import importlib.util
|
||||
import json
|
||||
import logging
|
||||
import mailbox
|
||||
@@ -15,6 +16,7 @@ import signal
|
||||
import stat
|
||||
import sys
|
||||
import tempfile
|
||||
import traceback
|
||||
import unittest
|
||||
import zipfile
|
||||
from collections.abc import Iterator, Sequence
|
||||
@@ -5452,6 +5454,323 @@ def _es_request_without_network(client, method, path, **kwargs):
|
||||
return {"count": 0}
|
||||
|
||||
|
||||
def _os_request_without_network(transport, method, url, *args, **kwargs):
|
||||
"""Stand-in for ``opensearchpy.Transport.perform_request``: answers
|
||||
"no such index" / zero documents like ``_es_request_without_network``."""
|
||||
if method == "HEAD":
|
||||
return {}
|
||||
return {"count": 0}
|
||||
|
||||
|
||||
class TestInitOutputClientsDoesNotLogCredentials(unittest.TestCase):
|
||||
"""GHSA-hjr3-rx4c-24cv: with --debug, the "Initializing ... client"
|
||||
lines must mask credentials embedded in configured URLs."""
|
||||
|
||||
SECRET = "s3cretPW"
|
||||
|
||||
def _init(self, opts):
|
||||
with self.assertLogs("parsedmarc.log", level="DEBUG") as logs:
|
||||
clients = parsedmarc.cli._init_output_clients(opts)
|
||||
self.addCleanup(parsedmarc.cli._close_output_clients, clients)
|
||||
return "\n".join(logs.output)
|
||||
|
||||
def test_hec_url_password_is_masked(self):
|
||||
opts = _output_client_opts(
|
||||
hec=f"https://hecuser:{self.SECRET}@hec.example:8088",
|
||||
hec_token="tok",
|
||||
hec_index="email",
|
||||
)
|
||||
output = self._init(opts)
|
||||
self.assertNotIn(self.SECRET, output)
|
||||
self.assertIn("url=https://hecuser:***@hec.example:8088", output)
|
||||
|
||||
def test_hec_malformed_url_password_is_masked(self):
|
||||
"""A "/", "?" or "#" in the password ends the authority before the
|
||||
"@", which left the --debug line unmasked; it is now masked through
|
||||
the last "@"."""
|
||||
for password in ("p4ssw0rd/extra", "p4ss?w0rd", "p4ss#w0rd"):
|
||||
with self.subTest(password=password):
|
||||
opts = _output_client_opts(
|
||||
hec=f"https://hecuser:{password}@hec.example:8088",
|
||||
hec_token="tok",
|
||||
hec_index="email",
|
||||
)
|
||||
output = self._init(opts)
|
||||
self.assertNotIn("p4ss", output)
|
||||
self.assertIn("url=https://***@hec.example:8088", output)
|
||||
|
||||
def test_loki_url_password_is_masked(self):
|
||||
opts = _output_client_opts(
|
||||
loki_url=f"https://lokiuser:{self.SECRET}@loki.example:3100",
|
||||
loki_timestamp="ingest",
|
||||
loki_timeout=1.0,
|
||||
loki_batch_size=100,
|
||||
)
|
||||
output = self._init(opts)
|
||||
self.assertNotIn(self.SECRET, output)
|
||||
self.assertIn("url=https://lokiuser:***@loki.example:3100", output)
|
||||
|
||||
def test_loki_malformed_url_error_has_no_password(self):
|
||||
"""A password with an unencoded "/" ends the URL's authority early
|
||||
(RFC 3986 section 3.2), so httpx raises ``InvalidURL("Invalid port:
|
||||
'p4ssw0rd'")`` (httpx 0.28.1 _urlparse.py), whose text and chained
|
||||
ValueError both repeat it. Neither the startup error nor the
|
||||
``--debug`` "Initializing Loki client" line may show any part of
|
||||
the password."""
|
||||
malformed = (
|
||||
("https://admin:p4ssw0rd/extra@loki.example:3100", "p4ssw0rd"),
|
||||
("https://admin:p4ss?w0rd@loki.example:3100", "p4ss"),
|
||||
("https://admin:p4ss#w0rd@loki.example:3100", "p4ss"),
|
||||
)
|
||||
for url, secret in malformed:
|
||||
with self.subTest(url=url):
|
||||
opts = _output_client_opts(
|
||||
loki_url=url,
|
||||
loki_timestamp="ingest",
|
||||
loki_timeout=1.0,
|
||||
loki_batch_size=100,
|
||||
)
|
||||
with (
|
||||
self.assertRaises(RuntimeError) as ctx,
|
||||
self.assertLogs("parsedmarc.log", level="DEBUG") as logs,
|
||||
):
|
||||
parsedmarc.cli._init_output_clients(opts)
|
||||
self.assertEqual(
|
||||
str(ctx.exception),
|
||||
"Loki: Invalid Loki URL (details withheld because they may "
|
||||
"include credentials)",
|
||||
)
|
||||
trace = "".join(traceback.format_exception(ctx.exception))
|
||||
self.assertNotIn(secret, trace)
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn(secret, output)
|
||||
self.assertIn("url=https://***@loki.example:3100", output)
|
||||
|
||||
def test_elasticsearch_hosts_password_is_masked(self):
|
||||
connections = parsedmarc.elastic.connections
|
||||
_isolate_default_alias(self, connections)
|
||||
opts = _output_client_opts(
|
||||
save_aggregate=True,
|
||||
elasticsearch_hosts=[f"https://u:{self.SECRET}@127.0.0.1:9299"],
|
||||
elasticsearch_timeout=1.0,
|
||||
)
|
||||
with (
|
||||
patch.object(Elasticsearch, "perform_request", _es_request_without_network),
|
||||
patch(
|
||||
"parsedmarc.elastic.connections.create_connection",
|
||||
wraps=connections.create_connection,
|
||||
) as mock_conn,
|
||||
):
|
||||
output = self._init(opts)
|
||||
self.assertNotIn(self.SECRET, output)
|
||||
self.assertIn("Initializing Elasticsearch client", output)
|
||||
# Redaction is log-only: the client still gets the real credentials
|
||||
self.assertEqual(
|
||||
mock_conn.call_args.kwargs["hosts"],
|
||||
[f"https://u:{self.SECRET}@127.0.0.1:9299"],
|
||||
)
|
||||
self.assertIn("https://u:***@127.0.0.1:9299", output)
|
||||
|
||||
def test_opensearch_hosts_password_is_masked_in_cli_and_set_hosts(self):
|
||||
connections = opensearch_module.connections
|
||||
_isolate_default_alias(self, connections)
|
||||
opts = _output_client_opts(
|
||||
save_aggregate=True,
|
||||
opensearch_hosts=[f"https://u:{self.SECRET}@127.0.0.1:9299"],
|
||||
opensearch_timeout=1.0,
|
||||
)
|
||||
with patch.object(
|
||||
opensearchpy.Transport, "perform_request", _os_request_without_network
|
||||
):
|
||||
output = self._init(opts)
|
||||
self.assertNotIn(self.SECRET, output)
|
||||
lines = output.splitlines()
|
||||
for marker in ("Initializing OpenSearch client", "Connecting to OpenSearch"):
|
||||
with self.subTest(marker=marker):
|
||||
matching = [line for line in lines if marker in line]
|
||||
self.assertEqual(len(matching), 1)
|
||||
self.assertIn("https://u:***@127.0.0.1:9299", matching[0])
|
||||
|
||||
|
||||
class TestHttpxLogRedaction(unittest.TestCase):
|
||||
"""GHSA-hjr3-rx4c-24cv: httpx logs the full request URL at INFO."""
|
||||
|
||||
def setUp(self):
|
||||
saved = {}
|
||||
for name in parsedmarc.cli._DEPENDENCY_LOGGERS:
|
||||
dep = logging.getLogger(name)
|
||||
saved[name] = (
|
||||
dep.level,
|
||||
list(dep.handlers),
|
||||
dep.propagate,
|
||||
list(dep.filters),
|
||||
)
|
||||
|
||||
def restore():
|
||||
for name, (level, handlers, propagate, filters) in saved.items():
|
||||
dep = logging.getLogger(name)
|
||||
dep.setLevel(level)
|
||||
dep.handlers = handlers
|
||||
dep.propagate = propagate
|
||||
dep.filters = filters
|
||||
|
||||
self.addCleanup(restore)
|
||||
parsedmarc.cli._configure_dependency_logging(logging.INFO)
|
||||
|
||||
def _client(self, seen):
|
||||
def handler(request):
|
||||
seen.append(request)
|
||||
return httpx.Response(200)
|
||||
|
||||
return httpx.Client(transport=httpx.MockTransport(handler))
|
||||
|
||||
def test_userinfo_password_masked_but_auth_still_sent(self):
|
||||
seen = []
|
||||
with self._client(seen) as client:
|
||||
with self.assertLogs("httpx", level="INFO") as logs:
|
||||
client.post("https://user:secret@hook.example/x", content=b"{}")
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn("secret", output)
|
||||
self.assertIn("https://user:***@hook.example/x", output)
|
||||
self.assertTrue(seen[0].headers["Authorization"].startswith("Basic "))
|
||||
|
||||
def test_ipinfo_token_query_parameter_masked(self):
|
||||
seen = []
|
||||
with self._client(seen) as client:
|
||||
with self.assertLogs("httpx", level="INFO") as logs:
|
||||
client.get(
|
||||
"https://api.ipinfo.io/lite/8.8.8.8", params={"token": "TOKSECRET"}
|
||||
)
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn("TOKSECRET", output)
|
||||
self.assertIn("api.ipinfo.io/lite/8.8.8.8", output)
|
||||
self.assertIn("token=***", output)
|
||||
self.assertEqual(seen[0].url.params["token"], "TOKSECRET")
|
||||
|
||||
def test_token_mask_leaves_literal_encoded_stars_in_path_alone(self):
|
||||
"""A path containing a literal ``%2A%2A%2A`` is not rewritten; only
|
||||
the token value is masked, with and without other query params and
|
||||
a fragment."""
|
||||
for url, expected in (
|
||||
(
|
||||
"https://h.example/a%2A%2A%2Ab?token=TOKSECRET",
|
||||
"https://h.example/a%2A%2A%2Ab?token=***",
|
||||
),
|
||||
(
|
||||
"https://h.example/a%2A%2A%2Ab?x=1&token=TOKSECRET",
|
||||
"https://h.example/a%2A%2A%2Ab?x=1&token=***",
|
||||
),
|
||||
(
|
||||
"https://h.example/a%2A%2A%2Ab?x=1&token=TOKSECRET#f",
|
||||
"https://h.example/a%2A%2A%2Ab?x=1&token=***#f",
|
||||
),
|
||||
):
|
||||
with self.subTest(url=url):
|
||||
result = parsedmarc.cli._RedactUrlCredentialsFilter._redact(
|
||||
httpx.URL(url)
|
||||
)
|
||||
self.assertNotIn("TOKSECRET", result)
|
||||
self.assertEqual(result, expected)
|
||||
|
||||
def test_reconfiguring_does_not_stack_filters(self):
|
||||
parsedmarc.cli._configure_dependency_logging(logging.INFO)
|
||||
parsedmarc.cli._configure_dependency_logging(logging.DEBUG)
|
||||
installed = [
|
||||
f
|
||||
for f in logging.getLogger("httpx").filters
|
||||
if isinstance(f, parsedmarc.cli._RedactUrlCredentialsFilter)
|
||||
]
|
||||
self.assertEqual(len(installed), 1)
|
||||
|
||||
def test_non_tuple_args_and_other_arg_types_pass_through(self):
|
||||
record = logging.LogRecord("httpx", logging.INFO, "f", 1, "x %d", (5,), None)
|
||||
self.assertTrue(parsedmarc.cli._RedactUrlCredentialsFilter().filter(record))
|
||||
self.assertEqual(record.args, (5,))
|
||||
record = logging.LogRecord("httpx", logging.INFO, "f", 1, "plain", None, None)
|
||||
self.assertTrue(parsedmarc.cli._RedactUrlCredentialsFilter().filter(record))
|
||||
self.assertIsNone(record.args)
|
||||
|
||||
|
||||
@unittest.skipUnless(
|
||||
importlib.util.find_spec("httpx2") is not None,
|
||||
"httpx2 (dnspython 2.9+ DNS over HTTPS) is not installed",
|
||||
)
|
||||
class TestHttpx2LogRedaction(unittest.TestCase):
|
||||
"""GHSA-hjr3-rx4c-24cv: dnspython 2.9+ does DNS over HTTPS through
|
||||
httpx2, which logs the request URL under the ``httpx2`` logger with an
|
||||
``httpx2.URL``, a different class from ``httpx.URL``."""
|
||||
|
||||
def setUp(self):
|
||||
import httpx2 # pyright: ignore[reportMissingImports]
|
||||
|
||||
self.httpx2 = httpx2
|
||||
saved = {}
|
||||
for name in parsedmarc.cli._DEPENDENCY_LOGGERS:
|
||||
dep = logging.getLogger(name)
|
||||
saved[name] = (
|
||||
dep.level,
|
||||
list(dep.handlers),
|
||||
dep.propagate,
|
||||
list(dep.filters),
|
||||
)
|
||||
|
||||
def restore():
|
||||
for name, (level, handlers, propagate, filters) in saved.items():
|
||||
dep = logging.getLogger(name)
|
||||
dep.setLevel(level)
|
||||
dep.handlers = handlers
|
||||
dep.propagate = propagate
|
||||
dep.filters = filters
|
||||
|
||||
self.addCleanup(restore)
|
||||
parsedmarc.cli._configure_dependency_logging(logging.INFO)
|
||||
|
||||
def test_httpx2_client_request_line_masks_userinfo(self):
|
||||
"""httpx2 itself emits the line; the credential still reaches the
|
||||
transport."""
|
||||
seen = []
|
||||
|
||||
def handler(request):
|
||||
seen.append(request)
|
||||
return self.httpx2.Response(200)
|
||||
|
||||
with self.httpx2.Client(transport=self.httpx2.MockTransport(handler)) as client:
|
||||
with self.assertLogs("httpx2", level="INFO") as logs:
|
||||
client.post("https://doh:S3CRET@doh.example/dns-query", content=b"{}")
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn("S3CRET", output)
|
||||
self.assertIn("https://doh:***@doh.example/dns-query", output)
|
||||
self.assertTrue(seen[0].headers["Authorization"].startswith("Basic "))
|
||||
|
||||
def test_httpx2_url_token_query_parameter_masked(self):
|
||||
url = self.httpx2.URL("https://h.example/a?x=1&token=TOKSECRET#f")
|
||||
result = parsedmarc.cli._RedactUrlCredentialsFilter._redact(url)
|
||||
self.assertEqual(result, "https://h.example/a?x=1&token=***#f")
|
||||
|
||||
def test_httpx2_logger_record_args_masked_and_ints_untouched(self):
|
||||
record = logging.LogRecord(
|
||||
"httpx2",
|
||||
logging.INFO,
|
||||
"f",
|
||||
1,
|
||||
"%s %d",
|
||||
(self.httpx2.URL("https://u:S3CRET@h.example/x"), 200),
|
||||
None,
|
||||
)
|
||||
self.assertTrue(parsedmarc.cli._RedactUrlCredentialsFilter().filter(record))
|
||||
self.assertEqual(record.args, ("https://u:***@h.example/x", 200))
|
||||
|
||||
def test_reconfiguring_attaches_one_filter_to_httpx2(self):
|
||||
parsedmarc.cli._configure_dependency_logging(logging.DEBUG)
|
||||
parsedmarc.cli._configure_dependency_logging(logging.INFO)
|
||||
installed = [
|
||||
f
|
||||
for f in logging.getLogger("httpx2").filters
|
||||
if isinstance(f, parsedmarc.cli._RedactUrlCredentialsFilter)
|
||||
]
|
||||
self.assertEqual(len(installed), 1)
|
||||
|
||||
|
||||
class TestInitOutputClientsRollback(unittest.TestCase):
|
||||
"""_init_output_clients() is all-or-nothing against the search backends'
|
||||
process-wide ``default`` connection alias.
|
||||
@@ -7636,6 +7955,100 @@ class TestParseConfigGeneral(unittest.TestCase):
|
||||
str(ctx.exception),
|
||||
)
|
||||
|
||||
def test_general_nameservers_pre_flight_errors_mask_url_credentials(self):
|
||||
"""GHSA-hjr3-rx4c-24cv: the pre-flight error is logged at the default
|
||||
level, so neither the "no PTR record ... from <nameservers>" list nor
|
||||
a transport error that quotes the DoH request URL may carry the
|
||||
nameserver URL's password, and the chained exception (printed on a
|
||||
crash) must not repeat it."""
|
||||
from parsedmarc.cli import ConfigurationError, _parse_config
|
||||
|
||||
url = "https://doh:S3CRETPW@dns.example/dns-query"
|
||||
|
||||
def nxdomain(request, *args, **kwargs):
|
||||
response = dns.message.make_response(request)
|
||||
response.set_rcode(dns.rcode.NXDOMAIN)
|
||||
return response
|
||||
|
||||
def transport_error(request, *args, **kwargs):
|
||||
raise RuntimeError(f"Server error '502' for url '{url}'")
|
||||
|
||||
for name, responder, expected in (
|
||||
("NXDOMAIN", nxdomain, "https://doh:***@dns.example/dns-query"),
|
||||
("transport", transport_error, "https://doh:***@dns.example/dns-query"),
|
||||
):
|
||||
with self.subTest(name):
|
||||
cp = _config_with(
|
||||
"general",
|
||||
{
|
||||
"dns_test_address": "1.1.1.1",
|
||||
"dns_timeout": "2.0",
|
||||
"nameservers": url,
|
||||
},
|
||||
)
|
||||
with patch("dns.query.https", side_effect=responder):
|
||||
with self.assertRaises(ConfigurationError) as ctx:
|
||||
_parse_config(cp, _opts())
|
||||
trace = "".join(traceback.format_exception(ctx.exception))
|
||||
self.assertNotIn("S3CRETPW", str(ctx.exception))
|
||||
self.assertNotIn("S3CRETPW", trace)
|
||||
self.assertIn(expected, str(ctx.exception))
|
||||
|
||||
def test_general_nameservers_pre_flight_masks_malformed_url_passwords(self):
|
||||
"""GHSA-hjr3-rx4c-24cv: a password containing an unencoded ``/``,
|
||||
``?`` or ``#`` ends the URL authority early (RFC 3986 section 3.2),
|
||||
so ``urlsplit()`` reports no password and reads its start as the
|
||||
port. dnspython 2.9.1 then raised ``ValueError("Port could not be
|
||||
cast to integer value as 'p4ss'")`` from
|
||||
``DoHNameserver.answer_port()`` (``dns/nameserver.py``), and
|
||||
``_parse_dot_nameserver()`` quoted the same urlsplit message for a
|
||||
``tls://`` entry, so the pre-flight error printed the start of the
|
||||
password. Neither the password nor that prefix may reach the
|
||||
message or the printed traceback. No query is sent: every entry is
|
||||
rejected before dnspython contacts a server, which the patched
|
||||
transports would turn into a different error."""
|
||||
from parsedmarc.cli import ConfigurationError, _parse_config
|
||||
|
||||
for url, expected in (
|
||||
(
|
||||
"https://doh:p4ss/w0rd@dns.example/dns-query",
|
||||
"https://***@dns.example/dns-query",
|
||||
),
|
||||
(
|
||||
"https://doh:p4ss?w0rd@dns.example/dns-query",
|
||||
"https://***@dns.example/dns-query",
|
||||
),
|
||||
(
|
||||
"https://doh:p4ss#w0rd@dns.example/dns-query",
|
||||
"https://***@dns.example/dns-query",
|
||||
),
|
||||
("tls://dot:p4ss/w0rd@1.1.1.1", "tls://***@1.1.1.1"),
|
||||
):
|
||||
with self.subTest(url=url):
|
||||
cp = _config_with(
|
||||
"general",
|
||||
{
|
||||
"dns_test_address": "1.1.1.1",
|
||||
"dns_timeout": "0.2",
|
||||
"nameservers": url,
|
||||
},
|
||||
)
|
||||
unreachable = OSError("no query may be sent")
|
||||
with (
|
||||
patch("dns.query.https", side_effect=unreachable),
|
||||
patch("dns.query.tls", side_effect=unreachable),
|
||||
):
|
||||
with self.assertRaises(ConfigurationError) as ctx:
|
||||
_parse_config(cp, _opts())
|
||||
message = str(ctx.exception)
|
||||
trace = "".join(traceback.format_exception(ctx.exception))
|
||||
for secret in ("p4ss", "w0rd"):
|
||||
self.assertNotIn(secret, message)
|
||||
self.assertNotIn(secret, trace)
|
||||
self.assertIn(expected, message)
|
||||
self.assertIn("DNS pre-flight check failed: ValueError: ", message)
|
||||
self.assertNotIn("no query may be sent", message)
|
||||
|
||||
def test_general_nameservers_pre_flight_rejects_invalid_test_address(self):
|
||||
"""An unparseable dns_test_address is reported by setting name and
|
||||
value (dns.reversename.from_address raises dns.exception.SyntaxError,
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
import json
|
||||
import os
|
||||
import time
|
||||
import traceback
|
||||
import unittest
|
||||
from typing import cast
|
||||
from unittest.mock import MagicMock, patch
|
||||
@@ -164,6 +165,44 @@ class TestLokiClientInit(unittest.TestCase):
|
||||
self.assertIsInstance(client.session.auth, httpx.BasicAuth)
|
||||
|
||||
|
||||
class TestLokiInvalidUrlDoesNotLeakPassword(unittest.TestCase):
|
||||
"""GHSA-hjr3-rx4c-24cv: a password with an unencoded "/", "?" or "#"
|
||||
ends the URL's authority early (RFC 3986 section 3.2), so httpx reads the
|
||||
text after the colon as the port and raises
|
||||
``InvalidURL("Invalid port: 'p4ssw0rd'")`` (httpx 0.28.1 _urlparse.py:
|
||||
``raise InvalidURL(f"Invalid port: {port!r}")``). Its implicit
|
||||
``__context__`` is a ValueError that repeats that part of the password
|
||||
too."""
|
||||
|
||||
MALFORMED = (
|
||||
("https://admin:p4ssw0rd/extra@loki.example.org:3100", "p4ssw0rd"),
|
||||
("https://admin:p4ss?w0rd@loki.example.org:3100", "p4ss"),
|
||||
("https://admin:p4ss#w0rd@loki.example.org:3100", "p4ss"),
|
||||
)
|
||||
|
||||
def test_invalid_url_raises_fixed_message_without_password(self):
|
||||
for url, secret in self.MALFORMED:
|
||||
with self.subTest(url=url):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
LokiClient(url)
|
||||
self.assertEqual(
|
||||
str(ctx.exception),
|
||||
"Invalid Loki URL (details withheld because they may "
|
||||
"include credentials)",
|
||||
)
|
||||
self.assertIsNone(ctx.exception.__cause__)
|
||||
self.assertTrue(ctx.exception.__suppress_context__)
|
||||
trace = "".join(traceback.format_exception(ctx.exception))
|
||||
self.assertNotIn(secret, trace)
|
||||
|
||||
def test_valid_url_keeps_userinfo_for_requests(self):
|
||||
client = LokiClient("https://admin:p4ssw0rd@loki.example.org:3100")
|
||||
self.assertEqual(
|
||||
client.url,
|
||||
"https://admin:p4ssw0rd@loki.example.org:3100/loki/api/v1/push",
|
||||
)
|
||||
|
||||
|
||||
@patch(
|
||||
"parsedmarc.loki.parsed_aggregate_reports_to_csv_rows", return_value=AGGREGATE_ROWS
|
||||
)
|
||||
@@ -370,6 +409,28 @@ class TestLokiErrors(unittest.TestCase):
|
||||
client.save_aggregate_report_to_loki(AGGREGATE)
|
||||
self.assertIn("refused", str(ctx.exception))
|
||||
|
||||
def test_errors_mask_url_password(self, _rows):
|
||||
"""GHSA-hjr3-rx4c-24cv: the URL keeps its userinfo, and the CLI
|
||||
logs LokiError text, so the password must be masked in it."""
|
||||
secret = "s3cretPW"
|
||||
client = LokiClient(f"http://u:{secret}@loki.example.org:3100")
|
||||
client.session = MagicMock()
|
||||
_session(client).post.return_value = _ok_response(500)
|
||||
with self.assertRaises(LokiError) as status_ctx:
|
||||
client.save_aggregate_report_to_loki(AGGREGATE)
|
||||
_session(client).post.side_effect = httpx.ConnectError("refused")
|
||||
with self.assertRaises(LokiError) as network_ctx:
|
||||
client.save_aggregate_report_to_loki(AGGREGATE)
|
||||
for ctx in (status_ctx, network_ctx):
|
||||
with self.subTest(error=str(ctx.exception)):
|
||||
self.assertNotIn(secret, str(ctx.exception))
|
||||
self.assertIn("http://u:***@loki.example.org:3100", str(ctx.exception))
|
||||
# Redaction is message-only: the request still goes to the real URL
|
||||
self.assertEqual(
|
||||
_session(client).post.call_args.args[0],
|
||||
f"http://u:{secret}@loki.example.org:3100/loki/api/v1/push",
|
||||
)
|
||||
|
||||
def test_blocked_ingestion_status_raises(self, _rows):
|
||||
"""Loki returns 260 (block_ingestion_status_code default) and drops
|
||||
the logs when ingestion is blocked; that must not read as success."""
|
||||
|
||||
@@ -241,6 +241,30 @@ class TestSetHosts(unittest.TestCase):
|
||||
kwargs = mock_conn.call_args.kwargs
|
||||
self.assertEqual(kwargs["hosts"], ["es1:9200", "es2:9200"])
|
||||
|
||||
def test_debug_log_masks_credentials_but_connects_with_them(self):
|
||||
"""GHSA-hjr3-rx4c-24cv: the "Connecting to OpenSearch" debug line
|
||||
must not carry the URL password, while the client still gets the
|
||||
real URL."""
|
||||
url = "https://admin:s3cretPW@es.example:9200"
|
||||
with patch("parsedmarc.opensearch.connections.create_connection") as mock_conn:
|
||||
with self.assertLogs("parsedmarc.log", level="DEBUG") as logs:
|
||||
set_hosts(url)
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn("s3cretPW", output)
|
||||
self.assertIn("https://admin:***@es.example:9200", output)
|
||||
self.assertEqual(mock_conn.call_args.kwargs["hosts"], [url])
|
||||
|
||||
def test_debug_log_masks_schemeless_host_list(self):
|
||||
hosts = ["admin:s3cretPW@es1:9200", "es2:9200"]
|
||||
with patch("parsedmarc.opensearch.connections.create_connection") as mock_conn:
|
||||
with self.assertLogs("parsedmarc.log", level="DEBUG") as logs:
|
||||
set_hosts(hosts)
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn("s3cretPW", output)
|
||||
self.assertIn("admin:***@es1:9200", output)
|
||||
self.assertIn("es2:9200", output)
|
||||
self.assertEqual(mock_conn.call_args.kwargs["hosts"], hosts)
|
||||
|
||||
def test_timeout_default_60s(self):
|
||||
with patch("parsedmarc.opensearch.connections.create_connection") as mock_conn:
|
||||
set_hosts("es:9200")
|
||||
|
||||
@@ -311,11 +311,74 @@ class TestPostgreSQLConstruction(unittest.TestCase):
|
||||
"""A driver-level connection error is wrapped in PostgreSQLError."""
|
||||
with patch("parsedmarc.postgres.psycopg") as mock_psycopg:
|
||||
mock_psycopg.Error = Exception
|
||||
mock_psycopg.ProgrammingError = type("ProgrammingError", (Exception,), {})
|
||||
mock_psycopg.connect.side_effect = mock_psycopg.Error("refused")
|
||||
with self.assertRaises(PostgreSQLError) as ctx:
|
||||
PostgreSQLClient(host="localhost")
|
||||
self.assertIn("refused", str(ctx.exception))
|
||||
|
||||
def test_connect_failure_detail_is_url_redacted(self):
|
||||
"""A non-parse driver error keeps its detail, with URL credentials
|
||||
masked."""
|
||||
programming_error = type("ProgrammingError", (Exception,), {})
|
||||
with patch("parsedmarc.postgres.psycopg") as mock_psycopg:
|
||||
mock_psycopg.Error = Exception
|
||||
mock_psycopg.ProgrammingError = programming_error
|
||||
mock_psycopg.connect.side_effect = mock_psycopg.Error(
|
||||
'connection to "postgresql://u:PGSECRET@db.example/x" failed'
|
||||
)
|
||||
with self.assertRaises(PostgreSQLError) as ctx:
|
||||
PostgreSQLClient(connection_string="postgresql://u:PGSECRET@h/db")
|
||||
message = str(ctx.exception)
|
||||
self.assertNotIn("PGSECRET", message)
|
||||
self.assertIn("u:***@db.example/x", message)
|
||||
|
||||
def test_malformed_connection_string_detail_withheld(self):
|
||||
"""A ProgrammingError from parsing the connection string (which libpq
|
||||
words with the whole string or a password fragment) is replaced with
|
||||
a fixed message that still identifies the problem."""
|
||||
programming_error = type("ProgrammingError", (Exception,), {})
|
||||
for dsn, driver_message in (
|
||||
(
|
||||
"postgresql://u:PGSECRET@[::1/db",
|
||||
'end of string reached when looking for matching "]" in IPv6 '
|
||||
'host address in URI: "postgresql://u:PGSECRET@[::1/db"',
|
||||
),
|
||||
(
|
||||
"postgresql://u:PG%ZZSECRET@h/db",
|
||||
'invalid percent-encoded token: "PG%ZZSECRET"',
|
||||
),
|
||||
):
|
||||
with self.subTest(dsn=dsn):
|
||||
with patch("parsedmarc.postgres.psycopg") as mock_psycopg:
|
||||
mock_psycopg.Error = Exception
|
||||
mock_psycopg.ProgrammingError = programming_error
|
||||
mock_psycopg.connect.side_effect = programming_error(driver_message)
|
||||
with self.assertRaises(PostgreSQLError) as ctx:
|
||||
PostgreSQLClient(connection_string=dsn)
|
||||
message = str(ctx.exception)
|
||||
self.assertNotIn("PGSECRET", message)
|
||||
self.assertNotIn("SECRET", message)
|
||||
self.assertIn("Invalid PostgreSQL connection_string", message)
|
||||
|
||||
@unittest.skipIf(
|
||||
importlib.util.find_spec("psycopg") is None, "psycopg is not installed"
|
||||
)
|
||||
def test_real_psycopg_malformed_connection_string_withheld(self):
|
||||
"""With the real driver, both malformed strings fail in parsing (no
|
||||
network I/O) and the raised message carries no part of the password."""
|
||||
for dsn in (
|
||||
"postgresql://u:PGSECRET@[::1/db",
|
||||
"postgresql://u:PG%ZZSECRET@h/db",
|
||||
):
|
||||
with self.subTest(dsn=dsn):
|
||||
with self.assertRaises(PostgreSQLError) as ctx:
|
||||
PostgreSQLClient(connection_string=dsn)
|
||||
self.assertNotIn("SECRET", str(ctx.exception))
|
||||
self.assertIn(
|
||||
"Invalid PostgreSQL connection_string", str(ctx.exception)
|
||||
)
|
||||
|
||||
def test_create_tables_executes_all_ddl(self):
|
||||
"""create_tables issues CREATE TABLE for every table and the indexes."""
|
||||
client, mock_conn = _make_client()
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
import json
|
||||
import time
|
||||
import traceback
|
||||
import unittest
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
@@ -469,6 +470,58 @@ class TestSaveSmtpTlsReportsToSplunk(unittest.TestCase):
|
||||
)
|
||||
|
||||
|
||||
class TestInvalidUrlDoesNotLeakPassword(unittest.TestCase):
|
||||
"""GHSA-hjr3-rx4c-24cv: a password with an unencoded "/", "?" or "#"
|
||||
ends the URL's authority early (RFC 3986 section 3.2), so httpx reads the
|
||||
text after the colon as the port and raises
|
||||
``InvalidURL("Invalid port: 'p4ssw0rd'")`` when posting (httpx 0.28.1
|
||||
_urlparse.py: ``raise InvalidURL(f"Invalid port: {port!r}")``), before
|
||||
any network I/O. The error reaches the log on every batch."""
|
||||
|
||||
MALFORMED = (
|
||||
("https://admin:p4ssw0rd/extra@splunk.example.com:8088", "p4ssw0rd"),
|
||||
("https://admin:p4ss?w0rd@splunk.example.com:8088", "p4ss"),
|
||||
("https://admin:p4ss#w0rd@splunk.example.com:8088", "p4ss"),
|
||||
)
|
||||
|
||||
def test_invalid_url_raises_fixed_message_without_password(self):
|
||||
saves = (
|
||||
("aggregate", "save_aggregate_reports_to_splunk", _aggregate_report),
|
||||
("failure", "save_failure_reports_to_splunk", _failure_report),
|
||||
("smtp_tls", "save_smtp_tls_reports_to_splunk", _smtp_tls_report),
|
||||
)
|
||||
for url, secret in self.MALFORMED:
|
||||
client = HECClient(url=url, access_token="tok", index="dmarc")
|
||||
for name, method, make_report in saves:
|
||||
with self.subTest(url=url, report=name):
|
||||
with self.assertRaises(SplunkError) as ctx:
|
||||
getattr(client, method)(make_report())
|
||||
self.assertEqual(
|
||||
str(ctx.exception),
|
||||
"Invalid Splunk HEC URL (details withheld because they "
|
||||
"may include credentials)",
|
||||
)
|
||||
self.assertIsNone(ctx.exception.__cause__)
|
||||
self.assertTrue(ctx.exception.__suppress_context__)
|
||||
trace = "".join(traceback.format_exception(ctx.exception))
|
||||
self.assertNotIn(secret, trace)
|
||||
|
||||
def test_valid_url_is_the_one_posted(self):
|
||||
client = HECClient(
|
||||
url="https://admin:p4ssw0rd@splunk.example.com:8088",
|
||||
access_token="tok",
|
||||
index="dmarc",
|
||||
)
|
||||
client.session = MagicMock()
|
||||
client.session.post.return_value = _ok_response()
|
||||
client.save_aggregate_reports_to_splunk(_aggregate_report())
|
||||
self.assertEqual(
|
||||
client.session.post.call_args.args[0],
|
||||
"https://admin:p4ssw0rd@splunk.example.com:8088"
|
||||
"/services/collector/event/1.0",
|
||||
)
|
||||
|
||||
|
||||
class TestHECClientClose(unittest.TestCase):
|
||||
def test_close_closes_session(self):
|
||||
client = _client()
|
||||
|
||||
+464
-6
@@ -4,6 +4,7 @@ import os
|
||||
import shutil
|
||||
import tempfile
|
||||
import time
|
||||
import traceback
|
||||
import unittest
|
||||
from datetime import datetime, timezone
|
||||
from importlib.resources import files
|
||||
@@ -516,6 +517,122 @@ class Test(unittest.TestCase):
|
||||
self.assertTrue(parsedmarc.utils.is_outlook_msg(magic))
|
||||
|
||||
|
||||
class TestRedactUrlCredentials(unittest.TestCase):
|
||||
"""Tests for parsedmarc.utils.redact_url_credentials()"""
|
||||
|
||||
def testMasksUserinfoAndKeepsHost(self):
|
||||
cases = [
|
||||
("https://user:secret@host", "https://user:***@host"),
|
||||
("https://user:secret@host:9200/", "https://user:***@host:9200/"),
|
||||
("localhost:9200", "localhost:9200"),
|
||||
("127.0.0.1:9299", "127.0.0.1:9299"),
|
||||
("user:secret@localhost:9200", "user:***@localhost:9200"),
|
||||
# A "://" in the path or query of a scheme-less value is not
|
||||
# the scheme separator; the userinfo at offset 0 is still masked
|
||||
(
|
||||
"user:secret@host:9200/x?r=http://y",
|
||||
"user:***@host:9200/x?r=http://y",
|
||||
),
|
||||
("user:secret@host?r=a://b", "user:***@host?r=a://b"),
|
||||
("https://user:p@ss@host/x", "https://user:***@host/x"),
|
||||
("https://user:p:w@[::1]:9200/", "https://user:***@[::1]:9200/"),
|
||||
("https://[::1]:9200", "https://[::1]:9200"),
|
||||
("//user:pw@host", "//user:***@host"),
|
||||
# Documented over-masking: with no "@" in the authority, an "@"
|
||||
# later in the value is masked through, since it may end a
|
||||
# password that contains "/", "?" or "#"
|
||||
("http://host/path?e=a@b.com", "http://***@b.com"),
|
||||
("https://host/path@file", "https://***@file"),
|
||||
("https://host/?redirect=https://u:p@evil", "https://***@evil"),
|
||||
# Documented non-goal: with userinfo in the authority, credentials
|
||||
# inside a later query value are not masked
|
||||
(
|
||||
"https://a:b@host/?redirect=https://u:p@evil",
|
||||
"https://a:***@host/?redirect=https://u:p@evil",
|
||||
),
|
||||
("https://tok@host", "https://***@host"),
|
||||
("user@host:9200", "***@host:9200"),
|
||||
("https://user:@host", "https://user:***@host"),
|
||||
("https://u%40x:p%3Aw@h", "https://u%40x:***@h"),
|
||||
("not a url @ all", "***@ all"),
|
||||
("", ""),
|
||||
("::::", "::::"),
|
||||
]
|
||||
for value, expected in cases:
|
||||
with self.subTest(value=value):
|
||||
self.assertEqual(
|
||||
parsedmarc.utils.redact_url_credentials(value), expected
|
||||
)
|
||||
|
||||
def testMasksPasswordContainingAuthorityDelimiter(self):
|
||||
"""A password with an unencoded "/", "?" or "#" ends the authority
|
||||
before the "@" (RFC 3986 section 3.2), which used to leave the value
|
||||
unmasked; everything through the last "@" is masked instead."""
|
||||
cases = [
|
||||
("https://admin:p4ssw0rd/extra@host:3100", "https://***@host:3100"),
|
||||
("https://admin:p4ss?w0rd@host", "https://***@host"),
|
||||
("https://admin:p4ss#w0rd@host", "https://***@host"),
|
||||
("admin:p4ss/w0rd@host:9200", "***@host:9200"),
|
||||
("//admin:p4ss/w0rd@host", "//***@host"),
|
||||
# The password may itself contain "@"; the last one is used
|
||||
("https://admin:p4ss/w@rd@host:3100/x", "https://***@host:3100/x"),
|
||||
# A "://" after the "/" is not the scheme separator
|
||||
(
|
||||
"admin:p4ss/w0rd@host/x?r=http://y",
|
||||
"***@host/x?r=http://y",
|
||||
),
|
||||
]
|
||||
for value, expected in cases:
|
||||
with self.subTest(value=value):
|
||||
result = parsedmarc.utils.redact_url_credentials(value)
|
||||
self.assertEqual(result, expected)
|
||||
self.assertNotIn("p4ss", result)
|
||||
|
||||
def testListMasksPasswordContainingAuthorityDelimiter(self):
|
||||
self.assertEqual(
|
||||
parsedmarc.utils.redact_url_credentials(
|
||||
["https://admin:p4ss/w0rd@h1:9200", "https://h2:9200"]
|
||||
),
|
||||
["https://***@h1:9200", "https://h2:9200"],
|
||||
)
|
||||
|
||||
def testListIsRedactedElementWiseIntoANewList(self):
|
||||
original = ["a:b@h1", "https://h2"]
|
||||
result = parsedmarc.utils.redact_url_credentials(original)
|
||||
self.assertEqual(result, ["a:***@h1", "https://h2"])
|
||||
self.assertIsNot(result, original)
|
||||
self.assertEqual(original, ["a:b@h1", "https://h2"])
|
||||
|
||||
def testListKeepsNonStringItems(self):
|
||||
mixed = cast(list[str], ["a:b@h1", None])
|
||||
self.assertEqual(
|
||||
parsedmarc.utils.redact_url_credentials(mixed), ["a:***@h1", None]
|
||||
)
|
||||
|
||||
def testNonStringInputIsReturnedUnchanged(self):
|
||||
for value in (None, 42):
|
||||
with self.subTest(value=value):
|
||||
self.assertEqual(
|
||||
parsedmarc.utils.redact_url_credentials(cast(str, value)), value
|
||||
)
|
||||
|
||||
def testLongInputsAreFast(self):
|
||||
"""No regex is used, so adversarial input is linear. A 1 MB string
|
||||
of repeated openers and a 200,000-character run of '@' both finish
|
||||
in milliseconds."""
|
||||
for value in (
|
||||
"://" * 350_000,
|
||||
"@" * 200_000,
|
||||
"a:b@" * 250_000,
|
||||
"a:b/" * 250_000 + "@",
|
||||
"/@" * 500_000,
|
||||
):
|
||||
with self.subTest(prefix=value[:8]):
|
||||
started = time.monotonic()
|
||||
parsedmarc.utils.redact_url_credentials(value)
|
||||
self.assertLess(time.monotonic() - started, 5)
|
||||
|
||||
|
||||
class TestSanitizeForLog(unittest.TestCase):
|
||||
"""Tests for parsedmarc.utils.sanitize_for_log()"""
|
||||
|
||||
@@ -541,6 +658,197 @@ class TestSanitizeForLog(unittest.TestCase):
|
||||
self.assertNotIn("\n", result)
|
||||
|
||||
|
||||
class TestDownloadUrlCredentialsNotLogged(unittest.TestCase):
|
||||
"""A download URL with userinfo must not leak its password through the
|
||||
"Trying to fetch" debug line or the warning that embeds the httpx
|
||||
``HTTPStatusError`` message (which quotes the full URL).
|
||||
|
||||
The loaders run for real; only the network is replaced, with an
|
||||
``httpx.MockTransport`` returning 401 so httpx itself builds the
|
||||
exception text.
|
||||
"""
|
||||
|
||||
URL = "https://dluser:DLSECRET@downloads.example.test/map.dat"
|
||||
|
||||
@staticmethod
|
||||
def _transport_returning(handler):
|
||||
def get(url, **kwargs):
|
||||
with httpx.Client(transport=httpx.MockTransport(handler)) as client:
|
||||
return client.get(url, **kwargs)
|
||||
|
||||
return get
|
||||
|
||||
@classmethod
|
||||
def _status_getter(cls, reason_phrase=None):
|
||||
extensions = {}
|
||||
if reason_phrase is not None:
|
||||
extensions["reason_phrase"] = reason_phrase
|
||||
return cls._transport_returning(
|
||||
lambda request: httpx.Response(401, extensions=extensions)
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
def _connect_error_getter(url, **kwargs):
|
||||
def handler(request):
|
||||
raise httpx.ConnectError("connection refused", request=request)
|
||||
|
||||
with httpx.Client(transport=httpx.MockTransport(handler)) as client:
|
||||
return client.get(url, **kwargs)
|
||||
|
||||
# Reason phrases containing URL delimiters used to defeat redaction of
|
||||
# the HTTPStatusError text, which puts the phrase before the URL.
|
||||
REASON_PHRASES = (
|
||||
None,
|
||||
b"Unauthorized/Expired",
|
||||
b"Unauthorized?",
|
||||
b"Gone#",
|
||||
b"Not @ home",
|
||||
)
|
||||
|
||||
def _assert_redacted(self, logs):
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn("DLSECRET", output)
|
||||
self.assertTrue(
|
||||
any(
|
||||
line.startswith("DEBUG") and "downloads.example.test" in line
|
||||
for line in logs.output
|
||||
),
|
||||
output,
|
||||
)
|
||||
warnings = [line for line in logs.output if line.startswith("WARNING")]
|
||||
self.assertTrue(
|
||||
any(
|
||||
"HTTP 401" in line
|
||||
and "'https://dluser:***@downloads.example.test/map.dat'" in line
|
||||
for line in warnings
|
||||
),
|
||||
output,
|
||||
)
|
||||
|
||||
def _run_psl(self, url=None):
|
||||
with self.assertLogs("parsedmarc.log", level="DEBUG") as logs:
|
||||
parsedmarc.utils.load_psl_overrides(url=url or self.URL)
|
||||
return logs
|
||||
|
||||
def _run_ip_db(self, url=None):
|
||||
tmp_dir = tempfile.mkdtemp()
|
||||
self.addCleanup(shutil.rmtree, tmp_dir, ignore_errors=True)
|
||||
old_path = parsedmarc.utils._IP_DB_PATH
|
||||
self.addCleanup(setattr, parsedmarc.utils, "_IP_DB_PATH", old_path)
|
||||
parsedmarc.utils._IP_DB_PATH = None
|
||||
with (
|
||||
patch("parsedmarc.utils.tempfile.gettempdir", return_value=tmp_dir),
|
||||
self.assertLogs("parsedmarc.log", level="DEBUG") as logs,
|
||||
):
|
||||
parsedmarc.utils.load_ip_db(url=url or self.URL)
|
||||
return logs
|
||||
|
||||
def _run_reverse_dns_map(self, url=None):
|
||||
with self.assertLogs("parsedmarc.log", level="DEBUG") as logs:
|
||||
parsedmarc.utils.load_reverse_dns_map({}, url=url or self.URL)
|
||||
return logs
|
||||
|
||||
def _check_loader(self, run):
|
||||
for phrase in self.REASON_PHRASES:
|
||||
with self.subTest(reason_phrase=phrase):
|
||||
with patch("parsedmarc.utils.httpx.get", self._status_getter(phrase)):
|
||||
self._assert_redacted(run())
|
||||
|
||||
def _check_transport_error(self, run):
|
||||
with patch("parsedmarc.utils.httpx.get", self._connect_error_getter):
|
||||
logs = run()
|
||||
output = "\n".join(logs.output)
|
||||
self.assertNotIn("DLSECRET", output)
|
||||
self.assertTrue(
|
||||
any(
|
||||
line.startswith("WARNING")
|
||||
and "ConnectError: connection refused" in line
|
||||
for line in logs.output
|
||||
),
|
||||
output,
|
||||
)
|
||||
|
||||
# The password holds an unencoded "/", "?" or "#", which RFC 3986 section
|
||||
# 3.2 ends the authority at, so the text after the colon is read as the
|
||||
# port. Real httpx raises InvalidURL("Invalid port: 'p4ssw0rd'") while
|
||||
# parsing, before any network I/O (httpx 0.28.1 _urlparse.py:
|
||||
# ``raise InvalidURL(f"Invalid port: {port!r}")``). InvalidURL is not an
|
||||
# httpx.HTTPError, and its message quotes part of the password.
|
||||
MALFORMED_URLS = (
|
||||
("https://admin:p4ssw0rd/extra@downloads.example.test/map.dat", "p4ssw0rd"),
|
||||
("https://admin:p4ss?w0rd@downloads.example.test/map.dat", "p4ss"),
|
||||
("https://admin:p4ss#w0rd@downloads.example.test/map.dat", "p4ss"),
|
||||
)
|
||||
INVALID_URL_MESSAGE = (
|
||||
"invalid URL (details withheld because they may include credentials)"
|
||||
)
|
||||
|
||||
def _check_invalid_url(self, run, source):
|
||||
for url, secret in self.MALFORMED_URLS:
|
||||
with self.subTest(url=url):
|
||||
requested = []
|
||||
real_get = httpx.get
|
||||
|
||||
def recording_get(requested_url, **kwargs):
|
||||
requested.append(requested_url)
|
||||
if requested_url != url:
|
||||
# e.g. the PSL overrides fetch that
|
||||
# load_reverse_dns_map triggers; stay offline
|
||||
raise httpx.ConnectError("offline")
|
||||
return real_get(requested_url, **kwargs)
|
||||
|
||||
with patch("parsedmarc.utils.httpx.get", recording_get):
|
||||
logs = run(url) # must return, not crash on InvalidURL
|
||||
# The loader still requested the real URL, not a redacted one
|
||||
self.assertIn(url, requested)
|
||||
output = "\n".join(logs.output)
|
||||
# Neither the warning nor the "Trying to fetch" debug line
|
||||
# may show any part of the password
|
||||
self.assertNotIn(secret, output)
|
||||
self.assertIn(
|
||||
f"DEBUG:parsedmarc.log:Trying to fetch {source} from "
|
||||
"https://***@downloads.example.test/map.dat...",
|
||||
logs.output,
|
||||
)
|
||||
self.assertIn(
|
||||
f"WARNING:parsedmarc.log:Failed to fetch {source}: "
|
||||
f"{self.INVALID_URL_MESSAGE}",
|
||||
logs.output,
|
||||
)
|
||||
self.assertNotIn("Not a valid CSV file", output)
|
||||
self.assertNotIn("Failed to save IP database", output)
|
||||
|
||||
def test_psl_overrides_loader_invalid_url(self):
|
||||
"""A malformed ``psl_overrides_url`` used to crash startup with the
|
||||
InvalidURL traceback; it now warns and falls back to the local file."""
|
||||
self._check_invalid_url(self._run_psl, "PSL overrides")
|
||||
self.assertIn(".linode.com", parsedmarc.utils.psl_overrides)
|
||||
|
||||
def test_ip_db_loader_invalid_url(self):
|
||||
self._check_invalid_url(self._run_ip_db, "IP database")
|
||||
|
||||
def test_reverse_dns_map_loader_invalid_url(self):
|
||||
self._check_invalid_url(self._run_reverse_dns_map, "reverse DNS map")
|
||||
|
||||
def test_psl_overrides_loader(self):
|
||||
self._check_loader(self._run_psl)
|
||||
|
||||
def test_ip_db_loader(self):
|
||||
self._check_loader(self._run_ip_db)
|
||||
|
||||
def test_reverse_dns_map_loader(self):
|
||||
self._check_loader(self._run_reverse_dns_map)
|
||||
|
||||
def test_psl_overrides_loader_transport_error(self):
|
||||
self._check_transport_error(self._run_psl)
|
||||
|
||||
def test_ip_db_loader_transport_error(self):
|
||||
self._check_transport_error(self._run_ip_db)
|
||||
|
||||
def test_reverse_dns_map_loader_transport_error(self):
|
||||
self._check_transport_error(self._run_reverse_dns_map)
|
||||
|
||||
|
||||
class TestLoadPSLOverrides(unittest.TestCase):
|
||||
"""Covers `parsedmarc.utils.load_psl_overrides`."""
|
||||
|
||||
@@ -1559,16 +1867,20 @@ class TestEncryptedDnsNameservers(unittest.TestCase):
|
||||
must fail at configuration time naming the entry, not parse
|
||||
"successfully" with no TLS certificate identity and then fail at
|
||||
query time with an opaque certificate error. Userinfo and query
|
||||
components are rejected the same way."""
|
||||
for entry in (
|
||||
"tls://9.9.9.9/dns.quad9.net",
|
||||
"tls://user@9.9.9.9",
|
||||
"tls://9.9.9.9?hostname=dns.quad9.net",
|
||||
components are rejected the same way, with the userinfo masked in
|
||||
the message (GHSA-hjr3-rx4c-24cv)."""
|
||||
for entry, shown in (
|
||||
("tls://9.9.9.9/dns.quad9.net", "tls://9.9.9.9/dns.quad9.net"),
|
||||
("tls://user@9.9.9.9", "tls://***@9.9.9.9"),
|
||||
(
|
||||
"tls://9.9.9.9?hostname=dns.quad9.net",
|
||||
"tls://9.9.9.9?hostname=dns.quad9.net",
|
||||
),
|
||||
):
|
||||
with self.subTest(entry=entry):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
parsedmarc.utils._nameservers_to_resolver_input([entry])
|
||||
self.assertIn(entry, str(ctx.exception))
|
||||
self.assertIn(shown, str(ctx.exception))
|
||||
|
||||
def test_unsplittable_entry_is_left_for_dnspython_to_reject(self):
|
||||
"""urlsplit itself raises on some malformed input (an unbalanced
|
||||
@@ -1712,6 +2024,152 @@ class TestEncryptedDnsNameservers(unittest.TestCase):
|
||||
self.assertEqual(parsedmarc.utils._DOH_SESSION_PID, os.getpid())
|
||||
|
||||
|
||||
class TestNameserverCredentialsNotLeaked(unittest.TestCase):
|
||||
"""GHSA-hjr3-rx4c-24cv: nameserver URL credentials must not reach an
|
||||
exception message, its printed traceback, or the log."""
|
||||
|
||||
def test_url_secrets_finds_the_password_and_its_port_like_prefix(self):
|
||||
"""The secret is the userinfo after its first ":" (the whole
|
||||
userinfo when there is none). A "/", "?" or "#" in it ends the URL
|
||||
authority (RFC 3986 section 3.2), so urlsplit() reads the text
|
||||
before it as the port and quotes it in its error; that prefix is a
|
||||
secret too."""
|
||||
cases = [
|
||||
("https://doh:p4ss@dns.example/q", ["p4ss"]),
|
||||
("https://doh:p4ss/w0rd@dns.example/q", ["p4ss/w0rd", "p4ss"]),
|
||||
("https://doh:p4ss?w0rd@dns.example/q", ["p4ss?w0rd", "p4ss"]),
|
||||
("https://doh:p4ss#w0rd@dns.example/q", ["p4ss#w0rd", "p4ss"]),
|
||||
# The prefix ends at the first of the three delimiters
|
||||
("https://doh:ab#cd/ef@dns.example/q", ["ab#cd/ef", "ab"]),
|
||||
("https://doh:p:w@dns.example/q", ["p:w"]),
|
||||
("https://doh:p@ss@dns.example/q", ["p@ss"]),
|
||||
("https://S3CRETTOKEN@dns.example/q", ["S3CRETTOKEN"]),
|
||||
("tls://dot:p4ss/w0rd@1.1.1.1", ["p4ss/w0rd", "p4ss"]),
|
||||
# Empty secrets are never returned: str.replace("", ...) would
|
||||
# insert the mask between every character
|
||||
("https://doh:@dns.example/q", []),
|
||||
("https://doh:/x@dns.example/q", ["/x"]),
|
||||
("https://dns.example/dns-query", []),
|
||||
("8.8.8.8", []),
|
||||
("", []),
|
||||
]
|
||||
for value, expected in cases:
|
||||
with self.subTest(value=value):
|
||||
self.assertEqual(parsedmarc.utils._url_secrets(value), expected)
|
||||
|
||||
def test_mask_nameserver_secrets_masks_entries_passwords_and_prefixes(self):
|
||||
"""Each configured entry is replaced by its redacted form, and each
|
||||
secret in it wherever it still appears, including the bare port-like
|
||||
prefix that urlsplit() and httpx quote."""
|
||||
mask = parsedmarc.utils._mask_nameserver_secrets
|
||||
url = "https://doh:p4ss/w0rd@dns.example/dns-query"
|
||||
text = (
|
||||
f"Server {url} answered x; "
|
||||
"Port could not be cast to integer value as 'p4ss'; "
|
||||
"Invalid port: 'p4ss/w0rd'"
|
||||
)
|
||||
self.assertEqual(
|
||||
mask(text, [url, "8.8.8.8"]),
|
||||
"Server https://***@dns.example/dns-query answered x; "
|
||||
"Port could not be cast to integer value as '***'; "
|
||||
"Invalid port: '***'",
|
||||
)
|
||||
self.assertEqual(
|
||||
mask(
|
||||
"Server https://u:PW123@h.example/q failed (PW123)",
|
||||
["https://u:PW123@h.example/q"],
|
||||
),
|
||||
"Server https://u:***@h.example/q failed (***)",
|
||||
)
|
||||
self.assertEqual(mask("abc", None), "abc")
|
||||
self.assertEqual(mask("abc", ["https://u:@h"]), "abc")
|
||||
|
||||
def test_mask_nameserver_secrets_handles_unsplittable_entries(self):
|
||||
"""urlsplit() raises ValueError for an entry such as ``https://[x``;
|
||||
the masking uses string methods only, so it still returns text."""
|
||||
text = "Server https://[x failed; Server https://u:PW@[x failed"
|
||||
self.assertEqual(
|
||||
parsedmarc.utils._mask_nameserver_secrets(
|
||||
text, ["https://[x", "https://u:PW@[x"]
|
||||
),
|
||||
"Server https://[x failed; Server https://u:***@[x failed",
|
||||
)
|
||||
|
||||
def test_malformed_https_and_tls_ports_are_reported_without_secrets(self):
|
||||
"""A password containing "/", "?" or "#" makes urlsplit() read its
|
||||
start as the port, and its ValueError quotes that text; dnspython
|
||||
2.9.1 raised it from ``DoHNameserver.answer_port()`` only after a
|
||||
failed query (``dns/nameserver.py``). Both schemes are now rejected
|
||||
before any query, naming the entry with its credentials masked and
|
||||
with no chained exception repeating the urlsplit message."""
|
||||
for entry, shown in (
|
||||
(
|
||||
"https://doh:p4ss/w0rd@dns.example/dns-query",
|
||||
"Invalid DNS over HTTPS nameserver https://***@dns.example/dns-query",
|
||||
),
|
||||
(
|
||||
"https://doh:p4ss?w0rd@dns.example/dns-query",
|
||||
"Invalid DNS over HTTPS nameserver https://***@dns.example/dns-query",
|
||||
),
|
||||
(
|
||||
"tls://dot:p4ss#w0rd@1.1.1.1",
|
||||
"Invalid DNS over TLS nameserver tls://***@1.1.1.1",
|
||||
),
|
||||
(
|
||||
"https://dns.example:notaport/dns-query",
|
||||
"Invalid DNS over HTTPS nameserver https://dns.example:notaport/",
|
||||
),
|
||||
):
|
||||
with self.subTest(entry=entry):
|
||||
with patch("dns.query.https", side_effect=AssertionError):
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
parsedmarc.utils.query_dns(
|
||||
"1.0.0.1.in-addr.arpa",
|
||||
"PTR",
|
||||
nameservers=[entry],
|
||||
timeout=0.2,
|
||||
)
|
||||
trace = "".join(traceback.format_exception(ctx.exception))
|
||||
self.assertIn(shown, str(ctx.exception))
|
||||
self.assertIsNone(ctx.exception.__cause__)
|
||||
self.assertIsNone(ctx.exception.__context__)
|
||||
for secret in ("p4ss", "w0rd"):
|
||||
self.assertNotIn(secret, trace)
|
||||
|
||||
def test_dnspython_rejection_of_an_entry_is_masked(self):
|
||||
"""dnspython rejects an entry urlsplit() cannot split with a message
|
||||
that quotes it as configured ("nameserver https://u:pw@[x is not a
|
||||
dns.nameserver.Nameserver instance ..."); query_dns() masks it."""
|
||||
entry = "https://doh:S3CRETPW@[dns.example/dns-query"
|
||||
with self.assertRaises(ValueError) as ctx:
|
||||
parsedmarc.utils.query_dns(
|
||||
"1.0.0.1.in-addr.arpa", "PTR", nameservers=[entry], timeout=0.2
|
||||
)
|
||||
trace = "".join(traceback.format_exception(ctx.exception))
|
||||
self.assertIn("https://doh:***@[dns.example/dns-query", str(ctx.exception))
|
||||
self.assertNotIn("S3CRETPW", trace)
|
||||
|
||||
def test_get_reverse_dns_debug_log_masks_nameserver_credentials(self):
|
||||
"""get_reverse_dns() logs the DNS error it swallows at debug level,
|
||||
and LifetimeTimeout/NoNameservers quote each failing nameserver
|
||||
URL, so the configured credentials are masked there too."""
|
||||
url = "https://doh:S3CRETPW@dns.example/dns-query"
|
||||
|
||||
def fail(*args, **kwargs):
|
||||
raise OSError(f"connection to {url} refused (S3CRETPW)")
|
||||
|
||||
with patch("dns.query.https", side_effect=fail):
|
||||
with self.assertLogs("parsedmarc.log", level="DEBUG") as logs:
|
||||
result = parsedmarc.utils.get_reverse_dns(
|
||||
"1.0.0.1", nameservers=[url], timeout=0.2, retries=0
|
||||
)
|
||||
self.assertIsNone(result)
|
||||
output = "\n".join(logs.output)
|
||||
self.assertIn("get_reverse_dns(1.0.0.1) exception:", output)
|
||||
self.assertIn("https://doh:***@dns.example/dns-query", output)
|
||||
self.assertNotIn("S3CRETPW", output)
|
||||
|
||||
|
||||
class TestLoadIpDb(unittest.TestCase):
|
||||
"""Tests for the load_ip_db() download/cache/bundled fallback chain,
|
||||
mocking at the httpx SDK boundary."""
|
||||
|
||||
@@ -114,6 +114,50 @@ class TestWebhookErrorHandling(unittest.TestCase):
|
||||
client.save_smtp_tls_report_to_webhook('{"t": 1}')
|
||||
|
||||
|
||||
class TestWebhookInvalidUrlDoesNotLeakPassword(unittest.TestCase):
|
||||
"""GHSA-hjr3-rx4c-24cv: a password with an unencoded "/", "?" or "#"
|
||||
ends the URL's authority early (RFC 3986 section 3.2), so httpx reads the
|
||||
text after the colon as the port and raises
|
||||
``InvalidURL("Invalid port: 'p4ssw0rd'")`` when posting (httpx 0.28.1
|
||||
_urlparse.py: ``raise InvalidURL(f"Invalid port: {port!r}")``), before
|
||||
any network I/O. The error is logged on every report."""
|
||||
|
||||
MALFORMED = (
|
||||
("https://admin:p4ssw0rd/extra@hooks.example.com/x", "p4ssw0rd"),
|
||||
("https://admin:p4ss?w0rd@hooks.example.com/x", "p4ss"),
|
||||
("https://admin:p4ss#w0rd@hooks.example.com/x", "p4ss"),
|
||||
)
|
||||
|
||||
def test_invalid_url_is_logged_without_password(self):
|
||||
saves = (
|
||||
"save_aggregate_report_to_webhook",
|
||||
"save_failure_report_to_webhook",
|
||||
"save_smtp_tls_report_to_webhook",
|
||||
)
|
||||
for url, secret in self.MALFORMED:
|
||||
client = WebhookClient(url, url, url)
|
||||
for method in saves:
|
||||
with self.subTest(url=url, method=method):
|
||||
with self.assertLogs("parsedmarc.log", level="ERROR") as cm:
|
||||
getattr(client, method)('{"a": 1}') # must not raise
|
||||
self.assertEqual(
|
||||
cm.output,
|
||||
[
|
||||
"ERROR:parsedmarc.log:Webhook Error: invalid webhook "
|
||||
"URL (details withheld because they may include "
|
||||
"credentials)"
|
||||
],
|
||||
)
|
||||
self.assertNotIn(secret, "\n".join(cm.output))
|
||||
|
||||
def test_valid_url_is_the_one_posted(self):
|
||||
url = "https://admin:p4ssw0rd@hooks.example.com/x"
|
||||
client = WebhookClient(url, url, url)
|
||||
client.session = MagicMock()
|
||||
client.save_aggregate_report_to_webhook('{"a": 1}')
|
||||
client.session.post.assert_called_once_with(url, content='{"a": 1}', timeout=60)
|
||||
|
||||
|
||||
class TestWebhookClientClose(unittest.TestCase):
|
||||
def test_close_closes_session(self):
|
||||
client = _client()
|
||||
|
||||
Reference in new issue
Block a user