requestclient: use a non-spoofable ForwardedFor for dot-com (#50979)

We rely on the X-Forwarded-For header in our access logs (and soon our
rate limiter). However, this field can be spoofed. For sourcegraph.com
we can use a more reliable field since we know we have cloudflare in
front.

Test Plan: CI and confirming my headers at https://sourcegraph.com/-/debug/headers

cmd/frontend/internal/cli/http.go

@@ -67,7 +67,7 @@ func newExternalHTTPHandler(
 	// origins, to avoid CSRF attacks. See session.CookieMiddlewareWithCSRFSafety for details.
 	apiHandler = session.CookieMiddlewareWithCSRFSafety(logger, db, apiHandler, corsAllowHeader, isTrustedOrigin) // API accepts cookies with special header
 	apiHandler = internalhttpapi.AccessTokenAuthMiddleware(db, logger, apiHandler)                                // API accepts access tokens
-	apiHandler = requestclient.HTTPMiddleware(apiHandler)
+	apiHandler = requestclient.ExternalHTTPMiddleware(apiHandler, envvar.SourcegraphDotComMode())
 	apiHandler = gziphandler.GzipHandler(apiHandler)
 	if envvar.SourcegraphDotComMode() {
 		apiHandler = deviceid.Middleware(apiHandler)
@@ -90,7 +90,7 @@ func newExternalHTTPHandler(
 	appHandler = middleware.OpenGraphMetadataMiddleware(db.FeatureFlags(), appHandler)
 	appHandler = session.CookieMiddleware(logger, db, appHandler)                  // app accepts cookies
 	appHandler = internalhttpapi.AccessTokenAuthMiddleware(db, logger, appHandler) // app accepts access tokens
-	appHandler = requestclient.HTTPMiddleware(appHandler)
+	appHandler = requestclient.ExternalHTTPMiddleware(appHandler, envvar.SourcegraphDotComMode())
 	if envvar.SourcegraphDotComMode() {
 		appHandler = deviceid.Middleware(appHandler)
 	}

cmd/gitserver/shared/shared.go

@@ -167,7 +167,7 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic
 	// TODO: Why do we set server state as a side effect of creating our handler?
 	handler := gitserver.Handler()
 	handler = actor.HTTPMiddleware(logger, handler)
-	handler = requestclient.HTTPMiddleware(handler)
+	handler = requestclient.InternalHTTPMiddleware(handler)
 	handler = trace.HTTPMiddleware(logger, handler, conf.DefaultClient())
 	handler = instrumentation.HTTPMiddleware("", handler)
 	handler = internalgrpc.MultiplexHandlers(grpcServer, handler)

internal/requestclient/client.go

@@ -11,6 +11,9 @@ type Client struct {
 	// IP identifies the IP of the client.
 	IP string
 	// ForwardedFor identifies the originating IP address of a client.
+	//
+	// Note: This header can be spoofed and relies on trusted clients/proxies.
+	// For sourcegraph.com we use cloudflare headers to avoid spoofing.
 	ForwardedFor string
 }
 

internal/requestclient/http.go

@@ -36,10 +36,43 @@ func (t *HTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	return t.RoundTripper.RoundTrip(req)
 }
 
-// HTTPMiddleware wraps the given handle func and attaches client IP data indicated in
+// ExternalHTTPMiddleware wraps the given handle func and attaches client IP
+// data indicated in incoming requests to the request header.
+//
+// This is meant to be used by http handlers which sit behind a reverse proxy
+// receiving user traffic. IE sourcegraph-frontend.
+func ExternalHTTPMiddleware(next http.Handler, isDotCom bool) http.Handler {
+	return httpMiddleware(next, isDotCom)
+}
+
+// InternalHTTPMiddleware wraps the given handle func and attaches client IP
+// data indicated in incoming requests to the request header.
+//
+// This is meant to be used by http handlers which receive internal traffic.
+// EG gitserver.
+func InternalHTTPMiddleware(next http.Handler) http.Handler {
+	return httpMiddleware(next, false)
+}
+
+// httpMiddleware wraps the given handle func and attaches client IP data indicated in
 // incoming requests to the request header.
-func HTTPMiddleware(next http.Handler) http.Handler {
+func httpMiddleware(next http.Handler, isDotCom bool) http.Handler {
+	forwardedForHeaders := []string{headerKeyForwardedFor}
+	if isDotCom {
+		// On Sourcegraph.com we have a more reliable header from cloudflare,
+		// since x-forwarded-for can be spoofed. So use that if available.
+		forwardedForHeaders = []string{"Cf-Connecting-Ip", headerKeyForwardedFor}
+	}
+
 	return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		forwardedFor := ""
+		for _, k := range forwardedForHeaders {
+			forwardedFor = req.Header.Get(k)
+			if forwardedFor != "" {
+				break
+			}
+		}
+
 		ctxWithClient := WithClient(req.Context(), &Client{
 			IP:           strings.Split(req.RemoteAddr, ":")[0],
 			ForwardedFor: req.Header.Get(headerKeyForwardedFor),