drdaeman commented 1 year ago

Hey, first of all, once again thank you for this project! I need to put maddy behind a reverse proxy (Traefik, but shouldn't really matter), but it needs to be aware about the original IP address, so I want it to support PROXY protocol.

What do you think about this implementation? I haven't fully battle-tested it yet, but it seem to work and to best of my awareness it should resolve two out of three remaining point of #369. Of course, I want this to be accepted, so I'm willing to make this PR as good as possible (if needed) and would appreciate any feedback.

Two potential issues I see are related to tests:

First is that I've (ab)used SPF to test that the proxied IP address is recognized, as I'm not aware of any way to get it back. So PROXY tests assume that SPF is working correctly, which isn't exactly great. Your advice on this is most appreciated - maybe there is some debug command that I can issue to get test SMTP server to echo who it thinks is connecting?
Another issue is that I cannot get tests to pass reliably. Even running one single test (./run.sh -run TestProxyProtocolTrusted) is flaky and half of the time results with a nondescript failure (all logs seem to be fine but it ends with "t.go:294: The server did not stop cleanly, deadlock?"). And when running multiple tests all together it seems that there's some interference between them and that trusted address list from one TestProxyProtocol function can somehow leak into another. I haven't investigated this further as it seems to be a different issue and could be just something about my local Go installation.

Thanks!

drdaeman commented 1 year ago

I've battle-tested this on my own SMTP service running latest Maddy with my patches included for the last week, using proxy_protocol behind Traefik - and it seem to work without issues. All the headers and logs are spot on, no 192.168s. I haven't production-tested IMAP - I run Dovecot (because of dsync) - but I don't suppose it should be any different.

codecov-commenter commented 1 year ago

Codecov Report

Merging #568 (3910d91) into master (96fce6b) will increase coverage by 0.03%. The diff coverage is 60.65%.

@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
+ Coverage   52.76%   52.79%   +0.03%     
==========================================
  Files         127      128       +1     
  Lines       13040    13101      +61     
==========================================
+ Hits         6880     6917      +37     
- Misses       5514     5532      +18     
- Partials      646      652       +6

Flag	Coverage Δ
integration	`34.02% <60.65%> (+0.14%)`	:arrow_up:
unit	`54.42% <25.00%> (-0.02%)`	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/endpoint/imap/imap.go	`60.75% <25.00%> (-0.93%)`	:arrow_down:
internal/proxy_protocol/proxy_protocol.go	`60.37% <60.37%> (ø)`
internal/endpoint/smtp/smtp.go	`67.35% <100.00%> (+0.45%)`	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

drdaeman commented 1 year ago

Hey, sorry for a long time to respond - had a few things going on in my life that took all of my time. My bad.

I've rebased my branch and updated the tests, using the recommended 127.0.0.17 address - good consideration, thank you.

I gave it some thought and I'm a bit reluctant to remove the tls directive from proxy_protocol. Here's my logic:

Normally, PROXY protocol is used between a load balancer (LB) and individual server (Maddy), and those are typically connected by a trusted network - so, usually one wouldn't use TLS on top of the LB-to-Maddy connection.

LB ---PROXY[TLS[IMAP]]----> Maddy

Here, global or IMAP (or SMTP)-level TLS would secure the communications inside the proxied connection, and the connection header would be sent as plaintext. Making proxy_protocol's tls inherit the configuration from the top-level tls directive would result in odd behavior, where TLS is applied on top of a wrong layer.
You are absolutely correct that if we'd just remove tls from proxy_protocol entirely, the issue would be moot.
However, I believe there is a legit use case where having tls would be useful. In case the connection between the LB and Maddy is not trusted (e.g. LB is CloudFlare-like service and the connection is over Internet, not some private LAN/VPC), one may want to put TLS on top of PROXY protocol, so no one in position to observe or mess with the traffic between the LB and Maddy would be able to learn or do anything harmful:

LB ---TLS[PROXY[IMAP]]----> Maddy

or possibly even

LB ---TLS_private[PROXY[TLS_public[IMAP]]]----> Maddy

Which is why I'd suggest to keep the directive available, but required to be configured very explicitly, without any inheritance (I've amended docs slightly to be clear about this). It's a low-maintenance thing after all, because all the primitives (tls2.TLSDirective and tls.NewListener) needed to support it are already there.

I admit, in such scenarios, one would most likely want to also have TLS client certificates for mutual authentication (so Maddy can authenticate the LB as well, possibly relying on PKI trust rather than IP whitelist), but I'd say that's some extra work that is kind of out-of-scope here for the base PROXY protocol support and that it could be done separately at some later point.

I hope this makes sense. If it doesn't, or you feel like that's not a scenario you want to support - let me know and I'll just nuke tls directive from there.

Again, sorry for a long silence and thanks!

foxcpp commented 1 year ago

Thanks for your work!

foxcpp / maddy

Proxy protocol support for SMTP and IMAP #568

Codecov Report