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

Signing #253

Merged
merged 11 commits into from
Aug 26, 2024
Merged

Signing #253

merged 11 commits into from
Aug 26, 2024

Conversation

pv42
Copy link
Contributor

@pv42 pv42 commented Aug 11, 2024

As pointed out in #82 MAVLink 2 message signing is not supported beyond being able to correctly parse signed messages.
This PR adds full support for signature creation and verification.
This is done by introducing the signing feature to the mavlink and mavlink-core crates.
The implementation is done in a way that the public interfaces remain unchanged without the feature and are semver compatible when it is enabled.

Additions

  • signing feature for mavlink-core and as passthrough to mavlink, all following additions require this, this is not enabled by default
  • add SigningConfig to configure signing with a secret key, enable sign outgoing and allow unsigned messages
  • add SigningData to execute signing with verify_signature(), sign_message()
  • MavConnection::setup_signing(..) to all connection implementations
  • add signing versions of pub fn that have an additional Option<&SigningData> paramter: read_versioned_msg_signed(), read_v2_raw_message_signed(), read_v2_msg_signed(), write_versioned_msg_signed(), write_v2_msg_signed()
  • add byte access function to MAVLinkV2MessageRaw: checksum_bytes(), signature_link_id[_mut](), signature_timestamp[_bytes[_mut]](), signature_value[_mut]()
  • fix MAVLinkV2MessageRaw::header() requiring a mut reference
  • add MAVLinkV2MessageRaw::calculate_signature() that calculates a messages current signature
  • add MAVLinkV2MessageRaw::serialize_message_for_signing() to serialze while setting the incompat_flag

Signing specifications

With the signing feature enabled and signing setup the following applies:

  • All incoming messages with invalid signature are ignored
  • All incoming messages that are not signed are ignored unless allow_unsigned is set
  • All outgoing message are signed when sign_outgoing is set.
  • If the message signature does not match the message is ignored.
  • If a messages timestamp is more then 60s older then the newest received message it is ignored
  • If a messages timestamp is older then the previous message from the same (system,component,link) it is ignored.
  • Timestamps are strictly monotonic increasing per send message allowing it get ahead of real time.

Tests

  • add signing test to verify the signing itself
  • add signing to tcp_loopback_tests to check the connection signing implementation
  • enable signing feature for all test

Caveats

  • The mavlink standart calls for a method to allow individuall unsigned messages, this currently has only a simple boolean flags. A better solution is to either allow messages ids based on a whitelist or a callback.
  • The link id used is fixed to 0. The standart describes that this should be different for each connection from this device, however this is difficult to implement and I could not come up with a satisfying solution.
  • The timestamps required for signing are generated using std::SystemTime::now() which might not exist in a none-std enviroment, maybe the signing feature should depend on std.
  • This PR suffers from the same issue as Mavlink v2 extension are broken for v0.13 for the mavlink crate #246 as test on mavlink are broken since they depend on the published version of mavlink-core which does not have signing, when testing locally with a path dependency they succeed.
  • The signing feature uses the incompat_flags field of the MAVLink header, for the purpose of preserving API stability the coresponding struct MavHeader is not modified but this should probably be done in a future breaking version.
  • It is possible to enable signing on a MavConnection and then set a it to use MAVLink 1 only which ignores the setup signing configuration.

@patrickelectric
Copy link
Member

Can you rebase over master ?

@pv42 pv42 marked this pull request as draft August 22, 2024 15:59
@pv42
Copy link
Contributor Author

pv42 commented Aug 22, 2024

I have fixed the merge conflicts for this, but I should probably add signing variant of the new async methods too. Hence I marked this a draft until I have implemented this.

@pv42
Copy link
Contributor Author

pv42 commented Aug 22, 2024

I have added read_v2_raw_message_async_signed, added link_id to signing setup and slightly modified tests.

@pv42 pv42 marked this pull request as ready for review August 22, 2024 20:55
@joaoantoniocardoso
Copy link
Contributor

Nice work, but I think you've merged the updated master into this branch instead of rebasing it over master

.github/workflows/test.yml Outdated Show resolved Hide resolved
mavlink-core/src/connection/udp.rs Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
…kV2MessageRaw

add calculate_signature, checksum_bytes, signature_link_id[_mut], signature_timestamp[_bytes[_mut]], signature_value[_mut] to MAVLinkV2MessageRaw
feat: add distinct pub fn for signing, signing in MavConnection send, add signing feature to mavlink
test: add --features signing to all test, add signing test, add Debug, Clone to SigningConfig
test: add signing to msrv check
test: add signing to internal-tests matrix
test: add signing as msrv matrix option
@patrickelectric patrickelectric merged commit e345857 into mavlink:master Aug 26, 2024
40 checks passed
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.

3 participants