search

Fruneau / pfixtools

The pfixtools project is a collection of postfix-related tools. The pfixtools are written in C.
Other
26 stars 7 forks source link

Docs update and decoding adjustment #8

Closed driskell closed 10 years ago

driskell commented 10 years ago

Hi Fruneau,

I've adjusted the docs slightly to fix links and add references to the socketmap protocol capability.

More importantly, I found an issue with the use of canonical to decode. When ignoring external domains, it would return 200/OK with the same address as given, if the domain was external. However, if it was internal, and didn't need decoding, it would return 400/NOT FOUND Not an SRS address (due to the error return from srs_decode).

However, it seems that if a canonical rewrite returns OK, postfix will bypass local recipient checks. This means if someone tries to send to an address that would normally be rejected as "not in virtual mailbox table" for example, and it was considered external domain, we'd return an OK with the same address and cause that check to be bypassed. This meant the email would be accepted, and a bounce report generated.

So I've adjusted the decoder to always return NOT FOUND unless it did in fact decode something.

Thanks,

Jason

driskell commented 10 years ago

Hi Fruneau,

Thanks for reviewing. However, I disagree on having an option as it does not provide a feature or change intended behaviour or what is documented.

The intention was just to fix what I consider a bug and unintended behaviour. From an end users point of view I would say that returning 400, or 200 with the email unchanged, makes no difference. However returning 200 does cause postfix to bypass recipient checks as a result. This I see as unexpected behaviour and cannot think of any reason a user would want this. And if they did I feel pfixtools is certainly not where they should be choosing this as it's not documented or within the project's scope (as I see it of course!)

If you are still keen to have an option I don't mind someone else dropping it in to allow this to merge.

Good work on the tools and I hope you don't mind the disagree :-) we can just leave as is if you prefer. But as I said I do feel strongly this is a bug fix and not a feature.

Thanks and again I appreciate all your time reviewing!

Jason

Fruneau commented 10 years ago

OK. No problem with the disagreement, as long as it ends in a constructive discussion :).

I'll check with other users that the patch does not seem to break there use cases.