diff --git a/ee/lib/ee/api/helpers/internal_helpers.rb b/ee/lib/ee/api/helpers/internal_helpers.rb index 8d549760579d9a79417e8bb995802f7519e3db97..988b2c22e6c13b5b48eeae7ee66acf014a82b1fa 100644 --- a/ee/lib/ee/api/helpers/internal_helpers.rb +++ b/ee/lib/ee/api/helpers/internal_helpers.rb @@ -15,15 +15,15 @@ def access_checker_for(actor, protocol) end override :send_git_audit_streaming_event - def send_git_audit_streaming_event(msg) - ::Gitlab::GitAuditEvent.new(actor, project).send_audit_event(msg) + def send_git_audit_streaming_event(msg, audit_actor = nil) + ::Gitlab::GitAuditEvent.new(audit_actor || actor, project).send_audit_event(msg) end override :need_git_audit_event? - def need_git_audit_event? + def need_git_audit_event?(audit_actor = nil) return true if super - ::Gitlab::GitAuditEvent.new(actor, project).enabled? + ::Gitlab::GitAuditEvent.new(audit_actor || actor, project).enabled? end end end diff --git a/ee/lib/gitlab/git_audit_event.rb b/ee/lib/gitlab/git_audit_event.rb index 3fc72752257c8d70e6c901cf33fb303cccbe87fb..856eefe436bd43391ac65d1773f3955f9ffb19c9 100644 --- a/ee/lib/gitlab/git_audit_event.rb +++ b/ee/lib/gitlab/git_audit_event.rb @@ -11,13 +11,11 @@ def initialize(player, project) end def enabled? - return false if user.blank? || project.blank? - - ::Feature.enabled?(:log_git_streaming_audit_events, project) + valid_user_or_author? && project.present? && ::Feature.enabled?(:log_git_streaming_audit_events, project) end def send_audit_event(message) - return if user.blank? || project.blank? + return unless valid_user_or_author? && project.present? ip_address = message.delete(:ip_address) if message.is_a?(Hash) @@ -34,5 +32,11 @@ def send_audit_event(message) ::Gitlab::Audit::Auditor.audit(audit_context) end + + private + + def valid_user_or_author? + user.present? || author.is_a?(DeployToken) + end end end diff --git a/ee/spec/lib/gitlab/git_audit_event_spec.rb b/ee/spec/lib/gitlab/git_audit_event_spec.rb index 27dc592998bda2a9bbfe264117e1ba9690b5c9e6..9fb8ca16ef5960e83ae711affb25a2fbbc2c7bd1 100644 --- a/ee/spec/lib/gitlab/git_audit_event_spec.rb +++ b/ee/spec/lib/gitlab/git_audit_event_spec.rb @@ -31,6 +31,14 @@ end end + context 'when player is a deploy token' do + let(:player) { create(:deploy_token, projects: [project]) } + + it 'is enabled' do + expect(subject.enabled?).to be_truthy + end + end + context 'when player is nil' do let(:player) { nil } @@ -76,6 +84,25 @@ end end + context 'when player is a deploy token' do + let_it_be(:deploy_token) { create(:deploy_token, projects: [project]) } + + subject { described_class.new(deploy_token, project) } + + it 'sends git audit event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(a_hash_including( + name: 'repository_git_operation', + stream_only: true, + author: deploy_token, + scope: project, + target: project, + message: msg + )).once + + subject.send_audit_event(msg) + end + end + context 'when player is ::API::Support::GitAccessActor' do let_it_be(:user) { player } let_it_be(:key) { create(:key, user: user) } diff --git a/ee/spec/requests/api/internal/shellhorse_spec.rb b/ee/spec/requests/api/internal/shellhorse_spec.rb index ea040529197f36ba6e2d8ef4f14338cb43417de8..ae4a7c574e01b5a6b94bf9b17e805bc7aa75f04f 100644 --- a/ee/spec/requests/api/internal/shellhorse_spec.rb +++ b/ee/spec/requests/api/internal/shellhorse_spec.rb @@ -79,6 +79,34 @@ expect(json_response["message"]).to eq(expected_msg.stringify_keys) end + context 'with deploy_token_id parameter' do + let_it_be(:deploy_token) { create(:deploy_token, projects: [project]) } + let(:audit_message) do + { + name: 'repository_git_operation', + stream_only: true, + author: deploy_token, + scope: project, + target: project, + message: expected_msg + } + end + + before do + valid_params.merge!({ username: deploy_token.username, deploy_token_id: deploy_token.id }) + end + + it 'recognizes the deploy token as the author of the audit event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(a_hash_including(audit_message)).once + + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response["status"]).to be_truthy + expect(json_response["message"]).to eq(expected_msg.stringify_keys) + end + end + context 'when log_git_streaming_audit_events is disabled' do before do stub_feature_flags(log_git_streaming_audit_events: false) @@ -128,89 +156,17 @@ end end - context "when user does not exist" do - before do - valid_params.merge!({ username: 'none_user' }) - end - - it 'response with not found' do - subject - - expect(response).to have_gitlab_http_status(:not_found) - expect(json_response["status"]).to be_falsey - expect(json_response['message']).to include("you don't have permission to view it") - end - end - context "when project does not exist" do before do valid_params.merge!({ gl_repository: "project-#{non_existing_record_id}" }) end - it 'response with not found' do + it 'response with no git audit event needed' do subject - expect(response).to have_gitlab_http_status(:not_found) + expect(response).to have_gitlab_http_status(:ok) expect(json_response["status"]).to be_falsey - expect(json_response['message']).to include("namespace you were looking for could not be found") - end - end - - context 'when request times out' do - it 'responds with a timeout' do - expect_next_instance_of(Gitlab::GitAccess) do |access| - expect(access).to receive(:check).and_raise(Gitlab::GitAccess::TimeoutError, "Timeout") - end - subject - - expect(response).to have_gitlab_http_status(:service_unavailable) - expect(json_response['status']).to be_falsey - expect(json_response['message']).to eq("Timeout") - end - end - - context "when access denied" do - before do - project.add_guest(user) - end - - context "when git upload pack" do - it 'response with none access' do - subject - - expect(response).to have_gitlab_http_status(:unauthorized) - expect(json_response["status"]).to be_falsey - end - end - - context "when git receive pack" do - let(:action) { 'git-receive-pack' } - let(:verb) { 'push' } - - it 'responsed with none access' do - subject - - expect(response).to have_gitlab_http_status(:unauthorized) - expect(json_response["status"]).to be_falsey - end - end - end - - context 'when result is not ::Gitlab::GitAccessResult::Success' do - it 'responds with 500' do - allow_next_instance_of(Gitlab::GitAccess) do |access| - allow(access).to receive(:check).and_return(nil) - end - - allow_next_instance_of(::Gitlab::GitAuditEvent) do |audit| - allow(audit).to receive(:enabled?).and_return(true) - end - - subject - - expect(response).to have_gitlab_http_status(:internal_server_error) - expect(json_response['status']).to be_falsey - expect(json_response['message']).to eq(::API::Helpers::InternalHelpers::UNKNOWN_CHECK_RESULT_ERROR) + expect(json_response['message']).to include("No git audit event needed") end end end diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 14e01f329f81c31d830b17e125aadbedf74bed90..f82136aa59bf77912ebd7be382faf842e73e9a7a 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -132,11 +132,11 @@ def with_admin_mode_bypass!(actor_id, &block) Gitlab::Auth::CurrentUserMode.bypass_session!(actor_id, &block) end - def send_git_audit_streaming_event(msg) + def send_git_audit_streaming_event(_msg, _audit_actor = nil) # Defined in EE end - def need_git_audit_event? + def need_git_audit_event?(_audit_actor = nil) false end diff --git a/lib/api/internal/shellhorse.rb b/lib/api/internal/shellhorse.rb index 958e428fffecce072f54cf2be1557ab229e791e1..326cfc82694051004a2091be71edb7823979faa5 100644 --- a/lib/api/internal/shellhorse.rb +++ b/lib/api/internal/shellhorse.rb @@ -34,6 +34,7 @@ def check_clone_or_pull_or_push_verb(params) requires :gl_repository, type: String # repository identifier, such as project-7 requires :changes, type: String optional :check_ip, type: String + optional :deploy_token_id, type: String optional :packfile_stats, type: Hash do # wants is the number of objects the client announced it wants. optional :wants, type: Integer @@ -47,18 +48,14 @@ def check_clone_or_pull_or_push_verb(params) break response_with_status(code: 400, success: false, message: "No valid action specified") end - check_result = access_check_result - break check_result if unsuccessful_response?(check_result) + # We need to recognize a deploy token as the author of the audit event + # if it was used as the authorized actor previously in a Git operation over HTTP + audit_actor = DeployToken.find_by_id(params[:deploy_token_id]) if params[:deploy_token_id] - unless need_git_audit_event? + unless need_git_audit_event?(audit_actor) break response_with_status(code: 200, success: false, message: "No git audit event needed") end - unless check_result.is_a?(::Gitlab::GitAccessResult::Success) - break response_with_status(code: 500, success: false, - message: ::API::Helpers::InternalHelpers::UNKNOWN_CHECK_RESULT_ERROR) - end - audit_message = { protocol: params[:protocol], action: params[:action], @@ -70,7 +67,7 @@ def check_clone_or_pull_or_push_verb(params) # is set through the `check_ip` parameter. audit_message[:ip_address] = params[:check_ip] if include_ip_address_in_audit_event?(params[:check_ip]) - send_git_audit_streaming_event(audit_message) + send_git_audit_streaming_event(audit_message, audit_actor) response_with_status(message: audit_message.except(:ip_address)) end end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 3bf09c711c3a025505b2d084d60669824173102d..4f6fe93f646d81e41638256e4079ed2a8e30b84d 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -45,6 +45,10 @@ def git_http_ok( } } + # Git audit events through /shellhorse endpoint need to identify + # if the Git action over HTTP was performed using a deploy token. + attrs[:GLDeployTokenID] = user.id.to_s if need_audit && user.is_a?(DeployToken) + if authentication_context[:authentication_method] == :ci_job_token attrs[:GlBuildID] = authentication_context[:authentication_method_id].to_s end diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index 811ca1e69dc98d4f6ff0aafb793d01f4678bfd0d..7b1472d4a03d01f570b3578e86256ab0c4bf9330 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -419,10 +419,31 @@ def call_verify(headers) end end - context 'when remote_ip is not available in the application context' do - it 'does not include RemoteIP params' do - result = described_class.git_http_ok(repository, Gitlab::GlRepository::PROJECT, user, action) - expect(result[:GitalyServer][:call_metadata]).not_to have_key('remote_ip') + context 'when user is a deploy token and audit is needed' do + let(:deploy_token) { create(:deploy_token) } + + subject { described_class.git_http_ok(repository, Gitlab::GlRepository::PROJECT, deploy_token, action, need_audit: true) } + + it 'includes GLDeployTokenId in the response' do + expect(subject).to include(GLDeployTokenID: deploy_token.id.to_s) + end + end + + context 'when user is a deploy token but audit is not needed' do + let(:deploy_token) { create(:deploy_token) } + + subject { described_class.git_http_ok(repository, Gitlab::GlRepository::PROJECT, deploy_token, action, need_audit: false) } + + it 'does not include GLDeployTokenId in the response' do + expect(subject).not_to have_key(:GLDeployTokenID) + end + end + + context 'when user is not a deploy token but audit is needed' do + subject { described_class.git_http_ok(repository, Gitlab::GlRepository::PROJECT, user, action, need_audit: true) } + + it 'does not include GLDeployTokenId in the response' do + expect(subject).not_to have_key(:GLDeployTokenID) end end end diff --git a/workhorse/internal/api/api.go b/workhorse/internal/api/api.go index 3026e22beddf7546597136329f4664e44fa28203..b1bf3cfbf891eaed1ca811027e07a8af801778a5 100644 --- a/workhorse/internal/api/api.go +++ b/workhorse/internal/api/api.go @@ -162,6 +162,9 @@ type Response struct { // 'git push' and 'git pull' GL_REPOSITORY string // nolint:revive // used as env variable + // GLDeployTokenID holds the already authenticated deploy token after a Git over HTTP operation + GLDeployTokenID string + // Id of the pipeline build GLBuildID string @@ -319,6 +322,7 @@ type GitAuditEventRequest struct { Protocol string `json:"protocol"` Repo string `json:"gl_repository"` Username string `json:"username"` + DeployTokenID string `json:"deploy_token_id,omitempty"` PackfileStats *gitalypb.PackfileNegotiationStatistics `json:"packfile_stats,omitempty"` Changes string `json:"changes"` } diff --git a/workhorse/internal/api/api_test.go b/workhorse/internal/api/api_test.go index 5292f7670561bae5b76f796d35f706e842553807..cb48b03139bd935ba84243104616f57cd387e911 100644 --- a/workhorse/internal/api/api_test.go +++ b/workhorse/internal/api/api_test.go @@ -197,43 +197,74 @@ func TestPreAuthorizeFixedPath(t *testing.T) { func TestSendGitAuditEvent(t *testing.T) { testhelper.ConfigureSecret() - var ( - requestHeaders http.Header - requestBody GitAuditEventRequest - ) + testCases := []struct { + name string + deployTokenID string + username string + }{ + { + name: "with user", + username: "GitLab-Shell", + }, + { + name: "with deploy token", + username: "gitlab-token-test", + deployTokenID: "3", + }, + } - ts := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/v4/internal/shellhorse/git_audit_event" { - return - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var ( + requestHeaders http.Header + requestBody GitAuditEventRequest + ) + + ts := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v4/internal/shellhorse/git_audit_event" { + return + } + + requestHeaders = r.Header + defer r.Body.Close() + b, err := io.ReadAll(r.Body) + assert.NoError(t, err) + err = json.Unmarshal(b, &requestBody) + assert.NoError(t, err) + })) + defer ts.Close() + + api := NewAPI(helper.URLMustParse(ts.URL), "123", http.DefaultTransport) + auditRequest := GitAuditEventRequest{ + Action: "git-receive-request", + Protocol: "http", + Repo: "project-1", + Username: tc.username, + Changes: "_any", + PackfileStats: &gitalypb.PackfileNegotiationStatistics{ + Wants: 3, + Haves: 23, + }, + } - requestHeaders = r.Header - defer r.Body.Close() - b, err := io.ReadAll(r.Body) - assert.NoError(t, err) - err = json.Unmarshal(b, &requestBody) - assert.NoError(t, err) - })) - defer ts.Close() + if tc.deployTokenID != "" { + auditRequest.DeployTokenID = tc.deployTokenID + } - api := NewAPI(helper.URLMustParse(ts.URL), "123", http.DefaultTransport) - auditRequest := GitAuditEventRequest{ - Action: "git-receive-request", - Protocol: "http", - Repo: "project-1", - Username: "GitLab-Shell", - Changes: "_any", - PackfileStats: &gitalypb.PackfileNegotiationStatistics{ - Wants: 3, - Haves: 23, - }, - } - err := api.SendGitAuditEvent(context.Background(), auditRequest) - require.NoError(t, err) + err := api.SendGitAuditEvent(context.Background(), auditRequest) + require.NoError(t, err) - require.NotEmpty(t, requestHeaders) - require.NotEmpty(t, requestHeaders["Gitlab-Workhorse-Api-Request"]) - require.Equal(t, auditRequest, requestBody) + require.NotEmpty(t, requestHeaders) + require.NotEmpty(t, requestHeaders["Gitlab-Workhorse-Api-Request"]) + require.Equal(t, auditRequest, requestBody) + + if tc.deployTokenID != "" { + require.Equal(t, tc.deployTokenID, requestBody.DeployTokenID) + } else { + require.Empty(t, requestBody.DeployTokenID) + } + }) + } } func Test_passResponseBack(t *testing.T) { diff --git a/workhorse/internal/git/git-http.go b/workhorse/internal/git/git-http.go index 52671d24c48680ed1f15a272ff3f486cc7065edc..145acb633e5c248551cb11b9f51a8a7d72b5c77c 100644 --- a/workhorse/internal/git/git-http.go +++ b/workhorse/internal/git/git-http.go @@ -114,6 +114,7 @@ func sendGitAuditEvent(action string) func(*api.API, *http.Request, *api.Respons Protocol: "http", Repo: response.GL_REPOSITORY, Username: response.GL_USERNAME, + DeployTokenID: response.GLDeployTokenID, PackfileStats: stats, Changes: "_any", })