TCP proxy broken since v1.5.8 #524
Comments
|
@sugdyzhekov Interesting, could you attach a tcpdump in addition to the nc output? |
|
this is the sni proxy. sorry |
|
Just discovered this issue too (I assume it's the same). We noticed because our SMTP server seemed to be unresponsive. If the server starts the conversation after a client connects (as opposed to the client sending the first message, which works fine), fabio doesn't send data to the client. Both SMTP and MySQL servers start a connection by sending a banner first. Here is my reproducer: // smtp_mock.go
package main
import (
"fmt"
"log"
"net"
)
func main() {
log.Println("Listening on :8025")
l, err := net.Listen("tcp", ":8025")
if err != nil {
log.Fatal(err)
}
for {
conn, err := l.Accept()
if err != nil {
log.Fatal(err)
}
log.Println("serving", conn.RemoteAddr())
fmt.Fprintf(conn, "220 localhost ESMTP Go\r\n")
conn.Close()
}
}Next shell: Next shell: There's nothing interesting in fabio's logs, not even at TRACE level. Git bisect points fingers at a28d244: |
|
Here's a tcpdump. Note the two second gap after the third packet. That's where telnet was connected to fabio, but I didn't do anything. Then I hit enter, and then fabio established a connection to the backend. |
|
Interesting and thank you for detailed troubleshooting. That makes a lot of sense. We use TCP proxy but haven't noticed any breakage but in our case it is all client initiated. |
|
... and it seems I may be to blame :) @pschultz could you try reproducing this with master? I just made some changes to the ip access control that might be the solution. In short that section wasn't properly getting the remote connection address previously. |
|
digging into this now ... using your steps I can reproduce it on master |
|
It is indeed the AccessDenied check but I'm not yet sure why. Will continue to investigate but commenting out the relevant lines in |
|
I found the issue but I'm going to need to do a bit of research ... steps so far: Using the same test scenario as above with these code changes in // AccessDeniedTCP checks rules on the target for TCP proxy routes.
func (t *Target) AccessDeniedTCP(c net.Conn) bool {
log.Printf("[DEBUG] in AccessDeniedTCP 0")
var addr *net.TCPAddr
var ok bool
// validate remote address assertion
if addr, ok = c.RemoteAddr().(*net.TCPAddr); !ok {
log.Printf("[ERROR] failed to assert remote connection address for %s", t.Service)
return false
}
log.Printf("[DEBUG] in AccessDeniedTCP 1 addr=%+v", addr)
// check remote connection address
if t.denyByIP(addr.IP) {
return true
}
log.Printf("[DEBUG] in AccessDeniedTCP 2")
// default allow
return false
}Watching the fabio logs when telnetting to port 8026 from a remote host on the LAN ... The |
|
This looks potentially related: golang/go#12943 |
|
This as well: armon/go-proxyproto#1 |
|
The issue seems to be in the proxy protocol implementation ... // RemoteAddr returns the address of the client if the proxy
// protocol is being used, otherwise just returns the address of
// the socket peer. If there is an error parsing the header, the
// address of the client is not returned, and the socket is closed.
// Once implication of this is that the call could block if the
// client is slow. Using a Deadline is recommended if this is called
// before Read()
func (p *Conn) RemoteAddr() net.Addr {
p.once.Do(func() {
if err := p.checkPrefix(); err != nil && err != io.EOF {
log.Printf("[ERR] Failed to read proxy prefix: %v", err)
}
})
if p.srcAddr != nil {
return p.srcAddr
}
return p.conn.RemoteAddr()
}We would either have to disable proxy protocol or get the remote IP without checking for proxy protocol for server initiated requests. |
|
Commenting out the proxy protocol wrapper allows the test to proceed. With Logging output from a telnet check against the mock server from a remote host on the LAN: How do we want to proceed? |
|
My initial thoughts:
I think making people explicitly enable proxy protocol is also better from a security perspective. |
|
So this is broken for TCP proxies which do not use the PROXY protocol and where the client is waiting for an initial handshake from the server which never comes b/c fabio does not forward the connection (i.e. MySQL)? And this started to become an issue when we added access control since we're looking at the Making the PROXY protocol configurable is the better solution, IMO. (I should have done this when I added that.) People know when they are using it, one would assume. The question is what to do with the default. We can have Who is affected if we switch the default to The 1.5.8 behavior was that TCP just worked and I'd prefer if we can get back to that state. Maybe we also add an However, keeping it simple also has advantages. If we decided to switch to
|
|
I'll see if I can put together a PoC for automatic detection. That being said, if there is a toggle for PROXY Protocol this enables another use-case: checking that a connection is from a particular proxy (as opposed to a particular client downstream). Not sure how common that is, or if it's ever been asked for, though. |
|
It would be good if the backend could decide if it wants to be enable PROXY or not. I dunno if thats doable (single listener on Fabio sends PROXY to one backend but not PROXY to another backend) |
|
armon/go-proxyproto#4 adds a After some superficial testing, updating goproxy to at least 49fdb5c and exposing that option seems to be a good compromise. We can have it default to something sensible (say, 5ms), and we can interpret zero or a negative value as "disable PROXY protocol support". In addition we shouldn't call This will make fabio work for MySQL in the default configuration, costing a slight increase in connect latency, for exactly those users that use access rules and no PROXY protocol. There is also a chance of subtle breakage by choosing too small a timeout. |
|
If you want to play around, I just pushed to https://github.com/classmarkets/fabio/tree/fix-tcp (classmarkets/fabio@7246b92) |
|
I made a few commits to a new branch referenced above that address the core points mentioned thus far. The current proxy-protocol toggle defaults to false but that may need to change per @magiconair comment's above. Regardless of the default setting I believe additional documentation and release notes will be warranted. |
|
@pschultz I've invited you to be a collaborator so that we can work on the same branches. |
Co-authored-by: Peter Schultz <peter.schultz@classmarkets.com>
address concerns raised while troubleshooting #524
|
Please note that this change disabled PROXY protocol by default and you have to enable it if you need it. If you feel this is a grave mistake, the please let us know. (I'll mull over this a bit more before creating a new release). |
This fixes a mistake made in #524 that put the deny check inside the assertion error block. This effectively disabled checking of access rules for TCP proxy requests.
* update documentation around the changes to PROXY protocol * be consistent with accessRules check * fix mistake made in #524 that put the deny check inside the assertion error block
@magiconair, thank you very much. Would you also add my SSH key, please? |
|
@pschultz I've enabled your account to be a collaborator with write permissions. That should be sufficient. If you want to use a deploy key instead then it has to be unique for this repo since you cannot use the same key for multiple repos. Trying to add this key returns "key already in use". |
|
When will this change be released? Can you make 1.5.11? |
|
I'll try to release 1.5.11 tonight CET time. |
Impact
It's not possible to connect to MySQL since v1.5.8
STR
Expected result
mysql client can establish connection to MySQL service
Actual result
mysql client hangs during connection establishment.
Additional info
v1.5.7
v1.5.8
Configuration
proxy_addr
mysql service register at Consul with following tag:
urlprefix-:3306 proto=tcpThe text was updated successfully, but these errors were encountered: