[Ovirt-devel] [PATCH node] add ovirt semodule in Node

[Jim Meyering]

Perry Myers <pmyers at redhat.com> wrote:

> For now, it is only to allow qemu to access disk partitions directly,
> required in order to use iSCSI storage pools with SELinux enabled.
>
> Signed-off-by: Alan Pevec <apevec at redhat.com>
>
> Moved from ovirt-node-image repository as it should be in the node
> RPM since that RPM is used for creating nodes from stock Fedora
> installs and this policy needs to be set there as well.  Added check
> for selinuxenabled before making the change.
>
> This is necessary to make clalance's patches for allowing the appliance
> to manage the host it is running on as a Node.
>
> FIXME: This patch is going in to fix the problem, but we should be using
> http://fedoraproject.org/wiki/PackagingDrafts/SELinux/PolicyModules
>
> Signed-off-by: Perry Myers <pmyers at redhat.com>
> ---
>  ovirt-listen-awake/ovirt-install-node |   22 ++++++++++++++++++++--
>  1 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/ovirt-listen-awake/ovirt-install-node b/ovirt-listen-awake/ovirt-install-node
> index 84c7b14..68f5b37 100644
> --- a/ovirt-listen-awake/ovirt-install-node
> +++ b/ovirt-listen-awake/ovirt-install-node
> @@ -25,7 +25,7 @@ add_if_not_exist() {
>      file="$2"
>
>      grep -qE "^[[:space:]]*$string($|#|[[:space:]])" "$file" \
> -	|| echo "$string" >> "$file"
> +        || echo "$string" >> "$file"
>  }
>
>  if [ "$1" = "stateless" ]; then
> @@ -70,7 +70,7 @@ elif [ "$1" = "stateful" ]; then
>      read yesno
>
>      if [ "$yesno" != "y" -a "$yesno" != "Y" ]; then
> -	exit 2
> +        exit 2
>      fi
>
>      mkdir -p $OVIRT_BACKUP_DIR
> @@ -100,3 +100,21 @@ elif [ "$1" = "stateful" ]; then
>  else
>      usage
>  fi
> +
> +# Common to both stateless and stateful Nodes
> +
> +if $(selinuxenabled) ; then

No need for a subshell: you can use this syntax:

   if selinuxenabled ; then

> +    # make disks available to VMs
> +    cat > /tmp/ovirt.te <<EOF

It's good to get into the habit of using quoted here scripts
when nothing in the body needs to be $var or ``-expanded, i.e.,

       cat > /tmp/ovirt.te <<\EOF

> +module ovirt 1.0.0;
> +require {
> +    type fixed_disk_device_t;
> +    type qemu_t;
> +    class blk_file { ioctl getattr setattr read write };
> +}
> +allow qemu_t fixed_disk_device_t:blk_file { ioctl getattr setattr read write };
> +EOF
> +    checkmodule -M -m -o /tmp/ovirt.mod /tmp/ovirt.te
> +    semodule_package -o /tmp/ovirt.pp -m /tmp/ovirt.mod
> +    semodule -i /tmp/ovirt.pp
> +fi

It probably isn't worth adding 3 uses of mktemp
but factoring out 6 uses of
/tmp/ovirt might win points:

----------------------
        t=/tmp/ovirt
        cat > $t.te <<\EOF
        ...
EOF
    checkmodule -M -m -o $t.mod $t.te
    semodule_package -o $t.pp -m $t.mod
    semodule -i $t.pp
----------------------

At first I was going to say no symlink-in-/tmp attack is possible,
because no one else is around...  But if some bad guy with shell access
to a stateful node knows this script will be running at next boot,
and /tmp will persist (I don't know about that), creating a symlink,
/tmp/ovirt.te, pointing to /etc/passwd would cause trouble.

So using mktemp might be a good idea after all:

        t=$(mktemp -d)
        cat > $t/te <<\EOF
        ...
EOF
    checkmodule -M -m -o $t/mod $t/te
    semodule_package -o $t/pp -m $t/mod
    semodule -i $t/pp