Filter out sensitive parameters on Auth logs

Merge branch 'security-489459-hide_private_token_logs_kibana' into 'master'

See merge request gitlab-org/security/gitlab!4636

Changelog: security

app/controllers/application_controller.rb

@@ -104,7 +104,7 @@ def self.endpoint_id_for_action(action_name)
       env: :blocklist,
       remote_ip: request.ip,
       request_method: request.request_method,
-      path: request.fullpath
+      path: request.filtered_path
     )
 
     render plain: e.message, status: :forbidden

app/controllers/concerns/invisible_captcha_on_signup.rb

@@ -45,7 +45,7 @@ def log_request(message)
       env: :invisible_captcha_signup_bot_detected,
       remote_ip: request.ip,
       request_method: request.request_method,
-      path: request.fullpath
+      path: request.filtered_path
     }
 
     Gitlab::AuthLogger.error(request_information)

config/application.rb

@@ -214,7 +214,7 @@ class Application < Rails::Application
     #       vulnerability:
     #       https://gitlab.com/gitlab-org/labkit/blob/master/mask/matchers.go
     config.filter_parameters += [
-      /token$/,
+      /token$/i,
       /password/,
       /secret/,
       /key$/,

lib/gitlab/application_rate_limiter.rb

@@ -11,6 +11,7 @@ module ApplicationRateLimiter
     LIMIT_USAGE_BUCKET = [0.25, 0.5, 0.75, 1].freeze
 
     class << self
+      include ::Gitlab::Utils::StrongMemoize
       # Application rate limits
       #
       # Threshold value can be either an Integer or a Proc
@@ -224,7 +225,7 @@ def log_request(request, type, current_user, logger = Gitlab::AuthLogger)
           env: type,
           remote_ip: request.ip,
           request_method: request.request_method,
-          path: request.fullpath
+          path: request_path(request)
         }
 
         if current_user
@@ -322,6 +323,31 @@ def scoped_user_in_allowlist?(scope, users_allowlist)
 
         scoped_user.username.downcase.in?(users_allowlist)
       end
+
+      def request_path(request)
+        # req is an ActionDispatch::Request
+        if request.respond_to?(:filtered_path)
+          request.filtered_path
+        else
+          # req is a Grape::Request < Rack::Request
+          other_filtered_path(request)
+        end
+      end
+
+      def other_filtered_path(request)
+        filtered_params = initialize_filtered_params.filter(request.GET)
+
+        if filtered_params.any?
+          "#{request.path}?#{filtered_params.to_query}"
+        else
+          request.fullpath
+        end
+      end
+
+      def initialize_filtered_params
+        ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters)
+      end
+      strong_memoize_attr :initialize_filtered_params
     end
   end
 end

lib/gitlab/auth.rb

@@ -176,7 +176,7 @@ def rate_limit!(rate_limiter, success:, login:, request:)
             env: :blocklist,
             remote_ip: request.ip,
             request_method: request.request_method,
-            path: request.fullpath,
+            path: request.filtered_path,
             login: login
           )
         end

lib/gitlab/middleware/go.rb

@@ -27,7 +27,7 @@ def call(env)
           env: :blocklist,
           remote_ip: request.ip,
           request_method: request.request_method,
-          path: request.fullpath
+          path: request.filtered_path
         )
         Rack::Response.new(e.message, 403).finish
       rescue Gitlab::Auth::MissingPersonalAccessTokenError

lib/gitlab/middleware/path_traversal_check.rb

@@ -34,7 +34,7 @@ def initialize(app)
       def call(env)
         return @app.call(env) unless Feature.enabled?(:check_path_traversal_middleware, Feature.current_request)
 
-        request = ::Rack::Request.new(env.dup)
+        request = ::ActionDispatch::Request.new(env.dup)
         log_params = {}
 
         return @app.call(env) unless path_traversal_attempt?(request, log_params)
@@ -56,7 +56,7 @@ def call(env)
 
       def path_traversal_attempt?(request, log_params)
         with_duration_metric do |metric_labels|
-          original_fullpath = request.fullpath
+          original_fullpath = request.filtered_path
           exclude_query_parameters(request)
 
           decoded_fullpath = CGI.unescape(request.fullpath)

spec/config/application_spec.rb

@@ -20,7 +20,10 @@ def request_for_url(input_url)
         where(:input_url, :output_query) do
           '/'                                      | {}
           '/?safe=1'                               | { 'safe' => '1' }
+          '/?token=secret'                         | { 'token' => filtered }
+          '/?TOKEN=secret'                         | { 'TOKEN' => filtered }
           '/?private_token=secret'                 | { 'private_token' => filtered }
+          '/?PRIVATE_TOKEN=secret'                 | { 'PRIVATE_TOKEN' => filtered }
           '/?mixed=1&private_token=secret'         | { 'mixed' => '1', 'private_token' => filtered }
           '/?note=secret&noteable=1&prefix_note=2' | { 'note' => filtered, 'noteable' => '1', 'prefix_note' => '2' }
           '/?note[note]=secret&target_type=1'      | { 'note' => filtered, 'target_type' => '1' }

spec/lib/gitlab/application_rate_limiter_spec.rb

@@ -2,7 +2,9 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_rate_limiting do
+RSpec.describe Gitlab::ApplicationRateLimiter, :clean_gitlab_redis_rate_limiting, feature_category: :system_access do
+  include StubRequests
+
   let_it_be(:user) { create(:user) }
   let_it_be(:project) { create(:project) }
 
@@ -532,28 +534,27 @@
   end
 
   describe '.log_request' do
-    let(:file_path) { 'master/README.md' }
-    let(:type) { :raw_blob_request_limit }
-    let(:fullpath) { "/#{project.full_path}/raw/#{file_path}" }
+    let(:token_prefix) { Gitlab::ApplicationSettingFetcher.current_application_settings.personal_access_token_prefix }
+    let(:token_string) { "#{token_prefix}PAT1234" }
+    let(:relative_url) { "/#{project.full_path}/raw/?private_token=#{token_string}" }
 
-    let(:request) do
-      double('request', ip: '127.0.0.1', request_method: 'GET', fullpath: fullpath)
-    end
+    let(:type) { :raw_blob_request_limit }
+    let(:request) { request_for_url(relative_url) }
 
     let(:base_attributes) do
       {
         message: 'Application_Rate_Limiter_Request',
         env: type,
-        remote_ip: '127.0.0.1',
+        remote_ip: request.ip,
         request_method: 'GET',
-        path: fullpath
+        path: request.filtered_path
       }
     end
 
     context 'without a current user' do
       let(:current_user) { nil }
 
-      it 'logs information to auth.log' do
+      it 'logs filtered information to auth.log' do
         expect(Gitlab::AuthLogger).to receive(:error).with(base_attributes).once
 
         subject.log_request(request, type, current_user)
@@ -570,7 +571,7 @@
         })
       end
 
-      it 'logs information to auth.log' do
+      it 'logs filtered information to auth.log' do
         expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
 
         subject.log_request(request, type, current_user)

spec/lib/gitlab/auth_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching, feature_category: :system_access do
+  include StubRequests
+
   let_it_be(:project) { create(:project) }
 
   let(:auth_failure) { { actor: nil, project: nil, type: nil, authentication_abilities: nil } }
@@ -296,7 +298,10 @@
         end
 
         context 'when failure goes over threshold' do
-          let(:request) { instance_double(ActionDispatch::Request, fullpath: '/some/project.git/info/refs', request_method: 'GET', ip: 'ip', path: '/some_path/example') }
+          let(:token_prefix) { Gitlab::ApplicationSettingFetcher.current_application_settings.personal_access_token_prefix }
+          let(:token_string) { "#{token_prefix}PAT1234" }
+          let(:relative_url) { "/some/project.git/info/refs?private_token=#{token_string}" }
+          let(:request) { request_for_url(relative_url) }
 
           before do
             expect_next_instance_of(Gitlab::Auth::IpRateLimiter) do |rate_limiter|
@@ -304,13 +309,14 @@
             end
           end
 
-          it 'logs a message' do
+          it 'logs a message with a filtered path' do
             expect(Gitlab::AuthLogger).to receive(:error).with(
-              message: include('IP has been temporarily banned from Git auth'),
+              message: "Rack_Attack: Git auth failures has exceeded the threshold. " \
+                "IP has been temporarily banned from Git auth.",
               env: :blocklist,
               remote_ip: request.ip,
               request_method: request.request_method,
-              path: request.fullpath,
+              path: request.filtered_path,
               login: user.username
             )
 

spec/lib/gitlab/middleware/go_spec.rb

@@ -188,9 +188,23 @@
                   end
 
                   context 'with a denylisted ip' do
-                    it 'returns forbidden' do
+                    let(:request) { ActionDispatch::Request.new(env) }
+                    let(:attributes) do
+                      {
+                        message: 'Rack_Attack',
+                        status: 403,
+                        env: :blocklist,
+                        remote_ip: env['REMOTE_ADDR'],
+                        request_method: request.request_method,
+                        path: request.filtered_path
+                      }
+                    end
+
+                    it 'returns forbidden', :aggregate_failures do
                       err = Gitlab::Auth::IpBlocked.new
                       expect(Gitlab::Auth).to receive(:find_for_git_client).and_raise(err)
+                      expect(Gitlab::AuthLogger).to receive(:error).with(attributes)
+
                       response = go
 
                       expect(response[0]).to eq(403)

spec/lib/gitlab/middleware/path_traversal_check_spec.rb

@@ -10,7 +10,7 @@
   let(:fake_app) { ->(_) { fake_response } }
 
   describe '#call' do
-    let(:fullpath) { ::Rack::Request.new(env).fullpath }
+    let(:fullpath) { ::ActionDispatch::Request.new(env).filtered_path }
     let(:decoded_fullpath) { CGI.unescape(fullpath) }
 
     let(:graphql_query) do

spec/support/helpers/stub_requests.rb

@@ -47,6 +47,13 @@ def stubbed_hostname(url, hostname: IP_ADDRESS_STUB)
     url.to_s
   end
 
+  def request_for_url(input_url)
+    env = Rack::MockRequest.env_for(input_url)
+    env['action_dispatch.parameter_filter'] = Gitlab::Application.config.filter_parameters
+
+    ActionDispatch::Request.new(env)
+  end
+
   private
 
   def parse_url(url)