From 299e0c4a91bbf22074e315d7607c7efa738430fc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Fri, 1 Sep 2023 10:39:49 -0400
Subject: [PATCH] conversation: use certificates from repository during
validation
If a new device is doing some actions for the first time, the merge
will detect a new certificate that is not yet in the certificate
store (unless if the sender is the new device).
This will block the validation. So, instead, use the certificate
in the processed commit.
GitLab: #892
Change-Id: I0ab492e700a2aa7ef4258e2bf711117f67e29fae
---
src/jamidht/conversationrepository.cpp | 119 ++++++++--------
test/unitTest/conversation/conversation.cpp | 147 --------------------
2 files changed, 63 insertions(+), 203 deletions(-)
diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp
index d6380bab29..40c7dd080e 100644
--- a/src/jamidht/conversationrepository.cpp
+++ b/src/jamidht/conversationrepository.cpp
@@ -302,6 +302,22 @@ public:
mutable std::mutex deviceToUriMtx_;
mutable std::map<std::string, std::string> deviceToUri_;
+ std::string uriFromDevice(const std::string& deviceId, const std::string& commitId) const
+ {
+ auto repo = repository();
+ if (!repo)
+ return {};
+ auto tree = treeAtCommit(repo.get(), commitId);
+ auto deviceFile = fmt::format("devices/{}.crt", deviceId);
+ auto blob_device = fileAtTree(deviceFile, tree);
+ if (!blob_device) {
+ JAMI_ERROR("{} announced but not found", deviceId);
+ return {};
+ }
+ auto deviceCert = dht::crypto::Certificate(as_view(blob_device));
+ return deviceCert.getIssuerUID();
+ }
+
/**
* Verify that a certificate modification is correct
* @param certPath Where the certificate is saved (relative path)
@@ -885,15 +901,6 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
auto repo = repository();
if (!repo)
return false;
- auto treeNew = treeAtCommit(repo.get(), commitId);
- if (not treeNew)
- return false;
- std::string deviceFile = fmt::format("devices/{}.crt", userDevice);
- if (!fileAtTree(deviceFile, treeNew)) {
- JAMI_ERROR("{} announced but not found", deviceFile.c_str());
- return false;
- }
-
// Here, we check that a file device is modified or not.
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId));
if (changedFiles.size() == 0)
@@ -901,10 +908,12 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
// If a certificate is modified (in the changedFiles), it MUST be a certificate from the user
// Retrieve userUri
- auto userUri = uriFromDevice(userDevice);
+ auto treeNew = treeAtCommit(repo.get(), commitId);
+ auto userUri = uriFromDevice(userDevice, commitId);
if (userUri.empty())
return false;
+ std::string userDeviceFile = fmt::format("devices/{}.crt", userDevice);
std::string adminsFile = fmt::format("admins/{}.crt", userUri);
std::string membersFile = fmt::format("members/{}.crt", userUri);
auto treeOld = treeAtCommit(repo.get(), parentId);
@@ -924,7 +933,7 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
JAMI_ERROR("Invalid certificate {}", changedFile);
return false;
}
- } else if (changedFile == deviceFile) {
+ } else if (changedFile == userDeviceFile) {
// In this case, device is added or modified (certificate expiration)
auto oldFile = fileAtTree(changedFile, treeOld);
std::string_view oldCert;
@@ -949,7 +958,12 @@ bool
ConversationRepository::Impl::checkEdit(const std::string& userDevice,
const ConversationCommit& commit) const
{
- auto userUri = uriFromDevice(userDevice);
+ auto repo = repository();
+ if (!repo)
+ return false;
+ auto userUri = uriFromDevice(userDevice, commit.id);
+ if (userUri.empty())
+ return false;
// Check that edited commit is found, for the same author, and editable (plain/text)
auto commitMap = convCommitToMap(commit);
if (commitMap == std::nullopt) {
@@ -1017,14 +1031,9 @@ ConversationRepository::Impl::checkVote(const std::string& userDevice,
if (not treeNew or not treeOld)
return false;
- if (not deviceFile.empty()) {
- if (fileAtTree(deviceFile, treeOld)) {
- JAMI_ERROR("Invalid device file modified: {}", deviceFile);
- return false;
- }
- }
-
- auto userUri = uriFromDevice(userDevice);
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
// Check that voter is admin
auto adminFile = fmt::format("admins/{}.crt", userUri);
@@ -1108,9 +1117,8 @@ ConversationRepository::Impl::checkValidAdd(const std::string& userDevice,
const std::string& commitId,
const std::string& parentId) const
{
- auto userUri = uriFromDevice(userDevice);
auto repo = repository();
- if (userUri.empty() or not repo)
+ if (not repo)
return false;
std::string repoPath = git_repository_workdir(repo.get());
@@ -1123,6 +1131,10 @@ ConversationRepository::Impl::checkValidAdd(const std::string& userDevice,
}
}
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
// Check that only /invited/uri.crt is added & deviceFile & CRLs
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId));
if (changedFiles.size() == 0) {
@@ -1161,8 +1173,6 @@ ConversationRepository::Impl::checkValidAdd(const std::string& userDevice,
if (not treeOld)
return false;
auto treeNew = treeAtCommit(repo.get(), commitId);
- if (not treeNew)
- return false;
auto blob_invite = fileAtTree(invitedFile, treeNew);
if (!blob_invite) {
JAMI_ERROR("Invitation not found for commit {}", commitId);
@@ -1265,8 +1275,6 @@ ConversationRepository::Impl::checkValidRemove(const std::string& userDevice,
const std::string& commitId,
const std::string& parentId) const
{
- auto userUri = uriFromDevice(userDevice);
-
// Retrieve tree for recent commit
auto repo = repository();
if (!repo)
@@ -1304,11 +1312,15 @@ ConversationRepository::Impl::checkValidRemove(const std::string& userDevice,
// Check that removed devices are for removed member (or directly uriMember)
for (const auto& deviceUri : devicesRemoved) {
deviceFile = fmt::format("devices/{}.crt", deviceUri);
- if (!fileAtTree(deviceFile, treeOld)) {
+ auto blob_device = fileAtTree(deviceFile, treeOld);
+ if (!blob_device) {
JAMI_ERROR("device not found added ({})", deviceFile);
return false;
}
- if (uriMember != uriFromDevice(deviceUri)
+ auto deviceCert = dht::crypto::Certificate(as_view(blob_device));
+ auto userUri = deviceCert.getIssuerUID();
+
+ if (uriMember != userUri
and uriMember != deviceUri /* If device is removed */) {
JAMI_ERROR("device removed but not for removed user ({})", deviceFile);
return false;
@@ -1325,8 +1337,6 @@ ConversationRepository::Impl::checkValidVoteResolution(const std::string& userDe
const std::string& parentId,
const std::string& voteType) const
{
- auto userUri = uriFromDevice(userDevice);
-
// Retrieve tree for recent commit
auto repo = repository();
if (!repo)
@@ -1408,6 +1418,10 @@ ConversationRepository::Impl::checkValidVoteResolution(const std::string& userDe
}
}
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
// Check that voters are admins
adminFile = fmt::format("admins/{}.crt", userUri);
if (!fileAtTree(adminFile, treeOld)) {
@@ -1445,7 +1459,19 @@ ConversationRepository::Impl::checkValidProfileUpdate(const std::string& userDev
const std::string& commitId,
const std::string& parentId) const
{
- auto userUri = uriFromDevice(userDevice);
+ // Retrieve tree for recent commit
+ auto repo = repository();
+ if (!repo)
+ return false;
+ auto treeNew = treeAtCommit(repo.get(), commitId);
+ auto treeOld = treeAtCommit(repo.get(), parentId);
+ if (not treeNew or not treeOld)
+ return false;
+
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
auto valid = false;
{
std::lock_guard<std::mutex> lk(membersMtx_);
@@ -1461,15 +1487,6 @@ ConversationRepository::Impl::checkValidProfileUpdate(const std::string& userDev
return false;
}
- // Retrieve tree for recent commit
- auto repo = repository();
- if (!repo)
- return false;
- auto treeNew = treeAtCommit(repo.get(), commitId);
- auto treeOld = treeAtCommit(repo.get(), parentId);
- if (not treeNew or not treeOld)
- return false;
-
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, parentId));
// Check that no weird file is added nor removed
for (const auto& f : changedFiles) {
@@ -1577,7 +1594,12 @@ ConversationRepository::Impl::checkInitialCommit(const std::string& userDevice,
JAMI_WARNING("Invalid repository detected");
return false;
}
- auto userUri = uriFromDevice(userDevice);
+
+ auto treeNew = treeAtCommit(repo.get(), commitId);
+ auto userUri = uriFromDevice(userDevice, commitId);
+ if (userUri.empty())
+ return false;
+
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, ""));
// NOTE: libgit2 return a diff with /, not DIR_SEPARATOR_DIR
@@ -1607,21 +1629,6 @@ ConversationRepository::Impl::checkInitialCommit(const std::string& userDevice,
std::string crlFile = fmt::format("CRLs/{}", userUri);
std::string invitedFile = fmt::format("invited/{}", invited);
- git_oid oid;
- git_commit* commit = nullptr;
- if (git_oid_fromstr(&oid, commitId.c_str()) < 0
- || git_commit_lookup(&commit, repo.get(), &oid) < 0) {
- JAMI_WARNING("Failed to look up commit {}", commitId);
- return false;
- }
- GitCommit gc = {commit, git_commit_free};
- git_tree* tNew = nullptr;
- if (git_commit_tree(&tNew, gc.get()) < 0) {
- JAMI_ERROR("Could not look up initial tree");
- return false;
- }
- GitTree treeNew = {tNew, git_tree_free};
-
// Check that admin cert is added
// Check that device cert is added
// Check CRLs added
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index 410a276bba..280b1218e5 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -87,7 +87,6 @@ private:
void testReplaceWithBadCertificate();
void testSendMessageTriggerMessageReceived();
void testMergeTwoDifferentHeads();
- void testMergeAfterMigration();
void testSendMessageToMultipleParticipants();
void testPingPongMessages();
void testIsComposing();
@@ -111,7 +110,6 @@ private:
void testMemberCannotUpdateProfile();
void testUpdateProfileWithBadFile();
void testFetchProfileUnauthorized();
- void testDoNotLoadIncorrectConversation();
void testSyncingWhileAccepting();
void testCountInteractions();
void testReplayConversation();
@@ -141,7 +139,6 @@ private:
CPPUNIT_TEST(testReplaceWithBadCertificate);
CPPUNIT_TEST(testSendMessageTriggerMessageReceived);
CPPUNIT_TEST(testMergeTwoDifferentHeads);
- CPPUNIT_TEST(testMergeAfterMigration);
CPPUNIT_TEST(testSendMessageToMultipleParticipants);
CPPUNIT_TEST(testPingPongMessages);
CPPUNIT_TEST(testIsComposing);
@@ -165,7 +162,6 @@ private:
CPPUNIT_TEST(testMemberCannotUpdateProfile);
CPPUNIT_TEST(testUpdateProfileWithBadFile);
CPPUNIT_TEST(testFetchProfileUnauthorized);
- CPPUNIT_TEST(testDoNotLoadIncorrectConversation);
CPPUNIT_TEST(testSyncingWhileAccepting);
CPPUNIT_TEST(testCountInteractions);
CPPUNIT_TEST(testReplayConversation);
@@ -725,122 +721,6 @@ ConversationTest::testMergeTwoDifferentHeads()
libjami::unregisterSignalHandlers();
}
-void
-ConversationTest::testMergeAfterMigration()
-{
- std::cout << "\nRunning test: " << __func__ << std::endl;
-
- auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
- auto carlaAccount = Manager::instance().getAccount<JamiAccount>(carlaId);
- auto aliceUri = aliceAccount->getUsername();
- auto carlaUri = carlaAccount->getUsername();
- aliceAccount->trackBuddyPresence(carlaUri, true);
- carlaAccount->trackBuddyPresence(aliceUri, true);
-
- std::map<std::string, std::shared_ptr<libjami::CallbackWrapperBase>> confHandlers;
- bool conversationReady = false, conversationAliceReady = false;
- confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::ConversationReady>(
- [&](const std::string& accountId, const std::string& /* conversationId */) {
- if (accountId == aliceId)
- conversationAliceReady = true;
- else if (accountId == carlaId)
- conversationReady = true;
- cv.notify_one();
- }));
- std::string carlaGotMessage, aliceGotMessage;
- confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::MessageReceived>(
- [&](const std::string& accountId,
- const std::string& conversationId,
- std::map<std::string, std::string> message) {
- if (accountId == carlaId) {
- carlaGotMessage = message["id"];
- } else if (accountId == aliceId) {
- aliceGotMessage = message["id"];
- }
- cv.notify_one();
- }));
- bool carlaConnected = false;
- confHandlers.insert(
- libjami::exportable_callback<libjami::ConfigurationSignal::VolatileDetailsChanged>(
- [&](const std::string&, const std::map<std::string, std::string>&) {
- auto details = carlaAccount->getVolatileAccountDetails();
- auto deviceAnnounced
- = details[libjami::Account::VolatileProperties::DEVICE_ANNOUNCED];
- if (deviceAnnounced == "true") {
- carlaConnected = true;
- cv.notify_one();
- }
- }));
- libjami::registerSignalHandlers(confHandlers);
-
- auto convId = libjami::startConversation(aliceId);
- CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&] { return conversationAliceReady; }));
-
- aliceGotMessage = "";
- aliceAccount->convModule()->addConversationMember(convId, carlaUri, false);
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return !aliceGotMessage.empty(); }));
-
- // Cp conversations & convInfo
- auto repoPathAlice = fileutils::get_data_dir() + DIR_SEPARATOR_STR
- + aliceAccount->getAccountID() + DIR_SEPARATOR_STR + "conversations";
- auto repoPathCarla = fileutils::get_data_dir() + DIR_SEPARATOR_STR
- + carlaAccount->getAccountID() + DIR_SEPARATOR_STR + "conversations";
- auto p = std::filesystem::path(repoPathCarla);
- dhtnet::fileutils::recursive_mkdir(p.parent_path());
- std::filesystem::copy(repoPathAlice, repoPathCarla, std::filesystem::copy_options::recursive);
- auto ciPathAlice = fileutils::get_data_dir() + DIR_SEPARATOR_STR + aliceAccount->getAccountID()
- + DIR_SEPARATOR_STR + "convInfo";
- auto ciPathCarla = fileutils::get_data_dir() + DIR_SEPARATOR_STR + carlaAccount->getAccountID()
- + DIR_SEPARATOR_STR + "convInfo";
- std::filesystem::remove_all(ciPathCarla);
- std::filesystem::copy(ciPathAlice, ciPathCarla);
-
- // Accept for carla
- ConversationRepository repo(carlaAccount, convId);
- repo.join();
-
- std::filesystem::remove_all(repoPathAlice);
- p = std::filesystem::path(repoPathAlice);
- dhtnet::fileutils::recursive_mkdir(p.parent_path());
- std::filesystem::copy(repoPathCarla, repoPathAlice, std::filesystem::copy_options::recursive);
-
- CPPUNIT_ASSERT(std::filesystem::is_directory(repoPathAlice));
- CPPUNIT_ASSERT(std::filesystem::is_directory(repoPathAlice + "/" + convId));
-
- // Makes different heads
- carlaAccount->convModule()->loadConversations(); // necessary to load conversation
- carlaGotMessage = "";
- libjami::sendMessage(carlaId, convId, "hi"s, "");
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return !carlaGotMessage.empty(); }));
- aliceGotMessage = "";
- libjami::sendMessage(aliceId, convId, "hi"s, "");
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return !aliceGotMessage.empty(); }));
-
- // Connect only carla to migrate it.
- Manager::instance().sendRegister(aliceId, false);
- Manager::instance().sendRegister(carlaId, true);
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return carlaConnected; }));
-
- // Migrate carla
- CPPUNIT_ASSERT(carlaAccount->setValidity("", {}, 10));
- auto now = std::chrono::system_clock::now();
- std::this_thread::sleep_until(now + 20s);
- carlaConnected = false;
- carlaAccount->forceReloadAccount();
- CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&] { return carlaConnected; }));
- // Force move certificate in alice's certStore (because forceReloadAccount is different than a real migration)
- // and DHT state is still the old one
- aliceAccount->certStore().pinCertificate(carlaAccount->identity().second);
-
- // Start Carla, should merge and all messages should be there
- Manager::instance().sendRegister(aliceId, true);
- carlaGotMessage = "";
- aliceGotMessage = "";
- CPPUNIT_ASSERT(
- cv.wait_for(lk, 60s, [&] { return !carlaGotMessage.empty() && !aliceGotMessage.empty(); }));
- libjami::unregisterSignalHandlers();
-}
-
void
ConversationTest::testSendMessageToMultipleParticipants()
{
@@ -2811,33 +2691,6 @@ END:VCARD";
libjami::unregisterSignalHandlers();
}
-void
-ConversationTest::testDoNotLoadIncorrectConversation()
-{
- std::cout << "\nRunning test: " << __func__ << std::endl;
-
- auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
- auto uri = aliceAccount->getUsername();
- auto convId = libjami::startConversation(aliceId);
-
- auto convInfos = libjami::conversationInfos(aliceId, convId);
- CPPUNIT_ASSERT(convInfos.find("mode") != convInfos.end());
- CPPUNIT_ASSERT(convInfos.find("syncing") == convInfos.end());
-
- Manager::instance().sendRegister(aliceId, false);
- auto repoGitPath = fileutils::get_data_dir() + DIR_SEPARATOR_STR + aliceAccount->getAccountID()
- + DIR_SEPARATOR_STR + "conversations" + DIR_SEPARATOR_STR + convId
- + DIR_SEPARATOR_STR + ".git";
- dhtnet::fileutils::removeAll(repoGitPath, true); // This make the repository not usable
-
- aliceAccount->convModule()
- ->loadConversations(); // Refresh. This should detect the incorrect conversations.
-
- // the conv should be detected as non existing
- convInfos = libjami::conversationInfos(aliceId, convId);
- CPPUNIT_ASSERT(convInfos.empty());
-}
-
void
ConversationTest::testSyncingWhileAccepting()
{
--
GitLab