From e21a425f88c7eb24a7f48a877122edecd03bf2b8 Mon Sep 17 00:00:00 2001 From: Richard Date: Wed, 25 Mar 2015 11:11:46 -0700 Subject: [PATCH] Verify IP addresses Fix lint errors Add more test --- AUTHORS | 2 ++ context/http.go | 31 ++++++++++++++++--------------- context/http_test.go | 28 ++++++++++++++++++++++++---- notifications/bridge.go | 1 - 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/AUTHORS b/AUTHORS index b9bcad6a..749f0af7 100644 --- a/AUTHORS +++ b/AUTHORS @@ -13,6 +13,8 @@ Frederick F. Kautz IV Josh Hawn Nghia Tran Olivier Gambier +Richard +Shreyas Karnik Stephen J Day Tianon Gravi xiekeyang diff --git a/context/http.go b/context/http.go index 01600c35..e92a145a 100644 --- a/context/http.go +++ b/context/http.go @@ -2,6 +2,7 @@ package context import ( "errors" + "net" "net/http" "strings" "sync" @@ -17,26 +18,27 @@ var ( ErrNoRequestContext = errors.New("no http request in context") ) -// http.Request.RemoteAddr is not the originating IP if the request -// came through a reverse proxy. -// X-Forwarded-For and X-Real-IP provide this information with decreased -// support, so check in that order. There may be multiple 'X-Forwarded-For' -// headers, but go maps don't retain order so just pick one. +// RemoteAddr extracts the remote address of the request, taking into +// account proxy headers. func RemoteAddr(r *http.Request) string { - remoteAddr := r.RemoteAddr - - if prior, ok := r.Header["X-Forwarded-For"]; ok { - proxies := strings.Split(prior[0], ",") + if prior := r.Header.Get("X-Forwarded-For"); prior != "" { + proxies := strings.Split(prior, ",") if len(proxies) > 0 { - remoteAddr = strings.Trim(proxies[0], " ") + remoteAddr := strings.Trim(proxies[0], " ") + if net.ParseIP(remoteAddr) != nil { + return remoteAddr + } } - } else if realip, ok := r.Header["X-Real-Ip"]; ok { - if len(realip) > 0 { - remoteAddr = realip[0] + } + // X-Real-Ip is less supported, but worth checking in the + // absence of X-Forwarded-For + if realIP := r.Header.Get("X-Real-Ip"); realIP != "" { + if net.ParseIP(realIP) != nil { + return realIP } } - return remoteAddr + return r.RemoteAddr } // WithRequest places the request on the context. The context of the request @@ -44,7 +46,6 @@ func RemoteAddr(r *http.Request) string { // is available at "http.request". Other common attributes are available under // the prefix "http.request.". If a request is already present on the context, // this method will panic. - func WithRequest(ctx context.Context, r *http.Request) context.Context { if ctx.Value("http.request") != nil { // NOTE(stevvooe): This needs to be considered a programming error. It diff --git a/context/http_test.go b/context/http_test.go index 1fee1761..28d2720c 100644 --- a/context/http_test.go +++ b/context/http_test.go @@ -214,16 +214,15 @@ func TestWithVars(t *testing.T) { // at the transport layer to 127.0.0.1: . However, as the X-Forwarded-For header // just contains the IP address, it is different enough for testing. func TestRemoteAddr(t *testing.T) { - expectedRemote := "127.0.0.1" - var actualRemote string + var expectedRemote string backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { defer r.Body.Close() if r.RemoteAddr == expectedRemote { t.Errorf("Unexpected matching remote addresses") } - actualRemote = RemoteAddr(r) + actualRemote := RemoteAddr(r) if expectedRemote != actualRemote { t.Errorf("Mismatching remote hosts: %v != %v", expectedRemote, actualRemote) } @@ -241,14 +240,35 @@ func TestRemoteAddr(t *testing.T) { frontend := httptest.NewServer(proxy) defer frontend.Close() - getReq, err := http.NewRequest("GET", frontend.URL, nil) + // X-Forwarded-For set by proxy + expectedRemote = "127.0.0.1" + proxyReq, err := http.NewRequest("GET", frontend.URL, nil) if err != nil { t.Fatal(err) } + _, err = http.DefaultClient.Do(proxyReq) + if err != nil { + t.Fatal(err) + } + + // RemoteAddr in X-Real-Ip + getReq, err := http.NewRequest("GET", backend.URL, nil) + if err != nil { + t.Fatal(err) + } + + expectedRemote = "1.2.3.4" + getReq.Header["X-Real-ip"] = []string{expectedRemote} _, err = http.DefaultClient.Do(getReq) if err != nil { t.Fatal(err) } + // Valid X-Real-Ip and invalid X-Forwarded-For + getReq.Header["X-forwarded-for"] = []string{"1.2.3"} + _, err = http.DefaultClient.Do(getReq) + if err != nil { + t.Fatal(err) + } } diff --git a/notifications/bridge.go b/notifications/bridge.go index 48a2bd5b..baa90a5b 100644 --- a/notifications/bridge.go +++ b/notifications/bridge.go @@ -43,7 +43,6 @@ func NewBridge(ub URLBuilder, source SourceRecord, actor ActorRecord, request Re // NewRequestRecord builds a RequestRecord for use in NewBridge from an // http.Request, associating it with a request id. - func NewRequestRecord(id string, r *http.Request) RequestRecord { return RequestRecord{ ID: id,