diff mbox series

[5/6] guestfs: add ansible group permisison check on libvirt system uri

Message ID 20250323115009.269172-6-mcgrof@kernel.org (mailing list archive)
State New
Headers show
Series debian / libvirt / devconfig fixes | expand

Checks

Context Check Description
mcgrof/vmtest-main-VM_Test-3 success Logs for Setup and Run Make Targets (debian:testing)
mcgrof/vmtest-main-VM_Test-7 success Logs for Setup and Run Make Targets (opensuse/tumbleweed)
mcgrof/vmtest-main-VM_Test-4 success Logs for Setup and Run Make Targets (fedora:latest)
mcgrof/vmtest-main-VM_Test-2 success Logs for Setup and Run Make Targets (debian:testing)
mcgrof/vmtest-main-VM_Test-6 success Logs for Setup and Run Make Targets (opensuse/tumbleweed)
mcgrof/vmtest-main-VM_Test-5 success Logs for Setup and Run Make Targets (fedora:latest)
mcgrof/vmtest-main-PR fail PR summary
mcgrof/vmtest-main-VM_Test-1 fail Logs for Run kdevops CI
mcgrof/vmtest-main-VM_Test-0 fail Logs for Run kdevops CI

Commit Message

Luis Chamberlain March 23, 2025, 11:50 a.m. UTC 
The bringup process for libvirt system URI support (not session),
so all debian based distros, requieres us to be paranoid about the
permissions of our storage directory where we place our libvirt
storage pool, and guestfs images.

We used to be stupid and were hammering with a sledge hammer a crazy
sudo chown -R on a target storage path. That was removed by commit
c31459dc384c ("scripts/bringup_guestfs.sh: fix silly directory permission
fix"). I rushed that change in because it was affecting live systems
and we needed to get testing moving.

This adds some sanity checks which don't do the crazy wild permission
checks, it will just fail if the permissions are not right.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kconfigs/Kconfig.guestfs                      | 10 ++++
 .../roles/bringup_guestfs/tasks/main.yml      | 59 +++++++++++++++++++
 scripts/bringup_guestfs.sh                    |  3 -
 scripts/guestfs.Makefile                      |  2 +-
 4 files changed, 70 insertions(+), 4 deletions(-)

Comments

Daniel Gomez March 25, 2025, 2:53 p.m. UTC | #1 
On Sun, Mar 23, 2025 at 04:50:08AM +0100, Luis Chamberlain wrote:
> The bringup process for libvirt system URI support (not session),
> so all debian based distros, requieres us to be paranoid about the
> permissions of our storage directory where we place our libvirt
> storage pool, and guestfs images.
> 
> We used to be stupid and were hammering with a sledge hammer a crazy
> sudo chown -R on a target storage path. That was removed by commit
> c31459dc384c ("scripts/bringup_guestfs.sh: fix silly directory permission
> fix"). I rushed that change in because it was affecting live systems
> and we needed to get testing moving.

That commit removes the permissions we don't want to force when host is shared
between users. However, the storage pool directory is created with:

$USE_SUDO mkdir -p $STORAGEDIR
$USE_SUDO mkdir -p $BASE_IMAGE_DIR

Which creates the pool directory with sudo command. The commit explains this
should result in:

    Essentially this now does:

    sudo chown -R foo.libvirt-qemu /path/to/storage/

Which I think is true as these series failed later at the new checks added
below with:

 TASK [bringup_guestfs : Verify storage pool path directory is group-writable
(libvirt system uri)] ***
fatal: [localhost]: FAILED! => {
    "changed": false
}

MSG:

The permissions for /xfs1/libvirt should be group
writeable by the group used by libvirt: libvirt-qemu
Current group: libvirt-qemu
Current permissions: drwxr-xr-x

Logs:
https://github.com/linux-kdevops/kdevops-kpd/actions/runs/14018680385/job/39247654400

Shall we ensure that the pool folder is created with group write permissions?
Or why we don't need to do that? It's unclear why the group had 'xr' permissions
but not 'w'.

Now, I've removed the folder state by pushing some patches for testing.

> 
> This adds some sanity checks which don't do the crazy wild permission
> checks, it will just fail if the permissions are not right.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  kconfigs/Kconfig.guestfs                      | 10 ++++
>  .../roles/bringup_guestfs/tasks/main.yml      | 59 +++++++++++++++++++
>  scripts/bringup_guestfs.sh                    |  3 -
>  scripts/guestfs.Makefile                      |  2 +-
>  4 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/kconfigs/Kconfig.guestfs b/kconfigs/Kconfig.guestfs
> index c6d2d1907dd5..d309436fa7c9 100644
> --- a/kconfigs/Kconfig.guestfs
> +++ b/kconfigs/Kconfig.guestfs
> @@ -5,6 +5,16 @@ config STORAGE_POOL_PATH
>  	output yaml
>  	default LIBVIRT_STORAGE_POOL_PATH
>  
> +config GUESTFS_STORAGE_DIR
> +	string
> +	output yaml
> +	default "{{ kdevops_storage_pool_path }}/kdevops/guestfs"
> +
> +config GUESTFS_BASE_IMAGE_DIR
> +	string
> +	output yaml
> +	default "{{ guestfs_storage_dir }}/base_images"
> +
>  config GUESTFS_HAS_CUSTOM_RAW_IMAGE
>  	bool
>  
> diff --git a/playbooks/roles/bringup_guestfs/tasks/main.yml b/playbooks/roles/bringup_guestfs/tasks/main.yml
> index dcbbaef02522..947d7dbc0b8b 100644
> --- a/playbooks/roles/bringup_guestfs/tasks/main.yml
> +++ b/playbooks/roles/bringup_guestfs/tasks/main.yml
> @@ -42,6 +42,65 @@
>    when: guestfs_subdirectories.matched == 0
>    tags: [ 'config-check' ]
>  
> +- name: Create kdevops guestfs storage directory if missing (libvirt session uri)
> +  file:
> +    path: "{{ guestfs_base_image_dir }}"
> +    state: directory
> +    mode: '0755'
> +  tags: ['storage-pool-path']
> +  when:
> +    - 'not libvirt_uri_system|bool'
> +
> +- name: Create kdevops guestfs storage directory if missing (libvirt system uri)
> +  become: yes
> +  become_flags: 'su - -c'
> +  become_method: sudo
> +  file:
> +    path: "{{ guestfs_base_image_dir }}"
> +    state: directory
> +    mode: '0775'
> +    group: "{{ libvirt_qemu_group }}"
> +  tags: ['storage-pool-path']
> +  when:
> +    - 'libvirt_uri_system|bool'
> +
> +- name: Check if directory is owned by the correct group (libvirt system uri)
> +  become: yes
> +  become_flags: 'su - -c'
> +  become_method: sudo
> +  command: stat -c '%G' "{{ libvirt_storage_pool_path }}"
> +  register: dir_group
> +  changed_when: false
> +  tags: ['storage-pool-path']
> +  when:
> +    - 'libvirt_uri_system|bool'
> +
> +- name: Check if directory has group write permissions (libvirt system uri)
> +  become: yes
> +  become_flags: 'su - -c'
> +  become_method: sudo
> +  command: stat -c '%A' "{{ libvirt_storage_pool_path }}"
> +  register: dir_perms
> +  changed_when: false
> +  tags: ['storage-pool-path']
> +  when:
> +    - 'libvirt_uri_system|bool'
> +
> +- name: Verify storage pool path directory is group-writable (libvirt system uri)
> +  become: yes
> +  become_flags: 'su - -c'
> +  become_method: sudo
> +  fail:
> +    msg: |
> +      The permissions for {{ libvirt_storage_pool_path }} should be group
> +      writeable by the group used by libvirt: {{ libvirt_qemu_group }}
> +      Current group: {{ dir_group.stdout }}
> +      Current permissions: {{ dir_perms.stdout }}
> +  tags: ['storage-pool-path']
> +  when:
> +    - 'libvirt_uri_system|bool'
> +    - (dir_group.stdout != libvirt_qemu_group) or (dir_perms.stdout[5] != 'w')
> +
>  - name: Check for dnsmasq configuration files
>    stat:
>      path: "{{ item }}"
> diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> index 976d1e78ed6a..bc0176f8f5b4 100755
> --- a/scripts/bringup_guestfs.sh
> +++ b/scripts/bringup_guestfs.sh
> @@ -271,9 +271,6 @@ if [[ "$CONFIG_LIBVIRT_URI_SYSTEM" == "y" ]]; then
>  	USE_SUDO="sudo "
>  fi
>  
> -$USE_SUDO mkdir -p $STORAGEDIR
> -$USE_SUDO mkdir -p $BASE_IMAGE_DIR
> -
>  cmdfile=$(mktemp)
>  
>  if [ ! -f $BASE_IMAGE ]; then
> diff --git a/scripts/guestfs.Makefile b/scripts/guestfs.Makefile
> index d08e697f3cfb..e1cf25d62d04 100644
> --- a/scripts/guestfs.Makefile
> +++ b/scripts/guestfs.Makefile
> @@ -83,7 +83,7 @@ bringup_guestfs: $(GUESTFS_BRINGUP_DEPS)
>  		playbooks/bringup_guestfs.yml \
>  		-e 'ansible_python_interpreter=/usr/bin/python3' \
>  		--extra-vars=@./extra_vars.yaml \
> -		--tags config-check,network
> +		--tags config-check,network,storage-pool-path
>  	$(Q)$(TOPDIR)/scripts/bringup_guestfs.sh
>  	$(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
>  		--inventory localhost, \
> -- 
> 2.47.2
>
Luis Chamberlain March 29, 2025, 9:55 p.m. UTC | #2 
On Tue, Mar 25, 2025 at 03:53:09PM +0100, Daniel Gomez wrote:
> Shall we ensure that the pool folder is created with group write permissions?

Yes.

That's a needed fix in this series.

  Luis
Luis Chamberlain March 29, 2025, 10:43 p.m. UTC | #3 
On Sat, Mar 29, 2025 at 02:55:27PM -0700, Luis Chamberlain wrote:
> On Tue, Mar 25, 2025 at 03:53:09PM +0100, Daniel Gomez wrote:
> > Shall we ensure that the pool folder is created with group write permissions?
> 
> Yes.
> 
> That's a needed fix in this series.

I'll replace the verify check for system session with just these two
tasks way above:

- name: Create storage pool path directory if using libvirt session URI
  file:
    path: "{{ libvirt_storage_pool_path }}"
    state: directory
    mode: "0775"
  when: libvirt_use_session_uri | default(false) | bool
  tags:
    - vars

- name: Create storage pool path directory and set group if using libvirt system URI
  file:
    path: "{{ libvirt_storage_pool_path }}"
    state: directory
    owner: root
    group: "{{ libvirt_qemu_group }}"
    mode: "0775"
  when: not libvirt_use_session_uri | default(false) | bool
  tags:
    - vars
Luis Chamberlain March 29, 2025, 10:55 p.m. UTC | #4 
On Sat, Mar 29, 2025 at 03:43:07PM -0700, Luis Chamberlain wrote:
> On Sat, Mar 29, 2025 at 02:55:27PM -0700, Luis Chamberlain wrote:
> > On Tue, Mar 25, 2025 at 03:53:09PM +0100, Daniel Gomez wrote:
> > > Shall we ensure that the pool folder is created with group write permissions?
> > 
> > Yes.
> > 
> > That's a needed fix in this series.
> 
> I'll replace the verify check for system session with just these two
> tasks way above:

Meh, I mean this (after testing this, I verified the check can stay, as
its just a check):

diff --git a/playbooks/roles/bringup_guestfs/tasks/main.yml b/playbooks/roles/bringup_guestfs/tasks/main.yml
index 947d7dbc0b8b..0b193dad807f 100644
--- a/playbooks/roles/bringup_guestfs/tasks/main.yml
+++ b/playbooks/roles/bringup_guestfs/tasks/main.yml
@@ -42,6 +42,23 @@
   when: guestfs_subdirectories.matched == 0
   tags: [ 'config-check' ]
 
+- name: Create storage pool path directory if (libvirt session uri)
+  file:
+    path: "{{ libvirt_storage_pool_path }}"
+    state: directory
+  when: 'not libvirt_uri_system|bool'
+  tags: ['storage-pool-path']
+
+- name: Create storage pool path directory and set group if using (libvirt system uri)
+  file:
+    path: "{{ libvirt_storage_pool_path }}"
+    state: directory
+    owner: root
+    group: "{{ libvirt_qemu_group }}"
+    mode: "0775"
+  when: 'libvirt_uri_system|bool'
+  tags: ['storage-pool-path']
+
 - name: Create kdevops guestfs storage directory if missing (libvirt session uri)
   file:
     path: "{{ guestfs_base_image_dir }}"
diff mbox series

Patch

diff --git a/kconfigs/Kconfig.guestfs b/kconfigs/Kconfig.guestfs
index c6d2d1907dd5..d309436fa7c9 100644
--- a/kconfigs/Kconfig.guestfs
+++ b/kconfigs/Kconfig.guestfs
@@ -5,6 +5,16 @@  config STORAGE_POOL_PATH
 	output yaml
 	default LIBVIRT_STORAGE_POOL_PATH
 
+config GUESTFS_STORAGE_DIR
+	string
+	output yaml
+	default "{{ kdevops_storage_pool_path }}/kdevops/guestfs"
+
+config GUESTFS_BASE_IMAGE_DIR
+	string
+	output yaml
+	default "{{ guestfs_storage_dir }}/base_images"
+
 config GUESTFS_HAS_CUSTOM_RAW_IMAGE
 	bool
 
diff --git a/playbooks/roles/bringup_guestfs/tasks/main.yml b/playbooks/roles/bringup_guestfs/tasks/main.yml
index dcbbaef02522..947d7dbc0b8b 100644
--- a/playbooks/roles/bringup_guestfs/tasks/main.yml
+++ b/playbooks/roles/bringup_guestfs/tasks/main.yml
@@ -42,6 +42,65 @@ 
   when: guestfs_subdirectories.matched == 0
   tags: [ 'config-check' ]
 
+- name: Create kdevops guestfs storage directory if missing (libvirt session uri)
+  file:
+    path: "{{ guestfs_base_image_dir }}"
+    state: directory
+    mode: '0755'
+  tags: ['storage-pool-path']
+  when:
+    - 'not libvirt_uri_system|bool'
+
+- name: Create kdevops guestfs storage directory if missing (libvirt system uri)
+  become: yes
+  become_flags: 'su - -c'
+  become_method: sudo
+  file:
+    path: "{{ guestfs_base_image_dir }}"
+    state: directory
+    mode: '0775'
+    group: "{{ libvirt_qemu_group }}"
+  tags: ['storage-pool-path']
+  when:
+    - 'libvirt_uri_system|bool'
+
+- name: Check if directory is owned by the correct group (libvirt system uri)
+  become: yes
+  become_flags: 'su - -c'
+  become_method: sudo
+  command: stat -c '%G' "{{ libvirt_storage_pool_path }}"
+  register: dir_group
+  changed_when: false
+  tags: ['storage-pool-path']
+  when:
+    - 'libvirt_uri_system|bool'
+
+- name: Check if directory has group write permissions (libvirt system uri)
+  become: yes
+  become_flags: 'su - -c'
+  become_method: sudo
+  command: stat -c '%A' "{{ libvirt_storage_pool_path }}"
+  register: dir_perms
+  changed_when: false
+  tags: ['storage-pool-path']
+  when:
+    - 'libvirt_uri_system|bool'
+
+- name: Verify storage pool path directory is group-writable (libvirt system uri)
+  become: yes
+  become_flags: 'su - -c'
+  become_method: sudo
+  fail:
+    msg: |
+      The permissions for {{ libvirt_storage_pool_path }} should be group
+      writeable by the group used by libvirt: {{ libvirt_qemu_group }}
+      Current group: {{ dir_group.stdout }}
+      Current permissions: {{ dir_perms.stdout }}
+  tags: ['storage-pool-path']
+  when:
+    - 'libvirt_uri_system|bool'
+    - (dir_group.stdout != libvirt_qemu_group) or (dir_perms.stdout[5] != 'w')
+
 - name: Check for dnsmasq configuration files
   stat:
     path: "{{ item }}"
diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
index 976d1e78ed6a..bc0176f8f5b4 100755
--- a/scripts/bringup_guestfs.sh
+++ b/scripts/bringup_guestfs.sh
@@ -271,9 +271,6 @@  if [[ "$CONFIG_LIBVIRT_URI_SYSTEM" == "y" ]]; then
 	USE_SUDO="sudo "
 fi
 
-$USE_SUDO mkdir -p $STORAGEDIR
-$USE_SUDO mkdir -p $BASE_IMAGE_DIR
-
 cmdfile=$(mktemp)
 
 if [ ! -f $BASE_IMAGE ]; then
diff --git a/scripts/guestfs.Makefile b/scripts/guestfs.Makefile
index d08e697f3cfb..e1cf25d62d04 100644
--- a/scripts/guestfs.Makefile
+++ b/scripts/guestfs.Makefile
@@ -83,7 +83,7 @@  bringup_guestfs: $(GUESTFS_BRINGUP_DEPS)
 		playbooks/bringup_guestfs.yml \
 		-e 'ansible_python_interpreter=/usr/bin/python3' \
 		--extra-vars=@./extra_vars.yaml \
-		--tags config-check,network
+		--tags config-check,network,storage-pool-path
 	$(Q)$(TOPDIR)/scripts/bringup_guestfs.sh
 	$(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
 		--inventory localhost, \