As we reevaluate how to best support and maintain Staging Ref in the future, we encourage development teams using this environment to highlight their use cases in the following issue: https://gitlab.com/gitlab-com/gl-infra/software-delivery/framework/software-delivery-framework-issue-tracker/-/issues/36.

Skip to content
Snippets Groups Projects
Commit 354f5bf2 authored by Stan Hu's avatar Stan Hu
Browse files

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
parent c09bdad6
No related branches found
No related tags found
No related merge requests found
Loading
Loading
@@ -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),
Loading
Loading
Loading
Loading
@@ -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=")
}
}
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment