Attached is an updated version of this patch which makes two more
changes to the CopyFile minion.
One major change is that it now tries to use the pyxattr library to
copy extended attributes (including SELinux contexts) from the target file to
the tempfile. Pyxattr is not a core library but it is packaged for RHEL5 and
6. If this library is not available then it falls back to calling chcon.
The other major change is that it explicitly looks for a SELinux
config file. If one is present then it will consider a failure to copy the
SELinux contexts from the target file to the tempfile to be an error and will
exit. Previously it just kept going when this failed.
The remaining changes are organizational, e.g. moving the "from
subprocess import..." to the top.
--
Marcus
On Monday, September 03, 2012 04:29:28 PM Marcus Lauer wrote:
> Since part #1 of my three-part plan is proving troublesome I moved
> ahead on #2.
>
> Although part #3 of my original plan borrows some code from #1 it
> does not require #1. If this patch is accepted then I will move ahead on
> #3.
>
> This patch changes copyfile in two ways:
>
> 1. Copyfile now opens a tempfile on the minion and appends data to this
> tempfile, then moves the tempfile over the target file after all of the data
> has been appended.
>
> The advantage of this is that updating the target file is atomic, or
> as close to it as possible in python. This minimizes the chance of some
> program accessing an empty or half-complete file. If the file in question
> were a server config file... surely you can imagine how this could be a
> problem.
>
> The tempfiles are created in the same directory as the target file
> (for security) and have the same name as the target file but with the
> extension ".functmp". Permissions are set on this file during the "open"
> stage, while it is still zero-size.
>
> 2. Copyfile has a new option called "retainperms" ("--retainperms" at the
> command line). It is "False" by default.
>
> Currently func tries to give the new file on the minion the same
> attributes (permissions and uid/gid) as the file on the overlord. The
> SELinux context of the new file is not set (and does not change). This
> overrides this behavior to make the permissions, flags, uid, gid and
> SELinux contexts of the new file identical to the existing file (if any) on
> the minion.
>
> This is intended to deal with the situation in which the file on the
> overlord has different permissions than the files on the minions and the
> user does not want to overwrite them. Perhaps the overlord computer does
> not have the same users and groups as the minions? I figured this might be
> useful for someone.
>
>
> On a related note, does anyone think it would be worthwhile to have
> copyfile copy the SELinux contexts of the source file to the target? How
> about making command-line options to allow the user to specify the desired
> mode, uid, gid, contexts, etc.?
>
> --
> Marcus
>
> On Saturday, August 25, 2012 04:55:09 PM Marcus Lauer wrote:
> > Attached are my original patch and a git-patch which
> > contains the
> >
> > same code. These implement changes I called #1 in my previous e-mail.
> >
> > #2 doesn't seem too hard. CopyFile.open will have to
> > open a
> >
> > tempfile rather than the target file. It already returns the filepath
> > on
> > success. Append already uses the return value of CopyFile.open as the
> > file to append to. After appending has finished there needs to be a
> > CopyFile.move function called by the overlord which takes both paths and
> > moves the tempfile over the source file. Making sure the
> > ownership/permissions/SELinux attributes of the target file do not
> > change
> > could be is important so had better be careful about that.
> >
> > Once this is in place #3 is almost trivial. The
> > overlord needs to>
> > calculate the MD5 of the source file. The code for doing this is also
> > needed by #1 and is included in my patches. That MD5 should be passed
> > to
> > the CopyFile.move function. It should calculate the MD5 of the tempfile
> > and if they don't match just delete the tempfile and return an error
> > instead of moving it. I figure that this MD5 check should be optional
> > so I will also create a flag to enable it.
> >
> > Can anyone see any issues with this general design? If
> > not I'll get>
> > working on it when I have the time.
> >
> > --
> > Marcus
> >
> > On Friday, August 24, 2012 07:32:14 PM you wrote:
> > > Copyfile could most certainly use some TLC, so let me know if I can
> > > help,
> > > that would certainly be of help if you'd like to work on that. The
> > > list has indeed changed to lists.fedorahosted.com, so definitely
> > > use that one.
> > >
> > > Would you mind shooting me your original patch as well? Thanks very
> > > much!>
> > > On Fri, Aug 24, 2012 at 7:22 PM, Marcus Lauer
>
> <marcuslauer@xxxxxxxxxx>wrote:
> > > > It seems to me that there are at least three
> > > > improvements which
> > > >
> > > > should be made to copyfile:
> > > >
> > > > 1. If the file on the overlord and minion are the same, that
> > > > file
> > > > should not
> > > > be copied. This saves bandwidth/processing time.
> > > >
> > > > 2. Overwriting the file on the minion should be atomic, e.g. the
> > > > file on the
> > > > overlord should be copied to a tempfile on the minion then moved
> > > > to
> > > > overwrite
> > > > the target file as a final step. Right now if funcd dies or
> > > > something
> > > > goes wrong mid-copy you end up with half a file on the minion.
> > > >
> > > > 3. There should be an option (off by default) such that if the
> > > > file
> > > > which
> > > > has
> > > > been copied over to the minion is not identical to the file on
> > > > the
> > > > overlord it
> > > > is re-copied, or at least does not overwrite the file on the
> > > > minion.
> > > > This won't work for some files (busy logfiles) but for most
> > > > files it
> > > > seems like an
> > > > extra bit of reliability which most people would want in most
> > > > situations.
> > > >
> > > > I submitted a patch (though not a git-patch...
> > > > my
> > > > bad!)
> > > > back in
> > > >
> > > > January 2011 which implements #1, but imperfectly. The overlord
> > > > still
> > > > has to
> > > > send a (small) packet to the minion for each 60KB chunk of
> > > > target
> > > > file.
> > > >
> > > > Would anyone be bothered terribly if I worked on
> > > > #2
> > > > and
> > > > #3? While
> > > >
> > > > I am
> > > > at it I will try to find a way to fix up my patch for #1 as
> > > > well.
> > > > There is some overlap between these patches. Both will involve
> > > > calculating an MD5 hash
> > > > of the file on the overlord, for example.
> > > >
> > > > P.S. Is the new mailing list on fedorahosted.org
> > > > live
> > > > yet? I am
> > > >
> > > > sending this message to both just in case.
> > > >
> > > > --
> > > > Marcus
> > > > _______________________________________________
> > > > func mailing list
> > > > func@xxxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.fedorahosted.org/mailman/listinfo/func
> > >
> > > --
> > > Steve Salevan
> > > steve@xxxxxxxxxx>From e45bc0b5774e68beb09e4bfab0f112bd9c9fa5c8 Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Wed, 12 Sep 2012 18:29:54 -0400
Subject: [PATCH 1/5] CopyFile: make tempfile on minion first then move it
over target. Add --retainperms flag.
---
func/minion/modules/copyfile.py | 102 ++++++++++++++++++++++++++-------
func/overlord/cmd_modules/copyfile.py | 4 +-
func/overlord/modules/copyfile.py | 9 ++-
3 files changed, 90 insertions(+), 25 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index 61c6a4f..5e47c1d 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -26,12 +26,12 @@ import time
import shutil
import func_module
-
+from func import utils as func_utils
class CopyFile(func_module.FuncModule):
version = "0.0.1"
- api_version = "0.0.2"
+ api_version = "0.0.3"
description = "Allows for smart copying of a file."
def _checksum_blob(self, blob):
@@ -40,7 +40,6 @@ class CopyFile(func_module.FuncModule):
return thissum.hexdigest()
def checksum(self, thing):
-
CHUNK=2**16
thissum = hashlib.new('sha1')
if os.path.exists(thing):
@@ -57,47 +56,106 @@ class CopyFile(func_module.FuncModule):
return thissum.hexdigest()
- def open(self, filepath, mode=None, uid=-1, gid=-1):
+ def open(self, filepath, retainperms=True, mode=None, uid=-1, gid=-1):
dirpath = os.path.dirname(filepath)
if not os.path.exists(dirpath):
os.makedirs(dirpath)
- # Create empty file
+ # Create an empty tempfile in the target directory.
+ writetarget = filepath + '.functmp'
+
try:
- fo = open(filepath, 'w')
+ fo = open(writetarget, 'w')
fo.close()
del fo
except (IOError, OSError), e:
# XXX logger output here
return -1
- try:
- # we could intify the mode here if it's a string
- if mode:
- os.chmod(filepath, mode)
- if uid != -1 or gid != -1:
- os.chown(filepath, uid, gid)
- except (IOError, OSError), e:
- return -1
+ if retainperms:
+ # Make the attributes of the tempfile match the target file.
+ if os.path.isfile(filepath):
+ try:
+ # Copy the mode of the existing file.
+ shutil.copystat(filepath,writetarget)
+
+ # Copy the uid/gid of the existing file.
+ fileinfo = os.stat(filepath)
+ os.chown(writetarget,fileinfo[4],fileinfo[5])
- return filepath
+ # Copy the file attributes (lsattr) of the existing file.
+ from subprocess import call, Popen, PIPE, STDOUT
+ command_string = "lsattr " + filepath + " | cut -d ' ' -f 1 | sed -s s/\-//g"
+ lsattr_result = Popen(command_string, shell=True, stdout=PIPE, stderr=None, close_fds=True)
+ file_attribs = lsattr_result.stdout.read()
+ attribstring = "=" + file_attribs.rstrip('\n')
+ chattr_result = call(["chattr", attribstring, writetarget], stdout=None, stderr=None)
+
+ # Copy the SELinux attribs of the existing file.
+ refstring = "--reference=" + filepath
+ selinx_result = call(["chcon", refstring, writetarget], stdout=None, stderr=None)
+ except (IOError, OSError), e:
+ os.unlink(writetarget)
+ return -1
+
+ else:
+ # Set the mode and ownership of the file to match the server.
+ try:
+ # we could intify the mode here if it's a string
+ if mode:
+ os.chmod(writetarget, mode)
+ if uid != -1 or gid != -1:
+ os.chown(writetarget, uid, gid)
+ except (IOError, OSError), e:
+ os.unlink(writetarget)
+ return -1
- def append(self, filepath, filebuf):
- if not os.path.exists(filepath):
- # file disaperead
+ return writetarget
+
+ def append(self, open_result, filebuf):
+ hostname = func_utils.get_hostname_by_route()
+ writetarget = open_result[hostname]
+
+ # If the result of the open function was 0 (or -1), do not append, return the same value.
+ if type(writetarget) == type(0):
+ return writetarget
+
+ if not os.path.exists(writetarget):
+ # file disappeared
return -1
try:
- fo = open(filepath, 'a')
+ fo = open(writetarget, 'a')
fo.write(filebuf.data)
fo.close()
del fo
except (IOError, OSError), e:
- # XXX logger output here
+ # Clean up by removing the tempfile, then return an error.
+ os.unlink(writetarget)
return -1
- return 1
+ return writetarget
+
+ def move(self, filepath, append_result):
+ hostname = func_utils.get_hostname_by_route()
+ writetarget = append_result[hostname]
+
+ # If the result of the append function was 0 (or -1), just return the same value.
+ if type(writetarget) == type(0):
+ return writetarget
+ if not os.path.exists(writetarget):
+ # file disappeared
+ return -1
+
+ try:
+ shutil.move(writetarget,filepath)
+ except (IOError, OSError), e:
+ # Clean up by removing the tempfile, then return an error.
+ os.unlink(writetarget)
+ return -1
+
+ return 1
def copyfile(self, filepath, filebuf, mode=0644, uid=0, gid=0, force=None):
# -1 = problem file was not copied
@@ -159,3 +217,5 @@ class CopyFile(func_module.FuncModule):
#XXX logger output here
return False
return True
+
+
diff --git a/func/overlord/cmd_modules/copyfile.py b/func/overlord/cmd_modules/copyfile.py
index 3f2263d..3796515 100644
--- a/func/overlord/cmd_modules/copyfile.py
+++ b/func/overlord/cmd_modules/copyfile.py
@@ -38,6 +38,8 @@ class CopyFile(base_command.BaseCommand):
action="store_true")
self.parser.add_option("-v", "--verbose", dest="verbose",
action="store_true")
+ self.parser.add_option("", "--retainperms", dest="retainperms",
+ action="store_true")
def handleOptions(self, options):
self.verbose = self.options.verbose
@@ -50,4 +52,4 @@ class CopyFile(base_command.BaseCommand):
self.server_spec = self.parentCommand.server_spec
self.getOverlord()
- return self.overlord_obj.local.copyfile.send(self.options.filename, self.options.remotepath)
+ return self.overlord_obj.local.copyfile.send(self.options.filename, self.options.remotepath, self.options.retainperms)
diff --git a/func/overlord/modules/copyfile.py b/func/overlord/modules/copyfile.py
index 9f385a9..e9c0a1d 100644
--- a/func/overlord/modules/copyfile.py
+++ b/func/overlord/modules/copyfile.py
@@ -5,9 +5,10 @@ import sys
import xmlrpclib
from func.overlord import overlord_module
+import func.minion.modules.copyfile as CopyFile
class copyfile(overlord_module.BaseModule):
- def send(self, localpath, remotepath, bufsize=60000):
+ def send(self, localpath, remotepath, retainperms, bufsize=60000):
try:
f = open(localpath, "r")
except IOError, e:
@@ -19,13 +20,15 @@ class copyfile(overlord_module.BaseModule):
uid = st.st_uid
gid = st.st_gid
- self.parent.run("copyfile", "open", [remotepath, mode, uid, gid])
+ open_result = self.parent.run("copyfile", "open", [remotepath, retainperms, mode, uid, gid])
while True:
data=f.read(bufsize)
if data:
- self.parent.run("copyfile", "append", [remotepath, xmlrpclib.Binary(data)])
+ append_result = self.parent.run("copyfile", "append", [open_result, xmlrpclib.Binary(data)])
else:
break
+ self.parent.run("copyfile", "move", [remotepath, append_result])
+
return True
--
1.7.7
>From adc1748d8491695ca5ea6b05292647ed2904a9ed Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Mon, 17 Sep 2012 18:20:11 -0400
Subject: [PATCH 2/5] CopyFile: try using pyxattr to copy SELinux contexts.
---
func/minion/modules/copyfile.py | 25 +++++++++++++++++++------
1 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index 5e47c1d..956eb8d 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -82,22 +82,35 @@ class CopyFile(func_module.FuncModule):
# Copy the uid/gid of the existing file.
fileinfo = os.stat(filepath)
os.chown(writetarget,fileinfo[4],fileinfo[5])
+ except (IOError, OSError), e:
+ os.unlink(writetarget)
+ return -1
- # Copy the file attributes (lsattr) of the existing file.
+ # Copy the file attributes (lsattr) of the existing file.
+ try:
from subprocess import call, Popen, PIPE, STDOUT
command_string = "lsattr " + filepath + " | cut -d ' ' -f 1 | sed -s s/\-//g"
lsattr_result = Popen(command_string, shell=True, stdout=PIPE, stderr=None, close_fds=True)
file_attribs = lsattr_result.stdout.read()
attribstring = "=" + file_attribs.rstrip('\n')
chattr_result = call(["chattr", attribstring, writetarget], stdout=None, stderr=None)
-
- # Copy the SELinux attribs of the existing file.
- refstring = "--reference=" + filepath
- selinx_result = call(["chcon", refstring, writetarget], stdout=None, stderr=None)
except (IOError, OSError), e:
- os.unlink(writetarget)
+ os.unlink(writetarget)
return -1
+ # Copy the SELinux attribs of the existing file.
+ try:
+ import xattr
+ SELinuxAttribs = xattr.get(filepath, 'security.selinux')
+ xattr.set(writetarget, SELinuxAttribs)
+ except (IOError, OSError), e:
+ try:
+ refstring = "--reference=" + filepath
+ selinx_result = call(["chcon", refstring, writetarget], stdout=None, stderr=None)
+ except (IOError, OSError), e:
+ os.unlink(writetarget)
+ return -1
+
else:
# Set the mode and ownership of the file to match the server.
try:
--
1.7.7
>From 9cb81d2cdc0e896265328aae57271129f831085c Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Mon, 17 Sep 2012 18:57:15 -0400
Subject: [PATCH 3/5] CopyFile: bugfixes for pyxattr usage.
---
func/minion/modules/copyfile.py | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index 956eb8d..986c2b5 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -94,7 +94,7 @@ class CopyFile(func_module.FuncModule):
file_attribs = lsattr_result.stdout.read()
attribstring = "=" + file_attribs.rstrip('\n')
chattr_result = call(["chattr", attribstring, writetarget], stdout=None, stderr=None)
- except (IOError, OSError), e:
+ except (IOError, OSError, ImportError), e:
os.unlink(writetarget)
return -1
@@ -102,8 +102,8 @@ class CopyFile(func_module.FuncModule):
try:
import xattr
SELinuxAttribs = xattr.get(filepath, 'security.selinux')
- xattr.set(writetarget, SELinuxAttribs)
- except (IOError, OSError), e:
+ xattr.set(writetarget, 'security.selinux', SELinuxAttribs)
+ except (IOError, OSError, ImportError), e:
try:
refstring = "--reference=" + filepath
selinx_result = call(["chcon", refstring, writetarget], stdout=None, stderr=None)
--
1.7.7
>From 1c235e8e040203c9d8188f78c2a0ac2f84a6eef7 Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Tue, 18 Sep 2012 19:09:29 -0400
Subject: [PATCH 4/5] CopyFile: fixes to using pyattr, detect SELinux support
before using chcon
---
func/minion/modules/copyfile.py | 24 ++++++++++++++----------
1 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index 986c2b5..cd79c6c 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -27,6 +27,7 @@ import shutil
import func_module
from func import utils as func_utils
+from subprocess import call, Popen, PIPE, STDOUT
class CopyFile(func_module.FuncModule):
@@ -88,7 +89,6 @@ class CopyFile(func_module.FuncModule):
# Copy the file attributes (lsattr) of the existing file.
try:
- from subprocess import call, Popen, PIPE, STDOUT
command_string = "lsattr " + filepath + " | cut -d ' ' -f 1 | sed -s s/\-//g"
lsattr_result = Popen(command_string, shell=True, stdout=PIPE, stderr=None, close_fds=True)
file_attribs = lsattr_result.stdout.read()
@@ -98,18 +98,22 @@ class CopyFile(func_module.FuncModule):
os.unlink(writetarget)
return -1
- # Copy the SELinux attribs of the existing file.
+ # If the system supports extended attributes, copy them too.
try:
import xattr
- SELinuxAttribs = xattr.get(filepath, 'security.selinux')
- xattr.set(writetarget, 'security.selinux', SELinuxAttribs)
+ AllAttribs = xattr.get_all(filepath)
+ for Eachattr in AllAttribs:
+ xattr.set(writetarget, Eachattr[0], Eachattr[1])
except (IOError, OSError, ImportError), e:
- try:
- refstring = "--reference=" + filepath
- selinx_result = call(["chcon", refstring, writetarget], stdout=None, stderr=None)
- except (IOError, OSError), e:
- os.unlink(writetarget)
- return -1
+ # Unfortunately each different type of xattr probably needs its own command.
+ # Here is code for copying SELinux contexts.
+ if os.path.exists('/etc/sysconfig/selinux') or os.path.exists('/etc/selinux/config'):
+ try:
+ refstring = "--reference=" + filepath
+ selinx_result = call(["chcon", refstring, writetarget], stdout=None, stderr=None)
+ except (IOError, OSError), e:
+ os.unlink(writetarget)
+ return -1
else:
# Set the mode and ownership of the file to match the server.
--
1.7.7
>From ae16c54a3d0b83eabae1b63ccbaafacca01f0b00 Mon Sep 17 00:00:00 2001
From: Marcus Lauer <marcuslauer@xxxxxxxxxx>
Date: Tue, 18 Sep 2012 19:50:53 -0400
Subject: [PATCH 5/5] CopyFile: determine whether SELinux is in use, try chcon
if pyattr fails to copy SELinux contexts.
---
func/minion/modules/copyfile.py | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/func/minion/modules/copyfile.py b/func/minion/modules/copyfile.py
index cd79c6c..86d12e5 100644
--- a/func/minion/modules/copyfile.py
+++ b/func/minion/modules/copyfile.py
@@ -99,15 +99,23 @@ class CopyFile(func_module.FuncModule):
return -1
# If the system supports extended attributes, copy them too.
+ if os.path.exists('/etc/sysconfig/selinux') or os.path.exists('/etc/selinux/config'):
+ SupportsSelinux = 1
+ else:
+ SupportsSelinux = 0
+
try:
import xattr
AllAttribs = xattr.get_all(filepath)
+ if ( len(AllAttribs) == 0 ) and ( SupportsSelinux == 1 ):
+ # If the system supports SELinux then it should return at least one Attrib set.
+ raise IOError
for Eachattr in AllAttribs:
xattr.set(writetarget, Eachattr[0], Eachattr[1])
except (IOError, OSError, ImportError), e:
# Unfortunately each different type of xattr probably needs its own command.
# Here is code for copying SELinux contexts.
- if os.path.exists('/etc/sysconfig/selinux') or os.path.exists('/etc/selinux/config'):
+ if ( SupportsSelinux == 1 ):
try:
refstring = "--reference=" + filepath
selinx_result = call(["chcon", refstring, writetarget], stdout=None, stderr=None)
--
1.7.7
_______________________________________________
func mailing list
func@xxxxxxxxxxxxxxxxxxxxxx
https://lists.fedorahosted.org/mailman/listinfo/func
