From a28d24440a78c441e189e90d8671332122fea90f Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Mon, 12 Feb 2018 21:24:00 -0600 Subject: [PATCH 01/10] add basic ip centric access control on routes --- docs/content/cfg/_index.md | 18 ++-- docs/content/feature/_index.md | 1 + docs/content/feature/access-control.md | 31 ++++++ proxy/http_proxy.go | 5 + proxy/tcp/sni_proxy.go | 5 + proxy/tcp/tcp_proxy.go | 5 + route/access_rules.go | 127 +++++++++++++++++++++++++ route/access_rules_test.go | 107 +++++++++++++++++++++ route/route.go | 6 ++ route/target.go | 3 + 10 files changed, 300 insertions(+), 8 deletions(-) create mode 100644 docs/content/feature/access-control.md create mode 100644 route/access_rules.go create mode 100644 route/access_rules_test.go diff --git a/docs/content/cfg/_index.md b/docs/content/cfg/_index.md index cbcd7c416..6bcfa9c53 100644 --- a/docs/content/cfg/_index.md +++ b/docs/content/cfg/_index.md @@ -24,14 +24,16 @@ Add a route for a service `svc` for the `src` (e.g. `/path` or `:port`) to a `ds `route add [ weight ][ tags ",,..."][ opts "k1=v1 k2=v2 ..."]` -Option | Description --------------------- | ----------- -`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 -`tlsskipverify=true` | Disable TLS cert validation for HTTPS upstream -`host=name` | Set the `Host` header to `name`. If `name == 'dst'` then the `Host` header will be set to the registered upstream host name -`register=name` | Register fabio as new service `name`. Useful for registering hostnames for host specific routes. +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. +`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 +`tlsskipverify=true` | Disable TLS cert validation for HTTPS upstream +`host=name` | Set the `Host` header to `name`. If `name == 'dst'` then the `Host` header will be set to the registered upstream host name +`register=name` | Register fabio as new service `name`. Useful for registering hostnames for host specific routes. ##### Example diff --git a/docs/content/feature/_index.md b/docs/content/feature/_index.md index 588074fa6..0a6a28984 100644 --- a/docs/content/feature/_index.md +++ b/docs/content/feature/_index.md @@ -6,6 +6,7 @@ weight: 200 The following list provides a list of features supported by fabio. * [Access Logging](/feature/access-logging/) - customizable access logs + * [Access Control](/feature/access-control/) - route specific access control * [Certificate Stores](/feature/certificate-stores/) - dynamic certificate stores like file system, HTTP server, [Consul](https://consul.io/) and [Vault](https://vaultproject.io/) * [Compression](/feature/compression/) - GZIP compression for HTTP responses * [Docker Support](/feature/docker/) - Official Docker image, Registrator and Docker Compose example diff --git a/docs/content/feature/access-control.md b/docs/content/feature/access-control.md new file mode 100644 index 000000000..5369af3df --- /dev/null +++ b/docs/content/feature/access-control.md @@ -0,0 +1,31 @@ +--- +title: "Access Control" +since: "1.5.8" +--- + +fabio supports basic ip centric access control per route. You may +specify one of `allow` or `deny` options per route to control access. +Currently only source ip control is available. + + + +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: + +``` +allow=ip:192.168.1.0/24,ip:10.0.0.0/8 +``` + +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 +following option syntax: + +``` +deny=ip:1.2.3.4/32,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. diff --git a/proxy/http_proxy.go b/proxy/http_proxy.go index ba505fc97..0898d7afe 100644 --- a/proxy/http_proxy.go +++ b/proxy/http_proxy.go @@ -84,6 +84,11 @@ func (p *HTTPProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + if t.AccessDeniedHTTP(r) { + http.Error(w, "access denied", http.StatusForbidden) + return + } + // build the request url since r.URL will get modified // by the reverse proxy and contains only the RequestURI anyway requestURL := &url.URL{ diff --git a/proxy/tcp/sni_proxy.go b/proxy/tcp/sni_proxy.go index b75d06705..b1361253b 100644 --- a/proxy/tcp/sni_proxy.go +++ b/proxy/tcp/sni_proxy.go @@ -78,6 +78,11 @@ 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 + } + out, err := net.DialTimeout("tcp", addr, p.DialTimeout) if err != nil { log.Print("[WARN] tcp+sni: cannot connect to upstream ", addr) diff --git a/proxy/tcp/tcp_proxy.go b/proxy/tcp/tcp_proxy.go index 5b14cc950..0551fa6f0 100644 --- a/proxy/tcp/tcp_proxy.go +++ b/proxy/tcp/tcp_proxy.go @@ -48,6 +48,11 @@ 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 + } + out, err := net.DialTimeout("tcp", addr, p.DialTimeout) if err != nil { log.Print("[WARN] tcp: cannot connect to upstream ", addr) diff --git a/route/access_rules.go b/route/access_rules.go new file mode 100644 index 000000000..fd00f2a4f --- /dev/null +++ b/route/access_rules.go @@ -0,0 +1,127 @@ +package route + +import ( + "errors" + "fmt" + "log" + "net" + "net/http" + "strings" +) + +const ( + ipAllowTag = "allow:ip" + ipDenyTag = "deny:ip" +) + +// AccessDeniedHTTP checks rules on the target for HTTP proxy routes. +func (t *Target) AccessDeniedHTTP(r *http.Request) bool { + host, _, err := net.SplitHostPort(r.RemoteAddr) + + if err != nil { + log.Printf("[ERROR] Failed to get host from RemoteAddr %s: %s", + r.RemoteAddr, err.Error()) + return false + } + + // prefer xff header if set + if xff := r.Header.Get("X-Forwarded-For"); xff != "" && xff != host { + host = xff + } + + // currently only one function - more may be added in the future + return t.denyByIP(net.ParseIP(host)) +} + +// 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())) +} + +func (t *Target) denyByIP(ip net.IP) bool { + if ip == nil || t.accessRules == nil { + return false + } + + // check allow (whitelist) first if it exists + if _, ok := t.accessRules[ipAllowTag]; ok { + var block *net.IPNet + for _, x := range t.accessRules[ipAllowTag] { + if block, ok = x.(*net.IPNet); !ok { + log.Print("[ERROR] failed to assert ip block while checking allow rule for ", t.Service) + continue + } + if block.Contains(ip) { + // specific allow matched - allow this request + return false + } + } + // we checked all the blocks - deny this request + return true + } + + // still going - check deny (blacklist) if it exists + if _, ok := t.accessRules[ipDenyTag]; ok { + var block *net.IPNet + for _, x := range t.accessRules[ipDenyTag] { + if block, ok = x.(*net.IPNet); !ok { + log.Print("[INFO] failed to assert ip block while checking deny rule for ", t.Service) + continue + } + if block.Contains(ip) { + // specific deny matched - deny this request + return true + } + } + } + + // default - do not deny + return false +} + +func (t *Target) parseAccessRule(allowDeny string) error { + var accessTag string + var temps []string + + // init rules if needed + if t.accessRules == nil { + t.accessRules = make(map[string][]interface{}) + } + + // loop over rule elements + for _, c := range strings.Split(t.Opts[allowDeny], ",") { + if temps = strings.SplitN(c, ":", 2); len(temps) != 2 { + return fmt.Errorf("invalid access item, expected :, got %s", temps) + } + accessTag = allowDeny + ":" + strings.ToLower(temps[0]) + switch accessTag { + case ipAllowTag, ipDenyTag: + _, net, err := net.ParseCIDR(strings.TrimSpace(temps[1])) + if err != nil { + return fmt.Errorf("failed to parse CIDR %s with error: %s", + c, err.Error()) + } + // add element to rule map + t.accessRules[accessTag] = append(t.accessRules[accessTag], net) + default: + return fmt.Errorf("unknown access item type: %s", temps[0]) + } + } + 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 new file mode 100644 index 000000000..ced406edb --- /dev/null +++ b/route/access_rules_test.go @@ -0,0 +1,107 @@ +package route + +import ( + "net" + "testing" +) + +func TestAccessRules_parseAccessRule(t *testing.T) { + tests := []struct { + desc string + allowDeny string + fail bool + }{ + { + desc: "parseAccessRuleGood", + allowDeny: "ip:10.0.0.0/8,ip:192.168.0.0/24,ip:1.2.3.4/32", + }, + { + desc: "parseAccessRuleBadType", + allowDeny: "x:10.0.0.0/8", + fail: true, + }, + { + desc: "parseAccessRuleIncompleteIP", + allowDeny: "ip:10/8", + fail: true, + }, + { + desc: "parseAccessRuleBadCIDR", + allowDeny: "ip:10.0.0.0/255", + fail: true, + }, + } + + for i, tt := range tests { + tt := tt // capture loop var + + t.Run(tt.desc, func(t *testing.T) { + for _, ad := range []string{"allow", "deny"} { + tgt := &Target{Opts: map[string]string{ad: tt.allowDeny}} + err := tgt.parseAccessRule(ad) + if err != nil && !tt.fail { + t.Errorf("%d: %s\nfailed: %s", i, tt.desc, err.Error()) + return + } + } + }) + } +} + +func TestAccessRules_denyByIP(t *testing.T) { + tests := []struct { + desc string + target *Target + remote net.IP + denied bool + }{ + { + desc: "denyByIPAllowAllowed", + target: &Target{ + Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + remote: net.ParseIP("10.10.0.1"), + denied: false, + }, + { + desc: "denyByIPAllowDenied", + target: &Target{ + Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + remote: net.ParseIP("1.2.3.4"), + denied: true, + }, + { + desc: "denyByIPDenyDenied", + target: &Target{ + Opts: map[string]string{"deny": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + remote: net.ParseIP("10.10.0.1"), + denied: true, + }, + { + desc: "denyByIPDenyAllow", + target: &Target{ + Opts: map[string]string{"deny": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + remote: net.ParseIP("1.2.3.4"), + denied: false, + }, + } + + for i, tt := range tests { + tt := tt // capture loop var + + t.Run(tt.desc, func(t *testing.T) { + if err := tt.target.processAccessRules(); err != nil { + t.Errorf("%d: %s - failed to process access rules: %s", + i, tt.desc, err.Error()) + } + 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) + return + } + }) + } +} diff --git a/route/route.go b/route/route.go index f0659c923..b3099cc90 100644 --- a/route/route.go +++ b/route/route.go @@ -65,6 +65,7 @@ func (r *Route) addTarget(service string, targetURL *url.URL, fixedWeight float6 Timer: ServiceRegistry.GetTimer(name), TimerName: name, } + if opts != nil { t.StripPath = opts["strip"] t.TLSSkipVerify = opts["tlsskipverify"] == "true" @@ -79,6 +80,11 @@ func (r *Route) addTarget(service string, targetURL *url.URL, fixedWeight float6 log.Printf("[ERROR] redirect status code should be in 3xx range. Got: %s", opts["redirect"]) } } + + if err = t.processAccessRules(); err != nil { + log.Printf("[ERROR] failed to process access rules: %s", + err.Error()) + } } r.Targets = append(r.Targets, t) diff --git a/route/target.go b/route/target.go index 6f5a70034..b5b63452a 100644 --- a/route/target.go +++ b/route/target.go @@ -50,6 +50,9 @@ type Target struct { // TimerName is the name of the timer in the metrics registry TimerName string + + // accessRules is map of access information for the target. + accessRules map[string][]interface{} } func (t *Target) GetRedirectURL(requestURL *url.URL) *url.URL { From d8e2b1ad75b7750a21e0dfd6ba2d383f4468c30b Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Mon, 12 Feb 2018 22:32:03 -0600 Subject: [PATCH 02/10] prevent access control bypass via xff header --- route/access_rules.go | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/route/access_rules.go b/route/access_rules.go index fd00f2a4f..65d754d0a 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -16,21 +16,41 @@ const ( // AccessDeniedHTTP checks rules on the target for HTTP proxy routes. func (t *Target) AccessDeniedHTTP(r *http.Request) bool { + var ip net.IP host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { - log.Printf("[ERROR] Failed to get host from RemoteAddr %s: %s", + log.Printf("[ERROR] failed to get host from remote header %s: %s", r.RemoteAddr, err.Error()) return false } - // prefer xff header if set - if xff := r.Header.Get("X-Forwarded-For"); xff != "" && xff != host { - host = xff + if ip = net.ParseIP(host); ip == nil { + log.Printf("[WARN] failed to parse remote address %s", host) } - // currently only one function - more may be added in the future - return t.denyByIP(net.ParseIP(host)) + // check remote source and return if denied + if ip != nil && t.denyByIP(ip) { + return true + } + + // check xff source if present + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + // only use left-most element (client) + xff = strings.TrimSpace(strings.SplitN(xff, ",", 2)[0]) + // only continue if xff differs from host + if xff != host { + if ip = net.ParseIP(xff); ip == nil { + log.Printf("[WARN] failed to parse xff address %s", xff) + } + if ip != nil && t.denyByIP(ip) { + return true + } + } + } + + // default allow + return false } // AccessDeniedTCP checks rules on the target for TCP proxy routes. From 8169a40c2c48252b3973f9f08f651890035430e1 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Mon, 12 Feb 2018 23:16:54 -0600 Subject: [PATCH 03/10] add test cases for http request parsing --- route/access_rules_test.go | 68 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/route/access_rules_test.go b/route/access_rules_test.go index ced406edb..1c9b3f4f9 100644 --- a/route/access_rules_test.go +++ b/route/access_rules_test.go @@ -2,6 +2,7 @@ package route import ( "net" + "net/http" "testing" ) @@ -105,3 +106,70 @@ func TestAccessRules_denyByIP(t *testing.T) { }) } } + +func TestAccessRules_AccessDeniedHTTP(t *testing.T) { + req, _ := http.NewRequest("GET", "http://example.com/", nil) + tests := []struct { + desc string + target *Target + xff string + remote string + denied bool + }{ + { + desc: "AccessDeniedHTTPwithDeniedXFFandAllowedRemote", + target: &Target{ + Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + xff: "1.1.1.2, 10.11.12.13, 10.11.12.14", + remote: "10.11.12.1:65500", + denied: true, + }, + { + desc: "AccessDeniedHTTPwithAllowedXFFandDeniedRemote", + target: &Target{ + Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + xff: "10.11.12.13, 1.2.3.4", + remote: "1.1.1.2:65500", + denied: true, + }, + { + desc: "AccessDeniedHTTPwitAllowedXFFandAllowedRemote", + target: &Target{ + Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + xff: "10.11.12.13, 1.2.3.4", + remote: "192.168.0.12:65500", + denied: false, + }, + { + desc: "AccessDeniedHTTPwithDeniedXFFandDeniedRemote", + target: &Target{ + Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, + }, + xff: "1.2.3.4, 10.11.12.13, 10.11.12.14", + remote: "200.17.18.20:65500", + denied: true, + }, + } + + for i, tt := range tests { + tt := tt // capture loop var + + req.Header = http.Header{"X-Forwarded-For": []string{tt.xff}} + req.RemoteAddr = tt.remote + + t.Run(tt.desc, func(t *testing.T) { + if err := tt.target.processAccessRules(); err != nil { + t.Errorf("%d: %s - failed to process access rules: %s", + i, tt.desc, err.Error()) + } + 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) + return + } + }) + } +} From c7b06ff4259ededb8230f6c25ee93ef6b0cd2e73 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Mon, 12 Feb 2018 23:23:31 -0600 Subject: [PATCH 04/10] cleanup test descriptions --- route/access_rules_test.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/route/access_rules_test.go b/route/access_rules_test.go index 1c9b3f4f9..69c5b7904 100644 --- a/route/access_rules_test.go +++ b/route/access_rules_test.go @@ -13,21 +13,21 @@ func TestAccessRules_parseAccessRule(t *testing.T) { fail bool }{ { - desc: "parseAccessRuleGood", + desc: "valid rule", allowDeny: "ip:10.0.0.0/8,ip:192.168.0.0/24,ip:1.2.3.4/32", }, { - desc: "parseAccessRuleBadType", - allowDeny: "x:10.0.0.0/8", + desc: "invalid rule type", + allowDeny: "xxx:10.0.0.0/8", fail: true, }, { - desc: "parseAccessRuleIncompleteIP", + desc: "ip rule with incomplete address", allowDeny: "ip:10/8", fail: true, }, { - desc: "parseAccessRuleBadCIDR", + desc: "ip rule with bad cidr mask", allowDeny: "ip:10.0.0.0/255", fail: true, }, @@ -57,7 +57,7 @@ func TestAccessRules_denyByIP(t *testing.T) { denied bool }{ { - desc: "denyByIPAllowAllowed", + desc: "allow rule with included ip", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -65,7 +65,7 @@ func TestAccessRules_denyByIP(t *testing.T) { denied: false, }, { - desc: "denyByIPAllowDenied", + desc: "allow rule with exluded ip", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -73,7 +73,7 @@ func TestAccessRules_denyByIP(t *testing.T) { denied: true, }, { - desc: "denyByIPDenyDenied", + desc: "deny rule with included ip", target: &Target{ Opts: map[string]string{"deny": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -81,7 +81,7 @@ func TestAccessRules_denyByIP(t *testing.T) { denied: true, }, { - desc: "denyByIPDenyAllow", + desc: "deny rule with excluded ip", target: &Target{ Opts: map[string]string{"deny": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -117,7 +117,7 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) { denied bool }{ { - desc: "AccessDeniedHTTPwithDeniedXFFandAllowedRemote", + desc: "denied xff and allowed remote addr", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -126,7 +126,7 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) { denied: true, }, { - desc: "AccessDeniedHTTPwithAllowedXFFandDeniedRemote", + desc: "allowed xff and denied remote addr", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -135,7 +135,7 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) { denied: true, }, { - desc: "AccessDeniedHTTPwitAllowedXFFandAllowedRemote", + desc: "allowed xff and allowed remote addr", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -144,7 +144,7 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) { denied: false, }, { - desc: "AccessDeniedHTTPwithDeniedXFFandDeniedRemote", + desc: "denied xff and denied remote addr", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, From b88044e3ab94158575b54e840e0a51775bc89dd2 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Thu, 15 Feb 2018 07:21:21 -0600 Subject: [PATCH 05/10] assume /32 prefix if not passed --- route/access_rules.go | 13 +++++++++++-- route/access_rules_test.go | 5 +++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/route/access_rules.go b/route/access_rules.go index 65d754d0a..d73b3f218 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -102,6 +102,7 @@ func (t *Target) denyByIP(ip net.IP) bool { func (t *Target) parseAccessRule(allowDeny string) error { var accessTag string + var value string var temps []string // init rules if needed @@ -114,10 +115,17 @@ func (t *Target) parseAccessRule(allowDeny string) error { if temps = strings.SplitN(c, ":", 2); len(temps) != 2 { return fmt.Errorf("invalid access item, expected :, got %s", temps) } - accessTag = allowDeny + ":" + strings.ToLower(temps[0]) + + // form access type tag + accessTag = allowDeny + ":" + strings.ToLower(strings.TrimSpace(temps[0])) + + // switch on formed access tag - currently only ip types are implemented switch accessTag { case ipAllowTag, ipDenyTag: - _, net, err := net.ParseCIDR(strings.TrimSpace(temps[1])) + if value = strings.TrimSpace(temps[1]); !strings.Contains(value, "/") { + value = value + "/32" + } + _, net, err := net.ParseCIDR(value) if err != nil { return fmt.Errorf("failed to parse CIDR %s with error: %s", c, err.Error()) @@ -128,6 +136,7 @@ func (t *Target) parseAccessRule(allowDeny string) error { return fmt.Errorf("unknown access item type: %s", temps[0]) } } + return nil } diff --git a/route/access_rules_test.go b/route/access_rules_test.go index 69c5b7904..8f5fe9a45 100644 --- a/route/access_rules_test.go +++ b/route/access_rules_test.go @@ -31,6 +31,11 @@ func TestAccessRules_parseAccessRule(t *testing.T) { allowDeny: "ip:10.0.0.0/255", fail: true, }, + { + desc: "single ip with no mask", + allowDeny: "ip:1.2.3.4", + fail: false, + }, } for i, tt := range tests { From cce3288c84e0c0c015878bd66e5aa20af6dbdfa2 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Thu, 15 Feb 2018 11:44:58 -0600 Subject: [PATCH 06/10] handle adding a prefix for plain v4 and v6 addresses --- route/access_rules.go | 12 ++++++++++-- route/access_rules_test.go | 7 ++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/route/access_rules.go b/route/access_rules.go index d73b3f218..422c85830 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -102,8 +102,9 @@ func (t *Target) denyByIP(ip net.IP) bool { func (t *Target) parseAccessRule(allowDeny string) error { var accessTag string - var value string var temps []string + var value string + var ip net.IP // init rules if needed if t.accessRules == nil { @@ -123,7 +124,14 @@ func (t *Target) parseAccessRule(allowDeny string) error { switch accessTag { case ipAllowTag, ipDenyTag: if value = strings.TrimSpace(temps[1]); !strings.Contains(value, "/") { - value = value + "/32" + if ip = net.ParseIP(value); ip == nil { + return fmt.Errorf("failed to parse IP %s with error", value) + } + if ip.To4() != nil { + value = ip.String() + "/32" + } else { + value = ip.String() + "/128" + } } _, net, err := net.ParseCIDR(value) if err != nil { diff --git a/route/access_rules_test.go b/route/access_rules_test.go index 8f5fe9a45..5035606a3 100644 --- a/route/access_rules_test.go +++ b/route/access_rules_test.go @@ -32,10 +32,15 @@ func TestAccessRules_parseAccessRule(t *testing.T) { fail: true, }, { - desc: "single ip with no mask", + desc: "single ipv4 with no mask", allowDeny: "ip:1.2.3.4", fail: false, }, + { + desc: "single ipv6 with no mask", + allowDeny: "ip:fe80::1", + fail: false, + }, } for i, tt := range tests { From 57dd6e5766a74799fa4dc15db9a53f1e32d85def Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Thu, 15 Feb 2018 15:56:27 -0600 Subject: [PATCH 07/10] add ipv6 tests --- route/access_rules.go | 2 +- route/access_rules_test.go | 46 +++++++++++++++++++++++++++++++++----- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/route/access_rules.go b/route/access_rules.go index 422c85830..a8e7b57d7 100644 --- a/route/access_rules.go +++ b/route/access_rules.go @@ -125,7 +125,7 @@ func (t *Target) parseAccessRule(allowDeny string) error { case ipAllowTag, ipDenyTag: if value = strings.TrimSpace(temps[1]); !strings.Contains(value, "/") { if ip = net.ParseIP(value); ip == nil { - return fmt.Errorf("failed to parse IP %s with error", value) + return fmt.Errorf("failed to parse IP %s", value) } if ip.To4() != nil { value = ip.String() + "/32" diff --git a/route/access_rules_test.go b/route/access_rules_test.go index 5035606a3..edc12861b 100644 --- a/route/access_rules_test.go +++ b/route/access_rules_test.go @@ -13,9 +13,13 @@ func TestAccessRules_parseAccessRule(t *testing.T) { fail bool }{ { - desc: "valid rule", + desc: "valid ipv4 rule", allowDeny: "ip:10.0.0.0/8,ip:192.168.0.0/24,ip:1.2.3.4/32", }, + { + desc: "valid ipv6 rule", + allowDeny: "ip:1234:567:beef:cafe::/64,ip:1234:5678:dead:beef::/32", + }, { desc: "invalid rule type", allowDeny: "xxx:10.0.0.0/8", @@ -67,7 +71,7 @@ func TestAccessRules_denyByIP(t *testing.T) { denied bool }{ { - desc: "allow rule with included ip", + desc: "allow rule with included ipv4", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -75,7 +79,7 @@ func TestAccessRules_denyByIP(t *testing.T) { denied: false, }, { - desc: "allow rule with exluded ip", + desc: "allow rule with exluded ipv4", target: &Target{ Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -83,7 +87,7 @@ func TestAccessRules_denyByIP(t *testing.T) { denied: true, }, { - desc: "deny rule with included ip", + desc: "deny rule with included ipv4", target: &Target{ Opts: map[string]string{"deny": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, @@ -91,13 +95,45 @@ func TestAccessRules_denyByIP(t *testing.T) { denied: true, }, { - desc: "deny rule with excluded ip", + desc: "deny rule with excluded ipv4", target: &Target{ Opts: map[string]string{"deny": "ip:10.0.0.0/8,ip:192.168.0.0/24"}, }, remote: net.ParseIP("1.2.3.4"), denied: false, }, + { + desc: "allow rule with included ipv6", + target: &Target{ + Opts: map[string]string{"allow": "ip:1234:dead:beef:cafe::/64"}, + }, + remote: net.ParseIP("1234:dead:beef:cafe::5678"), + denied: false, + }, + { + desc: "allow rule with exluded ipv6", + target: &Target{ + Opts: map[string]string{"allow": "ip:1234:dead:beef:cafe::/64"}, + }, + remote: net.ParseIP("1234:5678::1"), + denied: true, + }, + { + desc: "deny rule with included ipv6", + target: &Target{ + Opts: map[string]string{"deny": "ip:1234:dead:beef:cafe::/64"}, + }, + remote: net.ParseIP("1234:dead:beef:cafe::5678"), + denied: true, + }, + { + desc: "deny rule with excluded ipv6", + target: &Target{ + Opts: map[string]string{"deny": "ip:1234:dead:beef:cafe::/64"}, + }, + remote: net.ParseIP("1234:5678::1"), + denied: false, + }, } for i, tt := range tests { From 58c2d829ee1777fbd1e51d5af59ba56f445cb4d0 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Fri, 16 Feb 2018 08:09:46 -0600 Subject: [PATCH 08/10] address review comments --- docs/content/cfg/_index.md | 4 +-- docs/content/feature/access-control.md | 16 ++++++--- proxy/http_integration_test.go | 30 ++++++++++++++++ proxy/tcp/sni_proxy.go | 1 - proxy/tcp/tcp_proxy.go | 1 - route/access_rules.go | 48 ++++++++++++++++---------- route/access_rules_test.go | 7 ++-- route/route.go | 2 +- 8 files changed, 78 insertions(+), 31 deletions(-) diff --git a/docs/content/cfg/_index.md b/docs/content/cfg/_index.md index 6bcfa9c53..4d2a4791b 100644 --- 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 index 5369af3df..2887edb9f 100644 --- 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. 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 index c430c4ae0..1a0870306 100644 --- 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 := "503" noroute.SetHTML(want) diff --git a/proxy/tcp/sni_proxy.go b/proxy/tcp/sni_proxy.go index b1361253b..37ec6b7e6 100644 --- 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 index 0551fa6f0..17bf69572 100644 --- 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 index a8e7b57d7..c572d25a0 100644 --- 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 index edc12861b..34547b1c6 100644 --- 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 index b3099cc90..32dce6a60 100644 --- 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()) } From 05ae7746dcc659d1b5b85d54361d268c2e6fcc2c Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Sat, 17 Feb 2018 11:21:58 -0600 Subject: [PATCH 09/10] add extra documentation around source determination --- docs/content/feature/access-control.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/content/feature/access-control.md b/docs/content/feature/access-control.md index 2887edb9f..0ababa037 100644 --- a/docs/content/feature/access-control.md +++ b/docs/content/feature/access-control.md @@ -35,3 +35,15 @@ Single host addresses (addresses without a prefix) will have a 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. + +The source ip used for validation against the defined ruleset is +taken from information available in the request. + +For `HTTP` requests the client `RemoteAddr` is always validated +followed by the first element of the `X-Forwarded-For` header, if +present. When either of these elements match an `allow` the request +will be allowed; similarly when either element matches a `deny` the +request will be denied. + +For `TCP` requests the originating address of the network socket +is used as the sole paramater for validation. From 7c111ce2dbd890159ecbf4390313f691303b6981 Mon Sep 17 00:00:00 2001 From: Frank Schroeder Date: Sun, 18 Feb 2018 22:28:41 +0100 Subject: [PATCH 10/10] add note about PROXY protocol --- docs/content/feature/access-control.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/content/feature/access-control.md b/docs/content/feature/access-control.md index 0ababa037..90a4a195e 100644 --- a/docs/content/feature/access-control.md +++ b/docs/content/feature/access-control.md @@ -45,5 +45,10 @@ present. When either of these elements match an `allow` the request will be allowed; similarly when either element matches a `deny` the request will be denied. -For `TCP` requests the originating address of the network socket +For `TCP` requests the source address of the network socket is used as the sole paramater for validation. + +If the inbound connection uses the [PROXY protocol](https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt) +to transmit the true source address of the client then it will +be used for both `HTTP` and `TCP` connections for validating access. +