Refactor code related to non-privileged mounts.
This CL modifies the code to obtain the user and group ID of the user
for non-privileged mounts when the daemon starts and keep that
information in the Platform object such that other objects can simply
obtain the information when needed.
BUG=none
TEST=Ran unit tests and tested mounting/unmounting on Cr48.
Change-Id: Ib299191ff45da260d1a6de617e052131aa9e906a
Reviewed-on: http://gerrit.chromium.org/gerrit/5229
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
diff --git a/archive-manager.cc b/archive-manager.cc
index 59afe78..77a30e2 100644
--- a/archive-manager.cc
+++ b/archive-manager.cc
@@ -30,7 +30,6 @@
// in the path: '/', 'media', '<sub type>', '<mount dir>'
static size_t kNumComponentsInMountDirectoryPath = 4;
static const char kAVFSMountProgram[] = "/usr/bin/avfsd";
-static const char kAVFSMountUser[] = "chronos";
static const char kAVFSRootDirectory[] = "/var/run/avfsroot";
static const char kAVFSMediaDirectory[] = "/var/run/avfsroot/media";
static const char kAVFSUserFileDirectory[] = "/var/run/avfsroot/user";
@@ -52,10 +51,8 @@
}
bool ArchiveManager::StartSession(const string& user) {
- uid_t user_id;
- gid_t group_id;
- if (!platform_->GetUserAndGroupId(kAVFSMountUser, &user_id, &group_id))
- return false;
+ uid_t user_id = platform_->mount_user_id();
+ gid_t group_id = platform_->mount_group_id();
if (!platform_->CreateDirectory(kAVFSRootDirectory) ||
!platform_->SetOwnership(kAVFSRootDirectory, user_id, group_id) ||
@@ -191,12 +188,13 @@
}
chromeos::ProcessImpl mount_process;
- mount_process.AddArg("/bin/su");
- mount_process.AddArg(kAVFSMountUser);
- mount_process.AddArg("-c");
- mount_process.AddArg(string(kAVFSMountProgram) +
- " -o ro,nodev,noexec,nosuid,modules=subdir,subdir=" +
- base_path + " " + avfs_path);
+ mount_process.AddArg(kAVFSMountProgram);
+ mount_process.AddArg("-o");
+ mount_process.AddArg("ro,nodev,noexec,nosuid,modules=subdir,subdir=" +
+ base_path);
+ mount_process.AddArg(avfs_path);
+ mount_process.SetUid(platform_->mount_user_id());
+ mount_process.SetGid(platform_->mount_group_id());
if (mount_process.Run() != 0 ||
!mount_info.RetrieveFromCurrentProcess() ||
!mount_info.HasMountPath(avfs_path)) {
diff --git a/disk-manager.cc b/disk-manager.cc
index 9635d5c..6a1f36c 100644
--- a/disk-manager.cc
+++ b/disk-manager.cc
@@ -27,9 +27,6 @@
namespace cros_disks {
-// TODO(benchan): Infer the current user/group from session manager
-// instead of hardcoding as chronos
-static const char kMountDefaultUser[] = "chronos";
static const char kBlockSubsystem[] = "block";
static const char kScsiSubsystem[] = "scsi";
static const char kScsiDevice[] = "scsi_device";
@@ -337,12 +334,8 @@
string default_user_id, default_group_id;
bool set_user_and_group_id = filesystem.accepts_user_and_group_id();
if (set_user_and_group_id) {
- uid_t uid;
- gid_t gid;
- if (platform_->GetUserAndGroupId(kMountDefaultUser, &uid, &gid)) {
- default_user_id = base::StringPrintf("%d", uid);
- default_group_id = base::StringPrintf("%d", gid);
- }
+ default_user_id = base::StringPrintf("%d", platform_->mount_user_id());
+ default_group_id = base::StringPrintf("%d", platform_->mount_group_id());
}
MountOptions mount_options;
diff --git a/disk-manager.h b/disk-manager.h
index fe7a08f..a846d53 100644
--- a/disk-manager.h
+++ b/disk-manager.h
@@ -7,7 +7,6 @@
#include <blkid/blkid.h>
#include <libudev.h>
-#include <sys/types.h>
#include <map>
#include <set>
diff --git a/disk-manager_unittest.cc b/disk-manager_unittest.cc
index 1fa0d4f..e140f97 100644
--- a/disk-manager_unittest.cc
+++ b/disk-manager_unittest.cc
@@ -5,7 +5,6 @@
#include "cros-disks/disk-manager.h"
#include <sys/mount.h>
-#include <sys/types.h>
#include <base/memory/scoped_temp_dir.h>
#include <gtest/gtest.h>
diff --git a/main.cc b/main.cc
index 60f4315..b8335c6 100644
--- a/main.cc
+++ b/main.cc
@@ -44,6 +44,7 @@
static const char kUsageMessage[] = "Chromium OS Disk Daemon";
static const char kArchiveMountRootDirectory[] = "/media/archive";
static const char kDiskMountRootDirectory[] = "/media/removable";
+static const char kNonPrivilegedMountUser[] = "chronos";
// Always logs to the syslog and logs to stderr if
// we are running in the foreground.
@@ -97,10 +98,12 @@
server_conn.request_name("org.chromium.CrosDisks");
Platform platform;
+ CHECK(platform.SetMountUser(kNonPrivilegedMountUser))
+ << "'" << kNonPrivilegedMountUser
+ << "' is not available for non-privileged mount operations.";
LOG(INFO) << "Creating the archive manager";
- ArchiveManager archive_manager(kArchiveMountRootDirectory,
- &platform);
+ ArchiveManager archive_manager(kArchiveMountRootDirectory, &platform);
CHECK(archive_manager.Initialize())
<< "Failed to initialize the archive manager";
diff --git a/mount-manager.cc b/mount-manager.cc
index 79dd5c9..978d7da 100644
--- a/mount-manager.cc
+++ b/mount-manager.cc
@@ -102,9 +102,8 @@
return kMountErrorDirectoryCreationFailed;
}
- gid_t group_id;
- if (!platform_->GetGroupId(kMountDirectoryGroup, &group_id) ||
- !platform_->SetOwnership(actual_mount_path, getuid(), group_id) ||
+ if (!platform_->SetOwnership(actual_mount_path, getuid(),
+ platform_->mount_group_id()) ||
!platform_->SetPermissions(actual_mount_path,
kMountDirectoryPermissions)) {
LOG(ERROR) << "Failed to set ownership and permissions of directory '"
diff --git a/mount-manager_unittest.cc b/mount-manager_unittest.cc
index 6381fcc..b8cd502 100644
--- a/mount-manager_unittest.cc
+++ b/mount-manager_unittest.cc
@@ -35,8 +35,6 @@
MOCK_CONST_METHOD1(CreateOrReuseEmptyDirectory, bool(const string& path));
MOCK_CONST_METHOD2(CreateOrReuseEmptyDirectoryWithFallback,
bool(string* path, unsigned max_suffix_to_retry));
- MOCK_CONST_METHOD2(GetGroupId,
- bool(const string& group_name, gid_t* group_id));
MOCK_CONST_METHOD1(RemoveEmptyDirectory, bool(const string& path));
MOCK_CONST_METHOD3(SetOwnership, bool(const string& path,
uid_t user_id, gid_t group_id));
@@ -201,7 +199,6 @@
.WillOnce(Return(true));
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectoryWithFallback(_, _))
.Times(0);
- EXPECT_CALL(platform_, GetGroupId(_, _)).WillOnce(Return(true));
EXPECT_CALL(platform_, SetOwnership(mount_path_, _, _))
.WillOnce(Return(false));
EXPECT_CALL(platform_, SetPermissions(_, _)).Times(0);
@@ -226,7 +223,6 @@
.WillOnce(Return(true));
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectoryWithFallback(_, _))
.Times(0);
- EXPECT_CALL(platform_, GetGroupId(_, _)).WillOnce(Return(true));
EXPECT_CALL(platform_, SetOwnership(mount_path_, _, _))
.WillOnce(Return(true));
EXPECT_CALL(platform_, SetPermissions(mount_path_, _))
@@ -252,7 +248,6 @@
.WillOnce(Return(true));
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectoryWithFallback(_, _))
.Times(0);
- EXPECT_CALL(platform_, GetGroupId(_, _)).WillOnce(Return(true));
EXPECT_CALL(platform_, SetOwnership(mount_path_, _, _))
.WillOnce(Return(true));
EXPECT_CALL(platform_, SetPermissions(mount_path_, _))
@@ -281,7 +276,6 @@
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectory(_)).Times(0);
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectoryWithFallback(_, _))
.WillOnce(Return(true));
- EXPECT_CALL(platform_, GetGroupId(_, _)).WillOnce(Return(true));
EXPECT_CALL(platform_, SetOwnership(suggested_mount_path, _, _))
.WillOnce(Return(true));
EXPECT_CALL(platform_, SetPermissions(suggested_mount_path, _))
@@ -311,7 +305,6 @@
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectory(_)).Times(0);
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectoryWithFallback(_, _))
.WillOnce(Return(true));
- EXPECT_CALL(platform_, GetGroupId(_, _)).WillOnce(Return(true));
EXPECT_CALL(platform_, SetOwnership(suggested_mount_path, _, _))
.WillOnce(Return(true));
EXPECT_CALL(platform_, SetPermissions(suggested_mount_path, _))
@@ -395,7 +388,6 @@
.WillOnce(Return(true));
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectoryWithFallback(_, _))
.Times(0);
- EXPECT_CALL(platform_, GetGroupId(_, _)).WillOnce(Return(true));
EXPECT_CALL(platform_, SetOwnership(mount_path_, _, _))
.WillOnce(Return(true));
EXPECT_CALL(platform_, SetPermissions(mount_path_, _))
@@ -431,7 +423,6 @@
.WillOnce(Return(true));
EXPECT_CALL(platform_, CreateOrReuseEmptyDirectoryWithFallback(_, _))
.Times(0);
- EXPECT_CALL(platform_, GetGroupId(_, _)).WillOnce(Return(true));
EXPECT_CALL(platform_, SetOwnership(mount_path_, _, _))
.WillOnce(Return(true));
EXPECT_CALL(platform_, SetPermissions(mount_path_, _))
diff --git a/platform.cc b/platform.cc
index b4fd9c7..924d227 100644
--- a/platform.cc
+++ b/platform.cc
@@ -23,7 +23,9 @@
static const unsigned kFallbackPasswordBufferSize = 16384;
Platform::Platform()
- : experimental_features_enabled_(false) {
+ : experimental_features_enabled_(false),
+ mount_group_id_(0),
+ mount_user_id_(0) {
}
Platform::~Platform() {
@@ -114,6 +116,10 @@
return true;
}
+bool Platform::SetMountUser(const std::string& user_name) {
+ return GetUserAndGroupId(user_name, &mount_user_id_, &mount_group_id_);
+}
+
bool Platform::RemoveEmptyDirectory(const string& path) const {
if (rmdir(path.c_str()) != 0) {
PLOG(WARNING) << "Failed to remove directory '" << path << "'";
diff --git a/platform.h b/platform.h
index ea193d6..3ac212f 100644
--- a/platform.h
+++ b/platform.h
@@ -6,6 +6,7 @@
#define CROS_DISKS_PLATFORM_H_
#include <sys/stat.h>
+#include <sys/types.h>
#include <string>
@@ -50,6 +51,13 @@
// Returns true on success.
virtual bool RemoveEmptyDirectory(const std::string& path) const;
+ // Makes |user_name| to perform mount operations, which changes the value of
+ // mount_group_id_ and mount_user_id_. When |user_name| is a non-root user, a
+ // mount operation repsecting the value of mount_group_id_ and mount_user_id_
+ // becomes non-privileged. Returns false if it fails to obtain the user and
+ // group ID of |user_name|.
+ bool SetMountUser(const std::string& user_name);
+
// Sets the user ID and group ID of |path| to |user_id| and |group_id|,
// respectively. Returns true on success.
virtual bool SetOwnership(const std::string& path,
@@ -68,10 +76,20 @@
experimental_features_enabled_ = experimental_features_enabled;
}
+ gid_t mount_group_id() const { return mount_group_id_; }
+
+ uid_t mount_user_id() const { return mount_user_id_; }
+
private:
// This variable is set to true if the experimental features are enabled.
bool experimental_features_enabled_;
+ // Group ID to perform mount operations.
+ gid_t mount_group_id_;
+
+ // User ID to perform mount operations.
+ uid_t mount_user_id_;
+
DISALLOW_COPY_AND_ASSIGN(Platform);
};
diff --git a/platform_unittest.cc b/platform_unittest.cc
index 9825a55..5006b19 100644
--- a/platform_unittest.cc
+++ b/platform_unittest.cc
@@ -154,6 +154,20 @@
EXPECT_FALSE(platform_.RemoveEmptyDirectory(path));
}
+TEST_F(PlatformTest, SetMountUserToRoot) {
+ EXPECT_TRUE(platform_.SetMountUser("root"));
+ EXPECT_EQ(0, platform_.mount_user_id());
+ EXPECT_EQ(0, platform_.mount_group_id());
+}
+
+TEST_F(PlatformTest, SetMountUserToNonexistentUser) {
+ uid_t user_id = platform_.mount_user_id();
+ gid_t group_id = platform_.mount_group_id();
+ EXPECT_FALSE(platform_.SetMountUser("nonexistent-user"));
+ EXPECT_EQ(user_id, platform_.mount_user_id());
+ EXPECT_EQ(group_id, platform_.mount_group_id());
+}
+
TEST_F(PlatformTest, SetOwnershipOfNonExistentPath) {
EXPECT_FALSE(platform_.SetOwnership("/nonexistent-path", getuid(), getgid()));
}
