🔒️(pymta) harden pymta with new settings & limits - #777
Open
sylvinus wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pure-Python MTA now validates settings, supports trusted PROXY networks, enforces SMTP session and DATA deadlines, preserves counters across STARTTLS, and applies explicit MDA response and JWT TTL rules. Documentation and tests cover the new behavior. ChangesSMTP and MDA hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SMTPClient
participant HardenedSMTP
participant SMTPHandler
participant MDAClient
participant Transport
SMTPClient->>HardenedSMTP: Establish SMTP session
HardenedSMTP->>Transport: Arm session deadline
SMTPClient->>SMTPHandler: Send SMTP commands and DATA
SMTPHandler->>MDAClient: Deliver within remaining DATA budget
MDAClient-->>SMTPHandler: Return delivery result
SMTPHandler-->>HardenedSMTP: Return reply and disconnect request
HardenedSMTP->>Transport: Flush reply and close when requested
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mta-in/README.md`:
- Around line 49-51: Update the Authorization documentation’s body_hash
description to remove the “replay-proof per-request” claim, stating only that it
binds the token to the exact request bytes. Do not add replay tracking or other
implementation changes unless the documentation is intended to guarantee replay
prevention.
In `@src/mta-in/src/pymta/handler.py`:
- Around line 274-286: Update handle_MAIL() to count malformed sender addresses
and invalid or oversized SIZE parameters, then apply the shared
PYMTA_HARD_ERROR_LIMIT 421-and-disconnect guard before each soft-error return.
Reuse the existing guard behavior from handle_RCPT() and preserve the current
501 responses for individual MAIL validation failures.
- Around line 142-162: Update _remaining_data_budget so it never floors a
depleted budget to one second: after subtracting spent time and
_REPLY_RESERVE_SECONDS from settings.PYMTA_DATA_TIMEOUT, return the actual
nonnegative remaining value or raise TimeoutError when it is nonpositive,
allowing the existing timeout path to send 451 before the DATA deadline.
In `@src/mta-in/src/pymta/mda_async.py`:
- Around line 44-62: Scope _PERMANENT_STATUSES handling in _post to delivery
requests only, so check_recipient responses with 400, 413, or 415 defer instead
of producing temp_fail=False; preserve deferral for other unexpected
recipient-check responses. Update permanent-status tests to exercise deliver and
add check_recipient coverage verifying these statuses defer.
In `@src/mta-in/src/pymta/server.py`:
- Around line 53-59: Update the startup validation guarding
PYMTA_TRUSTED_PROXIES to reject entries representing zero-prefix networks,
including 0.0.0.0/0 and ::/0, in addition to an empty allowlist. Preserve the
existing RuntimeError behavior and ensure validation occurs before the server
accepts connections.
In `@src/mta-in/tests/test_settings.py`:
- Around line 76-79: Update _env_bool so unrecognized non-empty environment
values raise ValueError instead of silently returning the supplied default.
Revise test_bool_unrecognised_value_keeps_the_default to assert the exception
for both default values, while preserving recognized-value parsing and
missing-variable defaults.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9365f2bf-db5a-4c25-97bd-6bc7bb679c00
📒 Files selected for processing (16)
deploy/env/mta-in-py.defaultssrc/mta-in/README.mdsrc/mta-in/src/pymta/address.pysrc/mta-in/src/pymta/controller.pysrc/mta-in/src/pymta/handler.pysrc/mta-in/src/pymta/limits.pysrc/mta-in/src/pymta/mda_async.pysrc/mta-in/src/pymta/metrics.pysrc/mta-in/src/pymta/server.pysrc/mta-in/src/pymta/settings.pysrc/mta-in/src/pymta/smtp_protocol.pysrc/mta-in/tests/test_handler.pysrc/mta-in/tests/test_hardened_smtp.pysrc/mta-in/tests/test_mda_async.pysrc/mta-in/tests/test_server_config.pysrc/mta-in/tests/test_settings.py
Comment on lines
+76
to
+79
| def test_bool_unrecognised_value_keeps_the_default(monkeypatch): | ||
| monkeypatch.setenv("PYMTA_TEST_BOOL", "maybe") | ||
| assert _env_bool("PYMTA_TEST_BOOL", True) is True | ||
| assert _env_bool("PYMTA_TEST_BOOL", False) is False |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject unrecognized Boolean values.
This test requires invalid configuration to fall back silently. It contradicts the fail-fast contract on Lines 3-4.
For example, PYMTA_ENABLE_PROXY_PROTOCOL=treu uses the default False. A deployment behind a PROXY-enabled load balancer then attributes all sessions to the load balancer.
Update _env_bool to raise ValueError, and require that behavior here.
Proposed test change
-def test_bool_unrecognised_value_keeps_the_default(monkeypatch):
+def test_bool_rejects_unrecognised_value(monkeypatch):
monkeypatch.setenv("PYMTA_TEST_BOOL", "maybe")
- assert _env_bool("PYMTA_TEST_BOOL", True) is True
- assert _env_bool("PYMTA_TEST_BOOL", False) is False
+ with pytest.raises(ValueError, match="must be a boolean"):
+ _env_bool("PYMTA_TEST_BOOL", False)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mta-in/tests/test_settings.py` around lines 76 - 79, Update _env_bool so
unrecognized non-empty environment values raise ValueError instead of silently
returning the supplied default. Revise
test_bool_unrecognised_value_keeps_the_default to assert the exception for both
default values, while preserving recognized-value parsing and missing-variable
defaults.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mta-in/README.md (1)
29-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winScope the outcome table to
deliver.The table states that HTTP 400, 413, and 415 always produce
554.check_recipientdefers these statuses and the SMTP handler returns451.Mark this table as the
deliveroutcome mapping. Document that recipient checks defer these statuses.Proposed documentation change
-### Translating the MDA outcome +### Translating the MDA delivery outcome + +For `check/`, HTTP 400, 413, and 415 are temporary failures. They produce +SMTP `451` because a recipient check contains no message to reject. -Losing a legitimate message is worse than asking the sender to retry, so the permanent-rejection set is an explicit allow-list and everything else defers: +For `deliver/`, losing a legitimate message is worse than asking the sender to retry, so the permanent-rejection set is an explicit allow-list and everything else defers:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mta-in/README.md` around lines 29 - 42, Update the “Translating the MDA outcome” section to explicitly scope its outcome table to deliver responses. Add documentation stating that check_recipient defers 400, 413, and 415, so its SMTP handler returns 451 rather than applying the table’s 554 mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mta-in/README.md`:
- Around line 29-42: Update the “Translating the MDA outcome” section to
explicitly scope its outcome table to deliver responses. Add documentation
stating that check_recipient defers 400, 413, and 415, so its SMTP handler
returns 451 rather than applying the table’s 554 mapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: de4db120-4f89-4d92-8686-a1726bde25f0
📒 Files selected for processing (13)
src/mta-in/README.mdsrc/mta-in/pyproject.tomlsrc/mta-in/src/delivery_milter.pysrc/mta-in/src/pymta/handler.pysrc/mta-in/src/pymta/mda_async.pysrc/mta-in/src/pymta/server.pysrc/mta-in/src/pymta/settings.pysrc/mta-in/tests/test_handler.pysrc/mta-in/tests/test_limits.pysrc/mta-in/tests/test_mda_async.pysrc/mta-in/tests/test_security.pysrc/mta-in/tests/test_server_config.pysrc/mta-in/tests/test_settings.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mta-in/src/pymta/server.py`:
- Around line 56-77: Reject startup in the server configuration validation when
PROXY protocol is enabled and PYMTA_TRUSTED_PROXIES is empty or contains a
zero-prefix network, raising RuntimeError instead of warning. In
src/mta-in/src/pymta/handler.py lines 194-202, return False for an empty
allowlist; update src/mta-in/tests/test_server_config.py lines 26-64 and
src/mta-in/tests/test_handler.py lines 632-644 to assert rejection; document the
explicit trusted proxy IP/CIDR requirement at src/mta-in/README.md line 111 and
remove the unspecified trust-boundary topology at lines 131-140.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a04f3651-5248-4f1b-897c-550c63f8c90b
📒 Files selected for processing (7)
src/mta-in/README.mdsrc/mta-in/src/pymta/handler.pysrc/mta-in/src/pymta/mda_async.pysrc/mta-in/src/pymta/server.pysrc/mta-in/tests/test_handler.pysrc/mta-in/tests/test_mda_async.pysrc/mta-in/tests/test_server_config.py
Comment on lines
+56
to
+77
| if not settings.PYMTA_ENABLE_PROXY_PROTOCOL: | ||
| return | ||
| # A zero-prefix network (0.0.0.0/0, ::/0) matches every peer, so it is the | ||
| # empty allowlist wearing a disguise. Same posture, same warning. | ||
| catch_all = [net for net in settings.PYMTA_TRUSTED_PROXIES if net.prefixlen == 0] | ||
| if not settings.PYMTA_TRUSTED_PROXIES or catch_all: | ||
| why = ( | ||
| "PYMTA_TRUSTED_PROXIES is empty" | ||
| if not settings.PYMTA_TRUSTED_PROXIES | ||
| else f"PYMTA_TRUSTED_PROXIES contains {', '.join(str(n) for n in catch_all)}, " | ||
| "which matches every peer" | ||
| ) | ||
| logger.warning( | ||
| "SECURITY: PROXY protocol is enabled but %s, so a PROXY header is trusted " | ||
| "from any peer. Any host able to reach port %s directly can forge its " | ||
| "source IP past the per-IP caps and into the Received header. Set it to " | ||
| "the load balancer's IPs/CIDRs, and make sure the port is reachable only " | ||
| "from the balancer.", | ||
| why, | ||
| settings.PYMTA_SMTP_PORT, | ||
| ) | ||
| return |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject PROXY protocol without a specific trusted-peer allowlist.
An empty or catch-all allowlist authorizes any host that can reach the SMTP listener to forge src_addr. That forged address bypasses per-IP limits and enters the MDA Received metadata. A warning does not establish the required network boundary.
src/mta-in/src/pymta/server.py#L56-L77: raiseRuntimeErrorwhenPYMTA_TRUSTED_PROXIESis empty or contains a zero-prefix network.src/mta-in/src/pymta/handler.py#L194-L202: returnFalsefor an empty allowlist as defense in depth.src/mta-in/tests/test_server_config.py#L26-L64: change warning tests to require startup rejection.src/mta-in/tests/test_handler.py#L632-L644: require an empty allowlist to reject the PROXY header.src/mta-in/README.md#L111-L111: state that PROXY mode requires explicit trusted proxy IPs or CIDRs.src/mta-in/README.md#L131-L140: remove the topology that permits an unspecified proxy trust boundary.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 74-74: Clear-text logging of sensitive information
This expression logs sensitive data (secret) as clear text.
📍 Affects 5 files
src/mta-in/src/pymta/server.py#L56-L77(this comment)src/mta-in/src/pymta/handler.py#L194-L202src/mta-in/tests/test_server_config.py#L26-L64src/mta-in/tests/test_handler.py#L632-L644src/mta-in/README.md#L111-L111src/mta-in/README.md#L131-L140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mta-in/src/pymta/server.py` around lines 56 - 77, Reject startup in the
server configuration validation when PROXY protocol is enabled and
PYMTA_TRUSTED_PROXIES is empty or contains a zero-prefix network, raising
RuntimeError instead of warning. In src/mta-in/src/pymta/handler.py lines
194-202, return False for an empty allowlist; update
src/mta-in/tests/test_server_config.py lines 26-64 and
src/mta-in/tests/test_handler.py lines 632-644 to assert rejection; document the
explicit trusted proxy IP/CIDR requirement at src/mta-in/README.md line 111 and
remove the unspecified trust-boundary topology at lines 131-140.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation