Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TODO]: Tests - Replace nc / openssl mail sending with swaks (or similar) #3728

Closed
polarathene opened this issue Dec 27, 2023 · 1 comment · Fixed by #3732
Closed

[TODO]: Tests - Replace nc / openssl mail sending with swaks (or similar) #3728

polarathene opened this issue Dec 27, 2023 · 1 comment · Fixed by #3732
Assignees
Labels
area/tests kind/improvement Improve an existing feature, configuration file or the documentation meta/help wanted The OP requests help from others - chime in! :D service/postfix stale-bot/ignore Indicates that this issue / PR shall not be closed by our stale-checking CI

Comments

@polarathene
Copy link
Member

polarathene commented Dec 27, 2023

Description

Up for grabs for anyone who'd like to contribute, no rush :)

  • Add smtpd_data_restrictions = reject_unauth_pipelining to our Postfix main.cf. See below section for more info. This change is presently blocked by our test-suite. Hence the need for adopting something like swaks.
  • swaks or similar for sending mail in the test suite, replacing nc / openssl for this purpose.
    • nc usage has mostly been migrated into a helper method.
    • openssl is used elsewhere when necessary, but would ideally be unified with the helper as a command like swaks would handle both use-cases.

smtpd_data_restrictions = reject_unauth_pipelining

Reference (review comment plus feedback provides plenty of details on this setting).

  • Amavis currently sets smtpd_data_restrictions = reject_unauth_pipelining, which is standard config for integrating this service with Postfix.
  • With the recent SMTP smuggling vulnerability, it is also applicable as a short-term workaround for all postfix versions.
  • Presently DMS has this restriction applied to client and recipient restrictions, but these could be dropped in favor of data restriction only. Amavis should keep it's explicit override for better maintenance visibility, rather than relying on implicitly inheriting it from main.cf when this change is introduced.
  • The change is deferred due to breaking the test-suite. swaks or similar must replace our current reliance on nc and openssl for sending mail (via raw SMTP commands).
Reference (if linked review comment disappears)

This has happened previously, so in interest of not losing the referenced information a screenshot is taken:

image

Markdown backup:

**Change overview** (_see review feedback for insights_):
- [ ] Add `smtpd_data_restrictions` to `main.cf`, remove the client and recipient restriction for `reject_unauth_pipelining`.
  - This defers rejection to a later stage (_assuming a permit rule didn't skip restrictions prior_), where any detected pipelining in the exchange would trigger the restriction, not just within `DATA` command.
- [ ] Keep `smtpd_data_restrictions` in Amavis config.
- [ ] For a v14 target, `smtpd_data_restrictions` is not necessary to workaround SMTP smuggling vulnerability.
  - We would have Postfix 3.7.6, this makes `smtpd_forbid_unauth_pipelining = yes` available. It should be adopted instead (but the `smtpd_data_restrictions` checklist above is valid (_perhaps as a separate PR_).
  - Both short-term workarounds also reference enabling `smtpd_discard_ehlo_keywords = chunking`. This [conflicts with the same existing parameter already configured](https://github.com/docker-mailserver/docker-mailserver/blob/72517d3f824859cb15a3ccc653ad8cc4bb1a4c32/target/postfix/main.cf#L57), prepending/appending to that should be fine, but it ~~should be [assessed that the feature doesn't have any logic that would accidentally remove `chunking`](https://github.com/docker-mailserver/docker-mailserver/pull/3572), the feature tests may not be sufficient for that, but should be obvious looking over the PR diff~~ (_**EDIT:** Doesn't look like a concern_ 👍 ).
- [ ] Additional context in comments should be added regarding the long-term fix:
  - [ ] It replaces the short-term fix parameters as workarounds.
  - [ ] It's unclear when Debian Bookworm will update Postfix package with the available fix, but we can mention the min version needed to toggle the long-term fix.

---

**Overview for `smtpd_data_restrictions`:**
- `smtpd_data_restrictions = reject_unauth_pipelining` should probably remain on Amavis config, even though it'd be technically redundant (_since it would now inherit from `main.cf`_). Reasoning is for maintenance, where this is an explicit Amavis config (official docs), and DMS `main.cf` could change again as review feedback implies. Better for maintenance visibility to retain it.
- `smtpd_data_restrictions = reject_unauth_pipelining` on `main.cf` still may make sense, regardless of Amavis integration.
  - Presently we have this check at earlier restriction phases (client and recipient), but those seem unnecessary and can be dropped in favor of deferring to the data restriction which Postfix docs seem to indicate would be sufficient. Only difference would be rejecting due to the restriction slightly earlier (_at the `RCPT TO` SMTP command_).
  - Potentially breaking, but only in the sense that a client is not respecting waiting their turn at the `DATA` command, but for some reason was respectful earlier 🤷‍♂️ 
@polarathene polarathene added meta/help wanted The OP requests help from others - chime in! :D service/postfix area/tests kind/improvement Improve an existing feature, configuration file or the documentation stale-bot/ignore Indicates that this issue / PR shall not be closed by our stale-checking CI labels Dec 27, 2023
@georglauterbach georglauterbach self-assigned this Dec 28, 2023
@georglauterbach
Copy link
Member

I am currently working on this, should be relatively easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tests kind/improvement Improve an existing feature, configuration file or the documentation meta/help wanted The OP requests help from others - chime in! :D service/postfix stale-bot/ignore Indicates that this issue / PR shall not be closed by our stale-checking CI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants