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.

Sign up for GitHub

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

Jump to bottom

feat: Automatically set tls.servername option with rediss:// URLs #950

Closed
wants to merge 3 commits into from
Closed

feat: Automatically set tls.servername option with rediss:// URLs #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d190ec
fix: Don’t clobber passed-in tls options with rediss:/ URLs
paulmelnikow Aug 20, 2019
d6fc213
feat: Automatically set tls.servername option with rediss:// URLs
paulmelnikow Aug 20, 2019
e20df82
Fix test
paulmelnikow Aug 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/redis/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ Redis.prototype.resetOfflineQueue = function() {

Redis.prototype.parseOptions = function() {
this.options = {};
let isTls = false;
for (var i = 0; i < arguments.length; ++i) {
var arg = arguments[i];
if (arg === null || typeof arg === "undefined") {
Expand All @@ -201,12 +202,18 @@ Redis.prototype.parseOptions = function() {
defaults(this.options, arg);
} else if (typeof arg === "string") {
defaults(this.options, parseURL(arg));
if (arg.startsWith("rediss://")) {
isTls = true;
}
} else if (typeof arg === "number") {
this.options.port = arg;
} else {
throw new Error("Invalid argument " + arg);
}
}
if (isTls) {
defaults(this.options, { tls: { servername: this.options.host } });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if setting tls to this.options.host instead of true applies to more scenarios, but I think at least we need to check whether this.options.host is a hostname instead of an IP address.

As for me, I prefer the way that we just default tls options to true without setting any other properties (like servername) and let users handle it. The improvement we should make here is to keep the options provided by users not be overwritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if setting tls to this.options.host instead of true applies to more scenarios, but I think at least we need to check whether this.options.host is a hostname instead of an IP address.

I'm happy to make that modification if you'd like.

Setting tls.options.host to the hostname of the server is necessary to use many shared Redis hosting services, including Compose.io and Redis Labs. It reduces boilerplate and seems like a reasonable default as this is not really a fringe use case, but a common scenario for TLS.

The improvement we should make here is to keep the options provided by users not be overwritten.

Alternatively, #949 does just that!

}
defaults(this.options, Redis.defaultOptions);

if (typeof this.options.port === "string") {
Expand Down
3 changes: 0 additions & 3 deletions lib/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,6 @@ export function parseURL(url) {
if (parsed.port) {
result.port = parsed.port;
}
if (parsed.protocol === "rediss:") {
result.tls = true;
}
defaults(result, parsed.query);

return result;
Expand Down
8 changes: 8 additions & 0 deletions test/unit/redis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ describe("Redis", function() {
host: "192.168.1.1"
});
expect(option).to.have.property("port", 6380);

option = getOption("rediss://host.test");
expect(option.tls).to.deep.equal({ servername: "host.test" });

option = getOption("rediss://example.test", {
tls: { rejectUnauthorized: false }
});
expect(option.tls).to.deep.equal({ rejectUnauthorized: false });
} catch (err) {
stub.restore();
throw err;
Expand Down
1 change: 0 additions & 1 deletion test/unit/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ describe("utils", function() {
expect(
utils.parseURL("rediss://user:[email protected]:6380/4?key=value")
).to.eql({
tls: true,
host: "127.0.0.1",
port: "6380",
db: "4",
Expand Down