From 9969aef9680d7fcaee5684cba3082d37c669f518 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 4 Sep 2024 18:03:20 +0200 Subject: [PATCH 1/2] backup: Validate repository before calling updateref In `backup::localRepository.ResetRefs` we try to read the references from a local repository. There are situations where the repository doesn't even exist. In this case, we detect the failure when we try to read the 'ObjectHash' of the repository. This isn't ideal, since the value of 'ObjectHash' is cached and later when we do end up creating a repository, the 'ObjectHash' would still return the same error we get now. So instead, let's do the right thing and validate the repository at the start. --- internal/backup/repository.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/backup/repository.go b/internal/backup/repository.go index 1509bc2756..518b1c21a7 100644 --- a/internal/backup/repository.go +++ b/internal/backup/repository.go @@ -605,6 +605,10 @@ func (r *localRepository) HeadReference(ctx context.Context) (git.ReferenceName, // ResetRefs attempts to reset the list of refs in the repository to match the // specified refs slice. Do not include the symbolic HEAD reference in the list. func (r *localRepository) ResetRefs(ctx context.Context, refs []git.Reference) (returnedErr error) { + if err := r.locator.ValidateRepository(ctx, r.repo); err != nil { + return fmt.Errorf("invalid repository: %w", err) + } + u, err := updateref.New(ctx, r.repo) if err != nil { return fmt.Errorf("error when running creating new updater: %w", err) -- GitLab From 162d8b2dda1259432b86feab2803f243de66b8fc Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Tue, 3 Sep 2024 17:44:26 +0200 Subject: [PATCH 2/2] c1 --- cmd/gitaly-hooks/hooks_test.go | 12 +++++-- internal/git/gitcmd/hooks_options.go | 18 +++++----- internal/git/gitcmd/hooks_options_test.go | 22 ++++++++---- internal/git/housekeeping/worktrees.go | 16 +++++++-- internal/git/localrepo/bundle.go | 7 +++- internal/git/localrepo/refs_external_test.go | 5 ++- internal/git/localrepo/remote.go | 14 ++++++-- internal/git/objectpool/create.go | 2 +- internal/git/objectpool/fetch.go | 7 +++- internal/git/updateref/updateref.go | 12 +++---- .../gitaly/service/cleanup/rewrite_history.go | 7 +++- .../gitaly/service/operations/apply_patch.go | 20 ++++++++--- internal/gitaly/service/ref/find_tag.go | 8 ++++- .../gitaly/service/repository/create_fork.go | 8 +++-- .../repository/create_repository_from_url.go | 6 ++-- internal/gitaly/service/repository/util.go | 13 +++++-- .../gitaly/service/smarthttp/receive_pack.go | 16 ++++++--- .../gitaly/service/smarthttp/upload_pack.go | 36 +++++++++++++------ internal/gitaly/service/ssh/receive_pack.go | 15 +++++--- internal/gitaly/service/ssh/upload_pack.go | 19 ++++++---- 20 files changed, 190 insertions(+), 73 deletions(-) diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 840290f906..91962b3a85 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -18,6 +18,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/pktline" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/auth" @@ -995,9 +996,11 @@ func TestGitalyServerReturnsError_packObjects(t *testing.T) { Hooks: config.Hooks{CustomHooksDir: testhelper.TempDir(t)}, })) - repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ SkipCreationViaService: true, }) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch)) testcfg.BuildGitalyHooks(t, cfg) @@ -1023,8 +1026,11 @@ func TestGitalyServerReturnsError_packObjects(t *testing.T) { ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, enabledFeatureFlag, true) ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, disabledFeatureFlag, false) + objectHash, err := repo.ObjectHash(ctx) + require.NoError(t, err) + var stderr, stdout bytes.Buffer - cmd, err := cmdFactory.New(ctx, repo, gitcmd.Command{ + cmd, err := cmdFactory.New(ctx, repoProto, gitcmd.Command{ Name: "clone", Flags: []gitcmd.Option{ gitcmd.ValueFlag{Name: "-u", Value: "git -c uploadpack.allowFilter -c uploadpack.packObjectsHook=" + cfg.BinaryPath("gitaly-hooks") + " upload-pack"}, @@ -1034,7 +1040,7 @@ func TestGitalyServerReturnsError_packObjects(t *testing.T) { }, Args: []string{repoPath, testhelper.TempDir(t)}, }, - gitcmd.WithPackObjectsHookEnv(repo, "ssh"), + gitcmd.WithPackObjectsHookEnv(repoProto, objectHash, "ssh"), gitcmd.WithStdout(&stdout), gitcmd.WithStderr(&stderr), ) diff --git a/internal/git/gitcmd/hooks_options.go b/internal/git/gitcmd/hooks_options.go index 7aa3a5f282..0f5bafbd35 100644 --- a/internal/git/gitcmd/hooks_options.go +++ b/internal/git/gitcmd/hooks_options.go @@ -6,6 +6,7 @@ import ( "fmt" "gitlab.com/gitlab-org/gitaly/v16/internal/featureflag" + "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/metadata" @@ -30,7 +31,7 @@ func WithDisabledHooks() CmdOpt { // WithRefTxHook returns an option that populates the safe command with the // environment variables necessary to properly execute a reference hook for // repository changes that may possibly update references -func WithRefTxHook(repo storage.Repository) CmdOpt { +func WithRefTxHook(repo storage.Repository, objectHash git.ObjectHash) CmdOpt { return func(ctx context.Context, cfg config.Cfg, gitCmdFactory CommandFactory, cc *cmdCfg) error { if repo == nil { return fmt.Errorf("missing repo: %w", ErrInvalidArg) @@ -45,7 +46,7 @@ func WithRefTxHook(repo storage.Repository) CmdOpt { GitAlternateObjectDirectories: repo.GetGitAlternateObjectDirectories(), GitObjectDirectory: repo.GetGitObjectDirectory(), RelativePath: repo.GetRelativePath(), - }, cfg, gitCmdFactory, nil, ReferenceTransactionHook); err != nil { + }, objectHash, cfg, gitCmdFactory, nil, ReferenceTransactionHook); err != nil { return fmt.Errorf("ref hook env var: %w", err) } @@ -54,7 +55,7 @@ func WithRefTxHook(repo storage.Repository) CmdOpt { } // WithPackObjectsHookEnv provides metadata for gitaly-hooks so it can act as a pack-objects hook. -func WithPackObjectsHookEnv(repo *gitalypb.Repository, protocol string) CmdOpt { +func WithPackObjectsHookEnv(repo *gitalypb.Repository, objectHash git.ObjectHash, protocol string) CmdOpt { return func(ctx context.Context, cfg config.Cfg, gitCmdFactory CommandFactory, cc *cmdCfg) error { if !cfg.PackObjectsCache.Enabled { return nil @@ -74,6 +75,7 @@ func WithPackObjectsHookEnv(repo *gitalypb.Repository, protocol string) CmdOpt { if err := cc.configureHooks( ctx, repo, + objectHash, cfg, gitCmdFactory, userDetails, @@ -103,7 +105,7 @@ type ReceivePackRequest interface { // WithReceivePackHooks returns an option that populates the safe command with the environment // variables necessary to properly execute the pre-receive, update, post-receive, and proc-receive // (if enabled) hooks for git-receive-pack(1). -func WithReceivePackHooks(req ReceivePackRequest, protocol string, enableProcReceive bool) CmdOpt { +func WithReceivePackHooks(req ReceivePackRequest, objectHash git.ObjectHash, protocol string, enableProcReceive bool) CmdOpt { return func(ctx context.Context, cfg config.Cfg, gitCmdFactory CommandFactory, cc *cmdCfg) error { requestedHooks := ReceivePackHooks if enableProcReceive { @@ -112,7 +114,7 @@ func WithReceivePackHooks(req ReceivePackRequest, protocol string, enableProcRec requestedHooks = PreReceiveHook | ProcReceiveHook } - if err := cc.configureHooks(ctx, req.GetRepository(), cfg, gitCmdFactory, &UserDetails{ + if err := cc.configureHooks(ctx, req.GetRepository(), objectHash, cfg, gitCmdFactory, &UserDetails{ UserID: req.GetGlId(), Username: req.GetGlUsername(), Protocol: protocol, @@ -130,6 +132,7 @@ func WithReceivePackHooks(req ReceivePackRequest, protocol string, enableProcRec func (cc *cmdCfg) configureHooks( ctx context.Context, repo *gitalypb.Repository, + objectHash git.ObjectHash, cfg config.Cfg, gitCmdFactory CommandFactory, userDetails *UserDetails, @@ -139,11 +142,6 @@ func (cc *cmdCfg) configureHooks( return errors.New("hooks already configured") } - objectHash, err := DetectObjectHash(ctx, gitCmdFactory, repo) - if err != nil { - return fmt.Errorf("detecting object hash: %w", err) - } - var transaction *txinfo.Transaction if tx, err := txinfo.TransactionFromContext(ctx); err == nil { transaction = &tx diff --git a/internal/git/gitcmd/hooks_options_test.go b/internal/git/gitcmd/hooks_options_test.go index 76746c110f..9129d45286 100644 --- a/internal/git/gitcmd/hooks_options_test.go +++ b/internal/git/gitcmd/hooks_options_test.go @@ -8,6 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/command" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/metadata" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg" @@ -20,12 +21,17 @@ func TestWithRefHook(t *testing.T) { ctx := testhelper.Context(t) cfg := testcfg.Build(t) - repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ SkipCreationViaService: true, }) + + repo := localrepo.NewTestRepo(t, cfg, repoProto) + objectHash, err := repo.ObjectHash(ctx) + require.NoError(t, err) + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("refs/heads/master")) - opt := gitcmd.WithRefTxHook(repo) + opt := gitcmd.WithRefTxHook(repoProto, objectHash) subCmd := gitcmd.Command{Name: "update-ref", Args: []string{"refs/heads/master", gittest.DefaultObjectHash.ZeroOID.String()}} for _, tt := range []struct { @@ -35,7 +41,7 @@ func TestWithRefHook(t *testing.T) { { name: "NewCommand", fn: func() (*command.Command, error) { - return gittest.NewCommandFactory(t, cfg, gitcmd.WithSkipHooks()).New(ctx, repo, subCmd, opt) + return gittest.NewCommandFactory(t, cfg, gitcmd.WithSkipHooks()).New(ctx, repoProto, subCmd, opt) }, }, } { @@ -74,22 +80,26 @@ func TestWithPackObjectsHookEnv(t *testing.T) { cfg := testcfg.Build(t) cfg.PackObjectsCache.Enabled = true - repo, _ := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + repoProto, _ := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ SkipCreationViaService: true, }) + repo := localrepo.NewTestRepo(t, cfg, repoProto) + + objectHash, err := repo.ObjectHash(ctx) + require.NoError(t, err) userID := "user-123" username := "username" protocol := "protocol" remoteIP := "1.2.3.4" - opt := gitcmd.WithPackObjectsHookEnv(repo, protocol) + opt := gitcmd.WithPackObjectsHookEnv(repoProto, objectHash, protocol) subCmd := gitcmd.Command{Name: "upload-pack", Args: []string{"a/b/c"}} ctx = grpcmetadata.AppendToOutgoingContext(ctx, "user_id", userID, "username", username, "remote_ip", remoteIP) ctx = metadata.OutgoingToIncoming(ctx) - cmd, err := gittest.NewCommandFactory(t, cfg, gitcmd.WithSkipHooks()).New(ctx, repo, subCmd, opt) + cmd, err := gittest.NewCommandFactory(t, cfg, gitcmd.WithSkipHooks()).New(ctx, repoProto, subCmd, opt) require.NoError(t, err) payload, err := gitcmd.HooksPayloadFromEnv(cmd.Env()) diff --git a/internal/git/housekeeping/worktrees.go b/internal/git/housekeeping/worktrees.go index 6d081d9bcf..dd29f2ae34 100644 --- a/internal/git/housekeeping/worktrees.go +++ b/internal/git/housekeeping/worktrees.go @@ -100,14 +100,19 @@ func cleanStaleWorktrees(ctx context.Context, repo *localrepo.Repo, threshold ti var errUnknownWorktree = errors.New("unknown worktree") func removeWorktree(ctx context.Context, repo *localrepo.Repo, name string) error { + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + var stderr bytes.Buffer - err := repo.ExecAndWait(ctx, gitcmd.Command{ + err = repo.ExecAndWait(ctx, gitcmd.Command{ Name: "worktree", Action: "remove", Flags: []gitcmd.Option{gitcmd.Flag{Name: "--force"}}, Args: []string{name}, }, - gitcmd.WithRefTxHook(repo), + gitcmd.WithRefTxHook(repo, objectHash), gitcmd.WithStderr(&stderr), ) if isExitWithCode(err, 128) && strings.HasPrefix(stderr.String(), "fatal: '"+name+"' is not a working tree") { @@ -134,6 +139,11 @@ func cleanDisconnectedWorktrees(ctx context.Context, repo *localrepo.Repo) error return err } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + // Spawning a command is expensive. We thus try to avoid the overhead by first // determining if there could possibly be any work to be done by git-worktree(1). We do so // by reading the directory in which worktrees are stored, and if it's empty then we know @@ -167,5 +177,5 @@ func cleanDisconnectedWorktrees(ctx context.Context, repo *localrepo.Repo) error return repo.ExecAndWait(ctx, gitcmd.Command{ Name: "worktree", Action: "prune", - }, gitcmd.WithRefTxHook(repo)) + }, gitcmd.WithRefTxHook(repo, objectHash)) } diff --git a/internal/git/localrepo/bundle.go b/internal/git/localrepo/bundle.go index ad98ec06be..220345f8e7 100644 --- a/internal/git/localrepo/bundle.go +++ b/internal/git/localrepo/bundle.go @@ -83,6 +83,11 @@ func (repo *Repo) CloneBundle(ctx context.Context, reader io.Reader) error { return err } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("getting object hash: %w", err) + } + repoPath, err := repo.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) if err != nil { return fmt.Errorf("getting repo path: %w", err) @@ -124,7 +129,7 @@ func (repo *Repo) CloneBundle(ctx context.Context, reader io.Reader) error { Args: []string{"remove", "origin"}, }, gitcmd.WithStderr(&remoteErr), - gitcmd.WithRefTxHook(repo), + gitcmd.WithRefTxHook(repo, objectHash), ) if err != nil { return fmt.Errorf("spawning git-remote: %w", err) diff --git a/internal/git/localrepo/refs_external_test.go b/internal/git/localrepo/refs_external_test.go index eeb916b98d..1d92ad4204 100644 --- a/internal/git/localrepo/refs_external_test.go +++ b/internal/git/localrepo/refs_external_test.go @@ -213,6 +213,9 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { _, repo := setupRepoWithHooksServer(t, ctx, cfg, testserver.WithTransactionManager(&blockingManager{ch})) + objectHash, err := repo.ObjectHash(ctx) + require.NoError(t, err) + go func() { _ = repo.SetDefaultBranch(ctx, &blockingManager{ch}, "refs/heads/branch") doneCh <- struct{}{} @@ -223,7 +226,7 @@ func TestRepo_SetDefaultBranch_errors(t *testing.T) { err = repo.ExecAndWait(ctx, gitcmd.Command{ Name: "symbolic-ref", Args: []string{"HEAD", "refs/heads/otherbranch"}, - }, gitcmd.WithRefTxHook(repo), gitcmd.WithStderr(&stderr)) + }, gitcmd.WithRefTxHook(repo, objectHash), gitcmd.WithStderr(&stderr)) code, ok := command.ExitStatus(err) require.True(t, ok) diff --git a/internal/git/localrepo/remote.go b/internal/git/localrepo/remote.go index 07d528f3c3..770803c698 100644 --- a/internal/git/localrepo/remote.go +++ b/internal/git/localrepo/remote.go @@ -81,6 +81,11 @@ func (repo *Repo) FetchRemote(ctx context.Context, remoteName string, opts Fetch return err } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + var stderr bytes.Buffer if opts.Stderr == nil { opts.Stderr = &stderr @@ -99,7 +104,7 @@ func (repo *Repo) FetchRemote(ctx context.Context, remoteName string, opts Fetch if opts.DisableTransactions { commandOptions = append(commandOptions, gitcmd.WithDisabledHooks()) } else { - commandOptions = append(commandOptions, gitcmd.WithRefTxHook(repo)) + commandOptions = append(commandOptions, gitcmd.WithRefTxHook(repo, objectHash)) } commandOptions = append(commandOptions, opts.CommandOptions...) @@ -139,6 +144,11 @@ func (repo *Repo) FetchInternal( opts.Stderr = &stderr } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + commandOptions := []gitcmd.CmdOpt{ gitcmd.WithEnv(opts.Env...), gitcmd.WithStderr(opts.Stderr), @@ -159,7 +169,7 @@ func (repo *Repo) FetchInternal( if opts.DisableTransactions { commandOptions = append(commandOptions, gitcmd.WithDisabledHooks()) } else { - commandOptions = append(commandOptions, gitcmd.WithRefTxHook(repo)) + commandOptions = append(commandOptions, gitcmd.WithRefTxHook(repo, objectHash)) } commandOptions = append(commandOptions, opts.CommandOptions...) diff --git a/internal/git/objectpool/create.go b/internal/git/objectpool/create.go index 0dd6c832c7..3ab2dac5bb 100644 --- a/internal/git/objectpool/create.go +++ b/internal/git/objectpool/create.go @@ -76,7 +76,7 @@ func Create( }, Args: []string{sourceRepoPath, objectPoolPath}, }, - gitcmd.WithRefTxHook(sourceRepo), + gitcmd.WithRefTxHook(sourceRepo, objectHash), gitcmd.WithStderr(&stderr), // When cloning an empty repository then Git isn't capable to figure out the correct // object hash that the new repository needs to use and just uses the default object diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go index ebb5449fc2..6c930a571b 100644 --- a/internal/git/objectpool/fetch.go +++ b/internal/git/objectpool/fetch.go @@ -66,6 +66,11 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo return fmt.Errorf("pruning references: %w", err) } + objectHash, err := o.Repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + var stderr bytes.Buffer if err := o.Repo.ExecAndWait(ctx, gitcmd.Command{ @@ -88,7 +93,7 @@ func (o *ObjectPool) FetchFromOrigin(ctx context.Context, origin *localrepo.Repo }, Args: []string{originPath, objectPoolRefspec}, }, - gitcmd.WithRefTxHook(o.Repo), + gitcmd.WithRefTxHook(o.Repo, objectHash), gitcmd.WithStderr(&stderr), gitcmd.WithConfig(gitcmd.ConfigPair{ // Git is so kind to point out that we asked it to not show forced updates diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go index 697bb1b988..cfd2e2e8ff 100644 --- a/internal/git/updateref/updateref.go +++ b/internal/git/updateref/updateref.go @@ -300,7 +300,12 @@ func New(ctx context.Context, repo gitcmd.RepositoryExecutor, opts ...UpdaterOpt opt(&cfg) } - txOption := gitcmd.WithRefTxHook(repo) + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return nil, fmt.Errorf("detecting object hash: %w", err) + } + + txOption := gitcmd.WithRefTxHook(repo, objectHash) if cfg.disableTransactions { txOption = gitcmd.WithDisabledHooks() } @@ -325,11 +330,6 @@ func New(ctx context.Context, repo gitcmd.RepositoryExecutor, opts ...UpdaterOpt return nil, err } - objectHash, err := repo.ObjectHash(ctx) - if err != nil { - return nil, fmt.Errorf("detecting object hash: %w", err) - } - referenceBackend, err := repo.ReferenceBackend(ctx) if err != nil { return nil, fmt.Errorf("detecting reference backend: %w", err) diff --git a/internal/gitaly/service/cleanup/rewrite_history.go b/internal/gitaly/service/cleanup/rewrite_history.go index 6d05ed820a..4be48149df 100644 --- a/internal/gitaly/service/cleanup/rewrite_history.go +++ b/internal/gitaly/service/cleanup/rewrite_history.go @@ -140,6 +140,11 @@ func (s *server) rewriteHistory( ) } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + var stderr strings.Builder if err := repo.ExecAndWait(ctx, gitcmd.Command{ @@ -167,7 +172,7 @@ func (s *server) rewriteHistory( git.MirrorRefSpec, ), }, - gitcmd.WithRefTxHook(repo), + gitcmd.WithRefTxHook(repo, objectHash), gitcmd.WithStderr(&stderr), gitcmd.WithConfig(gitcmd.ConfigPair{ Key: "advice.fetchShowForcedUpdates", Value: "false", diff --git a/internal/gitaly/service/operations/apply_patch.go b/internal/gitaly/service/operations/apply_patch.go index 33f90e40df..5d4eb9a211 100644 --- a/internal/gitaly/service/operations/apply_patch.go +++ b/internal/gitaly/service/operations/apply_patch.go @@ -109,7 +109,7 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP defer cancel() worktreeName := filepath.Base(worktreePath) - if err := s.removeWorktree(ctx, header.Repository, worktreeName); err != nil { + if err := s.removeWorktree(ctx, repo, worktreeName); err != nil { s.logger.WithField("worktree_name", worktreeName).WithError(err).ErrorContext(ctx, "failed to remove worktree") } }() @@ -134,7 +134,7 @@ func (s *Server) userApplyPatch(ctx context.Context, header *gitalypb.UserApplyP })), gitcmd.WithStdout(&stdout), gitcmd.WithStderr(&stderr), - gitcmd.WithRefTxHook(header.Repository), + gitcmd.WithRefTxHook(repo, objectHash), gitcmd.WithWorktree(worktreePath), ); err != nil { // The Ruby implementation doesn't include stderr in errors, which makes @@ -238,20 +238,30 @@ func (s *Server) addWorktree(ctx context.Context, repo *localrepo.Repo, worktree flags = append(flags, gitcmd.Flag{Name: "--no-checkout"}) } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + var stderr bytes.Buffer if err := repo.ExecAndWait(ctx, gitcmd.Command{ Name: "worktree", Action: "add", Flags: flags, Args: args, - }, gitcmd.WithStderr(&stderr), gitcmd.WithRefTxHook(repo)); err != nil { + }, gitcmd.WithStderr(&stderr), gitcmd.WithRefTxHook(repo, objectHash)); err != nil { return fmt.Errorf("adding worktree: %w", gitError{ErrMsg: stderr.String(), Err: err}) } return nil } -func (s *Server) removeWorktree(ctx context.Context, repo *gitalypb.Repository, worktreeName string) error { +func (s *Server) removeWorktree(ctx context.Context, repo *localrepo.Repo, worktreeName string) error { + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + cmd, err := s.gitCmdFactory.New(ctx, repo, gitcmd.Command{ Name: "worktree", @@ -259,7 +269,7 @@ func (s *Server) removeWorktree(ctx context.Context, repo *gitalypb.Repository, Flags: []gitcmd.Option{gitcmd.Flag{Name: "--force"}}, Args: []string{worktreeName}, }, - gitcmd.WithRefTxHook(repo), + gitcmd.WithRefTxHook(repo, objectHash), ) if err != nil { return fmt.Errorf("creation of 'worktree remove': %w", err) diff --git a/internal/gitaly/service/ref/find_tag.go b/internal/gitaly/service/ref/find_tag.go index debe94013a..e6f3765259 100644 --- a/internal/gitaly/service/ref/find_tag.go +++ b/internal/gitaly/service/ref/find_tag.go @@ -66,6 +66,12 @@ func parseTagLine(ctx context.Context, objectReader catfile.ObjectContentReader, } func (s *server) findTag(ctx context.Context, repo gitcmd.RepositoryExecutor, tagName []byte) (*gitalypb.Tag, error) { + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return nil, fmt.Errorf("detecting object hash: %w", err) + } + + tagCmd, err := repo.Exec(ctx, gitcmd.Command{ Name: "tag", @@ -75,7 +81,7 @@ func (s *server) findTag(ctx context.Context, repo gitcmd.RepositoryExecutor, ta }, Args: []string{string(tagName)}, }, - gitcmd.WithRefTxHook(repo), + gitcmd.WithRefTxHook(repo, objectHash), gitcmd.WithSetupStdout(), ) if err != nil { diff --git a/internal/gitaly/service/repository/create_fork.go b/internal/gitaly/service/repository/create_fork.go index 66bfd14e96..8d9eaaee67 100644 --- a/internal/gitaly/service/repository/create_fork.go +++ b/internal/gitaly/service/repository/create_fork.go @@ -30,8 +30,10 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest targetRepository := req.Repository sourceRepository := req.SourceRepository - if err := repoutil.Create(ctx, s.logger, s.locator, s.gitCmdFactory, s.txManager, s.repositoryCounter, targetRepository, func(repo *gitalypb.Repository) error { - targetPath, err := s.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) + if err := repoutil.Create(ctx, s.logger, s.locator, s.gitCmdFactory, s.txManager, s.repositoryCounter, targetRepository, func(repoProto *gitalypb.Repository) error { + repo := s.localrepo(repoProto) + + targetPath, err := s.locator.GetRepoPath(ctx, repoProto, storage.WithRepositoryVerificationSkipped()) if err != nil { return err } @@ -98,7 +100,7 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest return fmt.Errorf("fetching source repo: %w, stderr: %q", err, stderr.String()) } - if target, err := gitcmd.GetSymbolicRef(ctx, s.localrepo(repo), "HEAD"); err != nil { + if target, err := gitcmd.GetSymbolicRef(ctx, repo, "HEAD"); err != nil { return fmt.Errorf("checking whether HEAD reference is sane: %w", err) } else if req.GetRevision() != nil && target.Target != string(req.GetRevision()) { return structerr.NewInternal("HEAD points to unexpected reference").WithMetadata("expected_target", string(req.GetRevision())).WithMetadata("actual_target", target) diff --git a/internal/gitaly/service/repository/create_repository_from_url.go b/internal/gitaly/service/repository/create_repository_from_url.go index b52bea258c..a055993bf8 100644 --- a/internal/gitaly/service/repository/create_repository_from_url.go +++ b/internal/gitaly/service/repository/create_repository_from_url.go @@ -89,8 +89,10 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea return nil, structerr.NewInvalidArgument("%w", err) } - if err := repoutil.Create(ctx, s.logger, s.locator, s.gitCmdFactory, s.txManager, s.repositoryCounter, req.GetRepository(), func(repo *gitalypb.Repository) error { - targetPath, err := s.locator.GetRepoPath(ctx, repo, storage.WithRepositoryVerificationSkipped()) + if err := repoutil.Create(ctx, s.logger, s.locator, s.gitCmdFactory, s.txManager, s.repositoryCounter, req.GetRepository(), func(repoProto *gitalypb.Repository) error { + repo := s.localrepo(repoProto) + + targetPath, err := s.locator.GetRepoPath(ctx, repoProto, storage.WithRepositoryVerificationSkipped()) if err != nil { return fmt.Errorf("getting temporary repository path: %w", err) } diff --git a/internal/gitaly/service/repository/util.go b/internal/gitaly/service/repository/util.go index f2e3ae7363..019d52ae13 100644 --- a/internal/gitaly/service/repository/util.go +++ b/internal/gitaly/service/repository/util.go @@ -5,11 +5,18 @@ import ( "fmt" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" - "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" ) -func (s *server) removeOriginInRepo(ctx context.Context, repository *gitalypb.Repository) error { - cmd, err := s.gitCmdFactory.New(ctx, repository, gitcmd.Command{Name: "remote", Args: []string{"remove", "origin"}}, gitcmd.WithRefTxHook(repository)) +func (s *server) removeOriginInRepo(ctx context.Context, repo *localrepo.Repo) error { + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + + cmd, err := s.gitCmdFactory.New(ctx, repo, + gitcmd.Command{Name: "remote", Args: []string{"remove", "origin"}}, + gitcmd.WithRefTxHook(repo, objectHash)) if err != nil { return fmt.Errorf("remote cmd start: %w", err) } diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go index da9e354428..f0a3e021ba 100644 --- a/internal/gitaly/service/smarthttp/receive_pack.go +++ b/internal/gitaly/service/smarthttp/receive_pack.go @@ -3,6 +3,7 @@ package smarthttp import ( "context" "errors" + "fmt" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" @@ -76,7 +77,10 @@ func (s *server) postReceivePack( return stream.Send(&gitalypb.PostReceivePackResponse{Data: p}) }) - repoPath, err := s.locator.GetRepoPath(ctx, req.Repository) + repoProto := req.GetRepository() + repo := s.localrepo(repoProto) + + repoPath, err := repo.Path(ctx) if err != nil { return err } @@ -89,7 +93,6 @@ func (s *server) postReceivePack( transactionID := storage.ExtractTransactionID(ctx) transactionsEnabled := transactionID > 0 if transactionsEnabled { - repo := s.localrepo(req.GetRepository()) procReceiveCleanup, err := receivepack.RegisterProcReceiveHook( ctx, s.logger, s.cfg, req, repo, s.hookManager, hook.NewTransactionRegistry(s.txRegistry), transactionID, ) @@ -103,7 +106,12 @@ func (s *server) postReceivePack( }() } - cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + + cmd, err := s.gitCmdFactory.New(ctx, repo, gitcmd.Command{ Name: "receive-pack", Flags: []gitcmd.Option{gitcmd.Flag{Name: "--stateless-rpc"}}, @@ -111,7 +119,7 @@ func (s *server) postReceivePack( }, gitcmd.WithStdin(stdin), gitcmd.WithStdout(stdout), - gitcmd.WithReceivePackHooks(req, "http", transactionsEnabled), + gitcmd.WithReceivePackHooks(req, objectHash, "http", transactionsEnabled), gitcmd.WithGitProtocol(s.logger, req), gitcmd.WithConfig(config...), ) diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go index e5d2c4a686..6ac5d7a1e3 100644 --- a/internal/gitaly/service/smarthttp/upload_pack.go +++ b/internal/gitaly/service/smarthttp/upload_pack.go @@ -10,6 +10,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/bundleuri" "gitlab.com/gitlab-org/gitaly/v16/internal/command" "gitlab.com/gitlab-org/gitaly/v16/internal/git/gitcmd" + "gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v16/internal/git/stats" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/sidechannel" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" @@ -17,7 +18,10 @@ import ( ) func (s *server) PostUploadPackWithSidechannel(ctx context.Context, req *gitalypb.PostUploadPackWithSidechannelRequest) (*gitalypb.PostUploadPackWithSidechannelResponse, error) { - repoPath, gitConfig, err := s.validateUploadPackRequest(ctx, req) + repoProto := req.GetRepository() + repo := s.localrepo(repoProto) + + repoPath, gitConfig, err := s.validateUploadPackRequest(ctx, repo, req) if err != nil { return nil, structerr.NewInvalidArgument("%w", err) } @@ -35,7 +39,7 @@ func (s *server) PostUploadPackWithSidechannel(ctx context.Context, req *gitalyp } defer conn.Close() - stats, err := s.runUploadPack(ctx, req, repoPath, gitConfig, conn, conn) + stats, err := s.runUploadPack(ctx, req, repo, repoPath, gitConfig, conn, conn) if err != nil { return nil, structerr.NewInternal("running upload-pack: %w", err) } @@ -83,17 +87,16 @@ func (s *server) runStatsCollector(ctx context.Context, r io.Reader) (io.Reader, return io.TeeReader(r, pw), sc } -func (s *server) validateUploadPackRequest(ctx context.Context, req *gitalypb.PostUploadPackWithSidechannelRequest) (string, []gitcmd.ConfigPair, error) { - repository := req.GetRepository() - if err := s.locator.ValidateRepository(ctx, repository); err != nil { +func (s *server) validateUploadPackRequest(ctx context.Context, repo *localrepo.Repo, req *gitalypb.PostUploadPackWithSidechannelRequest) (string, []gitcmd.ConfigPair, error) { + if err := s.locator.ValidateRepository(ctx, repo); err != nil { return "", nil, err } - repoPath, err := s.locator.GetRepoPath(ctx, repository) + repoPath, err := repo.Path(ctx) if err != nil { return "", nil, err } - gitcmd.WarnIfTooManyBitmaps(ctx, s.logger, s.locator, repository.GetStorageName(), repoPath) + gitcmd.WarnIfTooManyBitmaps(ctx, s.logger, s.locator, repo.GetStorageName(), repoPath) config, err := gitcmd.ConvertConfigOptions(req.GetGitConfigOptions()) if err != nil { @@ -103,7 +106,15 @@ func (s *server) validateUploadPackRequest(ctx context.Context, req *gitalypb.Po return repoPath, config, nil } -func (s *server) runUploadPack(ctx context.Context, req *gitalypb.PostUploadPackWithSidechannelRequest, repoPath string, gitConfig []gitcmd.ConfigPair, stdin io.Reader, stdout io.Writer) (stats *stats.PackfileNegotiation, _ error) { +func (s *server) runUploadPack( + ctx context.Context, + req *gitalypb.PostUploadPackWithSidechannelRequest, + repo *localrepo.Repo, + repoPath string, + gitConfig []gitcmd.ConfigPair, + stdin io.Reader, + stdout io.Writer, +) (stats *stats.PackfileNegotiation, _ error) { h := sha1.New() stdin = io.TeeReader(stdin, h) @@ -116,18 +127,23 @@ func (s *server) runUploadPack(ctx context.Context, req *gitalypb.PostUploadPack gitConfig = append(gitConfig, bundleuri.CapabilitiesGitConfig(ctx)...) - uploadPackConfig, err := bundleuri.UploadPackGitConfig(ctx, s.bundleURISink, req.GetRepository()) + uploadPackConfig, err := bundleuri.UploadPackGitConfig(ctx, s.bundleURISink, repo) if err != nil { } else { gitConfig = append(gitConfig, uploadPackConfig...) } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return nil, fmt.Errorf("detecting object hash: %w", err) + } + commandOpts := []gitcmd.CmdOpt{ gitcmd.WithStdin(stdin), gitcmd.WithSetupStdout(), gitcmd.WithGitProtocol(s.logger, req), gitcmd.WithConfig(gitConfig...), - gitcmd.WithPackObjectsHookEnv(req.GetRepository(), "http"), + gitcmd.WithPackObjectsHookEnv(req.GetRepository(), objectHash, "http"), } cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), gitcmd.Command{ diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go index 7229dc2633..3fd136fe6f 100644 --- a/internal/gitaly/service/ssh/receive_pack.go +++ b/internal/gitaly/service/ssh/receive_pack.go @@ -86,7 +86,10 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, }) stderr = io.MultiWriter(&stderrBuilder, stderr) - repoPath, err := s.locator.GetRepoPath(ctx, req.Repository) + repoProto := req.GetRepository() + repo := s.localrepo(repoProto) + + repoPath, err := repo.Path(ctx) if err != nil { return err } @@ -122,7 +125,6 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, transactionID := storage.ExtractTransactionID(ctx) transactionsEnabled := transactionID > 0 if transactionsEnabled { - repo := s.localrepo(req.GetRepository()) procReceiveCleanup, err := receivepack.RegisterProcReceiveHook( ctx, s.logger, s.cfg, req, repo, s.hookManager, hook.NewTransactionRegistry(s.txRegistry), transactionID, ) @@ -136,7 +138,12 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, }() } - cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(), + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return fmt.Errorf("detecting object hash: %w", err) + } + + cmd, err := s.gitCmdFactory.New(ctx, repoProto, gitcmd.Command{ Name: "receive-pack", Args: []string{repoPath}, @@ -144,7 +151,7 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer, gitcmd.WithStdin(pr), gitcmd.WithStdout(stdout), gitcmd.WithStderr(stderr), - gitcmd.WithReceivePackHooks(req, "ssh", transactionsEnabled), + gitcmd.WithReceivePackHooks(req, objectHash, "ssh", transactionsEnabled), gitcmd.WithGitProtocol(s.logger, req), gitcmd.WithConfig(config...), ) diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go index cde3cdde7c..29600e7977 100644 --- a/internal/gitaly/service/ssh/upload_pack.go +++ b/internal/gitaly/service/ssh/upload_pack.go @@ -74,13 +74,15 @@ type sshUploadPackRequest interface { } func (s *server) sshUploadPack(ctx context.Context, req sshUploadPackRequest, stdin io.Reader, stdout, stderr io.Writer) (negotiation *stats.PackfileNegotiation, _ int, _ error) { - repo := req.GetRepository() - repoPath, err := s.locator.GetRepoPath(ctx, repo) + repoProto := req.GetRepository() + repo := s.localrepo(repoProto) + + repoPath, err := repo.Path(ctx) if err != nil { return nil, 0, err } - gitcmd.WarnIfTooManyBitmaps(ctx, s.logger, s.locator, repo.StorageName, repoPath) + gitcmd.WarnIfTooManyBitmaps(ctx, s.logger, s.locator, repoProto.StorageName, repoPath) config, err := gitcmd.ConvertConfigOptions(req.GetGitConfigOptions()) if err != nil { @@ -114,17 +116,22 @@ func (s *server) sshUploadPack(ctx context.Context, req sshUploadPackRequest, st config = append(config, bundleuri.CapabilitiesGitConfig(ctx)...) - uploadPackConfig, err := bundleuri.UploadPackGitConfig(ctx, s.bundleURISink, req.GetRepository()) + uploadPackConfig, err := bundleuri.UploadPackGitConfig(ctx, s.bundleURISink, repo) if err != nil { log.AddFields(ctx, log.Fields{"bundle_uri_error": err}) } else { config = append(config, uploadPackConfig...) } + objectHash, err := repo.ObjectHash(ctx) + if err != nil { + return nil, 0, fmt.Errorf("detecting object hash: %w", err) + } + commandOpts := []gitcmd.CmdOpt{ gitcmd.WithGitProtocol(s.logger, req), gitcmd.WithConfig(config...), - gitcmd.WithPackObjectsHookEnv(repo, "ssh"), + gitcmd.WithPackObjectsHookEnv(repoProto, objectHash, "ssh"), } timeoutTicker := s.uploadPackRequestTimeoutTickerFactory() @@ -135,7 +142,7 @@ func (s *server) sshUploadPack(ctx context.Context, req sshUploadPackRequest, st // "flush" tells the server it can terminate, while "done" tells it to start // generating a packfile. Add a timeout to the second case to mitigate // use-after-check attacks. - if err := s.runUploadCommand(ctx, repo, stdin, stdout, stderr, timeoutTicker, pktline.PktDone(), gitcmd.Command{ + if err := s.runUploadCommand(ctx, repoProto, stdin, stdout, stderr, timeoutTicker, pktline.PktDone(), gitcmd.Command{ Name: "upload-pack", Args: []string{repoPath}, }, commandOpts...); err != nil { -- GitLab