-
Notifications
You must be signed in to change notification settings - Fork 1
feat: bring TLS to SASL-level support #134
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { Hookified } from "hookified"; | ||
| import { MemcacheNode } from "./node.js"; | ||
| import { MemcacheNode, type MemcacheTlsOption } from "./node.js"; | ||
| import type { | ||
| ClusterConfig, | ||
| DiscoveredNode, | ||
|
|
@@ -14,6 +14,7 @@ export interface AutoDiscoveryOptions { | |
| keepAlive: boolean; | ||
| keepAliveDelay: number; | ||
| sasl?: SASLCredentials; | ||
| tls?: MemcacheTlsOption; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -32,6 +33,7 @@ export class AutoDiscovery extends Hookified { | |
| private _keepAlive: boolean; | ||
| private _keepAliveDelay: number; | ||
| private _sasl: SASLCredentials | undefined; | ||
| private _tls: MemcacheTlsOption | undefined; | ||
| private _isRunning = false; | ||
| private _isPolling = false; | ||
|
|
||
|
|
@@ -44,6 +46,7 @@ export class AutoDiscovery extends Hookified { | |
| this._keepAlive = options.keepAlive; | ||
| this._keepAliveDelay = options.keepAliveDelay; | ||
| this._sasl = options.sasl; | ||
| this._tls = options.tls; | ||
| } | ||
|
|
||
| /** Current config version. -1 means no config has been fetched yet. */ | ||
|
|
@@ -61,6 +64,14 @@ export class AutoDiscovery extends Hookified { | |
| return this._configEndpoint; | ||
| } | ||
|
|
||
| /** | ||
| * TLS option applied to the configuration-endpoint connection. | ||
| * `memcaches://` endpoints enable TLS even when this was not set. | ||
| */ | ||
| public get tls(): MemcacheTlsOption | undefined { | ||
| return this._tls; | ||
| } | ||
|
|
||
| /** | ||
| * Start the auto discovery process. | ||
| * Performs an initial discovery, then starts the polling timer. | ||
|
|
@@ -207,13 +218,18 @@ export class AutoDiscovery extends Hookified { | |
| return this._configNode; | ||
| } | ||
|
|
||
| const { host, port } = this.parseEndpoint(this._configEndpoint); | ||
| const { host, port, secure } = this.parseEndpoint(this._configEndpoint); | ||
| const tls = secure ? this._tls || true : this._tls; | ||
| if (this._tls === undefined && tls) { | ||
| this._tls = tls; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. False TLS loses to memcaches schemeMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 2ee786d. Configure here. |
||
|
|
||
| this._configNode = new MemcacheNode(host, port, { | ||
| timeout: this._timeout, | ||
| keepAlive: this._keepAlive, | ||
| keepAliveDelay: this._keepAliveDelay, | ||
| sasl: this._sasl, | ||
| tls, | ||
|
Comment on lines
+221
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the configuration endpoint is a DNS name and TLS is enabled with Useful? React with 👍 / 👎. |
||
| }); | ||
|
|
||
| await this._configNode.connect(); | ||
|
|
@@ -276,7 +292,27 @@ export class AutoDiscovery extends Hookified { | |
| } | ||
| } | ||
|
|
||
| private parseEndpoint(endpoint: string): { host: string; port: number } { | ||
| private parseEndpoint(endpoint: string): { | ||
| host: string; | ||
| port: number; | ||
| secure?: boolean; | ||
| } { | ||
| let rest = endpoint; | ||
| let secure: true | undefined; | ||
| const schemeEnd = endpoint.indexOf("://"); | ||
| if (schemeEnd !== -1) { | ||
| const protocol = endpoint.slice(0, schemeEnd); | ||
| rest = endpoint.slice(schemeEnd + 3); | ||
| if (protocol === "memcaches") { | ||
| secure = true; | ||
| } | ||
| } | ||
|
|
||
| const parsed = this.parseHostPort(rest); | ||
| return secure ? { ...parsed, secure } : parsed; | ||
| } | ||
|
|
||
| private parseHostPort(endpoint: string): { host: string; port: number } { | ||
| // Handle IPv6 with brackets | ||
| if (endpoint.startsWith("[")) { | ||
| const bracketEnd = endpoint.indexOf("]"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { type CommandOptions, createNode, MemcacheNode } from "./node.js"; | |
| import { | ||
| type AutoDiscoverOptions, | ||
| type ClusterConfig, | ||
| type DiscoveredNode, | ||
| type ExecuteOptions, | ||
| type HashProvider, | ||
| MemcacheEvents, | ||
|
|
@@ -602,6 +603,13 @@ export class Memcache extends Hookified { | |
| cleanUri = protocolParts[1]; | ||
| } | ||
|
|
||
| // Unix path after a scheme, e.g. memcaches:///var/run/memcached.sock | ||
| if (cleanUri.startsWith("/")) { | ||
| return secure | ||
| ? { host: cleanUri, port: 0, secure } | ||
| : { host: cleanUri, port: 0 }; | ||
| } | ||
|
|
||
| // Handle IPv6 addresses with brackets [::1]:11211 | ||
| if (cleanUri.startsWith("[")) { | ||
| const bracketEnd = cleanUri.indexOf("]"); | ||
|
|
@@ -1643,6 +1651,7 @@ export class Memcache extends Hookified { | |
| keepAlive: this._keepAlive, | ||
| keepAliveDelay: this._keepAliveDelay, | ||
| sasl: this._sasl, | ||
| tls: this._tls !== undefined ? this._tls : this._nodes[0]?.tls, | ||
| }); | ||
|
|
||
| /* v8 ignore next -- @preserve */ | ||
|
|
@@ -1696,9 +1705,7 @@ export class Memcache extends Hookified { | |
| const id = AutoDiscovery.nodeId(node); | ||
| 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); | ||
|
Comment on lines
1706
to
+1708
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When TLS is inferred solely from a Useful? React with 👍 / 👎. |
||
| } catch (error) { | ||
| this.emit(MemcacheEvents.ERROR, id, error); | ||
| } | ||
|
|
@@ -1716,6 +1723,65 @@ export class Memcache extends Hookified { | |
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * TLS applied to newly discovered nodes: client-level option, else the | ||
| * auto-discovery config-endpoint option (`memcaches://` infers `true`). | ||
| */ | ||
| private get effectiveTls(): MemcacheTlsOption | undefined { | ||
| return this._tls ?? this._autoDiscovery?.tls; | ||
| } | ||
|
|
||
| /** | ||
| * Merge SNI (`servername`) into TLS options when discovery returns a DNS | ||
| * hostname plus an IP. Connecting to the IP keeps node IDs stable; SNI | ||
| * and certificate verification still use the hostname (required for | ||
| * ElastiCache in-transit encryption). | ||
| */ | ||
| private tlsOptionsForDiscoveredNode( | ||
| node: DiscoveredNode, | ||
| ): MemcacheTlsOption | undefined { | ||
| const tls = this.effectiveTls; | ||
| if (!tls) { | ||
| return tls; | ||
| } | ||
|
|
||
| const connectingHost = node.ip || node.hostname; | ||
| if ( | ||
| !node.hostname || | ||
| node.hostname === connectingHost || | ||
| node.hostname.includes(":") || | ||
| /^\d{1,3}(?:\.\d{1,3}){3}$/.test(node.hostname) | ||
| ) { | ||
| return tls; | ||
| } | ||
|
|
||
| const base = tls === true ? {} : { ...tls }; | ||
| if (base.servername) { | ||
| return tls; | ||
| } | ||
| return { ...base, servername: node.hostname }; | ||
| } | ||
|
|
||
| private async addDiscoveredNode(node: DiscoveredNode): Promise<void> { | ||
| const host = node.ip || node.hostname; | ||
| const tls = this.tlsOptionsForDiscoveredNode(node); | ||
| if (tls) { | ||
| await this.addNode( | ||
| new MemcacheNode(host, node.port, { | ||
| timeout: this._timeout, | ||
| keepAlive: this._keepAlive, | ||
| keepAliveDelay: this._keepAliveDelay, | ||
| sasl: this._sasl, | ||
| tls, | ||
| }), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const wrappedHost = host.includes(":") ? `[${host}]` : host; | ||
| await this.addNode(`${wrappedHost}:${node.port}`); | ||
| } | ||
| } | ||
|
|
||
| export { | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This claim is not supported for a SASL-protected configuration endpoint:
AutoDiscovery.fetchConfig()sends the ASCIIconfig get cluster/legacy command throughMemcacheNode.command(), while a SASL node requires the binary protocol and its permanent data handler explicitly skipshandleData()whenever_saslis 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 👍 / 👎.