Refactor Authenticate -> PacketProcessor, Add unit tests#383
Open
nateinaction wants to merge 46 commits into
Open
Refactor Authenticate -> PacketProcessor, Add unit tests#383nateinaction wants to merge 46 commits into
nateinaction wants to merge 46 commits into
Conversation
…-core-reference into authenticate-testing
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request replaces the Authenticate component with a new modular PacketProcessor (Parser, Validator, Authenticator), updates topologies and telemetry, adjusts build/test/CI/tooling, adds unit tests and SDD/docs, and adjusts board secure-storage partitions. ChangesPacketProcessor Component Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-core-reference into authenticate-testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-site/components/PacketProcessor.md`:
- Around line 3-15: The doc mixes HMAC and CMAC terminology which is
inconsistent with CCSDS 355.0-B-2; decide which MAC algorithm the implementation
uses (PacketProcessor uses either HMAC or CMAC) and replace all occurrences so
the page uses a single term consistently (e.g., change every "HMAC" to "CMAC" if
the implementation follows CCSDS 355.0-B-2, or update the CCSDS citation if HMAC
is actually used), including the overview bullets, any "HMAC verification
authentication" phrase, and the citation to CCSDS 355.0-B-2 so the algorithm
name and standard match across the PacketProcessor documentation.
In `@docs-site/index.md`:
- Around line 41-42: Add a blank line after the markdown heading "### [Security
Components](components/PacketProcessor.md)" so the following paragraph does not
start immediately after the heading; update docs-site/index.md by inserting a
single empty line between that heading and the subsequent paragraph to satisfy
markdownlint MD022.
In `@Framing/src/authenticate_plugin.py`:
- Line 34: The code sets path =
"PROVESFlightControllerReference/Components/PacketProcessor/AuthDefaultKey.h"
which is repo-relative and fails if the process CWD differs; change it to
compute an absolute path anchored to this module file (use __file__) so the
lookup is independent of CWD. Locate the symbol path in authenticate_plugin.py
and replace the literal with a join of os.path.dirname(__file__) (or repo root
derived from it) and the relative components so the resulting path is absolute
before use (ensure you import os if not already).
In `@PROVESFlightControllerReference/ComCcsdsSband/ComCcsds.fpp`:
- Line 101: The S-band packet processor instance (packetProcessorSband /
ComCcsdsSband) is created but not initialized; add a call to its configure() in
the deployment initialization alongside the UART and LoRa calls. Specifically,
invoke ComCcsdsSband::packetProcessorSband.configure() in the same place where
ComCcsdsUart::packetProcessor.configure() and
ComCcsdsLora::packetProcessorLora.configure() are called so the S-band processor
loads its persistent parameters.
In
`@PROVESFlightControllerReference/Components/PacketProcessor/Authenticator.cpp`:
- Around line 82-83: authenticatePacket currently only checks dataSize >=
kSecurityTrailerSize which lets trailer-only or undersized packets reach HMAC
verification; update the validation in Authenticator.cpp (inside
authenticatePacket) to compute and verify the full authenticated packet length
from the packet's header fields (not just kSecurityTrailerSize) before any
cryptographic operations, and if the computed total length is invalid or larger
than dataSize return
PacketAuthenticator::Result{PacketAuthenticator::Status::VerifyError,
PSA_ERROR_INVALID_ARGUMENT} (preserve existing error return) so HMAC
verification only runs on complete packets.
- Around line 15-16: Replace the hard-coded kKeyHexLength value with a
compile-time expression derived from the protocol constant kSecurityTrailerSize
so the hex length always follows the trailer/key size; e.g. change the
definition of kKeyHexLength to compute as (kSecurityTrailerSize * 2) (or
static_cast<size_t>(kSecurityTrailerSize) * 2 if types differ) and ensure the
header declaring kSecurityTrailerSize is available where Authenticator.cpp
defines kKeyHexLength so the constant remains in sync with kSecurityTrailerSize.
In `@PROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.md`:
- Around line 57-60: Update the class diagram so
PacketAuthenticator.authenticatePacket matches the implementation signature by
adding the missing key parameter; locate the PacketAuthenticator entry in the
diagram and change the method line from authenticatePacket(buffer, size, hmac)
to authenticatePacket(buffer, size, hmac, key) and ensure any namespace/Result
annotation remains consistent with the existing diagram style.
In
`@PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cpp`:
- Around line 171-176: The code is clearing the throttle even when a filesystem
read fails; change the logic so
log_WARNING_HI_SequenceNumberReadFailed_ThrottleClear() is only called on the
success path (i.e., when status == Os::File::OP_OK) instead of after the error
log. Update the block around status handling so that
SequenceNumberReadFailed(...) is called on error and
SequenceNumberReadFailed_ThrottleClear() is called only when the read succeeded;
apply the same change to the second occurrence that currently calls
SequenceNumberReadFailed and SequenceNumberReadFailed_ThrottleClear (lines
referencing log_WARNING_HI_SequenceNumberReadFailed and
log_WARNING_HI_SequenceNumberReadFailed_ThrottleClear).
- Around line 123-136: The SET_SEQ_NUM_cmdHandler currently only writes seq_num
to storage via writeSequenceNumber but doesn’t update the in-memory runtime
values, so m_sequenceNumber and the value used by CurrentSequenceNumber remain
stale; after a successful write (and before logging/returning OK) assign
m_sequenceNumber = seq_num and update any cached/current accessor (e.g. call the
setter or update CurrentSequenceNumber) so runtime validation uses the new
sequence number; keep the existing log_ACTIVITY_HI_SequenceNumberSet and
cmdResponse_out calls.
- Around line 168-182: In readSequenceNumber(), the missing-file branch is
reversed causing successful reads to reset the file to 0; change the conditional
that decides to create/write the default sequence file so it triggers only when
the read status equals Os::File::DOESNT_EXIST (not when it differs).
Specifically, in PacketProcessor::readSequenceNumber() replace the current check
around the call to writeSequenceNumber(0) so it uses status ==
Os::File::DOESNT_EXIST; keep the existing read call
(Utilities::FileHelper::readFromFile), the logging calls
(log_WARNING_HI_SequenceNumberReadFailed and
log_WARNING_HI_SequenceNumberReadFailed_ThrottleClear), and return values
otherwise.
- Around line 196-203: acceptPacket currently increments m_sequenceNumber, calls
writeSequenceNumber twice, ignores the return values, and never updates
CurrentSequenceNumber which allows forwarding to proceed even if persistence
fails; change acceptPacket to increment the in-memory sequence, call
writeSequenceNumber exactly once, check its return status and if it fails
abort/return without forwarding (fail closed), and only on success update
CurrentSequenceNumber (and any telemetry/ReferenceDeploymentPackets.fppi-related
state) so the persisted and in-memory sequence stay consistent; reference the
acceptPacket method, m_sequenceNumber, writeSequenceNumber, and
CurrentSequenceNumber when making the change.
In
`@PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.fpp`:
- Around line 21-22: The component comment for PacketProcessor is inaccurate: it
says the component sits between the radio and cdh while the actual uplink
pipeline wiring places PacketProcessor between TcDeframer and
SpacePacketDeframer. Update the descriptive comment above the passive component
PacketProcessor to describe its real position in the uplink pipeline (i.e., it
validates/authenticates commands after TcDeframer and before
SpacePacketDeframer) and note that some messages bypass authentication as
implemented.
In
`@PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.hpp`:
- Around line 117-121: The sequence number state (m_sequenceNumber,
m_sequenceNumberWindow, and related persisted state m_sequenceNumberFilePath) is
accessed/modified concurrently with separate atomics which allows TOCTOU races;
replace the separate atomic-only approach with a single mutex/critical section
(e.g., add std::mutex m_sequenceMutex) and wrap all accesses and mutations
related to sequence validation, incrementing and persistence (reads in
dataIn_handler, increments in acceptPacket, and updates in
SET_SEQ_NUM_cmdHandler and configure) with that lock so validation, increment
and file write are atomic with respect to each other; keep counters like
m_bypassPacketsCount/m_rejectedPacketsCount as atomics if they truly can be
updated independently.
In `@PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt`:
- Around line 67-84: The CMake logic only aborts if MBEDCRYPTO_LIB is missing
but silently continues when PSA_CRYPTO_H (psa/crypto.h) is not found, causing
later compile failures in Authenticator.cpp; remove the unused find_path for
MBEDTLS_MD_H (mbedtls/md.h) and change the find_path for PSA_CRYPTO_H into an
explicit failure check (or replace both with find_package(MbedTLS) if preferred)
so that if PSA_CRYPTO_H is not found CMake calls message(FATAL_ERROR) with a
clear message; ensure target_include_directories(packet_processor_authenticator
PUBLIC ${PSA_CRYPTO_H}) and target_link_libraries(packet_processor_authenticator
PUBLIC ${MBEDCRYPTO_LIB}) remain but only after the existence checks.
In
`@PROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Validator.cpp`:
- Around line 19-29: The bypass matrix tests only cover UART and LoRa
GET_SEQ_NUM opcodes; add an equivalent S-band test so the new
packetProcessorSband transport is covered. Add a third TEST in
test_PacketProcessor_Validator.cpp (matching the style of
BypassUartGetSeqNumOpCode and BypassLoraGetSeqNumOpCode) that constructs a
Packet with the S-band GET_SEQ_NUM opcode, calls validatePacket(...) and asserts
the result equals PacketValidator::Status::Bypass; this ensures the allowlist
for packetProcessorSband (from ComCcsds.fpp) is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2cf3493a-cc76-4bdc-a8ef-643ad95f4d57
📒 Files selected for processing (41)
.github/workflows/ci.yaml.gitignore.vscode/launch.json.vscode/tasks.jsonAGENTS.mdFraming/src/authenticate_plugin.pyMakefilePROVESFlightControllerReference/ComCcsdsLora/ComCcsds.fppPROVESFlightControllerReference/ComCcsdsSband/ComCcsds.fppPROVESFlightControllerReference/ComCcsdsUart/ComCcsds.fppPROVESFlightControllerReference/Components/Authenticate/Authenticate.cppPROVESFlightControllerReference/Components/Authenticate/Authenticate.fppPROVESFlightControllerReference/Components/Authenticate/Authenticate.hppPROVESFlightControllerReference/Components/Authenticate/docs/sdd.mdPROVESFlightControllerReference/Components/CMakeLists.txtPROVESFlightControllerReference/Components/PacketProcessor/Authenticator.cppPROVESFlightControllerReference/Components/PacketProcessor/Authenticator.hppPROVESFlightControllerReference/Components/PacketProcessor/CMakeLists.txtPROVESFlightControllerReference/Components/PacketProcessor/Const.hppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.fppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.hppPROVESFlightControllerReference/Components/PacketProcessor/Parser.cppPROVESFlightControllerReference/Components/PacketProcessor/Parser.hppPROVESFlightControllerReference/Components/PacketProcessor/Types.hppPROVESFlightControllerReference/Components/PacketProcessor/Validator.cppPROVESFlightControllerReference/Components/PacketProcessor/Validator.hppPROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.mdPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppiPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentTopology.cppPROVESFlightControllerReference/test/unit-tests/CMakeLists.txtPROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Authenticator.cppPROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Parser.cppPROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Validator.cppREADME.mddocs-site/components/Authenticate.mddocs-site/components/PacketProcessor.mddocs-site/index.mdmkdocs.ymlprj.confscripts/generate_auth_key_header.py
💤 Files with no reviewable changes (5)
- PROVESFlightControllerReference/Components/Authenticate/docs/sdd.md
- docs-site/components/Authenticate.md
- PROVESFlightControllerReference/Components/Authenticate/Authenticate.hpp
- PROVESFlightControllerReference/Components/Authenticate/Authenticate.fpp
- PROVESFlightControllerReference/Components/Authenticate/Authenticate.cpp
Collaborator
Author
|
Hold. Logic error. If someone transmits a sequence number more than one unit larger than the current sequence number, this change currently still only increases the sequence number by one. This will allow replay of any messages transmitted larger than one step above the sequence number count. i.e. if sequence number is 10, our window is 50k and I transmit a command with sequence number 10k, the sequence number will only increase to 11. This will allow the command to be replayed many more times. This issue was not in the original implementation and needs to be corrected before merge. |
nateinaction
commented
May 5, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
docs-site/components/PacketProcessor.md (1)
98-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame markdownlint MD022 and wording issues as in
sdd.md.Lines 98-99, 192, and 194 here have the same issues flagged in
sdd.md: missing blank line before### Additional resources, missing hyphen in "16 byte", and missing "packet" in AUTH006.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/components/PacketProcessor.md` around lines 98 - 99, Add a blank line before the "### Additional resources" heading to satisfy markdownlint MD022; replace any occurrences of the phrase "16 byte" with the hyphenated form "16-byte" to fix wording, and update the AUTH006 message to include the missing word "packet" (e.g., change AUTH006 text to reference "packet") so the error description reads correctly; locate these edits around the "### Additional resources" heading, the literal "16 byte" text, and the AUTH006 identifier in PacketProcessor.md.PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cpp (1)
211-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
acceptPacketdoes not updateCurrentSequenceNumbertelemetry.While
SET_SEQ_NUM_cmdHandlerandconfigure()both calltlmWrite_CurrentSequenceNumber,acceptPacketonly persists to disk without updating telemetry. TheCurrentSequenceNumberchannel (wired intoReferenceDeploymentPackets.fppi) will remain stale after authenticated packets are processed.📊 Proposed fix
void PacketProcessor ::acceptPacket(Fw::Buffer& data, const ComCfg::FrameContext& context, const U32 sequenceNumber) { // Update the sequence number and write it to disk // intentionally not checking the return value this->m_sequenceNumber = sequenceNumber; this->writeSequenceNumber(this->m_sequenceNumber); + + // Telemeter the updated sequence number + this->tlmWrite_CurrentSequenceNumber(this->m_sequenceNumber); // Authenticate the packet ComCfg::FrameContext contextOut = context;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cpp` around lines 211 - 223, acceptPacket updates m_sequenceNumber and persists it to disk via writeSequenceNumber but never updates the telemetry channel; modify PacketProcessor::acceptPacket to call tlmWrite_CurrentSequenceNumber(this->m_sequenceNumber) immediately after writeSequenceNumber (or whenever m_sequenceNumber is changed) so the CurrentSequenceNumber telemetry reflects the new value; reference functions/fields: PacketProcessor::acceptPacket, m_sequenceNumber, writeSequenceNumber, and tlmWrite_CurrentSequenceNumber.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@PROVESFlightControllerReference/Components/PacketProcessor/Authenticator.cpp`:
- Around line 93-97: keyBytes (the decoded HMAC key buffer used with parseHexKey
and importHmacKey) contains sensitive key material and must be securely erased
on every return path; replace any plain memset with a non-optimizable
zeroization (e.g. call mbedtls_platform_zeroize or psa_crypto_memzero)
immediately after importHmacKey completes and also before returning on
parseHexKey failure or other error returns (affecting functions/locations that
return PacketAuthenticator::Result with PacketAuthenticator::Status values), and
ensure the appropriate header is included so the chosen secure zeroize function
is available.
In `@PROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.md`:
- Around line 192-194: Update the two requirement sentences: in AUTH005 replace
"16 byte security trailer" with "16-byte security trailer" to add the hyphen,
and in AUTH006 fix the phrasing "remove the Security Header and Security Trailer
from the and pass the remaining packet data to data out" to "remove the Security
Header and Security Trailer from the packet and pass the remaining packet data
to data out" (reference AUTH005 and AUTH006).
- Around line 97-99: The markdown heading "### Additional resources" is missing
a blank line before it causing markdownlint MD022; edit the sdd.md content to
insert a single blank line immediately above the "### Additional resources"
heading (i.e., add an empty line before the line that starts with "###
Additional resources") so the heading is separated from the preceding
paragraph/list.
In
`@PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.hpp`:
- Around line 1-12: The header PacketProcessor.hpp is missing an inclusion
guard; add a header guard or a `#pragma` once to prevent multiple inclusions.
Specifically, at the very top of PacketProcessor.hpp (before any includes) add a
single-identifier guard (or `#pragma` once), and at the end of the file close the
guard if using `#ifndef/`#define/#endif; ensure the chosen macro name is unique
(e.g., based on PACKETPROCESSOR_HPP) to avoid collisions with other headers that
reference the PacketProcessor component and related types used in this file
(FileHelper, Fw::String, Os::File, Os::Mutex, etc.).
In
`@PROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Validator.cpp`:
- Around line 37-41: The comment on the test
PacketValidatorTest::BypassSpiInvalid contains a duplicated phrase ("but bypass
opcode" twice); update the inline comment on the Packet p initializer to a
concise description such as "non-zero SPI but bypass opcode" so it reads
clearly; locate the test case (TEST(PacketValidatorTest, BypassSpiInvalid)), the
Packet p declaration, and adjust only the comment text without changing test
logic or function calls to validatePacket.
- Around line 55-87: Add a unit test that verifies the inclusive upper boundary
of the sequence window by constructing a Packet whose sequence equals expected +
window and asserting validatePacket(...) returns PacketValidator::Status::Valid;
locate the packet tests near PacketValidatorTest (e.g., add a new TEST similar
to SequenceNumberInvalid/SequenceNumberWrapWithinWindow) and use validatePacket,
Packet, and the same window variable name (sequenceNumberWindow / the third
validatePacket parameter) to ensure the case expected + window is accepted.
---
Duplicate comments:
In `@docs-site/components/PacketProcessor.md`:
- Around line 98-99: Add a blank line before the "### Additional resources"
heading to satisfy markdownlint MD022; replace any occurrences of the phrase "16
byte" with the hyphenated form "16-byte" to fix wording, and update the AUTH006
message to include the missing word "packet" (e.g., change AUTH006 text to
reference "packet") so the error description reads correctly; locate these edits
around the "### Additional resources" heading, the literal "16 byte" text, and
the AUTH006 identifier in PacketProcessor.md.
In
`@PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cpp`:
- Around line 211-223: acceptPacket updates m_sequenceNumber and persists it to
disk via writeSequenceNumber but never updates the telemetry channel; modify
PacketProcessor::acceptPacket to call
tlmWrite_CurrentSequenceNumber(this->m_sequenceNumber) immediately after
writeSequenceNumber (or whenever m_sequenceNumber is changed) so the
CurrentSequenceNumber telemetry reflects the new value; reference
functions/fields: PacketProcessor::acceptPacket, m_sequenceNumber,
writeSequenceNumber, and tlmWrite_CurrentSequenceNumber.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30e315ea-81b6-4c96-9f31-8cd8ca0f634e
📒 Files selected for processing (12)
PROVESFlightControllerReference/Components/PacketProcessor/Authenticator.cppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.fppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.hppPROVESFlightControllerReference/Components/PacketProcessor/Validator.cppPROVESFlightControllerReference/Components/PacketProcessor/Validator.hppPROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.mdPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentTopology.cppPROVESFlightControllerReference/test/unit-tests/CMakeLists.txtPROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Authenticator.cppPROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Validator.cppdocs-site/components/PacketProcessor.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.md`:
- Around line 145-147: AUTH007's wording conflicts with the documented
commands/telemetry: the command entries GET_SEQ_NUM and SET_SEQ_NUM and the
telemetry CurrentSequenceNumber are global but AUTH007 requires sequence
reporting "for a given SPI"; update either the requirement or the interface to
be consistent. Fix option A (doc-only): change AUTH007 text to require reporting
a single global sequence number (remove "for a given SPI") so it references
SequenceNumberGet/SequenceNumberSet and CurrentSequenceNumber; or Fix option B
(interface change): make GET_SEQ_NUM and SET_SEQ_NUM include an spi parameter
(e.g., spi: U8/U16) and add SPI-scoped telemetry (e.g.,
CurrentSequenceNumber{spi}) and update SequenceNumberGet/SequenceNumberSet event
descriptions to document the SPI field. Ensure references to AUTH007,
GET_SEQ_NUM, SET_SEQ_NUM, SequenceNumberGet, SequenceNumberSet, and
CurrentSequenceNumber are updated accordingly.
- Around line 3-4: The doc's authentication spec incorrectly references "CCSDS
355.0-B-2 CMAC" while the implementation and tests use HMAC; update the
PacketProcessor SDD (sdd.md) to consistently call out HMAC instead of CMAC and
change the two occurrences that mention "CCSDS 355.0-B-2 CMAC" (around the text
discussing PacketProcessor authentication and the policy/spec reference near
line 100) to an HMAC-appropriate reference such as "HMAC (per RFC 2104)" or
another project-approved HMAC standard; ensure all mentions, the security
requirement language, and any normative references align with HMAC terminology
so the doc matches the implementation.
In
`@PROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cpp`:
- Around line 215-227: acceptPacket updates m_sequenceNumber and persists it via
writeSequenceNumber but does not update the telemetry; add a call to
tlmWrite_CurrentSequenceNumber(this->m_sequenceNumber) in
PacketProcessor::acceptPacket immediately after writeSequenceNumber (similar to
configure() and SET_SEQ_NUM_cmdHandler) so the CurrentSequenceNumber telemetry
reflects accepted packets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 183b743a-16ee-4c54-9e65-5d1e3666baf4
📒 Files selected for processing (8)
PROVESFlightControllerReference/Components/PacketProcessor/Authenticator.cppPROVESFlightControllerReference/Components/PacketProcessor/Authenticator.hppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.cppPROVESFlightControllerReference/Components/PacketProcessor/PacketProcessor.hppPROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.mdPROVESFlightControllerReference/test/unit-tests/test_PacketProcessor_Validator.cppboards/bronco_space/proves_flight_control_board_v5/proves_flight_control_board_v5.dtsiprj.conf
Comment on lines
+3
to
+4
| The PacketProcessor component verifies the integrity and authenticity of incoming CCSDS command packets using HMAC in the uplink path. It sits between TcDeframer and SpacePacketDeframer and enforces parse, policy validation, and authentication before forwarding commands. | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*PROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.md" -type fRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 167
🏁 Script executed:
cat -n PROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.md | head -250Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 11074
Resolve HMAC vs CMAC terminology conflict in authentication specification.
Line 14 references "CCSDS 355.0-B-2 CMAC" but the entire implementation uses HMAC (lines 3, 9, 193, 194, and all tests). CCSDS 355.0-B-2 specifies CMAC, not HMAC—these are distinct cryptographic algorithms. The documentation must consistently use one algorithm and reference its corresponding standard.
Since the implementation, requirements, and code all specify HMAC, update lines 14 and 100 to reference an HMAC-appropriate standard instead of the CMAC standard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PROVESFlightControllerReference/Components/PacketProcessor/docs/sdd.md`
around lines 3 - 4, The doc's authentication spec incorrectly references "CCSDS
355.0-B-2 CMAC" while the implementation and tests use HMAC; update the
PacketProcessor SDD (sdd.md) to consistently call out HMAC instead of CMAC and
change the two occurrences that mention "CCSDS 355.0-B-2 CMAC" (around the text
discussing PacketProcessor authentication and the policy/spec reference near
line 100) to an HMAC-appropriate reference such as "HMAC (per RFC 2104)" or
another project-approved HMAC standard; ensure all mentions, the security
requirement language, and any normative references align with HMAC terminology
so the doc matches the implementation.
Contributor
|
I like moving it from Authenticate since it doesnt show that anything is being processed but PacketProcessor is a bit vague, maybe SpaceSecurityDeframer since its in between two deframers in the topology |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is an attempt to simplify the Authenticate component so that it is easier to reason about and write automated tests for.
Notable items in this refactor:
AuthenticatetoPacketProcessorbecause I found that I needed functionality called "authenticate" inside the component and didn't want to introduce a stutter likeAuthenticate::authenticatePacket().Authenticator,Parser, andValidatorDetumbleManagerwhich was split into several classes, I took advice from Simple Made Easy - Rich Hickey to use namespaces instead of classes to organize code. I liked this approach.psa_mac_verifyinstead of performing the HMAC validation ourselvesThis screenshot shows sequence number out of window logging, a bypass opcode working, then a fully authenticated packet working:
This PR does not address #220 which can be addressed in a follow-on PR.
Useful resources: