6:54 p.m.
V2 of
https://www.redhat.com/archives/libvir-list/2017-August/msg00376.html
The first patch fixes a few issues I found in securityselinuxtest while
working on the second patch. In V2, a seclabel is not generated
if the domain has no seclabel and confinement is disabled.
Jim Fehlig (2):
Fix building domain def in securityselinuxtest
Don't autogenerate seclabels of type 'none'
src/security/security_manager.c | 42 +++++++++++++++++++++--------------------
tests/securityselinuxtest.c | 14 +++++++++-----
2 files changed, 31 insertions(+), 25 deletions(-)
--
2.13.1
6:54 p.m.
New subject: [libvirt] [PATCH V2 1/2] Fix building domain def in securityselinuxtest
The virDomainDef created by testBuildDomainDef in securityselinuxtest
adds a seclabel but does not increment nseclabels. Also, it should
populate seclabel->model with 'selinux'.
While at it, use the secdef itself to populate values instead of
the indirection through def->seclabels[0].
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
tests/securityselinuxtest.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c
index 767b6cc02..f6143fb06 100644
--- a/tests/securityselinuxtest.c
+++ b/tests/securityselinuxtest.c
@@ -73,24 +73,28 @@ testBuildDomainDef(bool dynamic,
if (!(def = virDomainDefNew()))
goto error;
+ def->virtType = VIR_DOMAIN_VIRT_KVM;
if (VIR_ALLOC_N(def->seclabels, 1) < 0)
goto error;
+ def->nseclabels++;
+
if (VIR_ALLOC(secdef) < 0)
goto error;
- def->virtType = VIR_DOMAIN_VIRT_KVM;
- def->seclabels[0] = secdef;
- def->seclabels[0]->type = dynamic ? VIR_DOMAIN_SECLABEL_DYNAMIC :
VIR_DOMAIN_SECLABEL_STATIC;
+ if (VIR_STRDUP(secdef->model, "selinux") < 0)
+ goto error;
+ secdef->type = dynamic ? VIR_DOMAIN_SECLABEL_DYNAMIC :
VIR_DOMAIN_SECLABEL_STATIC;
if (label &&
- VIR_STRDUP(def->seclabels[0]->label, label) < 0)
+ VIR_STRDUP(secdef->label, label) < 0)
goto error;
if (baselabel &&
- VIR_STRDUP(def->seclabels[0]->baselabel, baselabel) < 0)
+ VIR_STRDUP(secdef->baselabel, baselabel) < 0)
goto error;
+ def->seclabels[0] = secdef;
return def;
error:
--
2.13.1
2:55 a.m.
New subject: [libvirt] [PATCH V2 1/2] Fix building domain def in securityselinuxtest
On Wed, Aug 16, 2017 at 05:54:07PM -0600, Jim Fehlig wrote:
The virDomainDef created by testBuildDomainDef in
securityselinuxtest
adds a seclabel but does not increment nseclabels. Also, it should
populate seclabel->model with 'selinux'.
While at it, use the secdef itself to populate values instead of
the indirection through def->seclabels[0].
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
tests/securityselinuxtest.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c
index 767b6cc02..f6143fb06 100644
--- a/tests/securityselinuxtest.c
+++ b/tests/securityselinuxtest.c
@@ -73,24 +73,28 @@ testBuildDomainDef(bool dynamic,
if (!(def = virDomainDefNew()))
goto error;
+ def->virtType = VIR_DOMAIN_VIRT_KVM;
if (VIR_ALLOC_N(def->seclabels, 1) < 0)
goto error;
+ def->nseclabels++;
Personally, I'd put the increment after assigning secdef to the domain
definition, but the memory chunk is zeroed out, so it doesn't matter at all.
ACK
Erik
10:39 a.m.
New subject: [libvirt] [PATCH V2 1/2] Fix building domain def in securityselinuxtest
On 08/17/2017 01:55 AM, Erik Skultety wrote:
On Wed, Aug 16, 2017 at 05:54:07PM -0600, Jim Fehlig wrote:
> The virDomainDef created by testBuildDomainDef in securityselinuxtest
> adds a seclabel but does not increment nseclabels. Also, it should
> populate seclabel->model with 'selinux'.
>
> While at it, use the secdef itself to populate values instead of
> the indirection through def->seclabels[0].
>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> tests/securityselinuxtest.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tests/securityselinuxtest.c b/tests/securityselinuxtest.c
> index 767b6cc02..f6143fb06 100644
> --- a/tests/securityselinuxtest.c
> +++ b/tests/securityselinuxtest.c
> @@ -73,24 +73,28 @@ testBuildDomainDef(bool dynamic,
> if (!(def = virDomainDefNew()))
> goto error;
>
> + def->virtType = VIR_DOMAIN_VIRT_KVM;
> if (VIR_ALLOC_N(def->seclabels, 1) < 0)
> goto error;
>
> + def->nseclabels++;
Personally, I'd put the increment after assigning secdef to the domain
definition, but the memory chunk is zeroed out, so it doesn't matter at all.
I prefer it after the assignment too and moved it there.
ACK
Thanks! Pushed now.
Regards,
Jim
6:54 p.m.
New subject: [libvirt] [PATCH V2 2/2] Don't autogenerate seclabels of type 'none'
When security drivers are active but confinement is not enabled,
there is no need to autogenerate <seclabel> elements when starting
a domain def that contains no <seclabel> elements. In fact,
autogenerating the elements can result in needless save/restore and
migration failures when the security driver is not active on the
restore/migration target.
This patch changes the virSecurityManagerGenLabel function in
src/security_manager.c to only autogenerate a <seclabel> element
if none is already defined for the domain *and* default
confinement is enabled. Otherwise the needless <seclabel>
autogeneration is skipped.
Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
V2: Don't autogenerate a seclabel if domain does not contain one
and confinement is disabled.
src/security/security_manager.c | 42 +++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 013bbc37e..10515c314 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -650,30 +650,32 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
for (i = 0; sec_managers[i]; i++) {
generated = false;
seclabel = virDomainDefGetSecurityLabelDef(vm,
sec_managers[i]->drv->name);
- if (!seclabel) {
- if (!(seclabel = virSecurityLabelDefNew(sec_managers[i]->drv->name)))
- goto cleanup;
- generated = seclabel->implicit = true;
- }
+ if (seclabel) {
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) {
+ if (virSecurityManagerGetDefaultConfined(sec_managers[i])) {
+ seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
+ } else {
+ seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
+ seclabel->relabel = false;
+ }
+ }
- if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) {
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE) {
+ if (virSecurityManagerGetRequireConfined(sec_managers[i])) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Unconfined guests are not allowed on this
host"));
+ goto cleanup;
+ }
+ }
+ } else {
+ /* Only generate seclabel if confinement is enabled */
if (virSecurityManagerGetDefaultConfined(sec_managers[i])) {
+ if (!(seclabel =
virSecurityLabelDefNew(sec_managers[i]->drv->name)))
+ goto cleanup;
+ generated = seclabel->implicit = true;
seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
} else {
- seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
- seclabel->relabel = false;
- }
- }
-
- if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE) {
- if (virSecurityManagerGetRequireConfined(sec_managers[i])) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Unconfined guests are not allowed on this
host"));
- goto cleanup;
- } else if (vm->nseclabels && generated) {
- VIR_DEBUG("Skipping auto generated seclabel of type none");
- virSecurityLabelDefFree(seclabel);
- seclabel = NULL;
+ VIR_DEBUG("Skipping auto generated seclabel");
continue;
}
}
--
2.13.1
3:17 a.m.
New subject: [libvirt] [PATCH V2 2/2] Don't autogenerate seclabels of type 'none'
On Wed, Aug 16, 2017 at 05:54:08PM -0600, Jim Fehlig wrote:
When security drivers are active but confinement is not enabled,
there is no need to autogenerate <seclabel> elements when starting
a domain def that contains no <seclabel> elements. In fact,
autogenerating the elements can result in needless save/restore and
migration failures when the security driver is not active on the
restore/migration target.
This patch changes the virSecurityManagerGenLabel function in
src/security_manager.c to only autogenerate a <seclabel> element
if none is already defined for the domain *and* default
confinement is enabled. Otherwise the needless <seclabel>
autogeneration is skipped.
Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
V2: Don't autogenerate a seclabel if domain does not contain one
and confinement is disabled.
src/security/security_manager.c | 42 +++++++++++++++++++++--------------------
1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 013bbc37e..10515c314 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -650,30 +650,32 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
for (i = 0; sec_managers[i]; i++) {
generated = false;
seclabel = virDomainDefGetSecurityLabelDef(vm,
sec_managers[i]->drv->name);
- if (!seclabel) {
- if (!(seclabel = virSecurityLabelDefNew(sec_managers[i]->drv->name)))
- goto cleanup;
- generated = seclabel->implicit = true;
- }
+ if (seclabel) {
Just a tiny nitpick, generally we prefer the 'if' block to be shorter than the
corresponding 'else' block.
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) {
+ if (virSecurityManagerGetDefaultConfined(sec_managers[i])) {
+ seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
+ } else {
+ seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
+ seclabel->relabel = false;
+ }
+ }
- if (seclabel->type == VIR_DOMAIN_SECLABEL_DEFAULT) {
+ if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE) {
+ if (virSecurityManagerGetRequireConfined(sec_managers[i])) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Unconfined guests are not allowed on this
host"));
+ goto cleanup;
+ }
+ }
+ } else {
+ /* Only generate seclabel if confinement is enabled */
if (virSecurityManagerGetDefaultConfined(sec_managers[i])) {
The same applies to this nested if-else block.
+ if (!(seclabel =
virSecurityLabelDefNew(sec_managers[i]->drv->name)))
+ goto cleanup;
+ generated = seclabel->implicit = true;
seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
} else {
- seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
- seclabel->relabel = false;
- }
- }
-
- if (seclabel->type == VIR_DOMAIN_SECLABEL_NONE) {
- if (virSecurityManagerGetRequireConfined(sec_managers[i])) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Unconfined guests are not allowed on this
host"));
- goto cleanup;
- } else if (vm->nseclabels && generated) {
- VIR_DEBUG("Skipping auto generated seclabel of type none");
- virSecurityLabelDefFree(seclabel);
- seclabel = NULL;
+ VIR_DEBUG("Skipping auto generated seclabel");
continue;
}
}
ACK with the adjustment.
Erik
1:18 a.m.
New subject: [libvirt] [PATCH V2 2/2] Don't autogenerate seclabels of type 'none'
On 08/17/2017 02:17 AM, Erik Skultety wrote:
On Wed, Aug 16, 2017 at 05:54:08PM -0600, Jim Fehlig wrote:
> When security drivers are active but confinement is not enabled,
> there is no need to autogenerate <seclabel> elements when starting
> a domain def that contains no <seclabel> elements. In fact,
> autogenerating the elements can result in needless save/restore and
> migration failures when the security driver is not active on the
> restore/migration target.
>
> This patch changes the virSecurityManagerGenLabel function in
> src/security_manager.c to only autogenerate a <seclabel> element
> if none is already defined for the domain *and* default
> confinement is enabled. Otherwise the needless <seclabel>
> autogeneration is skipped.
>
> Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
>
> V2: Don't autogenerate a seclabel if domain does not contain one
> and confinement is disabled.
>
> src/security/security_manager.c | 42 +++++++++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 013bbc37e..10515c314 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -650,30 +650,32 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
> for (i = 0; sec_managers[i]; i++) {
> generated = false;
> seclabel = virDomainDefGetSecurityLabelDef(vm,
sec_managers[i]->drv->name);
> - if (!seclabel) {
> - if (!(seclabel =
virSecurityLabelDefNew(sec_managers[i]->drv->name)))
> - goto cleanup;
> - generated = seclabel->implicit = true;
> - }
> + if (seclabel) {
Just a tiny nitpick, generally we prefer the 'if' block to be shorter than the
corresponding 'else' block.
Thanks for the review! Sorry, but I'm not sure what you mean by "shorter".
Do
you mean the 'if' block should contain fewer lines of code than the 'else'
block?
Regards,
Jim
2:44 a.m.
New subject: [libvirt] [PATCH V2 2/2] Don't autogenerate seclabels of type 'none'
On Fri, Aug 18, 2017 at 12:18:20AM -0600, Jim Fehlig wrote:
On 08/17/2017 02:17 AM, Erik Skultety wrote:
> On Wed, Aug 16, 2017 at 05:54:08PM -0600, Jim Fehlig wrote:
> > When security drivers are active but confinement is not enabled,
> > there is no need to autogenerate <seclabel> elements when starting
> > a domain def that contains no <seclabel> elements. In fact,
> > autogenerating the elements can result in needless save/restore and
> > migration failures when the security driver is not active on the
> > restore/migration target.
> >
> > This patch changes the virSecurityManagerGenLabel function in
> > src/security_manager.c to only autogenerate a <seclabel> element
> > if none is already defined for the domain *and* default
> > confinement is enabled. Otherwise the needless <seclabel>
> > autogeneration is skipped.
> >
> > Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
> > Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> > ---
> >
> > V2: Don't autogenerate a seclabel if domain does not contain one
> > and confinement is disabled.
> >
> > src/security/security_manager.c | 42
+++++++++++++++++++++--------------------
> > 1 file changed, 22 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> > index 013bbc37e..10515c314 100644
> > --- a/src/security/security_manager.c
> > +++ b/src/security/security_manager.c
> > @@ -650,30 +650,32 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
> > for (i = 0; sec_managers[i]; i++) {
> > generated = false;
> > seclabel = virDomainDefGetSecurityLabelDef(vm,
sec_managers[i]->drv->name);
> > - if (!seclabel) {
> > - if (!(seclabel =
virSecurityLabelDefNew(sec_managers[i]->drv->name)))
> > - goto cleanup;
> > - generated = seclabel->implicit = true;
> > - }
> > + if (seclabel) {
>
> Just a tiny nitpick, generally we prefer the 'if' block to be shorter than
the
> corresponding 'else' block.
Thanks for the review! Sorry, but I'm not sure what you mean by "shorter".
Do you mean the 'if' block should contain fewer lines of code than the
'else' block?
Yeah, that's exactly what I meant, I'm sorry, not being a native speaker once
again resulted in a poor choice of wording :/. Next time I'll do better in
expressing myself more precisely, I promise :).
Erik
Regards,
Jim
10:44 a.m.
New subject: [libvirt] [PATCH V2 2/2] Don't autogenerate seclabels of type 'none'
On 08/18/2017 01:44 AM, Erik Skultety wrote:
On Fri, Aug 18, 2017 at 12:18:20AM -0600, Jim Fehlig wrote:
> On 08/17/2017 02:17 AM, Erik Skultety wrote:
>> On Wed, Aug 16, 2017 at 05:54:08PM -0600, Jim Fehlig wrote:
>>> When security drivers are active but confinement is not enabled,
>>> there is no need to autogenerate <seclabel> elements when starting
>>> a domain def that contains no <seclabel> elements. In fact,
>>> autogenerating the elements can result in needless save/restore and
>>> migration failures when the security driver is not active on the
>>> restore/migration target.
>>>
>>> This patch changes the virSecurityManagerGenLabel function in
>>> src/security_manager.c to only autogenerate a <seclabel> element
>>> if none is already defined for the domain *and* default
>>> confinement is enabled. Otherwise the needless <seclabel>
>>> autogeneration is skipped.
>>>
>>> Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
>>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>>> ---
>>>
>>> V2: Don't autogenerate a seclabel if domain does not contain one
>>> and confinement is disabled.
>>>
>>> src/security/security_manager.c | 42
+++++++++++++++++++++--------------------
>>> 1 file changed, 22 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/src/security/security_manager.c
b/src/security/security_manager.c
>>> index 013bbc37e..10515c314 100644
>>> --- a/src/security/security_manager.c
>>> +++ b/src/security/security_manager.c
>>> @@ -650,30 +650,32 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
>>> for (i = 0; sec_managers[i]; i++) {
>>> generated = false;
>>> seclabel = virDomainDefGetSecurityLabelDef(vm,
sec_managers[i]->drv->name);
>>> - if (!seclabel) {
>>> - if (!(seclabel =
virSecurityLabelDefNew(sec_managers[i]->drv->name)))
>>> - goto cleanup;
>>> - generated = seclabel->implicit = true;
>>> - }
>>> + if (seclabel) {
>>
>> Just a tiny nitpick, generally we prefer the 'if' block to be shorter
than the
>> corresponding 'else' block.
>
> Thanks for the review! Sorry, but I'm not sure what you mean by
"shorter".
> Do you mean the 'if' block should contain fewer lines of code than the
> 'else' block?
Yeah, that's exactly what I meant, I'm sorry, not being a native speaker once
again resulted in a poor choice of wording :/. Next time I'll do better in
expressing myself more precisely, I promise :).
I think you were pretty clear, its just that I've been around libvirt a long
time and don't recall hearing such a preference. I was simply double checking :-).
I've pushed this one after making the suggested change.
Regards,
Jim
2651
days inactive
2656
days old
8 comments
2 participants
participants (2)
-
Erik Skultety -
Jim Fehlig