security(remote_method): redact ingest key in trace level logs (#1948)

Author: bizob2828

lib/collector/remote-method.js

@@ -259,15 +259,19 @@ RemoteMethod.prototype._safeRequest = function _safeRequest(options) {
     level = 'info'
   }
 
-  const logBody = Buffer.isBuffer(options.body) ? 'Buffer ' + options.body.length : options.body
-  logger[level](
-    { body: logBody },
-    'Posting to %s://%s:%s%s',
-    protocol,
-    options.host,
-    options.port,
-    options.path
-  )
+  // check if trace is enabled or audit log(aka info in this case) before attemping
+  // to get the body length or parse the path with redacted key
+  if (logger.traceEnabled() || level === 'info') {
+    const logBody = Buffer.isBuffer(options.body) ? 'Buffer ' + options.body.length : options.body
+    logger[level](
+      { body: logBody },
+      'Posting to %s://%s:%s%s',
+      protocol,
+      options.host,
+      options.port,
+      this._path({ redactLicenseKey: true })
+    )
+  }
 
   this._request(options)
 }
@@ -336,13 +340,14 @@ RemoteMethod.prototype._userAgent = function _userAgent() {
 /**
  * Generate a URL the collector understands.
  *
+ * @param {boolean} redactLicenseKey flag to redact license key in path
  * @returns {string} The URL path to be POSTed to.
  */
-RemoteMethod.prototype._path = function _path() {
+RemoteMethod.prototype._path = function _path({ redactLicenseKey } = {}) {
   const query = {
     marshal_format: 'json',
     protocol_version: this._protocolVersion,
-    license_key: this._config.license_key,
+    license_key: redactLicenseKey ? 'REDACTED' : this._config.license_key,
     method: this.name
   }
 

test/unit/collector/remote-method.test.js

@@ -9,14 +9,14 @@ const tap = require('tap')
 const dns = require('dns')
 const events = require('events')
 const https = require('https')
-
+const sinon = require('sinon')
+const proxyquire = require('proxyquire')
+const RemoteMethod = require('../../../lib/collector/remote-method')
 const url = require('url')
 const Config = require('../../../lib/config')
-const RemoteMethod = require('../../../lib/collector/remote-method')
 const helper = require('../../lib/agent_helper')
 require('../../lib/metrics_helper')
 const NAMES = require('../../../lib/metrics/names')
-
 const BARE_AGENT = { config: {}, metrics: { measureBytes() {} } }
 
 function generate(method, runID, protocolVersion) {
@@ -369,7 +369,7 @@ tap.test('when posting to collector', (t) => {
       })
     })
 
-    t.end('should use the right URL', (t) => {
+    t.test('should use the right URL', (t) => {
       const sendMetrics = nockMetricDataUncompressed()
       method._post('[]', {}, (error) => {
         t.error(error)
@@ -378,12 +378,13 @@ tap.test('when posting to collector', (t) => {
       })
     })
 
-    t.end('should respect the put_for_data_send config', (t) => {
+    t.test('should respect the put_for_data_send config', (t) => {
       const putMetrics = nock(URL)
         .put(generate('metric_data', RUN_ID))
         .reply(200, { return_value: [] })
 
       config.put_for_data_send = true
+
       method._post('[]', {}, (error) => {
         t.error(error)
         t.ok(putMetrics.isDone())
@@ -937,3 +938,90 @@ tap.test('record data usage supportability metrics', (t) => {
     )
   })
 })
+
+tap.test('_safeRequest logging', (t) => {
+  t.autoend()
+  t.beforeEach((t) => {
+    const sandbox = sinon.createSandbox()
+    const loggerMock = require('../mocks/logger')(sandbox)
+    const RemoteMethod = proxyquire('../../../lib/collector/remote-method', {
+      '../logger': {
+        child: sandbox.stub().callsFake(() => loggerMock)
+      }
+    })
+    sandbox.stub(RemoteMethod.prototype, '_request')
+    t.context.loggerMock = loggerMock
+    t.context.RemoteMethod = RemoteMethod
+    t.context.sandbox = sandbox
+    t.context.options = {
+      host: 'collector.newrelic.com',
+      port: 80,
+      onError: () => {},
+      onResponse: () => {},
+      body: 'test-body',
+      path: '/nonexistent'
+    }
+    t.context.config = { license_key: 'shhh-dont-tell', max_payload_size_in_bytes: 10000 }
+  })
+
+  t.afterEach((t) => {
+    const { sandbox } = t.context
+    sandbox.restore()
+  })
+
+  t.test('should redact license key in logs', (t) => {
+    const { RemoteMethod, loggerMock, options, config } = t.context
+    loggerMock.traceEnabled.returns(true)
+    const method = new RemoteMethod('test', { config })
+    method._safeRequest(options)
+    t.same(
+      loggerMock.trace.args,
+      [
+        [
+          { body: options.body },
+          'Posting to %s://%s:%s%s',
+          'https',
+          options.host,
+          options.port,
+          '/agent_listener/invoke_raw_method?marshal_format=json&protocol_version=17&license_key=REDACTED&method=test'
+        ]
+      ],
+      'should redact key in trace level log'
+    )
+    t.end()
+  })
+
+  t.test('should call logger if trace is not enabled but audit logging is enabled', (t) => {
+    const { RemoteMethod, loggerMock, options, config } = t.context
+    loggerMock.traceEnabled.returns(false)
+    config.logging = { level: 'info' }
+    config.audit_log = { enabled: true, endpoints: ['test'] }
+    const method = new RemoteMethod('test', { config })
+    method._safeRequest(options)
+    t.same(
+      loggerMock.info.args,
+      [
+        [
+          { body: options.body },
+          'Posting to %s://%s:%s%s',
+          'https',
+          options.host,
+          options.port,
+          '/agent_listener/invoke_raw_method?marshal_format=json&protocol_version=17&license_key=REDACTED&method=test'
+        ]
+      ],
+      'should redact key in trace level log'
+    )
+    t.end()
+  })
+
+  t.test('should not call logger if trace or audit logging is not enabled', (t) => {
+    const { RemoteMethod, loggerMock, options, config } = t.context
+    loggerMock.traceEnabled.returns(false)
+    const method = new RemoteMethod('test', { config })
+    method._safeRequest(options)
+    t.ok(loggerMock.trace.callCount === 0, 'should not log outgoing message to collector')
+    t.ok(loggerMock.info.callCount === 0, 'should not log outgoing message to collector')
+    t.end()
+  })
+})

test/unit/mocks/logger.js

@@ -7,6 +7,7 @@
 const sinon = require('sinon')
 
 module.exports = (sandbox = sinon) => ({
+  traceEnabled: sandbox.stub().returns(true),
   trace: sandbox.stub(),
   info: sandbox.stub(),
   debug: sandbox.stub(),