restrict folder and file permissions created by libvirt-provider

[lukas016]

Proposed Changes

Folders permissions were restricted from 0777 to 0750
File permissions were restricted from 0666 to 0640 or 0660

Libvirtd has still access into folder.
Depends on setup current permissions can be problematic. Libvirtd is running as root in my current setup.

@hardikdr @lukasfrank could you retest it in your environment before merge?

Setup of environment will have to been properly documented in follow up issue.

.libvirt-provider/:
total 12
drwxr-x--- 4 lkoszegy developers 4096 Mar  1 11:40 images
drwxr-x--- 3 lkoszegy developers 4096 Mar  1 11:40 machines
drwxr-x--- 3 lkoszegy developers 4096 Mar  1 11:40 store

.libvirt-provider/images:
total 16
drwxr-xr-x 3 lkoszegy developers 4096 Mar  1 11:40 blobs
-rw-r--r-- 1 lkoszegy developers  464 Mar  1 11:41 index.json
drwxr-xr-x 2 lkoszegy developers 4096 Mar  1 11:41 ingest
-rw-r--r-- 1 lkoszegy developers   30 Mar  1 11:40 oci-layout

.libvirt-provider/images/blobs:
total 4
drwxr-xr-x 2 lkoszegy developers 4096 Mar  1 11:41 sha256

.libvirt-provider/images/blobs/sha256:
total 1211616
-r--r--r-- 1 libvirt-qemu libvirt-qemu   29939712 Mar  1 11:41 31de6bef6bec213c9bd7c7f04c6108e1aed775f717a2b9f0c108ac951af51c91
-r--r--r-- 1 lkoszegy     developers          718 Mar  1 11:41 4d4d0e52fa23a0cce90abc11cd27715e67caf44882027d490755b300ad7afa91
-r--r--r-- 1 lkoszegy     developers          209 Mar  1 11:40 9074eb95a07ed98222ffc0c94a3e6577b972ce62073fe4d14c32d005700f1aba
-r--r--r-- 1 libvirt-qemu libvirt-qemu   13258496 Mar  1 11:41 aa287755b872d982ba71294d377fae9c9b90b36d684e031b2fa197d30ac366b7
-r--r--r-- 1 lkoszegy     developers   1197473792 Mar  1 11:41 f5f836e857a67a97239a38717f2963211a2e7b2220d1c97643a1a3e84b5d45c6

.libvirt-provider/images/ingest:
total 0

.libvirt-provider/machines:
total 4
drwxr-x--- 6 lkoszegy developers 4096 Mar  1 11:40 d51edf2b-bae0-42d9-89e4-511e604869c4

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4:
total 16
drwxr-x--- 2 lkoszegy developers 4096 Mar  1 11:41 ignitions
drwxr-x--- 2 lkoszegy developers 4096 Mar  1 11:40 networkinterfaces
drwxr-x--- 2 lkoszegy developers 4096 Mar  1 11:41 rootfs
drwxr-x--- 3 lkoszegy developers 4096 Mar  1 11:41 volumes

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4/ignitions:
total 8
-rw-r----- 1 libvirt-qemu libvirt-qemu 3827 Mar  1 11:41 data.ign

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4/networkinterfaces:
total 0

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4/rootfs:
total 1169412
-rw-rw---- 1 libvirt-qemu libvirt-qemu 1197473792 Mar  1 11:41 rootfs

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4/volumes:
total 4
drwxr-x--- 4 lkoszegy developers 4096 Mar  1 11:41 libvirt-provider.ironcore.dev~empty-disk

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4/volumes/libvirt-provider.ironcore.dev~empty-disk:
total 8
drwxr-x--- 2 lkoszegy developers 4096 Mar  1 11:41 ephe-disk
drwxr-x--- 2 lkoszegy developers 4096 Mar  1 11:41 ephe-disk2

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4/volumes/libvirt-provider.ironcore.dev~empty-disk/ephe-disk:
total 4
-rw-rw---- 1 libvirt-qemu libvirt-qemu 5368709120 Mar  1 11:41 disk.raw

.libvirt-provider/machines/d51edf2b-bae0-42d9-89e4-511e604869c4/volumes/libvirt-provider.ironcore.dev~empty-disk/ephe-disk2:
total 4
-rw-rw---- 1 libvirt-qemu libvirt-qemu 530000000 Mar  1 11:41 disk.raw

.libvirt-provider/store:
total 4
drwxr-x--- 2 lkoszegy developers 4096 Mar  1 11:40 machines

.libvirt-provider/store/machines:
total 8
-rw-r----- 1 lkoszegy developers 6953 Mar  1 11:41 d51edf2b-bae0-42d9-89e4-511e604869c4

Fixes #197

[afritzler]

Those file permission constants are all over the place. If we want to use constants here, can't we put them globally into a central FS related file?

[lukas016]

Those file permission constants are all over the place. If we want to use constants here, can't we put them globally into a central FS related file?

It is possible and @so-sahu had the same recommendation. I am personally more against it. Main reason are:

• We have to be careful with import into constants package becasue we cannot create loop in imports (it isn't big problem)
• It can create bad pattern design, where developers will put all constants into one place or we will end with weird mix what will affect using of code.
• Developers can use constants automatically without thinking, if they are using the best options. (Example: ignition file use different permissions as empty disk 0640 vs 0660)
• We have to be more careful with naming of constants, if they are exported.

I am not strongly against it but i prefer do it for small projects with few packages. Here i maybe keep current solution.

But if other people will have different opinion and can rewrite it, no problem.

[afritzler]

Defining constants for file permissions might be a dangerous thing - but then I would say we stick to the octal numbers directly and don't use constants at all. That way you know that this operation is using 0640 for write a certain file.

[lukas016]

Defining constants for file permissions might be a dangerous thing - but then I would say we stick to the octal numbers directly and don't use constants at all. That way you know that this operation is using 0640 for write a certain file.

More restricted linters can have problem with that, because they look at it as magic number and best practices for magic number is use variable and add some semantic with name for it. It can be simple solve with ignore specific error regexp.

But using constants have some benefits. Maybe good example how names adding semantic meaning in permFolder and permFile. folder needs execution for open of folder by default. file doesn't need that, but if file is binary you can create new constant permBinary and you will directly see why are you using this permission for file.

But i am okay with your suggestion but i want to hear more opinion. @so-sahu @lukasfrank @hardikdr

[so-sahu]

In my opinion, in a public project with permissions used in multiple places, it makes more sense to use constants with meaningful names. This approach offers several benefits:

1. Readability and Maintainability: Constants like permFolder and permFile make the code more understandable without needing to refer elsewhere.

2. Consistency: Using constants ensures consistent permission settings throughout the codebase, reducing the risk of errors.

3. Ease of Change: Constants allow for easy and centralized permission modifications if needed in the future.

4. Linting and Best Practices: Many linters and coding standards recommend using constants over magic numbers.

5. Semantic Meaning: Constants add semantic meaning, making it clear why specific permissions are set.

Adopting this approach will improve code readability, maintainability, and consistency.

[so-sahu]

Below is an in-depth analysis comparing centralizing file permission constants in one package with global environment variables v/s the current decentralized approach.

Suggestion 1: Centralizing File Permission Constants

• Pros:
  • Centralizing constants can improve maintainability by providing a single source of truth for file permission values.
  • It can enhance consistency in the naming and usage of constants across the codebase.
• Cons:
  • Importing constants from a central package can lead to circular dependencies if not managed carefully.
  • It may encourage developers to dump all constants into a single package, potentially leading to a bloated and less organized constants package.
  • Developers might use constants without considering the context, which could lead to incorrect permissions being used in certain scenarios.

Suggestion 2: Keep Constants Distributed

• Pros:
  • Keeping constants distributed allows each package to manage its own constants, promoting encapsulation and reducing the risk of circular dependencies.
  • It avoids creating a monolithic constants package, which can be harder to manage and maintain.
  • Developers are forced to think about the appropriate permissions for each specific use case, potentially leading to better code quality.
• Cons:
  • Naming conflicts might occur if different packages use similar constant names for different permissions.
  • Ensuring consistency in naming and usage across packages may require more manual effort.

In conclusion, for larger projects like libvirt-provider with multiple packages and potential for complex dependencies, keeping constants distributed can be a more practical approach. It promotes better encapsulation and allows each package to manage its own constants without relying heavily on a centralized package.