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

tcp+sni route with allow=ip:something does not seem to work #576

Closed
KEZHwMlXV1vFzs6QvY8v5WjX5 opened this issue Nov 26, 2018 · 9 comments
Closed

Comments

@KEZHwMlXV1vFzs6QvY8v5WjX5
Copy link

KEZHwMlXV1vFzs6QvY8v5WjX5 commented Nov 26, 2018

I added routes like this:

Then from a host that is not in 10.0.0.0/8 I try a curl like this:

curl -v -k --resolve from.intern.net:443:$OUTEXTERNALIP https://from.intern.net

And fabio is happy to serve the request instead of denying it. Is this not compatible using tcp+sni? I thought that

https://github.com/fabiolb/fabio/blob/master/proxy/tcp/sni_proxy.go#L102

handled that?

@aaronhurt
Copy link
Member

aaronhurt commented Nov 26, 2018

I’ll take a look today. Thank you for the report.

@aaronhurt
Copy link
Member

@KEZHwMlXV1vFzs6QvY8v5WjX5 I can't reproduce the issue locally. I've created a branch with increased DEBUG level logging. Could you build that branch and report the results?

@aaronhurt
Copy link
Member

The branch is here: https://github.com/fabiolb/fabio/tree/issue-576-ip-access

You should be able to checkout and make to build the binary. If you're on a mac and deploying to linux you will need to run make linux instead.

@KEZHwMlXV1vFzs6QvY8v5WjX5
Copy link
Author

KEZHwMlXV1vFzs6QvY8v5WjX5 commented Nov 27, 2018

yep it does not even arrive at that part of the code. From what I can see it already stops here

return false

and in my case IP is nil and the allow opts is correctly set in the map

ip is already nil within AccessDeniedTCP while c.RemoteAddr().String() in that function is the public client IP:SourcePort

is it okay to feed https://golang.org/pkg/net/#ParseIP with an IP:PORT combo instead of only an IP?

-> no it's not ok. We might want something like this (found at https://stackoverflow.com/a/41602018)

if addr, ok := conn.RemoteAddr().(*net.TCPAddr); ok {
    fmt.Println(addr.IP.String())
}

@KEZHwMlXV1vFzs6QvY8v5WjX5
Copy link
Author

KEZHwMlXV1vFzs6QvY8v5WjX5 commented Nov 27, 2018

ok when I change the function to this it works for me.

diff --git a/route/access_rules.go b/route/access_rules.go
index 3901042..b3ce639 100644
--- a/route/access_rules.go
+++ b/route/access_rules.go
@@ -64,7 +64,8 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool {
 
 // AccessDeniedTCP checks rules on the target for TCP proxy routes.
 func (t *Target) AccessDeniedTCP(c net.Conn) bool {
-       ip := net.ParseIP(c.RemoteAddr().String())
+        addr := c.RemoteAddr().(*net.TCPAddr)
+        ip := net.ParseIP(addr.IP.String())
        if t.denyByIP(ip) {
                return true
        }

@aaronhurt
Copy link
Member

@KEZHwMlXV1vFzs6QvY8v5WjX5 Nice catch. Doing the assertion to a net.TCPAddr pointer and checking the assertion is the correct.

@aaronhurt
Copy link
Member

I pushed to the same branch and created #577
Let me know if that corrects the issue for you.

@KEZHwMlXV1vFzs6QvY8v5WjX5
Copy link
Author

positive. It passed my checks.

@aaronhurt
Copy link
Member

@KEZHwMlXV1vFzs6QvY8v5WjX5 awesome, I'll merge it. Thanks again.

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

No branches or pull requests

2 participants