security: apparmor: Label externalDataStore

Message ID 49dfeac6e8fabdd92981ef01a3fc509f504543f4.1570821213.git.crobinso@redhat.com
State Superseded
Headers show
Series
  • security: apparmor: Label externalDataStore
Related show

Commit Message

Cole Robinson Oct. 11, 2019, 7:14 p.m.
Teach virt-aa-helper how to label a qcow2 data_file, tracked internally
as externalDataStore. It should be treated the same as its sibling
disk image

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
Compiled but not runtime tested, I don't have an apparmor setup

 src/security/virt-aa-helper.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Cole Robinson Dec. 9, 2019, 12:39 a.m. | #1
ping

On 10/11/19 3:14 PM, Cole Robinson wrote:
> Teach virt-aa-helper how to label a qcow2 data_file, tracked internally

> as externalDataStore. It should be treated the same as its sibling

> disk image

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

> Compiled but not runtime tested, I don't have an apparmor setup

> 

>  src/security/virt-aa-helper.c | 4 ++++

>  1 file changed, 4 insertions(+)

> 

> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c

> index 509187ac36..fe6fa12550 100644

> --- a/src/security/virt-aa-helper.c

> +++ b/src/security/virt-aa-helper.c

> @@ -949,6 +949,10 @@ storage_source_add_files(virStorageSourcePtr src,

>          if (add_file_path(tmp, depth, buf) < 0)

>              return -1;

>  

> +        if (src->externalDataStore &&

> +            storage_source_add_files(src->externalDataStore, buf, depth) < 0)

> +            return -1;

> +

>          depth++;

>      }

>  

>


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Christian Ehrhardt Dec. 9, 2019, 6:50 a.m. | #2
On Fri, Oct 11, 2019 at 9:13 PM Cole Robinson <crobinso@redhat.com> wrote:
>

> Teach virt-aa-helper how to label a qcow2 data_file, tracked internally

> as externalDataStore. It should be treated the same as its sibling

> disk image

>

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

> Compiled but not runtime tested, I don't have an apparmor setup

>

>  src/security/virt-aa-helper.c | 4 ++++

>  1 file changed, 4 insertions(+)

>

> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c

> index 509187ac36..fe6fa12550 100644

> --- a/src/security/virt-aa-helper.c

> +++ b/src/security/virt-aa-helper.c

> @@ -949,6 +949,10 @@ storage_source_add_files(virStorageSourcePtr src,

>          if (add_file_path(tmp, depth, buf) < 0)

>              return -1;

>

> +        if (src->externalDataStore &&

> +            storage_source_add_files(src->externalDataStore, buf, depth)

< 0)
> +            return -1;

> +


After thinking twice about it ack to carry over depth as only the first
layer should only ever need write permissions on those extra files as well.

But I'm unsure about using "src->externalDataStore".
After all this does initialize "tmp = src".
And then iterates the (potential) backing chain.
That way each loop iteration has a different "tmp" along the backing chain
but "src" will always point to the first entry.

It comes down to a detail question about the design of external data in
qcow.
In a case with a base file and two snapshots, does the chain look like:

A:
Current  -> Snap2 -> Snap1
  \
  external data

or
B:

Current    ->   Snap2    -> Snap1
  \               \           \
  ext-dat2      ext-dat1     ext-dat

or
C:

Current    ->   Snap2    -> Snap1
                              \
                             external data

Depending on that it should IMHO be
a) call storage_source_add_files outside the loop and with depth arg always
set to zero
b+c)  call storage_source_add_files with tmp instead of src as argument
(also use tmp in the check if externalDataStore exists)


>          depth++;

>      }

>

> --

> 2.23.0

>




-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
<div dir="ltr"><font face="monospace"><br><br>On Fri, Oct 11, 2019 at 9:13 PM Cole Robinson &lt;<a href="mailto:crobinso@redhat.com">crobinso@redhat.com</a>&gt; wrote:<br>&gt;<br>&gt; Teach virt-aa-helper how to label a qcow2 data_file, tracked internally<br>&gt; as externalDataStore. It should be treated the same as its sibling<br>&gt; disk image<br>&gt;<br>&gt; Signed-off-by: Cole Robinson &lt;<a href="mailto:crobinso@redhat.com">crobinso@redhat.com</a>&gt;<br>&gt; ---<br>&gt; Compiled but not runtime tested, I don&#39;t have an apparmor setup<br>&gt;<br>&gt;  src/security/virt-aa-helper.c | 4 ++++<br>&gt;  1 file changed, 4 insertions(+)<br>&gt;<br>&gt; diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c<br>&gt; index 509187ac36..fe6fa12550 100644<br>&gt; --- a/src/security/virt-aa-helper.c<br>&gt; +++ b/src/security/virt-aa-helper.c<br>&gt; @@ -949,6 +949,10 @@ storage_source_add_files(virStorageSourcePtr src,<br>&gt;          if (add_file_path(tmp, depth, buf) &lt; 0)<br>&gt;              return -1;<br>&gt;<br>&gt; +        if (src-&gt;externalDataStore &amp;&amp;<br>&gt; +            storage_source_add_files(src-&gt;externalDataStore, buf, depth) &lt; 0)<br>&gt; +            return -1;<br>&gt; +<br><br>After thinking twice about it ack to carry over depth as only the first layer should only ever need write permissions on those extra files as well.<br><br>But I&#39;m unsure about using &quot;src-&gt;externalDataStore&quot;.<br>After all this does initialize &quot;tmp = src&quot;.<br>And then iterates the (potential) backing chain.<br>That way each loop iteration has a different &quot;tmp&quot; along the backing chain but &quot;src&quot; will always point to the first entry.<br><br>It comes down to a detail question about the design of external data in qcow.<br>In a case with a base file and two snapshots, does the chain look like:<br><br>A:<br>Current  -&gt; Snap2 -&gt; Snap1<br>  \<br>  external data<br><br>or<br>B:<br><br>Current    -&gt;   Snap2    -&gt; Snap1<br>  \               \           \<br>  ext-dat2      ext-dat1     ext-dat</font><div><font face="monospace"><br></font></div><div><font face="monospace">or</font></div><div><font face="monospace">C:</font></div><div><font face="monospace"><br></font><font face="monospace">Current    -&gt;   Snap2    -&gt; Snap1<br>                              \<br>                             external data</font><div></div><font face="monospace"><br></font>Depending on that it should IMHO be<br>a) call storage_source_add_files outside the loop and with depth arg always set to zero<br>b+c)  call storage_source_add_files with tmp instead of src as argument (also use tmp in the check if externalDataStore exists)<br><br><div><br><font face="monospace">&gt;          depth++;</font><br><font face="monospace">&gt;      }</font><br><font face="monospace">&gt;</font><br><font face="monospace">&gt; --</font><br><font face="monospace">&gt; 2.23.0</font><br><font face="monospace">&gt;</font><br><br><br><br><font face="monospace">-- </font><br><font face="monospace">Christian Ehrhardt</font><br><font face="monospace">Staff Engineer, Ubuntu Server</font><br><font face="monospace">Canonical Ltd</font></div></div></div>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Peter Krempa Dec. 9, 2019, 6:59 a.m. | #3
On Mon, Dec 09, 2019 at 07:50:09 +0100, Christian Ehrhardt wrote:
> On Fri, Oct 11, 2019 at 9:13 PM Cole Robinson <crobinso@redhat.com> wrote:

> >

> > Teach virt-aa-helper how to label a qcow2 data_file, tracked internally

> > as externalDataStore. It should be treated the same as its sibling

> > disk image

> >

> > Signed-off-by: Cole Robinson <crobinso@redhat.com>

> > ---

> > Compiled but not runtime tested, I don't have an apparmor setup

> >

> >  src/security/virt-aa-helper.c | 4 ++++

> >  1 file changed, 4 insertions(+)

> >

> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c

> > index 509187ac36..fe6fa12550 100644

> > --- a/src/security/virt-aa-helper.c

> > +++ b/src/security/virt-aa-helper.c

> > @@ -949,6 +949,10 @@ storage_source_add_files(virStorageSourcePtr src,

> >          if (add_file_path(tmp, depth, buf) < 0)

> >              return -1;

> >

> > +        if (src->externalDataStore &&

> > +            storage_source_add_files(src->externalDataStore, buf, depth)

> < 0)

> > +            return -1;

> > +

> 

> After thinking twice about it ack to carry over depth as only the first

> layer should only ever need write permissions on those extra files as well.

> 

> But I'm unsure about using "src->externalDataStore".

> After all this does initialize "tmp = src".

> And then iterates the (potential) backing chain.

> That way each loop iteration has a different "tmp" along the backing chain

> but "src" will always point to the first entry.

> 

> It comes down to a detail question about the design of external data in

> qcow.

> In a case with a base file and two snapshots, does the chain look like:

> 

> A:

> Current  -> Snap2 -> Snap1

>   \

>   external data

> 

> or

> B:

> 

> Current    ->   Snap2    -> Snap1

>   \               \           \

>   ext-dat2      ext-dat1     ext-dat


This is the correct situation as the store is an per-image property. Any
level in the backing chain may or may not have it depending on how you
create the image.

The external data store write permission must mirror the permission of
the image it belongs to, so that if you are e.g. committing one of the
snapshots the backing store must be turned writable as qemu will write
the data into it.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Dec. 10, 2019, 12:55 a.m. | #4
On 12/9/19 1:50 AM, Christian Ehrhardt wrote:
> 
> 
> On Fri, Oct 11, 2019 at 9:13 PM Cole Robinson <crobinso@redhat.com
> <mailto:crobinso@redhat.com>> wrote:
>>
>> Teach virt-aa-helper how to label a qcow2 data_file, tracked internally
>> as externalDataStore. It should be treated the same as its sibling
>> disk image
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com
> <mailto:crobinso@redhat.com>>
>> ---
>> Compiled but not runtime tested, I don't have an apparmor setup
>>
>>  src/security/virt-aa-helper.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index 509187ac36..fe6fa12550 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -949,6 +949,10 @@ storage_source_add_files(virStorageSourcePtr src,
>>          if (add_file_path(tmp, depth, buf) < 0)
>>              return -1;
>>
>> +        if (src->externalDataStore &&
>> +            storage_source_add_files(src->externalDataStore, buf,
> depth) < 0)
>> +            return -1;
>> +
> 
> After thinking twice about it ack to carry over depth as only the first
> layer should only ever need write permissions on those extra files as well.
> 
> But I'm unsure about using "src->externalDataStore".
> After all this does initialize "tmp = src".
> And then iterates the (potential) backing chain.
> That way each loop iteration has a different "tmp" along the backing
> chain but "src" will always point to the first entry.
> 
> It comes down to a detail question about the design of external data in
> qcow.
> In a case with a base file and two snapshots, does the chain look like:
> 
> A:
> Current  -> Snap2 -> Snap1
>   \
>   external data
> 
> or
> B:
> 
> Current    ->   Snap2    -> Snap1
>   \               \           \
>   ext-dat2      ext-dat1     ext-dat
> 
> or
> C:
> 
> Current    ->   Snap2    -> Snap1
>                               \
>                              external data
> 
> Depending on that it should IMHO be
> a) call storage_source_add_files outside the loop and with depth arg
> always set to zero
> b+c)  call storage_source_add_files with tmp instead of src as argument
> (also use tmp in the check if externalDataStore exists)

Indeed, it should be using tmp instead of src, nicely spotted. I'll send
a v2. Thankfully the already committed selinux code got it right

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 509187ac36..fe6fa12550 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -949,6 +949,10 @@  storage_source_add_files(virStorageSourcePtr src,
         if (add_file_path(tmp, depth, buf) < 0)
             return -1;
 
+        if (src->externalDataStore &&
+            storage_source_add_files(src->externalDataStore, buf, depth) < 0)
+            return -1;
+
         depth++;
     }