Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary permissions #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scy
Copy link

@scy scy commented Oct 27, 2024

This makes the application perfectly usable (including being able to save and load its own config), but without giving it access to the whole home directory.

In other words, it is an alternative to #14, while fixing #10 and #11 nevertheless.

And yes, the config directory it's expecting is really ~/.config/.X-AIR-Edit, with that exact capitalization, and with the leading dot.

Since X AIR Edit does not use normal GTK or Qt file pickers, users will need to share directories manually in order to save and restore scenes, or use --persist=. to avoid them being lost when closing the application. I think this is a valid tradeoff instead of giving a proprietary piece of software with internet connectivity access to all my files.

In case this PR is not accepted, or if you'd like to take advantage of the added security right away without building the Flatpak yourself, you can also use something like flatpak override --user --disallow=bluetooth --disallow=canbus --nosocket=cups --nosocket=gpg-agent --nosocket=pcsc --nosocket=pulseaudio --nosocket=ssh-auth --nofilesystem=host:reset --filesystem=xdg-config/.X-AIR-Edit:create --persist=shared com.behringer.XAirEdit to get basically the same effect. Note that I have included --persist=shared here, so that the application will see a directory ~/shared inside the sandbox, which maps to ~/.var/app/com.behringer.XAirEdit/shared on the host.

@flathubbot
Copy link

Started test build 157094

@flathubbot
Copy link

Build 157094 failed

@scy
Copy link
Author

scy commented Oct 27, 2024

I have looked at the build log, and I find it confusing.

The linter recommends against --nosocket={cups,gpg-agent,pcsc,pulseaudio,ssh-auth}, because

By default no access is granted unless --socket=socket_name is used.

This contradicts the standard permissions listed in the Flatpak documentation.

It also complains about the explicit --filesystem=xdg-config/…, because

Flatpak creates its own XDG cache [sic, I guess they mean config] directory under ~/.var/app/<app-id>/config. Applications do not need to access the host's config directory or subpaths of it.

On my system, this does not work, and it obviously neither worked for other people (see #10 & #11). Without the --filesystem=… option, there is no ~/.config in the sandbox (verified by building locally with --install, then flatpak run --user --command=/bin/bash com.behringer.XAirEdit), and X Air Edit will not save its configuration.

I guess I'm missing something here. Some guidance would be appreciated, but I'm also totally fine with dropping this PR because my overrides work and this PR is mainly a service to other people. 🤷‍♂️

@jmaibaum
Copy link
Collaborator

jmaibaum commented Oct 27, 2024

Hi and thanks for your PR. I am currently only on my mobile therefore I can neither verify nor correct anything yet.

A few general remarks: "Standard permissions" according to the docs are not granted to any app unless you explicitly add them. This is why the linter complains: There is no Bluetooth access given to the app by default, therefore there is no need to disallow it in the manifest.

I am happy to accept a PR that enables X-Air-Edit's config to be saved without giving access to the whole of the user's home dir, but I was not able to find another way yet. Maybe it helps to asks Behringer to use xdg-config instead of hardcoding .config as it seems to be the case right now. Then the standard flatpak sandbox config paths under .var/app/ID/config would work out of the box and no filesystem permissions were needed.

@scy
Copy link
Author

scy commented Oct 27, 2024

I am currently only on my mobile therefore I can neither verify nor correct anything yet.

No worries, I'm not in a rush. :) Thanks for getting back to me, but take your time.

A few general remarks: "Standard permissions" according to the docs are not granted to any app unless you explicitly add them.

Oh, I see. "Standard permissions" means that you can freely use them, not that they're set by default. I was misreading the documentation there.

I am happy to accept a PR that enables X-Air-Edit's config to be saved without giving access to the whole of the user's home dir, but I was not able to find another way yet. Maybe it helps to asks Behringer to use xdg-config instead of hardcoding .config as it seems to be the case right now. Then the standard flatpak sandbox config paths under .var/app/ID/config would work out of the box and no filesystem permissions were needed.

Now I understand what's going on here. Flatpak does not magically provide ~/.config in the sandbox and maps it to ~/.var/app/$id/config, no, it simply sets the $XDG_CONFIG_HOME variable and (rightly, I think) expects applications to use it.

Sure, we can try talking to Behringer about this. Alternatively (or additionally), we could ask for an exception to the linter rule.

This still doesn't solve the problem with saving/restoring scenes and presets and stuff. I doubt Behringer will switch to a toolkit with a supported file chooser dialog to set up the necessary permissions automatically, and I equally doubt they will implement the required portal stuff themselves.

Creating a shared directory with --persist=… is probably the most convenient way, but also a bit intrusive because it selects a name on behalf of the user. Advanced users might want a different one. And you'll still need to explain to beginners where to find the files on their host machine.

The more flexible way is having users configure a --persist=… themselves (or grant home dir access manually), but I'm pretty sure there will be some unexperienced person spending hours configuring their mixer without having any of those set up, and then be very sad when their exported scenes are gone when they reopen the app the next day.

That being said, I really do not want to give a closed-source, network-enabled app access to my home dir. But as I wrote above, I can just override the default permissions.

@scy scy force-pushed the minimal-permissions branch from 1ce06a7 to 9bc0e53 Compare October 28, 2024 11:55
@flathubbot
Copy link

Started test build 157305

@flathubbot
Copy link

Build 157305 failed

@scy
Copy link
Author

scy commented Oct 28, 2024

Updated the PR with what I've learned from jmaibaum's comment. It's now basically only one line, replacing homedir access with access to the hardcoded ~/.config/.X-AIR-Edit only. We'd still need to have an exception added to the Flathub linter in order to publish this. And the question of how to save/restore scenes/presets and share them with the host machine isn't discussed yet either.

I'll try writing Behringer an email about the hard-coded ~/.config though.

@scy
Copy link
Author

scy commented Oct 28, 2024

Sent this support request to Behringer via their customer support form:

First of all, thank you very much for providing a Linux version of X AIR Edit, it's really appreciated.

However, I have noticed that this version will write its configuration files into the directory ".config/.X-AIR-Edit" in the user's home directory. There are two problems with this. First of all, the additional dot before "X-AIR-Edit" unnecessarily hides the directory. This is not required, since ".config" is already hidden, and it goes against established standards and best practices.

Second, and more importantly, the XDG standard that almost all modern Linux software adheres to defines, among other things, an environment variable $XDG_CONFIG_HOME, that applications should use as the base directory for their own configuration. (See https://specifications.freedesktop.org/basedir-spec/latest/ for the standard.) If this variable is unset, it defaults to "$HOME/.config", i.e. the directory that's currently being used by X AIR Edit.

So far, so good. But if the user has customized the XDG_CONFIG_HOME environment variable, X AIR Edit does not respect it and still insists on putting its configuration directory under "$HOME/.config", no matter what is defined in XDG_CONFIG_HOME.

This is a problem for users that have customized this location, or when X AIR Edit is being used in combination with other software. For example, when trying to create a Flathub package for X AIR Edit, to allow users to easily install the software and run it in a safe, sandboxed environment, the Flatpak runtime requires the applications to respect its custom XDG_CONFIG_HOME location. Since X AIR Edit does not, the package's maintainers were forced to grant it additional permissions, which result in a large red "potentially unsafe" warning at the bottom of https://flathub.org/apps/com.behringer.XAirEdit.

To fix this issue, the developers of X AIR Edit would basically need to incorporate one small change: Instead of simply using "$HOME/.config" as the base directory for X AIR Edit's configuration, first look up whether $XDG_CONFIG_HOME is set. If it is, use the directory defined there instead of "$HOME/.config". If it does not, X AIR Edit should use "$HOME/.config" just like it does now.

Thank you for taking the time to consider this request.

@jmaibaum
Copy link
Collaborator

Thanks for taking the time to write to Behringer! I hope they consider it.

Regarding the linter exception, did you already take any steps here? I think we need to open an issue at github.com/flathub/flathub.

@scy
Copy link
Author

scy commented Oct 31, 2024

Thanks for taking the time to write to Behringer! I hope they consider it.

Behringer support asked me to submit the request to their "feedback platform" instead, which I did: X AIR Edit Linux version does not respect $XDG_CONFIG_HOME.

Regarding the linter exception, did you already take any steps here?

I didn't. Mainly because I didn't feel that it's my place as a new contributor without getting explicit approval first.

I think we need to open an issue at github.com/flathub/flathub.

Not quite, they want a PR against the JSON-based exception file. The process is outlined in the Flathub docs. I can open a pull request, if you like.

@jmaibaum
Copy link
Collaborator

jmaibaum commented Oct 31, 2024

Thanks for the link to the documentation. I might have time (and access to my Behringer mixer) again by the weekend to test your proposed change.

If you have time open the PR against the linter exception file in the meantime, please feel free to do so.

Though I understand that if Behringer reacts to your requests and updates X-Air-Edit, the exception would not be needed anymore. So let's see what happens first. FWIW I have upvoted your message at Behringer's feedback platform. 😊

Thanks for pushing this forward!

@scy
Copy link
Author

scy commented Oct 31, 2024

Thanks for getting back to me. :) Created a linter PR at flathub-infra/flatpak-builder-lint#526.

This makes the application perfectly usable (including being able to
save and load its own config), but without giving it access to the whole
home directory.

And yes, the config directory it's expecting is really
~/.config/.X-AIR-Edit, with that exact capitalization, and with the
leading dot.
@scy scy force-pushed the minimal-permissions branch from 9bc0e53 to 9e4e966 Compare November 1, 2024 13:08
@scy
Copy link
Author

scy commented Nov 1, 2024

The linter maintainers told me that we could just use --persist instead, and they're right. (Tested locally.) I have closed the linter PR and updated this PR here accordingly.

@flathubbot
Copy link

Started test build 158298

@flathubbot
Copy link

Build 158298 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/141382/com.behringer.XAirEdit.flatpakref

@jmaibaum
Copy link
Collaborator

jmaibaum commented Nov 1, 2024

Thanks for this. I will hopefully find time to take a look at this over the weekend.

@jmaibaum
Copy link
Collaborator

jmaibaum commented Nov 3, 2024

Hi, the changes to allow config to be stored in .config via persist seem to work as expected. I would be willing to accept this PR, yet, the issue being unable to store/load scenes/channel presets without manual intervention in the form of flatpak override does persist (also as expected).

I am reluctant in removing --filesystem=home in favour of requiring the average user to know about flatpak override or GUI tools like Flatseal, as those things are not easily discoverable from the "easy tools" like GNOME Software.

FWIW, GNOME Software (and also flatpak CLI) does report and warn about the home dir permissions the app asks for by default, thus the privacy-aware user can do something about it. Yet, the average user will likely be lost if we merge this PR.

I am not yet completely convinced that this is the way to go. I understand and acknowledge your concerns about a proprietary closed source tool like X-Air-Edit, but unless I see a more easier (discoverable!) way for the average user who just wants to use their mixer and save/load presets, I am going to leave this PR open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants