From 611a6b6eb9a1036748612d343b188b43c68212ac Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Wed, 28 Feb 2024 14:15:48 +0100 Subject: [PATCH] Add ssh-upload-pack endpoint to handle Git over SSH requests Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/456116 **Problem** For Cells and Geo needs we want to route Git over SSH requests to the correct instance. **Solution** Introduce an HTTP Workhorse endpoint that accepts requests coming from GitLab-Shell and responds with git upload pack data. Changelog: added EE: true --- .../repositories/git_http_controller.rb | 7 ++ config/routes/git_http.rb | 3 + .../ee/repositories/git_http_controller.rb | 14 +++ .../beta/geo_proxy_fetch_ssh_to_primary.yml | 9 ++ ee/lib/ee/gitlab/geo_git_access.rb | 3 + ee/spec/lib/gitlab/git_access_spec.rb | 3 + .../repositories/git_http_controller_spec.rb | 71 ++++++++++++++ .../lib/gitlab/git_access_shared_examples.rb | 2 + .../repositories/git_http_controller_spec.rb | 11 +++ .../lint_last_known_acceptable_go1.21.txt | 24 ++--- .../lint_last_known_acceptable_go1.22.txt | 24 ++--- workhorse/go.mod | 2 +- workhorse/go.sum | 4 +- workhorse/internal/git/ssh.go | 66 +++++++++++++ workhorse/internal/git/ssh_test.go | 94 +++++++++++++++++++ workhorse/internal/gitaly/gitaly.go | 7 ++ workhorse/internal/gitaly/gitaly_test.go | 7 ++ workhorse/internal/upstream/routes.go | 1 + 18 files changed, 315 insertions(+), 37 deletions(-) create mode 100644 ee/config/feature_flags/beta/geo_proxy_fetch_ssh_to_primary.yml create mode 100644 workhorse/internal/git/ssh.go create mode 100644 workhorse/internal/git/ssh_test.go diff --git a/app/controllers/repositories/git_http_controller.rb b/app/controllers/repositories/git_http_controller.rb index ee1951f6e14905..356cd5a9dc87b4 100644 --- a/app/controllers/repositories/git_http_controller.rb +++ b/app/controllers/repositories/git_http_controller.rb @@ -37,6 +37,11 @@ def git_receive_pack render_ok end + # POST /foo/bar.git/ssh-upload-pack" (git pull via SSH) + def ssh_upload_pack + render plain: "Not found", status: :not_found + end + private def deny_head_requests @@ -58,6 +63,8 @@ def receive_pack? def git_command if action_name == 'info_refs' params[:service] + elsif action_name == 'ssh_upload_pack' + 'git-upload-pack' else action_name.dasherize end diff --git a/config/routes/git_http.rb b/config/routes/git_http.rb index 6899a89cc7dc0e..20a08efa1c80fa 100644 --- a/config/routes/git_http.rb +++ b/config/routes/git_http.rb @@ -8,6 +8,9 @@ get '/info/refs', action: :info_refs post '/git-upload-pack', action: :git_upload_pack post '/git-receive-pack', action: :git_receive_pack + + # GitLab-Shell Git over SSH requests + post '/ssh-upload-pack', action: :ssh_upload_pack end # NOTE: LFS routes are exposed on all repository types, but we still check for diff --git a/ee/app/controllers/ee/repositories/git_http_controller.rb b/ee/app/controllers/ee/repositories/git_http_controller.rb index 48acf3720a76b9..fdbc69c7558e8a 100644 --- a/ee/app/controllers/ee/repositories/git_http_controller.rb +++ b/ee/app/controllers/ee/repositories/git_http_controller.rb @@ -30,6 +30,20 @@ def git_upload_pack super end + # This is reached on a primary for a request orignating on a secondary + # only when the repository on the secondary is out of date with that on + # the primary + override :ssh_upload_pack + def ssh_upload_pack + if geo? + set_workhorse_internal_api_content_type + + render json: ::Gitlab::Workhorse.git_http_ok(repository, repo_type, user, :git_upload_pack, show_all_refs: geo_request?, need_audit: need_git_audit_event?) + else + super + end + end + # Git push over HTTP override :git_receive_pack def git_receive_pack diff --git a/ee/config/feature_flags/beta/geo_proxy_fetch_ssh_to_primary.yml b/ee/config/feature_flags/beta/geo_proxy_fetch_ssh_to_primary.yml new file mode 100644 index 00000000000000..e954c884410df8 --- /dev/null +++ b/ee/config/feature_flags/beta/geo_proxy_fetch_ssh_to_primary.yml @@ -0,0 +1,9 @@ +--- +name: geo_proxy_fetch_ssh_to_primary +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/456116 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/152950 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/466045 +milestone: '17.1' +group: group::source code +type: beta +default_enabled: false diff --git a/ee/lib/ee/gitlab/geo_git_access.rb b/ee/lib/ee/gitlab/geo_git_access.rb index 27d26e8d93ecde..7055ba0f3ad77f 100644 --- a/ee/lib/ee/gitlab/geo_git_access.rb +++ b/ee/lib/ee/gitlab/geo_git_access.rb @@ -23,6 +23,9 @@ def geo_custom_action 'geo_proxy_fetch_direct_to_primary_with_options' => ::Feature.enabled?( :geo_proxy_fetch_direct_to_primary_with_options ), + 'geo_proxy_fetch_ssh_direct_to_primary' => ::Feature.enabled?( + :geo_proxy_fetch_ssh_to_primary + ), 'request_headers' => proxy_direct_to_primary_headers } } diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb index cd186fed91b0fb..32fd0ce5960bfb 100644 --- a/ee/spec/lib/gitlab/git_access_spec.rb +++ b/ee/spec/lib/gitlab/git_access_spec.rb @@ -718,6 +718,7 @@ def download_ability "geo_proxy_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary_with_options" => true, + "geo_proxy_fetch_ssh_direct_to_primary" => true, "request_headers" => include('Authorization') } } @@ -814,6 +815,7 @@ def download_ability "geo_proxy_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary_with_options" => true, + "geo_proxy_fetch_ssh_direct_to_primary" => true, "request_headers" => include('Authorization') } } @@ -839,6 +841,7 @@ def download_ability "geo_proxy_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary_with_options" => true, + "geo_proxy_fetch_ssh_direct_to_primary" => true, "request_headers" => include('Authorization') } } diff --git a/ee/spec/requests/repositories/git_http_controller_spec.rb b/ee/spec/requests/repositories/git_http_controller_spec.rb index f41a97bd1f5166..958f6f02a7afdb 100644 --- a/ee/spec/requests/repositories/git_http_controller_spec.rb +++ b/ee/spec/requests/repositories/git_http_controller_spec.rb @@ -7,6 +7,7 @@ include ::EE::GeoHelpers let_it_be(:user) { create(:user) } + let_it_be(:user_personal_key) { create(:personal_key, user: user) } let_it_be(:project) { create(:project, :repository, :private, developers: user) } let(:env) { { user: user.username, password: user.password } } @@ -66,6 +67,76 @@ end end + describe 'POST #ssh_upload_pack' do + subject(:make_request) { post "/#{path}/ssh-upload-pack", headers: headers } + + let(:geo_node) { create(:geo_node, :primary) } + let(:headers) { workhorse_header.merge(geo_header) } + let(:workhorse_header) { workhorse_internal_api_request_header } + let(:geo_header) { Gitlab::Geo::BaseRequest.new(scope: project.full_path, gl_id: "key-#{user_personal_key.id}").headers } + + before do + stub_current_geo_node(geo_node) + end + + it 'allows access' do + make_request + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to include( + "GL_ID" => "user-#{user.id}", + "GL_REPOSITORY" => "project-#{project.id}", + "GL_USERNAME" => user.username, + "NeedAudit" => false, + "ShowAllRefs" => true + ) + end + + context 'when Workhorse header is missing' do + let(:workhorse_header) { {} } + + it 'returns an error' do + make_request + + expect(response).to have_gitlab_http_status(:forbidden) + expect(response.body).to eq 'Nil JSON web token' + end + end + + context 'when Workhorse header is incorrect' do + let(:workhorse_header) { { Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => 'wrong' } } + + it 'returns an error' do + make_request + + expect(response).to have_gitlab_http_status(:forbidden) + expect(response.body).to eq 'Not enough or too many segments' + end + end + + context 'when Geo auth header is missing' do + let(:geo_header) { {} } + + it 'returns an error' do + make_request + + expect(response).to have_gitlab_http_status(:unauthorized) + expect(response.body).to include 'HTTP Basic: Access denied.' + end + end + + context 'when Geo auth header is invalid' do + let(:geo_header) { { 'Authorization' => 'GL-Geo wrong' } } + + it 'returns an error' do + make_request + + expect(response).to have_gitlab_http_status(:unauthorized) + expect(response.body).to eq 'Geo JWT authentication failed: Bad token' + end + end + end + describe 'GET #info_refs' do context 'smartcard session required' do subject { clone_get(path, **env) } diff --git a/ee/spec/support/shared_examples/lib/gitlab/git_access_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/git_access_shared_examples.rb index 554475247f2327..c5fe7f60eee5cf 100644 --- a/ee/spec/support/shared_examples/lib/gitlab/git_access_shared_examples.rb +++ b/ee/spec/support/shared_examples/lib/gitlab/git_access_shared_examples.rb @@ -59,6 +59,7 @@ "geo_proxy_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary_with_options" => true, + "geo_proxy_fetch_ssh_direct_to_primary" => true, "request_headers" => include('Authorization') } } @@ -83,6 +84,7 @@ "geo_proxy_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary" => true, "geo_proxy_fetch_direct_to_primary_with_options" => true, + "geo_proxy_fetch_ssh_direct_to_primary" => true, "request_headers" => include('Authorization') } } diff --git a/spec/controllers/repositories/git_http_controller_spec.rb b/spec/controllers/repositories/git_http_controller_spec.rb index b3a32c2c506623..6069c1b52f1eff 100644 --- a/spec/controllers/repositories/git_http_controller_spec.rb +++ b/spec/controllers/repositories/git_http_controller_spec.rb @@ -88,6 +88,17 @@ it_behaves_like 'handles logging git upload pack operation' it_behaves_like 'handles logging git receive pack operation' + describe 'POST #ssh_upload_pack' do + it 'returns not found error' do + allow(controller).to receive(:verify_workhorse_api!).and_return(true) + + post :ssh_upload_pack, params: params + + expect(response).to have_gitlab_http_status(:not_found) + expect(response.body).to eq 'Not found' + end + end + describe 'POST #git_upload_pack' do before do allow(controller).to receive(:verify_workhorse_api!).and_return(true) diff --git a/workhorse/_support/lint_last_known_acceptable_go1.21.txt b/workhorse/_support/lint_last_known_acceptable_go1.21.txt index 9961b4e285e50c..143b8b48656b97 100644 --- a/workhorse/_support/lint_last_known_acceptable_go1.21.txt +++ b/workhorse/_support/lint_last_known_acceptable_go1.21.txt @@ -199,45 +199,35 @@ internal/dependencyproxy/dependencyproxy_test.go:410:27: response body must be c internal/dependencyproxy/dependencyproxy_test.go:412:3: go-require: do not use require in http handlers (testifylint) internal/dependencyproxy/dependencyproxy_test.go:413:3: go-require: do not use require in http handlers (testifylint) internal/dependencyproxy/dependencyproxy_test.go:421: internal/dependencyproxy/dependencyproxy_test.go:421: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "note that the timeout duration here is s..." (godox) -internal/git/archive.go:5:1: package-comments: should have a package comment (revive) internal/git/archive.go:35:2: var-naming: struct field CommitId should be CommitID (revive) internal/git/archive.go:43:2: exported: exported var SendArchive should have comment or be unexported (revive) internal/git/archive.go:53: Function 'Inject' has too many statements (47 > 40) (funlen) internal/git/archive.go:73:29: Error return value of `cachedArchive.Close` is not checked (errcheck) internal/git/archive.go:99:23: Error return value of `tempFile.Close` is not checked (errcheck) internal/git/archive.go:100:18: Error return value of `os.Remove` is not checked (errcheck) -internal/git/blob.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/blob.go:21:5: exported: exported var SendBlob should have comment or be unexported (revive) internal/git/diff.go:1: 1-47 lines are duplicate of `internal/git/format-patch.go:1-48` (dupl) internal/git/diff.go:22:5: exported: exported var SendDiff should have comment or be unexported (revive) -internal/git/error.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/error.go:36:4: singleCaseSwitch: should rewrite switch statement to if statement (gocritic) internal/git/error_test.go:28: File is not `gofmt`-ed with `-s` (gofmt) internal/git/error_test.go:66:2: unnecessary trailing newline (whitespace) internal/git/format-patch.go:1: 1-48 lines are duplicate of `internal/git/diff.go:1-47` (dupl) internal/git/format-patch.go:22:5: exported: exported var SendPatch should have comment or be unexported (revive) -internal/git/git-http.go:5:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/git-http.go:21:2: exported: comment on exported const GitConfigShowAllRefs should be of the form "GitConfigShowAllRefs ..." (revive) internal/git/git-http.go:26:1: exported: exported function ReceivePack should have comment or be unexported (revive) internal/git/git-http.go:30:1: exported: exported function UploadPack should have comment or be unexported (revive) -internal/git/info-refs.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/info-refs.go:20:1: exported: exported function GetInfoRefsHandler should have comment or be unexported (revive) internal/git/info-refs_test.go:27:32: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) -internal/git/io.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/io.go:141:22: Error return value of `tempfile.Close` is not checked (errcheck) internal/git/io.go:173:17: Error return value of `tempfile.Close` is not checked (errcheck) internal/git/io_test.go:20:27: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive) internal/git/io_test.go:37:38: unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive) internal/git/io_test.go:89:14: string `test data` has 3 occurrences, make it a constant (goconst) -internal/git/receive-pack.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/receive-pack.go:21:16: Error return value of `cw.Flush` is not checked (errcheck) -internal/git/responsewriter.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/responsewriter.go:41:6: exported: exported type HTTPResponseWriter should have comment or be unexported (revive) internal/git/responsewriter.go:45:1: exported: exported function NewHTTPResponseWriter should have comment or be unexported (revive) internal/git/responsewriter.go:52:1: exported: exported method HTTPResponseWriter.Log should have comment or be unexported (revive) -internal/git/snapshot.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/snapshot.go:27:2: exported: exported var SendSnapshot should have comment or be unexported (revive) -internal/git/upload-pack.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/upload-pack.go:37:16: Error return value of `cw.Flush` is not checked (errcheck) internal/git/upload-pack_test.go:32:27: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive) internal/git/upload-pack_test.go:51:38: unused-parameter: parameter 'req' seems to be unused, consider removing or renaming it as _ (revive) @@ -256,11 +246,11 @@ internal/gitaly/gitaly.go:84:1: exported: exported function NewSmartHTTPClient s internal/gitaly/gitaly.go:97:1: exported: exported function NewBlobClient should have comment or be unexported (revive) internal/gitaly/gitaly.go:106:1: exported: exported function NewRepositoryClient should have comment or be unexported (revive) internal/gitaly/gitaly.go:115:1: exported: exported function NewDiffClient should have comment or be unexported (revive) -internal/gitaly/gitaly.go:138:5: shadow: declaration of "conn" shadows declaration at line 128 (govet) -internal/gitaly/gitaly.go:152:1: exported: exported function CloseConnections should have comment or be unexported (revive) -internal/gitaly/gitaly.go:157:13: Error return value of `conn.Close` is not checked (errcheck) -internal/gitaly/gitaly.go:162:14: appendAssign: append result not assigned to the same slice (gocritic) -internal/gitaly/gitaly.go:202:1: exported: exported function UnmarshalJSON should have comment or be unexported (revive) +internal/gitaly/gitaly.go:145:5: shadow: declaration of "conn" shadows declaration at line 135 (govet) +internal/gitaly/gitaly.go:159:1: exported: exported function CloseConnections should have comment or be unexported (revive) +internal/gitaly/gitaly.go:164:13: Error return value of `conn.Close` is not checked (errcheck) +internal/gitaly/gitaly.go:169:14: appendAssign: append result not assigned to the same slice (gocritic) +internal/gitaly/gitaly.go:209:1: exported: exported function UnmarshalJSON should have comment or be unexported (revive) internal/gitaly/repository.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/gitaly/smarthttp.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/gitaly/smarthttp.go:13:6: exported: exported type SmartHTTPClient should have comment or be unexported (revive) @@ -476,8 +466,8 @@ internal/upstream/roundtripper/roundtripper_test.go:49:72: unused-parameter: par internal/upstream/roundtripper/roundtripper_test.go:59:75: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) internal/upstream/routes.go:106:28: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) internal/upstream/routes.go:150:68: `(*upstream).wsRoute` - `matchers` always receives `nil` (unparam) -internal/upstream/routes.go:210: Function 'configureRoutes' is too long (234 > 60) (funlen) -internal/upstream/routes.go:382: internal/upstream/routes.go:382: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO: We should probably not return a HT..." (godox) +internal/upstream/routes.go:210: Function 'configureRoutes' is too long (235 > 60) (funlen) +internal/upstream/routes.go:383: internal/upstream/routes.go:383: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO: We should probably not return a HT..." (godox) internal/upstream/upstream.go:116: internal/upstream/upstream.go:116: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO: move to LabKit https://gitlab.com/..." (godox) internal/upstream/upstream_test.go:266:3: go-require: do not use require in http handlers (testifylint) internal/upstream/upstream_test.go:267:3: go-require: do not use require in http handlers (testifylint) diff --git a/workhorse/_support/lint_last_known_acceptable_go1.22.txt b/workhorse/_support/lint_last_known_acceptable_go1.22.txt index e92aabd02d5319..e6b7fd0245be47 100644 --- a/workhorse/_support/lint_last_known_acceptable_go1.22.txt +++ b/workhorse/_support/lint_last_known_acceptable_go1.22.txt @@ -199,45 +199,35 @@ internal/dependencyproxy/dependencyproxy_test.go:410:27: response body must be c internal/dependencyproxy/dependencyproxy_test.go:412:3: go-require: do not use require in http handlers (testifylint) internal/dependencyproxy/dependencyproxy_test.go:413:3: go-require: do not use require in http handlers (testifylint) internal/dependencyproxy/dependencyproxy_test.go:421: internal/dependencyproxy/dependencyproxy_test.go:421: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "note that the timeout duration here is s..." (godox) -internal/git/archive.go:5:1: package-comments: should have a package comment (revive) internal/git/archive.go:35:2: var-naming: struct field CommitId should be CommitID (revive) internal/git/archive.go:43:2: exported: exported var SendArchive should have comment or be unexported (revive) internal/git/archive.go:53: Function 'Inject' has too many statements (47 > 40) (funlen) internal/git/archive.go:73:29: Error return value of `cachedArchive.Close` is not checked (errcheck) internal/git/archive.go:99:23: Error return value of `tempFile.Close` is not checked (errcheck) internal/git/archive.go:100:18: Error return value of `os.Remove` is not checked (errcheck) -internal/git/blob.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/blob.go:21:5: exported: exported var SendBlob should have comment or be unexported (revive) internal/git/diff.go:1: 1-47 lines are duplicate of `internal/git/format-patch.go:1-48` (dupl) internal/git/diff.go:22:5: exported: exported var SendDiff should have comment or be unexported (revive) -internal/git/error.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/error.go:36:4: singleCaseSwitch: should rewrite switch statement to if statement (gocritic) internal/git/error_test.go:28: File is not `gofmt`-ed with `-s` (gofmt) internal/git/error_test.go:66:2: unnecessary trailing newline (whitespace) internal/git/format-patch.go:1: 1-48 lines are duplicate of `internal/git/diff.go:1-47` (dupl) internal/git/format-patch.go:22:5: exported: exported var SendPatch should have comment or be unexported (revive) -internal/git/git-http.go:5:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/git-http.go:21:2: exported: comment on exported const GitConfigShowAllRefs should be of the form "GitConfigShowAllRefs ..." (revive) internal/git/git-http.go:26:1: exported: exported function ReceivePack should have comment or be unexported (revive) internal/git/git-http.go:30:1: exported: exported function UploadPack should have comment or be unexported (revive) -internal/git/info-refs.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/info-refs.go:20:1: exported: exported function GetInfoRefsHandler should have comment or be unexported (revive) internal/git/info-refs_test.go:27:32: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) -internal/git/io.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/io.go:141:22: Error return value of `tempfile.Close` is not checked (errcheck) internal/git/io.go:173:17: Error return value of `tempfile.Close` is not checked (errcheck) internal/git/io_test.go:20:27: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive) internal/git/io_test.go:37:38: unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive) internal/git/io_test.go:89:14: string `test data` has 3 occurrences, make it a constant (goconst) -internal/git/receive-pack.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/receive-pack.go:21:16: Error return value of `cw.Flush` is not checked (errcheck) -internal/git/responsewriter.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/responsewriter.go:41:6: exported: exported type HTTPResponseWriter should have comment or be unexported (revive) internal/git/responsewriter.go:45:1: exported: exported function NewHTTPResponseWriter should have comment or be unexported (revive) internal/git/responsewriter.go:52:1: exported: exported method HTTPResponseWriter.Log should have comment or be unexported (revive) -internal/git/snapshot.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/snapshot.go:27:2: exported: exported var SendSnapshot should have comment or be unexported (revive) -internal/git/upload-pack.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/git/upload-pack.go:37:16: Error return value of `cw.Flush` is not checked (errcheck) internal/git/upload-pack_test.go:32:27: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive) internal/git/upload-pack_test.go:51:38: unused-parameter: parameter 'req' seems to be unused, consider removing or renaming it as _ (revive) @@ -256,11 +246,11 @@ internal/gitaly/gitaly.go:84:1: exported: exported function NewSmartHTTPClient s internal/gitaly/gitaly.go:97:1: exported: exported function NewBlobClient should have comment or be unexported (revive) internal/gitaly/gitaly.go:106:1: exported: exported function NewRepositoryClient should have comment or be unexported (revive) internal/gitaly/gitaly.go:115:1: exported: exported function NewDiffClient should have comment or be unexported (revive) -internal/gitaly/gitaly.go:138:5: shadow: declaration of "conn" shadows declaration at line 128 (govet) -internal/gitaly/gitaly.go:152:1: exported: exported function CloseConnections should have comment or be unexported (revive) -internal/gitaly/gitaly.go:157:13: Error return value of `conn.Close` is not checked (errcheck) -internal/gitaly/gitaly.go:162:14: appendAssign: append result not assigned to the same slice (gocritic) -internal/gitaly/gitaly.go:202:1: exported: exported function UnmarshalJSON should have comment or be unexported (revive) +internal/gitaly/gitaly.go:145:5: shadow: declaration of "conn" shadows declaration at line 135 (govet) +internal/gitaly/gitaly.go:159:1: exported: exported function CloseConnections should have comment or be unexported (revive) +internal/gitaly/gitaly.go:164:13: Error return value of `conn.Close` is not checked (errcheck) +internal/gitaly/gitaly.go:169:14: appendAssign: append result not assigned to the same slice (gocritic) +internal/gitaly/gitaly.go:209:1: exported: exported function UnmarshalJSON should have comment or be unexported (revive) internal/gitaly/repository.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/gitaly/smarthttp.go:1:1: ST1000: at least one file in a package should have a package comment (stylecheck) internal/gitaly/smarthttp.go:13:6: exported: exported type SmartHTTPClient should have comment or be unexported (revive) @@ -476,8 +466,8 @@ internal/upstream/roundtripper/roundtripper_test.go:49:72: unused-parameter: par internal/upstream/roundtripper/roundtripper_test.go:59:75: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) internal/upstream/routes.go:106:28: unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive) internal/upstream/routes.go:150:68: `(*upstream).wsRoute` - `matchers` always receives `nil` (unparam) -internal/upstream/routes.go:210: Function 'configureRoutes' is too long (234 > 60) (funlen) -internal/upstream/routes.go:382: internal/upstream/routes.go:382: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO: We should probably not return a HT..." (godox) +internal/upstream/routes.go:210: Function 'configureRoutes' is too long (235 > 60) (funlen) +internal/upstream/routes.go:383: internal/upstream/routes.go:383: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO: We should probably not return a HT..." (godox) internal/upstream/upstream.go:116: internal/upstream/upstream.go:116: Line contains TODO/BUG/FIXME/NOTE/OPTIMIZE/HACK: "TODO: move to LabKit https://gitlab.com/..." (godox) internal/upstream/upstream_test.go:266:3: go-require: do not use require in http handlers (testifylint) internal/upstream/upstream_test.go:267:3: go-require: do not use require in http handlers (testifylint) diff --git a/workhorse/go.mod b/workhorse/go.mod index 2e09f48e11c390..5de2f228f2feb8 100644 --- a/workhorse/go.mod +++ b/workhorse/go.mod @@ -18,7 +18,7 @@ require ( github.com/johannesboyne/gofakes3 v0.0.0-20240217095638-c55a48f17be6 github.com/jpillora/backoff v1.0.0 github.com/mitchellh/copystructure v1.2.0 - github.com/prometheus/client_golang v1.19.1 + github.com/prometheus/client_golang v1.19.1-0.20240328134234-93cf5d4f5f78 github.com/redis/go-redis/v9 v9.5.2 github.com/sebest/xff v0.0.0-20210106013422-671bd2870b3a github.com/sirupsen/logrus v1.9.3 diff --git a/workhorse/go.sum b/workhorse/go.sum index f6f3368137f016..034851a4909c25 100644 --- a/workhorse/go.sum +++ b/workhorse/go.sum @@ -407,8 +407,8 @@ github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRI github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c h1:ncq/mPwQF4JjgDlrVEn3C11VoGHZN7m8qihwgMEtzYw= github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c/go.mod h1:OmDBASR4679mdNQnz2pUhc2G8CO2JrUAVFDRBDP/hJE= -github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQeLaYJFJBOE= -github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho= +github.com/prometheus/client_golang v1.19.1-0.20240328134234-93cf5d4f5f78 h1:rSOwhjTtzeuOZS3pO9Gzy0vrGMHSR5s7eWiMKBTV8ns= +github.com/prometheus/client_golang v1.19.1-0.20240328134234-93cf5d4f5f78/go.mod h1:kDK4t8GKrX8Q1xkeHV0TTro2F3HIgGRx7X1Kt3GEku8= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= github.com/prometheus/client_model v0.6.0 h1:k1v3CzpSRUTrKMppY35TLwPvxHqBu0bYgxZzqGIgaos= github.com/prometheus/client_model v0.6.0/go.mod h1:NTQHnmxFpouOD0DpvP4XujX3CdOAGQPoaGhyTchlyt8= diff --git a/workhorse/internal/git/ssh.go b/workhorse/internal/git/ssh.go new file mode 100644 index 00000000000000..79fb9720bfa80f --- /dev/null +++ b/workhorse/internal/git/ssh.go @@ -0,0 +1,66 @@ +/* +In this file we handle the Git over SSH GitLab-Shell requests +*/ + +// Package git handles git operations +package git + +import ( + "fmt" + "net/http" + + "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" + + "gitlab.com/gitlab-org/gitaly/v16/client" + + "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/gitaly" + "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper/fail" +) + +type flushWriter struct { + http.ResponseWriter + controller *http.ResponseController +} + +func (f *flushWriter) Write(p []byte) (int, error) { + n, err := f.ResponseWriter.Write(p) + if err != nil { + return n, err + } + + return n, f.controller.Flush() +} + +// SSHUploadPack handles git pull SSH connection between GitLab-Shell and Gitaly through Workhorse +func SSHUploadPack(a *api.API) http.Handler { + return repoPreAuthorizeHandler(a, handleSSHUploadPack) +} + +func handleSSHUploadPack(w http.ResponseWriter, r *http.Request, a *api.Response) { + controller := http.NewResponseController(w) //nolint:bodyclose // false-positive https://github.com/timakin/bodyclose/issues/52 + if err := controller.EnableFullDuplex(); err != nil { + fail.Request(w, r, fmt.Errorf("enabling full duplex: %v", err)) + return + } + + conn, registry, err := gitaly.NewConnectionWithSidechannel(a.GitalyServer) + if err != nil { + fail.Request(w, r, fmt.Errorf("look up for gitaly connection: %v", err)) + return + } + + w.WriteHeader(http.StatusOK) + + request := &gitalypb.SSHUploadPackWithSidechannelRequest{ + Repository: &a.Repository, + GitProtocol: r.Header.Get("Git-Protocol"), + GitConfigOptions: a.GitConfigOptions, + } + out := &flushWriter{ResponseWriter: w, controller: controller} + _, err = client.UploadPackWithSidechannelWithResult(r.Context(), conn, registry, r.Body, out, out, request) + if err != nil { + fail.Request(w, r, fmt.Errorf("upload pack failed: %v", err)) + return + } +} diff --git a/workhorse/internal/git/ssh_test.go b/workhorse/internal/git/ssh_test.go new file mode 100644 index 00000000000000..70678ba2083fc1 --- /dev/null +++ b/workhorse/internal/git/ssh_test.go @@ -0,0 +1,94 @@ +package git + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v16/client" + "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" + + "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" +) + +const ( + sshUploadPackPath = "/ssh-upload-pack" +) + +func TestSSHUploadPack(t *testing.T) { + addr := setupGitalyServer(t) + a := &api.Response{GitalyServer: api.GitalyServer{Address: addr}} + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + handleSSHUploadPack(w, r, a) + })) + defer ts.Close() + + buf := &bytes.Buffer{} + res, err := http.Post(ts.URL+sshUploadPackPath, "", buf) + require.NoError(t, err) + + err = res.Body.Close() + require.NoError(t, err) + + require.Equal(t, http.StatusOK, res.StatusCode) +} + +func TestSSHUploadPack_GitalyConnection(t *testing.T) { + a := &api.Response{GitalyServer: api.GitalyServer{Address: "wrong"}} + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + handleSSHUploadPack(w, r, a) + })) + defer ts.Close() + + buf := &bytes.Buffer{} + res, err := http.Post(ts.URL+sshUploadPackPath, "", buf) + require.NoError(t, err) + + err = res.Body.Close() + require.NoError(t, err) + + require.Equal(t, http.StatusInternalServerError, res.StatusCode) +} + +func TestSSHUploadPack_FullDuplex(t *testing.T) { + addr := setupGitalyServer(t) + a := &api.Response{GitalyServer: api.GitalyServer{Address: addr}} + + time.Sleep(10 * time.Millisecond) + r := httptest.NewRequest("POST", sshUploadPackPath, nil) + w := httptest.NewRecorder() + + handleSSHUploadPack(w, r, a) + + res := w.Result() + + err := res.Body.Close() + require.NoError(t, err) + + require.Equal(t, http.StatusInternalServerError, res.StatusCode) +} + +func setupGitalyServer(t *testing.T) string { + t.Helper() + + return startSmartHTTPServer(t, &smartHTTPServiceServer{ + handler: func(ctx context.Context, _ *gitalypb.PostUploadPackWithSidechannelRequest) (*gitalypb.PostUploadPackWithSidechannelResponse, error) { + conn, err := client.OpenServerSidechannel(ctx) + require.NoError(t, err) + + defer conn.Close() + + _, err = io.Copy(io.Discard, conn) + require.NoError(t, err) + + return &gitalypb.PostUploadPackWithSidechannelResponse{}, nil + }, + }) +} diff --git a/workhorse/internal/gitaly/gitaly.go b/workhorse/internal/gitaly/gitaly.go index 98f73b40a9d51e..cdfda590dc0c07 100644 --- a/workhorse/internal/gitaly/gitaly.go +++ b/workhorse/internal/gitaly/gitaly.go @@ -121,6 +121,13 @@ func NewDiffClient(ctx context.Context, server api.GitalyServer) (context.Contex return withOutgoingMetadata(ctx, server), &DiffClient{grpcClient}, nil } +// NewConnectionWithSidechannel returns a Gitaly connection with a sidechannel +func NewConnectionWithSidechannel(server api.GitalyServer) (*grpc.ClientConn, *gitalyclient.SidechannelRegistry, error) { + conn, err := getOrCreateConnection(server) + + return conn, sidechannelRegistry, err +} + func getOrCreateConnection(server api.GitalyServer) (*grpc.ClientConn, error) { key := getCacheKey(server) diff --git a/workhorse/internal/gitaly/gitaly_test.go b/workhorse/internal/gitaly/gitaly_test.go index 04358ca598e438..59a0ec7dc480b5 100644 --- a/workhorse/internal/gitaly/gitaly_test.go +++ b/workhorse/internal/gitaly/gitaly_test.go @@ -55,6 +55,13 @@ func TestNewDiffClient(t *testing.T) { testOutgoingMetadata(ctx, t) } +func TestNewConnectionWithSidechannel(t *testing.T) { + conn, sidechannel, err := NewConnectionWithSidechannel(serverFixture()) + require.NotNil(t, conn) + require.Equal(t, sidechannelRegistry, sidechannel) + require.NoError(t, err) +} + func testOutgoingMetadata(ctx context.Context, t *testing.T) { t.Helper() md, ok := metadata.FromOutgoingContext(ctx) diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index 00bd32c7326387..466636097bcb34 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -252,6 +252,7 @@ func configureRoutes(u *upstream) { u.route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.UploadPack(api)), withMatcher(isContentType("application/x-git-upload-pack-request"))), u.route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.ReceivePack(api)), withMatcher(isContentType("application/x-git-receive-pack-request"))), u.route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, requestBodyUploader, withMatcher(isContentType("application/octet-stream"))), + u.route("POST", gitProjectPattern+`ssh-upload-pack\z`, git.SSHUploadPack(api)), // CI Artifacts u.route("POST", apiPattern+`v4/jobs/[0-9]+/artifacts\z`, contentEncodingHandler(upload.Artifacts(api, signingProxy, preparer, &u.Config))), -- GitLab