RAS-A Signed Packet Support

[stuart-auterion]

I expect this one to be controversial... but here goes. QGC-Gov is required to be RAS-A compliant. This means we should support the standard as written, and as "THE" RAS-A ground station, we need to include even the optional parts of RAS-A. I understand there are differing opinions about the usefulness of the signed packets portions of RAS-A, but regardless it is an official part of the standard. Since we are using libmav to handle all our low-level MAVlink packet encoding/decoding, we need libmav to support this.

Happy to provide details on or discuss design choices here if required. I can also walk through testing steps used to validate this. Or happy to discuss any of the above in more detail.

[ThomasDebrunner]

Thanks, looks fine

• I understand this doesn't allow for actually checking signatures of incoming messages?
• When you set "sign" to true in "finalize", it doesn't actually sign the message? This and the key should just be part of "finalize", otherwise the function loses its meaning, and you potentially end up with a bad state
• Can you add unit tests for all of this?

[stuart-auterion]

• Message::validate() performs a check on the signature of incoming messages.
• Yeah, I definitely see your point. I was interpreting "finalize" as "make the message header". And since the header is a different part of the message than the signature, it made sense to me to do the signing separately. Also, if we're doing the signing in finalize(), I'd need to pass both the timestamp and key in as arguments, which seems like a rather cumbersome amount of args for one function. But its feasible. Would you prefer that?
• I can indeed! I'll look into it!

[stuart-auterion]

Hope you don't mind, I also added a note to the readme about how to run the tests. I've never used doctest before, and took me a minute to figure out how to actually do it, so hopefully this can save someone else time in the future.

[ThomasDebrunner]

If you have signing enabled on a NetworkRuntime, would ever not want to validate the incoming messages? In case it is enabled, we should always validate and reject bad signatures.

[stuart-auterion]

If you have signing enabled on a NetworkRuntime, would ever not want to validate the incoming messages? In case it is enabled, we should always validate and reject bad signatures.

Depends. The standard is inspecific. To quote RAS-A:

Accepting Unsigned Packets

MAVLink libraries should provide a mechanism that allows a system to conditionally accept unsigned packets.

...

Accepting Incorrectly Signed Packets

MAVLink libraries should provide a mechanism that allows a system to conditionally accept incorrectly signed packets.

So the rejection needs to be done at the application level and it is not a decision libmav can make. You could make an argument that libmav should always at least try to validate? We could perhaps have libmav always do the calculation and then set a field on the message if invalid, and then the application could choose to check the validity field if it wants.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
49 Code Smells

No Coverage information
0.0% Duplication

[stuart-auterion]

@ThomasDebrunner Is there some way you can configure SonarCloud to ignore include/mav/picosha/*? I updated gcovr's config to ignore it in the coverage report, but SonarCloud still sees it as a changed file and reports 0%, blocking this PR.

[ThomasDebrunner]

@stuart-auterion Just rebase this to main - let's see what sonarcloud says. I guess it should be possible to exclude picohash. I think it looks good outside of that

[stuart-auterion]

Hmmm, looks like its still tracking coverage on picosha.

[jacobbandyk]

Looks good to me! Thanks for adding the docs on running the unit tests as well.

[jacobbandyk]

Hmmm, looks like its still tracking coverage on picosha.

Although, this is still an issue.

[ThomasDebrunner]

Yeah, I asked Jakob, maybe he knows why

[stuart-auterion]

Aw bummer. Looks like I'm still 0.8% short on new code coverage. I'll address that when I can.

Thanks for fixing sonarcloud!

[stuart-auterion]

@ThomasDebrunner should be ready now.
Thanks for the patience and help getting this PR in a good state.

[ThomasDebrunner]

Merged, thanks @stuart-auterion