[Feature suggestion] Limit ClientOptions visibility to prevent zero-value KeepAlive problems (v1.3.5 -> mosquitto 2.0.12)

[ebudan]

The documentation for ClientOptions gently nudges toward using setters, but it doesn't enforce.
I recently ran into the following scenario:

• 3rd party code constructed ClientOptions{...} explicitly (paho.mqtt.golang 1.3.5), leaving KeepAlive zero
• code ran against Mosquitto 1.6.x apparently fine
• code failed against Mosquitto 2.0.12 with the following broker-side message:

1633594305: New connection from ::1:33582 on port 1883.
1633594305: New client connected from ::1:33582 as test (p2, c0, k0).
1633594305: Bad socket read/write on client test: Invalid arguments provided.

The k0 output wasn't obvious to me, but comparing to a successful connection with 'k30' it was enough to let me look that up in the mosquitto source, then test the code with KeepAlive set to 30, connecting successfully.

Would it be an option to make ClientOptions private to prevent uninitialized usage? If not, should its documentation perhaps emphasize using NewClientOptions() in a stronger tone?

[MattBrittan]

This issue arises due to a change in Mosquitto 2.0.12 to address CVE-2020-13849. From the readme:

Fix max_keepalive not applying to MQTT v3.1.1 and v3.1 connections. These clients are now rejected if their keepalive value exceeds max_keepalive. This option allows CVE-2020-13849, which is for the MQTT v3.1.1 protocol itself rather than an implementation, to be addressed.

This appears to be a breaking change because max_keepalive defaults to 65535 meaning that a keep alive of 0 is rejected by default (which was previously accepted).

NewClientOptions sets KeepAlive to 30 by default (meaning a default of 30 seconds). However, as you say, ClientOptions is public so it's possible for users to create this themselves (which will result in a default of 0). Unfortunately the error message is fairly generic so unlikely to highlight the issue.

I don't think that making ClientOptions private is a good option; working with private variables is painful (users would be unable to store the config locally; would need a more convoluted structure) and this would be a breaking change. Adding a stronger warning is definitely an option.

[MattBrittan]

I have merged PR #546 to add comments pointing out the issues possible through creating ClientOptions manually.

[MattBrittan]

Closing this issue. I believe that the comments added address the issue (the alternative, making ClientOptions public, is not something I'm keen on).

[ebudan]

Thank you, those comments should help the casual user stay on track.