diff options
author | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2022-07-28 19:11:46 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com> | 2022-07-28 19:11:46 +0300 |
commit | 1ce91833ee8b048250cd6eb776ea7c6ab61f1366 (patch) | |
tree | 44144b187ce00782083bc008aea7d6cf6a3aff24 | |
parent | 2e5bfc20d0979effd9e0cc8b3a53d6cd6da703ab (diff) | |
parent | c3e12d4297ccf34d07ecb71b1fae04e3d72f6d51 (diff) |
Merge remote-tracking branch 'dev/master'
-rw-r--r-- | CHANGELOG.md | 18 | ||||
-rw-r--r-- | internal/git/command_description.go | 8 | ||||
-rw-r--r-- | internal/git/command_factory.go | 6 | ||||
-rw-r--r-- | internal/git/hooks_options_test.go | 4 | ||||
-rw-r--r-- | internal/git/objectpool/clone.go | 12 | ||||
-rw-r--r-- | internal/git/objectpool/clone_test.go | 68 | ||||
-rw-r--r-- | internal/gitaly/service/repository/archive_test.go | 9 | ||||
-rw-r--r-- | internal/gitaly/service/repository/create_fork.go | 6 | ||||
-rw-r--r-- | internal/gitaly/service/repository/create_fork_test.go | 58 | ||||
-rw-r--r-- | internal/gitaly/service/repository/create_repository_from_url_test.go | 34 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/inforefs_test.go | 123 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/upload_pack_test.go | 37 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_pack_test.go | 32 | ||||
-rw-r--r-- | internal/metadata/featureflag/ff_upload_pack_hide_refs.go | 9 |
14 files changed, 351 insertions, 73 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 175c35650..c971018cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Gitaly changelog +## 15.2.1 (2022-07-28) + +### Security (1 change) + +- [Import via git protocol allows to bypass checks on repository](gitlab-org/security/gitaly@d7af133ce2c05dcfc93ebf15a47154a0cd40e9bd) ([merge request](gitlab-org/security/gitaly!56)) + ## 15.2.0 (2022-07-21) ### Added (4 changes) @@ -50,6 +56,12 @@ - [linguist: Implement Stats in pure Go](gitlab-org/gitaly@efd9a598f50e03f05620b56f2e010600128f3b1c) ([merge request](gitlab-org/gitaly!4580)) +## 15.1.4 (2022-07-28) + +### Security (1 change) + +- [Import via git protocol allows to bypass checks on repository](gitlab-org/security/gitaly@f8909dcbab9ea2ddabad06577a1c2ae50ac5f8f3) ([merge request](gitlab-org/security/gitaly!54)) + ## 15.1.3 (2022-07-19) No changes. @@ -100,6 +112,12 @@ No changes. - [repository: Use long-running filter process for converting LFS pointers](gitlab-org/gitaly@0603c758d6e3580adbd3d0d69e326d05baa340b9) ([merge request](gitlab-org/gitaly!4595)) +## 15.0.5 (2022-07-28) + +### Security (1 change) + +- [Import via git protocol allows to bypass checks on repository](gitlab-org/security/gitaly@a110173b56e3bc8c7cb5db2e1e84f6cfb226c39e) ([merge request](gitlab-org/security/gitaly!55)) + ## 15.0.4 (2022-06-30) No changes. diff --git a/internal/git/command_description.go b/internal/git/command_description.go index bfc91117c..2ecd060b3 100644 --- a/internal/git/command_description.go +++ b/internal/git/command_description.go @@ -53,12 +53,12 @@ var commandDescriptions = map[string]commandDescription{ flags: scNoEndOfOptions, }, "clone": { - opts: append([]GlobalOption{ + opts: append(append([]GlobalOption{ // See "init" for why we set the template directory to the empty string. ConfigPair{Key: "init.templateDir", Value: ""}, // See "fetch" for why we disable following redirects. ConfigPair{Key: "http.followRedirects", Value: "false"}, - }, packConfiguration()...), + }, packConfiguration()...), fsckConfiguration("fetch")...), }, "commit": { flags: 0, @@ -298,12 +298,12 @@ var commandDescriptions = map[string]commandDescription{ }, "upload-pack": { flags: scNoRefUpdates, - opts: append(append([]GlobalOption{ + opts: append([]GlobalOption{ ConfigPair{Key: "uploadpack.allowFilter", Value: "true"}, // Enables the capability to request individual SHA1's from the // remote repo. ConfigPair{Key: "uploadpack.allowAnySHA1InWant", Value: "true"}, - }, hiddenUploadPackRefPrefixes()...), packConfiguration()...), + }, packConfiguration()...), }, "version": { flags: scNoRefUpdates, diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index 210288dd4..23506f192 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -16,6 +16,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v15/internal/log" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" ) // CommandFactory is designed to create and run git commands in a protected and fully managed manner. @@ -451,6 +452,11 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi } combinedGlobals = append(combinedGlobals, commandDescription.opts...) + + if sc.Subcommand() == "upload-pack" && featureflag.UploadPackHideRefs.IsEnabled(ctx) { + combinedGlobals = append(combinedGlobals, hiddenUploadPackRefPrefixes()...) + } + combinedGlobals = append(combinedGlobals, cc.globals...) for _, configPair := range gitConfig { combinedGlobals = append(combinedGlobals, ConfigPair{ diff --git a/internal/git/hooks_options_test.go b/internal/git/hooks_options_test.go index 24ebed76b..384c3c5ed 100644 --- a/internal/git/hooks_options_test.go +++ b/internal/git/hooks_options_test.go @@ -11,6 +11,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" grpcmetadata "google.golang.org/grpc/metadata" @@ -74,6 +75,9 @@ func TestWithPackObjectsHookEnv(t *testing.T) { ctx = grpcmetadata.AppendToOutgoingContext(ctx, "user_id", userID, "username", username) ctx = metadata.OutgoingToIncoming(ctx) + // We don't care about this feature flag as it doesn't impact behaviour of the system under + // test. + ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, featureflag.UploadPackHideRefs, true) cmd, err := gittest.NewCommandFactory(t, cfg, git.WithSkipHooks()).New(ctx, repo, subCmd, opt) require.NoError(t, err) diff --git a/internal/git/objectpool/clone.go b/internal/git/objectpool/clone.go index 79a88a035..45a48f864 100644 --- a/internal/git/objectpool/clone.go +++ b/internal/git/objectpool/clone.go @@ -1,7 +1,9 @@ package objectpool import ( + "bytes" "context" + "fmt" "os" "path/filepath" @@ -17,6 +19,7 @@ func (o *ObjectPool) clone(ctx context.Context, repo *localrepo.Repo) error { return err } + var stderr bytes.Buffer cmd, err := o.gitCmdFactory.NewWithoutRepo(ctx, git.SubCmd{ Name: "clone", @@ -28,12 +31,17 @@ func (o *ObjectPool) clone(ctx context.Context, repo *localrepo.Repo) error { Args: []string{repoPath, o.FullPath()}, }, git.WithRefTxHook(repo), + git.WithStderr(&stderr), ) if err != nil { - return err + return fmt.Errorf("spawning clone: %w", err) + } + + if err := cmd.Wait(); err != nil { + return fmt.Errorf("cloning to pool: %w, stderr: %q", err, stderr.String()) } - return cmd.Wait() + return nil } func (o *ObjectPool) removeHooksDir() error { diff --git a/internal/git/objectpool/clone_test.go b/internal/git/objectpool/clone_test.go index 97e9a26ff..4c6d5b0c0 100644 --- a/internal/git/objectpool/clone_test.go +++ b/internal/git/objectpool/clone_test.go @@ -3,40 +3,88 @@ package objectpool import ( + "bytes" + "fmt" "path/filepath" "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" ) -func TestClone(t *testing.T) { +func TestClone_successful(t *testing.T) { + t.Parallel() + ctx := testhelper.Context(t) cfg, pool, repoProto := setupObjectPool(t, ctx) repo := localrepo.NewTestRepo(t, cfg, repoProto) require.NoError(t, pool.clone(ctx, repo)) - defer func() { - require.NoError(t, pool.Remove(ctx)) - }() require.DirExists(t, pool.FullPath()) require.DirExists(t, filepath.Join(pool.FullPath(), "objects")) } -func TestCloneExistingPool(t *testing.T) { +func TestClone_existingPool(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + + cfg, pool, repoProto := setupObjectPool(t, ctx) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + // The first time around cloning should succeed, but ... + require.NoError(t, pool.clone(ctx, repo)) + + // ... when we try to clone the same pool a second time we should get an error because the + // destination exists already. + require.EqualError(t, pool.clone(ctx, repo), fmt.Sprintf( + "cloning to pool: exit status 128, stderr: \"fatal: destination path '%s' already exists and is not an empty directory.\\n\"", + pool.FullPath(), + )) +} + +func TestClone_fsck(t *testing.T) { + t.Parallel() + ctx := testhelper.Context(t) cfg, pool, repoProto := setupObjectPool(t, ctx) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + repoPath, err := repo.Path() + require.NoError(t, err) + + // Write a tree into the repository that's known-broken. + treeID := gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{ + {Content: "content", Path: "dup", Mode: "100644"}, + {Content: "content", Path: "dup", Mode: "100644"}, + }) + gittest.WriteCommit(t, cfg, repoPath, + gittest.WithParents(), + gittest.WithBranch("main"), + gittest.WithTree(treeID), + ) + + // While git-clone(1) would normally complain about the broken tree we have just cloned, we + // don't expect the clone to fail. This is because we know that the tree is already in one + // of our repositories that we have locally, so raising an error now doesn't make a whole + // lot of sense in the first place. + // + // Note: this works because we use `git clone --local`, which only creates a copy of the + // repository without performing consistency checks. require.NoError(t, pool.clone(ctx, repo)) - defer func() { - require.NoError(t, pool.Remove(ctx)) - }() - // re-clone on the directory - require.Error(t, pool.clone(ctx, repo)) + // Verify that the broken tree is indeed in the pool repository and that it is reported as + // broken by git-fsck(1). + var stderr bytes.Buffer + fsckCmd := gittest.NewCommand(t, cfg, "-C", pool.FullPath(), "fsck") + fsckCmd.Stderr = &stderr + + require.EqualError(t, fsckCmd.Run(), "exit status 1") + require.Equal(t, fmt.Sprintf("error in tree %s: duplicateEntries: contains duplicate file entries\n", treeID), stderr.String()) } diff --git a/internal/gitaly/service/repository/archive_test.go b/internal/gitaly/service/repository/archive_test.go index 5f18f8679..e897af1cf 100644 --- a/internal/gitaly/service/repository/archive_test.go +++ b/internal/gitaly/service/repository/archive_test.go @@ -5,6 +5,7 @@ package repository import ( "archive/zip" "bytes" + "context" "fmt" "io" "os" @@ -19,6 +20,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/gitlab" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver" @@ -165,10 +167,13 @@ func TestGetArchive_success(t *testing.T) { } } -func TestGetArchive_includeLfsBlobs(t *testing.T) { +func TestGetArchiveIncludeLfsBlobs(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testGetArchiveIncludeLfsBlobs) +} - ctx := testhelper.Context(t) +func testGetArchiveIncludeLfsBlobs(t *testing.T, ctx context.Context) { + t.Parallel() defaultOptions := gitlab.TestServerOptions{ SecretToken: secretToken, diff --git a/internal/gitaly/service/repository/create_fork.go b/internal/gitaly/service/repository/create_fork.go index cbdff0231..ea9fd3949 100644 --- a/internal/gitaly/service/repository/create_fork.go +++ b/internal/gitaly/service/repository/create_fork.go @@ -49,6 +49,12 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest }, }, git.WithInternalFetch(&gitalypb.SSHUploadPackRequest{ Repository: sourceRepository, + }), git.WithConfig(git.ConfigPair{ + // Disable consistency checks for fetched objects when creating a + // fork. We don't want to end up in a situation where it's + // impossible to create forks we already have anyway because we have + // e.g. retroactively tightened the consistency checks. + Key: "fetch.fsckObjects", Value: "false", }), git.WithDisabledHooks()) if err != nil { return fmt.Errorf("spawning fetch: %w", err) diff --git a/internal/gitaly/service/repository/create_fork_test.go b/internal/gitaly/service/repository/create_fork_test.go index a58ca9b28..cf140b9ae 100644 --- a/internal/gitaly/service/repository/create_fork_test.go +++ b/internal/gitaly/service/repository/create_fork_test.go @@ -3,8 +3,10 @@ package repository import ( + "bytes" "crypto/tls" "crypto/x509" + "fmt" "os" "path/filepath" "strings" @@ -160,6 +162,62 @@ func TestCreateFork_refs(t *testing.T) { ) } +func TestCreateFork_fsck(t *testing.T) { + t.Parallel() + + cfg := testcfg.Build(t) + + testcfg.BuildGitalyHooks(t, cfg) + testcfg.BuildGitalySSH(t, cfg) + + client, socketPath := runRepositoryService(t, cfg, nil) + cfg.SocketPath = socketPath + + ctx := testhelper.Context(t) + ctx = testhelper.MergeOutgoingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg)) + + repo, repoPath := gittest.CreateRepository(ctx, t, cfg) + + // Write a tree into the repository that's known-broken. + treeID := gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{ + {Content: "content", Path: "dup", Mode: "100644"}, + {Content: "content", Path: "dup", Mode: "100644"}, + }) + + gittest.WriteCommit(t, cfg, repoPath, + gittest.WithParents(), + gittest.WithBranch("main"), + gittest.WithTree(treeID), + ) + + forkedRepo := &gitalypb.Repository{ + RelativePath: gittest.NewRepositoryName(t, true), + StorageName: repo.GetStorageName(), + } + + // Create a fork from the repository with the broken tree. This should work alright: repos + // with preexisting broken objects that we already have on our disk anyway should not be + // subject to additional consistency checks. Otherwise we might end up in a situation where + // we retroactively tighten consistency checks for repositories such that preexisting repos + // wouldn't be forkable anymore. + _, err := client.CreateFork(ctx, &gitalypb.CreateForkRequest{ + Repository: forkedRepo, + SourceRepository: repo, + }) + require.NoError(t, err) + + forkedRepoPath := filepath.Join(cfg.Storages[0].Path, gittest.GetReplicaPath(ctx, t, cfg, forkedRepo)) + + // Verify that the broken tree is indeed in the fork and that it is reported as broken by + // git-fsck(1). + var stderr bytes.Buffer + fsckCmd := gittest.NewCommand(t, cfg, "-C", forkedRepoPath, "fsck") + fsckCmd.Stderr = &stderr + + require.EqualError(t, fsckCmd.Run(), "exit status 4") + require.Equal(t, fmt.Sprintf("error in tree %s: duplicateEntries: contains duplicate file entries\n", treeID), stderr.String()) +} + func TestCreateFork_targetExists(t *testing.T) { t.Parallel() ctx := testhelper.Context(t) diff --git a/internal/gitaly/service/repository/create_repository_from_url_test.go b/internal/gitaly/service/repository/create_repository_from_url_test.go index 54dd7e79e..764f0a12d 100644 --- a/internal/gitaly/service/repository/create_repository_from_url_test.go +++ b/internal/gitaly/service/repository/create_repository_from_url_test.go @@ -186,6 +186,40 @@ func TestCreateRepositoryFromURL_redirect(t *testing.T) { require.Contains(t, err.Error(), "The requested URL returned error: 301") } +func TestCreateRepositoryFromURL_fsck(t *testing.T) { + t.Parallel() + + ctx := testhelper.Context(t) + + cfg, client := setupRepositoryServiceWithoutRepo(t) + + _, sourceRepoPath := gittest.CreateRepository(ctx, t, cfg) + + // We're creating a new commit which has a root tree with duplicate entries. git-mktree(1) + // allows us to create these trees just fine, but git-fsck(1) complains. + gittest.WriteCommit(t, cfg, sourceRepoPath, + gittest.WithParents(), + gittest.WithBranch("main"), + gittest.WithTreeEntries( + gittest.TreeEntry{Content: "content", Path: "dup", Mode: "100644"}, + gittest.TreeEntry{Content: "content", Path: "dup", Mode: "100644"}, + ), + ) + + targetRepoProto := &gitalypb.Repository{ + RelativePath: gittest.NewRepositoryName(t, true), + StorageName: cfg.Storages[0].Name, + } + + _, err := client.CreateRepositoryFromURL(ctx, &gitalypb.CreateRepositoryFromURLRequest{ + Repository: targetRepoProto, + Url: "file://" + sourceRepoPath, + }) + require.Error(t, err) + testhelper.RequireGrpcCode(t, err, codes.Internal) + require.Contains(t, err.Error(), "duplicateEntries: contains duplicate file entries") +} + func TestServer_CloneFromURLCommand(t *testing.T) { t.Parallel() diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go index ef47aabb0..467282911 100644 --- a/internal/gitaly/service/smarthttp/inforefs_test.go +++ b/internal/gitaly/service/smarthttp/inforefs_test.go @@ -37,10 +37,14 @@ import ( func TestInfoRefsUploadPack_successful(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsUploadPackSuccessful) +} + +func testInfoRefsUploadPackSuccessful(t *testing.T, ctx context.Context) { + t.Parallel() cfg := testcfg.Build(t) cfg.SocketPath = runSmartHTTPServer(t, cfg) - ctx := testhelper.Context(t) repo, repoPath := gittest.CreateRepository(ctx, t, cfg) @@ -62,10 +66,14 @@ func TestInfoRefsUploadPack_successful(t *testing.T) { func TestInfoRefsUploadPack_internalRefs(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsUploadPackInternalRefs) +} + +func testInfoRefsUploadPackInternalRefs(t *testing.T, ctx context.Context) { + t.Parallel() cfg := testcfg.Build(t) cfg.SocketPath = runSmartHTTPServer(t, cfg) - ctx := testhelper.Context(t) for _, tc := range []struct { ref string @@ -97,17 +105,37 @@ func TestInfoRefsUploadPack_internalRefs(t *testing.T) { }, { ref: "refs/tmp/1", - expectedAdvertisements: []string{ - "HEAD", - "refs/heads/main\n", - }, + expectedAdvertisements: func() []string { + if featureflag.UploadPackHideRefs.IsDisabled(ctx) { + return []string{ + "HEAD", + "refs/heads/main\n", + "refs/tmp/1\n", + } + } + + return []string{ + "HEAD", + "refs/heads/main\n", + } + }(), }, { ref: "refs/keep-around/1", - expectedAdvertisements: []string{ - "HEAD", - "refs/heads/main\n", - }, + expectedAdvertisements: func() []string { + if featureflag.UploadPackHideRefs.IsDisabled(ctx) { + return []string{ + "HEAD", + "refs/heads/main\n", + "refs/keep-around/1\n", + } + } + + return []string{ + "HEAD", + "refs/heads/main\n", + } + }(), }, } { t.Run(tc.ref, func(t *testing.T) { @@ -130,18 +158,21 @@ func TestInfoRefsUploadPack_internalRefs(t *testing.T) { } } -func TestInfoRefsUploadPack_repositoryDoesntExist(t *testing.T) { +func TestInfoRefsUploadPackRepositoryDoesntExist(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsUploadPackRepositoryDoesntExist) +} - cfg := testcfg.Build(t) +func testInfoRefsUploadPackRepositoryDoesntExist(t *testing.T, ctx context.Context) { + t.Parallel() + cfg := testcfg.Build(t) serverSocketPath := runSmartHTTPServer(t, cfg) rpcRequest := &gitalypb.InfoRefsRequest{Repository: &gitalypb.Repository{ StorageName: cfg.Storages[0].Name, RelativePath: "doesnt/exist", }} - ctx := testhelper.Context(t) _, err := makeInfoRefsUploadPackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest) @@ -153,13 +184,16 @@ func TestInfoRefsUploadPack_repositoryDoesntExist(t *testing.T) { testhelper.RequireGrpcError(t, expectedErr, err) } -func TestInfoRefsUploadPack_partialClone(t *testing.T) { +func TestInfoRefsUploadPackPartialClone(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsUploadPackPartialClone) +} - cfg := testcfg.Build(t) +func testInfoRefsUploadPackPartialClone(t *testing.T, ctx context.Context) { + t.Parallel() + cfg := testcfg.Build(t) cfg.SocketPath = runSmartHTTPServer(t, cfg) - ctx := testhelper.Context(t) repo, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{ Seed: gittest.SeedGitLabTest, @@ -180,13 +214,17 @@ func TestInfoRefsUploadPack_partialClone(t *testing.T) { } } -func TestInfoRefsUploadPack_gitConfigOptions(t *testing.T) { +func TestInfoRefsUploadPackGitConfigOptions(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsUploadPackGitConfigOptions) +} + +func testInfoRefsUploadPackGitConfigOptions(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) cfg.SocketPath = runSmartHTTPServer(t, cfg) - ctx := testhelper.Context(t) repo, repoPath := gittest.CreateRepository(ctx, t, cfg) commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"), gittest.WithParents()) @@ -204,11 +242,15 @@ func TestInfoRefsUploadPack_gitConfigOptions(t *testing.T) { }) } -func TestInfoRefsUploadPack_gitProtocol(t *testing.T) { +func TestInfoRefsUploadPackGitProtocol(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsUploadPackGitProtocol) +} + +func testInfoRefsUploadPackGitProtocol(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) - ctx := testhelper.Context(t) protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg) @@ -262,12 +304,16 @@ func makeInfoRefsUploadPackRequest(ctx context.Context, t *testing.T, serverSock return response, err } -func TestInfoRefsReceivePack_successful(t *testing.T) { +func TestInfoRefsReceivePackSuccessful(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsReceivePackSuccessful) +} + +func testInfoRefsReceivePackSuccessful(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) cfg.SocketPath = runSmartHTTPServer(t, cfg) - ctx := testhelper.Context(t) repo, repoPath := gittest.CreateRepository(ctx, t, cfg) @@ -287,7 +333,12 @@ func TestInfoRefsReceivePack_successful(t *testing.T) { }) } -func TestInfoRefsReceivePack_hiddenRefs(t *testing.T) { +func TestInfoRefsReceivePackHiddenRefs(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsReceivePackHiddenRefs) +} + +func testInfoRefsReceivePackHiddenRefs(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) @@ -295,7 +346,6 @@ func TestInfoRefsReceivePack_hiddenRefs(t *testing.T) { testcfg.BuildGitalyHooks(t, cfg) cfg.SocketPath = runSmartHTTPServer(t, cfg) - ctx := testhelper.Context(t) repoProto, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{ Seed: gittest.SeedGitLabTest, @@ -330,7 +380,12 @@ func TestInfoRefsReceivePack_hiddenRefs(t *testing.T) { require.NotContains(t, string(response), commitID+" .have") } -func TestInfoRefsReceivePack_repoNotFound(t *testing.T) { +func TestInfoRefsReceivePackRepoNotFound(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsReceivePackRepoNotFound) +} + +func testInfoRefsReceivePackRepoNotFound(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) @@ -339,7 +394,6 @@ func TestInfoRefsReceivePack_repoNotFound(t *testing.T) { repo := &gitalypb.Repository{StorageName: cfg.Storages[0].Name, RelativePath: "testdata/scratch/another_repo"} rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo} - ctx := testhelper.Context(t) _, err := makeInfoRefsReceivePackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest) expectedErr := helper.ErrNotFoundf(`GetRepoPath: not a git repository: "` + cfg.Storages[0].Path + "/" + repo.RelativePath + `"`) @@ -350,7 +404,12 @@ func TestInfoRefsReceivePack_repoNotFound(t *testing.T) { testhelper.RequireGrpcError(t, expectedErr, err) } -func TestInfoRefsReceivePack_repoNotSet(t *testing.T) { +func TestInfoRefsReceivePackRepoNotSet(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsReceivePackRepoNotSet) +} + +func testInfoRefsReceivePackRepoNotSet(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) @@ -358,7 +417,6 @@ func TestInfoRefsReceivePack_repoNotSet(t *testing.T) { serverSocketPath := runSmartHTTPServer(t, cfg) rpcRequest := &gitalypb.InfoRefsRequest{} - ctx := testhelper.Context(t) _, err := makeInfoRefsReceivePackRequest(ctx, t, serverSocketPath, cfg.Auth.Token, rpcRequest) testhelper.RequireGrpcCode(t, err, codes.InvalidArgument) } @@ -418,7 +476,12 @@ func (ms *mockStreamer) PutStream(ctx context.Context, repo *gitalypb.Repository return ms.Streamer.PutStream(ctx, repo, req, src) } -func TestInfoRefsUploadPack_cache(t *testing.T) { +func TestInfoRefsUploadPackCache(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testInfoRefsUploadPackCache) +} + +func testInfoRefsUploadPackCache(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) @@ -434,8 +497,6 @@ func TestInfoRefsUploadPack_cache(t *testing.T) { gitalyServer := startSmartHTTPServer(t, cfg, withInfoRefCache(mockInfoRefCache)) cfg.SocketPath = gitalyServer.Address() - ctx := testhelper.Context(t) - repo, repoPath := gittest.CreateRepository(ctx, t, cfg) commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"), gittest.WithParents()) diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go index 02ca6296e..1bd42a95f 100644 --- a/internal/gitaly/service/smarthttp/upload_pack_test.go +++ b/internal/gitaly/service/smarthttp/upload_pack_test.go @@ -20,6 +20,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/git/pktline" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/internal/sidechannel" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" @@ -45,15 +46,15 @@ func runTestWithAndWithoutConfigOptions( makeRequest requestMaker, opts ...testcfg.Option, ) { - ctx := testhelper.Context(t) + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, func(t *testing.T, ctx context.Context) { + t.Run("no config options", func(t *testing.T) { tf(t, ctx, makeRequest) }) - t.Run("no config options", func(t *testing.T) { tf(t, ctx, makeRequest) }) - - if len(opts) > 0 { - t.Run("with config options", func(t *testing.T) { - tf(t, ctx, makeRequest, opts...) - }) - } + if len(opts) > 0 { + t.Run("with config options", func(t *testing.T) { + tf(t, ctx, makeRequest, opts...) + }) + } + }) } func TestServer_PostUpload(t *testing.T) { @@ -262,16 +263,18 @@ func testServerPostUploadPackSuppressDeepenExitError(t *testing.T, ctx context.C func TestServer_PostUploadPack_usesPackObjectsHook(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) - testServerPostUploadPackUsesPackObjectsHook(t, ctx, makePostUploadPackRequest) + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, func(t *testing.T, ctx context.Context) { + testServerPostUploadPackUsesPackObjectsHook(t, ctx, makePostUploadPackRequest) + }) } func TestServer_PostUploadPackWithSidechannel_usesPackObjectsHook(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) - testServerPostUploadPackUsesPackObjectsHook(t, ctx, makePostUploadPackWithSidechannelRequest) + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, func(t *testing.T, ctx context.Context) { + testServerPostUploadPackUsesPackObjectsHook(t, ctx, makePostUploadPackWithSidechannelRequest) + }) } func testServerPostUploadPackUsesPackObjectsHook(t *testing.T, ctx context.Context, makeRequest requestMaker, opts ...testcfg.Option) { @@ -492,16 +495,18 @@ func testServerPostUploadPackPartialClone(t *testing.T, ctx context.Context, mak func TestServer_PostUploadPack_allowAnySHA1InWant(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) - testServerPostUploadPackAllowAnySHA1InWant(t, ctx, makePostUploadPackRequest) + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, func(t *testing.T, ctx context.Context) { + testServerPostUploadPackAllowAnySHA1InWant(t, ctx, makePostUploadPackRequest) + }) } func TestServer_PostUploadPackWithSidechannel_allowAnySHA1InWant(t *testing.T) { t.Parallel() - ctx := testhelper.Context(t) - testServerPostUploadPackAllowAnySHA1InWant(t, ctx, makePostUploadPackWithSidechannelRequest) + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, func(t *testing.T, ctx context.Context) { + testServerPostUploadPackAllowAnySHA1InWant(t, ctx, makePostUploadPackWithSidechannelRequest) + }) } func testServerPostUploadPackAllowAnySHA1InWant(t *testing.T, ctx context.Context, makeRequest requestMaker, opts ...testcfg.Option) { diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go index c8b287b72..a366d0f80 100644 --- a/internal/gitaly/service/ssh/upload_pack_test.go +++ b/internal/gitaly/service/ssh/upload_pack_test.go @@ -137,7 +137,12 @@ func testUploadPackTimeout(t *testing.T, opts ...testcfg.Option) { }) } -func TestUploadPackWithSidechannel_client(t *testing.T) { +func TestUploadPackWithSidechannelClient(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testUploadPackWithSidechannelClient) +} + +func testUploadPackWithSidechannelClient(t *testing.T, ctx context.Context) { t.Parallel() cfg := testcfg.Build(t) @@ -376,7 +381,7 @@ func TestUploadPackWithSidechannel_client(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { - ctx, cancel := context.WithCancel(testhelper.Context(t)) + ctx, cancel := context.WithCancel(ctx) ctx, waiter := sidechannel.RegisterSidechannel(ctx, registry, func(clientConn *sidechannel.ClientConn) (returnedErr error) { errCh := make(chan error, 1) @@ -642,10 +647,13 @@ func testUploadPackSuccessful(t *testing.T, sidechannel bool, opts ...testcfg.Op } } -func TestUploadPack_packObjectsHook(t *testing.T) { +func TestUploadPackPackObjectsHook(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testUploadPackPackObjectsHook) +} - ctx := testhelper.Context(t) +func testUploadPackPackObjectsHook(t *testing.T, ctx context.Context) { + t.Parallel() cfg := testcfg.Build(t, testcfg.WithPackObjectsCacheEnabled()) @@ -738,10 +746,14 @@ func testUploadPackWithoutSideband(t *testing.T, opts ...testcfg.Option) { require.Contains(t, string(out), "PACK") } -func TestUploadPack_invalidStorage(t *testing.T) { +func TestUploadPackInvalidStorage(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testUploadPackInvalidStorage) +} + +func testUploadPackInvalidStorage(t *testing.T, ctx context.Context) { t.Parallel() - ctx := testhelper.Context(t) cfg := testcfg.Build(t) cfg.SocketPath = runSSHServer(t, cfg) @@ -773,10 +785,14 @@ func TestUploadPack_invalidStorage(t *testing.T) { } } -func TestUploadPack_gitFailure(t *testing.T) { +func TestUploadPackGitFailure(t *testing.T) { + t.Parallel() + testhelper.NewFeatureSets(featureflag.UploadPackHideRefs).Run(t, testUploadPackGitFailure) +} + +func testUploadPackGitFailure(t *testing.T, ctx context.Context) { t.Parallel() - ctx := testhelper.Context(t) cfg := testcfg.Build(t) cfg.SocketPath = runSSHServer(t, cfg) diff --git a/internal/metadata/featureflag/ff_upload_pack_hide_refs.go b/internal/metadata/featureflag/ff_upload_pack_hide_refs.go new file mode 100644 index 000000000..1e26e33e5 --- /dev/null +++ b/internal/metadata/featureflag/ff_upload_pack_hide_refs.go @@ -0,0 +1,9 @@ +package featureflag + +// UploadPackHideRefs causes git-upload-pack(1) to hide internal references. +var UploadPackHideRefs = NewFeatureFlag( + "upload_pack_hide_refs", + "v15.3.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/4390", + false, +) |