[PATCH] replace lokkit for selinux settings (#815540)

[Brian C. Lane]

From: "Brian C. Lane"

lokkit is going to be going away. This adds our own configuration editor
class and switches to using that to edit the selinux setting.
---
 pyanaconda/iutil.py    |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
 pyanaconda/security.py |   12 ++++------
 2 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
index 398ef77..d6a8e76 100644
--- a/pyanaconda/iutil.py
+++ b/pyanaconda/iutil.py
@@ -28,6 +28,7 @@ import os.path
 import errno
 import subprocess
 import threading
+import tempfile

from flags import flags
 from constants import *
@@ -1081,3 +1082,56 @@ def dracut_eject(device):
     except Exception, e:
         log.error("Error writing dracut shutdown eject hook for %s: %s" % (device, e))

+class EditConfig(object):
+    """
+    Edit values in a configuration file without changing comments.
+    Supports KEY=VALUE lines and ignores everything else.
+    Does not allow adding or deleting keys.
+    Does not support duplicate key entries.
+
+    ec = EditConfig("/path/to/file")
+    ec.values is a dict of keys from the file, edit as needed then call:
+    ec.write()
+
+    """
+    def __init__(self, filename):
+        """ filename is the full path of the file to edit
+        """
+        self.filename = filename
+        self.values = {}
+        self.read()
+
+    def read(self):
+        """ Read the config file and store the key/value pairs
+        """
+        with open(self.filename) as f:
+            for line in f:
+                if len(line)>0 and line[0] in '#";':
+                    continue
+                (key, sep, var) = [x.strip() for x in line.partition("=")]
+                if key and sep == "=":
+                    self.values[key] = var
+
+    def write(self):
+        """ Rewrite the file with the new key/value pairs
+            pass-thru comments and lines without =
+        """
+        tmpf = tempfile.NamedTemporaryFile(mode="w", delete=False)
+        with open(self.filename) as f:
+            for line in f:
+                if len(line)>0 and line[0] in '#";':
+                    tmpf.write(line)
+                    continue
+                (key, sep, var) = [x.strip() for x in line.partition("=")]
+                if key in self.values and sep == "=":
+                    tmpf.write("%s=%s\n" % (key, self.values[key]))
+                    continue
+                tmpf.write(line)
+        tmpf.close()
+
+        # Move the temporary file (with 0600 permissions) over the top of the
+        # original and preserve the original's permissions
+        m = os.stat(self.filename)
+        shutil.move(tmpf.name, self.filename)
+        os.chmod(self.filename, m.st_mode)
+
diff --git a/pyanaconda/security.py b/pyanaconda/security.py
index 50d92e2..b7d8dcc 100644
--- a/pyanaconda/security.py
+++ b/pyanaconda/security.py
@@ -65,14 +65,12 @@ class Security:
             log.error("unknown selinux state: %s" %(self.selinux,))
             return

-        args = args + [ "--selinux=%s" %(selinux_states[self.selinux],) ]
-
         try:
-            iutil.execWithRedirect("/usr/sbin/lokkit", args,
-                                   root=ROOT_PATH, stdout="/dev/null",
-                                   stderr="/dev/null")
-        except (RuntimeError, OSError) as msg:
-            log.error ("lokkit run failed: %s" %(msg,))
+            selinux_cfg = iutil.EditConfig(ROOT_PATH+"/etc/selinux/config")
+            selinux_cfg.values["SELINUX"] = selinux_states[self.selinux]
+            selinux_cfg.write()
+        except IOError as msg:
+            log.error ("Error setting selinux mode: %s" %(msg,))

args = ["--update", "--nostart"] + shlex.split(self.auth)
         if self._addFingerprint():

[Jesse Keating]

On 06/28/2012 12:12 PM, Brian C. Lane wrote:
> From: "Brian C. Lane"
>
> lokkit is going to be going away. This adds our own configuration editor
> class and switches to using that to edit the selinux setting.

The iutil code certainly looks like something that should exist 
somewhere, but I suppose our narrow use of it will keep things easy to 
debug.

+1

[Chris Lumens]

> +class EditConfig(object):
> """
> Edit values in a configuration file without changing comments.
> Supports KEY=VALUE lines and ignores everything else.
> Does not allow adding or deleting keys.
> Does not support duplicate key entries.
> ec = EditConfig("/path/to/file")
> ec.values is a dict of keys from the file, edit as needed then call:
> ec.write()
> """

Is python's ConfigParser insufficient for this purpose?

- Chris

[Brian C. Lane]

On Fri, Jun 29, 2012 at 09:28:55AM -0400, Chris Lumens wrote:
> > +class EditConfig(object):
> > """
> > Edit values in a configuration file without changing comments.
> > Supports KEY=VALUE lines and ignores everything else.
> > Does not allow adding or deleting keys.
> > Does not support duplicate key entries.
> > ec = EditConfig("/path/to/file")
> > ec.values is a dict of keys from the file, edit as needed then call:
> > ec.write()
> > """
>
> Is python's ConfigParser insufficient for this purpose?

ConfigParser operates on .ini style config files, not shell style. In
this case we just want to change an option in a shell-like config file
without disrupting the rest of it. wwoods also suggested that the shlex
module might be useful, but I think that's overkill for what we need.

[Will Woods]

The concept is fine, but there's a couple bugs. See comments below.
> +class EditConfig(object):

Kind of a weird class name. Maybe ShellConfig, or maybe we should just
replace SimpleConfig?
> def read(self):
>    """ Read the config file and store the key/value pairs
>    """
>    with open(self.filename) as f:
>        for line in f:
>            if len(line)>0 and line[0] in '#";':
>                continue

Bug. len(line) will never be 0, because empty lines will come out as
"\n". readline() is tricky like that.

Also, this line won't be considered a comment: '  #blah=funk\n'

Add a 'line = line.strip()' first?
>            (key, sep, var) = [x.strip() for x in line.partition("=")]

(With the strip happening above, you wouldn't need it here.)
>            if key and sep == "=":

For clarity, can you change this so the test on 'key' is more explicit?
That line looks like it means: if (key == '=') and (sep == '=').
Maybe:
    if key != '' and sep == '=':
Or:
    if sep == '=' and key:

Either way would be clearer, I think.
>            self.values[key] = var

This is going to leave any quoting that's in var, so if quotes are used
the value will be something like: '"fun"'. Conversely, in write(), we're
not adding quotes if the value has spaces, so if you change a value to
'something with spaces in it', it's not gonna be quoted in the resulting
file, which will cause problems if the parser expects that.

Do we care about that stuff?
> def write(self):
>    """ Rewrite the file with the new key/value pairs
>        pass-thru comments and lines without =
>    """
>    tmpf = tempfile.NamedTemporaryFile(mode="w", delete=False)
>    with open(self.filename) as f:

We're reopening the file here? What if the file has been changed/removed
since it was read, or we want to read the config from a string?

It might be a good idea to save the lines in an array on read(), and
just iterate over the saved lines in write().

I actually wrote a ConfigParser-style thingy for this out of curiosity -
I'll send it along for discussion.

-w

Parsing shell-style config files (was Re: [PATCH] replace lokkit for selinux settings (#815540))

[Will Woods]

So here's my shellconfig.py, presented for comment / inspiration.

A couple notes:

* This is an interesting construct that I wasn't aware of:

def quote(s):
    for c in s:
        if c not in _safechars:
            break
    else:
        return s
    ...

Apparently, if you have an 'else:' block after 'for:', it gets executed
if you 'break' out of the loop. Neat!

But _parseline is really the interesting part:

def _parseline(self, line):
        s = line.strip()
        if '#' in s:
            s = s[:s.find('#')] # remove from comment to EOL
            s = s.strip()       # and any unnecessary whitespace
        key, eq, val = s.partition('=')
        if self.read_unquote:
            val = unquote(val)
        if key != '' and eq == '=':
            return (key, val)
        else:
            return (None, None)

* You want to remove the comment before the partition(), otherwise this
line would end up with a weird value:

GRAPHICS="vnc"   # other choices: "spice", "none"

* quote() and unquote() use pipes._safechars and shlex.split(),
respectively.

* pipes.quote() doesn't work because it puts single-quotes around '$',
assuming that the given string is a literal filename

* There's no write() method but it's pretty obvious - you'd just open a
file and do f.write(str(config)) or similar.

Hope that's useful to somebody,

-w

Parsing shell-style config files (was Re: [PATCH] replace lokkit for selinux settings (#815540))

[Jesse Keating]

On 06/29/2012 01:29 PM, Will Woods wrote:
> Apparently, if you have an 'else:' block after 'for:', it gets executed
> if you 'break' out of the loop. Neat!

Er, no...  Reading the documentation at 
http://docs.python.org/reference/compound_stmts.html#the-for-statement 
if you break, you'll by-pass the else: clause.  If you want to get out 
of the loop but still run through the else you have to use 'continue'.

[Brian C. Lane]

On Fri, Jun 29, 2012 at 03:27:08PM -0400, Will Woods wrote:
> The concept is fine, but there's a couple bugs. See comments below.
> > +class EditConfig(object):
>
> Kind of a weird class name. Maybe ShellConfig, or maybe we should just
> replace SimpleConfig?

No, it Edits a Config file. Nothing more. It isn't used to create config
files or add/delete from them. Just edits the values.
> > def read(self):
> >    """ Read the config file and store the key/value pairs
> >    """
> >    with open(self.filename) as f:
> >        for line in f:
> >            if len(line)>0 and line[0] in '#";':
> >                continue
>
> Bug. len(line) will never be 0, because empty lines will come out as
> "\n". readline() is tricky like that.

Well, not a bug, just not needed.
> Also, this line won't be considered a comment: '  #blah=funk\n'
>
> Add a 'line = line.strip()' first?

Probably so.
> >            (key, sep, var) = [x.strip() for x in line.partition("=")]
>
> (With the strip happening above, you wouldn't need it here.)

This strip also removes spaces from around var.
> if sep == '=' and key:
>
> Either way would be clearer, I think.

Swapping them makes it a bit clearer.
> >            self.values[key] = var
>
> This is going to leave any quoting that's in var, so if quotes are used
> the value will be something like: '"fun"'. Conversely, in write(), we're
> not adding quotes if the value has spaces, so if you change a value to
> 'something with spaces in it', it's not gonna be quoted in the resulting
> file, which will cause problems if the parser expects that.
>
> Do we care about that stuff?

That was on purpose, if the variable needs to be quoted then reading
should leave them in and changing it should make sure it has quotes.
> > def write(self):
> >    """ Rewrite the file with the new key/value pairs
> >        pass-thru comments and lines without =
> >    """
> >    tmpf = tempfile.NamedTemporaryFile(mode="w", delete=False)
> >    with open(self.filename) as f:
>
> We're reopening the file here? What if the file has been changed/removed
> since it was read, or we want to read the config from a string?
>
> It might be a good idea to save the lines in an array on read(), and
> just iterate over the saved lines in write().
>
> I actually wrote a ConfigParser-style thingy for this out of curiosity -
> I'll send it along for discussion.

If you hold a copy in memory and re-write it you may end up stomping on
top of changes from something else. If you have two things fighting over
the same file you're going to lose one way or the other. My primary goal
for this class was to make a minimum of changes to the file in order to
change a variable, not provide a full-featured configuration editing
class.

It could be slightly improved by only writing variables that have been
changed, but for our limited needs in Anaconda I don't think it needs
it.