Bottles icon indicating copy to clipboard operation
Bottles copied to clipboard

[Flatpak] Reduce filesystem permissions

Open jannuary opened this issue 2 years ago • 41 comments

(Moved from com.usebottles.bottles#120)

Currently, Bottles has a whopping 12 different filesystem permissions. Now that we use portals, it wouldn't hurt to go through different permission and reduce ones that aren't needed.

jannuary avatar Jul 30 '21 06:07 jannuary

With https://github.com/bottlesdevs/Bottles/pull/436 - all filesystem permissions that are related to letting a user open an arbitrary file can be removed. As far as Bottles is concerned.

Regarding Windows app that open/save files;

I see 3 options

  1. Make Bottles request rw permission on ~
  2. Remove all filesystem permissions and instruct users to use Flatseal if they need a permission for a Windows app
  3. Let users pick folders (via the portal) which are exposed to a Bottle via a symbolic link in the wine prefix (is that even possible?)

WDYT?

sonnyp avatar Aug 05 '21 12:08 sonnyp

With #436 - all filesystem permissions that are related to letting a user open an arbitrary file can be removed. As far as Bottles is concerned.

Regarding Windows app that open/save files;

I see 3 options

  1. Make Bottles request rw permission on ~
  2. Remove all filesystem permissions and instruct users to use Flatseal if they need a permission for a Windows app
  3. Let users pick folders (via the portal) which are exposed to a Bottle via a symbolic link in the wine prefix (is that even possible?)

WDYT?

Surely the second and third points are the best. What I don't understand is how it is possible that in my tests, even removing the permissions I am able to browse and pick files from the system using wine explorer.

mirkobrombin avatar Aug 05 '21 12:08 mirkobrombin

image

Maybe the sandbox is disabled by GNOME Builder or flatpak-builder?

mirkobrombin avatar Aug 05 '21 12:08 mirkobrombin

Maybe the sandbox is disabled by GNOME Builder or flatpak-builder?

Exactly

sonnyp avatar Aug 05 '21 12:08 sonnyp

Maybe the sandbox is disabled by GNOME Builder or flatpak-builder?

Exactly

This explains A LOT of things. It is time for me to start using more Flatpak packages.

mirkobrombin avatar Aug 05 '21 12:08 mirkobrombin

Yes, it took me a while to understand the subtleties of flatpak when I started targeting it. It was quite frustrating. I think the developer experience could be better there.

When I want to test/share the real deal, I build a bundle

flatpak-builder --user  --force-clean --repo=repo --install-deps-from=flathub flatpak com.usebottles.bottles.json
flatpak build-bundle repo Bottles.flatpak com.usebottles.bottles --runtime-repo=https://flathub.org/repo/flathub.flatpakrepo
flatpak --user install Bottles.flatpak

GNOME Builder also offers this IIRC

EDIT:

Builds have the --allow=devel and --allow=multiarch permissions that regular flatpak runs don't have by default. This allows limits the syscall filtering that is normally done so development tools like debuggers work.

https://docs.flatpak.org/en/latest/flatpak-builder-command-reference.html#idm1400

sonnyp avatar Aug 05 '21 12:08 sonnyp

Yes, it took me a while to understand the subtleties of flatpak when I started targeting it. It was quite frustrating. I think the developer experience could be better there.

When I want to test the real deal, I build a bundle

flatpak-builder --user  --force-clean --repo=repo --install-deps-from=flathub flatpak com.usebottles.bottles.json
flatpak build-bundle repo Bottles.flatpak com.usebottles.bottles --runtime-repo=https://flathub.org/repo/flathub.flatpakrepo
flatpak --user install Bottles.flatpak

GNOME Builder also offers this IIRC

EDIT:

Builds have the --allow=devel and --allow=multiarch permissions that regular flatpak runs don't have by default. This allows limits the syscall filtering that is normally done so development tools like debuggers work.

https://docs.flatpak.org/en/latest/flatpak-builder-command-reference.html#idm1400

Thanks for the informations I understand their decision to add the devel flag but this is a big limitation for debugging :S

mirkobrombin avatar Aug 05 '21 13:08 mirkobrombin

We agree :smile:

Also, it was unclear to me that devel flag involve full fs access. It should at least be explicit.

Please do create an issue - I'll happily share my similar experience there as well https://github.com/flatpak/flatpak-builder/issues/new

sonnyp avatar Aug 05 '21 13:08 sonnyp

We agree

Also, it was unclear to me that devel flag involve full fs access. It should at least be explicit.

Please do create an issue - I'll happily share my similar experience there as well https://github.com/flatpak/flatpak-builder/issues/new

Great idea, I'll do it

mirkobrombin avatar Aug 05 '21 13:08 mirkobrombin

#130 is a requirement to consider dropping the following permissions

        "--filesystem=~/.Bottles:ro",
        "--filesystem=~/.local/share/bottles:ro",

note that Bottles will probably need a 'migrator' so even after #130 - these permissions might be always required

@mirkobrombin what is --filesystem=~/wineprefixes:ro for?

sonnyp avatar Aug 05 '21 14:08 sonnyp

#130 is a requirement to consider dropping the following permissions

        "--filesystem=~/.Bottles:ro",
        "--filesystem=~/.local/share/bottles:ro",

note that Bottles will probably need a 'migrator' so even after #130 - these permissions might be always required

@mirkobrombin what is --filesystem=~/wineprefixes:ro for?

.Bottles, Games, .wine and wineprefixes are used by the wineprefix importer. From these can be safely removed .Bottles (old path where we stored bottles in v1), wineprefixes and .wine.

Btw I just saw that .PlayOnLinux is missing and it is needed by the importer.

mirkobrombin avatar Aug 05 '21 14:08 mirkobrombin

Safely as in "app won't crash" or safely as in "won't break any feature"?

What is a user wants to impot a prefix from .wine ?

sonnyp avatar Aug 05 '21 14:08 sonnyp

Safely as in "app won't crash" or safely as in "won't break any feature"?

Safely as: "won't break anything and it is not necessary to import prefixes from there"

What is a user wants to impot a prefix from .wine ?

That is the default path of the prefix created by wine. The user might want to bring it to Bottles but even though we offer this functionality, it is not officially supported and there are very few cases where a 'bottled' prefix works.

mirkobrombin avatar Aug 05 '21 14:08 mirkobrombin

From these can be safely removed .Bottles (old path where we stored bottles in v1)

What about https://github.com/bottlesdevs/Bottles#backward-compatibility-triumph ?


Screenshot from 2021-08-05 18-09-29

this is the importer right? If I had something to import it would automatically list it?

Do you think it would make sense to let users choose a folder to import via portal instead? Perhaps as a fallback.

If so, only "--filesystem=~/.local/share/bottles:ro" would be required for now.

sonnyp avatar Aug 05 '21 16:08 sonnyp

From these can be safely removed .Bottles (old path where we stored bottles in v1)

What about https://github.com/bottlesdevs/Bottles#backward-compatibility-triumph ?

Screenshot from 2021-08-05 18-09-29

this is the importer right? If I had something to import it would automatically list it?

Do you think it would make sense to let users choose a folder to import via portal instead? Perhaps as a fallback.

If so, only "--filesystem=~/.local/share/bottles:ro" would be required for now.

It has been a very long time and two major versions since I wrote about back compatibility. I think it could be the first step to retire v1 due the next coming major.

At the moment it list automatically but it might be a good idea to allow the user to select the path using the portals.

Also .local/share/bottles will be removed in the near future, as the Flatpak now store inside the .var/app/com.usebottles.bottles/data directory.

mirkobrombin avatar Aug 05 '21 19:08 mirkobrombin

Ok then for now I suggest we remove all permissions (and break the importer) and add to the flathub repo README.md how to use the importer with Flatseal.

I will create a follow up ticket to change how the importer work.

WDYT?

sonnyp avatar Aug 11 '21 09:08 sonnyp

Ok then for now I suggest we remove all permissions (and break the importer) and add to the flathub repo README.md how to use the importer with Flatseal.

I will create a follow up ticket to change how the importer work.

WDYT?

I agree

mirkobrombin avatar Aug 11 '21 09:08 mirkobrombin

@mirkobrombin can you assign me ?

sonnyp avatar Aug 11 '21 13:08 sonnyp

sonnyp avatar Aug 11 '21 13:08 sonnyp

@mirkobrombin can you reopen this issue?

sonnyp avatar Aug 12 '21 10:08 sonnyp

Had the new update come through and noticed the changes of #460. It appears to have broken some apps that require folders like xdg-documents or xdg-music, and so forth. Whilst I go wild on locking down my own projects runtimes, I would personally think that it would make sense for an app like Bottles to have home folder access by default. Requiring users to also install Flatseal to include the permissions that they need might be a bit much, personally.

cc @mirkobrombin @sonnyp

BobyMCbobs avatar Aug 16 '21 04:08 BobyMCbobs

How many cases are these directories needed? In my common use and testing I have not encountered any problems.Give me one or more application examples please.

The way chosen by the flatpak is that of the full sandbox, otherwise it is possible to use one of the other packages (appimage, Deb, aur, snap, fedora ..).

It is not easy to please all users, there are those who want the full sandbox and those who do not .. at this point I invite users to choose the package that suits their needs.

mirkobrombin avatar Aug 16 '21 05:08 mirkobrombin

As the apps running in Wine are not native, I'm too sure if they end up using Flatpak portals or not. I personally use FL Studio, it requires C:\users${USER}\Documents/Image Line (or natively $HOME/${USER}/Documents/Image Line). Similar situation for Notepad++ too, as it can't read from the home folder.

Sandboxing is valuable, but exceptions in the sandbox (such as limited filesystem) will allow more apps to Just Work:tm: Flatpak is a valuable technology and I believe it allows the consistency of behaviour in apps, so for me I prefer it.

I understand, and will respect your decision. I have had the easiest experience with Wine ever coming to Bottles, it's really a true delight. I am wanting it to be the same joyful experience for others as it was for me.

My final suggestion relating to this is perhaps to put a small button/box or something built into the app to detail instructions on how to override with flatpak override or using Flatseal,

BobyMCbobs avatar Aug 16 '21 09:08 BobyMCbobs

As the apps running in Wine are not native, I'm too sure if they end up using Flatpak portals or not. I personally use FL Studio, it requires C:\users${USER}\Documents/Image Line (or natively $HOME/${USER}/Documents/Image Line). Similar situation for Notepad++ too, as it can't read from the home folder.

Sandboxing is valuable, but exceptions in the sandbox (such as limited filesystem) will allow more apps to Just Work Flatpak is a valuable technology and I believe it allows the consistency of behaviour in apps, so for me I prefer it.

I understand, and will respect your decision. I have had the easiest experience with Wine ever coming to Bottles, it's really a true delight. I am wanting it to be the same joyful experience for others as it was for me.

My final suggestion relating to this is perhaps to put a small button/box or something built into the app to detail instructions on how to override with flatpak override or using Flatseal,

That button is already be present on the top of the bottle details page😁:

image

mirkobrombin avatar Aug 16 '21 09:08 mirkobrombin

There is also a dedicated page in the documentation: https://docs.usebottles.com/flatpak/expose-directories Maybe it need to be improved?

Also using flatpak-spawn it should be possible to automate the process (at least I think).

mirkobrombin avatar Aug 16 '21 09:08 mirkobrombin

As the apps running in Wine are not native, I'm too sure if they end up using Flatpak portals or not. I personally use FL Studio, it requires C:\users${USER}\Documents/Image Line (or natively $HOME/${USER}/Documents/Image Line). Similar situation for Notepad++ too, as it can't read from the home folder. Sandboxing is valuable, but exceptions in the sandbox (such as limited filesystem) will allow more apps to Just Work Flatpak is a valuable technology and I believe it allows the consistency of behaviour in apps, so for me I prefer it. I understand, and will respect your decision. I have had the easiest experience with Wine ever coming to Bottles, it's really a true delight. I am wanting it to be the same joyful experience for others as it was for me. My final suggestion relating to this is perhaps to put a small button/box or something built into the app to detail instructions on how to override with flatpak override or using Flatseal,

That button is already be present on the top of the bottle details page:

image

Ah, that's a web link. That is useful. I'm unsure about the glyph. I wasn't so drawn to it for it's purpose. That's definitely the right direction however.

There is also a dedicated page in the documentation: https://docs.usebottles.com/flatpak/expose-directories Maybe it need to be improved?

Also using flatpak-spawn it should be possible to automate the process (at least I think).

The docs are fine.

It would definitely be less to do for new users, to add more permissions.

BobyMCbobs avatar Aug 16 '21 09:08 BobyMCbobs

The current situation isn't perfect but is temporary and will allow us to solve the problem once and for all.

See https://github.com/bottlesdevs/Bottles/issues/413#issuecomment-896837582

sonnyp avatar Aug 16 '21 09:08 sonnyp

The current situation isn't perfect but is temporary and will allow us to solve the problem once and for all.

See #413 (comment)

Thank you for your efforts and thank you for listening. Keep up the good work :heart:

@mirkobrombin @sonnyp

BobyMCbobs avatar Aug 16 '21 09:08 BobyMCbobs

Reducing filesystem permissions does not work well with programs that bundle their own DLLs (or other assets). Say I have DoSomething.exe and it requires special_do_things.dll. This DLL is included in the same folder as the exe. When opening via the file chooser via the "Run Executable" button, which I assume uses GtkFileChooserNative, Bottles is only given access to the individual file handed to it by the portal. It is not given access to the directory, thus things in the sandbox cannot access related files in the same folder. If you give Bottles filesystem permissions (e.g. --filesystem=home) it can access said files without issue.

BrainBlasted avatar Oct 04 '21 08:10 BrainBlasted

If we give the flatpak that permission again, the issue (https://github.com/bottlesdevs/Bottles/issues/572) should be closed.

I don't think it is possible to give access to an entire folder through the GtkFileChooserNative because the user has to select the file and not the path. I believe that the lack of at least this permission is blocking the functioning of the main Bottles feature (starting windows executables), but I also think we shouldn't give in, to keep Bottles on the road to full-sandbox.

@sonnyp what is your opinion on this? Do you think there is an easy way to solve or do you think it is better to explain better how to give the user the responsibility to add permissions to Bottles?

As I thought Bottles, the target is the inexperienced user (despite everything), maybe a step by step tutorial should be done? Or maybe it is possible to integrate the permissions management in Bottles and act through flatpak-spawn in a controlled way?

Regarding the last proposal, we could show a dialog box with the exposed paths and a simple + to expose others. Since we need to execute commands on the host, we could use flatpak-spawn (requires permissions in manifest) to give Bottles new permissions. So Bottles at the first start is full sandboxed and the user can (at his own risk) expose path in a simple way (even if I would point out the risk with a warning).

mirkobrombin avatar Oct 04 '21 08:10 mirkobrombin