fix: Work around gRPC-Gateway bug in X-Forwarded-For handling (#2152)

This PR primarily aims to work around an issue with `X-Forwarded-For`
handling in the gRPC-Gateway
(grpc-ecosystem/grpc-gateway#4320).

Given a remote IP of 4.4.4.4 sending incoming headers of 

```
X-Forwarded-For: 1.1.1.1, 2.2.2.2
X-Forwarded-For: 3.3.3.3
```

the resulting headers forwarded by the gRPC-gateway are

```
X-Forwarded-For: 1.1.1.1, 2.2.2.2
X-Forwarded-For: 3.3.3.3
X-Forwarded-For: 1.1.1.1, 2.2.2.2, 4.4.4.4
```

The workaround I have added means that the resulting headers are

```
X-Forwarded-For: 1.1.1.1, 2.2.2.2
X-Forwarded-For: 3.3.3.3
X-Forwarded-For: 4.4.4.4
```

We now join the values with `", "` rather than `"|"` to preserve the
[de-facto standard
syntax](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#syntax).

This PR also prevents spoofing the remote address in the audit logs by
setting the `x-cerbos-http-remote-addr` header; currently we take that
header into account even for requests not originating from the
gRPC-Gateway where it should be set. Even in the case of requests from
the gRPC-Gateway, our header is appended to the request headers, so we
need to make sure to use the last value for the header, not the first.

Signed-off-by: Andrew Haines <haines@cerbos.dev>

internal/audit/context.go

@@ -5,6 +5,10 @@ package audit
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/subtle"
+	"encoding/base64"
+	"fmt"
 	"strings"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/logging"
@@ -16,15 +20,21 @@ import (
 )
 
 const (
-	delimiter          = "|"
 	grpcGWUserAgentKey = "grpcgateway-user-agent"
 	userAgentKey       = "user-agent"
 	xffKey             = "x-forwarded-for"
 	callIDTagKey       = "call_id"
 
-	HTTPRemoteAddrKey = "x-cerbos-http-remote-addr"
+	SetByGRPCGatewayKey = "x-cerbos-set-by-grpc-gateway"
+	HTTPRemoteAddrKey   = "x-cerbos-http-remote-addr"
 )
 
+var SetByGRPCGatewayVal string
+
+func init() {
+	SetByGRPCGatewayVal = generateSetByGRPCGatewayVal()
+}
+
 type callIDCtxKeyType struct{}
 
 var callIDCtxKey = callIDCtxKeyType{}
@@ -51,22 +61,32 @@ func CallIDFromContext(ctx context.Context) (ID, bool) {
 func PeerFromContext(ctx context.Context) *auditv1.Peer {
 	p := peerFromContext(ctx)
 	md, ok := metadata.FromIncomingContext(ctx)
-	if ok {
-		if addr := md.Get(HTTPRemoteAddrKey); len(addr) > 0 {
-			p.Address = addr[0]
-		}
+	if !ok {
+		return p
+	}
+
+	setByGateway := checkSetByGRPCGateway(md)
 
-		if ua, ok := md[grpcGWUserAgentKey]; ok {
-			p.UserAgent = strings.Join(ua, delimiter)
-		} else if ua, ok := md[userAgentKey]; ok {
-			p.UserAgent = strings.Join(ua, delimiter)
+	var ua []string
+	xff := md[xffKey]
+	if setByGateway {
+		if addr := md[HTTPRemoteAddrKey]; len(addr) > 0 {
+			p.Address = addr[len(addr)-1]
 		}
 
-		if xff, ok := md[xffKey]; ok {
-			p.ForwardedFor = strings.Join(xff, delimiter)
+		ua = md[grpcGWUserAgentKey]
+
+		// https://github.com/grpc-ecosystem/grpc-gateway/issues/4320
+		if len(xff) > 1 {
+			xff = append(xff[:len(xff)-1], strings.TrimPrefix(xff[len(xff)-1], xff[0]+", "))
 		}
+	} else {
+		ua = md[userAgentKey]
 	}
 
+	p.UserAgent = strings.Join(ua, "|")
+	p.ForwardedFor = strings.Join(xff, ", ")
+
 	return p
 }
 
@@ -83,3 +103,18 @@ func peerFromContext(ctx context.Context) *auditv1.Peer {
 
 	return pp
 }
+
+func generateSetByGRPCGatewayVal() string {
+	const n = 32
+	b := make([]byte, n)
+	_, err := rand.Read(b)
+	if err != nil {
+		panic(fmt.Errorf("failed to generate %s header value: %w", SetByGRPCGatewayKey, err))
+	}
+	return base64.StdEncoding.EncodeToString(b)
+}
+
+func checkSetByGRPCGateway(md metadata.MD) bool {
+	v := md[SetByGRPCGatewayKey]
+	return len(v) > 0 && subtle.ConstantTimeCompare([]byte(v[len(v)-1]), []byte(SetByGRPCGatewayVal)) == 1
+}

internal/server/headers_test.go

@@ -0,0 +1,65 @@
+// Copyright 2021-2024 Zenauth Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+package server
+
+import (
+	"context"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/cerbos/cerbos/internal/audit"
+	gateway "github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/metadata"
+	"google.golang.org/grpc/peer"
+)
+
+func TestPeerFromContext(t *testing.T) {
+	t.Run("gRPC", func(t *testing.T) {
+		ctx := peer.NewContext(
+			metadata.NewIncomingContext(context.Background(), metadata.Pairs(
+				audit.HTTPRemoteAddrKey, "attempted spoof",
+				audit.SetByGRPCGatewayKey, "attempted spoof",
+				"User-Agent", "peer-from-context",
+				"X-Forwarded-For", "1.1.1.1, 2.2.2.2",
+				"X-Forwarded-For", "3.3.3.3",
+			)),
+			&peer.Peer{Addr: peerAddr("4.4.4.4:12345")},
+		)
+
+		p := audit.PeerFromContext(ctx)
+		assert.Equal(t, "4.4.4.4:12345", p.Address)
+		assert.Equal(t, "1.1.1.1, 2.2.2.2, 3.3.3.3", p.ForwardedFor)
+		assert.Equal(t, "peer-from-context", p.UserAgent)
+	})
+
+	t.Run("HTTP", func(t *testing.T) {
+		req := httptest.NewRequest("GET", "/", nil)
+		req.Header.Set(audit.HTTPRemoteAddrKey, "attempted spoof")
+		req.Header.Set(audit.SetByGRPCGatewayKey, "attempted spoof")
+		req.Header.Set("User-Agent", "peer-from-context")
+		req.Header.Add("X-Forwarded-For", "1.1.1.1, 2.2.2.2")
+		req.Header.Add("X-Forwarded-For", "3.3.3.3")
+		req.RemoteAddr = "4.4.4.4:12345"
+
+		ctx, err := gateway.AnnotateIncomingContext(context.Background(), mkGatewayMux(nil), req, "example.Service/Method")
+		require.NoError(t, err)
+
+		peer := audit.PeerFromContext(ctx)
+		assert.Equal(t, "4.4.4.4:12345", peer.Address)
+		assert.Equal(t, "1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4", peer.ForwardedFor)
+		assert.Equal(t, "peer-from-context", peer.UserAgent)
+	})
+}
+
+type peerAddr string
+
+func (peerAddr) Network() string {
+	return "tcp"
+}
+
+func (a peerAddr) String() string {
+	return string(a)
+}

internal/server/server.go

@@ -471,20 +471,7 @@ func (s *Server) startHTTPServer(ctx context.Context, l net.Listener, grpcSrv *g
 		return nil, err
 	}
 
-	gwmux := runtime.NewServeMux(
-		runtime.WithForwardResponseOption(customHTTPResponseCode),
-		runtime.WithIncomingHeaderMatcher(incomingHeaderMatcher),
-		runtime.WithMarshalerOption("application/json+pretty", &runtime.JSONPb{
-			MarshalOptions:   protojson.MarshalOptions{Indent: "  "},
-			UnmarshalOptions: protojson.UnmarshalOptions{DiscardUnknown: false},
-		}),
-		runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.JSONPb{
-			UnmarshalOptions: protojson.UnmarshalOptions{DiscardUnknown: false},
-		}),
-		runtime.WithMetadata(setPeerMetadata),
-		runtime.WithRoutingErrorHandler(handleRoutingError),
-		runtime.WithHealthEndpointAt(healthpb.NewHealthClient(grpcConn), healthEndpoint),
-	)
+	gwmux := mkGatewayMux(grpcConn)
 
 	if err := svcv1.RegisterCerbosServiceHandler(ctx, gwmux, grpcConn); err != nil {
 		log.Errorw("Failed to register Cerbos HTTP service", "error", err)
@@ -553,6 +540,23 @@ func (s *Server) startHTTPServer(ctx context.Context, l net.Listener, grpcSrv *g
 	return h, nil
 }
 
+func mkGatewayMux(grpcConn grpc.ClientConnInterface) *runtime.ServeMux {
+	return runtime.NewServeMux(
+		runtime.WithForwardResponseOption(customHTTPResponseCode),
+		runtime.WithIncomingHeaderMatcher(incomingHeaderMatcher),
+		runtime.WithMarshalerOption("application/json+pretty", &runtime.JSONPb{
+			MarshalOptions:   protojson.MarshalOptions{Indent: "  "},
+			UnmarshalOptions: protojson.UnmarshalOptions{DiscardUnknown: false},
+		}),
+		runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.JSONPb{
+			UnmarshalOptions: protojson.UnmarshalOptions{DiscardUnknown: false},
+		}),
+		runtime.WithMetadata(setPeerMetadata),
+		runtime.WithRoutingErrorHandler(handleRoutingError),
+		runtime.WithHealthEndpointAt(healthpb.NewHealthClient(grpcConn), healthEndpoint),
+	)
+}
+
 func defaultGRPCDialOpts() []grpc.DialOption {
 	// see https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md
 	return []grpc.DialOption{
@@ -655,5 +659,8 @@ func incomingHeaderMatcher(key string) (string, bool) {
 }
 
 func setPeerMetadata(_ context.Context, req *http.Request) metadata.MD {
-	return metadata.Pairs(audit.HTTPRemoteAddrKey, req.RemoteAddr)
+	return metadata.Pairs(
+		audit.SetByGRPCGatewayKey, audit.SetByGRPCGatewayVal,
+		audit.HTTPRemoteAddrKey, req.RemoteAddr,
+	)
 }