From 354f5bf20c3d1b48481bd4e6f4d4219f830b986b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 19 Oct 2020 13:06:59 -0700 Subject: Fix incorrect actor used to check permissions for SSH receive-pack During a SSH receive-pack request (e.g. `git push`), gitlab-shell was incorrectly using the user returned by the `/internal/allowed` API endpoint to make an SSHReceivePack RPC call. This caused a number of problems with deploy keys with write access: 1. Keys that were generated by a blocked user would be denied the ability to write. 2. Keys that were generated by user that did not have write access to the project would also be denied. GitLab 12.4 removed the Ruby implementation of gitlab-shell in favor of the Golang implementation, and these implementations worked slightly differently. In https://gitlab.com/gitlab-org/gitlab-shell/blob/v10.1.0/lib/gitlab_shell.rb, the Ruby implementation would always use `@who` (e.g. `key-123`), but in gitlab-shell v10.2.0 the Go implementation would always use the user from the API response. Reads did not have this issue because the user/deploy key is never passed to Gitaly for additional permission checks. Writes need this information for the pre-receive to check access to protected branches, push rules, etc. Relates to https://gitlab.com/gitlab-org/gitlab-shell/-/issues/479 --- internal/command/receivepack/gitalycall.go | 2 +- internal/command/receivepack/gitalycall_test.go | 72 ++++++++++++++++--------- 2 files changed, 49 insertions(+), 25 deletions(-) diff --git a/internal/command/receivepack/gitalycall.go b/internal/command/receivepack/gitalycall.go index 0754a3e..a983c1a 100644 --- a/internal/command/receivepack/gitalycall.go +++ b/internal/command/receivepack/gitalycall.go @@ -24,7 +24,7 @@ func (c *Command) performGitalyCall(response *accessverifier.Response) error { request := &pb.SSHReceivePackRequest{ Repository: &response.Gitaly.Repo, - GlId: response.UserId, + GlId: response.Who, GlRepository: response.Repo, GlUsername: response.Username, GitProtocol: os.Getenv(commandargs.GitProtocolEnv), diff --git a/internal/command/receivepack/gitalycall_test.go b/internal/command/receivepack/gitalycall_test.go index 2a0c146..ea07477 100644 --- a/internal/command/receivepack/gitalycall_test.go +++ b/internal/command/receivepack/gitalycall_test.go @@ -29,33 +29,57 @@ func TestReceivePack(t *testing.T) { require.NoError(t, err) defer envCleanup() - output := &bytes.Buffer{} - input := &bytes.Buffer{} + testCases := []struct { + username string + keyId string + }{ + { + username: "john.doe", + }, + { + keyId: "123", + }, + } - userId := "1" - repo := "group/repo" + for _, tc := range testCases { + output := &bytes.Buffer{} + input := &bytes.Buffer{} + repo := "group/repo" - cmd := &Command{ - Config: &config.Config{GitlabUrl: url}, - Args: &commandargs.Shell{GitlabKeyId: userId, CommandType: commandargs.ReceivePack, SshArgs: []string{"git-receive-pack", repo}}, - ReadWriter: &readwriter.ReadWriter{ErrOut: output, Out: output, In: input}, - } + args := &commandargs.Shell{CommandType: commandargs.ReceivePack, SshArgs: []string{"git-receive-pack", repo}} - hook := testhelper.SetupLogger() + if tc.username != "" { + args.GitlabUsername = tc.username + } else { + args.GitlabKeyId = tc.keyId + } - err = cmd.Execute(context.Background()) - require.NoError(t, err) + cmd := &Command{ + Config: &config.Config{GitlabUrl: url}, + Args: args, + ReadWriter: &readwriter.ReadWriter{ErrOut: output, Out: output, In: input}, + } + + hook := testhelper.SetupLogger() - require.Equal(t, "ReceivePack: "+userId+" "+repo, output.String()) - - require.True(t, testhelper.WaitForLogEvent(hook)) - entries := hook.AllEntries() - require.Equal(t, 2, len(entries)) - require.Equal(t, logrus.InfoLevel, entries[1].Level) - require.Contains(t, entries[1].Message, "executing git command") - require.Contains(t, entries[1].Message, "command=git-receive-pack") - require.Contains(t, entries[1].Message, "remote_ip=127.0.0.1") - require.Contains(t, entries[1].Message, "gl_key_type=key") - require.Contains(t, entries[1].Message, "gl_key_id=123") - require.Contains(t, entries[1].Message, "correlation_id=") + err = cmd.Execute(context.Background()) + require.NoError(t, err) + + if tc.username != "" { + require.Equal(t, "ReceivePack: 1 "+repo, output.String()) + } else { + require.Equal(t, "ReceivePack: key-123 "+repo, output.String()) + } + + require.True(t, testhelper.WaitForLogEvent(hook)) + entries := hook.AllEntries() + require.Equal(t, 2, len(entries)) + require.Equal(t, logrus.InfoLevel, entries[1].Level) + require.Contains(t, entries[1].Message, "executing git command") + require.Contains(t, entries[1].Message, "command=git-receive-pack") + require.Contains(t, entries[1].Message, "remote_ip=127.0.0.1") + require.Contains(t, entries[1].Message, "gl_key_type=key") + require.Contains(t, entries[1].Message, "gl_key_id=123") + require.Contains(t, entries[1].Message, "correlation_id=") + } } -- cgit v1.2.1