feat: bring TLS to SASL-level support - #134
Conversation
TLS now flows through Auto Discovery (config endpoint and discovered nodes), exposes node.tls/tlsEnabled like SASL credentials, composes with SASL on a dedicated compose service, and documents per-node setup, SNI, and server flags at the same depth as SASL. Co-authored-by: Jared Wray <me@jaredwray.com>
Mock data-node connect() so client.connect() does not TLS-handshake 10.0.0.1 before the config endpoint is polled, and cover empty-IP discovery for SNI host selection. Co-authored-by: Jared Wray <me@jaredwray.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #134 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 1411 1471 +60
Branches 310 333 +23
=========================================
+ Hits 1411 1471 +60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
client.connect() creates a new MemcacheNode for the configuration endpoint. Instance spies on client.nodes never wrap that handshake, so TLS against plaintext FakeConfigServer hung until Vitest's 5s timeout on CI. Spy MemcacheNode.prototype.connect instead. Co-authored-by: Jared Wray <me@jaredwray.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ee786d0ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!currentNodeIds.has(id)) { | ||
| try { | ||
| const host = node.ip || node.hostname; | ||
| const wrappedHost = host.includes(":") ? `[${host}]` : host; | ||
| await this.addNode(`${wrappedHost}:${node.port}`); | ||
| await this.addDiscoveredNode(node); |
There was a problem hiding this comment.
When TLS is inferred solely from a memcaches:// configuration endpoint, any preconfigured node whose ID also appears in the discovered topology is skipped here and retains its original plaintext configuration. For example, a client seeded with 10.0.0.1:11211 will leave that node plaintext while newly discovered peers use TLS/SNI, so keys hashed to the seed fail against a TLS-only cluster. Existing matching nodes need to be replaced or upgraded when their effective TLS/SNI settings differ.
Useful? React with 👍 / 👎.
| Auto Discovery uses the same client-level `tls` (and `sasl`) options as data | ||
| nodes, including the configuration-endpoint connection. Set `tls: true` (or a |
There was a problem hiding this comment.
This claim is not supported for a SASL-protected configuration endpoint: AutoDiscovery.fetchConfig() sends the ASCII config get cluster/legacy command through MemcacheNode.command(), while a SASL node requires the binary protocol and its permanent data handler explicitly skips handleData() whenever _sasl is set. Consequently, configuring the advertised TLS+SASL Auto Discovery flow cannot obtain a topology and may leave the command pending until the connection closes; either implement the discovery exchange over the authenticated protocol or remove the SASL claim and avoid forwarding credentials to this connection.
Useful? React with 👍 / 👎.
| const options: TlsConnectionOptions = tls === true ? {} : { ...tls }; | ||
| // Node identity always wins over any host/port/path in user options. | ||
| options.host = undefined; | ||
| options.port = undefined; | ||
| options.path = undefined; |
There was a problem hiding this comment.
A tls.ConnectionOptions object can contain a preconnected socket, and spreading it here preserves that field. Node's tls.connect() wraps the supplied socket and ignores the subsequently assigned host, port, and path, so callers can still retarget the connection despite the new invariant and may send SASL credentials or commands to the socket's actual peer. Remove or reject options.socket when the node identity is intended to always win.
Useful? React with 👍 / 👎.
| const { host, port, secure } = this.parseEndpoint(this._configEndpoint); | ||
| const tls = secure ? this._tls || true : this._tls; | ||
| if (this._tls === undefined && tls) { | ||
| this._tls = tls; | ||
| } | ||
|
|
||
| this._configNode = new MemcacheNode(host, port, { | ||
| timeout: this._timeout, | ||
| keepAlive: this._keepAlive, | ||
| keepAliveDelay: this._keepAliveDelay, | ||
| sasl: this._sasl, | ||
| tls, |
There was a problem hiding this comment.
When the configuration endpoint is a DNS name and TLS is enabled with true or inferred from memcaches://, this passes a bare TLS option to MemcacheNode. Node's tls.connect() does not infer servername from host, so a configuration service that selects its certificate through SNI returns its default certificate and the discovery handshake fails. For non-IP endpoint hosts, merge servername: host unless the caller already supplied a server name, as is done for discovered data nodes.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit 2ee786d. Configure here.
| const tls = secure ? this._tls || true : this._tls; | ||
| if (this._tls === undefined && tls) { | ||
| this._tls = tls; | ||
| } |
There was a problem hiding this comment.
False TLS loses to memcaches scheme
Medium Severity
this._tls || true treats an explicit tls: false as unset when the configuration endpoint is memcaches://, so the config node still does a TLS handshake. effectiveTls uses ??, so that same false keeps discovered data nodes on plaintext. The cluster can end up with a TLS config connection and unencrypted node connections.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2ee786d. Configure here.
2 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature / completeness: TLS now matches SASL in API surface, Auto Discovery, tests, and docs.
Audit (TLS vs SASL)
SASL already had client + node options, Auto Discovery wiring, introspection (
hasSaslCredentials/isAuthenticated), factory tests, a dedicated compose service, and a full README (options, per-node, events, server setup). TLS had the socket handshake andmemcaches://URIs, but Auto Discovery’s config endpoint ignored TLS, there was no node introspection, no TLS+SASL path, thin operation coverage, and the README called out Auto Discovery as unfinished.Changes
tlsinto Auto Discovery the same way assasl;memcaches://config endpoints enable TLSservernameto the hostname (ElastiCache in-transit certs)node.tlsEnabled/node.tls; node identity (host/port/path) always wins overtls.connect()overrides; Unixmemcaches://URIs round-tripmemcached-tls-saslon21215for combined encryption + AUTHTesting
pnpm buildsucceeded21211, SASL11215, TLS+SASL21215)startAutoDiscoveryTLS tests timed out becauseclient.connect()TLS-handshakes a new config-endpoint node against plaintextFakeConfigServer; instance spies onclient.nodesdid not cover that node. Fixed by mockingMemcacheNode.prototype.connect.