fix(TcSecurityDeframer): a factory-fresh board cannot be provisioned (ship blocker)

PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp

@@ -50,6 +50,34 @@ U32& keyStoreGeneration() {
     return generation;
 }
 
+//! Asks the filesystem holding `filePath` whether it is actually mounted.
+//!
+//! fs_statvfs resolves the mount point and queries the underlying FS, so a success is direct
+//! evidence that /keys is up. This is the signal that makes "stat says the file is missing" safe to
+//! act on: on Zephyr, fs_stat returns -ENOENT for an unmounted mount point just as it does for a
+//! missing file (subsys/fs/fs.c: fs_get_mnt_point), so absence alone proves nothing.
+KeyStore::MountProbe probeMount(const char* filePath) {
+    char directory[64];
+    const char* const lastSlash = std::strrchr(filePath, '/');
+    if (lastSlash == nullptr || lastSlash == filePath) {
+        directory[0] = '/';
+        directory[1] = '\0';
+    } else {
+        const size_t length = static_cast<size_t>(lastSlash - filePath);
+        if (length >= sizeof directory) {
+            return KeyStore::MountProbe::Unknown;
+        }
+        std::memcpy(directory, filePath, length);
+        directory[length] = '\0';
+    }
+
+    FwSizeType totalBytes = 0;
+    FwSizeType freeBytes = 0;
+    return (Os::FileSystem::getFreeSpace(directory, totalBytes, freeBytes) == Os::FileSystem::OP_OK)
+               ? KeyStore::MountProbe::Live
+               : KeyStore::MountProbe::Unknown;
+}
+
 }  // namespace
 
 // ----------------------------------------------------------------------
@@ -230,17 +258,25 @@ void TcSecurityDeframer ::PROVISION_KEY_cmdHandler(FwOpcodeType opCode,
     // of a key we managed to read. PROVISION_KEY is bypass-allowlisted so it works on a keyless
     // board, so if an unreadable store (a truncated record reading back as BAD_SIZE, a littlefs
     // error, a mount not ready yet) counted as "keyless", anyone in radio range could induce a read
-    // failure and install their own key while a valid one still sits on flash. DOESNT_EXIST is the
-    // one failure that *is* proof of emptiness.
-    if (loadStatus != Os::File::OP_OK && loadStatus != Os::File::DOESNT_EXIST) {
+    // failure and install their own key while a valid one still sits on flash.
+    //
+    // The proof cannot come from loadStatus alone: on the Zephyr target an absent file reports
+    // OTHER_ERROR, not DOESNT_EXIST, so gating on the status made a factory-fresh (or /keys-erased)
+    // board refuse its own bootstrap forever - keyless and unprovisionable, i.e. total command
+    // loss. probeKeyStore() asks the filesystem instead. See Components::KeyStore in Types.hpp.
+    KeyStore::MountProbe mountProbe = KeyStore::MountProbe::Unknown;
+    KeyStore::StoreProbe storeProbe = KeyStore::StoreProbe::Unreadable;
+    this->probeKeyStore(loadStatus, mountProbe, storeProbe);
+
+    if (!KeyStore::storeStateIsKnown(mountProbe, storeProbe)) {
         this->log_WARNING_HI_KeyProvisionFailed(KeyStoreProvisionStatus::StoreUnreadable);
         this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);
         return;
     }
 
     // PROVISION_KEY is trust-on-first-use bootstrap: only honored while the store is empty.
     // Once any key exists, rotation must go through ADD_KEY/REMOVE_KEY (which require auth).
-    if (this->activeKeyCount() != 0) {
+    if (!KeyStore::storeIsProvisionable(mountProbe, storeProbe, this->activeKeyCount())) {
         this->log_WARNING_HI_KeyProvisionFailed(KeyStoreProvisionStatus::NotEmpty);
         this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);
         return;
@@ -297,8 +333,13 @@ void TcSecurityDeframer ::ADD_KEY_cmdHandler(FwOpcodeType opCode, U32 cmdSeq, U1
 
     // If the store could not be read back, its contents are unknown, and the read-modify-write
     // below would persist a guess: writing the last in-memory copy over whatever is actually on
-    // flash. Refuse rather than risk dropping a live key.
-    if (loadStatus != Os::File::OP_OK && loadStatus != Os::File::DOESNT_EXIST) {
+    // flash. Refuse rather than risk dropping a live key. As in PROVISION_KEY, "could not be read"
+    // has to be established by probing the filesystem, not by reading loadStatus.
+    KeyStore::MountProbe mountProbe = KeyStore::MountProbe::Unknown;
+    KeyStore::StoreProbe storeProbe = KeyStore::StoreProbe::Unreadable;
+    this->probeKeyStore(loadStatus, mountProbe, storeProbe);
+
+    if (!KeyStore::storeStateIsKnown(mountProbe, storeProbe)) {
         this->log_WARNING_HI_KeyAddFailed(KeyStoreProvisionStatus::StoreUnreadable);
         this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);
         return;
@@ -375,7 +416,11 @@ void TcSecurityDeframer ::REMOVE_KEY_cmdHandler(FwOpcodeType opCode, U32 cmdSeq,
     const Os::File::Status loadStatus = this->loadKeyStore();
 
     // See ADD_KEY: an unreadable store makes the read-modify-write a guess.
-    if (loadStatus != Os::File::OP_OK && loadStatus != Os::File::DOESNT_EXIST) {
+    KeyStore::MountProbe mountProbe = KeyStore::MountProbe::Unknown;
+    KeyStore::StoreProbe storeProbe = KeyStore::StoreProbe::Unreadable;
+    this->probeKeyStore(loadStatus, mountProbe, storeProbe);
+
+    if (!KeyStore::storeStateIsKnown(mountProbe, storeProbe)) {
         this->log_WARNING_HI_KeyRemoveFailed(KeyStoreProvisionStatus::StoreUnreadable);
         this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);
         return;
@@ -544,6 +589,34 @@ Os::File::Status TcSecurityDeframer ::loadKeyStore() {
     return status;
 }
 
+void TcSecurityDeframer ::probeKeyStore(const Os::File::Status loadStatus,
+                                        KeyStore::MountProbe& mount,
+                                        KeyStore::StoreProbe& store) const {
+    if (loadStatus == Os::File::OP_OK) {
+        // A full, successful read is self-evidently proof the filesystem served the file.
+        mount = KeyStore::MountProbe::Live;
+        store = KeyStore::StoreProbe::Present;
+        return;
+    }
+
+    // The read failed. Do not infer why from its status: ZephyrFile::open collapses every fs_open
+    // errno into OTHER_ERROR, so on flight hardware a missing file and a corrupt filesystem are
+    // indistinguishable here. Ask the filesystem directly instead.
+    //
+    // Os::FileSystem::getPathType() is deliberately not used: it folds every error into NOT_EXIST,
+    // which would turn an I/O error into a false "absent" and re-open the very hole this gate
+    // exists to close. Go through the interface to keep the real status.
+    Os::FileSystem::PathType pathType = Os::FileSystem::PathType::NOT_EXIST;
+    const Os::FileSystem::Status statStatus =
+        Os::FileSystem::getSingleton()._getPathType(this->m_keyStoreFilePath.toChar(), pathType);
+
+    // DOESNT_EXIST from stat is a positive answer ("this path is not there"), but on Zephyr it is
+    // also what an unmounted /keys reports, so it only counts once probeMount() corroborates it.
+    store =
+        (statStatus == Os::FileSystem::DOESNT_EXIST) ? KeyStore::StoreProbe::Absent : KeyStore::StoreProbe::Unreadable;
+    mount = probeMount(this->m_keyStoreFilePath.toChar());
+}
+
 Os::File::Status TcSecurityDeframer ::writeKeyStore() {
     // Write to a temp file, flush, then rename over the target - the same pattern as
     // StartupManager::persist_boot_count, and for a more serious failure mode. Overwriting the

PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hpp

@@ -123,6 +123,15 @@ class TcSecurityDeframer final : public TcSecurityDeframerComponentBase {
     //! lock held. On a missing/unreadable file, m_keyStore is left with no valid slots (keyless state).
     Os::File::Status loadKeyStore();
 
+    //! Determines what is actually known about the on-flash key store after a loadKeyStore()
+    //! attempt, by interrogating the filesystem rather than trusting the read status (which cannot
+    //! distinguish "missing" from "broken" on the Zephyr target). Feeds the pure
+    //! Components::KeyStore policy predicates. Must be called with the key store lock held.
+    void probeKeyStore(const Os::File::Status loadStatus,  //!< Status returned by loadKeyStore()
+                       KeyStore::MountProbe& mount,        //!< Set to whether /keys is demonstrably mounted
+                       KeyStore::StoreProbe& store         //!< Set to what is known about the store file
+    ) const;
+
     //! Writes m_keyStore to the file system. Must be called with the key store lock held.
     Os::File::Status writeKeyStore();
 

PROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hpp

@@ -25,6 +25,56 @@ struct ActiveSpiSlot {
 
 using ActiveSpiSlots = std::array<ActiveSpiSlot, kMaxActiveKeys>;  //!< The set of currently active SPIs
 
+//! Key-store admission policy.
+//!
+//! The decision "may this board be provisioned / may its key store be rewritten" is expressed here
+//! as pure C++ over probe results, deliberately away from any Os:: type, so it can be exercised
+//! directly by the host gtest suite across the full status matrix.
+//!
+//! Why it cannot be written directly against Os::File::Status: on the Zephyr target a *genuinely
+//! missing* file does not report Os::File::DOESNT_EXIST. ZephyrFile::open discards fs_open's errno
+//! and returns OTHER_ERROR for every failure, so DOESNT_EXIST is unreachable on flight hardware
+//! while it is the normal result on the POSIX host. Any gate written as
+//! `status != OP_OK && status != DOESNT_EXIST -> refuse` therefore passes on the host and refuses
+//! on the target, which is exactly how a brand-new board became unprovisionable. The read status
+//! alone is not a usable signal; the caller must probe the filesystem for the answer.
+namespace KeyStore {
+
+//! Whether the filesystem holding the key store is demonstrably mounted.
+enum class MountProbe {
+    Live,     //!< The mount point answered a statvfs, so the filesystem is up
+    Unknown,  //!< The mount point could not be interrogated: not mounted, not ready, or erroring
+};
+
+//! What is demonstrably true about the key store file itself.
+enum class StoreProbe {
+    Present,     //!< The store was read back in full; its contents are known
+    Absent,      //!< The store was positively confirmed not to exist (stat says so)
+    Unreadable,  //!< The store may or may not exist, and its contents are unknown
+};
+
+//! True when the on-flash key store's contents are known with certainty.
+//!
+//! Note the asymmetry: `Present` needs no mount probe, because a successful full read is itself
+//! proof the filesystem served the file. `Absent` does need one, because on Zephyr fs_stat reports
+//! -ENOENT both for "file missing from a mounted filesystem" and for "no such mount point"
+//! (subsys/fs/fs.c fs_get_mnt_point). Treating the latter as emptiness would let an attacker who
+//! can keep /keys from mounting provision their own key over a board that still holds a valid one.
+constexpr bool storeStateIsKnown(MountProbe mount, StoreProbe store) {
+    return (store == StoreProbe::Present) || (store == StoreProbe::Absent && mount == MountProbe::Live);
+}
+
+//! True when trust-on-first-use provisioning (PROVISION_KEY) may proceed.
+//!
+//! PROVISION_KEY is bypass-allowlisted, i.e. reachable unauthenticated over RF, so it is only safe
+//! while the store is *proven* empty. Proven means: we know what is on flash (storeStateIsKnown)
+//! and it holds no active key. An unreadable store is never provisionable.
+constexpr bool storeIsProvisionable(MountProbe mount, StoreProbe store, uint8_t activeKeyCount) {
+    return storeStateIsKnown(mount, store) && activeKeyCount == 0;
+}
+
+}  // namespace KeyStore
+
 //! CCSDS 355.0-B-2
 //! https://ccsds.org/Pubs/355x0b2.pdf
 namespace Ccsds355_0_B_2 {

PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md

@@ -24,7 +24,9 @@ The store is shared across all `TcSecurityDeframer` instances (UART/LoRa/Sband):
 
 Three commands manage the store (see [Commands](#commands)): `PROVISION_KEY` (bootstrap, only while the store is empty), `ADD_KEY` (rotation, fails at 2 active keys), and `REMOVE_KEY` (rotation, fails at 1 remaining key). All three re-read the store from flash first, then re-import it into PSA and update `ActiveKeyCount` telemetry on success.
 
-All three also refuse to act (`StoreUnreadable`) when that re-read fails with anything other than "file does not exist", because the store's contents are then unknown. For `ADD_KEY`/`REMOVE_KEY` that prevents a read-modify-write from persisting a stale guess over the real store. For `PROVISION_KEY` it is a security property: the command is bypass-allowlisted so it works on a keyless board, so trust-on-first-use must be gated on *proof* that the store is empty. Treating an unreadable store as keyless would let anyone in radio range induce a read failure and install their own key while a valid one still sits on flash.
+All three also refuse to act (`StoreUnreadable`) unless the store's contents are *known*, because otherwise they would be acting on a guess.
+
+"Known" cannot be decided from the read status. On the Zephyr target `ZephyrFile::open` discards `fs_open`'s errno and reports `OTHER_ERROR` for every failure, so `Os::File::DOESNT_EXIST` is unreachable on flight hardware even though it is the normal result for a missing file on the POSIX host. Gating on the status therefore made a factory-fresh (or `/keys`-erased) board refuse `PROVISION_KEY` forever: keyless *and* unprovisionable, i.e. total command loss. Instead `probeKeyStore()` interrogates the filesystem — `fs_stat` on the store file for a positive absence answer, plus `fs_statvfs` on the mount point to prove `/keys` is actually mounted — and feeds the pure predicates in `Components::KeyStore` (`Types.hpp`). Both signals are required: Zephyr returns `-ENOENT` for an unmounted mount point exactly as it does for a missing file, so absence alone proves nothing. For `ADD_KEY`/`REMOVE_KEY` that prevents a read-modify-write from persisting a stale guess over the real store. For `PROVISION_KEY` it is a security property: the command is bypass-allowlisted so it works on a keyless board, so trust-on-first-use must be gated on *proof* that the store is empty. Treating an unreadable store as keyless would let anyone in radio range induce a read failure and install their own key while a valid one still sits on flash.
 
 `REMOVE_KEY` zeroizes the revoked slot's key bytes before persisting, so a removed key is not recoverable from the on-flash record; the slot is restored if the durable write fails.
 
@@ -201,6 +203,7 @@ TcSecurityDeframer helper functionality is covered by unit tests in PROVESFlight
 |---|---|
 | test_TcSecurityDeframer_Parser.cpp | Valid parse path plus parse failures for SPI, sequence number, and MAC size checks. |
 | test_TcSecurityDeframer_Validator.cpp | SPI validation against the active `ActiveSpiSlots` set (single slot, second slot, no valid slots), out-of-window and replayed sequence numbers, window boundary, and wraparound handling. |
+| test_TcSecurityDeframer_KeyStorePolicy.cpp | The key store admission predicates (`Components::KeyStore::storeStateIsKnown` / `storeIsProvisionable`) over the exhaustive mount x store-probe x active-key-count matrix: a cold board on a live mount is provisionable; an unreadable store never is; an absent store on an unproven mount never is; and a regression test documenting why a gate written over `Os::File::Status` diverges between host and target. |
 | test_TcSecurityDeframer_Authenticator.cpp | `parseHexKey` (valid upper/lowercase, null, wrong length, non-hex characters), key import via hex and raw bytes, `destroyHmacKey`, successful MAC verification, and failed verification with corrupted MAC, corrupted data, or a destroyed key. |
 
 These cover only the pure-function layer (Parser/Validator/Authenticator; no F Prime or Zephyr dependency). The key store mutation rules enforced in the command handlers (`PROVISION_KEY`/`ADD_KEY`/`REMOVE_KEY` — provision-only-when-empty, add fails at 2, remove fails at 1) are F-Prime-component-dependent and are not covered here; see the commented-out `register_fprime_ut` block in `CMakeLists.txt` for a future on-target/component test pass.

PROVESFlightControllerReference/test/int/provision_key_test.py

@@ -78,7 +78,45 @@ def test_provision_key(
 
     if evt.template.get_full_name().endswith("KeyProvisionFailed"):
         status = evt.args[0].val
+        # NotEmpty is the ONLY tolerated failure, and only because CI boards keep the key store on
+        # a littlefs partition that survives reflashing, so every run after the first re-provisions
+        # a board that is already provisioned.
+        #
+        # StoreUnreadable in particular must never be tolerated here. On a board that already holds
+        # a key it means the store went unreadable (a real fault); on a keyless board it is the
+        # signature of the cold-provisioning lockout - the board cannot be commanded at all, and
+        # letting this pass is precisely how that shipped. See test_cold_provision_gap below.
         assert status == "NotEmpty", (
             f"PROVISION_KEY failed with unexpected status {status!r}; "
             "board should either be keyless or already hold the CI key"
         )
+
+
+@pytest.mark.provision_key
+def test_cold_provision_gap():
+    """Placeholder for the cold-provision path, which is not yet reachable from CI.
+
+    The bug this file's assertions now guard against (PROVISION_KEY refused with StoreUnreadable on
+    a board whose key store file does not exist) can only be exercised against a *genuinely blank*
+    key store. Nothing in the test fleet can produce that state:
+
+      * every bench/CI board was provisioned under an earlier firmware revision, and the littlefs
+        keystore_partition survives reflashing, so the store is never absent;
+      * the existing ``fsFormat.FORMAT`` command formats the FatFS root ``/``, NOT the littlefs
+        ``/keys`` partition, so it cannot clear the store either.
+
+    Automating this therefore needs new firmware capability - either a command that erases the
+    keystore partition (which must itself be authenticated, since it is a remote-bricking primitive
+    if it is not) or a CI step that flashes a blank keystore partition image over SWD. Rather than
+    fake a cold board with a mock that would re-hide the host/target divergence, the invariant is
+    covered exhaustively at the unit level in
+    ``test/unit-tests/test_TcSecurityDeframer_KeyStorePolicy.cpp``, and this test is left as an
+    explicit, visible gap.
+
+    TODO(#472): implement once /keys can be erased or pre-flashed blank from CI.
+    """
+    pytest.skip(
+        "Cold-provision path needs a blank /keys partition; no mechanism exists yet to produce "
+        "one from CI (fsFormat.FORMAT targets FatFS /, not littlefs /keys). Covered at unit level "
+        "by test_TcSecurityDeframer_KeyStorePolicy.cpp."
+    )