Improve accuracy of 'Locked' property
The 'Locked' property in the volume interface is supposed to indicate
whether the LUKS volume is currently activated, but this property is
often inaccurate because it always defaults to false upon startup
(i.e. unlocked). However, the LUKS volume is usually locked at startup.
So, client daemons can get confused when looking at the Locked
property.
This commit reworks the functionality for the 'Locked' property, so that
it checks whether the mapped virtual crypt device exists, e.g. whether
/dev/mapper/<luks_device> exists. This way, the Locked property should
better reflect the actual state.
The one caveat to keep in mind is that 'Locked' will be True even if the
device isn't formatted as a LUKS volume. If client daemons need to know
whether it's already formatted, we may want to add another property to
the Volume interface for that purpose. But in the meantime, eStoraged
already exports an EncryptionStatus property as part of the Drive
interface. So, the information is already available, if needed.
Tested:
Checked 'Locked' property at startup
$ busctl get-property xyz.openbmc_project.eStoraged \
/xyz/openbmc_project/inventory/storage/mmcblk0 \
xyz.openbmc_project.Inventory.Item.Volume Locked
b true
Formatted the LUKS volume, then checked 'Locked' property again
$ busctl call xyz.openbmc_project.eStoraged \
/xyz/openbmc_project/inventory/storage/mmcblk0 \
xyz.openbmc_project.Inventory.Item.Volume FormatLuks ays 3 1 2 3 \
xyz.openbmc_project.Inventory.Item.Volume.FilesystemType.ext4 \
--timeout=60
$ busctl get-property xyz.openbmc_project.eStoraged \
/xyz/openbmc_project/inventory/storage/mmcblk0 \
xyz.openbmc_project.Inventory.Item.Volume Locked
b false
Restarted eStoraged and checked 'Locked' again.
$ systemctl restart xyz.openbmc_project.eStoraged
$ busctl get-property xyz.openbmc_project.eStoraged \
/xyz/openbmc_project/inventory/storage/mmcblk0 \
xyz.openbmc_project.Inventory.Item.Volume Locked
b false
Locked the LUKS volume, and checked 'Locked' again.
$ busctl call xyz.openbmc_project.eStoraged \
/xyz/openbmc_project/inventory/storage/mmcblk0 \
xyz.openbmc_project.Inventory.Item.Volume Lock
$ busctl get-property xyz.openbmc_project.eStoraged \
/xyz/openbmc_project/inventory/storage/mmcblk0 \
xyz.openbmc_project.Inventory.Item.Volume Locked
b true
Restarted eStoraged, and checked 'Locked' again.
$ systemctl restart xyz.openbmc_project.eStoraged
$ busctl get-property xyz.openbmc_project.eStoraged \
/xyz/openbmc_project/inventory/storage/mmcblk0 \
xyz.openbmc_project.Inventory.Item.Volume Locked
b true
Signed-off-by: John Wedig <johnwedig@google.com>
Change-Id: I5cd6bac4b4426c0e2579c3fc8cf7a27b4f2ccc08
diff --git a/include/cryptsetupInterface.hpp b/include/cryptsetupInterface.hpp
index e13320c..fb03a8b 100644
--- a/include/cryptsetupInterface.hpp
+++ b/include/cryptsetupInterface.hpp
@@ -140,7 +140,7 @@
*/
virtual int cryptKeyslotDestroy(struct crypt_device* cd, int keyslot) = 0;
- /** @breif Wapper around crypt_keyslot_max
+ /** @brief Wrapper around crypt_keyslot_max
* @details Used for mocking purposes.
*
* @param type crypt device type
@@ -150,7 +150,7 @@
*/
virtual int cryptKeySlotMax(const char* type) = 0;
- /** @breif Wapper around crypt_keyslot_status
+ /** @brief Wrapper around crypt_keyslot_status
* @details Used for mocking purposes.
* Get information about particular key slot.
*
@@ -158,10 +158,16 @@
* @param keyslot requested keyslot to check or CRYPT_ANY_SLOT
*
* @return value defined by crypt_keyslot_info
- *
*/
virtual crypt_keyslot_info cryptKeySlotStatus(struct crypt_device* cd,
int keyslot) = 0;
+
+ /** @brief Wrapper around crypt_get_dir.
+ * @details Used for mocking purposes.
+ *
+ * @returns the directory where mapped crypt devices are created.
+ */
+ virtual std::string cryptGetDir() = 0;
};
/** @class Cryptsetup
@@ -242,6 +248,11 @@
{
return crypt_keyslot_status(cd, keyslot);
}
+
+ std::string cryptGetDir() override
+ {
+ return {crypt_get_dir()};
+ }
};
/** @class CryptHandle
diff --git a/include/estoraged.hpp b/include/estoraged.hpp
index 42d60f3..8e4ceb7 100644
--- a/include/estoraged.hpp
+++ b/include/estoraged.hpp
@@ -103,6 +103,9 @@
/** @brief Get the mount point for the filesystem on the LUKS device. */
std::string_view getMountPoint() const;
+ /** @brief Get the path to the mapped crypt device. */
+ std::string_view getCryptDevicePath() const;
+
private:
/** @brief Full path of the device file, e.g. /dev/mmcblk0. */
std::string devPath;
@@ -126,6 +129,9 @@
*/
std::unique_ptr<FilesystemInterface> fsIface;
+ /** @brief Path where the mapped crypt device gets created. */
+ const std::string cryptDevicePath;
+
/** @brief D-Bus object server. */
sdbusplus::asio::object_server& objectServer;
@@ -187,12 +193,6 @@
/** @brief Unmount the filesystem. */
void unmountFilesystem();
-
- /** @brief Set the locked property.
- *
- * @param[in] isLocked - indicates whether the LUKS device is locked.
- */
- void locked(bool isLocked);
};
} // namespace estoraged
diff --git a/include/filesystemInterface.hpp b/include/filesystemInterface.hpp
index 12495cd..d5de5e8 100644
--- a/include/filesystemInterface.hpp
+++ b/include/filesystemInterface.hpp
@@ -27,11 +27,11 @@
/** @brief Runs the mkfs command to create the filesystem.
* @details Used for mocking purposes.
*
- * @param[in] logicalVolume - name of the mapped LUKS device.
+ * @param[in] logicalVolumePath - path to the mapped LUKS device.
*
* @returns 0 on success, nonzero on failure.
*/
- virtual int runMkfs(const std::string& logicalVolume) = 0;
+ virtual int runMkfs(const std::string& logicalVolumePath) = 0;
/** @brief Wrapper around mount().
* @details Used for mocking purposes.
@@ -101,9 +101,9 @@
Filesystem(Filesystem&&) = delete;
Filesystem& operator=(Filesystem&&) = delete;
- int runMkfs(const std::string& logicalVolume) override
+ int runMkfs(const std::string& logicalVolumePath) override
{
- std::string mkfsCommand("mkfs.ext4 /dev/mapper/" + logicalVolume);
+ std::string mkfsCommand("mkfs.ext4 " + logicalVolumePath);
// calling 'system' uses a command processor //NOLINTNEXTLINE
return system(mkfsCommand.c_str());
}
diff --git a/src/estoraged.cpp b/src/estoraged.cpp
index 5da6763..341440a 100644
--- a/src/estoraged.cpp
+++ b/src/estoraged.cpp
@@ -42,6 +42,7 @@
devPath(devPath),
containerName(luksName), mountPoint("/mnt/" + luksName + "_fs"),
cryptIface(std::move(cryptInterface)), fsIface(std::move(fsInterface)),
+ cryptDevicePath(cryptIface->cryptGetDir() + "/" + luksName),
objectServer(server)
{
/* Get the filename of the device (without "/dev/"). */
@@ -260,7 +261,23 @@
bool EStoraged::isLocked() const
{
- return lockedProperty;
+ /*
+ * Check if the mapped virtual device exists. If it exists, the LUKS volume
+ * is unlocked.
+ */
+ try
+ {
+ std::filesystem::path mappedDevicePath(cryptDevicePath);
+ return (std::filesystem::exists(mappedDevicePath) == false);
+ }
+ catch (const std::exception& e)
+ {
+ lg2::error("Failed to query locked status: {EXCEPT}", "EXCEPT",
+ e.what(), "REDFISH_MESSAGE_ID",
+ std::string("OpenBMC.0.1.IsLockedFail"));
+ /* If we couldn't query the filesystem path, assume unlocked. */
+ return false;
+ }
}
std::string_view EStoraged::getMountPoint() const
@@ -299,9 +316,6 @@
throw InternalFailure();
}
- /* Device is now encrypted. */
- locked(true);
-
/* Set the password. */
retval = cryptIface->cryptKeyslotAddByVolumeKey(
cryptHandle.get(), CRYPT_ANY_SLOT, nullptr, 0,
@@ -368,9 +382,6 @@
throw InternalFailure();
}
- /* Device is now unlocked. */
- locked(false);
-
lg2::info("Successfully activated LUKS dev {DEV}", "DEV", devPath,
"REDFISH_MESSAGE_ID",
std::string("OpenBMC.0.1.ActivateLuksDevSuccess"));
@@ -379,7 +390,7 @@
void EStoraged::createFilesystem()
{
/* Run the command to create the filesystem. */
- int retval = fsIface->runMkfs(containerName);
+ int retval = fsIface->runMkfs(cryptDevicePath);
if (retval != 0)
{
lg2::error("Failed to create filesystem: {RETVAL}", "RETVAL", retval,
@@ -387,8 +398,8 @@
std::string("OpenBMC.0.1.CreateFilesystemFail"));
throw InternalFailure();
}
- lg2::info("Successfully created filesystem for /dev/mapper/{CONTAINER}",
- "CONTAINER", containerName, "REDFISH_MESSAGE_ID",
+ lg2::info("Successfully created filesystem for {CONTAINER}", "CONTAINER",
+ cryptDevicePath, "REDFISH_MESSAGE_ID",
std::string("OpenBMC.0.1.CreateFilesystemSuccess"));
}
@@ -413,8 +424,7 @@
}
/* Run the command to mount the filesystem. */
- std::string luksContainer("/dev/mapper/" + containerName);
- int retval = fsIface->doMount(luksContainer.c_str(), mountPoint.c_str(),
+ int retval = fsIface->doMount(cryptDevicePath.c_str(), mountPoint.c_str(),
"ext4", 0, nullptr);
if (retval != 0)
{
@@ -478,17 +488,14 @@
throw InternalFailure();
}
- /* Device is now locked. */
- locked(true);
-
lg2::info("Successfully deactivated LUKS device {DEV}", "DEV", devPath,
"REDFISH_MESSAGE_ID",
std::string("OpenBMC.0.1.DeactivateLuksDevSuccess"));
}
-void EStoraged::locked(bool isLocked)
+std::string_view EStoraged::getCryptDevicePath() const
{
- lockedProperty = isLocked;
+ return cryptDevicePath;
}
} // namespace estoraged
diff --git a/src/test/estoraged_test.cpp b/src/test/estoraged_test.cpp
index 9fde533..a0c56fe 100644
--- a/src/test/estoraged_test.cpp
+++ b/src/test/estoraged_test.cpp
@@ -29,7 +29,6 @@
using sdbusplus::xyz::openbmc_project::Inventory::Item::server::Volume;
using std::filesystem::path;
using ::testing::_;
-using ::testing::ContainsRegex;
using ::testing::Return;
using ::testing::StrEq;
@@ -38,6 +37,7 @@
public:
const char* testFileName = "testfile";
const char* testLuksDevName = "testfile_luksDev";
+ const char* testCryptDir = "/tmp";
const std::string testConfigPath =
"/xyz/openbmc_project/inventory/system/board/test_board/test_emmc";
const uint64_t testSize = 24;
@@ -77,6 +77,9 @@
std::make_unique<MockFilesystemInterface>();
mockFsIface = fsIface.get();
+ /* Set up location of dummy mapped crypt file. */
+ EXPECT_CALL(*cryptIface, cryptGetDir).WillOnce(Return(testCryptDir));
+
conn = std::make_shared<sdbusplus::asio::connection>(io);
// request D-Bus server name.
conn->request_name("xyz.openbmc_project.eStoraged.test");
@@ -94,6 +97,29 @@
}
};
+const char* mappedDevicePath = "/tmp/testfile_luksDev";
+std::ofstream mappedDevice;
+
+int createMappedDev()
+{
+ mappedDevice.open(mappedDevicePath,
+ std::ios::out | std::ios::binary | std::ios::trunc);
+ mappedDevice.close();
+ if (mappedDevice.fail())
+ {
+ throw std::runtime_error("Failed to open test mapped device");
+ }
+
+ return 0;
+}
+
+int removeMappedDev()
+{
+ EXPECT_EQ(0, unlink(mappedDevicePath));
+
+ return 0;
+}
+
/* Test case to format and then lock the LUKS device. */
TEST_F(EStoragedTest, FormatPass)
{
@@ -105,9 +131,10 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(0));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(0));
EXPECT_CALL(*mockFsIface, directoryExists(path(esObject->getMountPoint())))
.WillOnce(Return(false));
@@ -116,7 +143,7 @@
.WillOnce(Return(true));
EXPECT_CALL(*mockFsIface,
- doMount(ContainsRegex("/dev/mapper/"),
+ doMount(StrEq(esObject->getCryptDevicePath()),
StrEq(esObject->getMountPoint()), _, _, _))
.WillOnce(Return(0));
@@ -126,7 +153,8 @@
EXPECT_CALL(*mockFsIface, removeDirectory(path(esObject->getMountPoint())))
.WillOnce(Return(true));
- EXPECT_CALL(*mockCryptIface, cryptDeactivate(_, _)).Times(1);
+ EXPECT_CALL(*mockCryptIface, cryptDeactivate(_, _))
+ .WillOnce(&removeMappedDev);
/* Format the encrypted device. */
esObject->formatLuks(password, Volume::FilesystemType::ext4);
@@ -150,9 +178,10 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(0));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(0));
EXPECT_CALL(*mockFsIface, directoryExists(path(esObject->getMountPoint())))
.WillOnce(Return(true));
@@ -161,7 +190,7 @@
.Times(0);
EXPECT_CALL(*mockFsIface,
- doMount(ContainsRegex("/dev/mapper/"),
+ doMount(StrEq(esObject->getCryptDevicePath()),
StrEq(esObject->getMountPoint()), _, _, _))
.WillOnce(Return(0));
@@ -171,7 +200,8 @@
EXPECT_CALL(*mockFsIface, removeDirectory(path(esObject->getMountPoint())))
.WillOnce(Return(true));
- EXPECT_CALL(*mockCryptIface, cryptDeactivate(_, _)).Times(1);
+ EXPECT_CALL(*mockCryptIface, cryptDeactivate(_, _))
+ .WillOnce(&removeMappedDev);
/* Format the encrypted device. */
esObject->formatLuks(password, Volume::FilesystemType::ext4);
@@ -189,7 +219,7 @@
EXPECT_THROW(esObject->formatLuks(password, Volume::FilesystemType::ext4),
ResourceNotFound);
- EXPECT_FALSE(esObject->isLocked());
+ EXPECT_TRUE(esObject->isLocked());
/* Create the test file again, so that the TearDown function works. */
testFile.open(testFileName,
@@ -205,7 +235,7 @@
EXPECT_THROW(esObject->formatLuks(password, Volume::FilesystemType::ext4),
InternalFailure);
- EXPECT_FALSE(esObject->isLocked());
+ EXPECT_TRUE(esObject->isLocked());
}
/* Test case where we fail to set the password for the LUKS device. */
@@ -265,13 +295,16 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(-1));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(-1));
EXPECT_THROW(esObject->formatLuks(password, Volume::FilesystemType::ext4),
InternalFailure);
EXPECT_FALSE(esObject->isLocked());
+
+ EXPECT_EQ(0, removeMappedDev());
}
/* Test case where we fail to create the mount point. */
@@ -285,9 +318,10 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(0));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(0));
EXPECT_CALL(*mockFsIface, directoryExists(path(esObject->getMountPoint())))
.WillOnce(Return(false));
@@ -298,6 +332,8 @@
EXPECT_THROW(esObject->formatLuks(password, Volume::FilesystemType::ext4),
InternalFailure);
EXPECT_FALSE(esObject->isLocked());
+
+ EXPECT_EQ(0, removeMappedDev());
}
/* Test case where we fail to mount the filesystem. */
@@ -311,9 +347,10 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(0));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(0));
EXPECT_CALL(*mockFsIface, directoryExists(path(esObject->getMountPoint())))
.WillOnce(Return(false));
@@ -322,7 +359,7 @@
.WillOnce(Return(true));
EXPECT_CALL(*mockFsIface,
- doMount(ContainsRegex("/dev/mapper/"),
+ doMount(StrEq(esObject->getCryptDevicePath()),
StrEq(esObject->getMountPoint()), _, _, _))
.WillOnce(Return(-1));
@@ -332,6 +369,8 @@
EXPECT_THROW(esObject->formatLuks(password, Volume::FilesystemType::ext4),
InternalFailure);
EXPECT_FALSE(esObject->isLocked());
+
+ EXPECT_EQ(0, removeMappedDev());
}
/* Test case where we fail to unmount the filesystem. */
@@ -345,9 +384,10 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(0));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(0));
EXPECT_CALL(*mockFsIface, directoryExists(path(esObject->getMountPoint())))
.WillOnce(Return(false));
@@ -356,7 +396,7 @@
.WillOnce(Return(true));
EXPECT_CALL(*mockFsIface,
- doMount(ContainsRegex("/dev/mapper/"),
+ doMount(StrEq(esObject->getCryptDevicePath()),
StrEq(esObject->getMountPoint()), _, _, _))
.WillOnce(Return(0));
@@ -368,6 +408,8 @@
EXPECT_THROW(esObject->lock(), InternalFailure);
EXPECT_FALSE(esObject->isLocked());
+
+ EXPECT_EQ(0, removeMappedDev());
}
/* Test case where we fail to remove the mount point. */
@@ -381,9 +423,10 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(0));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(0));
EXPECT_CALL(*mockFsIface, directoryExists(path(esObject->getMountPoint())))
.WillOnce(Return(false));
@@ -392,7 +435,7 @@
.WillOnce(Return(true));
EXPECT_CALL(*mockFsIface,
- doMount(ContainsRegex("/dev/mapper/"),
+ doMount(StrEq(esObject->getCryptDevicePath()),
StrEq(esObject->getMountPoint()), _, _, _))
.WillOnce(Return(0));
@@ -408,6 +451,8 @@
/* This will fail to remove the mount point. */
EXPECT_THROW(esObject->lock(), InternalFailure);
EXPECT_FALSE(esObject->isLocked());
+
+ EXPECT_EQ(0, removeMappedDev());
}
/* Test case where we fail to deactivate the LUKS device. */
@@ -421,9 +466,10 @@
EXPECT_CALL(*mockCryptIface, cryptLoad(_, _, _)).Times(1);
EXPECT_CALL(*mockCryptIface, cryptActivateByPassphrase(_, _, _, _, _, _))
- .Times(1);
+ .WillOnce(&createMappedDev);
- EXPECT_CALL(*mockFsIface, runMkfs(testLuksDevName)).WillOnce(Return(0));
+ EXPECT_CALL(*mockFsIface, runMkfs(StrEq(esObject->getCryptDevicePath())))
+ .WillOnce(Return(0));
EXPECT_CALL(*mockFsIface, directoryExists(path(esObject->getMountPoint())))
.WillOnce(Return(false));
@@ -432,7 +478,7 @@
.WillOnce(Return(true));
EXPECT_CALL(*mockFsIface,
- doMount(ContainsRegex("/dev/mapper/"),
+ doMount(StrEq(esObject->getCryptDevicePath()),
StrEq(esObject->getMountPoint()), _, _, _))
.WillOnce(Return(0));
@@ -450,6 +496,8 @@
EXPECT_THROW(esObject->lock(), InternalFailure);
EXPECT_FALSE(esObject->isLocked());
+
+ EXPECT_EQ(0, removeMappedDev());
}
/* Test case where we successfully change the password. */
diff --git a/src/test/include/estoraged_test.hpp b/src/test/include/estoraged_test.hpp
index 6809c37..a9dfbc0 100644
--- a/src/test/include/estoraged_test.hpp
+++ b/src/test/include/estoraged_test.hpp
@@ -19,7 +19,8 @@
class MockFilesystemInterface : public estoraged::FilesystemInterface
{
public:
- MOCK_METHOD(int, runMkfs, (const std::string& logicalVolume), (override));
+ MOCK_METHOD(int, runMkfs, (const std::string& logicalVolumePath),
+ (override));
MOCK_METHOD(int, doMount,
(const char* source, const char* target,
@@ -81,6 +82,8 @@
MOCK_METHOD(crypt_keyslot_info, cryptKeySlotStatus,
(struct crypt_device * cd, int keyslot), (override));
+
+ MOCK_METHOD(std::string, cryptGetDir, (), (override));
};
} // namespace estoraged_test