Skip to content
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

[C] Add protobuf implementation to C Command Samples #57

Merged
merged 37 commits into from
Jul 6, 2023
Merged

Conversation

vaavva
Copy link
Member

@vaavva vaavva commented Jun 27, 2023

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

  • I have read the contribution guidelines.
  • I submitted this PR against the correct branch:
    • This pull-request is submitted against the main branch.
    • I have merged the latest main branch prior to submission and re-merged as needed after I took any feedback.
    • 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

@vaavva vaavva requested review from RLeclair and ericwolz June 27, 2023 22:30
Copy link

@RLeclair RLeclair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

scenarios/command/c/command_client/main.c Outdated Show resolved Hide resolved
scenarios/command/c/command_client/main.c Show resolved Hide resolved
scenarios/command/unlock_command.proto Outdated Show resolved Hide resolved
Copy link
Member

@CIPop CIPop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the following observations:

  • 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).
  • 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.
    Furthermore, the generated code is not really meant to be human-readable or reviewed, not copyrighted properly, etc.

(Other nitpicks - safe to ignore)

mqttclients/c/README.md Show resolved Hide resolved
mqttclients/c/mosquitto_client_extensions/mqtt_callbacks.c Outdated Show resolved Hide resolved
scenarios/command/c/command_client/main.c Show resolved Hide resolved
scenarios/command/c/command_server/main.c Show resolved Hide resolved
scenarios/command/c/command_client/main.c Outdated Show resolved Hide resolved
@rido-min
Copy link
Contributor

  • 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
Copy link
Member

CIPop commented Jun 29, 2023

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

  1. Start the broker with persistence for QoS1.
  2. Start the client and server.
  3. Client makes a request.
  4. Place a breakpoint on the broker ACK (which means that the message was persisted).
  5. Kill the broker process or disconnect it from the network.
  6. After some time (minutes, hours, etc) resume normal broker operation.
  7. 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:

  1. crashes right after receipt and before ACK.
  2. 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
Copy link
Member Author

vaavva commented Jun 30, 2023

@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.

@vaavva vaavva merged commit a3bfb65 into main Jul 6, 2023
@vaavva vaavva deleted the vaavva/c-protobuf branch July 6, 2023 17:11
@rido-min
Copy link
Contributor

rido-min commented Sep 8, 2023

  • 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants