From dbe1b2510d609bf37b0874f0aafa91e982cdc5c2 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Tue, 7 Feb 2017 18:01:16 +0000 Subject: [PATCH] middleware/proxy: fix except keyword (#505) Fix the except keyword usage - the config would allow it, but it was not enforced in the code. Turns out that **FROM** was also not enforced, fix both, by (basically) copying the code from Caddy. Update the README and tests. Locally test as well, shows that this works: ~~~ .:1053 { proxy miek.nl 8.8.8.8:53 { except a.miek.nl } proxy a.miek.nl 8.8.4.4:53 errors stdout log stdout } ~~~ And gives the desired results, not having a proxy line for `a.miek.nl` results in a SERVFAIL (as expected). Fixes #502 --- middleware/proxy/README.md | 2 +- middleware/proxy/lookup.go | 9 ++++++--- middleware/proxy/proxy.go | 31 +++++++++++++++++++++++++++---- middleware/proxy/upstream.go | 7 ++++--- middleware/proxy/upstream_test.go | 10 +++++----- 5 files changed, 43 insertions(+), 16 deletions(-) diff --git a/middleware/proxy/README.md b/middleware/proxy/README.md index 2e6d84c2b..20fbef720 100644 --- a/middleware/proxy/README.md +++ b/middleware/proxy/README.md @@ -36,7 +36,7 @@ proxy FROM TO... { * `fail_timeout` specifies how long to consider a backend as down after it has failed. While it is down, requests will not be routed to that backend. A backend is "down" if CoreDNS fails to communicate with it. The default value is 10 seconds ("10s"). * `max_fails` is the number of failures within fail_timeout that are needed before considering a backend to be down. If 0, the backend will never be marked as down. Default is 1. * `health_check` will check path (on port) on each backend. If a backend returns a status code of 200-399, then that backend is healthy. If it doesn't, the backend is marked as unhealthy for duration and no requests are routed to it. If this option is not provided then health checks are disabled. The default duration is 10 seconds ("10s"). -* `ignored_names...` is a space-separated list of paths to exclude from proxying. Requests that match any of these paths will be passed through. +* **IGNORED_NAMES** is a space-separated list of domains to exclude from proxying. Requests that match none of these names will be passed through. * `spray` when all backends are unhealthy, randomly pick one to send the traffic to. (This is a failsafe.) * `protocol` specifies what protocol to use to speak to an upstream, `dns` (the default) is plain old DNS, and `https_google` uses `https://dns.google.com` and speaks a JSON DNS dialect. Note when using this diff --git a/middleware/proxy/lookup.go b/middleware/proxy/lookup.go index 23c086bbf..56dc0eb4c 100644 --- a/middleware/proxy/lookup.go +++ b/middleware/proxy/lookup.go @@ -16,7 +16,7 @@ func NewLookup(hosts []string) Proxy { p := Proxy{Next: nil} upstream := &staticUpstream{ - from: "", + from: ".", Hosts: make([]*UpstreamHost, len(hosts)), Policy: &Random{}, Spray: nil, @@ -71,7 +71,11 @@ func (p Proxy) Forward(state request.Request) (*dns.Msg, error) { } func (p Proxy) lookup(state request.Request) (*dns.Msg, error) { - for _, upstream := range *p.Upstreams { + upstream := p.match(state) + if upstream == nil { + return nil, errInvalidDomain + } + for { start := time.Now() // Since Select() should give us "up" hosts, keep retrying @@ -106,5 +110,4 @@ func (p Proxy) lookup(state request.Request) (*dns.Msg, error) { } return nil, errUnreachable } - return nil, errUnreachable } diff --git a/middleware/proxy/proxy.go b/middleware/proxy/proxy.go index 3f870661c..b8b50adc6 100644 --- a/middleware/proxy/proxy.go +++ b/middleware/proxy/proxy.go @@ -17,6 +17,7 @@ import ( var ( errUnreachable = errors.New("unreachable backend") errInvalidProtocol = errors.New("invalid protocol") + errInvalidDomain = errors.New("invalid path for proxy") ) // Proxy represents a middleware instance that can proxy requests to another (DNS) server. @@ -37,7 +38,7 @@ type Upstream interface { // Selects an upstream host to be routed to. Select() *UpstreamHost // Checks if subpdomain is not an ignored. - IsAllowedPath(string) bool + IsAllowedDomain(string) bool // Exchanger returns the exchanger to be used for this upstream. Exchanger() Exchanger } @@ -73,12 +74,17 @@ func (uh *UpstreamHost) Down() bool { var tryDuration = 60 * time.Second // ServeDNS satisfies the middleware.Handler interface. - func (p Proxy) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) { var span, child ot.Span span = ot.SpanFromContext(ctx) state := request.Request{W: w, Req: r} - for _, upstream := range *p.Upstreams { + + upstream := p.match(state) + if upstream == nil { + return middleware.NextOrFailure(p.Name(), p.Next, ctx, w, r) + } + + for { start := time.Now() // Since Select() should give us "up" hosts, keep retrying @@ -129,7 +135,24 @@ func (p Proxy) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) ( return dns.RcodeServerFailure, errUnreachable } - return middleware.NextOrFailure(p.Name(), p.Next, ctx, w, r) +} + +func (p Proxy) match(state request.Request) (u Upstream) { + longestMatch := 0 + for _, upstream := range *p.Upstreams { + from := upstream.From() + + if !middleware.Name(from).Matches(state.Name()) || !upstream.IsAllowedDomain(state.Name()) { + continue + } + + if lf := len(from); lf > longestMatch { + longestMatch = lf + u = upstream + } + } + return u + } // Name implements the Handler interface. diff --git a/middleware/proxy/upstream.go b/middleware/proxy/upstream.go index b02c08a42..ce3c085f6 100644 --- a/middleware/proxy/upstream.go +++ b/middleware/proxy/upstream.go @@ -46,7 +46,7 @@ func NewStaticUpstreams(c *caddyfile.Dispenser) ([]Upstream, error) { var upstreams []Upstream for c.Next() { upstream := &staticUpstream{ - from: "", + from: ".", Hosts: nil, Policy: &Random{}, Spray: nil, @@ -280,12 +280,13 @@ func (u *staticUpstream) Select() *UpstreamHost { return u.Spray.Select(pool) } -func (u *staticUpstream) IsAllowedPath(name string) bool { +func (u *staticUpstream) IsAllowedDomain(name string) bool { for _, ignoredSubDomain := range u.IgnoredSubDomains { if dns.Name(name) == dns.Name(u.From()) { return true } - if middleware.Name(name).Matches(ignoredSubDomain + u.From()) { + + if middleware.Name(ignoredSubDomain).Matches(name) { return false } } diff --git a/middleware/proxy/upstream_test.go b/middleware/proxy/upstream_test.go index 02099468c..6580a569a 100644 --- a/middleware/proxy/upstream_test.go +++ b/middleware/proxy/upstream_test.go @@ -13,7 +13,7 @@ import ( func TestHealthCheck(t *testing.T) { upstream := &staticUpstream{ - from: "", + from: ".", Hosts: testPool(), Policy: &Random{}, Spray: nil, @@ -31,7 +31,7 @@ func TestHealthCheck(t *testing.T) { func TestSelect(t *testing.T) { upstream := &staticUpstream{ - from: "", + from: ".", Hosts: testPool()[:3], Policy: &Random{}, FailTimeout: 10 * time.Second, @@ -59,10 +59,10 @@ func TestRegisterPolicy(t *testing.T) { } -func TestAllowedPaths(t *testing.T) { +func TestAllowedDomain(t *testing.T) { upstream := &staticUpstream{ from: "miek.nl.", - IgnoredSubDomains: []string{"download.", "static."}, // closing dot mandatory + IgnoredSubDomains: []string{"download.miek.nl.", "static.miek.nl."}, // closing dot mandatory } tests := []struct { name string @@ -75,7 +75,7 @@ func TestAllowedPaths(t *testing.T) { } for i, test := range tests { - isAllowed := upstream.IsAllowedPath(test.name) + isAllowed := upstream.IsAllowedDomain(test.name) if test.expected != isAllowed { t.Errorf("Test %d: expected %v found %v for %s", i+1, test.expected, isAllowed, test.name) }