Merge branch 'qmnguyen0711/parse-remote-ip-before-propagating' into 'master'

Ignore source port in pack-objects limiting

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5702

Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>

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()