diff --git a/internal/command/githttp/push_test.go b/internal/command/githttp/push_test.go index 215b11c2d4b3611933372839b8bf21bb0dce194d..942029532dbbde745c6c88f1b3d9dc09d89fcb9e 100644 --- a/internal/command/githttp/push_test.go +++ b/internal/command/githttp/push_test.go @@ -131,7 +131,6 @@ func TestPushExecuteWithSSHReceivePack(t *testing.T) { Payload: accessverifier.CustomPayload{ Data: accessverifier.CustomPayloadData{ PrimaryRepo: url, - GeoProxyDirectToPrimary: true, GeoProxyPushSSHDirectToPrimary: true, RequestHeaders: map[string]string{"Authorization": "token"}, }, diff --git a/internal/command/receivepack/receivepack.go b/internal/command/receivepack/receivepack.go index 6cb5692aac42d9a26aaea303283af3df310ff8a8..2d2da6a9cf17c0581978e5306b15609b86646cbd 100644 --- a/internal/command/receivepack/receivepack.go +++ b/internal/command/receivepack/receivepack.go @@ -10,7 +10,6 @@ import ( "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/githttp" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/readwriter" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/accessverifier" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/customaction" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/shared/disallowedcommand" "gitlab.com/gitlab-org/gitlab-shell/v14/internal/config" ) @@ -45,27 +44,16 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) { )) if response.IsCustomAction() { - // When `geo_proxy_direct_to_primary` feature flag is enabled, a Git over HTTP direct request - // to primary repo is performed instead of proxying the request through Gitlab Rails. - // After the feature flag is enabled by default and removed, - // custom action functionality will be removed along with it. - if response.Payload.Data.GeoProxyDirectToPrimary { - cmd := githttp.PushCommand{ - Config: c.Config, - ReadWriter: c.ReadWriter, - Response: response, - Args: c.Args, - } - - return ctxWithLogData, cmd.Execute(ctx) - } - - customAction := customaction.Command{ + // A Git over HTTP direct request to primary repo is performed + // (instead of formerly proxying the request through Gitlab Rails). + cmd := githttp.PushCommand{ Config: c.Config, ReadWriter: c.ReadWriter, - EOFSent: true, + Response: response, + Args: c.Args, } - return ctxWithLogData, customAction.Execute(ctx, response) + + return ctxWithLogData, cmd.Execute(ctx) } err = c.performGitalyCall(ctx, response) diff --git a/internal/command/receivepack/receivepack_test.go b/internal/command/receivepack/receivepack_test.go index d3dc18208003e7a067e4aa4360106a0fa9ca582e..815fdd0203a3b81ec2e1f8f4c05758fd71acd8a4 100644 --- a/internal/command/receivepack/receivepack_test.go +++ b/internal/command/receivepack/receivepack_test.go @@ -43,7 +43,9 @@ func TestCustomReceivePack(t *testing.T) { _, err := cmd.Execute(context.Background()) require.NoError(t, err) - require.Equal(t, "customoutput", output.String()) + + // Output of the Git HTTP protocol + require.Contains(t, output.String(), "ok refs/heads/master") } func setup(t *testing.T, keyID string, requests []testserver.TestRequestHandler) (*Command, *bytes.Buffer) { diff --git a/internal/command/shared/customaction/customaction.go b/internal/command/shared/customaction/customaction.go deleted file mode 100644 index 6d160c28171b4b857ff7e27ba10dd3e78a0915bc..0000000000000000000000000000000000000000 --- a/internal/command/shared/customaction/customaction.go +++ /dev/null @@ -1,171 +0,0 @@ -// Package customaction provides functionality for handling custom actions -package customaction - -import ( - "bytes" - "context" - "errors" - "io" - "net/http" - - "gitlab.com/gitlab-org/labkit/log" - - "gitlab.com/gitlab-org/gitlab-shell/v14/client" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/readwriter" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/config" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/accessverifier" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/pktline" -) - -// Request represents the request structure for custom actions -type Request struct { - SecretToken []byte `json:"secret_token"` - Data accessverifier.CustomPayloadData `json:"data"` - Output []byte `json:"output"` -} - -// Response represents the response structure for custom actions -type Response struct { - Result []byte `json:"result"` - Message string `json:"message"` -} - -// Command represents the custom action command -type Command struct { - Config *config.Config - ReadWriter *readwriter.ReadWriter - EOFSent bool -} - -// Execute method runs when `geo_proxy_direct_to_primary` feature flag is enabled, a Git over HTTP direct request -// to primary repo is performed instead of proxying the request through Gitlab Rails. -// After the feature flag is enabled by default and removed, this package will be removed along with it. -func (c *Command) Execute(ctx context.Context, response *accessverifier.Response) error { - data := response.Payload.Data - apiEndpoints := data.APIEndpoints - - if len(apiEndpoints) == 0 { - return errors.New("custom action error: Empty API endpoints") - } - - return c.processAPIEndpoints(ctx, response) -} - -func (c *Command) processAPIEndpoints(ctx context.Context, response *accessverifier.Response) error { - client, err := gitlabnet.GetClient(c.Config) - - if err != nil { - return err - } - - data := response.Payload.Data - request := &Request{Data: data} - request.Data.UserID = response.Who - - for _, endpoint := range data.APIEndpoints { - ctxlog := log.WithContextFields(ctx, log.Fields{ - "primary_repo": data.PrimaryRepo, - "endpoint": endpoint, - }) - - ctxlog.Info("customaction: processApiEndpoints: Performing custom action") - - response, err := c.performRequest(ctx, client, endpoint, request) - if err != nil { - return err - } - - // Print to os.Stdout the result contained in the response - // - if err = c.displayResult(response.Result); err != nil { - return err - } - - // In the context of the git push sequence of events, it's necessary to read - // stdin in order to capture output to pass onto subsequent commands - // - var output []byte - - if c.EOFSent { - output, err = c.readFromStdin() - if err != nil { - return err - } - } else { - output = c.readFromStdinNoEOF() - } - ctxlog.WithFields(log.Fields{ - "eof_sent": c.EOFSent, - "stdin_bytes": len(output), - }).Debug("customaction: processApiEndpoints: stdin buffered") - - request.Output = output - } - - return nil -} - -func (c *Command) performRequest(ctx context.Context, client *client.GitlabNetClient, endpoint string, request *Request) (*Response, error) { - response, err := client.DoRequest(ctx, http.MethodPost, endpoint, request) - if err != nil { - return nil, err - } - defer func() { _ = response.Body.Close() }() - - cr := &Response{} - if err := gitlabnet.ParseJSON(response, cr); err != nil { - return nil, err - } - - return cr, nil -} - -func (c *Command) readFromStdin() ([]byte, error) { - var output []byte - var needsPackData bool - - scanner := pktline.NewScanner(c.ReadWriter.In) - for scanner.Scan() { - line := scanner.Bytes() - output = append(output, line...) - - if pktline.IsFlush(line) { - break - } - - if !needsPackData && !pktline.IsRefRemoval(line) { - needsPackData = true - } - } - - if needsPackData { - packData := new(bytes.Buffer) - _, err := io.Copy(packData, c.ReadWriter.In) - - output = append(output, packData.Bytes()...) - return output, err - } - return output, nil -} - -func (c *Command) readFromStdinNoEOF() []byte { - var output []byte - - scanner := pktline.NewScanner(c.ReadWriter.In) - for scanner.Scan() { - line := scanner.Bytes() - output = append(output, line...) - - if pktline.IsDone(line) { - break - } - } - - return output -} - -func (c *Command) displayResult(result []byte) error { - _, err := io.Copy(c.ReadWriter.Out, bytes.NewReader(result)) - return err -} diff --git a/internal/command/shared/customaction/customaction_test.go b/internal/command/shared/customaction/customaction_test.go deleted file mode 100644 index b2a0c21831574cba53900e04ced3ed3d29ce3e08..0000000000000000000000000000000000000000 --- a/internal/command/shared/customaction/customaction_test.go +++ /dev/null @@ -1,156 +0,0 @@ -package customaction - -import ( - "bytes" - "context" - "encoding/json" - "io" - "net/http" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/readwriter" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/config" - "gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet/accessverifier" -) - -func TestExecuteEOFSent(t *testing.T) { - who := "key-1" - - requests := []testserver.TestRequestHandler{ - { - Path: "/geo/proxy/info_refs_receive_pack", - Handler: func(w http.ResponseWriter, r *http.Request) { - b, err := io.ReadAll(r.Body) - assert.NoError(t, err) - - var request *Request - assert.NoError(t, json.Unmarshal(b, &request)) - - assert.Equal(t, request.Data.UserID, who) - assert.Empty(t, request.Output) - - err = json.NewEncoder(w).Encode(Response{Result: []byte("custom")}) - assert.NoError(t, err) - }, - }, - { - Path: "/geo/proxy/receive_pack", - Handler: func(w http.ResponseWriter, r *http.Request) { - b, err := io.ReadAll(r.Body) - assert.NoError(t, err) - - var request *Request - assert.NoError(t, json.Unmarshal(b, &request)) - - assert.Equal(t, request.Data.UserID, who) - assert.Equal(t, "0009input", string(request.Output)) - - err = json.NewEncoder(w).Encode(Response{Result: []byte("output")}) - assert.NoError(t, err) - }, - }, - } - - url := testserver.StartSocketHTTPServer(t, requests) - - outBuf := &bytes.Buffer{} - errBuf := &bytes.Buffer{} - input := bytes.NewBufferString("0009input") - - response := &accessverifier.Response{ - Who: who, - Payload: accessverifier.CustomPayload{ - Action: "geo_proxy_to_primary", - Data: accessverifier.CustomPayloadData{ - APIEndpoints: []string{"/geo/proxy/info_refs_receive_pack", "/geo/proxy/receive_pack"}, - Username: "custom", - PrimaryRepo: "https://repo/path", - }, - }, - } - - cmd := &Command{ - Config: &config.Config{GitlabUrl: url}, - ReadWriter: &readwriter.ReadWriter{ErrOut: errBuf, Out: outBuf, In: input}, - EOFSent: true, - } - - require.NoError(t, cmd.Execute(context.Background(), response)) - - // expect printing of info message, "custom" string from the first request - // and "output" string from the second request - assert.Equal(t, "customoutput", outBuf.String()) -} - -func TestExecuteNoEOFSent(t *testing.T) { - who := "key-1" - - requests := []testserver.TestRequestHandler{ - { - Path: "/geo/proxy/info_refs_upload_pack", - Handler: func(w http.ResponseWriter, r *http.Request) { - b, err := io.ReadAll(r.Body) - assert.NoError(t, err) - - var request *Request - assert.NoError(t, json.Unmarshal(b, &request)) - - assert.Equal(t, request.Data.UserID, who) - assert.Empty(t, request.Output) - - err = json.NewEncoder(w).Encode(Response{Result: []byte("custom")}) - assert.NoError(t, err) - }, - }, - { - Path: "/geo/proxy/upload_pack", - Handler: func(w http.ResponseWriter, r *http.Request) { - b, err := io.ReadAll(r.Body) - assert.NoError(t, err) - - var request *Request - assert.NoError(t, json.Unmarshal(b, &request)) - - assert.Equal(t, request.Data.UserID, who) - assert.Equal(t, "0032want 343d70886785dc1f98aaf70f3b4ca87c93a5d0dd\n", string(request.Output)) - - err = json.NewEncoder(w).Encode(Response{Result: []byte("output")}) - assert.NoError(t, err) - }, - }, - } - - url := testserver.StartSocketHTTPServer(t, requests) - - outBuf := &bytes.Buffer{} - errBuf := &bytes.Buffer{} - input := bytes.NewBufferString("0032want 343d70886785dc1f98aaf70f3b4ca87c93a5d0dd\n") - - response := &accessverifier.Response{ - Who: who, - Payload: accessverifier.CustomPayload{ - Action: "geo_proxy_to_primary", - Data: accessverifier.CustomPayloadData{ - APIEndpoints: []string{"/geo/proxy/info_refs_upload_pack", "/geo/proxy/upload_pack"}, - Username: "custom", - PrimaryRepo: "https://repo/path", - }, - }, - } - - cmd := &Command{ - Config: &config.Config{GitlabUrl: url}, - ReadWriter: &readwriter.ReadWriter{ErrOut: errBuf, Out: outBuf, In: input}, - EOFSent: false, - } - - require.NoError(t, cmd.Execute(context.Background(), response)) - - // expect printing of info message, "custom" string from the first request - // and "output" string from the second request - assert.Equal(t, "customoutput", outBuf.String()) -} diff --git a/internal/gitlabnet/accessverifier/client.go b/internal/gitlabnet/accessverifier/client.go index 6bdf8f4bb89d42aa8076a31cdb928ba1b599459c..3fde8635e7c2f4b467a63b644c594a50c38e79b0 100644 --- a/internal/gitlabnet/accessverifier/client.go +++ b/internal/gitlabnet/accessverifier/client.go @@ -53,7 +53,6 @@ type CustomPayloadData struct { PrimaryRepo string `json:"primary_repo"` UserID string `json:"gl_id,omitempty"` RequestHeaders map[string]string `json:"request_headers"` - GeoProxyDirectToPrimary bool `json:"geo_proxy_direct_to_primary"` GeoProxyFetchSSHDirectToPrimary bool `json:"geo_proxy_fetch_ssh_direct_to_primary"` GeoProxyPushSSHDirectToPrimary bool `json:"geo_proxy_push_ssh_direct_to_primary"` } diff --git a/internal/gitlabnet/accessverifier/client_test.go b/internal/gitlabnet/accessverifier/client_test.go index dd571c42de96ef9aa5a74bd6aa49952f17a7de95..886c665683e1e69bd232db44daa522dc04fb65c5 100644 --- a/internal/gitlabnet/accessverifier/client_test.go +++ b/internal/gitlabnet/accessverifier/client_test.go @@ -112,11 +112,10 @@ func TestGeoPushGetCustomAction(t *testing.T) { response.Payload = CustomPayload{ Action: "geo_proxy_to_primary", Data: CustomPayloadData{ - APIEndpoints: []string{"geo/proxy_git_ssh/info_refs_receive_pack", "geo/proxy_git_ssh/receive_pack"}, - GeoProxyDirectToPrimary: true, - RequestHeaders: map[string]string{"Authorization": "Bearer token"}, - Username: "custom", - PrimaryRepo: "https://repo/path", + APIEndpoints: []string{"geo/proxy_git_ssh/info_refs_receive_pack", "geo/proxy_git_ssh/receive_pack"}, + RequestHeaders: map[string]string{"Authorization": "Bearer token"}, + Username: "custom", + PrimaryRepo: "https://repo/path", }, } response.StatusCode = 300 diff --git a/internal/testhelper/requesthandlers/requesthandlers.go b/internal/testhelper/requesthandlers/requesthandlers.go index 92ab1ba1b1c6273bca0345fc8ad3cb5894f0186a..6d368a371920e9a4aa035188a5ce190b0333a44a 100644 --- a/internal/testhelper/requesthandlers/requesthandlers.go +++ b/internal/testhelper/requesthandlers/requesthandlers.go @@ -4,6 +4,7 @@ package requesthandlers import ( "encoding/json" "net/http" + "net/http/httptest" "testing" "github.com/stretchr/testify/assert" @@ -70,6 +71,22 @@ func BuildAllowedWithGitalyHandlers(t *testing.T, gitalyAddress string) []testse // BuildAllowedWithCustomActionsHandlers returns test request handlers for allowed API calls with custom actions. func BuildAllowedWithCustomActionsHandlers(t *testing.T) []testserver.TestRequestHandler { + // Create a separate HTTP server for Git protocol responses + gitServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/info/refs" && r.URL.Query().Get("service") == "git-receive-pack" { + // Proper Git info/refs response format + w.Header().Set("Content-Type", "application/x-git-receive-pack-advertisement") + _, err := w.Write([]byte("001f# service=git-receive-pack\n0000\n0045abcdef1234567890abcdef1234567890abcdef1234 refs/heads/master\n0000")) + assert.NoError(t, err) + } else if r.URL.Path == "/git-receive-pack" { + // Proper Git receive-pack response format + w.Header().Set("Content-Type", "application/x-git-receive-pack-result") + _, err := w.Write([]byte("0008NAK\n0019ok refs/heads/master\n0000")) + assert.NoError(t, err) + } + })) + t.Cleanup(func() { gitServer.Close() }) + requests := []testserver.TestRequestHandler{ { Path: "/api/v4/internal/allowed", @@ -80,9 +97,8 @@ func BuildAllowedWithCustomActionsHandlers(t *testing.T) []testserver.TestReques "payload": map[string]interface{}{ "action": "geo_proxy_to_primary", "data": map[string]interface{}{ - "api_endpoints": []string{"/geo/proxy/info_refs", "/geo/proxy/push"}, - "gl_username": "custom", - "primary_repo": "https://repo/path", + "primary_repo": gitServer.URL, + "gl_username": "custom", }, }, } @@ -90,20 +106,6 @@ func BuildAllowedWithCustomActionsHandlers(t *testing.T) []testserver.TestReques assert.NoError(t, json.NewEncoder(w).Encode(body)) }, }, - { - Path: "/geo/proxy/info_refs", - Handler: func(w http.ResponseWriter, _ *http.Request) { - body := map[string]interface{}{"result": []byte("custom")} - assert.NoError(t, json.NewEncoder(w).Encode(body)) - }, - }, - { - Path: "/geo/proxy/push", - Handler: func(w http.ResponseWriter, _ *http.Request) { - body := map[string]interface{}{"result": []byte("output")} - assert.NoError(t, json.NewEncoder(w).Encode(body)) - }, - }, } return requests diff --git a/internal/testhelper/testdata/testroot/responses/allowed_with_geo_push_payload.json b/internal/testhelper/testdata/testroot/responses/allowed_with_geo_push_payload.json index 06d7c3e1983f712c602d5b30834cda0686cf5ccc..d76ff9635d0da749c797b2b55be9a8b60248a426 100644 --- a/internal/testhelper/testdata/testroot/responses/allowed_with_geo_push_payload.json +++ b/internal/testhelper/testdata/testroot/responses/allowed_with_geo_push_payload.json @@ -19,7 +19,6 @@ }, "payload" : { "data": { - "geo_proxy_direct_to_primary": true, "request_headers": { "Authorization": "Bearer token" }, "primary_repo": "PRIMARY_REPO" } diff --git a/internal/testhelper/testdata/testroot/responses/allowed_with_push_payload.json b/internal/testhelper/testdata/testroot/responses/allowed_with_push_payload.json index adedda454dfc790969cf72ec074713a754348977..b4a4a91f0bbd44c123f930a7f677d05eb5d24b49 100644 --- a/internal/testhelper/testdata/testroot/responses/allowed_with_push_payload.json +++ b/internal/testhelper/testdata/testroot/responses/allowed_with_push_payload.json @@ -21,7 +21,6 @@ "action": "geo_proxy_to_primary", "data": { "api_endpoints": ["geo/proxy_git_ssh/info_refs_receive_pack", "geo/proxy_git_ssh/receive_pack"], - "geo_proxy_direct_to_primary": true, "request_headers": { "Authorization": "Bearer token" }, "gl_username": "custom", "primary_repo": "https://repo/path" diff --git a/spec/gitlab_shell_custom_git_receive_pack_spec.rb b/spec/gitlab_shell_custom_git_receive_pack_spec.rb deleted file mode 100644 index 1dce4bbab8193e33416cfcb248ca0c36e6873a67..0000000000000000000000000000000000000000 --- a/spec/gitlab_shell_custom_git_receive_pack_spec.rb +++ /dev/null @@ -1,118 +0,0 @@ -require_relative 'spec_helper' - -require 'open3' -require 'json' -require 'base64' - -describe 'Custom bin/gitlab-shell git-receive-pack' do - include_context 'gitlab shell' - - let(:env) { {'SSH_CONNECTION' => 'fake', 'SSH_ORIGINAL_COMMAND' => 'git-receive-pack group/repo' } } - let(:divider) { "remote: ========================================================================\n" } - - before(:context) do - write_config("gitlab_url" => "http+unix://#{CGI.escape(tmp_socket_path)}") - end - - def mock_server(server) - server.mount_proc('/geo/proxy_git_ssh/info_refs_receive_pack') do |req, res| - res.content_type = 'application/json' - res.status = 200 - - res.body = {"result" => "#{Base64.encode64('custom')}"}.to_json - end - - server.mount_proc('/geo/proxy_git_ssh/receive_pack') do |req, res| - res.content_type = 'application/json' - res.status = 200 - - output = JSON.parse(req.body)['output'] - - res.body = {"result" => output}.to_json - end - - server.mount_proc('/api/v4/internal/allowed') do |req, res| - res.content_type = 'application/json' - - key_id = req.query['key_id'] || req.query['username'] - - unless key_id - body = JSON.parse(req.body) - key_id = body['key_id'] || body['username'].to_s - end - - case key_id - when '100', 'someone' then - res.status = 300 - body = { - "gl_id" => "user-100", - "status" => true, - "payload" => { - "action" => "geo_proxy_to_primary", - "data" => { - "api_endpoints" => ["/geo/proxy_git_ssh/info_refs_receive_pack", "/geo/proxy_git_ssh/receive_pack"], - "gl_username" => "custom", - "primary_repo" => "https://repo/path" - }, - }, - "gl_console_messages" => ["console", "message"] - } - res.body = body.to_json - else - res.status = 403 - end - end - end - - describe 'dialog for performing a custom action' do - context 'when API calls perform successfully' do - let(:remote_blank_line) { "remote: \n" } - def verify_successful_call!(cmd) - Open3.popen3(env, cmd) do |stdin, stdout, stderr| - expect(stderr.gets).to eq(remote_blank_line) - expect(stderr.gets).to eq("remote: console\n") - expect(stderr.gets).to eq("remote: message\n") - expect(stderr.gets).to eq(remote_blank_line) - - stdin.puts("0009input") - stdin.close - - expect(stdout.gets(6)).to eq("custom") - expect(stdout.flush.read).to eq("0009input") - end - end - - context 'when key is provided' do - let(:cmd) { "#{gitlab_shell_path} key-100" } - - it 'custom action is performed' do - verify_successful_call!(cmd) - end - end - - context 'when username is provided' do - let(:cmd) { "#{gitlab_shell_path} username-someone" } - - it 'custom action is performed' do - verify_successful_call!(cmd) - end - end - end - - context 'when API error occurs' do - let(:cmd) { "#{gitlab_shell_path} key-101" } - - it 'custom action is not performed' do - Open3.popen2e(env, cmd) do |stdin, stdout| - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq(divider) - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq("remote: Internal API error (403)\n") - expect(stdout.gets).to eq("remote: \n") - expect(stdout.gets).to eq(divider) - expect(stdout.gets).to eq("remote: \n") - end - end - end - end -end