From 5954e899a71ef79d274adb8dc5c92b43cd59abd8 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Tue, 20 Aug 2024 11:17:17 -0400 Subject: [PATCH 1/9] Add a test case that demonstrates tree validation failiure --- test/state.cpp | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/test/state.cpp b/test/state.cpp index 0f922ff2..50d2d7a4 100644 --- a/test/state.cpp +++ b/test/state.cpp @@ -1069,3 +1069,87 @@ TEST_CASE_METHOD(RelatedGroupTest, "Reinitialize the group") REQUIRE(state == new_states[0]); } } + +TEST_CASE_METHOD(StateTest, "Invalid Tree2") +{ + // ACT I: Member 0 sets up the group + + // Create the group + states.emplace_back(group_id, + suite, + leaf_privs[0], + identity_privs[0], + key_packages[0].leaf_node, + ExtensionList{}); + + // Add second and third members, each in their own Commit + for (size_t i = 1; i < 3; i += 1) { + auto add_proposal = states[0].add_proposal(key_packages[i]); + + auto [commit, welcome, new_state_0] = + states[0].commit(fresh_secret(), CommitOpts{ { add_proposal }, true, false, {} }, {}); + silence_unused(commit); + states[0] = new_state_0; + + states.push_back({ init_privs[i], + leaf_privs[i], + identity_privs[i], + key_packages[i], + welcome, + std::nullopt, + {} }); + } + + // ACT II: Member @2 makes additional changes to the group + + // Member @2 adds a fourth member + auto add_proposal_3 = states[2].add_proposal(key_packages[3]); + + auto [commit_add_3, welcome_add_3, new_state_add_3] = + states[2].commit(fresh_secret(), CommitOpts{ { add_proposal_3 }, true, false, {} }, {}); + silence_unused(commit_add_3); + states[2] = new_state_add_3; + + CHECK_NOTHROW(State{ init_privs[3], + leaf_privs[3], + identity_privs[3], + key_packages[3], + welcome_add_3, + std::nullopt, + {} }); + + // Member @2 removes member @1 + auto remove_proposal_1 = states[2].remove_proposal(LeafIndex { 1 }); + + auto [commit_remove_1, welcome_remove_1, new_state_remove_1] = states[2].commit( + fresh_secret(), CommitOpts{ { remove_proposal_1 }, true, false, {} }, {}); + silence_unused(welcome_remove_1); + states[2] = new_state_remove_1; + + // Have member at leaf index 2 remove member at leaf index 0 + auto remove_proposal_0 = states[2].remove_proposal(LeafIndex { 0 }); + + auto [commit_remove_0, welcome_remove_0, new_state_remove_0] = states[2].commit( + fresh_secret(), CommitOpts{ { remove_proposal_0 }, true, false, {} }, {}); + silence_unused(welcome_remove_0); + states[2] = new_state_remove_0; + + // ACT III: Member @2 adds another member + + // Add a new member + auto add_proposal_4 = states[2].add_proposal(key_packages[4]); + auto [commit_add_4, welcome_add_4, new_state_add_4] = + states[2].commit(fresh_secret(), CommitOpts{ { add_proposal_4 }, true, false, {} }, {}); + silence_unused(commit_add_4); + states[2] = new_state_add_4; + + CHECK_NOTHROW(State{ init_privs[4], + leaf_privs[4], + identity_privs[4], + key_packages[4], + welcome_add_4, + std::nullopt, + {} }); + + // verify_group_functionality(states); +} From 018fd9a913565546135e283aa8ae43ff09940b9b Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Tue, 20 Aug 2024 11:41:41 -0400 Subject: [PATCH 2/9] Fail faster --- test/state.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/state.cpp b/test/state.cpp index 50d2d7a4..f6a51230 100644 --- a/test/state.cpp +++ b/test/state.cpp @@ -1118,6 +1118,8 @@ TEST_CASE_METHOD(StateTest, "Invalid Tree2") std::nullopt, {} }); + REQUIRE(states[2].tree().parent_hash_valid()); + // Member @2 removes member @1 auto remove_proposal_1 = states[2].remove_proposal(LeafIndex { 1 }); @@ -1126,6 +1128,8 @@ TEST_CASE_METHOD(StateTest, "Invalid Tree2") silence_unused(welcome_remove_1); states[2] = new_state_remove_1; + REQUIRE(states[2].tree().parent_hash_valid()); + // Have member at leaf index 2 remove member at leaf index 0 auto remove_proposal_0 = states[2].remove_proposal(LeafIndex { 0 }); @@ -1134,6 +1138,8 @@ TEST_CASE_METHOD(StateTest, "Invalid Tree2") silence_unused(welcome_remove_0); states[2] = new_state_remove_0; + REQUIRE(states[2].tree().parent_hash_valid()); + // ACT III: Member @2 adds another member // Add a new member @@ -1143,6 +1149,8 @@ TEST_CASE_METHOD(StateTest, "Invalid Tree2") silence_unused(commit_add_4); states[2] = new_state_add_4; + REQUIRE(states[2].tree().parent_hash_valid()); + CHECK_NOTHROW(State{ init_privs[4], leaf_privs[4], identity_privs[4], From 3c8ad5c818990ba75655ff0583a82daf11c1f3c5 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Tue, 20 Aug 2024 13:47:28 -0400 Subject: [PATCH 3/9] Streamline test case --- test/state.cpp | 92 +++++++++++++------------------------------------- 1 file changed, 24 insertions(+), 68 deletions(-) diff --git a/test/state.cpp b/test/state.cpp index f6a51230..03026e1f 100644 --- a/test/state.cpp +++ b/test/state.cpp @@ -1070,11 +1070,9 @@ TEST_CASE_METHOD(RelatedGroupTest, "Reinitialize the group") } } -TEST_CASE_METHOD(StateTest, "Invalid Tree2") +TEST_CASE_METHOD(StateTest, "Parent Hash with Empty Left Subtree") { - // ACT I: Member 0 sets up the group - - // Create the group + // Create a group with 4 members states.emplace_back(group_id, suite, leaf_privs[0], @@ -1082,82 +1080,40 @@ TEST_CASE_METHOD(StateTest, "Invalid Tree2") key_packages[0].leaf_node, ExtensionList{}); - // Add second and third members, each in their own Commit - for (size_t i = 1; i < 3; i += 1) { - auto add_proposal = states[0].add_proposal(key_packages[i]); + const auto adds = std::vector{ + states[0].add_proposal(key_packages[1]), + states[0].add_proposal(key_packages[2]), + states[0].add_proposal(key_packages[3]), + }; - auto [commit, welcome, new_state_0] = - states[0].commit(fresh_secret(), CommitOpts{ { add_proposal }, true, false, {} }, {}); - silence_unused(commit); - states[0] = new_state_0; + auto [_commit0, welcome0, new_state_0] = + states[0].commit(fresh_secret(), CommitOpts{ adds, true, false, {} }, {}); + silence_unused(_commit0); + states[0] = new_state_0; + for (size_t i = 1; i < 4; i++) { states.push_back({ init_privs[i], leaf_privs[i], identity_privs[i], key_packages[i], - welcome, + welcome0, std::nullopt, {} }); } - // ACT II: Member @2 makes additional changes to the group - - // Member @2 adds a fourth member - auto add_proposal_3 = states[2].add_proposal(key_packages[3]); - - auto [commit_add_3, welcome_add_3, new_state_add_3] = - states[2].commit(fresh_secret(), CommitOpts{ { add_proposal_3 }, true, false, {} }, {}); - silence_unused(commit_add_3); - states[2] = new_state_add_3; - - CHECK_NOTHROW(State{ init_privs[3], - leaf_privs[3], - identity_privs[3], - key_packages[3], - welcome_add_3, - std::nullopt, - {} }); - - REQUIRE(states[2].tree().parent_hash_valid()); - - // Member @2 removes member @1 - auto remove_proposal_1 = states[2].remove_proposal(LeafIndex { 1 }); - - auto [commit_remove_1, welcome_remove_1, new_state_remove_1] = states[2].commit( - fresh_secret(), CommitOpts{ { remove_proposal_1 }, true, false, {} }, {}); - silence_unused(welcome_remove_1); - states[2] = new_state_remove_1; - - REQUIRE(states[2].tree().parent_hash_valid()); - - // Have member at leaf index 2 remove member at leaf index 0 - auto remove_proposal_0 = states[2].remove_proposal(LeafIndex { 0 }); - - auto [commit_remove_0, welcome_remove_0, new_state_remove_0] = states[2].commit( - fresh_secret(), CommitOpts{ { remove_proposal_0 }, true, false, {} }, {}); - silence_unused(welcome_remove_0); - states[2] = new_state_remove_0; - - REQUIRE(states[2].tree().parent_hash_valid()); + // Member @2 removes the members on the left side of the tree + const auto removes = std::vector{ + states[0].remove_proposal(LeafIndex{ 0 }), + states[0].remove_proposal(LeafIndex{ 1 }), + }; - // ACT III: Member @2 adds another member + auto [commit2, welcome2, new_state_2] = + states[2].commit(fresh_secret(), CommitOpts{ { removes }, true, false, {} }, {}); + silence_unused(commit2); + states[2] = new_state_2; - // Add a new member - auto add_proposal_4 = states[2].add_proposal(key_packages[4]); - auto [commit_add_4, welcome_add_4, new_state_add_4] = - states[2].commit(fresh_secret(), CommitOpts{ { add_proposal_4 }, true, false, {} }, {}); - silence_unused(commit_add_4); - states[2] = new_state_add_4; + // Member @2 should have a valid tree, even though its filtered direct path no + // longer goes to the root. REQUIRE(states[2].tree().parent_hash_valid()); - - CHECK_NOTHROW(State{ init_privs[4], - leaf_privs[4], - identity_privs[4], - key_packages[4], - welcome_add_4, - std::nullopt, - {} }); - - // verify_group_functionality(states); } From c3b564ad7cf8f3816f6dcbf431d0e840dd217ca9 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 20 Aug 2024 17:32:49 +0000 Subject: [PATCH 4/9] fix last parent pull in parent_hashes --- src/treekem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/treekem.cpp b/src/treekem.cpp index 53aeea1c..b9cefa00 100644 --- a/src/treekem.cpp +++ b/src/treekem.cpp @@ -906,6 +906,7 @@ TreeKEMPublicKey::parent_hashes( // excluding root, including leaf auto from_node = NodeIndex(from); auto dp = fdp; + auto [last, _res_last] = dp.back(); dp.pop_back(); dp.insert(dp.begin(), { from_node, {} }); @@ -914,7 +915,6 @@ TreeKEMPublicKey::parent_hashes( } // Parent hash for all the parents, starting from the root - auto last = NodeIndex::root(size); auto last_hash = bytes{}; auto ph = std::vector(dp.size()); for (int i = static_cast(dp.size()) - 1; i >= 0; i--) { From 5572bc5d9c14cebccdfa59ee04a4dcded56b61d0 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 20 Aug 2024 17:36:39 +0000 Subject: [PATCH 5/9] update the comment in parent_hashes --- src/treekem.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/treekem.cpp b/src/treekem.cpp index b9cefa00..d164289d 100644 --- a/src/treekem.cpp +++ b/src/treekem.cpp @@ -914,7 +914,8 @@ TreeKEMPublicKey::parent_hashes( throw ProtocolError("Malformed UpdatePath"); } - // Parent hash for all the parents, starting from the root + // Parent hash for all the parents, starting from the last entry of the + // filtered direct path auto last_hash = bytes{}; auto ph = std::vector(dp.size()); for (int i = static_cast(dp.size()) - 1; i >= 0; i--) { From cb57fb2702cd120f9600e2d50e1d7ffb41112bea Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 20 Aug 2024 17:50:57 +0000 Subject: [PATCH 6/9] make format as always --- test/state.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/state.cpp b/test/state.cpp index 03026e1f..8a8d2ef7 100644 --- a/test/state.cpp +++ b/test/state.cpp @@ -1107,11 +1107,10 @@ TEST_CASE_METHOD(StateTest, "Parent Hash with Empty Left Subtree") states[0].remove_proposal(LeafIndex{ 1 }), }; - auto [commit2, welcome2, new_state_2] = - states[2].commit(fresh_secret(), CommitOpts{ { removes }, true, false, {} }, {}); - silence_unused(commit2); - states[2] = new_state_2; - + auto [commit2, welcome2, new_state_2] = states[2].commit( + fresh_secret(), CommitOpts{ { removes }, true, false, {} }, {}); + silence_unused(commit2); + states[2] = new_state_2; // Member @2 should have a valid tree, even though its filtered direct path no // longer goes to the root. From 90924a12044dc24c74cc8dd08c9af757b793229f Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 20 Aug 2024 18:21:51 +0000 Subject: [PATCH 7/9] simplify the test further --- test/state.cpp | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/test/state.cpp b/test/state.cpp index 8a8d2ef7..b17cd4ca 100644 --- a/test/state.cpp +++ b/test/state.cpp @@ -1073,46 +1073,43 @@ TEST_CASE_METHOD(RelatedGroupTest, "Reinitialize the group") TEST_CASE_METHOD(StateTest, "Parent Hash with Empty Left Subtree") { // Create a group with 4 members - states.emplace_back(group_id, - suite, - leaf_privs[0], - identity_privs[0], - key_packages[0].leaf_node, - ExtensionList{}); + auto state_0 = State(group_id, + suite, + leaf_privs[0], + identity_privs[0], + key_packages[0].leaf_node, + ExtensionList{}); const auto adds = std::vector{ - states[0].add_proposal(key_packages[1]), - states[0].add_proposal(key_packages[2]), - states[0].add_proposal(key_packages[3]), + state_0.add_proposal(key_packages[1]), + state_0.add_proposal(key_packages[2]), + state_0.add_proposal(key_packages[3]), }; auto [_commit0, welcome0, new_state_0] = - states[0].commit(fresh_secret(), CommitOpts{ adds, true, false, {} }, {}); + state_0.commit(fresh_secret(), CommitOpts{ adds, true, false, {} }, {}); silence_unused(_commit0); - states[0] = new_state_0; + state_0 = new_state_0; - for (size_t i = 1; i < 4; i++) { - states.push_back({ init_privs[i], - leaf_privs[i], - identity_privs[i], - key_packages[i], + auto state_2 = State(init_privs[2], + leaf_privs[2], + identity_privs[2], + key_packages[2], welcome0, std::nullopt, - {} }); - } - + {}); // Member @2 removes the members on the left side of the tree const auto removes = std::vector{ - states[0].remove_proposal(LeafIndex{ 0 }), - states[0].remove_proposal(LeafIndex{ 1 }), + state_2.remove_proposal(LeafIndex{ 0 }), + state_2.remove_proposal(LeafIndex{ 1 }), }; - auto [commit2, welcome2, new_state_2] = states[2].commit( - fresh_secret(), CommitOpts{ { removes }, true, false, {} }, {}); + auto [commit2, welcome2, new_state_2] = + state_2.commit(fresh_secret(), CommitOpts{ removes, true, false, {} }, {}); silence_unused(commit2); - states[2] = new_state_2; + state_2 = new_state_2; // Member @2 should have a valid tree, even though its filtered direct path no // longer goes to the root. - REQUIRE(states[2].tree().parent_hash_valid()); + REQUIRE(state_2.tree().parent_hash_valid()); } From 0b8471b177ad9d176bf03ff74c6411874cbae067 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 20 Aug 2024 18:25:09 +0000 Subject: [PATCH 8/9] update comment for dp list --- src/treekem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/treekem.cpp b/src/treekem.cpp index d164289d..db12582a 100644 --- a/src/treekem.cpp +++ b/src/treekem.cpp @@ -903,7 +903,7 @@ TreeKEMPublicKey::parent_hashes( } // The list of nodes for whom parent hashes are computed, namely: Direct path - // excluding root, including leaf + // excluding the last entry, including leaf auto from_node = NodeIndex(from); auto dp = fdp; auto [last, _res_last] = dp.back(); From 53d955ef878c132cf3743599f909be12e49cda2a Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 20 Aug 2024 18:27:04 +0000 Subject: [PATCH 9/9] add a silence unused for welcome2 --- test/state.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/test/state.cpp b/test/state.cpp index b17cd4ca..fb5ef657 100644 --- a/test/state.cpp +++ b/test/state.cpp @@ -1107,6 +1107,7 @@ TEST_CASE_METHOD(StateTest, "Parent Hash with Empty Left Subtree") auto [commit2, welcome2, new_state_2] = state_2.commit(fresh_secret(), CommitOpts{ removes, true, false, {} }, {}); silence_unused(commit2); + silence_unused(welcome2); state_2 = new_state_2; // Member @2 should have a valid tree, even though its filtered direct path no