Skip to content

Conversation

Spiral90210
Copy link

I have need of support, and would like to contribute back if it fits in. XFORWARD is not an RFC standard (https://www.postfix.org/XFORWARD_README.html) , but it's used in postfix in more complicated proxy setups and content filters, and is much more reliable than trying to partially read the DATA of a message to parse a Received header (also, I dont like adding those for intermediate servers). I've tried to match existing style as much as I could.

The downside? Adding the function to the session interface breaks existing code, requiring at minimum a noop function in consumers. I didn't see an easy way around this and it's very easy to add to release notes, but it is a breaking change to existing code, which is never nice.

There's probably some more room for improvement (e.g. the permitted attributes are not an exhaustive list here), but wanted to submit this as an early PR to gauge appetite for it - I've tested it as working in my application, and would be perfectly happy to merge as-is. Here are some examples from my system logging (everything on a local system):

HAVE XFORWARD NAME:[UNAVAILABLE]
HAVE XFORWARD ADDR:172.18.0.1
HAVE XFORWARD HELO:localhost.dev.mydomain.com
HAVE XFORWARD PROTO:ESMTP

@Spiral90210
Copy link
Author

Another potential issue (which doesn't apply to my specific use case but may apply here) is this:

Security

The XFORWARD command changes audit trails. Use of this command must be restricted to authorized clients. 

I have added no such restriction, and it may be prudent to dissuade use of this as it currently is if a server is exposed directly to the internet (mine is not - I route via postfix). I could add network permit-list style config or such, but if it's a no go from the project POV then I won't bother.

@Spiral90210
Copy link
Author

And of course I thought I had the tests passing but missed an implementation in a test class 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant