[C] Add protobuf implementation to C Command Samples

vaavva commented 1 year ago

Purpose

Adds serialization and deserialization of protobuf payloads in the C command samples using the protobuf-c library
Please ignore *.pb-c.* files, they are auto generated

Checklist

[x] I have read the contribution guidelines.
I submitted this PR against the correct branch:
- [x] This pull-request is submitted against the main branch.
- [x] I have merged the latest main branch prior to submission and re-merged as needed after I took any feedback.
- [x] I will squashed my changes into one with a clear description of the change when I complete the PR.

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

...

What to Check

Verify that the following are valid

...

Other Information

rido-min commented 1 year ago

the door lock example, as shown, is dangerous and has multiple (physical) security consequences (e.g. opening the door a few hours after the request was made is not a good thing).

This is a sample to show the RPC pattern in action, it's not intended to be the ultimate reference to implement a safe unlock mechanism for production grade vehicle manufacturers. It's not only about the time sync, also about AuthN/AuthZ etc..

We inherit the scenario description from EG, and I believe it's a good sample, easy to understand and with a clear purpose.

Do you have a better suggestion?

the request isn't expired - door should not unlock if the original time was more than 5 seconds ago (requestor app should clearly wait for 5 seconds to indicate that there is a possibility that the door can open).

AFAIK we are not using persistent sessions in this sample, so the client expects the server to be online when the command is invoked.

In which case the command will be executed at a later time? do you have a repro?

CIPop commented 1 year ago

Do you have a better suggestion?

Yes - I've left suggestions in throughout my comments about these issues. We should at least clearly explain the constraints / dangers.

AFAIK we are not using persistent sessions in this sample, so the client expects the server to be online when the command is invoked.

That would indeed solve the issue when the RPC server is offline but not the case when the MQTT broker is either busy, disconnected from the network or shut down for any reasons (crash, maintenance, etc).

In which case the command will be executed at a later time? do you have a repro?

There are (at least) two cases: broker failure and server failure:

Broker failure

Start the broker with persistence for QoS1.
Start the client and server.
Client makes a request.
Place a breakpoint on the broker ACK (which means that the message was persisted).
Kill the broker process or disconnect it from the network.
After some time (minutes, hours, etc) resume normal broker operation.
Observe the QoS 1 message being now sent to the serverRPC.

Note that if messageExpiry is configured, depending on broker timer implementation, only killing the broker process would result in the message being transmitted later.

Server failure

In this case, the server receives the message but it either:

crashes right after receipt and before ACK.
has a half-opened (receive only) connection (and so ACKs cannot be sent back to the broker).

In case 2, depending on time to recover, the server may connect at a later time, receiving the (delayed) unack-ed QoS1 request. In case 1, the client will eventually be disconnected (and should disconnect itself) after keepalive failure. On re-connect (i.e. when network is connected both ways), the QoS1 message will be delivered again (delayed).

vaavva commented 1 year ago

@CIPop @rido-min, I think a lot of the concerns brought up here apply to all of the command samples and not just the C sample. I'll open up a task on our board to discuss and decide what pattern to align on for all of the languages, to be addressed in a future PR. For now, the C sample has parity with the other languages in regards to security, and I've added a disclaimer in the readme > NOTE: This code is a basic example of the request-response messaging pattern. It is not a secure solution for unlocking a vehicle without further security considerations.

rido-min commented 1 year ago

My preference for generated code is to have it generated pre-build: that way it can't ever run out of sync with the checked-in definitions.

Agree, however we have not been able to find a way to call the proto generator from CMake, and a result I logged #63

As a workaround, I think it might be ok to commit the generated files.

/c @vaavva

Azure-Samples / MqttApplicationSamples