Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use tls.ConnectionOptions instead of tls.SecureContextOptions #1312

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

bwalendz
Copy link
Contributor

Change TLS options to be consistent across the library and align with tls.connect() parameter types.

Allows sentinel connections to use all available TLS options. Right now the typings won't allow you to use the full range of TLS options for sentinelTLS that tls.ConnectionOptions provides.

Currently:

const redis = new ioredis({
  sentinels: [{
    host: 'localhost',
    port: 26379,
  }],
  name: 'mymaster',
  sentinelTLS: {
    servername: 'localhost', // <- NOT ALLOWED, type error
  },
});
const redis = new ioredis({
  host: 'localhost',
  port: 6379,
  tls: {
    servername: 'localhost', // <- ALLOWED
  },
});

ConnectionOptions is used by tls.connect()

function connect(options: ConnectionOptions, secureConnectListener?: () => void): TLSSocket;

ConnectionOptions extends SecureContextOptions, CommonConnectionOptions

interface ConnectionOptions extends SecureContextOptions, CommonConnectionOptions

… options

Allows sentinel connections to use all available TLS options

ConnectionOptions is used by tls.connect()

ConnectionOptions extends SecureContextOptions, CommonConnectionOptions
@luin
Copy link
Collaborator

luin commented Mar 29, 2021

Hey Brian, thanks for the contribution!

Can you elaborate a little more on the use cases that need to pass options other than ones defined in SecureContextOptions?

I tend to prevent users from passing CommonConnectionOptions via sentinelTLS as they may accidentally pass port/host which should be dynamic and resolved from Redis sentinel servers. Hard coding these dynamic options will make it not be able to connect to the server.

If servername is the only exception maybe we can just allow it?

@bwalendz
Copy link
Contributor Author

Ideally the sentinel tls options and redis tls options should be independent and controllable. Whitelisting a few common fields might be acceptable to not break existing configs.

The use case is you cannot connect to a TLS terminated reverse proxy for sentinel when its CA is unverified. So there are a few options you need to allow that like servername or rejectUnauthorized or SNICallback, etc for the sentinel connection. Then the redis connection can be controlled how it wants as well, like a non-TLS connection or more advanced options. I assume most use cases is TLS on for both redis and sentinel but would be useful to be able to control each independently.

@luin luin merged commit afd0c87 into redis:master Mar 29, 2021
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.24.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants