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

smtp forward dest split routing #1847

Merged
merged 4 commits into from Mar 8, 2017

Conversation

Projects
None yet
2 participants
@msimerson
Member

msimerson commented Mar 6, 2017

This is progress on #1472 and it improves upon a longstanding mail routing issue that affects users of per-domain mail forwarding.

I have a bunch of [domain.com] blocks in smtp_forward.ini and most domains have differing forward hosts. When a transaction has recipients in domains with different forward hosts, then those subsequent recipients are sent to the wrong destination host. The one_message_per_rcpt option in smtp_forward just does an RSET between transactions to the forward host of the first recipient.

The previous workaround was to forward transactions with multiple destination hosts to the default forward host and let the smart host sort it out. See #573 and #671. That works reasonably well the vast majority of the time. The edge case is split transactions with undeliverable recipients. They end up being queued and some of those recipients are ultimately undeliverable and bounce.

This PR removes the "forward multiple destination transactions to smarthost and let it sort it out" behavior and adds recipient validation to smtp_forward. Example:

 -> MAIL FROM:<matt@****.tnpi.net>
<-  250 sender <matt@****.tnpi.net> OK
 -> RCPT TO:<matt@tnpi.net>
<-  250 recipient <matt@tnpi.net> OK
 -> RCPT TO:<ryan@example.net>
<** 450 [7E794F58-35C@haraka] Queue error, try again soon
 -> DATA
<-  354 go ahead, make my day

Within a transaction, it defers subsequent recipients if they have a different forward host than the first recipient. This is possible because smtp_forward.ini already has the forward host information. Other recipient/queue plugins can cooperate by setting txn.notes.queue.

Changes proposed in this pull request:

  • smtp_forward: add check_sender() & check_recipient()
    • disabled by default
    • when enabled, if a domain has a definition block in smtp_forward.ini:
      • validate sender & recipients
        • for myself, the host_list plugin is no longer necessary
      • for relaying clients, set queue to txn.notes.queue:outbound
      • for inbound recipients, set txn.notes.queue=smtp_forward:hostname
      • if subsequent recipients on a transaction have differing forward routes:
        • DENYSOFT
        • it's the only safe/sane way to split the transaction
    • remove the "forward to default host when conflicting forward hosts" behavior added in #671
  • if txn.notes.queue is defined but doesn't start with smtp_forward, then smtp_forward doesn't queue.
  • moved the AUTH block out of hook_queue to its own function
  • add get_mx hook, so that recipients in connections where relaying=true that are destined to a local domain (defined in smtp_forward.ini and for which we are the MX for) can be queued to outbound and still be deliverable.
  • qmail_deliverable
    • config, add manually specified queue option
    • like smtp_forward, qmd already supports domain routing
      • specify a queue (for myself: smtp_forward), supporting the smtp_forward queue splitting
    • moved qmd options into get_qmd_response (was plugin global)

Checklist:

  • docs updated
  • tests updated

@msimerson msimerson changed the title from Smtp forward dest split to Smtp forward dest split routing Mar 6, 2017

@msimerson msimerson changed the title from Smtp forward dest split routing to smtp forward dest split routing Mar 6, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 6, 2017

Codecov Report

Merging #1847 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1847   +/-   ##
=======================================
  Coverage   46.23%   46.23%           
=======================================
  Files          22       22           
  Lines        5864     5864           
  Branches     1474     1474           
=======================================
  Hits         2711     2711           
  Misses       3153     3153
Impacted Files Coverage Δ
outbound.js 13.45% <ø> (ø)
smtp_client.js 6.8% <0%> (-0.03%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51589b3...d6dcf10. Read the comment docs.

codecov-io commented Mar 6, 2017

Codecov Report

Merging #1847 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1847   +/-   ##
=======================================
  Coverage   46.23%   46.23%           
=======================================
  Files          22       22           
  Lines        5864     5864           
  Branches     1474     1474           
=======================================
  Hits         2711     2711           
  Misses       3153     3153
Impacted Files Coverage Δ
outbound.js 13.45% <ø> (ø)
smtp_client.js 6.8% <0%> (-0.03%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51589b3...d6dcf10. Read the comment docs.

msimerson added some commits Mar 6, 2017

@msimerson msimerson merged commit 016a83d into haraka:master Mar 8, 2017

3 checks passed

codecov/patch Coverage not affected when comparing 51589b3...d6dcf10
Details
codecov/project 46.23% remains the same compared to 51589b3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@msimerson msimerson deleted the msimerson:smtp-forward-dest-split branch Mar 8, 2017

msimerson added a commit that referenced this pull request Mar 28, 2017

merge master into v3 (#1869)
* ignore undefined socket.remoteAddress (#1846)

similar to #239

fixes #1835

* URL to manual was 404, point to Plugins.md (#1844)

* smtp forward dest split routing (#1847)

* split transactions with different forward routing

* use tls_ini_section_with_defaults (#1848)

* use net_utils.tls_ini_section_with_defaults()
fixes #1691

* assure conn/tran still exists before storing results (#1849)

* chore(package): update redis to version 2.7.0 (#1854)

https://greenkeeper.io/

* fix(package): update ipaddr.js to version 1.3.0 (#1857)

https://greenkeeper.io/

* Fix modifier regexp (#1859)

* doc typo in config file name (#1865)

Actual this is typo in plugin source - it uses "ini" file from another plugin called rabbitmq . So, in fact, no one of the plugins uses config file with name "rabbitmq_amqplib.ini" . This cause error (default values for the plugin) if you trying use the official documentation.

* fix(package): update async to version 2.2.0 (#1863)

https://greenkeeper.io/

* replace util.inherits with class .. extends (#1862)

* replace the discouraged util.inherits with `class Foo extends Bar` syntax
* chunkemitter: remove Buffer.concat shim for node < 0.7.11
* fsync_writesteam: moved into ./outbound (only required once)
* tls_socket: always set `isServer=true` (an recent merge made it
conditional)
* removed test for cleartext.getCipher, it *should* always be present

* Added RabbitMQ vhost support (#1866)

* Add RabbitMQ vhost support
Virtual Host. Start with "/". Leave blank or not use if you don't want to use virtual hosts.

* Get pool (#1868)

* pass cfg object instead, simplifying get_pool
* more consistency with variable naming and passing
* add integration test: send message w/smtp_client

* tls consistency cleanups (#1851)

* tls_socket: add options.SNICallback

- refresh options.secureContext after setting a different cert in
options
- plugins/tls: add plugin.loadPemDir
- replace some + ‘ ‘ + patterns with string interpolation
- replace some `function (a,b) {` patterns with `(a,b) => {`

* add very basic tests for tls_socket

* move SNI into plugins/tls

* revert

msimerson added a commit that referenced this pull request Mar 28, 2017

merge master into v2 (#1870)
* ignore undefined socket.remoteAddress (#1846)

similar to #239

fixes #1835

* URL to manual was 404, point to Plugins.md (#1844)

* smtp forward dest split routing (#1847)

* split transactions with different forward routing

* use tls_ini_section_with_defaults (#1848)

* use net_utils.tls_ini_section_with_defaults()
fixes #1691

* assure conn/tran still exists before storing results (#1849)

* chore(package): update redis to version 2.7.0 (#1854)

https://greenkeeper.io/

* fix(package): update ipaddr.js to version 1.3.0 (#1857)

https://greenkeeper.io/

* Fix modifier regexp (#1859)

* doc typo in config file name (#1865)

Actual this is typo in plugin source - it uses "ini" file from another plugin called rabbitmq . So, in fact, no one of the plugins uses config file with name "rabbitmq_amqplib.ini" . This cause error (default values for the plugin) if you trying use the official documentation.

* fix(package): update async to version 2.2.0 (#1863)

https://greenkeeper.io/

* replace util.inherits with class .. extends (#1862)

* replace the discouraged util.inherits with `class Foo extends Bar` syntax
* chunkemitter: remove Buffer.concat shim for node < 0.7.11
* fsync_writesteam: moved into ./outbound (only required once)
* tls_socket: always set `isServer=true` (an recent merge made it
conditional)
* removed test for cleartext.getCipher, it *should* always be present

* Added RabbitMQ vhost support (#1866)

* Add RabbitMQ vhost support
Virtual Host. Start with "/". Leave blank or not use if you don't want to use virtual hosts.

* Get pool (#1868)

* pass cfg object instead, simplifying get_pool
* more consistency with variable naming and passing
* add integration test: send message w/smtp_client

* tls consistency cleanups (#1851)

* tls_socket: add options.SNICallback

- refresh options.secureContext after setting a different cert in
options
- plugins/tls: add plugin.loadPemDir
- replace some + ‘ ‘ + patterns with string interpolation
- replace some `function (a,b) {` patterns with `(a,b) => {`

* add very basic tests for tls_socket

* move SNI into plugins/tls

* revert

@msimerson msimerson referenced this pull request Mar 31, 2017

Merged

Split delivery relay #1875

msimerson added a commit that referenced this pull request Jul 29, 2017

merge master changes into V3 (#2027)
* ignore undefined socket.remoteAddress (#1846)

similar to #239

fixes #1835

* URL to manual was 404, point to Plugins.md (#1844)

* smtp forward dest split routing (#1847)

* split transactions with different forward routing

* use tls_ini_section_with_defaults (#1848)

* use net_utils.tls_ini_section_with_defaults()
fixes #1691

* assure conn/tran still exists before storing results (#1849)

* chore(package): update redis to version 2.7.0 (#1854)

https://greenkeeper.io/

* fix(package): update ipaddr.js to version 1.3.0 (#1857)

https://greenkeeper.io/

* Fix modifier regexp (#1859)

* doc typo in config file name (#1865)

Actual this is typo in plugin source - it uses "ini" file from another plugin called rabbitmq . So, in fact, no one of the plugins uses config file with name "rabbitmq_amqplib.ini" . This cause error (default values for the plugin) if you trying use the official documentation.

* fix(package): update async to version 2.2.0 (#1863)

https://greenkeeper.io/

* replace util.inherits with class .. extends (#1862)

* replace the discouraged util.inherits with `class Foo extends Bar` syntax
* chunkemitter: remove Buffer.concat shim for node < 0.7.11
* fsync_writesteam: moved into ./outbound (only required once)
* tls_socket: always set `isServer=true` (an recent merge made it
conditional)
* removed test for cleartext.getCipher, it *should* always be present

* Added RabbitMQ vhost support (#1866)

* Add RabbitMQ vhost support
Virtual Host. Start with "/". Leave blank or not use if you don't want to use virtual hosts.

* Get pool (#1868)

* pass cfg object instead, simplifying get_pool
* more consistency with variable naming and passing
* add integration test: send message w/smtp_client

* tls consistency cleanups (#1851)

* tls_socket: add options.SNICallback

- refresh options.secureContext after setting a different cert in
options
- plugins/tls: add plugin.loadPemDir
- replace some + ‘ ‘ + patterns with string interpolation
- replace some `function (a,b) {` patterns with `(a,b) => {`

* add very basic tests for tls_socket

* move SNI into plugins/tls

* revert

* tls: remove interim variables (#1871)

* - remove interim variables
* remove unused _controlReleased

* Split delivery relay (#1875)

* do not defer relaying clients with split transactions

* disable naïve comment stripping (#1876)

* disable naïve comment stripping

fixes #1874

* tickle travis

* fix(package): update async to version 2.3.0 (#1878)

https://greenkeeper.io/

* chore(package): update ocsp to version 1.2.0 (#1879)

https://greenkeeper.io/

* Added missing 'default' keyword in rspamd plugin. Score wasn't added … (#1856)

* Added missing 'default' keyword in rspamd plugin. Score wasn't added properly to header
* Logging envelope from / recipients in lowercase in order to avoid changing ES analyzer ( slower performance )

* handle case where OCSP server is unavailable (#1880)

this likely fixes #1855

* Add .editorconfig (#1884)

* Removing typo from 'relay.md' (#1886)

* Fix for issue #1410 (#1887)

* Fix for issue #1410

* Remove logging

* Confirmed that it works

* fixed auth_proxy socket error (#1894)

#1893

* Implement the missing upgrade method on SMTPClient. (#1901)

* fix(package): update async to version 2.4.0 (#1896)

https://greenkeeper.io/

* 1861 binary log (#1902)

* Binary log file

The log file without that replacement, is interpreted as binary file log issue.

* Binary log

replace regexp binary replace with code replace

* remove variable, line wrap

closes 1861

* Outbound split and move into folder (#1850)

* Move outbound to its own sub-folder

* Start of splitting up outbound into separate files

* Progress on splitting

* More progress, not working yet

* Outbound now seems to work. Tests still fail.

* Add missing file

* Fix outbound.js test

* Fix lint errors

* Outbound-Tests for RFC3464 bounce messages reworked (#1889)

* fix path to fsync_writestream (./ vs ../)

* make sure list_re is defined before access (#1903)

* Check error code instead of number.  Fixes GH-1895. (#1899)

* Bring client pool up to where we were before split (#1915)

* Bring client pool up to where we were before split
* Add changes from #1848
* Merge in class/extends changes

* Fix all logging to be [outbound] prefixed
* Fix self logger
* Fix net_utils call

* fix(package): update sprintf-js to version 1.1.0 (#1906)

* F/outbound client certificates (#1908)

* Add tls_options and associated tests to queue/smtp_forward
* Fix exception when no tls blacklist added. Add tests for smtp_client.
* Add documentation for outbound client_certificate

* Fix path parsing bug on Windows platform (#1919)

* Attempt to add some belt and braces around FIN (#1916)

* Attempt to add some belt and braces around FIN

* Add logging on write failure

* Allow outbound pools to be disabled (#1917)

* Allow outbound pools to be disabled

* Few changes after actually testing the code

* Fix for shutting down clients

* Set graceful shutdown off by default (#1927)

* Set graceful shutdown off by default

Still allow haraka -c <path> --graceful
Also sets graceful shutdown to shutdown in max 2xChildren amount
of time it sets to shutdown.

* Allow gracefulShutdown() to always be graceful

Makes the tests pass

* Fix comment

* Run dumped logs through plugins not console (#1929)

* Don't blow the stack on qstat (#1930)

* Don't blow the stack on qstat
* Should fix another call stack blowing issue

* Allow "Unknown Result" and Socket Error to Try Next Host (#1931)

* Allow "Unknown Result" and Socket Error to Try Next Host

If multiple hosts are listed for clamd and an "unknown result" is returned or a socket error occurs after connection, then attempt to try remaining hosts before returning DENYSOFT.

* Remove trailing spaces

* Clear off some done tasks (#1928)

* Clear off some done tasks

* remove completed or abandoned TODO items

* Check pool exists before delete (#1937)

* Fixes loading outbound under cluster. (#1934)

I don't really understand why this stuff broke.

It seems to be a bug in node's require() caching, frankly.

When loading modules they would return {} before actually
being loaded. This appears to be due to recursive requires.

This might fix #1933

Please test

* Fix cluster messaging for node v6+ (#1938)

* Fix undefined variable platformDOT in hmail.js (#1943)

* Update qfile.js

* Update hmail.js

* Update index.js

* Fix queue not loaded for single process (#1941)

* Fix queue not loaded for single process

* Update server.js

* Use the  sunset keyword in version specific code blocks (#1939)

Because `sunset` is the keyword we'll grep for (if we follow our instructions) when doing a semver major release.

* Fix PROTOCOL logs that have intermediate \n chars (#1947)

Most noted in SpamAssassin output

* Load logger in a setImmediate call (#1948)

* Bump haraka-results required version (#1949)

* Bump haraka-results required version

* Update package.json

* Fix undefined FsyncWriteStream var (#1953)

* Fix undefined FsyncWriteStream var

* Update hmail.js

* Be more strict in attachment filename matching (#1957)

* Be more strict in attachment filename matching

This copes with filename="d'euvre"

* Add a simple test

* Use punycode domain (#1944)

* Support SMTPUTF8 properly

* Use punycode for mail from too

* Enable SMTPUTF8 support

* Transaction is null there. Wait until it's set

* Add needed utf8 changes to connection.js

* punycode for the other addresses

* Fix trailing space

* Be consistent with todo.domain (#1960)

* Minor typo fix (#1963)

Thanks

* Update package.json (#1968)

* Fix unaquired socket errors in test suite (#1971)

* Support the new address-rfc2822 (#1970)

* Support the new address-rfc2822

New module supports Groups, which have no host method

* Force version of rfc2822 required by the fix

* Fix missing backslash

* RabbitMQ: fix encoding of user and password when creating connection string (#1964)

* fix(package): update ipaddr.js to version 1.4.0 (#1972)

* Fix link redirection (#1975)

* require node LTS version (6+) (#1958)

* require node LTS version (6+)

* appveyor: install node v6

* whitespace changes for eslint 4 compat (#1979)

* whitespace changes for eslint 4 compat

* one more ws change

* fix(package): update iconv to version 2.3.0 (#1981)

* fix(package): update async to version 2.5.0 (#1982)

* Add node v8 (#1951)

* Add node v8
* enable node v8 testing, permitting v8 failures

* import Plugins.md from v3 branch (#1991)

* remove logging line (#1985)

the prefix is not required in config/plugins as the message states, but it is required for an NPM packaged plugin that requires another NPM packaged plugin and is then spurious

* doc: add note that smtp_forward only supports STARTTLS (#1988)

* Update qmail-queue.js (#1997)

qmail-queue binary Adds a Received: header with bare LF. Haraka sets CRLF by default resulting in mixed line endings which break header processing in some e-mail clients.
messagestream can convert CRLF to LF on the fly when calling qmail-send.

* rebuild blacklist upon file change (#1990)

previously just added new entries

fixes 1987

* Remove spurious logs (#1989)

* replace plugins/log.elasticsearch with npm packaged (#2004)

* replace access with npm packaged haraka-plugin-access (#1992)

* use WRITE_EXCL from haraka-constants (#2011)

* fix(package): update js-yaml to version 3.9.0 (#2002)

* build_todo() is part of the outbound/index.js api (#2016)

* rename xclient.hosts to match plugin & docs (#2014)

fixes #1265

* correct the config file name to relay.ini (#2012)

* fix(package): update semver to version 5.4.0 (#2015)

* require node 8 tests to pass (#2017)

now that node 8.2+ is on Travis

* fixes #1765 (#2013)

* Fix auth plugin failure when re-selecting auth method (#2000)

When sending the following sequence of commands, the auth plugin fails
since it tries to decode an undefined value as base64:

    AUTH LOGIN Z2lyaXNo
    334 UGFzc3dvcmQ6
    AUTH LOGIN
    500 Unrecognized command # at this point plugin has failed

Fixes #1999

* Improve logging UUID tracking in Outbound (#2018)

* prep release 2.8.14 (#1932)

* prep for next release

* update Changes

* improve Changes formatting

* make the rest of the versions h2

* update Changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment