Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix in-place save for DefaultSessionResumptionStorage #23062

Merged
merged 6 commits into from
Oct 10, 2022
Merged

Fix in-place save for DefaultSessionResumptionStorage #23062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
724ee3d
Fix in-place save for DefaultSessionResumptionStorage
msandstedt Oct 6, 2022
2df949f
Add unit test for in-place save operations in DefaultSessionResumptio…
msandstedt Oct 7, 2022
e0007b3
per Andrei Litvin, disambiguate logging
msandstedt Oct 7, 2022
1642b9a
fix TestDefaultSessionResumptionStorage build
msandstedt Oct 7, 2022
03fb7d9
add explicit checks for storage leaks in TestDefaultSessionResumption…
msandstedt Oct 7, 2022
1029f85
initilize SyncGetKeyValue size param to 0
msandstedt Oct 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/protocols/secure_channel/DefaultSessionResumptionStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/protocols/secure_channel/SimpleSessionResumptionStorage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
238 changes: 217 additions & 21 deletions src/protocols/secure_channel/tests/TestDefaultSessionResumptionStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@
* limitations under the License.
*/

#include <lib/support/DefaultStorageKeyAllocator.h>
#include <lib/support/TestPersistentStorageDelegate.h>
#include <lib/support/UnitTestRegistration.h>
#include <nlunit-test.h>

#include <lib/support/TestPersistentStorageDelegate.h>

// DefaultSessionResumptionStorage is a partial implementation.
// Use SimpleSessionResumptionStorage, which extends it, to test.
#include <protocols/secure_channel/SimpleSessionResumptionStorage.h>

#define ARRAY_SIZE(_array) ((sizeof(_array) / sizeof(_array[0])))
msandstedt marked this conversation as resolved.
Show resolved Hide resolved

void TestSave(nlTestSuite * inSuite, void * inContext)
{
chip::SimpleSessionResumptionStorage sessionStorage;
Expand All @@ -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()));
Expand Down Expand Up @@ -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);
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"retrieval"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR here: #23110.

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
msandstedt marked this conversation as resolved.
Show resolved Hide resolved
// 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<chip::NodeId>(rand()), static_cast<chip::FabricIndex>(i + 1));
} while (!nodes[i].IsOperational());
}

// Populate test vectors.
for (size_t i = 0; i < ARRAY_SIZE(vectors); ++i)
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
{
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<uint8_t>(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<chip::CASEAuthTag>(rand());
vectors[i].cats.values[1] = static_cast<chip::CASEAuthTag>(rand());
vectors[i].cats.values[2] = static_cast<chip::CASEAuthTag>(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;
Expand Down Expand Up @@ -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);
msandstedt marked this conversation as resolved.
Show resolved Hide resolved

// 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)
Expand All @@ -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()));
Expand All @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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);
}
}
}
}
Expand All @@ -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),

Expand Down