mqtt_cpp icon indicating copy to clipboard operation
mqtt_cpp copied to clipboard

Protocol Error checking

Open redboltz opened this issue 4 years ago • 3 comments

There are many protocol errors in MQTT spec. mqtt_cpp doesn't check them enough. Some of check requires extra cost. For example, some of properties cannot appear in some MQTT control packet. In order to check it, I need to count them.

My idea is separate receiving and sending. And sending is relatively lower priority because It is application error. I think that mqtt_cpp should provide sending protocol error checker as optional feature.

Receiving is more important. Because the connection partner could send protocol error MQTT control packet. I think that protocol error can be checked at process_* functions. It doesn't require extra parse loop because it is only place to parse MQTT control packet. Maybe adding some protocol_checker class and passed it to process_* functions as a parameter. protocol_checker has state and it is updated by step by step for each element processed. When error condition is satisfied, then the connection partner is disconnected and the endpoint got on_error handler call.

redboltz avatar Dec 13 '20 09:12 redboltz

Maybe pass a list of protocol checkers, so you can have multiple protocol checks running ? Otherwise you create a protocol_checker which takes a list of protocol_checkers ..

And I think resending publish/pubrec/pubcomp can be a protocol check as well? I would not mind disconnecting these rogue clients.

kleunen avatar Dec 13 '20 09:12 kleunen

My image is as follows:

MQTT control packet
+----------+----------+----------+----------+----------+
|          |          |          |          |          |
+----------+----------+----------+----------+----------+
process_a   process_b  process_c  process_d  process_e

protocol_checker checker { publish }; // set MQTT control packet type.

void process_a(..., protocol_checker& checker) {
    if (!checker.some_flag(...)) {
        // protocol_error. DISCONNECT and call on_error()
    }
    ....
    process_b(checker); // pass the checker to the next process

}

When the control packet type is decided, then just create for the checker for that type.

I didn't consider yet about abstraction.

redboltz avatar Dec 13 '20 09:12 redboltz

I think that protocol checking on individual MQTT control packet and a series of send/receive of MQTT control packet should be separated. My first target is former. The lifetime of the checker ends on individual packet parsing is finished.

The latter is difficult to implement. Maybe endpoint needs to have state machine. And need to check the combination of send/receive packet. Especially the client uses async_ APIs, API call timing and actual sending timing is different. If we implement it invalidly, the connection is disconnected by false positive check. So I postpone it.

redboltz avatar Dec 13 '20 09:12 redboltz