Use clientip.Extract for esign and session IP capture

Several HTTP entry points still parsed RemoteAddr directly, so behind
a layer-7 proxy they recorded the load balancer IP instead of the
signer's. Route NDA acceptance, signing events, document sign/approve,
and session updates through clientip.Extract, which honors Forwarded
and X-Forwarded-For when trustedproxy allows them.

Signed-off-by: Ludovic Vielle <ludovic@probo.com>

pkg/server/api/authn/session_middleware.go

@@ -27,6 +27,7 @@ import (
 	"go.probo.inc/probo/pkg/gid"
 	"go.probo.inc/probo/pkg/iam"
 	"go.probo.inc/probo/pkg/securecookie"
+	"go.probo.inc/probo/pkg/server/api/clientip"
 	"go.probo.inc/probo/pkg/server/gqlutils"
 )
 
@@ -97,13 +98,7 @@ func NewSessionMiddleware(svc *iam.Service, cookieConfig securecookie.Config) fu
 				}
 
 				userAgent := r.UserAgent()
-				// TODO: will work well when no layer 7 proxy is in front of the server
-				var ipAddress net.IP
-				if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil {
-					ipAddress = net.ParseIP(host)
-				} else {
-					ipAddress = net.ParseIP(r.RemoteAddr)
-				}
+				ipAddress := net.ParseIP(clientip.Extract(r))
 
 				err = svc.SessionService.UpdateSessionInfo(ctx, session.ID, userAgent, ipAddress)
 				if err != nil {

pkg/server/api/clientip/clientip.go

@@ -27,28 +27,38 @@ import (
 // load balancer closest to us.
 func Extract(r *http.Request) string {
 	if fwd := r.Header.Get("Forwarded"); fwd != "" {
-		if ip := parseForwardedFor(fwd); ip != "" {
+		if ip := parseForwardedFor(fwd); net.ParseIP(ip) != nil {
 			return ip
 		}
 	}
 
 	if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
-		if i := strings.LastIndexByte(xff, ','); i != -1 {
-			xff = xff[i+1:]
-		}
-
-		xff = strings.TrimSpace(xff)
-
-		if ip, _, err := net.SplitHostPort(xff); err == nil {
+		if ip := parseXForwardedFor(xff); net.ParseIP(ip) != nil {
 			return ip
 		}
-
-		return xff
 	}
 
-	ip, _, err := net.SplitHostPort(r.RemoteAddr)
+	return extractRemoteAddr(r.RemoteAddr)
+}
+
+func parseXForwardedFor(xff string) string {
+	if i := strings.LastIndexByte(xff, ','); i != -1 {
+		xff = xff[i+1:]
+	}
+
+	xff = strings.TrimSpace(xff)
+
+	if ip, _, err := net.SplitHostPort(xff); err == nil {
+		return ip
+	}
+
+	return xff
+}
+
+func extractRemoteAddr(remoteAddr string) string {
+	ip, _, err := net.SplitHostPort(remoteAddr)
 	if err != nil {
-		return r.RemoteAddr
+		return remoteAddr
 	}
 
 	return ip

pkg/server/api/clientip/clientip_test.go

@@ -110,6 +110,27 @@ func TestExtract(t *testing.T) {
 			headers:    map[string]string{"Forwarded": "for=198.51.100.17;proto=https;by=203.0.113.60"},
 			want:       "198.51.100.17",
 		},
+		{
+			name:       "unparseable forwarded falls back to remote addr",
+			remoteAddr: "10.0.0.1:1234",
+			headers:    map[string]string{"Forwarded": "for=unknown"},
+			want:       "10.0.0.1",
+		},
+		{
+			name:       "unparseable x-forwarded-for falls back to remote addr",
+			remoteAddr: "10.0.0.1:1234",
+			headers:    map[string]string{"X-Forwarded-For": "unknown"},
+			want:       "10.0.0.1",
+		},
+		{
+			name:       "unparseable forwarded falls through to x-forwarded-for",
+			remoteAddr: "10.0.0.1:1234",
+			headers: map[string]string{
+				"Forwarded":       "for=unknown",
+				"X-Forwarded-For": "203.0.113.50",
+			},
+			want: "203.0.113.50",
+		},
 	}
 
 	for _, tt := range tests {

pkg/server/api/console/v1/document_resolvers.go

@@ -10,7 +10,6 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
-	"net"
 
 	"github.com/vikstrous/dataloadgen"
 	"go.gearno.de/kit/log"
@@ -21,6 +20,7 @@ import (
 	"go.probo.inc/probo/pkg/probo"
 	"go.probo.inc/probo/pkg/resourcealias"
 	"go.probo.inc/probo/pkg/server/api/authn"
+	"go.probo.inc/probo/pkg/server/api/clientip"
 	"go.probo.inc/probo/pkg/server/api/console/v1/dataloader"
 	"go.probo.inc/probo/pkg/server/api/console/v1/schema"
 	"go.probo.inc/probo/pkg/server/api/console/v1/types"
@@ -1433,10 +1433,7 @@ func (r *mutationResolver) SignDocument(ctx context.Context, input types.SignDoc
 	identity := authn.IdentityFromContext(ctx)
 	httpReq := gqlutils.HTTPRequestFromContext(ctx)
 
-	signerIP, _, _ := net.SplitHostPort(httpReq.RemoteAddr)
-	if signerIP == "" {
-		signerIP = httpReq.RemoteAddr
-	}
+	signerIP := clientip.Extract(httpReq)
 
 	documentVersionSignature, err := r.probo.Documents.SignDocumentVersionByIdentity(
 		ctx,
@@ -1487,10 +1484,7 @@ func (r *mutationResolver) ApproveDocumentVersion(ctx context.Context, input typ
 	identity := authn.IdentityFromContext(ctx)
 	httpReq := gqlutils.HTTPRequestFromContext(ctx)
 
-	signerIP, _, _ := net.SplitHostPort(httpReq.RemoteAddr)
-	if signerIP == "" {
-		signerIP = httpReq.RemoteAddr
-	}
+	signerIP := clientip.Extract(httpReq)
 
 	decision, err := r.probo.DocumentApprovals.Approve(ctx, scope, probo.ApproveDocumentVersionRequest{
 		DocumentVersionID: input.DocumentVersionID,

pkg/server/api/trust/v1/nda_resolvers.go

@@ -7,13 +7,13 @@ package trust_v1
 
 import (
 	"context"
-	"net"
 	"time"
 
 	"go.gearno.de/kit/log"
 	"go.probo.inc/probo/pkg/coredata"
 	"go.probo.inc/probo/pkg/esign"
 	"go.probo.inc/probo/pkg/server/api/authn"
+	"go.probo.inc/probo/pkg/server/api/clientip"
 	"go.probo.inc/probo/pkg/server/api/compliancepage"
 	"go.probo.inc/probo/pkg/server/api/trust/v1/schema"
 	"go.probo.inc/probo/pkg/server/api/trust/v1/types"
@@ -27,10 +27,7 @@ func (r *mutationResolver) AcceptElectronicSignature(ctx context.Context, input
 		httpReq  = gqlutils.HTTPRequestFromContext(ctx)
 	)
 
-	signerIP, _, _ := net.SplitHostPort(httpReq.RemoteAddr)
-	if signerIP == "" {
-		signerIP = httpReq.RemoteAddr
-	}
+	signerIP := clientip.Extract(httpReq)
 
 	signature, err := r.esign.AcceptSignature(
 		ctx,
@@ -59,10 +56,7 @@ func (r *mutationResolver) RecordSigningEvent(ctx context.Context, input types.R
 		httpReq  = gqlutils.HTTPRequestFromContext(ctx)
 	)
 
-	actorIP, _, _ := net.SplitHostPort(httpReq.RemoteAddr)
-	if actorIP == "" {
-		actorIP = httpReq.RemoteAddr
-	}
+	actorIP := clientip.Extract(httpReq)
 
 	if err := r.esign.RecordEvent(
 		ctx,