Ignore source port in pack-objects limiting

After deployment, it turns out that remote IP baggage may consist of
source port. Technically, this inclusion is not wrong. Apparently, the
source ports are usually ephemeral ports of clients. It always rotates.
Hence, it breaks the pack-objects limiting in such cases.

This commit is to sanitize the remote IP before applying the limiter.

2 files changed, 83 insertions, 1 deletions

diff --git a/internal/gitaly/service/hook/pack_objects.go b/internal/gitaly/service/hook/pack_objects.go
index 27bfc39b8..6d2f1dc4c 100644
--- a/internal/gitaly/service/hook/pack_objects.go
+++ b/internal/gitaly/service/hook/pack_objects.go
@@ -108,12 +108,18 @@ func (s *server) packObjectsHook(ctx context.Context, req *gitalypb.PackObjectsH
 
 		if featureflag.PackObjectsLimitingRemoteIP.IsEnabled(ctx) && req.GetRemoteIp() != "" {
 			ipAddr := net.ParseIP(req.GetRemoteIp())
+			if ipAddr == nil {
+				// Best effort, maybe the remote IP includes source port
+				if ip, _, err := net.SplitHostPort(req.GetRemoteIp()); err == nil {
+					ipAddr = net.ParseIP(ip)
+				}
+			}
 			// Ignore loop-back IPs
 			if ipAddr != nil && !ipAddr.IsLoopback() {
 				return s.runPackObjectsLimited(
 					ctx,
 					w,
-					req.GetRemoteIp(),
+					ipAddr.String(),
 					req,
 					args,
 					stdin,
diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go
index d05de7458..ae4b9d992 100644
--- a/internal/gitaly/service/hook/pack_objects_test.go
+++ b/internal/gitaly/service/hook/pack_objects_test.go
@@ -743,6 +743,32 @@ func testPackObjectsConcurrency(t *testing.T, ctx context.Context) {
 				featureflag.PackObjectsLimitingRemoteIP.IsEnabled(ctx),
 		},
 		{
+			desc: "IP addresses including source port",
+			requests: [2]*gitalypb.PackObjectsHookWithSidechannelRequest{
+				{
+					GlId:     "user-123",
+					RemoteIp: "1.2.3.4:47293",
+					Repository: &gitalypb.Repository{
+						StorageName:  "storage-0",
+						RelativePath: "a/b/c",
+					},
+					Args: args,
+				},
+				{
+					GlId:     "user-123",
+					RemoteIp: "1.2.3.4:51010",
+					Repository: &gitalypb.Repository{
+						StorageName:  "storage-0",
+						RelativePath: "a/b/c",
+					},
+					Args: args,
+				},
+			},
+			shouldLimit: featureflag.PackObjectsLimitingRepo.IsEnabled(ctx) ||
+				featureflag.PackObjectsLimitingUser.IsEnabled(ctx) ||
+				featureflag.PackObjectsLimitingRemoteIP.IsEnabled(ctx),
+		},
+		{
 			desc: "IPv4 loopback addresses",
 			requests: [2]*gitalypb.PackObjectsHookWithSidechannelRequest{
 				{
@@ -792,6 +818,56 @@ func testPackObjectsConcurrency(t *testing.T, ctx context.Context) {
 			shouldLimit: featureflag.PackObjectsLimitingRepo.IsEnabled(ctx) ||
 				featureflag.PackObjectsLimitingUser.IsEnabled(ctx),
 		},
+		{
+			desc: "invalid IP addresses",
+			requests: [2]*gitalypb.PackObjectsHookWithSidechannelRequest{
+				{
+					GlId:     "user-123",
+					RemoteIp: "hello-world",
+					Repository: &gitalypb.Repository{
+						StorageName:  "storage-0",
+						RelativePath: "a/b/c",
+					},
+					Args: args,
+				},
+				{
+					GlId:     "user-123",
+					RemoteIp: "hello-world",
+					Repository: &gitalypb.Repository{
+						StorageName:  "storage-0",
+						RelativePath: "a/b/c",
+					},
+					Args: args,
+				},
+			},
+			shouldLimit: featureflag.PackObjectsLimitingRepo.IsEnabled(ctx) ||
+				featureflag.PackObjectsLimitingUser.IsEnabled(ctx),
+		},
+		{
+			desc: "empty IP addresses",
+			requests: [2]*gitalypb.PackObjectsHookWithSidechannelRequest{
+				{
+					GlId:     "user-123",
+					RemoteIp: "",
+					Repository: &gitalypb.Repository{
+						StorageName:  "storage-0",
+						RelativePath: "a/b/c",
+					},
+					Args: args,
+				},
+				{
+					GlId:     "user-123",
+					RemoteIp: "",
+					Repository: &gitalypb.Repository{
+						StorageName:  "storage-0",
+						RelativePath: "a/b/c",
+					},
+					Args: args,
+				},
+			},
+			shouldLimit: featureflag.PackObjectsLimitingRepo.IsEnabled(ctx) ||
+				featureflag.PackObjectsLimitingUser.IsEnabled(ctx),
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			ticker := helper.NewManualTicker()