[libvirt] [PATCH] security: apparmor: Label externalDataStore

[Christian Ehrhardt]

On Fri, Oct 11, 2019 at 9:13 PM Cole Robinson <crobinso at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191209/2ba6a9c1/attachment-0001.htm>