fix: receiving central config with sanitize_field_names would crash (…

…#3249) The crash was with init of config.sanitizeFieldNamesRegExp in the central-config code path. I've fixed that. Other updates: - Updated config normalization of this var -- and similar config vars that set a `${name}RegExp` array var -- initialize the RegExp array to empty before populating it. - Updated the tests to ensure we always add to "central-config-enabled.test.js" whenever a new supported central config var is added. - Updated central-config handling to try/catch and log.error instead of *crashing* if there is an unexpected exception. Partially applied central config isn't great, but it is better than having a DoS avenue where central config can crash agents. For example: {"log.level":"error",...,"remoteConf":{"sanitize_field_names":"password, pass*"},"error":{"type":"TypeError","message":"Cannot read properties of undefined (reading 'push')","stack_trace":"..."},"message":"Central config error: exception while applying changes"} Fixes: #3247
elastic · Apr 6, 2023 · 7f1e8fd · 7f1e8fd
1 parent 42d7e69
commit 7f1e8fd
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 78 deletions.
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
@@ -32,6 +32,25 @@ Notes:
 === Node.js Agent version 3.x
 
 
+==== Unreleased
+
+[float]
+===== Breaking changes
+
+[float]
+===== Features
+
+[float]
+===== Bug fixes
+
+* Fix an issue where the APM agent receiving central config (from APM server)
+  containing a value for `sanitized_field_names` would crash.
+  ({issues}3247[#3247])
+
+[float]
+===== Chores
+
+
 [[release-notes-3.44.0]]
 ==== 3.44.0 2023/04/03
 

diff --git a/lib/config.js b/lib/config.js
@@ -185,7 +185,7 @@ var ENV_TABLE = {
   verifyServerCert: 'ELASTIC_APM_VERIFY_SERVER_CERT'
 }
 
-var CENTRAL_CONFIG = {
+var CENTRAL_CONFIG_OPTS = {
   log_level: 'logLevel',
   transaction_sample_rate: 'transactionSampleRate',
   transaction_max_spans: 'transactionMaxSpans',
@@ -478,34 +478,36 @@ class Config {
 
         transport.on('config', remoteConf => {
           agent.logger.debug({ remoteConf }, 'central config received')
-          const conf = {}
-          const unknown = []
-
-          for (const [key, value] of Object.entries(remoteConf)) {
-            const newKey = CENTRAL_CONFIG[key]
-            if (newKey) {
-              conf[newKey] = value
-            } else {
-              unknown.push(key)
+          try {
+            const conf = {}
+            const unknown = []
+
+            for (const [key, value] of Object.entries(remoteConf)) {
+              const newKey = CENTRAL_CONFIG_OPTS[key]
+              if (newKey) {
+                conf[newKey] = value
+              } else {
+                unknown.push(key)
+              }
+            }
+            if (unknown.length > 0) {
+              agent.logger.warn(`Central config warning: unsupported config names: ${unknown.join(', ')}`)
             }
-          }
-
-          if (unknown.length > 0) {
-            agent.logger.warn(`Central config warning: unsupported config names: ${unknown.join(', ')}`)
-          }
-
-          if (Object.keys(conf).length > 0) {
-            normalize(conf, agent.logger)
 
-            for (const [key, value] of Object.entries(conf)) {
-              const oldValue = agent._conf[key]
-              agent._conf[key] = value
-              if (key === 'logLevel' && value !== oldValue && !logging.isLoggerCustom(agent.logger)) {
-                logging.setLogLevel(agent.logger, value)
-                agent.logger.info(`Central config success: updated logger with new logLevel: ${value}`)
+            if (Object.keys(conf).length > 0) {
+              normalize(conf, agent.logger)
+              for (const [key, value] of Object.entries(conf)) {
+                const oldValue = agent._conf[key]
+                agent._conf[key] = value
+                if (key === 'logLevel' && value !== oldValue && !logging.isLoggerCustom(agent.logger)) {
+                  logging.setLogLevel(agent.logger, value)
+                  agent.logger.info(`Central config success: updated logger with new logLevel: ${value}`)
+                }
+                agent.logger.info(`Central config success: updated ${key}: ${value}`)
               }
-              agent.logger.info(`Central config success: updated ${key}: ${value}`)
             }
+          } catch (err) {
+            agent.logger.error({ remoteConf, err }, 'Central config error: exception while applying changes')
           }
         })
 
@@ -896,6 +898,7 @@ function normalizeTransactionSampleRate (opts, logger) {
 
 function normalizeSanitizeFieldNames (opts) {
   if (opts.sanitizeFieldNames) {
+    opts.sanitizeFieldNamesRegExp = []
     const wildcard = new WildcardMatcher()
     for (const ptn of opts.sanitizeFieldNames) {
       const re = wildcard.compile(ptn)
@@ -917,12 +920,7 @@ function normalizeCloudProvider (opts, logger) {
 
 function normalizeIgnoreOptions (opts) {
   if (opts.transactionIgnoreUrls) {
-    // We can't guarantee that opts will be a Config so set a
-    // default value. This is to work around CENTRAL_CONFIG tests
-    // that call this method with a plain object `{}`
-    if (!opts.transactionIgnoreUrlRegExp) {
-      opts.transactionIgnoreUrlRegExp = []
-    }
+    opts.transactionIgnoreUrlRegExp = []
     const wildcard = new WildcardMatcher()
     for (const ptn of opts.transactionIgnoreUrls) {
       const re = wildcard.compile(ptn)
@@ -931,25 +929,33 @@ function normalizeIgnoreOptions (opts) {
   }
 
   if (opts.ignoreUrls) {
+    opts.ignoreUrlStr = []
+    opts.ignoreUrlRegExp = []
     for (const ptn of opts.ignoreUrls) {
-      if (typeof ptn === 'string') opts.ignoreUrlStr.push(ptn)
-      else opts.ignoreUrlRegExp.push(ptn)
+      if (typeof ptn === 'string') {
+        opts.ignoreUrlStr.push(ptn)
+      } else {
+        opts.ignoreUrlRegExp.push(ptn)
+      }
     }
     delete opts.ignoreUrls
   }
 
   if (opts.ignoreUserAgents) {
+    opts.ignoreUserAgentStr = []
+    opts.ignoreUserAgentRegExp = []
     for (const ptn of opts.ignoreUserAgents) {
-      if (typeof ptn === 'string') opts.ignoreUserAgentStr.push(ptn)
-      else opts.ignoreUserAgentRegExp.push(ptn)
+      if (typeof ptn === 'string') {
+        opts.ignoreUserAgentStr.push(ptn)
+      } else {
+        opts.ignoreUserAgentRegExp.push(ptn)
+      }
     }
     delete opts.ignoreUserAgents
   }
 
   if (opts.ignoreMessageQueues) {
-    if (!opts.ignoreMessageQueuesRegExp) {
-      opts.ignoreMessageQueuesRegExp = []
-    }
+    opts.ignoreMessageQueuesRegExp = []
     const wildcard = new WildcardMatcher()
     for (const ptn of opts.ignoreMessageQueues) {
       const re = wildcard.compile(ptn)
@@ -960,6 +966,7 @@ function normalizeIgnoreOptions (opts) {
 
 function normalizeElasticsearchCaptureBodyUrls (opts) {
   if (opts.elasticsearchCaptureBodyUrls) {
+    opts.elasticsearchCaptureBodyUrlsRegExp = []
     const wildcard = new WildcardMatcher()
     for (const ptn of opts.elasticsearchCaptureBodyUrls) {
       const re = wildcard.compile(ptn)
@@ -1354,6 +1361,7 @@ module.exports = {
   TRACE_CONTINUATION_STRATEGY_RESTART_EXTERNAL,
 
   // The following are exported for tests.
+  CENTRAL_CONFIG_OPTS,
   DEFAULTS,
   DURATION_OPTS,
   secondsFromDuration,

diff --git a/test/central-config-enabled.test.js b/test/central-config-enabled.test.js
@@ -13,7 +13,9 @@ const { URL } = require('url')
 const http = require('http')
 
 const test = require('tape')
+
 const Agent = require('./_agent')
+const { CENTRAL_CONFIG_OPTS } = require('../lib/config')
 
 const runTestsWithServer = (t, updates, expect) => {
   let agent
@@ -65,59 +67,37 @@ const runTestsWithServer = (t, updates, expect) => {
 
 test('remote config enabled', function (t) {
   const updates = {
-    transaction_sample_rate: '0.42',
-    transaction_max_spans: '99',
     capture_body: 'all',
-    transaction_ignore_urls: ['foo'],
+    exit_span_min_duration: '25ms',
+    ignore_message_queues: 'spam,eggs, (?-i)ham ',
     log_level: 'warn',
+    sanitize_field_names: 'password, pass*, *auth*',
     span_stack_trace_min_duration: '50ms',
     trace_continuation_strategy: 'restart_external',
-    exit_span_min_duration: '25ms'
+    // Test that central config returing an array works, even though, IIUC,
+    // it will always return plain strings.
+    transaction_ignore_urls: ['foo', 'bar'],
+    transaction_max_spans: '99',
+    transaction_sample_rate: '0.42'
   }
   const expect = {
-    transactionSampleRate: 0.42,
-    transactionMaxSpans: 99,
     captureBody: 'all',
-    transactionIgnoreUrls: ['foo'],
+    exitSpanMinDuration: 0.025,
+    ignoreMessageQueues: ['spam', 'eggs', '(?-i)ham'],
+    ignoreMessageQueuesRegExp: [/^spam$/i, /^eggs$/i, /^ham$/],
     logLevel: 'warn',
+    sanitizeFieldNames: ['password', 'pass*', '*auth*'],
+    sanitizeFieldNamesRegExp: [/^password$/i, /^pass.*$/i, /^.*auth.*$/i],
     spanStackTraceMinDuration: 0.05,
     traceContinuationStrategy: 'restart_external',
-    exitSpanMinDuration: 0.025
-  }
-
-  runTestsWithServer(t, updates, expect)
-})
-
-test('remote config enabled: receives comma delimited', function (t) {
-  const updates = {
-    transaction_sample_rate: '0.42',
-    transaction_max_spans: '99',
-    capture_body: 'all',
-    transaction_ignore_urls: 'foo,bar , baz , bling'
-  }
-  const expect = {
-    transactionSampleRate: 0.42,
+    transactionIgnoreUrls: ['foo', 'bar'],
+    transactionIgnoreUrlRegExp: [/^foo$/i, /^bar$/i],
     transactionMaxSpans: 99,
-    captureBody: 'all',
-    transactionIgnoreUrls: ['foo', 'bar', 'baz', 'bling']
+    transactionSampleRate: 0.42
   }
 
-  runTestsWithServer(t, updates, expect)
-})
-
-test('remote config enabled: receives non delimited string', function (t) {
-  const updates = {
-    transaction_sample_rate: '0.42',
-    transaction_max_spans: '99',
-    capture_body: 'all',
-    transaction_ignore_urls: 'foo:bar'
-  }
-  const expect = {
-    transactionSampleRate: 0.42,
-    transactionMaxSpans: 99,
-    captureBody: 'all',
-    transactionIgnoreUrls: ['foo:bar']
-  }
+  t.deepEqual(Object.keys(updates).sort(), Object.keys(CENTRAL_CONFIG_OPTS).sort(),
+    'this test uses every available central config var name (config.CENTRAL_CONFIG_OPTS)')
 
   runTestsWithServer(t, updates, expect)
 })