node-newrelic 11.23.2 appears to override redis client options

[mike-lang]

Description

We upgraded to node-newrelic 11.23.2 and our application started encountering certificate validation errors when connecting to redis despite our code passing options to disable certificate verification.

Expected Behavior

New Relic instrumentation of redis should respect and preserve any client options specified by application code

Troubleshooting or NR Diag results

We discovered errors with the following stack trace in our logs which indicated that the code was demanding certificate verification despite the options that our application code was passing:

Stack: Error: self-signed certificate in certificate chain
    at TLSSocket.onConnectSecure (node:_tls_wrap:1600:34)
    at TLSSocket.emit (node:events:517:28)
    at TLSSocket._finishInit (node:_tls_wrap:1017:8)
    at ssl.onhandshakedone (node:_tls_wrap:803:12)
   at TLSWrap.callbackTrampoline (node:internal/async_hooks:130:17)
Session redis error event: Error: self-signed certificate in certificate chain

Steps to Reproduce

1. Create a heroku app
2. Provision for it the heroku redis and heroku new relic addons
3. Deploy application code that connects to the redis instance with tls.rejectUnauthorized: false option

Your Environment

Node version: 18.x
redis version: ^4.6.12
node-newrelic version: 11.23.2

redis provider: heroku redis

[workato-integration]

https://new-relic.atlassian.net/browse/NR-298929

[mrickard]

@mike-lang Sorry to hear you're having trouble. Which version of the New Relic Node Agent were you using before the update? The most recent change we made to redis instrumentation was in 11.23.1.

Do you see any difference if you revert to a version prior to 11.23.1? Would you be able to post a reproduction codebase?

[mike-lang]

@mrickard Before the update we were using 11.10.1 of the New Relic Node Agent.

We reverted to 11.23.0 and no longer encounter the error.

I'm afraid it's not feasible for us to create a reproduction codebase for you, but I can offer you this sketch of code that I expect would reproduce the error should it be deployed as a heroku app with a connection string to a heroku redis instance in an environment variable named REDIS_URL:

const Redis = require('redis');

const redisOptions = {
  url: process.env.REDIS_URL,
  tls: {
    rejectUnauthorized: false
  }
};

client = Redis.createClient(redisOptions);
client.on('error', function(err) {
  console.error('Session redis error event: ' + err + '\nStack: ' + err.stack);
});

client.on('connect', function() {
  console.log('Hello Redis!');
});

console.log('Connecting Session redis client...');
client.connect()

The key issue is that heroku redis uses self-signed certificates in their certificate chain, and the latest version of the newrelic module is overriding the configuration we have in place to accomodate their self-signed certificate.

[mrickard]

@mike-lang We're looking into this--I'm trying to reproduce on my end. There's an issue in the redis repo suggesting that a slight change to the redisOptions might be helpful. Do you still see the error if you try this? redis/node-redis#2347 (comment)

[mike-lang]

@mrickard While working to resolve the issue in our app, we also tried the socket property style of configuration. It did not work.

We've adopted the older version of node-newrelic and expect to use it until a patch can be developed that addresses this issue.

I don't know the full history of what's been going on with the node-redis project but comments on this issue suggest that configuration style has been in a state of change for some time. Hopefully that knowledge helps you sort out how to reproduce.

[jsumners-nr]

According to https://github.com/redis/node-redis/blob/2fc79bdfb375602e2aaba15962f57c88d8afe46b/docs/client-configuration.md, the configuration for the client should be:

redis.createClient({
	socket: {
		tls: true,
		rejectUnauthorized: false
	}
})

In #2446 I have added a test that verifies we can connect to a Redis server presenting a self-signed certificate. It uses the configuration block I have outlined and succeeds as expected.

[clacayetano]

Our team is also experiencing the same issue.

We were previously using 11.19.0. After upgrading to 11.23.2, we started seeing Error accepting a client connection: (null) in our logs. The issue doesn't happen if we downgrade to 11.23.0, or if the New Relic agent isn't connected. I also tried upgrading newrelic to 12.0.0, but that still causes Redis connection issues.

For now, we'll lock our newrelic version to 11.23.0 until this is addressed.

[mrickard]

@clacayetano I'm sorry to hear you're having trouble. We've been unable to reproduce this behavior, and the additions @jsumners-nr made to Redis versioned tests show that under our test conditions, tls configuration is maintained. Can you share a reproduction case, or any details that might help our investigation?

[clacayetano]

@mrickard sure! let me know if you need more info.

Environment
Node version: ^20.11.1
redis version: ^4.7.0
node-newrelic version: ^11.23.2 (version that starts breaking)

Redis provider: Heroku Data for Redis (premium-0 plan)

Redis client code

createClient({
  url: process.env.REDIS_URL,
  socket: {
    tls: true,
    rejectUnauthorized: false,
  },
});

[mrickard]

@clacayetano I've not been able to reproduce this behavior yet. Would you be able to supply a sanitized version of the output of this, included in your example? I'm curious if there's a difference in output between 11.23.0 and 11.23.2 or higher:

const cfg = await client.CONFIG_GET('tls*');
console.log("CONFIG", cfg)

(For me, the output is the same for 11.23.0 and 12.0.0.)

[jsumners-nr]

The replication is possible in #2446 by changing the connection config to:

t.beforeEach(async (t) => {
    t.agent = helper.instrumentMockedAgent()
    t.redis = require('redis')
    t.client = await t.redis
      .createClient({
        url: 'rediss://127.0.0.1:6380',
        socket: {
          // port: 6380,
          // host: '127.0.0.1',
          tls: true,
          rejectUnauthorized: false
        }
      })
      .on('error', (error) => {
        throw error
      })
      .connect()
    await t.client.flushAll()
  })

[bizob2828]

A fixed has been released in 12.1.0

[clacayetano]

Thanks everyone! I just upgraded to 12.1.0 and the issue no longer happens!