Fix MQTT client: clear credentials and will on connection teardown

[EdouardMALOT]

Summary

The credentials and will pointers stored in the client (nxd_mqtt_client_username / password / will_topic / will_message, set by nxd_mqtt_client_login_set() and nxd_mqtt_client_will_message_set()) must be cleared on every connection teardown, error paths included. Today only the clean-disconnect path clears them, in _nxd_mqtt_process_disconnect(); the failed-connect teardowns (CONNACK refused, TCP or TLS establish failure) go through _nxd_mqtt_client_connection_end() and keep the stale pointers. If the application then reconfigures the same client for an anonymous connection (no new login_set() call) and its old buffers have been zeroed or reused, the next CONNECT goes out with the username flag set but no valid username. Mosquitto drops it as a protocol error (CONNECT with username flag but no username) and nxd_mqtt_client_secure_connect() fails with NXD_MQTT_COMMUNICATION_FAILURE (0x10007), on every retry, until the device reboots.

Observed during TLS/MQTT test-bench runs: connect attempts with credentials failed while the server certificate was temporarily invalid, the configuration was then switched back to an anonymous account, and the live client could never reconnect.

Changes

• addons/mqtt/nxd_mqtt_client.c
  • Move the username/password/will cleanup from _nxd_mqtt_process_disconnect() into _nxd_mqtt_client_connection_end(), so every teardown path resets it. _nxd_mqtt_process_disconnect() calls _nxd_mqtt_client_connection_end(), so the clean-disconnect behavior is unchanged.
  • Apply the same cleanup in the early TCP-connect-failure branch of _nxd_mqtt_client_connect(), the only teardown that does not go through _nxd_mqtt_client_connection_end(). Also clear nxd_mqtt_client_use_tls there: that branch has the same stale-flag problem that Fix MQTT client: reset use_tls flag in connection_end (TLS to plain reconnect) #400 fixes in _nxd_mqtt_client_connection_end(), on a path Fix MQTT client: reset use_tls flag in connection_end (TLS to plain reconnect) #400 does not cover. (The two PRs touch different hunks and merge independently.)

Behavioral note: credentials were already cleared on every clean disconnect by _nxd_mqtt_process_disconnect(), so the effective contract is already "call login_set() before each connect that needs credentials". This change only extends the existing cleanup to the failure paths.

Test plan

• Reproduced on mosquitto: after a failed connect with credentials, an anonymous reconnect on the same client sends CONNECT with the username flag and no username, and is dropped by the broker as a protocol error
• Audit of all teardown call sites: _nxd_mqtt_process_disconnect, _nxd_mqtt_process_connack error path, TCP/TLS establish-process failures, and the early TCP-failure branch of _nxd_mqtt_client_connect all reset the credentials with this change
• Full device-level regression on our gateway test bench with the patched library, including secure and plain connect/disconnect/reconnect cycles with credentials re-set before each connect (in progress)

[fdesbiens]

Thanks — the failure you hit is real and the write-up is clear. Before the concerns, two things in the change are straightforwardly good:

Moving the cleanup into _nxd_mqtt_client_connection_end() genuinely widens its coverage. _nxd_mqtt_process_disconnect() returns early at :1883-1888 when the state is neither CONNECTED nor CONNECTING, so the old cleanup at :1953 was already being skipped on that path. Putting it in connection_end fixes that too, which the PR description does not claim credit for.

Clearing only the pointers is sufficient, and I want to be explicit that it is not a bug. I checked whether the stale _length fields could cause a mismatched CONNECT, and they cannot: _nxd_mqtt_client_connect_packet_send() drives every flag off the pointer, not the length — if (client_ptr -> nxd_mqtt_client_username) for the username flag, if (client_ptr -> nxd_mqtt_client_will_topic) for the will flag. With the pointer NULL the flag is never set and the length is never read.

Now the substance. The reported scenario has a supported application-side fix. _nxde_mqtt_client_login_set() explicitly permits a NULL username with a zero length — the validation only rejects NULL-with-nonzero-length. So an application switching a live client from authenticated to anonymous can and should call:

nxd_mqtt_client_login_set(client_ptr, NX_NULL, 0, NX_NULL, 0);

That produces exactly the anonymous CONNECT you want, today, with no library change. Which reframes the PR: it is not repairing a defect the application cannot work around, it is changing the library's contract so that the application does not have to.

And that contract change has a cost you have not mentioned. Today the failed-connect paths preserve credentials, which means the common embedded pattern — call login_set() once at startup, then retry the connect on transient failure — works. After this change, the first failed attempt clears the credentials and every subsequent retry goes out anonymous and is refused. A broker that is briefly unreachable, or returns a transient CONNACK refusal, would leave the device permanently unable to connect until something calls login_set() again. That is the same shape of failure your PR is trying to eliminate, just reached from the other direction, and I would rather not trade one for the other without deciding deliberately.

In fairness to your position, the counter-evidence is decent: every existing test that uses credentials already re-sets them before each connect — netx_mqtt_connect_with_auth_will_test.c:263-273 calls both login_set() and will_message_set() inside its connect loop — so "set before each connect" does appear to be the understood contract, and no test in the suite would break. That is an argument for your change being the consistent behaviour. It is not an argument for it being a bug fix.

So my ask is: land this as a deliberate, documented contract change with a release-note entry, not as a silent fix. And see finding 3 — there is a narrower fix hiding in here that I think is strictly better for the will half.

Also, we need a regression test as usual.

[fdesbiens]

_nxd_mqtt_client_connection_end() is reached from every terminal path, including the ones that today leave credentials intact: the CONNACK-refused path via _nxd_mqtt_process_connack() (:1113), the TCP and TLS establish failures (:2280, :2314, :2337, :2430), and the four error exits inside _nxd_mqtt_client_connect() (:3789, :3827, :3845, :3860).

None of those go through _nxd_mqtt_process_disconnect(), so none of them clear credentials today. After this change all of them do.

The consequence for an application that calls login_set() once at startup and then retries on failure — which is the normal shape of an embedded MQTT client — is that the first transient failure strips its credentials and every retry afterwards is anonymous and refused. A broker restart, an expired-then-renewed certificate, or a CONNACK "server unavailable" would each brick the connection until something re-sets the credentials.

Note the asymmetry that makes this easy to miss: a clean disconnect already clears them today, so an application that disconnects and reconnects is already forced to re-set. An application that only ever retries after failure is not. This change closes that gap, which is defensible — but it is a change in the documented contract of nxd_mqtt_client_login_set(), not a defect being repaired, and it should be called out as such in the changelog so that anyone relying on the current failure-path behaviour finds out from release notes rather than from a field failure.

If you would rather keep the blast radius small, an alternative that fixes your scenario without touching the retry case: clear on the paths that represent a completed connection ending, and leave the pre-CONNACK connect failures alone. That is a narrower rule and matches "the connection existed and is now over" rather than "an attempt failed".

[fdesbiens]

This is a side effect of the move that is not mentioned in the PR description, and it is observable by applications.

In _nxd_mqtt_process_disconnect(), connection_end() is called at :1893, and the callbacks run afterwards at :1917-1927 — nxd_mqtt_disconnect_notify() and, on the connecting path, nxd_mqtt_connect_notify() with NXD_MQTT_CONNECT_FAILURE. The cleanup you are moving sat at :1953, i.e. after those callbacks.

So today a disconnect or connect-failure callback can still read client_ptr -> nxd_mqtt_client_username, nxd_mqtt_client_will_topic and friends. After this change they are all NULL by the time either callback fires. An application that logs which identity failed, or that inspects the will configuration to decide whether to re-arm, changes behaviour silently.

Please either preserve the ordering — keep the clear at the end of _nxd_mqtt_process_disconnect() and add it separately to the paths that need it — or state the new ordering explicitly in the documentation for both callbacks, so implementers know those fields are no longer valid inside them.

[fdesbiens]

This is the part of your change I would keep regardless of how finding 1 is resolved, and it is a stronger argument than the one in the PR description.

The credentials and the will are not symmetric. _nxde_mqtt_client_login_set() accepts a NULL username with zero length, so an application can clear credentials through the public API. _nxde_mqtt_client_will_message_set() does the opposite — it rejects will_topic == NX_NULL outright with NXD_MQTT_INVALID_PARAMETER, and there is no will_message_clear() anywhere in the add-on.

So once an application sets a will, it has no supported way to unset it on that client instance. Its only options are to rely on the library's implicit clear, or to delete and recreate the client. That is a genuine API gap, and it is a much better justification for library-side clearing than the credential case is.

Two ways to close it properly, either of which I would welcome as a follow-up or as a reshaping of this PR:

1. Relax _nxde_mqtt_client_will_message_set() to accept will_topic == NX_NULL with will_topic_length == 0, mirroring what login_set() already allows. Small, symmetric, and gives applications explicit control.
2. Add an explicit clear entry point, if allowing NULL through the setter is considered too subtle.

Either would make the will case an application decision rather than a library side effect, which is the same footing the credentials are already on.

[fdesbiens]

Worth recording the relationship explicitly so neither PR is merged thinking it is complete on its own.

This line is exactly the gap I raised on #400: that PR clears nxd_mqtt_client_use_tls in _nxd_mqtt_client_connection_end(), which misses this early TCP-connect-failure branch because it does its own hand-rolled teardown rather than calling connection_end. Your line here covers that branch. So #400 plus #402 together fix the stale-TLS-flag bug completely; either one alone leaves half of it.

The placement is right — inside the NX_SECURE_ENABLE guard, since the field only exists there, and after the conditional nx_secure_tls_session_delete() that reads it.

Two requests. First, since the two PRs are independent hunks, please make sure the commit message here mentions that it completes #400 rather than duplicating it, so the history is readable. Second, given how easy it was for a teardown path to be missed twice, it would be worth a follow-up that either routes this early-failure branch through connection_end() or documents why it deliberately does not — as I noted on #400, the two teardowns differ in three ways (no nx_secure_tls_session_end(), no nx_tcp_socket_disconnect(), unconditional timer delete), so a straight substitution is not safe today.

[fdesbiens]

To be clear up front: this is not a bug today. As noted in the summary, every CONNECT flag is driven off the pointer rather than the length, so nxd_mqtt_client_username_length, nxd_mqtt_client_password_length, nxd_mqtt_client_will_topic_length and nxd_mqtt_client_will_message_length are never read while their pointers are NULL.

But leaving them holding values that describe buffers the client no longer points at is a trap for whoever next touches _nxd_mqtt_client_connect_packet_send(). If anyone ever changes a flag test from if (pointer) to if (length) — a plausible tidy-up — the result is a CONNECT with the flag set and a NULL pointer handed to _nxd_mqtt_client_append_message().

Since you are already writing five assignments, zeroing the four lengths alongside them costs nothing and removes the trap. nxd_mqtt_client_will_qos_retain is already being cleared, so the block would then leave no partial state at all.