address review comments

fabiolb · Feb 18, 2018 · 58c2d82 · 58c2d82
1 parent 57dd6e5
commit 58c2d82
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 31 deletions.
diff --git a/docs/content/cfg/_index.md b/docs/content/cfg/_index.md
@@ -26,8 +26,8 @@ Add a route for a service `svc` for the `src` (e.g. `/path` or `:port`) to a `ds
 
 Option                                     | Description
 ------------------------------------------ | -----------
-`allow=ip:10.0.0.0/8,ip:192.168.0.0/24`    | Restrict access to source addresses within the `10.0.0.0/8` or `192.168.0.0/24` CIDR mask.  All other requests will be denied.
-`deny=ip:10.0.0.0/8,ip:1.2.3.4/32`         | Deny requests that source from the `10.0.0.0/8` CIDR mask or `1.2.3.4`.  All other requests will be allowed.
+`allow=ip:10.0.0.0/8,ip:fe80::/10`         | Restrict access to source addresses within the `10.0.0.0/8` or `fe80::/10` CIDR mask.  All other requests will be denied.
+`deny=ip:10.0.0.0/8,ip:fe80::1234`         | Deny requests that source from the `10.0.0.0/8` CIDR mask or `fe80::1234`.  All other requests will be allowed.
 `strip=/path`                              | Forward `/path/to/file` as `/to/file`
 `proto=tcp`                                | Upstream service is TCP, `dst` must be `:port`
 `proto=https`                              | Upstream service is HTTPS

diff --git a/docs/content/feature/access-control.md b/docs/content/feature/access-control.md
@@ -10,22 +10,28 @@ Currently only source ip control is available.
 <!--more-->
 
 To allow access to a route from clients within the `192.168.1.0/24`
-and `10.0.0.0/8` subnet you would add the following option:
+and `fe80::/10` subnet you would add the following option:
 
 ```
-allow=ip:192.168.1.0/24,ip:10.0.0.0/8
+allow=ip:192.168.1.0/24,ip:fe80::/10
 ```
 
 With this specified only clients sourced from those two subnets will
 be allowed.  All other requests to that route will be denied.
 
 
-Inversely, to only deny a specific set of clients you can use the
+Inversely, to deny a specific set of clients you can use the
 following option syntax:
 
 ```
-deny=ip:1.2.3.4/32,100.123.0.0/16
+deny=ip:fe80::1234,100.123.0.0/16
 ```
 
 With this configuration access will be denied to any clients with
-the `1.2.3.4` address or coming from the `100.123.0.0/16` network.
+the `fe80::1234` address or coming from the `100.123.0.0/16` network.
+
+Single host addresses (addresses without a prefix) will have a
+`/32` prefix, for IPv4, or a `/128` prefix, for IPv6, added automatically.
+That means `1.2.3.4` is equivalent to `1.2.3.4/32` and `fe80::1234`
+is equivalent to `fe80::1234/128` when specifying
+address blocks for `allow` or `deny` rules.
diff --git a/proxy/http_integration_test.go b/proxy/http_integration_test.go
@@ -115,6 +115,36 @@ func TestProxySTSHeader(t *testing.T) {
 	}
 }
 
+func TestProxyChecksHeaderForAccessRules(t *testing.T) {
+	var hdr http.Header = make(http.Header)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		hdr = r.Header
+	}))
+	defer server.Close()
+
+	proxy := httptest.NewServer(&HTTPProxy{
+		Config:    config.Proxy{},
+		Transport: http.DefaultTransport,
+		Lookup: func(r *http.Request) *route.Target {
+			tgt := &route.Target{
+				URL:  mustParse(server.URL),
+				Opts: map[string]string{"allow": "ip:127.0.0.0/8,ip:fe80::/10,ip:::1"},
+			}
+			tgt.ProcessAccessRules()
+			return tgt
+		},
+	})
+	defer proxy.Close()
+
+	req, _ := http.NewRequest("GET", proxy.URL, nil)
+	req.Header.Set("X-Forwarded-For", "1.2.3.4")
+	resp, _ := mustDo(req)
+
+	if got, want := resp.StatusCode, http.StatusForbidden; got != want {
+		t.Errorf("got %v want %v", got, want)
+	}
+}
+
 func TestProxyNoRouteHTML(t *testing.T) {
 	want := "<html>503</html>"
 	noroute.SetHTML(want)

diff --git a/proxy/tcp/sni_proxy.go b/proxy/tcp/sni_proxy.go
@@ -79,7 +79,6 @@ func (p *SNIProxy) ServeTCP(in net.Conn) error {
 	addr := t.URL.Host
 
 	if t.AccessDeniedTCP(in) {
-		log.Print("[INFO] route rules denied access to ", t.URL.String())
 		return nil
 	}
 

diff --git a/proxy/tcp/tcp_proxy.go b/proxy/tcp/tcp_proxy.go
@@ -49,7 +49,6 @@ func (p *Proxy) ServeTCP(in net.Conn) error {
 	addr := t.URL.Host
 
 	if t.AccessDeniedTCP(in) {
-		log.Print("[INFO] route rules denied access to ", t.URL.String())
 		return nil
 	}
 

diff --git a/route/access_rules.go b/route/access_rules.go
@@ -30,7 +30,7 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool {
 	}
 
 	// check remote source and return if denied
-	if ip != nil && t.denyByIP(ip) {
+	if t.denyByIP(ip) {
 		return true
 	}
 
@@ -43,7 +43,8 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool {
 			if ip = net.ParseIP(xff); ip == nil {
 				log.Printf("[WARN] failed to parse xff address %s", xff)
 			}
-			if ip != nil && t.denyByIP(ip) {
+			// check xff source and return if denied
+			if t.denyByIP(ip) {
 				return true
 			}
 		}
@@ -55,8 +56,12 @@ 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 {
-	// currently only one function - more may be added in the future
-	return t.denyByIP(net.ParseIP(c.RemoteAddr().String()))
+	ip := net.ParseIP(c.RemoteAddr().String())
+	if t.denyByIP(ip) {
+		return true
+	}
+	// default allow
+	return false
 }
 
 func (t *Target) denyByIP(ip net.IP) bool {
@@ -78,6 +83,8 @@ func (t *Target) denyByIP(ip net.IP) bool {
 			}
 		}
 		// we checked all the blocks - deny this request
+		log.Printf("[INFO] route rules denied access from %s to %s",
+			ip.String(), t.URL.String())
 		return true
 	}
 
@@ -91,6 +98,8 @@ func (t *Target) denyByIP(ip net.IP) bool {
 			}
 			if block.Contains(ip) {
 				// specific deny matched - deny this request
+				log.Printf("[INFO] route rules denied access from %s to %s",
+					ip.String(), t.URL.String())
 				return true
 			}
 		}
@@ -100,6 +109,22 @@ func (t *Target) denyByIP(ip net.IP) bool {
 	return false
 }
 
+// ProcessAccessRules processes access rules from options specified on the target route
+func (t *Target) ProcessAccessRules() error {
+	if t.Opts["allow"] != "" && t.Opts["deny"] != "" {
+		return errors.New("specifying allow and deny on the same route is not supported")
+	}
+
+	for _, allowDeny := range []string{"allow", "deny"} {
+		if t.Opts[allowDeny] != "" {
+			if err := t.parseAccessRule(allowDeny); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
 func (t *Target) parseAccessRule(allowDeny string) error {
 	var accessTag string
 	var temps []string
@@ -147,18 +172,3 @@ func (t *Target) parseAccessRule(allowDeny string) error {
 
 	return nil
 }
-
-func (t *Target) processAccessRules() error {
-	if t.Opts["allow"] != "" && t.Opts["deny"] != "" {
-		return errors.New("specifying allow and deny on the same route is not supported")
-	}
-
-	for _, allowDeny := range []string{"allow", "deny"} {
-		if t.Opts[allowDeny] != "" {
-			if err := t.parseAccessRule(allowDeny); err != nil {
-				return err
-			}
-		}
-	}
-	return nil
-}
diff --git a/route/access_rules_test.go b/route/access_rules_test.go
@@ -3,6 +3,7 @@ package route
 import (
 	"net"
 	"net/http"
+	"net/url"
 	"testing"
 )
 
@@ -140,10 +141,11 @@ func TestAccessRules_denyByIP(t *testing.T) {
 		tt := tt // capture loop var
 
 		t.Run(tt.desc, func(t *testing.T) {
-			if err := tt.target.processAccessRules(); err != nil {
+			if err := tt.target.ProcessAccessRules(); err != nil {
 				t.Errorf("%d: %s - failed to process access rules: %s",
 					i, tt.desc, err.Error())
 			}
+			tt.target.URL, _ = url.Parse("http://testing.test/")
 			if deny := tt.target.denyByIP(tt.remote); deny != tt.denied {
 				t.Errorf("%d: %s\ngot denied: %t\nwant denied: %t\n",
 					i, tt.desc, deny, tt.denied)
@@ -207,10 +209,11 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) {
 		req.RemoteAddr = tt.remote
 
 		t.Run(tt.desc, func(t *testing.T) {
-			if err := tt.target.processAccessRules(); err != nil {
+			if err := tt.target.ProcessAccessRules(); err != nil {
 				t.Errorf("%d: %s - failed to process access rules: %s",
 					i, tt.desc, err.Error())
 			}
+			tt.target.URL, _ = url.Parse("http://testing.test/")
 			if deny := tt.target.AccessDeniedHTTP(req); deny != tt.denied {
 				t.Errorf("%d: %s\ngot denied: %t\nwant denied: %t\n",
 					i, tt.desc, deny, tt.denied)

diff --git a/route/route.go b/route/route.go
@@ -81,7 +81,7 @@ func (r *Route) addTarget(service string, targetURL *url.URL, fixedWeight float6
 			}
 		}
 
-		if err = t.processAccessRules(); err != nil {
+		if err = t.ProcessAccessRules(); err != nil {
 			log.Printf("[ERROR] failed to process access rules: %s",
 				err.Error())
 		}