-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(mqtt5): implement re-authentication and handle auth success #2048
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
base: main
Are you sure you want to change the base?
Changes from all commits
c305b81
689abba
682e53f
0ae96d0
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 |
|---|---|---|
|
|
@@ -27,8 +27,8 @@ import DefaultMessageIdProvider, { | |
| } from './default-message-id-provider' | ||
| import TopicAliasRecv from './topic-alias-recv' | ||
| import { | ||
| ErrorWithReasonCode, | ||
| type DoneCallback, | ||
| type ErrorWithReasonCode, | ||
| ErrorWithSubackPacket, | ||
| type GenericCallback, | ||
| type IStream, | ||
|
|
@@ -433,6 +433,7 @@ export interface MqttClientEventCallbacks { | |
| reconnect: VoidCallback | ||
| offline: VoidCallback | ||
| outgoingEmpty: VoidCallback | ||
| reauth: OnPacketCallback | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -521,6 +522,9 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac | |
|
|
||
| private connackPacket: IConnackPacket | ||
|
|
||
| /* @type {PacketCallback} - callback receives IAuthPacket */ | ||
| private _reauthCallback: PacketCallback = null | ||
|
|
||
| public static defaultId() { | ||
| return `mqttjs_${Math.random().toString(16).substr(2, 8)}` | ||
| } | ||
|
|
@@ -1678,6 +1682,116 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac | |
| return this | ||
| } | ||
|
|
||
| /** | ||
| * reauthenticateAsync - MQTT 5.0 Re-authentication (Promise-based) | ||
| * @param {Object} reauthOptions - Re-authentication properties | ||
| * @param {Buffer} [reauthOptions.authenticationData] - Binary data for auth exchange | ||
| * @param {string} [reauthOptions.reasonString] - Human-readable reason for re-auth | ||
| * @param {Object} [reauthOptions.userProperties] - Custom user properties | ||
| * @returns {Promise<IAuthPacket>} - Resolves with the AUTH packet from the broker | ||
| */ | ||
| public reauthenticateAsync( | ||
| reauthOptions: Pick< | ||
| NonNullable<IAuthPacket['properties']>, | ||
| 'authenticationData' | 'reasonString' | 'userProperties' | ||
| >, | ||
| ): Promise<IAuthPacket> { | ||
| return new Promise((resolve, reject) => { | ||
| this.reauthenticate(reauthOptions, (err, packet) => { | ||
| if (err) { | ||
| reject(err) | ||
| } else { | ||
| resolve(packet as IAuthPacket) | ||
|
Member
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. [Style]
|
||
| } | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * reauthenticate - MQTT 5.0 Re-authentication | ||
| * @param {Object} reauthOptions - Re-authentication properties | ||
| * @param {Buffer} [reauthOptions.authenticationData] - Binary data for auth exchange | ||
|
Member
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. [Doc] JSDoc says * @param {Buffer} [reauthOptions.authenticationData] - Binary data for auth exchangeThe square brackets in JSDoc denote an optional parameter, but |
||
| * @param {string} [reauthOptions.reasonString] - Human-readable reason for re-auth | ||
| * @param {Object} [reauthOptions.userProperties] - Custom user properties | ||
| * @param {PacketCallback} [callback] - Fired when the AUTH exchange completes or fails | ||
| * @returns {MqttClient} - Returns the client instance | ||
| */ | ||
| public reauthenticate( | ||
| reauthOptions: Pick< | ||
| NonNullable<IAuthPacket['properties']>, | ||
| 'authenticationData' | 'reasonString' | 'userProperties' | ||
| >, | ||
| callback?: PacketCallback, | ||
| ): MqttClient { | ||
| const fail = (msg: string) => { | ||
| const err = new Error(`reauthenticate: ${msg}`) | ||
| if (callback) callback(err) | ||
| else this.emit('error', err) | ||
| return this | ||
| } | ||
|
|
||
| if (this._reauthCallback) { | ||
|
Member
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. [High] The interrupt fires before the new request is validated. if (this._reauthCallback) {
this._handleReauth(new Error('reauthenticate: interrupted by new reauthentication request'))
}
if (this.options.protocolVersion !== 5) return fail(...)
else if (!this.connected) return fail(...)
else if (!reauthOptions.authenticationData) return fail(...)A second call with invalid arguments (wrong protocol version, missing data, etc.) will still cancel a perfectly valid in-flight re-auth before its own validation runs. The first caller is rejected with Move the validation above the interrupt branch so that interruption only happens when the new request will actually go on the wire. |
||
| this._handleReauth( | ||
| new Error( | ||
| 'reauthenticate: interrupted by new reauthentication request', | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| if (this.options.protocolVersion !== 5) | ||
| return fail('this feature works only with mqtt-v5') | ||
| else if (!this.connected) return fail('client is not connected') | ||
| else if (!reauthOptions.authenticationData) | ||
|
Member
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. [Medium] Null-safety on
|
||
| return fail('authenticationData is required') | ||
|
|
||
| const method = this.options.properties?.authenticationMethod | ||
|
|
||
| if (!method) | ||
| return fail('authenticationMethod is required from initial CONNECT') | ||
|
|
||
| const authPacket: IAuthPacket = { | ||
| cmd: 'auth', | ||
| reasonCode: 0x19, // Re-authentication (MQTT 5.0 Spec) | ||
| properties: { | ||
| ...reauthOptions, | ||
| authenticationMethod: method, | ||
| }, | ||
| } | ||
|
|
||
| this._reauthCallback = callback | ||
| this._sendPacket(authPacket) | ||
|
Member
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. [Medium] Two related issues here:
|
||
| return this | ||
| } | ||
|
|
||
| /** | ||
| * _handleReauth | ||
| * Internal method to finalize the re-authentication process. | ||
| * Clears the pending reauth callback and signals completion via callback or event. | ||
| * @param {Error | null} err - The error if the re-authentication failed | ||
| * @param {IAuthPacket} [packet] - The AUTH packet received from the broker | ||
| * @api private | ||
| */ | ||
| private _handleReauth(err: Error | null, packet?: IAuthPacket) { | ||
| if (!err && packet && packet.reasonCode !== 0) { | ||
|
Member
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. [High — companion to the auth.ts issue] if (!err && packet && packet.reasonCode !== 0) {
err = new ErrorWithReasonCode(`Re-auth failed: ${packet.reasonCode}`, packet.reasonCode)
}This is the wrong abstraction layer for that decision — |
||
| err = new ErrorWithReasonCode( | ||
| `Re-auth failed: ${packet.reasonCode}`, | ||
| packet.reasonCode, | ||
| ) | ||
| } | ||
|
|
||
| if (this._reauthCallback) { | ||
| const cb = this._reauthCallback | ||
| this._reauthCallback = null | ||
| cb(err, packet) | ||
| } else if (err) { | ||
| this.emit('error', err) | ||
|
Member
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. [Medium — availability/security] If no That means a remote MQTT broker — which is not necessarily trusted — can trigger a process exit purely by sending an unexpected AUTH packet after CONNACK. At minimum, swallow-and-log when there is no listener, and ideally don't classify broker-initiated AUTH as an error at all (see the auth-handler thread). |
||
| } | ||
|
|
||
| if (!err && packet) { | ||
| this.emit('reauth', packet) | ||
| } | ||
| } | ||
|
robertsLando marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * PRIVATE METHODS | ||
| * ===================== | ||
|
|
@@ -1864,6 +1978,10 @@ export default class MqttClient extends TypedEventEmitter<MqttClientEventCallbac | |
| }) | ||
| } | ||
|
|
||
| if (this._reauthCallback) { | ||
| this._handleReauth(new Error('client disconnected')) | ||
|
Member
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. [Minor] Re-auth cleanup placement in The call is fine, but consider whether |
||
| } | ||
|
|
||
| if (!this.disconnecting && !this.reconnecting) { | ||
| this.log( | ||
| '_cleanUp :: client not disconnecting/reconnecting. Clearing and resetting reconnect.', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,11 @@ const handleAuth: PacketHandler = ( | |
| return | ||
| } | ||
|
|
||
| if (client.connected) { | ||
| client['_handleReauth'](null, packet) | ||
|
Member
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. [High — protocol bug] Multi-step / broker-initiated AUTH is broken. This branch routes every AUTH packet received while
Downstream, The handler needs to dispatch by reason code: switch (packet.reasonCode) {
case 0x00: return client._handleReauth(null, packet) // complete
case 0x18: return client.handleAuth(packet, …) // continue — let user respond
case 0x19: return client.handleAuth(packet, …) // broker-initiated
default: return client._handleReauth(new ErrorWithReasonCode(…), packet)
}This is the most important issue in the PR — it silently regresses the multi-step auth flow that the existing
Author
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. I’ve refactored the auth.ts handler to use a switch on the reason codes to properly handle the multi-step handshake (0x18). Regarding the 0x19 (Re-authenticate) flow: I noticed Table 3.15.2.1 Authenticate Reason Code lists 0x19 as 'Sent by: Client' only, and §4.12.1 describes it as a client-initiated action. Is there a specific scenario or broker behavior where the server initiates with 0x19, or should we handle a server-sent 0x19 as a protocol error?
Member
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. [Style] Bracket-access to bypass
As written, every refactor of |
||
| return | ||
| } | ||
|
|
||
| client.handleAuth( | ||
| packet, | ||
| (err: ErrorWithReasonCode, packet2: IAuthPacket) => { | ||
|
|
||
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.
[Style] Type and JSDoc nits on the field declaration.
/* … */(single asterisk) is not parsed as JSDoc — this comment is invisible to tooling. Use/** … */if you want the annotation, or just delete it (the TS type already documents it).PacketCallback | null. It compiles today only becausestrict: falseintsconfig.json, but the project has been steadily tightening — this will trip the momentstrictNullChecksis enabled.