Merge pull request #1139 from NodeJS-agent/bclement/security-spec

NODE-1436 - Bclement/security spec
newrelic · Apr 29, 2017 · 702c9e1 · 702c9e1
2 parents a7e2332 + 711f4db
commit 702c9e1
Show file tree

Hide file tree

Showing 9 changed files with 206 additions and 22 deletions.
diff --git a/api.js b/api.js
@@ -149,6 +149,11 @@ API.prototype.addCustomParameter = function addCustomParameter(name, value) {
       "Custom parameters are disabled by high security mode."
     )
     return false
+  } else if (!this.agent.config.api.custom_parameters_enabled) {
+    logger.debug(
+      "Config.api.custom_parameters_enabled set to false, not collecting value"
+    )
+    return false
   }
 
   var ignored = this.agent.config.ignored_params || []
@@ -259,6 +264,19 @@ API.prototype.noticeError = function noticeError(error, customParameters) {
   )
   metric.incrementCallCount()
 
+  // If high security mode is on, noticeError is disabled.
+  if (this.agent.config.high_security === true) {
+    logger.warnOnce(
+      "Notice Error",
+      "Notice error API are disabled by high security mode."
+    )
+    return false
+  } else if (!this.agent.config.api.notice_error_enabled) {
+    logger.debug(
+      "Config.api.notice_error_enabled set to false, not collecting error"
+    )
+    return false
+  }
 
   if (typeof error === 'string') error = new Error(error)
   var transaction = this.agent.tracer.getTransaction()
@@ -801,6 +819,20 @@ API.prototype.recordCustomEvent = function recordCustomEvent(eventType, attribut
   )
   metric.incrementCallCount()
 
+  // If high security mode is on, custom events are disabled.
+  if (this.agent.config.high_security === true) {
+    logger.warnOnce(
+      "Custom Event",
+      "Custom events are disabled by high security mode."
+    )
+    return false
+  } else if (!this.agent.config.api.custom_events_enabled) {
+    logger.debug(
+      "Config.api.custom_events_enabled set to false, not collecting value"
+    )
+    return false
+  }
+
   if (!this.agent.config.custom_insights_events.enabled) {
     return
   }

diff --git a/lib/config.default.js b/lib/config.default.js
@@ -431,6 +431,33 @@ exports.config = {
      */
     debug: false
   },
+  /**
+   * API Configuration
+   *
+   * Some API end points can be turned off via configuration settings to
+   * allow for more flexible security options. All API configuration
+   * options are disabled when high-security mode is enabled.
+   */
+  api: {
+    /**
+     * Controls for the `API.addCustomParameter` method.
+     *
+     * @env NEW_RELIC_API_CUSTOM_PARAMETERS
+     */
+    custom_parameters_enabled: true,
+    /**
+     * Controls for the `API.recordCustomEvent` method.
+     *
+     * @env NEW_RELIC_API_CUSTOM_PARAMETERS
+     */
+    custom_events_enabled: true,
+    /**
+     * Controls for the `API.noticeError` method.
+     *
+     * @env NEW_RELIC_API_NOTICE_ERROR
+     */
+    notice_error_enabled: true,
+  },
   /**
    * Transaction Events
    *

diff --git a/lib/config.js b/lib/config.js
@@ -105,6 +105,11 @@ var ENV_MAPPING = {
     display_name: "NEW_RELIC_PROCESS_HOST_DISPLAY_NAME",
     ipv_preference: "NEW_RELIC_IPV_PREFERENCE"
   },
+  api: {
+    custom_parameters_enabled: "NEW_RELIC_API_CUSTOM_PARAMETERS",
+    custom_events_enabled: "NEW_RELIC_API_CUSTOM_EVENTS",
+    notice_error_enabled: "NEW_RELIC_API_NOTICE_ERROR"
+  },
   datastore_tracer: {
     instance_reporting: {
       enabled: "NEW_RELIC_DATASTORE_INSTANCE_REPORTING_ENABLED"
@@ -171,7 +176,7 @@ var HIGH_SECURITY_SETTINGS = {
   ssl: true,
   capture_params: false,
   transaction_tracer: {
-    record_sql: 'off'
+    record_sql: 'obfuscated'
   },
   slow_sql: {
     enabled: false

diff --git a/lib/errors/aggregator.js b/lib/errors/aggregator.js
@@ -286,7 +286,7 @@ function _collect(transaction, exception, customParameters, timestamp) {
 
   // add error event
   if (this.config.error_collector.capture_events === true) {
-    this.events.add(createEvent(transaction, error, timestamp))
+    this.events.add(createEvent(transaction, error, timestamp, this.config))
   }
   return true
 }

diff --git a/lib/errors/index.js b/lib/errors/index.js
@@ -36,7 +36,12 @@ function createError(transaction, exception, customParameters, config) {
   // user, but it is a common pattern.
   if (typeof exception === 'string') {
     message = exception
-  } else if (exception !== null && typeof exception === 'object' && exception.message) {
+  } else if (
+      exception !== null &&
+      typeof exception === 'object' &&
+      exception.message &&
+      !config.high_security
+    ) {
     message = exception.message
 
     if (exception.name) {
@@ -80,7 +85,12 @@ function createError(transaction, exception, customParameters, config) {
 
 
   var stack = exception && exception.stack
-  if (stack) params.stack_trace = ('' + stack).split(/[\n\r]/g)
+  if (stack) {
+    params.stack_trace = ('' + stack).split(/[\n\r]/g)
+    if (config.high_security) {
+      params.stack_trace[0] = exception.name + ': <redacted>'
+    }
+  }
 
   var res = [0, name, message, type, params]
   if (transaction) {
@@ -95,13 +105,18 @@ function createError(transaction, exception, customParameters, config) {
  * Creates a structure for error event that is sent to the collector.
  * The error parameter is an output of the createError() function for a given exception.
  */
-function createEvent(transaction, error, timestamp) {
+function createEvent(transaction, error, timestamp, config) {
   var message = error[2]
   var errorClass = error[3]
   var paramsFromError = error[4]
 
-  var intrinsicAttributes = _getErrorEventIntrinsicAttrs(transaction, errorClass, message,
-      timestamp)
+  var intrinsicAttributes = _getErrorEventIntrinsicAttrs(
+    transaction,
+    errorClass,
+    message,
+    timestamp,
+    config
+  )
 
   // the error structure created by createError() already performs filtering of custom
   // and agent attributes, so it is ok to just copy them
@@ -117,14 +132,14 @@ function createEvent(transaction, error, timestamp) {
   return errorEvent
 }
 
-function _getErrorEventIntrinsicAttrs(transaction, errorClass, message, timestamp) {
+function _getErrorEventIntrinsicAttrs(transaction, errorClass, message, timestamp, conf) {
   // the server expects seconds instead of milliseconds
   if (timestamp) timestamp = timestamp / 1000
 
   var attributes = {
     type: "TransactionError",
     "error.class": errorClass,
-    "error.message": message,
+    "error.message": conf.high_security ? '' : message,
     timestamp: timestamp
   }
 

diff --git a/test/unit/api/api.test.js b/test/unit/api/api.test.js
@@ -61,6 +61,76 @@ describe("the New Relic agent API", function () {
     expect(api.addCustomParameters).a('function')
   })
 
+  describe("when adding custom parameters", function () {
+    it("should properly add custom parameters", function () {
+      helper.runInTransaction(agent, function (transaction) {
+        api.addCustomParameter('test', 1)
+        expect(transaction.trace.custom['test']).to.equal(1)
+        transaction.end()
+      })
+    })
+
+    it("should properly add mutliple custom parameters", function () {
+      helper.runInTransaction(agent, function (transaction) {
+        api.addCustomParameters({
+          'test': 1,
+          'second': 2
+        })
+        expect(transaction.trace.custom['test']).to.equal(1)
+        expect(transaction.trace.custom['second']).to.equal(2)
+        transaction.end()
+      })
+    })
+
+    it("should not add custom parameters when disabled", function () {
+      helper.runInTransaction(agent, function (transaction) {
+        agent.config.api.custom_parameters_enabled = false
+        api.addCustomParameter('test', 1)
+        expect(transaction.trace.custom['test']).to.equal(undefined)
+        agent.config.api.custom_parameters_enabled = true
+        transaction.end()
+      })
+    })
+
+    it("should not add mutliple custom parameters when disabled", function () {
+      helper.runInTransaction(agent, function (transaction) {
+        agent.config.api.custom_parameters_enabled = false
+        api.addCustomParameters({
+          'test': 1,
+          'second': 2
+        })
+        expect(transaction.trace.custom['test']).to.equal(undefined)
+        expect(transaction.trace.custom['second']).to.equal(undefined)
+        agent.config.api.custom_parameters_enabled = true
+        transaction.end()
+      })
+    })
+
+    it("should not add custom parameters when in high security mode", function () {
+      helper.runInTransaction(agent, function (transaction) {
+        agent.config.high_security = true
+        api.addCustomParameter('test', 1)
+        expect(transaction.trace.custom['test']).to.equal(undefined)
+        agent.config.high_security = false
+        transaction.end()
+      })
+    })
+
+    it("should not add mutliple custom parameters when in high security mode", function () {
+      helper.runInTransaction(agent, function (transaction) {
+        agent.config.high_security = true
+        api.addCustomParameters({
+          'test': 1,
+          'second': 2
+        })
+        expect(transaction.trace.custom['test']).to.equal(undefined)
+        expect(transaction.trace.custom['second']).to.equal(undefined)
+        agent.config.high_security = false
+        transaction.end()
+      })
+    })
+  })
+
   describe("when explicitly naming transactions", function () {
     describe("in the simplest case", function () {
       var segment
@@ -561,6 +631,22 @@ describe("the New Relic agent API", function () {
       expect(agent.errors.errors.length).equal(1)
     })
 
+    it("should not add errors in high security mode", function () {
+      agent.config.high_security = true
+      expect(agent.errors.errors.length).equal(0)
+      api.noticeError(new TypeError('this test is bogus, man'))
+      expect(agent.errors.errors.length).equal(0)
+      agent.config.high_security = false
+    })
+
+    it("should not add errors when noticeErrors is disabled", function () {
+      agent.config.api.notice_error_enabled = false
+      expect(agent.errors.errors.length).equal(0)
+      api.noticeError(new TypeError('this test is bogus, man'))
+      expect(agent.errors.errors.length).equal(0)
+      agent.config.api.notice_error_enabled = true
+    })
+
     it("should track custom parameters on error without a transaction", function () {
       expect(agent.errors.errors.length).equal(0)
       api.noticeError(new TypeError('this test is bogus, man'), {present : 'yep'})

diff --git a/test/unit/api/custom-events.test.js b/test/unit/api/custom-events.test.js
@@ -37,6 +37,18 @@ describe('The custom events API', function () {
     expect(myEvent).to.exist()
   })
 
+  it('does not collect events when high security mode is on', function () {
+    agent.config.high_security = true
+    api.recordCustomEvent('EventName', {key: 'value'})
+    expect(agent.customEvents.toArray().length).to.equal(0)
+  })
+
+  it('does not collect events when the endpoint is disabled in the config', function () {
+    agent.config.api.custom_events_enabled = false
+    api.recordCustomEvent('EventName', {key: 'value'})
+    expect(agent.customEvents.toArray().length).to.equal(0)
+  })
+
   it('creates the proper intrinsic values when recorded', function () {
     var when = Date.now()
     api.recordCustomEvent('EventName', {key: 'value'})

diff --git a/test/unit/errors/aggregator.test.js b/test/unit/errors/aggregator.test.js
@@ -144,6 +144,15 @@ describe('agent attribute format', function () {
 
     expect(params).deep.equals({})
   })
+
+  it('redacts the error message in high security mode', function () {
+    agent.config.high_security = true
+    error.add(trans, new Error('this should not be here'), {a: 'AA'})
+    agent.errors.onTransactionFinished(trans, agent.metrics)
+
+    expect(error.errors[0][2]).to.equal('')
+    expect(error.errors[0][4].stack_trace[0]).to.equal('Error: <redacted>')
+  })
 })
 
 describe('display name', function () {
@@ -1577,6 +1586,15 @@ describe('error events', function() {
     })
   })
 
+  it('should omit the error message when in high security mode', function() {
+    agent.config.high_security = true
+    agent.errors.add(null, new Error('some error'))
+    var events = agent.errors.getEvents()
+    expect(events[0][0]['error.message']).to.equal('')
+    agent.config.high_security = false
+
+  })
+
   it('not spill over reservoir size', function() {
     if (agent) helper.unloadAgent(agent)
     agent = helper.loadMockedAgent(null,

diff --git a/test/unit/high-security.test.js b/test/unit/high-security.test.js
@@ -155,19 +155,8 @@ describe('high security mode', function () {
         var config = new Config({'high_security': true})
         config.transaction_tracer.record_sql = 'raw'
         config.on('transaction_tracer.record_sql', function(value) {
-          value.should.equal('off')
-          config.transaction_tracer.record_sql.should.equal('off')
-          done()
-        })
-        config._applyHighSecurity()
-      })
-
-      it('should detect that record_sql is obfuscated', function (done) {
-        var config = new Config({'high_security': true})
-        config.transaction_tracer.record_sql = 'obfuscated'
-        config.on('transaction_tracer.record_sql', function(value) {
-          value.should.equal('off')
-          config.transaction_tracer.record_sql.should.equal('off')
+          value.should.equal('obfuscated')
+          config.transaction_tracer.record_sql.should.equal('obfuscated')
           done()
         })
         config._applyHighSecurity()