From 89f90e35043cd677504e7c4cd08015449cc9d31c Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Tue, 15 Jul 2025 20:23:57 +0530 Subject: [PATCH 01/13] Add partner token verification service for Secret Detection Implements the GitLab-side service for verifying partner platform tokens through the Secret Detection Response Service (SDRS). - Add PartnerTokenVerificationService to handle verification requests - Validates prerequisites (feature flag, SDRS config, permissions) - Generates JWT tokens for secure SDRS authentication - Sends async verification requests with proper error handling - Updates token status throughout the verification process - Add CreateOrUpdateService for managing FindingTokenStatus records - Creates or updates token verification status - Validates status transitions - Maintains audit trail of verification attempts - Add comprehensive test coverage for both services - Tests for various error scenarios - Tests for JWT generation and SDRS communication - Tests for status transitions and validations This enables security teams to verify the status of leaked partner tokens, helping prioritize remediation efforts on active credentials. EE: true --- .../vulnerabilities/finding_token_status.rb | 5 +- .../partner_token_verification_service.rb | 210 +++++++++++++ .../create_or_update_service.rb | 39 +++ .../development/token_verification_flow.yml | 8 + .../finding_token_status_spec.rb | 5 +- ...partner_token_verification_service_spec.rb | 276 ++++++++++++++++++ .../create_or_update_service_spec.rb | 157 ++++++++++ 7 files changed, 698 insertions(+), 2 deletions(-) create mode 100644 ee/app/services/security/secret_detection/partner_token_verification_service.rb create mode 100644 ee/app/services/vulnerabilities/finding_token_statuses/create_or_update_service.rb create mode 100644 ee/config/feature_flags/development/token_verification_flow.yml create mode 100644 ee/spec/services/security/secret_detection/partner_token_verification_service_spec.rb create mode 100644 ee/spec/services/vulnerabilities/finding_token_statuses/create_or_update_service_spec.rb diff --git a/ee/app/models/vulnerabilities/finding_token_status.rb b/ee/app/models/vulnerabilities/finding_token_status.rb index 9f957d93249f8e..a0d2b57bdfb6db 100644 --- a/ee/app/models/vulnerabilities/finding_token_status.rb +++ b/ee/app/models/vulnerabilities/finding_token_status.rb @@ -8,7 +8,10 @@ class FindingTokenStatus < ::SecApplicationRecord enum :status, { unknown: 0, active: 1, - inactive: 2 + inactive: 2, + pending: 3, + error: 4, + revoked: 5 }, prefix: true belongs_to :finding, diff --git a/ee/app/services/security/secret_detection/partner_token_verification_service.rb b/ee/app/services/security/secret_detection/partner_token_verification_service.rb new file mode 100644 index 00000000000000..df292b1a114fc6 --- /dev/null +++ b/ee/app/services/security/secret_detection/partner_token_verification_service.rb @@ -0,0 +1,210 @@ +# frozen_string_literal: true + +module Security + module SecretDetection + class PartnerTokenVerificationService < ::BaseService + include Gitlab::Utils::StrongMemoize + + SDRS_REQUEST_TIMEOUT = 30.seconds + SDRS_ENDPOINT = '/api/v1/token/verify' + + attr_reader :finding, :current_user, :project + + def initialize(current_user, finding) + @finding = finding + @current_user = current_user + @project = finding&.project + end + + def execute + return error('Feature is disabled') unless feature_enabled? + return error('SDRS not configured') unless sdrs_configured? + return error('Invalid finding') unless valid_finding? + return error('Unauthorized') unless can_verify_token? + + update_token_status('pending') + + jwt_token = generate_jwt_token + return error('Failed to generate JWT') unless jwt_token + + response = send_verification_request(jwt_token) + return response if response.error? + + handle_sdrs_response(response.payload[:http_response]) + rescue StandardError => e + handle_unexpected_error(e) + end + + private + + def can_verify_token? + can?(current_user, :read_vulnerability, project) + end + + def feature_enabled? + project.present? && project.licensed_feature_available?(:secret_detection) && + Feature.enabled?(:token_verification_flow, project, type: :development) + end + + def sdrs_configured? + ::Gitlab::CurrentSettings.sdrs_enabled && sdrs_url.present? + end + + def valid_finding? + finding.report_type.to_s == 'secret_detection' && token_value.present? + end + + def generate_jwt_token + ::Authz::SdrsAuthenticationService.generate_token( + user: current_user, + project: project, + finding_id: finding.id + ) + rescue ::Authz::SdrsAuthenticationService::SigningKeyNotConfigured => e + log_error("JWT signing key not configured", e) + update_token_status('error', error: 'Configuration error') + nil + rescue StandardError => e + log_error("Failed to generate JWT token", e) + update_token_status('error', error: 'Token generation failed') + nil + end + + def send_verification_request(jwt_token) + http_response = Gitlab::HTTP.post( + request_url, + headers: request_headers(jwt_token), + body: request_body.to_json, + timeout: SDRS_REQUEST_TIMEOUT, + open_timeout: 5.seconds, + read_timeout: SDRS_REQUEST_TIMEOUT + ) + success(http_response: http_response) + rescue StandardError => e + log_error("Unexpected error during SDRS request", e) + update_token_status('error', error: 'Unexpected error during request') + error("Unexpected error during SDRS request: #{e.message}", e.class) + end + + def handle_sdrs_response(response) + return error('No response received') unless response + + if response.code == 202 + log_info("Token verification request accepted by SDRS") + success(finding_id: finding.id, request_id: request_id) + else + log_error("Unexpected SDRS response", nil, response_code: response.code) + update_token_status('error', error: 'Unexpected response', code: response.code) + error('Unexpected SDRS response', :unexpected_sdrs_response) + end + end + + def handle_unexpected_error(exception) + Gitlab::ErrorTracking.track_and_raise_for_dev_exception( + exception, + finding_id: finding.id, + user_id: current_user.id, + project_id: project.id, + token_type: token_type + ) + update_token_status('error', error: exception.message) + error(exception.message, :internal_error) + end + + def update_token_status(status, metadata = {}) + log_info("Token status metadata", metadata) if metadata.present? + + ::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService + .new(finding: finding, current_user: current_user) + .execute(status: status) + rescue StandardError => e + log_error("Failed to update token status", e, status: status) + end + + def request_url + "#{sdrs_url}#{SDRS_ENDPOINT}" + end + + def request_headers(jwt_token) + { + 'Authorization' => "Bearer #{jwt_token}", + 'Content-Type' => 'application/json', + 'Accept' => 'application/json', + 'X-Request-ID' => request_id + } + end + + def request_body + { + token_type: token_type, + token_value: token_value, + finding_id: finding.id, + callback_url: callback_url + } + end + + def token_type + finding.metadata['token_type'] || finding.primary_identifier&.external_id + end + strong_memoize_attr :token_type + + def token_value + finding.metadata['raw_source_code_extract'] || + finding.metadata['token_value'] + end + strong_memoize_attr :token_value + + def callback_url + # TODO: Change this to Gitlab::Routing.url_helpers.api_v4_internal_token_status_callback_url + # once https://gitlab.com/gitlab-org/gitlab/-/issues/551363 is done + "#{Gitlab.config.gitlab.url}/api/v4/internal/token_status/callback" + end + strong_memoize_attr :callback_url + + def sdrs_url + ::Gitlab::CurrentSettings.sdrs_url + end + strong_memoize_attr :sdrs_url + + def request_id + @request_id ||= SecureRandom.uuid + end + + def log_info(message, context = {}) + Gitlab::AppLogger.info(structured_payload(message, context)) + end + + def log_error(message, exception = nil, context = {}) + payload = structured_payload(message, context) + payload[:exception] = exception.class.name if exception + payload[:exception_message] = exception.message if exception + payload[:exception_backtrace] = Gitlab::BacktraceCleaner.clean_backtrace(exception.backtrace) if exception + + Gitlab::AppLogger.error(payload) + end + + def structured_payload(message, context = {}) + { + message: message, + service_class: self.class.name, + finding_id: finding.id, + project_id: project.id, + project_path: project.full_path, + user_id: current_user.id, + username: current_user.username, + token_type: token_type, + request_id: request_id, + feature_flag_enabled: feature_enabled? + }.merge(context) + end + + def error(message, type = nil) + ServiceResponse.error(message: message, payload: { error_type: type }) + end + + def success(payload) + ServiceResponse.success(payload: payload) + end + end + end +end diff --git a/ee/app/services/vulnerabilities/finding_token_statuses/create_or_update_service.rb b/ee/app/services/vulnerabilities/finding_token_statuses/create_or_update_service.rb new file mode 100644 index 00000000000000..db99808cc06198 --- /dev/null +++ b/ee/app/services/vulnerabilities/finding_token_statuses/create_or_update_service.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Vulnerabilities + module FindingTokenStatuses + class CreateOrUpdateService < ::BaseService + attr_reader :finding, :current_user, :project + + def initialize(finding:, current_user:) + @finding = finding + @current_user = current_user + @project = finding.project + end + + def execute(status:) + return ServiceResponse.error(message: 'Invalid parameters') unless valid_status?(status) + + token_status = finding.finding_token_status || + finding.build_finding_token_status + + token_status.assign_attributes( + status: status, + project_id: project.id + ) + + if token_status.save + ServiceResponse.success(payload: { token_status: token_status }) + else + ServiceResponse.error(message: token_status.errors.full_messages.to_sentence) + end + end + + private + + def valid_status?(status) + status.present? && status.in?(%w[pending active revoked error unknown inactive]) + end + end + end +end diff --git a/ee/config/feature_flags/development/token_verification_flow.yml b/ee/config/feature_flags/development/token_verification_flow.yml new file mode 100644 index 00000000000000..59546ecb35b0f8 --- /dev/null +++ b/ee/config/feature_flags/development/token_verification_flow.yml @@ -0,0 +1,8 @@ +--- +name: token_verification_flow +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/197819 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/551358 +milestone: '18.3' +type: development +group: group::secret detection +default_enabled: false \ No newline at end of file diff --git a/ee/spec/models/vulnerabilities/finding_token_status_spec.rb b/ee/spec/models/vulnerabilities/finding_token_status_spec.rb index 7fd6aaf777a1d2..514f9f545147a2 100644 --- a/ee/spec/models/vulnerabilities/finding_token_status_spec.rb +++ b/ee/spec/models/vulnerabilities/finding_token_status_spec.rb @@ -69,7 +69,10 @@ expect(described_class.statuses).to eq({ 'unknown' => 0, 'active' => 1, - 'inactive' => 2 + 'inactive' => 2, + 'pending' => 3, + 'error' => 4, + 'revoked' => 5 }) end diff --git a/ee/spec/services/security/secret_detection/partner_token_verification_service_spec.rb b/ee/spec/services/security/secret_detection/partner_token_verification_service_spec.rb new file mode 100644 index 00000000000000..948ddd3cb85fa9 --- /dev/null +++ b/ee/spec/services/security/secret_detection/partner_token_verification_service_spec.rb @@ -0,0 +1,276 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::SecretDetection::PartnerTokenVerificationService, feature_category: :secret_detection do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + + let(:finding) do + build_stubbed(:vulnerabilities_finding, + project: project, + report_type: 'secret_detection', + raw_metadata: Gitlab::Json.dump(finding_metadata)) + end + + let(:finding_metadata) do + { + 'token_type' => 'gitlab_pat', + 'raw_source_code_extract' => 'gtlb_test123abc' + } + end + + let(:service) { described_class.new(user, finding) } + let(:sdrs_url) { 'https://sdrs.example.com' } + let(:jwt_token) { 'valid.jwt.token' } + + before do + stub_licensed_features(secret_detection: true) + stub_application_setting( + sdrs_enabled: true, + sdrs_url: sdrs_url, + sdrs_jwt_signing_key: OpenSSL::PKey::RSA.generate(2048).to_pem + ) + + allow(Ability).to receive(:allowed?).with(user, :read_vulnerability, project).and_return(true) + allow(::Authz::SdrsAuthenticationService) + .to receive(:generate_token).and_return(jwt_token) + end + + describe '#execute' do + subject(:execute_service) { service.execute } + + context 'when feature is disabled' do + before do + stub_feature_flags(token_verification_flow: false) + end + + it 'returns error' do + result = execute_service + expect(result).to be_error + expect(result.message).to eq('Feature is disabled') + end + end + + context 'when SDRS is not configured' do + before do + stub_application_setting(sdrs_enabled: false) + end + + it 'returns error' do + result = execute_service + expect(result).to be_error + expect(result.message).to eq('SDRS not configured') + end + end + + context 'when finding is invalid' do + let(:finding_metadata) { { 'token_type' => 'gitlab_pat' } } + + it 'returns error' do + result = execute_service + expect(result).to be_error + expect(result.message).to eq('Invalid finding') + end + end + + context 'when user unauthorized' do + before do + allow(Ability).to receive(:allowed?).with(user, :read_vulnerability, project).and_return(false) + end + + it 'returns error' do + result = execute_service + expect(result).to be_error + expect(result.message).to eq('Unauthorized') + end + end + + context 'when all validations pass' do + let(:status_service) { instance_double(Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) } + + before do + allow(Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) + .to receive(:new).with(finding: finding, current_user: user) + .and_return(status_service) + allow(status_service).to receive(:execute).and_return(ServiceResponse.success) + end + + context 'when JWT generation fails' do + before do + allow(::Authz::SdrsAuthenticationService) + .to receive(:generate_token) + .and_raise(::Authz::SdrsAuthenticationService::SigningKeyNotConfigured) + end + + it 'returns error' do + result = execute_service + expect(result).to be_error + expect(result.message).to eq('Failed to generate JWT') + end + + it 'updates token status to pending then error' do + expect(status_service).to receive(:execute).with(status: 'pending').ordered + expect(status_service).to receive(:execute).with(status: 'error').ordered + + execute_service + end + end + + context 'when sending request to SDRS' do + let(:request_stub) do + stub_request(:post, "#{sdrs_url}/api/v1/token/verify") + .with( + headers: { + 'Authorization' => "Bearer #{jwt_token}", + 'Content-Type' => 'application/json' + }, + body: hash_including( + 'token_type' => 'gitlab_pat', + 'token_value' => 'gtlb_test123abc', + 'finding_id' => finding.id + ) + ) + end + + context 'when request is accepted' do + before do + request_stub.to_return(status: 202, body: '{}') + end + + it 'returns success' do + result = execute_service + expect(result).to be_success + expect(result.payload).to include(finding_id: finding.id) + end + + it 'updates token status to pending' do + expect(status_service).to receive(:execute).with(status: 'pending') + execute_service + end + end + + context 'when request fails' do + before do + request_stub.to_return(status: 400, body: '{}') + end + + it 'returns error' do + result = execute_service + expect(result).to be_error + expect(result.message).to eq('Unexpected SDRS response') + end + + it 'updates token status to pending then error' do + expect(status_service).to receive(:execute).with(status: 'pending').ordered + expect(status_service).to receive(:execute).with(status: 'error').ordered + + execute_service + end + end + + context 'when network error occurs' do + before do + request_stub.to_timeout + end + + it 'handles network error gracefully' do + response = execute_service + + expect(response).to be_error + expect(response.message).to eq('Unexpected error during SDRS request: execution expired') + expect(response.payload[:error_type]).to eq(Net::OpenTimeout) + end + + it 'updates token status to pending then error' do + expect(status_service).to receive(:execute).with(status: 'pending').ordered + expect(status_service).to receive(:execute).with(status: 'error').ordered + + response = execute_service + expect(response.payload[:error_type]).to eq(Net::OpenTimeout) + end + end + + context 'when unexpected error occurs during response handling' do + before do + request_stub.to_return(status: 202, body: { status: 'pending' }.to_json) + + allow(service).to receive(:handle_sdrs_response).and_raise(StandardError.new('Unexpected error')) + end + + it 'tracks the error and returns error response' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) + .with( + instance_of(StandardError), + hash_including( + finding_id: finding.id, + user_id: user.id, + project_id: project.id, + token_type: 'gitlab_pat' + ) + ) + + response = execute_service + + expect(response).to be_error + expect(response.message).to eq('Unexpected error') + expect(response.payload[:error_type]).to eq(:internal_error) + end + + it 'updates token status to pending then error' do + expect(status_service).to receive(:execute).with(status: 'pending').ordered + expect(status_service).to receive(:execute).with(status: 'error').ordered + + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) + + execute_service + end + end + end + end + + describe 'token type detection' do + let(:status_service) { instance_double(Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) } + + before do + allow(Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) + .to receive(:new).and_return(status_service) + allow(status_service).to receive(:execute).and_return(ServiceResponse.success) + end + + context 'when token_type is in metadata' do + it 'uses token_type from metadata' do + stub_request(:post, "#{sdrs_url}/api/v1/token/verify") + .with(body: hash_including('token_type' => 'gitlab_pat')) + .to_return(status: 202) + + execute_service + + expect(WebMock).to have_requested(:post, "#{sdrs_url}/api/v1/token/verify") + .with(body: hash_including('token_type' => 'gitlab_pat')) + end + end + + context 'when token_type is not in metadata' do + let(:finding_metadata) { { 'raw_source_code_extract' => 'gtlb_test123abc' } } + + before do + allow(finding).to receive(:primary_identifier).and_return( + build_stubbed(:vulnerabilities_identifier, external_id: 'gitlab_pat') + ) + end + + it 'uses token_type from primary identifier' do + stub_request(:post, "#{sdrs_url}/api/v1/token/verify") + .with(body: hash_including('token_type' => 'gitlab_pat')) + .to_return(status: 202) + + execute_service + + expect(WebMock).to have_requested(:post, "#{sdrs_url}/api/v1/token/verify") + .with(body: hash_including('token_type' => 'gitlab_pat')) + end + end + end + end +end diff --git a/ee/spec/services/vulnerabilities/finding_token_statuses/create_or_update_service_spec.rb b/ee/spec/services/vulnerabilities/finding_token_statuses/create_or_update_service_spec.rb new file mode 100644 index 00000000000000..563aff654f9917 --- /dev/null +++ b/ee/spec/services/vulnerabilities/finding_token_statuses/create_or_update_service_spec.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService, feature_category: :secret_detection do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + + let(:finding) { create(:vulnerabilities_finding, project: project) } + let(:service) { described_class.new(finding: finding, current_user: user) } + let(:status) { 'active' } + + describe '#execute' do + subject(:execute_service) { service.execute(status: status) } + + shared_examples 'a successful response' do + it 'returns success' do + expect(execute_service).to be_success + end + + it 'returns the token status in payload' do + result = execute_service + + expect(result.payload[:token_status]).to be_a(Vulnerabilities::FindingTokenStatus) + expect(result.payload[:token_status]).to have_attributes( + status: status, + project_id: project.id + ) + end + end + + shared_examples 'an error response' do |error_message| + it 'returns error' do + expect(execute_service).to be_error + end + + it 'returns the expected error message' do + expect(execute_service.message).to eq(error_message) + end + + it 'does not create or update any records' do + expect { execute_service }.not_to change { Vulnerabilities::FindingTokenStatus.count } + end + end + + context 'when creating a new token status' do + context 'with valid parameters' do + %w[pending active revoked error unknown inactive].each do |status_value| + context "with status #{status_value}" do + let(:status) { status_value } + + it_behaves_like 'a successful response' + + it 'creates a new token status record' do + expect { execute_service }.to change { Vulnerabilities::FindingTokenStatus.count }.by(1) + end + + it 'associates the token status with the finding' do + execute_service + + expect(finding.reload.finding_token_status).to be_present + expect(finding.finding_token_status.status).to eq(status) + end + end + end + end + end + + context 'when updating an existing token status' do + let!(:existing_status) do + create(:finding_token_status, + finding: finding, + status: 'pending', + project: project) + end + + context 'with valid status transition' do + let(:status) { 'active' } + + it_behaves_like 'a successful response' + + it 'does not create a new record' do + expect { execute_service }.not_to change { Vulnerabilities::FindingTokenStatus.count } + end + + it 'updates the existing status' do + expect { execute_service }.to change { existing_status.reload.status } + .from('pending') + .to('active') + end + end + + context 'when transitioning through different states' do + [ + %w[pending active], + %w[pending revoked], + %w[pending error], + %w[active revoked], + %w[active inactive], + %w[error pending] + ].each do |from_status, to_status| + context "when #{from_status} to #{to_status}" do + let!(:existing_status) do + create(:finding_token_status, + finding: finding, + status: from_status, + project: project) + end + + let(:status) { to_status } + + it_behaves_like 'a successful response' + + it 'transitions the status correctly' do + expect { execute_service }.to change { existing_status.reload.status } + .from(from_status) + .to(to_status) + end + end + end + end + end + + context 'with invalid parameters' do + context 'when status is blank' do + let(:status) { '' } + + it_behaves_like 'an error response', 'Invalid parameters' + end + + context 'when status is nil' do + let(:status) { nil } + + it_behaves_like 'an error response', 'Invalid parameters' + end + + context 'when status is not in allowed values' do + let(:status) { 'invalid_status' } + + it_behaves_like 'an error response', 'Invalid parameters' + end + end + + context 'when save fails due to validation errors' do + before do + allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| + allow(instance).to receive_messages( + save: false, + errors: instance_double(ActiveModel::Errors, full_messages: ['Custom validation error']) + ) + end + end + + it_behaves_like 'an error response', 'Custom validation error' + end + end +end -- GitLab From d04fbe42564242b4b71bcd6ebf593b3b5e25e30c Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Tue, 15 Jul 2025 23:49:03 +0530 Subject: [PATCH 02/13] Add async worker for partner token verification Implements background job processing for partner token verification requests with intelligent retry logic and error handling. - Add PartnerTokenVerificationWorker with exponential backoff retry - Retries network/timeout errors up to 3 times (4, 16, 64 seconds) - Fails fast for configuration/authorization errors - Maintains data consistency with sticky database reads - Implement comprehensive error classification - Network errors (timeout, SSL, connection) are retryable - Business logic errors (auth, config, validation) fail immediately - Prevents unnecessary retries for permanent failures - Add detailed logging and monitoring metadata - Tracks success/error status for each job - Includes request IDs for tracing - Logs retry decisions for debugging This enables asynchronous processing of token verification requests, preventing timeouts in the main request flow and providing resilience against temporary network issues. Changelog: added EE: true --- config/sidekiq_queues.yml | 2 + ee/app/workers/all_queues.yml | 10 ++ .../partner_token_verification_worker.rb | 108 ++++++++++++ .../partner_token_verification_worker_spec.rb | 157 ++++++++++++++++++ 4 files changed, 277 insertions(+) create mode 100644 ee/app/workers/security/secret_detection/partner_token_verification_worker.rb create mode 100644 ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index fe4fbf36e59d49..4fd8cb0882a734 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -999,6 +999,8 @@ - 2 - - security_scans_purge_by_job_id - 1 +- - security_secret_detection_partner_token_verification + - 1 - - security_secret_detection_update_token_status - 1 - - security_sync_linked_pipeline_execution_policy_configs diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 7f3373da70e2e9..3ed5ec68597dd1 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -3784,6 +3784,16 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: security_secret_detection_partner_token_verification + :worker_name: Security::SecretDetection::PartnerTokenVerificationWorker + :feature_category: :secret_detection + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: security_secret_detection_update_token_status :worker_name: Security::SecretDetection::UpdateTokenStatusWorker :feature_category: :secret_detection diff --git a/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb new file mode 100644 index 00000000000000..dce1752de26926 --- /dev/null +++ b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +module Security + module SecretDetection + class PartnerTokenVerificationWorker + include ApplicationWorker + include Gitlab::Utils::StrongMemoize + + MAX_RETRIES = 3 + + # Retryable exceptions - network and temporary errors that make sense to retry + RETRYABLE_EXCEPTIONS = [Gitlab::HTTP::HTTP_TIMEOUT_ERRORS, EOFError, SocketError, OpenSSL::SSL::SSLError, + OpenSSL::OpenSSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Errno::EHOSTUNREACH, Errno::ENETUNREACH, + Errno::ETIMEDOUT, Net::ReadTimeout, Net::OpenTimeout, Timeout::Error].flatten.freeze + + data_consistency :sticky + feature_category :secret_detection + urgency :low + defer_on_database_health_signal :gitlab_main + idempotent! + + sidekiq_options retry: MAX_RETRIES + + # Exponential backoff configuration + sidekiq_retry_in do |count, exception| + # Only apply exponential backoff for retryable exceptions + if RETRYABLE_EXCEPTIONS.include?(exception.class) + # 4^count seconds (4, 16, 64 seconds) + 4**count + else + # For non-retryable exceptions, fail fast + false + end + end + + def perform(finding_id, user_id) + @finding_id = finding_id + @user_id = user_id + + return log_and_return('Finding not found') unless finding + return log_and_return('User not found') unless user + + result = ::Security::SecretDetection::PartnerTokenVerificationService + .new(user, finding) + .execute + + log_result(result) + + # Re-raise retryable exceptions for Sidekiq to handle + handle_service_errors(result) unless result.success? + end + + private + + attr_reader :finding_id, :user_id + + def finding + Vulnerabilities::Finding.find_by_id(finding_id) + end + strong_memoize_attr :finding + + def user + User.find_by_id(user_id) + end + strong_memoize_attr :user + + def log_result(result) + if result.success? + log_extra_metadata_on_done(:status, 'success') + log_extra_metadata_on_done(:finding_id, result.payload[:finding_id]) + log_extra_metadata_on_done(:request_id, result.payload[:request_id]) if result.payload[:request_id] + else + log_extra_metadata_on_done(:status, 'error') + log_extra_metadata_on_done(:error_message, result.message) + log_extra_metadata_on_done(:error_type, result.payload[:error_type]) if result.payload[:error_type] + end + end + + def handle_service_errors(result) + error_type = result.payload[:error_type] + + # Only retry when send_verification_request fails with retryable network exceptions + raise StandardError, result.message if retryable_exception_class?(error_type) + + # For all other error types (configuration, authorization, validation, etc.) + # don't retry as they won't be resolved by retrying + log_extra_metadata_on_done(:retry_decision, 'not_retryable') + end + + def retryable_exception_class?(exception_class) + return false unless exception_class + + RETRYABLE_EXCEPTIONS.include?(exception_class) + end + + def log_and_return(message) + log_extra_metadata_on_done(:status, 'skipped') + log_extra_metadata_on_done(:reason, message) + Gitlab::AppLogger.info( + message: message, + worker_class: self.class.name, + finding_id: finding_id, + user_id: user_id + ) + end + end + end +end diff --git a/ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb b/ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb new file mode 100644 index 00000000000000..d7a8586064feaa --- /dev/null +++ b/ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::SecretDetection::PartnerTokenVerificationWorker, feature_category: :secret_detection do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:finding) { create(:vulnerabilities_finding, project: project) } + + subject(:worker) { described_class.new } + + describe '#perform' do + let(:service) { instance_double(Security::SecretDetection::PartnerTokenVerificationService) } + let(:service_response) { ServiceResponse.success(payload: { finding_id: finding.id, request_id: 'abc-123' }) } + + before do + allow(Security::SecretDetection::PartnerTokenVerificationService) + .to receive(:new).with(user, finding).and_return(service) + allow(service).to receive(:execute).and_return(service_response) + end + + context 'when finding and user exist' do + it 'calls the service with correct parameters' do + worker.perform(finding.id, user.id) + + expect(Security::SecretDetection::PartnerTokenVerificationService) + .to have_received(:new).with(user, finding) + expect(service).to have_received(:execute) + end + + context 'when service returns success' do + it 'logs success metadata' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:status, 'success') + expect(worker).to receive(:log_extra_metadata_on_done).with(:finding_id, finding.id) + expect(worker).to receive(:log_extra_metadata_on_done).with(:request_id, 'abc-123') + + worker.perform(finding.id, user.id) + end + end + + context 'when service returns error' do + let(:service_response) do + ServiceResponse.error( + message: 'Configuration error', + payload: { error_type: :configuration_error } + ) + end + + it 'logs error metadata and does not retry' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:status, 'error') + expect(worker).to receive(:log_extra_metadata_on_done).with(:error_message, 'Configuration error') + expect(worker).to receive(:log_extra_metadata_on_done).with(:error_type, :configuration_error) + expect(worker).to receive(:log_extra_metadata_on_done).with(:retry_decision, 'not_retryable') + + worker.perform(finding.id, user.id) + end + end + + context 'when service returns retryable network error' do + let(:service_response) do + ServiceResponse.error( + message: 'Unexpected error during SDRS request: Connection timeout', + payload: { error_type: Net::OpenTimeout } + ) + end + + it 'raises exception to trigger retry' do + expect { worker.perform(finding.id, user.id) } + .to raise_error(StandardError, 'Unexpected error during SDRS request: Connection timeout') + end + end + + context 'when service returns non-retryable error' do + let(:service_response) do + ServiceResponse.error( + message: 'Unauthorized', + payload: { error_type: :unauthorized } + ) + end + + it 'does not raise exception' do + expect { worker.perform(finding.id, user.id) }.not_to raise_error + end + end + end + + context 'when finding does not exist' do + it 'logs and returns early' do + expect(Gitlab::AppLogger).to receive(:info).with( + message: 'Finding not found', + worker_class: described_class.name, + finding_id: 999, + user_id: user.id + ) + expect(worker).to receive(:log_extra_metadata_on_done).with(:status, 'skipped') + expect(worker).to receive(:log_extra_metadata_on_done).with(:reason, 'Finding not found') + + worker.perform(999, user.id) + + expect(service).not_to have_received(:execute) + end + end + + context 'when user does not exist' do + it 'logs and returns early' do + expect(Gitlab::AppLogger).to receive(:info).with( + message: 'User not found', + worker_class: described_class.name, + finding_id: finding.id, + user_id: 999 + ) + expect(worker).to receive(:log_extra_metadata_on_done).with(:status, 'skipped') + expect(worker).to receive(:log_extra_metadata_on_done).with(:reason, 'User not found') + + worker.perform(finding.id, 999) + + expect(service).not_to have_received(:execute) + end + end + end + + describe '.sidekiq_retry_in' do + # Test the actual retry behavior by checking retryable exceptions + it 'has correct retry configuration' do + expect(described_class.sidekiq_options_hash['retry']).to eq(3) + end + end + + describe 'worker configuration' do + it 'has correct sidekiq options' do + expect(described_class.sidekiq_options_hash).to include( + 'retry' => 3 + ) + end + + it 'has correct feature category' do + expect(described_class.get_feature_category).to eq(:secret_detection) + end + + it 'has correct urgency' do + expect(described_class.get_urgency).to eq(:low) + end + end + + describe 'retryable exceptions' do + it 'includes expected network exceptions' do + expect(described_class::RETRYABLE_EXCEPTIONS).to include( + Net::OpenTimeout, + EOFError, + SocketError, + OpenSSL::SSL::SSLError, + Errno::ECONNRESET, + Errno::ECONNREFUSED + ) + end + end +end -- GitLab From 234fcdf3569889acc7aeb14b7d77188344c40532 Mon Sep 17 00:00:00 2001 From: Aditya Tiwari Date: Wed, 16 Jul 2025 00:04:56 +0530 Subject: [PATCH 03/13] Apply 2 suggestion(s) to 1 file(s) Co-authored-by: GitLab Duo --- .../secret_detection/partner_token_verification_worker.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb index dce1752de26926..ab02f5c68a1fd9 100644 --- a/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb +++ b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb @@ -55,7 +55,7 @@ def perform(finding_id, user_id) attr_reader :finding_id, :user_id def finding - Vulnerabilities::Finding.find_by_id(finding_id) + Vulnerabilities::Finding.includes(:project).find_by_id(finding_id) end strong_memoize_attr :finding @@ -80,7 +80,7 @@ def handle_service_errors(result) error_type = result.payload[:error_type] # Only retry when send_verification_request fails with retryable network exceptions - raise StandardError, result.message if retryable_exception_class?(error_type) + raise error_type.new(result.message) if retryable_exception_class?(error_type) # For all other error types (configuration, authorization, validation, etc.) # don't retry as they won't be resolved by retrying -- GitLab From 4e4e3fc491e70b98b26cbc74936c8e70059bed5d Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 16 Jul 2025 12:26:29 +0530 Subject: [PATCH 04/13] Fixes cop and test --- ee/app/models/vulnerabilities/finding.rb | 1 + .../secret_detection/partner_token_verification_worker.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ee/app/models/vulnerabilities/finding.rb b/ee/app/models/vulnerabilities/finding.rb index bbe7651e42ef28..1a0ff7f9abf9ae 100644 --- a/ee/app/models/vulnerabilities/finding.rb +++ b/ee/app/models/vulnerabilities/finding.rb @@ -168,6 +168,7 @@ class Finding < ::SecApplicationRecord scope :eager_load_comparison_entities, -> { includes(:scanner, :primary_identifier) } scope :by_primary_identifiers, ->(identifier_ids) { where(primary_identifier: identifier_ids) } scope :by_latest_pipeline, ->(pipeline_id) { where(latest_pipeline_id: pipeline_id) } + scope :with_project, -> { includes(:project) } scope :all_preloaded, -> do preload(:scanner, :identifiers, :feedbacks, project: [:namespace, :project_feature]) diff --git a/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb index ab02f5c68a1fd9..c3bb0658ccf470 100644 --- a/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb +++ b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb @@ -55,7 +55,7 @@ def perform(finding_id, user_id) attr_reader :finding_id, :user_id def finding - Vulnerabilities::Finding.includes(:project).find_by_id(finding_id) + Vulnerabilities::Finding.with_project.find_by_id(finding_id) end strong_memoize_attr :finding @@ -80,7 +80,7 @@ def handle_service_errors(result) error_type = result.payload[:error_type] # Only retry when send_verification_request fails with retryable network exceptions - raise error_type.new(result.message) if retryable_exception_class?(error_type) + raise StandardError, result.message if retryable_exception_class?(error_type) # For all other error types (configuration, authorization, validation, etc.) # don't retry as they won't be resolved by retrying -- GitLab From dfc26ba5e67875a101d637e90b89a486c13b3b81 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 16 Jul 2025 13:44:18 +0530 Subject: [PATCH 05/13] Fixes test --- .../partner_token_verification_worker.rb | 15 ++++++--------- .../partner_token_verification_worker_spec.rb | 9 +-------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb index c3bb0658ccf470..f0908c15e26167 100644 --- a/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb +++ b/ee/app/workers/security/secret_detection/partner_token_verification_worker.rb @@ -6,8 +6,6 @@ class PartnerTokenVerificationWorker include ApplicationWorker include Gitlab::Utils::StrongMemoize - MAX_RETRIES = 3 - # Retryable exceptions - network and temporary errors that make sense to retry RETRYABLE_EXCEPTIONS = [Gitlab::HTTP::HTTP_TIMEOUT_ERRORS, EOFError, SocketError, OpenSSL::SSL::SSLError, OpenSSL::OpenSSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Errno::EHOSTUNREACH, Errno::ENETUNREACH, @@ -19,16 +17,14 @@ class PartnerTokenVerificationWorker defer_on_database_health_signal :gitlab_main idempotent! - sidekiq_options retry: MAX_RETRIES - - # Exponential backoff configuration + # Control retry behavior through sidekiq_retry_in sidekiq_retry_in do |count, exception| - # Only apply exponential backoff for retryable exceptions - if RETRYABLE_EXCEPTIONS.include?(exception.class) - # 4^count seconds (4, 16, 64 seconds) + # Only retry up to 3 times for retryable exceptions + if count < 3 && RETRYABLE_EXCEPTIONS.include?(exception.class) + # Exponential backoff: 4, 16, 64 seconds 4**count else - # For non-retryable exceptions, fail fast + # For non-retryable exceptions or after 3 retries, don't retry false end end @@ -96,6 +92,7 @@ def retryable_exception_class?(exception_class) def log_and_return(message) log_extra_metadata_on_done(:status, 'skipped') log_extra_metadata_on_done(:reason, message) + Gitlab::AppLogger.info( message: message, worker_class: self.class.name, diff --git a/ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb b/ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb index d7a8586064feaa..0756e33a8c32b7 100644 --- a/ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb +++ b/ee/spec/workers/security/secret_detection/partner_token_verification_worker_spec.rb @@ -120,19 +120,12 @@ end describe '.sidekiq_retry_in' do - # Test the actual retry behavior by checking retryable exceptions it 'has correct retry configuration' do - expect(described_class.sidekiq_options_hash['retry']).to eq(3) + expect(described_class.sidekiq_options_hash['retry']).to be(true) end end describe 'worker configuration' do - it 'has correct sidekiq options' do - expect(described_class.sidekiq_options_hash).to include( - 'retry' => 3 - ) - end - it 'has correct feature category' do expect(described_class.get_feature_category).to eq(:secret_detection) end -- GitLab From ec629bea1ff8429fc0cc75d0c9c4acc2a145053d Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Sat, 19 Jul 2025 14:32:54 +0530 Subject: [PATCH 06/13] tmp --- .../vulnerabilities/finding/token_status.rb | 209 ++++++++++++++++++ 1 file changed, 209 insertions(+) create mode 100644 ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb diff --git a/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb new file mode 100644 index 00000000000000..960a087d95946c --- /dev/null +++ b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb @@ -0,0 +1,209 @@ +# ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb +# frozen_string_literal: true + +module API + module Internal + module AppSec + module Vulnerabilities + module Finding + class TokenStatus < ::API::Base + include APIGuard + + before { authenticate_sdrs_callback! } + + helpers do + def authenticate_sdrs_callback! + unauthorized! unless valid_sdrs_jwt? + end + + def valid_sdrs_jwt? + return false unless jwt_token.present? + + begin + @jwt_payload = decode_and_verify_jwt + verify_required_claims + verify_jti_uniqueness + true + rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::InvalidIssuerError, + JWT::InvalidAudError, JWT::InvalidIatError => e + log_jwt_validation_error(e) + false + end + end + + def jwt_token + @jwt_token ||= request.headers['Authorization']&.split(' ')&.last + end + + def decode_and_verify_jwt + JWT.decode( + jwt_token, + sdrs_public_key, + true, + { + algorithm: 'RS256', + verify_iss: true, + iss: 'secret-detection-response-service', + verify_aud: true, + aud: 'gitlab-secret-detection-callback', + verify_iat: true, + verify_expiration: true, + leeway: 30 # 30 seconds leeway for clock skew + } + ).first + end + + def verify_required_claims + required_claims = %w[iss aud exp iat jti gitlab] + missing_claims = required_claims - @jwt_payload.keys + + raise JWT::DecodeError, "Missing required claims: #{missing_claims.join(', ')}" if missing_claims.any? + + # Verify GitLab-specific claims + gitlab_claims = @jwt_payload['gitlab'] + raise JWT::DecodeError, 'Missing gitlab claims' unless gitlab_claims.is_a?(Hash) + + return if gitlab_claims['finding_id'].present? + + raise JWT::DecodeError, + 'Missing finding_id in gitlab claims' + end + + def verify_jti_uniqueness + jti = @jwt_payload['jti'] + + # Use Redis to track JTIs for the duration of token validity + redis_key = "sdrs:callback:jti:#{jti}" + + Gitlab::Redis::SharedState.with do |redis| + # Set with NX (only if not exists) and expiration + # Returns false if key already exists + raise JWT::DecodeError, 'JTI already used' unless redis.set(redis_key, '1', nx: true, ex: 3600) + end + end + + def sdrs_public_key + # In production, this would fetch from JWKS endpoint with caching + # For now, using the configured public key + @sdrs_public_key ||= begin + key_pem = ::Gitlab::CurrentSettings.sdrs_jwt_public_key + raise JWT::DecodeError, 'SDRS public key not configured' unless key_pem.present? + + OpenSSL::PKey::RSA.new(key_pem) + end + end + + def finding + @finding ||= ::Vulnerabilities::Finding.find(@jwt_payload.dig('gitlab', 'finding_id')) + rescue ActiveRecord::RecordNotFound + nil + end + + def log_jwt_validation_error(error) + Gitlab::AppLogger.error( + message: 'SDRS callback JWT validation failed', + error_class: error.class.name, + error_message: error.message, + jti: @jwt_payload&.dig('jti'), + finding_id: @jwt_payload&.dig('gitlab', 'finding_id') + ) + end + + def log_callback_attempt(status:, finding_id:, error: nil) + Gitlab::AppLogger.info( + message: 'SDRS callback received', + status: status, + finding_id: finding_id, + jti: @jwt_payload['jti'], + request_id: request.headers['X-Request-ID'], + error: error + ) + + # Also create an audit event for security tracking + return unless finding&.project + + ::AuditEventService.new( + nil, + nil, + { + action: :custom, + custom_message: "SDRS token verification callback: #{status}", + target_id: finding_id, + target_type: 'Vulnerabilities::Finding', + target_details: "Token verification status: #{status}" + } + ).for_project(finding.project).security_event + end + end + + namespace 'internal/app_sec/vulnerabilities/finding' do + desc 'Receive token status callback from SDRS' do + detail 'Updates the token verification status for a vulnerability finding' + tags %w[internal] + end + params do + requires :finding_id, type: Integer, desc: 'The vulnerability finding ID' + requires :status, type: String, values: %w[active revoked error unknown inactive], + desc: 'Token verification status' + requires :checked_at, type: DateTime, desc: 'When the token was checked' + optional :metadata, type: Hash, desc: 'Additional metadata about the token status' do + optional :scopes, type: Array[String], desc: 'Token scopes (if applicable)' + optional :expires_at, type: DateTime, desc: 'Token expiration time' + optional :revoked_at, type: DateTime, desc: 'When token was revoked' + optional :error_message, type: String, desc: 'Error details if status is error' + optional :account, type: String, desc: 'Account information' + end + end + post 'token_status/callback', urgency: :low, feature_category: :secret_detection do + # Apply rate limiting + check_rate_limit!(:sdrs_callback, scope: request.ip, threshold: 100, interval: 1.minute) + + # Verify finding_id matches JWT claim + forbidden!('Finding ID mismatch') if params[:finding_id] != @jwt_payload.dig('gitlab', 'finding_id') + + # Find the vulnerability finding + not_found!('Finding not found') unless finding + + # Update token status + result = ::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService + .new(finding: finding, current_user: nil) + .execute( + status: params[:status], + metadata: params[:metadata], + checked_at: params[:checked_at] + ) + + if result.success? + log_callback_attempt(status: 'success', finding_id: params[:finding_id]) + status 200 + { success: true } + else + log_callback_attempt( + status: 'error', + finding_id: params[:finding_id], + error: result.message + ) + bad_request!(result.message) + end + rescue StandardError => e + log_callback_attempt( + status: 'error', + finding_id: params[:finding_id], + error: e.message + ) + + Gitlab::ErrorTracking.track_exception( + e, + finding_id: params[:finding_id], + jwt_payload: @jwt_payload + ) + + internal_server_error!('Internal error processing callback') + end + end + end + end + end + end + end +end -- GitLab From 1c580724c34e8428ba1d1dc4d98812279b2496ee Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Tue, 22 Jul 2025 18:35:08 +0530 Subject: [PATCH 07/13] Add sdrs auth callback --- db/docs/p_ci_pipelines_config.yml | 15 + .../20250719093354_add_sdrs_jwt_public_key.rb | 18 + db/schema_migrations/20250719093354 | 1 + db/structure.sql | 1 + ee/app/models/ee/application_setting.rb | 7 +- .../authz/sdrs_authentication_service.rb | 142 ++++++-- ...on_setting_sdrs_jwt_key_configuration.json | 6 + ...lication_setting_sdrs_jwt_signing_key.json | 6 - .../authz/sdrs_authentication_service_spec.rb | 317 +++++++++++++++++- 9 files changed, 481 insertions(+), 32 deletions(-) create mode 100644 db/docs/p_ci_pipelines_config.yml create mode 100644 db/migrate/20250719093354_add_sdrs_jwt_public_key.rb create mode 100644 db/schema_migrations/20250719093354 create mode 100644 ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json delete mode 100644 ee/app/validators/json_schemas/application_setting_sdrs_jwt_signing_key.json diff --git a/db/docs/p_ci_pipelines_config.yml b/db/docs/p_ci_pipelines_config.yml new file mode 100644 index 00000000000000..2afd7d5ba4f796 --- /dev/null +++ b/db/docs/p_ci_pipelines_config.yml @@ -0,0 +1,15 @@ +--- +table_name: p_ci_pipelines_config +classes: +- Ci::PipelineConfig +feature_categories: +- continuous_integration +description: Routing table for ci_pipelines_config +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164455 +milestone: '12.7' +gitlab_schema: gitlab_ci +sharding_key: + project_id: projects +table_size: small +removed_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/194730 +removed_in_milestone: '18.2' diff --git a/db/migrate/20250719093354_add_sdrs_jwt_public_key.rb b/db/migrate/20250719093354_add_sdrs_jwt_public_key.rb new file mode 100644 index 00000000000000..f0618060425219 --- /dev/null +++ b/db/migrate/20250719093354_add_sdrs_jwt_public_key.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddSdrsJwtPublicKey < Gitlab::Database::Migration[2.3] + milestone '18.3' + disable_ddl_transaction! + + def up + with_lock_retries do + add_column :application_settings, :sdrs_jwt_public_key, :jsonb, null: true, if_not_exists: true + end + end + + def down + with_lock_retries do + remove_column :application_settings, :sdrs_jwt_public_key + end + end +end diff --git a/db/schema_migrations/20250719093354 b/db/schema_migrations/20250719093354 new file mode 100644 index 00000000000000..e25283f9c2c8ca --- /dev/null +++ b/db/schema_migrations/20250719093354 @@ -0,0 +1 @@ +bf2386326498c20aa99030d97525510931519533ba657200357280cdbf012256 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index f185f7888c54b8..d2608c4c6dba1a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9577,6 +9577,7 @@ CREATE TABLE application_settings ( default_profile_preferences jsonb DEFAULT '{}'::jsonb NOT NULL, sdrs_enabled boolean DEFAULT false NOT NULL, sdrs_jwt_signing_key jsonb, + sdrs_jwt_public_key jsonb, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)), CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)), diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index a952932bfb7e10..4eba8f2f0550b3 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -138,10 +138,15 @@ module ApplicationSetting encrypts :sdrs_jwt_signing_key - validates :sdrs_jwt_signing_key, json_schema: { filename: 'application_setting_sdrs_jwt_signing_key' }, + validates :sdrs_jwt_signing_key, json_schema: { filename: 'application_setting_sdrs_jwt_key_configuration' }, allow_nil: true validates :sdrs_jwt_signing_key, length: { maximum: 10_000 } + encrypts :sdrs_jwt_public_key + validates :sdrs_jwt_public_key, json_schema: { filename: 'application_setting_sdrs_jwt_key_configuration' }, + allow_nil: true + validates :sdrs_jwt_public_key, length: { maximum: 10_000 } + validates :mirror_capacity_threshold, :mirror_max_capacity, :elasticsearch_indexed_file_size_limit_kb, diff --git a/ee/app/services/authz/sdrs_authentication_service.rb b/ee/app/services/authz/sdrs_authentication_service.rb index 563648717f0055..08da26b8dabc0e 100644 --- a/ee/app/services/authz/sdrs_authentication_service.rb +++ b/ee/app/services/authz/sdrs_authentication_service.rb @@ -2,42 +2,136 @@ module Authz class SdrsAuthenticationService < BaseService + # When GitLab sends JWT to SDRS for verification requests AUDIENCE = 'sdrs' ISSUER = 'gitlab-secret-detection' + + # When SDRS sends JWT to GitLab for callbacks + AUDIENCE_CALLBACK = 'sdrs-callback' + ISSUER_CALLBACK = 'gitlab-secret-detection-callback' + ALGORITHM = 'RS256' VALIDITY_TIME = 1.hour + JTI_EXPIRY_TIME = 2.hours + LEEWAY = 30.seconds + SigningKeyNotConfigured = Class.new(StandardError) + PublicKeyNotConfigured = Class.new(StandardError) - def self.generate_token(user:, project:, finding_id:) - claims = { - iss: ISSUER, - sub: "user:#{user.id}", - aud: AUDIENCE, - exp: VALIDITY_TIME.from_now.to_i, - iat: Time.current.to_i, - jti: SecureRandom.uuid, - gitlab: { - user_id: user.id, - project_id: project.id, - finding_id: finding_id, - service: 'token-verification', - scopes: ['token:verify'] + class << self + def generate_token(user:, project:, finding_id:) + claims = { + iss: ISSUER, + sub: "user:#{user.id}", + aud: AUDIENCE, + exp: VALIDITY_TIME.from_now.to_i, + iat: Time.current.to_i, + nbf: now, + jti: SecureRandom.uuid, + gitlab: { + user_id: user.id, + project_id: project.id, + finding_id: finding_id, + service: 'token-verification', + scopes: ['token:verify'] + } } - } - begin - JWT.encode(claims, signing_key, ALGORITHM) - rescue OpenSSL::PKey::RSAError => e - Gitlab::ErrorTracking.log_and_raise_exception(e) + begin + JWT.encode(claims, signing_key, ALGORITHM) + rescue OpenSSL::PKey::RSAError => e + Gitlab::ErrorTracking.log_and_raise_exception(e) + end end - end - def self.signing_key - key_pem = ::Gitlab::CurrentSettings.sdrs_jwt_signing_key - raise SigningKeyNotConfigured, 'SDRS JWT signing key not configured' if key_pem.blank? + def verify_callback_token(token) + return unless token.present? + + payload = JWT.decode( + token, + sdrs_public_key, + true, + { + algorithm: ALGORITHM, + verify_iss: true, + iss: ISSUER_CALLBACK, + verify_aud: true, + aud: AUDIENCE_CALLBACK, + verify_exp: true, + verify_iat: true, + verify_jti: true, + + # Allow small clock skew + exp_leeway: LEEWAY, + iat_leeway: LEEWAY, + nbf_leeway: LEEWAY + } + ).first + + # Validate required GitLab claims + return unless valid_gitlab_claims?(payload) + + # Prevent replay attacks using JTI + return unless register_jti(payload['jti']) + + payload + rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::InvalidIssuerError, + JWT::InvalidAudError, JWT::InvalidIatError, JWT::InvalidJtiError => e + log_jwt_error(e, token) + nil + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e, jwt_context: { service: self.class.name }) + nil + end + + private + + def signing_key + key_pem = ::Gitlab::CurrentSettings.sdrs_jwt_signing_key + raise SigningKeyNotConfigured, 'SDRS JWT signing key not configured' if key_pem.blank? + + OpenSSL::PKey::RSA.new(key_pem) + end + + def sdrs_public_key + key_pem = ::Gitlab::CurrentSettings.sdrs_jwt_public_key + raise SigningKeyNotConfigured, 'SDRS public key not configured' unless key_pem.present? + + OpenSSL::PKey::RSA.new(key_pem) + end - OpenSSL::PKey::RSA.new(key_pem) + def valid_gitlab_claims?(payload) + gitlab_claims = payload['gitlab'] + + return false unless gitlab_claims.is_a?(Hash) + return false unless gitlab_claims['finding_id'].is_a?(Integer) + return false if gitlab_claims['finding_id'] <= 0 + + true + end + + def register_jti(jti) + return false if jti.blank? + + # Use Redis to track JTIs for replay prevention + redis_key = "sdrs:callback:jti:#{jti}" + + Gitlab::Redis::SharedState.with do |redis| + # SET NX returns true only if key doesn't exist + # EX sets expiration + redis.set(redis_key, Time.current.to_i, nx: true, ex: JTI_EXPIRY_TIME.to_i) + end + end + + def log_jwt_error(error, token) + Gitlab::AppLogger.warn( + message: 'SDRS JWT verification failed', + error_class: error.class.name, + error_message: error.message, + token_length: token&.length + ) + end end end end diff --git a/ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json b/ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json new file mode 100644 index 00000000000000..da9647cd5162e5 --- /dev/null +++ b/ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json @@ -0,0 +1,6 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "SDRS JWT Key Configuration", + "type": "string", + "description": "PEM-encoded RSA key (private or public) for JWT operations" +} \ No newline at end of file diff --git a/ee/app/validators/json_schemas/application_setting_sdrs_jwt_signing_key.json b/ee/app/validators/json_schemas/application_setting_sdrs_jwt_signing_key.json deleted file mode 100644 index 7ade3054926f6f..00000000000000 --- a/ee/app/validators/json_schemas/application_setting_sdrs_jwt_signing_key.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "title": "SDRS JWT Signing Key Configuration", - "type": "string", - "description": "PEM-encoded RSA private key for JWT signing" -} diff --git a/ee/spec/services/authz/sdrs_authentication_service_spec.rb b/ee/spec/services/authz/sdrs_authentication_service_spec.rb index d4d2b04364ccfb..2a8f71e62340e9 100644 --- a/ee/spec/services/authz/sdrs_authentication_service_spec.rb +++ b/ee/spec/services/authz/sdrs_authentication_service_spec.rb @@ -7,6 +7,7 @@ let_it_be(:project) { create(:project) } let(:finding_id) { 123 } let(:rsa_key) { OpenSSL::PKey::RSA.generate(2048) } + let(:sdrs_rsa_key) { OpenSSL::PKey::RSA.generate(2048) } describe '.generate_token' do subject(:token) { described_class.generate_token(user: user, project: project, finding_id: finding_id) } @@ -18,6 +19,7 @@ it 'includes correct claims', :freeze_time do allow(SecureRandom).to receive(:uuid).and_return('test-uuid-12345') + decoded = JWT.decode(token, rsa_key.public_key, true, algorithm: 'RS256') claims = decoded.first @@ -27,6 +29,7 @@ 'sub' => "user:#{user.id}", 'exp' => 1.hour.from_now.to_i, 'iat' => Time.current.to_i, + 'nbf' => Time.current.to_i, 'jti' => 'test-uuid-12345', 'gitlab' => { 'user_id' => user.id, @@ -47,6 +50,13 @@ expect(jti1).not_to eq(jti2) end + + it 'uses RS256 algorithm' do + decoded = JWT.decode(token, rsa_key.public_key, true, algorithm: 'RS256') + header = decoded.last + + expect(header['alg']).to eq('RS256') + end end context 'when signing key is not configured' do @@ -67,9 +77,314 @@ stub_application_setting(sdrs_jwt_signing_key: 'invalid-key') end - it 'raises an OpenSSL error' do + it 'logs and raises an OpenSSL error' do + expect(Gitlab::ErrorTracking).to receive(:log_and_raise_exception) + .with(instance_of(OpenSSL::PKey::RSAError)) + expect { token }.to raise_error(OpenSSL::PKey::RSAError) end end end + + describe '.verify_callback_token' do + let(:jti) { 'unique-jti-12345' } + let(:valid_payload) do + { + iss: 'gitlab-secret-detection-callback', + aud: 'sdrs-callback', + sub: 'sdrs-service', + exp: 1.hour.from_now.to_i, + iat: Time.current.to_i, + nbf: Time.current.to_i, + jti: jti, + gitlab: { + finding_id: finding_id + } + } + end + + subject(:verify) { described_class.verify_callback_token(token) } + + before do + stub_application_setting(sdrs_jwt_public_key: sdrs_rsa_key.public_key.to_pem) + end + + context 'with valid token' do + let(:token) { JWT.encode(valid_payload, sdrs_rsa_key, 'RS256') } + + it 'returns the decoded payload' do + expect(verify).to include( + 'iss' => 'gitlab-secret-detection-callback', + 'aud' => 'sdrs-callback', + 'gitlab' => { 'finding_id' => finding_id } + ) + end + + it 'registers JTI to prevent replay attacks' do + expect(Gitlab::Redis::SharedState).to receive(:with).and_call_original + + verify + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.exists("sdrs:callback:jti:#{jti}")).to be true + expect(redis.ttl("sdrs:callback:jti:#{jti}")).to be > 0 + expect(redis.ttl("sdrs:callback:jti:#{jti}")).to be <= 2.hours.to_i + end + end + + it 'rejects token if JTI was already used' do + # First use succeeds + expect(verify).not_to be_nil + + # Second use with same JTI fails + expect(described_class.verify_callback_token(token)).to be_nil + end + + context 'with clock skew' do + let(:token) do + payload = valid_payload.merge( + iat: 25.seconds.ago.to_i, + nbf: 25.seconds.ago.to_i + ) + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it 'accepts token within leeway' do + expect(verify).not_to be_nil + end + end + end + + context 'with nil token' do + let(:token) { nil } + + it 'returns nil' do + expect(verify).to be_nil + end + end + + context 'with empty token' do + let(:token) { '' } + + it 'returns nil' do + expect(verify).to be_nil + end + end + + context 'when public key is not configured' do + before do + stub_application_setting(sdrs_jwt_public_key: nil) + end + + let(:token) { JWT.encode(valid_payload, sdrs_rsa_key, 'RS256') } + + it 'raises PublicKeyNotConfigured error' do + expect { verify }.to raise_error( + described_class::PublicKeyNotConfigured, + 'SDRS public key not configured' + ) + end + end + + context 'with invalid signature' do + let(:wrong_key) { OpenSSL::PKey::RSA.generate(2048) } + let(:token) { JWT.encode(valid_payload, wrong_key, 'RS256') } + + it 'returns nil and logs error' do + expect(Gitlab::AppLogger).to receive(:warn).with( + hash_including( + message: 'SDRS JWT verification failed', + error_class: 'JWT::VerificationError' + ) + ) + + expect(verify).to be_nil + end + end + + context 'with wrong algorithm' do + let(:token) { JWT.encode(valid_payload, 'secret', 'HS256') } + + it 'returns nil' do + expect(verify).to be_nil + end + end + + context 'with expired token' do + let(:token) do + payload = valid_payload.merge(exp: 1.hour.ago.to_i) + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it 'returns nil and logs error' do + expect(Gitlab::AppLogger).to receive(:warn).with( + hash_including( + message: 'SDRS JWT verification failed', + error_class: 'JWT::ExpiredSignature' + ) + ) + + expect(verify).to be_nil + end + end + + context 'with invalid issuer' do + let(:token) do + payload = valid_payload.merge(iss: 'wrong-issuer') + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it 'returns nil and logs error' do + expect(Gitlab::AppLogger).to receive(:warn).with( + hash_including( + message: 'SDRS JWT verification failed', + error_class: 'JWT::InvalidIssuerError' + ) + ) + + expect(verify).to be_nil + end + end + + context 'with invalid audience' do + let(:token) do + payload = valid_payload.merge(aud: 'wrong-audience') + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it 'returns nil and logs error' do + expect(Gitlab::AppLogger).to receive(:warn).with( + hash_including( + message: 'SDRS JWT verification failed', + error_class: 'JWT::InvalidAudError' + ) + ) + + expect(verify).to be_nil + end + end + + context 'with missing gitlab claims' do + let(:token) do + payload = valid_payload.except(:gitlab) + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it 'returns nil' do + expect(verify).to be_nil + end + end + + context 'with invalid gitlab claims' do + shared_examples 'invalid gitlab claims' do + it 'returns nil' do + expect(verify).to be_nil + end + end + + context 'when gitlab claims is not a hash' do + let(:token) do + payload = valid_payload.merge(gitlab: 'not-a-hash') + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it_behaves_like 'invalid gitlab claims' + end + + context 'when finding_id is missing' do + let(:token) do + payload = valid_payload + payload[:gitlab].delete(:finding_id) + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it_behaves_like 'invalid gitlab claims' + end + + context 'when finding_id is not an integer' do + let(:token) do + payload = valid_payload + payload[:gitlab][:finding_id] = 'not-an-integer' + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it_behaves_like 'invalid gitlab claims' + end + + context 'when finding_id is zero' do + let(:token) do + payload = valid_payload + payload[:gitlab][:finding_id] = 0 + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it_behaves_like 'invalid gitlab claims' + end + + context 'when finding_id is negative' do + let(:token) do + payload = valid_payload + payload[:gitlab][:finding_id] = -1 + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it_behaves_like 'invalid gitlab claims' + end + end + + context 'with missing JTI' do + let(:token) do + payload = valid_payload.except(:jti) + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it 'returns nil' do + expect(verify).to be_nil + end + end + + context 'with empty JTI' do + let(:token) do + payload = valid_payload.merge(jti: '') + JWT.encode(payload, sdrs_rsa_key, 'RS256') + end + + it 'returns nil' do + expect(verify).to be_nil + end + end + + context 'when Redis is unavailable' do + let(:token) { JWT.encode(valid_payload, sdrs_rsa_key, 'RS256') } + + before do + allow(Gitlab::Redis::SharedState).to receive(:with).and_raise(Redis::CannotConnectError) + end + + it 'tracks exception and returns nil' do + expect(Gitlab::ErrorTracking).to receive(:track_exception) + .with( + instance_of(Redis::CannotConnectError), + jwt_context: { service: 'Authz::SdrsAuthenticationService' } + ) + + expect(verify).to be_nil + end + end + + context 'with malformed token' do + let(:token) { 'not.a.jwt' } + + it 'returns nil and logs error' do + expect(Gitlab::AppLogger).to receive(:warn).with( + hash_including( + message: 'SDRS JWT verification failed', + error_class: 'JWT::DecodeError' + ) + ) + + expect(verify).to be_nil + end + end + end end -- GitLab From 26d89253adf847bc0e7e788ed3fbfefc786e4a26 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Tue, 29 Jul 2025 20:42:28 +0530 Subject: [PATCH 08/13] tmp --- .../helpers/ee/application_settings_helper.rb | 1 + .../vulnerabilities/finding/token_status.rb | 128 +------ ee/lib/ee/api/api.rb | 1 + ee/lib/ee/gitlab/application_rate_limiter.rb | 1 + .../finding/token_status_spec.rb | 355 ++++++++++++++++++ 5 files changed, 378 insertions(+), 108 deletions(-) create mode 100644 ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb diff --git a/ee/app/helpers/ee/application_settings_helper.rb b/ee/app/helpers/ee/application_settings_helper.rb index 45491f41042ba6..20def1e7dd3964 100644 --- a/ee/app/helpers/ee/application_settings_helper.rb +++ b/ee/app/helpers/ee/application_settings_helper.rb @@ -163,6 +163,7 @@ def self.possible_licensed_attributes secret_detection_service_auth_token secret_detection_service_url virtual_registries_endpoints_api_limit + sdrs_callback_per_jti_api disable_invite_members ] end diff --git a/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb index 960a087d95946c..d864f2a9733667 100644 --- a/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb +++ b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb @@ -1,4 +1,3 @@ -# ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb # frozen_string_literal: true module API @@ -7,8 +6,6 @@ module AppSec module Vulnerabilities module Finding class TokenStatus < ::API::Base - include APIGuard - before { authenticate_sdrs_callback! } helpers do @@ -19,94 +16,16 @@ def authenticate_sdrs_callback! def valid_sdrs_jwt? return false unless jwt_token.present? - begin - @jwt_payload = decode_and_verify_jwt - verify_required_claims - verify_jti_uniqueness - true - rescue JWT::DecodeError, JWT::ExpiredSignature, JWT::InvalidIssuerError, - JWT::InvalidAudError, JWT::InvalidIatError => e - log_jwt_validation_error(e) - false - end + @jwt_payload = ::Authz::SdrsAuthenticationService.verify_callback_token(jwt_token) + @jwt_payload.present? end def jwt_token @jwt_token ||= request.headers['Authorization']&.split(' ')&.last end - def decode_and_verify_jwt - JWT.decode( - jwt_token, - sdrs_public_key, - true, - { - algorithm: 'RS256', - verify_iss: true, - iss: 'secret-detection-response-service', - verify_aud: true, - aud: 'gitlab-secret-detection-callback', - verify_iat: true, - verify_expiration: true, - leeway: 30 # 30 seconds leeway for clock skew - } - ).first - end - - def verify_required_claims - required_claims = %w[iss aud exp iat jti gitlab] - missing_claims = required_claims - @jwt_payload.keys - - raise JWT::DecodeError, "Missing required claims: #{missing_claims.join(', ')}" if missing_claims.any? - - # Verify GitLab-specific claims - gitlab_claims = @jwt_payload['gitlab'] - raise JWT::DecodeError, 'Missing gitlab claims' unless gitlab_claims.is_a?(Hash) - - return if gitlab_claims['finding_id'].present? - - raise JWT::DecodeError, - 'Missing finding_id in gitlab claims' - end - - def verify_jti_uniqueness - jti = @jwt_payload['jti'] - - # Use Redis to track JTIs for the duration of token validity - redis_key = "sdrs:callback:jti:#{jti}" - - Gitlab::Redis::SharedState.with do |redis| - # Set with NX (only if not exists) and expiration - # Returns false if key already exists - raise JWT::DecodeError, 'JTI already used' unless redis.set(redis_key, '1', nx: true, ex: 3600) - end - end - - def sdrs_public_key - # In production, this would fetch from JWKS endpoint with caching - # For now, using the configured public key - @sdrs_public_key ||= begin - key_pem = ::Gitlab::CurrentSettings.sdrs_jwt_public_key - raise JWT::DecodeError, 'SDRS public key not configured' unless key_pem.present? - - OpenSSL::PKey::RSA.new(key_pem) - end - end - def finding - @finding ||= ::Vulnerabilities::Finding.find(@jwt_payload.dig('gitlab', 'finding_id')) - rescue ActiveRecord::RecordNotFound - nil - end - - def log_jwt_validation_error(error) - Gitlab::AppLogger.error( - message: 'SDRS callback JWT validation failed', - error_class: error.class.name, - error_message: error.message, - jti: @jwt_payload&.dig('jti'), - finding_id: @jwt_payload&.dig('gitlab', 'finding_id') - ) + @finding ||= ::Vulnerabilities::Finding.find_by_id(@jwt_payload&.dig('gitlab', 'finding_id')) end def log_callback_attempt(status:, finding_id:, error: nil) @@ -114,25 +33,12 @@ def log_callback_attempt(status:, finding_id:, error: nil) message: 'SDRS callback received', status: status, finding_id: finding_id, - jti: @jwt_payload['jti'], + jti: @jwt_payload&.dig('jti'), request_id: request.headers['X-Request-ID'], error: error ) - # Also create an audit event for security tracking return unless finding&.project - - ::AuditEventService.new( - nil, - nil, - { - action: :custom, - custom_message: "SDRS token verification callback: #{status}", - target_id: finding_id, - target_type: 'Vulnerabilities::Finding', - target_details: "Token verification status: #{status}" - } - ).for_project(finding.project).security_event end end @@ -144,7 +50,7 @@ def log_callback_attempt(status:, finding_id:, error: nil) params do requires :finding_id, type: Integer, desc: 'The vulnerability finding ID' requires :status, type: String, values: %w[active revoked error unknown inactive], - desc: 'Token verification status' + desc: 'Token verification status' requires :checked_at, type: DateTime, desc: 'When the token was checked' optional :metadata, type: Hash, desc: 'Additional metadata about the token status' do optional :scopes, type: Array[String], desc: 'Token scopes (if applicable)' @@ -155,23 +61,29 @@ def log_callback_attempt(status:, finding_id:, error: nil) end end post 'token_status/callback', urgency: :low, feature_category: :secret_detection do - # Apply rate limiting - check_rate_limit!(:sdrs_callback, scope: request.ip, threshold: 100, interval: 1.minute) - + unauthorized!('Invalid JWT') unless @jwt_payload # Verify finding_id matches JWT claim forbidden!('Finding ID mismatch') if params[:finding_id] != @jwt_payload.dig('gitlab', 'finding_id') + # Rate limiting + if Gitlab::ApplicationRateLimiter.throttled?(:sdrs_callback_per_jti_api, + scope: [@jwt_payload['jti']], + threshold: 10, + interval: 1.minute) + too_many_requests!('Rate limit exceeded') + end + # Find the vulnerability finding not_found!('Finding not found') unless finding # Update token status result = ::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService - .new(finding: finding, current_user: nil) - .execute( - status: params[:status], - metadata: params[:metadata], - checked_at: params[:checked_at] - ) + .new(finding: finding, current_user: nil) + .execute( + status: params[:status], + metadata: params[:metadata], + checked_at: params[:checked_at] + ) if result.success? log_callback_attempt(status: 'success', finding_id: params[:finding_id]) diff --git a/ee/lib/ee/api/api.rb b/ee/lib/ee/api/api.rb index 3d9993000cf4ba..ad795c7082d0a1 100644 --- a/ee/lib/ee/api/api.rb +++ b/ee/lib/ee/api/api.rb @@ -89,6 +89,7 @@ module API mount ::API::VirtualRegistries::Packages::Maven::Endpoints mount ::API::Internal::AppSec::Dast::SiteValidations + mount ::API::Internal::AppSec::Vulnerabilities::Finding::TokenStatus mount ::API::Internal::Search::Zoekt mount ::API::Internal::SuggestedReviewers mount ::API::Internal::Ai::XRay::Scan diff --git a/ee/lib/ee/gitlab/application_rate_limiter.rb b/ee/lib/ee/gitlab/application_rate_limiter.rb index a6d17ae5d56aac..073650f4b6350e 100644 --- a/ee/lib/ee/gitlab/application_rate_limiter.rb +++ b/ee/lib/ee/gitlab/application_rate_limiter.rb @@ -35,6 +35,7 @@ def rate_limits interval: 1.day }, container_scanning_for_registry_scans: { threshold: 50, interval: 1.day }, + sdrs_callback_per_jti_api: { threshold: 5, interval: 1.minute }, virtual_registries_endpoints_api_limit: { threshold: -> { application_settings.virtual_registries_endpoints_api_limit }, interval: 15.seconds } diff --git a/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb b/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb new file mode 100644 index 00000000000000..ba2a38464e0756 --- /dev/null +++ b/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb @@ -0,0 +1,355 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::Internal::AppSec::Vulnerabilities::Finding::TokenStatus, feature_category: :secret_detection do + include ApiHelpers + + let(:finding) { create(:vulnerabilities_finding) } + let(:jwt_payload) do + { + 'jti' => 'test-jti-123', + 'gitlab' => { 'finding_id' => finding.id } + } + end + let(:valid_jwt_token) { 'valid_jwt_token' } + + let(:api_url) { '/internal/app_sec/vulnerabilities/finding/token_status/callback' } + let(:params) do + { + finding_id: finding.id, + status: 'active', + checked_at: Time.current.iso8601 + } + end + let(:headers) { { 'Authorization' => "Bearer #{valid_jwt_token}" } } + + describe 'POST /internal/app_sec/vulnerabilities/finding/token_status/callback' do + subject(:post_api) do + post api(api_url), params: params, headers: headers + end + + context 'authentication' do + context 'when JWT is missing' do + let(:headers) { {} } + + it 'returns unauthorized' do + post_api + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'when JWT is invalid' do + before do + allow(::Authz::SdrsAuthenticationService).to receive(:verify_callback_token) + .with(valid_jwt_token) + .and_return(nil) + end + + it 'returns unauthorized' do + post_api + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'when JWT is valid' do + before do + allow(::Authz::SdrsAuthenticationService).to receive(:verify_callback_token) + .with(valid_jwt_token) + .and_return(jwt_payload) + end + + it 'authenticates successfully' do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + + post_api + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + + context 'when authenticated' do + before do + allow(::Authz::SdrsAuthenticationService).to receive(:verify_callback_token) + .with(valid_jwt_token) + .and_return(jwt_payload) + end + + context 'authorization' do + context 'when finding_id does not match JWT claim' do + let(:params) { super().merge(finding_id: finding.id + 1) } + + it 'returns forbidden' do + post_api + + expect(response).to have_gitlab_http_status(:forbidden) + expect(json_response['message']).to eq('403 Forbidden - Finding ID mismatch') + end + end + + context 'when finding does not exist' do + let(:jwt_payload) do + super().tap { |p| p['gitlab']['finding_id'] = non_existing_record_id } + end + let(:params) { super().merge(finding_id: non_existing_record_id) } + + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + end + + it 'returns not found' do + post_api + + expect(response).to have_gitlab_http_status(:not_found) + expect(json_response['message']).to eq('404 Finding not found Not Found') + end + end + end + + context 'rate limiting' do + context 'when rate limit is exceeded' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:sdrs_callback_per_jti_api, scope: ['test-jti-123'], threshold: 10, interval: 1.minute) + .and_return(true) + end + + it 'returns too many requests' do + post_api + + expect(response).to have_gitlab_http_status(:too_many_requests) + expect(json_response['message']).to eq('429 Too Many Requests - Rate limit exceeded') + end + end + + context 'when rate limit is not exceeded' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:sdrs_callback_per_jti_api, scope: ['test-jti-123'], threshold: 10, interval: 1.minute) + .and_return(false) + end + + it 'processes the request' do + allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + + post_api + + expect(response).to have_gitlab_http_status(:ok) + end + end + end + + context 'service execution' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + end + + context 'when service returns success' do + let(:service_response) { ServiceResponse.success } + + before do + allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + allow(service).to receive(:execute).and_return(service_response) + end + end + + it 'returns success' do + post_api + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq({ 'success' => true }) + end + + it 'logs successful callback' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + message: 'SDRS callback received', + status: 'success', + finding_id: finding.id, + jti: 'test-jti-123' + ) + ) + + post_api + end + + it 'passes correct parameters to service' do + expect_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService, + finding: finding, + current_user: nil + ) do |service| + expect(service).to receive(:execute).with( + status: 'active', + metadata: nil, + checked_at: params[:checked_at] + ).and_return(service_response) + end + + post_api + end + end + + context 'when service returns error' do + let(:service_response) { ServiceResponse.error(message: 'Validation failed') } + + before do + allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + allow(service).to receive(:execute).and_return(service_response) + end + end + + it 'returns bad request' do + post_api + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eq('Validation failed') + end + + it 'logs error' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + message: 'SDRS callback received', + status: 'error', + finding_id: finding.id, + error: 'Validation failed' + ) + ) + + post_api + end + end + end + + context 'with optional metadata' do + let(:params) do + super().merge( + metadata: { + scopes: ['repo', 'user'], + expires_at: 1.year.from_now.iso8601, + account: 'test-account' + } + ) + end + + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + end + + it 'passes metadata to service' do + expect_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + expect(service).to receive(:execute).with( + hash_including( + metadata: hash_including( + 'scopes' => ['repo', 'user'], + 'account' => 'test-account' + ) + ) + ).and_return(ServiceResponse.success) + end + + post_api + end + end + + context 'with different status values' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + end + + %w[active revoked error unknown inactive].each do |status_value| + it "accepts #{status_value} status" do + post api(api_url), params: params.merge(status: status_value), headers: headers + + expect(response).to have_gitlab_http_status(:ok) + end + end + + it 'rejects invalid status' do + post api(api_url), params: params.merge(status: 'invalid'), headers: headers + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + + context 'error handling' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + end + + context 'when an exception occurs' do + let(:error) { StandardError.new('Something went wrong') } + + before do + allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + allow(service).to receive(:execute).and_raise(error) + end + end + + it 'returns internal server error' do + post_api + + expect(response).to have_gitlab_http_status(:internal_server_error) + expect(json_response['message']).to eq('500 Internal Server Error - Internal error processing callback') + end + + it 'logs the error' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + message: 'SDRS callback received', + status: 'error', + finding_id: finding.id, + error: 'Something went wrong' + ) + ) + + post_api + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + error, + finding_id: finding.id, + jwt_payload: jwt_payload + ) + + post_api + end + end + end + + context 'request headers' do + let(:headers) do + super().merge('X-Request-ID' => 'test-request-123') + end + + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + end + + it 'includes request ID in logs' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + request_id: 'test-request-123' + ) + ) + + post_api + end + end + end + end +end -- GitLab From 9c236898d407f48c1ab1e157288c5a73a13c47b8 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 30 Jul 2025 00:10:52 +0530 Subject: [PATCH 09/13] Working tests --- .../token_verification_callback_service.rb | 124 +++++ .../vulnerabilities/finding/token_status.rb | 101 ++-- .../finding/token_status_spec.rb | 444 ++++++++---------- ...oken_verification_callback_service_spec.rb | 245 ++++++++++ lib/api/helpers.rb | 4 + 5 files changed, 586 insertions(+), 332 deletions(-) create mode 100644 ee/app/services/security/secret_detection/token_verification_callback_service.rb create mode 100644 ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb diff --git a/ee/app/services/security/secret_detection/token_verification_callback_service.rb b/ee/app/services/security/secret_detection/token_verification_callback_service.rb new file mode 100644 index 00000000000000..9cce1accd30c8b --- /dev/null +++ b/ee/app/services/security/secret_detection/token_verification_callback_service.rb @@ -0,0 +1,124 @@ +# frozen_string_literal: true + +module Security + module SecretDetection + class TokenVerificationCallbackService < ::BaseService + RateLimitExceededError = Class.new(StandardError) + FindingMismatchError = Class.new(StandardError) + InvalidJWTError = Class.new(StandardError) + + attr_reader :jwt_payload, :params, :request_headers + + def initialize(jwt_payload:, params:, request_headers: {}) + @jwt_payload = jwt_payload + @params = params + @request_headers = request_headers + end + + def execute + validate_request! + check_rate_limit! + + finding = find_and_validate_finding + return error('Finding not found', :not_found) unless finding + + update_token_status(finding) + rescue RateLimitExceededError => e + error(e.message, :rate_limited) + rescue FindingMismatchError => e + error(e.message, :forbidden) + rescue InvalidJWTError => e + error(e.message, :unauthorized) + rescue ActiveRecord::RecordInvalid => e + log_callback_attempt(status: 'error', finding_id: params[:finding_id], error: e.message) + error(e.record.errors.full_messages.to_sentence, :bad_request) + rescue StandardError => e + handle_unexpected_error(e) + end + + private + + def validate_request! + raise InvalidJWTError, 'Invalid JWT' unless jwt_payload.present? + + jwt_finding_id = jwt_payload.dig('gitlab', 'finding_id') + return unless params[:finding_id] != jwt_finding_id + + raise FindingMismatchError, 'Finding ID mismatch' + end + + def check_rate_limit! + jti = jwt_payload['jti'] + + if Gitlab::ApplicationRateLimiter.throttled?( + :sdrs_callback_per_jti_api, + scope: [jti], + threshold: 10, + interval: 1.minute + ) + raise RateLimitExceededError, 'Rate limit exceeded' + end + end + + def find_and_validate_finding + finding_id = jwt_payload.dig('gitlab', 'finding_id') + finding = ::Vulnerabilities::Finding.find_by_id(finding_id) + + return unless finding + return unless finding.finding_token_status.present? + + finding + end + + def update_token_status(finding) + token_status = finding.finding_token_status + + token_status.update!( + status: params[:status] + ) + + log_callback_attempt( + status: 'success', + finding_id: finding.id + ) + + success(finding: finding) + end + + def handle_unexpected_error(exception) + log_callback_attempt( + status: 'error', + finding_id: params[:finding_id], + error: exception.message + ) + + Gitlab::ErrorTracking.track_exception( + exception, + finding_id: params[:finding_id], + jwt_payload: jwt_payload + ) + + error('Internal error processing callback', :internal_server_error) + end + + def log_callback_attempt(status:, finding_id:, error: nil) + Gitlab::AppLogger.info( + message: 'SDRS callback received', + status: status, + finding_id: finding_id, + jti: jwt_payload&.dig('jti'), + request_id: request_headers['X-Request-ID'], + error: error + ) + end + + def error(message, reason) + ServiceResponse.error(message: message, reason: reason) + end + + def success(payload = {}) + ServiceResponse.success(payload: payload) + end + end + end +end diff --git a/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb index d864f2a9733667..3711c982a29cbd 100644 --- a/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb +++ b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb @@ -10,35 +10,24 @@ class TokenStatus < ::API::Base helpers do def authenticate_sdrs_callback! - unauthorized! unless valid_sdrs_jwt? - end - - def valid_sdrs_jwt? - return false unless jwt_token.present? + unauthorized!('Missing authorization header') unless jwt_token.present? @jwt_payload = ::Authz::SdrsAuthenticationService.verify_callback_token(jwt_token) - @jwt_payload.present? + unauthorized!('Invalid JWT') unless @jwt_payload.present? end def jwt_token @jwt_token ||= request.headers['Authorization']&.split(' ')&.last end - def finding - @finding ||= ::Vulnerabilities::Finding.find_by_id(@jwt_payload&.dig('gitlab', 'finding_id')) - end - - def log_callback_attempt(status:, finding_id:, error: nil) - Gitlab::AppLogger.info( - message: 'SDRS callback received', - status: status, - finding_id: finding_id, - jti: @jwt_payload&.dig('jti'), - request_id: request.headers['X-Request-ID'], - error: error + def callback_service + ::Security::SecretDetection::TokenVerificationCallbackService.new( + jwt_payload: @jwt_payload, + params: declared_params, + request_headers: { + 'X-Request-Id' => request.headers['X-Request-Id'] + } ) - - return unless finding&.project end end @@ -49,68 +38,32 @@ def log_callback_attempt(status:, finding_id:, error: nil) end params do requires :finding_id, type: Integer, desc: 'The vulnerability finding ID' - requires :status, type: String, values: %w[active revoked error unknown inactive], - desc: 'Token verification status' - requires :checked_at, type: DateTime, desc: 'When the token was checked' - optional :metadata, type: Hash, desc: 'Additional metadata about the token status' do - optional :scopes, type: Array[String], desc: 'Token scopes (if applicable)' - optional :expires_at, type: DateTime, desc: 'Token expiration time' - optional :revoked_at, type: DateTime, desc: 'When token was revoked' - optional :error_message, type: String, desc: 'Error details if status is error' - optional :account, type: String, desc: 'Account information' - end + requires :status, type: String, + values: %w[active unknown inactive], + desc: 'Token verification status' end post 'token_status/callback', urgency: :low, feature_category: :secret_detection do - unauthorized!('Invalid JWT') unless @jwt_payload - # Verify finding_id matches JWT claim - forbidden!('Finding ID mismatch') if params[:finding_id] != @jwt_payload.dig('gitlab', 'finding_id') - - # Rate limiting - if Gitlab::ApplicationRateLimiter.throttled?(:sdrs_callback_per_jti_api, - scope: [@jwt_payload['jti']], - threshold: 10, - interval: 1.minute) - too_many_requests!('Rate limit exceeded') - end - - # Find the vulnerability finding - not_found!('Finding not found') unless finding - - # Update token status - result = ::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService - .new(finding: finding, current_user: nil) - .execute( - status: params[:status], - metadata: params[:metadata], - checked_at: params[:checked_at] - ) + result = callback_service.execute if result.success? - log_callback_attempt(status: 'success', finding_id: params[:finding_id]) status 200 { success: true } else - log_callback_attempt( - status: 'error', - finding_id: params[:finding_id], - error: result.message - ) - bad_request!(result.message) + case result.reason + when :not_found + not_found!(result.message) + when :forbidden + forbidden!(result.message) + when :unauthorized + unauthorized!(result.message) + when :bad_request + bad_request!(result.message) + when :rate_limited + too_many_requests!(result.message) + else + internal_server_error!(result.message) + end end - rescue StandardError => e - log_callback_attempt( - status: 'error', - finding_id: params[:finding_id], - error: e.message - ) - - Gitlab::ErrorTracking.track_exception( - e, - finding_id: params[:finding_id], - jwt_payload: @jwt_payload - ) - - internal_server_error!('Internal error processing callback') end end end diff --git a/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb b/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb index ba2a38464e0756..e0bcbbae0268f0 100644 --- a/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb +++ b/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb @@ -4,351 +4,279 @@ RSpec.describe API::Internal::AppSec::Vulnerabilities::Finding::TokenStatus, feature_category: :secret_detection do include ApiHelpers - - let(:finding) { create(:vulnerabilities_finding) } + + let_it_be(:project) { create(:project) } + let_it_be(:finding) { create(:vulnerabilities_finding, project: project, report_type: 'secret_detection') } + let_it_be(:finding_token_status) { create(:finding_token_status, finding: finding, status: 'unknown') } + + let(:api_url) { '/internal/app_sec/vulnerabilities/finding/token_status/callback' } + let(:jti) { SecureRandom.uuid } + let(:jwt_payload) do { - 'jti' => 'test-jti-123', - 'gitlab' => { 'finding_id' => finding.id } + 'jti' => jti, + 'iat' => Time.current.to_i, + 'exp' => 5.minutes.from_now.to_i, + 'gitlab' => { + 'finding_id' => finding.id, + 'project_id' => project.id + } } end - let(:valid_jwt_token) { 'valid_jwt_token' } - - let(:api_url) { '/internal/app_sec/vulnerabilities/finding/token_status/callback' } + + let(:valid_jwt_token) { 'valid.jwt.token' } + let(:params) do { finding_id: finding.id, - status: 'active', - checked_at: Time.current.iso8601 + status: 'active' + } + end + + let(:headers) do + { + 'Authorization' => "Bearer #{valid_jwt_token}", + 'X-Request-Id' => 'test-request-123' } end - let(:headers) { { 'Authorization' => "Bearer #{valid_jwt_token}" } } describe 'POST /internal/app_sec/vulnerabilities/finding/token_status/callback' do - subject(:post_api) do - post api(api_url), params: params, headers: headers - end + context 'when authentication is successful' do + before do + allow(::Authz::SdrsAuthenticationService) + .to receive(:verify_callback_token) + .with(valid_jwt_token) + .and_return(jwt_payload) + end - context 'authentication' do - context 'when JWT is missing' do - let(:headers) { {} } + context 'when callback service returns success' do + it 'returns 200 OK' do + post api(api_url), params: params, headers: headers - it 'returns unauthorized' do - post_api + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq({ 'success' => true }) + end - expect(response).to have_gitlab_http_status(:unauthorized) + it 'updates the token status' do + expect { post api(api_url), params: params, headers: headers } + .to change { finding_token_status.reload.status } + .from('unknown') + .to('active') end end - context 'when JWT is invalid' do - before do - allow(::Authz::SdrsAuthenticationService).to receive(:verify_callback_token) - .with(valid_jwt_token) - .and_return(nil) + context 'when callback service returns not_found' do + let(:non_existent_finding_id) { non_existing_record_id } + let(:params) { super().merge(finding_id: non_existent_finding_id) } + let(:jwt_payload) do + super().tap { |p| p['gitlab']['finding_id'] = non_existent_finding_id } end - it 'returns unauthorized' do - post_api + it 'returns 404 Not Found' do + post api(api_url), params: params, headers: headers - expect(response).to have_gitlab_http_status(:unauthorized) + expect(response).to have_gitlab_http_status(:not_found) + expect(json_response['message']).to eq('404 Finding not found Not Found') end end - context 'when JWT is valid' do - before do - allow(::Authz::SdrsAuthenticationService).to receive(:verify_callback_token) - .with(valid_jwt_token) - .and_return(jwt_payload) - end + context 'when callback service returns forbidden' do + let(:params) { super().merge(finding_id: finding.id + 1) } - it 'authenticates successfully' do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - allow(service).to receive(:execute).and_return(ServiceResponse.success) - end - - post_api + it 'returns 403 Forbidden' do + post api(api_url), params: params, headers: headers - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:forbidden) + expect(json_response['message']).to eq('403 Forbidden - Finding ID mismatch') end end - end - - context 'when authenticated' do - before do - allow(::Authz::SdrsAuthenticationService).to receive(:verify_callback_token) - .with(valid_jwt_token) - .and_return(jwt_payload) - end - - context 'authorization' do - context 'when finding_id does not match JWT claim' do - let(:params) { super().merge(finding_id: finding.id + 1) } - it 'returns forbidden' do - post_api + context 'when callback service returns bad_request' do + let(:params) { super().merge(status: 'active') } - expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq('403 Forbidden - Finding ID mismatch') + before do + allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| + allow(instance).to receive(:update!) + .and_raise(ActiveRecord::RecordInvalid.new(finding_token_status)) end + allow(finding_token_status).to receive_message_chain(:errors, :full_messages, :to_sentence) + .and_return('Status is invalid') end - context 'when finding does not exist' do - let(:jwt_payload) do - super().tap { |p| p['gitlab']['finding_id'] = non_existing_record_id } - end - let(:params) { super().merge(finding_id: non_existing_record_id) } - - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - end + it 'returns 400 Bad Request' do + post api(api_url), params: params, headers: headers - it 'returns not found' do - post_api - - expect(response).to have_gitlab_http_status(:not_found) - expect(json_response['message']).to eq('404 Finding not found Not Found') - end + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eq('400 Bad request - Status is invalid') end end - context 'rate limiting' do - context 'when rate limit is exceeded' do - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) - .with(:sdrs_callback_per_jti_api, scope: ['test-jti-123'], threshold: 10, interval: 1.minute) - .and_return(true) - end + context 'when status has invalid value' do + let(:params) { super().merge(status: 'invalid_status') } - it 'returns too many requests' do - post_api + it 'returns 400 Bad Request with Grape error' do + post api(api_url), params: params, headers: headers - expect(response).to have_gitlab_http_status(:too_many_requests) - expect(json_response['message']).to eq('429 Too Many Requests - Rate limit exceeded') - end + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('status does not have a valid value') end + end - context 'when rate limit is not exceeded' do - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) - .with(:sdrs_callback_per_jti_api, scope: ['test-jti-123'], threshold: 10, interval: 1.minute) - .and_return(false) - end - - it 'processes the request' do - allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - allow(service).to receive(:execute).and_return(ServiceResponse.success) - end + context 'when callback service returns rate_limited' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:sdrs_callback_per_jti_api, scope: [jti], threshold: 10, interval: 1.minute) + .and_return(true) + end - post_api + it 'returns 429 Too Many Requests' do + post api(api_url), params: params, headers: headers - expect(response).to have_gitlab_http_status(:ok) - end + expect(response).to have_gitlab_http_status(:too_many_requests) + expect(json_response['message']).to eq('Rate limit exceeded') end end - context 'service execution' do + context 'when callback service returns internal_server_error' do before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| + allow(instance).to receive(:update!) + .and_raise(StandardError.new('Database error')) + end end - context 'when service returns success' do - let(:service_response) { ServiceResponse.success } + it 'returns 503 Internal Server Error' do + post api(api_url), params: params, headers: headers - before do - allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - allow(service).to receive(:execute).and_return(service_response) - end - end - - it 'returns success' do - post_api + expect(response).to have_gitlab_http_status(:internal_server_error) + expect(json_response['message']).to eq('Internal error processing callback') + end + end + end - expect(response).to have_gitlab_http_status(:ok) - expect(json_response).to eq({ 'success' => true }) - end + context 'when authentication fails' do + context 'when authorization header is missing' do + let(:headers) { super().except('Authorization') } - it 'logs successful callback' do - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - message: 'SDRS callback received', - status: 'success', - finding_id: finding.id, - jti: 'test-jti-123' - ) - ) - - post_api - end + it 'returns 401 Unauthorized' do + post api(api_url), params: params, headers: headers - it 'passes correct parameters to service' do - expect_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService, - finding: finding, - current_user: nil - ) do |service| - expect(service).to receive(:execute).with( - status: 'active', - metadata: nil, - checked_at: params[:checked_at] - ).and_return(service_response) - end - - post_api - end + expect(response).to have_gitlab_http_status(:unauthorized) + expect(json_response['message']).to eq('401 Unauthorized - Missing authorization header') end + end - context 'when service returns error' do - let(:service_response) { ServiceResponse.error(message: 'Validation failed') } - - before do - allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - allow(service).to receive(:execute).and_return(service_response) - end - end + context 'when JWT is invalid' do + before do + allow(::Authz::SdrsAuthenticationService) + .to receive(:verify_callback_token) + .with('invalid.token') + .and_return(nil) + end - it 'returns bad request' do - post_api + let(:headers) { super().merge('Authorization' => 'Bearer invalid.token') } - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to eq('Validation failed') - end + it 'returns 401 Unauthorized' do + post api(api_url), params: params, headers: headers - it 'logs error' do - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - message: 'SDRS callback received', - status: 'error', - finding_id: finding.id, - error: 'Validation failed' - ) - ) - - post_api - end + expect(response).to have_gitlab_http_status(:unauthorized) + expect(json_response['message']).to eq('401 Unauthorized - Invalid JWT') end end - context 'with optional metadata' do - let(:params) do - super().merge( - metadata: { - scopes: ['repo', 'user'], - expires_at: 1.year.from_now.iso8601, - account: 'test-account' - } - ) - end + context 'when authorization header has invalid format' do + let(:headers) { super().merge('Authorization' => 'InvalidFormat token') } before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + allow(::Authz::SdrsAuthenticationService) + .to receive(:verify_callback_token) + .with('token') + .and_return(nil) end - it 'passes metadata to service' do - expect_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - expect(service).to receive(:execute).with( - hash_including( - metadata: hash_including( - 'scopes' => ['repo', 'user'], - 'account' => 'test-account' - ) - ) - ).and_return(ServiceResponse.success) - end + it 'returns 401 Unauthorized' do + post api(api_url), params: params, headers: headers - post_api + expect(response).to have_gitlab_http_status(:unauthorized) + expect(json_response['message']).to eq('401 Unauthorized - Invalid JWT') end end + end - context 'with different status values' do - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - allow(service).to receive(:execute).and_return(ServiceResponse.success) - end - end - - %w[active revoked error unknown inactive].each do |status_value| - it "accepts #{status_value} status" do - post api(api_url), params: params.merge(status: status_value), headers: headers + context 'when request parameters are invalid' do + before do + allow(::Authz::SdrsAuthenticationService) + .to receive(:verify_callback_token) + .with(valid_jwt_token) + .and_return(jwt_payload) + end - expect(response).to have_gitlab_http_status(:ok) - end - end + context 'when finding_id is missing' do + let(:params) { { status: 'active' } } - it 'rejects invalid status' do - post api(api_url), params: params.merge(status: 'invalid'), headers: headers + it 'returns 400 Bad Request' do + post api(api_url), params: params, headers: headers expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to include('finding_id is missing') end end - context 'error handling' do - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - end - - context 'when an exception occurs' do - let(:error) { StandardError.new('Something went wrong') } - - before do - allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - allow(service).to receive(:execute).and_raise(error) - end - end + context 'when status is missing' do + let(:params) { { finding_id: finding.id } } - it 'returns internal server error' do - post_api + it 'returns 400 Bad Request' do + post api(api_url), params: params, headers: headers - expect(response).to have_gitlab_http_status(:internal_server_error) - expect(json_response['message']).to eq('500 Internal Server Error - Internal error processing callback') - end + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to include('status is missing') + end + end - it 'logs the error' do - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - message: 'SDRS callback received', - status: 'error', - finding_id: finding.id, - error: 'Something went wrong' - ) - ) - - post_api - end + context 'when status has invalid value' do + let(:params) { super().merge(status: 'revoked') } - it 'tracks the exception' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).with( - error, - finding_id: finding.id, - jwt_payload: jwt_payload - ) + it 'returns 400 Bad Request' do + post api(api_url), params: params, headers: headers - post_api - end + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to include('status does not have a valid value') end end + end - context 'request headers' do - let(:headers) do - super().merge('X-Request-ID' => 'test-request-123') - end + context 'when service instantiation is verified' do + let(:service_double) { instance_double(::Security::SecretDetection::TokenVerificationCallbackService) } - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - allow_next_instance_of(::Vulnerabilities::FindingTokenStatuses::CreateOrUpdateService) do |service| - allow(service).to receive(:execute).and_return(ServiceResponse.success) - end - end + before do + allow(::Authz::SdrsAuthenticationService) + .to receive(:verify_callback_token) + .with(valid_jwt_token) + .and_return(jwt_payload) - it 'includes request ID in logs' do - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - request_id: 'test-request-123' - ) + allow(::Security::SecretDetection::TokenVerificationCallbackService) + .to receive(:new) + .with( + jwt_payload: jwt_payload, + params: params, + request_headers: { 'X-Request-Id' => 'test-request-123' } ) + .and_return(service_double) - post_api - end + allow(service_double).to receive(:execute) + .and_return(ServiceResponse.success) + end + + it 'passes correct parameters to the service' do + expect(::Security::SecretDetection::TokenVerificationCallbackService) + .to receive(:new) + .with( + jwt_payload: jwt_payload, + params: params, + request_headers: { 'X-Request-Id' => 'test-request-123' } + ) + + post api(api_url), params: params, headers: headers end end end diff --git a/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb b/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb new file mode 100644 index 00000000000000..e9bc77939b8777 --- /dev/null +++ b/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb @@ -0,0 +1,245 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Security::SecretDetection::TokenVerificationCallbackService, feature_category: :secret_detection do + let_it_be(:project) { create(:project) } + let_it_be(:finding) { create(:vulnerabilities_finding, project: project, report_type: 'secret_detection') } + + let(:finding_token_status) { create(:finding_token_status, finding: finding, status: 'unknown') } + let(:jti) { SecureRandom.uuid } + + let(:jwt_payload) do + { + 'jti' => jti, + 'iat' => Time.current.to_i, + 'exp' => 5.minutes.from_now.to_i, + 'gitlab' => { + 'finding_id' => finding.id, + 'project_id' => project.id + } + } + end + + let(:params) do + { + finding_id: finding.id, + status: 'active', + checked_at: Time.current.iso8601, + metadata: { 'scopes' => ['repo'] } + } + end + + let(:request_headers) { { 'X-Request-ID' => 'test-request-123' } } + + let(:service) do + described_class.new( + jwt_payload: jwt_payload, + params: params, + request_headers: request_headers + ) + end + + before do + finding_token_status # Ensure it's created before the test + end + + describe '#execute' do + subject(:execute_service) { service.execute } + + context 'when request is valid' do + before do + # Only stub the rate limiter, which is an external concern + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + end + + it 'updates the token status' do + expect { execute_service } + .to change { finding_token_status.reload.status } + .from('unknown') + .to('active') + end + + it 'returns success' do + result = execute_service + + expect(result).to be_success + expect(result.payload[:finding]).to eq(finding) + end + + it 'logs successful callback' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + message: 'SDRS callback received', + status: 'success', + finding_id: finding.id, + jti: jti, + request_id: 'test-request-123' + ) + ) + + execute_service + end + end + + context 'when JWT is invalid' do + let(:jwt_payload) { nil } + + it 'returns error with unauthorized reason' do + result = execute_service + + expect(result).to be_error + expect(result.message).to eq('Invalid JWT') + expect(result.reason).to eq(:unauthorized) + end + end + + context 'when finding_id does not match JWT' do + let(:params) { super().merge(finding_id: finding.id + 1) } + + it 'returns error with forbidden reason' do + result = execute_service + + expect(result).to be_error + expect(result.message).to eq('Finding ID mismatch') + expect(result.reason).to eq(:forbidden) + end + end + + context 'when rate limit is exceeded' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:sdrs_callback_per_jti_api, scope: [jti], threshold: 10, interval: 1.minute) + .and_return(true) + end + + it 'returns error with rate_limited reason' do + result = execute_service + + expect(result).to be_error + expect(result.message).to eq('Rate limit exceeded') + expect(result.reason).to eq(:rate_limited) + end + end + + context 'when finding does not exist' do + let(:jwt_payload) do + super().tap { |p| p['gitlab']['finding_id'] = non_existing_record_id } + end + + let(:params) { super().merge(finding_id: non_existing_record_id) } + + it 'returns error with not_found reason' do + result = execute_service + + expect(result).to be_error + expect(result.message).to eq('Finding not found') + expect(result.reason).to eq(:not_found) + end + end + + context 'when finding has no token status' do + let_it_be(:finding_without_status) do + create(:vulnerabilities_finding, project: project, report_type: 'secret_detection') + end + + let(:jwt_payload) do + super().tap { |p| p['gitlab']['finding_id'] = finding_without_status.id } + end + + let(:params) { super().merge(finding_id: finding_without_status.id) } + + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + end + + it 'returns error with not_found reason' do + result = execute_service + + expect(result).to be_error + expect(result.message).to eq('Finding not found') + expect(result.reason).to eq(:not_found) + end + end + + context 'when validation fails' do + let(:params) { super().merge(status: 'invalid_status') } + + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + # Assuming the model has validation for status + allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| + allow(instance).to receive(:update!).and_raise( + ActiveRecord::RecordInvalid.new(finding_token_status) + ) + end + allow(finding_token_status).to receive_message_chain(:errors, :full_messages, :to_sentence) + .and_return('Status is invalid') + end + + it 'returns error with bad_request reason' do + result = execute_service + + expect(result).to be_error + expect(result.message).to eq('Status is invalid') + expect(result.reason).to eq(:bad_request) + end + + it 'logs error' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + message: 'SDRS callback received', + status: 'error', + error: String + ) + ) + + execute_service + end + end + + context 'when unexpected error occurs' do + let(:error) { StandardError.new('Database error') } + + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) + allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| + allow(instance).to receive(:update!).and_raise(error) + end + end + + it 'returns error with internal_server_error reason' do + result = execute_service + + expect(result).to be_error + expect(result.message).to eq('Internal error processing callback') + expect(result.reason).to eq(:internal_server_error) + end + + it 'tracks the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + error, + hash_including( + finding_id: finding.id, + jwt_payload: jwt_payload + ) + ) + + execute_service + end + + it 'logs error' do + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including( + message: 'SDRS callback received', + status: 'error', + finding_id: finding.id, + error: 'Database error' + ) + ) + + execute_service + end + end + end +end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 7bf064d48e2b2f..7f7fee9db65ed8 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -575,6 +575,10 @@ def service_unavailable!(message = nil) render_api_error!(message || '503 Service Unavailable', 503) end + def internal_server_error!(message = nil) + render_api_error!(message || '500 Internal Server Error', 500) + end + def conflict!(message = nil) render_api_error!(message || '409 Conflict', 409) end -- GitLab From 24a8e2423d845d36cdd7eae46a8dfdc353433dcd Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 30 Jul 2025 18:39:22 +0530 Subject: [PATCH 10/13] Fixes rspec --- ...oken_verification_callback_service_spec.rb | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb b/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb index e9bc77939b8777..feaaa525ddf128 100644 --- a/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb +++ b/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb @@ -5,8 +5,8 @@ RSpec.describe Security::SecretDetection::TokenVerificationCallbackService, feature_category: :secret_detection do let_it_be(:project) { create(:project) } let_it_be(:finding) { create(:vulnerabilities_finding, project: project, report_type: 'secret_detection') } + let_it_be(:finding_token_status) { create(:finding_token_status, finding: finding, status: 'unknown') } - let(:finding_token_status) { create(:finding_token_status, finding: finding, status: 'unknown') } let(:jti) { SecureRandom.uuid } let(:jwt_payload) do @@ -15,8 +15,7 @@ 'iat' => Time.current.to_i, 'exp' => 5.minutes.from_now.to_i, 'gitlab' => { - 'finding_id' => finding.id, - 'project_id' => project.id + 'finding_id' => finding.id } } end @@ -24,9 +23,7 @@ let(:params) do { finding_id: finding.id, - status: 'active', - checked_at: Time.current.iso8601, - metadata: { 'scopes' => ['repo'] } + status: 'active' } end @@ -40,10 +37,6 @@ ) end - before do - finding_token_status # Ensure it's created before the test - end - describe '#execute' do subject(:execute_service) { service.execute } @@ -164,17 +157,25 @@ context 'when validation fails' do let(:params) { super().merge(status: 'invalid_status') } + let(:mocked_token_status) do + build_stubbed(:finding_token_status, finding: finding, status: 'unknown').tap do |fts| + fts.errors.add(:base, 'Status is invalid') + end + end before do allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - # Assuming the model has validation for status - allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| - allow(instance).to receive(:update!).and_raise( - ActiveRecord::RecordInvalid.new(finding_token_status) - ) - end - allow(finding_token_status).to receive_message_chain(:errors, :full_messages, :to_sentence) - .and_return('Status is invalid') + + # Mock the class method `find_by_id` to return our test `finding` object. + # This is the key change to make the test work. + allow(::Vulnerabilities::Finding).to receive(:find_by_id).with(finding.id).and_return(finding) + + # Now that the service will use our test `finding` object, we can mock its + # `finding_token_status` method to return our stubbed object. + allow(finding).to receive(:finding_token_status).and_return(mocked_token_status) + + # Finally, mock `update!` on our stubbed object to raise the exception. + allow(mocked_token_status).to receive(:update!).and_raise(ActiveRecord::RecordInvalid, mocked_token_status) end it 'returns error with bad_request reason' do @@ -203,9 +204,15 @@ before do allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| - allow(instance).to receive(:update!).and_raise(error) - end + + # Mock the class method to return the finding object from our test context + allow(::Vulnerabilities::Finding).to receive(:find_by_id).with(finding.id).and_return(finding) + + # Mock the instance method to return the finding_token_status from our test context + allow(finding).to receive(:finding_token_status).and_return(finding_token_status) + + # Mock the update! method to raise the unexpected error + allow(finding_token_status).to receive(:update!).and_raise(error) end it 'returns error with internal_server_error reason' do -- GitLab From 5d4c9723836a240e27132d98c1f49b26e3869071 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 30 Jul 2025 18:51:46 +0530 Subject: [PATCH 11/13] Fixes spec --- .../token_verification_callback_service.rb | 3 +- ...oken_verification_callback_service_spec.rb | 61 ++++--------------- 2 files changed, 12 insertions(+), 52 deletions(-) diff --git a/ee/app/services/security/secret_detection/token_verification_callback_service.rb b/ee/app/services/security/secret_detection/token_verification_callback_service.rb index 9cce1accd30c8b..47de5ceaac333a 100644 --- a/ee/app/services/security/secret_detection/token_verification_callback_service.rb +++ b/ee/app/services/security/secret_detection/token_verification_callback_service.rb @@ -64,8 +64,7 @@ def find_and_validate_finding finding_id = jwt_payload.dig('gitlab', 'finding_id') finding = ::Vulnerabilities::Finding.find_by_id(finding_id) - return unless finding - return unless finding.finding_token_status.present? + return unless finding && finding.finding_token_status finding end diff --git a/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb b/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb index feaaa525ddf128..088bdffa7e24cc 100644 --- a/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb +++ b/ee/spec/services/security/secret_detection/token_verification_callback_service_spec.rb @@ -41,11 +41,6 @@ subject(:execute_service) { service.execute } context 'when request is valid' do - before do - # Only stub the rate limiter, which is an external concern - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - end - it 'updates the token status' do expect { execute_service } .to change { finding_token_status.reload.status } @@ -142,10 +137,6 @@ let(:params) { super().merge(finding_id: finding_without_status.id) } - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - end - it 'returns error with not_found reason' do result = execute_service @@ -156,63 +147,33 @@ end context 'when validation fails' do - let(:params) { super().merge(status: 'invalid_status') } - let(:mocked_token_status) do - build_stubbed(:finding_token_status, finding: finding, status: 'unknown').tap do |fts| - fts.errors.add(:base, 'Status is invalid') - end - end - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - - # Mock the class method `find_by_id` to return our test `finding` object. - # This is the key change to make the test work. - allow(::Vulnerabilities::Finding).to receive(:find_by_id).with(finding.id).and_return(finding) - - # Now that the service will use our test `finding` object, we can mock its - # `finding_token_status` method to return our stubbed object. - allow(finding).to receive(:finding_token_status).and_return(mocked_token_status) + allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| + allow(instance).to receive(:update!) + .and_raise(ActiveRecord::RecordInvalid.new(finding_token_status)) + end - # Finally, mock `update!` on our stubbed object to raise the exception. - allow(mocked_token_status).to receive(:update!).and_raise(ActiveRecord::RecordInvalid, mocked_token_status) + finding_token_status.errors.add(:status, 'is not included in the list') end it 'returns error with bad_request reason' do result = execute_service expect(result).to be_error - expect(result.message).to eq('Status is invalid') + expect(result.message).to eq('Status is not included in the list') expect(result.reason).to eq(:bad_request) end - - it 'logs error' do - expect(Gitlab::AppLogger).to receive(:info).with( - hash_including( - message: 'SDRS callback received', - status: 'error', - error: String - ) - ) - - execute_service - end end context 'when unexpected error occurs' do let(:error) { StandardError.new('Database error') } before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(false) - - # Mock the class method to return the finding object from our test context - allow(::Vulnerabilities::Finding).to receive(:find_by_id).with(finding.id).and_return(finding) - - # Mock the instance method to return the finding_token_status from our test context - allow(finding).to receive(:finding_token_status).and_return(finding_token_status) - - # Mock the update! method to raise the unexpected error - allow(finding_token_status).to receive(:update!).and_raise(error) + # Force an error by making the status update fail + allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| + allow(instance).to receive(:update!) + .and_raise(error) + end end it 'returns error with internal_server_error reason' do -- GitLab From e3cb0c4d09a4035c8242d3deeb725874b95e5850 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 30 Jul 2025 22:58:05 +0530 Subject: [PATCH 12/13] Fixes spec --- .../vulnerabilities/finding/token_status.rb | 2 +- .../finding/token_status_spec.rb | 324 +++++------------- 2 files changed, 90 insertions(+), 236 deletions(-) diff --git a/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb index 3711c982a29cbd..cbf08398655a48 100644 --- a/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb +++ b/ee/lib/api/internal/app_sec/vulnerabilities/finding/token_status.rb @@ -31,7 +31,7 @@ def callback_service end end - namespace 'internal/app_sec/vulnerabilities/finding' do + namespace 'internal/vulnerabilities/finding' do desc 'Receive token status callback from SDRS' do detail 'Updates the token verification status for a vulnerability finding' tags %w[internal] diff --git a/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb b/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb index e0bcbbae0268f0..9776d8734a6a20 100644 --- a/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb +++ b/ee/spec/requests/api/internal/app_sec/vulnerabilities/finding/token_status_spec.rb @@ -3,281 +3,135 @@ require 'spec_helper' RSpec.describe API::Internal::AppSec::Vulnerabilities::Finding::TokenStatus, feature_category: :secret_detection do - include ApiHelpers - - let_it_be(:project) { create(:project) } - let_it_be(:finding) { create(:vulnerabilities_finding, project: project, report_type: 'secret_detection') } - let_it_be(:finding_token_status) { create(:finding_token_status, finding: finding, status: 'unknown') } - - let(:api_url) { '/internal/app_sec/vulnerabilities/finding/token_status/callback' } - let(:jti) { SecureRandom.uuid } - - let(:jwt_payload) do - { - 'jti' => jti, - 'iat' => Time.current.to_i, - 'exp' => 5.minutes.from_now.to_i, - 'gitlab' => { - 'finding_id' => finding.id, - 'project_id' => project.id - } - } - end - - let(:valid_jwt_token) { 'valid.jwt.token' } - - let(:params) do - { - finding_id: finding.id, - status: 'active' - } - end - - let(:headers) do - { - 'Authorization' => "Bearer #{valid_jwt_token}", - 'X-Request-Id' => 'test-request-123' - } - end - - describe 'POST /internal/app_sec/vulnerabilities/finding/token_status/callback' do - context 'when authentication is successful' do - before do - allow(::Authz::SdrsAuthenticationService) - .to receive(:verify_callback_token) - .with(valid_jwt_token) - .and_return(jwt_payload) - end - - context 'when callback service returns success' do - it 'returns 200 OK' do - post api(api_url), params: params, headers: headers - - expect(response).to have_gitlab_http_status(:ok) - expect(json_response).to eq({ 'success' => true }) - end - - it 'updates the token status' do - expect { post api(api_url), params: params, headers: headers } - .to change { finding_token_status.reload.status } - .from('unknown') - .to('active') - end - end - - context 'when callback service returns not_found' do - let(:non_existent_finding_id) { non_existing_record_id } - let(:params) { super().merge(finding_id: non_existent_finding_id) } - let(:jwt_payload) do - super().tap { |p| p['gitlab']['finding_id'] = non_existent_finding_id } - end + describe 'POST /internal/vulnerabilities/finding/token_status/callback' do + let(:finding) { create(:vulnerabilities_finding) } + let(:valid_params) { { finding_id: finding.id, status: 'active' } } + let(:jwt_payload) { { finding_id: finding.id, iss: 'sdrs' } } + let(:valid_jwt) { 'valid.jwt.token' } + let(:headers) { { 'Authorization' => "Bearer #{valid_jwt}" } } + + before do + allow(::Authz::SdrsAuthenticationService).to receive( + :verify_callback_token).with(valid_jwt).and_return(jwt_payload) + end - it 'returns 404 Not Found' do - post api(api_url), params: params, headers: headers + context 'when authenticating' do + it 'returns 401 when authorization header is missing' do + post api( + '/internal/vulnerabilities/finding/token_status/callback'), params: valid_params - expect(response).to have_gitlab_http_status(:not_found) - expect(json_response['message']).to eq('404 Finding not found Not Found') - end + expect(response).to have_gitlab_http_status(:unauthorized) + expect(json_response['message']).to eq('401 Unauthorized - Missing authorization header') end - context 'when callback service returns forbidden' do - let(:params) { super().merge(finding_id: finding.id + 1) } + it 'returns 401 when JWT is invalid' do + allow(::Authz::SdrsAuthenticationService).to receive( + :verify_callback_token).with('invalid.token').and_return(nil) - it 'returns 403 Forbidden' do - post api(api_url), params: params, headers: headers + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: valid_params, + headers: { 'Authorization' => 'Bearer invalid.token' } - expect(response).to have_gitlab_http_status(:forbidden) - expect(json_response['message']).to eq('403 Forbidden - Finding ID mismatch') - end + expect(response).to have_gitlab_http_status(:unauthorized) + expect(json_response['message']).to eq('401 Unauthorized - Invalid JWT') end + end - context 'when callback service returns bad_request' do - let(:params) { super().merge(status: 'active') } - - before do - allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| - allow(instance).to receive(:update!) - .and_raise(ActiveRecord::RecordInvalid.new(finding_token_status)) - end - allow(finding_token_status).to receive_message_chain(:errors, :full_messages, :to_sentence) - .and_return('Status is invalid') - end - - it 'returns 400 Bad Request' do - post api(api_url), params: params, headers: headers + context 'when validating parameters' do + it 'returns 400 when finding_id is missing' do + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: { status: 'active' }, + headers: headers - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to eq('400 Bad request - Status is invalid') - end + expect(response).to have_gitlab_http_status(:bad_request) end - context 'when status has invalid value' do - let(:params) { super().merge(status: 'invalid_status') } + it 'returns 400 when status is missing' do + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: { finding_id: finding.id }, + headers: headers - it 'returns 400 Bad Request with Grape error' do - post api(api_url), params: params, headers: headers - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to eq('status does not have a valid value') - end + expect(response).to have_gitlab_http_status(:bad_request) end - context 'when callback service returns rate_limited' do - before do - allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?) - .with(:sdrs_callback_per_jti_api, scope: [jti], threshold: 10, interval: 1.minute) - .and_return(true) - end - - it 'returns 429 Too Many Requests' do - post api(api_url), params: params, headers: headers + it 'returns 400 when status is invalid' do + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: { finding_id: finding.id, status: 'invalid_status' }, + headers: headers - expect(response).to have_gitlab_http_status(:too_many_requests) - expect(json_response['message']).to eq('Rate limit exceeded') - end - end - - context 'when callback service returns internal_server_error' do - before do - allow_next_instance_of(Vulnerabilities::FindingTokenStatus) do |instance| - allow(instance).to receive(:update!) - .and_raise(StandardError.new('Database error')) - end - end - - it 'returns 503 Internal Server Error' do - post api(api_url), params: params, headers: headers - - expect(response).to have_gitlab_http_status(:internal_server_error) - expect(json_response['message']).to eq('Internal error processing callback') - end + expect(response).to have_gitlab_http_status(:bad_request) end end - context 'when authentication fails' do - context 'when authorization header is missing' do - let(:headers) { super().except('Authorization') } - - it 'returns 401 Unauthorized' do - post api(api_url), params: params, headers: headers + context 'when callback is successful' do + let(:service_result) { ServiceResponse.success } - expect(response).to have_gitlab_http_status(:unauthorized) - expect(json_response['message']).to eq('401 Unauthorized - Missing authorization header') + before do + allow_next_instance_of(::Security::SecretDetection::TokenVerificationCallbackService) do |service| + allow(service).to receive(:execute).and_return(service_result) end end - context 'when JWT is invalid' do - before do - allow(::Authz::SdrsAuthenticationService) - .to receive(:verify_callback_token) - .with('invalid.token') - .and_return(nil) - end - - let(:headers) { super().merge('Authorization' => 'Bearer invalid.token') } - - it 'returns 401 Unauthorized' do - post api(api_url), params: params, headers: headers + it 'returns 200 with success response' do + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: valid_params, + headers: headers - expect(response).to have_gitlab_http_status(:unauthorized) - expect(json_response['message']).to eq('401 Unauthorized - Invalid JWT') - end + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq('success' => true) end - context 'when authorization header has invalid format' do - let(:headers) { super().merge('Authorization' => 'InvalidFormat token') } + it 'passes request headers to service' do + expect(::Security::SecretDetection::TokenVerificationCallbackService).to receive(:new).with( + jwt_payload: jwt_payload, + params: valid_params, + request_headers: { 'X-Request-Id' => 'test-request-id' } + ).and_call_original - before do - allow(::Authz::SdrsAuthenticationService) - .to receive(:verify_callback_token) - .with('token') - .and_return(nil) - end + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: valid_params, + headers: headers.merge('X-Request-Id' => 'test-request-id') + end - it 'returns 401 Unauthorized' do - post api(api_url), params: params, headers: headers + %w[active unknown inactive].each do |status_value| + it "accepts '#{status_value}' as valid status" do + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: { finding_id: finding.id, status: status_value }, + headers: headers - expect(response).to have_gitlab_http_status(:unauthorized) - expect(json_response['message']).to eq('401 Unauthorized - Invalid JWT') + expect(response).to have_gitlab_http_status(:ok) end end end - context 'when request parameters are invalid' do - before do - allow(::Authz::SdrsAuthenticationService) - .to receive(:verify_callback_token) - .with(valid_jwt_token) - .and_return(jwt_payload) - end - - context 'when finding_id is missing' do - let(:params) { { status: 'active' } } + context 'when service returns error' do + using RSpec::Parameterized::TableSyntax - it 'returns 400 Bad Request' do - post api(api_url), params: params, headers: headers - - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to include('finding_id is missing') - end + where(:reason, :http_status, :message) do + :not_found | 404 | 'Not Found' + :forbidden | 403 | 'Access denied' + :unauthorized | 401 | 'Unauthorized access' + :bad_request | 400 | 'Invalid request' + :rate_limited | 429 | 'Rate limit exceeded' + :unknown_error | 500 | 'Internal error' end - context 'when status is missing' do - let(:params) { { finding_id: finding.id } } - - it 'returns 400 Bad Request' do - post api(api_url), params: params, headers: headers + with_them do + it 'returns appropriate error response' do + service_result = ServiceResponse.error(message: message, reason: reason) - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to include('status is missing') - end - end - - context 'when status has invalid value' do - let(:params) { super().merge(status: 'revoked') } + allow_next_instance_of(::Security::SecretDetection::TokenVerificationCallbackService) do |service| + allow(service).to receive(:execute).and_return(service_result) + end - it 'returns 400 Bad Request' do - post api(api_url), params: params, headers: headers + post api('/internal/vulnerabilities/finding/token_status/callback'), + params: valid_params, + headers: headers - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['error']).to include('status does not have a valid value') + expect(response).to have_gitlab_http_status(http_status) + expect(json_response['message']).to include(message) end end end - - context 'when service instantiation is verified' do - let(:service_double) { instance_double(::Security::SecretDetection::TokenVerificationCallbackService) } - - before do - allow(::Authz::SdrsAuthenticationService) - .to receive(:verify_callback_token) - .with(valid_jwt_token) - .and_return(jwt_payload) - - allow(::Security::SecretDetection::TokenVerificationCallbackService) - .to receive(:new) - .with( - jwt_payload: jwt_payload, - params: params, - request_headers: { 'X-Request-Id' => 'test-request-123' } - ) - .and_return(service_double) - - allow(service_double).to receive(:execute) - .and_return(ServiceResponse.success) - end - - it 'passes correct parameters to the service' do - expect(::Security::SecretDetection::TokenVerificationCallbackService) - .to receive(:new) - .with( - jwt_payload: jwt_payload, - params: params, - request_headers: { 'X-Request-Id' => 'test-request-123' } - ) - - post api(api_url), params: params, headers: headers - end - end end end -- GitLab From a110e282535e9dd7db34ad0d7f2c1a69e4a90fe7 Mon Sep 17 00:00:00 2001 From: atiwari71 Date: Wed, 30 Jul 2025 23:44:32 +0530 Subject: [PATCH 13/13] Fixes json --- .../application_setting_sdrs_jwt_key_configuration.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json b/ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json index da9647cd5162e5..66f8bace2dccbd 100644 --- a/ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json +++ b/ee/app/validators/json_schemas/application_setting_sdrs_jwt_key_configuration.json @@ -3,4 +3,4 @@ "title": "SDRS JWT Key Configuration", "type": "string", "description": "PEM-encoded RSA key (private or public) for JWT operations" -} \ No newline at end of file +} -- GitLab