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

Regression: PROXY protocol line sent only when first data arrives from client #212

Open
thegcat opened this issue Jul 7, 2024 · 7 comments

Comments

@thegcat
Copy link

thegcat commented Jul 7, 2024

We have noticed a regression between 145ec36
 and ca3e2f3 when using proxy_protocol.

In 145ec36 the PROXY protocol line would be sent directly on connection from Caddy to the backend. In ca3e2f3 it seems the PROXY protocol line is sent to the backend only when the client connected to Caddy sends data.

This is problematic with Exim at least, as the SMTP protocol with PROXY (or at least the EXIM implementation of it) expects the mail server/backend to first receive the PROXY line, then answers with the version and host information of the mail server/backend, and then expects a HELO, EHLO or the like. Sending the PROXY line with the first line the client sends breaks this dance.

@mholt
Copy link
Owner

mholt commented Jul 7, 2024

Ah, sorry about that. We are refactoring the matching/peeking logic and it is still undergoing some improvements. @ydylla amd @WeidiDeng are working on it and I am behind on reviewing their patches 😅

@thegcat
Copy link
Author

thegcat commented Jul 7, 2024

No worries, we reverted to 145ec36 after we understood the problem, this was more a heads-up for you if this was unintended and maybe for other users if they also encounter a similar issue.

@ydylla
Copy link
Collaborator

ydylla commented Jul 8, 2024

Hi,
I don't think this has something to do with changes in the proxy_protocol matcher/handler, because there are none.
Your config likely already fails in the matching phase because the new matching behavior is not compatible with connections where the server has to speak first (and the client sends nothing). I did not consider this case during the rewrite. But without data to match on the use of caddy-l4 is also rather limited.
Can you please post the config you are using?

To fix this for at least some simple configs, we could likely add a special case that detects if the config has only one possible route (without matchers) and then don't call prefetch here. Which is what probably blocks your connections currently until the matching timeout is reached and matching is aborted.

@KlettIT
Copy link

KlettIT commented Aug 5, 2024

We have a similar problem. Also when trying to proxy smtp traffic. Our config looks like this:

    "layer4": {
      "servers": {
        "mail_clearswift": {
          "listen": ["192.168.178.10:25"],
          "routes": [
            {
              "handle": [
                {
                  "handler": "proxy",
                  "proxy_protocol": "v1",
                  "load_balancing": {
                    "selection": { "policy": "ip_hash" }
                  },
                  "upstreams": [
                    {
                      "dial": ["mail01:10125"]
                    },
                    {
                      "dial": ["mail02:10125"]
                    }
                  ]
                }
              ]
            }
          ]
        }
}
}

@ydylla: as we do not use any matcher here..any idea what this causes?

@ydylla
Copy link
Collaborator

ydylla commented Aug 6, 2024

@thegcat & @KlettIT the lazy-prefetch branch from #229 contains a fix for this issue.

@WeidiDeng
Copy link
Contributor

@thegcat @KlettIT FIY, it's also fixed here, build caddy by xcaddy build --with github.com/mholt/caddy-l4=github.com/WeidiDeng/caddy-l4@routes-fixes

@KlettIT
Copy link

KlettIT commented Aug 7, 2024

@WeidiDeng ah okay, thanks!

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 a pull request may close this issue.

5 participants