From c6fdfc9cd5c54751b28e82199d87e82fab9adaf2 Mon Sep 17 00:00:00 2001 From: Richard Date: Tue, 24 Mar 2015 16:46:08 -0700 Subject: [PATCH 1/3] Attempt to identify remote IP addresses for requests which come through proxies. Add a function to examine X-Forward-For and X-Real-Ip headers for originating IP addresses. Use RemoteAddr for notification request record and HTTP request context. --- context/http.go | 25 +++++++++++++++++++++- context/http_test.go | 47 +++++++++++++++++++++++++++++++++++++++++ notifications/bridge.go | 4 +++- 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/context/http.go b/context/http.go index 357f0dc3..01600c35 100644 --- a/context/http.go +++ b/context/http.go @@ -17,11 +17,34 @@ 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. +func RemoteAddr(r *http.Request) string { + remoteAddr := r.RemoteAddr + + if prior, ok := r.Header["X-Forwarded-For"]; ok { + proxies := strings.Split(prior[0], ",") + if len(proxies) > 0 { + remoteAddr = strings.Trim(proxies[0], " ") + } + } else if realip, ok := r.Header["X-Real-Ip"]; ok { + if len(realip) > 0 { + remoteAddr = realip[0] + } + } + + return remoteAddr +} + // WithRequest places the request on the context. The context of the request // is assigned a unique id, available at "http.request.id". The request itself // 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 @@ -147,7 +170,7 @@ func (ctx *httpRequestContext) Value(key interface{}) interface{} { case "uri": return ctx.r.RequestURI case "remoteaddr": - return ctx.r.RemoteAddr + return RemoteAddr(ctx.r) case "method": return ctx.r.Method case "host": diff --git a/context/http_test.go b/context/http_test.go index df3734e8..1fee1761 100644 --- a/context/http_test.go +++ b/context/http_test.go @@ -2,6 +2,9 @@ package context import ( "net/http" + "net/http/httptest" + "net/http/httputil" + "net/url" "reflect" "testing" "time" @@ -205,3 +208,47 @@ func TestWithVars(t *testing.T) { } } } + +// SingleHostReverseProxy will insert an X-Forwarded-For header, and can be used to test +// RemoteAddr(). A fake RemoteAddr cannot be set on the HTTP request - it is overwritten +// 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 + 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) + + if expectedRemote != actualRemote { + t.Errorf("Mismatching remote hosts: %v != %v", expectedRemote, actualRemote) + } + + w.WriteHeader(200) + })) + + defer backend.Close() + backendURL, err := url.Parse(backend.URL) + if err != nil { + t.Fatal(err) + } + + proxy := httputil.NewSingleHostReverseProxy(backendURL) + frontend := httptest.NewServer(proxy) + defer frontend.Close() + + getReq, err := http.NewRequest("GET", frontend.URL, nil) + if err != nil { + t.Fatal(err) + } + + _, err = http.DefaultClient.Do(getReq) + if err != nil { + t.Fatal(err) + } + +} diff --git a/notifications/bridge.go b/notifications/bridge.go index 21d2105d..48a2bd5b 100644 --- a/notifications/bridge.go +++ b/notifications/bridge.go @@ -6,6 +6,7 @@ import ( "code.google.com/p/go-uuid/uuid" "github.com/docker/distribution" + "github.com/docker/distribution/context" "github.com/docker/distribution/digest" "github.com/docker/distribution/manifest" ) @@ -42,10 +43,11 @@ 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, - Addr: r.RemoteAddr, + Addr: context.RemoteAddr(r), Host: r.Host, Method: r.Method, UserAgent: r.UserAgent(), From e21a425f88c7eb24a7f48a877122edecd03bf2b8 Mon Sep 17 00:00:00 2001 From: Richard Date: Wed, 25 Mar 2015 11:11:46 -0700 Subject: [PATCH 2/3] 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, From 78562258b2ae2cf2f83b8413cf53309643a6b0e7 Mon Sep 17 00:00:00 2001 From: Richard Date: Wed, 25 Mar 2015 13:44:16 -0700 Subject: [PATCH 3/3] Log invalid remote IPs --- context/http.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/context/http.go b/context/http.go index e92a145a..4f0a7b8a 100644 --- a/context/http.go +++ b/context/http.go @@ -9,6 +9,7 @@ import ( "time" "code.google.com/p/go-uuid/uuid" + log "github.com/Sirupsen/logrus" "github.com/gorilla/mux" "golang.org/x/net/context" ) @@ -18,6 +19,14 @@ var ( ErrNoRequestContext = errors.New("no http request in context") ) +func parseIP(ipStr string) net.IP { + ip := net.ParseIP(ipStr) + if ip == nil { + log.Warnf("invalid remote IP address: %q", ipStr) + } + return ip +} + // RemoteAddr extracts the remote address of the request, taking into // account proxy headers. func RemoteAddr(r *http.Request) string { @@ -25,7 +34,7 @@ func RemoteAddr(r *http.Request) string { proxies := strings.Split(prior, ",") if len(proxies) > 0 { remoteAddr := strings.Trim(proxies[0], " ") - if net.ParseIP(remoteAddr) != nil { + if parseIP(remoteAddr) != nil { return remoteAddr } } @@ -33,7 +42,7 @@ func RemoteAddr(r *http.Request) string { // 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 { + if parseIP(realIP) != nil { return realIP } }