As per design document (doc/design-remote-commands.rst), a number of
rather strict tests is applied to any incoming request, a delay is
inserted upon errors and returned error messages are very generic
(unless it's the actual command that failed). There are unit tests for
all of the newly added code.
+#: Minimum file permissions for remote command directory and executables
+_RCMD_MIN_MODE = (stat.S_IRWXU |
+ stat.S_IRGRP | stat.S_IXGRP |
+ stat.S_IROTH | stat.S_IXOTH)
+
+#: Delay before returning an error for remote commands
+_RCMD_INVALID_DELAY = 10
+
+#: How long to wait to acquire lock for remote commands (shorter than
+#: L{_RCMD_INVALID_DELAY}) to reduce blockage of noded forks when many
+#: command requests arrive
+_RCMD_LOCK_TIMEOUT = _RCMD_INVALID_DELAY * 0.8
+
+def _VerifyRemoteCommandName(cmd):
+ """Verifies a remote command name.
+
+ @type cmd: string
+ @param cmd: Command name
+ @rtype: string or None
+ @return: None if command name is acceptable, an error message otherwise
+
+ """
+ if not cmd.strip():
+ return "Missing command name"
+
+ if os.path.basename(cmd) != cmd:
+ return "Invalid command name"
+
+ if not constants.EXT_PLUGIN_MASK.match(cmd):
+ return "Command name contains forbidden characters"
+
+ return None
+
+
+def _CommonRemoteCommandCheck(path, getents_fn):
+ """Common checks for remote command file system directories and files.
+
+ @type path: string
+ @param path: Path to check
+ @type getents_fn: None or class
+ @param getents_fn: Must be C{None} or class whose instances have the same
+ interface as L{runtime.GetEnts} (used to resolve UID/GID)
+ @rtype: tuple; (string, stat result)
+ @return: First tuple element is an error message or C{None} if check was
+ successful; if the latter is the case, the second element is the result of
+ C{os.stat}, otherwise it's C{None}
+
+ """
+ if getents_fn is None:
+ getents_fn = runtime.GetEnts
+
+ try:
+ st = os.stat(path)
+ except EnvironmentError, err:
+ return ("Can't stat(2) '%s': %s" % (path, err), None)
+
+ if stat.S_IMODE(st.st_mode) & (~_RCMD_MIN_MODE):
+ return ("Permissions on '%s' are too permissive" % path, None)
+
+ getent = getents_fn()
+
+ if not (st.st_uid == getent.noded_uid and
+ st.st_gid == getent.noded_gid):
+ return ("'%s' is not owned by %s:%s" %
+ (path, getent.noded_uid, getent.noded_gid), None)
+
+ return (None, st)
+
+
+def _VerifyRemoteCommandDirectory(path, _getents_fn=None):
+ """Verifies remote command directory.
+
+ @type path: string
+ @param path: Path to check
+ @rtype: string or None
+ @return: If check was successful, C{None} is returned, otherwise an error
+ message
+
+ """
+ (msg, st) = _CommonRemoteCommandCheck(path, _getents_fn)
+
+ if msg:
+ return msg
+
+ if not stat.S_ISDIR(st.st_mode):
+ return "Path '%s' is not a directory" % path
+
+ return None
+
+
+def _VerifyRemoteCommand(path, cmd, _getents_fn=None):
+ """Verifies a whole remote command and returns its executable filename.
+
+ @type path: string
+ @param path: Directory containing remote commands
+ @type cmd: string
+ @param cmd: Command name
+ @rtype: tuple; (string, string)
+ @return: The first tuple element is an error message if some test failed,
+ otherwise it's C{None}; the second element is the absolute path to the
+ executable or C{None} if a test failed
+
+ """
+ executable = utils.PathJoin(path, cmd)
+
+ (msg, _) = _CommonRemoteCommandCheck(executable, _getents_fn)
+
+ if msg:
+ return (msg, None)
+
+ if not utils.IsExecutable(executable):
+ return ("access(2) thinks '%s' can't be executed" % executable, None)
+
+ return (None, executable)
+
+
+def _PrepareRemoteCommand(path, cmd,
+ _verify_dir=_VerifyRemoteCommandDirectory,
+ _verify_name=_VerifyRemoteCommandName,
+ _verify_cmd=_VerifyRemoteCommand):
+ """Performs a number of tests on a remote command.
+
+ @type path: string
+ @param path: Directory containing remote commands
+ @type cmd: string
+ @param cmd: Command name
+ @return: Same as L{_VerifyRemoteCommand}
+
+ """
+ # Verify the directory first
+ result = _verify_dir(path)
+ if result:
+ return (result, None)
+
+ # Check command if everything was alright
+ result = _verify_name(cmd)
+ if result:
+ return (result, None)
+
+ # Check actual executable
+ return _verify_cmd(path, cmd)
+
+
+def RunRemoteCommand(cmd,
+ _lock_timeout=_RCMD_LOCK_TIMEOUT,
+ _lock_file=pathutils.REMOTE_COMMANDS_LOCK_FILE,
+ _path=pathutils.REMOTE_COMMANDS_DIR,
+ _sleep_fn=time.sleep,
+ _prepare_fn=_PrepareRemoteCommand,
+ _runcmd_fn=utils.RunCmd):
+ """Executes a remote command after performing strict tests.
+
+ @type cmd: string
+ @param cmd: Command name
+ @rtype: string
+ @return: Command output
+ @raise RPCFail: In case of an error
+
+ """
+ logging.info("Preparing to run remote command '%s'", cmd)
+
+ lock = None
+ try:
+ cmdresult = None
+ try:
+ lock = utils.FileLock.Open(_lock_file)
+ lock.Exclusive(blocking=True, timeout=_lock_timeout)
+
+ (checkresult, executable) = _prepare_fn(_path, cmd)
+
+ if checkresult is not None:
+ assert executable is None
+ logging.error(checkresult)
+ else:
+ cmdresult = _runcmd_fn([executable], env={}, reset_env=True,
+ postfork_fn=lambda _: lock.Unlock())
+ except Exception: # pylint: disable=W0703
+ # Keep original error in log
+ logging.exception("Caught exception")
+
+ if cmdresult is None:
+ logging.info("Sleeping for %0.1f seconds before returning",
+ _RCMD_INVALID_DELAY)
+ _sleep_fn(_RCMD_INVALID_DELAY)
+
+ # Do not include original error message in returned error
+ _Fail("Executing command '%s' failed" % cmd)
+ elif cmdresult.failed or cmdresult.fail_reason:
+ _Fail("Remote command '%s' failed: %s; output: %s",
+ cmd, cmdresult.fail_reason, cmdresult.output)
+ else:
+ return cmdresult.output
+ finally:
+ if lock is not None:
+ # Release lock at last
+ lock.Close()
+ lock = None
+
+
class HooksRunner(object):
"""Hook runner.
diff --git a/test/ganeti.backend_unittest-runasroot.py b/test/ganeti.backend_unittest-runasroot.py
new file mode 100755
index 0000000..8b949ba
--- /dev/null
+++ b/test/ganeti.backend_unittest-runasroot.py
@@ -0,0 +1,82 @@
+#!/usr/bin/python
+#
+
+# Copyright (C) 2012 Google Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.
+
+
+"""Script for testing ganeti.backend (tests requiring root access)"""
+
+import os
+import tempfile
+import shutil
+import errno
+
+from ganeti import constants
+from ganeti import utils
+from ganeti import compat
+from ganeti import backend
+
+import testutils
+import mocks
+
+
+class TestWriteFile(testutils.GanetiTestCase):
+ def setUp(self):
+ self.tmpdir = tempfile.mkdtemp()
+
+ def tearDown(self):
+ shutil.rmtree(self.tmpdir)
+
+ def _PrepareTest(self):
+ tmpname = utils.PathJoin(self.tmpdir, "foobar")
+ os.mkdir(tmpname)
+ os.chmod(tmpname, 0700)
+ return tmpname
+
+ def testCorrectOwner(self):
+ tmpname = self._PrepareTest()
+
+ getents_fn = mocks.FakeGetentResolver
+ os.chown(tmpname, getents_fn().noded_uid, getents_fn().noded_gid)
+ (error, st) = backend._CommonRemoteCommandCheck(tmpname, getents_fn)
+ self.assertTrue(error is None)
+ self.assertTrue(st)
+
+ def testWrongOwner(self):
+ tmpname = self._PrepareTest()
+
+ getents_fn = mocks.FakeGetentResolver
+
+
In some cases it's nice to verify a function has been called exactly N
times. This is going to be used in tests for remote commands.
---
test/testutils.py | 29 +++++++++++++++++++++++++++++
1 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/test/testutils.py b/test/testutils.py
index 1a47a66..3fcfbc4 100644
--- a/test/testutils.py
+++ b/test/testutils.py
@@ -223,3 +223,32 @@ def UnifyValueType(data):
for (key, value) in data.iteritems()])
return data
+
+
+class CallCounter(object):
+ """Utility class to count number of calls to a function/method.
+
+ """
+ def __init__(self, fn):
+ """Initializes this class.
+
+ @type fn: Callable
+
+ """
+ self._fn = fn
+ self._count = 0
+
+ def __call__(self, *args, **kwargs):
+ """Calls wrapped function with given parameters.
+
+ """
+ self._count += 1
+ return self._fn(*args, **kwargs)
+
+ def Count(self):
+ """Returns number of calls.
+
+ @rtype: number
+
+ """
+ return self._count
-- 1.7.7.3
This series implements the backend part for remote commands as per the design
document in “doc/design-remote-commands.rst”.
Michael Hanselmann (5):
Add test utility to count calls to function
Add previously missing node daemon GID to getent mock
utils.process.RunResult: Always set "fail_reason" attribute
pathutils: Add directory for remote commands
backend: Implement remote commands
#: 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 d58ee22..00347f6 100755
--- a/test/ganeti.bdev_unittest.py
+++ b/test/ganeti.bdev_unittest.py
@@ -1,7 +1,7 @@
#!/usr/bin/python
#
-# Copyright (C) 2006, 2007, 2010 Google Inc.
+# Copyright (C) 2006, 2007, 2010, 2012 Google Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -29,6 +29,7 @@ from ganeti import bdev
from ganeti import errors
from ganeti import constants
from ganeti import utils
+from ganeti import pathutils
import testutils
@@ -391,6 +392,10 @@ class TestComputeWrongFileStoragePathsInternal(unittest.TestCase):
self.assertEqual(vfsp(["/usr/sbin/vim", "/srv/file-storage"]),
["/usr/sbin/vim"])
+ self.assertTrue(vfsp([pathutils.REMOTE_COMMANDS_DIR]),
+ msg=("Remote commands directory should never be valid"
+ "for file storage"))
+
class TestComputeWrongFileStoragePaths(testutils.GanetiTestCase):
def test(self):
-- 1.7.7.3
On Wed, Nov 14, 2012 at 03:03:09AM +0100, Michael Hanselmann wrote:
> As per design document (doc/design-remote-commands.rst), a number of
> rather strict tests is applied to any incoming request, a delay is
> inserted upon errors and returned error messages are very generic
> (unless it's the actual command that failed). There are unit tests for
> all of the newly added code.
> +#: Delay before returning an error for remote commands
> +_RCMD_INVALID_DELAY = 10
> +
> +#: How long to wait to acquire lock for remote commands (shorter than
> +#: L{_RCMD_INVALID_DELAY}) to reduce blockage of noded forks when many
> +#: command requests arrive
> +_RCMD_LOCK_TIMEOUT = _RCMD_INVALID_DELAY * 0.8
> +
> +def _VerifyRemoteCommandName(cmd):
> + """Verifies a remote command name.
> +
> + @type cmd: string
> + @param cmd: Command name
> + @rtype: string or None
> + @return: None if command name is acceptable, an error message otherwise
> +
> + """
> + if not cmd.strip():
> + return "Missing command name"
> +
> + if os.path.basename(cmd) != cmd:
> + return "Invalid command name"
> +
> + if not constants.EXT_PLUGIN_MASK.match(cmd):
> + return "Command name contains forbidden characters"
> +
> + return None
> +
> +
> +def _CommonRemoteCommandCheck(path, getents_fn):
> + """Common checks for remote command file system directories and files.
> +
> + @type path: string
> + @param path: Path to check
> + @type getents_fn: None or class
> + @param getents_fn: Must be C{None} or class whose instances have the same
> + interface as L{runtime.GetEnts} (used to resolve UID/GID)
> + @rtype: tuple; (string, stat result)
> + @return: First tuple element is an error message or C{None} if check was
> + successful; if the latter is the case, the second element is the result of
> + C{os.stat}, otherwise it's C{None}
This uses very weird return semantics. In all other parts of the code,
it's (status, result), status being a boolean.
On Wed, Nov 14, 2012 at 03:03:05AM +0100, Michael Hanselmann wrote:
> In some cases it's nice to verify a function has been called exactly N
> times. This is going to be used in tests for remote commands.
On Wed, Nov 14, 2012 at 03:03:08AM +0100, Michael Hanselmann wrote:
> Also add tests to ensure it's never allowed as a file storage path. A
> constant for the lock file is also added.
- if stat.S_IMODE(st.st_mode) & (~_RCMD_MIN_MODE):
+ if stat.S_IMODE(st.st_mode) & (~_RCMD_MAX_MODE):
return ("Permissions on '%s' are too permissive" % path, None)
getent = getents_fn()
>> +def _CommonRemoteCommandCheck(path, getents_fn):
>> + @rtype: tuple; (string, stat result)
>> + @return: First tuple element is an error message or C{None} if check was
>> + successful; if the latter is the case, the second element is the result of
>> + C{os.stat}, otherwise it's C{None}
> This uses very weird return semantics. In all other parts of the code,
> it's (status, result), status being a boolean.
These are only used within backend.py and never returned via RPC.
Since there are only two cases (success=None or error="string"), I
don't see why the code should be made articially complicated by adding
a boolean. The os.stat result is used to check if the path is a
directory (no need to call stat(2) again through os.path.isdir).
>> + if not (st.st_uid == getent.noded_uid and
>> + st.st_gid == getent.noded_gid):
>> + return ("'%s' is not owned by %s:%s" %
>> + (path, getent.noded_uid, getent.noded_gid), None)
> Hmm, why this and not require root? In case we move noded to non-root,
> then it's safer to have these require root.
Currently both are 0, so I thought … why not? Also, simpler unittests.
Anyway, I replaced getents completely with a new argument named
“owner”. I'll re-send the patch later.
> - if stat.S_IMODE(st.st_mode) & (~_RCMD_MIN_MODE):
> + if stat.S_IMODE(st.st_mode) & (~_RCMD_MAX_MODE):
> return ("Permissions on '%s' are too permissive" % path, None)
> >> + @rtype: tuple; (string, stat result)
> >> + @return: First tuple element is an error message or C{None} if check was
> >> + successful; if the latter is the case, the second element is the result of
> >> + C{os.stat}, otherwise it's C{None}
> > This uses very weird return semantics. In all other parts of the code,
> > it's (status, result), status being a boolean.
> These are only used within backend.py and never returned via RPC.
That doesn't matter, because:
> Since there are only two cases (success=None or error="string"), I
> don't see why the code should be made articially complicated by adding
> a boolean. The os.stat result is used to check if the path is a
> directory (no need to call stat(2) again through os.path.isdir).
For consistency. You add here a new Either data type, that is not
behaving like the other Either data type we have in Python. Let's be
consistent and use the same Either every where, so that programmers
don't have to learn multiple way of returning failure from functions.
Also, the (success, result) way can of course return a real result, so
your comment about stat result is not applicable:
> >> + if not (st.st_uid == getent.noded_uid and
> >> + st.st_gid == getent.noded_gid):
> >> + return ("'%s' is not owned by %s:%s" %
> >> + (path, getent.noded_uid, getent.noded_gid), None)
> > Hmm, why this and not require root? In case we move noded to non-root,
> > then it's safer to have these require root.
> Currently both are 0, so I thought … why not? Also, simpler unittests.
Because sometime some will change noded_uid, and will forget that
someone reused a constant name just because it's convenient. As long as
this is not explicitly needed to be noded_uid/gid, it shouldn't use
those constants.
> Anyway, I replaced getents completely with a new argument named
> “owner”. I'll re-send the patch later.
> > I don't see any check for build-time disables here?
> It wasn't in the design. :-)
> I'll add it locally and will re-send the whole patch once we figure
> out the above.
Definitely, it was in my original request - this needs to be
disable-able at configure time. It's true that the translation to the
design doc lost this :)
By default remote commands are disabled and need to be explicitely
enabled at build time.
---
Makefile.am | 1 +
configure.ac | 13 +++++++++++++
2 files changed, 14 insertions(+), 0 deletions(-)
Mmm... could this be a cluster level option instead (not modifiable
via rapi, or checked against a filesystem-based config)? As a
./configure argument it becomes completely useless to anybody who uses
a packaged version.
On Wed, Nov 14, 2012 at 11:25 AM, Michael Hanselmann <han...@google.com> wrote:
> By default remote commands are disabled and need to be explicitely
> enabled at build time.
> ---
> Makefile.am | 1 +
> configure.ac | 13 +++++++++++++
> 2 files changed, 14 insertions(+), 0 deletions(-)
On Wed, Nov 14, 2012 at 11:27:03AM +0100, Guido Trotter wrote:
> Mmm... could this be a cluster level option instead (not modifiable
> via rapi, or checked against a filesystem-based config)? As a
> ./configure argument it becomes completely useless to anybody who uses
> a packaged version.
Hmm… this feature is sensitive enough that it certainly can't be a
cluster option (no matter the way to modify it), but I'm file with a
filesystem based config, same way as file-based storage.
> Mmm... could this be a cluster level option instead (not modifiable
> via rapi, or checked against a filesystem-based config)? As a
> ./configure argument it becomes completely useless to anybody who uses
> a packaged version.
It is like file storage, which cannot be controlled at “runtime”
either. Anything which goes via the configuration is not good (can be
exploited if you have “server.pem”).
> On Wed, Nov 14, 2012 at 11:27:03AM +0100, Guido Trotter wrote:
>> Mmm... could this be a cluster level option instead (not modifiable
>> via rapi, or checked against a filesystem-based config)? As a
>> ./configure argument it becomes completely useless to anybody who uses
>> a packaged version.
> Hmm… this feature is sensitive enough that it certainly can't be a
> cluster option (no matter the way to modify it), but I'm file with a
> filesystem based config, same way as file-based storage.
> Anyone can see any problems with that?
Not per se, but it doesn't belong in this patch. I think we have other
options which could use a more flexible way for configuring them, such
as file storage. I'll file a feature request.
On Wed, Nov 14, 2012 at 11:35:53AM +0100, Michael Hanselmann wrote:
> 2012/11/14 Iustin Pop <ius...@google.com>:
> > On Wed, Nov 14, 2012 at 11:27:03AM +0100, Guido Trotter wrote:
> >> Mmm... could this be a cluster level option instead (not modifiable
> >> via rapi, or checked against a filesystem-based config)? As a
> >> ./configure argument it becomes completely useless to anybody who uses
> >> a packaged version.
> > Hmm… this feature is sensitive enough that it certainly can't be a
> > cluster option (no matter the way to modify it), but I'm file with a
> > filesystem based config, same way as file-based storage.
> > Anyone can see any problems with that?
> Not per se, but it doesn't belong in this patch. I think we have other
> options which could use a more flexible way for configuring them, such
> as file storage. I'll file a feature request.
Sorry, I don't understand your comment. Can you explain?
> On Wed, Nov 14, 2012 at 11:35:53AM +0100, Michael Hanselmann wrote:
>> Not per se, but it doesn't belong in this patch. I think we have other
>> options which could use a more flexible way for configuring them, such
>> as file storage. I'll file a feature request.
> Sorry, I don't understand your comment. Can you explain?
Remote commands are not the only option currently only available as a
build-time flag. Enabling file storage is in the same category, as
well as some of the paths. There should be a more generic way for
configuring these flags, not just a one-off for remote commands. As
such there should at least be a small design document.
On Wed, Nov 14, 2012 at 11:41:13AM +0100, Michael Hanselmann wrote:
> 2012/11/14 Iustin Pop <ius...@google.com>:
> > On Wed, Nov 14, 2012 at 11:35:53AM +0100, Michael Hanselmann wrote:
> >> Not per se, but it doesn't belong in this patch. I think we have other
> >> options which could use a more flexible way for configuring them, such
> >> as file storage. I'll file a feature request.
> > Sorry, I don't understand your comment. Can you explain?
> Remote commands are not the only option currently only available as a
> build-time flag. Enabling file storage is in the same category, as
> well as some of the paths. There should be a more generic way for
> configuring these flags, not just a one-off for remote commands. As
> such there should at least be a small design document.
As per design document (doc/design-remote-commands.rst), a number of
rather strict tests is applied to any incoming request, a delay is
inserted upon errors and returned error messages are very generic
(unless it's the actual command that failed). There are unit tests for
all of the newly added code.
+#: Maximum file permissions for remote command directory and executables
+_RCMD_MAX_MODE = (stat.S_IRWXU |
+ stat.S_IRGRP | stat.S_IXGRP |
+ stat.S_IROTH | stat.S_IXOTH)
+
+#: Delay before returning an error for remote commands
+_RCMD_INVALID_DELAY = 10
+
+#: How long to wait to acquire lock for remote commands (shorter than
+#: L{_RCMD_INVALID_DELAY}) to reduce blockage of noded forks when many
+#: command requests arrive
+_RCMD_LOCK_TIMEOUT = _RCMD_INVALID_DELAY * 0.8
+
+def _VerifyRemoteCommandName(cmd):
+ """Verifies a remote command name.
+
+ @type cmd: string
+ @param cmd: Command name
+ @rtype: tuple; (boolean, string or None)
+ @return: The tuple's first element is the status; if C{False}, the second
+ element is an error message string, otherwise it's C{None}
+
+ """
+ if not cmd.strip():
+ return (False, "Missing command name")
+
+ if os.path.basename(cmd) != cmd:
+ return (False, "Invalid command name")
+
+ if not constants.EXT_PLUGIN_MASK.match(cmd):
+ return (False, "Command name contains forbidden characters")
+
+ return (True, None)
+
+
+def _CommonRemoteCommandCheck(path, owner):
+ """Common checks for remote command file system directories and files.
+
+ @type path: string
+ @param path: Path to check
+ @param owner: C{None} or tuple containing UID and GID
+ @rtype: tuple; (boolean, string or C{os.stat} result)
+ @return: The tuple's first element is the status; if C{False}, the second
+ element is an error message string, otherwise it's the result of C{os.stat}
+
+ """
+ if owner is None:
+ # Default to root as owner
+ owner = (0, 0)
+
+ try:
+ st = os.stat(path)
+ except EnvironmentError, err:
+ return (False, "Can't stat(2) '%s': %s" % (path, err))
+
+ if stat.S_IMODE(st.st_mode) & (~_RCMD_MAX_MODE):
+ return (False, "Permissions on '%s' are too permissive" % path)
+
+ if (st.st_uid, st.st_gid) != owner:
+ (owner_uid, owner_gid) = owner
+ return (False, "'%s' is not owned by %s:%s" % (path, owner_uid, owner_gid))
+
+ return (True, st)
+
+
+def _VerifyRemoteCommandDirectory(path, _owner=None):
+ """Verifies remote command directory.
+
+ @type path: string
+ @param path: Path to check
+ @rtype: tuple; (boolean, string or None)
+ @return: The tuple's first element is the status; if C{False}, the second
+ element is an error message string, otherwise it's C{None}
+
+ """
+ (status, value) = _CommonRemoteCommandCheck(path, _owner)
+
+ if not status:
+ return (False, value)
+
+ if not stat.S_ISDIR(value.st_mode):
+ return (False, "Path '%s' is not a directory" % path)
+
+ return (True, None)
+
+
+def _VerifyRemoteCommand(path, cmd, _owner=None):
+ """Verifies a whole remote command and returns its executable filename.
+
+ @type path: string
+ @param path: Directory containing remote commands
+ @type cmd: string
+ @param cmd: Command name
+ @rtype: tuple; (boolean, string)
+ @return: The tuple's first element is the status; if C{False}, the second
+ element is an error message string, otherwise the second element is the
+ absolute path to the executable
+
+ """
+ executable = utils.PathJoin(path, cmd)
+
+ (status, msg) = _CommonRemoteCommandCheck(executable, _owner)
+
+ if not status:
+ return (False, msg)
+
+ if not utils.IsExecutable(executable):
+ return (False, "access(2) thinks '%s' can't be executed" % executable)
+
+ return (True, executable)
+
+
+def _PrepareRemoteCommand(path, cmd,
+ _verify_dir=_VerifyRemoteCommandDirectory,
+ _verify_name=_VerifyRemoteCommandName,
+ _verify_cmd=_VerifyRemoteCommand):
+ """Performs a number of tests on a remote command.
+
+ @type path: string
+ @param path: Directory containing remote commands
+ @type cmd: string
+ @param cmd: Command name
+ @return: Same as L{_VerifyRemoteCommand}
+
+ """
+ # Verify the directory first
+ (status, msg) = _verify_dir(path)
+ if status:
+ # Check command if everything was alright
+ (status, msg) = _verify_name(cmd)
+
+ if not status:
+ return (False, msg)
+
+ # Check actual executable
+ return _verify_cmd(path, cmd)
+
+
+def RunRemoteCommand(cmd,
+ _lock_timeout=_RCMD_LOCK_TIMEOUT,
+ _lock_file=pathutils.REMOTE_COMMANDS_LOCK_FILE,
+ _path=pathutils.REMOTE_COMMANDS_DIR,
+ _sleep_fn=time.sleep,
+ _prepare_fn=_PrepareRemoteCommand,
+ _runcmd_fn=utils.RunCmd,
+ _enabled=constants.ENABLE_REMOTE_COMMANDS):
+ """Executes a remote command after performing strict tests.
+
+ @type cmd: string
+ @param cmd: Command name
+ @rtype: string
+ @return: Command output
+ @raise RPCFail: In case of an error
+
+ """
+ logging.info("Preparing to run remote command '%s'", cmd)
+
+ if not _enabled:
+ _Fail("Remote commands disabled at configure time")
+
+ lock = None
+ try:
+ cmdresult = None
+ try:
+ lock = utils.FileLock.Open(_lock_file)
+ lock.Exclusive(blocking=True, timeout=_lock_timeout)
+
+ (status, value) = _prepare_fn(_path, cmd)
+
+ if status:
+ cmdresult = _runcmd_fn([value], env={}, reset_env=True,
+ postfork_fn=lambda _: lock.Unlock())
+ else:
+ logging.error(value)
+ except Exception: # pylint: disable=W0703
+ # Keep original error in log
+ logging.exception("Caught exception")
+
+ if cmdresult is None:
+ logging.info("Sleeping for %0.1f seconds before returning",
+ _RCMD_INVALID_DELAY)
+ _sleep_fn(_RCMD_INVALID_DELAY)
+
+ # Do not include original error message in returned error
+ _Fail("Executing command '%s' failed" % cmd)
+ elif cmdresult.failed or cmdresult.fail_reason:
+ _Fail("Remote command '%s' failed: %s; output: %s",
+ cmd, cmdresult.fail_reason, cmdresult.output)
+ else:
+ return cmdresult.output
+ finally:
+ if lock is not None:
+ # Release lock at last
+ lock.Close()
+ lock = None
+
+
class HooksRunner(object):
"""Hook runner.
diff --git a/test/ganeti.backend_unittest-runasroot.py b/test/ganeti.backend_unittest-runasroot.py
new file mode 100755
index 0000000..6d99d9c
--- /dev/null
+++ b/test/ganeti.backend_unittest-runasroot.py
@@ -0,0 +1,77 @@
+#!/usr/bin/python
+#
+
+# Copyright (C) 2012 Google Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301, USA.
+
+
+"""Script for testing ganeti.backend (tests requiring root access)"""
+
+import os
+import tempfile
+import shutil
+import errno
+
+from ganeti import constants
+from ganeti import utils
+from ganeti import compat
+from ganeti import backend
+
+import testutils
+
+
+class TestWriteFile(testutils.GanetiTestCase):
+ def setUp(self):
+ self.tmpdir = tempfile.mkdtemp()
+
+ def tearDown(self):
+ shutil.rmtree(self.tmpdir)
+
+ def _PrepareTest(self):
+ tmpname = utils.PathJoin(self.tmpdir, "foobar")
+ os.mkdir(tmpname)
+ os.chmod(tmpname, 0700)
+ return tmpname
+
+ def testCorrectOwner(self):
+ tmpname = self._PrepareTest()
+
+ os.chown(tmpname, 0, 0)
+ (status, value) = backend._CommonRemoteCommandCheck(tmpname, None)
+ self.assertTrue(status)
+ self.assertTrue(value)
+
+ def testWrongOwner(self):
+ tmpname = self._PrepareTest()
+
+ tests = [
+ (1, 0),
+ (0, 1),
+ (100, 50),
+
On Wed, Nov 14, 2012 at 01:38:50PM +0100, Michael Hanselmann wrote:
> As per design document (doc/design-remote-commands.rst), a number of
> rather strict tests is applied to any incoming request, a delay is
> inserted upon errors and returned error messages are very generic
> (unless it's the actual command that failed). There are unit tests for
> all of the newly added code.
LGTM. I don't like the fact that some tests are duplicated (e.g.
statfail) across dir/file, when the actual behaviour we want is to make
sure that both dir and file call the common behaviour, but…
On Wed, Nov 14, 2012 at 11:41:13AM +0100, Michael Hanselmann wrote:
> 2012/11/14 Iustin Pop <ius...@google.com>:
> > On Wed, Nov 14, 2012 at 11:35:53AM +0100, Michael Hanselmann wrote:
> >> Not per se, but it doesn't belong in this patch. I think we have other
> >> options which could use a more flexible way for configuring them, such
> >> as file storage. I'll file a feature request.
> > Sorry, I don't understand your comment. Can you explain?
> Remote commands are not the only option currently only available as a
> build-time flag. Enabling file storage is in the same category, as
> well as some of the paths. There should be a more generic way for
> configuring these flags, not just a one-off for remote commands. As
> such there should at least be a small design document.
