pires / go-proxyproto

A Go library implementation of the PROXY protocol, versions 1 and 2.
Apache License 2.0
478 stars 107 forks source link

Protocol: Keep listener listening if we don't trust the upstream address #110

Open peteski22 opened 2 months ago

peteski22 commented 2 months ago

This PR is designed to prevent listeners being stopped when an error is returned, if the upstream connection address is not trusted (ErrInvalidUpstream). Instead, we continue to close the connection but now the Accept method has a for loop to continue looking for other connections to accept.

In using this library we discovered that the listener Accept method returning an error caused the listener to be closed and never reopened when trying to serve HTTP endpoints.

The change was based on github.com/armon/go-proxyproto/ which does something similar with the loop and checking for a particular type of error.

Notes:

See: net/http/server.go => Serve(l net.Listener)

https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=3333-3351

rw, err := l.Accept()
if err != nil {
    if srv.shuttingDown() {
        return ErrServerClosed
    }
    if ne, ok := err.(net.Error); ok && ne.Temporary() {
        if tempDelay == 0 {
            tempDelay = 5 * time.Millisecond
        } else {
            tempDelay *= 2
        }
        if max := 1 * time.Second; tempDelay > max {
            tempDelay = max
        }
        srv.logf("http: Accept error: %v; retrying in %v", err, tempDelay)
        time.Sleep(tempDelay)
        continue
    }
    return err
}

Above we end up returning the err at the end, which stops the Serve method (so prevents us listening).

pires commented 2 months ago

Can you, please, rebase?

peteski22 commented 2 months ago

Can you, please, rebase?

@pires done now, thanks.

coveralls commented 2 months ago

Coverage Status

coverage: 94.924% (-0.2%) from 95.119% when pulling 956f8fea1341c4f082a21d226d434a5e48999340 on peteski22:peteski22/dont-error-listener-on-invalid-upstream into b718e7ce4964b060fd3ccc090b58cb74e1857c32 on pires:main.

camaeel commented 1 month ago

@pires any chance for merging this one?

pires commented 4 weeks ago

Sorry for the delay. Can we have some tests proving the changes work as expected?

peteski22 commented 3 days ago

@pires no worries, I've added a test to show the listener stays open when experiencing the invalid upstream error but closes on others.