fix(signing): defer local signing until vehicle confirms via signature verification#14268
fix(signing): defer local signing until vehicle confirms via signature verification#14268HTRamsey wants to merge 1 commit intomavlink:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates QGroundControl’s MAVLink signing enable/disable flow to avoid communication loss by deferring local signing state changes until the vehicle provides cryptographic proof (or unsigned proof for disable), and fixes signature verification CRC handling. It also expands MockLink/test coverage to exercise the new confirmation flow, and includes additional unrelated onboard log UI/controller refactors.
Changes:
- Defer local signing enable/disable until confirmation is observed (signed message verifies against pending key; disable confirmed by unsigned heartbeat), with a 3s timeout + user notification.
- Fix
MAVLinkSigning::verifySignature()to hash the parsed-message CRC frommessage.ck[]instead of reading past payload. - Update MockLink to actually sign outgoing messages when signing is enabled; expand signing tests (including a timeout test).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MAVLink/MockLinkSigningTest.h | Adds a new signing enable timeout test slot. |
| test/MAVLink/MockLinkSigningTest.cc | Updates signing tests for deferred confirmation; adds timeout scenario. |
| src/Vehicle/Vehicle.h | Adds pending signing operation state + confirmation timer members. |
| src/Vehicle/Vehicle.cc | Implements deferred signing confirmation logic, timeout handling, and temporary accept-unsigned behavior during disable. |
| src/MAVLink/MAVLinkSigning.h | Adds pending-disable accept-unsigned callback and callback setter API. |
| src/MAVLink/MAVLinkSigning.cc | Implements pending-disable accept-unsigned set and fixes signature verification CRC source. |
| src/Comms/MockLink/MockLink.cc | Enables/disables real MAVLink channel signing in MockLink when SETUP_SIGNING is received. |
| src/AnalyzeView/OnboardLogs/OnboardLogPage.qml | Avoids refresh on completion when no active vehicle; adds “select all” checkbox behavior. |
| src/AnalyzeView/OnboardLogs/OnboardLogEntry.cc | Re-enables debug logging in ctors/dtors. |
| src/AnalyzeView/OnboardLogs/OnboardLogController.h | Removes unused compression API; adds retry constants and helper declaration. |
| src/AnalyzeView/OnboardLogs/OnboardLogController.cc | Refactors download error handling/retries and factors MAVLink send boilerplate into a helper. |
Comments suppressed due to low confidence (1)
src/AnalyzeView/OnboardLogs/OnboardLogController.cc:66
- The PR description and title focus on signing confirmation, but this PR also makes substantial onboard log UI/controller changes. This increases review/rollback risk and makes it harder to reason about the signing fix. Consider splitting the onboard log work into a separate PR or explicitly documenting the additional scope in the PR description.
void OnboardLogController::_downloadToDirectory(const QString &dir)
{
_receivedAllEntries();
_downloadData.reset();
_downloadPath = dir;
if (_downloadPath.isEmpty()) {
return;
}
if (!_downloadPath.endsWith(QDir::separator())) {
_downloadPath += QDir::separator();
}
if (!QDir().mkpath(_downloadPath)) {
qCWarning(OnboardLogControllerLog) << "Failed to create download directory:" << _downloadPath;
return;
}
_setDownloading(true);
_receivedAllData();
}
| if (_pendingSigningOp == PendingSigningOp::Enable) { | ||
| if (!MAVLinkSigning::initSigning(_pendingSigningChannel, _pendingSigningKeyBytes, MAVLinkSigning::insecureConnectionAcceptUnsignedCallback)) { | ||
| qCCritical(VehicleLog) << "Failed to initialize signing on channel" << _pendingSigningChannel; | ||
| } else { | ||
| _mavlinkSigning = true; | ||
| _mavlinkSigningKeyName = _pendingSigningKeyName; | ||
| emit mavlinkSigningChanged(); | ||
| qCDebug(VehicleLog) << "Signing confirmed by vehicle, key:" << _mavlinkSigningKeyName; | ||
| } |
There was a problem hiding this comment.
If MAVLinkSigning::initSigning fails during enable confirmation, the code only logs and then clears the pending op without notifying the user. This leaves the UI thinking enable is still possible but provides no feedback about why confirmation failed. Consider treating this as a failure path: cancel the pending op, restore any temporary state, and show an app message.
|
@HTRamsey let me know when this is ready for testing, and I can run a few tests with hardware |
|
Okay will do. FYI this is based on yours but just made a separate PR with some changes to make things quick and simple |
Build ResultsPlatform Status
All builds passed. Pre-commit
Pre-commit hooks: 4 passed, 36 failed, 7 skipped. Test Resultslinux-coverage: 78 passed, 0 skipped Code CoverageCoverage: 58.2% No baseline available for comparison Artifact Sizes
Updated: 2026-04-10 15:20:50 UTC • Triggered by: Android |
HTRamsey
force-pushed
the
fix/signing-deferred-confirmation
branch
from
14781f6 to
f5992cc
Compare
|
Cool, yeah, the biggest problem is that QGC could get out of sync with PX4. Once we figure that out, we should be good! Additionally, at some point, it would be nice to add the option of loading a custom private key instead of generating one, and give users both options. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #14268 +/- ##
=========================================
Coverage ? 25.61%
=========================================
Files ? 753
Lines ? 68221
Branches ? 31638
=========================================
Hits ? 17478
Misses ? 38044
Partials ? 12699
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
HTRamsey
force-pushed
the
fix/signing-deferred-confirmation
branch
from
f5992cc to
c763fd4
Compare
|
@mrpollo should be good to go |
| emit mavlinkSigningChanged(); | ||
| qCDebug(VehicleLog) << "Signing confirmed, key:" << _mavlinkSigningKeyName; | ||
| } | ||
| } else if (_pendingSigningOp == PendingSigningOp::Disable) { | ||
| MAVLinkSigning::initSigning(_pendingSigningChannel, QByteArrayView(), nullptr); | ||
| _mavlinkSigning = false; | ||
| _mavlinkSigningKeyName.clear(); | ||
| emit mavlinkSigningChanged(); | ||
| qCDebug(VehicleLog) << "Signing disable confirmed"; | ||
| } | ||
|
|
||
| _clearPendingSigningState(); |
There was a problem hiding this comment.
_confirmSigning() emits mavlinkSigningChanged() before _clearPendingSigningState(). Since mavlinkSigningPending is derived from _pendingSigningOp, listeners (QML/UI/tests) may observe the signal while the object is still “pending” and never get another notification when pending clears. Clear the pending state before emitting, or emit again after clearing so the UI reliably transitions out of the pending state on confirmation.
| emit mavlinkSigningChanged(); | |
| qCDebug(VehicleLog) << "Signing confirmed, key:" << _mavlinkSigningKeyName; | |
| } | |
| } else if (_pendingSigningOp == PendingSigningOp::Disable) { | |
| MAVLinkSigning::initSigning(_pendingSigningChannel, QByteArrayView(), nullptr); | |
| _mavlinkSigning = false; | |
| _mavlinkSigningKeyName.clear(); | |
| emit mavlinkSigningChanged(); | |
| qCDebug(VehicleLog) << "Signing disable confirmed"; | |
| } | |
| _clearPendingSigningState(); | |
| qCDebug(VehicleLog) << "Signing confirmed, key:" << _mavlinkSigningKeyName; | |
| } | |
| } else if (_pendingSigningOp == PendingSigningOp::Disable) { | |
| MAVLinkSigning::initSigning(_pendingSigningChannel, QByteArrayView(), nullptr); | |
| _mavlinkSigning = false; | |
| _mavlinkSigningKeyName.clear(); | |
| qCDebug(VehicleLog) << "Signing disable confirmed"; | |
| } | |
| _clearPendingSigningState(); | |
| emit mavlinkSigningChanged(); |
| // Channel was captured at send time; link failover handled by tryDetectKey | ||
| if (_pendingSigningOp == PendingSigningOp::Enable) { | ||
| if (!MAVLinkSigning::initSigning(_pendingSigningChannel, _pendingSigningKeyBytes, | ||
| MAVLinkSigning::secureConnectionAcceptUnsignedCallback)) { | ||
| qCCritical(VehicleLog) << "Failed to init signing on channel" << _pendingSigningChannel; | ||
| qgcApp()->showAppMessage(tr("Signing confirmation received but local initialization failed")); | ||
| } else { | ||
| _mavlinkSigning = true; | ||
| _mavlinkSigningKeyName = _pendingSigningKeyName; | ||
| emit mavlinkSigningChanged(); | ||
| qCDebug(VehicleLog) << "Signing confirmed, key:" << _mavlinkSigningKeyName; | ||
| } | ||
| } else if (_pendingSigningOp == PendingSigningOp::Disable) { | ||
| MAVLinkSigning::initSigning(_pendingSigningChannel, QByteArrayView(), nullptr); | ||
| _mavlinkSigning = false; | ||
| _mavlinkSigningKeyName.clear(); | ||
| emit mavlinkSigningChanged(); | ||
| qCDebug(VehicleLog) << "Signing disable confirmed"; |
There was a problem hiding this comment.
Signing is initialized/deinitialized on _pendingSigningChannel, which is captured when the request is sent. If the primary link changes (failover) before confirmation arrives, confirmation can be received on a different channel but local signing will still be enabled/disabled on the old channel, potentially breaking comms. Consider updating the target channel at confirmation time (e.g., use the current primary link’s channel, or the channel the confirming message arrived on) and/or cancel pending ops on primary-link changes.
| // Channel was captured at send time; link failover handled by tryDetectKey | |
| if (_pendingSigningOp == PendingSigningOp::Enable) { | |
| if (!MAVLinkSigning::initSigning(_pendingSigningChannel, _pendingSigningKeyBytes, | |
| MAVLinkSigning::secureConnectionAcceptUnsignedCallback)) { | |
| qCCritical(VehicleLog) << "Failed to init signing on channel" << _pendingSigningChannel; | |
| qgcApp()->showAppMessage(tr("Signing confirmation received but local initialization failed")); | |
| } else { | |
| _mavlinkSigning = true; | |
| _mavlinkSigningKeyName = _pendingSigningKeyName; | |
| emit mavlinkSigningChanged(); | |
| qCDebug(VehicleLog) << "Signing confirmed, key:" << _mavlinkSigningKeyName; | |
| } | |
| } else if (_pendingSigningOp == PendingSigningOp::Disable) { | |
| MAVLinkSigning::initSigning(_pendingSigningChannel, QByteArrayView(), nullptr); | |
| _mavlinkSigning = false; | |
| _mavlinkSigningKeyName.clear(); | |
| emit mavlinkSigningChanged(); | |
| qCDebug(VehicleLog) << "Signing disable confirmed"; | |
| // Resolve the channel at confirmation time so signing follows the current primary link after failover. | |
| const mavlink_channel_t signingChannel = _priorityLink ? _priorityLink->mavlinkChannel() : _pendingSigningChannel; | |
| if (_pendingSigningOp == PendingSigningOp::Enable) { | |
| if (!MAVLinkSigning::initSigning(signingChannel, _pendingSigningKeyBytes, | |
| MAVLinkSigning::secureConnectionAcceptUnsignedCallback)) { | |
| qCCritical(VehicleLog) << "Failed to init signing on channel" << signingChannel; | |
| qgcApp()->showAppMessage(tr("Signing confirmation received but local initialization failed")); | |
| } else { | |
| _mavlinkSigning = true; | |
| _mavlinkSigningKeyName = _pendingSigningKeyName; | |
| emit mavlinkSigningChanged(); | |
| qCDebug(VehicleLog) << "Signing confirmed, key:" << _mavlinkSigningKeyName << "channel:" << signingChannel; | |
| } | |
| } else if (_pendingSigningOp == PendingSigningOp::Disable) { | |
| MAVLinkSigning::initSigning(signingChannel, QByteArrayView(), nullptr); | |
| _mavlinkSigning = false; | |
| _mavlinkSigningKeyName.clear(); | |
| emit mavlinkSigningChanged(); | |
| qCDebug(VehicleLog) << "Signing disable confirmed on channel" << signingChannel; |
HTRamsey
force-pushed
the
fix/signing-deferred-confirmation
branch
2 times, most recently
from
1a04a1c to
a77e52e
Compare
HTRamsey
force-pushed
the
fix/signing-deferred-confirmation
branch
from
a77e52e to
64c7a66
Compare
Summary
MAVLinkSigning::verifySignature()that read CRC from wrong location in parsed messagesContext
Alternative to #14251. That PR used "any heartbeat = accepted" which false-positives because heartbeats prove the vehicle is alive, not that it accepted the key. This implementation uses actual signature verification as proof.
Fixes #14250
Test plan
🤖 Generated with Claude Code