Merge pull request #2339 from newrelic/plub

don't log the license key even in debug/audit

lib/new_relic/agent/configuration/default_source.rb

@@ -336,6 +336,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
           :public => true,
           :type => String,
           :allowed_from_server => false,
+          :exclude_from_reported_settings => true,
           :description => 'Your New Relic <InlinePopover type="licenseKey" />.'
         },
         :log_level => {

lib/new_relic/agent/configuration/manager.rb

@@ -230,7 +230,7 @@ def apply_mask(hash)
         end
 
         def to_collector_hash
-          DottedHash.new(apply_mask(flattened)).to_hash.delete_if do |k, v|
+          DottedHash.new(apply_mask(flattened)).to_hash.delete_if do |k, _v|
             default = DEFAULTS[k]
             if default
               default[:exclude_from_reported_settings]
@@ -376,12 +376,13 @@ def new_cache
         end
 
         def log_config(direction, source)
-          # Just generating this log message (specifically calling
-          # flattened.inspect) is expensive enough that we don't want to do it
-          # unless we're actually going to be logging the message based on our
-          # current log level.
+          # Just generating this log message (specifically calling `flattened`)
+          # is expensive enough that we don't want to do it unless we're
+          # actually going to be logging the message based on our current log
+          # level, so use a `do` block.
           ::NewRelic::Agent.logger.debug do
-            "Updating config (#{direction}) from #{source.class}. Results: #{flattened.inspect}"
+            hash = flattened.delete_if { |k, _h| DEFAULTS.fetch(k, {}).fetch(:exclude_from_reported_settings, false) }
+            "Updating config (#{direction}) from #{source.class}. Results: #{hash.inspect}"
           end
         end
 

lib/new_relic/agent/new_relic_service.rb

@@ -414,7 +414,7 @@ def prep_request(opts)
         else
           request = Net::HTTP::Post.new(opts[:uri], headers)
         end
-        @audit_logger.log_request_headers(opts[:uri], headers)
+        @audit_logger.log_request_headers(filtered_uri(opts[:uri]), headers)
         request['user-agent'] = user_agent
         request.content_type = 'application/octet-stream'
         request.body = opts[:data]
@@ -431,7 +431,7 @@ def relay_request(request, opts)
         rescue *CONNECTION_ERRORS => e
           close_shared_connection
           if attempts < MAX_ATTEMPTS
-            ::NewRelic::Agent.logger.debug("Retrying request to #{opts[:collector]}#{opts[:uri]} after #{e}")
+            ::NewRelic::Agent.logger.debug("Retrying request to #{opts[:collector]}#{filtered_uri(opts[:uri])} after #{e}")
             retry
           else
             raise ServerConnectionException, "Recoverable error talking to #{@collector} after #{attempts} attempts: #{e}"
@@ -444,7 +444,7 @@ def relay_request(request, opts)
 
       def attempt_request(request, opts)
         conn = http_connection
-        ::NewRelic::Agent.logger.debug("Sending request to #{opts[:collector]}#{opts[:uri]} with #{request.method}")
+        ::NewRelic::Agent.logger.debug("Sending request to #{opts[:collector]}#{filtered_uri(opts[:uri])} with #{request.method}")
         conn.request(request)
       end
 
@@ -689,9 +689,7 @@ def prep_collector(method)
 
       def invoke_remote_send_request(method, payload, data, encoding)
         uri = remote_method_uri(method)
-        full_uri = "#{@collector}#{uri}"
-
-        @audit_logger.log_request(full_uri, payload, @marshaller)
+        @audit_logger.log_request("#{@collector}#{filtered_uri(uri)}", payload, @marshaller)
         request_send_ts = Process.clock_gettime(Process::CLOCK_MONOTONIC)
         response = send_request(:data => data,
           :uri       => uri,
@@ -700,6 +698,10 @@ def invoke_remote_send_request(method, payload, data, encoding)
           :endpoint  => method)
         [response, request_send_ts, Process.clock_gettime(Process::CLOCK_MONOTONIC)]
       end
+
+      def filtered_uri(uri)
+        uri.gsub(license_key, ASTERISK * license_key.size)
+      end
     end
   end
 end

lib/new_relic/constants.rb

@@ -3,6 +3,8 @@
 # frozen_string_literal: true
 
 module NewRelic
+  ASTERISK = '*'
+
   PRIORITY_PRECISION = 6
 
   EMPTY_ARRAY = [].freeze

test/new_relic/agent/configuration/manager_test.rb

@@ -530,6 +530,13 @@ def test_unsatisfied_values_stay_cached
       end
     end
 
+    def test_logger_does_not_receive_excluded_settings
+      log = with_array_logger(:debug) { @manager.log_config('direction', 'source') }.array.join('')
+
+      assert_includes(log, ':app_name')
+      refute_includes(log, ':license_key')
+    end
+
     private
 
     def assert_parsed_labels(expected)

test/new_relic/agent/connect/request_builder_test.rb

@@ -90,4 +90,13 @@ def test_enviroment_metadata
   ensure
     ENV[key] = nil
   end
+
+  def test_excluded_config_settings_are_in_fact_excluded
+    excluded = NewRelic::Agent::Configuration::DEFAULTS.select { |k, h|
+      k if h.fetch(:exclude_from_reported_settings, false)
+    }
+
+    refute @request_builder.connect_payload[:settings].keys.detect { |k| excluded.include?(k) },
+      "Did not expect the request builder's connect payload to include any excluded config settings"
+  end
 end

test/new_relic/agent/new_relic_service_test.rb

@@ -275,11 +275,13 @@ def test_preconnect_never_uses_redirect_host
     initial_connect_log = with_array_logger(level = :debug) { @service.connect }
 
     assert_log_contains initial_connect_log, 'Sending request to localhost'
+    assert_log_contains initial_connect_log, Regexp.escape('license_key=***********')
 
     # If we need to reconnect, preconnect should use the locally configured collector again
     reconnect_log = with_array_logger(level = :debug) { @service.preconnect }
 
     assert_log_contains reconnect_log, 'Sending request to somewhere.example.com'
+    assert_log_contains reconnect_log, Regexp.escape('license_key=***********')
   end
 
   def test_preconnect_with_no_token_and_no_lasp
@@ -1029,6 +1031,13 @@ def test_headers_cleared_on_subsequent_connect
     refute_includes header_keys, 'x-nr-metadata'
   end
 
+  def test_filtered_uri
+    base = 'https://cdpr_cp2077.com?license_key='
+    filtered = @service.send(:filtered_uri, base + @service.send(:license_key))
+
+    assert_equal base + '***********', filtered
+  end
+
   def build_stats_hash(items = {})
     hash = NewRelic::Agent::StatsHash.new
     items.each do |key, value|