On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> Curently security labels can be of type 'dynamic' or 'static'.
> If no security label is given, then 'dynamic' is assumed. The
> current code takes advantage of this default, and avoids even
> saving <seclabel> elements with type='dynamic' to disk. This
> means if you temporarily change security driver, the guests
> can all still start.
>
> With the introduce of sVirt to LXC though, there needs to be
s/introduce/introduction/
> a new default of 'none' to allow unconfined LXC containers.
>
> This patch introduces two new security label types
>
> - default: the host configuration decides whether to run the
> guest with type 'none' or 'dynamic' at guest start
> - none: the guest will run unconfined by security policy
>
> The 'none' label type will obviously be undesirable for some
> deployments, so a new qemu.conf option allows a host admin to
> mandate confined guests. It is also possible to turn off default
> confinement
>
> security_default_confined = 1|0 (default == 1)
> security_require_confined = 1|0 (default == 0)
>
> * src/conf/domain_conf.c, src/conf/domain_conf.h: Add new
> seclabel types
> * src/security/security_manager.c, src/security/security_manager.h:
> Set default sec label types
> * src/security/security_selinux.c: Handle 'none' seclabel type
> * src/qemu/qemu.conf, src/qemu/qemu_conf.c, src/qemu/qemu_conf.h,
> src/qemu/libvirtd_qemu.aug: New security config options
> * src/qemu/qemu_driver.c: Tell security driver about default
> config
Fails 'make syntax-check':
po_check
--- ./po/POTFILES.in
+++ ./po/POTFILES.in
@@ -89,6 +89,7 @@
src/security/security_apparmor.c
src/security/security_dac.c
src/security/security_driver.c
+src/security/security_manager.c
src/security/security_selinux.c
src/security/virt-aa-helper.c
src/storage/parthelper.c
maint.mk: you have changed the set of files with translatable diagnostics;
apply the above patch
Fails 'make check':
seclabeltest.c: In function 'main':
seclabeltest.c:16:5: error: too few arguments to function
'virSecurityManagerNew'
../src/security/security_manager.h:34:23: note: declared here
I had to squash this in to get tests to pass:
diff --git i/po/POTFILES.in w/po/POTFILES.in
index ca1db70..9b699bd 100644
--- i/po/POTFILES.in
+++ w/po/POTFILES.in
@@ -89,6 +89,7 @@ src/secret/secret_driver.c
src/security/security_apparmor.c
src/security/security_dac.c
src/security/security_driver.c
+src/security/security_manager.c
src/security/security_selinux.c
src/security/virt-aa-helper.c
src/storage/parthelper.c
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index 07080ce..91b95e2 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -9852,7 +9852,7 @@ virSecurityLabelDefFormat(virBufferPtr buf,
virSecurityLabelDefPtr def)
virBufferAsprintf(buf, "<seclabel type='%s'",
sectype);
if (def->model)
- virBufferEscapeString(buf, " model='%s' ", def->model);
+ virBufferEscapeString(buf, " model='%s'", def->model);
virBufferAsprintf(buf, " relabel='%s'",
def->norelabel ? "no" : "yes");
diff --git i/tests/seclabeltest.c w/tests/seclabeltest.c
index 5d87789..fca76b9 100644
--- i/tests/seclabeltest.c
+++ w/tests/seclabeltest.c
@@ -13,7 +13,7 @@ main (int argc ATTRIBUTE_UNUSED, char **argv
ATTRIBUTE_UNUSED)
virSecurityManagerPtr mgr;
const char *doi, *model;
- mgr = virSecurityManagerNew(NULL, false);
+ mgr = virSecurityManagerNew(NULL, false, true, false);
if (mgr == NULL) {
fprintf (stderr, "Failed to start security driver");
exit (-1);
> ---
> src/conf/domain_conf.c | 81 +++++++++++++++++++++++----------------
> src/conf/domain_conf.h | 2 +
> src/qemu/libvirtd_qemu.aug | 2 +
> src/qemu/qemu.conf | 8 ++++
> src/qemu/qemu_conf.c | 11 +++++
> src/qemu/qemu_conf.h | 2 +
> src/qemu/qemu_driver.c | 7 +++-
> src/security/security_manager.c | 51 ++++++++++++++++++++++--
> src/security/security_manager.h | 8 +++-
> src/security/security_selinux.c | 28 ++++++++++----
> 10 files changed, 152 insertions(+), 48 deletions(-)
You also need to list the new XML in docs/schemas/domaincommon.rng,
document it in docs/formatdomain.html.in, and add tests for the new XML
being parsed correctly. I don't feel comfortable acking with that much
missing, so I'll need a v2; but I can proceed to give code review.
> @@ -2547,13 +2549,15 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
> "%s", _("missing security type"));
> goto error;
> }
> +
> def->type = virDomainSeclabelTypeFromString(p);
> VIR_FREE(p);
> - if (def->type < 0) {
> + if (def->type <= 0) {
> virDomainReportError(VIR_ERR_XML_ERROR,
> "%s", _("invalid security type"));
This explicitly rejects type='default', was that intentional (where you
_want_ the user to omit the attribute to get the default)? It matches
what we do elsewhere, but that does mean that you have to be careful in
documenting it this way (type='none|dynamic|static' or omitted type to
form the four possibilities).
> @@ -2574,13 +2578,23 @@ virSecurityLabelDefParseXML(virSecurityLabelDefPtr def,
> "%s", _("dynamic label type must use resource relabeling"));
> goto error;
> }
> + if (def->type == VIR_DOMAIN_SECLABEL_NONE &&
> + !def->norelabel) {
> + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s", _("dynamic label type must use not resource relabeling"));
Bad grammar in the error message, but even with that fixed, it still
doesn't make sense compared to the if conditional. I think you meant
something more like:
"resource relabeling not compatible with 'none' label type"
> + goto error;
> + }
> } else {
> - if (def->type == VIR_DOMAIN_SECLABEL_STATIC)
> + if (def->type == VIR_DOMAIN_SECLABEL_STATIC ||
> + def->type == VIR_DOMAIN_SECLABEL_NONE)
> def->norelabel = true;
> else
> def->norelabel = false;
> }
>
> + if (def->type == VIR_DOMAIN_SECLABEL_NONE)
> + return 0;
So we explicitly ignore any <label> sub-elements with type of none; I
can live with that.
> @@ -9810,37 +9824,41 @@ virDomainLifecycleDefFormat(virBufferPtr buf,
> }
>
>
> -static int
> -virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def,
> - unsigned int flags)
> +static void
> +virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def)
I like that you are dropping dependency on the flags argument.
I will note that this means that 'virsh save-image-edit' might end up
changing a save image file created pre-patch into a new form post-patch
even when there are no changes made by the user (but that's not the
first time we've done that). But as long as we aren't changing the
output from the input _from the same version of libvirt_, I'm okay if
upgrading the libvirt version causes a rewrite when editing an older
save image.
> {
> const char *sectype = virDomainSeclabelTypeToString(def->type);
> - int ret = -1;
>
> if (!sectype)
> - goto cleanup;
> + return;
>
> - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
> - !def->baselabel &&
> - (flags & VIR_DOMAIN_XML_INACTIVE)) {
> - /* This is the default for inactive xml, so nothing to output. */
The old way omitted output if you use the defaults, but still generated
output if you do this as short-hand input:
<seclabel>
<baselabel>...</baselabel>
</seclabel>
for the intended:
<seclabel type='dynamic' model='selinux' relabel='yes'>
<baselabel>...</baselabel>
</seclabel>
> - } else {
> - virBufferAsprintf(buf, "<seclabel type='%s' model='%s' relabel='%s'>\n",
> - sectype, def->model,
> - def->norelabel ? "no" : "yes");
> - virBufferEscapeString(buf, " <label>%s</label>\n",
> - def->label);
> - if (!def->norelabel)
> - virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n",
> - def->imagelabel);
> - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC)
> - virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n",
> - def->baselabel);
> - virBufferAddLit(buf, "</seclabel>\n");
> + if (def->type == VIR_DOMAIN_SECLABEL_DEFAULT)
> + return;
...whereas your new code fails to look for the presence of <baselabel>,
and would silently lose the user's input. I argue that the bug is in
your parse routine, and not your output routine. That is, if the user
omitted type, but then provides a <baselabel>, you need to explicitly
set type to dynamic rather than leaving it at default, so that the
output routine can blindly omit output when type is default precisely
because <baselabel> no longer maps to default.
> +
> + virBufferAsprintf(buf, "<seclabel type='%s'",
> + sectype);
> + if (def->model)
> + virBufferEscapeString(buf, " model='%s' ", def->model);
Lose the trailing space. Also, you can exploit the fact that
virBufferEscapeString does a NULL check for you, and lose the 'if
(def->model)' with no change in output.
> +
> + virBufferAsprintf(buf, " relabel='%s'",
> + def->norelabel ? "no" : "yes");
> +
> + if (def->type == VIR_DOMAIN_SECLABEL_NONE) {
> + virBufferAddLit(buf, "/>\n");
> + return;
> }
> - ret = 0;
> -cleanup:
> - return ret;
> +
> + virBufferAddLit(buf, ">\n");
> +
> + virBufferEscapeString(buf, " <label>%s</label>\n",
> + def->label);
> + if (!def->norelabel)
> + virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n",
> + def->imagelabel);
> + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC)
> + virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n",
> + def->baselabel);
> + virBufferAddLit(buf, "</seclabel>\n");
This can result in:
<seclabel type='dynamic' model='selinux' relabel='yes'>
</seclabel>
for an inactive domain. Do we want to go to any effort to shorten this to:
<seclabel type='dynamic' model='selinux' relabel='yes'/>
> +++ b/src/qemu/qemu.conf
> @@ -138,6 +138,14 @@
> #
> # security_driver = "selinux"
>
> +# If set to non-zero, then the default security labelling
Do we want the US spelling of labeling here?
> +# will make guests confined. If set to zero, then guests
> +# will be unconfined by default. Defaults to 1
> +# security_default_confined = 1
> +
> +# If set to non-zero, then attempts to create unconfined
> +# guests will be blocked. Defaults to zero.
> +# security_require_confined = 1
Consistency argues that we should use the digit rather than the name of
the number, and end with a full stop, as in:
Defaults to 1.
Defaults to 0.
(that is, both variable descriptions need a tweak, but for different
reasons)
> @@ -195,6 +197,15 @@ int qemudLoadDriverConfig(struct qemud_driver *driver,
> }
> }
>
> + p = virConfGetValue (conf, "security_default_confined");
> + CHECK_TYPE ("security_default_confined", VIR_CONF_LONG);
> + if (p) driver->securityDefaultConfined = p->l;
> +
> + p = virConfGetValue (conf, "security_require_confined");
> + CHECK_TYPE ("security_require_confined", VIR_CONF_LONG);
> + if (p) driver->securityRequireConfined = p->l;
Conversion of long to boolean. I guess this is copy-and-paste from
other places doing this, and also that since we require C99 it will be
safe. But in C89 projects, the gnulib replacement for <stdbool.h> means
that this is non-portable, even though it should work just fine with C99
bool. (That is, the gnulib replacement bool can compile (bool)256L to
false instead of the desired true, depending on your platform and
compiler). No need to fix it in your patch (if we do anything, it
should be a global style scrub of all offenders in this file in one go).
> @@ -246,12 +250,20 @@ SELinuxGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> _("cannot generate selinux context for %s"), mcs);
> goto cleanup;
> }
> + break;
> +
> + case VIR_DOMAIN_SECLABEL_NONE:
> + /* no op */
> + break;
Might be safer to put a default case that errors out, so that we catch
any future additions.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list