From 02fcfd6e82575c52bdbc27cef967db2628c306b7 Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Tue, 10 Nov 2026 18:44:59 -0500 Subject: [PATCH] Merge commit from fork * Evict expired dedup-cache entries on insert and hash the key (GHSA-9fw6-mwqr-p43m) The seen-aggregate-report cache is an expiringdict.ExpiringDict, which removes an expired entry only when that same key is read again, or drops its oldest entry once it holds max_len (100 million) entries. A report that is never received again is never read again, so in watch mode every distinct aggregate report stayed in memory for the life of the process. The key was the JSON of the sender-controlled reporter, Report-ID and policy domain, so ten 5.6 KB attachments with a 5 MiB report_id held 64 MiB indefinitely. SeenReportCache (parsedmarc/config.py) subclasses ExpiringDict and removes expired entries from the head of its insertion order on every insert; the cache's max_len is now 100,000. _aggregate_report_dedup_key() now returns the SHA-256 hex digest of that JSON, so a key is 64 characters whatever the report contains. Co-Authored-By: Claude Opus 5.5 * Keep a re-added dedup-cache key at the tail so the expiry sweep is not blocked OrderedDict refreshes a re-set key's time in place, so a key added again while still stored kept its old position at the head and stopped the insert-time sweep before the expired entries behind it. SeenReportCache now moves such a key to the tail before storing it, keeping insertion order equal to age order. Co-Authored-By: Claude Fable 5.1 * Bound failed reverse-DNS lookups to 120 seconds per aggregate report (GHSA-rwfh-ww9c-jm9m) Every record of an aggregate report made one blocking PTR lookup of a sender-chosen source IP, with no bound on count or time; an unanswering reverse zone costs the full 8 s DNS lifetime with the default nameservers, and a weak-fallback result is never cached, so one IP repeated in every record was looked up every time. A 3,500-record report could block processing for hours. Lookups that return no PTR now share a 120-second budget per report; once it is spent, the remaining records are enriched from the local IP database only, their results are not cached, and one warning is logged. Each distinct source IP is looked up at most once per report, and lookups that find a PTR are not charged, so large legitimate reports keep their enrichment. Co-Authored-By: Claude Fable 5.1 * Keep values PostgreSQL rejects out of its output, and let one rejected report not hold the batch (GHSA-fxr7-7h9q-v949) An aggregate count above 2**31 - 1, an SMTP TLS sending-mta-ip or receiving-ip that is not an IP address, and a NUL in a failure report's sample each made the PostgreSQL save raise PostgreSQLError, which marked the whole mailbox batch unsaved, so legitimate reports from other senders were never archived. The parser now skips a record whose count is outside 0..MAX_MESSAGE_COUNT and rejects such an SMTP TLS session count (closing the same rejection in Elasticsearch/OpenSearch), drops an invalid TLS IP with a warning, and the PostgreSQL output replaces NUL with U+FFFD in text, text array and JSONB values. Any other psycopg.DataError raises the new PostgreSQLRejectedReport, which the CLI logs and skips for that one report unless fail_on_output_error is set. Co-Authored-By: Claude Fable 5.1 * Check an aggregate record's count before its reverse-DNS lookup A record whose count is outside 0..MAX_MESSAGE_COUNT is dropped, so its source IP is no longer looked up first, which could cost up to 8 seconds per record. A regression test mocks dnspython and asserts that the dropped record's address is never queried. Co-Authored-By: Claude Fable 5.1 * Require TLS for the PostgreSQL output by default and add [postgresql] sslmode and sslrootcert (GHSA-xvp7-7762-gx9g) PostgreSQLClient connected without an sslmode, so libpq used its default, prefer: when the server, or an on-path attacker, declined TLS, it continued over plain TCP and answered a clear-text password request. The client now passes sslmode=require unless the new sslmode argument, an sslmode in the connection string, or PGSSLMODE chooses a mode, and sslrootcert names the CA certificate for verify-ca and verify-full. A warning is logged at startup when a TCP host other than localhost or a loopback address is used with a mode that does not verify the server certificate. A malformed connection string is now rejected by a parse before connecting, with the same withheld-details error. A server without TLS fails with libpq's "server does not support SSL, but SSL was required" until sslmode is set to disable or prefer. Co-Authored-By: Claude Fable 5.1 * Withhold the hosts URL from Elasticsearch and OpenSearch client-build errors (GHSA-74xv-rmpx-2pjm) A password with an unencoded "/", "?" or "#" in a [elasticsearch] or [opensearch] hosts URL makes elastic_transport raise "Could not parse URL ''" and opensearch-py (via urllib.parse) raise "Port could not be cast to integer value as ''". The startup wrapper repeated that text at ERROR without --debug and chained the original, which the SIGHUP reload's logger.exception printed again. set_hosts() now runs through _set_search_hosts(), which replaces any ValueError with a fixed message raised outside the except block. Other errors in the block keep their text with URL credentials masked and are raised from None. This completes the GHSA-hjr3-rx4c-24cv fix. Co-Authored-By: Claude Fable 5.1 * Cache the downloaded IP database in a private per-user directory and validate it (GHSA-5f56-8ph8-v9q5) load_ip_db() cached the IPinfo Lite download at the fixed, shared /parsedmarc/ipinfo_lite.mmdb, where another local user could plant a symlink to have the download overwrite a file the parsedmarc user can write, or plant a database that was used whenever the download failed. The cache now lives in $XDG_CACHE_HOME/parsedmarc or ~/.cache/parsedmarc (%LOCALAPPDATA%\parsedmarc\cache on Windows), created 0700 and used only if it is a real directory the user owns and can write that is not group- or world-writable; otherwise a per-run mkdtemp() directory is used. The download is written with mkstemp(), opened with maxminddb to validate it, and moved into place with os.replace(). A cached copy is used only if it is a regular file owned by the user. Co-Authored-By: Claude Fable 5.1 * Skip reports over the Logs Ingestion API size limit and publish the Log Analytics streams independently (GHSA-rg4h-58fm-8pc9) The Azure Monitor ingestion SDK packs whole reports into chunks of up to 1 MiB and never splits one, so a report whose JSON is larger is sent as a call of its own, and the Logs Ingestion API rejects any call over 1 MB (https://learn.microsoft.com/en-us/azure/azure-monitor/fundamentals/service-limits). The rejected upload raised before the failure and SMTP TLS streams were sent, and the CLI held the whole batch for retry and finally Unsaved. publish_json() now measures each report as the SDK does and leaves out one over 1,000,000 bytes with a warning naming it, and publish_results() publishes each stream in its own try and raises one LogAnalyticsException naming every stream that failed. Co-Authored-By: Claude Fable 5.1 * Address review: redact the PostgreSQL warning host, drop the connect error chain, and fix Log Analytics and docs wording libpq ends a URI's userinfo at the first "@", so the rest of a password containing "@" becomes part of the host. The PostgreSQL startup warning now credential-masks every host-like value it logs (host list entries, hostaddr, PGHOST, PGHOSTADDR), checks hostaddr before host as libpq does, and the general connect error is raised from None so the driver's unredacted text no longer reaches a logged traceback. The CLI's output-client Raises docs now say which outputs do not chain. publish_json() returns the number of reports uploaded, and publish_results() logs "Successfully pushed" only for a stream that uploaded something. The [postgresql] URI example uses the Unix socket and explains that TCP to localhost needs TLS on the server or sslmode = disable, the [log_analytics] docs note the SDK's 1 MiB call packing, the reverse-DNS-budget wording says "local IP database and reverse DNS map" everywhere, and the GHSA-fxr7 entry no longer claims the parser rejects every unreadable start-datetime. Co-Authored-By: Claude Fable 5.1 * Require mailsuite 2.3.7 mailsuite 2.3.7 fixes parse_email() keeping a CRLF message's carriage returns in address header values (seanthegeek/mailsuite#74). The floor is raised in the base dependency and the msgraph, gmail and all extras, and the usage docs' stated requirement is updated to match. Co-Authored-By: Claude Fable 5.1 * Drop Python 3.10 and add Python 3.15 to the supported versions and CI (#953) Python 3.10 reaches end of life in October 2026. requires-python is now >=3.11, the unused requires_python key under [build-system] (not read by hatchling, and its <3.15 bound read as excluding 3.15) is removed, and Python 3.15 joins the CI matrix, classifiers, and support tables. Docs, AGENTS.md, and comments that named 3.10 as the floor are updated. Co-authored-by: Claude Sonnet 5.5 * Keep aggregate identifiers and reasons within their schema (#950) * Keep aggregate identifiers and reasons within their schema AI assistance: OpenAI Codex. * Expand the #948 CHANGELOG entry and tighten its tests RFC 9990 Appendix A and RFC 7489 Appendix C define only header_from, envelope_from and envelope_to under identifiers, and only type and comment in a reason. Cite them in the test, check that envelope_from and envelope_to survive next to a dropped nested unknown element, and cover an unknown key on the legacy identities path. The CHANGELOG entry now names the affected outputs and the compatibility change. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 * Reject reports dated in the future and bound what parse_report_file() reads (#954) * Reject reports whose index-naming date is more than two days in the future (#947) The date that names a report's dated Elasticsearch/OpenSearch index (an aggregate report's begin date, a failure report's Arrival-Date, an SMTP TLS report's start-datetime) was never compared with the present, so a report dated 2100 named an index for 2100. Each parser now rejects such a report with its own Invalid*Report; past dates stay unbounded. Co-Authored-By: Claude Sonnet 5.5 * Refuse an input larger than the report size cap before parse_report_file() reads it (#949) The 32 MiB cap was applied only after the whole input was read. A path is now checked with os.path.getsize(), bytes-like input with len(), and a file object is read at most one byte past the cap. The error is the ParserError with the message the content check already gives. Three existing tests that fed over-cap content now see the earlier rejection. Co-Authored-By: Claude Sonnet 5.5 * Measure a memoryview input by its byte size len() of a memoryview counts items, which are not bytes for every format, so the size cap in parse_report_file() compares nbytes instead. Co-Authored-By: Claude Fable 5.2 * Bound the aggregate end date and read file objects to their end Address the Copilot review of #954. A normalized report's per-day records name their indexes from their own interval starts, so the report's end date is bounded like its begin date. A raw stream may return fewer bytes than asked for before its end, so parse_report_file() now reads in a loop until end of file or one byte past the cap. Co-Authored-By: Claude Fable 5.2 * Accumulate bounded reads in one buffer Address the second Copilot review of #954. Keeping every chunk in a list cost one bytes object per read, about 85 bytes per byte for a stream that returns a byte at a time (22.7 MiB for 256 KiB of input). One bytearray keeps memory proportional to the bytes read. Co-Authored-By: Claude Fable 5.2 --------- Co-authored-by: Claude Sonnet 5.5 * Skip a message with an invalid report attachment instead of aborting the mbox (#955) * Skip a message with an invalid report attachment instead of aborting the mbox parse_report_email() raised a bare ParserError when a report attachment was invalid, which get_dmarc_reports_from_mbox() re-raises, so one such message ended the whole file. Raise InvalidDMARCReport (a ParserError subclass), which the mbox loop skips with a warning. Co-Authored-By: Claude Sonnet 5.5 * Skip an invalid SMTP TLS attachment in an mbox too Address the Copilot review of #955. An invalid SMTP TLS attachment raised InvalidSMTPTLSReport, which is not an InvalidDMARCReport, so it still stopped the mbox. parse_report_email() now names the message's subject in that error, and both mbox loops skip it with a warning. A message that cannot be parsed at all still raises a plain ParserError. Co-Authored-By: Claude Fable 5.2 --------- Co-authored-by: Claude Sonnet 5.5 * Address the second review: refuse an unencoded @ in a PostgreSQL URI, charge slow reverse-DNS lookups, and smaller fixes - PostgreSQL: a connection string whose parsed host, hostaddr, port or dbname holds an "@" (the rest of a URI password with an unencoded "@", which libpq splits at the first "@") is refused with a fixed error before any warning is logged or any connection is attempted; _connect parses the string on every path. A leading "@" in a host entry (Linux abstract socket) is still allowed. _log_safe_host stays as defence in depth for the host argument and PGHOST/PGHOSTADDR. - Log Analytics: publish_json catches AzureError, so a ServiceRequestError or ServiceResponseError no longer stops the remaining streams. - MAX_MESSAGE_COUNT comment and CHANGELOG: the bound matches PostgreSQL's INTEGER columns and the Elasticsearch/OpenSearch integer mapping of the SMTP TLS session counts; the aggregate message_count is dynamically mapped as long. - Reverse DNS budget: a lookup taking at least SLOW_REVERSE_DNS_SECONDS (1.0) is charged whether or not it returned a name; the warning now says "failed or were slow". - An IPv6 zone ID in an aggregate record's source IP skips the record before any DNS lookup, and in a failure report's Source-IP raises InvalidFailureReport; one helper serves these and the SMTP TLS path. - IP database cache: a relative candidate directory (HOME unset, no passwd entry) is rejected; offline / always_use_local_file creates no directory and no temporary directory and logs no "downloaded again". - No startup warning for sslmode=require with an existing sslrootcert file, which libpq verifies as verify-ca. - PostgreSQLRejectedReport truncates the driver text to 200 characters. Co-Authored-By: Claude Fable 5.2 * Address the third review: refuse a URI with more than one @, bound total reverse-DNS time, strip IPv6 zone IDs - postgres: a URI connection string (postgresql:// or postgres://) with more than one literal "@" is refused on the raw string, before any log line or connect, closing the abstract-socket host-list and query parameter bypasses of the parsed-value check. libpq decodes %40 in a password, host, database name and query value, so a URI never needs a second "@". The parsed-value check remains for keyword/value strings. - Reverse DNS: every online lookup is charged to a new 600 s per-report total budget (MAX_REVERSE_DNS_SECONDS_PER_REPORT); failed or slow ones still also use the 120 s budget. The warning names the limit reached. - An IPv6 zone ID in an aggregate source_ip or a failure report's Source-IP is now stripped with a warning instead of rejecting the record or report; SMTP TLS still drops its optional IP fields. - _rejection_detail truncates the driver text before escaping it. - Docs and CHANGELOG entries updated. Co-Authored-By: Claude Fable 5.2 * Withhold everything derived from a PostgreSQL connection string from logs and errors Fourth review of GHSA-xvp7-7762-gx9g: libpq ends a URI's userinfo at the first "@" or "/", so postgresql://u:SECRETX/x@db.invalid/d parses to host "u", port "SECRETX" and dbname "x@db.invalid/d". It has one "@", so the raw-count refusal let it through, and connecting raised 'invalid integer value "SECRETX" for connection option "port"', a password fragment in the error and its traceback. A variant put the fragment in sslmode, which the startup warning printed. Three rounds of shape checks each missed a shape. A. With a connection_string, nothing parsed from it is logged or raised: - A connection failure raises a fixed "PostgreSQL connection failed (details withheld ...)" from None, with " [SQLSTATE xxxxx]" appended only when the driver error carries a well-formed SQLSTATE (^[0-9A-Z]{5}$). A string libpq cannot parse keeps its own fixed message, and any error from the pre-parse is replaced by it. - The startup warning names no host and no value from the string; the mode is printed only when it is one of SSL_MODES. With the separate host/port/user/password/database arguments, the driver's message (credential-masked) and the host in the warning are kept. B. The early refusal stays as a convenience that names the common mistake, mirroring libpq's scan: a URI is refused when the text after "://" holds more than one "@", or an "@" with a "/" before it, and the parsed-value check (_has_misplaced_at) runs on URIs again as well as keyword/value strings. Docstrings, usage.md and the CHANGELOG now say the refusal is a convenience and A is what keeps the password out. Co-Authored-By: Claude Fable 5.2 * Wrap every PostgreSQL connect failure and narrow the connection-string refusal - postgres._connect(): everything from parsing the settings to holding a configured connection (pre-parse, ssl kwargs, connect, autocommit, dumper registration) is protected. A non-driver error (psycopg's UnicodeEncodeError carrying the whole built connection string for a password with a lone surrogate; on Python 3.15 the unwrapped UnicodeEncodeError('idna', '') for an over-long host label) is replaced with a fixed message from None: the withheld connection failure message with a connection string, a new fixed "a connection setting could not be passed to the driver" message with the separate settings. KeyboardInterrupt/SystemExit still propagate. - cli._init_output_clients(): the PostgreSQL wrapper no longer chains (from None), and a non-PostgreSQLError gets a fixed message. - _has_unencoded_at() now mirrors libpq's URI scan (userinfo up to the first "@" or "/") instead of counting "@", so a percent-encoded database name (d%40x) and an "@" in a query value after a "/" (a certificate path) are accepted, while every password-spill shape in the table is still refused except two that read like legitimate strings. - _has_misplaced_at() is removed: a keyword/value password is its own keyword, so an "@" in a kv host, hostaddr or dbname is not a password fragment, and libpq does not percent-decode kv values, so the encoding advice was wrong. The refusal message now names the database name. - Docstrings, usage.md and the GHSA-xvp7 CHANGELOG entry updated; tests for real psycopg (surrogate password, long host label), fake-psycopg non-driver errors at connect/autocommit/dumpers for both modes, a KeyboardInterrupt check, the rule table (also checked against the real parser), legitimate strings that must connect, and the CLI wrapper. Co-Authored-By: Claude Fable 5.2 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: jwalith4418-Mj --- CHANGELOG.md | 15 + docs/source/usage.md | 109 ++- parsedmarc/__init__.py | 256 ++++++- parsedmarc/cli.py | 149 +++- parsedmarc/config.py | 48 +- parsedmarc/constants.py | 38 + parsedmarc/loganalytics.py | 120 ++- parsedmarc/postgres.py | 529 ++++++++++++- parsedmarc/utils.py | 208 ++++- pyproject.toml | 8 +- tests/test_cli.py | 384 +++++++++- tests/test_config.py | 76 ++ tests/test_init.py | 605 +++++++++++++++ tests/test_loganalytics.py | 210 ++++- tests/test_postgres.py | 1482 +++++++++++++++++++++++++++++++++++- tests/test_utils.py | 532 ++++++++++++- 16 files changed, 4591 insertions(+), 178 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bec2ea9e..add774ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,23 @@ ## Unreleased +### Security + +- **The seen-aggregate-report cache no longer keeps every distinct report in memory for the life of the process** ([GHSA-9fw6-mwqr-p43m](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-9fw6-mwqr-p43m), affects 11.2.2 and earlier). The cache that recognizes a duplicate aggregate report is meant to forget a report after one hour, but the `expiringdict` package it is built on removes an expired entry only when that same key is looked up again, and otherwise drops its oldest entry only once it holds `max_len` entries, which was 100 million. A report that is never received again is never looked up again, so in watch mode every distinct aggregate report stayed in memory until the process exited. The cache key was the reporter's name, the Report-ID and the policy domain as JSON, all three chosen by the sender with no length limit below the 32 MiB decompression cap: ten emails each carrying a 5.6 KB gzip attachment with a 5 MiB `` left 64 MiB held by the cache, and none of it was released two hours after the one-hour expiry. Expired entries are now removed each time a report is added to the cache, the cache holds at most 100,000 entries, and the key is a SHA-256 digest of the three fields, so its size no longer depends on the report. **Compatibility:** the cache is process-local and never saved, so nothing persisted changes; a report is still recognized as a duplicate for one hour. Reported by helixnebulatech. +- **Reverse DNS lookups are limited to 120 seconds per aggregate report when they fail or are slow, and to 600 seconds in total** ([GHSA-rwfh-ww9c-jm9m](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-rwfh-ww9c-jm9m), affects 11.2.2 and earlier). Each `` of an aggregate report triggered one blocking reverse DNS (PTR) lookup of its source IP, one record after another, with no bound on the number of lookups or the time they took, and the sender chooses those IPs. An IP whose reverse zone never answers costs the whole DNS lifetime, `dns_timeout` times the number of nameservers: 2 s × 4 default nameservers = 8 s. Repeating one such IP did not help, because a result with no PTR whose AS domain is not in the reverse DNS map is deliberately kept out of the IP address cache, so the same IP was looked up again in every record. Measured against a nameserver that never answers, parsing took 2.2 s per record with one nameserver and 8.2 s per record with four, so a 1 MB report of about 3,500 records held up processing for hours, and no other message was processed meanwhile. Lookups that return no PTR, and lookups that take at least one second whether or not they return a name (a sender who controls the reverse zone could otherwise answer every query with a name just before the timeout), now share a budget of 120 seconds per report. Fast lookups that find a PTR are not counted toward it, so a large legitimate report keeps its enrichment, but every lookup counts toward a second budget of 600 seconds per report for all reverse DNS lookups, since a sender who controls the reverse zone could otherwise answer every query just under one second (5,000 distinct source IPs resolving in 100 ms each use 500 seconds). Once either budget is spent, the report's remaining records are enriched from the local IP database and reverse DNS map only, and one warning is logged: "Reverse DNS lookups for this report failed or were slow for 120 seconds; the remaining records are enriched from the local IP database and reverse DNS map only", or "Reverse DNS lookups for this report took more than 600 seconds; ..." with the same ending. Results produced after the limit are not cached, so those IPs are looked up normally in the next report. Each distinct source IP is also looked up at most once per report. Deployments with `offline = True` make no DNS queries and were never affected. Reported by helixnebulatech. +- **A report value that PostgreSQL rejects no longer holds back the whole batch** ([GHSA-fxr7-7h9q-v949](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-fxr7-7h9q-v949), affects the PostgreSQL output in 11.2.2 and earlier). The PostgreSQL output inserted report values into typed columns without checking them, and three values a sender controls were rejected by the server or the driver: an aggregate `` above 2,147,483,647 (`integer out of range` in the `INTEGER` `message_count` column), an SMTP TLS `sending-mta-ip` or `receiving-ip` that is not an IP address (`invalid input syntax for type inet`), and a NUL character in a failure report's sample, for example a `Subject:` of `=?utf-8?q?a=00b?=` (PostgreSQL cannot store NUL in `text` or `jsonb`). A fourth trigger, an SMTP TLS `start-datetime` PostgreSQL cannot read, has mostly been rejected by the parser since 11.2.2; the remainder (partial dates such as `2016`, which the parser still accepts and PostgreSQL may reject) now raise `PostgreSQLRejectedReport` like any other rejected value and no longer hold back the batch. Each rejection was recorded as an output error, which marks the whole batch as not saved, so none of the batch's messages was archived, including legitimate reports from other senders. In watch mode the batch was retried `max_unsaved_retries` times and then moved to `Archive/Unsaved`; in one-shot mode (cron) the retry counter restarts with every run, so the batch stayed in the inbox and its legitimate reports were sent again to every output that does not deduplicate on every run. An aggregate record whose count is outside 0 to 2,147,483,647 is now skipped with a "Could not parse record" warning (a report with only that one record becomes invalid, as with any other bad record), and an SMTP TLS session count outside that range makes the report invalid; the bound matches PostgreSQL's `INTEGER` count columns and the Elasticsearch and OpenSearch `integer` mapping of the SMTP TLS session counts, so it also closes the same rejection of an oversized session count in those outputs (the aggregate `message_count` is not in their mapping and is mapped dynamically as `long`). An SMTP TLS `sending-mta-ip` or `receiving-ip` that is not an IP address (or carries an IPv6 zone ID) is dropped from the failure details with a warning, which also keeps it out of the Elasticsearch and OpenSearch `ip` fields. An IPv6 zone ID (`fe80::1%eth0`), which the `inet` type cannot hold, is removed from an aggregate record's `source_ip` and a failure report's `Source-IP` (everything from the first `%`) with a warning naming the field, and the record or report is kept with the bare address. The PostgreSQL output replaces NUL with U+FFFD, the replacement character, in every text, text array and JSONB value it writes. Any other value PostgreSQL rejects (a `psycopg.DataError`) now raises the new `PostgreSQLRejectedReport`, a subclass of `PostgreSQLError` whose message names the report; the CLI logs it as an error and skips that report in the PostgreSQL output only, and the batch is saved and archived as usual. With `fail_on_output_error` set it is an output error that stops the run, as before. The in-memory `max_unsaved_retries` counter in one-shot mode is unchanged: the batches it still holds back are the ones an output could not save at all, such as an unreachable server, which the documentation describes as the intended safe direction. Reported by helixnebulatech. +- **The PostgreSQL output no longer sends the database password in plain text when an attacker declines TLS** ([GHSA-xvp7-7762-gx9g](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-xvp7-7762-gx9g), affects the PostgreSQL output in 11.2.2 and earlier). `PostgreSQLClient` connected without an `sslmode`, so libpq used its default, `prefer`: it asks the server for TLS and, when the answer is no, continues over plain TCP and answers whatever authentication request comes next. That answer arrives before any encryption, so an on-path attacker who declined TLS and then requested a clear-text password received the database password (CWE-319), and with it every stored report. The output now connects with `sslmode=require` by default, so a connection without TLS fails before the user name or password is sent. The new `[postgresql]` `sslmode` option (and the `sslmode` argument of `PostgreSQLClient`) sets any libpq mode, and `sslrootcert` names the CA certificate that `verify-ca` and `verify-full` check the server against. When `sslmode` is not set, an `sslmode` in `connection_string` or the `PGSSLMODE` environment variable is respected. A warning is logged at startup when a TCP host other than `localhost` or a loopback address is used with a mode that does not verify the server certificate (`require` with an existing `sslrootcert` file is not warned about, because libpq then verifies the certificate as `verify-ca` does). With a `connection_string`, nothing parsed from the string reaches the log or an error message: a connection failure is reported with a fixed message (plus the server's SQLSTATE code, when it sent one) instead of the driver's, and the startup warning names neither the server nor any other value from the string. libpq ends a URI's user name and password at the first `@` or `/`, so the rest of a password containing either becomes the host, port, database name, or a query parameter, which the warning and the driver's errors would otherwise repeat (`postgresql://u:SECRET/x@db/d` failed with `invalid integer value "SECRET" for connection option "port"`). The separate `host`, `port`, `user`, `password`, and `database` settings keep the driver's message, with URL credentials masked. As a convenience, a URI that libpq would parse that way (an unencoded `@` in its host list or database name, or in a query value before the first `/`, or an `@` after a host whose port is not a number) is refused with a fixed error before anything is logged or a connection is attempted; this recognizes the common mistake but is not what keeps the password out of the logs. Percent-encode `@` and `/` in a URI's user name or password (`%40`, `%2F`). A keyword/value connection string is not checked, because its password is a keyword of its own. Any other error while connecting, such as the `UnicodeEncodeError` psycopg raises with the whole connection string it built when the password holds a character it cannot encode, is replaced with a fixed message as well, and the CLI no longer chains the PostgreSQL client's error to the `RuntimeError` it raises. **Compatibility:** a server that offers no TLS at all now fails with libpq's "server does not support SSL, but SSL was required". Set `[postgresql] sslmode = disable` (or `prefer`) to allow it again, recommended only for a Unix socket or the loopback address. `require` still accepts any certificate, so a remote server should use `sslmode = verify-full` with `sslrootcert`. Reported by helixnebulatech. +- **An Elasticsearch or OpenSearch password containing `/`, `?` or `#` no longer appears in the startup error** ([GHSA-74xv-rmpx-2pjm](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-74xv-rmpx-2pjm), affects 11.2.2 and earlier; an incomplete fix for [GHSA-hjr3-rx4c-24cv](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-hjr3-rx4c-24cv)). 11.2.0 masked the credentials in `hosts` URLs logged with `--debug`, but not in the error raised when the client cannot be built. When the password in a `hosts` URL contains an unencoded `/`, `?` or `#`, the URL cannot be parsed, and the client library's error quotes the whole URL (Elasticsearch) or the start of the password (OpenSearch). parsedmarc repeated that text at ERROR level, without `--debug`, on the console, in `log_file` and in any log pipeline: `Output client error: Elasticsearch: Could not parse URL 'https://dmarc:@es.example:9200'` and `Output client error: OpenSearch: Port could not be cast to integer value as ''`. The error was also chained to the library's, so a failed SIGHUP reload, which logs the full traceback, printed it a second time. A `hosts` value that cannot be parsed now fails with a fixed message that names no part of it: "Elasticsearch: could not build a client from the hosts setting (details withheld because they may include credentials; URL-encode special characters in the password, or use the separate user and password settings)", and likewise for OpenSearch. Any other Elasticsearch or OpenSearch startup error keeps its text with URL credentials masked, and neither is chained to the original error any more. Reported by helixnebulatech. +- **The downloaded IP database is cached in a private per-user directory and checked before it is used** ([GHSA-5f56-8ph8-v9q5](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-5f56-8ph8-v9q5), affects 9.6.0 through 11.2.2). `load_ip_db()` cached the IPinfo Lite database it downloads at a fixed path in the shared system temporary directory, `/tmp/parsedmarc/ipinfo_lite.mmdb` on Linux. Another local user could create `/tmp/parsedmarc` first; parsedmarc accepted the existing directory, wrote the download through a symlink planted at `ipinfo_lite.mmdb.tmp`, and used whatever file was at `ipinfo_lite.mmdb` whenever the download was skipped or failed, which that user could force, with no check of its owner or content. That let them overwrite any file the parsedmarc user can write (its configuration, a Gmail `token.json`, an output file) with the download, or supply the MMDB file that decides every source IP's country and ASN and that `maxminddb` then parses. The database is now cached in `$XDG_CACHE_HOME/parsedmarc` or `~/.cache/parsedmarc` (`%LOCALAPPDATA%\parsedmarc\cache` on Windows), created with mode `0700` and used only if it is a real directory, not a symlink, that the user can write to and, on POSIX, owns and is not writable by its group or by others. Otherwise parsedmarc logs a warning and uses a new private temporary directory for that run, downloading the database again on every run, and never changes the permissions of a directory it did not create. The download is written to a new file created exclusively with `mkstemp()`, opened with `maxminddb` to check that it is a valid MMDB, and only then moved into place with an atomic `os.replace()`, which replaces a symlink rather than following it. A cached copy is used only if it is a regular file and, on POSIX, owned by the user; otherwise the bundled copy is used. The old `/tmp/parsedmarc/ipinfo_lite.mmdb` is no longer read, and can be removed by hand. Only multi-user hosts whose temporary directory is shared were exposed; the Docker image and systemd units with `PrivateTmp=` were not. Reported by helixnebulatech. +- **One aggregate report too large for the Logs Ingestion API no longer stops the Log Analytics output from publishing the batch** ([GHSA-rg4h-58fm-8pc9](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-rg4h-58fm-8pc9), affects the Log Analytics output in 11.2.2 and earlier). The Azure Monitor ingestion SDK packs whole reports into upload calls of up to 1 MiB and never splits a report, so a report whose JSON is larger than that is sent as a call of its own, and the Logs Ingestion API rejects any call over 1 MB. An aggregate report of a few thousand records is that large, while its gzipped XML is only a few kilobytes. The rejected upload raised an error before the failure and SMTP TLS streams were sent, and parsedmarc treated it as an output error: the whole batch, legitimate reports from other senders included, was retried and finally moved to the `Unsaved` folder. A report whose JSON is over 1,000,000 bytes is now left out with a warning that names it, and the other reports are uploaded. Each of the three streams is now published even when another fails; a failed upload is still reported as an output error, naming every stream that failed. Reported by helixnebulatech. + +### Added + +- `[postgresql]` `sslmode` and `sslrootcert` options (and `PostgreSQLClient` arguments) to choose the TLS mode of the PostgreSQL connection and the CA certificate that verifies the server. + ### Changed +- **mailsuite 2.3.7 or later is now required.** It fixes mailsuite's `parse_email()` keeping the carriage returns of a CRLF message's address headers, which left a trailing `\r` in the `from`, `sender` and `delivered-to` values, or added an extra entry to `to`, `cc`, `reply-to` and `bcc` when the value ended with a comma; empty values are now treated as absent for both CRLF and LF input ([seanthegeek/mailsuite#74](https://github.com/seanthegeek/mailsuite/issues/74)). parsedmarc parses report emails with its own code, not mailsuite's `parse_email()`. - **`parse_report_file()` now refuses an input larger than 32 MiB before reading it into memory** ([#949](https://github.com/domainaware/parsedmarc/issues/949)). The size cap (`MAX_DECOMPRESSED_REPORT_SIZE`) was applied only after the whole input had been read, so a path, a `bytes` object or a file object larger than the cap was held in memory first. The size of a path and the length of a `bytes`, `bytearray` or `memoryview` input are now checked up front, and a file object is read at most one byte past the cap (it is left positioned there). The input is rejected with `ParserError` and the same message as before (`the report is larger than 32 MiB`). A valid compressed report is smaller than its decompressed form, so the one cap applies to both. - **A report dated more than two days in the future is now rejected** ([#947](https://github.com/domainaware/parsedmarc/issues/947)). With Elasticsearch or OpenSearch output a report's own date names its dated index (`dmarc_aggregate-YYYY-MM-DD`), and nothing compared that date with the present, so a report with `4102444800` created an index for the year 2100. An aggregate report whose begin or end date (a report split per day names an index from each day it covers), an SMTP TLS report whose `start-datetime`, or a failure report whose `Arrival-Date` is more than 2 days ahead of the current time (`MAX_REPORT_FUTURE_DAYS`) is now rejected with `InvalidAggregateReport`, `InvalidSMTPTLSReport` or `InvalidFailureReport`, so a mailbox message is moved to the `Invalid` folder. The two days absorb a reporter's clock skew and time-zone mistakes; dates in the past are not bounded, so archives of old reports still parse. - **Python 3.10 is no longer supported, and Python 3.15 is.** Python 3.10 reached end of life in October 2026; `requires-python` is now `>=3.11`. Python 3.15 is now tested in CI and listed as supported. diff --git a/docs/source/usage.md b/docs/source/usage.md index a8836535..7dab32e3 100644 --- a/docs/source/usage.md +++ b/docs/source/usage.md @@ -187,6 +187,27 @@ The full set of configuration options is: bundled IPinfo Lite MMDB (env var: `PARSEDMARC_GENERAL_IPINFO_URL`). The pre-9.10 name `ip_db_url` is still accepted as a deprecated alias and logs a warning. + + :::{note} + The downloaded database is cached as `ipinfo_lite.mmdb` in a per-user + directory: `$XDG_CACHE_HOME/parsedmarc` (when `XDG_CACHE_HOME` is an + absolute path), otherwise `~/.cache/parsedmarc`, or + `%LOCALAPPDATA%\parsedmarc\cache` on Windows. parsedmarc creates the + directory with mode `0700` and uses it only if it is a real directory + (not a symlink) that the user can write to and, on Linux and macOS, + owns and is not writable by its group or by others; an existing + directory's permissions are never changed. Otherwise it logs a warning + and caches the download in a new private temporary directory that is + removed when parsedmarc exits, so the database is downloaded again on + every run. A download is cached only if it opens as a valid MMDB file, + and when the download fails (or `offline` or `always_use_local_files` + is set) the cached copy is used only if it is a regular file (not a + symlink) and, on Linux and macOS, owned by the user; otherwise the + bundled copy is used. Earlier releases cached the download at a shared + path in the system temporary directory + (`/tmp/parsedmarc/ipinfo_lite.mmdb` on Linux), which is no longer read; + its owner can delete it. + ::: - `ipinfo_api_token` - str: Optional [IPinfo Lite REST API] token. When set, IP lookups hit the API first for the freshest country/ASN data and fall back to the local MMDB on rate limit, quota exhaustion, or @@ -210,7 +231,12 @@ The full set of configuration options is: see [Encrypted DNS](#encrypted-dns) - `dns_test_address` - str: A dummy address used for the DNS pre-flight check that runs when `nameservers` is set (Default: `1.1.1.1`) - - `dns_timeout` - float: DNS timeout period in seconds (Default: `2.0`) + - `dns_timeout` - float: DNS timeout period in seconds (Default: `2.0`). + Reverse DNS lookups that fail or are slow (one second or more) are + limited to 120 seconds per aggregate report in total, and all of a + report's reverse DNS lookups, however fast, to 600 seconds; after + either limit, the report's remaining records are enriched from the + local IP database and reverse DNS map only, and a warning is logged. - `dns_retries` - int: Number of times to retry a DNS query after a timeout or other transient error (Default: `0`) - `debug` - bool: Print debugging messages @@ -221,7 +247,10 @@ The full set of configuration options is: reports (Default: `False`) :::{note} - This option only controls the process exit code. Retaining mailbox + This option only controls the process exit code, with one exception: + a report containing a value PostgreSQL rejects is logged and skipped + by the PostgreSQL output unless this option is set, and counts as an + output error when it is (see `postgresql` below). Retaining mailbox messages whose reports could not be saved is automatic and happens either way — see [Mailbox messages are only archived once the reports are saved](#mailbox-messages-are-only-archived-once-the-reports-are-saved). @@ -615,7 +644,7 @@ The full set of configuration options is: |---|---|---| | *"...needs permission to access resources in your organization that only an admin can grant"* / "Admin consent required" | A delegated auth method (`UsernamePassword`, `DeviceCode`) is authenticating with a scope (`Mail.ReadWrite` or `Mail.ReadWrite.Shared`) the tenant admin hasn't consented to yet. | Have an Entra ID admin grant consent: Azure Portal → **Enterprise Applications** → *your app* → **Permissions** → **Grant admin consent**, or `az ad app permission admin-consent --id `. This is separate from the `New-ApplicationAccessPolicy` step above, which only applies to app-only auth. | | `ErrorItemNotFound: ... Default folder Root not found` | `mailsuite` can resolve the well-known folders (`Inbox`, `Archive`, `Drafts`, `Sent Items`, `Deleted Items`, `Junk Email`) even when a mailbox's folder hierarchy hasn't fully provisioned, but a **custom, non-well-known** `reports_folder` name still fails to resolve on such a mailbox. | Point `reports_folder` at (or under) one of the six well-known folder names above, or sign into the (shared) mailbox once via Outlook/OWA to force Exchange to provision it, then retry. | - | `RuntimeError: Event loop is closed` | Historical bug, fixed in `mailsuite` 2.0.2. Not reachable with the `mailsuite>=2.2.2` this project requires. | Confirm your installed `mailsuite` version is current (`pip show mailsuite`); upgrade if it's somehow pinned below 2.0.2. | + | `RuntimeError: Event loop is closed` | Historical bug, fixed in `mailsuite` 2.0.2. Not reachable with the `mailsuite>=2.3.7` this project requires. | Confirm your installed `mailsuite` version is current (`pip show mailsuite`); upgrade if it's somehow pinned below 2.0.2. | | Invalid/rejected timestamp in the `since`/`receivedDateTime` filter | Historical bug (parsedmarc [#706](https://github.com/domainaware/parsedmarc/pull/706)/[#708](https://github.com/domainaware/parsedmarc/pull/708)): older versions appended a spurious `Z` to an already-UTC-offset ISO timestamp. Fixed since parsedmarc 9.5.1/9.5.5. | Upgrade parsedmarc if you're on a version older than 9.5.5. | ::: - `elasticsearch` @@ -777,8 +806,50 @@ The full set of configuration options is: `host`, `port`, `user`, `password`, and `database` settings and do not put the password in the connection string: a malformed connection string can make the driver echo it back in an error - message. parsedmarc replaces such errors with a fixed message in - its log, but cannot stop the driver from producing them. + message. With a connection string, parsedmarc withholds the + driver's message for a connection failure (keeping only the + server's SQLSTATE code, when there is one) and does not name the + server in its startup warning, because either could repeat part of + a password; the separate settings give full diagnostics. An `@` or + `/` in a URI's user name or password must be percent-encoded (`@` + as `%40`, `/` as `%2F`): libpq ends the user name and password at + the first `@` or `/`, so the rest of the password would be parsed + as the host, port, database name, or a query parameter. As a + convenience, a URI that libpq would parse that way is refused with a + fixed error before anything is logged: one with an unencoded `@` in + its host list or database name, or in a query value written before + the first `/`, or with an `@` after a host whose port is not a + number. An `@` in such a place is written `%40` (an abstract socket + host as `%40sock`, a database name `d@x` as `d%40x`); an `@` in a + query value after a `/`, such as a certificate path, is accepted as + written. A keyword/value connection string is not checked: its + password is a keyword of its own, and libpq does not percent-decode + it. + - `sslmode` - str: The libpq TLS mode of the connection, one of + `disable`, `allow`, `prefer`, `require`, `verify-ca`, or + `verify-full` (Default: `require`). It applies with `host` and with + `connection_string`. + - `require` encrypts the connection but does not check the server's + identity, so a server that offers no TLS is refused, but anyone + who can impersonate the server still receives the password. + - `verify-ca` also checks that the server's certificate was issued + by the CA in `sslrootcert`, and `verify-full` also checks that it + names the host. Use `verify-full` with `sslrootcert` for any + remote server. + - `disable`, `allow`, and `prefer` can let the connection, and the + password, travel without encryption. Use them only for a server + without TLS on a trusted network. A Unix socket or the loopback + address does not need TLS. + + When `sslmode` is not set, an `sslmode=` inside `connection_string` + and the `PGSSLMODE` environment variable are respected, in that + order, before the default. A warning is logged at startup when the + host is a TCP host other than `localhost` or a loopback address and + the mode is below `verify-ca`, except `require` with an existing + `sslrootcert` file, which libpq verifies as `verify-ca` does. + - `sslrootcert` - str: Path to the CA certificate file that + `verify-ca` and `verify-full` check the server's certificate + against (Optional; `~` and environment variables are expanded) The PostgreSQL backend is an optional extra. Install it with `pip install "parsedmarc[postgresql]"` (it pulls in `psycopg`); the @@ -798,11 +869,13 @@ The full set of configuration options is: ```ini [postgresql] - host = localhost + host = db.example.com port = 5432 user = parsedmarc password = secret database = parsedmarc + sslmode = verify-full + sslrootcert = /etc/ssl/certs/db-ca.pem ``` Or using a DSN/URI, with the password supplied separately (for @@ -811,9 +884,15 @@ The full set of configuration options is: ```ini [postgresql] - connection_string = postgresql://parsedmarc@localhost/parsedmarc + connection_string = postgresql:///parsedmarc?host=/var/run/postgresql ``` + That example connects over the local Unix socket, which needs no TLS. A + TCP connection to `localhost` (`postgresql://parsedmarc@localhost/parsedmarc`) + is refused under the default `sslmode = require` unless the server has + TLS enabled; for a server without TLS, set `sslmode = disable`, which + is safe only for the loopback address, as described under `sslmode`. + Saving parsed data to PostgreSQL is controlled by the `[general]` options `save_aggregate`, `save_failure`, and `save_smtp_tls` (`save_forensic` is still accepted as a deprecated alias for @@ -822,6 +901,13 @@ The full set of configuration options is: SMTP TLS reports) or no data will be written to PostgreSQL, even if this section is configured. + A report with a value PostgreSQL rejects (the server raises a data + error, so the connection and the tables are fine) is logged and + skipped by the PostgreSQL output, and the rest of the batch is saved + and archived as usual; the other outputs still receive that report. Set + `fail_on_output_error` to treat it as an output error that stops the + run instead. + - `s3` Requires the `s3` extra: `pip install "parsedmarc[s3]"` @@ -978,6 +1064,9 @@ The full set of configuration options is: - `dcr_failure_stream` - str: The stream name for the failure reports in the DCR - `dcr_smtp_tls_stream` - str: The stream name for the SMTP TLS reports in the DCR + A report whose JSON is larger than 1,000,000 bytes, the limit of one Logs Ingestion API call, is not published to Log Analytics, and a warning naming it is logged. + The client library packs several reports into one call of up to 1 MiB (1,048,576 bytes), slightly over the API's 1 MB, so a call of several reports that are each under the limit can still be rejected; that failure is reported as an output error, and the batch is retried. + :::{note} Information regarding the setup of the Data Collection Rule can be found [in the Azure documentation](https://learn.microsoft.com/en-us/azure/azure-monitor/logs/tutorial-logs-ingestion-portal). ::: @@ -1176,7 +1265,9 @@ destinations accepted it would still permanently lose the data for the one that didn't. It also applies regardless of `fail_on_output_error`, which only controls the process exit code. Messages that could not be parsed at all carry no report data, so they are filed under `Invalid` (or deleted per -`delete_invalid`) as usual. +`delete_invalid`) as usual. A report whose value PostgreSQL rejects is +not a failure of the destination: the PostgreSQL output logs and skips it +unless `fail_on_output_error` is set (see `postgresql` above). A destination that is broken rather than briefly unavailable would otherwise be retried forever, so retries are capped. Once a message's batch @@ -1713,7 +1804,7 @@ could not be opened when parsedmarc started (a warning at startup) is retried by the next reload even if the setting is unchanged. (One thing an aborted reload does not put back: if it got as far as downloading a new IP database from a changed `ipinfo_url`, that file stays in the -shared cache directory. Which database parsedmarc *uses* is unchanged, +cache directory. Which database parsedmarc *uses* is unchanged, and the cached file is the same one the next restart would download.) Elasticsearch and OpenSearch are the exception to "cannot be reached": their clients connect lazily and the index migration only warns when the diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index dcf7aec5..0e5b07ba 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -10,6 +10,8 @@ import email.errors import email.message import email.utils import functools +import hashlib +import ipaddress import json import logging import mailbox @@ -18,6 +20,7 @@ import quopri import re import shutil import tempfile +import time import traceback import xml.parsers.expat as expat import zipfile @@ -26,6 +29,7 @@ from base64 import b64decode from collections import deque from collections.abc import Callable, Sequence from csv import DictWriter +from dataclasses import dataclass, field from datetime import date, datetime, timedelta, timezone, tzinfo from io import BytesIO, StringIO from typing import ( @@ -52,6 +56,10 @@ from parsedmarc.config import ( from parsedmarc.constants import ( DEFAULT_DNS_MAX_RETRIES, DEFAULT_DNS_TIMEOUT, + MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT, + MAX_REVERSE_DNS_SECONDS_PER_REPORT, + SLOW_REVERSE_DNS_SECONDS, + MAX_MESSAGE_COUNT, MAX_NORMALIZED_RECORDS, MAX_REPORT_FUTURE_DAYS, MAX_REPORT_SPAN_DAYS, @@ -78,6 +86,7 @@ from parsedmarc.types import ( SMTPTLSReport, ) from parsedmarc.utils import ( + IPAddressInfo, convert_outlook_msg, get_base_domain, get_filename_safe_string, @@ -751,11 +760,143 @@ def _scalarize_report(report: dict[str, Any]) -> None: _scalar_fields(policy, _POLICY_PUBLISHED_SCALARS) +def _strip_zone_id(value: str, field_name: str) -> str: + """Return an IP address without its IPv6 zone ID, if it has one. + + A zone ID (``fe80::1%eth0``) is local to the reporting host, and the + PostgreSQL ``inet`` type cannot hold one (GHSA-fxr7-7h9q-v949). ``%`` + appears in no other form of an IP address, so everything from the + first ``%`` is removed (``fe80::1%25eth0`` and ``fe80::1%`` also give + ``fe80::1``), and a warning naming the field is logged. The remaining + text is not checked here; the caller's own handling of an invalid IP + address applies to it. + + Args: + value: The address, as the report gives it + field_name: The report field that gave it, for the warning + + Returns: + str: ``value`` up to its first ``%`` + """ + address, sep, _zone = value.partition("%") + if sep: + logger.warning( + f"Removed the IPv6 zone ID from {field_name}: {sanitize_for_log(value)}" + ) + return address + + +@dataclass +class _ReverseDNSBudget: + """Per-report state that bounds the time an aggregate report spends in + reverse DNS lookups (GHSA-rwfh-ww9c-jm9m). + + Attributes: + seconds_left: Seconds of failed or slow lookups the report may still + spend (``MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT``) + total_seconds_left: Seconds of lookups of any kind the report may + still spend (``MAX_REVERSE_DNS_SECONDS_PER_REPORT``) + memo: Source information already produced in this report, by + lowercase IP address, so each distinct IP is looked up at most + once per report + exhausted: Whether either budget is spent; the report's remaining + IPs are then enriched from the local IP database and reverse + DNS map only + """ + + seconds_left: float = field( + default_factory=lambda: MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT + ) + total_seconds_left: float = field( + default_factory=lambda: MAX_REVERSE_DNS_SECONDS_PER_REPORT + ) + memo: dict[str, IPAddressInfo] = field(default_factory=dict) + exhausted: bool = False + + +_REVERSE_DNS_BUDGET_FALLBACK = ( + "; the remaining records are enriched from the local IP database and " + "reverse DNS map only" +) + + +def _enrich_record_source( + source_ip: str, config: ParserConfig, budget: _ReverseDNSBudget +) -> IPAddressInfo: + """Return the source information for one record's ``source_ip``. + + A record's source IP is chosen by the report's sender, and a reverse + zone that never answers costs the whole DNS lifetime (8 s with the + default nameservers), so the time of every online lookup is charged to + ``budget``. The time of a lookup that returns no PTR, or that takes at + least ``SLOW_REVERSE_DNS_SECONDS`` even though it found one (the sender + may control the reverse zone and answer every query just before the + timeout), is charged to the failed-or-slow budget; the time of every + lookup, however fast, is charged to the total budget as well, because a + zone that answers just under the slow threshold is otherwise never + charged and the number of records is not bounded. A lookup answered + from the IP address cache is charged too, but takes microseconds. Once + either budget is spent, one warning names the limit reached, and the + remaining IPs are enriched from the local IP database and reverse DNS + map only; those results are not cached, because the IP may well have a + PTR. An IP already seen in this report is answered from ``budget.memo`` + and not charged. + """ + ip = source_ip.lower() + if ip in budget.memo: + return budget.memo[ip] + common: dict[str, Any] = dict( + ip_db_path=config.ip_db_path, + always_use_local_files=config.always_use_local_files, + reverse_dns_map_path=config.reverse_dns_map_path, + reverse_dns_map_url=config.reverse_dns_map_url, + reverse_dns_map=config.reverse_dns_map, + nameservers=config.nameservers, + timeout=config.dns_timeout, + retries=config.dns_retries, + psl_overrides_path=config.psl_overrides_path, + psl_overrides_url=config.psl_overrides_url, + ) + if budget.exhausted or config.offline: + result = get_ip_address_info( + ip, + offline=True, + cache=None if budget.exhausted else config.ip_address_cache, + **common, + ) + else: + start = time.monotonic() + result = get_ip_address_info( + ip, offline=False, cache=config.ip_address_cache, **common + ) + elapsed = time.monotonic() - start + budget.total_seconds_left -= elapsed + if result["reverse_dns"] is None or elapsed >= SLOW_REVERSE_DNS_SECONDS: + budget.seconds_left -= elapsed + if budget.seconds_left <= 0: + budget.exhausted = True + logger.warning( + "Reverse DNS lookups for this report failed or were slow for " + f"{MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT:.0f} seconds" + + _REVERSE_DNS_BUDGET_FALLBACK + ) + elif budget.total_seconds_left <= 0: + budget.exhausted = True + logger.warning( + "Reverse DNS lookups for this report took more than " + f"{MAX_REVERSE_DNS_SECONDS_PER_REPORT:.0f} seconds" + + _REVERSE_DNS_BUDGET_FALLBACK + ) + budget.memo[ip] = result + return result + + def _parse_report_record( record: dict[str, Any], *, config: ParserConfig, is_rfc_9990: bool = False, + dns_budget: _ReverseDNSBudget | None = None, ) -> dict[str, Any]: """ Converts a record from a DMARC aggregate report into a more consistent @@ -766,9 +907,16 @@ def _parse_report_record( config (ParserConfig): Parsing and enrichment options, plus caches is_rfc_9990 (bool): Whether the enclosing report was detected as RFC 9990-shaped, for RFC 9990-aware validation warnings + dns_budget (_ReverseDNSBudget | None): The enclosing report's + reverse DNS budget; ``None`` gives the record a budget of its own Returns: dict: The converted record + + Raises: + ValueError: If the source IP is empty or not an IP address (after + its IPv6 zone ID, if any, is removed), or the count is not an + integer between 0 and ``MAX_MESSAGE_COUNT`` """ record = record.copy() new_record: dict[str, Any] = {} @@ -777,25 +925,19 @@ def _parse_report_record( _scalar_fields(record["row"], ("source_ip",)) if record["row"]["source_ip"] is None: raise ValueError("Source IP address is empty") - new_record_source = get_ip_address_info( - record["row"]["source_ip"], - cache=config.ip_address_cache, - ip_db_path=config.ip_db_path, - always_use_local_files=config.always_use_local_files, - reverse_dns_map_path=config.reverse_dns_map_path, - reverse_dns_map_url=config.reverse_dns_map_url, - reverse_dns_map=config.reverse_dns_map, - offline=config.offline, - nameservers=config.nameservers, - timeout=config.dns_timeout, - retries=config.dns_retries, - psl_overrides_path=config.psl_overrides_path, - psl_overrides_url=config.psl_overrides_url, - ) - new_record["source"] = new_record_source + source_ip = _strip_zone_id(record["row"]["source_ip"], "source_ip") # A with no text of its own (child elements only, or repeated) # becomes "", so int() raises the ValueError of a non-numeric count. - new_record["count"] = int(_scalar(record["row"]["count"]) or "") + count = int(_scalar(record["row"]["count"]) or "") + # A count no output's 32-bit integer column can hold is rejected here, + # not by the output (GHSA-fxr7-7h9q-v949), and before the reverse DNS + # lookup, which a record that is dropped anyway should not wait on. + if not 0 <= count <= MAX_MESSAGE_COUNT: + raise ValueError(f"count {count} is outside 0..{MAX_MESSAGE_COUNT}") + if dns_budget is None: + dns_budget = _ReverseDNSBudget() + new_record["source"] = _enrich_record_source(source_ip, config, dns_budget) + new_record["count"] = count policy_evaluated = record["row"]["policy_evaluated"].copy() _scalar_fields(policy_evaluated, ("disposition", "dkim", "spf")) new_policy_evaluated: dict[str, Any] = { @@ -1019,17 +1161,26 @@ def _smtp_tls_mx_host_patterns(value: Any, field: str) -> list[str]: def _smtp_tls_count(value: Any, field: str) -> int | None: - """Return ``value`` if it is an integer or null. + """Return ``value`` if it is null or an integer from 0 to + ``MAX_MESSAGE_COUNT``. RFC 8460 §4.4 encodes each session count as "an integer, encoded as a JSON number". ``null`` is tolerated as an unreported count. A JSON ``true``/``false`` is rejected even though ``bool`` subclasses ``int``. + A count outside ``0..MAX_MESSAGE_COUNT`` is rejected because the + PostgreSQL, Elasticsearch and OpenSearch outputs store counts as 32-bit + integers (GHSA-fxr7-7h9q-v949). Raises ``InvalidSMTPTLSReport`` + otherwise. """ - if value is None or (isinstance(value, int) and not isinstance(value, bool)): + if value is None: return value - raise InvalidSMTPTLSReport( - f"{field} must be an integer, not {type(value).__name__}" - ) + if not isinstance(value, int) or isinstance(value, bool): + raise InvalidSMTPTLSReport( + f"{field} must be an integer, not {type(value).__name__}" + ) + if not 0 <= value <= MAX_MESSAGE_COUNT: + raise InvalidSMTPTLSReport(f"{field} must be between 0 and {MAX_MESSAGE_COUNT}") + return value def _smtp_tls_datetime(value: Any, field: str) -> str: @@ -1040,11 +1191,12 @@ def _smtp_tls_datetime(value: Any, field: str) -> str: Elasticsearch, OpenSearch and S3 outputs (a UTC ``datetime``) and the Splunk output (a UNIX timestamp) make later. dateutil also accepts a partial date such as ``"2016"``, which PostgreSQL may still reject; that - failure is raised as ``PostgreSQLError`` and recorded as an output - error. A ``start-datetime`` more than ``MAX_REPORT_FUTURE_DAYS`` days in - the future is also rejected, because it names the dated index (#947); - ``end-datetime`` is not checked. Raises ``InvalidSMTPTLSReport`` - otherwise. + failure is raised as ``PostgreSQLRejectedReport``, which skips the + report in the PostgreSQL output only (an output error with + ``fail_on_output_error``). A ``start-datetime`` more than + ``MAX_REPORT_FUTURE_DAYS`` days in the future is also rejected, because + it names the dated index (#947); ``end-datetime`` is not checked. Raises + ``InvalidSMTPTLSReport`` otherwise. """ value = _smtp_tls_string(value, field) if len(value) > _SMTP_TLS_MAX_DATETIME_LENGTH: @@ -1089,6 +1241,34 @@ def _parse_smtp_tls_failure_details(failure_details: dict[str, Any]): if value is not None: new_failure_details[dest_key] = _smtp_tls_string(value, src_key) + # RFC 8460 §4.4 defines both as the IP address of the sending or + # receiving MTA, and both are optional. Drop a value that is not an + # IP address, which the PostgreSQL inet column and the + # Elasticsearch/OpenSearch ip mappings reject (GHSA-fxr7-7h9q-v949). + # An IPv6 zone ID (fe80::1%eth0) is local to the reporting host and + # the inet type cannot hold one, and ipaddress.ip_address() accepts + # it, so such a value is dropped as well. The aggregate and failure + # paths strip the zone ID instead (_strip_zone_id()), because the + # source IP is required there; these fields are optional, so + # dropping one loses nothing a report needs. + for src_key, dest_key in ( + ("sending-mta-ip", "sending_mta_ip"), + ("receiving-ip", "receiving_ip"), + ): + value = new_failure_details.get(dest_key) + if value is None: + continue + try: + if "%" in value: + raise ValueError("zone ID") + ipaddress.ip_address(value) + except ValueError: + logger.warning( + f"Ignoring {src_key} that is not an IP address: " + f"{sanitize_for_log(value)}" + ) + del new_failure_details[dest_key] + # RFC 8460 §4.4 names the JSON key "additional-information" (its # value is described as an "additional-info-uri"). Some reporters # instead send the non-RFC key "additional-info-uri" directly; fall @@ -1583,10 +1763,15 @@ def _parse_report_records( end_dt: datetime, normalize: bool, keep_alive: Callable | None, + dns_budget: _ReverseDNSBudget | None = None, ) -> list[dict[str, Any]]: """Parse an aggregate report's ```` elements (a list, or one record) into output records, split per day when ``normalize`` is set. - A record of a list that fails to parse is skipped with a warning.""" + A record of a list that fails to parse is skipped with a warning. + ``dns_budget`` is shared by every record (``None`` creates one); see + :class:`_ReverseDNSBudget`.""" + if dns_budget is None: + dns_budget = _ReverseDNSBudget() records: list[dict[str, Any]] = [] if type(xml_records) is list: for i in range(len(xml_records)): @@ -1599,6 +1784,7 @@ def _parse_report_records( xml_records[i], config=config, is_rfc_9990=is_rfc_9990, + dns_budget=dns_budget, ) _append_parsed_record( parsed_record=report_record, @@ -1615,6 +1801,7 @@ def _parse_report_records( xml_records, config=config, is_rfc_9990=is_rfc_9990, + dns_budget=dns_budget, ) _append_parsed_record( parsed_record=report_record, @@ -1898,6 +2085,7 @@ def parse_aggregate_report_xml( end_dt=end_dt, normalize=normalize_timespan, keep_alive=keep_alive, + dns_budget=_ReverseDNSBudget(), ) new_report["records"] = records @@ -2472,7 +2660,9 @@ def parse_failure_report( arrival_utc = arrival_utc.strftime("%Y-%m-%d %H:%M:%S") parsed_report["arrival_date_utc"] = arrival_utc - ip_address = re.split(r"\s", parsed_report["source_ip"]).pop(0) + ip_address = _strip_zone_id( + re.split(r"\s", parsed_report["source_ip"]).pop(0), "Source-IP" + ) parsed_report_source = get_ip_address_info( ip_address, cache=cfg.ip_address_cache, @@ -3327,7 +3517,10 @@ def _aggregate_report_dedup_key(report: AggregateReport) -> str: domain. ASCII domain letters are compared without case per RFC 4343 §3, without changing the domain in the report itself. A JSON array encodes the three fields without the ambiguity of a delimiter that a field could - contain. + contain, and the key is the SHA-256 hex digest of that JSON, so it is 64 + characters long whatever the sender puts in the three fields + (GHSA-9fw6-mwqr-p43m: the JSON itself was the key, so a report with a + multi-megabyte Report-ID cost that much memory in the cache). Every writer of the shared cache must build its key here -- the mbox and mailbox classifier and the CLI's direct-file path -- or a report read @@ -3340,7 +3533,8 @@ def _aggregate_report_dedup_key(report: AggregateReport) -> str: # The existing XML parser returns None for an empty . if isinstance(report_domain, str): report_domain = report_domain.translate(_ASCII_UPPER_TO_LOWER) - return json.dumps((report_org, report_id, report_domain)) + encoded = json.dumps((report_org, report_id, report_domain)) + return hashlib.sha256(encoded.encode("utf-8")).hexdigest() def _classify_parsed_email( diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py index 1e62695b..2836646c 100644 --- a/parsedmarc/cli.py +++ b/parsedmarc/cli.py @@ -203,6 +203,53 @@ def _missing_extra_hint(section: str, extra: str) -> str: ) +class _HostsSettingError(RuntimeError): + """A search client could not be built from its ``hosts`` setting. + + The message is fixed and carries none of the SDK's text; see + :func:`_set_search_hosts`. + """ + + +def _set_search_hosts(label: str, set_hosts, hosts, **kwargs): + """Call a search output's ``set_hosts()`` without leaking its hosts. + + When a password in a ``hosts`` URL contains an unencoded ``/``, ``?`` + or ``#``, the SDK fails to parse the URL and raises a ``ValueError`` + that quotes the URL or the start of the password + (GHSA-74xv-rmpx-2pjm): elastic_transport's ``url_to_node_config()`` + (``elastic_transport/client_utils.py``) raises + ``ValueError("Could not parse URL ''")``, and opensearch-py's + ``_normalize_hosts()`` (``opensearchpy/client/utils.py``) reads + ``urllib.parse``'s ``port``, which raises ``ValueError("Port could not + be cast to integer value as ''")``. Any such error is + replaced by one with a fixed message, raised outside the ``except`` + block so the original is neither its cause nor its context. + + Args: + label (str): The output's display name, e.g. ``"Elasticsearch"``. + set_hosts: The output module's ``set_hosts`` function. + hosts (str | list[str]): The ``hosts`` setting. + **kwargs: Passed through to ``set_hosts``. + + Returns: + The client ``set_hosts`` returned. + + Raises: + _HostsSettingError: If ``set_hosts`` raised a ``ValueError``. + """ + try: + return set_hosts(hosts, **kwargs) + except ValueError: + pass + raise _HostsSettingError( + f"{label}: could not build a client from the hosts setting (details " + "withheld because they may include credentials; URL-encode special " + "characters in the password, or use the separate user and password " + "settings)" + ) + + def _normalize_graph_auth_method(value: str) -> str: """Return the canonical :class:`AuthMethod` member name for *value*. @@ -1556,6 +1603,15 @@ def _parse_config(config: ConfigParser, opts): "host (or connection_string) setting missing from the " "postgresql config section" ) + if "sslmode" in pg_config: + sslmode = pg_config["sslmode"].strip().lower() + if sslmode not in postgres.SSL_MODES: + raise ConfigurationError( + f"postgresql sslmode must be one of {', '.join(postgres.SSL_MODES)}" + ) + opts.postgresql_sslmode = sslmode + if "sslrootcert" in pg_config: + opts.postgresql_sslrootcert = _expand_path(pg_config["sslrootcert"]) if "syslog" in config.sections(): syslog_config = config["syslog"] @@ -2121,8 +2177,13 @@ def _build_output_clients(opts, clients, index_prefix_domain_map=None): Raises: ConfigurationError: If a required output client cannot be created. - RuntimeError: If constructing an output client fails, chained to the - error the SDK raised. + RuntimeError: If constructing an output client fails, naming the + output. For outputs whose SDK errors can carry credentials + (Elasticsearch, OpenSearch and PostgreSQL), the SDK's original + error is not chained, and its text has URL credentials masked, + or is withheld entirely when the ``hosts`` setting cannot be + parsed (GHSA-74xv-rmpx-2pjm) and, for PostgreSQL, whenever + ``connection_string`` is set (GHSA-xvp7-7762-gx9g). """ # Each check below is deliberately outside the try/except that wraps # its constructor: those handlers re-raise everything as RuntimeError, @@ -2163,11 +2224,24 @@ def _build_output_clients(opts, clients, index_prefix_domain_map=None): user=opts.postgresql_user, password=opts.postgresql_password, database=opts.postgresql_database, + sslmode=opts.postgresql_sslmode, + sslrootcert=opts.postgresql_sslrootcert, ) pg_client.create_tables() clients["postgresql_client"] = pg_client - except Exception as e: - raise RuntimeError(f"PostgreSQL: {e}") from e + # A PostgreSQLError's text is built by postgres.py: a fixed message, the + # driver's text with URL credentials masked, or, with a connection + # string, withheld (GHSA-xvp7-7762-gx9g). Any other error is replaced, + # since its text can quote a setting. Neither is chained, because the + # SIGHUP reload path logs it with logger.exception, which would print a + # chained original in full. + except postgres.PostgreSQLError as e: + raise RuntimeError(f"PostgreSQL: {e}") from None + except Exception: + raise RuntimeError( + "PostgreSQL: the client could not be created (details withheld " + "because they may include the password)" + ) from None try: if opts.syslog_server: @@ -2354,7 +2428,9 @@ def _build_output_clients(opts, clients, index_prefix_domain_map=None): if opts.elasticsearch_timeout is not None else 60.0 ) - elasticsearch_connection = elastic.set_hosts( + elasticsearch_connection = _set_search_hosts( + "Elasticsearch", + elastic.set_hosts, opts.elasticsearch_hosts, use_ssl=opts.elasticsearch_ssl, ssl_cert_path=opts.elasticsearch_ssl_cert_path, @@ -2382,8 +2458,17 @@ def _build_output_clients(opts, clients, index_prefix_domain_map=None): clients["elasticsearch"] = _ElasticsearchHandle( elasticsearch_connection ) + # A rejected hosts value already has its fixed, secret-free message + # (GHSA-74xv-rmpx-2pjm). Any other error is wrapped with URL + # credentials masked, and from None, because the SIGHUP reload path + # logs it with logger.exception, which would print a chained + # original in full. + except _HostsSettingError: + raise except Exception as e: - raise RuntimeError(f"Elasticsearch: {e}") from e + raise RuntimeError( + f"Elasticsearch: {redact_url_credentials(str(e))}" + ) from None try: if opts.opensearch_hosts: @@ -2425,7 +2510,9 @@ def _build_output_clients(opts, clients, index_prefix_domain_map=None): if opts.opensearch_timeout is not None else 60.0 ) - opensearch_connection = opensearch.set_hosts( + opensearch_connection = _set_search_hosts( + "OpenSearch", + opensearch.set_hosts, opts.opensearch_hosts, use_ssl=opts.opensearch_ssl, ssl_cert_path=opts.opensearch_ssl_cert_path, @@ -2453,8 +2540,13 @@ def _build_output_clients(opts, clients, index_prefix_domain_map=None): legacy_fo_indexes=os_legacy_fo_indexes, ) clients["opensearch"] = _OpenSearchHandle(opensearch_connection) + # See the Elasticsearch block above (GHSA-74xv-rmpx-2pjm). + except _HostsSettingError: + raise except Exception as e: - raise RuntimeError(f"OpenSearch: {e}") from e + raise RuntimeError( + f"OpenSearch: {redact_url_credentials(str(e))}" + ) from None return clients @@ -2489,8 +2581,13 @@ def _init_output_clients(opts, index_prefix_domain_map=None): Raises: ConfigurationError: If a required output client cannot be created. - RuntimeError: If constructing an output client fails, chained to the - error the SDK raised. + RuntimeError: If constructing an output client fails, naming the + output. For outputs whose SDK errors can carry credentials + (Elasticsearch, OpenSearch and PostgreSQL), the SDK's original + error is not chained, and its text has URL credentials masked, + or is withheld entirely when the ``hosts`` setting cannot be + parsed (GHSA-74xv-rmpx-2pjm) and, for PostgreSQL, whenever + ``connection_string`` is set (GHSA-xvp7-7762-gx9g). """ previous_search_state = _search_alias_snapshot() clients: dict[str, Any] = {} @@ -2788,6 +2885,14 @@ def _main(): pg_client.save_aggregate_report_to_postgresql(report) except postgres.AlreadySaved as warning: logger.warning(warning.__str__()) + except postgres.PostgreSQLRejectedReport as error_: + # PostgreSQL rejected a value of this one report; the + # connection is fine, so it does not hold the batch + # (GHSA-fxr7-7h9q-v949). + if opts.fail_on_output_error: + log_output_error("PostgreSQL", error_.__str__()) + else: + logger.error(error_.__str__()) except postgres.PostgreSQLError as error_: log_output_error("PostgreSQL", error_.__str__()) except Exception as error_: @@ -2897,6 +3002,14 @@ def _main(): pg_client.save_failure_report_to_postgresql(report) except postgres.AlreadySaved as warning: logger.warning(warning.__str__()) + except postgres.PostgreSQLRejectedReport as error_: + # PostgreSQL rejected a value of this one report; the + # connection is fine, so it does not hold the batch + # (GHSA-fxr7-7h9q-v949). + if opts.fail_on_output_error: + log_output_error("PostgreSQL", error_.__str__()) + else: + logger.error(error_.__str__()) except postgres.PostgreSQLError as error_: log_output_error("PostgreSQL", error_.__str__()) except Exception as error_: @@ -3006,6 +3119,14 @@ def _main(): pg_client.save_smtp_tls_report_to_postgresql(report) except postgres.AlreadySaved as warning: logger.warning(warning.__str__()) + except postgres.PostgreSQLRejectedReport as error_: + # PostgreSQL rejected a value of this one report; the + # connection is fine, so it does not hold the batch + # (GHSA-fxr7-7h9q-v949). + if opts.fail_on_output_error: + log_output_error("PostgreSQL", error_.__str__()) + else: + logger.error(error_.__str__()) except postgres.PostgreSQLError as error_: log_output_error("PostgreSQL", error_.__str__()) except Exception as error_: @@ -3382,6 +3503,8 @@ def _main(): postgresql_password=None, postgresql_database=None, postgresql_connection_string=None, + postgresql_sslmode=None, + postgresql_sslrootcert=None, fail_on_output_error=False, ) @@ -4192,7 +4315,7 @@ def _main(): # db path/URL in the config take effect. Assigns # parsedmarc.utils._IP_DB_PATH, which the utils snapshot # covers, and -- when the download succeeds -- rewrites the - # shared cache file, which nothing can cover. Last but one + # cached download, which nothing can cover. Last but one # for that reason. load_ip_db( always_use_local_file=new_opts.always_use_local_files, @@ -4224,8 +4347,8 @@ def _main(): # it is put back here, in that order. # # One side effect is outside them and cannot be undone: a - # load_ip_db() whose download succeeds rewrites the shared - # cache file at /parsedmarc/ipinfo_lite.mmdb before it + # load_ip_db() whose download succeeds rewrites the cached + # ipinfo_lite.mmdb in the per-user cache directory before it # assigns _IP_DB_PATH. Phase 1 runs that step last but one # so that as little as possible can fail behind it, and the # file it leaves is the same refresh startup performs on diff --git a/parsedmarc/config.py b/parsedmarc/config.py index 8810cd30..d2b9fe87 100644 --- a/parsedmarc/config.py +++ b/parsedmarc/config.py @@ -23,6 +23,8 @@ circular, since ``parsedmarc/__init__.py`` imports from this module. from __future__ import annotations +import time +from collections import OrderedDict from dataclasses import dataclass, field, fields from expiringdict import ExpiringDict @@ -36,9 +38,51 @@ def _new_ip_address_cache() -> ExpiringDict: return ExpiringDict(max_len=10000, max_age_seconds=14400) +class SeenReportCache(ExpiringDict): + """An :class:`~expiringdict.ExpiringDict` that drops expired entries + whenever an entry is added. + + ``ExpiringDict`` removes an expired entry only when that same key is + read again (``in``, ``[]``, ``get()``), and otherwise only drops its + oldest entry once it holds ``max_len`` entries. The seen-aggregate-report + cache is read only with the key of the report being classified, so the + key of a report that is never seen again stays in memory for the life of + the process, not for ``max_age`` seconds (GHSA-9fw6-mwqr-p43m: in watch + mode every distinct report was kept indefinitely). + + ``ExpiringDict`` stores entries in insertion order as ``(value, + set_time)`` tuples, so the expired entries are at the head. Before + adding an entry, this subclass removes entries from the head until it + reaches one that has not expired, which keeps the cache bounded by the + number of reports seen in the last ``max_age`` seconds. The removal + uses the plain :class:`~collections.OrderedDict` methods, as + ``ExpiringDict`` does internally, so it neither re-enters the expiring + accessors nor changes their behaviour. A key that is set again while + still stored is moved to the tail first, because ``OrderedDict`` would + otherwise refresh its time in place and leave a fresh entry at the head + that stops the sweep before the expired entries behind it. + """ + + def __setitem__(self, key, value, set_time=None) -> None: + with self.lock: + now = time.time() + while len(self): + first_key = next(iter(OrderedDict.keys(self))) + if now - OrderedDict.__getitem__(self, first_key)[1] < self.max_age: + break + OrderedDict.__delitem__(self, first_key) + if OrderedDict.__contains__(self, key): + self.move_to_end(key) + super().__setitem__(key, value, set_time) + + def _new_seen_aggregate_report_ids() -> ExpiringDict: - """Build a fresh seen-aggregate-report-ID cache (1 hour expiry).""" - return ExpiringDict(max_len=100000000, max_age_seconds=3600) + """Build a fresh seen-aggregate-report-ID cache (1 hour expiry). + + Expired entries are removed on each insert (see :class:`SeenReportCache`), + and ``max_len`` is a hard ceiling on the number of entries kept. + """ + return SeenReportCache(max_len=100_000, max_age_seconds=3600) # Canonical default caches. parsedmarc/__init__.py re-exports these three diff --git a/parsedmarc/constants.py b/parsedmarc/constants.py index e0353fb3..c5635c1b 100644 --- a/parsedmarc/constants.py +++ b/parsedmarc/constants.py @@ -47,3 +47,41 @@ MAX_INDEXES_PER_REPORT = 31 # Most records per-day normalization may produce from one report (records # times days). A larger report keeps its records whole, over its own range. MAX_NORMALIZED_RECORDS = 250_000 + +# Most seconds one aggregate report may spend in reverse DNS lookups that +# return no PTR or are slow (GHSA-rwfh-ww9c-jm9m). The sender picks each +# record's source IP, and a reverse zone that never answers costs the whole +# DNS lifetime: 2 s per nameserver times the 4 default nameservers, 8 s per +# lookup. 120 s is 15 such lookups, so a black-holed 3,500-record report now +# ends in about two minutes instead of eight hours, while 1,000 fast no-PTR +# answers (about 50 ms each) cost about 50 s and stay under it. Lookups that +# find a PTR are charged here only when they are slow +# (SLOW_REVERSE_DNS_SECONDS), since a sender who controls a reverse zone can +# answer every query just before the timeout; every lookup is also charged +# to MAX_REVERSE_DNS_SECONDS_PER_REPORT. Past either limit, the report's +# remaining records are enriched from the local IP database and reverse DNS +# map only. +MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT = 120.0 + +# A reverse DNS lookup that takes at least this many seconds is charged to +# MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT whether or not it returned a +# name. A healthy resolver answers in milliseconds. +SLOW_REVERSE_DNS_SECONDS = 1.0 + +# Most seconds one aggregate report may spend in all of its reverse DNS +# lookups, answered or not (GHSA-rwfh-ww9c-jm9m). Any per-lookup threshold +# leaves the total unbounded, because the number of records is not: a +# reverse zone that answers every query in just under +# SLOW_REVERSE_DNS_SECONDS is never charged to the failed-or-slow budget. +# 5,000 distinct source IPs resolving in 100 ms each use 500 s, so real +# reports fit; a reverse zone that answers every query slowly is cut off +# after ten minutes. +MAX_REVERSE_DNS_SECONDS_PER_REPORT = 600.0 + +# Largest message or session count a report may give (GHSA-fxr7-7h9q-v949). +# It matches the PostgreSQL output's INTEGER count columns and the +# Elasticsearch/OpenSearch integer mapping of the SMTP TLS session counts, +# all signed 32-bit, so a larger count was rejected by those outputs. (The +# aggregate message_count is not in the Elasticsearch/OpenSearch mapping, +# so it is mapped dynamically as long.) No real reporter approaches it. +MAX_MESSAGE_COUNT = 2**31 - 1 diff --git a/parsedmarc/loganalytics.py b/parsedmarc/loganalytics.py index 5fc4a3be..c4cbc401 100644 --- a/parsedmarc/loganalytics.py +++ b/parsedmarc/loganalytics.py @@ -2,19 +2,61 @@ from __future__ import annotations +import json from typing import Any -from azure.core.exceptions import HttpResponseError +from azure.core.exceptions import AzureError from azure.identity import ClientSecretCredential from azure.monitor.ingestion import LogsIngestionClient from parsedmarc.log import logger +from parsedmarc.utils import sanitize_for_log + +# The Logs Ingestion API rejects a call larger than 1 MB, compressed or +# not ("Maximum size of API call: 1 MB", Logs Ingestion API section of +# https://learn.microsoft.com/en-us/azure/azure-monitor/fundamentals/service-limits). +# LogsIngestionClient.upload() packs whole reports into chunks of up to +# 1 MiB of JSON and never splits a report, so a report larger than that +# is sent as a call of its own and rejected. The documented decimal limit +# is used here because it is the smaller of the two. +MAX_UPLOAD_ITEM_BYTES = 1_000_000 class LogAnalyticsException(Exception): """Raised when a Log Analytics error occurs""" +def _describe_report(report: dict[str, Any]) -> str: + """Names a parsed report for a log line, from the keys of + ``AggregateReport``, ``SMTPTLSReport`` or ``FailureReport``. + + Args: + report (dict): A parsed report + + Returns: + str: The report's sender and ID (or arrival date for a failure + report), with non-printable characters escaped + """ + if "report_metadata" in report: + metadata = report.get("report_metadata") or {} + sender = metadata.get("org_name") + report_id = metadata.get("report_id") + return ( + f"org_name {sanitize_for_log(str(sender))}, " + f"report_id {sanitize_for_log(str(report_id))}" + ) + if "organization_name" in report: + return ( + f"organization_name " + f"{sanitize_for_log(str(report.get('organization_name')))}, " + f"report_id {sanitize_for_log(str(report.get('report_id')))}" + ) + return ( + f"reported_domain {sanitize_for_log(str(report.get('reported_domain')))}, " + f"arrival_date_utc {sanitize_for_log(str(report.get('arrival_date_utc')))}" + ) + + class LogAnalyticsConfig: """ The LogAnalyticsConfig class is used to define the configuration @@ -113,11 +155,15 @@ class LogAnalyticsClient(object): results, logs_client: LogsIngestionClient, dcr_stream: str, - ): + ) -> int: """ Publishes the given reports to the specified Data Collection Rule stream. + A report whose JSON is larger than ``MAX_UPLOAD_ITEM_BYTES`` would + be sent as an API call of its own and rejected by the service, so + it is left out with a warning and the other reports are uploaded. + Args: results (list): The parsed reports generated by parsedmarc. @@ -125,11 +171,35 @@ class LogAnalyticsClient(object): The client used to send the reports. dcr_stream (str): The stream name where the reports need to be pushed. + + Returns: + int: The number of reports uploaded (0 when every report was + left out). + + Raises: + LogAnalyticsException: The upload failed """ + reports = [] + for report in results: + # Measured as LogsIngestionClient.upload() measures it when it + # splits the reports into chunks (json.dumps with its defaults) + size = len(json.dumps(report).encode("utf-8")) + if size > MAX_UPLOAD_ITEM_BYTES: + logger.warning( + f"Not publishing a report of {size} bytes to the " + f"{dcr_stream} stream: larger than the 1,000,000-byte " + f"limit of one Logs Ingestion API call " + f"({_describe_report(report)})" + ) + continue + reports.append(report) + if not reports: + return 0 try: - logs_client.upload(self.conf.dcr_immutable_id, dcr_stream, results) - except HttpResponseError as e: + logs_client.upload(self.conf.dcr_immutable_id, dcr_stream, reports) + except AzureError as e: raise LogAnalyticsException(f"Upload failed: {e}") + return len(reports) def publish_results( self, @@ -161,6 +231,7 @@ class LogAnalyticsClient(object): client_secret=conf.client_secret, ) logs_client = LogsIngestionClient(conf.dce, credential=credential) + errors = [] if ( results["aggregate_reports"] and conf.dcr_aggregate_stream @@ -168,10 +239,15 @@ class LogAnalyticsClient(object): and save_aggregate ): logger.info("Publishing aggregate reports.") - self.publish_json( - results["aggregate_reports"], logs_client, conf.dcr_aggregate_stream - ) - logger.info("Successfully pushed aggregate reports.") + try: + published = self.publish_json( + results["aggregate_reports"], logs_client, conf.dcr_aggregate_stream + ) + # Oversize reports were already warned about one by one + if published: + logger.info("Successfully pushed aggregate reports.") + except LogAnalyticsException as e: + errors.append(f"aggregate reports: {e}") if ( results["failure_reports"] and conf.dcr_failure_stream @@ -179,10 +255,15 @@ class LogAnalyticsClient(object): and save_failure ): logger.info("Publishing failure reports.") - self.publish_json( - results["failure_reports"], logs_client, conf.dcr_failure_stream - ) - logger.info("Successfully pushed failure reports.") + try: + published = self.publish_json( + results["failure_reports"], logs_client, conf.dcr_failure_stream + ) + # Oversize reports were already warned about one by one + if published: + logger.info("Successfully pushed failure reports.") + except LogAnalyticsException as e: + errors.append(f"failure reports: {e}") if ( results["smtp_tls_reports"] and conf.dcr_smtp_tls_stream @@ -190,7 +271,14 @@ class LogAnalyticsClient(object): and save_smtp_tls ): logger.info("Publishing SMTP TLS reports.") - self.publish_json( - results["smtp_tls_reports"], logs_client, conf.dcr_smtp_tls_stream - ) - logger.info("Successfully pushed SMTP TLS reports.") + try: + published = self.publish_json( + results["smtp_tls_reports"], logs_client, conf.dcr_smtp_tls_stream + ) + # Oversize reports were already warned about one by one + if published: + logger.info("Successfully pushed SMTP TLS reports.") + except LogAnalyticsException as e: + errors.append(f"SMTP TLS reports: {e}") + if errors: + raise LogAnalyticsException("; ".join(errors)) diff --git a/parsedmarc/postgres.py b/parsedmarc/postgres.py index 42b546ca..23a9480c 100644 --- a/parsedmarc/postgres.py +++ b/parsedmarc/postgres.py @@ -2,25 +2,39 @@ from __future__ import annotations +import ipaddress +import os +import re from datetime import datetime -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any if TYPE_CHECKING: # LiteralString requires Python >= 3.11, so only import it for type checking from typing import LiteralString import psycopg + from psycopg import conninfo as psycopg_conninfo + from psycopg.abc import Buffer from psycopg.types import json as psycopg_json + from psycopg.types import string as psycopg_string else: try: import psycopg + from psycopg import conninfo as psycopg_conninfo from psycopg.types import json as psycopg_json + from psycopg.types import string as psycopg_string except ImportError: psycopg = None + psycopg_conninfo = None psycopg_json = None + psycopg_string = None from parsedmarc.log import logger -from parsedmarc.utils import human_timestamp_to_datetime, redact_url_credentials +from parsedmarc.utils import ( + human_timestamp_to_datetime, + redact_url_credentials, + sanitize_for_log, +) # psycopg is an optional dependency (the PostgreSQL backend is opt-in). The # pure helper functions below work without it; only PostgreSQLClient needs a @@ -31,6 +45,177 @@ _PSYCOPG_INSTALL_HINT = ( "Install it with: pip install parsedmarc[postgresql]" ) +# libpq's sslmode values, weakest first. libpq's own default is "prefer", +# which falls back to plain TCP when the server (or an on-path attacker) +# declines TLS, so parsedmarc defaults to "require" (GHSA-xvp7-7762-gx9g). +SSL_MODES = ("disable", "allow", "prefer", "require", "verify-ca", "verify-full") +DEFAULT_SSL_MODE = "require" +_VERIFYING_SSL_MODES = ("verify-ca", "verify-full") + +_WITHHELD_CONNECTION_STRING_ERROR = ( + "Invalid PostgreSQL connection_string (details withheld " + "because they may include the password)" +) + +_UNENCODED_AT_CONNECTION_STRING_ERROR = ( + 'PostgreSQL connection_string appears to have an unencoded "@" or "/" ' + "in its user name, password or database name (details withheld because " + 'they may include the password); percent-encode "@" as %40 and "/" as ' + "%2F, or use the separate host, port, user, password and database " + "settings" +) + +# With a connection string, a connection failure is reported with this fixed +# text and not libpq's: libpq's message can quote any value parsed from the +# string (a port, a host, an sslmode), and when the string's user name or +# password holds an unencoded "@" or "/", such a value is a fragment of the +# password. See PostgreSQLClient._connect(). +_WITHHELD_CONNECTION_FAILURE_ERROR = ( + "PostgreSQL connection failed (details withheld because the " + "connection string may include the password; use the separate host, " + "port, user, password and database settings to see the driver's " + "message)" +) + +# With the separate settings, an error that is not the driver's own (a +# password the driver cannot encode, say) is reported with this fixed text: +# such an error quotes the value it could not handle, or the whole +# connection string psycopg built from the settings, password included. +_WITHHELD_CONNECTION_SETTING_ERROR = ( + "PostgreSQL connection failed: a connection setting could not be passed " + "to the driver (details withheld because they may include the password)" +) + +# A SQLSTATE is five digits or upper-case letters (PostgreSQL docs, Appendix +# A, "PostgreSQL Error Codes"); anything else is not included. +_SQLSTATE_PATTERN = re.compile(r"[0-9A-Z]{5}") + + +def _sqlstate_suffix(exc: BaseException) -> str: + """Return `` [SQLSTATE xxxxx]`` for a driver error that carries a + well-formed SQLSTATE, else an empty string. + + psycopg sets ``sqlstate`` when the server answered with an error (for + example ``28P01``, a failed password). It is a fixed code chosen by the + server, not text taken from the connection string, so it is safe to + show even when the driver's message is withheld. + """ + sqlstate = getattr(exc, "sqlstate", None) + if isinstance(sqlstate, str) and _SQLSTATE_PATTERN.fullmatch(sqlstate): + return f" [SQLSTATE {sqlstate}]" + return "" + + +def _is_local_host(host: str) -> bool: + """Return whether one libpq ``host`` entry is a Unix socket or loopback. + + An empty entry means libpq's default Unix socket, and a value starting + with ``/`` (a socket directory) or ``@`` (a Linux abstract socket) is a + socket as well. + """ + if not host or host.startswith(("/", "@")) or host.lower() == "localhost": + return True + try: + return ipaddress.ip_address(host).is_loopback + except ValueError: + return False + + +def _log_safe_host(host: str) -> str: + """Return a libpq ``host`` (or ``hostaddr``) value safe to log. + + Each entry of a comma-separated list is credential-masked with + ``redact_url_credentials()`` (``pw@db.example`` becomes + ``***@db.example``), and the result is escaped for the log. + """ + entries = [str(redact_url_credentials(entry)) for entry in host.split(",")] + return sanitize_for_log(",".join(entries)) + + +_URI_PREFIXES = ("postgresql://", "postgres://") + + +def _is_uri(connection_string: str) -> bool: + """Return whether a connection string is in libpq's URI form. + + libpq itself recognizes only the exact lowercase prefixes + ``postgresql://`` and ``postgres://`` at the very start of the string, + and parses anything else as keyword/value pairs. The check here also + ignores case and leading whitespace: such a string fails libpq's + keyword/value parse anyway, so treating it as a URI only refuses it + with a different fixed message. + """ + return connection_string.lstrip().lower().startswith(_URI_PREFIXES) + + +def _port_is_numeric(entry: str) -> bool: + """Return whether one ``host[:port]`` entry of a URI authority has no + port or an all-digit one. A bracketed IPv6 address is skipped first, so + its colons are not taken for a port separator.""" + entry = entry.strip() + if entry.startswith("["): + entry = entry.partition("]")[2] + if ":" not in entry: + return True + port = entry.rpartition(":")[2] + return port.isascii() and port.isdigit() + + +def _has_unencoded_at(connection_string: str) -> bool: + """Return whether a URI connection string looks like its user name or + password holds an unencoded ``@`` or ``/``. + + The scan mirrors libpq's URI parser. libpq looks for the userinfo by + scanning the text after ``://`` up to the first ``@`` or ``/``; when it + meets ``@`` first, everything before it is the user name and password, + the host list runs to the next ``/`` or ``?``, and the database name + from that ``/`` to the next ``?``. So the rest of a password containing + an unencoded ``@`` lands in the host + (``postgresql://u:s3c@pw@db.example/d`` has host ``pw@db.example``), + the port, the database name or a query parameter, and a password + containing an unencoded ``/`` ends the authority early + (``postgresql://u:SECRET/x@db.example/d`` has no userinfo, host ``u``, + port ``SECRET`` and dbname ``x@db.example/d``). + + The string is refused when, with userinfo, the text from it to the next + ``/`` holds another ``@`` (the host list, or a query parameter written + before any ``/``), or the database name holds an ``@``; and, without + userinfo, when the database name holds an ``@``, or an ``@`` follows an + authority whose port is not a number (``u:pa?sslmode=SECRET/x@db/d`` + has port ``pa`` and sslmode ``SECRET/x@db/d``). A URI never needs an + unencoded ``@`` in those places: libpq decodes ``%40`` and ``%2F`` in + every component. An ``@`` in a query value after a ``/`` + (``?sslrootcert=/home/a@b/ca.pem``) is not refused, because nothing + tells it apart from a legitimate one. For the same reason two shapes + are not recognized: an all-digit password before a ``/``, followed by + an ``@`` only in a query value (``u:12345/x?sslrootcert=a@b``), and a + password with an ``@``, then a ``?``, then a ``/`` + (``u:s3c@pw?sslmode=SECRET/x@db``). + + This refusal is a convenience that names the common mistake; it does + not keep the password out of the logs. That is done by withholding + every value parsed from a connection string from the startup warning + and the connection error (``PostgreSQLClient._connect()``), whatever + shape the string has. + """ + rest = connection_string.partition("://")[2] + first = next((i for i, char in enumerate(rest) if char in "@/"), -1) + if first == -1: + return False + if rest[first] == "@": + segment, slash, tail = rest[first + 1 :].partition("/") + if "@" in segment: + return True + # A "?" before the "/" puts that "/" in a query value. + return bool(slash) and "?" not in segment and "@" in tail.partition("?")[0] + authority, question, _ = rest[:first].partition("?") + tail = rest[first + 1 :] + if not question and "@" in tail.partition("?")[0]: + return True + return "@" in tail and not all( + _port_is_numeric(entry) for entry in authority.split(",") + ) + # Two timestamp conventions coexist in parsed reports, so two helpers are # needed — do not collapse them into one. Aggregate *report* begin/end dates @@ -114,10 +299,93 @@ def _contact_info_to_text( return str(value) +# PostgreSQL cannot store a NUL character in text or jsonb, and a sender can +# plant one in a failure report's sample through an RFC 2047 encoded-word +# (=?utf-8?q?a=00b?=). It is replaced with U+FFFD, the replacement character, +# which keeps the text searchable and marks the spot (GHSA-fxr7-7h9q-v949). +_NUL_REPLACEMENT = "\ufffd" + + +def _strip_nul(value: Any) -> Any: + """Return ``value`` with every NUL in its strings replaced by U+FFFD. + + Dict keys and values and list items are replaced recursively; any + other value is returned unchanged. Used for the values wrapped in + ``Jsonb``, which the server rejects when they contain ``\\u0000``. + """ + if isinstance(value, str): + return value.replace("\x00", _NUL_REPLACEMENT) + if isinstance(value, dict): + return {_strip_nul(k): _strip_nul(v) for k, v in value.items()} + if isinstance(value, list): + return [_strip_nul(item) for item in value] + return value + + +def _nul_safe_dumper_classes() -> tuple[type, type]: + """Return the text and binary ``str`` dumpers that replace NUL. + + psycopg dumps a ``str`` parameter with ``StrDumperUnknown`` (text + format) or ``StrBinaryDumper`` (binary format); the first raises + ``DataError`` on a NUL and the server rejects the second. ``list[str]`` + parameters (``TEXT[]`` columns) dump each item with the same dumpers. + The subclasses replace NUL with U+FFFD first. + """ + + class _NulSafeStrDumper(psycopg_string.StrDumperUnknown): + def dump(self, obj: str) -> Buffer | None: + return super().dump(obj.replace("\x00", _NUL_REPLACEMENT)) + + class _NulSafeStrBinaryDumper(psycopg_string.StrBinaryDumper): + def dump(self, obj: str) -> Buffer | None: + return super().dump(obj.replace("\x00", _NUL_REPLACEMENT)) + + return _NulSafeStrDumper, _NulSafeStrBinaryDumper + + +def _register_nul_safe_dumpers(conn: psycopg.Connection) -> None: + """Make ``conn`` dump every ``str`` parameter with NUL replaced. + + psycopg uses the dumper registered last for a type as its default, so + the binary dumper is registered first and the text one last, the order + psycopg's own ``register_default_adapters`` uses. + """ + text_dumper, binary_dumper = _nul_safe_dumper_classes() + conn.adapters.register_dumper(str, binary_dumper) + conn.adapters.register_dumper(str, text_dumper) + + +# Most characters of a driver message that a PostgreSQLRejectedReport quotes: +# a DataError can quote the rejected value, which the sender chose. +_MAX_REJECTION_DETAIL_CHARS = 200 + + +def _rejection_detail(exc: BaseException) -> str: + """Return the driver's message for a rejected value, cut to + ``_MAX_REJECTION_DETAIL_CHARS`` characters with an ellipsis and then + escaped for the log (cutting first means the cut cannot split an + escape sequence).""" + detail = str(exc) + suffix = "" + if len(detail) > _MAX_REJECTION_DETAIL_CHARS: + detail = detail[:_MAX_REJECTION_DETAIL_CHARS] + suffix = "..." + return sanitize_for_log(detail) + suffix + + class PostgreSQLError(RuntimeError): """Raised when a PostgreSQL-level error occurs""" +class PostgreSQLRejectedReport(PostgreSQLError): + """Raised when PostgreSQL rejected a value of one report. + + psycopg raised ``DataError`` (a value the column type cannot hold) and + that report's transaction was rolled back. The connection and the + schema are fine, so the next report will save. + """ + + class AlreadySaved(ValueError): """Raised when an identical report already exists in the database""" @@ -138,6 +406,8 @@ class PostgreSQLClient: user: str | None = None, password: str | None = None, database: str | None = None, + sslmode: str | None = None, + sslrootcert: str | None = None, ) -> None: """ Initializes the PostgreSQLClient and opens a database connection. @@ -145,16 +415,25 @@ class PostgreSQLClient: Args: connection_string: A libpq connection string or URI (e.g. ``postgresql://user:pass@host/dbname``). When - present, individual keyword arguments are ignored. + present, *host*, *port*, *user*, *password* and *database* + are ignored; *sslmode* and *sslrootcert* still apply and + override the string's own values. host: Database server hostname or IP address. port: Database server port (default: 5432). user: Database user name. password: Database user password. database: Database name to connect to. + sslmode (str | None): A libpq sslmode, one of ``SSL_MODES``. + ``None`` keeps an ``sslmode`` set in *connection_string* or + the ``PGSSLMODE`` environment variable, and otherwise uses + ``require``. + sslrootcert: Path to the CA certificate file that + ``verify-ca`` and ``verify-full`` check the server's + certificate against. Raises: - PostgreSQLError: If psycopg is not installed or the connection - attempt fails. + PostgreSQLError: If psycopg is not installed, the connection + string is malformed, or the connection attempt fails. """ if psycopg is None: raise PostgreSQLError(_PSYCOPG_INSTALL_HINT) @@ -166,20 +445,160 @@ class PostgreSQLClient: self._user = user self._password = password self._database = database + self._sslmode = sslmode + self._sslrootcert = sslrootcert self._conn: psycopg.Connection | None = None + self._warn_if_server_unverified() self._connect() + def _connection_string_params(self) -> dict[str, Any]: + """Return the parameters set in the connection string, if any. + + Raises: + PostgreSQLError: If the connection string is malformed. libpq + words its parse errors with the string or a fragment of it, + which can contain the password, so the detail is withheld + (any error from the parse is replaced). Also, as a + convenience that names the common mistake, if a URI looks + like its user name or password holds an unencoded ``@`` or + ``/`` (``_has_unencoded_at()``). That check runs before + anything is logged, but it recognizes shapes and is not + what keeps the password out of the logs; the startup + warning and the connection error withhold every value + parsed from the string instead. A keyword/value string is + not checked: its password is a keyword of its own, which + an ``@`` or ``/`` does not end. + """ + if not self._connection_string: + return {} + if _is_uri(self._connection_string) and _has_unencoded_at( + self._connection_string + ): + raise PostgreSQLError(_UNENCODED_AT_CONNECTION_STRING_ERROR) from None + try: + params = psycopg_conninfo.conninfo_to_dict(self._connection_string) + except Exception: + raise PostgreSQLError(_WITHHELD_CONNECTION_STRING_ERROR) from None + return params + + def _ssl_connect_kwargs(self) -> dict[str, Any]: + """Return the TLS keyword arguments for ``psycopg.connect()``. + + An explicit *sslmode* wins. Without one, an ``sslmode`` in the + connection string or the ``PGSSLMODE`` environment variable, both + of which libpq reads itself, is left in effect, and otherwise + ``require`` replaces libpq's ``prefer`` default. + """ + kwargs: dict[str, Any] = {} + if self._sslmode: + kwargs["sslmode"] = self._sslmode + elif not ( + self._connection_string_params().get("sslmode") + or os.environ.get("PGSSLMODE") + ): + kwargs["sslmode"] = DEFAULT_SSL_MODE + if self._sslrootcert: + kwargs["sslrootcert"] = self._sslrootcert + return kwargs + + def _warn_if_server_unverified(self) -> None: + """Log a warning when a remote server's certificate is not checked. + + Below ``verify-ca``, anyone who can impersonate a TCP host can + receive the password. A Unix socket, ``localhost`` or a loopback + address is not warned about, and neither is an unset host (libpq's + default socket). + The host is the one libpq connects to: the connection string's + ``hostaddr`` (else the ``PGHOSTADDR`` environment variable) when + set, since libpq connects to it and uses ``host`` only to name the + server, and otherwise the *host* argument or the connection + string's ``host``, else the ``PGHOST`` environment variable. Every + entry of a comma-separated host list is considered. + + With a connection string, the warning names no host and no other + value from the string, whatever its shape: when the string's user + name or password holds an unencoded ``@`` or ``/``, libpq parses a + fragment of the password as the host, the port or a query + parameter such as ``sslmode``. The mode is printed only when it is + one of ``SSL_MODES``. With the separate *host* argument (or + ``PGHOST``/``PGHOSTADDR``, which the operator sets), the warning + names the host, credential-masked and escaped, which covers a host + with an ``@``. + + With ``sslmode=require`` and a root certificate file that exists, + libpq verifies the server as ``verify-ca`` does ("if a root CA file + exists, the behavior of sslmode=require will be the same as that of + verify-ca", PostgreSQL docs, libpq SSL support), so that is not + warned about either. + """ + params = self._connection_string_params() + if self._connection_string: + host = params.get("host") + hostaddr = params.get("hostaddr") + else: + host = self._host + hostaddr = None + host = ( + hostaddr or os.environ.get("PGHOSTADDR") or host or os.environ.get("PGHOST") + ) + if not host or all(_is_local_host(h.strip()) for h in host.split(",")): + return + mode = ( + self._ssl_connect_kwargs().get("sslmode") + or params.get("sslmode") + or os.environ.get("PGSSLMODE") + ) + if mode in _VERIFYING_SSL_MODES: + return + rootcert = ( + self._sslrootcert + or params.get("sslrootcert") + or os.environ.get("PGSSLROOTCERT") + ) + if mode == "require" and rootcert and os.path.isfile(rootcert): + return + if self._connection_string: + shown_mode = mode if mode in SSL_MODES else "not a recognized value" + logger.warning( + "The PostgreSQL connection_string does not verify the server " + f"certificate (sslmode is {shown_mode}); set [postgresql] " + "sslmode = verify-full and sslrootcert to protect the password " + "from an impersonated server" + ) + return + logger.warning( + f"PostgreSQL connection to {_log_safe_host(host)} uses " + f"sslmode={sanitize_for_log(str(mode))}, which does not verify the " + "server certificate; set [postgresql] sslmode = verify-full and " + "sslrootcert to protect the password from an impersonated server" + ) + def _connect(self) -> psycopg.Connection: """Open a new database connection using stored parameters. Raises: - PostgreSQLError: If the connection attempt fails. + PostgreSQLError: If the connection string is malformed or the + connection attempt fails. With a connection string, the + driver's message is withheld (only a well-formed SQLSTATE + is kept); with the separate arguments, it is passed on with + URL credentials masked. Any other error raised between + reading the settings and holding a configured connection + (a password the driver cannot encode, a host name the + resolver rejects) is replaced with a fixed message, since + its text can quote the password or the whole connection + string. Either way the original error is not chained. """ logger.debug("Connecting to PostgreSQL") try: + # Parse the connection string first, on every path (the sslmode + # lookup below skips it when sslmode is set), so a malformed + # string or one the early refusal recognizes never reaches the + # driver. + self._connection_string_params() + ssl_kwargs = self._ssl_connect_kwargs() if self._connection_string: - conn = psycopg.connect(self._connection_string) + conn = psycopg.connect(self._connection_string, **ssl_kwargs) else: conn = psycopg.connect( host=self._host, @@ -187,19 +606,50 @@ class PostgreSQLClient: user=self._user, password=self._password, dbname=self._database, + **ssl_kwargs, ) conn.autocommit = False + _register_nul_safe_dumpers(conn) + except PostgreSQLError: + # Already a fixed, secret-free message (_connection_string_params). + raise except psycopg.Error as exc: - # libpq reports a malformed connection string by echoing the - # whole string, or the fragment it could not parse, and either - # can contain the password. psycopg raises ProgrammingError for - # those parse failures, so withhold their detail entirely. - if self._connection_string and isinstance(exc, psycopg.ProgrammingError): + # Chaining would print the driver's own text, unredacted, + # wherever the error is logged with its traceback (the SIGHUP + # reload path does that), so every branch raises from None. + if self._connection_string: + # libpq reports a malformed connection string by echoing the + # whole string, or the fragment it could not parse. psycopg + # raises ProgrammingError for those parse failures. The + # string was already parsed once (_connection_string_params), + # so this guards the same failure if it surfaces here. + if isinstance(exc, psycopg.ProgrammingError): + raise PostgreSQLError(_WITHHELD_CONNECTION_STRING_ERROR) from None + # Any other failure can quote a value parsed from the string + # (libpq: 'invalid integer value "..." for connection option + # "port"'), and with an unencoded "@" or "/" in the user + # name or password, that value is part of the password. No + # check of the string's shape can rule that out, so the text + # is withheld whatever the string looks like. raise PostgreSQLError( - "Invalid PostgreSQL connection_string (details withheld " - "because they may include the password)" + _WITHHELD_CONNECTION_FAILURE_ERROR + _sqlstate_suffix(exc) ) from None - raise PostgreSQLError(redact_url_credentials(str(exc))) from exc + # With the separate arguments, the text names the host, and a + # host from the argument or PGHOST can hold anything, so it is + # credential-masked. + raise PostgreSQLError(redact_url_credentials(str(exc))) from None + except Exception: + # Not the driver's own error: psycopg encodes the connection + # string it builds from the separate arguments, so a password + # holding a lone surrogate (an environment variable with invalid + # UTF-8) raises UnicodeEncodeError carrying that whole string, + # and on Python 3.15 a host label over 63 characters raises + # UnicodeEncodeError('idna', '', ...) from name resolution, + # which psycopg does not wrap. Such text is never passed on. + # KeyboardInterrupt and SystemExit are not Exceptions and pass. + if self._connection_string: + raise PostgreSQLError(_WITHHELD_CONNECTION_FAILURE_ERROR) from None + raise PostgreSQLError(_WITHHELD_CONNECTION_SETTING_ERROR) from None self._conn = conn return conn @@ -486,7 +936,9 @@ class PostgreSQLClient: Raises: AlreadySaved: If an identical report is already present. - PostgreSQLError: If a database error occurs. + PostgreSQLRejectedReport: If PostgreSQL rejected a value of the + report. + PostgreSQLError: If another database error occurs. """ conn = self._ensure_connected() meta = report.get("report_metadata", {}) @@ -651,6 +1103,13 @@ class PostgreSQLClient: except AlreadySaved: raise + except psycopg.DataError as exc: + raise PostgreSQLRejectedReport( + "PostgreSQL rejected a value of the aggregate report from " + f"{sanitize_for_log(str(meta.get('org_name')))} with ID " + f"{sanitize_for_log(str(meta.get('report_id')))}: " + f"{_rejection_detail(exc)}" + ) from exc except psycopg.Error as exc: raise PostgreSQLError(str(exc)) from exc @@ -663,7 +1122,9 @@ class PostgreSQLClient: Raises: AlreadySaved: If a matching failure report is already present. - PostgreSQLError: If a database error occurs. + PostgreSQLRejectedReport: If PostgreSQL rejected a value of the + report. + PostgreSQLError: If another database error occurs. """ conn = self._ensure_connected() sample = report.get("parsed_sample", {}) or {} @@ -671,11 +1132,21 @@ class PostgreSQLClient: arrival_date_utc = _ensure_utc_suffix(report.get("arrival_date_utc")) sample_subject = sample.get("subject") # JSONB values are reused by both the dedup check and the INSERT. + # jsonb cannot hold \u0000, so NUL is replaced before wrapping. The + # str dumpers registered in _connect() cover the text columns. sample_headers = ( - psycopg_json.Jsonb(sample["headers"]) if sample.get("headers") else None + psycopg_json.Jsonb(_strip_nul(sample["headers"])) + if sample.get("headers") + else None + ) + sample_from = ( + psycopg_json.Jsonb(_strip_nul(sample["from"])) + if sample.get("from") + else None + ) + sample_to = ( + psycopg_json.Jsonb(_strip_nul(sample["to"])) if sample.get("to") else None ) - sample_from = psycopg_json.Jsonb(sample["from"]) if sample.get("from") else None - sample_to = psycopg_json.Jsonb(sample["to"]) if sample.get("to") else None try: with conn.transaction(): @@ -790,6 +1261,13 @@ class PostgreSQLClient: except AlreadySaved: raise + except psycopg.DataError as exc: + raise PostgreSQLRejectedReport( + "PostgreSQL rejected a value of the failure report for " + f"{sanitize_for_log(str(report.get('reported_domain')))} " + f"arriving at {sanitize_for_log(str(arrival_date_utc))}: " + f"{_rejection_detail(exc)}" + ) from exc except psycopg.Error as exc: raise PostgreSQLError(str(exc)) from exc @@ -802,7 +1280,9 @@ class PostgreSQLClient: Raises: AlreadySaved: If an identical report is already present. - PostgreSQLError: If a database error occurs. + PostgreSQLRejectedReport: If PostgreSQL rejected a value of the + report. + PostgreSQLError: If another database error occurs. """ conn = self._ensure_connected() try: @@ -889,5 +1369,12 @@ class PostgreSQLClient: except AlreadySaved: raise + except psycopg.DataError as exc: + raise PostgreSQLRejectedReport( + "PostgreSQL rejected a value of the SMTP TLS report from " + f"{sanitize_for_log(str(report.get('organization_name')))} with ID " + f"{sanitize_for_log(str(report.get('report_id')))}: " + f"{_rejection_detail(exc)}" + ) from exc except psycopg.Error as exc: raise PostgreSQLError(str(exc)) from exc diff --git a/parsedmarc/utils.py b/parsedmarc/utils.py index 74db74d8..4ffd5225 100644 --- a/parsedmarc/utils.py +++ b/parsedmarc/utils.py @@ -4,7 +4,9 @@ from __future__ import annotations +import atexit import base64 +import contextlib import csv import hashlib import io @@ -13,6 +15,7 @@ import logging import mailbox import os import shutil +import stat import subprocess import tempfile from datetime import datetime, timedelta, timezone @@ -719,6 +722,125 @@ _IP_DB_PATH: str | None = None # selection is logged when it changes rather than on every lookup. _LAST_LOGGED_IP_DB_PATH: str | None = None +# The directory _ip_db_cache_dir() chose, keyed by the candidate path it +# checked, so a rejected candidate is warned about once per process and +# the temporary directory that replaces it is reused rather than recreated. +_IP_DB_CACHE_DIR: tuple[str, str] | None = None + + +def _ip_db_cache_dir_candidate() -> str: + """Returns the per-user directory the IP database download is cached + in: ``%LOCALAPPDATA%\\parsedmarc\\cache`` on Windows, otherwise + ``$XDG_CACHE_HOME/parsedmarc`` when ``XDG_CACHE_HOME`` is set to an + absolute path (the XDG Base Directory specification says a relative one + is invalid and must be ignored), else ``~/.cache/parsedmarc``.""" + if os.name == "nt": + base = os.environ.get("LOCALAPPDATA") or os.path.expanduser( + os.path.join("~", "AppData", "Local") + ) + return os.path.join(base, "parsedmarc", "cache") + xdg_cache_home = os.environ.get("XDG_CACHE_HOME") + if xdg_cache_home and os.path.isabs(xdg_cache_home): + return os.path.join(xdg_cache_home, "parsedmarc") + return os.path.join(os.path.expanduser("~"), ".cache", "parsedmarc") + + +def _ip_db_cache_dir_problem(path: str, *, create: bool = True) -> str | None: + """Creates ``path`` (mode 0700) if it does not exist and ``create`` is + true, and returns why it is unsafe to cache the IP database in, or + ``None`` if it is safe: it must be an absolute path, a directory and not + a symlink, writable, and on POSIX owned by the current user and not + writable by its group or by others. An existing directory's mode is + never changed, since it may belong to someone else. + + A relative path is refused before anything is created: it is what + ``os.path.expanduser()`` returns, unchanged, when ``HOME`` is unset and + the user has no passwd entry, and it would put the cache under the + current directory. With ``create`` false, a missing directory is + reported as a problem rather than created. + """ + if not os.path.isabs(path): + return "it is not an absolute path" + try: + if create: + # mode applies only to directories this call creates. + os.makedirs(path, mode=0o700, exist_ok=True) + st = os.lstat(path) + except OSError as e: + if create: + return f"it cannot be created ({e.__class__.__name__}: {e.strerror or e})" + return f"it cannot be read ({e.__class__.__name__}: {e.strerror or e})" + if not stat.S_ISDIR(st.st_mode): + # makedirs() or os.path.isabs() accepted it, so this is a symlink + # (lstat() does not follow it), another kind of file, or a + # directory replaced since. + return "it is not a directory (a symlink, or another kind of file)" + if hasattr(os, "geteuid"): + if st.st_uid != os.geteuid(): + return "it is owned by another user" + if st.st_mode & 0o022: + return "it is writable by its group or by others" + if not os.access(path, os.W_OK): + return "it is not writable" + return None + + +def _ip_db_cache_dir() -> str: + """Returns the directory to cache the downloaded IP database in: the + per-user directory from :func:`_ip_db_cache_dir_candidate` when + :func:`_ip_db_cache_dir_problem` finds nothing wrong with it, otherwise + a new private temporary directory (mode 0700, unpredictable name) that + lasts only for this process, so the database is downloaded again on + every run. The choice is remembered for the process.""" + global _IP_DB_CACHE_DIR + candidate = _ip_db_cache_dir_candidate() + if _IP_DB_CACHE_DIR is not None and _IP_DB_CACHE_DIR[0] == candidate: + return _IP_DB_CACHE_DIR[1] + chosen = candidate + problem = _ip_db_cache_dir_problem(candidate) + if problem is not None: + chosen = tempfile.mkdtemp(prefix="parsedmarc-") + atexit.register(shutil.rmtree, chosen, ignore_errors=True) + logger.warning( + f"Not using {candidate} for the IP database cache: {problem}; " + f"using a temporary directory for this run, so the IP database " + f"will be downloaded again on every run" + ) + _IP_DB_CACHE_DIR = (candidate, chosen) + return chosen + + +def _existing_ip_db_cache_dir() -> str | None: + """Returns the directory to look for a cached IP database in when no + download will happen, or ``None`` if there is none to look in. + + This is the directory :func:`_ip_db_cache_dir` already chose in this + process for the current candidate, else the per-user candidate if it + exists and :func:`_ip_db_cache_dir_problem` finds nothing wrong with + it. Nothing is created and nothing is warned about: with no download, + an unusable candidate only means there is no cached copy to use.""" + candidate = _ip_db_cache_dir_candidate() + if _IP_DB_CACHE_DIR is not None and _IP_DB_CACHE_DIR[0] == candidate: + return _IP_DB_CACHE_DIR[1] + problem = _ip_db_cache_dir_problem(candidate, create=False) + if problem is not None: + logger.debug(f"Not looking for a cached IP database in {candidate}: {problem}") + return None + return candidate + + +def _cached_ip_db_problem(st: os.stat_result) -> str | None: + """Returns why the cached IP database whose ``os.lstat()`` result is + ``st`` must not be used, or ``None`` if it may: it must be a regular + file (not a symlink) and, on POSIX, owned by the current user.""" + if stat.S_ISLNK(st.st_mode): + return "it is a symlink" + if not stat.S_ISREG(st.st_mode): + return "it is not a regular file" + if hasattr(os, "geteuid") and st.st_uid != os.geteuid(): + return "it is owned by another user" + return None + def load_ip_db( *, @@ -733,6 +855,22 @@ def load_ip_db( download. On download failure (or when offline), a previously cached download is used if available, falling back to the bundled copy. + The download is cached as ``ipinfo_lite.mmdb`` in a per-user directory + chosen by :func:`_ip_db_cache_dir` (``$XDG_CACHE_HOME/parsedmarc`` or + ``~/.cache/parsedmarc``; ``%LOCALAPPDATA%\\parsedmarc\\cache`` on + Windows), which must be a real directory, not a symlink, writable, and + on POSIX owned by the current user and not group- or world-writable; + otherwise a private temporary directory is used for this process only. + With ``offline`` or ``always_use_local_file`` nothing is downloaded, so + no directory is created: a cached copy is looked for only in the + directory an earlier download in this process used, else in the + per-user directory if it already exists and passes the same checks. + The download is written to a new file created exclusively in that + directory, opened with ``maxminddb`` to check that it is a valid MMDB, + and only then renamed over the cached copy. A cached copy is used only + if it is a regular file (not a symlink) and, on POSIX, owned by the + current user. + Args: always_use_local_file: Always use a local/bundled database file local_file_path: Path to a local MMDB file @@ -753,10 +891,28 @@ def load_ip_db( logger.info(f"Using local IP database at {local_file_path}") return - cache_dir = os.path.join(tempfile.gettempdir(), "parsedmarc") + download = not (offline or always_use_local_file) + if download: + try: + cache_dir: str | None = _ip_db_cache_dir() + except OSError as e: + # Only the temporary-directory fallback can raise. + logger.warning(f"Failed to create a temporary IP database cache: {e}") + cache_dir = None + else: + # No download, so nothing to cache: look for an earlier download + # without creating a directory or warning about one. + cache_dir = _existing_ip_db_cache_dir() + if cache_dir is None: + _IP_DB_PATH = str( + files(parsedmarc.resources.ipinfo).joinpath("ipinfo_lite.mmdb") + ) + logger.info("Using bundled IP database") + return cached_path = os.path.join(cache_dir, "ipinfo_lite.mmdb") - if not (offline or always_use_local_file): + if download: + tmp_path: str | None = None try: logger.debug( f"Trying to fetch IP database from {redact_url_credentials(url)}..." @@ -766,24 +922,50 @@ def load_ip_db( url, headers=headers, timeout=60, follow_redirects=True ) response.raise_for_status() - os.makedirs(cache_dir, exist_ok=True) - tmp_path = cached_path + ".tmp" - with open(tmp_path, "wb") as f: + # mkstemp() creates a new file with O_CREAT | O_EXCL, so it never + # opens (or follows a symlink to) a file that already exists. + fd, tmp_path = tempfile.mkstemp( + dir=cache_dir, prefix="ipinfo_lite.", suffix=".tmp" + ) + with os.fdopen(fd, "wb") as f: f.write(response.content) - shutil.move(tmp_path, cached_path) - _IP_DB_PATH = cached_path - logger.info("IP database updated successfully") - return + try: + reader = maxminddb.open_database(tmp_path) + reader.close() + except Exception: + # maxminddb raises InvalidDatabaseError (a RuntimeError) for + # a file that is not an MMDB; anything else it raises means + # the same here, so the body never becomes the cached copy. + logger.warning("Downloaded IP database is not a valid MMDB file") + else: + # An atomic rename: a symlink at cached_path is replaced as a + # directory entry, not followed. + os.replace(tmp_path, cached_path) + tmp_path = None + _IP_DB_PATH = cached_path + logger.info("IP database updated successfully") + return except (httpx.HTTPError, httpx.InvalidURL) as e: logger.warning(f"Failed to fetch IP database: {_describe_http_error(e)}") except Exception as e: logger.warning(f"Failed to save IP database: {e}") + finally: + if tmp_path is not None: + with contextlib.suppress(OSError): + os.remove(tmp_path) # Fall back to a previously cached copy if available - if os.path.isfile(cached_path): - _IP_DB_PATH = cached_path - logger.info("Using cached IP database") - return + try: + cached_st: os.stat_result | None = os.lstat(cached_path) + except OSError: + cached_st = None # No cached copy + if cached_st is not None: + problem = _cached_ip_db_problem(cached_st) + if problem is None: + _IP_DB_PATH = cached_path + logger.info("Using cached IP database") + return + logger.warning(f"Ignoring the cached IP database at {cached_path}: {problem}") # Final fallback: bundled copy _IP_DB_PATH = str(files(parsedmarc.resources.ipinfo).joinpath("ipinfo_lite.mmdb")) diff --git a/pyproject.toml b/pyproject.toml index f7503ba7..f0f3daa2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -58,7 +58,7 @@ dependencies = [ # No extras: the base mailsuite supplies IMAP and Maildir connections # plus mail-parser. Gmail and Microsoft Graph come from the gmail and # msgraph extras below. - "mailsuite>=2.3.5", + "mailsuite>=2.3.7", "maxminddb>=2.0.0", "publicsuffixlist>=0.10.0", # Imported directly at utils.py:39 (dateutil.parser). It used to @@ -95,13 +95,13 @@ loganalytics = [ "azure-monitor-ingestion>=1.0.0", ] msgraph = [ - "mailsuite[msgraph]>=2.3.5", + "mailsuite[msgraph]>=2.3.7", # Imported directly in cli.py for Graph error handling; otherwise # only a transitive dep of mailsuite[msgraph] -> msgraph-sdk. "microsoft-kiota-abstractions>=1.8.0", ] gmail = [ - "mailsuite[gmail]>=2.3.5", + "mailsuite[gmail]>=2.3.7", ] # `all` must stay the union of every extra above — and must keep # excluding `postgresql`: psycopg's prebuilt binary wheels do not exist @@ -117,7 +117,7 @@ all = [ "boto3>=1.16.63", "elasticsearch>=8.18,<9", "kafka-python>=2.3.2", - "mailsuite[gmail,msgraph]>=2.3.5", + "mailsuite[gmail,msgraph]>=2.3.7", "microsoft-kiota-abstractions>=1.8.0", "opensearch-py>=2.4.2,<=4.0.0", "pygelf>=0.4.2", diff --git a/tests/test_cli.py b/tests/test_cli.py index 71b7c2c3..5dc564b1 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1225,8 +1225,8 @@ class TestMixedInputAggregateDedup(unittest.TestCase): ``SEEN_AGGREGATE_REPORT_IDS`` cache, so they must key it identically. If the direct-file path in ``_main()`` wrote ``{org_name}_{report_id}`` - while ``get_dmarc_reports_from_mbox`` looked up a JSON - ``[org_name, report_id, policy_domain]`` key, a report passed both as a + while ``get_dmarc_reports_from_mbox`` looked up a key built from + ``[org_name, report_id, policy_domain]``, a report passed both as a file and inside an mbox in the same run would be saved twice. RFC 9990 §3.5.1 scopes Report-ID uniqueness to "reports to the same domain", so the shared key also keeps same-ID reports for different domains apart in @@ -4724,6 +4724,9 @@ batch_size = {batch_size} # copy some earlier run downloaded and must reach the bundled file. cache_dir = tempfile.mkdtemp() self.addCleanup(shutil.rmtree, cache_dir, True) + old_cache_dir = parsedmarc.utils._IP_DB_CACHE_DIR + self.addCleanup(setattr, parsedmarc.utils, "_IP_DB_CACHE_DIR", old_cache_dir) + parsedmarc.utils._IP_DB_CACHE_DIR = None real_files = parsedmarc.utils.files @@ -4737,7 +4740,7 @@ batch_size = {batch_size} with ( patch("parsedmarc.utils.files", side_effect=files_without_bundled_mmdb), - patch("parsedmarc.utils.tempfile.gettempdir", return_value=cache_dir), + patch.dict(os.environ, {"XDG_CACHE_HOME": cache_dir}), ): state, mock_watch, logs = self._drive_reload( # Startup resolves the local file directly and never reaches @@ -5890,6 +5893,176 @@ class TestInitOutputClientsDoesNotLogCredentials(unittest.TestCase): self.assertIn("https://u:***@127.0.0.1:9299", matching[0]) +class TestSearchClientBuildErrorsDoNotLeakCredentials(unittest.TestCase): + """GHSA-74xv-rmpx-2pjm (an incomplete fix for GHSA-hjr3-rx4c-24cv): a + password with an unencoded "/", "?" or "#" in an Elasticsearch or + OpenSearch ``hosts`` URL makes the SDK raise a ValueError quoting the + URL -- elastic_transport's ``url_to_node_config()`` raises ``Could not + parse URL ''`` -- or the start of the password -- opensearch-py's + ``_normalize_hosts()`` reads ``urllib.parse``'s ``port``, which raises + ``Port could not be cast to integer value as ''``. The wrapping + RuntimeError repeated that text, and was chained to the original, so + the startup retry loop logged the password at ERROR without --debug and + the SIGHUP reload's logger.exception printed it again.""" + + WITHHELD = ( + ": could not build a client from the hosts setting (details withheld " + "because they may include credentials; URL-encode special characters " + "in the password, or use the separate user and password settings)" + ) + + def setUp(self): + _isolate_default_alias(self, parsedmarc.elastic.connections) + _isolate_default_alias(self, opensearch_module.connections) + + def _init_error(self, opts): + """Run _init_output_clients(), which must fail; return the error, its + formatted traceback, and the captured log.""" + with ( + self.assertRaises(RuntimeError) as ctx, + self.assertLogs("parsedmarc.log", level="DEBUG") as logs, + ): + parsedmarc.cli._init_output_clients(opts) + trace = "".join(traceback.format_exception(ctx.exception)) + return ctx.exception, trace, "\n".join(logs.output) + + def _assert_withheld(self, label, opts, secret): + exc, trace, output = self._init_error(opts) + self.assertEqual(str(exc), label + self.WITHHELD) + self.assertIsNone(exc.__cause__) + self.assertIsNone(exc.__context__) + for text in (str(exc), trace, output): + self.assertNotIn(secret, text) + + def test_elasticsearch_unparsable_hosts_password_is_withheld(self): + malformed = ( + ("https://dmarc:Pa/ss-S3cret@es.example:9200", "Pa/ss-S3cret"), + ("https://dmarc:Pa?ss-S3cret@es.example:9200", "ss-S3cret"), + ("https://dmarc:Pa#ss-S3cret@es.example:9200", "ss-S3cret"), + ) + for url, secret in malformed: + with self.subTest(url=url): + opts = _output_client_opts( + save_aggregate=True, + elasticsearch_hosts=[url], + elasticsearch_timeout=1.0, + ) + self._assert_withheld("Elasticsearch", opts, secret) + + def test_opensearch_unparsable_hosts_password_is_withheld(self): + malformed = ( + ("https://dmarc:S3cretPrefix/rest@os.example:9200", "S3cretPrefix"), + ("https://dmarc:S3cretPrefix?rest@os.example:9200", "S3cretPrefix"), + ("https://dmarc:S3cretPrefix#rest@os.example:9200", "S3cretPrefix"), + ) + for url, secret in malformed: + with self.subTest(url=url): + opts = _output_client_opts( + save_aggregate=True, + opensearch_hosts=[url], + opensearch_timeout=1.0, + ) + self._assert_withheld("OpenSearch", opts, secret) + + def test_other_errors_mask_url_credentials_and_are_not_chained(self): + """An error after set_hosts() succeeded -- here from the index + migration, and a ValueError, which must not be mistaken for an + unparsable hosts value -- keeps its text with credentials masked.""" + cases = ( + ("Elasticsearch", "elastic", "elasticsearch"), + ("OpenSearch", "opensearch", "opensearch"), + ) + for label, module, prefix in cases: + for error_class in (Exception, ValueError): + with self.subTest(label=label, error=error_class.__name__): + opts = _output_client_opts( + save_aggregate=True, + **{ + f"{prefix}_hosts": ["https://u:p4ss@127.0.0.1:9299"], + f"{prefix}_timeout": 1.0, + }, + ) + with patch( + f"parsedmarc.cli.{module}.migrate_indexes", + side_effect=error_class("boom https://u:p@h/x"), + ): + exc, trace, _output = self._init_error(opts) + self.assertEqual(str(exc), f"{label}: boom https://u:***@h/x") + self.assertIsNone(exc.__cause__) + self.assertNotIn("u:p@", trace) + + +class TestPostgreSQLClientBuildErrorsDoNotLeakCredentials(unittest.TestCase): + """Fifth review of GHSA-xvp7-7762-gx9g: _init_output_clients() wrapped + any PostgreSQL construction failure as ``RuntimeError(f"PostgreSQL: + {e}") from e``, so a non-driver error carrying the password (psycopg's + UnicodeEncodeError for a password with a lone surrogate quotes the + whole connection string it built) was printed in full by the chained + traceback the SIGHUP reload logs with logger.exception. The wrapper is + now not chained, and an error that is not a PostgreSQLError gets a + fixed message.""" + + def _init_error(self, opts): + with ( + self.assertRaises(RuntimeError) as ctx, + self.assertLogs("parsedmarc.log", level="DEBUG") as logs, + ): + parsedmarc.cli._init_output_clients(opts) + trace = "".join(traceback.format_exception(ctx.exception)) + return ctx.exception, trace, "\n".join(logs.output) + + def _assert_clean(self, exc, trace, output): + self.assertIsNone(exc.__cause__) + self.assertTrue(exc.__suppress_context__) + for text in (str(exc), repr(exc), trace, output): + self.assertNotIn("SECRETX", text) + + def test_driver_boundary_error_is_not_chained(self): + """psycopg.connect raising UnicodeEncodeError with the password + (the real driver's behaviour for a surrogate in the password) + surfaces as the client's fixed PostgreSQLError text, unchained.""" + opts = _output_client_opts( + postgresql_host="db.invalid", + postgresql_user="u", + postgresql_password="SECRETX", + postgresql_sslmode="verify-full", + ) + mock_psycopg = MagicMock() + mock_psycopg.Error = type("Error", (Exception,), {}) + mock_psycopg.connect.side_effect = UnicodeEncodeError( + "utf-8", "host=db.invalid user=u password=SECRETX", 0, 1, "x" + ) + with patch.object(parsedmarc.cli.postgres, "psycopg", mock_psycopg): + exc, trace, output = self._init_error(opts) + self.assertEqual( + str(exc), + "PostgreSQL: PostgreSQL connection failed: a connection setting " + "could not be passed to the driver (details withheld because they " + "may include the password)", + ) + self._assert_clean(exc, trace, output) + + def test_other_errors_get_a_fixed_message(self): + """An error that is not a PostgreSQLError (whose text postgres.py + builds) is replaced with a fixed message, also unchained.""" + opts = _output_client_opts(postgresql_host="db.invalid") + with ( + patch.object(parsedmarc.cli.postgres, "psycopg", MagicMock()), + patch.object( + parsedmarc.cli.postgres, + "PostgreSQLClient", + side_effect=ValueError("SECRETX"), + ), + ): + exc, trace, output = self._init_error(opts) + self.assertEqual( + str(exc), + "PostgreSQL: the client could not be created (details withheld " + "because they may include the password)", + ) + self._assert_clean(exc, trace, output) + + class TestHttpxLogRedaction(unittest.TestCase): """GHSA-hjr3-rx4c-24cv: httpx logs the full request URL at INFO.""" @@ -9902,6 +10075,65 @@ class TestParseConfigPostgreSQL(unittest.TestCase): _parse_config(cp, _opts()) self.assertIn("postgresql", str(ctx.exception)) + def test_postgresql_sslmode_and_sslrootcert_with_host_and_dsn(self): + """sslmode and sslrootcert are read with either connection style + (GHSA-xvp7-7762-gx9g), and sslmode is matched case-insensitively.""" + from parsedmarc.cli import _parse_config + + for connection in ( + {"host": "db.example.com"}, + {"connection_string": "host=db.example.com"}, + ): + with self.subTest(connection=connection): + cp = _config_with( + "postgresql", + { + **connection, + "sslmode": "Verify-Full", + "sslrootcert": "/etc/ssl/db-ca.pem", + }, + ) + opts = _opts() + _parse_config(cp, opts) + self.assertEqual(opts.postgresql_sslmode, "verify-full") + self.assertEqual(opts.postgresql_sslrootcert, "/etc/ssl/db-ca.pem") + + def test_postgresql_sslmode_unset_leaves_defaults(self): + """Without the keys, _parse_config sets neither option, so the + Namespace defaults (None) reach the client.""" + from parsedmarc.cli import _parse_config + + cp = _config_with("postgresql", {"host": "db.example.com"}) + opts = _opts() + _parse_config(cp, opts) + self.assertFalse(hasattr(opts, "postgresql_sslmode")) + self.assertFalse(hasattr(opts, "postgresql_sslrootcert")) + + def test_postgresql_invalid_sslmode_raises(self): + from parsedmarc.cli import ConfigurationError, _parse_config + + cp = _config_with("postgresql", {"host": "h", "sslmode": "on"}) + with self.assertRaises(ConfigurationError) as ctx: + _parse_config(cp, _opts()) + self.assertEqual( + str(ctx.exception), + "postgresql sslmode must be one of disable, allow, prefer, " + "require, verify-ca, verify-full", + ) + + def test_postgresql_sslrootcert_path_is_expanded(self): + from parsedmarc.cli import _parse_config + + cp = _config_with( + "postgresql", {"host": "h", "sslrootcert": "~/certs/db-ca.pem"} + ) + opts = _opts() + _parse_config(cp, opts) + self.assertEqual( + opts.postgresql_sslrootcert, + os.path.join(os.path.expanduser("~"), "certs", "db-ca.pem"), + ) + class TestPostgreSQLCliWiring(unittest.TestCase): """End-to-end: a [postgresql] config reaches PostgreSQLClient + create_tables. @@ -9938,6 +10170,7 @@ port = 6543 user = pmarc password = secret database = dmarc +sslmode = verify-ca """ with tempfile.NamedTemporaryFile( "w", suffix=".ini", delete=False @@ -9967,6 +10200,8 @@ database = dmarc self.assertEqual(kwargs.get("port"), 6543) self.assertEqual(kwargs.get("user"), "pmarc") self.assertEqual(kwargs.get("database"), "dmarc") + self.assertEqual(kwargs.get("sslmode"), "verify-ca") + self.assertIsNone(kwargs.get("sslrootcert")) mock_client_cls.return_value.create_tables.assert_called_once() def test_postgresql_aggregate_report_is_saved(self): @@ -10585,6 +10820,8 @@ def _output_client_opts(**overrides) -> SimpleNamespace: "postgresql_host", "postgresql_password", "postgresql_port", + "postgresql_sslmode", + "postgresql_sslrootcert", "postgresql_user", "s3_access_key_id", "s3_bucket", @@ -10826,6 +11063,37 @@ class TestOptionalIntegrationExtras(unittest.TestCase): self.assertEqual(sorted(clients), ["postgresql_client"]) self.assertIs(clients["postgresql_client"], mock_client.return_value) + def test_postgresql_tls_options_reach_the_client(self): + """_init_output_clients passes sslmode and sslrootcert to + PostgreSQLClient, and passes None for both when they are unset so + the client applies its own default (GHSA-xvp7-7762-gx9g).""" + cases = ( + ( + { + "postgresql_sslmode": "verify-full", + "postgresql_sslrootcert": "/etc/ssl/db-ca.pem", + }, + ("verify-full", "/etc/ssl/db-ca.pem"), + ), + ({}, (None, None)), + ) + for overrides, expected in cases: + with self.subTest(overrides=overrides): + opts = _output_client_opts( + postgresql_host="db.example.com", **overrides + ) + with ( + patch.object(parsedmarc.cli.postgres, "psycopg", MagicMock()), + patch.object( + parsedmarc.cli.postgres, "PostgreSQLClient" + ) as mock_client, + ): + clients = parsedmarc.cli._init_output_clients(opts) + kwargs = mock_client.call_args.kwargs + self.assertEqual((kwargs["sslmode"], kwargs["sslrootcert"]), expected) + self.assertEqual(kwargs["host"], "db.example.com") + self.assertIs(clients["postgresql_client"], mock_client.return_value) + def test_missing_mailbox_extras_fail_fast_at_config_time(self): """[msgraph]/[gmail_api] sections without their extra fail in _parse_config with parsedmarc's own install hint. @@ -11342,5 +11610,115 @@ class TestOutputDestinationExceptionsMarkTheBatchUnsaved(unittest.TestCase): self.assertFalse(any("Mailbox Error" in line for line in output), output) +class TestPostgreSQLRejectedReportKeepsTheBatch(unittest.TestCase): + """GHSA-fxr7-7h9q-v949: a value PostgreSQL rejected in one report raised + PostgreSQLError, which process_reports() recorded as an output error, so + the whole batch was reported unsaved and none of its messages was + archived, including legitimate reports from other senders. Such a + rejection now raises PostgreSQLRejectedReport, which is logged and skips + only that report in the PostgreSQL output, unless fail_on_output_error + is set.""" + + _REPORTS = ( + ("aggregate", "aggregate_reports", {"policy_published": {"domain": "a"}}), + ("failure", "failure_reports", {"reported_domain": "example.com"}), + ("smtp_tls", "smtp_tls_reports", {"policies": []}), + ) + + setUp = TestOutputDestinationExceptionsMarkTheBatchUnsaved.setUp + _run = TestOutputDestinationExceptionsMarkTheBatchUnsaved._run + + def _run_with_save_error(self, report_type, batch_key, reports, error, **kw): + saver = f"save_{report_type}_report_to_postgresql" + client_cls = MagicMock() + getattr(client_cls.return_value, saver).side_effect = [error] + [None] * ( + len(reports) - 1 + ) + result = self._run( + "[postgresql]\nhost = db.example.com\n", + {batch_key: reports}, + patch("parsedmarc.cli.postgres.PostgreSQLClient", client_cls), + # The startup guard checks the SDK, not the client class. + patch("parsedmarc.cli.postgres.psycopg", object()), + **kw, + ) + return result, getattr(client_cls.return_value, saver) + + def test_rejected_report_is_logged_and_the_batch_is_saved(self): + from parsedmarc import postgres + + for report_type, batch_key, report in self._REPORTS: + message = f"PostgreSQL rejected a value of the {report_type} report" + with self.subTest(report_type=report_type): + other = dict(report, marker="second") + (verdicts, output, exit_code, _mocks), saver = ( + self._run_with_save_error( + report_type, + batch_key, + [report, other], + postgres.PostgreSQLRejectedReport(message), + ) + ) + self.assertIsNone(exit_code) + self.assertEqual(verdicts, [True]) + # The next report of the batch is still saved. + self.assertEqual( + [c.args[0] for c in saver.call_args_list], [report, other] + ) + self.assertTrue(any(message in line for line in output), output) + self.assertFalse( + any("PostgreSQL Error:" in line for line in output), output + ) + + def test_other_postgresql_error_still_marks_the_batch_unsaved(self): + from parsedmarc import postgres + + for report_type, batch_key, report in self._REPORTS: + with self.subTest(report_type=report_type): + (verdicts, output, exit_code, _mocks), _saver = ( + self._run_with_save_error( + report_type, + batch_key, + [report], + postgres.PostgreSQLError("server closed the connection"), + ) + ) + self.assertIsNone(exit_code) + self.assertEqual(verdicts, [False]) + self.assertTrue( + any( + "PostgreSQL Error: server closed the connection" in line + for line in output + ), + output, + ) + + def test_rejected_report_is_an_output_error_with_fail_on_output_error(self): + from parsedmarc import postgres + + for report_type, batch_key, report in self._REPORTS: + message = f"PostgreSQL rejected a value of the {report_type} report" + with self.subTest(report_type=report_type): + (verdicts, output, exit_code, _mocks), _saver = ( + self._run_with_save_error( + report_type, + batch_key, + [report], + postgres.PostgreSQLRejectedReport(message), + general="fail_on_output_error = true", + ) + ) + self.assertEqual(exit_code, 1) + self.assertEqual(verdicts, []) + self.assertTrue( + any( + "Output destination failures detected" in line + and f"PostgreSQL Error: {message}" in line + for line in output + ), + output, + ) + + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/tests/test_config.py b/tests/test_config.py index f721916b..17f8db29 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -2,7 +2,10 @@ import dataclasses import pickle +import time import unittest +from collections import OrderedDict +from unittest import mock import parsedmarc import parsedmarc.config as parsedmarc_config @@ -70,6 +73,79 @@ class TestParserConfigCaches(unittest.TestCase): self.assertIs(parsedmarc.REVERSE_DNS_MAP, parsedmarc_config.REVERSE_DNS_MAP) +class TestSeenReportCache(unittest.TestCase): + """Covers `SeenReportCache`, the seen-aggregate-report-ID cache that + drops expired entries on insert (GHSA-9fw6-mwqr-p43m).""" + + def test_expired_entry_is_removed_when_another_is_added(self): + """An entry older than ``max_age`` is removed from storage when a + different key is added, not only when its own key is read again. + + On a plain ``ExpiringDict`` the same sequence leaves both entries + stored (``len == 2``, measured against expiringdict 1.2.2), which is + how every distinct report stayed in memory for the life of a watch + process. ``len()`` and ``OrderedDict.keys()`` look at storage + directly; ``in`` would hide the stale entry by expiring it on read. + """ + cache = parsedmarc_config.SeenReportCache(max_len=100, max_age_seconds=3600) + cache.__setitem__("old", True, set_time=time.time() - 7200) + + cache["new"] = True + + self.assertEqual(len(cache), 1) + self.assertEqual(list(OrderedDict.keys(cache)), ["new"]) + + def test_unexpired_entry_survives_the_next_insert(self): + """The sweep stops at the first entry that has not expired, so an + entry still inside ``max_age`` is kept and still reads as seen.""" + cache = parsedmarc_config.SeenReportCache(max_len=100, max_age_seconds=3600) + cache.__setitem__("recent", True, set_time=time.time() - 60) + + cache["new"] = True + + self.assertEqual(len(cache), 2) + self.assertEqual(list(OrderedDict.keys(cache)), ["recent", "new"]) + self.assertIn("recent", cache) + + def test_re_added_key_moves_to_the_tail(self): + """A key set again while still stored moves to the tail, so an + expired entry behind it is still swept on the next insert. + + A is added, B a second later, A again 30 minutes later (still + unexpired, so the sweep keeps it), and C once B is over an hour old + but A is not. Without the move, ``OrderedDict.__setitem__`` refreshed + A in place at the head, the sweep stopped there, and B survived: + storage held ``['A', 'B', 'C']`` (measured on c8b3c26 with this same + fake clock). + """ + clock = [1_000_000.0] + with mock.patch("time.time", side_effect=lambda: clock[0]): + cache = parsedmarc_config.SeenReportCache(max_len=100, max_age_seconds=3600) + cache["A"] = True + clock[0] += 1 + cache["B"] = True + clock[0] += 1800 + cache["A"] = True + clock[0] += 1900 + cache["C"] = True + + self.assertEqual(list(OrderedDict.keys(cache)), ["A", "C"]) + self.assertIn("A", cache) + + def test_default_seen_report_cache_is_bounded(self): + """The factory builds a `SeenReportCache` with a one-hour expiry and + a hard ceiling of 100,000 entries (it was 100 million).""" + cache = parsedmarc_config._new_seen_aggregate_report_ids() + + self.assertIsInstance(cache, parsedmarc_config.SeenReportCache) + self.assertEqual(cache.max_len, 100_000) + self.assertEqual(cache.max_age, 3600) + self.assertIsInstance( + parsedmarc_config.SEEN_AGGREGATE_REPORT_IDS, + parsedmarc_config.SeenReportCache, + ) + + class TestParserConfigPickling(unittest.TestCase): """Covers the pickle strategy documented on `ParserConfig`: option fields round-trip, but cache contents never cross process boundaries and the diff --git a/tests/test_init.py b/tests/test_init.py index a83c6b98..182c9a42 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -41,10 +41,14 @@ from tempfile import NamedTemporaryFile, mkdtemp from typing import Any, BinaryIO, cast from unittest.mock import MagicMock, patch +import dns.exception +import dns.resolver +import xmltodict from lxml import etree # type: ignore[import-untyped] import parsedmarc import parsedmarc.constants as constants +from parsedmarc.constants import MAX_MESSAGE_COUNT from parsedmarc.attempts import MAX_PARSE_ATTEMPTS, attempt_key from parsedmarc.mail import IMAPConnection, MaildirConnection, MSGraphConnection from parsedmarc.parallel import _parse_report_email_job @@ -2960,6 +2964,329 @@ class TestMalformedXmlRecovery(unittest.TestCase): self.assertIn("DOCTYPE", str(ctx.exception)) +def _aggregate_xml_with_sources(ips: list[str]) -> str: + """Build an aggregate report with one record per entry of ``ips``.""" + records = "".join( + f"{ip}1" + "nonepass" + "pass" + "example.com" + "example.compass" + "" + for ip in ips + ) + return ( + 'T' + "t@example.netdns-budget" + "17040672001704153599" + "" + f"example.com

none

{records}" + "" + ) + + +class _FakePTR: + """The one thing ``query_dns()`` reads from an answer's rdata.""" + + def __init__(self, text: str): + self._text = text + + def to_text(self) -> str: + return self._text + + +class TestReverseDNSBudget(unittest.TestCase): + """Covers the per-report bounds on reverse DNS lookups + (GHSA-rwfh-ww9c-jm9m). DNS is mocked at ``dns.resolver.Resolver.resolve``; + the source IPs are in 8.8.8.0/24, which the bundled IP database maps to + AS15169, so local-only enrichment is observable.""" + + IPS = [f"8.8.8.{i}" for i in range(1, 51)] + + def setUp(self): + parsedmarc.IP_ADDRESS_CACHE.clear() + self.addCleanup(parsedmarc.IP_ADDRESS_CACHE.clear) + + def _parse(self, ips: list[str]) -> dict[str, Any]: + return cast( + dict[str, Any], + parsedmarc.parse_aggregate_report_xml( + _aggregate_xml_with_sources(ips), + always_use_local_files=True, + nameservers=["192.0.2.254"], + ), + ) + + def test_unanswered_lookups_stop_at_the_budget(self): + """Once failed lookups have used the report's budget, the remaining + records are enriched from the local IP database and reverse DNS map + without DNS, one + warning is logged, and those results are not cached. + + Before the bound, every record made its own lookup: 50 calls here, + and a measured 2.2 s per record with one unanswering nameserver and + 8.2 s with the four default ones, so a 3,500-record report took + hours.""" + + def unanswered(*args, **kwargs): + time.sleep(0.02) + raise dns.exception.Timeout() + + with ( + patch.object( + dns.resolver.Resolver, "resolve", side_effect=unanswered + ) as resolve, + patch.object(parsedmarc, "MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT", 0.05), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + report = self._parse(self.IPS) + + self.assertLess(resolve.call_count, 10) + records = report["records"] + self.assertEqual([r["source"]["ip_address"] for r in records], self.IPS) + for r in records: + self.assertIsNone(r["source"]["reverse_dns"]) + self.assertEqual(r["source"]["asn"], 15169) + self.assertEqual(r["source"]["country"], "US") + warnings = [ + m + for m in cm.output + if "Reverse DNS lookups for this report failed or were slow" in m + ] + self.assertEqual(len(warnings), 1) + self.assertIn( + "the remaining records are enriched from the local IP database " + "and reverse DNS map only", + warnings[0], + ) + looked_up = self.IPS[: resolve.call_count] + for ip in self.IPS[resolve.call_count :]: + self.assertNotIn(ip, parsedmarc.IP_ADDRESS_CACHE) + # The IPs that were looked up went through the cache as before. + for ip in looked_up: + self.assertIn(ip, parsedmarc.IP_ADDRESS_CACHE) + + def test_repeated_ip_is_looked_up_once_per_report(self): + """A source IP repeated in many records is looked up once per report, + even when its result is never cached: 9.9.9.9 has no PTR here and its + AS domain (quad9.net) is not in the reverse DNS map, so + ``get_ip_address_info()`` keeps the weak ASN fallback out of the IP + cache and, before the per-report memo, looked it up in every record + (20 calls here).""" + with patch.object( + dns.resolver.Resolver, "resolve", side_effect=dns.resolver.NXDOMAIN() + ) as resolve: + report = self._parse(["9.9.9.9"] * 20) + + self.assertEqual(resolve.call_count, 1) + self.assertNotIn("9.9.9.9", parsedmarc.IP_ADDRESS_CACHE) + self.assertEqual(len(report["records"]), 20) + for r in report["records"]: + self.assertEqual(r["source"]["asn"], 19281) + self.assertEqual(r["source"]["name"], "Quad9") + + def test_records_without_a_budget_share_one(self): + """``_parse_report_records()`` called without ``dns_budget`` gives + all of its records one shared budget, not one per record, so a + repeated IP is still looked up once.""" + record = xmltodict.parse(_aggregate_xml_with_sources(["9.9.9.9"]))["feedback"][ + "record" + ] + config = parsedmarc.ParserConfig( + always_use_local_files=True, nameservers=["192.0.2.254"] + ) + begin = datetime(2024, 1, 1, tzinfo=timezone.utc) + with patch.object( + dns.resolver.Resolver, "resolve", side_effect=dns.resolver.NXDOMAIN() + ) as resolve: + records = parsedmarc._parse_report_records( + [record] * 5, + config=config, + is_rfc_9990=False, + begin_dt=begin, + end_dt=begin + timedelta(days=1), + normalize=False, + keep_alive=None, + ) + + self.assertEqual(resolve.call_count, 1) + self.assertEqual(len(records), 5) + + def test_slow_answered_lookups_are_charged(self): + """A sender who controls a reverse zone can answer every PTR query + with a name just before the timeout. Before slow lookups were + charged, only lookups that returned no PTR used the budget, so such + a report made every lookup at full cost: 50 calls here. A lookup + that takes at least SLOW_REVERSE_DNS_SECONDS is now charged whether + or not it found a name, so lookups stop at the budget, one warning + is logged, and the remaining records are enriched offline.""" + + def slow_answer(qname, *args, **kwargs): + time.sleep(0.02) + return [_FakePTR(f"host-{str(qname).split('.')[0]}.attacker.example.")] + + with ( + patch.object( + dns.resolver.Resolver, "resolve", side_effect=slow_answer + ) as resolve, + patch.object(parsedmarc, "SLOW_REVERSE_DNS_SECONDS", 0.01), + patch.object(parsedmarc, "MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT", 0.05), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + report = self._parse(self.IPS) + + self.assertLess(resolve.call_count, 10) + records = report["records"] + self.assertEqual([r["source"]["ip_address"] for r in records], self.IPS) + for r in records[: resolve.call_count]: + self.assertTrue(r["source"]["reverse_dns"].endswith(".attacker.example")) + for r in records[resolve.call_count :]: + self.assertIsNone(r["source"]["reverse_dns"]) + self.assertEqual(r["source"]["asn"], 15169) + self.assertEqual(r["source"]["country"], "US") + warnings = [ + m + for m in cm.output + if "Reverse DNS lookups for this report failed or were slow" in m + ] + self.assertEqual(len(warnings), 1) + + def test_source_ip_zone_id_is_stripped(self): + """An IPv6 zone ID (fe80::1%eth0) is local to the reporting host and + the PostgreSQL inet type cannot hold one (GHSA-fxr7-7h9q-v949). + Everything from the first "%" is removed from a record's source IP, + with one warning naming the field, and the record is kept with the + bare address; ``%25eth0`` (a URI-style encoded "%") and a bare "%" + give the same address.""" + for source_ip in ("fe80::1%eth0", "fe80::1%25eth0", "fe80::1%"): + with self.subTest(source_ip=source_ip): + parsedmarc.IP_ADDRESS_CACHE.clear() + with ( + patch.object( + dns.resolver.Resolver, + "resolve", + side_effect=dns.resolver.NXDOMAIN(), + ) as resolve, + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + report = self._parse([source_ip, "8.8.8.8"]) + + self.assertEqual( + [r["source"]["ip_address"] for r in report["records"]], + ["fe80::1", "8.8.8.8"], + ) + self.assertEqual(resolve.call_count, 2) + self.assertEqual( + str(resolve.call_args_list[0].args[0]).lower(), + "1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f" + ".ip6.arpa.", + ) + zone_warnings = [m for m in cm.output if "zone ID" in m] + self.assertEqual( + zone_warnings, + [ + "WARNING:parsedmarc.log:Removed the IPv6 zone ID from " + f"source_ip: {source_ip}" + ], + ) + + def test_source_ip_that_is_not_an_ip_after_stripping_is_skipped(self): + """The text left once the zone ID is removed must still be an IP + address: ``not-an-ip%eth0`` and a lone "%" leave a record that is + skipped with the "Could not parse record" warning, as any other + invalid source IP is, and the report's other records are kept.""" + for source_ip in ("not-an-ip%eth0", "%"): + with self.subTest(source_ip=source_ip): + with ( + patch.object( + dns.resolver.Resolver, + "resolve", + side_effect=dns.resolver.NXDOMAIN(), + ), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + report = self._parse([source_ip, "8.8.8.8"]) + + self.assertEqual( + [r["source"]["ip_address"] for r in report["records"]], + ["8.8.8.8"], + ) + self.assertTrue( + any("Could not parse record" in m for m in cm.output), cm.output + ) + + def test_fast_answered_lookups_stop_at_the_total_budget(self): + """Every lookup's time is charged to the total budget + (MAX_REVERSE_DNS_SECONDS_PER_REPORT), so a reverse zone that answers + every query with a name just under SLOW_REVERSE_DNS_SECONDS, which + the failed-or-slow budget never charges, is still cut off. Before + the total budget, all 50 lookups here were made (with a fake clock, + 10,000 such lookups spent 9,900 s and left the budget untouched). + With each answer taking 0.99 s on a fake clock and a 5 s total, + lookups stop after the sixth, one warning names the total limit, + and the remaining records are enriched offline.""" + clock = [0.0] + + def slow_answer(qname, *args, **kwargs): + clock[0] += parsedmarc.SLOW_REVERSE_DNS_SECONDS - 0.01 + return [_FakePTR(f"host-{str(qname).split('.')[0]}.attacker.example.")] + + with ( + patch.object( + dns.resolver.Resolver, "resolve", side_effect=slow_answer + ) as resolve, + patch.object(parsedmarc.time, "monotonic", side_effect=lambda: clock[0]), + patch.object(parsedmarc, "MAX_REVERSE_DNS_SECONDS_PER_REPORT", 5.0), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + report = self._parse(self.IPS) + + self.assertEqual(resolve.call_count, 6) + records = report["records"] + self.assertEqual([r["source"]["ip_address"] for r in records], self.IPS) + for r in records[:6]: + self.assertTrue(r["source"]["reverse_dns"].endswith(".attacker.example")) + for r in records[6:]: + self.assertIsNone(r["source"]["reverse_dns"]) + self.assertEqual(r["source"]["asn"], 15169) + budget_warnings = [m for m in cm.output if "Reverse DNS lookups" in m] + self.assertEqual( + budget_warnings, + [ + "WARNING:parsedmarc.log:Reverse DNS lookups for this report took " + "more than 5 seconds; the remaining records are enriched from " + "the local IP database and reverse DNS map only" + ], + ) + + def test_answered_lookups_are_not_charged(self): + """Lookups that find a PTR faster than SLOW_REVERSE_DNS_SECONDS do + not use the failed-or-slow budget, however many there are (they + count toward the total budget only, which 50 fast lookups are far + from): every record keeps its reverse DNS and nothing is logged.""" + + def answered(qname, *args, **kwargs): + time.sleep(0.002) + return [_FakePTR(f"host-{str(qname).split('.')[0]}.google.com.")] + + with ( + patch.object( + dns.resolver.Resolver, "resolve", side_effect=answered + ) as resolve, + patch.object(parsedmarc, "MAX_FAILED_REVERSE_DNS_SECONDS_PER_REPORT", 0.05), + self.assertNoLogs("parsedmarc.log", level="WARNING"), + ): + report = self._parse(self.IPS) + + self.assertEqual(resolve.call_count, 50) + for ip, r in zip(self.IPS, report["records"]): + last_octet = ip.rsplit(".", 1)[1] + self.assertEqual( + r["source"]["reverse_dns"], f"host-{last_octet}.google.com" + ) + self.assertEqual(r["source"]["base_domain"], "google.com") + + class TestPolicyPublishedEdgeCases(unittest.TestCase): """Tests for edge cases in policy_published parsing""" @@ -3846,6 +4173,44 @@ Test body""" self.assertIn("Arrival-Date", str(ctx.exception)) self.assertNotIn("Unexpected error", str(ctx.exception)) + def testSourceIPZoneIDIsStripped(self): + """An IPv6 zone ID (fe80::1%eth0) is local to the reporting host and + the PostgreSQL inet type cannot hold one (GHSA-fxr7-7h9q-v949). + Everything from the first "%" of Source-IP is removed, with one + warning naming the field, and the report is kept with the bare + address; ``%25eth0`` and a bare "%" give the same address.""" + for source_ip in ("fe80::1%eth0", "fe80::1%25eth0", "fe80::1%"): + with self.subTest(source_ip=source_ip): + report_str = self._make_feedback_report(**{"Source-IP": source_ip}) + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + report = parsedmarc.parse_failure_report( + report_str, + self._make_sample(), + self._default_msg_date(), + offline=True, + ) + self.assertEqual(report["source"]["ip_address"], "fe80::1") + self.assertIn( + "WARNING:parsedmarc.log:Removed the IPv6 zone ID from " + f"Source-IP: {source_ip}", + cm.output, + ) + + def testSourceIPNotAnIPAfterStrippingIsInvalid(self): + """The text left once the zone ID is removed must still be an IP + address; otherwise the report is invalid, as with any other + Source-IP that is not one.""" + for source_ip in ("not-an-ip%eth0", "%"): + with self.subTest(source_ip=source_ip): + report_str = self._make_feedback_report(**{"Source-IP": source_ip}) + with self.assertRaises(parsedmarc.InvalidFailureReport): + parsedmarc.parse_failure_report( + report_str, + self._make_sample(), + self._default_msg_date(), + offline=True, + ) + def testMissingUserAgent(self): """Missing user_agent defaults to None""" report_str = self._make_feedback_report() @@ -4046,6 +4411,206 @@ class TestSmtpTlsReportErrors(unittest.TestCase): self.assertIsInstance(ctx.exception.__cause__, KeyError) +def _aggregate_xml_with_counts(counts: list[str]) -> str: + """Build an aggregate report with one record per entry of ``counts``.""" + records = "".join( + f"192.0.2.{i}{count}" + "nonepass" + "pass" + "example.com" + "example.compass" + "" + for i, count in enumerate(counts, start=1) + ) + return ( + 'T' + "t@example.netcounts" + "17040672001704153599" + "" + f"example.com

none

{records}" + "" + ) + + +def _smtp_tls_report_json(summary=None, failure_details=None) -> str: + """Build an SMTP TLS report with one policy.""" + return json.dumps( + { + "organization-name": "Org", + "date-range": { + "start-datetime": "2024-01-01T00:00:00Z", + "end-datetime": "2024-01-02T00:00:00Z", + }, + "contact-info": "a@b.example", + "report-id": "r1", + "policies": [ + { + "policy": { + "policy-type": "sts", + "policy-domain": "example.com", + }, + "summary": summary + or { + "total-successful-session-count": 10, + "total-failure-session-count": 1, + }, + "failure-details": failure_details or [], + } + ], + } + ) + + +class TestValuesTypedOutputsReject(unittest.TestCase): + """GHSA-fxr7-7h9q-v949: values the PostgreSQL output's typed columns + (and the Elasticsearch/OpenSearch Integer and ip mappings) reject are + kept out of the parsed report. Before the fix, an aggregate of + 3000000000 failed the PostgreSQL save with "integer out of range" and an + SMTP TLS sending-mta-ip of "not-an-ip" with "invalid input syntax for + type inet", and either failure held back the whole batch.""" + + def _parse_aggregate(self, counts: list[str]) -> dict[str, Any]: + return cast( + dict[str, Any], + parsedmarc.parse_aggregate_report_xml( + _aggregate_xml_with_counts(counts), + always_use_local_files=True, + offline=True, + ), + ) + + def test_record_count_outside_32_bit_range_is_skipped(self): + for bad in ("3000000000", str(MAX_MESSAGE_COUNT + 1), "-1"): + with self.subTest(count=bad): + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + report = self._parse_aggregate(["5", bad, "7"]) + self.assertEqual([r["count"] for r in report["records"]], [5, 7]) + warnings = [m for m in cm.output if "Could not parse record" in m] + self.assertEqual(len(warnings), 1) + self.assertIn(f"count {bad} is outside", warnings[0]) + + def test_record_counts_at_the_bounds_are_kept(self): + report = self._parse_aggregate(["0", str(MAX_MESSAGE_COUNT)]) + self.assertEqual( + [r["count"] for r in report["records"]], [0, MAX_MESSAGE_COUNT] + ) + + def test_single_record_with_bad_count_invalidates_the_report(self): + with self.assertRaises(parsedmarc.InvalidAggregateReport): + self._parse_aggregate(["3000000000"]) + + def test_record_with_bad_count_is_dropped_before_its_dns_lookup(self): + """The count is checked before the source IP's reverse DNS lookup, + so a record that is dropped for its count does not spend lookup + time (up to 8 s with unanswering nameservers). DNS is mocked at + ``dns.resolver.Resolver.resolve``; before the reorder, the lookup for + 192.0.2.2 ran and its record was dropped afterwards.""" + parsedmarc.IP_ADDRESS_CACHE.clear() + self.addCleanup(parsedmarc.IP_ADDRESS_CACHE.clear) + with patch.object( + dns.resolver.Resolver, "resolve", side_effect=dns.resolver.NXDOMAIN() + ) as resolve: + report = parsedmarc.parse_aggregate_report_xml( + _aggregate_xml_with_counts(["5", "3000000000", "7"]), + always_use_local_files=True, + nameservers=["192.0.2.254"], + ) + + self.assertEqual([r["count"] for r in report["records"]], [5, 7]) + queried = {str(c.args[0]) for c in resolve.call_args_list} + self.assertIn("1.2.0.192.in-addr.arpa.", queried) + self.assertIn("3.2.0.192.in-addr.arpa.", queried) + self.assertNotIn("2.2.0.192.in-addr.arpa.", queried) + + def test_smtp_tls_session_count_outside_32_bit_range_is_rejected(self): + cases = { + "total-successful-session-count": _smtp_tls_report_json( + summary={ + "total-successful-session-count": MAX_MESSAGE_COUNT + 1, + "total-failure-session-count": 0, + } + ), + "total-failure-session-count": _smtp_tls_report_json( + summary={ + "total-successful-session-count": 0, + "total-failure-session-count": -1, + } + ), + "failed-session-count": _smtp_tls_report_json( + failure_details=[ + { + "result-type": "certificate-expired", + "failed-session-count": 3000000000, + } + ] + ), + } + for field, report in cases.items(): + with self.subTest(field=field): + with self.assertRaises(parsedmarc.InvalidSMTPTLSReport) as ctx: + parsedmarc.parse_smtp_tls_report_json(report) + self.assertIn( + f"{field} must be between 0 and {MAX_MESSAGE_COUNT}", + str(ctx.exception), + ) + + def test_smtp_tls_session_count_at_the_bound_is_kept(self): + parsed = parsedmarc.parse_smtp_tls_report_json( + _smtp_tls_report_json( + summary={ + "total-successful-session-count": MAX_MESSAGE_COUNT, + "total-failure-session-count": 0, + } + ) + ) + self.assertEqual( + parsed["policies"][0]["successful_session_count"], MAX_MESSAGE_COUNT + ) + + def test_smtp_tls_ip_that_is_not_an_ip_address_is_dropped(self): + """RFC 8460 §4.4 defines sending-mta-ip and receiving-ip as the IP + address of the sending and receiving MTA; both are optional.""" + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + parsed = parsedmarc.parse_smtp_tls_report_json( + _smtp_tls_report_json( + failure_details=[ + { + "result-type": "certificate-expired", + "failed-session-count": 1, + "sending-mta-ip": "not-an-ip\r\nFAKE", + "receiving-ip": "2001:db8::1", + }, + { + "result-type": "certificate-expired", + "failed-session-count": 1, + "sending-mta-ip": "203.0.113.5", + "receiving-ip": "fe80::1%eth0", + }, + ] + ) + ) + first, second = cast(dict[str, Any], parsed)["policies"][0]["failure_details"] + self.assertNotIn("sending_mta_ip", first) + self.assertEqual(first["receiving_ip"], "2001:db8::1") + self.assertEqual(second["sending_mta_ip"], "203.0.113.5") + self.assertNotIn("receiving_ip", second) + self.assertTrue( + any( + "Ignoring sending-mta-ip that is not an IP address: " + "not-an-ip\\r\\nFAKE" in m + for m in cm.output + ), + cm.output, + ) + self.assertTrue( + any( + "Ignoring receiving-ip that is not an IP address: fe80::1%eth0" in m + for m in cm.output + ), + cm.output, + ) + + class TestBucketIntervalEdgeCases(unittest.TestCase): """Tests for _bucket_interval_by_day edge cases""" @@ -7820,6 +8385,46 @@ class TestAggregateReportIdentityIsDomainScoped(unittest.TestCase): ) +class TestAggregateReportDedupKey(unittest.TestCase): + """``_aggregate_report_dedup_key()`` returns a fixed-size digest of the + reporter, Report-ID and policy domain (GHSA-9fw6-mwqr-p43m). + + The key used to be the JSON of the three sender-controlled fields, so a + report with a multi-megabyte ```` kept that many bytes in the + seen-report cache. + """ + + @staticmethod + def _key( + policy_domain: str = "a.example", + report_id: str = DOMAIN_SCOPE_REPORT_ID, + ) -> str: + report = parsedmarc.parse_aggregate_report_xml( + _domain_scope_aggregate_xml(policy_domain, 1, report_id=report_id), + offline=True, + ) + return parsedmarc._aggregate_report_dedup_key(report) + + def test_key_size_does_not_depend_on_report_id_size(self): + """A 1 MiB Report-ID still yields a 64-character hex key.""" + report_id = "x" * (1024 * 1024) + + key = self._key(report_id=report_id) + + self.assertEqual(len(key), 64) + self.assertEqual(set(key) - set("0123456789abcdef"), set()) + + def test_domain_case_is_folded_before_hashing(self): + """RFC 4343 §3: domain names compare without ASCII case, so two + reports differing only in the policy domain's case share one key.""" + self.assertEqual(self._key("A.Example"), self._key("a.example")) + + def test_different_report_ids_give_different_keys(self): + self.assertNotEqual( + self._key(report_id="id-one"), self._key(report_id="id-two") + ) + + class TestAggregateReportIdentityDomainScopePendingSave(unittest.TestCase): """The same domain-scoped identity contract on the mailbox path that defers its dedup-cache writes until the batch is known to be saved. diff --git a/tests/test_loganalytics.py b/tests/test_loganalytics.py index 5fbcdb12..2e8d2843 100644 --- a/tests/test_loganalytics.py +++ b/tests/test_loganalytics.py @@ -1,11 +1,18 @@ """Tests for parsedmarc.loganalytics""" +import json import unittest from unittest.mock import MagicMock, patch -from azure.core.exceptions import HttpResponseError +from azure.core.exceptions import ( + AzureError, + HttpResponseError, + ServiceRequestError, + ServiceResponseError, +) from parsedmarc.loganalytics import ( + MAX_UPLOAD_ITEM_BYTES, LogAnalyticsClient, LogAnalyticsConfig, LogAnalyticsException, @@ -27,6 +34,26 @@ def _valid_kwargs(**overrides): return base +def _json_size(report): + """The size LogsIngestionClient.upload() measures a report at.""" + return len(json.dumps(report).encode("utf-8")) + + +def _aggregate_report(org_name, report_id, size=None): + """An aggregate report, padded with records to ``size`` bytes of JSON.""" + report = { + "report_metadata": {"org_name": org_name, "report_id": report_id}, + "policy_published": {"domain": "example.com"}, + "records": [], + } + if size is not None: + padding = size - _json_size(report) + # One record of the required length: ``"x"`` is 3 bytes of JSON, + # and each extra character of the string adds one + report["records"].append("x" * (padding - 2)) + return report + + class TestLogAnalyticsConfig(unittest.TestCase): """The config dataclass holds every credential and stream needed to push to Log Analytics. A typo on any attribute would silently @@ -95,6 +122,90 @@ class TestPublishJson(unittest.TestCase): self.assertIn("forbidden", str(ctx.exception)) +class TestPublishJsonSizeLimit(unittest.TestCase): + """The Logs Ingestion API rejects a call over 1 MB, and the SDK sends a + report larger than its 1 MiB chunk as a call of its own, so one + oversize report used to fail the upload of every report in the batch + (GHSA-rg4h-58fm-8pc9). Such a report is now left out with a warning.""" + + def test_oversize_report_is_skipped_and_the_rest_uploaded(self): + client = LogAnalyticsClient(**_valid_kwargs()) + logs_client = MagicMock() + big = _aggregate_report("Big Sender", "big-1", size=1_200_000) + small = _aggregate_report("Small Sender", "small-1") + self.assertGreater(_json_size(big), MAX_UPLOAD_ITEM_BYTES) + with self.assertLogs("parsedmarc.log", level="WARNING") as logs: + published = client.publish_json([big, small], logs_client, "agg-stream") + logs_client.upload.assert_called_once_with("dcr-123", "agg-stream", [small]) + self.assertEqual(published, 1) + self.assertEqual(len(logs.output), 1) + self.assertIn(f"{_json_size(big)} bytes", logs.output[0]) + self.assertIn("agg-stream", logs.output[0]) + self.assertIn("Big Sender", logs.output[0]) + self.assertIn("big-1", logs.output[0]) + + def test_all_reports_oversize_uploads_nothing(self): + client = LogAnalyticsClient(**_valid_kwargs()) + logs_client = MagicMock() + reports = [ + _aggregate_report("Sender A", "a-1", size=1_100_000), + _aggregate_report("Sender B", "b-1", size=1_100_000), + ] + for report in reports: + self.assertGreater(_json_size(report), MAX_UPLOAD_ITEM_BYTES) + with self.assertLogs("parsedmarc.log", level="WARNING") as logs: + published = client.publish_json(reports, logs_client, "agg-stream") + logs_client.upload.assert_not_called() + self.assertEqual(published, 0) + self.assertEqual(len(logs.output), 2) + self.assertIn("a-1", logs.output[0]) + self.assertIn("b-1", logs.output[1]) + + def test_report_just_under_the_limit_is_uploaded(self): + client = LogAnalyticsClient(**_valid_kwargs()) + logs_client = MagicMock() + report = _aggregate_report("Sender", "r-1", size=MAX_UPLOAD_ITEM_BYTES - 1) + self.assertEqual(_json_size(report), MAX_UPLOAD_ITEM_BYTES - 1) + client.publish_json([report], logs_client, "agg-stream") + logs_client.upload.assert_called_once_with("dcr-123", "agg-stream", [report]) + + def test_report_one_byte_over_the_limit_is_skipped(self): + client = LogAnalyticsClient(**_valid_kwargs()) + logs_client = MagicMock() + report = _aggregate_report("Sender", "r-1", size=MAX_UPLOAD_ITEM_BYTES + 1) + self.assertEqual(_json_size(report), MAX_UPLOAD_ITEM_BYTES + 1) + with self.assertLogs("parsedmarc.log", level="WARNING"): + client.publish_json([report], logs_client, "agg-stream") + logs_client.upload.assert_not_called() + + def test_warning_names_smtp_tls_and_failure_reports(self): + """SMTP TLS reports are named by organization and report ID, and + failure reports by reported domain and arrival date; a CR/LF a + sender planted in a name is escaped so it cannot forge a line.""" + client = LogAnalyticsClient(**_valid_kwargs()) + logs_client = MagicMock() + padding = "x" * 1_100_000 + tls = { + "organization_name": "TLS\r\nForged line", + "report_id": "tls-1", + "policies": [padding], + } + failure = { + "reported_domain": "example.org", + "arrival_date_utc": "2026-11-10 12:00:00", + "sample": padding, + } + with self.assertLogs("parsedmarc.log", level="WARNING") as logs: + client.publish_json([tls], logs_client, "tls-stream") + client.publish_json([failure], logs_client, "fail-stream") + logs_client.upload.assert_not_called() + self.assertIn("organization_name TLS\\r\\nForged line", logs.output[0]) + self.assertNotIn("\n", logs.output[0]) + self.assertIn("report_id tls-1", logs.output[0]) + self.assertIn("reported_domain example.org", logs.output[1]) + self.assertIn("arrival_date_utc 2026-11-10 12:00:00", logs.output[1]) + + class TestPublishResults(unittest.TestCase): """publish_results gates each report type behind both a config flag (save_aggregate / save_failure / save_smtp_tls) and a configured @@ -206,6 +317,103 @@ class TestPublishResults(unittest.TestCase): ) logs_client.upload.assert_not_called() + def test_stream_whose_reports_are_all_oversize_logs_no_success(self): + """A stream whose every report is over the size limit uploads + nothing, so it must not log "Successfully pushed" (it used to, + after the per-report warnings), while the other streams still + upload and log their success.""" + big = _aggregate_report("Big Sender", "big-1", size=1_100_000) + with self.assertLogs("parsedmarc.log", level="INFO") as logs: + logs_client = self._publish_with( + { + "aggregate_reports": [big], + "failure_reports": [{"id": "f"}], + "smtp_tls_reports": [{"id": "t"}], + } + ) + streams_uploaded = [call.args[1] for call in logs_client.upload.call_args_list] + self.assertEqual(streams_uploaded, ["fail-stream", "tls-stream"]) + messages = [record.getMessage() for record in logs.records] + self.assertNotIn("Successfully pushed aggregate reports.", messages) + self.assertIn("Successfully pushed failure reports.", messages) + self.assertIn("Successfully pushed SMTP TLS reports.", messages) + self.assertTrue(any("big-1" in message for message in messages)) + + def _publish_failing( + self, failing_streams, error_class: type[AzureError] = HttpResponseError + ): + """Publishes one report of each kind with upload() raising + ``error_class`` for the streams in ``failing_streams``.""" + + def upload(rule_id, stream_name, logs): + if stream_name in failing_streams: + raise error_class(f"rejected by {stream_name}") + + client = LogAnalyticsClient(**_valid_kwargs()) + with ( + patch("parsedmarc.loganalytics.ClientSecretCredential"), + patch("parsedmarc.loganalytics.LogsIngestionClient") as mock_client_cls, + ): + mock_logs_client = mock_client_cls.return_value + mock_logs_client.upload.side_effect = upload + with self.assertRaises(LogAnalyticsException) as ctx: + client.publish_results( + { + "aggregate_reports": [{"id": "a"}], + "failure_reports": [{"id": "f"}], + "smtp_tls_reports": [{"id": "t"}], + }, + save_aggregate=True, + save_failure=True, + save_smtp_tls=True, + ) + streams = [call.args[1] for call in mock_logs_client.upload.call_args_list] + return streams, str(ctx.exception) + + def test_failed_aggregate_upload_does_not_stop_other_streams(self): + """GHSA-rg4h-58fm-8pc9: a failed aggregate upload returned before + the failure and SMTP TLS streams were sent.""" + streams, message = self._publish_failing({"agg-stream"}) + self.assertEqual(streams, ["agg-stream", "fail-stream", "tls-stream"]) + self.assertIn("aggregate reports", message) + self.assertIn("rejected by agg-stream", message) + self.assertNotIn("failure reports", message) + self.assertNotIn("SMTP TLS reports", message) + + def test_connection_error_does_not_stop_other_streams(self): + """upload() re-raises any exception, and a connection reset or + timeout is a ServiceRequestError or ServiceResponseError: an + AzureError but not an HttpResponseError, so it escaped + publish_results at the first stream and the other two were never + attempted.""" + for error_class in (ServiceRequestError, ServiceResponseError): + with self.subTest(error_class=error_class.__name__): + streams, message = self._publish_failing( + {"agg-stream"}, error_class=error_class + ) + self.assertEqual(streams, ["agg-stream", "fail-stream", "tls-stream"]) + self.assertEqual( + message, "aggregate reports: Upload failed: rejected by agg-stream" + ) + + def test_two_failed_streams_raise_one_exception_naming_both(self): + streams, message = self._publish_failing({"agg-stream", "tls-stream"}) + self.assertEqual(streams, ["agg-stream", "fail-stream", "tls-stream"]) + self.assertIn( + "aggregate reports: Upload failed: rejected by agg-stream", message + ) + self.assertIn( + "SMTP TLS reports: Upload failed: rejected by tls-stream", message + ) + self.assertNotIn("failure reports", message) + + def test_failed_failure_upload_does_not_stop_smtp_tls_stream(self): + streams, message = self._publish_failing({"fail-stream"}) + self.assertEqual(streams, ["agg-stream", "fail-stream", "tls-stream"]) + self.assertEqual( + message, "failure reports: Upload failed: rejected by fail-stream" + ) + def test_missing_aggregate_stream_skips_aggregate(self): """If the operator hasn't configured a stream for one of the report types, the corresponding publish branch is skipped diff --git a/tests/test_postgres.py b/tests/test_postgres.py index 0274ae16..4378f492 100644 --- a/tests/test_postgres.py +++ b/tests/test_postgres.py @@ -7,11 +7,20 @@ the bound parameters that a real PostgreSQL server would receive, plus the real-sample round trip, so the tests fail if the dict-key mapping regresses. """ +import contextlib import importlib.util import os +import shlex +import socket +import struct import sys +import tempfile +import threading +import traceback import unittest +import urllib.parse from glob import glob +from types import SimpleNamespace from typing import cast from unittest.mock import MagicMock, patch @@ -20,34 +29,132 @@ from parsedmarc.postgres import ( AlreadySaved, PostgreSQLClient, PostgreSQLError, + PostgreSQLRejectedReport, _contact_info_to_text, _ensure_utc_suffix, + _has_unencoded_at, _naive_local_to_timestamptz, _normalize_arrival_date, + _nul_safe_dumper_classes, + _register_nul_safe_dumpers, + _strip_nul, ) OFFLINE_MODE = os.environ.get("GITHUB_ACTIONS", "false").lower() == "true" +PSYCOPG_INSTALLED = importlib.util.find_spec("psycopg") is not None + + +class _FakeStrDumper: + """Stands in for a psycopg ``str`` dumper: encodes with no NUL check.""" + + def __init__(self, cls, context=None): + pass + + def dump(self, obj): + return obj.encode("utf-8") + + +class _FakeStrBinaryDumper(_FakeStrDumper): + pass + + +class _FakeConninfoError(Exception): + """Stands in for the ProgrammingError of a malformed connection string.""" + + +def _fake_conninfo_to_dict(conninfo: str = "") -> dict: + """Stands in for ``psycopg.conninfo.conninfo_to_dict`` when psycopg is + absent: parses ``key=value`` pairs (a value may be single-quoted, as + in libpq) and the host and query of a URI, + and, like libpq, raises on a word without ``=`` (quoting with that + word in the message). Like libpq, a URI's userinfo ends at the + *first* ``@``, and the authority at the first ``/`` (so + ``postgresql://u:x/y@db/d`` has host ``u``, port ``x`` and dbname + ``y@db/d``), so the rest of a password containing ``@`` is part of + the host, the port or the database name, as with + ``psycopg.conninfo.conninfo_to_dict`` 3.3 on libpq: + ``postgresql://u:s3c@pw@db.example/d`` has host ``pw@db.example``, + ``postgresql://u:s3c@pw:x@db.example/d`` host ``pw`` and port + ``x@db.example``, and ``postgresql://u:s3c@pw/x@db.example/d`` host + ``pw`` and dbname ``x@db.example/d``. Components are percent-decoded + (``u:s3c%40pw@db.example`` has password ``s3c@pw``).""" + unquote = urllib.parse.unquote + if conninfo.startswith(("postgresql://", "postgres://")): + try: + parts = urllib.parse.urlsplit(conninfo) + except ValueError: + raise _FakeConninfoError(f'invalid URI: "{conninfo}"') from None + params = {} + authority = parts.netloc + if "@" in authority: + userinfo, _, authority = authority.partition("@") + user, sep, password = userinfo.partition(":") + if user: + params["user"] = unquote(user) + if sep: + params["password"] = unquote(password) + if parts.path[1:]: + params["dbname"] = unquote(parts.path[1:]) + if authority.startswith("["): + host, _, rest = authority[1:].partition("]") + port = rest[1:] if rest.startswith(":") else "" + else: + host, _, port = authority.partition(":") + if host: + params["host"] = unquote(host) + if port: + params["port"] = unquote(port) + params.update(urllib.parse.parse_qsl(parts.query)) + return params + params = {} + for word in shlex.split(conninfo): + key, sep, value = word.partition("=") + if not sep: + raise _FakeConninfoError(f'missing "=" after "{word}"') + params[key] = value + return params + + # psycopg is an optional dependency and is not installed in CI (whose # unit-test job installs the [build,all] extras, deliberately without # postgresql). The save methods mock the connection, but the -# failure path also references ``psycopg_json.Jsonb`` at module scope, so -# mock that SDK boundary for the whole module when psycopg is absent. -_types_patcher = None +# failure path also references ``psycopg_json.Jsonb`` at module scope, and +# connecting subclasses the dumpers of ``psycopg_string`` and parses a +# connection string with ``psycopg_conninfo``, so mock those SDK boundaries +# for the whole module when psycopg is absent. +_types_patchers = [] def setUpModule(): - global _types_patcher import parsedmarc.postgres as pg if pg.psycopg_json is None: - _types_patcher = patch("parsedmarc.postgres.psycopg_json", MagicMock()) - _types_patcher.start() + _types_patchers.append(patch("parsedmarc.postgres.psycopg_json", MagicMock())) + if pg.psycopg_conninfo is None: + _types_patchers.append( + patch( + "parsedmarc.postgres.psycopg_conninfo", + SimpleNamespace(conninfo_to_dict=_fake_conninfo_to_dict), + ) + ) + if pg.psycopg_string is None: + _types_patchers.append( + patch( + "parsedmarc.postgres.psycopg_string", + SimpleNamespace( + StrDumperUnknown=_FakeStrDumper, + StrBinaryDumper=_FakeStrBinaryDumper, + ), + ) + ) + for patcher in _types_patchers: + patcher.start() def tearDownModule(): - if _types_patcher is not None: - _types_patcher.stop() + for patcher in _types_patchers: + patcher.stop() class TestPsycopgImportFallback(unittest.TestCase): @@ -60,8 +167,8 @@ class TestPsycopgImportFallback(unittest.TestCase): def _exec_fresh_module_with(self, modules): """Execute parsedmarc.postgres's source into a fresh, throwaway module object under a patched sys.modules, and return the - (psycopg, psycopg_json) it bound. The canonical module in - sys.modules is untouched, so the identity of its classes + (psycopg, psycopg_json, psycopg_string, psycopg_conninfo) it bound. + The canonical module in sys.modules is untouched, so the identity of its classes (PostgreSQLError, AlreadySaved, ...) that other tests hold references to is preserved.""" spec = importlib.util.find_spec("parsedmarc.postgres") @@ -69,30 +176,44 @@ class TestPsycopgImportFallback(unittest.TestCase): mod = importlib.util.module_from_spec(spec) with patch.dict(sys.modules, modules): spec.loader.exec_module(mod) - return mod.psycopg, mod.psycopg_json + return mod.psycopg, mod.psycopg_json, mod.psycopg_string, mod.psycopg_conninfo def test_missing_psycopg_falls_back_to_none(self): """A None entry in sys.modules makes ``import psycopg`` raise ImportError, exercising the fallback arm.""" - psycopg_mod, json_mod = self._exec_fresh_module_with( - {"psycopg": None, "psycopg.types": None, "psycopg.types.json": None} + psycopg_mod, json_mod, string_mod, conninfo_mod = self._exec_fresh_module_with( + { + "psycopg": None, + "psycopg.conninfo": None, + "psycopg.types": None, + "psycopg.types.json": None, + "psycopg.types.string": None, + } ) self.assertIsNone(psycopg_mod) self.assertIsNone(json_mod) + self.assertIsNone(string_mod) + self.assertIsNone(conninfo_mod) - def test_installed_psycopg_binds_module_and_json_submodule(self): + def test_installed_psycopg_binds_module_and_type_submodules(self): fake_json = MagicMock() - fake_types = MagicMock(json=fake_json) - fake_psycopg = MagicMock(types=fake_types) - psycopg_mod, json_mod = self._exec_fresh_module_with( + fake_string = MagicMock() + fake_types = MagicMock(json=fake_json, string=fake_string) + fake_conninfo = MagicMock() + fake_psycopg = MagicMock(types=fake_types, conninfo=fake_conninfo) + psycopg_mod, json_mod, string_mod, conninfo_mod = self._exec_fresh_module_with( { "psycopg": fake_psycopg, + "psycopg.conninfo": fake_conninfo, "psycopg.types": fake_types, "psycopg.types.json": fake_json, + "psycopg.types.string": fake_string, } ) self.assertIs(psycopg_mod, fake_psycopg) self.assertIs(json_mod, fake_json) + self.assertIs(string_mod, fake_string) + self.assertIs(conninfo_mod, fake_conninfo) class TestPostgreSQLHelpers(unittest.TestCase): @@ -224,6 +345,50 @@ class TestPostgreSQLHelpers(unittest.TestCase): self.assertEqual(_contact_info_to_text(123), "123") # pyright: ignore[reportArgumentType] +def _without_libpq_env(): + """Patch os.environ for the duration of a ``with`` block so that none of + libpq's ``PG*`` variables (PGSSLMODE, PGHOST, ...) from the developer's + shell reaches the code under test.""" + return patch.dict( + os.environ, + {k: v for k, v in os.environ.items() if not k.startswith("PG")}, + clear=True, + ) + + +def _without_early_refusal(): + """Patch out the shape check that refuses a URI connection string + before anything is logged (``_has_unencoded_at``), so a test exercises + what keeps the password out of the logs whatever shape gets past it: + withholding every value parsed from the string.""" + return patch("parsedmarc.postgres._has_unencoded_at", MagicMock(return_value=False)) + + +@contextlib.contextmanager +def _captured_log_text(): + """Capture every parsedmarc log record, DEBUG and up, into a list of + formatted strings (unlike assertLogs, an empty capture is allowed).""" + import logging + + from parsedmarc.log import logger + + records: list[str] = [] + + class _ListHandler(logging.Handler): + def emit(self, record): + records.append(self.format(record)) + + handler = _ListHandler(level=logging.DEBUG) + old_level = logger.level + logger.addHandler(handler) + logger.setLevel(logging.DEBUG) + try: + yield records + finally: + logger.removeHandler(handler) + logger.setLevel(old_level) + + def _make_client(): """Create a PostgreSQLClient with a fully-mocked psycopg connection.""" with patch("parsedmarc.postgres.psycopg") as mock_psycopg: @@ -301,11 +466,17 @@ class TestPostgreSQLConstruction(unittest.TestCase): mock_reconnect.assert_called_once() def test_connect_uses_connection_string_when_provided(self): - """A DSN/URI is passed straight to psycopg.connect.""" - with patch("parsedmarc.postgres.psycopg") as mock_psycopg: + """A DSN/URI is passed straight to psycopg.connect, with the default + sslmode as a keyword argument because the string sets none.""" + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + ): mock_psycopg.Error = Exception PostgreSQLClient(connection_string="postgresql://u:p@h/db") - mock_psycopg.connect.assert_called_once_with("postgresql://u:p@h/db") + mock_psycopg.connect.assert_called_once_with( + "postgresql://u:p@h/db", sslmode="require" + ) def test_connect_failure_raises_postgresql_error(self): """A driver-level connection error is wrapped in PostgreSQLError.""" @@ -318,8 +489,8 @@ class TestPostgreSQLConstruction(unittest.TestCase): 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.""" + """With the separate arguments, a 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 @@ -328,15 +499,53 @@ class TestPostgreSQLConstruction(unittest.TestCase): 'connection to "postgresql://u:PGSECRET@db.example/x" failed' ) with self.assertRaises(PostgreSQLError) as ctx: - PostgreSQLClient(connection_string="postgresql://u:PGSECRET@h/db") + PostgreSQLClient(host="h", user="u", password="PGSECRET") message = str(ctx.exception) self.assertNotIn("PGSECRET", message) - self.assertIn("u:***@db.example/x", message) + self.assertIn('connection to "postgresql://u:***@db.example/x" failed', message) + + def test_connect_failure_is_not_chained_to_the_driver_error(self): + """The driver names the host when it cannot connect, and a host + with an "@" can hold part of a password (a URI connection string + with one is refused before connecting; the *host* argument is + passed on). The replacement message is credential-masked, and it is + raised from None: chaining printed the driver's own text, unmasked, + wherever the error was logged with its traceback (the SIGHUP reload + path's logger.exception).""" + operational_error = type("OperationalError", (Exception,), {}) + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + ): + mock_psycopg.Error = Exception + mock_psycopg.ProgrammingError = type("ProgrammingError", (Exception,), {}) + mock_psycopg.connect.side_effect = operational_error( + "failed to resolve host 'pw@db.example': [Errno -2] Name or " + "service not known" + ) + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient( + host="pw@db.example", password="s3c", sslmode="verify-full" + ) + exc = ctx.exception + rendered = "".join(traceback.format_exception(exc)) + for secret in ("s3c", "pw@"): + self.assertNotIn(secret, str(exc)) + self.assertNotIn(secret, rendered) + self.assertIn("***@db.example", str(exc)) + self.assertIsNone(exc.__cause__) + self.assertTrue(exc.__suppress_context__) + # The real host and password still reach the driver. + self.assertEqual(mock_psycopg.connect.call_args.kwargs["host"], "pw@db.example") + self.assertEqual(mock_psycopg.connect.call_args.kwargs["password"], "s3c") 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.""" + a fixed message that still identifies the problem. The string is + parsed before connecting too; that parse is stubbed to succeed here + so the error surfaces from psycopg.connect, the guard under test + (the pre-parse is tested in TestPostgreSQLTLS).""" programming_error = type("ProgrammingError", (Exception,), {}) for dsn, driver_message in ( ( @@ -350,7 +559,13 @@ class TestPostgreSQLConstruction(unittest.TestCase): ), ): with self.subTest(dsn=dsn): - with patch("parsedmarc.postgres.psycopg") as mock_psycopg: + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + patch( + "parsedmarc.postgres.psycopg_conninfo", + SimpleNamespace(conninfo_to_dict=lambda conninfo: {}), + ), + ): mock_psycopg.Error = Exception mock_psycopg.ProgrammingError = programming_error mock_psycopg.connect.side_effect = programming_error(driver_message) @@ -908,6 +1123,7 @@ class TestPostgreSQLSaveErrors(unittest.TestCase): cur.execute.side_effect = self._FakeDriverError("db boom") with patch("parsedmarc.postgres.psycopg") as mp: mp.Error = self._FakeDriverError + mp.DataError = type("DataError", (self._FakeDriverError,), {}) with self.assertRaises(PostgreSQLError) as ctx: getattr(client, method)(report) self.assertIn("db boom", str(ctx.exception)) @@ -1027,5 +1243,1219 @@ class TestEnsureUtcSuffixNonString(unittest.TestCase): ) +class TestNulReplacement(unittest.TestCase): + """GHSA-fxr7-7h9q-v949: PostgreSQL cannot store NUL in ``text`` or + ``jsonb``. psycopg's text-format ``str`` dumper raises ``DataError`` + ("PostgreSQL text fields cannot contain NUL (0x00) bytes") and the server + rejects ``\\u0000`` in ``jsonb``, so a failure report whose sample + Subject was ``=?utf-8?q?a=00b?=`` failed the save and held the batch. + NUL is now replaced with U+FFFD.""" + + def test_strip_nul_replaces_nested_strings_and_keys(self): + value = { + "Sub\x00ject": ["a\x00b", {"x": "\x00\x00"}], + "count": 3, + "none": None, + "flag": True, + } + self.assertEqual( + _strip_nul(value), + { + "Sub\ufffdject": ["a\ufffdb", {"x": "\ufffd\ufffd"}], + "count": 3, + "none": None, + "flag": True, + }, + ) + + def test_connect_registers_binary_then_text_nul_safe_dumper(self): + """psycopg uses the dumper registered last for a type as its + default, so the text dumper must be registered after the binary one. + Each registered class must replace NUL before its parent dumps.""" + _make_client_conn = MagicMock() + fake_string = SimpleNamespace( + StrDumperUnknown=_FakeStrDumper, StrBinaryDumper=_FakeStrBinaryDumper + ) + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + patch("parsedmarc.postgres.psycopg_string", fake_string), + ): + mock_psycopg.Error = Exception + mock_psycopg.connect.return_value = _make_client_conn + client = PostgreSQLClient(host="localhost") + self.assertIs(client._conn, _make_client_conn) + calls = _make_client_conn.adapters.register_dumper.call_args_list + self.assertEqual([c.args[0] for c in calls], [str, str]) + binary_cls, text_cls = (c.args[1] for c in calls) + self.assertTrue(issubclass(binary_cls, _FakeStrBinaryDumper)) + self.assertTrue(issubclass(text_cls, _FakeStrDumper)) + self.assertFalse(issubclass(text_cls, _FakeStrBinaryDumper)) + for cls in (binary_cls, text_cls): + self.assertEqual(cls(str, None).dump("a\x00b"), "a\ufffdb".encode()) + + @unittest.skipUnless(PSYCOPG_INSTALLED, "psycopg is not installed") + def test_real_psycopg_parent_dumpers_reject_or_keep_nul(self): + """The behavior being worked around: the text dumper raises + DataError and the binary one passes the NUL on to the server.""" + import psycopg + from psycopg.types.string import StrBinaryDumper, StrDumperUnknown + + with self.assertRaises(psycopg.DataError): + StrDumperUnknown(str, None).dump("a\x00b") + self.assertEqual(StrBinaryDumper(str, None).dump("a\x00b"), b"a\x00b") + + text_cls, binary_cls = _nul_safe_dumper_classes() + self.assertEqual(text_cls(str, None).dump("a\x00b"), "a\ufffdb".encode()) + self.assertEqual(binary_cls(str, None).dump("a\x00b"), "a\ufffdb".encode()) + + @unittest.skipUnless(PSYCOPG_INSTALLED, "psycopg is not installed") + def test_real_psycopg_dumps_str_and_str_list_without_nul(self): + """With the real adapters map, every format psycopg may pick for a + ``str`` parameter, and for a ``list[str]`` (``TEXT[]`` columns), + resolves to a NUL-safe dumper.""" + import psycopg + from psycopg.abc import AdaptContext + from psycopg.adapt import AdaptersMap, PyFormat, Transformer + + context = SimpleNamespace( + adapters=AdaptersMap(psycopg.adapters), connection=None + ) + _register_nul_safe_dumpers(cast(psycopg.Connection, context)) + transformer = Transformer(cast(AdaptContext, context)) + for fmt in (PyFormat.AUTO, PyFormat.TEXT, PyFormat.BINARY): + with self.subTest(fmt=fmt): + dumped = transformer.get_dumper("a\x00b", fmt).dump("a\x00b") + self.assertEqual(bytes(cast(bytes, dumped)), "a\ufffdb".encode()) + list_dumped = bytes( + cast( + bytes, + transformer.get_dumper(["a\x00b"], fmt).dump(["a\x00b"]), + ) + ) + self.assertNotIn(b"\x00b", list_dumped) + self.assertIn("a\ufffdb".encode(), list_dumped) + + def test_failure_report_jsonb_values_have_nul_replaced(self): + """The sample headers, From and To reach Jsonb with NUL replaced.""" + + class FakeJsonb: + def __init__(self, obj): + self.obj = obj + + client, mock_conn = _make_client() + cur = _mock_cursor(mock_conn, [None, (1,)]) + report = { + "arrival_date_utc": "2024-01-15 10:30:00", + "reported_domain": "example.com", + "source": {"ip_address": "203.0.113.1"}, + "parsed_sample": { + "subject": "a\x00b", + "headers": {"Subject": "a\x00b"}, + "from": {"display_name": "N\x00", "address": "x@example.com"}, + "to": [{"display_name": None, "address": "y\x00@example.com"}], + }, + } + with patch( + "parsedmarc.postgres.psycopg_json", SimpleNamespace(Jsonb=FakeJsonb) + ): + client.save_failure_report_to_postgresql(report) + insert = next( + c + for c in cur.execute.call_args_list + if "INSERT INTO dmarc_failure_report" in c.args[0] + ) + params = _named_params(insert) + self.assertEqual(params["sample_headers"].obj, {"Subject": "a\ufffdb"}) + self.assertEqual( + params["sample_from"].obj, + {"display_name": "N\ufffd", "address": "x@example.com"}, + ) + self.assertEqual( + params["sample_to"].obj, + [{"display_name": None, "address": "y\ufffd@example.com"}], + ) + # The input report is not modified. + self.assertEqual(report["parsed_sample"]["headers"], {"Subject": "a\x00b"}) + + +class TestPostgreSQLRejectedReport(unittest.TestCase): + """GHSA-fxr7-7h9q-v949: a value PostgreSQL rejects (``psycopg.DataError``) + raises PostgreSQLRejectedReport naming the report, so the CLI can skip + that one report instead of holding the batch; any other driver error + stays a plain PostgreSQLError.""" + + class _FakeError(Exception): + pass + + class _FakeDataError(_FakeError): + pass + + class _FakeOperationalError(_FakeError): + pass + + REPORTS = { + "save_aggregate_report_to_postgresql": ( + { + "report_metadata": {"org_name": "Example Inc.", "report_id": "agg-1"}, + "policy_published": {}, + "records": [], + }, + ("aggregate report from Example Inc.", "agg-1"), + ), + "save_failure_report_to_postgresql": ( + { + "reported_domain": "example.com", + "arrival_date_utc": "2024-01-15 10:30:00", + "parsed_sample": {}, + "source": {}, + }, + ("failure report for example.com", "2024-01-15 10:30:00+00"), + ), + "save_smtp_tls_report_to_postgresql": ( + {"organization_name": "TLS Org", "report_id": "tls-1", "policies": []}, + ("SMTP TLS report from TLS Org", "tls-1"), + ), + } + + def _save(self, method, report, error): + client, mock_conn = _make_client() + cur = _mock_cursor(mock_conn, []) + cur.execute.side_effect = error + with patch("parsedmarc.postgres.psycopg") as mp: + mp.Error = self._FakeError + mp.DataError = self._FakeDataError + with self.assertRaises(PostgreSQLError) as ctx: + getattr(client, method)(report) + return ctx.exception + + def test_data_error_raises_rejected_report_naming_the_report(self): + for method, (report, expected) in self.REPORTS.items(): + with self.subTest(method=method): + exc = self._save( + method, + report, + self._FakeDataError('invalid input syntax for type inet: "x"'), + ) + self.assertIsInstance(exc, PostgreSQLRejectedReport) + message = str(exc) + self.assertTrue(message.startswith("PostgreSQL rejected a value")) + for part in expected: + self.assertIn(part, message) + self.assertIn("invalid input syntax for type inet", message) + + def test_rejected_report_message_escapes_line_breaks(self): + """The org name and the driver text come from the sender, so a + CR/LF in them cannot forge a log line.""" + exc = self._save( + "save_aggregate_report_to_postgresql", + { + "report_metadata": {"org_name": "Evil\r\nFAKE", "report_id": "1"}, + "policy_published": {}, + "records": [], + }, + self._FakeDataError("bad value\nFAKE"), + ) + self.assertNotIn("\n", str(exc)) + self.assertNotIn("\r", str(exc)) + self.assertIn("Evil\\r\\nFAKE", str(exc)) + + def test_rejected_report_message_truncates_long_driver_text(self): + """A DataError can quote the rejected value, which the sender chose + and can make any length; the driver text in the message is cut to + 200 characters with an ellipsis, for every report type.""" + driver_text = 'invalid input syntax for type inet: "' + "A" * 5000 + '"' + for method, (report, expected) in self.REPORTS.items(): + with self.subTest(method=method): + exc = self._save(method, report, self._FakeDataError(driver_text)) + self.assertIsInstance(exc, PostgreSQLRejectedReport) + message = str(exc) + for part in expected: + self.assertIn(part, message) + self.assertTrue(message.endswith(": " + driver_text[:200] + "...")) + self.assertNotIn("A" * 201, message) + + def test_rejected_report_message_truncates_before_escaping(self): + """The driver text is cut before it is escaped, so the cut cannot + split an escape sequence: a line break at character 200 is kept + whole as ``\\n``, and one after it is cut off.""" + exc = self._save( + "save_aggregate_report_to_postgresql", + self.REPORTS["save_aggregate_report_to_postgresql"][0], + self._FakeDataError("x" * 199 + "\n" + "\r" * 50), + ) + self.assertTrue(str(exc).endswith(": " + "x" * 199 + "\\n...")) + self.assertNotIn("\\r", str(exc)) + + def test_rejected_report_message_keeps_short_driver_text_whole(self): + exc = self._save( + "save_aggregate_report_to_postgresql", + self.REPORTS["save_aggregate_report_to_postgresql"][0], + self._FakeDataError("x" * 200), + ) + self.assertTrue(str(exc).endswith(": " + "x" * 200)) + + def test_other_driver_error_stays_plain_postgresql_error(self): + for method, (report, _expected) in self.REPORTS.items(): + with self.subTest(method=method): + exc = self._save( + method, report, self._FakeOperationalError("server closed") + ) + self.assertNotIsInstance(exc, PostgreSQLRejectedReport) + self.assertEqual(str(exc), "server closed") + + if __name__ == "__main__": unittest.main() + + +# A password with the characters that end a URL's authority early, so a +# leak through a URL-shaped value would not be masked by accident. +_TLS_TEST_PASSWORD = "s3c/r?t#pw" + + +# (text after "postgresql://", refused by _has_unencoded_at(), the part of +# the password libpq parses into another component, or None for a string +# libpq parses as written). +_AT_RULE_CASES = ( + ("u:SECRETX/x@db.invalid/d", True, "SECRETX"), + ("127.0.0.1:SECRETX/x@db.invalid/d", True, "SECRETX"), + ("u:pa?sslmode=SECRETX/x@db.invalid/d", True, "SECRETX"), + ("u:SECRETX/x?sslrootcert=a@b", True, "SECRETX"), + ("u:pa@ss-SECRETX,@db.invalid/d", True, "SECRETX"), + ("u:s3c@pw@db.example/d", True, "pw@"), + ("u:s3c@pw:x@db.example/d", True, "x@db"), + ("u:s3c@pw/x@db.example/d", True, "x@db"), + ("u:s3c@p,w@db.example/d", True, "p,w"), + ("u:a@b?sslmode=require@db/d", True, "require@"), + ("u:a@b?sslrootcert=SECRET@db", True, "SECRET"), + ("u/x:SECRETX@db.invalid/d", True, "SECRETX"), + ("u:/SECRETX@db.invalid/d", True, "SECRETX"), + ("u:12345/x@db.invalid/d", True, "12345"), + # Not recognized, because each reads like a legitimate string: an + # all-digit password before a "/", with the "@" only in a query value + # (host "u", port 12345), and an "@", "?" and "/" in a password (host + # "pw" and an sslmode after a "?", like a certificate path after one). + ("u:12345/x?sslrootcert=a@b", False, "12345"), + ("u:s3c@pw?sslmode=SECRET/x@db", False, "SECRET"), + ("u:p%40w@db/d", False, None), + ("u@db/d", False, None), + ("db.invalid/d?sslrootcert=/home/a@b/ca.pem", False, None), + ("db.invalid:5432/d?sslrootcert=/home/a@b/ca.pem", False, None), + ("u:p@db.invalid/d?sslrootcert=/home/a@b/ca.pem", False, None), + ("u:p@db.invalid?sslrootcert=/home/a@b/ca.pem", False, None), + ("/d?host=/tmp", False, None), + ("?host=/tmp&sslrootcert=/a@b/c", False, None), + ("u:pw@%40sock/d", False, None), + ("u:p@db.invalid/d%40x", False, None), + ("[::1]:5432/d", False, None), + ("[::1]:5432/d?sslrootcert=/a@b/c", False, None), + ("u:p@[::1]:5432/d", False, None), + ("u:pa?x@db/d", False, None), + ("u:p@h1:5432,h2:5433/d", False, None), + ("db.invalid", False, None), +) + + +# Connection strings whose password a shape check once let through, or +# that the early refusal catches, each with the secret (and fragments of it) +# that must never be logged or raised. The driver error each test raises +# quotes the first one. +_SECRET_CONNECTION_STRINGS = ( + ("postgresql://u:SECRETX/x@db.invalid/d", ("SECRETX",)), + ("postgresql://127.0.0.1:SECRETX/x@db.invalid/d", ("SECRETX",)), + ("postgresql://u:pa?sslmode=SECRETX/x@db.invalid/d", ("SECRETX",)), + ("postgresql://u:pa@ss-SECRETX,@db.invalid/d", ("SECRETX", "ss-")), + ("postgresql://u:s3c@pw@db.example/d", ("s3c", "pw@")), + ( + "host=db.invalid password='SEC RETX' port=notanumber", + ("SEC RETX", "SEC", "RETX"), + ), +) + + +class TestConnectionStringDetailsWithheld(unittest.TestCase): + """Fourth review of GHSA-xvp7-7762-gx9g: libpq ends a URI's userinfo + at the first "@" *or* "/", so ``postgresql://u:SECRETX/x@db.invalid/d`` + parses (real psycopg 3.3 ``conninfo_to_dict``) to host ``u``, port + ``SECRETX`` and dbname ``x@db.invalid/d``, and connecting raised + ``connection is bad: invalid integer value "SECRETX" for connection + option "port"``, which the client passed on. Three rounds of shape + checks each missed a shape, so with a connection string nothing + derived from it is logged or raised any more: a connection failure + carries a fixed message (plus a well-formed SQLSTATE) and the startup + warning names no host or value from the string.""" + + def _construct(self, dsn, driver_error, refuse_early=True): + """Construct a client from *dsn* with psycopg.connect raising + *driver_error*; return (exception, log lines, connect mock).""" + operational_error = type(driver_error) + with contextlib.ExitStack() as stack: + mock_psycopg = stack.enter_context(patch("parsedmarc.postgres.psycopg")) + stack.enter_context(_without_libpq_env()) + if not refuse_early: + stack.enter_context(_without_early_refusal()) + logs = stack.enter_context(_captured_log_text()) + mock_psycopg.Error = operational_error.__mro__[1] + mock_psycopg.ProgrammingError = type( + "ProgrammingError", (mock_psycopg.Error,), {} + ) + mock_psycopg.connect.side_effect = driver_error + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient(connection_string=dsn) + return ctx.exception, logs, mock_psycopg.connect + + @staticmethod + def _driver_error(text, sqlstate=None): + """Return a stand-in psycopg OperationalError (a subclass of a + fresh Error class) with *text* and a ``sqlstate`` attribute.""" + error = type("Error", (Exception,), {}) + operational_error = type("OperationalError", (error,), {"sqlstate": sqlstate}) + return operational_error(text) + + def _assert_nothing_leaks(self, dsn, secrets, exc, logs): + rendered = "".join(traceback.format_exception(exc)) + log_text = "\n".join(logs) + for secret in (dsn, *secrets): + self.assertNotIn(secret, str(exc)) + self.assertNotIn(secret, rendered) + self.assertNotIn(secret, log_text) + self.assertIsNone(exc.__cause__) + self.assertTrue(exc.__suppress_context__) + + def test_secret_never_logged_or_raised_with_the_early_refusal(self): + """Each string is refused before anything is logged, or (the + keyword/value one, whose password is its own keyword) connects + and fails with the driver's text withheld.""" + for dsn, secrets in _SECRET_CONNECTION_STRINGS: + with self.subTest(dsn=dsn): + driver_error = self._driver_error( + f'invalid integer value "{secrets[0]}" for connection option "port"' + ) + exc, logs, _ = self._construct(dsn, driver_error) + self._assert_nothing_leaks(dsn, secrets, exc, logs) + + def test_secret_never_logged_or_raised_without_the_early_refusal(self): + """With the shape checks patched out, every string reaches the + driver, whose error quotes the secret, and still nothing derived + from the string is logged or raised: the message is the fixed + withheld one, with no chain. This is what holds whatever shape + gets past the early refusal.""" + for dsn, secrets in _SECRET_CONNECTION_STRINGS: + with self.subTest(dsn=dsn): + driver_error = self._driver_error( + f'connection is bad: invalid integer value "{secrets[0]}" ' + 'for connection option "port"' + ) + exc, logs, connect = self._construct( + dsn, driver_error, refuse_early=False + ) + connect.assert_called_once() + self.assertEqual(connect.call_args.args, (dsn,)) + self.assertEqual( + str(exc), + "PostgreSQL connection failed (details withheld because " + "the connection string may include the password; use the " + "separate host, port, user, password and database " + "settings to see the driver's message)", + ) + self._assert_nothing_leaks(dsn, secrets, exc, logs) + + def test_sqlstate_is_appended_only_when_well_formed(self): + """A server's SQLSTATE (five digits or upper-case letters) is a + fixed code, not text from the string, so it is kept; a missing, + empty or malformed one is not shown.""" + dsn = "host=db.example password=SECRETX" + for sqlstate, suffix in ( + ("28P01", " [SQLSTATE 28P01]"), + (None, ""), + ("", ""), + ("28P01; DROP", ""), + ("28p01", ""), + ): + with self.subTest(sqlstate=sqlstate): + driver_error = self._driver_error( + 'password authentication failed for user "SECRETX"', sqlstate + ) + exc, _, _ = self._construct(dsn, driver_error) + self.assertTrue(str(exc).endswith("driver's message)" + suffix)) + self.assertNotIn("SECRETX", str(exc)) + self.assertNotIn("DROP", str(exc)) + + def test_separate_arguments_keep_the_driver_message(self): + """Without a connection string the driver's message is passed on + (with URL credentials masked), since no value in it is derived + from a string a password fragment could land in.""" + driver_error = self._driver_error( + 'connection to "postgresql://u:PGSECRET@db.example/x" failed: ' + "Connection refused", + "08001", + ) + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + ): + mock_psycopg.Error = type(driver_error).__mro__[1] + mock_psycopg.ProgrammingError = type("ProgrammingError", (Exception,), {}) + mock_psycopg.connect.side_effect = driver_error + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient(host="db.example", sslmode="verify-full") + exc = ctx.exception + self.assertEqual( + str(exc), + 'connection to "postgresql://u:***@db.example/x" failed: ' + "Connection refused", + ) + self.assertIsNone(exc.__cause__) + self.assertTrue(exc.__suppress_context__) + + def _assert_no_secret_anywhere(self, exc, logs, secret="SECRETX"): + """Assert *secret* is in no rendering of *exc* (str, repr, args, + formatted traceback) nor in the captured log, and that nothing is + chained.""" + rendered = "".join(traceback.format_exception(exc)) + for text in (str(exc), repr(exc), repr(exc.args), rendered, *logs): + self.assertNotIn(secret, text) + self.assertIsNone(exc.__cause__) + self.assertTrue(exc.__suppress_context__) + + def _non_driver_failure(self, failure, client_kwargs, stage="connect"): + """Construct a client whose *stage* (``connect``, ``autocommit`` or + ``dumpers``) raises *failure*, an error that is not the driver's + own (psycopg.Error is a fresh class it does not derive from); + return (exception, log lines).""" + with contextlib.ExitStack() as stack: + mock_psycopg = stack.enter_context(patch("parsedmarc.postgres.psycopg")) + stack.enter_context(_without_libpq_env()) + logs = stack.enter_context(_captured_log_text()) + mock_psycopg.Error = type("Error", (Exception,), {}) + mock_psycopg.ProgrammingError = type( + "ProgrammingError", (mock_psycopg.Error,), {} + ) + conn = MagicMock() + if stage == "connect": + mock_psycopg.connect.side_effect = failure + elif stage == "autocommit": + type(conn).autocommit = property( + lambda self: False, + lambda self, value: (_ for _ in ()).throw(failure), + ) + mock_psycopg.connect.return_value = conn + else: + conn.adapters.register_dumper.side_effect = failure + mock_psycopg.connect.return_value = conn + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient(**client_kwargs) + return ctx.exception, logs + + def test_non_driver_connect_errors_are_withheld(self): + """Fifth review of GHSA-xvp7-7762-gx9g: _connect caught only + psycopg.Error, so any other exception escaped unwrapped. psycopg + encodes the connection string it builds from the separate + arguments, so a password holding a lone surrogate (an environment + variable with invalid UTF-8) raised UnicodeEncodeError with the + whole string, password included, in ``.object``, ``.args`` and + ``repr()``. Any such error is now a PostgreSQLError with a fixed + message and no chain, from the connect call, setting autocommit, + or registering the dumpers.""" + failures = ( + lambda: UnicodeEncodeError("utf-8", "password=SECRETX", 0, 1, "x"), + lambda: ValueError("SECRETX"), + ) + cases = ( + ( + {"connection_string": "host=db.invalid password=SECRETX"}, + "PostgreSQL connection failed (details withheld because the " + "connection string may include the password; use the " + "separate host, port, user, password and database settings " + "to see the driver's message)", + ), + ( + {"host": "db.invalid", "user": "u", "password": "SECRETX"}, + "PostgreSQL connection failed: a connection setting could not " + "be passed to the driver (details withheld because they may " + "include the password)", + ), + ) + for make_failure in failures: + for client_kwargs, message in cases: + for stage in ("connect", "autocommit", "dumpers"): + failure = make_failure() + with self.subTest( + failure=type(failure).__name__, + kwargs=sorted(client_kwargs), + stage=stage, + ): + exc, logs = self._non_driver_failure( + failure, client_kwargs | {"sslmode": "verify-full"}, stage + ) + self.assertEqual(str(exc), message) + self._assert_no_secret_anywhere(exc, logs) + + def test_keyboard_interrupt_is_not_swallowed(self): + """The catch-all is ``except Exception``: a Ctrl-C during the + connect attempt still propagates as KeyboardInterrupt.""" + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + ): + mock_psycopg.Error = type("Error", (Exception,), {}) + mock_psycopg.connect.side_effect = KeyboardInterrupt + with self.assertRaises(KeyboardInterrupt): + PostgreSQLClient(host="db.invalid", sslmode="verify-full") + + @unittest.skipUnless(PSYCOPG_INSTALLED, "psycopg is not installed") + def test_real_psycopg_surrogate_password_is_withheld(self): + """With the real driver, a password holding a lone surrogate makes + psycopg raise UnicodeEncodeError('utf-8', 'host=127.0.0.1 port=1 + user=u password=\\udcffSECRETX ...') while it encodes the + connection string, before any network I/O (Python 3.12 and 3.15). + The client raises a PostgreSQLError with none of it.""" + with _without_libpq_env(), _captured_log_text() as logs: + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient( + host="127.0.0.1", + port=1, + user="u", + password="\udcffSECRETX", + database="d", + sslmode="disable", + ) + exc = ctx.exception + self.assertIsInstance(exc, PostgreSQLError) + self.assertTrue(str(exc).startswith("PostgreSQL connection failed: a ")) + self._assert_no_secret_anywhere(exc, logs) + + @unittest.skipUnless(PSYCOPG_INSTALLED, "psycopg is not installed") + def test_real_psycopg_overlong_host_label_is_withheld(self): + """A host label over 63 characters fails in psycopg's name + resolution before any network I/O: on Python 3.15 with + UnicodeEncodeError('idna', '', ...), which psycopg's + getaddrinfo wrapper (it catches only OSError) does not wrap, and + on 3.12 with a bare UnicodeError. Here the host holds the rest of + a password (``password=se host=a`` + ``SECRETX`` * 10 + + ``.invalid``); the client raises the withheld message instead.""" + dsn = "password=se host=a" + "SECRETX" * 10 + ".invalid" + with _without_libpq_env(), _captured_log_text() as logs: + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient(connection_string=dsn, sslmode="disable") + exc = ctx.exception + self.assertIsInstance(exc, PostgreSQLError) + self.assertTrue(str(exc).startswith("PostgreSQL connection failed (")) + self._assert_no_secret_anywhere(exc, logs) + + @unittest.skipUnless(PSYCOPG_INSTALLED, "psycopg is not installed") + def test_real_psycopg_withholds_a_password_parsed_as_the_port(self): + """With the real driver and the early refusal patched out, + ``postgresql://127.0.0.1:SECRETX/x@db.invalid/d`` reaches libpq, + which fails locally on the port ``SECRETX`` (no server is + contacted) with ``invalid integer value "SECRETX" for connection + option "port"``; the raised error, its traceback and the log carry + no part of it.""" + dsn = "postgresql://127.0.0.1:SECRETX/x@db.invalid/d" + with ( + _without_libpq_env(), + _without_early_refusal(), + _captured_log_text() as logs, + ): + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient(connection_string=dsn) + exc = ctx.exception + self.assertTrue(str(exc).startswith("PostgreSQL connection failed")) + self._assert_nothing_leaks(dsn, ("SECRETX",), exc, logs) + + +class TestPostgreSQLTLS(unittest.TestCase): + """GHSA-xvp7-7762-gx9g: the client connects with ``sslmode=require`` + unless the caller, the connection string, or ``PGSSLMODE`` chooses a + mode, and warns at startup when a remote server's certificate is not + verified. Before the fix no sslmode was passed, so libpq used + ``prefer`` and continued over plain TCP when an on-path attacker + declined TLS, then answered its clear-text password request. + + psycopg is mocked at ``parsedmarc.postgres.psycopg``; the libpq + environment is cleared so the developer's shell does not leak in.""" + + def _connect_call(self, env=None, **client_kwargs): + """Construct a client with psycopg.connect mocked and return the + call psycopg.connect received.""" + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + patch.dict(os.environ, env or {}), + ): + mock_psycopg.Error = Exception + PostgreSQLClient(**client_kwargs) + mock_psycopg.connect.assert_called_once() + return mock_psycopg.connect.call_args + + def _warnings(self, env=None, **client_kwargs) -> list[str]: + """Construct a client and return the warnings it logged.""" + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + self._connect_call(env=env, **client_kwargs) + return cm.output + + def _assert_no_warning(self, env=None, **client_kwargs): + with self.assertNoLogs("parsedmarc.log", level="WARNING"): + self._connect_call(env=env, **client_kwargs) + + # -- the sslmode passed to psycopg.connect ------------------------------- + + def test_default_sslmode_is_require_with_host(self): + call = self._connect_call( + host="db.example", user="u", password="pw", database="d" + ) + self.assertEqual(call.args, ()) + self.assertEqual(call.kwargs["sslmode"], "require") + self.assertNotIn("sslrootcert", call.kwargs) + self.assertEqual(call.kwargs["host"], "db.example") + self.assertEqual(call.kwargs["password"], "pw") + + def test_default_sslmode_is_require_with_connection_string(self): + call = self._connect_call(connection_string="host=db.example user=u") + self.assertEqual(call.args, ("host=db.example user=u",)) + self.assertEqual(call.kwargs, {"sslmode": "require"}) + + def test_explicit_sslmode_and_sslrootcert_pass_through(self): + call = self._connect_call( + host="db.example", sslmode="verify-full", sslrootcert="/etc/ca.pem" + ) + self.assertEqual(call.kwargs["sslmode"], "verify-full") + self.assertEqual(call.kwargs["sslrootcert"], "/etc/ca.pem") + + def test_explicit_sslmode_overrides_connection_string_and_environment(self): + call = self._connect_call( + env={"PGSSLMODE": "disable"}, + connection_string="host=db.example sslmode=prefer", + sslmode="verify-ca", + ) + self.assertEqual(call.kwargs, {"sslmode": "verify-ca"}) + + def test_connection_string_sslmode_is_left_to_libpq(self): + """A string that sets sslmode gets no sslmode keyword, which psycopg + would merge over the string's own value.""" + call = self._connect_call( + connection_string="host=db.example sslmode=verify-full user=u", + sslrootcert="/etc/ca.pem", + ) + self.assertEqual(call.args, ("host=db.example sslmode=verify-full user=u",)) + self.assertEqual(call.kwargs, {"sslrootcert": "/etc/ca.pem"}) + + def test_pgsslmode_environment_is_left_to_libpq(self): + call = self._connect_call(env={"PGSSLMODE": "verify-ca"}, host="db.example") + self.assertNotIn("sslmode", call.kwargs) + + def test_malformed_connection_string_withheld_before_connecting(self): + """The string is parsed before connecting; a parse failure, whose + libpq message quotes the offending word, raises the withheld-details + error from None, so neither the message nor the traceback carries + the string or the password, and no connection is attempted.""" + dsn = f"host=db.example {_TLS_TEST_PASSWORD}" + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + ): + mock_psycopg.Error = Exception + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient(connection_string=dsn) + mock_psycopg.connect.assert_not_called() + exc = ctx.exception + self.assertIn("Invalid PostgreSQL connection_string", str(exc)) + rendered = "".join(traceback.format_exception(exc)) + for secret in (dsn, _TLS_TEST_PASSWORD, "s3c"): + self.assertNotIn(secret, str(exc)) + self.assertNotIn(secret, rendered) + self.assertIsNone(exc.__cause__) + self.assertTrue(exc.__suppress_context__) + + # -- the startup warning ------------------------------------------------- + + def test_warning_for_remote_host_without_verification(self): + cases = ( + ({"host": "db.example"}, "require"), + ({"host": "db.example", "sslmode": "require"}, "require"), + ({"host": "db.example", "sslmode": "prefer"}, "prefer"), + ({"connection_string": "host=db.example"}, "require"), + ({"connection_string": "hostaddr=192.0.2.10"}, "require"), + ({"host": "localhost,db.example"}, "require"), + ({"host": "192.0.2.10"}, "require"), + # libpq connects to hostaddr, not host, when both are set. + ({"connection_string": "host=localhost hostaddr=203.0.113.5"}, "require"), + ) + for kwargs, mode in cases: + with self.subTest(kwargs=kwargs): + output = self._warnings(**kwargs) + self.assertEqual(len(output), 1) + if "connection_string" in kwargs: + self.assertIn( + "The PostgreSQL connection_string does not verify the " + f"server certificate (sslmode is {mode});", + output[0], + ) + else: + self.assertIn( + f"uses sslmode={mode}, which does not verify", output[0] + ) + self.assertIn("set [postgresql] sslmode = verify-full", output[0]) + + def test_warning_names_the_host_but_no_credential(self): + """With the separate arguments the warning names the host only: + not the password (one with "/", "?" and "#") or the user.""" + output = "\n".join( + self._warnings( + host="db.example", user="dmarcuser", password=_TLS_TEST_PASSWORD + ) + ) + self.assertIn("PostgreSQL connection to db.example uses", output) + for secret in (_TLS_TEST_PASSWORD, "s3c", "dmarcuser"): + self.assertNotIn(secret, output) + + def test_connection_string_warning_names_no_value_from_the_string(self): + """With a connection string the warning names no host, user, + password or other value from the string: when the user name or + password holds an unencoded "@" or "/", libpq parses part of the + password as the host or a query parameter. PGHOST from the + environment is not named either.""" + dsn = f"host=db.example user=dmarcuser password={_TLS_TEST_PASSWORD}" + for env in ({}, {"PGHOST": "pghost.example"}): + with self.subTest(env=env): + output = "\n".join(self._warnings(env=env, connection_string=dsn)) + self.assertIn( + "The PostgreSQL connection_string does not verify the server " + "certificate (sslmode is require)", + output, + ) + for value in ( + "db.example", + "pghost.example", + _TLS_TEST_PASSWORD, + "s3c", + "dmarcuser", + dsn, + ): + self.assertNotIn(value, output) + + def test_connection_string_warning_prints_only_a_recognized_mode(self): + """The mode is printed only when it is one of SSL_MODES: a typo + (``verfy-full``) and an sslmode that holds a password fragment + (``postgresql://u:pa?sslmode=SECRETX/x@db.invalid/d`` parses with + host ``u``, port ``pa`` and sslmode ``SECRETX/x@db.invalid/d``) are + both shown as "not a recognized value". The early refusal is + patched out so the warning itself is what is tested.""" + for dsn, secrets in ( + ("host=db.example sslmode=verfy-full", ("db.example", "verfy")), + ( + "postgresql://u:pa?sslmode=SECRETX/x@db.invalid/d", + ("SECRETX", "db.invalid", "x@"), + ), + ): + with self.subTest(dsn=dsn), _without_early_refusal(): + output = "\n".join(self._warnings(connection_string=dsn)) + self.assertIn("(sslmode is not a recognized value)", output) + for secret in secrets: + self.assertNotIn(secret, output) + + def test_warning_masks_a_host_with_an_at_sign(self): + """A URI connection string whose host holds an "@" is refused (see + test_unencoded_at_in_connection_string_is_refused), but the *host* + argument, ``PGHOST`` and ``PGHOSTADDR`` are passed on, so the + warning still credential-masks the host it names, in a host list + too.""" + cases = ( + ({"host": "pw@db.example"}, {}), + ({"host": "pw@db.example,h2.example"}, {}), + ({}, {"PGHOST": "pw@db.example"}), + ({}, {"PGHOSTADDR": "pw@db.example"}), + ) + for kwargs, env in cases: + with self.subTest(kwargs=kwargs, env=env): + output = "\n".join(self._warnings(env=env, **kwargs)) + self.assertIn("PostgreSQL connection to ***@db.example", output) + self.assertNotIn("pw@", output) + + def test_unencoded_at_in_connection_string_is_refused(self): + """libpq ends a URI's userinfo at the first "@", so the rest of a + password containing an unencoded "@" lands in the host, the port or + the database name (with psycopg 3.3's conninfo_to_dict: + ``u:s3c@pw:x@db.example`` gives host ``pw`` and port + ``x@db.example``; ``u:s3c@pw/x@db.example/d`` host ``pw`` and + dbname ``x@db.example/d``; ``u:s3c@p,w@db.example`` host + ``p,w@db.example``; ``u:s3c@pw@db.example`` host + ``pw@db.example``). The startup warning then named host ``pw`` and + the driver failed to resolve it, a password fragment in the log + both times. Such a string is now refused before anything is logged + or any connection is attempted, with every sslmode, and neither + the message nor the traceback holds the password or a fragment.""" + cases = ( + ("postgresql://u:s3c@pw:x@db.example/d", ("s3c", "pw", "x@db")), + ("postgresql://u:s3c@pw/x@db.example/d", ("s3c", "pw", "x@db")), + ("postgresql://u:s3c@p,w@db.example/d", ("s3c", "p,w")), + ("postgresql://u:s3c@pw@db.example/d", ("s3c", "pw")), + # An "@" at the start of a host-list entry names an abstract + # socket, which a check on the parsed host exempted. + ("postgresql://u:pa@ss-SECRETX,@db.invalid/d", ("SECRETX", "ss-", "pa@ss")), + # The fragment lands in a query parameter: sslmode (which the + # warning names) or sslrootcert (which libpq's errors quote). + ("postgresql://u:a@b?sslmode=require@db/d", ("a@b", "sslmode=require")), + ("postgresql://u:a@b?sslrootcert=SECRET@db", ("SECRET", "a@b")), + # libpq's scan for the userinfo stops at the first "/" as well as + # the first "@", so an unencoded "/" in the password ends the + # authority: host "u", port "SECRETX", dbname "x@db.invalid/d". + ("postgresql://u:SECRETX/x@db.invalid/d", ("SECRETX",)), + ("postgresql://127.0.0.1:SECRETX/x@db.invalid/d", ("SECRETX",)), + ("postgresql://u:pa?sslmode=SECRETX/x@db.invalid/d", ("SECRETX",)), + # Port "SECRETX", and the "@" only in a query value. + ("postgresql://u:SECRETX/x?sslrootcert=a@b", ("SECRETX",)), + # A "/" in the user name, or a password that starts with "/" or + # is all digits: the "@" lands in the database name. + ("postgresql://u/x:SECRETX@db.invalid/d", ("SECRETX",)), + ("postgresql://u:/SECRETX@db.invalid/d", ("SECRETX",)), + ("postgresql://u:12345/x@db.invalid/d", ("12345",)), + # Case and leading whitespace do not hide the URI form. + (" PostgreSQL://u:s3c@pw@db.example/d", ("s3c", "pw")), + ) + for dsn, secrets in cases: + for sslmode in (None, "verify-full"): + with self.subTest(dsn=dsn, sslmode=sslmode): + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + self.assertNoLogs("parsedmarc.log", level="DEBUG"), + ): + mock_psycopg.Error = Exception + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient(connection_string=dsn, sslmode=sslmode) + mock_psycopg.connect.assert_not_called() + exc = ctx.exception + self.assertIn( + 'appears to have an unencoded "@" or "/" in its user ' + "name, password or database name", + str(exc), + ) + self.assertIn("percent-encode", str(exc)) + rendered = "".join(traceback.format_exception(exc)) + for secret in (dsn, *secrets): + self.assertNotIn(secret, str(exc)) + self.assertNotIn(secret, rendered) + self.assertIsNone(exc.__cause__) + + def test_unencoded_at_rule_table(self): + """_has_unencoded_at() mirrors libpq's URI scan (userinfo up to the + first "@" or "/") and refuses each string in which a part of the + password lands in another component, except two shapes it does not + recognize because each reads like a legitimate string (see the + table), which the withheld messages keep out of the logs + regardless. Strings libpq parses as written are accepted.""" + for rest, refused, fragment in _AT_RULE_CASES: + with self.subTest(rest=rest): + self.assertIs(_has_unencoded_at("postgresql://" + rest), refused) + if refused: + self.assertIsNotNone(fragment) + + @unittest.skipUnless(PSYCOPG_INSTALLED, "psycopg is not installed") + def test_real_parser_agrees_with_the_rule_table(self): + """With the real conninfo_to_dict, the password fragment of each + misparsed string in the table is in a component other than the + user name and password, and each string listed as parsed as + written has its "@" only where it was written: in the userinfo + separator, a %40, or a query value.""" + from psycopg.conninfo import conninfo_to_dict + + for rest, _refused, fragment in _AT_RULE_CASES: + with self.subTest(rest=rest): + params = conninfo_to_dict("postgresql://" + rest) + others = [ + str(value) + for key, value in params.items() + if key not in ("user", "password") + ] + if fragment is not None: + self.assertTrue(any(fragment in v for v in others), params) + continue + for key in ("host", "port", "dbname"): + value = str(params.get(key) or "") + self.assertTrue("@" not in value or "%40" in rest, (key, params)) + port = str(params.get("port") or "") + self.assertTrue(all(p.isdigit() for p in port.split(",") if p)) + + def test_unencoded_at_is_refused_on_reconnect(self): + """_connect parses the string itself as well, so a reconnect (whose + sslmode lookup skips the string when sslmode is set) never hands a + refused string to the driver either.""" + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + ): + mock_psycopg.Error = Exception + client = PostgreSQLClient( + connection_string="postgresql://u:p@db.example/d", + sslmode="verify-full", + ) + client._connection_string = "postgresql://u:s3c@pw@db.example/d" + with self.assertRaises(PostgreSQLError) as ctx: + client._connect() + self.assertEqual(mock_psycopg.connect.call_count, 1) + self.assertIn('unencoded "@"', str(ctx.exception)) + + def test_percent_encoded_at_in_password_connects(self): + """A password whose "@" is percent-encoded parses with the right + host, and the string reaches the driver unchanged.""" + dsn = "postgresql://u:s3c%40pw@db.example/d" + call = self._connect_call(connection_string=dsn, sslmode="verify-full") + self.assertEqual(call.args, (dsn,)) + + def test_percent_encoded_at_elsewhere_in_uri_connects(self): + """A URI never needs a second literal "@": one in an abstract + socket host or a query value is written %40, which libpq decodes + (psycopg 3.3: ``postgresql://u@%40sock/d`` has host ``@sock``, + ``?sslrootcert=a%40b`` sslrootcert ``a@b``), so such a URI is + accepted and passed on unchanged.""" + for dsn in ( + "postgresql://u@%40pgsock/d", + "postgresql://u:pw@/d?host=%40pgsock", + "postgresql://u:pw@db.example/d?sslrootcert=a%40b", + "postgresql://u:p%2Fw@db.example/d", + ): + with self.subTest(dsn=dsn): + call = self._connect_call(connection_string=dsn, sslmode="verify-full") + self.assertEqual(call.args, (dsn,)) + + def test_legitimate_connection_strings_are_not_refused(self): + """Strings libpq parses as written are passed on unchanged: an "@" + that is percent-encoded (a database name ``d%40x`` cannot be fixed + by encoding, so refusing it would leave no way to name it), one in + a query value after a "/" (a certificate path), IPv6 hosts with a + port, host lists, a "?" in a password, and keyword/value strings, + whose password is a keyword of its own: an "@" in their host, + hostaddr or dbname is not a password fragment, and libpq does not + percent-decode them, so encoding advice would be wrong.""" + for dsn in ( + "postgresql://u:p%40w@db.invalid/d", + "postgresql://u@db.invalid/d", + "postgresql://db.invalid/d?sslrootcert=/home/a@b/ca.pem", + "postgresql://db.invalid:5432/d?sslrootcert=/home/a@b/ca.pem", + "postgresql://u:p@db.invalid/d?sslrootcert=/home/a@b/ca.pem", + "postgresql://u:p@db.invalid?sslrootcert=/home/a@b/ca.pem", + "postgresql:///d?host=/tmp", + "postgresql://u:pw@%40sock/d", + "postgresql://u:p@db.invalid/d%40x", + "postgresql://[::1]:5432/d", + "postgresql://u:p@[::1]:5432/d", + "postgresql://u:pa?x@db.invalid/d", + "postgresql://u:p@h1.invalid:5432,h2.invalid:5433/d", + "dbname=a@b host=db.invalid", + "host=pw@db.invalid,h2.invalid", + "hostaddr=pw@db.invalid", + ): + with self.subTest(dsn=dsn): + call = self._connect_call(connection_string=dsn, sslmode="verify-full") + self.assertEqual(call.args, (dsn,)) + + def test_abstract_socket_host_in_connection_string_is_allowed(self): + """An "@" at the start of a host entry names a Linux abstract + socket, not a password fragment, in a keyword/value string (whose + password is its own keyword, so no fragment of it can land in the + host).""" + for dsn in ("host=@pgsock dbname=d", "host=db.example,@pgsock dbname=d"): + with self.subTest(dsn=dsn): + call = self._connect_call(connection_string=dsn) + self.assertEqual(call.args, (dsn,)) + + def test_warning_host_and_mode_come_from_the_environment(self): + """With no host argument libpq connects to PGHOST (or PGHOSTADDR), + and with no sslmode set elsewhere it uses PGSSLMODE.""" + output = self._warnings(env={"PGHOST": "db.example", "PGSSLMODE": "allow"}) + self.assertIn("connection to db.example uses sslmode=allow", output[0]) + output = self._warnings(env={"PGHOSTADDR": "192.0.2.10"}) + self.assertIn("connection to 192.0.2.10 uses sslmode=require", output[0]) + output = self._warnings( + env={"PGSSLMODE": "prefer"}, + connection_string="host=db.example sslmode=disable", + ) + self.assertIn("(sslmode is disable)", output[0]) + # PGHOST decides that a connection string without a host is remote, + # but is not named. + output = self._warnings( + env={"PGHOST": "pghost.example"}, connection_string="dbname=d" + ) + self.assertIn("(sslmode is require)", output[0]) + self.assertNotIn("pghost.example", output[0]) + + def test_warning_host_is_escaped(self): + """A line break in the host is escaped, so it cannot forge a log + line.""" + output = self._warnings(host="db.example\nforged") + self.assertIn("db.example\\nforged", output[0]) + self.assertNotIn("\nforged", output[0]) + + def test_no_warning_for_local_or_verified_connections(self): + cases = ( + {"host": "127.0.0.1"}, + {"host": "127.0.0.53"}, + {"host": "::1"}, + {"host": "localhost"}, + {"host": "LocalHost"}, + {"host": "/var/run/postgresql"}, + {"host": "@abstract"}, + {"host": "localhost,::1,/tmp"}, + {"host": None}, + {"connection_string": "dbname=dmarc"}, + {"connection_string": "postgresql:///dmarc"}, + {"host": "db.example", "sslmode": "verify-full"}, + {"host": "db.example", "sslmode": "verify-ca"}, + {"connection_string": "host=db.example sslmode=verify-full"}, + ) + for kwargs in cases: + with self.subTest(kwargs=kwargs): + self._assert_no_warning(**kwargs) + with self.subTest(env="PGSSLMODE=verify-full"): + self._assert_no_warning(env={"PGSSLMODE": "verify-full"}, host="db.example") + + def test_no_warning_for_require_with_an_existing_root_certificate(self): + """libpq: "if a root CA file exists, the behavior of sslmode=require + will be the same as that of verify-ca" (PostgreSQL docs, libpq SSL + support), so the default mode with an sslrootcert that exists is + not warned about, from the argument, the string or PGSSLROOTCERT. + A root certificate that does not exist leaves require unverified.""" + with tempfile.TemporaryDirectory() as tmp: + ca = os.path.join(tmp, "ca.pem") + with open(ca, "w") as f: + f.write("not parsed") + for env, kwargs in ( + ({}, {"host": "db.example", "sslrootcert": ca}), + ({}, {"host": "db.example", "sslmode": "require", "sslrootcert": ca}), + ({}, {"connection_string": f"host=db.example sslrootcert={ca}"}), + ({"PGSSLROOTCERT": ca}, {"host": "db.example"}), + ): + with self.subTest(env=env, kwargs=kwargs): + self._assert_no_warning(env=env, **kwargs) + missing = os.path.join(tmp, "missing.pem") + output = self._warnings(host="db.example", sslrootcert=missing) + self.assertIn("uses sslmode=require", output[0]) + output = self._warnings(host="db.example", sslmode="prefer", sslrootcert=ca) + self.assertIn("uses sslmode=prefer", output[0]) + + def test_warning_is_logged_once_not_on_reconnect(self): + with ( + patch("parsedmarc.postgres.psycopg") as mock_psycopg, + _without_libpq_env(), + ): + mock_psycopg.Error = Exception + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + client = PostgreSQLClient(host="db.example") + mock_psycopg.connect.return_value.closed = True + client._ensure_connected() + self.assertEqual(mock_psycopg.connect.call_count, 2) + self.assertEqual( + mock_psycopg.connect.call_args_list[1].kwargs["sslmode"], "require" + ) + tls = [m for m in cm.output if "does not verify the server" in m] + self.assertEqual(len(tls), 1) + + # -- the real driver against a server that declines TLS ------------------ + + @unittest.skipUnless(PSYCOPG_INSTALLED, "psycopg is not installed") + def test_real_psycopg_sends_nothing_after_tls_is_declined(self): + """With the default sslmode, libpq gives up when the server answers + the SSLRequest with "N": no startup packet (which names the user), + let alone a password, follows. The same server with + ``sslmode="disable"`` receives the startup packet, so the server + does observe what it claims to. Before the fix, libpq's ``prefer`` + sent the startup packet over plain TCP after the "N" and then + answered a clear-text password request.""" + with _without_libpq_env(): + server = _TLSDecliningServer() + with self.assertRaises(PostgreSQLError) as ctx: + PostgreSQLClient( + host="127.0.0.1", + port=server.port, + user="u", + password=_TLS_TEST_PASSWORD, + database="d", + ) + server.join() + message = str(ctx.exception) + self.assertIn("SSL", message) + self.assertNotIn(_TLS_TEST_PASSWORD, message) + self.assertNotIn("s3c", message) + self.assertTrue(server.ssl_requested) + self.assertEqual(server.after_refusal, b"") + + with _without_libpq_env(): + server = _TLSDecliningServer() + with self.assertRaises(PostgreSQLError): + PostgreSQLClient( + host="127.0.0.1", + port=server.port, + user="u", + password=_TLS_TEST_PASSWORD, + database="d", + sslmode="disable", + ) + server.join() + self.assertFalse(server.ssl_requested) + self.assertIn(b"user\x00u\x00", server.startup) + + +# PostgreSQL protocol request codes (protocol docs, "Message Formats"): +# SSLRequest and GSSENCRequest are an Int32 length of 8 and one of these. +_SSL_REQUEST_CODE = 80877103 +_GSSENC_REQUEST_CODE = 80877104 + + +class _TLSDecliningServer: + """A one-connection loopback server that declines encryption. + + It answers an SSLRequest (and a GSSENCRequest, which libpq built with + GSSAPI may send first) with "N", then records every byte the client + sends afterwards until the client closes or 1 s passes. A first + message that is not such a request is a startup packet, recorded the + same way. No authentication request is ever sent, so the client fails. + """ + + def __init__(self): + self._sock = socket.create_server(("127.0.0.1", 0)) + self.port = self._sock.getsockname()[1] + self.ssl_requested = False + self.after_refusal = b"" + self.startup = b"" + self._thread = threading.Thread(target=self._serve, daemon=True) + self._thread.start() + + def join(self): + self._thread.join(timeout=10) + self._sock.close() + + @staticmethod + def _read_until_idle(conn) -> bytes: + data = b"" + conn.settimeout(1.0) + try: + while chunk := conn.recv(4096): + data += chunk + except (TimeoutError, OSError): + pass + return data + + def _serve(self): + self._sock.settimeout(10) + conn, _ = self._sock.accept() + with conn: + conn.settimeout(5) + while True: + first = b"" + while len(first) < 8: + chunk = conn.recv(8 - len(first)) + if not chunk: + return + first += chunk + length, code = struct.unpack("!ii", first) + if length == 8 and code == _GSSENC_REQUEST_CODE: + conn.sendall(b"N") + continue + if length == 8 and code == _SSL_REQUEST_CODE: + self.ssl_requested = True + conn.sendall(b"N") + self.after_refusal = self._read_until_idle(conn) + else: + self.startup = first + self._read_until_idle(conn) + return diff --git a/tests/test_utils.py b/tests/test_utils.py index b0851fe3..e03d6f7e 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -2,6 +2,7 @@ import os import shutil +import stat import tempfile import time import traceback @@ -739,8 +740,11 @@ class TestDownloadUrlCredentialsNotLogged(unittest.TestCase): old_path = parsedmarc.utils._IP_DB_PATH self.addCleanup(setattr, parsedmarc.utils, "_IP_DB_PATH", old_path) parsedmarc.utils._IP_DB_PATH = None + old_cache_dir = parsedmarc.utils._IP_DB_CACHE_DIR + self.addCleanup(setattr, parsedmarc.utils, "_IP_DB_CACHE_DIR", old_cache_dir) + parsedmarc.utils._IP_DB_CACHE_DIR = None with ( - patch("parsedmarc.utils.tempfile.gettempdir", return_value=tmp_dir), + patch.dict(os.environ, {"XDG_CACHE_HOME": tmp_dir}), self.assertLogs("parsedmarc.log", level="DEBUG") as logs, ): parsedmarc.utils.load_ip_db(url=url or self.URL) @@ -2249,27 +2253,86 @@ class TestNameserverCredentialsNotLeaked(unittest.TestCase): class TestLoadIpDb(unittest.TestCase): """Tests for the load_ip_db() download/cache/bundled fallback chain, - mocking at the httpx SDK boundary.""" + mocking at the httpx SDK boundary, and for the per-user cache directory + and cached-file checks added for GHSA-5f56-8ph8-v9q5 (the cache used to + live at the fixed, shared /parsedmarc/ipinfo_lite.mmdb, where + another local user could plant a symlink or a database).""" + + _bundled_bytes: bytes | None = None + + @classmethod + def bundled_bytes(cls) -> bytes: + """A real MMDB body for a download to return.""" + if cls._bundled_bytes is None: + cls._bundled_bytes = ( + files(parsedmarc.resources.ipinfo) + .joinpath("ipinfo_lite.mmdb") + .read_bytes() + ) + return cls._bundled_bytes def setUp(self): old_ip_db_path = parsedmarc.utils._IP_DB_PATH + old_cache_dir = parsedmarc.utils._IP_DB_CACHE_DIR parsedmarc.utils._IP_DB_PATH = None + parsedmarc.utils._IP_DB_CACHE_DIR = None def restore(): parsedmarc.utils._IP_DB_PATH = old_ip_db_path + parsedmarc.utils._IP_DB_CACHE_DIR = old_cache_dir self.addCleanup(restore) - # Redirect the download cache into a per-test directory so the - # tests never touch (or depend on) the real tempdir cache. + # Point the per-user cache into a per-test directory so the tests + # never touch (or depend on) the real ~/.cache/parsedmarc. self.tmp_dir = tempfile.mkdtemp() self.addCleanup(lambda: shutil.rmtree(self.tmp_dir, ignore_errors=True)) - patcher = patch( - "parsedmarc.utils.tempfile.gettempdir", return_value=self.tmp_dir - ) + self.cache_home = os.path.join(self.tmp_dir, "cache-home") + self.cache_dir = os.path.join(self.cache_home, "parsedmarc") + self.cached_path = os.path.join(self.cache_dir, "ipinfo_lite.mmdb") + patcher = patch.dict(os.environ, {"XDG_CACHE_HOME": self.cache_home}) patcher.start() self.addCleanup(patcher.stop) + @property + def bundled(self) -> str: + return str(files(parsedmarc.resources.ipinfo).joinpath("ipinfo_lite.mmdb")) + + def _response(self, content: bytes) -> MagicMock: + response = MagicMock() + response.content = content + response.raise_for_status.return_value = None + return response + + def _track_temp_fallback(self) -> None: + """Removes the temporary directory a rejected cache directory made + load_ip_db() create, once the test ends.""" + + def remove(): + chosen = parsedmarc.utils._IP_DB_CACHE_DIR + if chosen is not None and chosen[1] != chosen[0]: + shutil.rmtree(chosen[1], ignore_errors=True) + + self.addCleanup(remove) + + def _assert_temp_fallback(self, cm) -> str: + """Asserts the rejected-directory warning and that the cache moved + to a fresh parsedmarc- directory under the system temp dir, and + returns that directory.""" + self.assertTrue( + any( + f"Not using {self.cache_dir} for the IP database cache" in m + for m in cm.output + ), + cm.output, + ) + chosen = parsedmarc.utils._IP_DB_CACHE_DIR + assert chosen is not None + fallback = chosen[1] + self.assertEqual(os.path.dirname(fallback), tempfile.gettempdir()) + self.assertTrue(os.path.basename(fallback).startswith("parsedmarc-")) + return fallback + def testExistingLocalFileIsUsedDirectly(self): """An existing local_file_path wins without any network request""" local_path = os.path.join(self.tmp_dir, "local.mmdb") @@ -2281,24 +2344,25 @@ class TestLoadIpDb(unittest.TestCase): self.assertEqual(parsedmarc.utils._IP_DB_PATH, local_path) def testDownloadSuccessWritesCacheFile(self): - """A successful download is written to the cache path and selected""" - response = MagicMock() - response.content = b"downloaded db bytes" - response.raise_for_status.return_value = None - with patch("parsedmarc.utils.httpx.get", return_value=response) as mock_get: + """A successful download of a valid MMDB is written to the per-user + cache path, created 0700, and selected; no temporary file is left""" + body = self.bundled_bytes() + with patch( + "parsedmarc.utils.httpx.get", return_value=self._response(body) + ) as mock_get: parsedmarc.utils.load_ip_db(url="https://example.com/db.mmdb") self.assertEqual(mock_get.call_args.args[0], "https://example.com/db.mmdb") - cached_path = os.path.join(self.tmp_dir, "parsedmarc", "ipinfo_lite.mmdb") - self.assertEqual(parsedmarc.utils._IP_DB_PATH, cached_path) - with open(cached_path, "rb") as f: - self.assertEqual(f.read(), b"downloaded db bytes") + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.cached_path) + with open(self.cached_path, "rb") as f: + self.assertEqual(f.read(), body) + self.assertEqual(os.listdir(self.cache_dir), ["ipinfo_lite.mmdb"]) + if os.name != "nt": + self.assertEqual(os.stat(self.cache_dir).st_mode & 0o777, 0o700) def testDownloadFailureFallsBackToCachedCopy(self): """On a network error, a previously cached copy is selected""" - cache_dir = os.path.join(self.tmp_dir, "parsedmarc") - os.makedirs(cache_dir) - cached_path = os.path.join(cache_dir, "ipinfo_lite.mmdb") - with open(cached_path, "wb") as f: + os.makedirs(self.cache_dir, mode=0o700) + with open(self.cached_path, "wb") as f: f.write(b"stale cached db") with patch( "parsedmarc.utils.httpx.get", @@ -2309,7 +2373,7 @@ class TestLoadIpDb(unittest.TestCase): self.assertTrue( any("Failed to fetch IP database" in message for message in cm.output) ) - self.assertEqual(parsedmarc.utils._IP_DB_PATH, cached_path) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.cached_path) def testDownloadFailureFallsBackToBundledCopy(self): """On a network error with no cached copy, the bundled db is used""" @@ -2318,28 +2382,418 @@ class TestLoadIpDb(unittest.TestCase): side_effect=httpx.ConnectError("no network"), ): parsedmarc.utils.load_ip_db() - bundled = str(files(parsedmarc.resources.ipinfo).joinpath("ipinfo_lite.mmdb")) - self.assertEqual(parsedmarc.utils._IP_DB_PATH, bundled) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) def testSaveFailureFallsBackToBundledCopy(self): - """A download that cannot be written to disk logs a warning and - falls back to the bundled db instead of crashing. The cache dir is - made uncreatable by pointing gettempdir at a regular file.""" - blocker = os.path.join(self.tmp_dir, "blocker") - with open(blocker, "wb") as f: - f.write(b"not a directory") - response = MagicMock() - response.content = b"downloaded db bytes" - response.raise_for_status.return_value = None - with patch("parsedmarc.utils.tempfile.gettempdir", return_value=blocker): - with patch("parsedmarc.utils.httpx.get", return_value=response): - with self.assertLogs("parsedmarc.log", level="WARNING") as cm: - parsedmarc.utils.load_ip_db() + """A download that cannot be written to disk logs a warning, leaves + no temporary file, and falls back to the bundled db instead of + crashing.""" + with ( + patch( + "parsedmarc.utils.httpx.get", + return_value=self._response(self.bundled_bytes()), + ), + patch("parsedmarc.utils.os.replace", side_effect=OSError("disk full")), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + parsedmarc.utils.load_ip_db() self.assertTrue( - any("Failed to save IP database" in message for message in cm.output) + any( + "Failed to save IP database: disk full" in message + for message in cm.output + ), + cm.output, + ) + self.assertEqual(os.listdir(self.cache_dir), []) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + def testUncreatableCacheDirUsesTemporaryDirectory(self): + """When the per-user cache directory cannot be created (here its + path is a regular file), a warning names it, and the download goes + to a private temporary directory for this run instead.""" + self._track_temp_fallback() + os.makedirs(self.cache_home) + with open(self.cache_dir, "wb") as f: + f.write(b"not a directory") + with ( + patch( + "parsedmarc.utils.httpx.get", + return_value=self._response(self.bundled_bytes()), + ), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + parsedmarc.utils.load_ip_db() + fallback = self._assert_temp_fallback(cm) + self.assertIn("cannot be created", "\n".join(cm.output)) + self.assertEqual( + parsedmarc.utils._IP_DB_PATH, os.path.join(fallback, "ipinfo_lite.mmdb") + ) + if os.name != "nt": + self.assertEqual(os.stat(fallback).st_mode & 0o777, 0o700) + + # A second call in the same process reuses that directory without + # repeating the warning. + with ( + patch( + "parsedmarc.utils.httpx.get", + side_effect=httpx.ConnectError("no network"), + ), + self.assertLogs("parsedmarc.log", level="INFO") as cm2, + ): + parsedmarc.utils.load_ip_db() + self.assertFalse(any("Not using" in m for m in cm2.output), cm2.output) + self.assertEqual( + parsedmarc.utils._IP_DB_PATH, os.path.join(fallback, "ipinfo_lite.mmdb") + ) + + def testTemporaryDirectoryFailureFallsBackToBundledCopy(self): + """If even the temporary directory cannot be created, load_ip_db() + warns and uses the bundled db rather than crashing startup.""" + os.makedirs(self.cache_home) + with open(self.cache_dir, "wb") as f: + f.write(b"not a directory") + with ( + patch( + "parsedmarc.utils.tempfile.mkdtemp", + side_effect=OSError("no temp space"), + ), + patch("parsedmarc.utils.httpx.get") as mock_get, + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + parsedmarc.utils.load_ip_db() + mock_get.assert_not_called() + self.assertTrue( + any( + "Failed to create a temporary IP database cache: no temp space" in m + for m in cm.output + ), + cm.output, + ) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + @unittest.skipIf(os.name == "nt", "POSIX symlink semantics") + def testSymlinkAtCachedPathIsReplacedNotFollowed(self): + """A symlink planted at the cached path does not redirect the + download into the file it points at: the victim file is unchanged + and the cached path becomes a regular file holding the download.""" + os.makedirs(self.cache_dir, mode=0o700) + victim = os.path.join(self.tmp_dir, "victim.ini") + with open(victim, "w") as f: + f.write("[general]\noriginal\n") + os.symlink(victim, self.cached_path) + body = self.bundled_bytes() + with patch("parsedmarc.utils.httpx.get", return_value=self._response(body)): + parsedmarc.utils.load_ip_db() + with open(victim) as f: + self.assertEqual(f.read(), "[general]\noriginal\n") + self.assertTrue(stat.S_ISREG(os.lstat(self.cached_path).st_mode)) + with open(self.cached_path, "rb") as f: + self.assertEqual(f.read(), body) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.cached_path) + + @unittest.skipIf(os.name == "nt", "POSIX symlink semantics") + def testOfflineIgnoresSymlinkAtCachedPath(self): + """Offline, a symlink at the cached path is not used; the bundled + db is.""" + os.makedirs(self.cache_dir, mode=0o700) + planted = os.path.join(self.tmp_dir, "planted.mmdb") + with open(planted, "wb") as f: + f.write(self.bundled_bytes()[:16]) + os.symlink(planted, self.cached_path) + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + parsedmarc.utils.load_ip_db(offline=True) + self.assertTrue( + any( + f"Ignoring the cached IP database at {self.cached_path}: " + "it is a symlink" in m + for m in cm.output + ), + cm.output, + ) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + def testOfflineIgnoresNonRegularCachedPath(self): + """A directory at the cached path is not a usable cached copy.""" + os.makedirs(self.cached_path, mode=0o700) + with self.assertLogs("parsedmarc.log", level="WARNING") as cm: + parsedmarc.utils.load_ip_db(offline=True) + self.assertTrue( + any("it is not a regular file" in m for m in cm.output), cm.output + ) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + @unittest.skipIf(not hasattr(os, "geteuid"), "POSIX ownership") + def testOfflineIgnoresCachedCopyOwnedByAnotherUser(self): + """A cached copy owned by another user is not used. Creating a file + owned by someone else needs root, so the effective uid is reported + as a different one instead: from the code's side the two are the + same observation (st_uid != geteuid()). The cache directory is + created first, under the real uid, so only the file check sees the + mismatch.""" + os.makedirs(self.cache_dir, mode=0o700) + with open(self.cached_path, "wb") as f: + f.write(b"someone else's db") + parsedmarc.utils._IP_DB_CACHE_DIR = (self.cache_dir, self.cache_dir) + with ( + patch("parsedmarc.utils.os.geteuid", return_value=os.geteuid() + 1), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + parsedmarc.utils.load_ip_db(offline=True) + self.assertTrue( + any("it is owned by another user" in m for m in cm.output), cm.output + ) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + def testInvalidDownloadIsDiscarded(self): + """A download that is not an MMDB is not cached: a warning is + logged, no file is left in the cache directory, and the bundled db + is used.""" + with ( + patch( + "parsedmarc.utils.httpx.get", + return_value=self._response(b"not a database"), + ), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + parsedmarc.utils.load_ip_db() + self.assertTrue( + any( + "Downloaded IP database is not a valid MMDB file" in m + for m in cm.output + ), + cm.output, + ) + self.assertEqual(os.listdir(self.cache_dir), []) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + def testInvalidDownloadKeepsPreviousCachedCopy(self): + """An invalid download neither replaces nor hides a valid cached + copy from an earlier run.""" + os.makedirs(self.cache_dir, mode=0o700) + with open(self.cached_path, "wb") as f: + f.write(b"earlier download") + with patch( + "parsedmarc.utils.httpx.get", + return_value=self._response(b"not a database"), + ): + parsedmarc.utils.load_ip_db() + with open(self.cached_path, "rb") as f: + self.assertEqual(f.read(), b"earlier download") + self.assertEqual(os.listdir(self.cache_dir), ["ipinfo_lite.mmdb"]) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.cached_path) + + @unittest.skipIf(not hasattr(os, "geteuid"), "POSIX permissions") + def testWorldWritableCacheDirIsNotUsed(self): + """A world-writable cache directory (which another user could have + filled) is rejected and left as it is: a database planted in it is + not selected, and the download goes to a temporary directory.""" + self._track_temp_fallback() + os.makedirs(self.cache_dir) + os.chmod(self.cache_dir, 0o777) + with open(self.cached_path, "wb") as f: + f.write(b"PLANTED") + with ( + patch( + "parsedmarc.utils.httpx.get", + return_value=self._response(self.bundled_bytes()), + ), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + parsedmarc.utils.load_ip_db() + fallback = self._assert_temp_fallback(cm) + self.assertIn("writable by its group or by others", "\n".join(cm.output)) + self.assertEqual( + parsedmarc.utils._IP_DB_PATH, os.path.join(fallback, "ipinfo_lite.mmdb") + ) + # Not repaired: the directory may belong to someone else. + self.assertEqual(os.stat(self.cache_dir).st_mode & 0o777, 0o777) + with open(self.cached_path, "rb") as f: + self.assertEqual(f.read(), b"PLANTED") + + def _parsedmarc_temp_dirs(self) -> set[str]: + return { + name + for name in os.listdir(tempfile.gettempdir()) + if name.startswith("parsedmarc-") + } + + def _load_with_failed_download(self): + """Runs load_ip_db() online with the download failing, capturing + warnings; a rejected cache directory is still replaced by a + temporary one, because a download was attempted.""" + with ( + patch( + "parsedmarc.utils.httpx.get", + side_effect=httpx.ConnectError("no network"), + ), + self.assertLogs("parsedmarc.log", level="WARNING") as cm, + ): + parsedmarc.utils.load_ip_db() + return cm + + @unittest.skipIf(not hasattr(os, "geteuid"), "POSIX permissions") + def testWorldWritableCacheDirOfflineUsesBundledCopy(self): + """Offline (and with always_use_local_file) nothing is downloaded, + so a rejected cache directory is not replaced: the planted database + in it is not selected, the bundled db is, no temporary directory is + created, and no warning claims the database "will be downloaded + again on every run".""" + self._track_temp_fallback() + os.makedirs(self.cache_dir) + os.chmod(self.cache_dir, 0o777) + with open(self.cached_path, "wb") as f: + f.write(b"PLANTED") + for offline, always_use_local_file in ((True, False), (False, True)): + with self.subTest( + offline=offline, always_use_local_file=always_use_local_file + ): + parsedmarc.utils._IP_DB_PATH = None + before = self._parsedmarc_temp_dirs() + with ( + patch("parsedmarc.utils.httpx.get") as mock_get, + self.assertNoLogs("parsedmarc.log", level="WARNING"), + self.assertLogs("parsedmarc.log", level="DEBUG") as cm, + ): + parsedmarc.utils.load_ip_db( + offline=offline, always_use_local_file=always_use_local_file + ) + mock_get.assert_not_called() + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + self.assertIsNone(parsedmarc.utils._IP_DB_CACHE_DIR) + self.assertEqual(self._parsedmarc_temp_dirs(), before) + self.assertNotIn("downloaded again", "\n".join(cm.output)) + + def testOfflineDoesNotCreateTheCacheDir(self): + """Offline, a missing per-user cache directory is not created.""" + parsedmarc.utils.load_ip_db(offline=True) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + self.assertFalse(os.path.exists(self.cache_home)) + + def testOfflineReusesTheDirectoryChosenEarlier(self): + """Offline after an online load in the same process, a copy in the + temporary directory that replaced a rejected candidate is used.""" + chosen = os.path.join(self.tmp_dir, "chosen") + os.makedirs(chosen, mode=0o700) + with open(os.path.join(chosen, "ipinfo_lite.mmdb"), "wb") as f: + f.write(b"earlier download") + parsedmarc.utils._IP_DB_CACHE_DIR = (self.cache_dir, chosen) + parsedmarc.utils.load_ip_db(offline=True) + self.assertEqual( + parsedmarc.utils._IP_DB_PATH, os.path.join(chosen, "ipinfo_lite.mmdb") + ) + + @unittest.skipIf(not hasattr(os, "geteuid"), "POSIX ownership") + def testCacheDirOwnedByAnotherUserIsNotUsed(self): + """A cache directory owned by another user is rejected. As in + testOfflineIgnoresCachedCopyOwnedByAnotherUser, a directory really + owned by someone else needs root, so the effective uid is reported + as a different one.""" + self._track_temp_fallback() + os.makedirs(self.cache_dir, mode=0o700) + with open(self.cached_path, "wb") as f: + f.write(b"PLANTED") + with patch("parsedmarc.utils.os.geteuid", return_value=os.geteuid() + 1): + cm = self._load_with_failed_download() + self._assert_temp_fallback(cm) + self.assertIn("it is owned by another user", "\n".join(cm.output)) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + @unittest.skipIf( + not hasattr(os, "geteuid") or os.geteuid() == 0, + "POSIX permissions, which root bypasses", + ) + def testReadOnlyCacheDirIsNotUsed(self): + """A cache directory the user cannot write to is rejected.""" + self._track_temp_fallback() + os.makedirs(self.cache_dir, mode=0o700) + os.chmod(self.cache_dir, 0o500) + self.addCleanup(os.chmod, self.cache_dir, 0o700) + cm = self._load_with_failed_download() + self._assert_temp_fallback(cm) + self.assertIn("it is not writable", "\n".join(cm.output)) + + @unittest.skipIf(os.name == "nt", "POSIX symlink semantics") + def testSymlinkedCacheDirIsNotUsed(self): + """A cache directory that is a symlink (here to a directory holding + a planted database) is rejected, and the planted file not used, + online or offline.""" + self._track_temp_fallback() + target = os.path.join(self.tmp_dir, "elsewhere") + os.makedirs(target, mode=0o700) + with open(os.path.join(target, "ipinfo_lite.mmdb"), "wb") as f: + f.write(b"PLANTED") + os.makedirs(self.cache_home) + os.symlink(target, self.cache_dir) + parsedmarc.utils.load_ip_db(offline=True) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + cm = self._load_with_failed_download() + self._assert_temp_fallback(cm) + self.assertIn("it is not a directory", "\n".join(cm.output)) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + @unittest.skipIf(os.name == "nt", "POSIX cache location") + def testHomelessCacheDirUsesTemporaryDirectory(self): + """With HOME unset and no passwd entry for the uid, + os.path.expanduser() returns "~/.cache/parsedmarc" unchanged, a + relative path, which would have put the cache under the current + directory. It is now rejected like any unsafe directory, and + nothing is created in the current directory.""" + self._track_temp_fallback() + workdir = os.path.join(self.tmp_dir, "cwd") + os.makedirs(workdir) + old_cwd = os.getcwd() + os.chdir(workdir) + self.addCleanup(os.chdir, old_cwd) + env = {k: v for k, v in os.environ.items() if k != "XDG_CACHE_HOME"} + with ( + patch.dict(os.environ, env, clear=True), + patch("parsedmarc.utils.os.path.expanduser", side_effect=lambda p: p), + ): + cm = self._load_with_failed_download() + output = "\n".join(cm.output) + self.assertIn("for the IP database cache: it is not an absolute path", output) + self.assertIn("will be downloaded again on every run", output) + chosen = parsedmarc.utils._IP_DB_CACHE_DIR + assert chosen is not None + self.assertTrue(os.path.isabs(chosen[1])) + self.assertEqual(os.listdir(workdir), []) + self.assertEqual(parsedmarc.utils._IP_DB_PATH, self.bundled) + + @unittest.skipIf(os.name == "nt", "POSIX cache location") + def testRelativeXdgCacheHomeIsIgnored(self): + """A relative XDG_CACHE_HOME is invalid per the XDG Base Directory + specification and ignored in favor of ~/.cache/parsedmarc.""" + home = os.path.join(self.tmp_dir, "home") + with patch.dict(os.environ, {"XDG_CACHE_HOME": "relative/cache", "HOME": home}): + with patch( + "parsedmarc.utils.httpx.get", + return_value=self._response(self.bundled_bytes()), + ): + parsedmarc.utils.load_ip_db() + expected = os.path.join(home, ".cache", "parsedmarc", "ipinfo_lite.mmdb") + self.assertEqual(parsedmarc.utils._IP_DB_PATH, expected) + self.assertTrue(os.path.isfile(expected)) + self.assertFalse(os.path.exists("relative")) + + def testWindowsCacheLocation(self): + """On Windows the cache lives under %LOCALAPPDATA%, or under + ~/AppData/Local when that variable is unset.""" + home = os.path.join(self.tmp_dir, "home") + with ( + patch("parsedmarc.utils.os.name", "nt"), + patch.dict( + os.environ, + {"LOCALAPPDATA": "/lad", "HOME": home, "USERPROFILE": home}, + ), + ): + with_var = parsedmarc.utils._ip_db_cache_dir_candidate() + del os.environ["LOCALAPPDATA"] + without_var = parsedmarc.utils._ip_db_cache_dir_candidate() + self.assertEqual(with_var, os.path.join("/lad", "parsedmarc", "cache")) + self.assertEqual( + without_var, + os.path.join(home, "AppData", "Local", "parsedmarc", "cache"), ) - bundled = str(files(parsedmarc.resources.ipinfo).joinpath("ipinfo_lite.mmdb")) - self.assertEqual(parsedmarc.utils._IP_DB_PATH, bundled) class TestConfigureIpinfoApiProbe(unittest.TestCase):