[PATCH master 0/8] File storage whitelist
44 views
Skip to first unread message
Michael Hanselmann
Oct 4, 2012, 9:55:36 PM10/4/12
to ganeti...@googlegroups.com
This is a first implementation of a concept Iustin Pop proposed (message ID
“20120908204...@google.com”) for fixing inherently insecure file
storage operations. Before this series the file storage paths would be taken
from the configuration on a node, but said configuration file is also updated
via RPC. After this changelist a new file, “/etc/ganeti/file-storage-paths”,
is used as a whitelist.
Michael Hanselmann (8):
backend: Check for shared storage also
LUClusterVerifyGroup: Localize virtual file paths
bdev: Add functions to verify file storage paths
Check fingerprint of file with allowed file storage paths
Check allowed file storage paths during cluster-verify
backend: Switch to new file storage directory verification
cfgupgrade: Write file for file storage paths
Update NEWS for file storage paths
NEWS | 11 ++++
lib/backend.py | 30 ++++++-----
lib/bdev.py | 62 ++++++++++++++++++++++
lib/cmdlib.py | 105 +++++++++++++++++++++++++++++++++++---
lib/constants.py | 4 ++
lib/errors.py | 6 ++
lib/opcodes.py | 1 +
lib/pathutils.py | 1 +
test/cfgupgrade_unittest.py | 52 ++++++++++++++++++-
test/ganeti.bdev_unittest.py | 51 ++++++++++++++++++-
test/ganeti.cmdlib_unittest.py | 20 +++++++
test/ganeti.utils.io_unittest.py | 6 ++
tools/cfgupgrade | 41 +++++++++++++++
13 files changed, 365 insertions(+), 25 deletions(-)
--
1.7.7.3
“20120908204...@google.com”) for fixing inherently insecure file
storage operations. Before this series the file storage paths would be taken
from the configuration on a node, but said configuration file is also updated
via RPC. After this changelist a new file, “/etc/ganeti/file-storage-paths”,
is used as a whitelist.
Michael Hanselmann (8):
backend: Check for shared storage also
LUClusterVerifyGroup: Localize virtual file paths
bdev: Add functions to verify file storage paths
Check fingerprint of file with allowed file storage paths
Check allowed file storage paths during cluster-verify
backend: Switch to new file storage directory verification
cfgupgrade: Write file for file storage paths
Update NEWS for file storage paths
NEWS | 11 ++++
lib/backend.py | 30 ++++++-----
lib/bdev.py | 62 ++++++++++++++++++++++
lib/cmdlib.py | 105 +++++++++++++++++++++++++++++++++++---
lib/constants.py | 4 ++
lib/errors.py | 6 ++
lib/opcodes.py | 1 +
lib/pathutils.py | 1 +
test/cfgupgrade_unittest.py | 52 ++++++++++++++++++-
test/ganeti.bdev_unittest.py | 51 ++++++++++++++++++-
test/ganeti.cmdlib_unittest.py | 20 +++++++
test/ganeti.utils.io_unittest.py | 6 ++
tools/cfgupgrade | 41 +++++++++++++++
13 files changed, 365 insertions(+), 25 deletions(-)
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:37 PM10/4/12
to ganeti...@googlegroups.com
If normal file storage was disabled but shared storage enabled,
“_TransformFileStorageDir” would still throw an exception.
in “opcodes._CheckStorageType” there's also a check, but I wasn't quite
sure what the correct way of handling it was, so I added a TODO comment.
---
lib/backend.py | 3 ++-
lib/opcodes.py | 1 +
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index dc1378f..07ffa75 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -2702,7 +2702,8 @@ def _TransformFileStorageDir(fs_dir):
@return: the normalized path if valid, None otherwise
"""
- if not constants.ENABLE_FILE_STORAGE:
+ if not (constants.ENABLE_FILE_STORAGE or
+ constants.ENABLE_SHARED_FILE_STORAGE):
_Fail("File storage disabled at configure time")
cfg = _GetConfig()
fs_dir = os.path.normpath(fs_dir)
diff --git a/lib/opcodes.py b/lib/opcodes.py
index dba9fd5..8a88bf0 100644
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -334,6 +334,7 @@ def _CheckStorageType(storage_type):
raise errors.OpPrereqError("Unknown storage type: %s" % storage_type,
errors.ECODE_INVAL)
if storage_type == constants.ST_FILE:
+ # TODO: What about shared file storage?
RequireFileStorage()
return True
--
1.7.7.3
“_TransformFileStorageDir” would still throw an exception.
in “opcodes._CheckStorageType” there's also a check, but I wasn't quite
sure what the correct way of handling it was, so I added a TODO comment.
---
lib/backend.py | 3 ++-
lib/opcodes.py | 1 +
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index dc1378f..07ffa75 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -2702,7 +2702,8 @@ def _TransformFileStorageDir(fs_dir):
@return: the normalized path if valid, None otherwise
"""
- if not constants.ENABLE_FILE_STORAGE:
+ if not (constants.ENABLE_FILE_STORAGE or
+ constants.ENABLE_SHARED_FILE_STORAGE):
_Fail("File storage disabled at configure time")
cfg = _GetConfig()
fs_dir = os.path.normpath(fs_dir)
diff --git a/lib/opcodes.py b/lib/opcodes.py
index dba9fd5..8a88bf0 100644
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -334,6 +334,7 @@ def _CheckStorageType(storage_type):
raise errors.OpPrereqError("Unknown storage type: %s" % storage_type,
errors.ECODE_INVAL)
if storage_type == constants.ST_FILE:
+ # TODO: What about shared file storage?
RequireFileStorage()
return True
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:42 PM10/4/12
to ganeti...@googlegroups.com
The configuration is no longer used for verifying file storage paths.
---
lib/backend.py | 14 ++++----------
lib/bdev.py | 7 +++++++
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index a4e7c9a..9500e10 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -2714,16 +2714,10 @@ def _TransformFileStorageDir(fs_dir):
if not (constants.ENABLE_FILE_STORAGE or
- fs_dir = os.path.normpath(fs_dir)
- base_fstore = cfg.GetFileStorageDir()
- base_shared = cfg.GetSharedFileStorageDir()
- if not (utils.IsBelowDir(base_fstore, fs_dir) or
- utils.IsBelowDir(base_shared, fs_dir)):
- _Fail("File storage directory '%s' is not under base file"
- " storage directory '%s' or shared storage directory '%s'",
- fs_dir, base_fstore, base_shared)
- return fs_dir
+
+ bdev.CheckFileStoragePath(fs_dir)
+
+ return os.path.normpath(fs_dir)
def CreateFileStorageDir(file_storage_dir):
diff --git a/lib/bdev.py b/lib/bdev.py
index 8273eb6..486ef12 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -2169,6 +2169,9 @@ class FileStorage(BlockDev):
raise ValueError("Invalid configuration data %s" % str(unique_id))
self.driver = unique_id[0]
self.dev_path = unique_id[1]
+
+ CheckFileStoragePath(self.dev_path)
+
self.Attach()
def Assemble(self):
@@ -2285,7 +2288,11 @@ class FileStorage(BlockDev):
"""
if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
raise ValueError("Invalid configuration data %s" % str(unique_id))
+
dev_path = unique_id[1]
+
+ CheckFileStoragePath(dev_path)
+
try:
fd = os.open(dev_path, os.O_RDWR | os.O_CREAT | os.O_EXCL)
f = os.fdopen(fd, "w")
--
1.7.7.3
---
lib/backend.py | 14 ++++----------
lib/bdev.py | 7 +++++++
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index a4e7c9a..9500e10 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -2714,16 +2714,10 @@ def _TransformFileStorageDir(fs_dir):
if not (constants.ENABLE_FILE_STORAGE or
constants.ENABLE_SHARED_FILE_STORAGE):
_Fail("File storage disabled at configure time")
- cfg = _GetConfig()
_Fail("File storage disabled at configure time")
- fs_dir = os.path.normpath(fs_dir)
- base_fstore = cfg.GetFileStorageDir()
- base_shared = cfg.GetSharedFileStorageDir()
- if not (utils.IsBelowDir(base_fstore, fs_dir) or
- utils.IsBelowDir(base_shared, fs_dir)):
- _Fail("File storage directory '%s' is not under base file"
- " storage directory '%s' or shared storage directory '%s'",
- fs_dir, base_fstore, base_shared)
- return fs_dir
+
+ bdev.CheckFileStoragePath(fs_dir)
+
+ return os.path.normpath(fs_dir)
def CreateFileStorageDir(file_storage_dir):
diff --git a/lib/bdev.py b/lib/bdev.py
index 8273eb6..486ef12 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -2169,6 +2169,9 @@ class FileStorage(BlockDev):
raise ValueError("Invalid configuration data %s" % str(unique_id))
self.driver = unique_id[0]
self.dev_path = unique_id[1]
+
+ CheckFileStoragePath(self.dev_path)
+
self.Attach()
def Assemble(self):
@@ -2285,7 +2288,11 @@ class FileStorage(BlockDev):
"""
if not isinstance(unique_id, (tuple, list)) or len(unique_id) != 2:
raise ValueError("Invalid configuration data %s" % str(unique_id))
+
dev_path = unique_id[1]
+
+ CheckFileStoragePath(dev_path)
+
try:
fd = os.open(dev_path, os.O_RDWR | os.O_CREAT | os.O_EXCL)
f = os.fdopen(fd, "w")
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:41 PM10/4/12
to ganeti...@googlegroups.com
Some paths, such as /bin or /usr/lib, should not be used for file
storage. This patch implements a check during cluster verification
to show a warning in case such a path has been used.
---
lib/backend.py | 6 +++
lib/cmdlib.py | 74 ++++++++++++++++++++++++++++++++++++++++
lib/constants.py | 4 ++
test/ganeti.cmdlib_unittest.py | 20 +++++++++++
4 files changed, 104 insertions(+), 0 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index 4a87fdb..a4e7c9a 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -797,6 +797,12 @@ def VerifyNode(what, cluster_name):
result[constants.NV_BRIDGES] = [bridge
for bridge in what[constants.NV_BRIDGES]
if not utils.BridgeExists(bridge)]
+
+ if what.get(constants.NV_FILE_STORAGE_PATHS) == my_name:
+ result[constants.NV_FILE_STORAGE_PATHS] = \
+ (pathutils.FILE_STORAGE_PATHS_FILE,
+ bdev.LoadAllowedFileStoragePaths(pathutils.FILE_STORAGE_PATHS_FILE))
+
return result
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 5e6d5e9..aee786a 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -1783,6 +1783,48 @@ def _VerifyCertificate(filename):
raise errors.ProgrammerError("Unhandled certificate error code %r" % errcode)
+def _GetFileStorageWarningPaths():
+ """Builds a list of path prefixes which shouldn't be used for file storage.
+
+ """
+ paths = set([
+ "/boot",
+ "/dev",
+ "/etc",
+ "/home",
+ "/proc",
+ "/root",
+ "/sys",
+ ])
+
+ for prefix in ["", "/usr", "/usr/local"]:
+ paths.update(map(lambda s: "%s/%s" % (prefix, s),
+ ["bin", "lib", "lib32", "lib64", "sbin"]))
+
+ return frozenset(map(os.path.normpath, paths))
+
+
+def _CheckAllowedFileStoragePaths(paths, _warn=_GetFileStorageWarningPaths()):
+ """Cross-checks a list of paths for prefixes considered critical.
+
+ Some paths, e.g. "/bin", should not be used for file storage.
+
+ @type paths: list
+ @param paths: List of paths to be checked
+ @rtype: list
+ @return: Sorted list of paths for which the user should be warned
+
+ """
+ clean_paths = map(os.path.normpath, paths)
+
+ result = [path
+ for path in clean_paths
+ for warn_path in _warn
+ if warn_path == path or utils.IsBelowDir(warn_path, path)]
+
+ return utils.NiceSort(result)
+
+
def _GetAllHypervisorParameters(cluster, instances):
"""Compute the set of all hypervisor parameters.
@@ -2768,6 +2810,32 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
"OSes present on reference node %s but missing on this node: %s",
base.name, utils.CommaJoin(missing))
+ def _VerifyFileStoragePaths(self, ninfo, nresult, is_master):
+ """Verifies paths in L{pathutils.FILE_STORAGE_PATHS_FILE}.
+
+ @type ninfo: L{objects.Node}
+ @param ninfo: the node to check
+ @param nresult: the remote results for the node
+ @type is_master: bool
+ @param is_master: Whether node is the master node
+
+ """
+ node = ninfo.name
+
+ if is_master:
+ (filename_on_node, allowed_paths) = \
+ nresult[constants.NV_FILE_STORAGE_PATHS]
+
+ warn = _CheckAllowedFileStoragePaths(allowed_paths)
+
+ self._ErrorIf(warn, constants.CV_ENODEFILESTORAGEPATHS, node,
+ ("File '%s' allows file storage paths which shouldn't"
+ " be used: %s"),
+ filename_on_node, utils.CommaJoin(warn),
+ code=self.ETYPE_WARNING)
+ else:
+ assert constants.NV_FILE_STORAGE_PATHS not in nresult
+
def _VerifyOob(self, ninfo, nresult):
"""Verifies out of band functionality of a node.
@@ -3110,6 +3178,10 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
node_verify_param[constants.NV_DRBDLIST] = None
node_verify_param[constants.NV_DRBDHELPER] = drbd_helper
+ if constants.ENABLE_FILE_STORAGE or constants.ENABLE_SHARED_FILE_STORAGE:
+ # Load file storage paths only from master node
+ node_verify_param[constants.NV_FILE_STORAGE_PATHS] = master_node
+
# bridge checks
# FIXME: this needs to be changed per node-group, not cluster-wide
bridges = set()
@@ -3263,6 +3335,8 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
self._VerifyNodeNetwork(node_i, nresult)
self._VerifyNodeUserScripts(node_i, nresult)
self._VerifyOob(node_i, nresult)
+ self._VerifyFileStoragePaths(node_i, nresult,
+ node == master_node)
if nimg.vm_capable:
self._VerifyNodeLVM(node_i, nresult, vg_name)
diff --git a/lib/constants.py b/lib/constants.py
index 1d9787c..59fd225 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -1326,6 +1326,8 @@ CV_ENODEOOBPATH = \
(CV_TNODE, "ENODEOOBPATH", "Invalid Out Of Band path")
CV_ENODEUSERSCRIPTS = \
(CV_TNODE, "ENODEUSERSCRIPTS", "User scripts not present or not executable")
+CV_ENODEFILESTORAGEPATHS = \
+ (CV_TNODE, "ENODEFILESTORAGEPATHS", "Detected bad file storage paths")
CV_ALL_ECODES = frozenset([
CV_ECLUSTERCFG,
@@ -1359,6 +1361,7 @@ CV_ALL_ECODES = frozenset([
CV_ENODETIME,
CV_ENODEOOBPATH,
CV_ENODEUSERSCRIPTS,
+ CV_ENODEFILESTORAGEPATHS,
])
CV_ALL_ECODES_STRINGS = frozenset(estr for (_, estr, _) in CV_ALL_ECODES)
@@ -1385,6 +1388,7 @@ NV_VMNODES = "vmnodes"
NV_OOB_PATHS = "oob-paths"
NV_BRIDGES = "bridges"
NV_USERSCRIPTS = "user-scripts"
+NV_FILE_STORAGE_PATHS = "file-storage-paths"
# Instance status
INSTST_RUNNING = "running"
diff --git a/test/ganeti.cmdlib_unittest.py b/test/ganeti.cmdlib_unittest.py
index ab5ef6f..7128b58 100755
--- a/test/ganeti.cmdlib_unittest.py
+++ b/test/ganeti.cmdlib_unittest.py
@@ -1478,5 +1478,25 @@ class TestDiskSizeInBytesToMebibytes(unittest.TestCase):
self.assertEqual(warnsize, (1024 * 1024) - j)
+class TestCheckAllowedFileStoragePaths(unittest.TestCase):
+ def testPaths(self):
+ warn_paths = cmdlib._GetFileStorageWarningPaths()
+
+ for path in ["/bin", "/usr/local/sbin", "/lib64", "/etc"]:
+ self.assertTrue(path in warn_paths)
+
+ self.assertEqual(set(map(os.path.normpath, warn_paths)),
+ warn_paths)
+
+ def test(self):
+ cafsp = cmdlib._CheckAllowedFileStoragePaths
+ self.assertEqual(cafsp([]), [])
+ self.assertEqual(cafsp(["/tmp"]), [])
+ self.assertEqual(cafsp(["/bin/ls"]), ["/bin/ls"])
+ self.assertEqual(cafsp(["/bin"]), ["/bin"])
+ self.assertEqual(cafsp(["/usr/sbin/vim", "/srv/file-storage"]),
+ ["/usr/sbin/vim"])
+
+
if __name__ == "__main__":
testutils.GanetiTestProgram()
--
1.7.7.3
storage. This patch implements a check during cluster verification
to show a warning in case such a path has been used.
---
lib/backend.py | 6 +++
lib/cmdlib.py | 74 ++++++++++++++++++++++++++++++++++++++++
lib/constants.py | 4 ++
test/ganeti.cmdlib_unittest.py | 20 +++++++++++
4 files changed, 104 insertions(+), 0 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index 4a87fdb..a4e7c9a 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -797,6 +797,12 @@ def VerifyNode(what, cluster_name):
result[constants.NV_BRIDGES] = [bridge
for bridge in what[constants.NV_BRIDGES]
if not utils.BridgeExists(bridge)]
+
+ if what.get(constants.NV_FILE_STORAGE_PATHS) == my_name:
+ result[constants.NV_FILE_STORAGE_PATHS] = \
+ (pathutils.FILE_STORAGE_PATHS_FILE,
+ bdev.LoadAllowedFileStoragePaths(pathutils.FILE_STORAGE_PATHS_FILE))
+
return result
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 5e6d5e9..aee786a 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -1783,6 +1783,48 @@ def _VerifyCertificate(filename):
raise errors.ProgrammerError("Unhandled certificate error code %r" % errcode)
+def _GetFileStorageWarningPaths():
+ """Builds a list of path prefixes which shouldn't be used for file storage.
+
+ """
+ paths = set([
+ "/boot",
+ "/dev",
+ "/etc",
+ "/home",
+ "/proc",
+ "/root",
+ "/sys",
+ ])
+
+ for prefix in ["", "/usr", "/usr/local"]:
+ paths.update(map(lambda s: "%s/%s" % (prefix, s),
+ ["bin", "lib", "lib32", "lib64", "sbin"]))
+
+ return frozenset(map(os.path.normpath, paths))
+
+
+def _CheckAllowedFileStoragePaths(paths, _warn=_GetFileStorageWarningPaths()):
+ """Cross-checks a list of paths for prefixes considered critical.
+
+ Some paths, e.g. "/bin", should not be used for file storage.
+
+ @type paths: list
+ @param paths: List of paths to be checked
+ @rtype: list
+ @return: Sorted list of paths for which the user should be warned
+
+ """
+ clean_paths = map(os.path.normpath, paths)
+
+ result = [path
+ for path in clean_paths
+ for warn_path in _warn
+ if warn_path == path or utils.IsBelowDir(warn_path, path)]
+
+ return utils.NiceSort(result)
+
+
def _GetAllHypervisorParameters(cluster, instances):
"""Compute the set of all hypervisor parameters.
@@ -2768,6 +2810,32 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
"OSes present on reference node %s but missing on this node: %s",
base.name, utils.CommaJoin(missing))
+ def _VerifyFileStoragePaths(self, ninfo, nresult, is_master):
+ """Verifies paths in L{pathutils.FILE_STORAGE_PATHS_FILE}.
+
+ @type ninfo: L{objects.Node}
+ @param ninfo: the node to check
+ @param nresult: the remote results for the node
+ @type is_master: bool
+ @param is_master: Whether node is the master node
+
+ """
+ node = ninfo.name
+
+ if is_master:
+ (filename_on_node, allowed_paths) = \
+ nresult[constants.NV_FILE_STORAGE_PATHS]
+
+ warn = _CheckAllowedFileStoragePaths(allowed_paths)
+
+ self._ErrorIf(warn, constants.CV_ENODEFILESTORAGEPATHS, node,
+ ("File '%s' allows file storage paths which shouldn't"
+ " be used: %s"),
+ filename_on_node, utils.CommaJoin(warn),
+ code=self.ETYPE_WARNING)
+ else:
+ assert constants.NV_FILE_STORAGE_PATHS not in nresult
+
def _VerifyOob(self, ninfo, nresult):
"""Verifies out of band functionality of a node.
@@ -3110,6 +3178,10 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
node_verify_param[constants.NV_DRBDLIST] = None
node_verify_param[constants.NV_DRBDHELPER] = drbd_helper
+ if constants.ENABLE_FILE_STORAGE or constants.ENABLE_SHARED_FILE_STORAGE:
+ # Load file storage paths only from master node
+ node_verify_param[constants.NV_FILE_STORAGE_PATHS] = master_node
+
# bridge checks
# FIXME: this needs to be changed per node-group, not cluster-wide
bridges = set()
@@ -3263,6 +3335,8 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
self._VerifyNodeNetwork(node_i, nresult)
self._VerifyNodeUserScripts(node_i, nresult)
self._VerifyOob(node_i, nresult)
+ self._VerifyFileStoragePaths(node_i, nresult,
+ node == master_node)
if nimg.vm_capable:
self._VerifyNodeLVM(node_i, nresult, vg_name)
diff --git a/lib/constants.py b/lib/constants.py
index 1d9787c..59fd225 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -1326,6 +1326,8 @@ CV_ENODEOOBPATH = \
(CV_TNODE, "ENODEOOBPATH", "Invalid Out Of Band path")
CV_ENODEUSERSCRIPTS = \
(CV_TNODE, "ENODEUSERSCRIPTS", "User scripts not present or not executable")
+CV_ENODEFILESTORAGEPATHS = \
+ (CV_TNODE, "ENODEFILESTORAGEPATHS", "Detected bad file storage paths")
CV_ALL_ECODES = frozenset([
CV_ECLUSTERCFG,
@@ -1359,6 +1361,7 @@ CV_ALL_ECODES = frozenset([
CV_ENODETIME,
CV_ENODEOOBPATH,
CV_ENODEUSERSCRIPTS,
+ CV_ENODEFILESTORAGEPATHS,
])
CV_ALL_ECODES_STRINGS = frozenset(estr for (_, estr, _) in CV_ALL_ECODES)
@@ -1385,6 +1388,7 @@ NV_VMNODES = "vmnodes"
NV_OOB_PATHS = "oob-paths"
NV_BRIDGES = "bridges"
NV_USERSCRIPTS = "user-scripts"
+NV_FILE_STORAGE_PATHS = "file-storage-paths"
# Instance status
INSTST_RUNNING = "running"
diff --git a/test/ganeti.cmdlib_unittest.py b/test/ganeti.cmdlib_unittest.py
index ab5ef6f..7128b58 100755
--- a/test/ganeti.cmdlib_unittest.py
+++ b/test/ganeti.cmdlib_unittest.py
@@ -1478,5 +1478,25 @@ class TestDiskSizeInBytesToMebibytes(unittest.TestCase):
self.assertEqual(warnsize, (1024 * 1024) - j)
+class TestCheckAllowedFileStoragePaths(unittest.TestCase):
+ def testPaths(self):
+ warn_paths = cmdlib._GetFileStorageWarningPaths()
+
+ for path in ["/bin", "/usr/local/sbin", "/lib64", "/etc"]:
+ self.assertTrue(path in warn_paths)
+
+ self.assertEqual(set(map(os.path.normpath, warn_paths)),
+ warn_paths)
+
+ def test(self):
+ cafsp = cmdlib._CheckAllowedFileStoragePaths
+ self.assertEqual(cafsp([]), [])
+ self.assertEqual(cafsp(["/tmp"]), [])
+ self.assertEqual(cafsp(["/bin/ls"]), ["/bin/ls"])
+ self.assertEqual(cafsp(["/bin"]), ["/bin"])
+ self.assertEqual(cafsp(["/usr/sbin/vim", "/srv/file-storage"]),
+ ["/usr/sbin/vim"])
+
+
if __name__ == "__main__":
testutils.GanetiTestProgram()
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:44 PM10/4/12
to ganeti...@googlegroups.com
Mention that the file is something new and should be written by
cfgupgrade.
---
NEWS | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/NEWS b/NEWS
index f9b8a72..d678b49 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,17 @@ Version 2.7.0 beta0
opcode from Ganeti. This lead to incompatible changes in the format of
the JSON file. It's now not a custom dict anymore but a dict
compatible with the ``OpInstanceCreate`` opcode.
+- Parent directories for file storage need now to be listed in
+ ``$sysconfdir/ganeti/file-storage-paths``. ``cfgupgrade`` will write
+ the file automatically based on old configuration values, but it can
+ not distribute it across all nodes and the file contents should be
+ verified. Use ``gnt-cluster copyfile
+ $sysconfdir/ganeti/file-storage-paths`` once the cluster has been
+ upgraded. The reason for requiring this list of paths now is that
+ before it would have been possible to inject new paths via RPC,
+ allowing files to be created in arbitrary locations. The RPC protocol
+ is protected using SSL/X.509 certificates, but as a design principle
+ Ganeti does not permit arbitrary paths to be passed.
Version 2.6.0
--
1.7.7.3
cfgupgrade.
---
NEWS | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/NEWS b/NEWS
index f9b8a72..d678b49 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,17 @@ Version 2.7.0 beta0
opcode from Ganeti. This lead to incompatible changes in the format of
the JSON file. It's now not a custom dict anymore but a dict
compatible with the ``OpInstanceCreate`` opcode.
+- Parent directories for file storage need now to be listed in
+ ``$sysconfdir/ganeti/file-storage-paths``. ``cfgupgrade`` will write
+ the file automatically based on old configuration values, but it can
+ not distribute it across all nodes and the file contents should be
+ verified. Use ``gnt-cluster copyfile
+ $sysconfdir/ganeti/file-storage-paths`` once the cluster has been
+ upgraded. The reason for requiring this list of paths now is that
+ before it would have been possible to inject new paths via RPC,
+ allowing files to be created in arbitrary locations. The RPC protocol
+ is protected using SSL/X.509 certificates, but as a design principle
+ Ganeti does not permit arbitrary paths to be passed.
Version 2.6.0
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:38 PM10/4/12
to ganeti...@googlegroups.com
The check for file consistency didn't properly handle virtual paths
in case of a virtual cluster. This didn't cause any breakage as in
a standard virtual cluster setup with only one node all files are
visible for every node.
---
lib/backend.py | 7 +++++--
lib/cmdlib.py | 13 +++++++++----
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index 07ffa75..4a87fdb 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -644,8 +644,11 @@ def VerifyNode(what, cluster_name):
tmp.append((source, hv_name, str(err)))
if constants.NV_FILELIST in what:
- result[constants.NV_FILELIST] = utils.FingerprintFiles(
- what[constants.NV_FILELIST])
+ fingerprints = utils.FingerprintFiles(map(vcluster.LocalizeVirtualPath,
+ what[constants.NV_FILELIST]))
+ result[constants.NV_FILELIST] = \
+ dict((vcluster.MakeVirtualPath(key), value)
+ for (key, value) in fingerprints.items())
if constants.NV_NODELIST in what:
(nodes, bynode) = what[constants.NV_NODELIST]
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index ae08146..b72276c 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -60,6 +60,7 @@ from ganeti import ht
from ganeti import rpc
from ganeti import runtime
from ganeti import pathutils
+from ganeti import vcluster
from ganeti.masterd import iallocator
import ganeti.masterd.instance # pylint: disable=W0611
@@ -2549,7 +2550,10 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
if nresult.fail_msg or not nresult.payload:
node_files = None
else:
- node_files = nresult.payload.get(constants.NV_FILELIST, None)
+ fingerprints = nresult.payload.get(constants.NV_FILELIST, None)
+ node_files = dict((vcluster.LocalizeVirtualPath(key), value)
+ for (key, value) in fingerprints.items())
+ del fingerprints
test = not (node_files and isinstance(node_files, dict))
errorif(test, constants.CV_ENODEFILECHECK, node.name,
@@ -3073,9 +3077,10 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
node_verify_param = {
constants.NV_FILELIST:
- utils.UniqueSequence(filename
- for files in filemap
- for filename in files),
+ map(vcluster.MakeVirtualPath,
+ utils.UniqueSequence(filename
+ for files in filemap
+ for filename in files)),
constants.NV_NODELIST:
self._SelectSshCheckNodes(node_data_list, self.group_uuid,
self.all_node_info.values()),
--
1.7.7.3
in case of a virtual cluster. This didn't cause any breakage as in
a standard virtual cluster setup with only one node all files are
visible for every node.
---
lib/backend.py | 7 +++++--
lib/cmdlib.py | 13 +++++++++----
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index 07ffa75..4a87fdb 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -644,8 +644,11 @@ def VerifyNode(what, cluster_name):
tmp.append((source, hv_name, str(err)))
if constants.NV_FILELIST in what:
- result[constants.NV_FILELIST] = utils.FingerprintFiles(
- what[constants.NV_FILELIST])
+ fingerprints = utils.FingerprintFiles(map(vcluster.LocalizeVirtualPath,
+ what[constants.NV_FILELIST]))
+ result[constants.NV_FILELIST] = \
+ dict((vcluster.MakeVirtualPath(key), value)
+ for (key, value) in fingerprints.items())
if constants.NV_NODELIST in what:
(nodes, bynode) = what[constants.NV_NODELIST]
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index ae08146..b72276c 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -60,6 +60,7 @@ from ganeti import ht
from ganeti import rpc
from ganeti import runtime
from ganeti import pathutils
+from ganeti import vcluster
from ganeti.masterd import iallocator
import ganeti.masterd.instance # pylint: disable=W0611
@@ -2549,7 +2550,10 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
if nresult.fail_msg or not nresult.payload:
node_files = None
else:
- node_files = nresult.payload.get(constants.NV_FILELIST, None)
+ fingerprints = nresult.payload.get(constants.NV_FILELIST, None)
+ node_files = dict((vcluster.LocalizeVirtualPath(key), value)
+ for (key, value) in fingerprints.items())
+ del fingerprints
test = not (node_files and isinstance(node_files, dict))
errorif(test, constants.CV_ENODEFILECHECK, node.name,
@@ -3073,9 +3077,10 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
node_verify_param = {
constants.NV_FILELIST:
- utils.UniqueSequence(filename
- for files in filemap
- for filename in files),
+ map(vcluster.MakeVirtualPath,
+ utils.UniqueSequence(filename
+ for files in filemap
+ for filename in files)),
constants.NV_NODELIST:
self._SelectSshCheckNodes(node_data_list, self.group_uuid,
self.all_node_info.values()),
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:43 PM10/4/12
to ganeti...@googlegroups.com
When file storage is used this file is now mandatory.
---
test/cfgupgrade_unittest.py | 52 ++++++++++++++++++++++++++++++++++++++++--
tools/cfgupgrade | 41 +++++++++++++++++++++++++++++++++
2 files changed, 90 insertions(+), 3 deletions(-)
diff --git a/test/cfgupgrade_unittest.py b/test/cfgupgrade_unittest.py
index 7ca5c3d..9764b6e 100755
--- a/test/cfgupgrade_unittest.py
+++ b/test/cfgupgrade_unittest.py
@@ -39,7 +39,7 @@ import testutils
def _RunUpgrade(path, dry_run, no_verify, ignore_hostname=True):
cmd = [sys.executable, "%s/tools/cfgupgrade" % testutils.GetSourceDir(),
- "--debug", "--force", "--path=%s" % path]
+ "--debug", "--force", "--path=%s" % path, "--confdir=%s" % path]
if ignore_hostname:
cmd.append("--ignore-hostname")
@@ -67,6 +67,7 @@ class TestCfgupgrade(unittest.TestCase):
self.confd_hmac_path = utils.PathJoin(self.tmpdir, "hmac.key")
self.cds_path = utils.PathJoin(self.tmpdir, "cluster-domain-secret")
self.ss_master_node_path = utils.PathJoin(self.tmpdir, "ssconf_master_node")
+ self.file_storage_paths = utils.PathJoin(self.tmpdir, "file-storage-paths")
def tearDown(self):
shutil.rmtree(self.tmpdir)
@@ -133,10 +134,19 @@ class TestCfgupgrade(unittest.TestCase):
utils.WriteFile(self.config_path, data=serializer.DumpJson({}))
self.assertRaises(Exception, _RunUpgrade, self.tmpdir, False, True)
- def _TestSimpleUpgrade(self, from_version, dry_run):
+ def _TestSimpleUpgrade(self, from_version, dry_run,
+ file_storage_dir=None,
+ shared_file_storage_dir=None):
+ cluster = {}
+
+ if file_storage_dir:
+ cluster["file_storage_dir"] = file_storage_dir
+ if shared_file_storage_dir:
+ cluster["shared_file_storage_dir"] = shared_file_storage_dir
+
cfg = {
"version": from_version,
- "cluster": {},
+ "cluster": cluster,
"instances": {},
}
self._CreateValidConfigDir()
@@ -271,6 +281,42 @@ class TestCfgupgrade(unittest.TestCase):
for path in [self.rapi_users_path, self.rapi_users_path_pre24]:
self.assertEqual(utils.ReadFile(path), "hello world\n")
+ def testFileStoragePathsDryRun(self):
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), True,
+ file_storage_dir=self.tmpdir,
+ shared_file_storage_dir="/tmp")
+
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ def testFileStoragePathsBoth(self):
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), False,
+ file_storage_dir=self.tmpdir,
+ shared_file_storage_dir="/tmp")
+
+ lines = utils.ReadFile(self.file_storage_paths).splitlines()
+ self.assertTrue(lines.pop(0).startswith("# "))
+ self.assertTrue(lines.pop(0).startswith("# cfgupgrade"))
+ self.assertEqual(lines.pop(0), self.tmpdir)
+ self.assertEqual(lines.pop(0), "/tmp")
+ self.assertFalse(lines)
+
+ def testFileStoragePathsSharedOnly(self):
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ self._TestSimpleUpgrade(constants.BuildVersion(2, 5, 0), False,
+ file_storage_dir=None,
+ shared_file_storage_dir=self.tmpdir)
+
+ lines = utils.ReadFile(self.file_storage_paths).splitlines()
+ self.assertTrue(lines.pop(0).startswith("# "))
+ self.assertTrue(lines.pop(0).startswith("# cfgupgrade"))
+ self.assertEqual(lines.pop(0), self.tmpdir)
+ self.assertFalse(lines)
+
def testUpgradeFrom_2_0(self):
self._TestSimpleUpgrade(constants.BuildVersion(2, 0, 0), False)
diff --git a/tools/cfgupgrade b/tools/cfgupgrade
index 604d8aa..5aeb7e4 100755
--- a/tools/cfgupgrade
+++ b/tools/cfgupgrade
@@ -32,6 +32,8 @@ import os.path
import sys
import optparse
import logging
+import time
+from cStringIO import StringIO
from ganeti import constants
from ganeti import serializer
@@ -117,6 +119,10 @@ def main():
parser.add_option("--path", help="Convert configuration in this"
" directory instead of '%s'" % pathutils.DATA_DIR,
default=pathutils.DATA_DIR, dest="data_dir")
+ parser.add_option("--confdir",
+ help=("Use this directory instead of '%s'" %
+ pathutils.CONF_DIR),
+ default=pathutils.CONF_DIR, dest="conf_dir")
parser.add_option("--no-verify",
help="Do not verify configuration after upgrade",
action="store_true", dest="no_verify", default=False)
@@ -137,6 +143,7 @@ def main():
options.CDS_FILE = options.data_dir + "/cluster-domain-secret"
options.SSCONF_MASTER_NODE = options.data_dir + "/ssconf_master_node"
options.WATCHER_STATEFILE = options.data_dir + "/watcher.data"
+ options.FILE_STORAGE_PATHS_FILE = options.conf_dir + "/file-storage-paths"
SetupLogging()
@@ -164,6 +171,9 @@ def main():
raise Error(("%s does not seem to be a Ganeti configuration"
" directory") % options.data_dir)
+ if not os.path.isdir(options.conf_dir):
+ raise Error("Not a directory: %s" % options.conf_dir)
+
config_data = serializer.LoadJson(utils.ReadFile(options.CONFIG_DATA_PATH))
try:
@@ -238,6 +248,37 @@ def main():
if not options.dry_run:
utils.RemoveFile(options.WATCHER_STATEFILE)
+ # Write file storage paths
+ if not os.path.exists(options.FILE_STORAGE_PATHS_FILE):
+ cluster = config_data["cluster"]
+ file_storage_dir = cluster.get("file_storage_dir")
+ shared_file_storage_dir = cluster.get("shared_file_storage_dir")
+ del cluster
+
+ logging.info("Ganeti 2.7 and later only allow whitelisted directories"
+ " for file storage; writing existing configuration values"
+ " into '%s'",
+ options.FILE_STORAGE_PATHS_FILE)
+
+ if file_storage_dir:
+ logging.info("File storage directory: %s", file_storage_dir)
+ if shared_file_storage_dir:
+ logging.info("Shared file storage directory: %s",
+ shared_file_storage_dir)
+
+ buf = StringIO()
+ buf.write("# List automatically generated from configuration by\n")
+ buf.write("# cfgupgrade at %s\n" % time.asctime())
+ if file_storage_dir:
+ buf.write("%s\n" % file_storage_dir)
+ if shared_file_storage_dir:
+ buf.write("%s\n" % shared_file_storage_dir)
+ utils.WriteFile(file_name=options.FILE_STORAGE_PATHS_FILE,
+ data=buf.getvalue(),
+ mode=0644,
+ dry_run=options.dry_run,
+ backup=True)
+
try:
logging.info("Writing configuration file to %s", options.CONFIG_DATA_PATH)
utils.WriteFile(file_name=options.CONFIG_DATA_PATH,
--
1.7.7.3
---
test/cfgupgrade_unittest.py | 52 ++++++++++++++++++++++++++++++++++++++++--
tools/cfgupgrade | 41 +++++++++++++++++++++++++++++++++
2 files changed, 90 insertions(+), 3 deletions(-)
diff --git a/test/cfgupgrade_unittest.py b/test/cfgupgrade_unittest.py
index 7ca5c3d..9764b6e 100755
--- a/test/cfgupgrade_unittest.py
+++ b/test/cfgupgrade_unittest.py
@@ -39,7 +39,7 @@ import testutils
def _RunUpgrade(path, dry_run, no_verify, ignore_hostname=True):
cmd = [sys.executable, "%s/tools/cfgupgrade" % testutils.GetSourceDir(),
- "--debug", "--force", "--path=%s" % path]
+ "--debug", "--force", "--path=%s" % path, "--confdir=%s" % path]
if ignore_hostname:
cmd.append("--ignore-hostname")
@@ -67,6 +67,7 @@ class TestCfgupgrade(unittest.TestCase):
self.confd_hmac_path = utils.PathJoin(self.tmpdir, "hmac.key")
self.cds_path = utils.PathJoin(self.tmpdir, "cluster-domain-secret")
self.ss_master_node_path = utils.PathJoin(self.tmpdir, "ssconf_master_node")
+ self.file_storage_paths = utils.PathJoin(self.tmpdir, "file-storage-paths")
def tearDown(self):
shutil.rmtree(self.tmpdir)
@@ -133,10 +134,19 @@ class TestCfgupgrade(unittest.TestCase):
utils.WriteFile(self.config_path, data=serializer.DumpJson({}))
self.assertRaises(Exception, _RunUpgrade, self.tmpdir, False, True)
- def _TestSimpleUpgrade(self, from_version, dry_run):
+ def _TestSimpleUpgrade(self, from_version, dry_run,
+ file_storage_dir=None,
+ shared_file_storage_dir=None):
+ cluster = {}
+
+ if file_storage_dir:
+ cluster["file_storage_dir"] = file_storage_dir
+ if shared_file_storage_dir:
+ cluster["shared_file_storage_dir"] = shared_file_storage_dir
+
cfg = {
"version": from_version,
- "cluster": {},
+ "cluster": cluster,
"instances": {},
}
self._CreateValidConfigDir()
@@ -271,6 +281,42 @@ class TestCfgupgrade(unittest.TestCase):
for path in [self.rapi_users_path, self.rapi_users_path_pre24]:
self.assertEqual(utils.ReadFile(path), "hello world\n")
+ def testFileStoragePathsDryRun(self):
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), True,
+ file_storage_dir=self.tmpdir,
+ shared_file_storage_dir="/tmp")
+
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ def testFileStoragePathsBoth(self):
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ self._TestSimpleUpgrade(constants.BuildVersion(2, 6, 0), False,
+ file_storage_dir=self.tmpdir,
+ shared_file_storage_dir="/tmp")
+
+ lines = utils.ReadFile(self.file_storage_paths).splitlines()
+ self.assertTrue(lines.pop(0).startswith("# "))
+ self.assertTrue(lines.pop(0).startswith("# cfgupgrade"))
+ self.assertEqual(lines.pop(0), self.tmpdir)
+ self.assertEqual(lines.pop(0), "/tmp")
+ self.assertFalse(lines)
+
+ def testFileStoragePathsSharedOnly(self):
+ self.assertFalse(os.path.exists(self.file_storage_paths))
+
+ self._TestSimpleUpgrade(constants.BuildVersion(2, 5, 0), False,
+ file_storage_dir=None,
+ shared_file_storage_dir=self.tmpdir)
+
+ lines = utils.ReadFile(self.file_storage_paths).splitlines()
+ self.assertTrue(lines.pop(0).startswith("# "))
+ self.assertTrue(lines.pop(0).startswith("# cfgupgrade"))
+ self.assertEqual(lines.pop(0), self.tmpdir)
+ self.assertFalse(lines)
+
def testUpgradeFrom_2_0(self):
self._TestSimpleUpgrade(constants.BuildVersion(2, 0, 0), False)
diff --git a/tools/cfgupgrade b/tools/cfgupgrade
index 604d8aa..5aeb7e4 100755
--- a/tools/cfgupgrade
+++ b/tools/cfgupgrade
@@ -32,6 +32,8 @@ import os.path
import sys
import optparse
import logging
+import time
+from cStringIO import StringIO
from ganeti import constants
from ganeti import serializer
@@ -117,6 +119,10 @@ def main():
parser.add_option("--path", help="Convert configuration in this"
" directory instead of '%s'" % pathutils.DATA_DIR,
default=pathutils.DATA_DIR, dest="data_dir")
+ parser.add_option("--confdir",
+ help=("Use this directory instead of '%s'" %
+ pathutils.CONF_DIR),
+ default=pathutils.CONF_DIR, dest="conf_dir")
parser.add_option("--no-verify",
help="Do not verify configuration after upgrade",
action="store_true", dest="no_verify", default=False)
@@ -137,6 +143,7 @@ def main():
options.CDS_FILE = options.data_dir + "/cluster-domain-secret"
options.SSCONF_MASTER_NODE = options.data_dir + "/ssconf_master_node"
options.WATCHER_STATEFILE = options.data_dir + "/watcher.data"
+ options.FILE_STORAGE_PATHS_FILE = options.conf_dir + "/file-storage-paths"
SetupLogging()
@@ -164,6 +171,9 @@ def main():
raise Error(("%s does not seem to be a Ganeti configuration"
" directory") % options.data_dir)
+ if not os.path.isdir(options.conf_dir):
+ raise Error("Not a directory: %s" % options.conf_dir)
+
config_data = serializer.LoadJson(utils.ReadFile(options.CONFIG_DATA_PATH))
try:
@@ -238,6 +248,37 @@ def main():
if not options.dry_run:
utils.RemoveFile(options.WATCHER_STATEFILE)
+ # Write file storage paths
+ if not os.path.exists(options.FILE_STORAGE_PATHS_FILE):
+ cluster = config_data["cluster"]
+ file_storage_dir = cluster.get("file_storage_dir")
+ shared_file_storage_dir = cluster.get("shared_file_storage_dir")
+ del cluster
+
+ logging.info("Ganeti 2.7 and later only allow whitelisted directories"
+ " for file storage; writing existing configuration values"
+ " into '%s'",
+ options.FILE_STORAGE_PATHS_FILE)
+
+ if file_storage_dir:
+ logging.info("File storage directory: %s", file_storage_dir)
+ if shared_file_storage_dir:
+ logging.info("Shared file storage directory: %s",
+ shared_file_storage_dir)
+
+ buf = StringIO()
+ buf.write("# List automatically generated from configuration by\n")
+ buf.write("# cfgupgrade at %s\n" % time.asctime())
+ if file_storage_dir:
+ buf.write("%s\n" % file_storage_dir)
+ if shared_file_storage_dir:
+ buf.write("%s\n" % shared_file_storage_dir)
+ utils.WriteFile(file_name=options.FILE_STORAGE_PATHS_FILE,
+ data=buf.getvalue(),
+ mode=0644,
+ dry_run=options.dry_run,
+ backup=True)
+
try:
logging.info("Writing configuration file to %s", options.CONFIG_DATA_PATH)
utils.WriteFile(file_name=options.CONFIG_DATA_PATH,
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:39 PM10/4/12
to ganeti...@googlegroups.com
- LoadAllowedFileStoragePaths: Loads a list of allowed file storage
paths from a file
- CheckFileStoragePath: Checks a path against the list of allowed paths
The unit test for “utils.IsBelowDir” is updated with cases which weren't
tested before.
---
lib/bdev.py | 55 ++++++++++++++++++++++++++++++++++++++
lib/errors.py | 6 ++++
lib/pathutils.py | 1 +
test/ganeti.bdev_unittest.py | 51 ++++++++++++++++++++++++++++++++++-
test/ganeti.utils.io_unittest.py | 6 ++++
5 files changed, 118 insertions(+), 1 deletions(-)
diff --git a/lib/bdev.py b/lib/bdev.py
index d6c1a66..8273eb6 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -36,6 +36,7 @@ from ganeti import constants
from ganeti import objects
from ganeti import compat
from ganeti import netutils
+from ganeti import pathutils
# Size of reads in _CanReadDevice
@@ -88,6 +89,60 @@ def _CanReadDevice(path):
return False
+def _CheckFileStoragePath(path, allowed):
+ """Checks if a path is in a list of allowed paths for file storage.
+
+ @type path: string
+ @param path: Path to check
+ @type allowed: list
+ @param allowed: List of allowed paths
+ @raise errors.FileStoragePathError: If the path is not allowed
+
+ """
+ if not os.path.isabs(path):
+ raise errors.FileStoragePathError("File storage path must be absolute,"
+ " got '%s'" % path)
+
+ for i in allowed:
+ if not os.path.isabs(i):
+ logging.info("Ignoring relative path '%s' for file storage", i)
+ continue
+
+ if utils.IsBelowDir(i, path):
+ break
+ else:
+ raise errors.FileStoragePathError("Path '%s' is not acceptable for file"
+ " storage" % path)
+
+
+def LoadAllowedFileStoragePaths(filename):
+ """Loads file containing allowed file storage paths.
+
+ @rtype: list
+ @return: List of allowed paths (can be an empty list)
+
+ """
+ try:
+ contents = utils.ReadFile(filename)
+ except EnvironmentError:
+ return []
+ else:
+ return utils.FilterEmptyLinesAndComments(contents)
+
+
+def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
+ """Checks if a path is allowed for file storage.
+
+ @type path: string
+ @param path: Path to check
+ @raise errors.FileStoragePathError: If the path is not allowed
+
+ """
+ allowed = LoadAllowedFileStoragePaths(_filename)
+
+ return _CheckFileStoragePath(path, allowed)
+
+
class BlockDev(object):
"""Block device abstract class.
diff --git a/lib/errors.py b/lib/errors.py
index b9d578f..062b658 100644
--- a/lib/errors.py
+++ b/lib/errors.py
@@ -430,6 +430,12 @@ class RapiTestResult(GenericError):
"""
+class FileStoragePathError(GenericError):
+ """Error from file storage path validation.
+
+ """
+
+
# errors should be added above
diff --git a/lib/pathutils.py b/lib/pathutils.py
index e60a7a4..457b227 100644
--- a/lib/pathutils.py
+++ b/lib/pathutils.py
@@ -86,6 +86,7 @@ CONF_DIR = SYSCONFDIR + "/ganeti"
USER_SCRIPTS_DIR = CONF_DIR + "/scripts"
VNC_PASSWORD_FILE = CONF_DIR + "/vnc-cluster-password"
HOOKS_BASE_DIR = CONF_DIR + "/hooks"
+FILE_STORAGE_PATHS_FILE = CONF_DIR + "/file-storage-paths"
#: Lock file for watcher, locked in shared mode by watcher; lock in exclusive
# mode to block watcher (see L{cli._RunWhileClusterStoppedHelper.Call}
diff --git a/test/ganeti.bdev_unittest.py b/test/ganeti.bdev_unittest.py
index e1aeef6..e900719 100755
--- a/test/ganeti.bdev_unittest.py
+++ b/test/ganeti.bdev_unittest.py
@@ -28,6 +28,7 @@ import unittest
from ganeti import bdev
from ganeti import errors
from ganeti import constants
+from ganeti import utils
import testutils
@@ -372,5 +373,53 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase):
output_extra_matches, volume_name)
-if __name__ == '__main__':
+class TestCheckFileStoragePath(unittest.TestCase):
+ def testNonAbsolute(self):
+ for i in ["", "tmp", "foo/bar/baz"]:
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, i, ["/tmp"])
+
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, "/tmp", ["tmp", "xyz"])
+
+ def testNoAllowed(self):
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, "/tmp", [])
+
+ def testNoAdditionalPathComponent(self):
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, "/tmp/foo", ["/tmp/foo"])
+
+ def testAllowed(self):
+ bdev._CheckFileStoragePath("/tmp/foo/a", ["/tmp/foo"])
+ bdev._CheckFileStoragePath("/tmp/foo/a/x", ["/tmp/foo"])
+
+
+class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
+ def testDevNull(self):
+ self.assertEqual(bdev.LoadAllowedFileStoragePaths("/dev/null"), [])
+
+ def testNonExistantFile(self):
+ filename = "/tmp/this/file/does/not/exist"
+ assert not os.path.exists(filename)
+ self.assertEqual(bdev.LoadAllowedFileStoragePaths(filename), [])
+
+ def test(self):
+ tmpfile = self._CreateTempFile()
+
+ utils.WriteFile(tmpfile, data="""
+ # This is a test file
+ /tmp
+ /srv/storage
+ relative/path
+ """)
+
+ self.assertEqual(bdev.LoadAllowedFileStoragePaths(tmpfile), [
+ "/tmp",
+ "/srv/storage",
+ "relative/path",
+ ])
+
+
+if __name__ == "__main__":
testutils.GanetiTestProgram()
diff --git a/test/ganeti.utils.io_unittest.py b/test/ganeti.utils.io_unittest.py
index 109232a..bb9dc5e 100755
--- a/test/ganeti.utils.io_unittest.py
+++ b/test/ganeti.utils.io_unittest.py
@@ -646,6 +646,12 @@ class TestIsNormAbsPath(unittest.TestCase):
class TestIsBelowDir(unittest.TestCase):
"""Testing case for IsBelowDir"""
+ def testExactlyTheSame(self):
+ self.assertFalse(utils.IsBelowDir("/a/b", "/a/b"))
+ self.assertFalse(utils.IsBelowDir("/a/b", "/a/b/"))
+ self.assertFalse(utils.IsBelowDir("/a/b/", "/a/b"))
+ self.assertFalse(utils.IsBelowDir("/a/b/", "/a/b/"))
+
def testSamePrefix(self):
self.assertTrue(utils.IsBelowDir("/a/b", "/a/b/c"))
self.assertTrue(utils.IsBelowDir("/a/b/", "/a/b/e"))
--
1.7.7.3
paths from a file
- CheckFileStoragePath: Checks a path against the list of allowed paths
The unit test for “utils.IsBelowDir” is updated with cases which weren't
tested before.
---
lib/bdev.py | 55 ++++++++++++++++++++++++++++++++++++++
lib/errors.py | 6 ++++
lib/pathutils.py | 1 +
test/ganeti.bdev_unittest.py | 51 ++++++++++++++++++++++++++++++++++-
test/ganeti.utils.io_unittest.py | 6 ++++
5 files changed, 118 insertions(+), 1 deletions(-)
diff --git a/lib/bdev.py b/lib/bdev.py
index d6c1a66..8273eb6 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -36,6 +36,7 @@ from ganeti import constants
from ganeti import objects
from ganeti import compat
from ganeti import netutils
+from ganeti import pathutils
# Size of reads in _CanReadDevice
@@ -88,6 +89,60 @@ def _CanReadDevice(path):
return False
+def _CheckFileStoragePath(path, allowed):
+ """Checks if a path is in a list of allowed paths for file storage.
+
+ @type path: string
+ @param path: Path to check
+ @type allowed: list
+ @param allowed: List of allowed paths
+ @raise errors.FileStoragePathError: If the path is not allowed
+
+ """
+ if not os.path.isabs(path):
+ raise errors.FileStoragePathError("File storage path must be absolute,"
+ " got '%s'" % path)
+
+ for i in allowed:
+ if not os.path.isabs(i):
+ logging.info("Ignoring relative path '%s' for file storage", i)
+ continue
+
+ if utils.IsBelowDir(i, path):
+ break
+ else:
+ raise errors.FileStoragePathError("Path '%s' is not acceptable for file"
+ " storage" % path)
+
+
+def LoadAllowedFileStoragePaths(filename):
+ """Loads file containing allowed file storage paths.
+
+ @rtype: list
+ @return: List of allowed paths (can be an empty list)
+
+ """
+ try:
+ contents = utils.ReadFile(filename)
+ except EnvironmentError:
+ return []
+ else:
+ return utils.FilterEmptyLinesAndComments(contents)
+
+
+def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
+ """Checks if a path is allowed for file storage.
+
+ @type path: string
+ @param path: Path to check
+ @raise errors.FileStoragePathError: If the path is not allowed
+
+ """
+ allowed = LoadAllowedFileStoragePaths(_filename)
+
+ return _CheckFileStoragePath(path, allowed)
+
+
class BlockDev(object):
"""Block device abstract class.
diff --git a/lib/errors.py b/lib/errors.py
index b9d578f..062b658 100644
--- a/lib/errors.py
+++ b/lib/errors.py
@@ -430,6 +430,12 @@ class RapiTestResult(GenericError):
"""
+class FileStoragePathError(GenericError):
+ """Error from file storage path validation.
+
+ """
+
+
# errors should be added above
diff --git a/lib/pathutils.py b/lib/pathutils.py
index e60a7a4..457b227 100644
--- a/lib/pathutils.py
+++ b/lib/pathutils.py
@@ -86,6 +86,7 @@ CONF_DIR = SYSCONFDIR + "/ganeti"
USER_SCRIPTS_DIR = CONF_DIR + "/scripts"
VNC_PASSWORD_FILE = CONF_DIR + "/vnc-cluster-password"
HOOKS_BASE_DIR = CONF_DIR + "/hooks"
+FILE_STORAGE_PATHS_FILE = CONF_DIR + "/file-storage-paths"
#: Lock file for watcher, locked in shared mode by watcher; lock in exclusive
# mode to block watcher (see L{cli._RunWhileClusterStoppedHelper.Call}
diff --git a/test/ganeti.bdev_unittest.py b/test/ganeti.bdev_unittest.py
index e1aeef6..e900719 100755
--- a/test/ganeti.bdev_unittest.py
+++ b/test/ganeti.bdev_unittest.py
@@ -28,6 +28,7 @@ import unittest
from ganeti import bdev
from ganeti import errors
from ganeti import constants
+from ganeti import utils
import testutils
@@ -372,5 +373,53 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase):
output_extra_matches, volume_name)
-if __name__ == '__main__':
+class TestCheckFileStoragePath(unittest.TestCase):
+ def testNonAbsolute(self):
+ for i in ["", "tmp", "foo/bar/baz"]:
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, i, ["/tmp"])
+
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, "/tmp", ["tmp", "xyz"])
+
+ def testNoAllowed(self):
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, "/tmp", [])
+
+ def testNoAdditionalPathComponent(self):
+ self.assertRaises(errors.FileStoragePathError,
+ bdev._CheckFileStoragePath, "/tmp/foo", ["/tmp/foo"])
+
+ def testAllowed(self):
+ bdev._CheckFileStoragePath("/tmp/foo/a", ["/tmp/foo"])
+ bdev._CheckFileStoragePath("/tmp/foo/a/x", ["/tmp/foo"])
+
+
+class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
+ def testDevNull(self):
+ self.assertEqual(bdev.LoadAllowedFileStoragePaths("/dev/null"), [])
+
+ def testNonExistantFile(self):
+ filename = "/tmp/this/file/does/not/exist"
+ assert not os.path.exists(filename)
+ self.assertEqual(bdev.LoadAllowedFileStoragePaths(filename), [])
+
+ def test(self):
+ tmpfile = self._CreateTempFile()
+
+ utils.WriteFile(tmpfile, data="""
+ # This is a test file
+ /tmp
+ /srv/storage
+ relative/path
+ """)
+
+ self.assertEqual(bdev.LoadAllowedFileStoragePaths(tmpfile), [
+ "/tmp",
+ "/srv/storage",
+ "relative/path",
+ ])
+
+
+if __name__ == "__main__":
testutils.GanetiTestProgram()
diff --git a/test/ganeti.utils.io_unittest.py b/test/ganeti.utils.io_unittest.py
index 109232a..bb9dc5e 100755
--- a/test/ganeti.utils.io_unittest.py
+++ b/test/ganeti.utils.io_unittest.py
@@ -646,6 +646,12 @@ class TestIsNormAbsPath(unittest.TestCase):
class TestIsBelowDir(unittest.TestCase):
"""Testing case for IsBelowDir"""
+ def testExactlyTheSame(self):
+ self.assertFalse(utils.IsBelowDir("/a/b", "/a/b"))
+ self.assertFalse(utils.IsBelowDir("/a/b", "/a/b/"))
+ self.assertFalse(utils.IsBelowDir("/a/b/", "/a/b"))
+ self.assertFalse(utils.IsBelowDir("/a/b/", "/a/b/"))
+
def testSamePrefix(self):
self.assertTrue(utils.IsBelowDir("/a/b", "/a/b/c"))
self.assertTrue(utils.IsBelowDir("/a/b/", "/a/b/e"))
--
1.7.7.3
Michael Hanselmann
Oct 4, 2012, 9:55:40 PM10/4/12
to ganeti...@googlegroups.com
This makes differences show up in “gnt-cluster verify”.
---
lib/cmdlib.py | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index b72276c..5e6d5e9 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -4292,12 +4292,12 @@ def _ComputeAncillaryFiles(cluster, redist):
pathutils.RAPI_USERS_FILE,
])
- if not redist:
- files_all.update(pathutils.ALL_CERT_FILES)
- files_all.update(ssconf.SimpleStore().GetFileList())
- else:
+ if redist:
# we need to ship at least the RAPI certificate
files_all.add(pathutils.RAPI_CERT_FILE)
+ else:
+ files_all.update(pathutils.ALL_CERT_FILES)
+ files_all.update(ssconf.SimpleStore().GetFileList())
if cluster.modify_etc_hosts:
files_all.add(constants.ETC_HOSTS)
@@ -4318,6 +4318,12 @@ def _ComputeAncillaryFiles(cluster, redist):
if not redist:
files_mc.add(pathutils.CLUSTER_CONF_FILE)
+ # File storage
+ if (not redist and
+ (constants.ENABLE_FILE_STORAGE or constants.ENABLE_SHARED_FILE_STORAGE)):
+ files_all.add(pathutils.FILE_STORAGE_PATHS_FILE)
+ files_opt.add(pathutils.FILE_STORAGE_PATHS_FILE)
+
# Files which should only be on VM-capable nodes
files_vm = set(
filename
@@ -4339,6 +4345,10 @@ def _ComputeAncillaryFiles(cluster, redist):
assert all_files_set.issuperset(files_opt), \
"Optional file not in a different required list"
+ # This one file should never ever be re-distributed via RPC
+ assert not (redist and
+ pathutils.FILE_STORAGE_PATHS_FILE in all_files_set)
+
return (files_all, files_opt, files_mc, files_vm)
--
1.7.7.3
---
lib/cmdlib.py | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index b72276c..5e6d5e9 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -4292,12 +4292,12 @@ def _ComputeAncillaryFiles(cluster, redist):
pathutils.RAPI_USERS_FILE,
])
- if not redist:
- files_all.update(pathutils.ALL_CERT_FILES)
- files_all.update(ssconf.SimpleStore().GetFileList())
- else:
+ if redist:
# we need to ship at least the RAPI certificate
files_all.add(pathutils.RAPI_CERT_FILE)
+ else:
+ files_all.update(pathutils.ALL_CERT_FILES)
+ files_all.update(ssconf.SimpleStore().GetFileList())
if cluster.modify_etc_hosts:
files_all.add(constants.ETC_HOSTS)
@@ -4318,6 +4318,12 @@ def _ComputeAncillaryFiles(cluster, redist):
if not redist:
files_mc.add(pathutils.CLUSTER_CONF_FILE)
+ # File storage
+ if (not redist and
+ (constants.ENABLE_FILE_STORAGE or constants.ENABLE_SHARED_FILE_STORAGE)):
+ files_all.add(pathutils.FILE_STORAGE_PATHS_FILE)
+ files_opt.add(pathutils.FILE_STORAGE_PATHS_FILE)
+
# Files which should only be on VM-capable nodes
files_vm = set(
filename
@@ -4339,6 +4345,10 @@ def _ComputeAncillaryFiles(cluster, redist):
assert all_files_set.issuperset(files_opt), \
"Optional file not in a different required list"
+ # This one file should never ever be re-distributed via RPC
+ assert not (redist and
+ pathutils.FILE_STORAGE_PATHS_FILE in all_files_set)
+
return (files_all, files_opt, files_mc, files_vm)
--
1.7.7.3
Iustin Pop
Oct 4, 2012, 10:00:01 PM10/4/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:37AM +0200, Michael Hanselmann wrote:
> If normal file storage was disabled but shared storage enabled,
> “_TransformFileStorageDir” would still throw an exception.
>
> in “opcodes._CheckStorageType” there's also a check, but I wasn't quite
> sure what the correct way of handling it was, so I added a TODO comment.
LGTM, thanks.
> If normal file storage was disabled but shared storage enabled,
> “_TransformFileStorageDir” would still throw an exception.
>
> in “opcodes._CheckStorageType” there's also a check, but I wasn't quite
> sure what the correct way of handling it was, so I added a TODO comment.
iustin
Iustin Pop
Oct 4, 2012, 10:02:42 PM10/4/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:38AM +0200, Michael Hanselmann wrote:
> The check for file consistency didn't properly handle virtual paths
> in case of a virtual cluster. This didn't cause any breakage as in
> a standard virtual cluster setup with only one node all files are
> visible for every node.
LGTM, thanks.
> The check for file consistency didn't properly handle virtual paths
> in case of a virtual cluster. This didn't cause any breakage as in
> a standard virtual cluster setup with only one node all files are
> visible for every node.
Do I understand correctly that what we're sending over RPC now are
virtualised paths? If so, do we have a check/list of what RPCs have
changed from file paths to the new virtual paths (all?)?
iustin
Iustin Pop
Oct 4, 2012, 10:05:37 PM10/4/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:39AM +0200, Michael Hanselmann wrote:
> - LoadAllowedFileStoragePaths: Loads a list of allowed file storage
> paths from a file
> - CheckFileStoragePath: Checks a path against the list of allowed paths
>
> The unit test for “utils.IsBelowDir” is updated with cases which weren't
> tested before.
LGTM, one comment:
> - LoadAllowedFileStoragePaths: Loads a list of allowed file storage
> paths from a file
> - CheckFileStoragePath: Checks a path against the list of allowed paths
>
> The unit test for “utils.IsBelowDir” is updated with cases which weren't
> tested before.
> +def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
> + """Checks if a path is allowed for file storage.
> +
> + @type path: string
> + @param path: Path to check
> + @raise errors.FileStoragePathError: If the path is not allowed
> +
> + """
> + allowed = LoadAllowedFileStoragePaths(_filename)
> +
> + return _CheckFileStoragePath(path, allowed)
doesn't have an explicit return value.
thanks,
iustin
Iustin Pop
Oct 4, 2012, 10:07:47 PM10/4/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:40AM +0200, Michael Hanselmann wrote:
> This makes differences show up in “gnt-cluster verify”.
> ---
> lib/cmdlib.py | 18 ++++++++++++++----
> 1 files changed, 14 insertions(+), 4 deletions(-)
LGTM, one comment:
> This makes differences show up in “gnt-cluster verify”.
> ---
> lib/cmdlib.py | 18 ++++++++++++++----
> 1 files changed, 14 insertions(+), 4 deletions(-)
> + # This one file should never ever be re-distributed via RPC
> + assert not (redist and
> + pathutils.FILE_STORAGE_PATHS_FILE in all_files_set)
_BuildUploadFileList(), more precisely.
thanks,
iustin
Iustin Pop
Oct 4, 2012, 10:11:52 PM10/4/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:41AM +0200, Michael Hanselmann wrote:
> Some paths, such as /bin or /usr/lib, should not be used for file
> storage. This patch implements a check during cluster verification
> to show a warning in case such a path has been used.
I haven't reviewed the patch, just one side question:
> Some paths, such as /bin or /usr/lib, should not be used for file
> storage. This patch implements a check during cluster verification
> to show a warning in case such a path has been used.
> + if what.get(constants.NV_FILE_STORAGE_PATHS) == my_name:
> + result[constants.NV_FILE_STORAGE_PATHS] = \
> + (pathutils.FILE_STORAGE_PATHS_FILE,
> + bdev.LoadAllowedFileStoragePaths(pathutils.FILE_STORAGE_PATHS_FILE))
know what paths exactly the node allows? I mean, as opposed to just
returning the messages about not recommended paths being present (as
text) from the node.
thanks,
iustin
Michael Hanselmann
Oct 4, 2012, 10:20:21 PM10/4/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/5 Iustin Pop <ius...@google.com>:
We don't, and I'm somewhat afraid there might be more. It isn't as
easy as changing the RPC encoding either as some are hidden in data
structures. File storage paths in logical IDs are still left
untouched. Do you have a better idea for what we could do?
I will not yet push this patch as I noticed /etc/hosts is special (not
virtualized). While virtual clusters might not want to modify
/etc/hosts in the first place, an exception shouldn't be thrown on
virtualizing paths if “--no-etc-hosts” wasn't specific while
initializing the cluster.
Michael
easy as changing the RPC encoding either as some are hidden in data
structures. File storage paths in logical IDs are still left
untouched. Do you have a better idea for what we could do?
I will not yet push this patch as I noticed /etc/hosts is special (not
virtualized). While virtual clusters might not want to modify
/etc/hosts in the first place, an exception shouldn't be thrown on
virtualizing paths if “--no-etc-hosts” wasn't specific while
initializing the cluster.
Michael
Iustin Pop
Oct 4, 2012, 10:24:33 PM10/4/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 04:20:21AM +0200, Michael Hanselmann wrote:
> 2012/10/5 Iustin Pop <ius...@google.com>:
> > On Fri, Oct 05, 2012 at 03:55:38AM +0200, Michael Hanselmann wrote:
> >> The check for file consistency didn't properly handle virtual paths
> >> in case of a virtual cluster. This didn't cause any breakage as in
> >> a standard virtual cluster setup with only one node all files are
> >> visible for every node.
> >
> > LGTM, thanks.
> >
> > Do I understand correctly that what we're sending over RPC now are
> > virtualised paths? If so, do we have a check/list of what RPCs have
> > changed from file paths to the new virtual paths (all?)?
>
> We don't, and I'm somewhat afraid there might be more. It isn't as
> easy as changing the RPC encoding either as some are hidden in data
> structures. File storage paths in logical IDs are still left
> untouched. Do you have a better idea for what we could do?
Nope, sorry. I was mainly asking because I need to keep this is mind,
> 2012/10/5 Iustin Pop <ius...@google.com>:
> > On Fri, Oct 05, 2012 at 03:55:38AM +0200, Michael Hanselmann wrote:
> >> The check for file consistency didn't properly handle virtual paths
> >> in case of a virtual cluster. This didn't cause any breakage as in
> >> a standard virtual cluster setup with only one node all files are
> >> visible for every node.
> >
> > LGTM, thanks.
> >
> > Do I understand correctly that what we're sending over RPC now are
> > virtualised paths? If so, do we have a check/list of what RPCs have
> > changed from file paths to the new virtual paths (all?)?
>
> We don't, and I'm somewhat afraid there might be more. It isn't as
> easy as changing the RPC encoding either as some are hidden in data
> structures. File storage paths in logical IDs are still left
> untouched. Do you have a better idea for what we could do?
and introduce a new type, when/if we add support for path-containing
RPCs in Haskell (at least there the separate type will solve the issue
of correctly virtualising all paths, but this doesn't help the Python
side).
> I will not yet push this patch as I noticed /etc/hosts is special (not
> virtualized). While virtual clusters might not want to modify
> /etc/hosts in the first place, an exception shouldn't be thrown on
> virtualizing paths if “--no-etc-hosts” wasn't specific while
> initializing the cluster.
thanks,
iustin
Michael Hanselmann
Oct 5, 2012, 12:22:25 PM10/5/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/5 Iustin Pop <ius...@google.com>:
“_CheckFileStoragePath(path, LoadAllowedFileStoragePaths(_filename))”.
Michael
> On Fri, Oct 05, 2012 at 03:55:39AM +0200, Michael Hanselmann wrote:
>> +def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
>> + allowed = LoadAllowedFileStoragePaths(_filename)
>> +
>> + return _CheckFileStoragePath(path, allowed)
>
> When reading the patch, this return was confusing, since _CheckFile…
> doesn't have an explicit return value.
That was indeed not what intended. Changed it to
>> +
>> + return _CheckFileStoragePath(path, allowed)
>
> When reading the patch, this return was confusing, since _CheckFile…
> doesn't have an explicit return value.
“_CheckFileStoragePath(path, LoadAllowedFileStoragePaths(_filename))”.
Michael
Michael Hanselmann
Oct 5, 2012, 12:25:49 PM10/5/12
to Iustin Pop, ganeti...@googlegroups.com
Michael Hanselmann
Oct 5, 2012, 12:31:05 PM10/5/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/5 Iustin Pop <ius...@google.com>:
I think you are too paranoid. If someone has access to this
information (by getting the contents of “server.pem”), that someone
can also read the contents of the whitelist file, which is the same on
all nodes, or the configuration, which has the file storage paths as
well.
Michael
information (by getting the contents of “server.pem”), that someone
can also read the contents of the whitelist file, which is the same on
all nodes, or the configuration, which has the file storage paths as
well.
Michael
Iustin Pop
Oct 5, 2012, 2:26:17 PM10/5/12
to Michael Hanselmann, Iustin Pop, ganeti...@googlegroups.com
argument: just because I can read local server.pem, it doesn't
necessarily follow that I can read remote filepaths.
iustin
Iustin Pop
Oct 5, 2012, 6:00:11 PM10/5/12
to Michael Hanselmann, ganeti...@googlegroups.com
iustin
Michael Hanselmann
Oct 8, 2012, 2:38:39 AM10/8/12
to Michael Hanselmann, Iustin Pop, ganeti...@googlegroups.com
2012/10/5 Iustin Pop <iu...@k1024.org>:
You can not read the remote paths, but since the file is required to
be equal on all nodes (or else cluster-verify complains every time),
you may as well read the local version.
Michael
be equal on all nodes (or else cluster-verify complains every time),
you may as well read the local version.
Michael
Iustin Pop
Oct 16, 2012, 8:08:13 AM10/16/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:44AM +0200, Michael Hanselmann wrote:
> Mention that the file is something new and should be written by
> cfgupgrade.
LGTM, thanks.
> Mention that the file is something new and should be written by
> cfgupgrade.
iustin
Iustin Pop
Oct 16, 2012, 8:10:10 AM10/16/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:43AM +0200, Michael Hanselmann wrote:
> When file storage is used this file is now mandatory.
Not convinced about the 0644 mode for the file, but LGTM.
> When file storage is used this file is now mandatory.
iustin
Iustin Pop
Oct 16, 2012, 8:19:24 AM10/16/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:41AM +0200, Michael Hanselmann wrote:
all paths in there.
I'm wondering whether backend itself shouldn't filter out "deemed
unsafe" paths, rather than just cluster verify. The return tuple out of
the node verify rpc could be (good paths, bad paths) instead of just the
paths, but otherwise the verification wouldn't be much different.
iustin
Michael Hanselmann
Oct 16, 2012, 11:36:40 AM10/16/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/16 Iustin Pop <ius...@google.com>:
ensure-dirs doesn't touch files in /etc. It seemed a safe choice. Do
you agree?
Michael
> On Fri, Oct 05, 2012 at 03:55:43AM +0200, Michael Hanselmann wrote:
>> When file storage is used this file is now mandatory.
>
> Not convinced about the 0644 mode for the file, but LGTM.
I chose it as cfgupgrade has no concept of owners/groups, and
>> When file storage is used this file is now mandatory.
>
> Not convinced about the 0644 mode for the file, but LGTM.
ensure-dirs doesn't touch files in /etc. It seemed a safe choice. Do
you agree?
Michael
Michael Hanselmann
Oct 16, 2012, 11:37:59 AM10/16/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/16 Iustin Pop <ius...@google.com>:
if some unwanted paths were listed, but to still allow them being
used.
> the node verify rpc could be (good paths, bad paths) instead of just the
> paths, but otherwise the verification wouldn't be much different.
Michael
> I'm wondering whether backend itself shouldn't filter out "deemed
> unsafe" paths, rather than just cluster verify. The return tuple out of
Do you mean even for actual file operations? I thought we'd just warn
> unsafe" paths, rather than just cluster verify. The return tuple out of
if some unwanted paths were listed, but to still allow them being
used.
> the node verify rpc could be (good paths, bad paths) instead of just the
> paths, but otherwise the verification wouldn't be much different.
Iustin Pop
Oct 16, 2012, 11:41:22 AM10/16/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Tue, Oct 16, 2012 at 05:37:59PM +0200, Michael Hanselmann wrote:
> 2012/10/16 Iustin Pop <ius...@google.com>:
> > I'm wondering whether backend itself shouldn't filter out "deemed
> > unsafe" paths, rather than just cluster verify. The return tuple out of
>
> Do you mean even for actual file operations? I thought we'd just warn
> if some unwanted paths were listed, but to still allow them being
> used.
For the paths that we have listed, I think it would be better to err on
> 2012/10/16 Iustin Pop <ius...@google.com>:
> > I'm wondering whether backend itself shouldn't filter out "deemed
> > unsafe" paths, rather than just cluster verify. The return tuple out of
>
> Do you mean even for actual file operations? I thought we'd just warn
> if some unwanted paths were listed, but to still allow them being
> used.
the side of safety, even to the point of adding duplicate checks, as I
see no point in anyone hosting VMs under /bin and its friends or
similarly /lib or /etc.
thanks,
iustin
Iustin Pop
Oct 16, 2012, 11:42:24 AM10/16/12
to Michael Hanselmann, ganeti...@googlegroups.com
it, but I personally would have gone for a 0600.
Anyway, as I said, LGTM.
iustin
Michael Hanselmann
Oct 17, 2012, 6:12:26 AM10/17/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/16 Iustin Pop <ius...@google.com>:
> On Tue, Oct 16, 2012 at 05:36:40PM +0200, Michael Hanselmann wrote:
>> 2012/10/16 Iustin Pop <ius...@google.com>:
>> > On Fri, Oct 05, 2012 at 03:55:43AM +0200, Michael Hanselmann wrote:
>> >> When file storage is used this file is now mandatory.
>> >
>> > Not convinced about the 0644 mode for the file, but LGTM.
>>
>> I chose it as cfgupgrade has no concept of owners/groups, and
>> ensure-dirs doesn't touch files in /etc. It seemed a safe choice. Do
>> you agree?
>
> It's less about owners/groups, but rather wrr versus w--. I'm OK with
> it, but I personally would have gone for a 0600.
I just realized this file is only read by the node daemon, not by the
> On Tue, Oct 16, 2012 at 05:36:40PM +0200, Michael Hanselmann wrote:
>> 2012/10/16 Iustin Pop <ius...@google.com>:
>> > On Fri, Oct 05, 2012 at 03:55:43AM +0200, Michael Hanselmann wrote:
>> >> When file storage is used this file is now mandatory.
>> >
>> > Not convinced about the 0644 mode for the file, but LGTM.
>>
>> I chose it as cfgupgrade has no concept of owners/groups, and
>> ensure-dirs doesn't touch files in /etc. It seemed a safe choice. Do
>> you agree?
>
> It's less about owners/groups, but rather wrr versus w--. I'm OK with
> it, but I personally would have gone for a 0600.
master daemon. I'll change it to 0600. LGTY?
Michael
Iustin Pop
Oct 17, 2012, 7:09:55 AM10/17/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Wed, Oct 17, 2012 at 12:12:26PM +0200, Michael Hanselmann wrote:
> 2012/10/16 Iustin Pop <ius...@google.com>:
> > On Tue, Oct 16, 2012 at 05:36:40PM +0200, Michael Hanselmann wrote:
> >> 2012/10/16 Iustin Pop <ius...@google.com>:
> >> > On Fri, Oct 05, 2012 at 03:55:43AM +0200, Michael Hanselmann wrote:
> >> >> When file storage is used this file is now mandatory.
> >> >
> >> > Not convinced about the 0644 mode for the file, but LGTM.
> >>
> >> I chose it as cfgupgrade has no concept of owners/groups, and
> >> ensure-dirs doesn't touch files in /etc. It seemed a safe choice. Do
> >> you agree?
> >
> > It's less about owners/groups, but rather wrr versus w--. I'm OK with
> > it, but I personally would have gone for a 0600.
>
> I just realized this file is only read by the node daemon, not by the
> master daemon. I'll change it to 0600. LGTY?
Yes, of course, that's what I meant :)
> 2012/10/16 Iustin Pop <ius...@google.com>:
> > On Tue, Oct 16, 2012 at 05:36:40PM +0200, Michael Hanselmann wrote:
> >> 2012/10/16 Iustin Pop <ius...@google.com>:
> >> > On Fri, Oct 05, 2012 at 03:55:43AM +0200, Michael Hanselmann wrote:
> >> >> When file storage is used this file is now mandatory.
> >> >
> >> > Not convinced about the 0644 mode for the file, but LGTM.
> >>
> >> I chose it as cfgupgrade has no concept of owners/groups, and
> >> ensure-dirs doesn't touch files in /etc. It seemed a safe choice. Do
> >> you agree?
> >
> > It's less about owners/groups, but rather wrr versus w--. I'm OK with
> > it, but I personally would have gone for a 0600.
>
> I just realized this file is only read by the node daemon, not by the
> master daemon. I'll change it to 0600. LGTY?
iustin
Michael Hanselmann
Oct 17, 2012, 10:17:50 AM10/17/12
to ganeti...@googlegroups.com
Some paths, such as /bin or /usr/lib, should not be used for file
storage. This patch implements a check during cluster verification to
fail in case such a path has been used.
storage. This patch implements a check during cluster verification to
---
lib/backend.py | 4 ++++
lib/cmdlib.py | 29 +++++++++++++++++++++++++++++
lib/constants.py | 4 ++++
3 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index 4c537b5..2e8ec7a 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -800,6 +800,10 @@ def VerifyNode(what, cluster_name):
result[constants.NV_BRIDGES] = [bridge
for bridge in what[constants.NV_BRIDGES]
if not utils.BridgeExists(bridge)]
+
+ if what.get(constants.NV_FILE_STORAGE_PATHS) == my_name:
+ result[constants.NV_FILE_STORAGE_PATHS] = bdev.VerifyFileStoragePaths()
for bridge in what[constants.NV_BRIDGES]
if not utils.BridgeExists(bridge)]
+
+ if what.get(constants.NV_FILE_STORAGE_PATHS) == my_name:
+
return result
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index ba13cac..9a0930e 100644
return result
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -2784,6 +2784,29 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
"OSes present on reference node %s but missing on this node: %s",
base.name, utils.CommaJoin(missing))
+ def _VerifyFileStoragePaths(self, ninfo, nresult, is_master):
+ """Verifies paths in L{pathutils.FILE_STORAGE_PATHS_FILE}.
+
+ @type ninfo: L{objects.Node}
+ @param ninfo: the node to check
+ @param nresult: the remote results for the node
+ @type is_master: bool
+ @param is_master: Whether node is the master node
+
+ """
+ node = ninfo.name
+
+ if (is_master and
+ (constants.ENABLE_FILE_STORAGE or
+ constants.ENABLE_SHARED_FILE_STORAGE)):
+ fspaths = nresult[constants.NV_FILE_STORAGE_PATHS]
+
+ self._ErrorIf(fspaths, constants.CV_ENODEFILESTORAGEPATHS, node,
+ "Found forbidden file storage paths: %s",
+ utils.CommaJoin(fspaths))
+ else:
+ assert constants.NV_FILE_STORAGE_PATHS not in nresult
+
def _VerifyOob(self, ninfo, nresult):
"""Verifies out of band functionality of a node.
@@ -3126,6 +3149,10 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
node_verify_param[constants.NV_DRBDLIST] = None
node_verify_param[constants.NV_DRBDHELPER] = drbd_helper
+ if constants.ENABLE_FILE_STORAGE or constants.ENABLE_SHARED_FILE_STORAGE:
+ # Load file storage paths only from master node
+ node_verify_param[constants.NV_FILE_STORAGE_PATHS] = master_node
+
# bridge checks
# FIXME: this needs to be changed per node-group, not cluster-wide
bridges = set()
@@ -3279,6 +3306,8 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
self._VerifyNodeNetwork(node_i, nresult)
self._VerifyNodeUserScripts(node_i, nresult)
self._VerifyOob(node_i, nresult)
+ self._VerifyFileStoragePaths(node_i, nresult,
+ node == master_node)
if nimg.vm_capable:
self._VerifyNodeLVM(node_i, nresult, vg_name)
diff --git a/lib/constants.py b/lib/constants.py
index 0342ddf..06dc5cd 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -1330,6 +1330,8 @@ CV_ENODEOOBPATH = \
(CV_TNODE, "ENODEOOBPATH", "Invalid Out Of Band path")
CV_ENODEUSERSCRIPTS = \
(CV_TNODE, "ENODEUSERSCRIPTS", "User scripts not present or not executable")
+CV_ENODEFILESTORAGEPATHS = \
+ (CV_TNODE, "ENODEFILESTORAGEPATHS", "Detected bad file storage paths")
CV_ALL_ECODES = frozenset([
CV_ECLUSTERCFG,
@@ -1363,6 +1365,7 @@ CV_ALL_ECODES = frozenset([
CV_ENODETIME,
CV_ENODEOOBPATH,
CV_ENODEUSERSCRIPTS,
+ CV_ENODEFILESTORAGEPATHS,
])
CV_ALL_ECODES_STRINGS = frozenset(estr for (_, estr, _) in CV_ALL_ECODES)
@@ -1389,6 +1392,7 @@ NV_VMNODES = "vmnodes"
NV_OOB_PATHS = "oob-paths"
NV_BRIDGES = "bridges"
NV_USERSCRIPTS = "user-scripts"
+NV_FILE_STORAGE_PATHS = "file-storage-paths"
# Instance status
INSTST_RUNNING = "running"
--
1.7.7.3
Michael Hanselmann
Oct 17, 2012, 10:18:00 AM10/17/12
to ganeti...@googlegroups.com
An earlier version of this patch series verified all paths in cmdlib in
the master daemon. With this change all that verification code is moved
to bdev to run inside the node daemon. The checks are much stricter
now--it is no longer possible to use forbidden paths (e.g. /bin) to
manipulate file storage devices (once these checks are being used).
---
lib/bdev.py | 61 +++++++++++++++++++++++++++++++-
test/ganeti.bdev_unittest.py | 79 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 134 insertions(+), 6 deletions(-)
diff --git a/lib/bdev.py b/lib/bdev.py
index 7f34699..46d1687 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -89,6 +89,57 @@ def _CanReadDevice(path):
return False
+def _GetForbiddenFileStoragePaths():
+ """Builds a list of path prefixes which shouldn't be used for file storage.
+
+ @rtype: frozenset
+
+def _VerifyFileStoragePaths(paths, _forbidden=_GetForbiddenFileStoragePaths()):
+ """Cross-checks a list of paths for prefixes considered bad.
+
+ Some paths, e.g. "/bin", should not be used for file storage.
+
+ @type paths: list
+ @param paths: List of paths to be checked
+ @rtype: list
+ @return: Sorted list of paths for which the user should be warned
+
+ """
+ def _Check(path):
+ return (not os.path.isabs(path) or
+ path in _forbidden or
+ filter(lambda p: utils.IsBelowDir(p, path), _forbidden))
+
+ return utils.NiceSort(filter(_Check, map(os.path.normpath, paths)))
+
+
+def VerifyFileStoragePaths(_filename=pathutils.FILE_STORAGE_PATHS_FILE):
+ """Returns a list of file storage paths whose prefix is considered bad.
+
+ See L{_VerifyFileStoragePaths}.
+
+ """
+ return _VerifyFileStoragePaths(_LoadAllowedFileStoragePaths(_filename))
+
+
def _CheckFileStoragePath(path, allowed):
" storage" % path)
-def LoadAllowedFileStoragePaths(filename):
+def _LoadAllowedFileStoragePaths(filename):
@@ -138,7 +189,13 @@ def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
+ allowed = _LoadAllowedFileStoragePaths(_filename)
+
+ if _VerifyFileStoragePaths([path]):
+ raise errors.FileStoragePathError("Path '%s' uses a forbidden prefix" %
+ path)
+
+ _CheckFileStoragePath(path, allowed)
class BlockDev(object):
diff --git a/test/ganeti.bdev_unittest.py b/test/ganeti.bdev_unittest.py
index e900719..7eb25aa 100755
--- a/test/ganeti.bdev_unittest.py
+++ b/test/ganeti.bdev_unittest.py
@@ -373,7 +373,48 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase):
output_extra_matches, volume_name)
-class TestCheckFileStoragePath(unittest.TestCase):
+class TestVerifyFileStoragePathsInternal(unittest.TestCase):
+ def testPaths(self):
+ paths = bdev._GetForbiddenFileStoragePaths()
+
+ for path in ["/bin", "/usr/local/sbin", "/lib64", "/etc", "/sys"]:
+ self.assertTrue(path in paths)
+
+ self.assertEqual(set(map(os.path.normpath, paths)), paths)
+
+ def test(self):
+ vfsp = bdev._VerifyFileStoragePaths
+ self.assertEqual(vfsp([]), [])
+ self.assertEqual(vfsp(["/tmp"]), [])
+ self.assertEqual(vfsp(["/bin/ls"]), ["/bin/ls"])
+ self.assertEqual(vfsp(["/bin"]), ["/bin"])
+ self.assertEqual(vfsp(["/usr/sbin/vim", "/srv/file-storage"]),
+ ["/usr/sbin/vim"])
+
+
+class TestVerifyFileStoragePaths(testutils.GanetiTestCase):
+ x/y///z/relative
+ /bin
+ /usr/local/lib32/
+ "/bin",
+ "/usr/local/lib32",
+ "relative/path",
+ "x/y/z/relative",
+ ])
+
+
+class TestCheckFileStoragePathInternal(unittest.TestCase):
def testNonAbsolute(self):
@@ -395,14 +436,44 @@ class TestCheckFileStoragePath(unittest.TestCase):
+ bdev.CheckFileStoragePath, "/bin/", _filename=filename)
+ self.assertRaises(errors.FileStoragePathError,
+ bdev.CheckFileStoragePath, "/srv/file-storage",
+ _filename=filename)
+
+ def testAllowedPath(self):
+ """)
+
+ bdev.CheckFileStoragePath("/srv/storage/inst1", _filename=tmpfile)
+
+ # No additional path component
+ self.assertRaises(errors.FileStoragePathError,
+ bdev.CheckFileStoragePath, "/srv/storage",
+ _filename=tmpfile)
+
+ # Forbidden path
+ self.assertRaises(errors.FileStoragePathError,
+ bdev.CheckFileStoragePath, "/usr/lib64/xyz",
+ _filename=tmpfile)
+
+
class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
def testDevNull(self):
- self.assertEqual(bdev.LoadAllowedFileStoragePaths("/dev/null"), [])
+ self.assertEqual(bdev._LoadAllowedFileStoragePaths("/dev/null"), [])
def testNonExistantFile(self):
filename = "/tmp/this/file/does/not/exist"
assert not os.path.exists(filename)
- self.assertEqual(bdev.LoadAllowedFileStoragePaths(filename), [])
+ self.assertEqual(bdev._LoadAllowedFileStoragePaths(filename), [])
def test(self):
tmpfile = self._CreateTempFile()
@@ -414,7 +485,7 @@ class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
relative/path
""")
- self.assertEqual(bdev.LoadAllowedFileStoragePaths(tmpfile), [
+ self.assertEqual(bdev._LoadAllowedFileStoragePaths(tmpfile), [
"/tmp",
"/srv/storage",
"relative/path",
--
1.7.7.3
the master daemon. With this change all that verification code is moved
to bdev to run inside the node daemon. The checks are much stricter
now--it is no longer possible to use forbidden paths (e.g. /bin) to
manipulate file storage devices (once these checks are being used).
---
lib/bdev.py | 61 +++++++++++++++++++++++++++++++-
test/ganeti.bdev_unittest.py | 79 +++++++++++++++++++++++++++++++++++++++--
2 files changed, 134 insertions(+), 6 deletions(-)
diff --git a/lib/bdev.py b/lib/bdev.py
index 7f34699..46d1687 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -89,6 +89,57 @@ def _CanReadDevice(path):
return False
+def _GetForbiddenFileStoragePaths():
+ """Builds a list of path prefixes which shouldn't be used for file storage.
+
+ @rtype: frozenset
+
+ """
+ paths = set([
+ "/boot",
+ "/dev",
+ "/etc",
+ "/home",
+ "/proc",
+ "/root",
+ "/sys",
+ ])
+
+ for prefix in ["", "/usr", "/usr/local"]:
+ paths.update(map(lambda s: "%s/%s" % (prefix, s),
+ ["bin", "lib", "lib32", "lib64", "sbin"]))
+
+ return frozenset(map(os.path.normpath, paths))
+
+ """
+ paths = set([
+ "/boot",
+ "/dev",
+ "/etc",
+ "/home",
+ "/proc",
+ "/root",
+ "/sys",
+ ])
+
+ for prefix in ["", "/usr", "/usr/local"]:
+ paths.update(map(lambda s: "%s/%s" % (prefix, s),
+ ["bin", "lib", "lib32", "lib64", "sbin"]))
+
+ return frozenset(map(os.path.normpath, paths))
+
+def _VerifyFileStoragePaths(paths, _forbidden=_GetForbiddenFileStoragePaths()):
+ """Cross-checks a list of paths for prefixes considered bad.
+
+ Some paths, e.g. "/bin", should not be used for file storage.
+
+ @type paths: list
+ @param paths: List of paths to be checked
+ @rtype: list
+ @return: Sorted list of paths for which the user should be warned
+
+ """
+ def _Check(path):
+ return (not os.path.isabs(path) or
+ path in _forbidden or
+ filter(lambda p: utils.IsBelowDir(p, path), _forbidden))
+
+ return utils.NiceSort(filter(_Check, map(os.path.normpath, paths)))
+
+
+def VerifyFileStoragePaths(_filename=pathutils.FILE_STORAGE_PATHS_FILE):
+ """Returns a list of file storage paths whose prefix is considered bad.
+
+ See L{_VerifyFileStoragePaths}.
+
+ """
+ return _VerifyFileStoragePaths(_LoadAllowedFileStoragePaths(_filename))
+
+
def _CheckFileStoragePath(path, allowed):
"""Checks if a path is in a list of allowed paths for file storage.
@@ -115,7 +166,7 @@ def _CheckFileStoragePath(path, allowed):
" storage" % path)
-def LoadAllowedFileStoragePaths(filename):
+def _LoadAllowedFileStoragePaths(filename):
"""Loads file containing allowed file storage paths.
@rtype: list
@@ -138,7 +189,13 @@ def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
@raise errors.FileStoragePathError: If the path is not allowed
"""
- _CheckFileStoragePath(path, LoadAllowedFileStoragePaths(_filename))
"""
+ allowed = _LoadAllowedFileStoragePaths(_filename)
+
+ if _VerifyFileStoragePaths([path]):
+ raise errors.FileStoragePathError("Path '%s' uses a forbidden prefix" %
+ path)
+
+ _CheckFileStoragePath(path, allowed)
class BlockDev(object):
diff --git a/test/ganeti.bdev_unittest.py b/test/ganeti.bdev_unittest.py
index e900719..7eb25aa 100755
--- a/test/ganeti.bdev_unittest.py
+++ b/test/ganeti.bdev_unittest.py
@@ -373,7 +373,48 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase):
output_extra_matches, volume_name)
-class TestCheckFileStoragePath(unittest.TestCase):
+class TestVerifyFileStoragePathsInternal(unittest.TestCase):
+ def testPaths(self):
+ paths = bdev._GetForbiddenFileStoragePaths()
+
+ for path in ["/bin", "/usr/local/sbin", "/lib64", "/etc", "/sys"]:
+ self.assertTrue(path in paths)
+
+ self.assertEqual(set(map(os.path.normpath, paths)), paths)
+
+ def test(self):
+ vfsp = bdev._VerifyFileStoragePaths
+ self.assertEqual(vfsp([]), [])
+ self.assertEqual(vfsp(["/tmp"]), [])
+ self.assertEqual(vfsp(["/bin/ls"]), ["/bin/ls"])
+ self.assertEqual(vfsp(["/bin"]), ["/bin"])
+ self.assertEqual(vfsp(["/usr/sbin/vim", "/srv/file-storage"]),
+ ["/usr/sbin/vim"])
+
+
+class TestVerifyFileStoragePaths(testutils.GanetiTestCase):
+ def test(self):
+ tmpfile = self._CreateTempFile()
+
+ utils.WriteFile(tmpfile, data="""
+ /tmp
+ tmpfile = self._CreateTempFile()
+
+ utils.WriteFile(tmpfile, data="""
+ x/y///z/relative
+ # This is a test file
+ /srv/storage
+ /bin
+ /usr/local/lib32/
+ relative/path
+ """)
+
+ self.assertEqual(bdev.VerifyFileStoragePaths(_filename=tmpfile), [
+ """)
+
+ "/bin",
+ "/usr/local/lib32",
+ "relative/path",
+ "x/y/z/relative",
+ ])
+
+
+class TestCheckFileStoragePathInternal(unittest.TestCase):
def testNonAbsolute(self):
for i in ["", "tmp", "foo/bar/baz"]:
self.assertRaises(errors.FileStoragePathError,
@@ -395,14 +436,44 @@ class TestCheckFileStoragePath(unittest.TestCase):
bdev._CheckFileStoragePath("/tmp/foo/a/x", ["/tmp/foo"])
+class TestCheckFileStoragePath(testutils.GanetiTestCase):
+ def testNonExistantFile(self):
+ filename = "/tmp/this/file/does/not/exist"
+ assert not os.path.exists(filename)
+ self.assertRaises(errors.FileStoragePathError,
+ filename = "/tmp/this/file/does/not/exist"
+ assert not os.path.exists(filename)
+ bdev.CheckFileStoragePath, "/bin/", _filename=filename)
+ self.assertRaises(errors.FileStoragePathError,
+ bdev.CheckFileStoragePath, "/srv/file-storage",
+ _filename=filename)
+
+ def testAllowedPath(self):
+ tmpfile = self._CreateTempFile()
+
+ utils.WriteFile(tmpfile, data="""
+ /srv/storage
+
+ utils.WriteFile(tmpfile, data="""
+ """)
+
+ bdev.CheckFileStoragePath("/srv/storage/inst1", _filename=tmpfile)
+
+ # No additional path component
+ self.assertRaises(errors.FileStoragePathError,
+ bdev.CheckFileStoragePath, "/srv/storage",
+ _filename=tmpfile)
+
+ # Forbidden path
+ self.assertRaises(errors.FileStoragePathError,
+ bdev.CheckFileStoragePath, "/usr/lib64/xyz",
+ _filename=tmpfile)
+
+
class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
def testDevNull(self):
- self.assertEqual(bdev.LoadAllowedFileStoragePaths("/dev/null"), [])
+ self.assertEqual(bdev._LoadAllowedFileStoragePaths("/dev/null"), [])
def testNonExistantFile(self):
filename = "/tmp/this/file/does/not/exist"
assert not os.path.exists(filename)
- self.assertEqual(bdev.LoadAllowedFileStoragePaths(filename), [])
+ self.assertEqual(bdev._LoadAllowedFileStoragePaths(filename), [])
def test(self):
tmpfile = self._CreateTempFile()
@@ -414,7 +485,7 @@ class TestLoadAllowedFileStoragePaths(testutils.GanetiTestCase):
relative/path
""")
- self.assertEqual(bdev.LoadAllowedFileStoragePaths(tmpfile), [
+ self.assertEqual(bdev._LoadAllowedFileStoragePaths(tmpfile), [
"/tmp",
"/srv/storage",
"relative/path",
--
1.7.7.3
Iustin Pop
Oct 23, 2012, 7:55:20 AM10/23/12
to Michael Hanselmann, ganeti...@googlegroups.com
iustin
Iustin Pop
Oct 23, 2012, 8:00:02 AM10/23/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Wed, Oct 17, 2012 at 04:18:00PM +0200, Michael Hanselmann wrote:
> An earlier version of this patch series verified all paths in cmdlib in
> the master daemon. With this change all that verification code is moved
> to bdev to run inside the node daemon. The checks are much stricter
> now--it is no longer possible to use forbidden paths (e.g. /bin) to
> manipulate file storage devices (once these checks are being used).
> An earlier version of this patch series verified all paths in cmdlib in
> the master daemon. With this change all that verification code is moved
> to bdev to run inside the node daemon. The checks are much stricter
> now--it is no longer possible to use forbidden paths (e.g. /bin) to
> manipulate file storage devices (once these checks are being used).
> +def _VerifyFileStoragePaths(paths, _forbidden=_GetForbiddenFileStoragePaths()):
> + """Cross-checks a list of paths for prefixes considered bad.
> +
> + Some paths, e.g. "/bin", should not be used for file storage.
> +
> + @type paths: list
> + @param paths: List of paths to be checked
> + @rtype: list
> + @return: Sorted list of paths for which the user should be warned
> +
> + """
> + def _Check(path):
> + return (not os.path.isabs(path) or
> + path in _forbidden or
> + filter(lambda p: utils.IsBelowDir(p, path), _forbidden))
This filter is not very clear here, because you're using it in the
> + """Cross-checks a list of paths for prefixes considered bad.
> +
> + Some paths, e.g. "/bin", should not be used for file storage.
> +
> + @type paths: list
> + @param paths: List of paths to be checked
> + @rtype: list
> + @return: Sorted list of paths for which the user should be warned
> +
> + """
> + def _Check(path):
> + return (not os.path.isabs(path) or
> + path in _forbidden or
> + filter(lambda p: utils.IsBelowDir(p, path), _forbidden))
bool(list) context (i.e. not empty list), rather than for the results. I
was tempted to ask to replace this with a compat.any, but that function
doesn't take a predicate (bad Python!), so LGTM as is.
thanks,
iustin
Iustin Pop
Oct 23, 2012, 8:02:24 AM10/23/12
to Michael Hanselmann, ganeti...@googlegroups.com
cmdlib are named exactly the same.
Could we name the ones in bdev ComputeWrongFileStoragePaths? Because
this is what they do, instead of verifying them.
thanks,
iustin
Iustin Pop
Oct 23, 2012, 8:04:43 AM10/23/12
to Michael Hanselmann, ganeti...@googlegroups.com
check.
> +
> + self._ErrorIf(fspaths, constants.CV_ENODEFILESTORAGEPATHS, node,
> + "Found forbidden file storage paths: %s",
> + utils.CommaJoin(fspaths))
> + else:
> + assert constants.NV_FILE_STORAGE_PATHS not in nresult
entire LU), rather than an _ErrorIf.
Rest LGTM.
Iustin Pop
Oct 23, 2012, 8:05:35 AM10/23/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Fri, Oct 05, 2012 at 03:55:42AM +0200, Michael Hanselmann wrote:
> The configuration is no longer used for verifying file storage paths.
LGTM, thanks.
iustin
> The configuration is no longer used for verifying file storage paths.
LGTM, thanks.
iustin
Michael Hanselmann
Oct 23, 2012, 9:50:34 AM10/23/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/23 Iustin Pop <ius...@google.com>:
> Could we name the ones in bdev ComputeWrongFileStoragePaths? Because
> this is what they do, instead of verifying them.
Done, sending full patch again.
> this is what they do, instead of verifying them.
Michael Hanselmann
Oct 23, 2012, 9:54:32 AM10/23/12
to ganeti...@googlegroups.com
An earlier version of this patch series verified all paths in cmdlib in
the master daemon. With this change all that verification code is moved
to bdev to run inside the node daemon. The checks are much stricter
now--it is no longer possible to use forbidden paths (e.g. /bin) to
manipulate file storage devices (once these checks are being used).
---
the master daemon. With this change all that verification code is moved
to bdev to run inside the node daemon. The checks are much stricter
now--it is no longer possible to use forbidden paths (e.g. /bin) to
manipulate file storage devices (once these checks are being used).
lib/backend.py | 1 +
lib/bdev.py | 63 ++++++++++++++++++++++++++++++++-
test/ganeti.bdev_unittest.py | 79 +++++++++++++++++++++++++++++++++++++++--
3 files changed, 137 insertions(+), 6 deletions(-)
diff --git a/lib/backend.py b/lib/backend.py
index 032547d..6da70d6 100644
--- a/lib/backend.py
+++ b/lib/backend.py
@@ -800,6 +800,7 @@ def VerifyNode(what, cluster_name):
result[constants.NV_BRIDGES] = [bridge
for bridge in what[constants.NV_BRIDGES]
if not utils.BridgeExists(bridge)]
+
return result
for bridge in what[constants.NV_BRIDGES]
if not utils.BridgeExists(bridge)]
+
diff --git a/lib/bdev.py b/lib/bdev.py
index 8d41f53..487298d 100644
--- a/lib/bdev.py
+++ b/lib/bdev.py
@@ -29,6 +29,7 @@ import stat
import pyparsing as pyp
import os
import logging
+import itertools
from ganeti import utils
from ganeti import errors
@@ -100,6 +101,58 @@ def _CanReadDevice(path):
return False
+def _GetForbiddenFileStoragePaths():
+ """Builds a list of path prefixes which shouldn't be used for file storage.
+
+ @rtype: frozenset
+
+ """
+ paths = set([
+ "/boot",
+ "/dev",
+ "/etc",
+ "/home",
+ "/proc",
+ "/root",
+ "/sys",
+ ])
+
+ for prefix in ["", "/usr", "/usr/local"]:
+ paths.update(map(lambda s: "%s/%s" % (prefix, s),
+ ["bin", "lib", "lib32", "lib64", "sbin"]))
+
+ return frozenset(map(os.path.normpath, paths))
+
+
+def _ComputeWrongFileStoragePaths(paths,
+def _GetForbiddenFileStoragePaths():
+ """Builds a list of path prefixes which shouldn't be used for file storage.
+
+ @rtype: frozenset
+
+ """
+ paths = set([
+ "/boot",
+ "/dev",
+ "/etc",
+ "/home",
+ "/proc",
+ "/root",
+ "/sys",
+ ])
+
+ for prefix in ["", "/usr", "/usr/local"]:
+ paths.update(map(lambda s: "%s/%s" % (prefix, s),
+ ["bin", "lib", "lib32", "lib64", "sbin"]))
+
+ return frozenset(map(os.path.normpath, paths))
+
+
+ _forbidden=_GetForbiddenFileStoragePaths()):
+ """Cross-checks a list of paths for prefixes considered bad.
+
+ Some paths, e.g. "/bin", should not be used for file storage.
+
+ @type paths: list
+ @param paths: List of paths to be checked
+ @rtype: list
+ @return: Sorted list of paths for which the user should be warned
+
+ """
+ def _Check(path):
+ return (not os.path.isabs(path) or
+ path in _forbidden or
+ filter(lambda p: utils.IsBelowDir(p, path), _forbidden))
+
+ Some paths, e.g. "/bin", should not be used for file storage.
+
+ @type paths: list
+ @param paths: List of paths to be checked
+ @rtype: list
+ @return: Sorted list of paths for which the user should be warned
+
+ """
+ def _Check(path):
+ return (not os.path.isabs(path) or
+ path in _forbidden or
+ filter(lambda p: utils.IsBelowDir(p, path), _forbidden))
+
+ return utils.NiceSort(filter(_Check, map(os.path.normpath, paths)))
+
+
+def ComputeWrongFileStoragePaths(_filename=pathutils.FILE_STORAGE_PATHS_FILE):
+ return utils.NiceSort(filter(_Check, map(os.path.normpath, paths)))
+
+
+ """Returns a list of file storage paths whose prefix is considered bad.
+
+ See L{_VerifyFileStoragePaths}.
+
+ """
+ return _ComputeWrongFileStoragePaths(_LoadAllowedFileStoragePaths(_filename))
+
+ See L{_VerifyFileStoragePaths}.
+
+ """
+
+
def _CheckFileStoragePath(path, allowed):
"""Checks if a path is in a list of allowed paths for file storage.
@@ -126,7 +179,7 @@ def _CheckFileStoragePath(path, allowed):
+
def _CheckFileStoragePath(path, allowed):
"""Checks if a path is in a list of allowed paths for file storage.
" storage" % path)
-def LoadAllowedFileStoragePaths(filename):
+def _LoadAllowedFileStoragePaths(filename):
"""Loads file containing allowed file storage paths.
@rtype: list
@@ -149,7 +202,13 @@ def CheckFileStoragePath(path, _filename=pathutils.FILE_STORAGE_PATHS_FILE):
-def LoadAllowedFileStoragePaths(filename):
+def _LoadAllowedFileStoragePaths(filename):
"""Loads file containing allowed file storage paths.
@rtype: list
@raise errors.FileStoragePathError: If the path is not allowed
"""
- _CheckFileStoragePath(path, LoadAllowedFileStoragePaths(_filename))
+ allowed = _LoadAllowedFileStoragePaths(_filename)
+
+ if _ComputeWrongFileStoragePaths([path]):
"""
- _CheckFileStoragePath(path, LoadAllowedFileStoragePaths(_filename))
+ allowed = _LoadAllowedFileStoragePaths(_filename)
+
+ raise errors.FileStoragePathError("Path '%s' uses a forbidden prefix" %
+ path)
+
+ _CheckFileStoragePath(path, allowed)
class BlockDev(object):
diff --git a/test/ganeti.bdev_unittest.py b/test/ganeti.bdev_unittest.py
index e900719..d58ee22 100755
+ path)
+
+ _CheckFileStoragePath(path, allowed)
class BlockDev(object):
diff --git a/test/ganeti.bdev_unittest.py b/test/ganeti.bdev_unittest.py
--- a/test/ganeti.bdev_unittest.py
+++ b/test/ganeti.bdev_unittest.py
@@ -373,7 +373,48 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase):
output_extra_matches, volume_name)
-class TestCheckFileStoragePath(unittest.TestCase):
+class TestComputeWrongFileStoragePathsInternal(unittest.TestCase):
+++ b/test/ganeti.bdev_unittest.py
@@ -373,7 +373,48 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase):
output_extra_matches, volume_name)
-class TestCheckFileStoragePath(unittest.TestCase):
+ def testPaths(self):
+ paths = bdev._GetForbiddenFileStoragePaths()
+
+ for path in ["/bin", "/usr/local/sbin", "/lib64", "/etc", "/sys"]:
+ self.assertTrue(path in paths)
+
+ self.assertEqual(set(map(os.path.normpath, paths)), paths)
+
+ def test(self):
+ vfsp = bdev._ComputeWrongFileStoragePaths
+ paths = bdev._GetForbiddenFileStoragePaths()
+
+ for path in ["/bin", "/usr/local/sbin", "/lib64", "/etc", "/sys"]:
+ self.assertTrue(path in paths)
+
+ self.assertEqual(set(map(os.path.normpath, paths)), paths)
+
+ def test(self):
+ self.assertEqual(vfsp([]), [])
+ self.assertEqual(vfsp(["/tmp"]), [])
+ self.assertEqual(vfsp(["/bin/ls"]), ["/bin/ls"])
+ self.assertEqual(vfsp(["/bin"]), ["/bin"])
+ self.assertEqual(vfsp(["/usr/sbin/vim", "/srv/file-storage"]),
+ ["/usr/sbin/vim"])
+
+
+class TestComputeWrongFileStoragePaths(testutils.GanetiTestCase):
+ self.assertEqual(vfsp(["/tmp"]), [])
+ self.assertEqual(vfsp(["/bin/ls"]), ["/bin/ls"])
+ self.assertEqual(vfsp(["/bin"]), ["/bin"])
+ self.assertEqual(vfsp(["/usr/sbin/vim", "/srv/file-storage"]),
+ ["/usr/sbin/vim"])
+
+
+ def test(self):
+ tmpfile = self._CreateTempFile()
+
+ utils.WriteFile(tmpfile, data="""
+ /tmp
+ x/y///z/relative
+ # This is a test file
+ /srv/storage
+ /bin
+ /usr/local/lib32/
+ relative/path
+ """)
+
+ self.assertEqual(bdev.ComputeWrongFileStoragePaths(_filename=tmpfile), [
+ tmpfile = self._CreateTempFile()
+
+ utils.WriteFile(tmpfile, data="""
+ /tmp
+ x/y///z/relative
+ # This is a test file
+ /srv/storage
+ /bin
+ /usr/local/lib32/
+ relative/path
+ """)
+
Michael Hanselmann
Oct 23, 2012, 10:01:37 AM10/23/12
to Iustin Pop, ganeti...@googlegroups.com
2012/10/23 Iustin Pop <ius...@google.com>:
Interdiff:
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -2799,13 +2799,21 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
if (is_master and
(constants.ENABLE_FILE_STORAGE or
constants.ENABLE_SHARED_FILE_STORAGE)):
- fspaths = nresult[constants.NV_FILE_STORAGE_PATHS]
-
- self._ErrorIf(fspaths, constants.CV_ENODEFILESTORAGEPATHS, node,
- "Found forbidden file storage paths: %s",
- utils.CommaJoin(fspaths))
- else:
- assert constants.NV_FILE_STORAGE_PATHS not in nresult
+ try:
+ # This should never happen
+ self._ErrorIf(True, constants.CV_ENODEFILESTORAGEPATHS, node,
+ "Node did not return forbidden file storage paths")
+ else:
+ constants.CV_ENODEFILESTORAGEPATHS, node,
+ "Node should not have returned forbidden file storage"
+ " paths")
def _VerifyOob(self, ninfo, nresult):
"""Verifies out of band functionality of a node.
Michael
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -2799,13 +2799,21 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
if (is_master and
(constants.ENABLE_FILE_STORAGE or
constants.ENABLE_SHARED_FILE_STORAGE)):
- fspaths = nresult[constants.NV_FILE_STORAGE_PATHS]
-
- self._ErrorIf(fspaths, constants.CV_ENODEFILESTORAGEPATHS, node,
- "Found forbidden file storage paths: %s",
- utils.CommaJoin(fspaths))
- else:
- assert constants.NV_FILE_STORAGE_PATHS not in nresult
+ try:
+ fspaths = nresult[constants.NV_FILE_STORAGE_PATHS]
+ except KeyError:
+ # This should never happen
+ self._ErrorIf(True, constants.CV_ENODEFILESTORAGEPATHS, node,
+ "Node did not return forbidden file storage paths")
+ else:
+ self._ErrorIf(fspaths, constants.CV_ENODEFILESTORAGEPATHS, node,
+ "Found forbidden file storage paths: %s",
+ utils.CommaJoin(fspaths))
+ else:
+ self._ErrorIf(constants.NV_FILE_STORAGE_PATHS in nresult,
+ "Found forbidden file storage paths: %s",
+ utils.CommaJoin(fspaths))
+ else:
+ constants.CV_ENODEFILESTORAGEPATHS, node,
+ "Node should not have returned forbidden file storage"
+ " paths")
def _VerifyOob(self, ninfo, nresult):
"""Verifies out of band functionality of a node.
Iustin Pop
Oct 23, 2012, 10:03:32 AM10/23/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Tue, Oct 23, 2012 at 03:54:32PM +0200, Michael Hanselmann wrote:
> An earlier version of this patch series verified all paths in cmdlib in
> the master daemon. With this change all that verification code is moved
> to bdev to run inside the node daemon. The checks are much stricter
> now--it is no longer possible to use forbidden paths (e.g. /bin) to
> manipulate file storage devices (once these checks are being used).
LGTM, thanks.
> An earlier version of this patch series verified all paths in cmdlib in
> the master daemon. With this change all that verification code is moved
> to bdev to run inside the node daemon. The checks are much stricter
> now--it is no longer possible to use forbidden paths (e.g. /bin) to
> manipulate file storage devices (once these checks are being used).
iustin
Iustin Pop
Oct 26, 2012, 10:27:39 AM10/26/12
to Michael Hanselmann, ganeti...@googlegroups.com
On Tue, Oct 23, 2012 at 04:01:37PM +0200, Michael Hanselmann wrote:
> 2012/10/23 Iustin Pop <ius...@google.com>:
> > On Wed, Oct 17, 2012 at 04:17:50PM +0200, Michael Hanselmann wrote:
> >> + if (is_master and
> >> + (constants.ENABLE_FILE_STORAGE or
> >> + constants.ENABLE_SHARED_FILE_STORAGE)):
> >> + fspaths = nresult[constants.NV_FILE_STORAGE_PATHS]
> >
> > The node might not have returned the value here. Please add a proper
> > check.
> >
> >> + self._ErrorIf(fspaths, constants.CV_ENODEFILESTORAGEPATHS, node,
> >> + "Found forbidden file storage paths: %s",
> >> + utils.CommaJoin(fspaths))
> >> + else:
> >> + assert constants.NV_FILE_STORAGE_PATHS not in nresult
> >
> > … and here I don't think this should be an assert (which will break the
> > entire LU), rather than an _ErrorIf.
>
> Interdiff:
LGTM, thanks.
> 2012/10/23 Iustin Pop <ius...@google.com>:
> > On Wed, Oct 17, 2012 at 04:17:50PM +0200, Michael Hanselmann wrote:
> >> + if (is_master and
> >> + (constants.ENABLE_FILE_STORAGE or
> >> + constants.ENABLE_SHARED_FILE_STORAGE)):
> >> + fspaths = nresult[constants.NV_FILE_STORAGE_PATHS]
> >
> > The node might not have returned the value here. Please add a proper
> > check.
> >
> >> + self._ErrorIf(fspaths, constants.CV_ENODEFILESTORAGEPATHS, node,
> >> + "Found forbidden file storage paths: %s",
> >> + utils.CommaJoin(fspaths))
> >> + else:
> >> + assert constants.NV_FILE_STORAGE_PATHS not in nresult
> >
> > … and here I don't think this should be an assert (which will break the
> > entire LU), rather than an _ErrorIf.
>
> Interdiff:
iustin
Reply all
Reply to author
Forward
0 new messages