diff --git a/src/protocols/secure_channel/DefaultSessionResumptionStorage.cpp b/src/protocols/secure_channel/DefaultSessionResumptionStorage.cpp index 0a84ec88768f7e..1d7b907fa062e7 100644 --- a/src/protocols/secure_channel/DefaultSessionResumptionStorage.cpp +++ b/src/protocols/secure_channel/DefaultSessionResumptionStorage.cpp @@ -52,6 +52,44 @@ CHIP_ERROR DefaultSessionResumptionStorage::Save(const ScopedNodeId & node, Cons SessionIndex index; ReturnErrorOnFailure(LoadIndex(index)); + for (size_t i = 0; i < index.mSize; ++i) + { + if (index.mNodes[i] == node) + { + // Node already exists in the index. Save in place. + CHIP_ERROR err = CHIP_NO_ERROR; + ResumptionIdStorage oldResumptionId; + Crypto::P256ECDHDerivedSecret oldSharedSecret; + CATValues oldPeerCATs; + // This follows the approach in Delete. Removal of the old + // resumption-id-keyed link is best effort. If we cannot load + // state to lookup the resumption ID for the key, the entry in + // the link table will be leaked. + err = LoadState(node, oldResumptionId, oldSharedSecret, oldPeerCATs); + if (err != CHIP_NO_ERROR) + { + ChipLogError(SecureChannel, + "LoadState failed; unable to fully delete session resumption record for node " ChipLogFormatX64 + ": %" CHIP_ERROR_FORMAT, + ChipLogValueX64(node.GetNodeId()), err.Format()); + } + else + { + err = DeleteLink(oldResumptionId); + if (err != CHIP_NO_ERROR) + { + ChipLogError(SecureChannel, + "DeleteLink failed; unable to fully delete session resumption record for node " ChipLogFormatX64 + ": %" CHIP_ERROR_FORMAT, + ChipLogValueX64(node.GetNodeId()), err.Format()); + } + } + ReturnErrorOnFailure(SaveState(node, resumptionId, sharedSecret, peerCATs)); + ReturnErrorOnFailure(SaveLink(resumptionId, node)); + return CHIP_NO_ERROR; + } + } + if (index.mSize == CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE) { // TODO: implement LRU for resumption diff --git a/src/protocols/secure_channel/SimpleSessionResumptionStorage.h b/src/protocols/secure_channel/SimpleSessionResumptionStorage.h index 875ad79af8203b..0fd2a9e3620354 100644 --- a/src/protocols/secure_channel/SimpleSessionResumptionStorage.h +++ b/src/protocols/secure_channel/SimpleSessionResumptionStorage.h @@ -56,10 +56,10 @@ class SimpleSessionResumptionStorage : public DefaultSessionResumptionStorage Crypto::P256ECDHDerivedSecret & sharedSecret, CATValues & peerCATs) override; CHIP_ERROR DeleteState(const ScopedNodeId & node) override; -private: static const char * StorageKey(DefaultStorageKeyAllocator & keyAlloc, const ScopedNodeId & node); static const char * StorageKey(DefaultStorageKeyAllocator & keyAlloc, ConstResumptionIdView resumptionId); +private: static constexpr size_t MaxScopedNodeIdSize() { return TLV::EstimateStructOverhead(sizeof(NodeId), sizeof(FabricIndex)); } static constexpr size_t MaxIndexSize() diff --git a/src/protocols/secure_channel/tests/TestDefaultSessionResumptionStorage.cpp b/src/protocols/secure_channel/tests/TestDefaultSessionResumptionStorage.cpp index fc55355f57b556..334801ae98dc95 100644 --- a/src/protocols/secure_channel/tests/TestDefaultSessionResumptionStorage.cpp +++ b/src/protocols/secure_channel/tests/TestDefaultSessionResumptionStorage.cpp @@ -15,15 +15,17 @@ * limitations under the License. */ +#include +#include #include #include -#include - // DefaultSessionResumptionStorage is a partial implementation. // Use SimpleSessionResumptionStorage, which extends it, to test. #include +#define ARRAY_SIZE(_array) ((sizeof(_array) / sizeof(_array[0]))) + void TestSave(nlTestSuite * inSuite, void * inContext) { chip::SimpleSessionResumptionStorage sessionStorage; @@ -38,7 +40,7 @@ void TestSave(nlTestSuite * inSuite, void * inContext) } vectors[CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE + 1]; // Populate test vectors. - for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i) + for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i) { NL_TEST_ASSERT( inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(vectors[i].resumptionId.data(), vectors[i].resumptionId.size())); @@ -68,7 +70,7 @@ void TestSave(nlTestSuite * inSuite, void * inContext) // If more sophisticated LRU behavior is implemented, this test // case should be modified to match. { - size_t last = sizeof(vectors) / sizeof(vectors[0]) - 1; + size_t last = ARRAY_SIZE(vectors) - 1; NL_TEST_ASSERT(inSuite, sessionStorage.Save(vectors[last].node, vectors[last].resumptionId, vectors[last].sharedSecret, vectors[last].cats) == CHIP_NO_ERROR); @@ -81,7 +83,124 @@ void TestSave(nlTestSuite * inSuite, void * inContext) } // Read back and verify values. - for (size_t i = 0; i < CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE; ++i) + for (auto & vector : vectors) + { + chip::ScopedNodeId outNode; + chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId; + chip::Crypto::P256ECDHDerivedSecret outSharedSecret; + chip::CATValues outCats; + + // Verify retrieval by node. + NL_TEST_ASSERT(inSuite, + sessionStorage.FindByScopedNodeId(vector.node, outResumptionId, outSharedSecret, outCats) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, memcmp(vector.resumptionId.data(), outResumptionId.data(), vector.resumptionId.size()) == 0); + NL_TEST_ASSERT(inSuite, memcmp(vector.sharedSecret.ConstBytes(), outSharedSecret, vector.sharedSecret.Length()) == 0); + NL_TEST_ASSERT(inSuite, vector.cats.values[0] == outCats.values[0]); + NL_TEST_ASSERT(inSuite, vector.cats.values[1] == outCats.values[1]); + NL_TEST_ASSERT(inSuite, vector.cats.values[2] == outCats.values[2]); + + // Validate retreiveal by resumption ID. + NL_TEST_ASSERT(inSuite, + sessionStorage.FindByResumptionId(vector.resumptionId, outNode, outSharedSecret, outCats) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, vector.node == outNode); + NL_TEST_ASSERT(inSuite, memcmp(vector.sharedSecret.Bytes(), outSharedSecret, vector.sharedSecret.Length()) == 0); + NL_TEST_ASSERT(inSuite, vector.cats.values[0] == outCats.values[0]); + NL_TEST_ASSERT(inSuite, vector.cats.values[1] == outCats.values[1]); + NL_TEST_ASSERT(inSuite, vector.cats.values[2] == outCats.values[2]); + } +} + +void TestInPlaceSave(nlTestSuite * inSuite, void * inContext) +{ + chip::SimpleSessionResumptionStorage sessionStorage; + chip::TestPersistentStorageDelegate storage; + sessionStorage.Init(&storage); + struct + { + chip::SessionResumptionStorage::ResumptionIdStorage resumptionId; + chip::Crypto::P256ECDHDerivedSecret sharedSecret; + chip::ScopedNodeId node; + chip::CATValues cats; + } vectors[CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE + 10]; + + // Construct only a few unique node identities to simiulate talking to a + // couple peers. + chip::ScopedNodeId nodes[3]; + static_assert(ARRAY_SIZE(nodes) < CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE, + "must have fewer nodes than slots in session resumption storage"); + for (size_t i = 0; i < ARRAY_SIZE(nodes); ++i) + { + do + { + nodes[i] = chip::ScopedNodeId(static_cast(rand()), static_cast(i + 1)); + } while (!nodes[i].IsOperational()); + } + + // Populate test vectors. + for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i) + { + NL_TEST_ASSERT( + inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(vectors[i].resumptionId.data(), vectors[i].resumptionId.size())); + *vectors[i].resumptionId.data() = + static_cast(i); // set first byte to our index to ensure uniqueness for the FindByResumptionId call + vectors[i].sharedSecret.SetLength(vectors[i].sharedSecret.Capacity()); + NL_TEST_ASSERT(inSuite, + CHIP_NO_ERROR == + chip::Crypto::DRBG_get_bytes(vectors[i].sharedSecret.Bytes(), vectors[i].sharedSecret.Length())); + vectors[i].node = nodes[i % ARRAY_SIZE(nodes)]; + vectors[i].cats.values[0] = static_cast(rand()); + vectors[i].cats.values[1] = static_cast(rand()); + vectors[i].cats.values[2] = static_cast(rand()); + } + + // Add one entry for each node. + for (size_t i = 0; i < ARRAY_SIZE(nodes); ++i) + { + NL_TEST_ASSERT(inSuite, + sessionStorage.Save(vectors[i].node, vectors[i].resumptionId, vectors[i].sharedSecret, vectors[i].cats) == + CHIP_NO_ERROR); + } + + // Read back and verify values. + for (size_t i = 0; i < ARRAY_SIZE(nodes); ++i) + { + chip::ScopedNodeId outNode; + chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId; + chip::Crypto::P256ECDHDerivedSecret outSharedSecret; + chip::CATValues outCats; + + // Verify retrieval by node. + NL_TEST_ASSERT(inSuite, + sessionStorage.FindByScopedNodeId(vectors[i].node, outResumptionId, outSharedSecret, outCats) == + CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, + memcmp(vectors[i].resumptionId.data(), outResumptionId.data(), vectors[i].resumptionId.size()) == 0); + NL_TEST_ASSERT(inSuite, memcmp(vectors[i].sharedSecret.Bytes(), outSharedSecret, vectors[i].sharedSecret.Length()) == 0); + NL_TEST_ASSERT(inSuite, vectors[i].cats.values[0] == outCats.values[0]); + NL_TEST_ASSERT(inSuite, vectors[i].cats.values[1] == outCats.values[1]); + NL_TEST_ASSERT(inSuite, vectors[i].cats.values[2] == outCats.values[2]); + + // Validate retreiveal by resumption ID. + NL_TEST_ASSERT(inSuite, + sessionStorage.FindByResumptionId(vectors[i].resumptionId, outNode, outSharedSecret, outCats) == + CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, vectors[i].node == outNode); + NL_TEST_ASSERT(inSuite, memcmp(vectors[i].sharedSecret.Bytes(), outSharedSecret, vectors[i].sharedSecret.Length()) == 0); + NL_TEST_ASSERT(inSuite, vectors[i].cats.values[0] == outCats.values[0]); + NL_TEST_ASSERT(inSuite, vectors[i].cats.values[1] == outCats.values[1]); + NL_TEST_ASSERT(inSuite, vectors[i].cats.values[2] == outCats.values[2]); + } + + // Now add all test vectors. This should overwrite each node's record + // many times. + for (auto & vector : vectors) + { + NL_TEST_ASSERT(inSuite, + sessionStorage.Save(vector.node, vector.resumptionId, vector.sharedSecret, vector.cats) == CHIP_NO_ERROR); + } + + // Read back and verify that only the last record for each node was retained. + for (size_t i = ARRAY_SIZE(vectors) - ARRAY_SIZE(nodes); i < ARRAY_SIZE(vectors); ++i) { chip::ScopedNodeId outNode; chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId; @@ -109,6 +228,49 @@ void TestSave(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, vectors[i].cats.values[1] == outCats.values[1]); NL_TEST_ASSERT(inSuite, vectors[i].cats.values[2] == outCats.values[2]); } + + // Remove all records for all fabrics. If all three tables of (index, state, + // links) are in sync, deleting for each fabric should clean error free. + for (const auto & node : nodes) + { + NL_TEST_ASSERT(inSuite, sessionStorage.DeleteAll(node.GetFabricIndex()) == CHIP_NO_ERROR); + } + + // Verify that no entries can be located any longer for any node or + // resumption ID. + for (auto & vector : vectors) + { + chip::ScopedNodeId outNode; + chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId; + chip::Crypto::P256ECDHDerivedSecret outSharedSecret; + chip::CATValues outCats; + + // Verify all records for all nodes are gone. + NL_TEST_ASSERT(inSuite, + sessionStorage.FindByScopedNodeId(vector.node, outResumptionId, outSharedSecret, outCats) != CHIP_NO_ERROR); + + // Verify all records for all resumption IDs are gone. + NL_TEST_ASSERT(inSuite, + sessionStorage.FindByResumptionId(vector.resumptionId, outNode, outSharedSecret, outCats) != CHIP_NO_ERROR); + } + + // Verify no state table persistent storage entries were leaked. + for (const auto & node : nodes) + { + uint16_t size = 0; + chip::DefaultStorageKeyAllocator keyAlloc; + auto rv = storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, node), nullptr, size); + NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + } + // Verify no link table persistent storage entries were leaked. + for (auto & vector : vectors) + { + uint16_t size = 0; + chip::DefaultStorageKeyAllocator keyAlloc; + auto rv = + storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, vector.resumptionId), nullptr, size); + NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + } } void TestDelete(nlTestSuite * inSuite, void * inContext) @@ -128,7 +290,7 @@ void TestDelete(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(sharedSecret.Bytes(), sharedSecret.Length())); // Populate test vectors. - for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i) + for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i) { NL_TEST_ASSERT( inSuite, CHIP_NO_ERROR == chip::Crypto::DRBG_get_bytes(vectors[i].resumptionId.data(), vectors[i].resumptionId.size())); @@ -146,7 +308,7 @@ void TestDelete(nlTestSuite * inSuite, void * inContext) } // Delete values in turn from storage and verify they are removed. - for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i) + for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i) { chip::ScopedNodeId outNode; chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId; @@ -160,6 +322,23 @@ void TestDelete(nlTestSuite * inSuite, void * inContext) sessionStorage.FindByResumptionId(vectors[i].resumptionId, outNode, outSharedSecret, outCats) != CHIP_NO_ERROR); } + + // Verify no state or link table persistent storage entries were leaked. + for (auto & vector : vectors) + { + uint16_t size = 0; + chip::DefaultStorageKeyAllocator keyAlloc; + { + auto rv = + storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, vector.node), nullptr, size); + NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + } + { + auto rv = storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, vector.resumptionId), + nullptr, size); + NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + } + } } void TestDeleteAll(nlTestSuite * inSuite, void * inContext) @@ -197,38 +376,54 @@ void TestDeleteAll(nlTestSuite * inSuite, void * inContext) } // Fill storage. - for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i) + for (auto & vector : vectors) { - for (size_t j = 0; j < sizeof(vectors[0].nodes) / sizeof(vectors[0].nodes[0]); ++j) + for (auto & node : vector.nodes) { NL_TEST_ASSERT(inSuite, - sessionStorage.Save(vectors[i].nodes[j].node, vectors[i].nodes[j].resumptionId, sharedSecret, - chip::CATValues{}) == CHIP_NO_ERROR); + sessionStorage.Save(node.node, node.resumptionId, sharedSecret, chip::CATValues{}) == CHIP_NO_ERROR); } } // Validate Fabric deletion. - for (size_t i = 0; i < sizeof(vectors) / sizeof(vectors[0]); ++i) + for (const auto & vector : vectors) { chip::ScopedNodeId outNode; chip::SessionResumptionStorage::ResumptionIdStorage outResumptionId; chip::Crypto::P256ECDHDerivedSecret outSharedSecret; chip::CATValues outCats; // Verify fabric node entries exist. - for (size_t j = 0; j < sizeof(vectors[0].nodes) / sizeof(vectors[0].nodes[0]); ++j) + for (const auto & node : vector.nodes) { - NL_TEST_ASSERT(inSuite, - sessionStorage.FindByScopedNodeId(vectors[i].nodes[j].node, outResumptionId, outSharedSecret, outCats) == - CHIP_NO_ERROR); + NL_TEST_ASSERT( + inSuite, sessionStorage.FindByScopedNodeId(node.node, outResumptionId, outSharedSecret, outCats) == CHIP_NO_ERROR); } // Delete fabric. - NL_TEST_ASSERT(inSuite, sessionStorage.DeleteAll(vectors[i].fabricIndex) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, sessionStorage.DeleteAll(vector.fabricIndex) == CHIP_NO_ERROR); // Verify fabric node entries no longer exist. - for (size_t j = 0; j < sizeof(vectors[0].nodes) / sizeof(vectors[0].nodes[0]); ++j) + for (const auto & node : vector.nodes) { - NL_TEST_ASSERT(inSuite, - sessionStorage.FindByScopedNodeId(vectors[i].nodes[j].node, outResumptionId, outSharedSecret, outCats) != - CHIP_NO_ERROR); + NL_TEST_ASSERT( + inSuite, sessionStorage.FindByScopedNodeId(node.node, outResumptionId, outSharedSecret, outCats) != CHIP_NO_ERROR); + } + } + // Verify no state or link table persistent storage entries were leaked. + for (auto & vector : vectors) + { + for (auto & node : vector.nodes) + { + uint16_t size = 0; + chip::DefaultStorageKeyAllocator keyAlloc; + { + auto rv = + storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, node.node), nullptr, size); + NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + } + { + auto rv = storage.SyncGetKeyValue(chip::SimpleSessionResumptionStorage::StorageKey(keyAlloc, node.resumptionId), + nullptr, size); + NL_TEST_ASSERT(inSuite, rv == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND); + } } } } @@ -242,6 +437,7 @@ void TestDeleteAll(nlTestSuite * inSuite, void * inContext) static const nlTest sTests[] = { NL_TEST_DEF("TestSave", TestSave), + NL_TEST_DEF("TestInPlaceSave", TestInPlaceSave), NL_TEST_DEF("TestDelete", TestDelete), NL_TEST_DEF("TestDeleteAll", TestDeleteAll),