From 570ad65f9f4567428ee5214a470a1f97146d58c8 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Thu, 8 Aug 2019 15:49:09 +0800 Subject: Implement AuthorizedKeys command Build this command when `Executable` name is `gitlab-shell-authorized-keys-check`. Feature flag is the same name. --- .../command/authorizedkeys/authorized_keys.go | 61 ++++++++++++ .../command/authorizedkeys/authorized_keys_test.go | 90 ++++++++++++++++++ go/internal/command/command.go | 11 +++ go/internal/command/command_test.go | 11 +++ go/internal/command/commandargs/authorized_keys.go | 51 ++++++++++ go/internal/command/commandargs/command_args.go | 2 + .../command/commandargs/command_args_test.go | 33 ++++++- go/internal/executable/executable.go | 1 + go/internal/gitlabnet/authorizedkeys/client.go | 65 +++++++++++++ .../gitlabnet/authorizedkeys/client_test.go | 105 +++++++++++++++++++++ go/internal/keyline/key_line.go | 53 +++++++++++ go/internal/keyline/key_line_test.go | 51 ++++++++++ spec/gitlab_shell_authorized_keys_check_spec.rb | 2 +- 13 files changed, 533 insertions(+), 3 deletions(-) create mode 100644 go/internal/command/authorizedkeys/authorized_keys.go create mode 100644 go/internal/command/authorizedkeys/authorized_keys_test.go create mode 100644 go/internal/command/commandargs/authorized_keys.go create mode 100644 go/internal/gitlabnet/authorizedkeys/client.go create mode 100644 go/internal/gitlabnet/authorizedkeys/client_test.go create mode 100644 go/internal/keyline/key_line.go create mode 100644 go/internal/keyline/key_line_test.go diff --git a/go/internal/command/authorizedkeys/authorized_keys.go b/go/internal/command/authorizedkeys/authorized_keys.go new file mode 100644 index 0000000..d5837b0 --- /dev/null +++ b/go/internal/command/authorizedkeys/authorized_keys.go @@ -0,0 +1,61 @@ +package authorizedkeys + +import ( + "fmt" + "strconv" + + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/readwriter" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/config" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet/authorizedkeys" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/keyline" +) + +type Command struct { + Config *config.Config + Args *commandargs.AuthorizedKeys + ReadWriter *readwriter.ReadWriter +} + +func (c *Command) Execute() error { + // Do and return nothing when the expected and actual user don't match. + // This can happen when the user in sshd_config doesn't match the user + // trying to login. When nothing is printed, the user will be denied access. + if c.Args.ExpectedUser != c.Args.ActualUser { + // TODO: Log this event once we have a consistent way to log in Go. + // See https://gitlab.com/gitlab-org/gitlab-shell/issues/192 for more info. + return nil + } + + if err := c.printKeyLine(); err != nil { + return err + } + + return nil +} + +func (c *Command) printKeyLine() error { + response, err := c.getAuthorizedKey() + if err != nil { + fmt.Fprintln(c.ReadWriter.Out, fmt.Sprintf("# No key was found for %s", c.Args.Key)) + return nil + } + + keyLine, err := keyline.NewPublicKeyLine(strconv.FormatInt(response.Id, 10), response.Key, c.Config.RootDir) + if err != nil { + return err + } + + fmt.Fprintln(c.ReadWriter.Out, keyLine.ToString()) + + return nil +} + +func (c *Command) getAuthorizedKey() (*authorizedkeys.Response, error) { + client, err := authorizedkeys.NewClient(c.Config) + if err != nil { + return nil, err + } + + return client.GetByKey(c.Args.Key) +} diff --git a/go/internal/command/authorizedkeys/authorized_keys_test.go b/go/internal/command/authorizedkeys/authorized_keys_test.go new file mode 100644 index 0000000..5cde366 --- /dev/null +++ b/go/internal/command/authorizedkeys/authorized_keys_test.go @@ -0,0 +1,90 @@ +package authorizedkeys + +import ( + "bytes" + "encoding/json" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/readwriter" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/config" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet/testserver" +) + +var ( + requests = []testserver.TestRequestHandler{ + { + Path: "/api/v4/internal/authorized_keys", + Handler: func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("key") == "key" { + body := map[string]interface{}{ + "id": 1, + "key": "public-key", + } + json.NewEncoder(w).Encode(body) + } else if r.URL.Query().Get("key") == "broken-message" { + body := map[string]string{ + "message": "Forbidden!", + } + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(body) + } else if r.URL.Query().Get("key") == "broken" { + w.WriteHeader(http.StatusInternalServerError) + } else { + w.WriteHeader(http.StatusNotFound) + } + }, + }, + } +) + +func TestExecute(t *testing.T) { + url, cleanup := testserver.StartSocketHttpServer(t, requests) + defer cleanup() + + testCases := []struct { + desc string + arguments *commandargs.AuthorizedKeys + expectedOutput string + }{ + { + desc: "With matching username and key", + arguments: &commandargs.AuthorizedKeys{ExpectedUser: "user", ActualUser: "user", Key: "key"}, + expectedOutput: "command=\"/tmp/bin/gitlab-shell key-1\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty public-key\n", + }, + { + desc: "When key doesn't match any existing key", + arguments: &commandargs.AuthorizedKeys{ExpectedUser: "user", ActualUser: "user", Key: "not-found"}, + expectedOutput: "# No key was found for not-found\n", + }, + { + desc: "When the API returns an error", + arguments: &commandargs.AuthorizedKeys{ExpectedUser: "user", ActualUser: "user", Key: "broken-message"}, + expectedOutput: "# No key was found for broken-message\n", + }, + { + desc: "When the API fails", + arguments: &commandargs.AuthorizedKeys{ExpectedUser: "user", ActualUser: "user", Key: "broken"}, + expectedOutput: "# No key was found for broken\n", + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + buffer := &bytes.Buffer{} + cmd := &Command{ + Config: &config.Config{RootDir: "/tmp", GitlabUrl: url}, + Args: tc.arguments, + ReadWriter: &readwriter.ReadWriter{Out: buffer}, + } + + err := cmd.Execute() + + require.NoError(t, err) + require.Equal(t, tc.expectedOutput, buffer.String()) + }) + } +} diff --git a/go/internal/command/command.go b/go/internal/command/command.go index 27378aa..d6b96f1 100644 --- a/go/internal/command/command.go +++ b/go/internal/command/command.go @@ -1,6 +1,7 @@ package command import ( + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedkeys" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/commandargs" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/discover" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback" @@ -35,6 +36,8 @@ func buildCommand(e *executable.Executable, args commandargs.CommandArgs, config switch e.Name { case executable.GitlabShell: return buildShellCommand(args.(*commandargs.Shell), config, readWriter) + case executable.AuthorizedKeysCheck: + return buildAuthorizedKeysCommand(args.(*commandargs.AuthorizedKeys), config, readWriter) } return nil @@ -62,3 +65,11 @@ func buildShellCommand(args *commandargs.Shell, config *config.Config, readWrite return nil } + +func buildAuthorizedKeysCommand(args *commandargs.AuthorizedKeys, config *config.Config, readWriter *readwriter.ReadWriter) Command { + if !config.FeatureEnabled(executable.AuthorizedKeysCheck) { + return nil + } + + return &authorizedkeys.Command{Config: config, Args: args, ReadWriter: readWriter} +} diff --git a/go/internal/command/command_test.go b/go/internal/command/command_test.go index ea88a6a..b651c78 100644 --- a/go/internal/command/command_test.go +++ b/go/internal/command/command_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/authorizedkeys" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/discover" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/fallback" "gitlab.com/gitlab-org/gitlab-shell/go/internal/command/lfsauthenticate" @@ -124,6 +125,16 @@ func TestNew(t *testing.T) { arguments: []string{}, expectedType: &uploadarchive.Command{}, }, + { + desc: "it returns a AuthorizedKeys command if the feature is enabled", + executable: &executable.Executable{Name: executable.AuthorizedKeysCheck}, + config: &config.Config{ + Migration: config.MigrationConfig{Enabled: true, Features: []string{"gitlab-shell-authorized-keys-check"}}, + }, + environment: map[string]string{}, + arguments: []string{"git", "git", "key"}, + expectedType: &authorizedkeys.Command{}, + }, { desc: "it returns a Fallback command if the feature is unimplemented", executable: &executable.Executable{Name: executable.GitlabShell}, diff --git a/go/internal/command/commandargs/authorized_keys.go b/go/internal/command/commandargs/authorized_keys.go new file mode 100644 index 0000000..2733954 --- /dev/null +++ b/go/internal/command/commandargs/authorized_keys.go @@ -0,0 +1,51 @@ +package commandargs + +import ( + "errors" + "fmt" +) + +type AuthorizedKeys struct { + Arguments []string + ExpectedUser string + ActualUser string + Key string +} + +func (ak *AuthorizedKeys) Parse() error { + if err := ak.validate(); err != nil { + return err + } + + ak.ExpectedUser = ak.Arguments[0] + ak.ActualUser = ak.Arguments[1] + ak.Key = ak.Arguments[2] + + return nil +} + +func (ak *AuthorizedKeys) GetArguments() []string { + return ak.Arguments +} + +func (ak *AuthorizedKeys) validate() error { + argsSize := len(ak.Arguments) + + if argsSize != 3 { + return errors.New(fmt.Sprintf("# Insufficient arguments. %d. Usage\n#\tgitlab-shell-authorized-keys-check ", argsSize)) + } + + expectedUsername := ak.Arguments[0] + actualUsername := ak.Arguments[1] + key := ak.Arguments[2] + + if expectedUsername == "" || actualUsername == "" { + return errors.New("# No username provided") + } + + if key == "" { + return errors.New("# No key provided") + } + + return nil +} diff --git a/go/internal/command/commandargs/command_args.go b/go/internal/command/commandargs/command_args.go index 5338d6b..dfdf8e5 100644 --- a/go/internal/command/commandargs/command_args.go +++ b/go/internal/command/commandargs/command_args.go @@ -17,6 +17,8 @@ func Parse(e *executable.Executable, arguments []string) (CommandArgs, error) { switch e.Name { case executable.GitlabShell: args = &Shell{Arguments: arguments} + case executable.AuthorizedKeysCheck: + args = &AuthorizedKeys{Arguments: arguments} } if err := args.Parse(); err != nil { diff --git a/go/internal/command/commandargs/command_args_test.go b/go/internal/command/commandargs/command_args_test.go index 148c987..e981c22 100644 --- a/go/internal/command/commandargs/command_args_test.go +++ b/go/internal/command/commandargs/command_args_test.go @@ -119,6 +119,11 @@ func TestParseSuccess(t *testing.T) { }, arguments: []string{}, expectedArgs: &Shell{Arguments: []string{}, SshArgs: []string{"git-lfs-authenticate", "group/repo", "download"}, CommandType: LfsAuthenticate}, + }, { + desc: "It parses authorized-keys command", + executable: &executable.Executable{Name: executable.AuthorizedKeysCheck}, + arguments: []string{"git", "git", "key"}, + expectedArgs: &AuthorizedKeys{Arguments: []string{"git", "git", "key"}, ExpectedUser: "git", ActualUser: "git", Key: "key"}, }, { desc: "Unknown executable", executable: &executable.Executable{Name: "unknown"}, @@ -162,7 +167,31 @@ func TestParseFailure(t *testing.T) { "SSH_ORIGINAL_COMMAND": `git receive-pack "`, }, arguments: []string{}, - expectedError: "Invalid SSH allowed", + expectedError: "Invalid SSH command", + }, + { + desc: "With not enough arguments for the AuthorizedKeysCheck", + executable: &executable.Executable{Name: executable.AuthorizedKeysCheck}, + arguments: []string{"user"}, + expectedError: "# Insufficient arguments. 1. Usage\n#\tgitlab-shell-authorized-keys-check ", + }, + { + desc: "With too many arguments for the AuthorizedKeysCheck", + executable: &executable.Executable{Name: executable.AuthorizedKeysCheck}, + arguments: []string{"user", "user", "key", "something-else"}, + expectedError: "# Insufficient arguments. 4. Usage\n#\tgitlab-shell-authorized-keys-check ", + }, + { + desc: "With missing username for the AuthorizedKeysCheck", + executable: &executable.Executable{Name: executable.AuthorizedKeysCheck}, + arguments: []string{"user", "", "key"}, + expectedError: "# No username provided", + }, + { + desc: "With missing key for the AuthorizedKeysCheck", + executable: &executable.Executable{Name: executable.AuthorizedKeysCheck}, + arguments: []string{"user", "user", ""}, + expectedError: "# No key provided", }, } @@ -173,7 +202,7 @@ func TestParseFailure(t *testing.T) { _, err := Parse(tc.executable, tc.arguments) - require.Error(t, err, tc.expectedError) + require.EqualError(t, err, tc.expectedError) }) } } diff --git a/go/internal/executable/executable.go b/go/internal/executable/executable.go index 63f4c90..ef07115 100644 --- a/go/internal/executable/executable.go +++ b/go/internal/executable/executable.go @@ -6,6 +6,7 @@ import ( ) const ( + BinDir = "bin" GitlabShell = "gitlab-shell" AuthorizedKeysCheck = "gitlab-shell-authorized-keys-check" AuthorizedPrincipalsCheck = "gitlab-shell-authorized-principals-check" diff --git a/go/internal/gitlabnet/authorizedkeys/client.go b/go/internal/gitlabnet/authorizedkeys/client.go new file mode 100644 index 0000000..28b85fc --- /dev/null +++ b/go/internal/gitlabnet/authorizedkeys/client.go @@ -0,0 +1,65 @@ +package authorizedkeys + +import ( + "fmt" + "net/url" + + "gitlab.com/gitlab-org/gitlab-shell/go/internal/config" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet" +) + +const ( + AuthorizedKeysPath = "/authorized_keys" +) + +type Client struct { + config *config.Config + client *gitlabnet.GitlabClient +} + +type Response struct { + Id int64 `json:"id"` + Key string `json:"key"` +} + +func NewClient(config *config.Config) (*Client, error) { + client, err := gitlabnet.GetClient(config) + if err != nil { + return nil, fmt.Errorf("Error creating http client: %v", err) + } + + return &Client{config: config, client: client}, nil +} + +func (c *Client) GetByKey(key string) (*Response, error) { + path, err := pathWithKey(key) + if err != nil { + return nil, err + } + + response, err := c.client.Get(path) + if err != nil { + return nil, err + } + defer response.Body.Close() + + parsedResponse := &Response{} + if err := gitlabnet.ParseJSON(response, parsedResponse); err != nil { + return nil, err + } + + return parsedResponse, nil +} + +func pathWithKey(key string) (string, error) { + u, err := url.Parse(AuthorizedKeysPath) + if err != nil { + return "", err + } + + params := u.Query() + params.Set("key", key) + u.RawQuery = params.Encode() + + return u.String(), nil +} diff --git a/go/internal/gitlabnet/authorizedkeys/client_test.go b/go/internal/gitlabnet/authorizedkeys/client_test.go new file mode 100644 index 0000000..c73aab2 --- /dev/null +++ b/go/internal/gitlabnet/authorizedkeys/client_test.go @@ -0,0 +1,105 @@ +package authorizedkeys + +import ( + "encoding/json" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-shell/go/internal/config" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet" + "gitlab.com/gitlab-org/gitlab-shell/go/internal/gitlabnet/testserver" +) + +var ( + requests []testserver.TestRequestHandler +) + +func init() { + requests = []testserver.TestRequestHandler{ + { + Path: "/api/v4/internal/authorized_keys", + Handler: func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("key") == "key" { + body := &Response{ + Id: 1, + Key: "public-key", + } + json.NewEncoder(w).Encode(body) + } else if r.URL.Query().Get("key") == "broken-message" { + w.WriteHeader(http.StatusForbidden) + body := &gitlabnet.ErrorResponse{ + Message: "Not allowed!", + } + json.NewEncoder(w).Encode(body) + } else if r.URL.Query().Get("key") == "broken-json" { + w.Write([]byte("{ \"message\": \"broken json!\"")) + } else if r.URL.Query().Get("key") == "broken-empty" { + w.WriteHeader(http.StatusForbidden) + } else { + w.WriteHeader(http.StatusNotFound) + } + }, + }, + } +} + +func TestGetByKey(t *testing.T) { + client, cleanup := setup(t) + defer cleanup() + + result, err := client.GetByKey("key") + require.NoError(t, err) + require.Equal(t, &Response{Id: 1, Key: "public-key"}, result) +} + +func TestGetByKeyErrorResponses(t *testing.T) { + client, cleanup := setup(t) + defer cleanup() + + testCases := []struct { + desc string + key string + expectedError string + }{ + { + desc: "A response with an error message", + key: "broken-message", + expectedError: "Not allowed!", + }, + { + desc: "A response with bad JSON", + key: "broken-json", + expectedError: "Parsing failed", + }, + { + desc: "A forbidden (403) response without message", + key: "broken-empty", + expectedError: "Internal API error (403)", + }, + { + desc: "A not found (404) response without message", + key: "not-found", + expectedError: "Internal API error (404)", + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + resp, err := client.GetByKey(tc.key) + + require.EqualError(t, err, tc.expectedError) + require.Nil(t, resp) + }) + } +} + +func setup(t *testing.T) (*Client, func()) { + url, cleanup := testserver.StartSocketHttpServer(t, requests) + + client, err := NewClient(&config.Config{GitlabUrl: url}) + require.NoError(t, err) + + return client, cleanup +} diff --git a/go/internal/keyline/key_line.go b/go/internal/keyline/key_line.go new file mode 100644 index 0000000..7b19c87 --- /dev/null +++ b/go/internal/keyline/key_line.go @@ -0,0 +1,53 @@ +package keyline + +import ( + "errors" + "fmt" + "path" + "regexp" + "strings" + + "gitlab.com/gitlab-org/gitlab-shell/go/internal/executable" +) + +var ( + keyRegex = regexp.MustCompile(`\A[a-z0-9-]+\z`) +) + +const ( + PublicKeyPrefix = "key" + SshOptions = "no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty" +) + +type KeyLine struct { + Id string // This can be either an ID of a Key or username + Value string // This can be either a public key or a principal name + Prefix string + RootDir string +} + +func NewPublicKeyLine(id string, publicKey string, rootDir string) (*KeyLine, error) { + if err := validate(id, publicKey); err != nil { + return nil, err + } + + return &KeyLine{Id: id, Value: publicKey, Prefix: PublicKeyPrefix, RootDir: rootDir}, nil +} + +func (k *KeyLine) ToString() string { + command := fmt.Sprintf("%s %s-%s", path.Join(k.RootDir, executable.BinDir, executable.GitlabShell), k.Prefix, k.Id) + + return fmt.Sprintf(`command="%s",%s %s`, command, SshOptions, k.Value) +} + +func validate(id string, value string) error { + if !keyRegex.MatchString(id) { + return errors.New(fmt.Sprintf("Invalid key_id: %s", id)) + } + + if strings.Contains(value, "\n") { + return errors.New(fmt.Sprintf("Invalid value: %s", value)) + } + + return nil +} diff --git a/go/internal/keyline/key_line_test.go b/go/internal/keyline/key_line_test.go new file mode 100644 index 0000000..fc2bdf3 --- /dev/null +++ b/go/internal/keyline/key_line_test.go @@ -0,0 +1,51 @@ +package keyline + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestFailingNewPublicKeyLine(t *testing.T) { + testCases := []struct { + desc string + id string + publicKey string + expectedError string + }{ + { + desc: "When Id has non-alphanumeric and non-dash characters in it", + id: "key\n1", + publicKey: "public-key", + expectedError: "Invalid key_id: key\n1", + }, + { + desc: "When public key has newline in it", + id: "key", + publicKey: "public\nkey", + expectedError: "Invalid value: public\nkey", + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + result, err := NewPublicKeyLine(tc.id, tc.publicKey, "root-dir") + + require.Empty(t, result) + require.EqualError(t, err, tc.expectedError) + }) + } +} + +func TestToString(t *testing.T) { + keyLine := &KeyLine{ + Id: "1", + Value: "public-key", + Prefix: "key", + RootDir: "/tmp", + } + + result := keyLine.ToString() + + require.Equal(t, `command="/tmp/bin/gitlab-shell key-1",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty public-key`, result) +} diff --git a/spec/gitlab_shell_authorized_keys_check_spec.rb b/spec/gitlab_shell_authorized_keys_check_spec.rb index eb0cbca..91b874d 100644 --- a/spec/gitlab_shell_authorized_keys_check_spec.rb +++ b/spec/gitlab_shell_authorized_keys_check_spec.rb @@ -87,7 +87,7 @@ describe 'bin/gitlab-shell-authorized-keys-check' do it_behaves_like 'authorized keys check' end - pending 'with the go authorized-keys-check feature', :go do + describe 'with the go authorized-keys-check feature', :go do before(:all) do write_config( 'gitlab_url' => "http+unix://#{CGI.escape(tmp_socket_path)}", -- cgit v1.2.1