diff --git a/docs/conventions.md b/docs/conventions.md index ee0dbc0..e1d196a 100644 --- a/docs/conventions.md +++ b/docs/conventions.md @@ -47,7 +47,7 @@ The design standards that code in this repository is held to. This document is n - **Held in wiping types, wiped after use.** A private key or PSK lives in a type that wipes it when destroyed (`Identity`, `HandshakePsk`, `ClientRecord`, the ephemeral key a `NoiseInitiator` keeps), and a buffer that held one (a blob loaded or encoded for the provider) is wiped with `secure_zero()` once used. A secret is never formatted into a string. - **Pairing secrets the same way.** A pairing code or token from the host, the CPace run and the values it unwraps are held in wiping types (`PairingAttempt`, `PairingPskToken`, the code's buffer on its way to core), and the buffers a pairing message was read from are wiped once it is parsed, whether or not it passed validation, spare capacity included, as are the decrypted message's buffer and the reassembly buffer when a message fails to decrypt, a fragment sequence fails, the session ends or either is destroyed; either grows through `secure_reserve()`, which wipes the block it leaves. The message trace never sees one: `redact_pairing_secrets()` replaces each secret field a client sends before the plaintext is traced. -- **Persisted before use.** A credential the server will reference in later handshakes (a long-term PSK) is referenced only once the provider reports it durable; until then the record as it was applies (`ServerCore::store_long_term_psk()`). +- **Persisted before use.** A credential the server will reference in later handshakes (a long-term PSK) is referenced only once the provider reports it durable; until then the record as it was applies (`ServerCore::store_long_term_psk()`). Only the credential's own save writes it: a save of the record made meanwhile writes the record without it, and the credential is durable only once a save of it lands with none made meanwhile. - **Removed at once, written until durable.** A removed long-term PSK or approval (an unpairing, a revocation) stops applying at once, and a record write no caller waits on that the provider fails is written again until it lands or the server stops (`ServerCore::FailedWrites`), so a restart does not restore it once the provider recovers before the stop. ## Logging diff --git a/src/server_core.cpp b/src/server_core.cpp index 41bd1b6..d5abc4a 100644 --- a/src/server_core.cpp +++ b/src/server_core.cpp @@ -909,8 +909,25 @@ std::string ServerCore::record_owner(const std::string& client_id) const { return "a client not connected"; } -void ServerCore::save_record(const std::string& client_id, std::function done) { +void ServerCore::save_record(const std::string& client_id) { const ClientRecord* record = this->records_.find(client_id); + const auto unsaved = this->unsaved_records_.find(client_id); + if (record == nullptr || unsaved == this->unsaved_records_.end()) { + this->save_record(client_id, record, nullptr); + return; + } + // pairing.md "server/pair-finalize": only its own save persists a long-term PSK, so a save + // behind it writes the record as put_back() would leave it, and a change whose save lands is + // saved again + unsaved->second.saved_meanwhile = true; + // A record the change created and put_back() removes is not written + if (const std::optional kept = without_change(record, unsaved->second)) { + this->save_record(client_id, &*kept, nullptr); + } +} + +void ServerCore::save_record(const std::string& client_id, const ClientRecord* record, + std::function done) { const std::optional slot = this->records_.slot_of(client_id); auto blob = std::make_shared(); const bool encoded = record != nullptr && encode_client_record(*record, blob->bytes); @@ -1083,8 +1100,19 @@ void ServerCore::store_long_term_psk(const std::string& client_id, const Psk& ps this->unsaved_records_.emplace(client_id, UnsavedRecord{std::move(before), existing != nullptr, false}); this->records_.set(record); - this->save_record(client_id, [this, id = client_id, done = std::move(done)](bool saved) { + this->save_change(client_id, std::move(done)); +} + +void ServerCore::save_change(const std::string& client_id, + std::function done) { + auto on_saved = [this, id = client_id, done = std::move(done)](bool saved) mutable { const auto found = this->unsaved_records_.find(id); + // A save of the record was made meanwhile, so the record in memory is saved again + if (saved && found->second.saved_meanwhile) { + found->second.saved_meanwhile = false; + this->save_change(id, std::move(done)); + return; + } const UnsavedRecord unsaved = std::move(found->second); this->unsaved_records_.erase(found); if (saved) { @@ -1109,24 +1137,31 @@ void ServerCore::store_long_term_psk(const std::string& client_id, const Psk& ps for (Client* client : connected) { this->reactivate(*client); } - }); + }; + this->save_record(client_id, this->records_.find(client_id), std::move(on_saved)); } -void ServerCore::put_back(const std::string& client_id, const UnsavedRecord& unsaved) { - const ClientRecord* current = this->records_.find(client_id); +std::optional ServerCore::without_change(const ClientRecord* current, + const UnsavedRecord& unsaved) { if (!unsaved.existed && !unsaved.approval_set) { - if (const std::optional slot = this->records_.remove(client_id)) { - this->erase_record(client_id, *slot); - } - } else { - // A name saved meanwhile stays - ClientRecord restored = current != nullptr ? *current : unsaved.before; - restored.long_term_psk = unsaved.before.long_term_psk; - if (!unsaved.approval_set) { - restored.approval = unsaved.before.approval; - } - this->records_.set(restored); + return std::nullopt; + } + // A name saved meanwhile stays + ClientRecord restored = current != nullptr ? *current : unsaved.before; + restored.long_term_psk = unsaved.before.long_term_psk; + if (!unsaved.approval_set) { + restored.approval = unsaved.before.approval; + } + return restored; +} + +void ServerCore::put_back(const std::string& client_id, const UnsavedRecord& unsaved) { + if (const std::optional restored = + without_change(this->records_.find(client_id), unsaved)) { + this->records_.set(*restored); this->save_record(client_id); + } else if (const std::optional slot = this->records_.remove(client_id)) { + this->erase_record(client_id, *slot); } const bool paired = this->long_term_psk(client_id) != nullptr; this->for_each_connection(client_id, [this, &client_id, paired](Client& client) { diff --git a/src/server_core.h b/src/server_core.h index c332b87..245bab1 100644 --- a/src/server_core.h +++ b/src/server_core.h @@ -227,6 +227,9 @@ class ServerCore { bool existed{false}; /// set_approval() ran meanwhile, so putting the change back keeps the approval it set bool approval_set{false}; + /// A save of the record was made meanwhile, so the record in memory is saved again before + /// the change is durable + bool saved_meanwhile{false}; }; /// @brief A completed pairing's record (pairing.md "Pairing Records"): sets client_id's @@ -234,10 +237,12 @@ class ServerCore { /// record and calls done on the core thread with whether it is durable and the change's /// UnsavedRecord. Until then handshakes reference the PSK of the record as it was, and a /// failed save puts the change back (put_back()), so nothing uses the new PSK before it is - /// persisted (pairing.md "server/pair-finalize"). After done, every connection of the client - /// is activated again under its record. A second store for a client while one is in flight - /// gets done(false) at once. done is dropped once the core stops, and shutdown() puts the - /// change back on disk. + /// persisted (pairing.md "server/pair-finalize"). A save of the record made meanwhile leaves + /// the change out (save_record()), so the change is durable only once a save of it lands with + /// none made meanwhile (save_change()). After done, every connection of the client is + /// activated again under its record. A second store for a client while one is in flight gets + /// done(false) at once. done is dropped once the core stops, and shutdown() puts the change + /// back on disk. void store_long_term_psk(const std::string& client_id, const Psk& psk, std::function done); @@ -605,13 +610,29 @@ class ServerCore { /// @brief Re-handshakes every activated connection of client_id to psk (connection.md /// "Re-handshake") void rekey(const std::string& client_id, const HandshakePsk& psk); - /// @brief Saves client_id's record through the provider on the dispatch thread, reporting a - /// failure with on_error(); then, unless the core stopped, calls done on the core thread with - /// whether it is durable. A failed save without done is written again (FailedWrites). Without - /// a provider nothing is saved and done gets true. The save may finish after the core is - /// destroyed: it reaches the core only through loop_, which outlives the dispatch thread, and - /// a handler posted after the loop ended never runs. - void save_record(const std::string& client_id, std::function done = nullptr); + /// @brief Saves client_id's record in memory, as the other overload describes, without done. + /// While a store_long_term_psk() change is being saved, the record saved is without_change()'s, + /// and none when that is nullopt. + void save_record(const std::string& client_id); + /// @brief Saves record (nullptr: none, which fails) as client_id's through the provider on the + /// dispatch thread, reporting a failure with on_error(); then, unless the core stopped, calls + /// done on the core thread with whether it is durable. A failed save without done is written + /// again (FailedWrites). Without a provider nothing is saved and done gets true. The save may + /// finish after the core is destroyed: it reaches the core only through loop_, which outlives + /// the dispatch thread, and a handler posted after the loop ended never runs. + void save_record(const std::string& client_id, const ClientRecord* record, + std::function done); + /// @brief current, client_id's record in memory, as put_back() leaves it: the long-term PSK + /// and, unless set_approval() ran meanwhile, the approval of unsaved's record before, with + /// current's other fields; nullopt when the change created the record and no approval was set + static std::optional without_change(const ClientRecord* current, + const UnsavedRecord& unsaved); + + /// @brief Saves the record in memory, which holds client_id's store_long_term_psk() change, + /// again when a save made meanwhile left the change out, then ends the change as + /// store_long_term_psk() describes + void save_change(const std::string& client_id, + std::function done); /// @brief Saves an all-zero blob, a free slot, in slot, once client_id's record is removed void erase_record(const std::string& client_id, size_t slot); /// @brief A record's blob, wiped when the last holder releases it diff --git a/tests/test_server_trust.cpp b/tests/test_server_trust.cpp index e5b6b67..f8dfb9e 100644 --- a/tests/test_server_trust.cpp +++ b/tests/test_server_trust.cpp @@ -613,15 +613,36 @@ TEST(ServerCoreTrust, KeysASendspinCppSessionWithItsPairingPsk) { } } -/// A provider whose saves wait for release(), and fail when told to -class HeldProvider : public MemoryProvider { +/// A provider whose first saves succeed or fail as scripted; every later save succeeds +class ScriptedProvider : public MemoryProvider { public: + explicit ScriptedProvider(std::vector accepted = {}) : accepted_(std::move(accepted)) {} + + bool save_blob(const std::string& key, const uint8_t* data, size_t len) override { + const auto index = static_cast(this->saves); + if (index < this->accepted_.size() && !this->accepted_[index]) { + ++this->saves; + return false; + } + return MemoryProvider::save_blob(key, data, len); + } + +private: + std::vector accepted_; +}; + +/// A provider whose saves wait for release(), then succeed or fail as scripted, and fail when told +/// to +class HeldProvider : public ScriptedProvider { +public: + using ScriptedProvider::ScriptedProvider; + bool save_blob(const std::string& key, const uint8_t* data, size_t len) override { std::unique_lock lock(this->mutex_); this->changed_.wait(lock, [this] { return this->released_ > 0; }); --this->released_; lock.unlock(); - return MemoryProvider::save_blob(key, data, len); + return ScriptedProvider::save_blob(key, data, len); } /// Lets count saves through @@ -743,8 +764,8 @@ TEST(ServerCoreTrust, HoldsOneLongTermPskChangePerClient) { ASSERT_NO_FATAL_FAILURE(client.activate(player_hello("New"))); core.on_core( [&] { core.core->set_approval(id, sendspin::server::ApprovalScope::PLAYBACK); }); - // The pairing's, the name's, the approval's, and for a failure the restoring save and its - // retry at shutdown + // The pairing's, the name's, the approval's, then the pairing's again for a durable save, + // or the restoring save and its retry at shutdown for a failure provider.release(row.refuse_save ? 5 : 4); EXPECT_EQ(done.get_future().get(), !row.refuse_save); const ClientRecord record = core.on_core([&] { return *core.core->records_.find(id); }); @@ -798,24 +819,6 @@ TEST(ServerCoreTrust, FinishesASaveTheCoreDidNotOutlive) { } } -/// A provider whose first saves succeed or fail as scripted; every later save succeeds -class ScriptedProvider : public MemoryProvider { -public: - explicit ScriptedProvider(std::vector accepted) : accepted_(std::move(accepted)) {} - - bool save_blob(const std::string& key, const uint8_t* data, size_t len) override { - const auto index = static_cast(this->saves); - if (index < this->accepted_.size() && !this->accepted_[index]) { - ++this->saves; - return false; - } - return MemoryProvider::save_blob(key, data, len); - } - -private: - std::vector accepted_; -}; - /// A provider whose saves wait for release(): the first lands but its commit is refused, and the /// second is refused class PutBackProvider : public MemoryProvider { @@ -982,6 +985,138 @@ TEST(ServerCoreTrust, RetriesAPutBackMadeAtShutdown) { EXPECT_EQ(*loaded, approved); } +// A record saved while a long-term PSK's save is in flight leaves that PSK out, so a refused PSK +// save never reaches disk through a save queued behind it (pairing.md "server/pair-finalize"), +// even when the put-back's write is refused too. Meanwhile the client connects under a new name, +// which saves its record, the operator approves it, or both. The change is durable only once a +// save of it lands with none made meanwhile, so a refused save of it again puts it back. Control: +// a PSK save that lands is saved again behind them, and durable. +TEST(ServerCoreTrust, KeepsAnUnconfirmedLongTermPskOffDisk) { + ASSERT_EQ(noise_init_framework(), NOISE_ERROR_NONE); + using sendspin::server::ApprovalScope; + using sendspin::server::ServerCore; + // Each row's connection is the same client + const Identity identity = *Identity::generate(); + const std::string a = identity.peer_id(); + const std::string b = Identity::generate()->peer_id(); + const Psk psk = psk_of(0xF0); + ClientRecord named_a{a, std::nullopt}; + named_a.name = "Old"; + ClientRecord paired_b{b, std::nullopt}; + paired_b.long_term_psk = psk_of(0xF1); + const auto renamed = [&](std::optional approval, std::optional long_term) { + ClientRecord record{a, approval}; + record.name = "New"; + record.long_term_psk = long_term; + return record; + }; + ClientRecord approved_a = named_a; + approved_a.approval = ApprovalScope::PLAYBACK; + struct Row { + const char* name; + /// Saved in slots 0, 1 and so on before the start + std::vector stored; + /// The client connects under the name "New" while the PSK is being saved + bool connect; + std::function meanwhile; + /// The outcomes of the first saves + std::vector accepted; + /// What the next start loads + std::vector restored; + /// The change's outcome + bool durable; + }; + const auto approve = [&](ServerCore& core) { core.set_approval(a, ApprovalScope::PLAYBACK); }; + const Row rows[] = { + {"Control: the PSK saved, a name and an approval behind it", + {named_a}, + true, + approve, + {}, + {renamed(ApprovalScope::PLAYBACK, psk)}, + true}, + {"Control: the PSK saved for a new record, a name behind it", + {}, + true, + nullptr, + {}, + {renamed(std::nullopt, psk)}, + true}, + // The PSK's, the name's, the approval's, the put-back's and its retry at shutdown + {"the PSK's save refused, a name's and an approval's saved, the put-back's refused", + {named_a}, + true, + approve, + {false, true, true, false, false}, + {renamed(ApprovalScope::PLAYBACK, std::nullopt)}, + false}, + // Nothing is saved meanwhile for a record the change created, so the erase that puts it + // back is the second save; a name saved second would leave the erase and its retry refused + {"the PSK's save for a new record refused, a name meanwhile", + {}, + true, + nullptr, + {false, true, false, false}, + {}, + false}, + // The PSK's, the approval's, the PSK's again, refused, and the put-back's + {"the PSK's saved, an approval's behind it, the PSK's again refused", + {named_a}, + false, + approve, + {true, true, false}, + {approved_a}, + false}, + // The approval's save, refused, goes again ahead of b's erase + {"the PSK's and an approval's saves refused, the approval's written again", + {named_a, paired_b}, + false, + [&](ServerCore& core) { + approve(core); + core.unpair(b); + }, + {false, false, true, true, false, false}, + {approved_a}, + false}, + }; + for (const Row& row : rows) { + SCOPED_TRACE(row.name); + HeldProvider provider(row.accepted); + for (size_t slot = 0; slot < row.stored.size(); ++slot) { + provider.blobs[persistence_keys::client_record_key(slot)] = + record_blob(row.stored[slot]); + } + { + Core core(load_records(provider), &provider); + std::promise done; + core.on_core([&] { + core.core->store_long_term_psk( + a, psk, [&](bool saved, const auto& /*unsaved*/) { done.set_value(saved); }); + }); + RawClient client(sendspin::server::CipherSuite::CHACHAPOLY_SHA256, + *Identity::from_private_bytes(identity.private_bytes)); + if (row.connect) { + core.dial(client.url()); + ASSERT_NO_FATAL_FAILURE(client.activate(player_hello("New"))); + } + if (row.meanwhile) { + core.on_core([&] { row.meanwhile(*core.core); }); + } + constexpr int EVERY_SAVE = 8; + provider.release(EVERY_SAVE); + // The outcome before the core stops + EXPECT_EQ(done.get_future().get(), row.durable); + } + const sendspin::server::ClientRecords restored = load_records(provider); + EXPECT_EQ(restored.size(), row.restored.size()); + for (const ClientRecord& expected : row.restored) { + const ClientRecord* loaded = restored.find(expected.client_id); + ASSERT_NE(loaded, nullptr); + EXPECT_EQ(*loaded, expected); + } + } +} + // An attempt whose client stops answering mid-round (here after server/pair-auth) ends as // TIMED_OUT after ServerCore's client-step timeout (PAIRING_CLIENT_STEP_TIMEOUT_US, shortened // here), with an activation without 'pairing'