From 914a77b72ffb229c88d2ae070734e0a6eae99ea9 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Mon, 16 Apr 2018 21:23:39 -0400 Subject: [PATCH 1/9] WIP: X-Forwarded-For --- command/server.go | 34 ++++++++++--- command/server/config.go | 4 ++ command/server/listener_tcp.go | 52 ++++++++++++++++++++ helper/parseutil/parseutil.go | 43 ++++++++++++++++ helper/proxyutil/proxyutil.go | 40 ++------------- http/handler.go | 90 ++++++++++++++++++++++++++++++++++ 6 files changed, 221 insertions(+), 42 deletions(-) diff --git a/command/server.go b/command/server.go index cb5bd9b7b59f..bbdfdee5ae1c 100644 --- a/command/server.go +++ b/command/server.go @@ -32,6 +32,7 @@ import ( "github.com/hashicorp/errwrap" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-multierror" + sockaddr "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/vault/audit" "github.com/hashicorp/vault/command/server" "github.com/hashicorp/vault/helper/gated-writer" @@ -92,6 +93,11 @@ type ServerCommand struct { flagTestVerifyOnly bool } +type ServerListener struct { + net.Listener + config map[string]interface{} +} + func (c *ServerCommand) Synopsis() string { return "Start a Vault server" } @@ -670,8 +676,8 @@ CLUSTER_SYNTHESIS_COMPLETE: clusterAddrs := []*net.TCPAddr{} // Initialize the listeners + lns := make([]ServerListener, 0, len(config.Listeners)) c.reloadFuncsLock.Lock() - lns := make([]net.Listener, 0, len(config.Listeners)) for i, lnConfig := range config.Listeners { ln, props, reloadFunc, err := server.NewListener(lnConfig.Type, lnConfig.Config, c.logGate, c.UI) if err != nil { @@ -679,7 +685,10 @@ CLUSTER_SYNTHESIS_COMPLETE: return 1 } - lns = append(lns, ln) + lns = append(lns, ServerListener{ + Listener: ln, + config: lnConfig.Config, + }) if reloadFunc != nil { relSlice := (*c.reloadFuncs)["listener|"+lnConfig.Type] @@ -738,7 +747,7 @@ CLUSTER_SYNTHESIS_COMPLETE: // Make sure we close all listeners from this point on listenerCloseFunc := func() { for _, ln := range lns { - ln.Close() + ln.Listener.Close() } } @@ -776,12 +785,10 @@ CLUSTER_SYNTHESIS_COMPLETE: return 0 } - handler := vaulthttp.Handler(core) - // This needs to happen before we first unseal, so before we trigger dev // mode if it's set core.SetClusterListenerAddrs(clusterAddrs) - core.SetClusterHandler(handler) + core.SetClusterHandler(vaulthttp.Handler(core)) err = core.UnsealWithStoredKeys(context.Background()) if err != nil { @@ -914,10 +921,23 @@ CLUSTER_SYNTHESIS_COMPLETE: // Initialize the HTTP servers for _, ln := range lns { + handler := vaulthttp.Handler(core) + + // We perform validation on the config earlier, we can just cast here + if _, ok := ln.config["forwarded_for_authorized_addrs"]; ok { + hopSkips := ln.config["forwarded_for_hop_skips"].(int) + authzdAddrs := ln.config["forwarded_for_authorized_addrs"].([]*sockaddr.SockAddrMarshaler) + rejectNotPresent := ln.config["forwarded_for_reject_not_present"].(bool) + rejectNonAuthz := ln.config["forwarded_for_reject_non_authorized"].(bool) + if len(authzdAddrs) > 0 { + handler = vaulthttp.WrapForwardedForHandler(handler, authzdAddrs, rejectNotPresent, rejectNonAuthz, hopSkips) + } + } + server := &http.Server{ Handler: handler, } - go server.Serve(ln) + go server.Serve(ln.Listener) } if newCoreError != nil { diff --git a/command/server/config.go b/command/server/config.go index b150b83bd8c4..79e606ff7a13 100644 --- a/command/server/config.go +++ b/command/server/config.go @@ -761,6 +761,10 @@ func parseListeners(result *Config, list *ast.ObjectList) error { "address", "cluster_address", "endpoint", + "forwarded_for_authorized_addrs", + "forwarded_for_hop_skips", + "forwarded_for_reject_non_authorized", + "forwarded_for_reject_not_present", "infrastructure", "node_id", "proxy_protocol_behavior", diff --git a/command/server/listener_tcp.go b/command/server/listener_tcp.go index bf39615a69f9..19ffd210824a 100644 --- a/command/server/listener_tcp.go +++ b/command/server/listener_tcp.go @@ -1,11 +1,15 @@ package server import ( + "fmt" "io" "net" + "strconv" "strings" "time" + "github.com/hashicorp/errwrap" + "github.com/hashicorp/vault/helper/parseutil" "github.com/hashicorp/vault/helper/reload" "github.com/mitchellh/cli" ) @@ -39,6 +43,54 @@ func tcpListenerFactory(config map[string]interface{}, _ io.Writer, ui cli.Ui) ( } props := map[string]string{"addr": addr} + + ffAllowedRaw, ffAllowedOK := config["forwarded_for_authorized_addrs"] + if ffAllowedOK { + ffAllowed, err := parseutil.ParseAddrs(ffAllowedRaw) + if err != nil { + return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_authorized_addrs\": {{err}}", err) + } + props["forwarded_for_authorized_addrs"] = fmt.Sprintf("%v", ffAllowed) + config["forwarded_for_authorized_addrs"] = ffAllowed + } + + if ffHopsRaw, ok := config["forwarded_for_hop_skips"]; ok { + ffHops, err := parseutil.ParseInt(ffHopsRaw) + if err != nil { + return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_hop_skips\": {{err}}", err) + } + props["forwarded_for_hop_skips"] = strconv.Itoa(int(ffHops)) + config["forwarded_for_hop_skips"] = ffHops + } else if ffAllowedOK { + ffHops := 0 + props["forwarded_for_hop_skips"] = "0" + config["forwarded_for_hop_skips"] = int(ffHops) + } + + if ffRejectNotPresentRaw, ok := config["forwarded_for_reject_not_present"]; ok { + ffRejectNotPresent, err := parseutil.ParseBool(ffRejectNotPresentRaw) + if err != nil { + return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_reject_not_present\": {{err}}", err) + } + props["forwarded_for_reject_not_present"] = strconv.FormatBool(ffRejectNotPresent) + config["forwarded_for_reject_not_present"] = ffRejectNotPresent + } else if ffAllowedOK { + props["forwarded_for_reject_not_present"] = "true" + config["forwarded_for_reject_not_present"] = true + } + + if ffRejectNonAuthorizedRaw, ok := config["forwarded_for_reject_non_authorized"]; ok { + ffRejectNonAuthorized, err := parseutil.ParseBool(ffRejectNonAuthorizedRaw) + if err != nil { + return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_reject_non_authorized\": {{err}}", err) + } + props["forwarded_for_reject_non_authorized"] = strconv.FormatBool(ffRejectNonAuthorized) + config["forwarded_for_reject_non_authorized"] = ffRejectNonAuthorized + } else if ffAllowedOK { + props["forwarded_for_reject_non_authorized"] = "true" + config["forwarded_for_reject_non_authorized"] = true + } + return listenerWrapTLS(ln, props, config, ui) } diff --git a/helper/parseutil/parseutil.go b/helper/parseutil/parseutil.go index 464b50899cf9..0a2e5537dc0a 100644 --- a/helper/parseutil/parseutil.go +++ b/helper/parseutil/parseutil.go @@ -3,10 +3,13 @@ package parseutil import ( "encoding/json" "errors" + "fmt" "strconv" "strings" "time" + "github.com/hashicorp/errwrap" + sockaddr "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/vault/helper/strutil" "github.com/mitchellh/mapstructure" ) @@ -118,3 +121,43 @@ func ParseCommaStringSlice(in interface{}) ([]string, error) { } return strutil.TrimStrings(result), nil } + +func ParseAddrs(addrs interface{}) ([]*sockaddr.SockAddrMarshaler, error) { + out := make([]*sockaddr.SockAddrMarshaler, 0) + stringAddrs := make([]string, 0) + + switch addrs.(type) { + case string: + stringAddrs = strutil.ParseArbitraryStringSlice(addrs.(string), ",") + if len(stringAddrs) == 0 { + return nil, fmt.Errorf("unable to parse addresses from %v", addrs) + } + + case []string: + stringAddrs = addrs.([]string) + + case []interface{}: + for _, v := range addrs.([]interface{}) { + stringAddr, ok := v.(string) + if !ok { + return nil, fmt.Errorf("error parsing %v as string", v) + } + stringAddrs = append(stringAddrs, stringAddr) + } + + default: + return nil, fmt.Errorf("unknown address input type %T", addrs) + } + + for _, addr := range stringAddrs { + sa, err := sockaddr.NewSockAddr(addr) + if err != nil { + return nil, errwrap.Wrapf("error parsing address: {{err}}", err) + } + out = append(out, &sockaddr.SockAddrMarshaler{ + SockAddr: sa, + }) + } + + return out, nil +} diff --git a/helper/proxyutil/proxyutil.go b/helper/proxyutil/proxyutil.go index 06371b29e548..875e74831c94 100644 --- a/helper/proxyutil/proxyutil.go +++ b/helper/proxyutil/proxyutil.go @@ -8,7 +8,7 @@ import ( proxyproto "github.com/armon/go-proxyproto" "github.com/hashicorp/errwrap" sockaddr "github.com/hashicorp/go-sockaddr" - "github.com/hashicorp/vault/helper/strutil" + "github.com/hashicorp/vault/helper/parseutil" ) // ProxyProtoConfig contains configuration for the PROXY protocol @@ -19,42 +19,12 @@ type ProxyProtoConfig struct { } func (p *ProxyProtoConfig) SetAuthorizedAddrs(addrs interface{}) error { - p.AuthorizedAddrs = make([]*sockaddr.SockAddrMarshaler, 0) - stringAddrs := make([]string, 0) - - switch addrs.(type) { - case string: - stringAddrs = strutil.ParseArbitraryStringSlice(addrs.(string), ",") - if len(stringAddrs) == 0 { - return fmt.Errorf("unable to parse addresses from %v", addrs) - } - - case []string: - stringAddrs = addrs.([]string) - - case []interface{}: - for _, v := range addrs.([]interface{}) { - stringAddr, ok := v.(string) - if !ok { - return fmt.Errorf("error parsing %v as string", v) - } - stringAddrs = append(stringAddrs, stringAddr) - } - - default: - return fmt.Errorf("unknown address input type %T", addrs) - } - - for _, addr := range stringAddrs { - sa, err := sockaddr.NewSockAddr(addr) - if err != nil { - return errwrap.Wrapf("error parsing authorized address: {{err}}", err) - } - p.AuthorizedAddrs = append(p.AuthorizedAddrs, &sockaddr.SockAddrMarshaler{ - SockAddr: sa, - }) + aa, err := parseutil.ParseAddrs(addrs) + if err != nil { + return err } + p.AuthorizedAddrs = aa return nil } diff --git a/http/handler.go b/http/handler.go index a4e284dc3601..d788c6c0ab03 100644 --- a/http/handler.go +++ b/http/handler.go @@ -4,7 +4,9 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" + "net/textproto" "net/url" "os" "strings" @@ -13,6 +15,7 @@ import ( "github.com/elazarl/go-bindata-assetfs" "github.com/hashicorp/errwrap" cleanhttp "github.com/hashicorp/go-cleanhttp" + sockaddr "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/vault/helper/consts" "github.com/hashicorp/vault/helper/jsonutil" "github.com/hashicorp/vault/helper/parseutil" @@ -124,6 +127,93 @@ func wrapGenericHandler(h http.Handler) http.Handler { }) } +func WrapForwardedForHandler(h http.Handler, authorizedAddrs []*sockaddr.SockAddrMarshaler, rejectNotPresent, rejectNonAuthz bool, hopSkips int) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + headers, headersOK := r.Header[textproto.CanonicalMIMEHeaderKey("X-Forwarded-For")] + if !headersOK || len(headers) == 0 { + if !rejectNotPresent { + h.ServeHTTP(w, r) + return + } + respondError(w, http.StatusBadRequest, fmt.Errorf("missing x-forwarded-for header and configured to reject when not present")) + return + } + + host, port, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + // If not rejecting treat it like we just don't have a valid + // header because we can't do a comparison against an address we + // can't understand + if !rejectNotPresent { + h.ServeHTTP(w, r) + return + } + respondError(w, http.StatusBadRequest, errwrap.Wrapf("error parsing client hostport: {{err}}", err)) + return + } + + addr, err := sockaddr.NewIPAddr(host) + if err != nil { + // We treat this the same as the case above + if !rejectNotPresent { + h.ServeHTTP(w, r) + return + } + respondError(w, http.StatusBadRequest, errwrap.Wrapf("error parsing client address: {{err}}", err)) + return + } + + var found bool + for _, authz := range authorizedAddrs { + if authz.Contains(addr) { + found = true + break + } + } + if !found { + // If we didn't find it and aren't configured to reject, simply + // don't trust it + if !rejectNonAuthz { + h.ServeHTTP(w, r) + return + } + respondError(w, http.StatusBadRequest, fmt.Errorf("client address not authorized for x-forwarded-for and configured to reject connection")) + return + } + + // At this point we have at least one value and it's authorized + + // Split comma separated ones, which are common. This brings it in line + // to the multiple-header case. + if len(headers) == 1 { + headers = strings.Split(headers[0], ",") + } + for i, v := range headers { + headers[i] = strings.TrimSpace(v) + } + + indexToUse := len(headers) - 1 - hopSkips + if indexToUse < 0 { + // This is likely an error in either configuration or other + // infrastructure. We could either deny the request, or we + // could simply not trust the value. Denying the request is + // "safer" since if this logic is configured at all there may + // be an assumption it can always be trusted. Given that we can + // deny accepting the request at all if it's not from an + // authorized address, if we're at this point the address is + // authorized (or we've turned off explicit rejection) and we + // should assume that what comes in should be properly + // formatted. + respondError(w, http.StatusBadRequest, fmt.Errorf("malformed X-Forwarded-For configuration or request, hops to skip (%d) would skip before earliest chain link (chain length %d)", hopSkips, len(headers))) + return + } + + r.RemoteAddr = net.JoinHostPort(headers[indexToUse], port) + h.ServeHTTP(w, r) + return + }) +} + // A lookup on a token that is about to expire returns nil, which means by the // time we can validate a wrapping token lookup will return nil since it will // be revoked after the call. So we have to do the validation here. From 6f450e6fc997cd28cff83c0bd7d23625e9f86280 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 10:52:29 -0400 Subject: [PATCH 2/9] Add beginnings of tests and some minor code updates --- command/server/listener_tcp.go | 3 +++ http/handler.go | 2 +- http/handler_test.go | 36 ++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/command/server/listener_tcp.go b/command/server/listener_tcp.go index 19ffd210824a..956ea1437e6c 100644 --- a/command/server/listener_tcp.go +++ b/command/server/listener_tcp.go @@ -59,6 +59,9 @@ func tcpListenerFactory(config map[string]interface{}, _ io.Writer, ui cli.Ui) ( if err != nil { return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_hop_skips\": {{err}}", err) } + if ffHops < 0 { + return nil, nil, nil, fmt.Errorf("\"forwarded_for_hop_skips\" cannot be negative") + } props["forwarded_for_hop_skips"] = strconv.Itoa(int(ffHops)) config["forwarded_for_hop_skips"] = ffHops } else if ffAllowedOK { diff --git a/http/handler.go b/http/handler.go index d788c6c0ab03..3573a352a8d0 100644 --- a/http/handler.go +++ b/http/handler.go @@ -204,7 +204,7 @@ func WrapForwardedForHandler(h http.Handler, authorizedAddrs []*sockaddr.SockAdd // authorized (or we've turned off explicit rejection) and we // should assume that what comes in should be properly // formatted. - respondError(w, http.StatusBadRequest, fmt.Errorf("malformed X-Forwarded-For configuration or request, hops to skip (%d) would skip before earliest chain link (chain length %d)", hopSkips, len(headers))) + respondError(w, http.StatusBadRequest, fmt.Errorf("malformed x-forwarded-for configuration or request, hops to skip (%d) would skip before earliest chain link (chain length %d)", hopSkips, len(headers))) return } diff --git a/http/handler_test.go b/http/handler_test.go index 3760b4c98b08..939ecd92f8aa 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/hashicorp/go-cleanhttp" + sockaddr "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/vault/helper/consts" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/vault" @@ -408,3 +409,38 @@ func TestHandler_nonPrintableChars(t *testing.T) { testResponseStatus(t, resp, 400) } + +func TestHandler_XForwardedFor(t *testing.T) { + addr, err := sockaddr.NewIPAddr("127.0.0.1") + if err != nil { + t.Fatal(err) + } + + // First: test reject not present + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: addr, + }, + }, true, false, 0) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, + }) + cluster.Start() + defer cluster.Cleanup() + + client := cluster.Cores[0].Client + req := client.NewRequest("GET", "/") + _, err = client.RawRequest(req) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "missing x-forwarded-for") { + t.Fatalf("bad error message: %v", err) + } +} From a632325046fb18124c02ef433b7aeedc914d861e Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 12:23:01 -0400 Subject: [PATCH 3/9] Reshuffle some test logic and add more tests --- http/forwarded_for_test.go | 130 +++++++++++++++++++++++++++++++++++++ http/handler_test.go | 36 ---------- 2 files changed, 130 insertions(+), 36 deletions(-) create mode 100644 http/forwarded_for_test.go diff --git a/http/forwarded_for_test.go b/http/forwarded_for_test.go new file mode 100644 index 000000000000..56c460a110e8 --- /dev/null +++ b/http/forwarded_for_test.go @@ -0,0 +1,130 @@ +package http + +import ( + "bytes" + "net/http" + "strings" + "testing" + + sockaddr "github.com/hashicorp/go-sockaddr" + "github.com/hashicorp/vault/vault" +) + +func TestHandler_XForwardedFor(t *testing.T) { + goodAddr, err := sockaddr.NewIPAddr("127.0.0.1") + if err != nil { + t.Fatal(err) + } + + badAddr, err := sockaddr.NewIPAddr("1.2.3.4") + if err != nil { + t.Fatal(err) + } + + // First: test reject not present + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: goodAddr, + }, + }, true, false, 0) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, + }) + cluster.Start() + defer cluster.Cleanup() + client := cluster.Cores[0].Client + + req := client.NewRequest("GET", "/") + _, err = client.RawRequest(req) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "missing x-forwarded-for") { + t.Fatalf("bad error message: %v", err) + } + req = client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "1.2.3.4") + resp, err := client.RawRequest(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + buf := bytes.NewBuffer(nil) + buf.ReadFrom(resp.Body) + if !strings.HasPrefix(buf.String(), "1.2.3.4:") { + t.Fatalf("bad body: %s", buf.String()) + } + + // Next: test allow unauth + testHandler = func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: badAddr, + }, + }, true, false, 0) + } + + cluster = vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, + }) + cluster.Start() + defer cluster.Cleanup() + client = cluster.Cores[0].Client + + req = client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "5.6.7.8") + resp, err = client.RawRequest(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + buf = bytes.NewBuffer(nil) + buf.ReadFrom(resp.Body) + if !strings.HasPrefix(buf.String(), "127.0.0.1:") { + t.Fatalf("bad body: %s", buf.String()) + } + + // Next: test fail unauth + testHandler = func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: badAddr, + }, + }, true, true, 0) + } + + cluster = vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, + }) + cluster.Start() + defer cluster.Cleanup() + client = cluster.Cores[0].Client + + req = client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "5.6.7.8") + _, err = client.RawRequest(req) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "not authorized for x-forwarded-for") { + t.Fatalf("bad error message: %v", err) + } +} diff --git a/http/handler_test.go b/http/handler_test.go index 939ecd92f8aa..3760b4c98b08 100644 --- a/http/handler_test.go +++ b/http/handler_test.go @@ -11,7 +11,6 @@ import ( "testing" "github.com/hashicorp/go-cleanhttp" - sockaddr "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/vault/helper/consts" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/vault" @@ -409,38 +408,3 @@ func TestHandler_nonPrintableChars(t *testing.T) { testResponseStatus(t, resp, 400) } - -func TestHandler_XForwardedFor(t *testing.T) { - addr, err := sockaddr.NewIPAddr("127.0.0.1") - if err != nil { - t.Fatal(err) - } - - // First: test reject not present - testHandler := func(c *vault.Core) http.Handler { - origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNoContent) - }) - return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ - &sockaddr.SockAddrMarshaler{ - SockAddr: addr, - }, - }, true, false, 0) - } - - cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ - HandlerFunc: testHandler, - }) - cluster.Start() - defer cluster.Cleanup() - - client := cluster.Cores[0].Client - req := client.NewRequest("GET", "/") - _, err = client.RawRequest(req) - if err == nil { - t.Fatal("expected error") - } - if !strings.Contains(err.Error(), "missing x-forwarded-for") { - t.Fatalf("bad error message: %v", err) - } -} From d051c9025f7967006b224f73c62388724b46529a Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 13:43:36 -0400 Subject: [PATCH 4/9] Finish tests and parallelize them --- http/forwarded_for_test.go | 305 ++++++++++++++++++++++++++----------- 1 file changed, 213 insertions(+), 92 deletions(-) diff --git a/http/forwarded_for_test.go b/http/forwarded_for_test.go index 56c460a110e8..22106d14ab73 100644 --- a/http/forwarded_for_test.go +++ b/http/forwarded_for_test.go @@ -22,109 +22,230 @@ func TestHandler_XForwardedFor(t *testing.T) { } // First: test reject not present - testHandler := func(c *vault.Core) http.Handler { - origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - w.Write([]byte(r.RemoteAddr)) + t.Run("reject_not_present", func(t *testing.T) { + t.Parallel() + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: goodAddr, + }, + }, true, false, 0) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, }) - return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ - &sockaddr.SockAddrMarshaler{ - SockAddr: goodAddr, - }, - }, true, false, 0) - } + cluster.Start() + defer cluster.Cleanup() + client := cluster.Cores[0].Client - cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ - HandlerFunc: testHandler, + req := client.NewRequest("GET", "/") + _, err = client.RawRequest(req) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "missing x-forwarded-for") { + t.Fatalf("bad error message: %v", err) + } + req = client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "1.2.3.4") + resp, err := client.RawRequest(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + buf := bytes.NewBuffer(nil) + buf.ReadFrom(resp.Body) + if !strings.HasPrefix(buf.String(), "1.2.3.4:") { + t.Fatalf("bad body: %s", buf.String()) + } }) - cluster.Start() - defer cluster.Cleanup() - client := cluster.Cores[0].Client - - req := client.NewRequest("GET", "/") - _, err = client.RawRequest(req) - if err == nil { - t.Fatal("expected error") - } - if !strings.Contains(err.Error(), "missing x-forwarded-for") { - t.Fatalf("bad error message: %v", err) - } - req = client.NewRequest("GET", "/") - req.Headers = make(http.Header) - req.Headers.Set("x-forwarded-for", "1.2.3.4") - resp, err := client.RawRequest(req) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - buf := bytes.NewBuffer(nil) - buf.ReadFrom(resp.Body) - if !strings.HasPrefix(buf.String(), "1.2.3.4:") { - t.Fatalf("bad body: %s", buf.String()) - } // Next: test allow unauth - testHandler = func(c *vault.Core) http.Handler { - origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - w.Write([]byte(r.RemoteAddr)) + t.Run("allow_unauth", func(t *testing.T) { + t.Parallel() + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: badAddr, + }, + }, true, false, 0) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, }) - return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ - &sockaddr.SockAddrMarshaler{ - SockAddr: badAddr, - }, - }, true, false, 0) - } + cluster.Start() + defer cluster.Cleanup() + client := cluster.Cores[0].Client - cluster = vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ - HandlerFunc: testHandler, + req := client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "5.6.7.8") + resp, err := client.RawRequest(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + buf := bytes.NewBuffer(nil) + buf.ReadFrom(resp.Body) + if !strings.HasPrefix(buf.String(), "127.0.0.1:") { + t.Fatalf("bad body: %s", buf.String()) + } }) - cluster.Start() - defer cluster.Cleanup() - client = cluster.Cores[0].Client - - req = client.NewRequest("GET", "/") - req.Headers = make(http.Header) - req.Headers.Set("x-forwarded-for", "5.6.7.8") - resp, err = client.RawRequest(req) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - buf = bytes.NewBuffer(nil) - buf.ReadFrom(resp.Body) - if !strings.HasPrefix(buf.String(), "127.0.0.1:") { - t.Fatalf("bad body: %s", buf.String()) - } // Next: test fail unauth - testHandler = func(c *vault.Core) http.Handler { - origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - w.Write([]byte(r.RemoteAddr)) + t.Run("fail_unauth", func(t *testing.T) { + t.Parallel() + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: badAddr, + }, + }, true, true, 0) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, }) - return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ - &sockaddr.SockAddrMarshaler{ - SockAddr: badAddr, - }, - }, true, true, 0) - } + cluster.Start() + defer cluster.Cleanup() + client := cluster.Cores[0].Client - cluster = vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ - HandlerFunc: testHandler, + req := client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "5.6.7.8") + _, err = client.RawRequest(req) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "not authorized for x-forwarded-for") { + t.Fatalf("bad error message: %v", err) + } }) - cluster.Start() - defer cluster.Cleanup() - client = cluster.Cores[0].Client - - req = client.NewRequest("GET", "/") - req.Headers = make(http.Header) - req.Headers.Set("x-forwarded-for", "5.6.7.8") - _, err = client.RawRequest(req) - if err == nil { - t.Fatal("expected error") - } - if !strings.Contains(err.Error(), "not authorized for x-forwarded-for") { - t.Fatalf("bad error message: %v", err) - } + + // Next: test bad hops (too many) + t.Run("too_many_hops", func(t *testing.T) { + t.Parallel() + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: goodAddr, + }, + }, true, true, 4) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, + }) + cluster.Start() + defer cluster.Cleanup() + client := cluster.Cores[0].Client + + req := client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "2.3.4.5,3.4.5.6") + _, err = client.RawRequest(req) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "would skip before earliest") { + t.Fatalf("bad error message: %v", err) + } + }) + + // Next: test picking correct value + t.Run("correct_hop_skipping", func(t *testing.T) { + t.Parallel() + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: goodAddr, + }, + }, true, true, 1) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, + }) + cluster.Start() + defer cluster.Cleanup() + client := cluster.Cores[0].Client + + req := client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Set("x-forwarded-for", "2.3.4.5,3.4.5.6,4.5.6.7,5.6.7.8") + resp, err := client.RawRequest(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + buf := bytes.NewBuffer(nil) + buf.ReadFrom(resp.Body) + if !strings.HasPrefix(buf.String(), "4.5.6.7:") { + t.Fatalf("bad body: %s", buf.String()) + } + }) + + // Next: multi-header approach + t.Run("correct_hop_skipping_multi_header", func(t *testing.T) { + t.Parallel() + testHandler := func(c *vault.Core) http.Handler { + origHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(r.RemoteAddr)) + }) + return WrapForwardedForHandler(origHandler, []*sockaddr.SockAddrMarshaler{ + &sockaddr.SockAddrMarshaler{ + SockAddr: goodAddr, + }, + }, true, true, 1) + } + + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: testHandler, + }) + cluster.Start() + defer cluster.Cleanup() + client := cluster.Cores[0].Client + + req := client.NewRequest("GET", "/") + req.Headers = make(http.Header) + req.Headers.Add("x-forwarded-for", "2.3.4.5") + req.Headers.Add("x-forwarded-for", "3.4.5.6") + req.Headers.Add("x-forwarded-for", "4.5.6.7") + req.Headers.Add("x-forwarded-for", "5.6.7.8") + resp, err := client.RawRequest(req) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + buf := bytes.NewBuffer(nil) + buf.ReadFrom(resp.Body) + if !strings.HasPrefix(buf.String(), "4.5.6.7:") { + t.Fatalf("bad body: %s", buf.String()) + } + }) + } From 58c51ec5e1faa9f423d60761b680e66132448eb0 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 16:45:43 -0400 Subject: [PATCH 5/9] Address review feedback --- command/server.go | 2 +- command/server/config.go | 2 +- command/server/listener_tcp.go | 12 ++++++------ helper/parseutil/parseutil.go | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/command/server.go b/command/server.go index bbdfdee5ae1c..db00de31b5c0 100644 --- a/command/server.go +++ b/command/server.go @@ -928,7 +928,7 @@ CLUSTER_SYNTHESIS_COMPLETE: hopSkips := ln.config["forwarded_for_hop_skips"].(int) authzdAddrs := ln.config["forwarded_for_authorized_addrs"].([]*sockaddr.SockAddrMarshaler) rejectNotPresent := ln.config["forwarded_for_reject_not_present"].(bool) - rejectNonAuthz := ln.config["forwarded_for_reject_non_authorized"].(bool) + rejectNonAuthz := ln.config["forwarded_for_reject_not_authorized"].(bool) if len(authzdAddrs) > 0 { handler = vaulthttp.WrapForwardedForHandler(handler, authzdAddrs, rejectNotPresent, rejectNonAuthz, hopSkips) } diff --git a/command/server/config.go b/command/server/config.go index 79e606ff7a13..eb5f9fc36e32 100644 --- a/command/server/config.go +++ b/command/server/config.go @@ -763,7 +763,7 @@ func parseListeners(result *Config, list *ast.ObjectList) error { "endpoint", "forwarded_for_authorized_addrs", "forwarded_for_hop_skips", - "forwarded_for_reject_non_authorized", + "forwarded_for_reject_not_authorized", "forwarded_for_reject_not_present", "infrastructure", "node_id", diff --git a/command/server/listener_tcp.go b/command/server/listener_tcp.go index 956ea1437e6c..8a6e6fdc1508 100644 --- a/command/server/listener_tcp.go +++ b/command/server/listener_tcp.go @@ -82,16 +82,16 @@ func tcpListenerFactory(config map[string]interface{}, _ io.Writer, ui cli.Ui) ( config["forwarded_for_reject_not_present"] = true } - if ffRejectNonAuthorizedRaw, ok := config["forwarded_for_reject_non_authorized"]; ok { + if ffRejectNonAuthorizedRaw, ok := config["forwarded_for_reject_not_authorized"]; ok { ffRejectNonAuthorized, err := parseutil.ParseBool(ffRejectNonAuthorizedRaw) if err != nil { - return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_reject_non_authorized\": {{err}}", err) + return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_reject_not_authorized\": {{err}}", err) } - props["forwarded_for_reject_non_authorized"] = strconv.FormatBool(ffRejectNonAuthorized) - config["forwarded_for_reject_non_authorized"] = ffRejectNonAuthorized + props["forwarded_for_reject_not_authorized"] = strconv.FormatBool(ffRejectNonAuthorized) + config["forwarded_for_reject_not_authorized"] = ffRejectNonAuthorized } else if ffAllowedOK { - props["forwarded_for_reject_non_authorized"] = "true" - config["forwarded_for_reject_non_authorized"] = true + props["forwarded_for_reject_not_authorized"] = "true" + config["forwarded_for_reject_not_authorized"] = true } return listenerWrapTLS(ln, props, config, ui) diff --git a/helper/parseutil/parseutil.go b/helper/parseutil/parseutil.go index 0a2e5537dc0a..ae8c58ba7840 100644 --- a/helper/parseutil/parseutil.go +++ b/helper/parseutil/parseutil.go @@ -152,7 +152,7 @@ func ParseAddrs(addrs interface{}) ([]*sockaddr.SockAddrMarshaler, error) { for _, addr := range stringAddrs { sa, err := sockaddr.NewSockAddr(addr) if err != nil { - return nil, errwrap.Wrapf("error parsing address: {{err}}", err) + return nil, errwrap.Wrapf(fmt.Sprintf("error parsing address %q: {{err}}", addr), err) } out = append(out, &sockaddr.SockAddrMarshaler{ SockAddr: sa, From 8bb6a8c7aaffc016f6bd92e141f7dc9e00894238 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 17:04:02 -0400 Subject: [PATCH 6/9] Update config params so we won't clash if we support Forwarded in the future --- command/server.go | 10 ++++---- command/server/config.go | 8 +++--- command/server/listener_tcp.go | 46 +++++++++++++++++----------------- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/command/server.go b/command/server.go index db00de31b5c0..630f9aada727 100644 --- a/command/server.go +++ b/command/server.go @@ -924,11 +924,11 @@ CLUSTER_SYNTHESIS_COMPLETE: handler := vaulthttp.Handler(core) // We perform validation on the config earlier, we can just cast here - if _, ok := ln.config["forwarded_for_authorized_addrs"]; ok { - hopSkips := ln.config["forwarded_for_hop_skips"].(int) - authzdAddrs := ln.config["forwarded_for_authorized_addrs"].([]*sockaddr.SockAddrMarshaler) - rejectNotPresent := ln.config["forwarded_for_reject_not_present"].(bool) - rejectNonAuthz := ln.config["forwarded_for_reject_not_authorized"].(bool) + if _, ok := ln.config["x_forwarded_for_authorized_addrs"]; ok { + hopSkips := ln.config["x_forwarded_for_hop_skips"].(int) + authzdAddrs := ln.config["x_forwarded_for_authorized_addrs"].([]*sockaddr.SockAddrMarshaler) + rejectNotPresent := ln.config["x_forwarded_for_reject_not_present"].(bool) + rejectNonAuthz := ln.config["x_forwarded_for_reject_not_authorized"].(bool) if len(authzdAddrs) > 0 { handler = vaulthttp.WrapForwardedForHandler(handler, authzdAddrs, rejectNotPresent, rejectNonAuthz, hopSkips) } diff --git a/command/server/config.go b/command/server/config.go index eb5f9fc36e32..21520ce094fc 100644 --- a/command/server/config.go +++ b/command/server/config.go @@ -761,10 +761,10 @@ func parseListeners(result *Config, list *ast.ObjectList) error { "address", "cluster_address", "endpoint", - "forwarded_for_authorized_addrs", - "forwarded_for_hop_skips", - "forwarded_for_reject_not_authorized", - "forwarded_for_reject_not_present", + "x_forwarded_for_authorized_addrs", + "x_forwarded_for_hop_skips", + "x_forwarded_for_reject_not_authorized", + "x_forwarded_for_reject_not_present", "infrastructure", "node_id", "proxy_protocol_behavior", diff --git a/command/server/listener_tcp.go b/command/server/listener_tcp.go index 8a6e6fdc1508..44151521f975 100644 --- a/command/server/listener_tcp.go +++ b/command/server/listener_tcp.go @@ -44,54 +44,54 @@ func tcpListenerFactory(config map[string]interface{}, _ io.Writer, ui cli.Ui) ( props := map[string]string{"addr": addr} - ffAllowedRaw, ffAllowedOK := config["forwarded_for_authorized_addrs"] + ffAllowedRaw, ffAllowedOK := config["x_forwarded_for_authorized_addrs"] if ffAllowedOK { ffAllowed, err := parseutil.ParseAddrs(ffAllowedRaw) if err != nil { - return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_authorized_addrs\": {{err}}", err) + return nil, nil, nil, errwrap.Wrapf("error parsing \"x_forwarded_for_authorized_addrs\": {{err}}", err) } - props["forwarded_for_authorized_addrs"] = fmt.Sprintf("%v", ffAllowed) - config["forwarded_for_authorized_addrs"] = ffAllowed + props["x_forwarded_for_authorized_addrs"] = fmt.Sprintf("%v", ffAllowed) + config["x_forwarded_for_authorized_addrs"] = ffAllowed } - if ffHopsRaw, ok := config["forwarded_for_hop_skips"]; ok { + if ffHopsRaw, ok := config["x_forwarded_for_hop_skips"]; ok { ffHops, err := parseutil.ParseInt(ffHopsRaw) if err != nil { - return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_hop_skips\": {{err}}", err) + return nil, nil, nil, errwrap.Wrapf("error parsing \"x_forwarded_for_hop_skips\": {{err}}", err) } if ffHops < 0 { - return nil, nil, nil, fmt.Errorf("\"forwarded_for_hop_skips\" cannot be negative") + return nil, nil, nil, fmt.Errorf("\"x_forwarded_for_hop_skips\" cannot be negative") } - props["forwarded_for_hop_skips"] = strconv.Itoa(int(ffHops)) - config["forwarded_for_hop_skips"] = ffHops + props["x_forwarded_for_hop_skips"] = strconv.Itoa(int(ffHops)) + config["x_forwarded_for_hop_skips"] = ffHops } else if ffAllowedOK { ffHops := 0 - props["forwarded_for_hop_skips"] = "0" - config["forwarded_for_hop_skips"] = int(ffHops) + props["x_forwarded_for_hop_skips"] = "0" + config["x_forwarded_for_hop_skips"] = int(ffHops) } - if ffRejectNotPresentRaw, ok := config["forwarded_for_reject_not_present"]; ok { + if ffRejectNotPresentRaw, ok := config["x_forwarded_for_reject_not_present"]; ok { ffRejectNotPresent, err := parseutil.ParseBool(ffRejectNotPresentRaw) if err != nil { - return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_reject_not_present\": {{err}}", err) + return nil, nil, nil, errwrap.Wrapf("error parsing \"x_forwarded_for_reject_not_present\": {{err}}", err) } - props["forwarded_for_reject_not_present"] = strconv.FormatBool(ffRejectNotPresent) - config["forwarded_for_reject_not_present"] = ffRejectNotPresent + props["x_forwarded_for_reject_not_present"] = strconv.FormatBool(ffRejectNotPresent) + config["x_forwarded_for_reject_not_present"] = ffRejectNotPresent } else if ffAllowedOK { - props["forwarded_for_reject_not_present"] = "true" - config["forwarded_for_reject_not_present"] = true + props["x_forwarded_for_reject_not_present"] = "true" + config["x_forwarded_for_reject_not_present"] = true } - if ffRejectNonAuthorizedRaw, ok := config["forwarded_for_reject_not_authorized"]; ok { + if ffRejectNonAuthorizedRaw, ok := config["x_forwarded_for_reject_not_authorized"]; ok { ffRejectNonAuthorized, err := parseutil.ParseBool(ffRejectNonAuthorizedRaw) if err != nil { - return nil, nil, nil, errwrap.Wrapf("error parsing \"forwarded_for_reject_not_authorized\": {{err}}", err) + return nil, nil, nil, errwrap.Wrapf("error parsing \"x_forwarded_for_reject_not_authorized\": {{err}}", err) } - props["forwarded_for_reject_not_authorized"] = strconv.FormatBool(ffRejectNonAuthorized) - config["forwarded_for_reject_not_authorized"] = ffRejectNonAuthorized + props["x_forwarded_for_reject_not_authorized"] = strconv.FormatBool(ffRejectNonAuthorized) + config["x_forwarded_for_reject_not_authorized"] = ffRejectNonAuthorized } else if ffAllowedOK { - props["forwarded_for_reject_not_authorized"] = "true" - config["forwarded_for_reject_not_authorized"] = true + props["x_forwarded_for_reject_not_authorized"] = "true" + config["x_forwarded_for_reject_not_authorized"] = true } return listenerWrapTLS(ln, props, config, ui) From 6f9caf29d7a7fb60c6e58a663947c4297b804660 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 18:02:52 -0400 Subject: [PATCH 7/9] Address review feedback --- http/forwarded_for_test.go | 4 +--- http/handler.go | 15 ++++++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/http/forwarded_for_test.go b/http/forwarded_for_test.go index 22106d14ab73..5d60391353d1 100644 --- a/http/forwarded_for_test.go +++ b/http/forwarded_for_test.go @@ -233,8 +233,7 @@ func TestHandler_XForwardedFor(t *testing.T) { req := client.NewRequest("GET", "/") req.Headers = make(http.Header) req.Headers.Add("x-forwarded-for", "2.3.4.5") - req.Headers.Add("x-forwarded-for", "3.4.5.6") - req.Headers.Add("x-forwarded-for", "4.5.6.7") + req.Headers.Add("x-forwarded-for", "3.4.5.6,4.5.6.7") req.Headers.Add("x-forwarded-for", "5.6.7.8") resp, err := client.RawRequest(req) if err != nil { @@ -247,5 +246,4 @@ func TestHandler_XForwardedFor(t *testing.T) { t.Fatalf("bad body: %s", buf.String()) } }) - } diff --git a/http/handler.go b/http/handler.go index 3573a352a8d0..72294b2bdad1 100644 --- a/http/handler.go +++ b/http/handler.go @@ -185,14 +185,15 @@ func WrapForwardedForHandler(h http.Handler, authorizedAddrs []*sockaddr.SockAdd // Split comma separated ones, which are common. This brings it in line // to the multiple-header case. - if len(headers) == 1 { - headers = strings.Split(headers[0], ",") - } - for i, v := range headers { - headers[i] = strings.TrimSpace(v) + var acc []string + for _, header := range headers { + vals := strings.Split(header, ",") + for _, v := range vals { + acc = append(acc, strings.TrimSpace(v)) + } } - indexToUse := len(headers) - 1 - hopSkips + indexToUse := len(acc) - 1 - hopSkips if indexToUse < 0 { // This is likely an error in either configuration or other // infrastructure. We could either deny the request, or we @@ -208,7 +209,7 @@ func WrapForwardedForHandler(h http.Handler, authorizedAddrs []*sockaddr.SockAdd return } - r.RemoteAddr = net.JoinHostPort(headers[indexToUse], port) + r.RemoteAddr = net.JoinHostPort(acc[indexToUse], port) h.ServeHTTP(w, r) return }) From 72b3f4056988bad8ae719bddff7499f8ec53a15c Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 18:36:51 -0400 Subject: [PATCH 8/9] Fix interface conversion panic --- command/server/listener_tcp.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/command/server/listener_tcp.go b/command/server/listener_tcp.go index 44151521f975..201e124f3aae 100644 --- a/command/server/listener_tcp.go +++ b/command/server/listener_tcp.go @@ -55,19 +55,19 @@ func tcpListenerFactory(config map[string]interface{}, _ io.Writer, ui cli.Ui) ( } if ffHopsRaw, ok := config["x_forwarded_for_hop_skips"]; ok { - ffHops, err := parseutil.ParseInt(ffHopsRaw) + ffHops64, err := parseutil.ParseInt(ffHopsRaw) if err != nil { return nil, nil, nil, errwrap.Wrapf("error parsing \"x_forwarded_for_hop_skips\": {{err}}", err) } - if ffHops < 0 { + if ffHops64 < 0 { return nil, nil, nil, fmt.Errorf("\"x_forwarded_for_hop_skips\" cannot be negative") } - props["x_forwarded_for_hop_skips"] = strconv.Itoa(int(ffHops)) + ffHops := int(ffHops64) + props["x_forwarded_for_hop_skips"] = strconv.Itoa(ffHops) config["x_forwarded_for_hop_skips"] = ffHops } else if ffAllowedOK { - ffHops := 0 props["x_forwarded_for_hop_skips"] = "0" - config["x_forwarded_for_hop_skips"] = int(ffHops) + config["x_forwarded_for_hop_skips"] = int(0) } if ffRejectNotPresentRaw, ok := config["x_forwarded_for_reject_not_present"]; ok { From 92e7cf84d4dfb36ebb7d1361163b38391d11e50d Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 17 Apr 2018 18:51:38 -0400 Subject: [PATCH 9/9] Add website docs --- .../docs/configuration/listener/tcp.html.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/website/source/docs/configuration/listener/tcp.html.md b/website/source/docs/configuration/listener/tcp.html.md index 45d70a982e69..ff539fddc02a 100644 --- a/website/source/docs/configuration/listener/tcp.html.md +++ b/website/source/docs/configuration/listener/tcp.html.md @@ -85,6 +85,25 @@ listener "tcp" { authentication for this listener. The default behavior (when this is false) is for Vault to request client certificates when available. +- `x_forwarded_for_authorized_addrs` `(string: )` – + Specifies the list of source IP addresses for which an X-Forwarded-For header + will be trusted. Comma-separated list or JSON array. This turns on + X-Forwarded-For support. + +- `x_forwarded_for_hop_skips` `(string: "0")` – The number of addresses that will be + skipped from the *rear* of the set of hops. For instance, for a header value + of `1.2.3.4, 2.3.4.5, 3.4.5.6`, if this value is set to `"1"`, the address that + will be used as the originating client IP is `2.3.4.5`. + +- `x_forwarded_for_reject_not_authorized` `(string: "true")` – If set false, + if there is an X-Forwarded-For header in a connection from an unauthorized + address, the header will be ignored and the client connection used as-is, + rather than the client connection rejected. + +- `x_forwarded_for_reject_not_present` `(string: "true")` – If set false, if + there is no X-Forwarded-For header or it is empty, the client address will be + used as-is, rather than the client connection rejected. + ## `tcp` Listener Examples ### Configuring TLS